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

["Yao, Jiewen" <jiewen.yao@intel.com>]

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