[RFC,3/4] hw/arm/virt: Enable pc-dimm hotplug support

Commit Message

Shameerali Kolothum Thodi Jan. 28, 2019, 11:05 a.m. UTC

pc-dimm memory hotplug is enabled using GPIO(Pin 2) based ACPI
event. Hot removal functionality is not yet supported.

Signed-off-by: Shameer Kolothum <[e-mail]>
---
 hw/arm/virt.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 55 insertions(+), 2 deletions(-)

Comments

Igor Mammedov Feb. 27, 2019, 5:14 p.m. UTC | #1

On Mon, 28 Jan 2019 11:05:45 +0000
Shameer Kolothum <[e-mail]> wrote:

> pc-dimm memory hotplug is enabled using GPIO(Pin 2) based ACPI
> event. Hot removal functionality is not yet supported.
> 
> Signed-off-by: Shameer Kolothum <[e-mail]>
> ---
>  hw/arm/virt.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 55 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 884960d..cf64554 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -62,6 +62,7 @@
>  #include "hw/mem/pc-dimm.h"
>  #include "hw/mem/nvdimm.h"
>  #include "hw/acpi/acpi.h"
> +#include "hw/acpi/pc-hotplug.h"
>  
>  #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
>      static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> @@ -1651,7 +1652,14 @@ static void machvirt_init(MachineState *machine)
>          nvdimm_init_acpi_state(acpi_nvdimm_state, sysmem,
>                                 vms->fw_cfg, OBJECT(vms));
>      }
> +    if (vms->acpi_memhp_state.is_enabled) {
> +        MemHotplugState *state =  &vms->acpi_memhp_state;
> +        hwaddr base;
>  
> +        state->hw_reduced_acpi = true;
> +        base = vms->memmap[VIRT_ACPI_IO].base + ACPI_MEMORY_HOTPLUG_BASE;
> +        acpi_memory_hotplug_init(sysmem, OBJECT(vms), state, base);
> +    }
this hunk should be a part of 'acpi' device that owns respective interrupts and mmio regions.
(something like we do in x86)
In this case I'd suggest to make 'base' its property and the board will set it during
device creation.

>      vms->bootinfo.ram_size = machine->ram_size;
>      vms->bootinfo.kernel_filename = machine->kernel_filename;
>      vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> @@ -1819,6 +1827,20 @@ static void virt_set_nvdimm_persistence(Object *obj, const char *value,
>      nvdimm_state->persistence_string = g_strdup(value);
>  }
>  
> +static bool virt_get_memhp_support(Object *obj, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    return vms->acpi_memhp_state.is_enabled;
> +}
> +
> +static void virt_set_memhp_support(Object *obj, bool value, Error **errp)
> +{
> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> +
> +    vms->acpi_memhp_state.is_enabled = value;
> +}
> +
>  static CpuInstanceProperties
>  virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
>  {
> @@ -1863,8 +1885,8 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>      VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
>  
> -    if (dev->hotplugged) {
> -        error_setg(errp, "memory hotplug is not supported");
> +    if (dev->hotplugged && is_nvdimm) {
> +        error_setg(errp, "nvdimm hotplug is not supported");
>      }
>  
>      if (is_nvdimm && !vms->acpi_nvdimm_state.is_enabled) {
> @@ -1875,6 +1897,22 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL, errp);
>  }
>  
> +static void virt_memhp_send_event(HotplugHandler *hotplug_dev, DeviceState *dev,
> +                                  Error **errp)
> +{
> +    DeviceState *gpio_dev;
> +    VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> +
> +    gpio_dev = virt_get_gpio_dev(GPIO_PCDIMM);
> +    if (!gpio_dev) {
> +        error_setg(errp, "No dev interface to send hotplug event");
                             ^^^^^^ confusing
> +        return;
> +    }
> +    acpi_memory_plug_cb(hotplug_dev, &vms->acpi_memhp_state,
> +                        dev, errp);
> +    qemu_set_irq(qdev_get_gpio_in(gpio_dev, 0), 1);
> +}
> +
>  static void virt_memory_plug(HotplugHandler *hotplug_dev,
>                               DeviceState *dev, Error **errp)
>  {
> @@ -1891,6 +1929,10 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
>          nvdimm_plug(&vms->acpi_nvdimm_state);
>      }
>  
> +    if (dev->hotplugged && !is_nvdimm) {
> +        virt_memhp_send_event(hotplug_dev, dev, errp);
...
  acpi_memory_plug_cb();
  hotplug_handler_plug(HOTPLUG_HANDLER(pcms->gpio_dev), dev, &error_abort);
  ^^^^ forward snd process hotplug notification event in gpio_dev,
       machine should not care about which and how to deal with random IRQs

> +    }
> +
>  out:
>      error_propagate(errp, local_err);
>  }
> @@ -1898,6 +1940,11 @@ out:
>  static void virt_memory_unplug(HotplugHandler *hotplug_dev,
>                                 DeviceState *dev, Error **errp)
>  {
> +    if (dev->hotplugged) {
> +        error_setg(errp, "memory hot unplug is not supported");
> +        return;
> +    }
what if unplug is called on cold-plugged device?

Better way to disable mgmt initiated unplug is to forbid it in unplug_request()
For guest initiated one ('unplug' handler), the best we can do is log error
and ignore it (provided guest won't get in confused). it's also possible 
to hide _EJ method and then it would be even fine to abort if it gets here,
since guest is not supposed to interface with MMIO interface without using AML.

> +
>      pc_dimm_unplug(PC_DIMM(dev), MACHINE(hotplug_dev));
>      object_unparent(OBJECT(dev));
>  }
> @@ -2085,6 +2132,12 @@ static void virt_instance_init(Object *obj)
>                                      "Set NVDIMM persistence"
>                                      "Valid values are cpu and mem-ctrl", NULL);
>  
> +    vms->acpi_memhp_state.is_enabled = true;
> +    object_property_add_bool(obj, "memory-hotplug-support",
> +                             virt_get_memhp_support,
> +                             virt_set_memhp_support,
> +                             NULL);
> +
>      vms->irqmap = a15irqmap;
>  }
>

Eric Auger Feb. 28, 2019, 9:57 a.m. UTC | #2

Hi Igor,

