Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuDxe: Fix boot error

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 12/10/20 2:49 AM, Laszlo Ersek wrote:
> On 12/09/20 21:02, Tom Lendacky wrote:
>> On 12/2/20 3:38 PM, Guo Dong via groups.io wrote:
>>> REF: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3084&data=04%7C01%7Cthomas.lendacky%40amd.com%7Ce2b6480c67df4f62e2ba08d89ce89aab%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637431870560945022%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=OuvOcnWku0ct%2FHYebIVYoJ6vsqN%2F56%2BMANNkvc%2BLW38%3D&reserved=0
>>>
>>> When DXE drivers are dispatched above 4GB memory and
>>> the system is already in 64bit mode, the address
>>> setCodeSelectorLongJump in stack will be override
>>> by parameter. so change to use 64bit address and
>>> jump to qword address.
>>
>> This patch breaks AMD processors. AMD processors cannot do far jumps to
>> 64-bit targets. Please see AMD APM Vol. 3 [1], JMP (Far), where it states:
>>
>> Target is a code segment — Control is transferred to the target CS:rIP. In
>> this case, the target offset can only be a 16 or 32 bit value, depending
>> on operand-size, and is zero-extended to 64 bits; 64-bit offsets are only
>> available via call gates. No CPL change is allowed.
>>
>> [1] https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fsupport.amd.com%2FTechDocs%2F24594.pdf&data=04%7C01%7Cthomas.lendacky%40amd.com%7Ce2b6480c67df4f62e2ba08d89ce89aab%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637431870560945022%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=2rw8eZJNB5EgNR9JN87eWLgnHCYM0mWVJIphSyrtmug%3D&reserved=0
>>
> 
> Should we revert the patch, or predicate the change on something similar
> to StandardSignatureIsAuthenticAMD()
> [UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib.c]? The CPUID check
> could be open-coded in the assembly file. (Maybe there's a better
> method, I'm not sure.)

I'm not sure what the best approach would be. Guo, thoughts?

If there aren't any plans to enable shadow stacks, I think you can 
accomplish the 64-bit support with a far ret instead of a far jmp. If 
shadow stack is enabled, then that becomes a problem when tracking stack 
usage through shadow stack.

If more time is needed to figure it out, though, it is probably best to 
revert this in the mean time since I can't launch a VM (be it legacy or 
SEV) on the latest tree.

Thanks,
Tom

> 
> Thanks
> Laszlo
> 
>> Thanks,
>> Tom
>>
>>>> Signed-off-by: Guo Dong <guo.dong@intel.com>
>>> ---
>>>   UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm b/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
>>> index c3489bcc3e..6ad32b49f4 100644
>>> --- a/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
>>> +++ b/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
>>> @@ -23,8 +23,8 @@ ASM_PFX(SetCodeSelector):
>>>       sub     rsp, 0x10
>>>       lea     rax, [setCodeSelectorLongJump]
>>>       mov     [rsp], rax
>>> -    mov     [rsp+4], cx
>>> -    jmp     dword far [rsp]
>>> +    mov     [rsp+8], cx
>>> +    jmp     qword far [rsp]
>>>   setCodeSelectorLongJump:
>>>       add     rsp, 0x10
>>>       ret
>>>
>>
>