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

["Guo Dong" <guo.dong@intel.com>]

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Lendacky,
> Thomas
> Sent: Thursday, December 10, 2020 7:38 AM
> To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io; Dong, Guo
> <guo.dong@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar,
> Rahul1 <rahul1.kumar@intel.com>
> Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/CpuDxe: Fix boot error
> 
> 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.t
> ianocore.org%2Fshow_bug.cgi%3Fid%3D3084&amp;data=04%7C01%7Cthomas.
> lendacky%40amd.com%7Ce2b6480c67df4f62e2ba08d89ce89aab%7C3dd8961fe
> 4884e608e11a82d994e183d%7C0%7C0%7C637431870560945022%7CUnknown
> %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWw
> iLCJXVCI6Mn0%3D%7C1000&amp;sdata=OuvOcnWku0ct%2FHYebIVYoJ6vsqN%
> 2F56%2BMANNkvc%2BLW38%3D&amp;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.a
> md.com%2FTechDocs%2F24594.pdf&amp;data=04%7C01%7Cthomas.lendacky
> %40amd.com%7Ce2b6480c67df4f62e2ba08d89ce89aab%7C3dd8961fe4884e60
> 8e11a82d994e183d%7C0%7C0%7C637431870560945022%7CUnknown%7CTWF
> pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> 6Mn0%3D%7C1000&amp;sdata=2rw8eZJNB5EgNR9JN87eWLgnHCYM0mWVJIp
> hSyrtmug%3D&amp;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.

[Guo] From my view point, it is not necessary to have these code for X64 since CS base
address would always use 0 in long mode. But I will leave this to package owner
to decide to remove it or not.
For now to support AMD case, maybe we could check rax value to decide to use
dword jump or qword jump. If high 4 bytes of rax is zero, dword jump will be used.
With this, it has exact same behavior when CpuDxe driver is dispatch below 4GB.

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

[Guo] If we agree the above change to check rax, I could create a patch today.
Let me know if having other comments.

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