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

["Ni, Ray" <ray.ni@intel.com>]

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