On 2/27/19 6:14 PM, Igor Mammedov wrote:
> On Mon, 28 Jan 2019 11:05:45 +0000
> Shameer Kolothum <[e-mail]> wrote:
> 
>> pc-dimm memory hotplug is enabled using GPIO(Pin 2) based ACPI
>> event. Hot removal functionality is not yet supported.
>>
>> Signed-off-by: Shameer Kolothum <[e-mail]>
>> ---
>>  hw/arm/virt.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 55 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
>> index 884960d..cf64554 100644
>> --- a/hw/arm/virt.c
>> +++ b/hw/arm/virt.c
>> @@ -62,6 +62,7 @@
>>  #include "hw/mem/pc-dimm.h"
>>  #include "hw/mem/nvdimm.h"
>>  #include "hw/acpi/acpi.h"
>> +#include "hw/acpi/pc-hotplug.h"
>>  
>>  #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
>>      static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
>> @@ -1651,7 +1652,14 @@ static void machvirt_init(MachineState *machine)
>>          nvdimm_init_acpi_state(acpi_nvdimm_state, sysmem,
>>                                 vms->fw_cfg, OBJECT(vms));
>>      }
>> +    if (vms->acpi_memhp_state.is_enabled) {
>> +        MemHotplugState *state =  &vms->acpi_memhp_state;
>> +        hwaddr base;
>>  
>> +        state->hw_reduced_acpi = true;
>> +        base = vms->memmap[VIRT_ACPI_IO].base + ACPI_MEMORY_HOTPLUG_BASE;
>> +        acpi_memory_hotplug_init(sysmem, OBJECT(vms), state, base);
>> +    }
> this hunk should be a part of 'acpi' device that owns respective interrupts and mmio regions.
> (something like we do in x86)
> In this case I'd suggest to make 'base' its property and the board will set it during
> device creation.
> 
>>      vms->bootinfo.ram_size = machine->ram_size;
>>      vms->bootinfo.kernel_filename = machine->kernel_filename;
>>      vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
>> @@ -1819,6 +1827,20 @@ static void virt_set_nvdimm_persistence(Object *obj, const char *value,
>>      nvdimm_state->persistence_string = g_strdup(value);
>>  }
>>  
>> +static bool virt_get_memhp_support(Object *obj, Error **errp)
>> +{
>> +    VirtMachineState *vms = VIRT_MACHINE(obj);
>> +
>> +    return vms->acpi_memhp_state.is_enabled;
>> +}
>> +
>> +static void virt_set_memhp_support(Object *obj, bool value, Error **errp)
>> +{
>> +    VirtMachineState *vms = VIRT_MACHINE(obj);
>> +
>> +    vms->acpi_memhp_state.is_enabled = value;
>> +}
>> +
>>  static CpuInstanceProperties
>>  virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
>>  {
>> @@ -1863,8 +1885,8 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>      const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>>      VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
>>  
>> -    if (dev->hotplugged) {
>> -        error_setg(errp, "memory hotplug is not supported");
>> +    if (dev->hotplugged && is_nvdimm) {
>> +        error_setg(errp, "nvdimm hotplug is not supported");
>>      }
>>  
>>      if (is_nvdimm && !vms->acpi_nvdimm_state.is_enabled) {
>> @@ -1875,6 +1897,22 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>>      pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL, errp);
>>  }
>>  
>> +static void virt_memhp_send_event(HotplugHandler *hotplug_dev, DeviceState *dev,
>> +                                  Error **errp)
>> +{
>> +    DeviceState *gpio_dev;
>> +    VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
>> +
>> +    gpio_dev = virt_get_gpio_dev(GPIO_PCDIMM);
>> +    if (!gpio_dev) {
>> +        error_setg(errp, "No dev interface to send hotplug event");
>                              ^^^^^^ confusing
>> +        return;
>> +    }
>> +    acpi_memory_plug_cb(hotplug_dev, &vms->acpi_memhp_state,
>> +                        dev, errp);
>> +    qemu_set_irq(qdev_get_gpio_in(gpio_dev, 0), 1);
>> +}
>> +
>>  static void virt_memory_plug(HotplugHandler *hotplug_dev,
>>                               DeviceState *dev, Error **errp)
>>  {
>> @@ -1891,6 +1929,10 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
>>          nvdimm_plug(&vms->acpi_nvdimm_state);
>>      }
>>  
>> +    if (dev->hotplugged && !is_nvdimm) {
>> +        virt_memhp_send_event(hotplug_dev, dev, errp);
> ...
>   acpi_memory_plug_cb();
>   hotplug_handler_plug(HOTPLUG_HANDLER(pcms->gpio_dev), dev, &error_abort);
>   ^^^^ forward snd process hotplug notification event in gpio_dev,
>        machine should not care about which and how to deal with random IRQs
> 
>> +    }
>> +
>>  out:
>>      error_propagate(errp, local_err);
>>  }
>> @@ -1898,6 +1940,11 @@ out:
>>  static void virt_memory_unplug(HotplugHandler *hotplug_dev,
>>                                 DeviceState *dev, Error **errp)
>>  {
>> +    if (dev->hotplugged) {
>> +        error_setg(errp, "memory hot unplug is not supported");
>> +        return;
>> +    }
> what if unplug is called on cold-plugged device?
> 
> Better way to disable mgmt initiated unplug is to forbid it in unplug_request()
> For guest initiated one ('unplug' handler), the best we can do is log error
> and ignore it (provided guest won't get in confused). it's also possible 
> to hide _EJ method and then it would be even fine to abort if it gets here,
> since guest is not supposed to interface with MMIO interface without using AML.
I don't understand the guest initiated unplug handling. How does it
differ from x86 handling as we are using the same dynamic methods?

Besides In ARM case I checked
/sys/devices/system/memory/memoryX/removable state for cold-plugged and
hot-plugged DIMMs and it is equal to 0. I don't know if it is a
limitation of the guest or a bug in our ACPI description somewhere. I
would not be surprised if hot-unplug is not supported at kernel leve
though.

Thanks

Eric
> 
>> +
>>      pc_dimm_unplug(PC_DIMM(dev), MACHINE(hotplug_dev));
>>      object_unparent(OBJECT(dev));
>>  }
>> @@ -2085,6 +2132,12 @@ static void virt_instance_init(Object *obj)
>>                                      "Set NVDIMM persistence"
>>                                      "Valid values are cpu and mem-ctrl", NULL);
>>  
>> +    vms->acpi_memhp_state.is_enabled = true;
>> +    object_property_add_bool(obj, "memory-hotplug-support",
>> +                             virt_get_memhp_support,
>> +                             virt_set_memhp_support,
>> +                             NULL);
>> +
>>      vms->irqmap = a15irqmap;
>>  }
>>  
>

Shameerali Kolothum Thodi Feb. 28, 2019, 12:27 p.m. UTC | #3

