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

[edk2-devel] [PATCH V2] 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 "o64 retf" 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..a8216cd56f 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
+    o64 retf
 setCodeSelectorLongJump:
-    add     rsp, 0x10
     ret
 
 ;------------------------------------------------------------------------------
-- 
2.16.2.windows.1

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

[Laszlo Ersek]

Adding James, Mike, and Tom:

On 01/07/21 04:10, Guo Dong wrote:
> 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 "o64 retf" 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..a8216cd56f 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
> +    o64 retf
>  setCodeSelectorLongJump:
> -    add     rsp, 0x10
>      ret
>  
>  ;------------------------------------------------------------------------------
> 

I'm asking for:

- a Reviewed-by or Tested-by from James please, and
- an Acked-by or Reviewed-by from Mike please, and
- a Reviewed-by or Tested-by from Tom please.

Personally I'm not going to verify the details of the conversion of the
mode switch, from the far jump to the far ret, but squinting from a
distance, the patch looks plausible.

Acked-by: Laszlo Ersek <lersek@redhat.com>

Thanks!
Laszlo

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

[Lendacky, Thomas]

On 1/6/21 9:10 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 in 64bit
> mode, the address setCodeSelectorLongJump in stack will
> be override by parameter. Jump to Qword is not supported
> by some processors. So use "o64 retf" instead.
> 
> Signed-off-by: Guo Dong <guo.dong@intel.com>

With one little comment below, I verified this method allows my system to 
boot.

Tested-by: Tom Lendacky <thomas.lendacky@amd.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..a8216cd56f 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,

Extraneous comma after rcx.

Thanks,
Tom

>       lea     rax, [setCodeSelectorLongJump]
> -    mov     [rsp], rax
> -    mov     [rsp+4], cx
> -    jmp     dword far [rsp]
> +    push    rax
> +    o64 retf
>   setCodeSelectorLongJump:
> -    add     rsp, 0x10
>       ret
>   
>   ;------------------------------------------------------------------------------
> 

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

[Guo Dong]

Thanks Tom to help verify this change and point out an extra comma after rcx.
I will remove that extra comma when pushing this patch.
I could send V3 patch if required to remove this extra comma.

Thanks,
Guo

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Lendacky,
> Thomas
> Sent: Thursday, January 7, 2021 8:02 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>;
> lersek@redhat.com; Kumar, Rahul1 <rahul1.kumar@intel.com>
> Subject: Re: [edk2-devel] [PATCH V2] UefiCpuPkg/CpuDxe: Fix boot error
> 
> On 1/6/21 9:10 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 in 64bit
> > mode, the address setCodeSelectorLongJump in stack will
> > be override by parameter. Jump to Qword is not supported
> > by some processors. So use "o64 retf" instead.
> >
> > Signed-off-by: Guo Dong <guo.dong@intel.com>
> 
> With one little comment below, I verified this method allows my system to
> boot.
> 
> Tested-by: Tom Lendacky <thomas.lendacky@amd.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..a8216cd56f 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,
> 
> Extraneous comma after rcx.
> 
> Thanks,
> Tom
> 
> >       lea     rax, [setCodeSelectorLongJump]
> > -    mov     [rsp], rax
> > -    mov     [rsp+4], cx
> > -    jmp     dword far [rsp]
> > +    push    rax
> > +    o64 retf
> >   setCodeSelectorLongJump:
> > -    add     rsp, 0x10
> >       ret
> >
> >   ;------------------------------------------------------------------------------
> >
> 
> 
> 
> 

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

[Laszlo Ersek]

On 01/07/21 17:14, Dong, Guo wrote:
> 
> Thanks Tom to help verify this change and point out an extra comma after rcx.
> I will remove that extra comma when pushing this patch.
> I could send V3 patch if required to remove this extra comma.

I think a v3 is unneeded just for that.

Thanks
Laszlo

