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

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

On 1/6/23 09:03, Gerd Hoffmann wrote:
> On Fri, Jan 06, 2023 at 07:42:20AM +0100, Laszlo Ersek wrote:
>> On 1/6/23 05:12, Ni, Ray wrote:
>>>
>>> Ard,
>>>
>>> Only AMD X64 (including SEV and without SEV) runs the code that
>>> switches to 32bit paging disabled mode.
>>> Intel X64 runs the code that stays at 64bit paging mode. So no need
>>> for <4G memory.
>>> All IA32 CPUs (including intel and AMD) stays at 32bit paging disabled
>>> mode. The AllocateReservedPages() call should not return a memory
>>> above 4GB in 32bit env.
>>
>> This argument about the allocations sounds valid, thanks.
>>
>> The code still remains incredibly hard to read. It needs serious
>> cleanup.
>>
>> (1) Wherever we have "Amd" in an identifier, let's rename it to "Amd64",
>>     to better reflect the revised check.
> 
> Maybe even better:  Use PcdConfidentialComputingGuestAttr to figure
> whenever SEV is active, if so branch into Amd assembler code.  Rename
> "Amd" to "AmdSev".
> 
> Otherwise just call normal X64 / Ia32 code.
> 
> Amd assembler code can subsequently be simplified, the checks for SEV
> are not needed any more (but should not harm either).
> 
> [ Adding Tom to CC ]
> 
>>     Commit 73ccde8f6d04 ("UefiCpuPkg: Has APs in 64 bit long-mode before
>>     booting to OS.", 2022-12-20) *removed* the executable marking.
>>
>> (4a) Is that not a problem?
> 
> I think so.

Ah... OK, my fault: one should never ask questions in English the
negative! :)

So, based on your next paragraph, I think you agree that this *is* a
problem. (I first thought you agreed with the lack of executable marking
*not* being a problem -- again, my mistake for formulating the question
in the negative!)

> 
> Booting with strict NX checking (PcdDxeNxMemoryProtectionPolicy =
> 0xC000000000007FD5) and "qemu -smp 2" makes my qemu hang with 200% CPU,
> so probably both vcpus are spinning in a dead loop.  For the BSP this is
> expected behavior (buggy grub.efi, see parallel thread).  For the AP it
> is not, so apparently it is not running idle in hlt like it is supposed
> to.
> 
>> Honestly, at this point I'm *even more convinced* that the original
>> series should be reverted, and redone from the ground up.
> 
> Yes, "back to drawing board" looks like the best option at this point.

Let me see if I can post a revert series today.

Thanks
Laszlo