> -----Original Message-----
> From: Igor Mammedov [mailto:[e-mail]]
> Sent: 27 February 2019 17:14
> To: Shameerali Kolothum Thodi <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Linuxarm <[e-mail]>; xuwei (O) <[e-mail]>
> Subject: Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm
> hotplug support
> 
> On Mon, 28 Jan 2019 11:05:45 +0000
> Shameer Kolothum <[e-mail]> wrote:
> 
> > pc-dimm memory hotplug is enabled using GPIO(Pin 2) based ACPI
> > event. Hot removal functionality is not yet supported.
> >
> > Signed-off-by: Shameer Kolothum <[e-mail]>
> > ---
> >  hw/arm/virt.c | 57
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 55 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 884960d..cf64554 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -62,6 +62,7 @@
> >  #include "hw/mem/pc-dimm.h"
> >  #include "hw/mem/nvdimm.h"
> >  #include "hw/acpi/acpi.h"
> > +#include "hw/acpi/pc-hotplug.h"
> >
> >  #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
> >      static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> > @@ -1651,7 +1652,14 @@ static void machvirt_init(MachineState
> *machine)
> >          nvdimm_init_acpi_state(acpi_nvdimm_state, sysmem,
> >                                 vms->fw_cfg, OBJECT(vms));
> >      }
> > +    if (vms->acpi_memhp_state.is_enabled) {
> > +        MemHotplugState *state =  &vms->acpi_memhp_state;
> > +        hwaddr base;
> >
> > +        state->hw_reduced_acpi = true;
> > +        base = vms->memmap[VIRT_ACPI_IO].base +
> ACPI_MEMORY_HOTPLUG_BASE;
> > +        acpi_memory_hotplug_init(sysmem, OBJECT(vms), state, base);
> > +    }
> this hunk should be a part of 'acpi' device that owns respective interrupts and
> mmio regions.
> (something like we do in x86)
> In this case I'd suggest to make 'base' its property and the board will set it
> during
> device creation.

Ok. I will take a look at x86.

