diff mbox series

[PATCH-for-4.2,v8,6/9] hw/arm/virt: Enable device memory cold/hot plug with ACPI boot

Message ID 20190726104519.23812-7-shameerali.kolothum.thodi@huawei.com
State New
Headers show
Series ARM virt: ACPI memory hotplug support | expand

Commit Message

Shameerali Kolothum Thodi July 26, 2019, 10:45 a.m. UTC 
This initializes the GED device with base memory and irq, configures
ged memory hotplug event and builds the corresponding aml code. With
this, both hot and cold plug of device memory is enabled now for Guest
with ACPI boot.

Memory cold plug support with Guest DT boot is not yet supported.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
---
 -Changed no_acpi_dev to no_ged.
 -Fixed 'dev' reference leak by object_new().
 -Updated bios-tables-test-allowed-diff.h to avoid "make check"
  failure.

---
 hw/arm/Kconfig                        |  2 +
 hw/arm/virt-acpi-build.c              | 14 +++++++
 hw/arm/virt.c                         | 54 +++++++++++++++++++++++----
 include/hw/arm/virt.h                 |  4 ++
 tests/bios-tables-test-allowed-diff.h |  1 +
 5 files changed, 68 insertions(+), 7 deletions(-)

Comments

Igor Mammedov Aug. 6, 2019, 1:08 p.m. UTC | #1 
On Fri, 26 Jul 2019 11:45:16 +0100
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> This initializes the GED device with base memory and irq, configures
> ged memory hotplug event and builds the corresponding aml code. With
> this, both hot and cold plug of device memory is enabled now for Guest
> with ACPI boot.
> 
> Memory cold plug support with Guest DT boot is not yet supported.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> ---
>  -Changed no_acpi_dev to no_ged.
>  -Fixed 'dev' reference leak by object_new().
>  -Updated bios-tables-test-allowed-diff.h to avoid "make check"
>   failure.
> 
> ---
>  hw/arm/Kconfig                        |  2 +
>  hw/arm/virt-acpi-build.c              | 14 +++++++
>  hw/arm/virt.c                         | 54 +++++++++++++++++++++++----
>  include/hw/arm/virt.h                 |  4 ++
>  tests/bios-tables-test-allowed-diff.h |  1 +
>  5 files changed, 68 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index 84961c17ab..ad7f7c089b 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -22,6 +22,8 @@ config ARM_VIRT
>      select ACPI_PCI
>      select MEM_DEVICE
>      select DIMM
> +    select ACPI_MEMORY_HOTPLUG
> +    select ACPI_HW_REDUCED
>  
>  config CHEETAH
>      bool
> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> index 0afb372769..018b1e326d 100644
> --- a/hw/arm/virt-acpi-build.c
> +++ b/hw/arm/virt-acpi-build.c
> @@ -40,6 +40,8 @@
>  #include "hw/acpi/aml-build.h"
>  #include "hw/acpi/utils.h"
>  #include "hw/acpi/pci.h"
> +#include "hw/acpi/memory_hotplug.h"
> +#include "hw/acpi/generic_event_device.h"
>  #include "hw/pci/pcie_host.h"
>  #include "hw/pci/pci.h"
>  #include "hw/arm/virt.h"
> @@ -705,6 +707,7 @@ static void
>  build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>  {
>      Aml *scope, *dsdt;
> +    MachineState *ms = MACHINE(vms);
>      const MemMapEntry *memmap = vms->memmap;
>      const int *irqmap = vms->irqmap;
>  
> @@ -729,6 +732,17 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
>                        vms->highmem, vms->highmem_ecam);
>      acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
>                         (irqmap[VIRT_GPIO] + ARM_SPI_BASE));
> +    if (vms->acpi_dev) {
> +        build_ged_aml(scope, "\\_SB."GED_DEVICE,
> +                      HOTPLUG_HANDLER(vms->acpi_dev),
> +                      irqmap[VIRT_ACPI_GED] + ARM_SPI_BASE, AML_SYSTEM_MEMORY);
> +    }
> +
> +    if (vms->acpi_dev && ms->ram_slots) {
> +        build_memory_hotplug_aml(scope, ms->ram_slots, "\\_SB", NULL,
> +                                 AML_SYSTEM_MEMORY);
> +    }
> +
>      acpi_dsdt_add_power_button(scope);
>  
>      aml_append(dsdt, scope);
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index bbe156dc35..41386a6eb7 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -66,6 +66,7 @@
>  #include "target/arm/internals.h"
>  #include "hw/mem/pc-dimm.h"
>  #include "hw/mem/nvdimm.h"
> +#include "hw/acpi/generic_event_device.h"
>  
>  #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
>      static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
> @@ -136,6 +137,8 @@ static const MemMapEntry base_memmap[] = {
>      [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
>      [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
>      [VIRT_SMMU] =               { 0x09050000, 0x00020000 },
> +    [VIRT_PCDIMM_ACPI] =        { 0x09070000, MEMORY_HOTPLUG_IO_LEN },
> +    [VIRT_ACPI_GED] =           { 0x09080000, ACPI_GED_EVT_SEL_LEN },
>      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> @@ -171,6 +174,7 @@ static const int a15irqmap[] = {
>      [VIRT_PCIE] = 3, /* ... to 6 */
>      [VIRT_GPIO] = 7,
>      [VIRT_SECURE_UART] = 8,
> +    [VIRT_ACPI_GED] = 9,
>      [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
>      [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */
>      [VIRT_SMMU] = 74,    /* ...to 74 + NUM_SMMU_IRQS - 1 */
> @@ -520,6 +524,27 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms)
>      }
>  }
>  
> +static inline DeviceState *create_acpi_ged(VirtMachineState *vms, qemu_irq *pic)
> +{
> +    DeviceState *dev;
> +    int irq = vms->irqmap[VIRT_ACPI_GED];
> +    uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT;
> +
> +    dev = DEVICE(object_new(TYPE_ACPI_GED));
> +    qdev_prop_set_uint64(dev, "memhp-base",
> +                         vms->memmap[VIRT_PCDIMM_ACPI].base);
> +    qdev_prop_set_uint64(dev, "ged-base", vms->memmap[VIRT_ACPI_GED].base);
> +    qdev_prop_set_uint32(dev, "ged-event", event);
> +    object_property_add_child(qdev_get_machine(), "acpi-ged",
> +                              OBJECT(dev), NULL);
> +    qdev_init_nofail(dev);
> +    qdev_connect_gpio_out_named(dev, "ged-irq", 0, pic[irq]);
> +
> +    object_unref(OBJECT(dev));
> +
> +    return dev;
> +}

this function will need changes to accommodate for sysbus device
init sequence [3/9].

> +
>  static void create_its(VirtMachineState *vms, DeviceState *gicdev)
>  {
>      const char *itsclass = its_class_name();
> @@ -1483,6 +1508,7 @@ static void machvirt_init(MachineState *machine)
>      MemoryRegion *ram = g_new(MemoryRegion, 1);
>      bool firmware_loaded;
>      bool aarch64 = true;
> +    bool has_ged = !vmc->no_ged;
>      unsigned int smp_cpus = machine->smp.cpus;
>      unsigned int max_cpus = machine->smp.max_cpus;
>  
> @@ -1697,6 +1723,10 @@ static void machvirt_init(MachineState *machine)
>  
>      create_gpio(vms, pic);
>  
> +    if (has_ged && aarch64 && firmware_loaded && acpi_enabled) {
> +        vms->acpi_dev = create_acpi_ged(vms, pic);
> +    }
> +
>      /* Create mmio transports, so the user can create virtio backends
>       * (which will be automatically plugged in to the transports). If
>       * no backend is created the transport will just sit harmlessly idle.
> @@ -1876,27 +1906,34 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>  static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                                   Error **errp)
>  {
> +    VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> +    const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
>  
> -    /*
> -     * The device memory is not yet exposed to the Guest either through
> -     * DT or ACPI and hence both cold/hot plug of memory is explicitly
> -     * disabled for now.
> -     */
> -    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> -        error_setg(errp, "memory cold/hot plug is not yet supported");
> +    if (is_nvdimm) {
> +        error_setg(errp, "nvdimm is not yet supported");
>          return;
>      }
>  
> +    if (!vms->acpi_dev) {
> +        error_setg(errp, "memory hotplug is not enabled: missing acpi device");
> +        return;
> +    }
> +
> +    hotplug_handler_pre_plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, errp);
use local_error and check for error condition here. see pc_memory_pre_plug()

> +
>      pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL, errp);
>  }
>  
>  static void virt_memory_plug(HotplugHandler *hotplug_dev,
>                               DeviceState *dev, Error **errp)
>  {
> +    HotplugHandlerClass *hhc;
>      VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
>  
>      pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), NULL);
                                                ^^^^
