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

["Gerd Hoffmann" <kraxel@redhat.com>]

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.

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.

take care,
  Gerd