UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3084
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.
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
--
2.16.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#68199): https://edk2.groups.io/g/devel/message/68199
Mute This Topic: https://groups.io/mt/78671060/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Reviewed-by: Eric Dong <eric.dong@intel.com>
-----Original Message-----
From: Guo Dong <guo.dong@intel.com>
Sent: Thursday, December 3, 2020 5:39 AM
To: devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; lersek@redhat.com; Kumar, Rahul1 <rahul1.kumar@intel.com>
Subject: [edk2-devel] [PATCH] UefiCpuPkg/CpuDxe: Fix boot error
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3084
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.
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
--
2.16.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#68223): https://edk2.groups.io/g/devel/message/68223
Mute This Topic: https://groups.io/mt/78671060/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 12/2/20 3:38 PM, Guo Dong via groups.io wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3084 > > 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] http://support.amd.com/TechDocs/24594.pdf 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 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#68597): https://edk2.groups.io/g/devel/message/68597 Mute This Topic: https://groups.io/mt/78671060/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 12/09/20 21:02, Tom Lendacky wrote: > On 12/2/20 3:38 PM, Guo Dong via groups.io wrote: >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3084 >> >> 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] http://support.amd.com/TechDocs/24594.pdf > 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.) 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 >> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#68642): https://edk2.groups.io/g/devel/message/68642 Mute This Topic: https://groups.io/mt/78671060/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 >>> >> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#68663): https://edk2.groups.io/g/devel/message/68663 Mute This Topic: https://groups.io/mt/78671060/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> -----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&data=04%7C01%7Cthomas. > lendacky%40amd.com%7Ce2b6480c67df4f62e2ba08d89ce89aab%7C3dd8961fe > 4884e608e11a82d994e183d%7C0%7C0%7C637431870560945022%7CUnknown > %7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWw > iLCJXVCI6Mn0%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.a > md.com%2FTechDocs%2F24594.pdf&data=04%7C01%7Cthomas.lendacky > %40amd.com%7Ce2b6480c67df4f62e2ba08d89ce89aab%7C3dd8961fe4884e60 > 8e11a82d994e183d%7C0%7C0%7C637431870560945022%7CUnknown%7CTWF > pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI > 6Mn0%3D%7C1000&sdata=2rw8eZJNB5EgNR9JN87eWLgnHCYM0mWVJIp > hSyrtmug%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. [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 > >>> > >> > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#68666): https://edk2.groups.io/g/devel/message/68666 Mute This Topic: https://groups.io/mt/78671060/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
> > [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. > Tom, We don't need to consider shadow stack because shadow stack is only enabled in SMM environment. Guo, I suggest we either revert the patch to keep the original 4G limitation, or use RETF to be maximum compatible to Intel and AMD processors. Checking high 4 bytes of RAX hides the problem in AMD processors. Thanks, Ray -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#68681): https://edk2.groups.io/g/devel/message/68681 Mute This Topic: https://groups.io/mt/78671060/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
I agree with Ray to use RETF. Thanks, Guo > -----Original Message----- > From: Ni, Ray <ray.ni@intel.com> > Sent: Thursday, December 10, 2020 6:48 PM > To: Dong, Guo <guo.dong@intel.com>; devel@edk2.groups.io; > thomas.lendacky@amd.com; Laszlo Ersek <lersek@redhat.com> > Cc: Dong, Eric <eric.dong@intel.com>; Kumar, Rahul1 > <rahul1.kumar@intel.com> > Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg/CpuDxe: Fix boot error > > > > > [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. > > > > Tom, > We don't need to consider shadow stack because shadow stack is only enabled > in SMM environment. > > Guo, > I suggest we either revert the patch to keep the original 4G limitation, or use > RETF to be maximum > compatible to Intel and AMD processors. > Checking high 4 bytes of RAX hides the problem in AMD processors. > > Thanks, > Ray -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#68686): https://edk2.groups.io/g/devel/message/68686 Mute This Topic: https://groups.io/mt/78671060/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 12/02/20 22:38, Guo Dong wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3084 > > 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. > > 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 I'll let Ray and Eric review this patch, just one question for my understanding: for clarity, shouldn't the above MOV have referred to "eax" in the first place? Because (before this patch) it seems to store 8 bytes (from rax), and then to overwrite the high-address DWORD of those 8 bytes, with the next operation (which stores 2 bytes from CX). IMO, for clarity's sake, the original code should have used EAX. Why write 8 bytes when only the low-address 4 bytes matter anyway? Anyhow, this is no longer relevant, because of the present patch; I just wanted to ask about it so I understand better. Thanks Laszlo > - mov [rsp+4], cx > - jmp dword far [rsp] > + mov [rsp+8], cx > + jmp qword far [rsp] > setCodeSelectorLongJump: > add rsp, 0x10 > ret > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#68235): https://edk2.groups.io/g/devel/message/68235 Mute This Topic: https://groups.io/mt/78671060/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Hi Laszlo, See my comments below. Thanks, Guo > -----Original Message----- > From: Laszlo Ersek <lersek@redhat.com> > Sent: Thursday, December 3, 2020 3:22 AM > To: 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/02/20 22:38, Guo Dong wrote: > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3084 > > > > 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. > > > > 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 > > I'll let Ray and Eric review this patch, just one question for my > understanding: > > for clarity, shouldn't the above MOV have referred to "eax" in the first > place? Because (before this patch) it seems to store 8 bytes (from rax), > and then to overwrite the high-address DWORD of those 8 bytes, with the > next operation (which stores 2 bytes from CX). > > IMO, for clarity's sake, the original code should have used EAX. Why > write 8 bytes when only the low-address 4 bytes matter anyway? > These asm code is in X64 folder for long mode, the variable address could be below 4GB or above 4GB. If it is below 4GB, using eax or overriding the high 4 bytes doesn't matter. If it is in above 4GB, then the high 4 bytes matters, it should not be override by others. > Anyhow, this is no longer relevant, because of the present patch; I just > wanted to ask about it so I understand better. > > Thanks > Laszlo > > > - mov [rsp+4], cx > > - jmp dword far [rsp] > > + mov [rsp+8], cx > > + jmp qword far [rsp] > > setCodeSelectorLongJump: > > add rsp, 0x10 > > ret > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#68482): https://edk2.groups.io/g/devel/message/68482 Mute This Topic: https://groups.io/mt/78671060/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Reviewed-by: Ray Ni <ray.ni@intel.com> > -----Original Message----- > From: Guo Dong <guo.dong@intel.com> > Sent: Thursday, December 3, 2020 5:39 AM > To: devel@edk2.groups.io > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; > lersek@redhat.com; Kumar, Rahul1 <rahul1.kumar@intel.com> > Subject: [edk2-devel] [PATCH] UefiCpuPkg/CpuDxe: Fix boot error > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3084 > > 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. > > 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 > -- > 2.16.2.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#68207): https://edk2.groups.io/g/devel/message/68207 Mute This Topic: https://groups.io/mt/78671060/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.