> 
> Thanks,
> Guo
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Lendacky,
>> Thomas
>> Sent: Thursday, January 7, 2021 8:02 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>;
>> lersek@redhat.com; Kumar, Rahul1 <rahul1.kumar@intel.com>
>> Subject: Re: [edk2-devel] [PATCH V2] UefiCpuPkg/CpuDxe: Fix boot error
>>
>> On 1/6/21 9:10 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 in 64bit
>>> mode, the address setCodeSelectorLongJump in stack will
>>> be override by parameter. Jump to Qword is not supported
>>> by some processors. So use "o64 retf" instead.
>>>
>>> Signed-off-by: Guo Dong <guo.dong@intel.com>
>>
>> With one little comment below, I verified this method allows my system to
>> boot.
>>
>> Tested-by: Tom Lendacky <thomas.lendacky@amd.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..a8216cd56f 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,
>>
>> Extraneous comma after rcx.
>>
>> Thanks,
>> Tom
>>
>>>       lea     rax, [setCodeSelectorLongJump]
>>> -    mov     [rsp], rax
>>> -    mov     [rsp+4], cx
>>> -    jmp     dword far [rsp]
>>> +    push    rax
>>> +    o64 retf
>>>   setCodeSelectorLongJump:
>>> -    add     rsp, 0x10
>>>       ret
>>>
>>>   ;------------------------------------------------------------------------------
>>>
>>
>>
>> 
>>
> 

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

[Michael D Kinney]

Reviewed-by: Michael D Kinney <michael.d.kinney@intel.com>

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Guo Dong
> Sent: Wednesday, January 6, 2021 7:11 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 V2] 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 "o64 retf" 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..a8216cd56f 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
> +    o64 retf
>  setCodeSelectorLongJump:
> -    add     rsp, 0x10
>      ret
> 
>  ;------------------------------------------------------------------------------
> --
> 2.16.2.windows.1
> 
> 
> 
> 
> 

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

[James Bottomley]

On Thu, 2021-01-07 at 12:56 +0100, Laszlo Ersek wrote:
> Adding James, Mike, and Tom:
> 
> On 01/07/21 04:10, Guo Dong wrote:
> > 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 "o64 retf" 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..a8216cd56f 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
> > +    o64 retf
> >  setCodeSelectorLongJump:
> > -    add     rsp, 0x10
> >      ret
> >  
> >  ;-----------------------------------------------------------------
> > -------------
> > 
> 
> I'm asking for:
> James Bottomley <jejb@linux.ibm.com>
> - a Reviewed-by or Tested-by from James please, and

Tested-by: James Bottomley <jejb@linux.ibm.com>

James

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

[Ni, Ray]

Reviewed-by: Ray Ni <ray.ni@intel.com>
I will remove the additional comma when creating the pull request.

> -----Original Message-----
> From: James Bottomley <jejb@linux.ibm.com>
> Sent: Friday, January 8, 2021 6:25 AM
> To: devel@edk2.groups.io; lersek@redhat.com; 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>; Tom Lendacky
> <thomas.lendacky@amd.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] [PATCH V2] UefiCpuPkg/CpuDxe: Fix boot error
> 
> On Thu, 2021-01-07 at 12:56 +0100, Laszlo Ersek wrote:
> > Adding James, Mike, and Tom:
> >
> > On 01/07/21 04:10, Guo Dong wrote:
> > > 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 "o64 retf" 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..a8216cd56f 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
> > > +    o64 retf
> > >  setCodeSelectorLongJump:
> > > -    add     rsp, 0x10
> > >      ret
> > >
> > >  ;-----------------------------------------------------------------
> > > -------------
> > >
> >
> > I'm asking for:
> > James Bottomley <jejb@linux.ibm.com>
> > - a Reviewed-by or Tested-by from James please, and
> 
> Tested-by: James Bottomley <jejb@linux.ibm.com>
> 
> James
>