public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH] UefiCpuPkg: Enhance logic in InitializeMpExceptionStackSwitchHandlers
@ 2022-08-25  2:55 Zhiguang Liu
  2022-08-25  2:55 ` [PATCH] UefiCpuPkg/MpInitLib: Fix potential issue when IDT table is at above 4G Zhiguang Liu
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Zhiguang Liu @ 2022-08-25  2:55 UTC (permalink / raw)
  To: devel; +Cc: Zhiguang Liu, Eric Dong, Ray Ni, Rahul Kumar

Parallelly run the function to SeparateExceptionStacks for all CPUs and
allocate buffers together for better performance.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 UefiCpuPkg/CpuDxe/CpuMp.c      | 93 ++++++++++++++++++--------------
 UefiCpuPkg/CpuMpPei/CpuMpPei.c | 96 +++++++++++++++++++---------------
 2 files changed, 109 insertions(+), 80 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c
index f3ca813d2a..6362e03f1b 100644
--- a/UefiCpuPkg/CpuDxe/CpuMp.c
+++ b/UefiCpuPkg/CpuDxe/CpuMp.c
@@ -600,8 +600,9 @@ CollectBistDataFromHob (
 // Structure for InitializeSeparateExceptionStacks
 //
 typedef struct {
-  VOID     *Buffer;
-  UINTN    *BufferSize;
+  VOID          *Buffer;
+  UINTN         BufferSize;
+  EFI_STATUS    Status;
 } EXCEPTION_STACK_SWITCH_CONTEXT;
 
 /**
@@ -620,9 +621,13 @@ InitializeExceptionStackSwitchHandlers (
   )
 {
   EXCEPTION_STACK_SWITCH_CONTEXT  *SwitchStackData;
+  UINTN                           Index;
 
+  MpInitLibWhoAmI (&Index);
   SwitchStackData = (EXCEPTION_STACK_SWITCH_CONTEXT *)Buffer;
-  InitializeSeparateExceptionStacks (SwitchStackData->Buffer, SwitchStackData->BufferSize);
+  if ((SwitchStackData[Index].Status == EFI_NOT_STARTED) || (SwitchStackData[Index].Status == EFI_BUFFER_TOO_SMALL)) {
+    SwitchStackData[Index].Status = InitializeSeparateExceptionStacks (SwitchStackData[Index].Buffer, &SwitchStackData[Index].BufferSize);
+  }
 }
 
 /**
@@ -638,53 +643,63 @@ InitializeMpExceptionStackSwitchHandlers (
   )
 {
   UINTN                           Index;
-  UINTN                           Bsp;
-  EXCEPTION_STACK_SWITCH_CONTEXT  SwitchStackData;
+  EXCEPTION_STACK_SWITCH_CONTEXT  *SwitchStackData;
   UINTN                           BufferSize;
+  EFI_STATUS                      Status;
+  VOID                            *Buffer;
 
-  SwitchStackData.BufferSize = &BufferSize;
-  MpInitLibWhoAmI (&Bsp);
+  SwitchStackData = AllocateRuntimeZeroPool (mNumberOfProcessors * sizeof (EXCEPTION_STACK_SWITCH_CONTEXT));
 
   for (Index = 0; Index < mNumberOfProcessors; ++Index) {
-    SwitchStackData.Buffer = NULL;
-    BufferSize             = 0;
+    //
+    // Because the procedure may runs multiple times, use the status EFI_NOT_STARTED
+    // to indicate the procedure haven't been run yet.
+    //
+    SwitchStackData[Index].Status = EFI_NOT_STARTED;
+  }
+
+  Status = MpInitLibStartupAllCPUs (
+             InitializeExceptionStackSwitchHandlers,
+             0,
+             SwitchStackData
+             );
+  ASSERT_EFI_ERROR (Status);
 
-    if (Index == Bsp) {
-      InitializeExceptionStackSwitchHandlers (&SwitchStackData);
+  BufferSize = 0;
+  for (Index = 0; Index < mNumberOfProcessors; ++Index) {
+    if (SwitchStackData[Index].Status == EFI_BUFFER_TOO_SMALL) {
+      BufferSize += SwitchStackData[Index].BufferSize;
     } else {
-      //
-      // AP might need different buffer size from BSP.
-      //
-      MpInitLibStartupThisAP (InitializeExceptionStackSwitchHandlers, Index, NULL, 0, (VOID *)&SwitchStackData, NULL);
+      ASSERT (SwitchStackData[Index].Status == EFI_SUCCESS);
     }
+  }
 
-    if (BufferSize == 0) {
-      continue;
+  if (BufferSize != 0) {
+    Buffer     = AllocateRuntimeZeroPool (BufferSize);
+    BufferSize = 0;
+    for (Index = 0; Index < mNumberOfProcessors; ++Index) {
+      if (SwitchStackData[Index].Status == EFI_BUFFER_TOO_SMALL) {
+        SwitchStackData[Index].Buffer = (VOID *)(BufferSize + (UINTN)Buffer);
+        BufferSize                   += SwitchStackData[Index].BufferSize;
+        DEBUG ((
+          DEBUG_INFO,
+          "Buffer[cpu%lu] for InitializeExceptionStackSwitchHandlers: 0x%lX with size 0x%lX\n",
+          (UINT64)(UINTN)Index,
+          (UINT64)(UINTN)SwitchStackData[Index].Buffer,
+          (UINT64)(UINTN)SwitchStackData[Index].BufferSize
+          ));
+      }
     }
 
-    SwitchStackData.Buffer = AllocateRuntimeZeroPool (BufferSize);
-    ASSERT (SwitchStackData.Buffer != NULL);
-    DEBUG ((
-      DEBUG_INFO,
-      "Buffer[cpu%lu] for InitializeExceptionStackSwitchHandlers: 0x%lX with size 0x%x\n",
-      (UINT64)(UINTN)Index,
-      (UINT64)(UINTN)SwitchStackData.Buffer,
-      (UINT32)BufferSize
-      ));
-
-    if (Index == Bsp) {
-      InitializeExceptionStackSwitchHandlers (&SwitchStackData);
-    } else {
-      MpInitLibStartupThisAP (
-        InitializeExceptionStackSwitchHandlers,
-        Index,
-        NULL,
-        0,
-        (VOID *)&SwitchStackData,
-        NULL
-        );
-    }
+    Status = MpInitLibStartupAllCPUs (
+               InitializeExceptionStackSwitchHandlers,
+               0,
+               SwitchStackData
+               );
+    ASSERT_EFI_ERROR (Status);
   }
+
+  FreePool (SwitchStackData);
 }
 
 /**
diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
index c0be11d3ad..5e4a7a3dcf 100644
--- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
+++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
@@ -415,8 +415,9 @@ PeiWhoAmI (
 // Structure for InitializeSeparateExceptionStacks
 //
 typedef struct {
-  VOID     *Buffer;
-  UINTN    *BufferSize;
+  VOID          *Buffer;
+  UINTN         BufferSize;
+  EFI_STATUS    Status;
 } EXCEPTION_STACK_SWITCH_CONTEXT;
 
 /**
@@ -435,9 +436,13 @@ InitializeExceptionStackSwitchHandlers (
   )
 {
   EXCEPTION_STACK_SWITCH_CONTEXT  *SwitchStackData;
+  UINTN                           Index;
 
+  MpInitLibWhoAmI (&Index);
   SwitchStackData = (EXCEPTION_STACK_SWITCH_CONTEXT *)Buffer;
-  InitializeSeparateExceptionStacks (SwitchStackData->Buffer, SwitchStackData->BufferSize);
+  if ((SwitchStackData[Index].Status == EFI_NOT_STARTED) || (SwitchStackData[Index].Status == EFI_BUFFER_TOO_SMALL)) {
+    SwitchStackData[Index].Status = InitializeSeparateExceptionStacks (SwitchStackData[Index].Buffer, &SwitchStackData[Index].BufferSize);
+  }
 }
 
 /**
@@ -453,60 +458,69 @@ InitializeMpExceptionStackSwitchHandlers (
   )
 {
   UINTN                           Index;
-  UINTN                           Bsp;
-  EXCEPTION_STACK_SWITCH_CONTEXT  SwitchStackData;
-  UINTN                           BufferSize;
   UINTN                           NumberOfProcessors;
+  EXCEPTION_STACK_SWITCH_CONTEXT  *SwitchStackData;
+  UINTN                           BufferSize;
+  EFI_STATUS                      Status;
+  VOID                            *Buffer;
 
   if (!PcdGetBool (PcdCpuStackGuard)) {
     return;
   }
 
-  SwitchStackData.BufferSize = &BufferSize;
   MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
-  MpInitLibWhoAmI (&Bsp);
+  SwitchStackData = AllocateRuntimeZeroPool (NumberOfProcessors * sizeof (EXCEPTION_STACK_SWITCH_CONTEXT));
 
   for (Index = 0; Index < NumberOfProcessors; ++Index) {
-    SwitchStackData.Buffer = NULL;
-    BufferSize             = 0;
+    //
+    // Because the procedure may runs multiple times, use the status EFI_NOT_STARTED
+    // to indicate the procedure haven't been run yet.
+    //
+    SwitchStackData[Index].Status = EFI_NOT_STARTED;
+  }
 
-    if (Index == Bsp) {
-      InitializeExceptionStackSwitchHandlers (&SwitchStackData);
+  Status = MpInitLibStartupAllCPUs (
+             InitializeExceptionStackSwitchHandlers,
+             0,
+             SwitchStackData
+             );
+  ASSERT_EFI_ERROR (Status);
+
+  BufferSize = 0;
+  for (Index = 0; Index < NumberOfProcessors; ++Index) {
+    if (SwitchStackData[Index].Status == EFI_BUFFER_TOO_SMALL) {
+      BufferSize += SwitchStackData[Index].BufferSize;
     } else {
-      //
-      // AP might need different buffer size from BSP.
-      //
-      MpInitLibStartupThisAP (InitializeExceptionStackSwitchHandlers, Index, NULL, 0, (VOID *)&SwitchStackData, NULL);
+      ASSERT (SwitchStackData[Index].Status == EFI_SUCCESS);
     }
+  }
 
-    if (BufferSize == 0) {
-      continue;
+  if (BufferSize != 0) {
+    Buffer     = AllocateRuntimeZeroPool (BufferSize);
+    BufferSize = 0;
+    for (Index = 0; Index < NumberOfProcessors; ++Index) {
+      if (SwitchStackData[Index].Status == EFI_BUFFER_TOO_SMALL) {
+        SwitchStackData[Index].Buffer = (VOID *)(BufferSize + (UINTN)Buffer);
+        BufferSize                   += SwitchStackData[Index].BufferSize;
+        DEBUG ((
+          DEBUG_INFO,
+          "Buffer[cpu%lu] for InitializeExceptionStackSwitchHandlers: 0x%lX with size 0x%lX\n",
+          (UINT64)(UINTN)Index,
+          (UINT64)(UINTN)SwitchStackData[Index].Buffer,
+          (UINT64)(UINTN)SwitchStackData[Index].BufferSize
+          ));
+      }
     }
 
-    SwitchStackData.Buffer = AllocatePages (EFI_SIZE_TO_PAGES (BufferSize));
-    ASSERT (SwitchStackData.Buffer != NULL);
-    ZeroMem (SwitchStackData.Buffer, EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (BufferSize)));
-    DEBUG ((
-      DEBUG_INFO,
-      "Buffer[cpu%lu] for InitializeExceptionStackSwitchHandlers: 0x%lX with size 0x%x\n",
-      (UINT64)(UINTN)Index,
-      (UINT64)(UINTN)SwitchStackData.Buffer,
-      (UINT32)BufferSize
-      ));
-
-    if (Index == Bsp) {
-      InitializeExceptionStackSwitchHandlers (&SwitchStackData);
-    } else {
-      MpInitLibStartupThisAP (
-        InitializeExceptionStackSwitchHandlers,
-        Index,
-        NULL,
-        0,
-        (VOID *)&SwitchStackData,
-        NULL
-        );
-    }
+    Status = MpInitLibStartupAllCPUs (
+               InitializeExceptionStackSwitchHandlers,
+               0,
+               SwitchStackData
+               );
+    ASSERT_EFI_ERROR (Status);
   }
+
+  FreePool (SwitchStackData);
 }
 
 /**
-- 
2.31.1.windows.1


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

* [PATCH] UefiCpuPkg/MpInitLib: Fix potential issue when IDT table is at above 4G
  2022-08-25  2:55 [PATCH] UefiCpuPkg: Enhance logic in InitializeMpExceptionStackSwitchHandlers Zhiguang Liu
@ 2022-08-25  2:55 ` Zhiguang Liu
  2022-08-25  9:32   ` Ni, Ray
  2022-08-25  2:55 ` [PATCH] UefiCpuPkg/MpInitLib: Simplify logic in SwitchBsp Zhiguang Liu
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 7+ messages in thread
From: Zhiguang Liu @ 2022-08-25  2:55 UTC (permalink / raw)
  To: devel; +Cc: Zhiguang Liu, Eric Dong, Ray Ni, Rahul Kumar

