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

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

[Guo Dong]

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

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

[Laszlo Ersek]

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

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

[Ni, Ray]

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