Re: [Qemu-devel] [PATCH-for-4.2 v10 05/11] hw/arm/virt: Enable device memory cold/hot plug with ACPI boot

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

Hi Igor,

> -----Original Message-----
> From: Igor Mammedov [mailto:imammedo@redhat.com]
> Sent: 11 September 2019 14:07
> 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;
> shannon.zhaosl@gmail.com; sameo@linux.intel.com;
> sebastien.boeuf@intel.com; xuwei (O) <xuwei5@huawei.com>;
> lersek@redhat.com; ard.biesheuvel@linaro.org; Linuxarm
> <linuxarm@huawei.com>
> Subject: Re: [PATCH-for-4.2 v10 05/11] hw/arm/virt: Enable device memory
> cold/hot plug with ACPI boot
> 
> On Wed, 4 Sep 2019 09:56:23 +0100
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> 
> [...]
> > @@ -730,6 +733,19 @@ 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,
> > +                      memmap[VIRT_ACPI_GED].base);
> > +    }
> > +
> > +    if (vms->acpi_dev && ms->ram_slots) {
> > +        build_memory_hotplug_aml(scope, ms->ram_slots, "\\_SB",
> NULL,
> > +                                 AML_SYSTEM_MEMORY,
> > +
> memmap[VIRT_PCDIMM_ACPI].base);
> > +    }
> one more thing (though non critical), if ms->ram_slots == 0 ^^^^
> makes IASL spew a warning
> 
>     External (_SB_.MHPC.MSCN, MethodObj)    // Warning: Unknown
> method, guessing 0 arguments
> 
> In general non-existing references within methods are fine if they aren't ever
> used.
> however we can be a little bit less sloppy.
> Below you advertise "event = ACPI_GED_MEM_HOTPLUG_EVT", and then here
> suddenly
> don't generate essential AML part for it here.

Ok.

> For consistency if above is conditioned on ms->ram_slots != 0, probably
> it would be better to move that condition where you set 'event' value and
> check property value above instead of ms->ram_slots

I understand the concern here, but not sure I get the suggestion to check 
the "property" instead of ms->ram_slots correctly. 

Is this what you have in mind?

diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
index 538b3bbefa..5c9269dca1 100644
--- a/hw/arm/virt-acpi-build.c
+++ b/hw/arm/virt-acpi-build.c
@@ -742,10 +742,15 @@ build_dsdt(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
                       memmap[VIRT_ACPI_GED].base);
     }
 
-    if (vms->acpi_dev && ms->ram_slots) {
-        build_memory_hotplug_aml(scope, ms->ram_slots, "\\_SB", NULL,
-                                 AML_SYSTEM_MEMORY,
-                                 memmap[VIRT_PCDIMM_ACPI].base);
+    if (vms->acpi_dev) {
+        uint32_t event = object_property_get_uint(OBJECT(vms->acpi_dev),
+                                                  "ged-event", NULL);
+
+        if (event & ACPI_GED_MEM_HOTPLUG_EVT) {
+            build_memory_hotplug_aml(scope, ms->ram_slots, "\\_SB", NULL,
+                                     AML_SYSTEM_MEMORY,
+                                     memmap[VIRT_PCDIMM_ACPI].base);
+        }
     }
 
     acpi_dsdt_add_power_button(scope);
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index bc152ea2b0..6b024b16df 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -534,8 +534,13 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms)
 static inline DeviceState *create_acpi_ged(VirtMachineState *vms, qemu_irq *pic)
 {
     DeviceState *dev;
+    MachineState *ms = MACHINE(vms);
     int irq = vms->irqmap[VIRT_ACPI_GED];
-    uint32_t event = ACPI_GED_MEM_HOTPLUG_EVT;
+    uint32_t event = 0;
+
+    if (ms->ram_slots) {
+        event = ACPI_GED_MEM_HOTPLUG_EVT;
+    }
 
     dev = qdev_create(NULL, TYPE_ACPI_GED);
     qdev_prop_set_uint32(dev, "ged-event", event);

---8---

Please let me know.

Thanks,
Shameer

 
> [...]
> > +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 = qdev_create(NULL, TYPE_ACPI_GED);
> > +    qdev_prop_set_uint32(dev, "ged-event", event);
> > +    qdev_init_nofail(dev);
> > +
> > +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0,
> vms->memmap[VIRT_ACPI_GED].base);
> > +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1,
> vms->memmap[VIRT_PCDIMM_ACPI].base);
> > +
> > +    sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic[irq]);
> > +
> > +    return dev;
> > +}
> > +
> [...]