Currently, when waking up AP, IDT table of AP will be set in 16 bit code,
and assume the IDT table base is 32 bit. However, the IDT table is created
by BSP. Issue will happen if the BSP allocates memory above 4G for BSP's
IDT table. Moreover, even the IDT table location is below 4G, the handler
function inside the IDT table is 64 bit, and it won't take effect until
CPU transfers to 64 bit long mode. There is no benefit to set IDT table in
such an early phase.
To avoid such issue, this patch moves the LIDT instruction into 64 bit
code.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index 1daaa72b1e..cd95b03da8 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -64,9 +64,6 @@ BITS 16
     mov        si, MP_CPU_EXCHANGE_INFO_FIELD (GdtrProfile)
 o32 lgdt       [cs:si]
 
-    mov        si, MP_CPU_EXCHANGE_INFO_FIELD (IdtrProfile)
-o32 lidt       [cs:si]
-
     ;
     ; Switch to protected mode
     ;
@@ -154,6 +151,11 @@ BITS 64
 
 LongModeStart:
     mov        esi, ebx
+
+    ; Set IDT table at the start of 64 bit code
+    lea        edi, [esi + MP_CPU_EXCHANGE_INFO_FIELD (IdtrProfile)]
+    lidt       [edi]
+
     lea        edi, [esi + MP_CPU_EXCHANGE_INFO_FIELD (InitFlag)]
     cmp        qword [edi], 1       ; ApInitConfig
     jnz        GetApicId
-- 
2.31.1.windows.1


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

