Re: [edk2-devel] [PATCH] UefiCpuPkg:Fixed AsmRelocateApLoopStart and ensure allocated memory <4GB

["Laszlo Ersek" <lersek@redhat.com>]

On 1/6/23 10:45, Ni, Ray wrote:
>> This makes sense, but, again, even disregarding the problem that the
>> code forgot to switch to the new page table, the idea should be spelled
>> out in the commit message and/or in code comments. Preferably: both.
>>
>> (In fact if the idea had been documented, Yuanhao might not have
>> forgotten to implement the switch.)
>>
> 
> But the following cases still require the XD bit be removed from the active page table:
> *. 32bit mode
>     In fact, old 32bit logic contains a bug that paging might still be enabled when AP is in the idle loop.
>     That's a long-exist bug. QEMU doesn't enable the memory protection feature resulting the paging is disabled in 32bit mode.
>     Hence the bug doesn't appear in QEMU 32bit image.
>     Yuanhao's logic tries to not create page table for 32bit mode. I agree with this because it simplifies the 32bit flow.
>     But considering the paging might be enabled, XD removal logic should be kept.
> 
> *. 64bit AMD
>     Today Yuanhao's logic tries to not modify AMD 64bit flow no matter SEV is enabled or not.
>     But if the BSP page table is used by AP when running the code in the reserved memory, the XD bit removal logic should be
>     kept for 64bit AMD.
>     So, that means only 64bit Intel flow doesn't require the XD bit removal logic.
>     Then, for code simplicity (reducing the branches), I recommend keeping the XD bit removal logic always.

That sounds OK to me.

But then, can we go back to the original purpose here? What is achieved
by simplifying the current MpFuncs stuff, for 64-bit Intel processors?
What is achieved?

Commit 73ccde8f6d04 says things like "avoiding switching modes", and
"reclaiming memory by OS". I don't think I understand the importance of
those.

First, "reclaiming memory by OS" seems questionable, as both before and
after, we need reserved memory. In fact, with the modification, we might
need even more reserved memory (which the OS cannot reclaim): we still
need the portion for the loop func, we still need the portion for the
stacks, and we also need a new page table. So that actually seems *worse*.

The edk2 codebase seen as a whole gets more complicated, that's a
negative too. There are some savings in X64/MpFuncs.nasm for 64-bit
Intel processors, but we need to preserve (and maintain) the preexistent
code too, so it's a pure code growth, from the codebase perspective.

All this against the alleged benefit of "avoiding switching modes".

What is wrong with switching modes? The OS needs to boot up the APs
*once*, when it starts. The mode in which the firmware parked the APs is
effectively irrelevant.

Is this change worth the effort and code complications at all?

Now, there *is* one benefit I can imagine. For Intel maintainers, it may
be difficult to maintain and to "route around" the SEV-related stuff in
"X64/MpFuncs.nasm", in the long term. I can wholly accept that. So
splitting and duplicating the assembly code for that purpose is
justified. But then the commit message should state this, and not
present "staying in 64-bit" as a benefit per se.

Then the purpose is to ease the assembly code maintenance for Intel
developers. Entirely justified goal in my view; nobody likes to work
with complicated code they can't regression-test (and I presume Intel
developers can't easily test the various SEV enablement levels in-house,
on a range of AMD processors).

Thanks
Laszlo

> Imaging in a long future that 32bit x86 support is removed from edk2 and the legacy SEV is removed as well, we can cleanup
> the unnecessary XD bit removal then.
>     
> 
> 
> 
> 
>