> >      vms->bootinfo.ram_size = machine->ram_size;
> >      vms->bootinfo.kernel_filename = machine->kernel_filename;
> >      vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> > @@ -1819,6 +1827,20 @@ static void virt_set_nvdimm_persistence(Object
> *obj, const char *value,
> >      nvdimm_state->persistence_string = g_strdup(value);
> >  }
> >
> > +static bool virt_get_memhp_support(Object *obj, Error **errp)
> > +{
> > +    VirtMachineState *vms = VIRT_MACHINE(obj);
> > +
> > +    return vms->acpi_memhp_state.is_enabled;
> > +}
> > +
> > +static void virt_set_memhp_support(Object *obj, bool value, Error **errp)
> > +{
> > +    VirtMachineState *vms = VIRT_MACHINE(obj);
> > +
> > +    vms->acpi_memhp_state.is_enabled = value;
> > +}
> > +
> >  static CpuInstanceProperties
> >  virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
> >  {
> > @@ -1863,8 +1885,8 @@ static void virt_memory_pre_plug(HotplugHandler
> *hotplug_dev, DeviceState *dev,
> >      const bool is_nvdimm = object_dynamic_cast(OBJECT(dev),
> TYPE_NVDIMM);
> >      VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> >
> > -    if (dev->hotplugged) {
> > -        error_setg(errp, "memory hotplug is not supported");
> > +    if (dev->hotplugged && is_nvdimm) {
> > +        error_setg(errp, "nvdimm hotplug is not supported");
> >      }
> >
> >      if (is_nvdimm && !vms->acpi_nvdimm_state.is_enabled) {
> > @@ -1875,6 +1897,22 @@ static void
> virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >      pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL,
> errp);
> >  }
> >
> > +static void virt_memhp_send_event(HotplugHandler *hotplug_dev,
> DeviceState *dev,
> > +                                  Error **errp)
> > +{
> > +    DeviceState *gpio_dev;
> > +    VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> > +
> > +    gpio_dev = virt_get_gpio_dev(GPIO_PCDIMM);
> > +    if (!gpio_dev) {
> > +        error_setg(errp, "No dev interface to send hotplug event");
>                              ^^^^^^ confusing

Ok. I think this will anyway change with AcpiDeviceIfClass implementation.

> > +        return;
> > +    }
> > +    acpi_memory_plug_cb(hotplug_dev, &vms->acpi_memhp_state,
> > +                        dev, errp);
> > +    qemu_set_irq(qdev_get_gpio_in(gpio_dev, 0), 1);
> > +}
> > +
> >  static void virt_memory_plug(HotplugHandler *hotplug_dev,
> >                               DeviceState *dev, Error **errp)
> >  {
> > @@ -1891,6 +1929,10 @@ static void virt_memory_plug(HotplugHandler
> *hotplug_dev,
> >          nvdimm_plug(&vms->acpi_nvdimm_state);
> >      }
> >
> > +    if (dev->hotplugged && !is_nvdimm) {
> > +        virt_memhp_send_event(hotplug_dev, dev, errp);
> ...
>   acpi_memory_plug_cb();
>   hotplug_handler_plug(HOTPLUG_HANDLER(pcms->gpio_dev), dev,
> &error_abort);
>   ^^^^ forward snd process hotplug notification event in gpio_dev,
>        machine should not care about which and how to deal with random
> IRQs

Ok.

> > +    }
> > +
> >  out:
> >      error_propagate(errp, local_err);
> >  }
> > @@ -1898,6 +1940,11 @@ out:
> >  static void virt_memory_unplug(HotplugHandler *hotplug_dev,
> >                                 DeviceState *dev, Error **errp)
> >  {
> > +    if (dev->hotplugged) {
> > +        error_setg(errp, "memory hot unplug is not supported");
> > +        return;
> > +    }
> what if unplug is called on cold-plugged device?
> 
> Better way to disable mgmt initiated unplug is to forbid it in unplug_request()
> For guest initiated one ('unplug' handler), the best we can do is log error
> and ignore it (provided guest won't get in confused). it's also possible
> to hide _EJ method and then it would be even fine to abort if it gets here,
> since guest is not supposed to interface with MMIO interface without using
> AML.

Hmm.. I haven't tested yet with Guest initiated unplug as ARM kernel doesn't
support it yet. I will try that if possible.

Thanks,
Shameer
 
> > +
> >      pc_dimm_unplug(PC_DIMM(dev), MACHINE(hotplug_dev));
> >      object_unparent(OBJECT(dev));
> >  }
> > @@ -2085,6 +2132,12 @@ static void virt_instance_init(Object *obj)
> >                                      "Set NVDIMM persistence"
> >                                      "Valid values are cpu and
> mem-ctrl", NULL);
> >
> > +    vms->acpi_memhp_state.is_enabled = true;
> > +    object_property_add_bool(obj, "memory-hotplug-support",
> > +                             virt_get_memhp_support,
> > +                             virt_set_memhp_support,
> > +                             NULL);
> > +
> >      vms->irqmap = a15irqmap;
> >  }
> >

Igor Mammedov Feb. 28, 2019, 12:44 p.m. UTC | #4

On Thu, 28 Feb 2019 10:57:41 +0100
Auger Eric <[e-mail]> wrote:

> Hi Igor,
> 
> On 2/27/19 6:14 PM, Igor Mammedov wrote:
> > On Mon, 28 Jan 2019 11:05:45 +0000
> > Shameer Kolothum <[e-mail]> wrote:
> >   
> >> pc-dimm memory hotplug is enabled using GPIO(Pin 2) based ACPI
> >> event. Hot removal functionality is not yet supported.
> >>
> >> Signed-off-by: Shameer Kolothum <[e-mail]>
> >> ---
> >>  hw/arm/virt.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >>  1 file changed, 55 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> >> index 884960d..cf64554 100644
> >> --- a/hw/arm/virt.c
> >> +++ b/hw/arm/virt.c
> >> @@ -62,6 +62,7 @@
> >>  #include "hw/mem/pc-dimm.h"
> >>  #include "hw/mem/nvdimm.h"
> >>  #include "hw/acpi/acpi.h"
> >> +#include "hw/acpi/pc-hotplug.h"
> >>  
> >>  #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
> >>      static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> >> @@ -1651,7 +1652,14 @@ static void machvirt_init(MachineState *machine)
> >>          nvdimm_init_acpi_state(acpi_nvdimm_state, sysmem,
> >>                                 vms->fw_cfg, OBJECT(vms));
> >>      }
> >> +    if (vms->acpi_memhp_state.is_enabled) {
> >> +        MemHotplugState *state =  &vms->acpi_memhp_state;
> >> +        hwaddr base;
> >>  
> >> +        state->hw_reduced_acpi = true;
> >> +        base = vms->memmap[VIRT_ACPI_IO].base + ACPI_MEMORY_HOTPLUG_BASE;
> >> +        acpi_memory_hotplug_init(sysmem, OBJECT(vms), state, base);
> >> +    }  
> > this hunk should be a part of 'acpi' device that owns respective interrupts and mmio regions.
> > (something like we do in x86)
> > In this case I'd suggest to make 'base' its property and the board will set it during
> > device creation.
> >   
> >>      vms->bootinfo.ram_size = machine->ram_size;
> >>      vms->bootinfo.kernel_filename = machine->kernel_filename;
> >>      vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> >> @@ -1819,6 +1827,20 @@ static void virt_set_nvdimm_persistence(Object *obj, const char *value,
> >>      nvdimm_state->persistence_string = g_strdup(value);
> >>  }
> >>  
> >> +static bool virt_get_memhp_support(Object *obj, Error **errp)
> >> +{
> >> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> >> +
> >> +    return vms->acpi_memhp_state.is_enabled;
> >> +}
> >> +
> >> +static void virt_set_memhp_support(Object *obj, bool value, Error **errp)
> >> +{
> >> +    VirtMachineState *vms = VIRT_MACHINE(obj);
> >> +
> >> +    vms->acpi_memhp_state.is_enabled = value;
> >> +}
> >> +
> >>  static CpuInstanceProperties
> >>  virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
> >>  {
> >> @@ -1863,8 +1885,8 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >>      const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
> >>      VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> >>  
> >> -    if (dev->hotplugged) {
> >> -        error_setg(errp, "memory hotplug is not supported");
> >> +    if (dev->hotplugged && is_nvdimm) {
> >> +        error_setg(errp, "nvdimm hotplug is not supported");
> >>      }
> >>  
> >>      if (is_nvdimm && !vms->acpi_nvdimm_state.is_enabled) {
> >> @@ -1875,6 +1897,22 @@ static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >>      pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL, errp);
> >>  }
> >>  
> >> +static void virt_memhp_send_event(HotplugHandler *hotplug_dev, DeviceState *dev,
> >> +                                  Error **errp)
> >> +{
> >> +    DeviceState *gpio_dev;
> >> +    VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> >> +
> >> +    gpio_dev = virt_get_gpio_dev(GPIO_PCDIMM);
> >> +    if (!gpio_dev) {
> >> +        error_setg(errp, "No dev interface to send hotplug event");  
> >                              ^^^^^^ confusing  
> >> +        return;
> >> +    }
> >> +    acpi_memory_plug_cb(hotplug_dev, &vms->acpi_memhp_state,
> >> +                        dev, errp);
> >> +    qemu_set_irq(qdev_get_gpio_in(gpio_dev, 0), 1);
> >> +}
> >> +
> >>  static void virt_memory_plug(HotplugHandler *hotplug_dev,
> >>                               DeviceState *dev, Error **errp)
> >>  {
> >> @@ -1891,6 +1929,10 @@ static void virt_memory_plug(HotplugHandler *hotplug_dev,
> >>          nvdimm_plug(&vms->acpi_nvdimm_state);
> >>      }
> >>  
> >> +    if (dev->hotplugged && !is_nvdimm) {
> >> +        virt_memhp_send_event(hotplug_dev, dev, errp);  
> > ...
> >   acpi_memory_plug_cb();
> >   hotplug_handler_plug(HOTPLUG_HANDLER(pcms->gpio_dev), dev, &error_abort);
> >   ^^^^ forward snd process hotplug notification event in gpio_dev,
> >        machine should not care about which and how to deal with random IRQs
> >   
> >> +    }
> >> +
> >>  out:
> >>      error_propagate(errp, local_err);
> >>  }
> >> @@ -1898,6 +1940,11 @@ out:
> >>  static void virt_memory_unplug(HotplugHandler *hotplug_dev,
> >>                                 DeviceState *dev, Error **errp)
> >>  {
> >> +    if (dev->hotplugged) {
> >> +        error_setg(errp, "memory hot unplug is not supported");
> >> +        return;
> >> +    }  
> > what if unplug is called on cold-plugged device?
> > 
> > Better way to disable mgmt initiated unplug is to forbid it in unplug_request()
> > For guest initiated one ('unplug' handler), the best we can do is log error
> > and ignore it (provided guest won't get in confused). it's also possible 
> > to hide _EJ method and then it would be even fine to abort if it gets here,
> > since guest is not supposed to interface with MMIO interface without using AML.  
> I don't understand the guest initiated unplug handling. How does it
> differ from x86 handling as we are using the same dynamic methods?
The only differences I'm aware of, should be event delivery mechanism and
using MMIO vs IO. Otherwise it's the same as described in
docs/specs/acpi_mem_hotplug.txt

> Besides In ARM case I checked
> /sys/devices/system/memory/memoryX/removable state for cold-plugged and
> hot-plugged DIMMs and it is equal to 0. I don't know if it is a
> limitation of the guest or a bug in our ACPI description somewhere. I
> would not be surprised if hot-unplug is not supported at kernel leve
> though.
enabling mhp_acpi_foo trace events might help to check what's going on,
and a dump of ACPI tables too (SRAT + DSDT).

It's quite possible that kernel doesn't support unplug yet, all I care at
the moment is to make sure that we won't have cross-version compatibility
issues when kernel gets there so we won't have to add and support compat
knobs later on.

> Thanks
> 
> Eric
> >   
> >> +
> >>      pc_dimm_unplug(PC_DIMM(dev), MACHINE(hotplug_dev));
> >>      object_unparent(OBJECT(dev));
> >>  }
> >> @@ -2085,6 +2132,12 @@ static void virt_instance_init(Object *obj)
> >>                                      "Set NVDIMM persistence"
> >>                                      "Valid values are cpu and mem-ctrl", NULL);
> >>  
> >> +    vms->acpi_memhp_state.is_enabled = true;
> >> +    object_property_add_bool(obj, "memory-hotplug-support",
> >> +                             virt_get_memhp_support,
> >> +                             virt_set_memhp_support,
> >> +                             NULL);
> >> +
> >>      vms->irqmap = a15irqmap;
> >>  }
> >>    
> >

Igor Mammedov March 1, 2019, 9:12 a.m. UTC | #5

On Mon, 28 Jan 2019 11:05:45 +0000
Shameer Kolothum <[e-mail]> wrote:

> pc-dimm memory hotplug is enabled using GPIO(Pin 2) based ACPI
> event. Hot removal functionality is not yet supported.
> 
> Signed-off-by: Shameer Kolothum <[e-mail]>
> ---
>  hw/arm/virt.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 55 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 884960d..cf64554 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -62,6 +62,7 @@
>  #include "hw/mem/pc-dimm.h"
>  #include "hw/mem/nvdimm.h"
>  #include "hw/acpi/acpi.h"
> +#include "hw/acpi/pc-hotplug.h"
it looks like x86 specific file, what is this here for?

>  
>  #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
>      static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> @@ -1651,7 +1652,14 @@ static void machvirt_init(MachineState *machine)
>          nvdimm_init_acpi_state(acpi_nvdimm_state, sysmem,
>                                 vms->fw_cfg, OBJECT(vms));
>      }
> +    if (vms->acpi_memhp_state.is_enabled) {
> +        MemHotplugState *state =  &vms->acpi_memhp_state;
> +        hwaddr base;
>  
> +        state->hw_reduced_acpi = true;
> +        base = vms->memmap[VIRT_ACPI_IO].base + ACPI_MEMORY_HOTPLUG_BASE;
> +        acpi_memory_hotplug_init(sysmem, OBJECT(vms), state, base);
> +    }
>      vms->bootinfo.ram_size = machine->ram_size;
>      vms->bootinfo.kernel_filename = machine->kernel_filename;
>      vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
[...]

