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

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

On 1/6/23 09:39, Ni, Ray wrote:
> 
> 
>> -----Original Message-----
>> From: Laszlo Ersek <lersek@redhat.com>
>> Sent: Friday, January 6, 2023 4:31 PM
>> To: Gerd Hoffmann <kraxel@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 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!)
> 
> I agree it's a problem. Original thought was since AP is using a brand-new page table
> that doesn't have the XD bit set. There is no need for removing the XD bit in
> existing page table.

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.)

Laszlo

> But the final code change forgot to switch to the new page table before calling to
> code in the reserved memory.
> 
> 
> 
> 
> 
>