public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [Patch V2 1/2] UefiCpuPkg:Fix stack offset mismatch in 32bit AsmRelocateApLoopStart
@ 2023-01-06  3:11 Yuanhao Xie
  2023-01-06  3:11 ` [Patch V2 2/2] UefiCpuPkg: Keep 4GB limitation of memory allocation Yuanhao Xie
  0 siblings, 1 reply; 3+ messages in thread
From: Yuanhao Xie @ 2023-01-06  3:11 UTC (permalink / raw)
  To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar

Fix 32bit version of AsmRelocateApLoopStart to retrieve the
parameters from correct stack offset.

Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4234

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Yuanhao Xie <yuanhao.xie@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/DxeMpLib.c        | 2 +-
 UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 9 +++------
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index beab06a5b1..acbbf155c0 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -389,7 +389,7 @@ RelocateApLoop (
   MpInitLibWhoAmI (&ProcessorNumber);
   CpuMpData    = GetCpuMpData ();
   MwaitSupport = IsMwaitSupport ();
-  if (StandardSignatureIsAuthenticAMD ()) {
+  if (StandardSignatureIsAuthenticAMD () && (sizeof (UINTN) == sizeof (UINT64))) {
     StackStart               = CpuMpData->UseSevEsAPMethod ? CpuMpData->SevEsAPResetStackStart : mReservedTopOfApStack;
     AsmRelocateApLoopFuncAmd = (ASM_RELOCATE_AP_LOOP_AMD)(UINTN)mReservedApLoopFunc;
     AsmRelocateApLoopFuncAmd (
diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
index bfcdbd31c1..5cffa632ab 100644
--- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
@@ -219,20 +219,17 @@ SwitchToRealProcEnd:
 RendezvousFunnelProcEnd:
 
 ;-------------------------------------------------------------------------------------
-;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack, CountTofinish, Pm16CodeSegment, SevEsAPJumpTable, WakeupBuffer);
-;
-;  The last three parameters (Pm16CodeSegment, SevEsAPJumpTable and WakeupBuffer) are
-;  specific to SEV-ES support and are not applicable on IA32.
+;  AsmRelocateApLoop (MwaitSupport, ApTargetCState, TopOfApStack, CountTofinish, Cr3);
 ;-------------------------------------------------------------------------------------
 AsmRelocateApLoopStart:
     mov        eax, esp
-    mov        esp, [eax + 16]     ; TopOfApStack
+    mov        esp, [eax + 12]     ; TopOfApStack
     push       dword [eax]         ; push return address for stack trace
     push       ebp
     mov        ebp, esp
     mov        ebx, [eax + 8]      ; ApTargetCState
     mov        ecx, [eax + 4]      ; MwaitSupport
-    mov        eax, [eax + 20]     ; CountTofinish
+    mov        eax, [eax + 16]     ; CountTofinish
     lock dec   dword [eax]         ; (*CountTofinish)--
     cmp        cl,  1              ; Check mwait-monitor support
     jnz        HltLoop
-- 
2.36.1.windows.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* [Patch V2 2/2] UefiCpuPkg: Keep 4GB limitation of memory allocation
  2023-01-06  3:11 [Patch V2 1/2] UefiCpuPkg:Fix stack offset mismatch in 32bit AsmRelocateApLoopStart Yuanhao Xie
@ 2023-01-06  3:11 ` Yuanhao Xie
  2023-01-06  6:48   ` [edk2-devel] " Laszlo Ersek
  0 siblings, 1 reply; 3+ messages in thread
From: Yuanhao Xie @ 2023-01-06  3:11 UTC (permalink / raw)
  To: devel; +Cc: Eric Dong, Ray Ni, Rahul Kumar

Keep 4GB limitation of memory allocation if APs need transferring to
32bit mode before handoff to the OS.

Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4234

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Yuanhao Xie <yuanhao.xie@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
index acbbf155c0..e4c42e34ce 100644
--- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
@@ -480,6 +480,7 @@ InitMpGlobalData (
   EFI_GCD_MEMORY_SPACE_DESCRIPTOR  MemDesc;
   UINTN                            StackBase;
   CPU_INFO_IN_HOB                  *CpuInfoInHob;
+  EFI_PHYSICAL_ADDRESS             Address;
 
   SaveCpuMpData (CpuMpData);
 
@@ -561,13 +562,25 @@ InitMpGlobalData (
   ASSERT ((AP_SAFE_STACK_SIZE & (CPU_STACK_ALIGNMENT - 1)) == 0);
 
   mReservedApLoopFunc = (VOID *)(mReservedTopOfApStack + CpuMpData->CpuCount * AP_SAFE_STACK_SIZE);
-  if (StandardSignatureIsAuthenticAMD ()) {
+  if (StandardSignatureIsAuthenticAMD () && (sizeof (UINTN) == sizeof (UINT64))) {
+    Address = BASE_4GB - 1;
+    Status  = gBS->AllocatePages (
+                     AllocateMaxAddress,
+                     EfiReservedMemoryType,
+                     EFI_SIZE_TO_PAGES (ApSafeBufferSize),
+                     &Address
+                     );
+    ASSERT_EFI_ERROR (Status);
+    mReservedApLoopFunc = (VOID *)((UINTN)Address + CpuMpData->CpuCount * AP_SAFE_STACK_SIZE);
     CopyMem (
       mReservedApLoopFunc,
       CpuMpData->AddressMap.RelocateApLoopFuncAddressAmd,
       CpuMpData->AddressMap.RelocateApLoopFuncSizeAmd
       );
   } else {
+    Address = (UINTN)AllocateReservedPages (EFI_SIZE_TO_PAGES (ApSafeBufferSize));
+    ASSERT (Address != 0);
+    mReservedApLoopFunc = (VOID *)((UINTN)Address + CpuMpData->CpuCount * AP_SAFE_STACK_SIZE);
     CopyMem (
       mReservedApLoopFunc,
       CpuMpData->AddressMap.RelocateApLoopFuncAddress,
@@ -575,12 +588,14 @@ InitMpGlobalData (
       );
 
     mApPageTable = CreatePageTable (
-                     mReservedTopOfApStack,
+                     (UINTN)Address,
                      ApSafeBufferSize
                      );
   }
 
-  mReservedTopOfApStack += CpuMpData->CpuCount * AP_SAFE_STACK_SIZE;
+  mReservedTopOfApStack = (UINTN)Address + CpuMpData->CpuCount * AP_SAFE_STACK_SIZE;
+  ASSERT ((mReservedTopOfApStack & (UINTN)(CPU_STACK_ALIGNMENT - 1)) == 0);
+  ASSERT ((AP_SAFE_STACK_SIZE & (CPU_STACK_ALIGNMENT - 1)) == 0);
 
   Status = gBS->CreateEvent (
                   EVT_TIMER | EVT_NOTIFY_SIGNAL,
-- 
2.36.1.windows.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [edk2-devel] [Patch V2 2/2] UefiCpuPkg: Keep 4GB limitation of memory allocation
  2023-01-06  3:11 ` [Patch V2 2/2] UefiCpuPkg: Keep 4GB limitation of memory allocation Yuanhao Xie