Shameerali Kolothum Thodi March 1, 2019, 9:23 a.m. UTC | #6

> -----Original Message-----
> From: Igor Mammedov [mailto:[e-mail]]
> Sent: 01 March 2019 09:12
> To: Shameerali Kolothum Thodi <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Linuxarm <[e-mail]>; xuwei (O) <[e-mail]>
> Subject: Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm
> hotplug support
> 
> On Mon, 28 Jan 2019 11:05:45 +0000
> Shameer Kolothum <[e-mail]> wrote:
> 
> > pc-dimm memory hotplug is enabled using GPIO(Pin 2) based ACPI
> > event. Hot removal functionality is not yet supported.
> >
> > Signed-off-by: Shameer Kolothum <[e-mail]>
> > ---
> >  hw/arm/virt.c | 57
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 55 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 884960d..cf64554 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -62,6 +62,7 @@
> >  #include "hw/mem/pc-dimm.h"
> >  #include "hw/mem/nvdimm.h"
> >  #include "hw/acpi/acpi.h"
> > +#include "hw/acpi/pc-hotplug.h"
> it looks like x86 specific file, what is this here for?

Yes. That is for ACPI_MEMORY_HOTPLUG_BASE which is only used by x86
at the moment. I guess, it can be moved to hw/acpi/memory_hotplug.h ?

> >
> >  #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
> >      static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> > @@ -1651,7 +1652,14 @@ static void machvirt_init(MachineState
> *machine)
> >          nvdimm_init_acpi_state(acpi_nvdimm_state, sysmem,
> >                                 vms->fw_cfg, OBJECT(vms));
> >      }
> > +    if (vms->acpi_memhp_state.is_enabled) {
> > +        MemHotplugState *state =  &vms->acpi_memhp_state;
> > +        hwaddr base;
> >
> > +        state->hw_reduced_acpi = true;
> > +        base = vms->memmap[VIRT_ACPI_IO].base +
> ACPI_MEMORY_HOTPLUG_BASE;
> > +        acpi_memory_hotplug_init(sysmem, OBJECT(vms), state, base);
> > +    }
> >      vms->bootinfo.ram_size = machine->ram_size;
> >      vms->bootinfo.kernel_filename = machine->kernel_filename;
> >      vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> [...]

Igor Mammedov March 1, 2019, 10:26 a.m. UTC | #7

On Fri, 1 Mar 2019 09:23:11 +0000
Shameerali Kolothum Thodi <[e-mail]> wrote:

