Re: [Qemu-devel] [PATCH v3 07/10] hw/arm/virt: Introduce opt-in feature "fdt"

[Shameerali Kolothum Thodi]

Hi Laszlo,

> -----Original Message-----
> From: Laszlo Ersek [mailto:address@hidden
> Sent: 01 April 2019 14:07
> To: Auger Eric <address@hidden>; Ard Biesheuvel
> <address@hidden>
> Cc: address@hidden; address@hidden;
> address@hidden; Shameerali Kolothum Thodi
> <address@hidden>; Linuxarm
> <address@hidden>; address@hidden;
> address@hidden; xuwei (O) <address@hidden>;
> address@hidden; address@hidden; Leif Lindholm
> <address@hidden>
> Subject: Re: [Qemu-devel] [PATCH v3 07/10] hw/arm/virt: Introduce opt-in
> feature "fdt"
> 
> On 03/29/19 14:56, Auger Eric wrote:

[...]

> > Sure, sorry.
> >
> > This series:
> > - [PATCH v3 00/10] ARM virt: ACPI memory hotplug support,
> > https://patchwork.kernel.org/cover/10863301/
> >
> > aims to introduce PCDIMM support in qemu. In ACPI mode, it builds the
> > SRAT and DSDT parts and relies on GED to trigger the hotplug.
> >
> > We noticed that if we build the hotpluggable memory dt nodes on top of
> > the above ACPI tables, the DIMM slots are interpreted as not
> > hotpluggable memory slots (at least we think so).
> >
> > We think the EDK2 GetMemoryMap() uses the dt node info and ignores the
> > fact that those slots are exposed as hotpluggable in the SRAT for example.
> >
> > So in this series, we are forced to not generate the hotpluggable memory
> > dt nodes if we want the DIMM slots to be effectively recognized as
> > hotpluggable.
> >
> > Could you confirm we have a correct understanding of the EDK2 behaviour
> > and if so, would there be any solution for EDK2 to absorb both the DT
> > nodes and the relevant SRAT/DSDT tables and make the slots hotpluggable.
> >
> > At qemu level, detecting we are booting in ACPI mode and purposely
> > removing the above mentioned DT nodes does not look straightforward.
> 
> The firmware is not enlightened about the ACPI content that comes from
> QEMU / fw_cfg. That ACPI content is *blindly* processed by the firmware,
> as instructed through the ACPI linker/loader script, in order to install
> the ACPI content for the OS. No actual information is consumed by the
> firmware from the ACPI payload -- and that's a feature.
> 
> The firmware does consume DT:
> 
> - If you start QEMU *with* "-no-acpi", then the DT is both consumed by
> the firmware (for its own information needs), and passed on to the OS.
> 
> - If you start QEMU *without* "-no-acpi" (the default), then the DT is
> consumed only by the firmware (for its own information needs), and the
> DT is hidden from the OS. The OS gets only the ACPI content
> (processed/prepared as described above).
> 
> 
> In the firmware, the "ArmVirtPkg/HighMemDxe" driver iterates over the
> base/size pairs in all the memory nodes in the DT. For each such base
> address that is currently tracked as "nonexistent" in the GCD memory
> space map, the driver currently adds the base/size range as "system
> memory". This in turn is reflected by the UEFI memmap that the OS gets
> to see as "conventional memory".
> 
> If you need some memory ranges to show up as "special" in the UEFI
> memmap, then you need to distinguish them somehow from the "regular"
> memory areas, in the DT. And then extend "ArmVirtPkg/HighMemDxe" in the
> firmware, so that it act upon the discriminator that you set in the DT.
> 
> 
> Now... from a brief look at the Platform Init and UEFI specs, my
> impression is that the hotpluggable (but presently not plugged) DIMM
> ranges should simply be *absent* from the UEFI memmap; is that correct?
> (I didn't check the ACPI spec, maybe it specifies the expected behavior
> in full.) If my impression is correct, then two options (alternatives)
> exist:
> 
> (1) Hide the affected memory nodes -- or at least the affected base/size
> pairs -- from the DT, in case you boot without "-no-acpi" but with an
> external firmware loaded. Then the firmware will not expose those ranges
> as "conventional memory" in the UEFI memmap. This approach requires no
> changes to edk2.
> 
> This option is precisely what Eric described up-thread, at
> <http://mid.mail-archive.com/address@hidden
> at.com>:
> 
> > in machvirt_init, there is firmware_loaded that tells you whether you
> > have a FW image. If this one is not set, you can induce dt. But if
> > there is a FW it can be either DT or ACPI booted. You also have the
> > acpi_enabled knob.
> 
> (The "-no-acpi" cmdline option clears the "acpi_enabled" variable in
> "vl.c").
> 
> So, the condition for hiding the hotpluggable memory nodes in question
> from the DT is:
> 
>   (aarch64 && firmware_loaded && acpi_enabled)

Thanks for your explanation and suggestions. I had a quick run with the above
and it seems to do the job. I will drop this extra opt-in feature patch from 
this
series and instead have this check.

Thanks,
Shameer

> 
> 
> (2) Invent and set an "ignore me, firmware" property for the
> hotpluggable memory nodes in the DT, and update the firmware to honor
> that property.
> 
> Thanks
> Laszlo