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.w...@intel.com>; Yao, Jiewen <jiewen....@intel.com>;
> edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Laszlo Ersek
> <ler...@redhat.com>; Dong, Eric <eric.d...@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-boun...@lists.01.org] On Behalf Of
> Wang,
> > Jian J
> > Sent: Thursday, November 23, 2017 1:04 PM
> > To: Yao, Jiewen <jiewen....@intel.com>; edk2-devel@lists.01.org
> > Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Laszlo Ersek
> > <ler...@redhat.com>; Dong, Eric <eric.d...@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.w...@intel.com>; edk2-devel@lists.01.org
> > > Cc: Dong, Eric <eric.d...@intel.com>; Laszlo Ersek <ler...@redhat.com>;
> > > Kinney, Michael D <michael.d.kin...@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.d...@intel.com>; Laszlo Ersek <ler...@redhat.com>;
> > > Yao,
> > > > Jiewen <jiewen....@intel.com>; Kinney, Michael D
> > > > <michael.d.kin...@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.d...@intel.com>
> > > > Cc: Laszlo Ersek <ler...@redhat.com>
> > > > Cc: Jiewen Yao <jiewen....@intel.com>
> > > > Cc: Michael Kinney <michael.d.kin...@intel.com>
> > > > Suggested-by: Ayellet Wolman <ayellet.wol...@intel.com>
> > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > Signed-off-by: Jian J Wang <jian.j.w...@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