>  
> +    hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
> +    hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, NULL);
                                                      ^^^^
why errors are ignored here, pls check for errors and propagate
them to the caller.

>  }
>  
>  static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
> @@ -2102,8 +2139,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(4, 2)
>  
>  static void virt_machine_4_1_options(MachineClass *mc)
>  {
> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> +
>      virt_machine_4_2_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_4_1, hw_compat_4_1_len);
> +    vmc->no_ged = true;
>  }
>  DEFINE_VIRT_MACHINE(4, 1)
>  
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index a72094204e..577ee49b4b 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -77,6 +77,8 @@ enum {
>      VIRT_GPIO,
>      VIRT_SECURE_UART,
>      VIRT_SECURE_MEM,
> +    VIRT_PCDIMM_ACPI,
> +    VIRT_ACPI_GED,
>      VIRT_LOWMEMMAP_LAST,
>  };
>  
> @@ -106,6 +108,7 @@ typedef struct {
>      bool claim_edge_triggered_timers;
>      bool smbios_old_sys_ver;
>      bool no_highmem_ecam;
> +    bool no_ged;   /* Machines < 4.2 has no support for ACPI GED device */
>  } VirtMachineClass;
>  
>  typedef struct {
> @@ -133,6 +136,7 @@ typedef struct {
>      uint32_t iommu_phandle;
>      int psci_conduit;
>      hwaddr highest_gpa;
> +    DeviceState *acpi_dev;
>  } VirtMachineState;
>  
>  #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)
> diff --git a/tests/bios-tables-test-allowed-diff.h b/tests/bios-tables-test-allowed-diff.h
> index dfb8523c8b..7b4adbc822 100644
> --- a/tests/bios-tables-test-allowed-diff.h
> +++ b/tests/bios-tables-test-allowed-diff.h
> @@ -1 +1,2 @@
>  /* List of comma-separated changed AML files to ignore */
> +"tests/data/acpi/virt/DSDT",
Shameerali Kolothum Thodi Aug. 7, 2019, 8:19 a.m. UTC | #2 
Hi Igor,

> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: 06 August 2019 14:09
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org; sameo@linux.intel.com;
> ard.biesheuvel@linaro.org; Linuxarm <linuxarm@huawei.com>; xuwei (O)
> <xuwei5@huawei.com>; shannon.zhaosl@gmail.com;
> sebastien.boeuf@intel.com; lersek@redhat.com
> Subject: Re: [Qemu-devel] [PATCH-for-4.2 v8 6/9] hw/arm/virt: Enable device
> memory cold/hot plug with ACPI boot
 
[...]

> > +static inline DeviceState *create_acpi_ged(VirtMachineState *vms,
> qemu_irq *pic)
> > +{
> > +    DeviceState *dev;
> > +    int irq = vms->irqmap[VIRT_ACPI_GED];
> > +    uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT;
> > +
> > +    dev = DEVICE(object_new(TYPE_ACPI_GED));
> > +    qdev_prop_set_uint64(dev, "memhp-base",
> > +                         vms->memmap[VIRT_PCDIMM_ACPI].base);
> > +    qdev_prop_set_uint64(dev, "ged-base",
> vms->memmap[VIRT_ACPI_GED].base);
> > +    qdev_prop_set_uint32(dev, "ged-event", event);
> > +    object_property_add_child(qdev_get_machine(), "acpi-ged",
> > +                              OBJECT(dev), NULL);
> > +    qdev_init_nofail(dev);
> > +    qdev_connect_gpio_out_named(dev, "ged-irq", 0, pic[irq]);
> > +
> > +    object_unref(OBJECT(dev));
> > +
> > +    return dev;
> > +}
> 
> this function will need changes to accommodate for sysbus device
> init sequence [3/9].

Yes. I think we are proposing to use sysbus_mmio_map() here for "ged-base".
But what about " memhp-base"? Is it ok to invoke
acpi_memory_hotplug_init(get_system_memoty(), ...) from ged device?

Or go with _set_link() function to pass the address space ?

Thanks,
Shameer

 
> > +
> >  static void create_its(VirtMachineState *vms, DeviceState *gicdev)
> >  {
> >      const char *itsclass = its_class_name();
> > @@ -1483,6 +1508,7 @@ static void machvirt_init(MachineState *machine)
> >      MemoryRegion *ram = g_new(MemoryRegion, 1);
> >      bool firmware_loaded;
> >      bool aarch64 = true;
> > +    bool has_ged = !vmc->no_ged;
> >      unsigned int smp_cpus = machine->smp.cpus;
> >      unsigned int max_cpus = machine->smp.max_cpus;
> >
> > @@ -1697,6 +1723,10 @@ static void machvirt_init(MachineState
> *machine)
> >
> >      create_gpio(vms, pic);
> >
> > +    if (has_ged && aarch64 && firmware_loaded && acpi_enabled) {
> > +        vms->acpi_dev = create_acpi_ged(vms, pic);
> > +    }
> > +
> >      /* Create mmio transports, so the user can create virtio backends
> >       * (which will be automatically plugged in to the transports). If
> >       * no backend is created the transport will just sit harmlessly idle.
> > @@ -1876,27 +1906,34 @@ static const CPUArchIdList
> *virt_possible_cpu_arch_ids(MachineState *ms)
> >  static void virt_memory_pre_plug(HotplugHandler *hotplug_dev,
> DeviceState *dev,
> >                                   Error **errp)
> >  {
> > +    VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> > +    const bool is_nvdimm = object_dynamic_cast(OBJECT(dev),
> TYPE_NVDIMM);
> >
> > -    /*
> > -     * The device memory is not yet exposed to the Guest either through
> > -     * DT or ACPI and hence both cold/hot plug of memory is explicitly
> > -     * disabled for now.
> > -     */
> > -    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > -        error_setg(errp, "memory cold/hot plug is not yet supported");
> > +    if (is_nvdimm) {
> > +        error_setg(errp, "nvdimm is not yet supported");
> >          return;
> >      }
> >
> > +    if (!vms->acpi_dev) {
> > +        error_setg(errp, "memory hotplug is not enabled: missing acpi
> device");
> > +        return;
> > +    }
> > +
> > +    hotplug_handler_pre_plug(HOTPLUG_HANDLER(vms->acpi_dev), dev,
> errp);
> use local_error and check for error condition here. see pc_memory_pre_plug()
> 
> > +
> >      pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL,
> errp);
> >  }
> >
> >  static void virt_memory_plug(HotplugHandler *hotplug_dev,
> >                               DeviceState *dev, Error **errp)
> >  {
> > +    HotplugHandlerClass *hhc;
> >      VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> >
> >      pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), NULL);
>                                                 ^^^^
> >
> > +    hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
> > +    hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, NULL);
>                                                       ^^^^
> why errors are ignored here, pls check for errors and propagate
> them to the caller.
> 
> >  }
> >
> >  static void virt_machine_device_pre_plug_cb(HotplugHandler
> *hotplug_dev,
> > @@ -2102,8 +2139,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(4, 2)
> >
> >  static void virt_machine_4_1_options(MachineClass *mc)
> >  {
> > +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> > +
> >      virt_machine_4_2_options(mc);
> >      compat_props_add(mc->compat_props, hw_compat_4_1,
> hw_compat_4_1_len);
> > +    vmc->no_ged = true;
> >  }
> >  DEFINE_VIRT_MACHINE(4, 1)
> >
> > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > index a72094204e..577ee49b4b 100644
> > --- a/include/hw/arm/virt.h
> > +++ b/include/hw/arm/virt.h
> > @@ -77,6 +77,8 @@ enum {
> >      VIRT_GPIO,
> >      VIRT_SECURE_UART,
> >      VIRT_SECURE_MEM,
> > +    VIRT_PCDIMM_ACPI,
> > +    VIRT_ACPI_GED,
> >      VIRT_LOWMEMMAP_LAST,
> >  };
> >
> > @@ -106,6 +108,7 @@ typedef struct {
> >      bool claim_edge_triggered_timers;
> >      bool smbios_old_sys_ver;
> >      bool no_highmem_ecam;
> > +    bool no_ged;   /* Machines < 4.2 has no support for ACPI GED device
> */
> >  } VirtMachineClass;
> >
> >  typedef struct {
> > @@ -133,6 +136,7 @@ typedef struct {
> >      uint32_t iommu_phandle;
> >      int psci_conduit;
> >      hwaddr highest_gpa;
> > +    DeviceState *acpi_dev;
> >  } VirtMachineState;
> >
> >  #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM :
> VIRT_PCIE_ECAM)
> > diff --git a/tests/bios-tables-test-allowed-diff.h
> b/tests/bios-tables-test-allowed-diff.h
> > index dfb8523c8b..7b4adbc822 100644
> > --- a/tests/bios-tables-test-allowed-diff.h
> > +++ b/tests/bios-tables-test-allowed-diff.h
> > @@ -1 +1,2 @@
> >  /* List of comma-separated changed AML files to ignore */
> > +"tests/data/acpi/virt/DSDT",
Igor Mammedov Aug. 7, 2019, 9:15 a.m. UTC | #3 
On Wed, 7 Aug 2019 08:19:16 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:

> Hi Igor,
> 
> > -----Original Message-----
> > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > Sent: 06 August 2019 14:09
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> > eric.auger@redhat.com; peter.maydell@linaro.org; sameo@linux.intel.com;
> > ard.biesheuvel@linaro.org; Linuxarm <linuxarm@huawei.com>; xuwei (O)
> > <xuwei5@huawei.com>; shannon.zhaosl@gmail.com;
> > sebastien.boeuf@intel.com; lersek@redhat.com
> > Subject: Re: [Qemu-devel] [PATCH-for-4.2 v8 6/9] hw/arm/virt: Enable device
> > memory cold/hot plug with ACPI boot  
>  
> [...]
> 
> > > +static inline DeviceState *create_acpi_ged(VirtMachineState *vms,  
> > qemu_irq *pic)  
> > > +{
> > > +    DeviceState *dev;
> > > +    int irq = vms->irqmap[VIRT_ACPI_GED];
> > > +    uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT;
> > > +
> > > +    dev = DEVICE(object_new(TYPE_ACPI_GED));
> > > +    qdev_prop_set_uint64(dev, "memhp-base",
> > > +                         vms->memmap[VIRT_PCDIMM_ACPI].base);
> > > +    qdev_prop_set_uint64(dev, "ged-base",  
> > vms->memmap[VIRT_ACPI_GED].base);  
> > > +    qdev_prop_set_uint32(dev, "ged-event", event);
> > > +    object_property_add_child(qdev_get_machine(), "acpi-ged",
> > > +                              OBJECT(dev), NULL);
> > > +    qdev_init_nofail(dev);
> > > +    qdev_connect_gpio_out_named(dev, "ged-irq", 0, pic[irq]);
> > > +
> > > +    object_unref(OBJECT(dev));
> > > +
> > > +    return dev;
> > > +}  
> > 
> > this function will need changes to accommodate for sysbus device
> > init sequence [3/9].  
> 
> Yes. I think we are proposing to use sysbus_mmio_map() here for "ged-base".
> But what about " memhp-base"? Is it ok to invoke
> acpi_memory_hotplug_init(get_system_memoty(), ...) from ged device?
no it's not ok.

One could expose container memory region as sysbus mmio and then put
ged-io and AcpiGedState::memhp_state::io within it.
something like:

        board:
            sysbus_mmio_map(ged, 0 /* io_contaner number */, ged-base)     

        ged_initfn()
            register io_container as sysbus mmio region

        ged_realize()
            memory_region_add_subregion(&ged->io_container, 0, &ged_st->io);
            acpi_memory_hotplug_init(&ged->io_container,, &ged->acpi_memory_hotplug, AFTER_GED_IO_OFFSET)
    
that would make GED's MMIO available to guest at ged-base and memhp IO
will be available at address after it.
You can go even further (more flexible) and register ged_st->io as separate
sysbus mmio and use a container exclusively for memhp, in this case you'd be
able to map memhp io from board independently from ged-base.


> Or go with _set_link() function to pass the address space ?
> 
> Thanks,
> Shameer
> 
>  
> > > +
> > >  static void create_its(VirtMachineState *vms, DeviceState *gicdev)
> > >  {
> > >      const char *itsclass = its_class_name();
> > > @@ -1483,6 +1508,7 @@ static void machvirt_init(MachineState *machine)
> > >      MemoryRegion *ram = g_new(MemoryRegion, 1);
> > >      bool firmware_loaded;
> > >      bool aarch64 = true;
> > > +    bool has_ged = !vmc->no_ged;
> > >      unsigned int smp_cpus = machine->smp.cpus;
> > >      unsigned int max_cpus = machine->smp.max_cpus;
> > >
> > > @@ -1697,6 +1723,10 @@ static void machvirt_init(MachineState  
> > *machine)  
> > >
> > >      create_gpio(vms, pic);
> > >
> > > +    if (has_ged && aarch64 && firmware_loaded && acpi_enabled) {
> > > +        vms->acpi_dev = create_acpi_ged(vms, pic);
> > > +    }
> > > +
> > >      /* Create mmio transports, so the user can create virtio backends
> > >       * (which will be automatically plugged in to the transports). If
> > >       * no backend is created the transport will just sit harmlessly idle.
> > > @@ -1876,27 +1906,34 @@ static const CPUArchIdList  
> > *virt_possible_cpu_arch_ids(MachineState *ms)  
> > >  static void virt_memory_pre_plug(HotplugHandler *hotplug_dev,  
> > DeviceState *dev,  
> > >                                   Error **errp)
> > >  {
> > > +    VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> > > +    const bool is_nvdimm = object_dynamic_cast(OBJECT(dev),  
> > TYPE_NVDIMM);  
> > >
> > > -    /*
> > > -     * The device memory is not yet exposed to the Guest either through
> > > -     * DT or ACPI and hence both cold/hot plug of memory is explicitly
> > > -     * disabled for now.
> > > -     */
> > > -    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > > -        error_setg(errp, "memory cold/hot plug is not yet supported");
> > > +    if (is_nvdimm) {
> > > +        error_setg(errp, "nvdimm is not yet supported");
> > >          return;
> > >      }
> > >
> > > +    if (!vms->acpi_dev) {
> > > +        error_setg(errp, "memory hotplug is not enabled: missing acpi  
> > device");  
> > > +        return;
> > > +    }
> > > +
> > > +    hotplug_handler_pre_plug(HOTPLUG_HANDLER(vms->acpi_dev), dev,  
> > errp);
> > use local_error and check for error condition here. see pc_memory_pre_plug()
> >   
> > > +
> > >      pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL,  
> > errp);  
> > >  }
> > >
> > >  static void virt_memory_plug(HotplugHandler *hotplug_dev,
> > >                               DeviceState *dev, Error **errp)
> > >  {
> > > +    HotplugHandlerClass *hhc;
> > >      VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> > >
> > >      pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), NULL);  
> >                                                 ^^^^  
> > >
> > > +    hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
> > > +    hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, NULL);  
> >                                                       ^^^^
> > why errors are ignored here, pls check for errors and propagate
> > them to the caller.
> >   
> > >  }
> > >
> > >  static void virt_machine_device_pre_plug_cb(HotplugHandler  
> > *hotplug_dev,  
> > > @@ -2102,8 +2139,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(4, 2)
> > >
> > >  static void virt_machine_4_1_options(MachineClass *mc)
> > >  {
> > > +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> > > +
> > >      virt_machine_4_2_options(mc);
> > >      compat_props_add(mc->compat_props, hw_compat_4_1,  
> > hw_compat_4_1_len);  
> > > +    vmc->no_ged = true;
> > >  }
> > >  DEFINE_VIRT_MACHINE(4, 1)
> > >
> > > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > > index a72094204e..577ee49b4b 100644
> > > --- a/include/hw/arm/virt.h
> > > +++ b/include/hw/arm/virt.h
> > > @@ -77,6 +77,8 @@ enum {
> > >      VIRT_GPIO,
> > >      VIRT_SECURE_UART,
> > >      VIRT_SECURE_MEM,
> > > +    VIRT_PCDIMM_ACPI,
> > > +    VIRT_ACPI_GED,
> > >      VIRT_LOWMEMMAP_LAST,
> > >  };
> > >
> > > @@ -106,6 +108,7 @@ typedef struct {
> > >      bool claim_edge_triggered_timers;
> > >      bool smbios_old_sys_ver;
> > >      bool no_highmem_ecam;
> > > +    bool no_ged;   /* Machines < 4.2 has no support for ACPI GED device  
> > */  
> > >  } VirtMachineClass;
> > >
> > >  typedef struct {
> > > @@ -133,6 +136,7 @@ typedef struct {
> > >      uint32_t iommu_phandle;
> > >      int psci_conduit;
> > >      hwaddr highest_gpa;
> > > +    DeviceState *acpi_dev;
> > >  } VirtMachineState;
> > >
> > >  #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM :  
> > VIRT_PCIE_ECAM)  
> > > diff --git a/tests/bios-tables-test-allowed-diff.h  
> > b/tests/bios-tables-test-allowed-diff.h  
> > > index dfb8523c8b..7b4adbc822 100644
> > > --- a/tests/bios-tables-test-allowed-diff.h
> > > +++ b/tests/bios-tables-test-allowed-diff.h
> > > @@ -1 +1,2 @@
> > >  /* List of comma-separated changed AML files to ignore */
> > > +"tests/data/acpi/virt/DSDT",  
>
Shameerali Kolothum Thodi Aug. 7, 2019, 2 p.m. UTC | #4 
> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: 07 August 2019 10:15
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> eric.auger@redhat.com; peter.maydell@linaro.org; sameo@linux.intel.com;
> ard.biesheuvel@linaro.org; Linuxarm <linuxarm@huawei.com>; xuwei (O)
> <xuwei5@huawei.com>; shannon.zhaosl@gmail.com;
> sebastien.boeuf@intel.com; lersek@redhat.com
> Subject: Re: [Qemu-devel] [PATCH-for-4.2 v8 6/9] hw/arm/virt: Enable device
> memory cold/hot plug with ACPI boot
> 
> On Wed, 7 Aug 2019 08:19:16 +0000
> Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
> 
> > Hi Igor,
> >
> > > -----Original Message-----
> > > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > > Sent: 06 August 2019 14:09
> > > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> > > eric.auger@redhat.com; peter.maydell@linaro.org;
> sameo@linux.intel.com;
> > > ard.biesheuvel@linaro.org; Linuxarm <linuxarm@huawei.com>; xuwei (O)
> > > <xuwei5@huawei.com>; shannon.zhaosl@gmail.com;
> > > sebastien.boeuf@intel.com; lersek@redhat.com
> > > Subject: Re: [Qemu-devel] [PATCH-for-4.2 v8 6/9] hw/arm/virt: Enable
> device
> > > memory cold/hot plug with ACPI boot
> >
> > [...]
> >
> > > > +static inline DeviceState *create_acpi_ged(VirtMachineState *vms,
> > > qemu_irq *pic)
> > > > +{
> > > > +    DeviceState *dev;
> > > > +    int irq = vms->irqmap[VIRT_ACPI_GED];
> > > > +    uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT;
> > > > +
> > > > +    dev = DEVICE(object_new(TYPE_ACPI_GED));
> > > > +    qdev_prop_set_uint64(dev, "memhp-base",
> > > > +
> vms->memmap[VIRT_PCDIMM_ACPI].base);
> > > > +    qdev_prop_set_uint64(dev, "ged-base",
> > > vms->memmap[VIRT_ACPI_GED].base);
> > > > +    qdev_prop_set_uint32(dev, "ged-event", event);
> > > > +    object_property_add_child(qdev_get_machine(), "acpi-ged",
> > > > +                              OBJECT(dev), NULL);
> > > > +    qdev_init_nofail(dev);
> > > > +    qdev_connect_gpio_out_named(dev, "ged-irq", 0, pic[irq]);
> > > > +
> > > > +    object_unref(OBJECT(dev));
> > > > +
> > > > +    return dev;
> > > > +}
> > >
> > > this function will need changes to accommodate for sysbus device
> > > init sequence [3/9].
> >
> > Yes. I think we are proposing to use sysbus_mmio_map() here for "ged-base".
> > But what about " memhp-base"? Is it ok to invoke
> > acpi_memory_hotplug_init(get_system_memoty(), ...) from ged device?
> no it's not ok.
> 
> One could expose container memory region as sysbus mmio and then put
> ged-io and AcpiGedState::memhp_state::io within it.
> something like:
> 
>         board:
>             sysbus_mmio_map(ged, 0 /* io_contaner number */, ged-base)
> 
>         ged_initfn()
>             register io_container as sysbus mmio region
> 
>         ged_realize()
>             memory_region_add_subregion(&ged->io_container, 0,
> &ged_st->io);
>             acpi_memory_hotplug_init(&ged->io_container,,
> &ged->acpi_memory_hotplug, AFTER_GED_IO_OFFSET)
> 
> that would make GED's MMIO available to guest at ged-base and memhp IO
> will be available at address after it.
> You can go even further (more flexible) and register ged_st->io as separate
> sysbus mmio and use a container exclusively for memhp, in this case you'd be
> able to map memhp io from board independently from ged-base.

Ok. Understood. Thanks.

But looks like both the approaches would require changes to build_memory_hotplug_aml()
code as acpi_memory_hotplug_init() stores the io_base and reuse that in _aml() code.

I will have a go and see.

Thanks,
Shameer

> 
> > Or go with _set_link() function to pass the address space ?
> >
> > Thanks,
> > Shameer
> >
> >
> > > > +
> > > >  static void create_its(VirtMachineState *vms, DeviceState *gicdev)
> > > >  {
> > > >      const char *itsclass = its_class_name();
> > > > @@ -1483,6 +1508,7 @@ static void machvirt_init(MachineState
> *machine)
> > > >      MemoryRegion *ram = g_new(MemoryRegion, 1);
> > > >      bool firmware_loaded;
> > > >      bool aarch64 = true;
> > > > +    bool has_ged = !vmc->no_ged;
> > > >      unsigned int smp_cpus = machine->smp.cpus;
> > > >      unsigned int max_cpus = machine->smp.max_cpus;
> > > >
> > > > @@ -1697,6 +1723,10 @@ static void machvirt_init(MachineState
> > > *machine)
> > > >
> > > >      create_gpio(vms, pic);
> > > >
> > > > +    if (has_ged && aarch64 && firmware_loaded && acpi_enabled) {
> > > > +        vms->acpi_dev = create_acpi_ged(vms, pic);
> > > > +    }
> > > > +
> > > >      /* Create mmio transports, so the user can create virtio backends
> > > >       * (which will be automatically plugged in to the transports). If
> > > >       * no backend is created the transport will just sit harmlessly idle.
> > > > @@ -1876,27 +1906,34 @@ static const CPUArchIdList
> > > *virt_possible_cpu_arch_ids(MachineState *ms)
> > > >  static void virt_memory_pre_plug(HotplugHandler *hotplug_dev,
> > > DeviceState *dev,
> > > >                                   Error **errp)
> > > >  {
> > > > +    VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> > > > +    const bool is_nvdimm = object_dynamic_cast(OBJECT(dev),
> > > TYPE_NVDIMM);
> > > >
> > > > -    /*
> > > > -     * The device memory is not yet exposed to the Guest either
> through
> > > > -     * DT or ACPI and hence both cold/hot plug of memory is explicitly
> > > > -     * disabled for now.
> > > > -     */
> > > > -    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > > > -        error_setg(errp, "memory cold/hot plug is not yet supported");
> > > > +    if (is_nvdimm) {
> > > > +        error_setg(errp, "nvdimm is not yet supported");
> > > >          return;
> > > >      }
> > > >
> > > > +    if (!vms->acpi_dev) {
> > > > +        error_setg(errp, "memory hotplug is not enabled: missing acpi
> > > device");
> > > > +        return;
> > > > +    }
> > > > +
> > > > +    hotplug_handler_pre_plug(HOTPLUG_HANDLER(vms->acpi_dev),
> dev,
> > > errp);
> > > use local_error and check for error condition here. see
> pc_memory_pre_plug()
> > >
> > > > +
> > > >      pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev),
> NULL,
> > > errp);
> > > >  }
> > > >
> > > >  static void virt_memory_plug(HotplugHandler *hotplug_dev,
> > > >                               DeviceState *dev, Error **errp)
> > > >  {
> > > > +    HotplugHandlerClass *hhc;
> > > >      VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> > > >
> > > >      pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), NULL);
> > >                                                 ^^^^
> > > >
> > > > +    hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
> > > > +    hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, NULL);
> > >                                                       ^^^^
> > > why errors are ignored here, pls check for errors and propagate
> > > them to the caller.
> > >
> > > >  }
> > > >
> > > >  static void virt_machine_device_pre_plug_cb(HotplugHandler
> > > *hotplug_dev,
> > > > @@ -2102,8 +2139,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(4, 2)
> > > >
> > > >  static void virt_machine_4_1_options(MachineClass *mc)
> > > >  {
> > > > +    VirtMachineClass *vmc =
> VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> > > > +
> > > >      virt_machine_4_2_options(mc);
> > > >      compat_props_add(mc->compat_props, hw_compat_4_1,
> > > hw_compat_4_1_len);
> > > > +    vmc->no_ged = true;
> > > >  }
> > > >  DEFINE_VIRT_MACHINE(4, 1)
> > > >
> > > > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > > > index a72094204e..577ee49b4b 100644
> > > > --- a/include/hw/arm/virt.h
> > > > +++ b/include/hw/arm/virt.h
> > > > @@ -77,6 +77,8 @@ enum {
> > > >      VIRT_GPIO,
> > > >      VIRT_SECURE_UART,
> > > >      VIRT_SECURE_MEM,
> > > > +    VIRT_PCDIMM_ACPI,
> > > > +    VIRT_ACPI_GED,
> > > >      VIRT_LOWMEMMAP_LAST,
> > > >  };
> > > >
> > > > @@ -106,6 +108,7 @@ typedef struct {
> > > >      bool claim_edge_triggered_timers;
> > > >      bool smbios_old_sys_ver;
> > > >      bool no_highmem_ecam;
> > > > +    bool no_ged;   /* Machines < 4.2 has no support for ACPI GED
> device
> > > */
> > > >  } VirtMachineClass;
> > > >
> > > >  typedef struct {
> > > > @@ -133,6 +136,7 @@ typedef struct {
> > > >      uint32_t iommu_phandle;
> > > >      int psci_conduit;
> > > >      hwaddr highest_gpa;
> > > > +    DeviceState *acpi_dev;
> > > >  } VirtMachineState;
> > > >
> > > >  #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM :
> > > VIRT_PCIE_ECAM)
> > > > diff --git a/tests/bios-tables-test-allowed-diff.h
> > > b/tests/bios-tables-test-allowed-diff.h
> > > > index dfb8523c8b..7b4adbc822 100644
> > > > --- a/tests/bios-tables-test-allowed-diff.h
> > > > +++ b/tests/bios-tables-test-allowed-diff.h
> > > > @@ -1 +1,2 @@
> > > >  /* List of comma-separated changed AML files to ignore */
> > > > +"tests/data/acpi/virt/DSDT",
> >
Igor Mammedov Aug. 7, 2019, 3:04 p.m. UTC | #5 
On Wed, 7 Aug 2019 14:00:44 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:

> > -----Original Message-----
> > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > Sent: 07 August 2019 10:15
> > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> > eric.auger@redhat.com; peter.maydell@linaro.org; sameo@linux.intel.com;
> > ard.biesheuvel@linaro.org; Linuxarm <linuxarm@huawei.com>; xuwei (O)
> > <xuwei5@huawei.com>; shannon.zhaosl@gmail.com;
> > sebastien.boeuf@intel.com; lersek@redhat.com
> > Subject: Re: [Qemu-devel] [PATCH-for-4.2 v8 6/9] hw/arm/virt: Enable device
> > memory cold/hot plug with ACPI boot
> > 
> > On Wed, 7 Aug 2019 08:19:16 +0000
> > Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
> >   
> > > Hi Igor,
> > >  
> > > > -----Original Message-----
> > > > From: Igor Mammedov [mailto:imammedo@redhat.com]
> > > > Sent: 06 August 2019 14:09
> > > > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> > > > Cc: qemu-devel@nongnu.org; qemu-arm@nongnu.org;
> > > > eric.auger@redhat.com; peter.maydell@linaro.org;  
> > sameo@linux.intel.com;  
> > > > ard.biesheuvel@linaro.org; Linuxarm <linuxarm@huawei.com>; xuwei (O)
> > > > <xuwei5@huawei.com>; shannon.zhaosl@gmail.com;
> > > > sebastien.boeuf@intel.com; lersek@redhat.com
> > > > Subject: Re: [Qemu-devel] [PATCH-for-4.2 v8 6/9] hw/arm/virt: Enable  
> > device  
> > > > memory cold/hot plug with ACPI boot  
> > >
> > > [...]
> > >  
> > > > > +static inline DeviceState *create_acpi_ged(VirtMachineState *vms,  
> > > > qemu_irq *pic)  
> > > > > +{
> > > > > +    DeviceState *dev;
> > > > > +    int irq = vms->irqmap[VIRT_ACPI_GED];
> > > > > +    uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT;
> > > > > +
> > > > > +    dev = DEVICE(object_new(TYPE_ACPI_GED));
> > > > > +    qdev_prop_set_uint64(dev, "memhp-base",
> > > > > +  
> > vms->memmap[VIRT_PCDIMM_ACPI].base);  
> > > > > +    qdev_prop_set_uint64(dev, "ged-base",  
> > > > vms->memmap[VIRT_ACPI_GED].base);  
> > > > > +    qdev_prop_set_uint32(dev, "ged-event", event);
> > > > > +    object_property_add_child(qdev_get_machine(), "acpi-ged",
> > > > > +                              OBJECT(dev), NULL);
> > > > > +    qdev_init_nofail(dev);
> > > > > +    qdev_connect_gpio_out_named(dev, "ged-irq", 0, pic[irq]);
> > > > > +
> > > > > +    object_unref(OBJECT(dev));
> > > > > +
> > > > > +    return dev;
> > > > > +}  
> > > >
> > > > this function will need changes to accommodate for sysbus device
> > > > init sequence [3/9].  
> > >
> > > Yes. I think we are proposing to use sysbus_mmio_map() here for "ged-base".
> > > But what about " memhp-base"? Is it ok to invoke
> > > acpi_memory_hotplug_init(get_system_memoty(), ...) from ged device?  
> > no it's not ok.
> > 
> > One could expose container memory region as sysbus mmio and then put
> > ged-io and AcpiGedState::memhp_state::io within it.
> > something like:
> > 
> >         board:
> >             sysbus_mmio_map(ged, 0 /* io_contaner number */, ged-base)
> > 
> >         ged_initfn()
> >             register io_container as sysbus mmio region
> > 
> >         ged_realize()
> >             memory_region_add_subregion(&ged->io_container, 0,
> > &ged_st->io);
> >             acpi_memory_hotplug_init(&ged->io_container,,
> > &ged->acpi_memory_hotplug, AFTER_GED_IO_OFFSET)
> > 
> > that would make GED's MMIO available to guest at ged-base and memhp IO
> > will be available at address after it.
> > You can go even further (more flexible) and register ged_st->io as separate
> > sysbus mmio and use a container exclusively for memhp, in this case you'd be
> > able to map memhp io from board independently from ged-base.  
> 
> Ok. Understood. Thanks.
> 
> But looks like both the approaches would require changes to build_memory_hotplug_aml()
> code as acpi_memory_hotplug_init() stores the io_base and reuse that in _aml() code.
I'd just pass io_base as an argument to build_memory_hotplug_aml().
For x86 we can add a field to PCMachine to provide address for pc or q35
and just use constant in arm/virt case.


> 
> I will have a go and see.
> 
> Thanks,
> Shameer
> 
> >   
> > > Or go with _set_link() function to pass the address space ?
> > >
> > > Thanks,
> > > Shameer
> > >
> > >  
> > > > > +
> > > > >  static void create_its(VirtMachineState *vms, DeviceState *gicdev)
> > > > >  {
> > > > >      const char *itsclass = its_class_name();
> > > > > @@ -1483,6 +1508,7 @@ static void machvirt_init(MachineState  
> > *machine)  
> > > > >      MemoryRegion *ram = g_new(MemoryRegion, 1);
> > > > >      bool firmware_loaded;
> > > > >      bool aarch64 = true;
> > > > > +    bool has_ged = !vmc->no_ged;
> > > > >      unsigned int smp_cpus = machine->smp.cpus;
> > > > >      unsigned int max_cpus = machine->smp.max_cpus;
> > > > >
> > > > > @@ -1697,6 +1723,10 @@ static void machvirt_init(MachineState  
> > > > *machine)  
> > > > >
> > > > >      create_gpio(vms, pic);
> > > > >
> > > > > +    if (has_ged && aarch64 && firmware_loaded && acpi_enabled) {
> > > > > +        vms->acpi_dev = create_acpi_ged(vms, pic);
> > > > > +    }
> > > > > +
> > > > >      /* Create mmio transports, so the user can create virtio backends
> > > > >       * (which will be automatically plugged in to the transports). If
> > > > >       * no backend is created the transport will just sit harmlessly idle.
> > > > > @@ -1876,27 +1906,34 @@ static const CPUArchIdList  
> > > > *virt_possible_cpu_arch_ids(MachineState *ms)  
> > > > >  static void virt_memory_pre_plug(HotplugHandler *hotplug_dev,  
> > > > DeviceState *dev,  
> > > > >                                   Error **errp)
> > > > >  {
> > > > > +    VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> > > > > +    const bool is_nvdimm = object_dynamic_cast(OBJECT(dev),  
> > > > TYPE_NVDIMM);  
> > > > >
> > > > > -    /*
> > > > > -     * The device memory is not yet exposed to the Guest either  
> > through  
> > > > > -     * DT or ACPI and hence both cold/hot plug of memory is explicitly
> > > > > -     * disabled for now.
> > > > > -     */
> > > > > -    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
> > > > > -        error_setg(errp, "memory cold/hot plug is not yet supported");
> > > > > +    if (is_nvdimm) {
> > > > > +        error_setg(errp, "nvdimm is not yet supported");
> > > > >          return;
> > > > >      }
> > > > >
> > > > > +    if (!vms->acpi_dev) {
> > > > > +        error_setg(errp, "memory hotplug is not enabled: missing acpi  
> > > > device");  
> > > > > +        return;
> > > > > +    }
> > > > > +
> > > > > +    hotplug_handler_pre_plug(HOTPLUG_HANDLER(vms->acpi_dev),  
> > dev,  
> > > > errp);
> > > > use local_error and check for error condition here. see  
> > pc_memory_pre_plug()  
> > > >  
> > > > > +
> > > > >      pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev),  
> > NULL,  
> > > > errp);  
> > > > >  }
> > > > >
> > > > >  static void virt_memory_plug(HotplugHandler *hotplug_dev,
> > > > >                               DeviceState *dev, Error **errp)
> > > > >  {
> > > > > +    HotplugHandlerClass *hhc;
> > > > >      VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
> > > > >
> > > > >      pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), NULL);  
> > > >                                                 ^^^^  
> > > > >
> > > > > +    hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
> > > > > +    hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, NULL);  
> > > >                                                       ^^^^
> > > > why errors are ignored here, pls check for errors and propagate
> > > > them to the caller.
> > > >  
> > > > >  }
> > > > >
> > > > >  static void virt_machine_device_pre_plug_cb(HotplugHandler  
> > > > *hotplug_dev,  
> > > > > @@ -2102,8 +2139,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(4, 2)
> > > > >
> > > > >  static void virt_machine_4_1_options(MachineClass *mc)
> > > > >  {
> > > > > +    VirtMachineClass *vmc =  
> > VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));  
> > > > > +
> > > > >      virt_machine_4_2_options(mc);
> > > > >      compat_props_add(mc->compat_props, hw_compat_4_1,  
> > > > hw_compat_4_1_len);  
> > > > > +    vmc->no_ged = true;
> > > > >  }
> > > > >  DEFINE_VIRT_MACHINE(4, 1)
> > > > >
> > > > > diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> > > > > index a72094204e..577ee49b4b 100644
> > > > > --- a/include/hw/arm/virt.h
> > > > > +++ b/include/hw/arm/virt.h
> > > > > @@ -77,6 +77,8 @@ enum {
> > > > >      VIRT_GPIO,
> > > > >      VIRT_SECURE_UART,
> > > > >      VIRT_SECURE_MEM,
> > > > > +    VIRT_PCDIMM_ACPI,
> > > > > +    VIRT_ACPI_GED,
> > > > >      VIRT_LOWMEMMAP_LAST,
> > > > >  };
> > > > >
> > > > > @@ -106,6 +108,7 @@ typedef struct {
> > > > >      bool claim_edge_triggered_timers;
> > > > >      bool smbios_old_sys_ver;
> > > > >      bool no_highmem_ecam;
> > > > > +    bool no_ged;   /* Machines < 4.2 has no support for ACPI GED  
> > device  
> > > > */  
> > > > >  } VirtMachineClass;
> > > > >
> > > > >  typedef struct {
> > > > > @@ -133,6 +136,7 @@ typedef struct {
> > > > >      uint32_t iommu_phandle;
> > > > >      int psci_conduit;
> > > > >      hwaddr highest_gpa;
> > > > > +    DeviceState *acpi_dev;
> > > > >  } VirtMachineState;
> > > > >
> > > > >  #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM :  
> > > > VIRT_PCIE_ECAM)  
> > > > > diff --git a/tests/bios-tables-test-allowed-diff.h  
> > > > b/tests/bios-tables-test-allowed-diff.h  
> > > > > index dfb8523c8b..7b4adbc822 100644
> > > > > --- a/tests/bios-tables-test-allowed-diff.h
> > > > > +++ b/tests/bios-tables-test-allowed-diff.h
> > > > > @@ -1 +1,2 @@
> > > > >  /* List of comma-separated changed AML files to ignore */
> > > > > +"tests/data/acpi/virt/DSDT",  
> > >  
>
diff mbox series

