[v2] Implement stack guard feature

[edk2] [PATCH v2 8/8] UefiCpuPkg/CpuDxe: Initialize stack switch for MP

Posted by Jian J Wang 7 years, 1 month ago

> v2:
>    Add code to reserve resources and initialize AP exception with stack
>    switch besides BSP, if PcdCpuStackGuard is enabled.

In current MP implementation, BSP and AP shares the same exception
configuration. Stack switch required by Stack Guard feature needs that BSP
and AP have their own configuration. This patch adds code to ask BSP and AP
to do exception handler initialization separately.

Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Michael Kinney <michael.d.kinney@intel.com>
Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 UefiCpuPkg/CpuDxe/CpuDxe.inf |   3 +
 UefiCpuPkg/CpuDxe/CpuMp.c    | 168 +++++++++++++++++++++++++++++++++++++++++++
 UefiCpuPkg/CpuDxe/CpuMp.h    |  12 ++++
 3 files changed, 183 insertions(+)

diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf b/UefiCpuPkg/CpuDxe/CpuDxe.inf
index 3e8d196739..02f86b774c 100644
--- a/UefiCpuPkg/CpuDxe/CpuDxe.inf
+++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf
@@ -81,6 +81,9 @@
 
 [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask    ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                       ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuStackSwitchExceptionList              ## CONSUMES
+  gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownGoodStackSize                    ## CONSUMES
 
 [Depex]
   TRUE
diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c
index b3c0178d07..6b2ceacb39 100644
--- a/UefiCpuPkg/CpuDxe/CpuMp.c
+++ b/UefiCpuPkg/CpuDxe/CpuMp.c
@@ -601,6 +601,169 @@ CollectBistDataFromHob (
   }
 }
 
