Re: [edk2] [PATCH 4/4] UefiCpuPkg/CpuMpPei: support stack guard feature

2018-09-05 Thread Wang, Jian J 
Thanks for the comments. It will be fixed.

Regards,
Jian

From: Dong, Eric
Sent: Thursday, September 06, 2018 9:46 AM
To: Wang, Jian J ; edk2-devel@lists.01.org
Cc: Laszlo Ersek ; Ni, Ruiyu ; Yao, 
Jiewen ; Zeng, Star ; Ware, Ryan R 

Subject: RE: [PATCH 4/4] UefiCpuPkg/CpuMpPei: support stack guard feature

Hi Jian,

Below code has ECC issue, please check with ECC tool to fix all ECC issues.

  /**
Initializes MP and exceptions handlers.

@retval EFI_SUCCESS MP was successfully initialized.
@retval others  Error occurred in MP initialization.

  **/
  EFI_STATUS
  InitializeCpuMpWorker (
IN CONST EFI_PEI_SERVICES **PeiServices
)

With ECC issues fixed, Reviewed-by: Eric Dong 
mailto:eric.d...@intel.com>>

Thanks,
Eric

> -Original Message-
> From: Wang, Jian J
> Sent: Monday, September 3, 2018 11:16 AM
> To: edk2-devel@lists.01.org
> Cc: Dong, Eric mailto:eric.d...@intel.com>>; Laszlo 
> Ersek mailto:ler...@redhat.com>>; Ni,
> Ruiyu mailto:ruiyu...@intel.com>>; Yao, Jiewen 
> mailto:jiewen@intel.com>>; Zeng,
> Star mailto:star.z...@intel.com>>; Ware, Ryan R 
> mailto:ryan.r.w...@intel.com>>
> Subject: [PATCH 4/4] UefiCpuPkg/CpuMpPei: support stack guard feature
>
> This feature is the same as Stack Guard enabled in driver CpuDxe but applies
> to PEI phase. Due to the specialty in PEI module dispatching, this driver is
> changed to do the actual initialization in notify callback of event
> gEfiPeiMemoryDiscoveredPpiGuid. This can let the stack guard apply to as
> most PEI drivers as possible.
>
> To let Stack Guard work, some simple page table management code are
> introduced to setup Guard page at base of stack for each processor.
>
> Cc: Eric Dong mailto:eric.d...@intel.com>>
> Cc: Laszlo Ersek mailto:ler...@redhat.com>>
> Cc: Ruiyu Ni mailto:ruiyu...@intel.com>>
> Cc: Jiewen Yao mailto:jiewen@intel.com>>
> Cc: Star Zeng mailto:star.z...@intel.com>>
> Cc: "Ware, Ryan R" mailto:ryan.r.w...@intel.com>>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> mailto:jian.j.w...@intel.com>>
> ---
>  UefiCpuPkg/CpuMpPei/CpuMpPei.c   | 269 -
>  UefiCpuPkg/CpuMpPei/CpuMpPei.h   |  14 +
>  UefiCpuPkg/CpuMpPei/CpuMpPei.inf |  11 +-
> UefiCpuPkg/CpuMpPei/CpuPaging.c  | 637
> +++
>  4 files changed, 916 insertions(+), 15 deletions(-)  create mode 100644
> UefiCpuPkg/CpuMpPei/CpuPaging.c
>
> diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> b/UefiCpuPkg/CpuMpPei/CpuMpPei.c index 7c94d5a6d7..e3762daf39 100644
> --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> @@ -409,25 +409,225 @@ PeiWhoAmI (
>  }
>
>  /**
> -  The Entry point of the MP CPU PEIM.
> +  Get GDT register value.
>
> -  This function will wakeup APs and collect CPU AP count and install the
> -  Mp Service Ppi.
> +  This function is mainly for AP purpose because AP may have different
> + GDT  table than BSP.
>
> -  @param  FileHandleHandle of the file being invoked.
> -  @param  PeiServices   Describes the list of possible PEI Services.
> +  @param[in,out] Buffer  The pointer to private data buffer.
>
> -  @retval EFI_SUCCESS   MpServicePpi is installed successfully.
> +**/
> +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 InitializeCpuExceptionHandlersEx. It's
> + mainly  for the sake of AP's init because of EFI_AP_PROCEDURE API
> requirement.
> +
> +  @param[in,out] Buffer  The pointer to private data buffer.
>
>  **/
> -EFI_STATUS
> +VOID
>  EFIAPI
> -CpuMpPeimInit (
> -  IN   EFI_PEI_FILE_HANDLE  FileHandle,
> +InitializeExceptionStackSwitchHandlers (
> +  IN OUT VOID *Buffer
> +  )
> +{
> +  CPU_EXCEPTION_INIT_DATA   *EssData;
> +  IA32_DESCRIPTOR   Idtr;
> +  EFI_STATUSStatus;
> +
> +  EssData = Buffer;
> +  //
> +  // We don't plan to replace IDT table with a new one, but we should
> +not assume
> +  // the AP's IDT is the same as BSP's IDT either.
> +  //
> +  AsmReadIdtr ();
> +  EssData->Ia32.IdtTable = (VOID *)Idtr.Base;
> +  EssData->Ia32.IdtTableSize = Idtr.Limit + 1;
> +  Status = InitializeCpuExceptionHandlersEx (NULL, EssData);
> +  ASSERT_EFI_ERROR (Status);
> +}
> +
> +/**
> +  Initializes MP exceptions handlers for the sake of stack switch 
> requirement.
> +
> +  This function will allocate required resources required to setup
> + stack switch  and pass them through CPU_EXCEPTION_INIT_DATA to each
> logic processor.
> +
> +**/
> +VOID
> +InitializeMpExceptionStackSwitchHandlers (
> +  VOID
> +  )
> +{
> +  EFI_STATUS  Status;
> +  UINTN   Index;
> +  UINTN   Bsp;
> +  UINTN

Re: [edk2] [PATCH 4/4] UefiCpuPkg/CpuMpPei: support stack guard feature

2018-09-05 Thread Dong, Eric 
Hi Jian,

Below code has ECC issue, please check with ECC tool to fix all ECC issues.

/**
Initializes MP and exceptions handlers.

@retval EFI_SUCCESS MP was successfully initialized.
@retval others  Error occurred in MP initialization.

**/
EFI_STATUS
InitializeCpuMpWorker (
  IN CONST EFI_PEI_SERVICES **PeiServices
 )

With ECC issues fixed, Reviewed-by: Eric Dong 

Thanks,
Eric

> -Original Message-
> From: Wang, Jian J
> Sent: Monday, September 3, 2018 11:16 AM
> To: edk2-devel@lists.01.org
> Cc: Dong, Eric ; Laszlo Ersek ; Ni,
> Ruiyu ; Yao, Jiewen ; Zeng,
> Star ; Ware, Ryan R 
> Subject: [PATCH 4/4] UefiCpuPkg/CpuMpPei: support stack guard feature
> 
> This feature is the same as Stack Guard enabled in driver CpuDxe but applies
> to PEI phase. Due to the specialty in PEI module dispatching, this driver is
> changed to do the actual initialization in notify callback of event
> gEfiPeiMemoryDiscoveredPpiGuid. This can let the stack guard apply to as
> most PEI drivers as possible.
> 
> To let Stack Guard work, some simple page table management code are
> introduced to setup Guard page at base of stack for each processor.
> 
> Cc: Eric Dong 
> Cc: Laszlo Ersek 
> Cc: Ruiyu Ni 
> Cc: Jiewen Yao 
> Cc: Star Zeng 
> Cc: "Ware, Ryan R" 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang 
> ---
>  UefiCpuPkg/CpuMpPei/CpuMpPei.c   | 269 -
>  UefiCpuPkg/CpuMpPei/CpuMpPei.h   |  14 +
>  UefiCpuPkg/CpuMpPei/CpuMpPei.inf |  11 +-
> UefiCpuPkg/CpuMpPei/CpuPaging.c  | 637
> +++
>  4 files changed, 916 insertions(+), 15 deletions(-)  create mode 100644
> UefiCpuPkg/CpuMpPei/CpuPaging.c
> 
> diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> b/UefiCpuPkg/CpuMpPei/CpuMpPei.c index 7c94d5a6d7..e3762daf39 100644
> --- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> +++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
> @@ -409,25 +409,225 @@ PeiWhoAmI (
>  }
> 
>  /**
> -  The Entry point of the MP CPU PEIM.
> +  Get GDT register value.
> 
> -  This function will wakeup APs and collect CPU AP count and install the
> -  Mp Service Ppi.
> +  This function is mainly for AP purpose because AP may have different
> + GDT  table than BSP.
> 
> -  @param  FileHandleHandle of the file being invoked.
> -  @param  PeiServices   Describes the list of possible PEI Services.
> +  @param[in,out] Buffer  The pointer to private data buffer.
> 
> -  @retval EFI_SUCCESS   MpServicePpi is installed successfully.
> +**/
> +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 InitializeCpuExceptionHandlersEx. It's
> + mainly  for the sake of AP's init because of EFI_AP_PROCEDURE API
> requirement.
> +
> +  @param[in,out] Buffer  The pointer to private data buffer.
> 
>  **/
> -EFI_STATUS
> +VOID
>  EFIAPI
> -CpuMpPeimInit (
> -  IN   EFI_PEI_FILE_HANDLE  FileHandle,
> +InitializeExceptionStackSwitchHandlers (
> +  IN OUT VOID *Buffer
> +  )
> +{
> +  CPU_EXCEPTION_INIT_DATA   *EssData;
> +  IA32_DESCRIPTOR   Idtr;
> +  EFI_STATUSStatus;
> +
> +  EssData = Buffer;
> +  //
> +  // We don't plan to replace IDT table with a new one, but we should
> +not assume
> +  // the AP's IDT is the same as BSP's IDT either.
> +  //
> +  AsmReadIdtr ();
> +  EssData->Ia32.IdtTable = (VOID *)Idtr.Base;
> +  EssData->Ia32.IdtTableSize = Idtr.Limit + 1;
> +  Status = InitializeCpuExceptionHandlersEx (NULL, EssData);
> +  ASSERT_EFI_ERROR (Status);
> +}
> +
> +/**
> +  Initializes MP exceptions handlers for the sake of stack switch 
> requirement.
> +
> +  This function will allocate required resources required to setup
> + stack switch  and pass them through CPU_EXCEPTION_INIT_DATA to each
> logic processor.
> +
> +**/
> +VOID
> +InitializeMpExceptionStackSwitchHandlers (
> +  VOID
> +  )
> +{
> +  EFI_STATUS  Status;
> +  UINTN   Index;
> +  UINTN   Bsp;
> +  UINTN   ExceptionNumber;
> +  UINTN   OldGdtSize;
> +  UINTN   NewGdtSize;
> +  UINTN   NewStackSize;
> +  IA32_DESCRIPTOR Gdtr;
> +  CPU_EXCEPTION_INIT_DATA EssData;
> +  UINT8   *GdtBuffer;
> +  UINT8   *StackTop;
> +  UINTN   NumberOfProcessors;
> +
> +  if (!PcdGetBool (PcdCpuStackGuard)) {
> +return;
> +  }
> +
> +  MpInitLibGetNumberOfProcessors(, NULL);
> + MpInitLibWhoAmI ();
> +
> +  ExceptionNumber = FixedPcdGetSize (PcdCpuStackSwitchExceptionList);
> + NewStackSize = FixedPcdGet32 (PcdCpuKnownGoodStackSize) *
> +

[edk2] [PATCH 4/4] UefiCpuPkg/CpuMpPei: support stack guard feature

2018-09-02 Thread Jian J Wang 
This feature is the same as Stack Guard enabled in driver CpuDxe but
applies to PEI phase. Due to the specialty in PEI module dispatching,
this driver is changed to do the actual initialization in notify
callback of event gEfiPeiMemoryDiscoveredPpiGuid. This can let the
stack guard apply to as most PEI drivers as possible.

To let Stack Guard work, some simple page table management code are
introduced to setup Guard page at base of stack for each processor.

Cc: Eric Dong 
Cc: Laszlo Ersek 
Cc: Ruiyu Ni 
Cc: Jiewen Yao 
Cc: Star Zeng 
Cc: "Ware, Ryan R" 
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang 
---
 UefiCpuPkg/CpuMpPei/CpuMpPei.c   | 269 -
 UefiCpuPkg/CpuMpPei/CpuMpPei.h   |  14 +
 UefiCpuPkg/CpuMpPei/CpuMpPei.inf |  11 +-
 UefiCpuPkg/CpuMpPei/CpuPaging.c  | 637 +++
 4 files changed, 916 insertions(+), 15 deletions(-)
 create mode 100644 UefiCpuPkg/CpuMpPei/CpuPaging.c

diff --git a/UefiCpuPkg/CpuMpPei/CpuMpPei.c b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
index 7c94d5a6d7..e3762daf39 100644
--- a/UefiCpuPkg/CpuMpPei/CpuMpPei.c
+++ b/UefiCpuPkg/CpuMpPei/CpuMpPei.c
@@ -409,25 +409,225 @@ PeiWhoAmI (
 }
 
 /**
-  The Entry point of the MP CPU PEIM.
+  Get GDT register value.
 
-  This function will wakeup APs and collect CPU AP count and install the
-  Mp Service Ppi.
+  This function is mainly for AP purpose because AP may have different GDT
+  table than BSP.
 
-  @param  FileHandleHandle of the file being invoked.
-  @param  PeiServices   Describes the list of possible PEI Services.
+  @param[in,out] Buffer  The pointer to private data buffer.
 
-  @retval EFI_SUCCESS   MpServicePpi is installed successfully.
+**/
+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 InitializeCpuExceptionHandlersEx. It's mainly
+  for the sake of AP's init because of EFI_AP_PROCEDURE API requirement.
+
+  @param[in,out] Buffer  The pointer to private data buffer.
 
 **/
-EFI_STATUS
+VOID
 EFIAPI
-CpuMpPeimInit (
-  IN   EFI_PEI_FILE_HANDLE  FileHandle,
+InitializeExceptionStackSwitchHandlers (
+  IN OUT VOID *Buffer
+  )
+{
+  CPU_EXCEPTION_INIT_DATA   *EssData;
+  IA32_DESCRIPTOR   Idtr;
+  EFI_STATUSStatus;
+
+  EssData = Buffer;
+  //
+  // We don't plan to replace IDT table with a new one, but we should not 
assume
+  // the AP's IDT is the same as BSP's IDT either.
+  //
+  AsmReadIdtr ();
+  EssData->Ia32.IdtTable = (VOID *)Idtr.Base;
+  EssData->Ia32.IdtTableSize = Idtr.Limit + 1;
+  Status = InitializeCpuExceptionHandlersEx (NULL, EssData);
+  ASSERT_EFI_ERROR (Status);
+}
+
+/**
+  Initializes MP exceptions handlers for the sake of stack switch requirement.
+
+  This function will allocate required resources required to setup stack switch
+  and pass them through CPU_EXCEPTION_INIT_DATA to each logic processor.
+
+**/
+VOID
+InitializeMpExceptionStackSwitchHandlers (
+  VOID
+  )
+{
+  EFI_STATUS  Status;
+  UINTN   Index;
+  UINTN   Bsp;
+  UINTN   ExceptionNumber;
+  UINTN   OldGdtSize;
+  UINTN   NewGdtSize;
+  UINTN   NewStackSize;
+  IA32_DESCRIPTOR Gdtr;
+  CPU_EXCEPTION_INIT_DATA EssData;
+  UINT8   *GdtBuffer;
+  UINT8   *StackTop;
+  UINTN   NumberOfProcessors;
+
+  if (!PcdGetBool (PcdCpuStackGuard)) {
+return;
+  }
+
+  MpInitLibGetNumberOfProcessors(, NULL);
+  MpInitLibWhoAmI ();
+
+  ExceptionNumber = FixedPcdGetSize (PcdCpuStackSwitchExceptionList);
+  NewStackSize = FixedPcdGet32 (PcdCpuKnownGoodStackSize) * ExceptionNumber;
+
+  Status = PeiServicesAllocatePool (
+ NewStackSize * NumberOfProcessors,
+ (VOID **)
+ );
+  ASSERT(StackTop != NULL);
+  if (EFI_ERROR (Status)) {
+ASSERT_EFI_ERROR (Status);
+return;
+  }
+  StackTop += NewStackSize  * NumberOfProcessors;
+
+  //
+  // The default exception handlers must have been initialized. Let's just skip
+  // it in this method.
+  //
+  EssData.Ia32.Revision = CPU_EXCEPTION_INIT_DATA_REV;
+  EssData.Ia32.InitDefaultHandlers = FALSE;
+
+  EssData.Ia32.StackSwitchExceptions = 
FixedPcdGetPtr(PcdCpuStackSwitchExceptionList);
+  EssData.Ia32.StackSwitchExceptionNumber = ExceptionNumber;
+  EssData.Ia32.KnownGoodStackSize = FixedPcdGet32(PcdCpuKnownGoodStackSize);
+
+  //
+  // Initialize Gdtr to suppress incorrect compiler/analyzer warnings.
+  //
+  Gdtr.Base = 0;
+  Gdtr.Limit = 0;
+  for (Index = 0; Index < NumberOfProcessors; ++Index) {
+//
+// To support stack switch, we need to re-construct GDT but not IDT.
+//
+if