Patch

diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
index 84961c17ab..ad7f7c089b 100644
--- a/hw/arm/Kconfig
+++ b/hw/arm/Kconfig
@@ -22,6 +22,8 @@  config ARM_VIRT
     select ACPI_PCI
     select MEM_DEVICE
     select DIMM
+    select ACPI_MEMORY_HOTPLUG
+    select ACPI_HW_REDUCED
 
 config CHEETAH
     bool
diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 0afb372769..018b1e326d 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -40,6 +40,8 @@ 
 #include "hw/acpi/aml-build.h"
 #include "hw/acpi/utils.h"
 #include "hw/acpi/pci.h"
+#include "hw/acpi/memory_hotplug.h"
+#include "hw/acpi/generic_event_device.h"
 #include "hw/pci/pcie_host.h"
 #include "hw/pci/pci.h"
 #include "hw/arm/virt.h"
@@ -705,6 +707,7 @@  static void
 build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
 {
     Aml *scope, *dsdt;
+    MachineState *ms = MACHINE(vms);
     const MemMapEntry *memmap = vms->memmap;
     const int *irqmap = vms->irqmap;
 
@@ -729,6 +732,17 @@  build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
                       vms->highmem, vms->highmem_ecam);
     acpi_dsdt_add_gpio(scope, &memmap[VIRT_GPIO],
                        (irqmap[VIRT_GPIO] + ARM_SPI_BASE));