> > -----Original Message-----
> > From: Igor Mammedov [mailto:[e-mail]]
> > Sent: 01 March 2019 09:12
> > To: Shameerali Kolothum Thodi <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > Linuxarm <[e-mail]>; xuwei (O) <[e-mail]>
> > Subject: Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm
> > hotplug support
> > 
> > On Mon, 28 Jan 2019 11:05:45 +0000
> > Shameer Kolothum <[e-mail]> wrote:
> >   
> > > pc-dimm memory hotplug is enabled using GPIO(Pin 2) based ACPI
> > > event. Hot removal functionality is not yet supported.
> > >
> > > Signed-off-by: Shameer Kolothum <[e-mail]>
> > > ---
> > >  hw/arm/virt.c | 57  
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++++--  
> > >  1 file changed, 55 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > index 884960d..cf64554 100644
> > > --- a/hw/arm/virt.c
> > > +++ b/hw/arm/virt.c
> > > @@ -62,6 +62,7 @@
> > >  #include "hw/mem/pc-dimm.h"
> > >  #include "hw/mem/nvdimm.h"
> > >  #include "hw/acpi/acpi.h"
> > > +#include "hw/acpi/pc-hotplug.h"  
> > it looks like x86 specific file, what is this here for?  
> 
> Yes. That is for ACPI_MEMORY_HOTPLUG_BASE which is only used by x86
> at the moment. I guess, it can be moved to hw/acpi/memory_hotplug.h ?
it's GPA and pc/q35 impl. specific so you should use it,
this address will always be board specific one.
Makeup a virt specific one

> 
> > >
> > >  #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
> > >      static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> > > @@ -1651,7 +1652,14 @@ static void machvirt_init(MachineState  
> > *machine)  
> > >          nvdimm_init_acpi_state(acpi_nvdimm_state, sysmem,
> > >                                 vms->fw_cfg, OBJECT(vms));
> > >      }
> > > +    if (vms->acpi_memhp_state.is_enabled) {
> > > +        MemHotplugState *state =  &vms->acpi_memhp_state;
> > > +        hwaddr base;
> > >
> > > +        state->hw_reduced_acpi = true;
> > > +        base = vms->memmap[VIRT_ACPI_IO].base +  
> > ACPI_MEMORY_HOTPLUG_BASE;  
well, this is confusing, why adding 2 base addresses?
If vms->memmap[VIRT_ACPI_IO].base is already set than why not use it
as is without adding an offset?


> > > +        acpi_memory_hotplug_init(sysmem, OBJECT(vms), state, base);
> > > +    }
> > >      vms->bootinfo.ram_size = machine->ram_size;
> > >      vms->bootinfo.kernel_filename = machine->kernel_filename;
> > >      vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;  
> > [...]

Igor Mammedov March 1, 2019, 10:33 a.m. UTC | #8

On Fri, 1 Mar 2019 11:26:35 +0100
Igor Mammedov <[e-mail]> wrote:

> On Fri, 1 Mar 2019 09:23:11 +0000
> Shameerali Kolothum Thodi <[e-mail]> wrote:
> 
> > > -----Original Message-----
> > > From: Igor Mammedov [mailto:[e-mail]]
> > > Sent: 01 March 2019 09:12
> > > To: Shameerali Kolothum Thodi <[e-mail]>
> > > Cc: [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> > > Linuxarm <[e-mail]>; xuwei (O) <[e-mail]>
> > > Subject: Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm
> > > hotplug support
> > > 
> > > On Mon, 28 Jan 2019 11:05:45 +0000
> > > Shameer Kolothum <[e-mail]> wrote:
> > >     
> > > > pc-dimm memory hotplug is enabled using GPIO(Pin 2) based ACPI
> > > > event. Hot removal functionality is not yet supported.
> > > >
> > > > Signed-off-by: Shameer Kolothum <[e-mail]>
> > > > ---
> > > >  hw/arm/virt.c | 57    
> > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++--    
> > > >  1 file changed, 55 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > > index 884960d..cf64554 100644
> > > > --- a/hw/arm/virt.c
> > > > +++ b/hw/arm/virt.c
> > > > @@ -62,6 +62,7 @@
> > > >  #include "hw/mem/pc-dimm.h"
> > > >  #include "hw/mem/nvdimm.h"
> > > >  #include "hw/acpi/acpi.h"
> > > > +#include "hw/acpi/pc-hotplug.h"    
> > > it looks like x86 specific file, what is this here for?    
> > 
> > Yes. That is for ACPI_MEMORY_HOTPLUG_BASE which is only used by x86
> > at the moment. I guess, it can be moved to hw/acpi/memory_hotplug.h ?  
> it's GPA and pc/q35 impl. specific so you should use it,
s/should/should not/

> this address will always be board specific one.
> Makeup a virt specific one
> 
> >   
> > > >
> > > >  #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
> > > >      static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> > > > @@ -1651,7 +1652,14 @@ static void machvirt_init(MachineState    
> > > *machine)    
> > > >          nvdimm_init_acpi_state(acpi_nvdimm_state, sysmem,
> > > >                                 vms->fw_cfg, OBJECT(vms));
> > > >      }
> > > > +    if (vms->acpi_memhp_state.is_enabled) {
> > > > +        MemHotplugState *state =  &vms->acpi_memhp_state;
> > > > +        hwaddr base;
> > > >
> > > > +        state->hw_reduced_acpi = true;
> > > > +        base = vms->memmap[VIRT_ACPI_IO].base +    
> > > ACPI_MEMORY_HOTPLUG_BASE;    
> well, this is confusing, why adding 2 base addresses?
> If vms->memmap[VIRT_ACPI_IO].base is already set than why not use it
> as is without adding an offset?
> 
> 
> > > > +        acpi_memory_hotplug_init(sysmem, OBJECT(vms), state, base);
> > > > +    }
> > > >      vms->bootinfo.ram_size = machine->ram_size;
> > > >      vms->bootinfo.kernel_filename = machine->kernel_filename;
> > > >      vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;    
> > > [...]    
> 
>

Shameerali Kolothum Thodi March 1, 2019, 10:51 a.m. UTC | #9