* [PATCH] UefiCpuPkg/MpInitLib: Simplify logic in SwitchBsp
  2022-08-25  2:55 [PATCH] UefiCpuPkg: Enhance logic in InitializeMpExceptionStackSwitchHandlers Zhiguang Liu
  2022-08-25  2:55 ` [PATCH] UefiCpuPkg/MpInitLib: Fix potential issue when IDT table is at above 4G Zhiguang Liu
@ 2022-08-25  2:55 ` Zhiguang Liu
  2022-08-25  9:18   ` Ni, Ray
  2022-08-25  2:55 ` [PATCH] UefiCpuPkg: Simplify the implementation when separate exception stacks Zhiguang Liu
  2022-08-25  9:32 ` [edk2-devel] [PATCH] UefiCpuPkg: Enhance logic in InitializeMpExceptionStackSwitchHandlers Ni, Ray
  3 siblings, 1 reply; 7+ messages in thread
From: Zhiguang Liu @ 2022-08-25  2:55 UTC (permalink / raw)
  To: devel; +Cc: Zhiguang Liu, Eric Dong, Ray Ni, Rahul Kumar

When switch bsp, old bsp and new bsp put CR0/CR4 into stack, and put IDT
and GDT register into a structure. After they exchange their stack, they
restore these registers. This logic is now implemented by assembly code.
This patch aims to reuse (Save/Restore)VolatileRegisters function to
replace such assembly code for better code readability.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 .../Library/MpInitLib/Ia32/MpFuncs.nasm       | 18 --------
 UefiCpuPkg/Library/MpInitLib/MpLib.c          | 35 ++++++++++++++-
 UefiCpuPkg/Library/MpInitLib/MpLib.h          | 43 +++++++++----------
 UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 21 ---------
 4 files changed, 55 insertions(+), 62 deletions(-)

diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
index 28301bb8f0..7a4d5b35db 100644
--- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm
@@ -286,13 +286,6 @@ ASM_PFX(AsmExchangeRole):
 
     ;Store EFLAGS, GDTR and IDTR register to stack
     pushfd
-    mov        eax, cr4
-    push       eax       ; push cr4 firstly
-    mov        eax, cr0
-    push       eax
-
-    sgdt       [esi + CPU_EXCHANGE_ROLE_INFO.Gdtr]
-    sidt       [esi + CPU_EXCHANGE_ROLE_INFO.Idtr]
 
     ; Store the its StackPointer
     mov        [esi + CPU_EXCHANGE_ROLE_INFO.StackPointer],esp
@@ -308,13 +301,6 @@ WaitForOtherStored:
     jmp        WaitForOtherStored
 
 OtherStored:
-    ; Since another CPU already stored its state, load them
-    ; load GDTR value
-    lgdt       [edi + CPU_EXCHANGE_ROLE_INFO.Gdtr]
-
-    ; load IDTR value
-    lidt       [edi + CPU_EXCHANGE_ROLE_INFO.Idtr]
-
     ; load its future StackPointer
     mov        esp, [edi + CPU_EXCHANGE_ROLE_INFO.StackPointer]
 
@@ -331,10 +317,6 @@ WaitForOtherLoaded:
 
 OtherLoaded:
     ; since the other CPU already get the data it want, leave this procedure
-    pop        eax
-    mov        cr0, eax
-    pop        eax
-    mov        cr4, eax
     popfd
 
     popad
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 8d1f24370a..041a32e659 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -1,7 +1,7 @@
 /** @file
   CPU MP Initialize Library common functions.
 
-  Copyright (c) 2016 - 2021, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2016 - 2022, Intel Corporation. All rights reserved.<BR>
   Copyright (c) 2020, AMD Inc. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -15,6 +15,29 @@
 
 EFI_GUID  mCpuInitMpLibHobGuid = CPU_INIT_MP_LIB_HOB_GUID;
 
+/**
+  Save the volatile registers required to be restored following INIT IPI.
+
+  @param[out]  VolatileRegisters    Returns buffer saved the volatile resisters
+**/
+VOID
+SaveVolatileRegisters (
+  OUT CPU_VOLATILE_REGISTERS  *VolatileRegisters
+  );
+
+/**
+  Restore the volatile registers following INIT IPI.
+
+  @param[in]  VolatileRegisters   Pointer to volatile resisters
+  @param[in]  IsRestoreDr         TRUE:  Restore DRx if supported
+                                  FALSE: Do not restore DRx
+**/
+VOID
+RestoreVolatileRegisters (
+  IN CPU_VOLATILE_REGISTERS  *VolatileRegisters,
+  IN BOOLEAN                 IsRestoreDr
+  );
+
 /**
   The function will check if BSP Execute Disable is enabled.
 
@@ -83,7 +106,12 @@ FutureBSPProc (
   CPU_MP_DATA  *DataInHob;
 
   DataInHob = (CPU_MP_DATA *)Buffer;
+  //
+  // Save and restore volatile registers when switch BSP
+  //
+  SaveVolatileRegisters (&DataInHob->APInfo.VolatileRegisters);
   AsmExchangeRole (&DataInHob->APInfo, &DataInHob->BSPInfo);
+  RestoreVolatileRegisters (&DataInHob->APInfo.VolatileRegisters, FALSE);
 }
 
 /**
@@ -2233,7 +2261,12 @@ SwitchBSPWorker (
   //
   WakeUpAP (CpuMpData, FALSE, ProcessorNumber, FutureBSPProc, CpuMpData, TRUE);
 
+  //
+  // Save and restore volatile registers when switch BSP
+  //
+  SaveVolatileRegisters (&CpuMpData->BSPInfo.VolatileRegisters);
   AsmExchangeRole (&CpuMpData->BSPInfo, &CpuMpData->APInfo);
+  RestoreVolatileRegisters (&CpuMpData->BSPInfo.VolatileRegisters, FALSE);
 
   //
   // Set the BSP bit of MSR_IA32_APIC_BASE on new BSP
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h
index 974fb76019..47b722cb2f 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.h
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h
@@ -68,14 +68,31 @@ typedef struct {
   UINTN    Size;
 } MICROCODE_PATCH_INFO;
 
+//
+// CPU volatile registers around INIT-SIPI-SIPI
+//
+typedef struct {
+  UINTN              Cr0;
+  UINTN              Cr3;
+  UINTN              Cr4;
+  UINTN              Dr0;
+  UINTN              Dr1;
+  UINTN              Dr2;
+  UINTN              Dr3;
+  UINTN              Dr6;
+  UINTN              Dr7;
+  IA32_DESCRIPTOR    Gdtr;
+  IA32_DESCRIPTOR    Idtr;
+  UINT16             Tr;
+} CPU_VOLATILE_REGISTERS;
+
 //
 // CPU exchange information for switch BSP
 //
 typedef struct {
-  UINT8              State;        // offset 0
-  UINTN              StackPointer; // offset 4 / 8
-  IA32_DESCRIPTOR    Gdtr;         // offset 8 / 16
-  IA32_DESCRIPTOR    Idtr;         // offset 14 / 26
+  UINT8                     State;             // offset 0
+  UINTN                     StackPointer;      // offset 4 / 8
+  CPU_VOLATILE_REGISTERS    VolatileRegisters; // offset 8 / 16
 } CPU_EXCHANGE_ROLE_INFO;
 
 //
@@ -112,24 +129,6 @@ typedef enum {
   CpuStateDisabled
 } CPU_STATE;
 
-//
-// CPU volatile registers around INIT-SIPI-SIPI
-//
-typedef struct {
-  UINTN              Cr0;
-  UINTN              Cr3;
-  UINTN              Cr4;
-  UINTN              Dr0;
-  UINTN              Dr1;
-  UINTN              Dr2;
-  UINTN              Dr3;
-  UINTN              Dr6;
-  UINTN              Dr7;
-  IA32_DESCRIPTOR    Gdtr;
-  IA32_DESCRIPTOR    Idtr;
-  UINT16             Tr;
-} CPU_VOLATILE_REGISTERS;
-
 //
 // AP related data
 //
diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
index cd95b03da8..b7f8d48504 100644
--- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
+++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
@@ -482,22 +482,13 @@ ASM_PFX(AsmExchangeRole):
     push       r14
     push       r15
 
-    mov        rax, cr0
-    push       rax
-
-    mov        rax, cr4
-    push       rax
-
     ; rsi contains MyInfo pointer
     mov        rsi, rcx
 
     ; rdi contains OthersInfo pointer
     mov        rdi, rdx
 
-    ;Store EFLAGS, GDTR and IDTR regiter to stack
     pushfq
-    sgdt       [rsi + CPU_EXCHANGE_ROLE_INFO.Gdtr]
-    sidt       [rsi + CPU_EXCHANGE_ROLE_INFO.Idtr]
 
     ; Store the its StackPointer
     mov        [rsi + CPU_EXCHANGE_ROLE_INFO.StackPointer], rsp
@@ -513,12 +504,6 @@ WaitForOtherStored:
     jmp        WaitForOtherStored
 
 OtherStored:
-    ; Since another CPU already stored its state, load them
-    ; load GDTR value
-    lgdt       [rdi + CPU_EXCHANGE_ROLE_INFO.Gdtr]
-
-    ; load IDTR value
-    lidt       [rdi + CPU_EXCHANGE_ROLE_INFO.Idtr]
 
     ; load its future StackPointer
     mov        rsp, [rdi + CPU_EXCHANGE_ROLE_INFO.StackPointer]
@@ -538,12 +523,6 @@ OtherLoaded:
     ; since the other CPU already get the data it want, leave this procedure
     popfq
 
-    pop        rax
-    mov        cr4, rax
-
-    pop        rax
-    mov        cr0, rax
-
     pop        r15
     pop        r14
     pop        r13
-- 
2.31.1.windows.1


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

* [PATCH] UefiCpuPkg: Simplify the implementation when separate exception stacks
  2022-08-25  2:55 [PATCH] UefiCpuPkg: Enhance logic in InitializeMpExceptionStackSwitchHandlers Zhiguang Liu
  2022-08-25  2:55 ` [PATCH] UefiCpuPkg/MpInitLib: Fix potential issue when IDT table is at above 4G Zhiguang Liu
  2022-08-25  2:55 ` [PATCH] UefiCpuPkg/MpInitLib: Simplify logic in SwitchBsp Zhiguang Liu
@ 2022-08-25  2:55 ` Zhiguang Liu
  2022-08-25  9:32 ` [edk2-devel] [PATCH] UefiCpuPkg: Enhance logic in InitializeMpExceptionStackSwitchHandlers Ni, Ray
  3 siblings, 0 replies; 7+ messages in thread
From: Zhiguang Liu @ 2022-08-25  2:55 UTC (permalink / raw)
  To: devel; +Cc: Zhiguang Liu, Eric Dong, Ray Ni, Rahul Kumar

The API of InitializeSeparateExceptionStacks is just changed before, and
makes the struct CPU_EXCEPTION_INIT_DATA an internal definition.
Furthermore, we can even remove the struct to make core simpler.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rahul Kumar <rahul1.kumar@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 .../CpuExceptionCommon.h                      |  73 ++-------
 .../CpuExceptionHandlerLib/DxeException.c     |  94 ++---------
 .../Ia32/ArchExceptionHandler.c               | 148 ++++++++----------
 .../CpuExceptionHandlerLib/PeiCpuException.c  |  77 +--------
 .../SecPeiCpuExceptionHandlerLib.inf          |   7 +-
 .../SmmCpuExceptionHandlerLib.inf             |   7 +-
 .../X64/ArchExceptionHandler.c                | 147 ++++++++---------
 .../Xcode5SecPeiCpuExceptionHandlerLib.inf    |   7 +-
 8 files changed, 177 insertions(+), 383 deletions(-)

diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
index 11a5624f51..4593c204a6 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
@@ -49,61 +49,6 @@
 
 #define CPU_TSS_GDT_SIZE  (SIZE_2KB + CPU_TSS_DESC_SIZE + CPU_TSS_SIZE)
 
-typedef struct {
-  //
-  // The address of top of known good stack reserved for *ALL* exceptions
-  // listed in field StackSwitchExceptions.
-  //
-  UINTN    KnownGoodStackTop;
-  //
-  // The size of known good stack for *ONE* exception only.
-  //
-  UINTN    KnownGoodStackSize;
-  //
-  // Buffer of exception vector list for stack switch.
-  //
-  UINT8    *StackSwitchExceptions;
-  //
-  // Number of exception vectors in StackSwitchExceptions.
-  //
-  UINTN    StackSwitchExceptionNumber;
-  //
-  // Buffer of IDT table. It must be type of IA32_IDT_GATE_DESCRIPTOR.
-  // Normally there's no need to change IDT table size.
-  //
-  VOID     *IdtTable;
-  //
-  // Size of buffer for IdtTable.
-  //
-  UINTN    IdtTableSize;
-  //
-  // Buffer of GDT table. It must be type of IA32_SEGMENT_DESCRIPTOR.
-  //
-  VOID     *GdtTable;
-  //
-  // Size of buffer for GdtTable.
-  //
-  UINTN    GdtTableSize;
-  //
-  // Pointer to start address of descriptor of exception task gate in the
-  // GDT table. It must be type of IA32_TSS_DESCRIPTOR.
-  //
-  VOID     *ExceptionTssDesc;
-  //
-  // Size of buffer for ExceptionTssDesc.
-  //
-  UINTN    ExceptionTssDescSize;
-  //
-  // Buffer of task-state segment for exceptions. It must be type of
-  // IA32_TASK_STATE_SEGMENT.
-  //
-  VOID     *ExceptionTss;
-  //
-  // Size of buffer for ExceptionTss.
-  //
-  UINTN    ExceptionTssSize;
-} CPU_EXCEPTION_INIT_DATA;
-
 //
 // Record exception handler information
 //
@@ -345,18 +290,22 @@ CommonExceptionHandlerWorker (
   );
 
 /**
-  Setup separate stack for specific exceptions.
+  Setup separate stacks for certain exception handlers.
 
-  @param[in] StackSwitchData      Pointer to data required for setuping up
-                                  stack switch.
+  @param[in]       Buffer        Point to buffer used to separate exception stack.
+  @param[in, out]  BufferSize    On input, it indicates the byte size of Buffer.
+                                 If the size is not enough, the return status will
+                                 be EFI_BUFFER_TOO_SMALL, and output BufferSize
+                                 will be the size it needs.
 
-  @retval EFI_SUCCESS             The exceptions have been successfully
-                                  initialized with new stack.
-  @retval EFI_INVALID_PARAMETER   StackSwitchData contains invalid content.
+  @retval EFI_SUCCESS             The stacks are assigned successfully.
+  @retval EFI_BUFFER_TOO_SMALL    This BufferSize is too small.
+  @retval EFI_UNSUPPORTED         This function is not supported.
 **/
 EFI_STATUS
 ArchSetupExceptionStack (
-  IN CPU_EXCEPTION_INIT_DATA  *StackSwitchData
+  IN     VOID   *Buffer,
+  IN OUT UINTN  *BufferSize
   );
 
 /**
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
index d90c607bd7..076382a9b5 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeException.c
@@ -23,9 +23,8 @@ EXCEPTION_HANDLER_DATA     mExceptionHandlerData = {
   mExternalInterruptHandlerTable
 };
 
-UINT8  mNewStack[CPU_STACK_SWITCH_EXCEPTION_NUMBER *
-                 CPU_KNOWN_GOOD_STACK_SIZE];
-UINT8  mNewGdt[CPU_TSS_GDT_SIZE];
+UINT8  mBuffer[CPU_STACK_SWITCH_EXCEPTION_NUMBER * CPU_KNOWN_GOOD_STACK_SIZE
+               + CPU_TSS_GDT_SIZE];
 
 /**
   Common exception handler.
@@ -123,85 +122,18 @@ InitializeSeparateExceptionStacks (
   IN OUT UINTN  *BufferSize
   )
 {
-  CPU_EXCEPTION_INIT_DATA  EssData;
-  IA32_DESCRIPTOR          Idtr;
-  IA32_DESCRIPTOR          Gdtr;
-  UINTN                    NeedBufferSize;
-  UINTN                    StackTop;
-  UINT8                    *NewGdtTable;
-
-  //
-  // X64 needs only one TSS of current task working for all exceptions
-  // because of its IST feature. IA32 needs one TSS for each exception
-  // in addition to current task. To simplify the code, we report the
-  // needed memory for IA32 case to cover both IA32 and X64 exception
-  // stack switch.
-  //
-  // Layout of memory needed for each processor:
-  //    --------------------------------
-  //    |            Alignment         |  (just in case)
-  //    --------------------------------
-  //    |                              |
-  //    |        Original GDT          |
-  //    |                              |
-  //    --------------------------------
-  //    |    Current task descriptor   |
-  //    --------------------------------
-  //    |                              |
-  //    |  Exception task descriptors  |  X ExceptionNumber
-  //    |                              |
-  //    --------------------------------
-  //    |  Current task-state segment  |
-  //    --------------------------------
-  //    |                              |
-  //    | Exception task-state segment |  X ExceptionNumber
-  //    |                              |
-  //    --------------------------------
-  //
-  AsmReadGdtr (&Gdtr);
+  VOID        *LocalBuffer;
+  UINTN       LocalBufferSize;
+  EFI_STATUS  Status;
+
   if ((Buffer == NULL) && (BufferSize == NULL)) {
-    SetMem (mNewGdt, sizeof (mNewGdt), 0);
-    StackTop    = (UINTN)mNewStack + sizeof (mNewStack);
-    NewGdtTable = mNewGdt;
+    SetMem (mBuffer, sizeof (mBuffer), 0);
+    LocalBuffer     = mBuffer;
+    LocalBufferSize = sizeof (mBuffer);
+    Status          = ArchSetupExceptionStack (LocalBuffer, &LocalBufferSize);
+    ASSERT_EFI_ERROR (Status);
+    return Status;
   } else {
-    if (BufferSize == NULL) {
-      return EFI_INVALID_PARAMETER;
-    }
-
-    //
-    // Total needed size includes stack size, new GDT table size, TSS size.
-    // Add another DESCRIPTOR size for alignment requiremet.
-    //
-    NeedBufferSize = CPU_STACK_SWITCH_EXCEPTION_NUMBER * CPU_KNOWN_GOOD_STACK_SIZE +
-                     CPU_TSS_DESC_SIZE + Gdtr.Limit + 1 +
-                     CPU_TSS_SIZE +
-                     sizeof (IA32_TSS_DESCRIPTOR);
-    if (*BufferSize < NeedBufferSize) {
-      *BufferSize = NeedBufferSize;
-      return EFI_BUFFER_TOO_SMALL;
-    }
-
-    if (Buffer == NULL) {
-      return EFI_INVALID_PARAMETER;
-    }
-
-    StackTop    = (UINTN)Buffer + CPU_STACK_SWITCH_EXCEPTION_NUMBER * CPU_KNOWN_GOOD_STACK_SIZE;
-    NewGdtTable = ALIGN_POINTER (StackTop, sizeof (IA32_TSS_DESCRIPTOR));
+    return ArchSetupExceptionStack (Buffer, BufferSize);
   }
-
-  AsmReadIdtr (&Idtr);
-  EssData.KnownGoodStackTop          = StackTop;
-  EssData.KnownGoodStackSize         = CPU_KNOWN_GOOD_STACK_SIZE;
-  EssData.StackSwitchExceptions      = CPU_STACK_SWITCH_EXCEPTION_LIST;
-  EssData.StackSwitchExceptionNumber = CPU_STACK_SWITCH_EXCEPTION_NUMBER;
-  EssData.IdtTable                   = (VOID *)Idtr.Base;
-  EssData.IdtTableSize               = Idtr.Limit + 1;
-  EssData.GdtTable                   = NewGdtTable;
-  EssData.GdtTableSize               = CPU_TSS_DESC_SIZE + Gdtr.Limit + 1;
-  EssData.ExceptionTssDesc           = NewGdtTable + Gdtr.Limit + 1;
-  EssData.ExceptionTssDescSize       = CPU_TSS_DESC_SIZE;
-  EssData.ExceptionTss               = NewGdtTable + Gdtr.Limit + 1 + CPU_TSS_DESC_SIZE;
-  EssData.ExceptionTssSize           = CPU_TSS_SIZE;
-
-  return ArchSetupExceptionStack (&EssData);
 }
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
index 194d3a499b..8c398ebc5b 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Ia32/ArchExceptionHandler.c
@@ -104,108 +104,97 @@ ArchRestoreExceptionContext (
 }
 
 /**
-  Setup separate stack for given exceptions.
+  Setup separate stacks for certain exception handlers.
 
-  @param[in] StackSwitchData      Pointer to data required for setuping up
-                                  stack switch.
-
-  @retval EFI_SUCCESS             The exceptions have been successfully
-                                  initialized with new stack.
-  @retval EFI_INVALID_PARAMETER   StackSwitchData contains invalid content.
+  @param[in]       Buffer        Point to buffer used to separate exception stack.
+  @param[in, out]  BufferSize    On input, it indicates the byte size of Buffer.
+                                 If the size is not enough, the return status will
+                                 be EFI_BUFFER_TOO_SMALL, and output BufferSize
+                                 will be the size it needs.
 
+  @retval EFI_SUCCESS             The stacks are assigned successfully.
+  @retval EFI_BUFFER_TOO_SMALL    This BufferSize is too small.
 **/
 EFI_STATUS
 ArchSetupExceptionStack (
-  IN CPU_EXCEPTION_INIT_DATA  *StackSwitchData
+  IN     VOID   *Buffer,
+  IN OUT UINTN  *BufferSize
   )
 {
   IA32_DESCRIPTOR                 Gdtr;
   IA32_DESCRIPTOR                 Idtr;
   IA32_IDT_GATE_DESCRIPTOR        *IdtTable;
   IA32_TSS_DESCRIPTOR             *TssDesc;
+  IA32_TSS_DESCRIPTOR             *TssDescBase;
   IA32_TASK_STATE_SEGMENT         *Tss;
+  VOID                            *NewGdtTable;
   UINTN                           StackTop;
   UINTN                           Index;
   UINTN                           Vector;
   UINTN                           TssBase;
-  UINTN                           GdtSize;
+  UINT8                           *StackSwitchExceptions;
+  UINTN                           NeedBufferSize;
   EXCEPTION_HANDLER_TEMPLATE_MAP  TemplateMap;
 
-  if ((StackSwitchData == NULL) ||
-      (StackSwitchData->KnownGoodStackTop == 0) ||
-      (StackSwitchData->KnownGoodStackSize == 0) ||
-      (StackSwitchData->StackSwitchExceptions == NULL) ||
-      (StackSwitchData->StackSwitchExceptionNumber == 0) ||
-      (StackSwitchData->StackSwitchExceptionNumber > CPU_EXCEPTION_NUM) ||
-      (StackSwitchData->GdtTable == NULL) ||
-      (StackSwitchData->IdtTable == NULL) ||
-      (StackSwitchData->ExceptionTssDesc == NULL) ||
-      (StackSwitchData->ExceptionTss == NULL))
-  {
+  if (BufferSize == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
   //
-  // The caller is responsible for that the GDT table, no matter the existing
-  // one or newly allocated, has enough space to hold descriptors for exception
-  // task-state segments.
+  // Total needed size includes stack size, new GDT table size, TSS size.
+  // Add another DESCRIPTOR size for alignment requiremet.
   //
-  if (((UINTN)StackSwitchData->GdtTable & (IA32_GDT_ALIGNMENT - 1)) != 0) {
-    return EFI_INVALID_PARAMETER;
-  }
-
-  if ((UINTN)StackSwitchData->ExceptionTssDesc < (UINTN)(StackSwitchData->GdtTable)) {
-    return EFI_INVALID_PARAMETER;
-  }
-
-  if ((UINTN)StackSwitchData->ExceptionTssDesc + StackSwitchData->ExceptionTssDescSize >
-      ((UINTN)(StackSwitchData->GdtTable) + StackSwitchData->GdtTableSize))
-  {
-    return EFI_INVALID_PARAMETER;
-  }
-
+  // Layout of memory needed for each processor:
+  //    --------------------------------
+  //    |                              |
+  //    |          Stack Size          |  X ExceptionNumber
+  //    |                              |
+  //    --------------------------------
+  //    |          Alignment           |  (just in case)
+  //    --------------------------------
+  //    |                              |
+  //    |         Original GDT         |
+  //    |                              |
+  //    --------------------------------
+  //    |    Current task descriptor   |
+  //    --------------------------------
+  //    |                              |
+  //    |  Exception task descriptors  |  X ExceptionNumber
+  //    |                              |
+  //    --------------------------------
+  //    |  Current task-state segment  |
+  //    --------------------------------
+  //    |                              |
+  //    | Exception task-state segment |  X ExceptionNumber
+  //    |                              |
+  //    --------------------------------
   //
-  // We need one descriptor and one TSS for current task and every exception
-  // specified.
-  //
-  if (StackSwitchData->ExceptionTssDescSize <
-      sizeof (IA32_TSS_DESCRIPTOR) * (StackSwitchData->StackSwitchExceptionNumber + 1))
-  {
-    return EFI_INVALID_PARAMETER;
+  AsmReadGdtr (&Gdtr);
+  NeedBufferSize = CPU_STACK_SWITCH_EXCEPTION_NUMBER * CPU_KNOWN_GOOD_STACK_SIZE +
+                   sizeof (IA32_TSS_DESCRIPTOR) +
+                   Gdtr.Limit + 1 + CPU_TSS_DESC_SIZE +
+                   CPU_TSS_SIZE;
+
+  if (*BufferSize < NeedBufferSize) {
+    *BufferSize = NeedBufferSize;
+    return EFI_BUFFER_TOO_SMALL;
   }
 
-  if (StackSwitchData->ExceptionTssSize <
-      sizeof (IA32_TASK_STATE_SEGMENT) * (StackSwitchData->StackSwitchExceptionNumber + 1))
-  {
+  if (Buffer == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
-  TssDesc = StackSwitchData->ExceptionTssDesc;
-  Tss     = StackSwitchData->ExceptionTss;
-
-  //
-  // Initialize new GDT table and/or IDT table, if any
-  //
   AsmReadIdtr (&Idtr);
-  AsmReadGdtr (&Gdtr);
-
-  GdtSize = (UINTN)TssDesc +
-            sizeof (IA32_TSS_DESCRIPTOR) *
-            (StackSwitchData->StackSwitchExceptionNumber + 1) -
-            (UINTN)(StackSwitchData->GdtTable);
-  if ((UINTN)StackSwitchData->GdtTable != Gdtr.Base) {
-    CopyMem (StackSwitchData->GdtTable, (VOID *)Gdtr.Base, Gdtr.Limit + 1);
-    Gdtr.Base  = (UINTN)StackSwitchData->GdtTable;
-    Gdtr.Limit = (UINT16)GdtSize - 1;
-  }
-
-  if ((UINTN)StackSwitchData->IdtTable != Idtr.Base) {
-    Idtr.Base = (UINTN)StackSwitchData->IdtTable;
-  }
+  StackSwitchExceptions = CPU_STACK_SWITCH_EXCEPTION_LIST;
+  StackTop              = (UINTN)Buffer + CPU_STACK_SWITCH_EXCEPTION_NUMBER * CPU_KNOWN_GOOD_STACK_SIZE;
+  NewGdtTable           = ALIGN_POINTER (StackTop, sizeof (IA32_TSS_DESCRIPTOR));
+  TssDesc               = (IA32_TSS_DESCRIPTOR *)((UINTN)NewGdtTable + Gdtr.Limit + 1);
+  Tss                   = (IA32_TASK_STATE_SEGMENT *)((UINTN)TssDesc + CPU_TSS_DESC_SIZE);
+  TssDescBase           = TssDesc;
 
-  if (StackSwitchData->IdtTableSize > 0) {
-    Idtr.Limit = (UINT16)(StackSwitchData->IdtTableSize - 1);
-  }
+  CopyMem (NewGdtTable, (VOID *)Gdtr.Base, Gdtr.Limit + 1);
+  Gdtr.Base  = (UINTN)NewGdtTable;
+  Gdtr.Limit = (UINT16)(Gdtr.Limit + CPU_TSS_DESC_SIZE);
 
   //
   // Fixup current task descriptor. Task-state segment for current task will
@@ -226,10 +215,10 @@ ArchSetupExceptionStack (
   // Fixup exception task descriptor and task-state segment
   //
   AsmGetTssTemplateMap (&TemplateMap);
-  StackTop = StackSwitchData->KnownGoodStackTop - CPU_STACK_ALIGNMENT;
+  StackTop = StackTop - CPU_STACK_ALIGNMENT;
   StackTop = (UINTN)ALIGN_POINTER (StackTop, CPU_STACK_ALIGNMENT);
-  IdtTable = StackSwitchData->IdtTable;
-  for (Index = 0; Index < StackSwitchData->StackSwitchExceptionNumber; ++Index) {
+  IdtTable = (IA32_IDT_GATE_DESCRIPTOR  *)Idtr.Base;
+  for (Index = 0; Index < CPU_STACK_SWITCH_EXCEPTION_NUMBER; ++Index) {
     TssDesc += 1;
     Tss     += 1;
 
@@ -250,7 +239,7 @@ ArchSetupExceptionStack (
     //
     // Fixup TSS
     //
-    Vector = StackSwitchData->StackSwitchExceptions[Index];
+    Vector = StackSwitchExceptions[Index];
     if ((Vector >= CPU_EXCEPTION_NUM) ||
         (Vector >= (Idtr.Limit + 1) / sizeof (IA32_IDT_GATE_DESCRIPTOR)))
     {
@@ -270,7 +259,7 @@ ArchSetupExceptionStack (
     Tss->FS     = AsmReadFs ();
     Tss->GS     = AsmReadGs ();
 
-    StackTop -= StackSwitchData->KnownGoodStackSize;
+    StackTop -= CPU_KNOWN_GOOD_STACK_SIZE;
 
     //
     // Update IDT to use Task Gate for given exception
@@ -290,12 +279,7 @@ ArchSetupExceptionStack (
   //
   // Load current task
   //
-  AsmWriteTr ((UINT16)((UINTN)StackSwitchData->ExceptionTssDesc - Gdtr.Base));
-
-  //
-  // Publish IDT
-  //
-  AsmWriteIdtr (&Idtr);
+  AsmWriteTr ((UINT16)((UINTN)TssDescBase - Gdtr.Base));
 
   return EFI_SUCCESS;
 }
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuException.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuException.c
index 5952295126..940d83a92f 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuException.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuException.c
@@ -170,84 +170,9 @@ InitializeSeparateExceptionStacks (
   IN OUT UINTN  *BufferSize
   )
 {
-  CPU_EXCEPTION_INIT_DATA  EssData;
-  IA32_DESCRIPTOR          Idtr;
-  IA32_DESCRIPTOR          Gdtr;
-  UINTN                    NeedBufferSize;
-  UINTN                    StackTop;
-  UINT8                    *NewGdtTable;
-
-  //
-  // X64 needs only one TSS of current task working for all exceptions
-  // because of its IST feature. IA32 needs one TSS for each exception
-  // in addition to current task. To simplify the code, we report the
-  // needed memory for IA32 case to cover both IA32 and X64 exception
-  // stack switch.
-  //
-  // Layout of memory needed for each processor:
-  //    --------------------------------
-  //    |            Alignment         |  (just in case)
-  //    --------------------------------
-  //    |                              |
-  //    |        Original GDT          |
-  //    |                              |
-  //    --------------------------------
-  //    |    Current task descriptor   |
-  //    --------------------------------
-  //    |                              |
-  //    |  Exception task descriptors  |  X ExceptionNumber
-  //    |                              |
-  //    --------------------------------
-  //    |  Current task-state segment  |
-  //    --------------------------------
-  //    |                              |
-  //    | Exception task-state segment |  X ExceptionNumber
-  //    |                              |
-  //    --------------------------------
-  //
-
   if ((Buffer == NULL) && (BufferSize == NULL)) {
     return EFI_UNSUPPORTED;
   }
 
-  if (BufferSize == NULL) {
-    return EFI_INVALID_PARAMETER;
-  }
-
-  AsmReadGdtr (&Gdtr);
-  //
-  // Total needed size includes stack size, new GDT table size, TSS size.
-  // Add another DESCRIPTOR size for alignment requiremet.
-  //
-  NeedBufferSize = CPU_STACK_SWITCH_EXCEPTION_NUMBER * CPU_KNOWN_GOOD_STACK_SIZE +
-                   CPU_TSS_DESC_SIZE + Gdtr.Limit + 1 +
-                   CPU_TSS_SIZE +
-                   sizeof (IA32_TSS_DESCRIPTOR);
-  if (*BufferSize < NeedBufferSize) {
-    *BufferSize = NeedBufferSize;
-    return EFI_BUFFER_TOO_SMALL;
-  }
-
-  if (Buffer == NULL) {
-    return EFI_INVALID_PARAMETER;
-  }
-
-  StackTop    = (UINTN)Buffer + CPU_STACK_SWITCH_EXCEPTION_NUMBER * CPU_KNOWN_GOOD_STACK_SIZE;
-  NewGdtTable = ALIGN_POINTER (StackTop, sizeof (IA32_TSS_DESCRIPTOR));
-
-  AsmReadIdtr (&Idtr);
-  EssData.KnownGoodStackTop          = StackTop;
-  EssData.KnownGoodStackSize         = CPU_KNOWN_GOOD_STACK_SIZE;
-  EssData.StackSwitchExceptions      = CPU_STACK_SWITCH_EXCEPTION_LIST;
-  EssData.StackSwitchExceptionNumber = CPU_STACK_SWITCH_EXCEPTION_NUMBER;
-  EssData.IdtTable                   = (VOID *)Idtr.Base;
-  EssData.IdtTableSize               = Idtr.Limit + 1;
-  EssData.GdtTable                   = NewGdtTable;
-  EssData.GdtTableSize               = CPU_TSS_DESC_SIZE + Gdtr.Limit + 1;
-  EssData.ExceptionTssDesc           = NewGdtTable + Gdtr.Limit + 1;
-  EssData.ExceptionTssDescSize       = CPU_TSS_DESC_SIZE;
-  EssData.ExceptionTss               = NewGdtTable + Gdtr.Limit + 1 + CPU_TSS_DESC_SIZE;
-  EssData.ExceptionTssSize           = CPU_TSS_SIZE;
-
-  return ArchSetupExceptionStack (&EssData);
+  return ArchSetupExceptionStack (Buffer, BufferSize);
 }
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
index 8ae4feae62..6a170286c8 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.inf
@@ -1,7 +1,7 @@
 ## @file
 #  CPU Exception Handler library instance for SEC/PEI modules.
 #
-#  Copyright (c) 2012 - 2018, Intel Corporation. All rights reserved.<BR>
+#  Copyright (c) 2012 - 2022, Intel Corporation. All rights reserved.<BR>
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
 ##
@@ -50,6 +50,11 @@
   PeCoffGetEntryPointLib
   VmgExitLib
 
+[Pcd]
+  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuStackSwitchExceptionList
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownGoodStackSize
+
 [FeaturePcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                    ## CONSUMES
 
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
index c9f20da058..9dde07612a 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
@@ -1,7 +1,7 @@
 ## @file
 #  CPU Exception Handler library instance for SMM modules.
 #
-#  Copyright (c) 2013 - 2018, Intel Corporation. All rights reserved.<BR>
+#  Copyright (c) 2013 - 2022, Intel Corporation. All rights reserved.<BR>
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
 ##
@@ -53,6 +53,11 @@
   DebugLib
   VmgExitLib
 
+[Pcd]
+  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuStackSwitchExceptionList
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownGoodStackSize
+
 [FeaturePcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                    ## CONSUMES
 
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
index c14ac66c43..e6b2a83575 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ArchExceptionHandler.c
@@ -109,19 +109,22 @@ ArchRestoreExceptionContext (
 }
 
 /**
-  Setup separate stack for given exceptions.
+  Setup separate stacks for certain exception handlers.
 
-  @param[in] StackSwitchData      Pointer to data required for setuping up
-                                  stack switch.
-
-  @retval EFI_SUCCESS             The exceptions have been successfully
-                                  initialized with new stack.
-  @retval EFI_INVALID_PARAMETER   StackSwitchData contains invalid content.
+  @param[in]       Buffer        Point to buffer used to separate exception stack.
+  @param[in, out]  BufferSize    On input, it indicates the byte size of Buffer.
+                                 If the size is not enough, the return status will
+                                 be EFI_BUFFER_TOO_SMALL, and output BufferSize
+                                 will be the size it needs.
 
+  @retval EFI_SUCCESS             The stacks are assigned successfully.
+  @retval EFI_BUFFER_TOO_SMALL    This BufferSize is too small.
+  @retval EFI_UNSUPPORTED         This function is not supported.
 **/
 EFI_STATUS
 ArchSetupExceptionStack (
-  IN CPU_EXCEPTION_INIT_DATA  *StackSwitchData
+  IN     VOID   *Buffer,
+  IN OUT UINTN  *BufferSize
   )
 {
   IA32_DESCRIPTOR           Gdtr;
@@ -129,86 +132,77 @@ ArchSetupExceptionStack (
   IA32_IDT_GATE_DESCRIPTOR  *IdtTable;
   IA32_TSS_DESCRIPTOR       *TssDesc;
   IA32_TASK_STATE_SEGMENT   *Tss;
+  VOID                      *NewGdtTable;
   UINTN                     StackTop;
   UINTN                     Index;
   UINTN                     Vector;
   UINTN                     TssBase;
-  UINTN                     GdtSize;
-
-  if ((StackSwitchData == NULL) ||
-      (StackSwitchData->KnownGoodStackTop == 0) ||
-      (StackSwitchData->KnownGoodStackSize == 0) ||
-      (StackSwitchData->StackSwitchExceptions == NULL) ||
-      (StackSwitchData->StackSwitchExceptionNumber == 0) ||
-      (StackSwitchData->StackSwitchExceptionNumber > CPU_EXCEPTION_NUM) ||
-      (StackSwitchData->GdtTable == NULL) ||
-      (StackSwitchData->IdtTable == NULL) ||
-      (StackSwitchData->ExceptionTssDesc == NULL) ||
-      (StackSwitchData->ExceptionTss == NULL))
-  {
-    return EFI_INVALID_PARAMETER;
-  }
-
-  //
-  // The caller is responsible for that the GDT table, no matter the existing
-  // one or newly allocated, has enough space to hold descriptors for exception
-  // task-state segments.
-  //
-  if (((UINTN)StackSwitchData->GdtTable & (IA32_GDT_ALIGNMENT - 1)) != 0) {
-    return EFI_INVALID_PARAMETER;
-  }
-
-  if ((UINTN)StackSwitchData->ExceptionTssDesc < (UINTN)(StackSwitchData->GdtTable)) {
-    return EFI_INVALID_PARAMETER;
-  }
-
-  if (((UINTN)StackSwitchData->ExceptionTssDesc + StackSwitchData->ExceptionTssDescSize) >
-      ((UINTN)(StackSwitchData->GdtTable) + StackSwitchData->GdtTableSize))
-  {
-    return EFI_INVALID_PARAMETER;
-  }
+  UINT8                     *StackSwitchExceptions;
+  UINTN                     NeedBufferSize;
 
-  //
-  // One task gate descriptor and one task-state segment are needed.
-  //
-  if (StackSwitchData->ExceptionTssDescSize < sizeof (IA32_TSS_DESCRIPTOR)) {
-    return EFI_INVALID_PARAMETER;
-  }
-
-  if (StackSwitchData->ExceptionTssSize < sizeof (IA32_TASK_STATE_SEGMENT)) {
+  if (BufferSize == NULL) {
     return EFI_INVALID_PARAMETER;
   }
 
   //
   // Interrupt stack table supports only 7 vectors.
   //
-  TssDesc = StackSwitchData->ExceptionTssDesc;
-  Tss     = StackSwitchData->ExceptionTss;
-  if (StackSwitchData->StackSwitchExceptionNumber > ARRAY_SIZE (Tss->IST)) {
-    return EFI_INVALID_PARAMETER;
+  if (CPU_STACK_SWITCH_EXCEPTION_NUMBER > ARRAY_SIZE (Tss->IST)) {
+    return EFI_UNSUPPORTED;
   }
 
   //
-  // Initialize new GDT table and/or IDT table, if any
+  // Total needed size includes stack size, new GDT table size, TSS size.
+  // Add another DESCRIPTOR size for alignment requiremet.
+  //
+  // Layout of memory needed for each processor:
+  //    --------------------------------
+  //    |                              |
+  //    |          Stack Size          |  X ExceptionNumber
+  //    |                              |
+  //    --------------------------------
+  //    |          Alignment           |  (just in case)
+  //    --------------------------------
+  //    |                              |
+  //    |         Original GDT         |
+  //    |                              |
+  //    --------------------------------
+  //    |    Current task descriptor   |
+  //    --------------------------------
+  //    |                              |
+  //    |  Exception task descriptors  |  X 1
+  //    |                              |
+  //    --------------------------------
+  //    |                              |
+  //    | Exception task-state segment |  X 1
+  //    |                              |
+  //    --------------------------------
   //
-  AsmReadIdtr (&Idtr);
   AsmReadGdtr (&Gdtr);
-
-  GdtSize = (UINTN)TssDesc + sizeof (IA32_TSS_DESCRIPTOR) -
-            (UINTN)(StackSwitchData->GdtTable);
-  if ((UINTN)StackSwitchData->GdtTable != Gdtr.Base) {
-    CopyMem (StackSwitchData->GdtTable, (VOID *)Gdtr.Base, Gdtr.Limit + 1);
-    Gdtr.Base  = (UINTN)StackSwitchData->GdtTable;
-    Gdtr.Limit = (UINT16)GdtSize - 1;
+  NeedBufferSize = CPU_STACK_SWITCH_EXCEPTION_NUMBER * CPU_KNOWN_GOOD_STACK_SIZE +
+                   sizeof (IA32_TSS_DESCRIPTOR) +
+                   Gdtr.Limit + 1 + CPU_TSS_DESC_SIZE +
+                   CPU_TSS_SIZE;
+
+  if (*BufferSize < NeedBufferSize) {
+    *BufferSize = NeedBufferSize;
+    return EFI_BUFFER_TOO_SMALL;
   }
 
-  if ((UINTN)StackSwitchData->IdtTable != Idtr.Base) {
-    Idtr.Base = (UINTN)StackSwitchData->IdtTable;
+  if (Buffer == NULL) {
+    return EFI_INVALID_PARAMETER;
   }
 
-  if (StackSwitchData->IdtTableSize > 0) {
-    Idtr.Limit = (UINT16)(StackSwitchData->IdtTableSize - 1);
-  }
+  AsmReadIdtr (&Idtr);
+  StackSwitchExceptions = CPU_STACK_SWITCH_EXCEPTION_LIST;
+  StackTop              = (UINTN)Buffer + CPU_STACK_SWITCH_EXCEPTION_NUMBER * CPU_KNOWN_GOOD_STACK_SIZE;
+  NewGdtTable           = ALIGN_POINTER (StackTop, sizeof (IA32_TSS_DESCRIPTOR));
+  TssDesc               = (IA32_TSS_DESCRIPTOR *)((UINTN)NewGdtTable + Gdtr.Limit + 1);
+  Tss                   = (IA32_TASK_STATE_SEGMENT *)((UINTN)TssDesc + CPU_TSS_DESC_SIZE);
+
+  CopyMem (NewGdtTable, (VOID *)Gdtr.Base, Gdtr.Limit + 1);
+  Gdtr.Base  = (UINTN)NewGdtTable;
+  Gdtr.Limit = (UINT16)(Gdtr.Limit + CPU_TSS_DESC_SIZE);
 
   //
   // Fixup current task descriptor. Task-state segment for current task will
@@ -231,20 +225,20 @@ ArchSetupExceptionStack (
   // Fixup exception task descriptor and task-state segment
   //
   ZeroMem (Tss, sizeof (*Tss));
-  StackTop = StackSwitchData->KnownGoodStackTop - CPU_STACK_ALIGNMENT;
+  StackTop = StackTop - CPU_STACK_ALIGNMENT;
   StackTop = (UINTN)ALIGN_POINTER (StackTop, CPU_STACK_ALIGNMENT);
-  IdtTable = StackSwitchData->IdtTable;
-  for (Index = 0; Index < StackSwitchData->StackSwitchExceptionNumber; ++Index) {
+  IdtTable = (IA32_IDT_GATE_DESCRIPTOR  *)Idtr.Base;
+  for (Index = 0; Index < CPU_STACK_SWITCH_EXCEPTION_NUMBER; ++Index) {
     //
     // Fixup IST
     //
     Tss->IST[Index] = StackTop;
-    StackTop       -= StackSwitchData->KnownGoodStackSize;
+    StackTop       -= CPU_KNOWN_GOOD_STACK_SIZE;
 
     //
     // Set the IST field to enable corresponding IST
     //
-    Vector = StackSwitchData->StackSwitchExceptions[Index];
+    Vector = StackSwitchExceptions[Index];
     if ((Vector >= CPU_EXCEPTION_NUM) ||
         (Vector >= (Idtr.Limit + 1) / sizeof (IA32_IDT_GATE_DESCRIPTOR)))
     {
@@ -262,12 +256,7 @@ ArchSetupExceptionStack (
   //
   // Load current task
   //
-  AsmWriteTr ((UINT16)((UINTN)StackSwitchData->ExceptionTssDesc - Gdtr.Base));
-
-  //
-  // Publish IDT
-  //
-  AsmWriteIdtr (&Idtr);
+  AsmWriteTr ((UINT16)((UINTN)TssDesc - Gdtr.Base));
 
   return EFI_SUCCESS;
 }
diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
index a15f125d5b..6d2f66504a 100644
--- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
+++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf
@@ -2,7 +2,7 @@
 #  CPU Exception Handler library instance for SEC/PEI modules.
 #
 #  Copyright (C) 2020, Advanced Micro Devices, Inc. All rights reserved.<BR>
-#  Copyright (c) 2012 - 2018, Intel Corporation. All rights reserved.<BR>
+#  Copyright (c) 2012 - 2022, Intel Corporation. All rights reserved.<BR>
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
 #  This is the XCODE5 variant of the SEC/PEI CpuExceptionHandlerLib. This
@@ -55,6 +55,11 @@
   PeCoffGetEntryPointLib
   VmgExitLib
 
+[Pcd]
+  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuStackSwitchExceptionList
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownGoodStackSize
+
 [FeaturePcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                    ## CONSUMES
 
-- 
2.31.1.windows.1


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

* Re: [PATCH] UefiCpuPkg/MpInitLib: Simplify logic in SwitchBsp
  2022-08-25  2:55 ` [PATCH] UefiCpuPkg/MpInitLib: Simplify logic in SwitchBsp Zhiguang Liu