+    if (vms->acpi_dev) {
+        build_ged_aml(scope, "\\_SB."GED_DEVICE,
+                      HOTPLUG_HANDLER(vms->acpi_dev),
+                      irqmap[VIRT_ACPI_GED] + ARM_SPI_BASE, AML_SYSTEM_MEMORY);
+    }
+
+    if (vms->acpi_dev && ms->ram_slots) {
+        build_memory_hotplug_aml(scope, ms->ram_slots, "\\_SB", NULL,
+                                 AML_SYSTEM_MEMORY);
+    }
+
     acpi_dsdt_add_power_button(scope);
 
     aml_append(dsdt, scope);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index bbe156dc35..41386a6eb7 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -66,6 +66,7 @@ 
 #include "target/arm/internals.h"
 #include "hw/mem/pc-dimm.h"
 #include "hw/mem/nvdimm.h"
+#include "hw/acpi/generic_event_device.h"
 
 #define DEFINE_VIRT_MACHINE_LATEST(major, minor, latest) \
     static void virt_##major##_##minor##_class_init(ObjectClass *oc, \
@@ -136,6 +137,8 @@  static const MemMapEntry base_memmap[] = {
     [VIRT_GPIO] =               { 0x09030000, 0x00001000 },
     [VIRT_SECURE_UART] =        { 0x09040000, 0x00001000 },
     [VIRT_SMMU] =               { 0x09050000, 0x00020000 },
+    [VIRT_PCDIMM_ACPI] =        { 0x09070000, MEMORY_HOTPLUG_IO_LEN },
+    [VIRT_ACPI_GED] =           { 0x09080000, ACPI_GED_EVT_SEL_LEN },
     [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
@@ -171,6 +174,7 @@  static const int a15irqmap[] = {
     [VIRT_PCIE] = 3, /* ... to 6 */
     [VIRT_GPIO] = 7,
     [VIRT_SECURE_UART] = 8,
+    [VIRT_ACPI_GED] = 9,
     [VIRT_MMIO] = 16, /* ...to 16 + NUM_VIRTIO_TRANSPORTS - 1 */
     [VIRT_GIC_V2M] = 48, /* ...to 48 + NUM_GICV2M_SPIS - 1 */
     [VIRT_SMMU] = 74,    /* ...to 74 + NUM_SMMU_IRQS - 1 */
@@ -520,6 +524,27 @@  static void fdt_add_pmu_nodes(const VirtMachineState *vms)
     }
 }
 
+static inline DeviceState *create_acpi_ged(VirtMachineState *vms, qemu_irq *pic)
+{
+    DeviceState *dev;
+    int irq = vms->irqmap[VIRT_ACPI_GED];
+    uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT;
+
+    dev = DEVICE(object_new(TYPE_ACPI_GED));
+    qdev_prop_set_uint64(dev, "memhp-base",
+                         vms->memmap[VIRT_PCDIMM_ACPI].base);
+    qdev_prop_set_uint64(dev, "ged-base", vms->memmap[VIRT_ACPI_GED].base);
+    qdev_prop_set_uint32(dev, "ged-event", event);
+    object_property_add_child(qdev_get_machine(), "acpi-ged",
+                              OBJECT(dev), NULL);
+    qdev_init_nofail(dev);
+    qdev_connect_gpio_out_named(dev, "ged-irq", 0, pic[irq]);
+
+    object_unref(OBJECT(dev));
+
+    return dev;
+}
+
 static void create_its(VirtMachineState *vms, DeviceState *gicdev)
 {
     const char *itsclass = its_class_name();
@@ -1483,6 +1508,7 @@  static void machvirt_init(MachineState *machine)
     MemoryRegion *ram = g_new(MemoryRegion, 1);
     bool firmware_loaded;
     bool aarch64 = true;
+    bool has_ged = !vmc->no_ged;
     unsigned int smp_cpus = machine->smp.cpus;
     unsigned int max_cpus = machine->smp.max_cpus;
 
@@ -1697,6 +1723,10 @@  static void machvirt_init(MachineState *machine)
 
     create_gpio(vms, pic);
 
+    if (has_ged && aarch64 && firmware_loaded && acpi_enabled) {
+        vms->acpi_dev = create_acpi_ged(vms, pic);
+    }
+
     /* Create mmio transports, so the user can create virtio backends
      * (which will be automatically plugged in to the transports). If
      * no backend is created the transport will just sit harmlessly idle.
@@ -1876,27 +1906,34 @@  static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
 static void virt_memory_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                                  Error **errp)
 {
+    VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
+    const bool is_nvdimm = object_dynamic_cast(OBJECT(dev), TYPE_NVDIMM);
 
-    /*
-     * The device memory is not yet exposed to the Guest either through
-     * DT or ACPI and hence both cold/hot plug of memory is explicitly
-     * disabled for now.
-     */
-    if (object_dynamic_cast(OBJECT(dev), TYPE_PC_DIMM)) {
-        error_setg(errp, "memory cold/hot plug is not yet supported");
+    if (is_nvdimm) {
+        error_setg(errp, "nvdimm is not yet supported");
         return;
     }
 
+    if (!vms->acpi_dev) {
+        error_setg(errp, "memory hotplug is not enabled: missing acpi device");
+        return;
+    }
+
+    hotplug_handler_pre_plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, errp);
+
     pc_dimm_pre_plug(PC_DIMM(dev), MACHINE(hotplug_dev), NULL, errp);
 }
 
 static void virt_memory_plug(HotplugHandler *hotplug_dev,
                              DeviceState *dev, Error **errp)
 {
+    HotplugHandlerClass *hhc;
     VirtMachineState *vms = VIRT_MACHINE(hotplug_dev);
 
     pc_dimm_plug(PC_DIMM(dev), MACHINE(vms), NULL);
 
+    hhc = HOTPLUG_HANDLER_GET_CLASS(vms->acpi_dev);
+    hhc->plug(HOTPLUG_HANDLER(vms->acpi_dev), dev, NULL);
 }
 
 static void virt_machine_device_pre_plug_cb(HotplugHandler *hotplug_dev,
@@ -2102,8 +2139,11 @@  DEFINE_VIRT_MACHINE_AS_LATEST(4, 2)
 
 static void virt_machine_4_1_options(MachineClass *mc)
 {
+    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
+
     virt_machine_4_2_options(mc);
     compat_props_add(mc->compat_props, hw_compat_4_1, hw_compat_4_1_len);
+    vmc->no_ged = true;
 }
 DEFINE_VIRT_MACHINE(4, 1)
 
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index a72094204e..577ee49b4b 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -77,6 +77,8 @@  enum {
     VIRT_GPIO,
     VIRT_SECURE_UART,
     VIRT_SECURE_MEM,
+    VIRT_PCDIMM_ACPI,
+    VIRT_ACPI_GED,
     VIRT_LOWMEMMAP_LAST,
 };
 
@@ -106,6 +108,7 @@  typedef struct {
     bool claim_edge_triggered_timers;
     bool smbios_old_sys_ver;
     bool no_highmem_ecam;
+    bool no_ged;   /* Machines < 4.2 has no support for ACPI GED device */
 } VirtMachineClass;
 
 typedef struct {
@@ -133,6 +136,7 @@  typedef struct {
     uint32_t iommu_phandle;
     int psci_conduit;
     hwaddr highest_gpa;
+    DeviceState *acpi_dev;
 } VirtMachineState;
 
 #define VIRT_ECAM_ID(high) (high ? VIRT_HIGH_PCIE_ECAM : VIRT_PCIE_ECAM)
diff --git a/tests/bios-tables-test-allowed-diff.h b/tests/bios-tables-test-allowed-diff.h
index dfb8523c8b..7b4adbc822 100644
--- a/tests/bios-tables-test-allowed-diff.h
+++ b/tests/bios-tables-test-allowed-diff.h
@@ -1 +1,2 @@ 
 /* List of comma-separated changed AML files to ignore */
+"tests/data/acpi/virt/DSDT",