@ 2023-01-06  6:48   ` Laszlo Ersek
  0 siblings, 0 replies; 3+ messages in thread
From: Laszlo Ersek @ 2023-01-06  6:48 UTC (permalink / raw)
  To: devel, yuanhao.xie; +Cc: Eric Dong, Ray Ni, Rahul Kumar

On 1/6/23 04:11, Yuanhao Xie wrote:
> Keep 4GB limitation of memory allocation if APs need transferring to
> 32bit mode before handoff to the OS.
>
> Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=4234
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Yuanhao Xie <yuanhao.xie@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 21 ++++++++++++++++++---
>  1 file changed, 18 insertions(+), 3 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> index acbbf155c0..e4c42e34ce 100644
> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c
> @@ -480,6 +480,7 @@ InitMpGlobalData (
>    EFI_GCD_MEMORY_SPACE_DESCRIPTOR  MemDesc;
>    UINTN                            StackBase;
>    CPU_INFO_IN_HOB                  *CpuInfoInHob;
> +  EFI_PHYSICAL_ADDRESS             Address;
>
>    SaveCpuMpData (CpuMpData);
>
> @@ -561,13 +562,25 @@ InitMpGlobalData (
>    ASSERT ((AP_SAFE_STACK_SIZE & (CPU_STACK_ALIGNMENT - 1)) == 0);
>
>    mReservedApLoopFunc = (VOID *)(mReservedTopOfApStack + CpuMpData->CpuCount * AP_SAFE_STACK_SIZE);
> -  if (StandardSignatureIsAuthenticAMD ()) {
> +  if (StandardSignatureIsAuthenticAMD () && (sizeof (UINTN) == sizeof (UINT64))) {
> +    Address = BASE_4GB - 1;
> +    Status  = gBS->AllocatePages (
> +                     AllocateMaxAddress,
> +                     EfiReservedMemoryType,
> +                     EFI_SIZE_TO_PAGES (ApSafeBufferSize),
> +                     &Address
> +                     );
> +    ASSERT_EFI_ERROR (Status);
> +    mReservedApLoopFunc = (VOID *)((UINTN)Address + CpuMpData->CpuCount * AP_SAFE_STACK_SIZE);
>      CopyMem (
>        mReservedApLoopFunc,
>        CpuMpData->AddressMap.RelocateApLoopFuncAddressAmd,
>        CpuMpData->AddressMap.RelocateApLoopFuncSizeAmd
>        );
>    } else {
> +    Address = (UINTN)AllocateReservedPages (EFI_SIZE_TO_PAGES (ApSafeBufferSize));
> +    ASSERT (Address != 0);
> +    mReservedApLoopFunc = (VOID *)((UINTN)Address + CpuMpData->CpuCount * AP_SAFE_STACK_SIZE);
>      CopyMem (
>        mReservedApLoopFunc,
>        CpuMpData->AddressMap.RelocateApLoopFuncAddress,
> @@ -575,12 +588,14 @@ InitMpGlobalData (
>        );
>
>      mApPageTable = CreatePageTable (
> -                     mReservedTopOfApStack,
> +                     (UINTN)Address,
>                       ApSafeBufferSize
>                       );
>    }
>
> -  mReservedTopOfApStack += CpuMpData->CpuCount * AP_SAFE_STACK_SIZE;
> +  mReservedTopOfApStack = (UINTN)Address + CpuMpData->CpuCount * AP_SAFE_STACK_SIZE;
> +  ASSERT ((mReservedTopOfApStack & (UINTN)(CPU_STACK_ALIGNMENT - 1)) == 0);
> +  ASSERT ((AP_SAFE_STACK_SIZE & (CPU_STACK_ALIGNMENT - 1)) == 0);
>
>    Status = gBS->CreateEvent (
>                    EVT_TIMER | EVT_NOTIFY_SIGNAL,

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

The code remains in a bad shape. Please see my comments here (all three
links point to the same message):

  https://listman.redhat.com/archives/edk2-devel-archive/2023-January/057365.html
  https://edk2.groups.io/g/devel/message/98067
  http://mid.mail-archive.com/ad89a4bb-12b5-020f-8b83-b7833dfcd0d3@redhat.com

Please revert the original series, reimplement it in minimal steps, and
always check for opportunities to clean up the existent code before you
try to add new functionality. Just to name one example, calling
AllocateReservedPages() on one branch and gBS->AllocatePages() on the
other branch is unjustifiable. Both can be expressed with
gBS->AllocatePages(AllocateMaxAddress), you just need to set the limit
properly on each branch. Avoid duplicating logic if you can express the
desired behavior with shared code, by abstracting *data differences*.

Laszlo


^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2023-01-06  6:48 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-06  3:11 [Patch V2 1/2] UefiCpuPkg:Fix stack offset mismatch in 32bit AsmRelocateApLoopStart Yuanhao Xie
2023-01-06  3:11 ` [Patch V2 2/2] UefiCpuPkg: Keep 4GB limitation of memory allocation Yuanhao Xie
2023-01-06  6:48   ` [edk2-devel] " Laszlo Ersek

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox