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

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Auger Eric [mailto:address@hidden
> Sent: 29 March 2019 13:56
> To: 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; Laszlo Ersek
> <address@hidden>; Leif Lindholm <address@hidden>
> Subject: Re: [Qemu-devel] [PATCH v3 07/10] hw/arm/virt: Introduce opt-in
> feature "fdt"
> 
> Hi Ard,
> 
> On 3/29/19 2:14 PM, Ard Biesheuvel wrote:
> > On Fri, 29 Mar 2019 at 14:12, Auger Eric <address@hidden> wrote:
> >>
> >> Hi Shameer,
> >>
> >> On 3/29/19 10:59 AM, Shameerali Kolothum Thodi wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Auger Eric [mailto:address@hidden
> >>>> Sent: 29 March 2019 09:32
> >>>> To: Shameerali Kolothum Thodi
> <address@hidden>;
> >>>> address@hidden; address@hidden;
> address@hidden;
> >>>> address@hidden; address@hidden;
> >>>> address@hidden; address@hidden
> >>>> Cc: Linuxarm <address@hidden>; xuwei (O)
> <address@hidden>;
> >>>> Laszlo Ersek <address@hidden>; Ard Biesheuvel
> >>>> <address@hidden>; Leif Lindholm <address@hidden>
> >>>> Subject: Re: [PATCH v3 07/10] hw/arm/virt: Introduce opt-in feature "fdt"
> >>>>
> >>>> Hi Shameer,
> >>>>
> >>>> [ + Laszlo, Ard, Leif ]
> >>>>
> >>>> On 3/21/19 11:47 AM, Shameer Kolothum wrote:
> >>>>> This is to disable/enable populating DT nodes in case
> >>>>> any conflict with acpi tables. The default is "off".
> >>>> The name of the option sounds misleading to me. Also we don't really
> >>>> know the scope of the disablement. At the moment this just aims to
> >>>> prevent the hotpluggable dt nodes from being added if we boot in ACPI
> mode.
> >>>>
> >>>>>
> >>>>> This will be used in subsequent patch where cold plug
> >>>>> device-memory support is added for DT boot.
> >>>> I am concerned about the fact that in dt mode, by default, you won't see
> >>>> any PCDIMM nodes.
> >>>>>
> >>>>> If DT memory node support is added for cold-plugged device
> >>>>> memory, those memory will be visible to Guest kernel via
> >>>>> UEFI GetMemoryMap() and gets treated as early boot memory.
> >>>> Don't we have an issue in UEFI then. Normally the SRAT indicates whether
> >>>> the slots are hotpluggable or not. Shouldn't the UEFI code look at this
> >>>> info.
> >>>
> >>> Sorry I missed this part. Yes, that will be a more cleaner solution.
> >>>
> >>> Also, to be more clear on what happens,
> >>>
> >>> Guest ACPI boot with "fdt=on" ,
> >>>
> >>> From kernel log,
> >>>
> >>> [    0.000000] Early memory node ranges
> >>> [    0.000000]   node   0: [mem
> 0x0000000040000000-0x00000000bbf5ffff]
> >>> [    0.000000]   node   0: [mem
> 0x00000000bbf60000-0x00000000bbffffff]
> >>> [    0.000000]   node   0: [mem
> 0x00000000bc000000-0x00000000bc02ffff]
> >>> [    0.000000]   node   0: [mem
> 0x00000000bc030000-0x00000000bc36ffff]
> >>> [    0.000000]   node   0: [mem
> 0x00000000bc370000-0x00000000bf64ffff]
> >>> [    0.000000]   node   0: [mem
> 0x00000000bf650000-0x00000000bf6dffff]
> >>> [    0.000000]   node   0: [mem
> 0x00000000bf6e0000-0x00000000bf6effff]
> >>> [    0.000000]   node   0: [mem
> 0x00000000bf6f0000-0x00000000bf80ffff]
> >>> [    0.000000]   node   0: [mem
> 0x00000000bf810000-0x00000000bfffffff]
> >>> [    0.000000]   node   0: [mem
> 0x00000000c0000000-0x00000000ffffffff]  --> This is the hotpluggable memory
> node from DT.
> >>> [    0.000000] Zeroed struct page in unavailable ranges: 1040 pages
> >>> [    0.000000] Initmem setup node 0 [mem
> 0x0000000040000000-0x00000000ffffffff]
> >>>
> >>>
> >>> Guest ACPI boot with "fdt=off" ,
> >>>
> >>> [    0.000000] Movable zone start for each node
> >>> [    0.000000] Early memory node ranges
> >>> [    0.000000]   node   0: [mem
> 0x0000000040000000-0x00000000bbf5ffff]
> >>> [    0.000000]   node   0: [mem
> 0x00000000bbf60000-0x00000000bbffffff]
> >>> [    0.000000]   node   0: [mem
> 0x00000000bc000000-0x00000000bc02ffff]
> >>> [    0.000000]   node   0: [mem
> 0x00000000bc030000-0x00000000bc36ffff]
> >>> [    0.000000]   node   0: [mem
> 0x00000000bc370000-0x00000000bf64ffff]
> >>> [    0.000000]   node   0: [mem
> 0x00000000bf650000-0x00000000bf6dffff]
> >>> [    0.000000]   node   0: [mem
> 0x00000000bf6e0000-0x00000000bf6effff]
> >>> [    0.000000]   node   0: [mem
> 0x00000000bf6f0000-0x00000000bf80ffff]
> >>> [    0.000000]   node   0: [mem
> 0x00000000bf810000-0x00000000bfffffff]
> >>> [    0.000000] Zeroed struct page in unavailable ranges: 1040 pages
> >>> [    0.000000] Initmem setup node 0 [mem
> 0x0000000040000000-0x00000000bfffffff
> >>>
> >>> The hotpluggable memory node is absent from early memory nodes here.
> >>
> >> OK thank you for the example illustrating the concern.
> >>>
> >>> As you said, it could be possible to detect this node using SRAT in UEFI.
> >>
> >> Let's wait for EDK2 experts on this.
> >>
> >
> > Happy to chime in, but I need a bit more context here.
> >
> > What is the problem, how does this path try to solve it, and why is
> > that a bad idea?
> >
> 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.
> 
> Hope this clarifies

Thanks Eric for this. And this is where you can find the initial discussion on 
this,

https://www.mail-archive.com/address@hidden/msg599076.html

Thanks,
Shameer