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

["Yuanhao Xie" <yuanhao.xie@intel.com>]

Hi all,

Thanks for feedbacks. I will revert the original ones, and send the new version.

Yuanhao

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd Hoffmann
Sent: Friday, January 6, 2023 4:03 PM
To: Laszlo Ersek <lersek@redhat.com>
Cc: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; ardb@kernel.org; Xie, Yuanhao <yuanhao.xie@intel.com>; thomas.lendacky@amd.com
Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg:Fixed AsmRelocateApLoopStart and ensure allocated memory <4GB

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