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

[Auger Eric <eric.auger@redhat.com>]

Hi Igor,

On 2/27/19 6:14 PM, Igor Mammedov wrote:
> 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.
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;
>>  }
>>  
>