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

[Auger Eric]

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