Re: [Qemu-devel] [PATCH v4 26/33] acpi:ich9: add memory hotplug handling

[Michael S. Tsirkin]

On Mon, Jun 02, 2014 at 03:25:22PM +0200, Igor Mammedov wrote:
> Add memory hotplug initialization/handling to ICH9 LPC device
> and enable it by default for post 2.0 machine types
> 
> Signed-off-by: Igor Mammedov <address@hidden>

I applied this, resolving a conflict.
Minor comments below, would like to see them addressed
in follow-up patches.

> ---
>  hw/acpi/ich9.c         |   38 ++++++++++++++++++++++++++++++++++++++
>  hw/isa/lpc_ich9.c      |   20 ++++++++++++++++++++
>  include/hw/acpi/ich9.h |    4 ++++
>  include/hw/i386/pc.h   |    7 ++++++-
>  4 files changed, 68 insertions(+), 1 deletions(-)
> 
> diff --git a/hw/acpi/ich9.c b/hw/acpi/ich9.c
> index 0afac42..7b10c27 100644
> --- a/hw/acpi/ich9.c
> +++ b/hw/acpi/ich9.c
> @@ -34,6 +34,7 @@
>  #include "exec/address-spaces.h"
>  
>  #include "hw/i386/ich9.h"
> +#include "hw/mem/pc-dimm.h"
>  
>  //#define DEBUG
>  
> @@ -224,6 +225,11 @@ void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
>                          &pm->gpe_cpu, ICH9_CPU_HOTPLUG_IO_BASE);
>      pm->cpu_added_notifier.notify = ich9_cpu_added_req;
>      qemu_register_cpu_added_notifier(&pm->cpu_added_notifier);
> +
> +    if (pm->acpi_memory_hotplug.is_enabled) {
> +        acpi_memory_hotplug_init(pci_address_space_io(lpc_pci), 
> OBJECT(lpc_pci),
> +                                 &pm->acpi_memory_hotplug);
> +    }
>  }
>  
>  static void ich9_pm_get_gpe0_blk(Object *obj, Visitor *v,
> @@ -236,9 +242,25 @@ static void ich9_pm_get_gpe0_blk(Object *obj, Visitor *v,
>      visit_type_uint32(v, &value, name, errp);
>  }
>  
> +static bool ich9_pm_get_memory_hotplug_support(Object *obj, Error **errp)
> +{
> +    ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> +
> +    return s->pm.acpi_memory_hotplug.is_enabled;
> +}
> +
> +static void ich9_pm_set_memory_hotplug_support(Object *obj, bool value,
> +                                               Error **errp)
> +{
> +    ICH9LPCState *s = ICH9_LPC_DEVICE(obj);
> +
> +    s->pm.acpi_memory_hotplug.is_enabled = value;
> +}
> +
>  void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp)
>  {
>      static const uint32_t gpe0_len = ICH9_PMIO_GPE0_LEN;
> +    pm->acpi_memory_hotplug.is_enabled = true;
>  
>      object_property_add_uint32_ptr(obj, ACPI_PM_PROP_PM_IO_BASE,
>                                     &pm->pm_io_base, errp);
> @@ -247,4 +269,20 @@ void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs 
> *pm, Error **errp)
>                          NULL, NULL, pm, NULL);
>      object_property_add_uint32_ptr(obj, ACPI_PM_PROP_GPE0_BLK_LEN,
>                                     &gpe0_len, errp);
> +    object_property_add_bool(obj, "memory-hotplug-support",
> +                             ich9_pm_get_memory_hotplug_support,
> +                             ich9_pm_set_memory_hotplug_support,
> +                             NULL);
> +}
> +
> +void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error 
> **errp)
> +{
> +    if (pm->acpi_memory_hotplug.is_enabled &&
> +        object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> +        acpi_memory_plug_cb(&pm->acpi_regs, pm->irq, 
> &pm->acpi_memory_hotplug,
> +                            dev, errp);
> +    } else {
> +        error_setg(errp, "acpi: device plug request for not supported device"
> +                   " type: %s", object_get_typename(OBJECT(dev)));
> +    }
>  }
> diff --git a/hw/isa/lpc_ich9.c b/hw/isa/lpc_ich9.c
> index 46de3b6..2adf29a 100644
> --- a/hw/isa/lpc_ich9.c
> +++ b/hw/isa/lpc_ich9.c
> @@ -599,6 +599,19 @@ static int ich9_lpc_init(PCIDevice *d)
>      return 0;
>  }
>  
> +static void ich9_device_plug_cb(HotplugHandler *hotplug_dev,
> +                                DeviceState *dev, Error **errp)
> +{
> +    ICH9LPCState *lpc = ICH9_LPC_DEVICE(hotplug_dev);
> +
> +    ich9_pm_device_plug_cb(&lpc->pm, dev, errp);
> +}
> +
> +static void ich9_device_unplug_cb(HotplugHandler *hotplug_dev,
> +                                  DeviceState *dev, Error **errp)
> +{

Can this set error so user knows this does not work?

> +}
> +
>  static bool ich9_rst_cnt_needed(void *opaque)
>  {
>      ICH9LPCState *lpc = opaque;
> @@ -643,6 +656,7 @@ static void ich9_lpc_class_init(ObjectClass *klass, void 
> *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    HotplugHandlerClass *hc = HOTPLUG_HANDLER_CLASS(klass);
>  
>      set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories);
>      dc->reset = ich9_lpc_reset;
> @@ -659,6 +673,8 @@ static void ich9_lpc_class_init(ObjectClass *klass, void 
> *data)
>       * pc_q35_init()
>       */
>      dc->cannot_instantiate_with_device_add_yet = true;
> +    hc->plug = ich9_device_plug_cb;
> +    hc->unplug = ich9_device_unplug_cb;
>  }
>  
>  static const TypeInfo ich9_lpc_info = {
> @@ -667,6 +683,10 @@ static const TypeInfo ich9_lpc_info = {
>      .instance_size = sizeof(struct ICH9LPCState),
>      .instance_init = ich9_lpc_initfn,
>      .class_init  = ich9_lpc_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_HOTPLUG_HANDLER },
> +        { }
> +    }
>  };
>  
>  static void ich9_lpc_register(void)
> diff --git a/include/hw/acpi/ich9.h b/include/hw/acpi/ich9.h
> index 104f419..1977f1b 100644
> --- a/include/hw/acpi/ich9.h
> +++ b/include/hw/acpi/ich9.h
> @@ -23,6 +23,7 @@
>  
>  #include "hw/acpi/acpi.h"
>  #include "hw/acpi/cpu_hotplug.h"
> +#include "hw/acpi/memory_hotplug.h"
>  
>  typedef struct ICH9LPCPMRegs {
>      /*
> @@ -46,6 +47,8 @@ typedef struct ICH9LPCPMRegs {
>  
>      AcpiCpuHotplug gpe_cpu;
>      Notifier cpu_added_notifier;
> +
> +    MemHotplugState acpi_memory_hotplug;
>  } ICH9LPCPMRegs;
>  
>  void ich9_pm_init(PCIDevice *lpc_pci, ICH9LPCPMRegs *pm,
> @@ -55,4 +58,5 @@ extern const VMStateDescription vmstate_ich9_pm;
>  
>  void ich9_pm_add_properties(Object *obj, ICH9LPCPMRegs *pm, Error **errp);
>  
> +void ich9_pm_device_plug_cb(ICH9LPCPMRegs *pm, DeviceState *dev, Error 
> **errp);
>  #endif /* HW_ACPI_ICH9_H */
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 34c3a63..cecd4c2 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -286,7 +286,12 @@ int e820_get_num_entries(void);
>  bool e820_get_entry(int, uint32_t, uint64_t *, uint64_t *);
>  
>  #define PC_Q35_COMPAT_2_0 \
> -        PC_COMPAT_2_0
> +        PC_COMPAT_2_0, \
> +        {\
> +            .driver   = "ICH9 LPC",\
> +            .property = "memory-hotplug-support",\
> +            .value    = "off",\
> +        }
> 

Spaces in names are evil.
Let's rename to ICH9_LPC or something.

 
>  #define PC_Q35_COMPAT_1_7 \
>          PC_COMPAT_1_7, \
> -- 
> 1.7.1