> -----Original Message-----
> From: Qemu-devel
> [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> u.org] On Behalf Of Igor Mammedov
> Sent: 01 March 2019 10:34
> To: Shameerali Kolothum Thodi <[e-mail]>
> Cc: [e-mail]; [e-mail];
> [e-mail]; Linuxarm <[e-mail]>;
> [e-mail]; [e-mail]; xuwei (O)
> <[e-mail]>
> Subject: Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm
> hotplug support
> 
> On Fri, 1 Mar 2019 11:26:35 +0100
> Igor Mammedov <[e-mail]> wrote:
> 
> > On Fri, 1 Mar 2019 09:23:11 +0000
> > Shameerali Kolothum Thodi <[e-mail]>
> wrote:
> >
> > > > -----Original Message-----
> > > > From: Igor Mammedov [mailto:[e-mail]]
> > > > Sent: 01 March 2019 09:12
> > > > To: Shameerali Kolothum Thodi
> <[e-mail]>
> > > > Cc: [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail];
> [e-mail];
> > > > Linuxarm <[e-mail]>; xuwei (O) <[e-mail]>
> > > > Subject: Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm
> > > > hotplug support
> > > >
> > > > On Mon, 28 Jan 2019 11:05:45 +0000
> > > > Shameer Kolothum <[e-mail]> wrote:
> > > >
> > > > > pc-dimm memory hotplug is enabled using GPIO(Pin 2) based ACPI
> > > > > event. Hot removal functionality is not yet supported.
> > > > >
> > > > > Signed-off-by: Shameer Kolothum
> <[e-mail]>
> > > > > ---
> > > > >  hw/arm/virt.c | 57
> > > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> > > > >  1 file changed, 55 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > > > index 884960d..cf64554 100644
> > > > > --- a/hw/arm/virt.c
> > > > > +++ b/hw/arm/virt.c
> > > > > @@ -62,6 +62,7 @@
> > > > >  #include "hw/mem/pc-dimm.h"
> > > > >  #include "hw/mem/nvdimm.h"
> > > > >  #include "hw/acpi/acpi.h"
> > > > > +#include "hw/acpi/pc-hotplug.h"
> > > > it looks like x86 specific file, what is this here for?
> > >
> > > Yes. That is for ACPI_MEMORY_HOTPLUG_BASE which is only used by x86
> > > at the moment. I guess, it can be moved to hw/acpi/memory_hotplug.h ?
> > it's GPA and pc/q35 impl. specific so you should use it,
> s/should/should not/
> 
> > this address will always be board specific one.
> > Makeup a virt specific one

Ok. I was under the impression that the offsets can be reused as it is defined
here docs/specs/acpi_mem_hotplug.txt(though that is GPE and pc/q35 acpi dev 
specific). But agree, it doesn't make sense to make it generic.

> > >
> > > > >
> > > > >  #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
> > > > >      static void virt_##major##_##minor##_class_init(ObjectClass *oc,
> \
> > > > > @@ -1651,7 +1652,14 @@ static void machvirt_init(MachineState
> > > > *machine)
> > > > >          nvdimm_init_acpi_state(acpi_nvdimm_state, sysmem,
> > > > >                                 vms->fw_cfg, OBJECT(vms));
> > > > >      }
> > > > > +    if (vms->acpi_memhp_state.is_enabled) {
> > > > > +        MemHotplugState *state =  &vms->acpi_memhp_state;
> > > > > +        hwaddr base;
> > > > >
> > > > > +        state->hw_reduced_acpi = true;
> > > > > +        base = vms->memmap[VIRT_ACPI_IO].base +
> > > > ACPI_MEMORY_HOTPLUG_BASE;
> > well, this is confusing, why adding 2 base addresses?
> > If vms->memmap[VIRT_ACPI_IO].base is already set than why not use it
> > as is without adding an offset?

Well, Eric's work on which this was based had one NVDIMM_ACPI_IO_BASE offset
from what appears to be a generic VIRT_ACPI_IO region. Now I see that, it is
renamed to VIRT_NVDIMM_ACPI_IO. Do we really need two separate regions?

Thanks,
Shameer

> >
> > > > > +        acpi_memory_hotplug_init(sysmem, OBJECT(vms), state,
> base);
> > > > > +    }
> > > > >      vms->bootinfo.ram_size = machine->ram_size;
> > > > >      vms->bootinfo.kernel_filename = machine->kernel_filename;
> > > > >      vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
> > > > [...]
> >
> >
>

Igor Mammedov March 1, 2019, 1:09 p.m. UTC | #10

On Fri, 1 Mar 2019 10:51:45 +0000
Shameerali Kolothum Thodi <[e-mail]> wrote:

> > -----Original Message-----
> > From: Qemu-devel
> > [mailto:qemu-devel-bounces+shameerali.kolothum.thodi=huawei.com@nongn
> > u.org] On Behalf Of Igor Mammedov
> > Sent: 01 March 2019 10:34
> > To: Shameerali Kolothum Thodi <[e-mail]>
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; Linuxarm <[e-mail]>;
> > [e-mail]; [e-mail]; xuwei (O)
> > <[e-mail]>
> > Subject: Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm
> > hotplug support
> > 
> > On Fri, 1 Mar 2019 11:26:35 +0100
> > Igor Mammedov <[e-mail]> wrote:
> >   
> > > On Fri, 1 Mar 2019 09:23:11 +0000
> > > Shameerali Kolothum Thodi <[e-mail]>  
> > wrote:  
> > >  
> > > > > -----Original Message-----
> > > > > From: Igor Mammedov [mailto:[e-mail]]
> > > > > Sent: 01 March 2019 09:12
> > > > > To: Shameerali Kolothum Thodi  
> > <[e-mail]>  
> > > > > Cc: [e-mail]; [e-mail];
> > > > > [e-mail]; [e-mail];  
> > [e-mail];  
> > > > > Linuxarm <[e-mail]>; xuwei (O) <[e-mail]>
> > > > > Subject: Re: [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm
> > > > > hotplug support
> > > > >
> > > > > On Mon, 28 Jan 2019 11:05:45 +0000
> > > > > Shameer Kolothum <[e-mail]> wrote:
> > > > >  
> > > > > > pc-dimm memory hotplug is enabled using GPIO(Pin 2) based ACPI
> > > > > > event. Hot removal functionality is not yet supported.
> > > > > >
> > > > > > Signed-off-by: Shameer Kolothum  
> > <[e-mail]>  
> > > > > > ---
> > > > > >  hw/arm/virt.c | 57  
> > > > > +++++++++++++++++++++++++++++++++++++++++++++++++++++++--  
> > > > > >  1 file changed, 55 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > > > > > index 884960d..cf64554 100644
> > > > > > --- a/hw/arm/virt.c
> > > > > > +++ b/hw/arm/virt.c
> > > > > > @@ -62,6 +62,7 @@
> > > > > >  #include "hw/mem/pc-dimm.h"
> > > > > >  #include "hw/mem/nvdimm.h"
> > > > > >  #include "hw/acpi/acpi.h"
> > > > > > +#include "hw/acpi/pc-hotplug.h"  
> > > > > it looks like x86 specific file, what is this here for?  
> > > >
> > > > Yes. That is for ACPI_MEMORY_HOTPLUG_BASE which is only used by x86
> > > > at the moment. I guess, it can be moved to hw/acpi/memory_hotplug.h ?  
> > > it's GPA and pc/q35 impl. specific so you should use it,  
> > s/should/should not/
> >   
> > > this address will always be board specific one.
> > > Makeup a virt specific one  
> 
> Ok. I was under the impression that the offsets can be reused as it is defined
> here docs/specs/acpi_mem_hotplug.txt(though that is GPE and pc/q35 acpi dev 
> specific). But agree, it doesn't make sense to make it generic.
Offsets defined by docs/specs/acpi_mem_hotplug.txt are meant to be reused
but IO port address (0xa00) where interface's address space starts is board specific.


> > > >  
> > > > > >
> > > > > >  #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
> > > > > >      static void virt_##major##_##minor##_class_init(ObjectClass *oc,  
> > \  
> > > > > > @@ -1651,7 +1652,14 @@ static void machvirt_init(MachineState  
> > > > > *machine)  
> > > > > >          nvdimm_init_acpi_state(acpi_nvdimm_state, sysmem,
> > > > > >                                 vms->fw_cfg, OBJECT(vms));
> > > > > >      }
> > > > > > +    if (vms->acpi_memhp_state.is_enabled) {
> > > > > > +        MemHotplugState *state =  &vms->acpi_memhp_state;
> > > > > > +        hwaddr base;
> > > > > >
> > > > > > +        state->hw_reduced_acpi = true;
> > > > > > +        base = vms->memmap[VIRT_ACPI_IO].base +  
> > > > > ACPI_MEMORY_HOTPLUG_BASE;  
> > > well, this is confusing, why adding 2 base addresses?
> > > If vms->memmap[VIRT_ACPI_IO].base is already set than why not use it
> > > as is without adding an offset?  
> 
> Well, Eric's work on which this was based had one NVDIMM_ACPI_IO_BASE offset
> from what appears to be a generic VIRT_ACPI_IO region. Now I see that, it is
> renamed to VIRT_NVDIMM_ACPI_IO. Do we really need two separate regions?
I'm afraid we can't reuse MMIO regions as ther might be used at the same time
and do different things (we would have done this for x86 if it was possible)

As for naming try to find some consensus/coordinate it with Eric


> Thanks,
> Shameer
> 
> > >  
> > > > > > +        acpi_memory_hotplug_init(sysmem, OBJECT(vms), state,  
> > base);  
> > > > > > +    }
> > > > > >      vms->bootinfo.ram_size = machine->ram_size;
> > > > > >      vms->bootinfo.kernel_filename = machine->kernel_filename;
> > > > > >      vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;  
> > > > > [...]  
> > >
> > >  
> >   
>