@ 2022-08-25  9:18   ` Ni, Ray
  0 siblings, 0 replies; 7+ messages in thread
From: Ni, Ray @ 2022-08-25  9:18 UTC (permalink / raw)
  To: Liu, Zhiguang, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R

> 
>      ;Store EFLAGS, GDTR and IDTR register to stack
>      pushfd

Can you update the comments?

> -    mov        eax, cr4
> -    push       eax       ; push cr4 firstly
> -    mov        eax, cr0
> -    push       eax
> -
> -    sgdt       [esi + CPU_EXCHANGE_ROLE_INFO.Gdtr]
> -    sidt       [esi + CPU_EXCHANGE_ROLE_INFO.Idtr]
> 
>  


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

* Re: [edk2-devel] [PATCH] UefiCpuPkg: Enhance logic in InitializeMpExceptionStackSwitchHandlers
  2022-08-25  2:55 [PATCH] UefiCpuPkg: Enhance logic in InitializeMpExceptionStackSwitchHandlers Zhiguang Liu
                   ` (2 preceding siblings ...)
  2022-08-25  2:55 ` [PATCH] UefiCpuPkg: Simplify the implementation when separate exception stacks Zhiguang Liu
@ 2022-08-25  9:32 ` Ni, Ray
  3 siblings, 0 replies; 7+ messages in thread
