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

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

[Guo Dong]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3084

When DXE drivers are dispatched above 4GB memory in 64bit
mode, the address setCodeSelectorLongJump in stack will
be override by parameter. Jump to Qword is not supported
by some processors. So use retfq instead.

Signed-off-by: Guo Dong <guo.dong@intel.com>
---
 UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm b/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
index c3489bcc3e..e33ddb2784 100644
--- a/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
+++ b/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
@@ -20,13 +20,11 @@
 ;------------------------------------------------------------------------------
 global ASM_PFX(SetCodeSelector)
 ASM_PFX(SetCodeSelector):
-    sub     rsp, 0x10
+    push    rcx,
     lea     rax, [setCodeSelectorLongJump]
-    mov     [rsp], rax
-    mov     [rsp+4], cx
-    jmp     dword far [rsp]
+    push    rax
+    DB 0x48, 0xcb                      ; retfq
 setCodeSelectorLongJump:
-    add     rsp, 0x10
     ret
 
 ;------------------------------------------------------------------------------
-- 
2.16.2.windows.1

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

[Michael D Kinney]

Hi Guo,

NASM has good support for instructions.  Can the DB be removed and replaced with the equivalent instruction?

Thanks,

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Guo Dong
> Sent: Thursday, December 24, 2020 12:04 PM
> 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 in 64bit
> mode, the address setCodeSelectorLongJump in stack will
> be override by parameter. Jump to Qword is not supported
> by some processors. So use retfq instead.
> 
> Signed-off-by: Guo Dong <guo.dong@intel.com>
> ---
>  UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm b/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
> index c3489bcc3e..e33ddb2784 100644
> --- a/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
> +++ b/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
> @@ -20,13 +20,11 @@
>  ;------------------------------------------------------------------------------
>  global ASM_PFX(SetCodeSelector)
>  ASM_PFX(SetCodeSelector):
> -    sub     rsp, 0x10
> +    push    rcx,
>      lea     rax, [setCodeSelectorLongJump]
> -    mov     [rsp], rax
> -    mov     [rsp+4], cx
> -    jmp     dword far [rsp]
> +    push    rax
> +    DB 0x48, 0xcb                      ; retfq
>  setCodeSelectorLongJump:
> -    add     rsp, 0x10
>      ret
> 
>  ;------------------------------------------------------------------------------
> --
> 2.16.2.windows.1
> 
> 
> 
> 
> 

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

[Guo Dong]

Hi Mike,

Thanks for the comments. I will remove DB and submit a new patch.
I used DB because retfq is used in EDK2 only in OvmfPkg\Library\LoadLinuxLib\X64\JumpToKernel.nasm and it used DB.
Not sure if there is any BKM why they use it.

Thanks,
Guo

> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com>
> Sent: Monday, January 4, 2021 9:31 PM
> To: devel@edk2.groups.io; Dong, Guo <guo.dong@intel.com>; Kinney, Michael
> D <michael.d.kinney@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> lersek@redhat.com; Kumar, Rahul1 <rahul1.kumar@intel.com>
> Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg/CpuDxe: Fix boot error
> 
> Hi Guo,
> 
> NASM has good support for instructions.  Can the DB be removed and replaced
> with the equivalent instruction?
> 
> Thanks,
> 
> Mike
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Guo Dong
> > Sent: Thursday, December 24, 2020 12:04 PM
> > 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 in 64bit
> > mode, the address setCodeSelectorLongJump in stack will
> > be override by parameter. Jump to Qword is not supported
> > by some processors. So use retfq instead.
> >
> > Signed-off-by: Guo Dong <guo.dong@intel.com>
> > ---
> >  UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm | 8 +++-----
> >  1 file changed, 3 insertions(+), 5 deletions(-)
> >
> > diff --git a/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
> b/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
> > index c3489bcc3e..e33ddb2784 100644
> > --- a/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
> > +++ b/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
> > @@ -20,13 +20,11 @@
> >  ;------------------------------------------------------------------------------
> >  global ASM_PFX(SetCodeSelector)
> >  ASM_PFX(SetCodeSelector):
> > -    sub     rsp, 0x10
> > +    push    rcx,
> >      lea     rax, [setCodeSelectorLongJump]
> > -    mov     [rsp], rax
> > -    mov     [rsp+4], cx
> > -    jmp     dword far [rsp]
> > +    push    rax
> > +    DB 0x48, 0xcb                      ; retfq
> >  setCodeSelectorLongJump:
> > -    add     rsp, 0x10
> >      ret
> >
> >  ;------------------------------------------------------------------------------
> > --
> > 2.16.2.windows.1
> >
> >
> >
> > 
> >

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