Patch

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 884960d..cf64554 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -62,6 +62,7 @@ 
 #include "hw/mem/pc-dimm.h"
 #include "hw/mem/nvdimm.h"
 #include "hw/acpi/acpi.h"
+#include "hw/acpi/pc-hotplug.h"
 
 #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
     static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
@@ -1651,7 +1652,14 @@  static void machvirt_init(MachineState *machine)
         nvdimm_init_acpi_state(acpi_nvdimm_state, sysmem,
                                vms->fw_cfg, OBJECT(vms));
     }
+    if (vms->acpi_memhp_state.is_enabled) {
+        MemHotplugState *state =  &vms->acpi_memhp_state;
+        hwaddr base;
 
+        state->hw_reduced_acpi = true;
+        base = vms->memmap[VIRT_ACPI_IO].base + ACPI_MEMORY_HOTPLUG_BASE;
+        acpi_memory_hotplug_init(sysmem, OBJECT(vms), state, base);
+    }
     vms->bootinfo.ram_size = machine->ram_size;
     vms->bootinfo.kernel_filename = machine->kernel_filename;
     vms->bootinfo.kernel_cmdline = machine->kernel_cmdline;
@@ -1819,6 +1827,20 @@  static void virt_set_nvdimm_persistence(Object *obj, const char *value,
     nvdimm_state->persistence_string = g_strdup(value);
 }
 
+static bool virt_get_memhp_support(Object *obj, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(obj);
+
+    return vms->acpi_memhp_state.is_enabled;
+}
+
+static void virt_set_memhp_support(Object *obj, bool value, Error **errp)
+{
+    VirtMachineState *vms = VIRT_MACHINE(obj);
+
+    vms->acpi_memhp_state.is_enabled = value;
+}
+
 static CpuInstanceProperties
 virt_cpu_index_to_props(MachineState *ms, unsigned cpu_index)
 {
@@ -1863,8 +1885,8 @@  static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
     VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
 
-    if (dev->hotplugged) {
-        error_setg(errp, "memory hotplug is not supported");
+    if (dev->hotplugged && is_nvdimm) {
+        error_setg(errp, "nvdimm hotplug is not supported");
     }
 
     if (is_nvdimm && !vms->acpi_nvdimm_state.is_enabled) {
@@ -1875,6 +1897,22 @@  static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL, errp);
 }
 
+static void virt_memhp_send_event(HotplugHandler *hotplug_dev, DeviceState *dev,
+                                  Error **errp)
+{
+    DeviceState *gpio_dev;
+    VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
+
+    gpio_dev = virt_get_gpio_dev(GPIO_PCDIMM);
+    if (!gpio_dev) {
+        error_setg(errp, "No dev interface to send hotplug event");
+        return;
+    }
+    acpi_memory_plug_cb(hotplug_dev, &vms->acpi_memhp_state,
+                        dev, errp);
+    qemu_set_irq(qdev_get_gpio_in(gpio_dev, 0), 1);
+}
+
 static void virt_memory_plug(HotplugHandler *hotplug_dev,
                              DeviceState *dev, Error **errp)
 {
@@ -1891,6 +1929,10 @@  static void virt_memory_plug(HotplugHandler *hotplug_dev,
         nvdimm_plug(&vms->acpi_nvdimm_state);
     }
 
+    if (dev->hotplugged && !is_nvdimm) {
+        virt_memhp_send_event(hotplug_dev, dev, errp);
+    }
+
 out:
     error_propagate(errp, local_err);
 }
@@ -1898,6 +1940,11 @@  out:
 static void virt_memory_unplug(HotplugHandler *hotplug_dev,
                                DeviceState *dev, Error **errp)
 {
+    if (dev->hotplugged) {
+        error_setg(errp, "memory hot unplug is not supported");
+        return;
+    }
+
     pc_dimm_unplug(PC_DIMM(dev), MACHINE(hotplug_dev));
     object_unparent(OBJECT(dev));
 }
@@ -2085,6 +2132,12 @@  static void virt_instance_init(Object *obj)
                                     "Set NVDIMM persistence"
                                     "Valid values are cpu and mem-ctrl", NULL);
 
+    vms->acpi_memhp_state.is_enabled = true;
+    object_property_add_bool(obj, "memory-hotplug-support",
+                             virt_get_memhp_support,
+                             virt_set_memhp_support,
+                             NULL);
+
     vms->irqmap = a15irqmap;
 }