From: Ni, Ray @ 2022-08-25  9:32 UTC (permalink / raw)
  To: devel@edk2.groups.io, Liu, Zhiguang; +Cc: Dong, Eric, Kumar, Rahul R

> +  MpInitLibWhoAmI (&Index);
>    SwitchStackData = (EXCEPTION_STACK_SWITCH_CONTEXT *)Buffer;
> -  InitializeSeparateExceptionStacks (SwitchStackData->Buffer,
> SwitchStackData->BufferSize);
> +  if ((SwitchStackData[Index].Status == EFI_NOT_STARTED) ||
> (SwitchStackData[Index].Status == EFI_BUFFER_TOO_SMALL)) {
> +    SwitchStackData[Index].Status = InitializeSeparateExceptionStacks
> (SwitchStackData[Index].Buffer, &SwitchStackData[Index].BufferSize);
> +  }
>  }

1. Can you add comment to explain why BufferTooSmall is checked?
I guess you want to handle the case when calls in some APs succeed
while some calls in other APs get BufferTooSmall.
So this check makes sure that the 2nd call happens only on those failed APs.

> +  SwitchStackData = AllocateRuntimeZeroPool (mNumberOfProcessors *
> sizeof (EXCEPTION_STACK_SWITCH_CONTEXT));
2. Why runtime type memory?

