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

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Igor Mammedov [mailto:address@hidden
> Sent: 03 April 2019 10:49
> To: Laszlo Ersek <address@hidden>
> Cc: Auger Eric <address@hidden>; Ard Biesheuvel
> <address@hidden>; address@hidden;
> address@hidden; address@hidden; Shameerali Kolothum Thodi
> <address@hidden>; Linuxarm
> <address@hidden>; address@hidden;
> address@hidden; xuwei (O) <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 Tue, 2 Apr 2019 17:38:26 +0200
> Laszlo Ersek <address@hidden> 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 a lot for all those inputs!
> > >
> > > I don't get why we test aarch64 in above condition (this was useful for
> > > high ECAM range as the aarch32 FW was not supporting it but here, is it
> > > still meaningful?)
> >
> > Sorry, I should have clarified that. Yes, it is meaningful:
> >
> > While UEFI has bindings for both 32-bit and 64-bit ARM, ACPI has a
> > 64-bit-only binding for ARM. (And you can have UEFI without ACPI, but
> > not the reverse, on ARM.) So if you run the 32-bit build of the
> > ArmVirtQemu firmware, you get no ACPI at all; all you can rely on with
> > the OS is the DT.

Just to confirm, does that mean with 32-bit build of the UEFI, the OS cannot
boot with ACPI and uses DT only. So,

If ((aarch64 && firmware_loaded && acpi_enabled) {
   Hide_hotpluggable_memory_nodes()
} else {
   Add_ hotpluggable_memory_nodes()
}

should work for all cases?

> > This "bitness distinction" is implemented in the firmware already. If
> > you hid the memory nodes from the DT under the condition
> >
> >   (!aarch64 && firmware_loaded && acpi_enabled)
> >
> > then the nodes would not be seen by the OS at all (because
> > "acpi_enabled" is irrelevant for the 32-bit build of ArmVirtQemu, and
> > all the OS can ever get is DT).
> 
> It's getting tricky and I don't like a bit that we are trying to carter
> 64 bit only UEFI build (or any other build) on QEMU side. Also Peter has
> a valid about guessing on QEMU side (that's usually a source of problem
> in the future).

If the above is correct(with 32-bit variant of UEFI, OS cannot have ACPI boot),
then do we really have the issue of memory becoming non hot-un-unpluggable?
May be I am missing something. 

Thanks,
Shameer
 
> Perhaps we should reconsider and think about marking hotplugbbale RAM
> in DT and let firmware to exclude it from memory map.
> 
> > Thanks,
> > Laszlo
> >
> > >
> > > Thanks
> > >
> > > Eric
> > >
> > >>
> > >>
> > >> (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
> > >>
> >