[edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Fix a hang in above 4GB case

Guo Dong posted 1 patch 1 week, 4 days ago
Failed in applying to current master (apply log)
UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

[edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Fix a hang in above 4GB case

Posted by Guo Dong 1 week, 4 days ago
This patch fixed the hang in UEFICpuPkg when it is dispatched above 4GB.
In UEFI BIOS case CpuInfoInHob is provided to DXE under 4GB from PEI.
When using UEFI payload and bootloaders, CpuInfoInHob will be allocated
above 4GB since it is not provided from bootloader. so we need update
the code to make sure this hob could be accessed correctly in this case.

Signed-off-by: Guo Dong <guo.dong@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index 5532a1d391..aecfd07bc0 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -303,17 +303,17 @@ GetProcessorNumber:
     ;
     xor         ebx, ebx
     lea         eax, [esi + CpuInfoLocation]
-    mov         edi, [eax]
+    mov         rdi, [eax]
 
 GetNextProcNumber:
-    cmp         dword [edi], edx                      ; APIC ID match?
+    cmp         dword [rdi], edx                      ; APIC ID match?
     jz          ProgramStack
-    add         edi, 20
+    add         rdi, 20
     inc         ebx
     jmp         GetNextProcNumber
 
 ProgramStack:
-    mov         rsp, qword [edi + 12]
+    mov         rsp, qword [rdi + 12]
 
 CProcedureInvoke:
     push       rbp               ; Push BIST data at top of AP stack
-- 
2.16.2.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69880): https://edk2.groups.io/g/devel/message/69880
Mute This Topic: https://groups.io/mt/79488123/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Fix a hang in above 4GB case

Posted by Laszlo Ersek 1 week, 4 days ago
On 01/07/21 00:53, Guo Dong wrote:
> This patch fixed the hang in UEFICpuPkg when it is dispatched above 4GB.
> In UEFI BIOS case CpuInfoInHob is provided to DXE under 4GB from PEI.
> When using UEFI payload and bootloaders, CpuInfoInHob will be allocated
> above 4GB since it is not provided from bootloader. so we need update
> the code to make sure this hob could be accessed correctly in this case.
> 
> Signed-off-by: Guo Dong <guo.dong@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> index 5532a1d391..aecfd07bc0 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> @@ -303,17 +303,17 @@ GetProcessorNumber:
>      ;
>      xor         ebx, ebx
>      lea         eax, [esi + CpuInfoLocation]
> -    mov         edi, [eax]
> +    mov         rdi, [eax]
>  

This looks valid; MP_CPU_EXCHANGE_INFO.CpuInfo is a pointer, and on X64,
that means 8 bytes.

>  GetNextProcNumber:
> -    cmp         dword [edi], edx                      ; APIC ID match?
> +    cmp         dword [rdi], edx                      ; APIC ID match?

So this is "CpuInfo->InitialApicId", as far as I can tell. Using rdi
makes sense again.

>      jz          ProgramStack
> -    add         edi, 20
> +    add         rdi, 20

And this seems to advance CpuInfo to the next CPU_INFO_IN_HOB element in
the array (3 UINT32 fields, 1 UINT64 field).

>      inc         ebx
>      jmp         GetNextProcNumber
>  
>  ProgramStack:
> -    mov         rsp, qword [edi + 12]
> +    mov         rsp, qword [rdi + 12]

Seems to be CpuInfo->ApTopOfStack.

>  
>  CProcedureInvoke:
>      push       rbp               ; Push BIST data at top of AP stack
> 

Seems OK to me. Again, given the size of the
"MP_CPU_EXCHANGE_INFO.CpuInfo" pointer field, on X64, this assembly code
should have used rdi from the start, arguably.

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

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69915): https://edk2.groups.io/g/devel/message/69915
Mute This Topic: https://groups.io/mt/79488123/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Fix a hang in above 4GB case

Posted by Ni, Ray 1 week, 2 days ago
Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Thursday, January 7, 2021 7:52 PM
> To: Dong, Guo <guo.dong@intel.com>; devel@edk2.groups.io
> 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/MpInitLib: Fix a hang in above 4GB case
> 
> On 01/07/21 00:53, Guo Dong wrote:
> > This patch fixed the hang in UEFICpuPkg when it is dispatched above 4GB.
> > In UEFI BIOS case CpuInfoInHob is provided to DXE under 4GB from PEI.
> > When using UEFI payload and bootloaders, CpuInfoInHob will be allocated
> > above 4GB since it is not provided from bootloader. so we need update
> > the code to make sure this hob could be accessed correctly in this case.
> >
> > Signed-off-by: Guo Dong <guo.dong@intel.com>
> > ---
> >  UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> > index 5532a1d391..aecfd07bc0 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> > +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> > @@ -303,17 +303,17 @@ GetProcessorNumber:
> >      ;
> >      xor         ebx, ebx
> >      lea         eax, [esi + CpuInfoLocation]
> > -    mov         edi, [eax]
> > +    mov         rdi, [eax]
> >
> 
> This looks valid; MP_CPU_EXCHANGE_INFO.CpuInfo is a pointer, and on X64,
> that means 8 bytes.
> 
> >  GetNextProcNumber:
> > -    cmp         dword [edi], edx                      ; APIC ID match?
> > +    cmp         dword [rdi], edx                      ; APIC ID match?
> 
> So this is "CpuInfo->InitialApicId", as far as I can tell. Using rdi
> makes sense again.
> 
> >      jz          ProgramStack
> > -    add         edi, 20
> > +    add         rdi, 20
> 
> And this seems to advance CpuInfo to the next CPU_INFO_IN_HOB element in
> the array (3 UINT32 fields, 1 UINT64 field).
> 
> >      inc         ebx
> >      jmp         GetNextProcNumber
> >
> >  ProgramStack:
> > -    mov         rsp, qword [edi + 12]
> > +    mov         rsp, qword [rdi + 12]
> 
> Seems to be CpuInfo->ApTopOfStack.
> 
> >
> >  CProcedureInvoke:
> >      push       rbp               ; Push BIST data at top of AP stack
> >
> 
> Seems OK to me. Again, given the size of the
> "MP_CPU_EXCHANGE_INFO.CpuInfo" pointer field, on X64, this assembly code
> should have used rdi from the start, arguably.
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks
> Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70074): https://edk2.groups.io/g/devel/message/70074
Mute This Topic: https://groups.io/mt/79488123/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-