Re: [Qemu-arm] [Qemu-devel] [RFC PATCH 3/4] hw/arm/virt: Enable pc-dimm hotplug support

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Igor Mammedov [mailto:address@hidden
> Sent: 27 February 2019 17:14
> To: Shameerali Kolothum Thodi <address@hidden>
> Cc: address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> Linuxarm <address@hidden>; xuwei (O) <address@hidden>
> 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 <address@hidden> 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 <address@hidden>
> > ---
> >  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;
> >  }
> >