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

[Igor Mammedov <imammedo@redhat.com>]

On Mon, 16 Sep 2019 10:30:45 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:

> 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);
s/NULL/error_abort/

> +
> +        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.
Looks good to me.

Thanks!


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