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

[Shameerali Kolothum Thodi]

Hi Igor,

> -----Original Message-----
> From: Igor Mammedov [mailto:address@hidden]
> Sent: 11 September 2019 14:07
> To: Shameerali Kolothum Thodi <address@hidden>
> Cc: address@hidden; address@hidden;
> address@hidden; address@hidden;
> address@hidden; address@hidden;
> address@hidden; xuwei (O) <address@hidden>;
> address@hidden; address@hidden; Linuxarm
> <address@hidden>
> 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 <address@hidden> 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;
> > +}
> > +
> [...]