[Michael D Kinney]

Hi Guo,

Could be the port from MASM to NASM did not check to see if NASM supported the instruction.

You can verify the NASM disassembly to make sure it matches the DB bytes.

Mike

> -----Original Message-----
> From: Dong, Guo <guo.dong@intel.com>
> Sent: Tuesday, January 5, 2021 4:51 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; 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: RE: [edk2-devel] [PATCH] UefiCpuPkg/CpuDxe: Fix boot error
> 
> 
> Hi Mike,
> 
> Thanks for the comments. I will remove DB and submit a new patch.
> I used DB because retfq is used in EDK2 only in OvmfPkg\Library\LoadLinuxLib\X64\JumpToKernel.nasm and it used DB.
> Not sure if there is any BKM why they use it.
> 
> Thanks,
> Guo
> 
> > -----Original Message-----
> > From: Kinney, Michael D <michael.d.kinney@intel.com>
> > Sent: Monday, January 4, 2021 9:31 PM
> > To: devel@edk2.groups.io; Dong, Guo <guo.dong@intel.com>; Kinney, Michael
> > D <michael.d.kinney@intel.com>
> > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
> > lersek@redhat.com; Kumar, Rahul1 <rahul1.kumar@intel.com>
> > Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg/CpuDxe: Fix boot error
> >
> > Hi Guo,
> >
> > NASM has good support for instructions.  Can the DB be removed and replaced
> > with the equivalent instruction?
> >
> > Thanks,
> >
> > Mike
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Guo Dong
> > > Sent: Thursday, December 24, 2020 12:04 PM
> > > 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 in 64bit
> > > mode, the address setCodeSelectorLongJump in stack will
> > > be override by parameter. Jump to Qword is not supported
> > > by some processors. So use retfq instead.
> > >
> > > Signed-off-by: Guo Dong <guo.dong@intel.com>
> > > ---
> > >  UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm | 8 +++-----
> > >  1 file changed, 3 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
> > b/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
> > > index c3489bcc3e..e33ddb2784 100644
> > > --- a/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
> > > +++ b/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
> > > @@ -20,13 +20,11 @@
> > >  ;------------------------------------------------------------------------------
> > >  global ASM_PFX(SetCodeSelector)
> > >  ASM_PFX(SetCodeSelector):
> > > -    sub     rsp, 0x10
> > > +    push    rcx,
> > >      lea     rax, [setCodeSelectorLongJump]
> > > -    mov     [rsp], rax
> > > -    mov     [rsp+4], cx
> > > -    jmp     dword far [rsp]
> > > +    push    rax
> > > +    DB 0x48, 0xcb                      ; retfq
> > >  setCodeSelectorLongJump:
> > > -    add     rsp, 0x10
> > >      ret
> > >
> > >  ;------------------------------------------------------------------------------
> > > --
> > > 2.16.2.windows.1
> > >
> > >
> > >
> > > 
> > >

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

[Laszlo Ersek]

On 01/06/21 03:00, Kinney, Michael D wrote:
> Hi Guo,
> 
> Could be the port from MASM to NASM did not check to see if NASM supported the instruction.

