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

["Wang, Jian J" <jian.j.wang@intel.com>]

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