Re: [PATCH 2/2] UefiCpuPkg/CpuExceptionHandlerLib: Avoid calling PEI services from AP

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

Validated-by: Jian J Wang <jian.j.wang@intel.com>
Reviewed-by: Jian J Wang <jian.j.wang@intel.com>

> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Friday, August 31, 2018 4:36 PM
> To: edk2-devel@lists.01.org
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Fan Jeff <vanjeff_919@hotmail.com>
> Subject: [PATCH 2/2] UefiCpuPkg/CpuExceptionHandlerLib: Avoid calling PEI
> services from AP
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1128
> 
> When an exception happens in AP, system hangs at
> GetPeiServicesTablePointer(), complaining the PeiServices retrieved
> from memory before IDT is NULL.
> 
> Due to the following commit:
> c563077a380437c114aba4c95be65eb963ebc1f3
> * UefiCpuPkg/MpInitLib: Avoid calling PEI services from AP
> the IDT used by AP no longer preserve PeiServices pointer in the
> very beginning.
> But the implementation of PeiExceptionHandlerLib still assumes
> the PeiServices pointer is there, so the assertion happens.
> 
> The patch fixes the exception handler library to not call
> PEI services from AP.
> 
> The patch duplicates the #0 exception stub header in an allocated
> pool but with extra 4-byte/8-byte to store the exception handler
> data which was originally stored in HOB.
> When AP exception happens, the code gets the exception handler data
> from the exception handler for #0.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Fan Jeff <vanjeff_919@hotmail.com>
> ---
>  .../CpuExceptionHandlerLib/CpuExceptionCommon.h    |  7 +-
>  .../CpuExceptionHandlerLib/PeiCpuException.c       | 77 +++++++++++++++-------
>  2 files changed, 55 insertions(+), 29 deletions(-)
> 
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
> index e10d9379d5..459f06ac84 100644
> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/CpuExceptionCommon.h
> @@ -1,7 +1,7 @@
>  /** @file
>    Common header file for CPU Exception Handler Library.
> 
> -  Copyright (c) 2012 - 2017, Intel Corporation. All rights reserved.<BR>
> +  Copyright (c) 2012 - 2018, Intel Corporation. All rights reserved.<BR>
>    This program and the accompanying materials
>    are licensed and made available under the terms and conditions of the BSD
> License
>    which accompanies this distribution.  The full text of the license may be found
> at
> @@ -43,11 +43,6 @@
> 
>  #include "ArchInterruptDefs.h"
> 
> -#define CPU_EXCEPTION_HANDLER_LIB_HOB_GUID \
> -  { \
> -    0xb21d9148, 0x9211, 0x4d8f, { 0xad, 0xd3, 0x66, 0xb1, 0x89, 0xc9, 0x2c,
> 0x83 } \
> -  }
> -
>  #define CPU_STACK_SWITCH_EXCEPTION_NUMBER \
>    FixedPcdGetSize (PcdCpuStackSwitchExceptionList)
> 
> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuException.c
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuException.c
> index 6f271983f2..5dd8423d2f 100644
> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuException.c
> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuException.c
> @@ -1,7 +1,7 @@
>  /** @file
>    CPU exception handler library implementation for PEIM module.
> 
> -Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
>  This program and the accompanying materials are licensed and made available
> under
>  the terms and conditions of the BSD License that accompanies this distribution.
>  The full text of the license may be found at
> @@ -20,10 +20,17 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
> KIND, EITHER EXPRESS OR IMPLIED.
> 
>  CONST UINTN    mDoFarReturnFlag  = 0;
> 
> -EFI_GUID mCpuExceptrionHandlerLibHobGuid =
> CPU_EXCEPTION_HANDLER_LIB_HOB_GUID;
> +typedef struct {
> +  UINT8                   ExceptionStubHeader[HOOKAFTER_STUB_SIZE];
> +  EXCEPTION_HANDLER_DATA  *ExceptionHandlerData;
> +} EXCEPTION0_STUB_HEADER;
> 
>  /**
> -  Get exception handler data pointer from GUIDed HOb.
> +  Get exception handler data pointer from IDT[0].
> +
> +  The exception #0 stub header is duplicated in an allocated pool with extra 4-
> byte/8-byte to store the
> +  exception handler data. The new allocated memory layout follows structure
> EXCEPTION0_STUB_HEADER.
> +  The code assumes that all processors uses the same exception handler for #0
> exception.
> 
>    @return  pointer to exception handler data.
>  **/
> @@ -32,18 +39,50 @@ GetExceptionHandlerData (
>    VOID
>    )
>  {
> -  EFI_HOB_GUID_TYPE       *GuidHob;
> -  VOID                    *DataInHob;
> -  EXCEPTION_HANDLER_DATA  *ExceptionHandlerData;
> +  IA32_DESCRIPTOR                  IdtDescriptor;
> +  IA32_IDT_GATE_DESCRIPTOR         *IdtTable;
> +  EXCEPTION0_STUB_HEADER           *Exception0StubHeader;
> +
> +  AsmReadIdtr (&IdtDescriptor);
> +  IdtTable = (IA32_IDT_GATE_DESCRIPTOR *)IdtDescriptor.Base;
> +
> +  Exception0StubHeader = (EXCEPTION0_STUB_HEADER *)ArchGetIdtHandler
> (&IdtTable[0]);
> +  return Exception0StubHeader->ExceptionHandlerData;
> +}
> 
> -  ExceptionHandlerData = NULL;
> -  GuidHob = GetFirstGuidHob (&mCpuExceptrionHandlerLibHobGuid);
> -  if (GuidHob != NULL) {
> -    DataInHob = GET_GUID_HOB_DATA (GuidHob);
> -    ExceptionHandlerData = (EXCEPTION_HANDLER_DATA *)(*(UINTN
> *)DataInHob);
> -  }
> -  ASSERT (ExceptionHandlerData != NULL);
> -  return ExceptionHandlerData;
> +/**
> +  Set exception handler data pointer to IDT[0].
> +
> +  The exception #0 stub header is duplicated in an allocated pool with extra 4-
> byte/8-byte to store the
> +  exception handler data. The new allocated memory layout follows structure
> EXCEPTION0_STUB_HEADER.
> +  The code assumes that all processors uses the same exception handler for #0
> exception.
> +
> +  @param  pointer to exception handler data.
> +**/
> +VOID
> +SetExceptionHandlerData (
> +  IN EXCEPTION_HANDLER_DATA        *ExceptionHandlerData
> +  )
> +{
> +  EXCEPTION0_STUB_HEADER           *Exception0StubHeader;
> +  IA32_DESCRIPTOR                  IdtDescriptor;
> +  IA32_IDT_GATE_DESCRIPTOR         *IdtTable;
> +  //
> +  // Duplicate the exception #0 stub header in pool and cache the
> ExceptionHandlerData just after the stub header.
> +  // So AP can get the ExceptionHandlerData by reading the IDT[0].
> +  //
> +  AsmReadIdtr (&IdtDescriptor);
> +  IdtTable = (IA32_IDT_GATE_DESCRIPTOR *)IdtDescriptor.Base;
> +
> +  Exception0StubHeader = AllocatePool (sizeof (*Exception0StubHeader));
> +  ASSERT (Exception0StubHeader != NULL);
> +  CopyMem (
> +    Exception0StubHeader->ExceptionStubHeader,
> +    (VOID *)ArchGetIdtHandler (&IdtTable[0]),
> +    sizeof (Exception0StubHeader->ExceptionStubHeader)
> +    );
> +  Exception0StubHeader->ExceptionHandlerData = ExceptionHandlerData;
> +  ArchUpdateIdtEntry (&IdtTable[0], (UINTN)Exception0StubHeader-
> >ExceptionStubHeader);
>  }
> 
>  /**
> @@ -109,15 +148,7 @@ InitializeCpuExceptionHandlers (
>      return Status;
>    }
> 
> -  //
> -  // Build location of CPU MP DATA buffer in HOB
> -  //
> -  BuildGuidDataHob (
> -    &mCpuExceptrionHandlerLibHobGuid,
> -    (VOID *)&ExceptionHandlerData,
> -    sizeof(UINT64)
> -    );
> -
> +  SetExceptionHandlerData (ExceptionHandlerData);
>    return EFI_SUCCESS;
>  }
> 
> --
> 2.16.1.windows.1