I agree: ad8ae98d2fa2 ("OvmfPkg LoadLinuxLib: Convert
X64/JumpToKernel.asm to NASM", 2014-10-31). I seem to remember that
Jordan implemented that large MASM->NASM conversion series mostly
mechanically (which was the right approach, of course, given the number
of assembly files, and the regression risks).

> You can verify the NASM disassembly to make sure it matches the DB bytes.

Thanks!
Laszlo

> 
> Mike
> 
>> -----Original Message-----
>> From: Dong, Guo <guo.dong@intel.com>
>> Sent: Tuesday, January 5, 2021 4:51 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; 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: RE: [edk2-devel] [PATCH] UefiCpuPkg/CpuDxe: Fix boot error
>>
>>
>> Hi Mike,
>>
>> Thanks for the comments. I will remove DB and submit a new patch.
>> I used DB because retfq is used in EDK2 only in OvmfPkg\Library\LoadLinuxLib\X64\JumpToKernel.nasm and it used DB.
>> Not sure if there is any BKM why they use it.
>>
>> Thanks,
>> Guo
>>
>>> -----Original Message-----
>>> From: Kinney, Michael D <michael.d.kinney@intel.com>
>>> Sent: Monday, January 4, 2021 9:31 PM
>>> To: devel@edk2.groups.io; Dong, Guo <guo.dong@intel.com>; Kinney, Michael
>>> D <michael.d.kinney@intel.com>
>>> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>;
>>> lersek@redhat.com; Kumar, Rahul1 <rahul1.kumar@intel.com>
>>> Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg/CpuDxe: Fix boot error
>>>
>>> Hi Guo,
>>>
>>> NASM has good support for instructions.  Can the DB be removed and replaced
>>> with the equivalent instruction?
>>>
>>> Thanks,
>>>
>>> Mike
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Guo Dong
>>>> Sent: Thursday, December 24, 2020 12:04 PM
>>>> 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 in 64bit
>>>> mode, the address setCodeSelectorLongJump in stack will
>>>> be override by parameter. Jump to Qword is not supported
>>>> by some processors. So use retfq instead.
>>>>
>>>> Signed-off-by: Guo Dong <guo.dong@intel.com>
>>>> ---
>>>>  UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm | 8 +++-----
>>>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
>>> b/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
>>>> index c3489bcc3e..e33ddb2784 100644
>>>> --- a/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
>>>> +++ b/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
>>>> @@ -20,13 +20,11 @@
>>>>  ;------------------------------------------------------------------------------
>>>>  global ASM_PFX(SetCodeSelector)
>>>>  ASM_PFX(SetCodeSelector):
>>>> -    sub     rsp, 0x10
>>>> +    push    rcx,
>>>>      lea     rax, [setCodeSelectorLongJump]
>>>> -    mov     [rsp], rax
>>>> -    mov     [rsp+4], cx
>>>> -    jmp     dword far [rsp]
>>>> +    push    rax
>>>> +    DB 0x48, 0xcb                      ; retfq
>>>>  setCodeSelectorLongJump:
>>>> -    add     rsp, 0x10
>>>>      ret
>>>>
>>>>  ;------------------------------------------------------------------------------
>>>> --
>>>> 2.16.2.windows.1
>>>>
>>>>
>>>>
>>>> 
>>>>
> 

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

[Laszlo Ersek]

On 01/05/21 05:31, Michael D Kinney wrote:
> Hi Guo,
> 
> NASM has good support for instructions.  Can the DB be removed and replaced with the equivalent instruction?

Indeed. Thanks!
Laszlo

> 
> Thanks,
> 
> Mike
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Guo Dong
>> Sent: Thursday, December 24, 2020 12:04 PM
>> 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 in 64bit
>> mode, the address setCodeSelectorLongJump in stack will
>> be override by parameter. Jump to Qword is not supported
>> by some processors. So use retfq instead.
>>
>> Signed-off-by: Guo Dong <guo.dong@intel.com>
>> ---
>>  UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm b/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
>> index c3489bcc3e..e33ddb2784 100644
>> --- a/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
>> +++ b/UefiCpuPkg/CpuDxe/X64/CpuAsm.nasm
>> @@ -20,13 +20,11 @@
>>  ;------------------------------------------------------------------------------
>>  global ASM_PFX(SetCodeSelector)
>>  ASM_PFX(SetCodeSelector):
>> -    sub     rsp, 0x10
>> +    push    rcx,
>>      lea     rax, [setCodeSelectorLongJump]
>> -    mov     [rsp], rax
>> -    mov     [rsp+4], cx
>> -    jmp     dword far [rsp]
>> +    push    rax
>> +    DB 0x48, 0xcb                      ; retfq
>>  setCodeSelectorLongJump:
>> -    add     rsp, 0x10
>>      ret
>>
>>  ;------------------------------------------------------------------------------
>> --
>> 2.16.2.windows.1
>>
>>
>>
>>
>>
> 
> 
> 
> 
> 
> 

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

[Guo Dong]

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

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

[Ni, Ray]

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

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

[Dong, Eric]

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

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

[Laszlo Ersek]

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
> 

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

[Guo Dong]

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

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

[Lendacky, Thomas]

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
> 

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

[Laszlo Ersek]

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

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

[Lendacky, Thomas]

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

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

[Guo Dong]

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

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

[Ni, Ray]

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

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

[Guo Dong]

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