[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-arm] [PATCH-for-4.2 v10 05/11] hw/arm/virt: Enable device memo
From: |
Igor Mammedov |
Subject: |
Re: [Qemu-arm] [PATCH-for-4.2 v10 05/11] hw/arm/virt: Enable device memory cold/hot plug with ACPI boot |
Date: |
Tue, 17 Sep 2019 17:25:45 +0200 |
On Mon, 16 Sep 2019 10:30:45 +0000
Shameerali Kolothum Thodi <address@hidden> wrote:
> 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);
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;
> > > +}
> > > +
> > [...]
- [Qemu-arm] [PATCH-for-4.2 v10 02/11] hw/acpi: Do not create memory hotplug method when handler is not defined, (continued)
- [Qemu-arm] [PATCH-for-4.2 v10 02/11] hw/acpi: Do not create memory hotplug method when handler is not defined, Shameer Kolothum, 2019/09/04
- [Qemu-arm] [PATCH-for-4.2 v10 03/11] hw/acpi: Add ACPI Generic Event Device Support, Shameer Kolothum, 2019/09/04
- [Qemu-arm] [PATCH-for-4.2 v10 04/11] hw/arm/virt: Add memory hotplug framework, Shameer Kolothum, 2019/09/04
- [Qemu-arm] [PATCH-for-4.2 v10 06/11] hw/arm/virt-acpi-build: Add PC-DIMM in SRAT, Shameer Kolothum, 2019/09/04
- [Qemu-arm] [PATCH-for-4.2 v10 05/11] hw/arm/virt: Enable device memory cold/hot plug with ACPI boot, Shameer Kolothum, 2019/09/04
- [Qemu-arm] [PATCH-for-4.2 v10 07/11] hw/arm: Factor out powerdown notifier from GPIO, Shameer Kolothum, 2019/09/04
- [Qemu-arm] [PATCH-for-4.2 v10 08/11] hw/arm: Use GED for system_powerdown event, Shameer Kolothum, 2019/09/04
- [Qemu-arm] [PATCH-for-4.2 v10 09/11] docs/specs: Add ACPI GED documentation, Shameer Kolothum, 2019/09/04
- [Qemu-arm] [PATCH-for-4.2 v10 10/11] tests: add dummy ACPI tables for arm/virt board, Shameer Kolothum, 2019/09/04