> +      if (SwitchStackData[Index].Status == EFI_BUFFER_TOO_SMALL) {
3. Can you add assertion to make sure " SwitchStackData[Index].BufferSize != 0"?


> +        SwitchStackData[Index].Buffer = (VOID *)(BufferSize + (UINTN)Buffer);
4. Use "&Buffer[BufferSize]";



> 
> -  SwitchStackData.BufferSize = &BufferSize;
>    MpInitLibGetNumberOfProcessors (&NumberOfProcessors, NULL);
> -  MpInitLibWhoAmI (&Bsp);
> +  SwitchStackData = AllocateRuntimeZeroPool (NumberOfProcessors *
> sizeof (EXCEPTION_STACK_SWITCH_CONTEXT));

5. No need for runtime memory.

> +  for (Index = 0; Index < NumberOfProcessors; ++Index) {
> +    if (SwitchStackData[Index].Status == EFI_BUFFER_TOO_SMALL) {
> +      BufferSize += SwitchStackData[Index].BufferSize;

6. add assertion to make sure ".BufferSize == 0" when ".Status == EFI_SUCCESS".
I think assertion here is better than my last comment which requests assertion in
the other place.

7. Please use AllocatePages in PEI because in a many-core platform the pool 64KB
limitation may cause the code fail to allocate.

Thanks,
Ray

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

* Re: [PATCH] UefiCpuPkg/MpInitLib: Fix potential issue when IDT table is at above 4G
  2022-08-25  2:55 ` [PATCH] UefiCpuPkg/MpInitLib: Fix potential issue when IDT table is at above 4G Zhiguang Liu
@ 2022-08-25  9:32   ` Ni, Ray
  0 siblings, 0 replies; 7+ messages in thread
From: Ni, Ray @ 2022-08-25  9:32 UTC (permalink / raw)
  To: Liu, Zhiguang, devel@edk2.groups.io; +Cc: Dong, Eric, Kumar, Rahul R

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Thursday, August 25, 2022 10:55 AM
> To: devel@edk2.groups.io
> Cc: Liu, Zhiguang <zhiguang.liu@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>
> Subject: [PATCH] UefiCpuPkg/MpInitLib: Fix potential issue when IDT table is
> at above 4G
> 
> Currently, when waking up AP, IDT table of AP will be set in 16 bit code,
> and assume the IDT table base is 32 bit. However, the IDT table is created
> by BSP. Issue will happen if the BSP allocates memory above 4G for BSP's
> IDT table. Moreover, even the IDT table location is below 4G, the handler
> function inside the IDT table is 64 bit, and it won't take effect until
> CPU transfers to 64 bit long mode. There is no benefit to set IDT table in
> such an early phase.
> To avoid such issue, this patch moves the LIDT instruction into 64 bit
> code.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> index 1daaa72b1e..cd95b03da8 100644
> --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm
> @@ -64,9 +64,6 @@ BITS 16
>      mov        si, MP_CPU_EXCHANGE_INFO_FIELD (GdtrProfile)
>  o32 lgdt       [cs:si]
> 
> -    mov        si, MP_CPU_EXCHANGE_INFO_FIELD (IdtrProfile)
> -o32 lidt       [cs:si]
> -
>      ;
>      ; Switch to protected mode
>      ;
> @@ -154,6 +151,11 @@ BITS 64
> 
>  LongModeStart:
>      mov        esi, ebx
> +
> +    ; Set IDT table at the start of 64 bit code
> +    lea        edi, [esi + MP_CPU_EXCHANGE_INFO_FIELD (IdtrProfile)]
> +    lidt       [edi]
> +
>      lea        edi, [esi + MP_CPU_EXCHANGE_INFO_FIELD (InitFlag)]
>      cmp        qword [edi], 1       ; ApInitConfig
>      jnz        GetApicId
> --
> 2.31.1.windows.1


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

end of thread, other threads:[~2022-08-25  9:33 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-25  2:55 [PATCH] UefiCpuPkg: Enhance logic in InitializeMpExceptionStackSwitchHandlers Zhiguang Liu
2022-08-25  2:55 ` [PATCH] UefiCpuPkg/MpInitLib: Fix potential issue when IDT table is at above 4G Zhiguang Liu
2022-08-25  9:32   ` Ni, Ray
2022-08-25  2:55 ` [PATCH] UefiCpuPkg/MpInitLib: Simplify logic in SwitchBsp Zhiguang Liu
2022-08-25  9:18   ` Ni, Ray
2022-08-25  2:55 ` [PATCH] UefiCpuPkg: Simplify the implementation when separate exception stacks Zhiguang Liu
2022-08-25  9:32 ` [edk2-devel] [PATCH] UefiCpuPkg: Enhance logic in InitializeMpExceptionStackSwitchHandlers Ni, Ray

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