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

[Igor Mammedov]

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.

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

[...]
> +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;
> +}
> +
[...]