+/**
+  Get GDT register content.
+
+  This function is mainly for AP purpose because AP may have different GDT
+  table than BSP.
+
+**/
+VOID
+EFIAPI
+GetGdtr (
+  IN OUT VOID *Buffer
+  )
+{
+  AsmReadGdtr ((IA32_DESCRIPTOR *)Buffer);
+}
+
+/**
+  Initializes CPU exceptions handlers for the sake of stack switch requirement.
+
+  This function is a wrapper of InitializeCpuExceptionStackSwitchHandlers.
+  It's mainly for AP purpose because of EFI_AP_PROCEDURE API requirement.
+
+**/
+VOID
+EFIAPI
+InitializeExceptionStackSwitchHandlers (
+  IN OUT VOID *Buffer
+  )
+{
+  EXCEPTION_STACK_SWITCH_DATA     *EssData;
+  IA32_DESCRIPTOR                 Idtr;
+  EFI_STATUS                      Status;
+
+  EssData = Buffer;
+  //
+  // We don't plan to replace IDT table with a new one, and we don't assume
+  // the AP's IDT is the same as BSP's IDT either.
+  //
+  AsmReadIdtr (&Idtr);
+  EssData->IdtTable = (IA32_IDT_GATE_DESCRIPTOR *)Idtr.Base;
+  Status = InitializeCpuExceptionStackSwitchHandlers (EssData);
+  ASSERT_EFI_ERROR (Status);
+}
+
+/**
+  Initializes MP exceptions handlers for the sake of stack switch requirement.
+
+  This function will allocate required resources for stack switch and pass
+  them through EXCEPTION_STACK_SWITCH_DATA to each logic processor.
+
+**/
+VOID
+InitializeMpExceptionStackSwitchHandlers (
+  VOID
+  )
+{
+  UINTN                           Index;
+  UINTN                           Bsp;
+  UINTN                           ExceptionNumber;
+  UINTN                           NewGdtSize;
+  UINTN                           NewStackSize;
+  IA32_DESCRIPTOR                 Gdtr;
+  EXCEPTION_STACK_SWITCH_DATA     EssData;
+  UINT8                           *GdtBuffer;
+  UINT8                           *StackTop;
+
+  if (!PcdGetBool (PcdCpuStackGuard)) {
+    return;
+  }
+
+  ExceptionNumber = FixedPcdGetSize (PcdCpuStackSwitchExceptionList);
+  NewStackSize = FixedPcdGet32 (PcdCpuKnownGoodStackSize) * ExceptionNumber;
+
+  StackTop = AllocateRuntimeZeroPool (NewStackSize * mNumberOfProcessors);
+  ASSERT (StackTop != NULL);
+  StackTop += NewStackSize  * mNumberOfProcessors;
+
+  EssData.Exceptions = FixedPcdGetPtr (PcdCpuStackSwitchExceptionList);
+  EssData.ExceptionNumber = ExceptionNumber;
+  EssData.StackSize = FixedPcdGet32 (PcdCpuKnownGoodStackSize);
+
+  MpInitLibWhoAmI (&Bsp);
+  for (Index = 0; Index < mNumberOfProcessors; ++Index) {
+    //
+    // To support stack switch, we need to re-construct GDT but not IDT.
+    //
+    if (Index == Bsp) {
+      GetGdtr (&Gdtr);
+    } else {
+      //
+      // AP might have different size of GDT from BSP.
+      //
+      MpInitLibStartupThisAP (GetGdtr, Index, NULL, 0, (VOID *)&Gdtr, NULL);
+    }
+
+    //
+    // 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. Since AP is not supposed to allocate
+    // memory, we have to do it in BSP. To simplify the code, we allocate
+    // memory for IA32 case to cover both IA32 and X64 exception stack
+    // switch.
+    //
+    // Layout of memory to allocate 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
+    //    |                              |
+    //    --------------------------------
+    //
+    NewGdtSize = sizeof (IA32_TSS_DESCRIPTOR) +
+                 (Gdtr.Limit + 1) +
+                 sizeof (IA32_TSS_DESCRIPTOR) * (ExceptionNumber + 1) +
+                 sizeof (IA32_TASK_STATE_SEGMENT) * (ExceptionNumber + 1);
+    GdtBuffer = AllocateRuntimeZeroPool (NewGdtSize);
+    ASSERT (GdtBuffer != NULL);
+
+    EssData.GdtTable = ALIGN_POINTER(GdtBuffer, sizeof (IA32_TSS_DESCRIPTOR));
+    NewGdtSize -= ((UINT8 *)EssData.GdtTable - GdtBuffer);
+    EssData.GdtSize = NewGdtSize;
+
+    EssData.TssDesc = (IA32_TSS_DESCRIPTOR *)((UINTN)EssData.GdtTable +
+                                              Gdtr.Limit + 1);
+    EssData.Tss = (IA32_TASK_STATE_SEGMENT *)((UINTN)EssData.GdtTable +
+                                              Gdtr.Limit + 1 +
+                                              sizeof (IA32_TSS_DESCRIPTOR) *
+                                              (ExceptionNumber + 1));
+
+    EssData.StackTop = (UINTN)StackTop;
+    DEBUG ((DEBUG_INFO, "Exception stack top[%d]: 0x%lX\n", Index,
+            (UINT64)(UINTN)StackTop));
+
+    if (Index == Bsp) {
+      InitializeExceptionStackSwitchHandlers (&EssData);
+    } else {
+      MpInitLibStartupThisAP (
+        InitializeExceptionStackSwitchHandlers,
+        Index,
+        NULL,
+        0,
+        (VOID *)&EssData,
+        NULL
+        );
+    }
+
+    StackTop  -= NewStackSize;
+  }
+}
+
 /**
   Initialize Multi-processor support.
 
@@ -624,6 +787,11 @@ InitializeMpSupport (
   mNumberOfProcessors = NumberOfProcessors;
   DEBUG ((DEBUG_INFO, "Detect CPU count: %d\n", mNumberOfProcessors));
 
+  //
+  // Initialize exception stack switch handlers for each logic processor.
+  //
+  InitializeMpExceptionStackSwitchHandlers ();
+
   //
   // Update CPU healthy information from Guided HOB
   //
diff --git a/UefiCpuPkg/CpuDxe/CpuMp.h b/UefiCpuPkg/CpuDxe/CpuMp.h
index d530149d7e..86d54a95e9 100644
--- a/UefiCpuPkg/CpuDxe/CpuMp.h
+++ b/UefiCpuPkg/CpuDxe/CpuMp.h
@@ -15,6 +15,18 @@
 #ifndef _CPU_MP_H_
 #define _CPU_MP_H_
 
+typedef struct {
+  UINTN                     StackTop;
+  UINTN                     StackSize;
+  UINT8                     *Exceptions;
+  UINTN                     ExceptionNumber;
+  IA32_IDT_GATE_DESCRIPTOR  *IdtTable;
+  IA32_SEGMENT_DESCRIPTOR   *GdtTable;
+  UINTN                     GdtSize;
+  IA32_TSS_DESCRIPTOR       *TssDesc;
+  IA32_TASK_STATE_SEGMENT   *Tss;
+} EXCEPTION_STACK_SWITCH_DATA;
+
 /**
   Initialize Multi-processor support.
 
-- 
2.14.1.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 8/8] UefiCpuPkg/CpuDxe: Initialize stack switch for MP

Posted by Yao, Jiewen 7 years, 1 month ago

Hi
1) Can we enable this feature in early DxeCore?

Current DxeCore still calling InitializeCpuExceptionHandlers().
But I hope InitializeExceptionStackSwitchHandlers() can be used here.

In order to handle buffer from different arch, the DxeIpl can help provide some data in hob and pass to DxeCore.

2) In addition, InitializeCpuExceptionHandlers () has VectorInfoList as parameter.
Do we also need that for InitializeExceptionStackSwitchHandlers()?

Thank you
Yao Jiewen

> -----Original Message-----
> From: Wang, Jian J
> Sent: Wednesday, November 22, 2017 4:46 PM
> To: edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>
> Subject: [PATCH v2 8/8] UefiCpuPkg/CpuDxe: Initialize stack switch for MP
> 
> > v2:
> >    Add code to reserve resources and initialize AP exception with stack
> >    switch besides BSP, if PcdCpuStackGuard is enabled.
> 
> In current MP implementation, BSP and AP shares the same exception
> configuration. Stack switch required by Stack Guard feature needs that BSP
> and AP have their own configuration. This patch adds code to ask BSP and AP
> to do exception handler initialization separately.
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Michael Kinney <michael.d.kinney@intel.com>
> Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  UefiCpuPkg/CpuDxe/CpuDxe.inf |   3 +
>  UefiCpuPkg/CpuDxe/CpuMp.c    | 168
> +++++++++++++++++++++++++++++++++++++++++++
>  UefiCpuPkg/CpuDxe/CpuMp.h    |  12 ++++
>  3 files changed, 183 insertions(+)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> index 3e8d196739..02f86b774c 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.inf
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> @@ -81,6 +81,9 @@
> 
>  [Pcd]
> 
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
> ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard
> ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuStackSwitchExceptionList
> ## CONSUMES
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownGoodStackSize
> ## CONSUMES
> 
>  [Depex]
>    TRUE
> diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c
> index b3c0178d07..6b2ceacb39 100644
> --- a/UefiCpuPkg/CpuDxe/CpuMp.c
> +++ b/UefiCpuPkg/CpuDxe/CpuMp.c
> @@ -601,6 +601,169 @@ CollectBistDataFromHob (
>    }
>  }
> 
> +/**
> +  Get GDT register content.
> +
> +  This function is mainly for AP purpose because AP may have different GDT
> +  table than BSP.
> +
> +**/
> +VOID
> +EFIAPI
> +GetGdtr (
> +  IN OUT VOID *Buffer
> +  )
> +{
> +  AsmReadGdtr ((IA32_DESCRIPTOR *)Buffer);
> +}
> +
> +/**
> +  Initializes CPU exceptions handlers for the sake of stack switch requirement.
> +
> +  This function is a wrapper of InitializeCpuExceptionStackSwitchHandlers.
> +  It's mainly for AP purpose because of EFI_AP_PROCEDURE API requirement.
> +
> +**/
> +VOID
> +EFIAPI
> +InitializeExceptionStackSwitchHandlers (
> +  IN OUT VOID *Buffer
> +  )
> +{
> +  EXCEPTION_STACK_SWITCH_DATA     *EssData;
> +  IA32_DESCRIPTOR                 Idtr;
> +  EFI_STATUS                      Status;
> +
> +  EssData = Buffer;
> +  //
> +  // We don't plan to replace IDT table with a new one, and we don't assume
> +  // the AP's IDT is the same as BSP's IDT either.
> +  //
> +  AsmReadIdtr (&Idtr);
> +  EssData->IdtTable = (IA32_IDT_GATE_DESCRIPTOR *)Idtr.Base;
> +  Status = InitializeCpuExceptionStackSwitchHandlers (EssData);
> +  ASSERT_EFI_ERROR (Status);
> +}
> +
> +/**
> +  Initializes MP exceptions handlers for the sake of stack switch requirement.
> +
> +  This function will allocate required resources for stack switch and pass
> +  them through EXCEPTION_STACK_SWITCH_DATA to each logic processor.
> +
> +**/
> +VOID
> +InitializeMpExceptionStackSwitchHandlers (
> +  VOID
> +  )
> +{
> +  UINTN                           Index;
> +  UINTN                           Bsp;
> +  UINTN                           ExceptionNumber;
> +  UINTN                           NewGdtSize;
> +  UINTN                           NewStackSize;
> +  IA32_DESCRIPTOR                 Gdtr;
> +  EXCEPTION_STACK_SWITCH_DATA     EssData;
> +  UINT8                           *GdtBuffer;
> +  UINT8                           *StackTop;
> +
> +  if (!PcdGetBool (PcdCpuStackGuard)) {
> +    return;
> +  }
> +
> +  ExceptionNumber = FixedPcdGetSize (PcdCpuStackSwitchExceptionList);
> +  NewStackSize = FixedPcdGet32 (PcdCpuKnownGoodStackSize) *
> ExceptionNumber;
> +
> +  StackTop = AllocateRuntimeZeroPool (NewStackSize *
> mNumberOfProcessors);
> +  ASSERT (StackTop != NULL);
> +  StackTop += NewStackSize  * mNumberOfProcessors;
> +
> +  EssData.Exceptions = FixedPcdGetPtr (PcdCpuStackSwitchExceptionList);
> +  EssData.ExceptionNumber = ExceptionNumber;
> +  EssData.StackSize = FixedPcdGet32 (PcdCpuKnownGoodStackSize);
> +
> +  MpInitLibWhoAmI (&Bsp);
> +  for (Index = 0; Index < mNumberOfProcessors; ++Index) {
> +    //
> +    // To support stack switch, we need to re-construct GDT but not IDT.
> +    //
> +    if (Index == Bsp) {
> +      GetGdtr (&Gdtr);
> +    } else {
> +      //
> +      // AP might have different size of GDT from BSP.
> +      //
> +      MpInitLibStartupThisAP (GetGdtr, Index, NULL, 0, (VOID *)&Gdtr, NULL);
> +    }
> +
> +    //
> +    // 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. Since AP is not supposed to allocate
> +    // memory, we have to do it in BSP. To simplify the code, we allocate
> +    // memory for IA32 case to cover both IA32 and X64 exception stack
> +    // switch.
> +    //
> +    // Layout of memory to allocate 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
> +    //    |                              |
> +    //    --------------------------------
> +    //
> +    NewGdtSize = sizeof (IA32_TSS_DESCRIPTOR) +
> +                 (Gdtr.Limit + 1) +
> +                 sizeof (IA32_TSS_DESCRIPTOR) * (ExceptionNumber + 1) +
> +                 sizeof (IA32_TASK_STATE_SEGMENT) * (ExceptionNumber +
> 1);
> +    GdtBuffer = AllocateRuntimeZeroPool (NewGdtSize);
> +    ASSERT (GdtBuffer != NULL);
> +
> +    EssData.GdtTable = ALIGN_POINTER(GdtBuffer, sizeof
> (IA32_TSS_DESCRIPTOR));
> +    NewGdtSize -= ((UINT8 *)EssData.GdtTable - GdtBuffer);
> +    EssData.GdtSize = NewGdtSize;
> +
> +    EssData.TssDesc = (IA32_TSS_DESCRIPTOR *)((UINTN)EssData.GdtTable +
> +                                              Gdtr.Limit + 1);
> +    EssData.Tss = (IA32_TASK_STATE_SEGMENT *)((UINTN)EssData.GdtTable +
> +                                              Gdtr.Limit + 1 +
> +                                              sizeof
> (IA32_TSS_DESCRIPTOR) *
> +                                              (ExceptionNumber + 1));
> +
> +    EssData.StackTop = (UINTN)StackTop;
> +    DEBUG ((DEBUG_INFO, "Exception stack top[%d]: 0x%lX\n", Index,
> +            (UINT64)(UINTN)StackTop));
> +
> +    if (Index == Bsp) {
> +      InitializeExceptionStackSwitchHandlers (&EssData);
> +    } else {
> +      MpInitLibStartupThisAP (
> +        InitializeExceptionStackSwitchHandlers,
> +        Index,
> +        NULL,
> +        0,
> +        (VOID *)&EssData,
> +        NULL
> +        );
> +    }
> +
> +    StackTop  -= NewStackSize;
> +  }
> +}
> +
>  /**
>    Initialize Multi-processor support.
> 
> @@ -624,6 +787,11 @@ InitializeMpSupport (
>    mNumberOfProcessors = NumberOfProcessors;
>    DEBUG ((DEBUG_INFO, "Detect CPU count: %d\n", mNumberOfProcessors));
> 
> +  //
> +  // Initialize exception stack switch handlers for each logic processor.
> +  //
> +  InitializeMpExceptionStackSwitchHandlers ();
> +
>    //
>    // Update CPU healthy information from Guided HOB
>    //
> diff --git a/UefiCpuPkg/CpuDxe/CpuMp.h b/UefiCpuPkg/CpuDxe/CpuMp.h
> index d530149d7e..86d54a95e9 100644
> --- a/UefiCpuPkg/CpuDxe/CpuMp.h
> +++ b/UefiCpuPkg/CpuDxe/CpuMp.h
> @@ -15,6 +15,18 @@
>  #ifndef _CPU_MP_H_
>  #define _CPU_MP_H_
> 
> +typedef struct {
> +  UINTN                     StackTop;
> +  UINTN                     StackSize;
> +  UINT8                     *Exceptions;
> +  UINTN                     ExceptionNumber;
> +  IA32_IDT_GATE_DESCRIPTOR  *IdtTable;
> +  IA32_SEGMENT_DESCRIPTOR   *GdtTable;
> +  UINTN                     GdtSize;
> +  IA32_TSS_DESCRIPTOR       *TssDesc;
> +  IA32_TASK_STATE_SEGMENT   *Tss;
> +} EXCEPTION_STACK_SWITCH_DATA;
> +
>  /**
>    Initialize Multi-processor support.
> 
> --
> 2.14.1.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 8/8] UefiCpuPkg/CpuDxe: Initialize stack switch for MP

Posted by Wang, Jian J 7 years, 1 month ago

Hi,

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Thursday, November 23, 2017 12:14 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [PATCH v2 8/8] UefiCpuPkg/CpuDxe: Initialize stack switch for MP
> 
> Hi
> 1) Can we enable this feature in early DxeCore?
> 
Yes. Intead of calling InitializeExceptionStackSwitchHandlers () directly in DxeCore,
InitializeCpuExceptionHandlers() calls InitializeExceptionStackSwitchHandlers().

I think it's reasonable to do this because InitializeExceptionStackSwitchHandlers()
is arch dependent. It'd be better not to call it in DxeCore. Another benefit is that 
this can avoid backward compatibility issue introduced by new API, which hasn't 
been implemented by cpu driver or lib of other archs.

> Current DxeCore still calling InitializeCpuExceptionHandlers().
> But I hope InitializeExceptionStackSwitchHandlers() can be used here.
> 
> In order to handle buffer from different arch, the DxeIpl can help provide some
> data in hob and pass to DxeCore.
> 
> 2) In addition, InitializeCpuExceptionHandlers () has VectorInfoList as parameter.
> Do we also need that for InitializeExceptionStackSwitchHandlers()?
> 
I don't see the need. Do you have any use cases in mind?

> Thank you
> Yao Jiewen
> 
> > -----Original Message-----
> > From: Wang, Jian J
> > Sent: Wednesday, November 22, 2017 4:46 PM
> > To: edk2-devel@lists.01.org
> > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> Yao,
> > Jiewen <jiewen.yao@intel.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>
> > Subject: [PATCH v2 8/8] UefiCpuPkg/CpuDxe: Initialize stack switch for MP
> >
> > > v2:
> > >    Add code to reserve resources and initialize AP exception with stack
> > >    switch besides BSP, if PcdCpuStackGuard is enabled.
> >
> > In current MP implementation, BSP and AP shares the same exception
> > configuration. Stack switch required by Stack Guard feature needs that BSP
> > and AP have their own configuration. This patch adds code to ask BSP and AP
> > to do exception handler initialization separately.
> >
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Michael Kinney <michael.d.kinney@intel.com>
> > Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > ---
> >  UefiCpuPkg/CpuDxe/CpuDxe.inf |   3 +
> >  UefiCpuPkg/CpuDxe/CpuMp.c    | 168
> > +++++++++++++++++++++++++++++++++++++++++++
> >  UefiCpuPkg/CpuDxe/CpuMp.h    |  12 ++++
> >  3 files changed, 183 insertions(+)
> >
> > diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> > index 3e8d196739..02f86b774c 100644
> > --- a/UefiCpuPkg/CpuDxe/CpuDxe.inf
> > +++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> > @@ -81,6 +81,9 @@
> >
> >  [Pcd]
> >
> > gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
> > ## CONSUMES
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard
> > ## CONSUMES
> > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuStackSwitchExceptionList
> > ## CONSUMES
> > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownGoodStackSize
> > ## CONSUMES
> >
> >  [Depex]
> >    TRUE
> > diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c
> > index b3c0178d07..6b2ceacb39 100644
> > --- a/UefiCpuPkg/CpuDxe/CpuMp.c
> > +++ b/UefiCpuPkg/CpuDxe/CpuMp.c
> > @@ -601,6 +601,169 @@ CollectBistDataFromHob (
> >    }
> >  }
> >
> > +/**
> > +  Get GDT register content.
> > +
> > +  This function is mainly for AP purpose because AP may have different GDT
> > +  table than BSP.
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +GetGdtr (
> > +  IN OUT VOID *Buffer
> > +  )
> > +{
> > +  AsmReadGdtr ((IA32_DESCRIPTOR *)Buffer);
> > +}
> > +
> > +/**
> > +  Initializes CPU exceptions handlers for the sake of stack switch requirement.
> > +
> > +  This function is a wrapper of InitializeCpuExceptionStackSwitchHandlers.
> > +  It's mainly for AP purpose because of EFI_AP_PROCEDURE API requirement.
> > +
> > +**/
> > +VOID
> > +EFIAPI
> > +InitializeExceptionStackSwitchHandlers (
> > +  IN OUT VOID *Buffer
> > +  )
> > +{
> > +  EXCEPTION_STACK_SWITCH_DATA     *EssData;
> > +  IA32_DESCRIPTOR                 Idtr;
> > +  EFI_STATUS                      Status;
> > +
> > +  EssData = Buffer;
> > +  //
> > +  // We don't plan to replace IDT table with a new one, and we don't assume
> > +  // the AP's IDT is the same as BSP's IDT either.
> > +  //
> > +  AsmReadIdtr (&Idtr);
> > +  EssData->IdtTable = (IA32_IDT_GATE_DESCRIPTOR *)Idtr.Base;
> > +  Status = InitializeCpuExceptionStackSwitchHandlers (EssData);
> > +  ASSERT_EFI_ERROR (Status);
> > +}
> > +
> > +/**
> > +  Initializes MP exceptions handlers for the sake of stack switch requirement.
> > +
> > +  This function will allocate required resources for stack switch and pass
> > +  them through EXCEPTION_STACK_SWITCH_DATA to each logic processor.
> > +
> > +**/
> > +VOID
> > +InitializeMpExceptionStackSwitchHandlers (
> > +  VOID
> > +  )
> > +{
> > +  UINTN                           Index;
> > +  UINTN                           Bsp;
> > +  UINTN                           ExceptionNumber;
> > +  UINTN                           NewGdtSize;
> > +  UINTN                           NewStackSize;
> > +  IA32_DESCRIPTOR                 Gdtr;
> > +  EXCEPTION_STACK_SWITCH_DATA     EssData;
> > +  UINT8                           *GdtBuffer;
> > +  UINT8                           *StackTop;
> > +
> > +  if (!PcdGetBool (PcdCpuStackGuard)) {
> > +    return;
> > +  }
> > +
> > +  ExceptionNumber = FixedPcdGetSize (PcdCpuStackSwitchExceptionList);
> > +  NewStackSize = FixedPcdGet32 (PcdCpuKnownGoodStackSize) *
> > ExceptionNumber;
> > +
> > +  StackTop = AllocateRuntimeZeroPool (NewStackSize *
> > mNumberOfProcessors);
> > +  ASSERT (StackTop != NULL);
> > +  StackTop += NewStackSize  * mNumberOfProcessors;
> > +
> > +  EssData.Exceptions = FixedPcdGetPtr (PcdCpuStackSwitchExceptionList);
> > +  EssData.ExceptionNumber = ExceptionNumber;
> > +  EssData.StackSize = FixedPcdGet32 (PcdCpuKnownGoodStackSize);
> > +
> > +  MpInitLibWhoAmI (&Bsp);
> > +  for (Index = 0; Index < mNumberOfProcessors; ++Index) {
> > +    //
> > +    // To support stack switch, we need to re-construct GDT but not IDT.
> > +    //
> > +    if (Index == Bsp) {
> > +      GetGdtr (&Gdtr);
> > +    } else {
> > +      //
> > +      // AP might have different size of GDT from BSP.
> > +      //
> > +      MpInitLibStartupThisAP (GetGdtr, Index, NULL, 0, (VOID *)&Gdtr, NULL);
> > +    }
> > +
> > +    //
> > +    // 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. Since AP is not supposed to allocate
> > +    // memory, we have to do it in BSP. To simplify the code, we allocate
> > +    // memory for IA32 case to cover both IA32 and X64 exception stack
> > +    // switch.
> > +    //
> > +    // Layout of memory to allocate 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
> > +    //    |                              |
> > +    //    --------------------------------
> > +    //
> > +    NewGdtSize = sizeof (IA32_TSS_DESCRIPTOR) +
> > +                 (Gdtr.Limit + 1) +
> > +                 sizeof (IA32_TSS_DESCRIPTOR) * (ExceptionNumber + 1) +
> > +                 sizeof (IA32_TASK_STATE_SEGMENT) * (ExceptionNumber +
> > 1);
> > +    GdtBuffer = AllocateRuntimeZeroPool (NewGdtSize);
> > +    ASSERT (GdtBuffer != NULL);
> > +
> > +    EssData.GdtTable = ALIGN_POINTER(GdtBuffer, sizeof
> > (IA32_TSS_DESCRIPTOR));
> > +    NewGdtSize -= ((UINT8 *)EssData.GdtTable - GdtBuffer);
> > +    EssData.GdtSize = NewGdtSize;
> > +
> > +    EssData.TssDesc = (IA32_TSS_DESCRIPTOR *)((UINTN)EssData.GdtTable +
> > +                                              Gdtr.Limit + 1);
> > +    EssData.Tss = (IA32_TASK_STATE_SEGMENT *)((UINTN)EssData.GdtTable +
> > +                                              Gdtr.Limit + 1 +
> > +                                              sizeof
> > (IA32_TSS_DESCRIPTOR) *
> > +                                              (ExceptionNumber + 1));
> > +
> > +    EssData.StackTop = (UINTN)StackTop;
> > +    DEBUG ((DEBUG_INFO, "Exception stack top[%d]: 0x%lX\n", Index,
> > +            (UINT64)(UINTN)StackTop));
> > +
> > +    if (Index == Bsp) {
> > +      InitializeExceptionStackSwitchHandlers (&EssData);
> > +    } else {
> > +      MpInitLibStartupThisAP (
> > +        InitializeExceptionStackSwitchHandlers,
> > +        Index,
> > +        NULL,
> > +        0,
> > +        (VOID *)&EssData,
> > +        NULL
> > +        );
> > +    }
> > +
> > +    StackTop  -= NewStackSize;
> > +  }
> > +}
> > +
> >  /**
> >    Initialize Multi-processor support.
> >
> > @@ -624,6 +787,11 @@ InitializeMpSupport (
> >    mNumberOfProcessors = NumberOfProcessors;
> >    DEBUG ((DEBUG_INFO, "Detect CPU count: %d\n", mNumberOfProcessors));
> >
> > +  //
> > +  // Initialize exception stack switch handlers for each logic processor.
> > +  //
> > +  InitializeMpExceptionStackSwitchHandlers ();
> > +
> >    //
> >    // Update CPU healthy information from Guided HOB
> >    //
> > diff --git a/UefiCpuPkg/CpuDxe/CpuMp.h b/UefiCpuPkg/CpuDxe/CpuMp.h
> > index d530149d7e..86d54a95e9 100644
> > --- a/UefiCpuPkg/CpuDxe/CpuMp.h
> > +++ b/UefiCpuPkg/CpuDxe/CpuMp.h
> > @@ -15,6 +15,18 @@
> >  #ifndef _CPU_MP_H_
> >  #define _CPU_MP_H_
> >
> > +typedef struct {
> > +  UINTN                     StackTop;
> > +  UINTN                     StackSize;
> > +  UINT8                     *Exceptions;
> > +  UINTN                     ExceptionNumber;
> > +  IA32_IDT_GATE_DESCRIPTOR  *IdtTable;
> > +  IA32_SEGMENT_DESCRIPTOR   *GdtTable;
> > +  UINTN                     GdtSize;
> > +  IA32_TSS_DESCRIPTOR       *TssDesc;
> > +  IA32_TASK_STATE_SEGMENT   *Tss;
> > +} EXCEPTION_STACK_SWITCH_DATA;
> > +
> >  /**
> >    Initialize Multi-processor support.
> >
> > --
> > 2.14.1.windows.1

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 8/8] UefiCpuPkg/CpuDxe: Initialize stack switch for MP

Posted by Wang, Jian J 7 years, 1 month ago

About 1), the code is in [PATCH v2 7/8]. Following is part of it.

@@ -63,10 +67,34 @@ InitializeCpuExceptionHandlers (
   IN EFI_VECTOR_HANDOFF_INFO       *VectorInfo OPTIONAL
   )
 {
+  EFI_STATUS                        Status;
+  EXCEPTION_STACK_SWITCH_DATA       StackSwitchData;
+  IA32_DESCRIPTOR                   Idtr;
+  IA32_DESCRIPTOR                   Gdtr;
+
   mExceptionHandlerData.ReservedVectors          = mReservedVectorsData;
   mExceptionHandlerData.ExternalInterruptHandler = mExternalInterruptHandlerTable;
   InitializeSpinLock (&mExceptionHandlerData.DisplayMessageSpinLock);
-  return InitializeCpuExceptionHandlersWorker (VectorInfo, &mExceptionHandlerData);
+  Status = InitializeCpuExceptionHandlersWorker (VectorInfo, &mExceptionHandlerData);
+  if (!EFI_ERROR (Status) && PcdGetBool (PcdCpuStackGuard)) {
+    AsmReadIdtr (&Idtr);
+    AsmReadGdtr (&Gdtr);
+
+    StackSwitchData.StackTop = (UINTN)mNewStack;
+    StackSwitchData.StackSize = CPU_KNOWN_GOOD_STACK_SIZE;
+    StackSwitchData.Exceptions = CPU_STACK_SWITCH_EXCEPTION_LIST;
+    StackSwitchData.ExceptionNumber = CPU_STACK_SWITCH_EXCEPTION_NUMBER;
+    StackSwitchData.IdtTable = (IA32_IDT_GATE_DESCRIPTOR *)Idtr.Base;
+    StackSwitchData.GdtTable = (IA32_SEGMENT_DESCRIPTOR *)mNewGdt;
+    StackSwitchData.GdtSize = sizeof (mNewGdt);
+    StackSwitchData.TssDesc = (IA32_TSS_DESCRIPTOR *)(mNewGdt + Gdtr.Limit + 1);
+    StackSwitchData.Tss = (IA32_TASK_STATE_SEGMENT *)(mNewGdt + Gdtr.Limit + 1 +
+                                                      CPU_TSS_DESC_SIZE);
+    Status = InitializeCpuExceptionStackSwitchHandlers (
+               &StackSwitchData
+               );
+  }
+  return Status;
 }

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Wang,
> Jian J
> Sent: Thursday, November 23, 2017 1:04 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH v2 8/8] UefiCpuPkg/CpuDxe: Initialize stack switch
> for MP
> 
> Hi,
> 
> > -----Original Message-----
> > From: Yao, Jiewen
> > Sent: Thursday, November 23, 2017 12:14 PM
> > To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> > Kinney, Michael D <michael.d.kinney@intel.com>
> > Subject: RE: [PATCH v2 8/8] UefiCpuPkg/CpuDxe: Initialize stack switch for MP
> >
> > Hi
> > 1) Can we enable this feature in early DxeCore?
> >
> Yes. Intead of calling InitializeExceptionStackSwitchHandlers () directly in
> DxeCore,
> InitializeCpuExceptionHandlers() calls InitializeExceptionStackSwitchHandlers().
> 
> I think it's reasonable to do this because InitializeExceptionStackSwitchHandlers()
> is arch dependent. It'd be better not to call it in DxeCore. Another benefit is that
> this can avoid backward compatibility issue introduced by new API, which hasn't
> been implemented by cpu driver or lib of other archs.
> 
> > Current DxeCore still calling InitializeCpuExceptionHandlers().
> > But I hope InitializeExceptionStackSwitchHandlers() can be used here.
> >
> > In order to handle buffer from different arch, the DxeIpl can help provide some
> > data in hob and pass to DxeCore.
> >
> > 2) In addition, InitializeCpuExceptionHandlers () has VectorInfoList as
> parameter.
> > Do we also need that for InitializeExceptionStackSwitchHandlers()?
> >
> I don't see the need. Do you have any use cases in mind?
> 
> > Thank you
> > Yao Jiewen
> >
> > > -----Original Message-----
> > > From: Wang, Jian J
> > > Sent: Wednesday, November 22, 2017 4:46 PM
> > > To: edk2-devel@lists.01.org
> > > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> > Yao,
> > > Jiewen <jiewen.yao@intel.com>; Kinney, Michael D
> > > <michael.d.kinney@intel.com>
> > > Subject: [PATCH v2 8/8] UefiCpuPkg/CpuDxe: Initialize stack switch for MP
> > >
> > > > v2:
> > > >    Add code to reserve resources and initialize AP exception with stack
> > > >    switch besides BSP, if PcdCpuStackGuard is enabled.
> > >
> > > In current MP implementation, BSP and AP shares the same exception
> > > configuration. Stack switch required by Stack Guard feature needs that BSP
> > > and AP have their own configuration. This patch adds code to ask BSP and AP
> > > to do exception handler initialization separately.
> > >
> > > Cc: Eric Dong <eric.dong@intel.com>
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > Cc: Michael Kinney <michael.d.kinney@intel.com>
> > > Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com>
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > > ---
> > >  UefiCpuPkg/CpuDxe/CpuDxe.inf |   3 +
> > >  UefiCpuPkg/CpuDxe/CpuMp.c    | 168
> > > +++++++++++++++++++++++++++++++++++++++++++
> > >  UefiCpuPkg/CpuDxe/CpuMp.h    |  12 ++++
> > >  3 files changed, 183 insertions(+)
> > >
> > > diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf
> b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> > > index 3e8d196739..02f86b774c 100644
> > > --- a/UefiCpuPkg/CpuDxe/CpuDxe.inf
> > > +++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> > > @@ -81,6 +81,9 @@
> > >
> > >  [Pcd]
> > >
> > >
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
> > > ## CONSUMES
> > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard
> > > ## CONSUMES
> > > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuStackSwitchExceptionList
> > > ## CONSUMES
> > > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownGoodStackSize
> > > ## CONSUMES
> > >
> > >  [Depex]
> > >    TRUE
> > > diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c
> > > index b3c0178d07..6b2ceacb39 100644
> > > --- a/UefiCpuPkg/CpuDxe/CpuMp.c
> > > +++ b/UefiCpuPkg/CpuDxe/CpuMp.c
> > > @@ -601,6 +601,169 @@ CollectBistDataFromHob (
> > >    }
> > >  }
> > >
> > > +/**
> > > +  Get GDT register content.
> > > +
> > > +  This function is mainly for AP purpose because AP may have different GDT
> > > +  table than BSP.
> > > +
> > > +**/
> > > +VOID
> > > +EFIAPI
> > > +GetGdtr (
> > > +  IN OUT VOID *Buffer
> > > +  )
> > > +{
> > > +  AsmReadGdtr ((IA32_DESCRIPTOR *)Buffer);
> > > +}
> > > +
> > > +/**
> > > +  Initializes CPU exceptions handlers for the sake of stack switch
> requirement.
> > > +
> > > +  This function is a wrapper of InitializeCpuExceptionStackSwitchHandlers.
> > > +  It's mainly for AP purpose because of EFI_AP_PROCEDURE API
> requirement.
> > > +
> > > +**/
> > > +VOID
> > > +EFIAPI
> > > +InitializeExceptionStackSwitchHandlers (
> > > +  IN OUT VOID *Buffer
> > > +  )
> > > +{
> > > +  EXCEPTION_STACK_SWITCH_DATA     *EssData;
> > > +  IA32_DESCRIPTOR                 Idtr;
> > > +  EFI_STATUS                      Status;
> > > +
> > > +  EssData = Buffer;
> > > +  //
> > > +  // We don't plan to replace IDT table with a new one, and we don't
> assume
> > > +  // the AP's IDT is the same as BSP's IDT either.
> > > +  //
> > > +  AsmReadIdtr (&Idtr);
> > > +  EssData->IdtTable = (IA32_IDT_GATE_DESCRIPTOR *)Idtr.Base;
> > > +  Status = InitializeCpuExceptionStackSwitchHandlers (EssData);
> > > +  ASSERT_EFI_ERROR (Status);
> > > +}
> > > +
> > > +/**
> > > +  Initializes MP exceptions handlers for the sake of stack switch requirement.
> > > +
> > > +  This function will allocate required resources for stack switch and pass
> > > +  them through EXCEPTION_STACK_SWITCH_DATA to each logic processor.
> > > +
> > > +**/
> > > +VOID
> > > +InitializeMpExceptionStackSwitchHandlers (
> > > +  VOID
> > > +  )
> > > +{
> > > +  UINTN                           Index;
> > > +  UINTN                           Bsp;
> > > +  UINTN                           ExceptionNumber;
> > > +  UINTN                           NewGdtSize;
> > > +  UINTN                           NewStackSize;
> > > +  IA32_DESCRIPTOR                 Gdtr;
> > > +  EXCEPTION_STACK_SWITCH_DATA     EssData;
> > > +  UINT8                           *GdtBuffer;
> > > +  UINT8                           *StackTop;
> > > +
> > > +  if (!PcdGetBool (PcdCpuStackGuard)) {
> > > +    return;
> > > +  }
> > > +
> > > +  ExceptionNumber = FixedPcdGetSize (PcdCpuStackSwitchExceptionList);
> > > +  NewStackSize = FixedPcdGet32 (PcdCpuKnownGoodStackSize) *
> > > ExceptionNumber;
> > > +
> > > +  StackTop = AllocateRuntimeZeroPool (NewStackSize *
> > > mNumberOfProcessors);
> > > +  ASSERT (StackTop != NULL);
> > > +  StackTop += NewStackSize  * mNumberOfProcessors;
> > > +
> > > +  EssData.Exceptions = FixedPcdGetPtr (PcdCpuStackSwitchExceptionList);
> > > +  EssData.ExceptionNumber = ExceptionNumber;
> > > +  EssData.StackSize = FixedPcdGet32 (PcdCpuKnownGoodStackSize);
> > > +
> > > +  MpInitLibWhoAmI (&Bsp);
> > > +  for (Index = 0; Index < mNumberOfProcessors; ++Index) {
> > > +    //
> > > +    // To support stack switch, we need to re-construct GDT but not IDT.
> > > +    //
> > > +    if (Index == Bsp) {
> > > +      GetGdtr (&Gdtr);
> > > +    } else {
> > > +      //
> > > +      // AP might have different size of GDT from BSP.
> > > +      //
> > > +      MpInitLibStartupThisAP (GetGdtr, Index, NULL, 0, (VOID *)&Gdtr, NULL);
> > > +    }
> > > +
> > > +    //
> > > +    // 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. Since AP is not supposed to allocate
> > > +    // memory, we have to do it in BSP. To simplify the code, we allocate
> > > +    // memory for IA32 case to cover both IA32 and X64 exception stack
> > > +    // switch.
> > > +    //
> > > +    // Layout of memory to allocate 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
> > > +    //    |                              |
> > > +    //    --------------------------------
> > > +    //
> > > +    NewGdtSize = sizeof (IA32_TSS_DESCRIPTOR) +
> > > +                 (Gdtr.Limit + 1) +
> > > +                 sizeof (IA32_TSS_DESCRIPTOR) * (ExceptionNumber + 1) +
> > > +                 sizeof (IA32_TASK_STATE_SEGMENT) * (ExceptionNumber +
> > > 1);
> > > +    GdtBuffer = AllocateRuntimeZeroPool (NewGdtSize);
> > > +    ASSERT (GdtBuffer != NULL);
> > > +
> > > +    EssData.GdtTable = ALIGN_POINTER(GdtBuffer, sizeof
> > > (IA32_TSS_DESCRIPTOR));
> > > +    NewGdtSize -= ((UINT8 *)EssData.GdtTable - GdtBuffer);
> > > +    EssData.GdtSize = NewGdtSize;
> > > +
> > > +    EssData.TssDesc = (IA32_TSS_DESCRIPTOR *)((UINTN)EssData.GdtTable +
> > > +                                              Gdtr.Limit + 1);
> > > +    EssData.Tss = (IA32_TASK_STATE_SEGMENT *)((UINTN)EssData.GdtTable
> +
> > > +                                              Gdtr.Limit + 1 +
> > > +                                              sizeof
> > > (IA32_TSS_DESCRIPTOR) *
> > > +                                              (ExceptionNumber + 1));
> > > +
> > > +    EssData.StackTop = (UINTN)StackTop;
> > > +    DEBUG ((DEBUG_INFO, "Exception stack top[%d]: 0x%lX\n", Index,
> > > +            (UINT64)(UINTN)StackTop));
> > > +
> > > +    if (Index == Bsp) {
> > > +      InitializeExceptionStackSwitchHandlers (&EssData);
> > > +    } else {
> > > +      MpInitLibStartupThisAP (
> > > +        InitializeExceptionStackSwitchHandlers,
> > > +        Index,
> > > +        NULL,
> > > +        0,
> > > +        (VOID *)&EssData,
> > > +        NULL
> > > +        );
> > > +    }
> > > +
> > > +    StackTop  -= NewStackSize;
> > > +  }
> > > +}
> > > +
> > >  /**
> > >    Initialize Multi-processor support.
> > >
> > > @@ -624,6 +787,11 @@ InitializeMpSupport (
> > >    mNumberOfProcessors = NumberOfProcessors;
> > >    DEBUG ((DEBUG_INFO, "Detect CPU count: %d\n",
> mNumberOfProcessors));
> > >
> > > +  //
> > > +  // Initialize exception stack switch handlers for each logic processor.
> > > +  //
> > > +  InitializeMpExceptionStackSwitchHandlers ();
> > > +
> > >    //
> > >    // Update CPU healthy information from Guided HOB
> > >    //
> > > diff --git a/UefiCpuPkg/CpuDxe/CpuMp.h b/UefiCpuPkg/CpuDxe/CpuMp.h
> > > index d530149d7e..86d54a95e9 100644
> > > --- a/UefiCpuPkg/CpuDxe/CpuMp.h
> > > +++ b/UefiCpuPkg/CpuDxe/CpuMp.h
> > > @@ -15,6 +15,18 @@
> > >  #ifndef _CPU_MP_H_
> > >  #define _CPU_MP_H_
> > >
> > > +typedef struct {
> > > +  UINTN                     StackTop;
> > > +  UINTN                     StackSize;
> > > +  UINT8                     *Exceptions;
> > > +  UINTN                     ExceptionNumber;
> > > +  IA32_IDT_GATE_DESCRIPTOR  *IdtTable;
> > > +  IA32_SEGMENT_DESCRIPTOR   *GdtTable;
> > > +  UINTN                     GdtSize;
> > > +  IA32_TSS_DESCRIPTOR       *TssDesc;
> > > +  IA32_TASK_STATE_SEGMENT   *Tss;
> > > +} EXCEPTION_STACK_SWITCH_DATA;
> > > +
> > >  /**
> > >    Initialize Multi-processor support.
> > >
> > > --
> > > 2.14.1.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [edk2] [PATCH v2 8/8] UefiCpuPkg/CpuDxe: Initialize stack switch for MP

Posted by Yao, Jiewen 7 years, 1 month ago

Got it. I like this idea.
It is better to hide it from DxeCore.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Wang, Jian J
> Sent: Thursday, November 23, 2017 1:19 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Dong, Eric <eric.dong@intel.com>
> Subject: RE: [PATCH v2 8/8] UefiCpuPkg/CpuDxe: Initialize stack switch for MP
> 
> About 1), the code is in [PATCH v2 7/8]. Following is part of it.
> 
> @@ -63,10 +67,34 @@ InitializeCpuExceptionHandlers (
>    IN EFI_VECTOR_HANDOFF_INFO       *VectorInfo OPTIONAL
>    )
>  {
> +  EFI_STATUS                        Status;
> +  EXCEPTION_STACK_SWITCH_DATA       StackSwitchData;
> +  IA32_DESCRIPTOR                   Idtr;
> +  IA32_DESCRIPTOR                   Gdtr;
> +
>    mExceptionHandlerData.ReservedVectors          =
> mReservedVectorsData;
>    mExceptionHandlerData.ExternalInterruptHandler =
> mExternalInterruptHandlerTable;
>    InitializeSpinLock (&mExceptionHandlerData.DisplayMessageSpinLock);
> -  return InitializeCpuExceptionHandlersWorker (VectorInfo,
> &mExceptionHandlerData);
> +  Status = InitializeCpuExceptionHandlersWorker (VectorInfo,
> &mExceptionHandlerData);
> +  if (!EFI_ERROR (Status) && PcdGetBool (PcdCpuStackGuard)) {
> +    AsmReadIdtr (&Idtr);
> +    AsmReadGdtr (&Gdtr);
> +
> +    StackSwitchData.StackTop = (UINTN)mNewStack;
> +    StackSwitchData.StackSize = CPU_KNOWN_GOOD_STACK_SIZE;
> +    StackSwitchData.Exceptions = CPU_STACK_SWITCH_EXCEPTION_LIST;
> +    StackSwitchData.ExceptionNumber =
> CPU_STACK_SWITCH_EXCEPTION_NUMBER;
> +    StackSwitchData.IdtTable = (IA32_IDT_GATE_DESCRIPTOR *)Idtr.Base;
> +    StackSwitchData.GdtTable = (IA32_SEGMENT_DESCRIPTOR *)mNewGdt;
> +    StackSwitchData.GdtSize = sizeof (mNewGdt);
> +    StackSwitchData.TssDesc = (IA32_TSS_DESCRIPTOR *)(mNewGdt +
> Gdtr.Limit + 1);
> +    StackSwitchData.Tss = (IA32_TASK_STATE_SEGMENT *)(mNewGdt +
> Gdtr.Limit + 1 +
> +
> CPU_TSS_DESC_SIZE);
> +    Status = InitializeCpuExceptionStackSwitchHandlers (
> +               &StackSwitchData
> +               );
> +  }
> +  return Status;
>  }
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Wang,
> > Jian J
> > Sent: Thursday, November 23, 2017 1:04 PM
> > To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Laszlo Ersek
> > <lersek@redhat.com>; Dong, Eric <eric.dong@intel.com>
> > Subject: Re: [edk2] [PATCH v2 8/8] UefiCpuPkg/CpuDxe: Initialize stack switch
> > for MP
> >
> > Hi,
> >
> > > -----Original Message-----
> > > From: Yao, Jiewen
> > > Sent: Thursday, November 23, 2017 12:14 PM
> > > To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> > > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> > > Kinney, Michael D <michael.d.kinney@intel.com>
> > > Subject: RE: [PATCH v2 8/8] UefiCpuPkg/CpuDxe: Initialize stack switch for MP
> > >
> > > Hi
> > > 1) Can we enable this feature in early DxeCore?
> > >
> > Yes. Intead of calling InitializeExceptionStackSwitchHandlers () directly in
> > DxeCore,
> > InitializeCpuExceptionHandlers() calls InitializeExceptionStackSwitchHandlers().
> >
> > I think it's reasonable to do this because
> InitializeExceptionStackSwitchHandlers()
> > is arch dependent. It'd be better not to call it in DxeCore. Another benefit is
> that
> > this can avoid backward compatibility issue introduced by new API, which hasn't
> > been implemented by cpu driver or lib of other archs.
> >
> > > Current DxeCore still calling InitializeCpuExceptionHandlers().
> > > But I hope InitializeExceptionStackSwitchHandlers() can be used here.
> > >
> > > In order to handle buffer from different arch, the DxeIpl can help provide
> some
> > > data in hob and pass to DxeCore.
> > >
> > > 2) In addition, InitializeCpuExceptionHandlers () has VectorInfoList as
> > parameter.
> > > Do we also need that for InitializeExceptionStackSwitchHandlers()?
> > >
> > I don't see the need. Do you have any use cases in mind?
> >
> > > Thank you
> > > Yao Jiewen
> > >
> > > > -----Original Message-----
> > > > From: Wang, Jian J
> > > > Sent: Wednesday, November 22, 2017 4:46 PM
> > > > To: edk2-devel@lists.01.org
> > > > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> > > Yao,
> > > > Jiewen <jiewen.yao@intel.com>; Kinney, Michael D
> > > > <michael.d.kinney@intel.com>
> > > > Subject: [PATCH v2 8/8] UefiCpuPkg/CpuDxe: Initialize stack switch for MP
> > > >
> > > > > v2:
> > > > >    Add code to reserve resources and initialize AP exception with stack
> > > > >    switch besides BSP, if PcdCpuStackGuard is enabled.
> > > >
> > > > In current MP implementation, BSP and AP shares the same exception
> > > > configuration. Stack switch required by Stack Guard feature needs that BSP
> > > > and AP have their own configuration. This patch adds code to ask BSP and
> AP
> > > > to do exception handler initialization separately.
> > > >
> > > > Cc: Eric Dong <eric.dong@intel.com>
> > > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > > Cc: Michael Kinney <michael.d.kinney@intel.com>
> > > > Suggested-by: Ayellet Wolman <ayellet.wolman@intel.com>
> > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > > > ---
> > > >  UefiCpuPkg/CpuDxe/CpuDxe.inf |   3 +
> > > >  UefiCpuPkg/CpuDxe/CpuMp.c    | 168
> > > > +++++++++++++++++++++++++++++++++++++++++++
> > > >  UefiCpuPkg/CpuDxe/CpuMp.h    |  12 ++++
> > > >  3 files changed, 183 insertions(+)
> > > >
> > > > diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf
> > b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> > > > index 3e8d196739..02f86b774c 100644
> > > > --- a/UefiCpuPkg/CpuDxe/CpuDxe.inf
> > > > +++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> > > > @@ -81,6 +81,9 @@
> > > >
> > > >  [Pcd]
> > > >
> > > >
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
> > > > ## CONSUMES
> > > > +  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard
> > > > ## CONSUMES
> > > > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuStackSwitchExceptionList
> > > > ## CONSUMES
> > > > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownGoodStackSize
> > > > ## CONSUMES
> > > >
> > > >  [Depex]
> > > >    TRUE
> > > > diff --git a/UefiCpuPkg/CpuDxe/CpuMp.c b/UefiCpuPkg/CpuDxe/CpuMp.c
> > > > index b3c0178d07..6b2ceacb39 100644
> > > > --- a/UefiCpuPkg/CpuDxe/CpuMp.c
> > > > +++ b/UefiCpuPkg/CpuDxe/CpuMp.c
> > > > @@ -601,6 +601,169 @@ CollectBistDataFromHob (
> > > >    }
> > > >  }
> > > >
> > > > +/**
> > > > +  Get GDT register content.
> > > > +
> > > > +  This function is mainly for AP purpose because AP may have different
> GDT
> > > > +  table than BSP.
> > > > +
> > > > +**/
> > > > +VOID
> > > > +EFIAPI
> > > > +GetGdtr (
> > > > +  IN OUT VOID *Buffer
> > > > +  )
> > > > +{
> > > > +  AsmReadGdtr ((IA32_DESCRIPTOR *)Buffer);
> > > > +}
> > > > +
> > > > +/**
> > > > +  Initializes CPU exceptions handlers for the sake of stack switch
> > requirement.
> > > > +
> > > > +  This function is a wrapper of InitializeCpuExceptionStackSwitchHandlers.
> > > > +  It's mainly for AP purpose because of EFI_AP_PROCEDURE API
> > requirement.
> > > > +
> > > > +**/
> > > > +VOID
> > > > +EFIAPI
> > > > +InitializeExceptionStackSwitchHandlers (
> > > > +  IN OUT VOID *Buffer
> > > > +  )
> > > > +{
> > > > +  EXCEPTION_STACK_SWITCH_DATA     *EssData;
> > > > +  IA32_DESCRIPTOR                 Idtr;
> > > > +  EFI_STATUS                      Status;
> > > > +
> > > > +  EssData = Buffer;
> > > > +  //
> > > > +  // We don't plan to replace IDT table with a new one, and we don't
> > assume
> > > > +  // the AP's IDT is the same as BSP's IDT either.
> > > > +  //
> > > > +  AsmReadIdtr (&Idtr);
> > > > +  EssData->IdtTable = (IA32_IDT_GATE_DESCRIPTOR *)Idtr.Base;
> > > > +  Status = InitializeCpuExceptionStackSwitchHandlers (EssData);
> > > > +  ASSERT_EFI_ERROR (Status);
> > > > +}
> > > > +
> > > > +/**
> > > > +  Initializes MP exceptions handlers for the sake of stack switch
> requirement.
> > > > +
> > > > +  This function will allocate required resources for stack switch and pass
> > > > +  them through EXCEPTION_STACK_SWITCH_DATA to each logic
> processor.
> > > > +
> > > > +**/
> > > > +VOID
> > > > +InitializeMpExceptionStackSwitchHandlers (
> > > > +  VOID
> > > > +  )
> > > > +{
> > > > +  UINTN                           Index;
> > > > +  UINTN                           Bsp;
> > > > +  UINTN                           ExceptionNumber;
> > > > +  UINTN                           NewGdtSize;
> > > > +  UINTN                           NewStackSize;
> > > > +  IA32_DESCRIPTOR                 Gdtr;
> > > > +  EXCEPTION_STACK_SWITCH_DATA     EssData;
> > > > +  UINT8                           *GdtBuffer;
> > > > +  UINT8                           *StackTop;
> > > > +
> > > > +  if (!PcdGetBool (PcdCpuStackGuard)) {
> > > > +    return;
> > > > +  }
> > > > +
> > > > +  ExceptionNumber = FixedPcdGetSize
> (PcdCpuStackSwitchExceptionList);
> > > > +  NewStackSize = FixedPcdGet32 (PcdCpuKnownGoodStackSize) *
> > > > ExceptionNumber;
> > > > +
> > > > +  StackTop = AllocateRuntimeZeroPool (NewStackSize *
> > > > mNumberOfProcessors);
> > > > +  ASSERT (StackTop != NULL);
> > > > +  StackTop += NewStackSize  * mNumberOfProcessors;
> > > > +
> > > > +  EssData.Exceptions = FixedPcdGetPtr
> (PcdCpuStackSwitchExceptionList);
> > > > +  EssData.ExceptionNumber = ExceptionNumber;
> > > > +  EssData.StackSize = FixedPcdGet32 (PcdCpuKnownGoodStackSize);
> > > > +
> > > > +  MpInitLibWhoAmI (&Bsp);
> > > > +  for (Index = 0; Index < mNumberOfProcessors; ++Index) {
> > > > +    //
> > > > +    // To support stack switch, we need to re-construct GDT but not IDT.
> > > > +    //
> > > > +    if (Index == Bsp) {
> > > > +      GetGdtr (&Gdtr);
> > > > +    } else {
> > > > +      //
> > > > +      // AP might have different size of GDT from BSP.
> > > > +      //
> > > > +      MpInitLibStartupThisAP (GetGdtr, Index, NULL, 0, (VOID *)&Gdtr,
> NULL);
> > > > +    }
> > > > +
> > > > +    //
> > > > +    // 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. Since AP is not supposed to allocate
> > > > +    // memory, we have to do it in BSP. To simplify the code, we allocate
> > > > +    // memory for IA32 case to cover both IA32 and X64 exception stack
> > > > +    // switch.
> > > > +    //
> > > > +    // Layout of memory to allocate 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
> > > > +    //    |                              |
> > > > +    //    --------------------------------
> > > > +    //
> > > > +    NewGdtSize = sizeof (IA32_TSS_DESCRIPTOR) +
> > > > +                 (Gdtr.Limit + 1) +
> > > > +                 sizeof (IA32_TSS_DESCRIPTOR) * (ExceptionNumber +
> 1) +
> > > > +                 sizeof (IA32_TASK_STATE_SEGMENT) *
> (ExceptionNumber +
> > > > 1);
> > > > +    GdtBuffer = AllocateRuntimeZeroPool (NewGdtSize);
> > > > +    ASSERT (GdtBuffer != NULL);
> > > > +
> > > > +    EssData.GdtTable = ALIGN_POINTER(GdtBuffer, sizeof
> > > > (IA32_TSS_DESCRIPTOR));
> > > > +    NewGdtSize -= ((UINT8 *)EssData.GdtTable - GdtBuffer);
> > > > +    EssData.GdtSize = NewGdtSize;
> > > > +
> > > > +    EssData.TssDesc = (IA32_TSS_DESCRIPTOR
> *)((UINTN)EssData.GdtTable +
> > > > +                                              Gdtr.Limit + 1);
> > > > +    EssData.Tss = (IA32_TASK_STATE_SEGMENT
> *)((UINTN)EssData.GdtTable
> > +
> > > > +                                              Gdtr.Limit + 1 +
> > > > +                                              sizeof
> > > > (IA32_TSS_DESCRIPTOR) *
> > > > +                                              (ExceptionNumber +
> 1));
> > > > +
> > > > +    EssData.StackTop = (UINTN)StackTop;
> > > > +    DEBUG ((DEBUG_INFO, "Exception stack top[%d]: 0x%lX\n", Index,
> > > > +            (UINT64)(UINTN)StackTop));
> > > > +
> > > > +    if (Index == Bsp) {
> > > > +      InitializeExceptionStackSwitchHandlers (&EssData);
> > > > +    } else {
> > > > +      MpInitLibStartupThisAP (
> > > > +        InitializeExceptionStackSwitchHandlers,
> > > > +        Index,
> > > > +        NULL,
> > > > +        0,
> > > > +        (VOID *)&EssData,
> > > > +        NULL
> > > > +        );
> > > > +    }
> > > > +
> > > > +    StackTop  -= NewStackSize;
> > > > +  }
> > > > +}
> > > > +
> > > >  /**
> > > >    Initialize Multi-processor support.
> > > >
> > > > @@ -624,6 +787,11 @@ InitializeMpSupport (
> > > >    mNumberOfProcessors = NumberOfProcessors;
> > > >    DEBUG ((DEBUG_INFO, "Detect CPU count: %d\n",
> > mNumberOfProcessors));
> > > >
> > > > +  //
> > > > +  // Initialize exception stack switch handlers for each logic processor.
> > > > +  //
> > > > +  InitializeMpExceptionStackSwitchHandlers ();
> > > > +
> > > >    //
> > > >    // Update CPU healthy information from Guided HOB
> > > >    //
> > > > diff --git a/UefiCpuPkg/CpuDxe/CpuMp.h b/UefiCpuPkg/CpuDxe/CpuMp.h
> > > > index d530149d7e..86d54a95e9 100644
> > > > --- a/UefiCpuPkg/CpuDxe/CpuMp.h
> > > > +++ b/UefiCpuPkg/CpuDxe/CpuMp.h
> > > > @@ -15,6 +15,18 @@
> > > >  #ifndef _CPU_MP_H_
> > > >  #define _CPU_MP_H_
> > > >
> > > > +typedef struct {
> > > > +  UINTN                     StackTop;
> > > > +  UINTN                     StackSize;
> > > > +  UINT8                     *Exceptions;
> > > > +  UINTN                     ExceptionNumber;
> > > > +  IA32_IDT_GATE_DESCRIPTOR  *IdtTable;
> > > > +  IA32_SEGMENT_DESCRIPTOR   *GdtTable;
> > > > +  UINTN                     GdtSize;
> > > > +  IA32_TSS_DESCRIPTOR       *TssDesc;
> > > > +  IA32_TASK_STATE_SEGMENT   *Tss;
> > > > +} EXCEPTION_STACK_SWITCH_DATA;
> > > > +
> > > >  /**
> > > >    Initialize Multi-processor support.
> > > >
> > > > --
> > > > 2.14.1.windows.1
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel