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

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 28 Jan 2019 11:05:45 +0000
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> 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 <shameerali.kolothum.thodi@huawei.com>
> ---
>  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;
>  }
>