Re: [edk2-platforms] [PATCH V1 09/13] MinPlatformPkg: FSP Dispatch Mode Support for PlatformSecLib

["Kubacki, Michael A" <michael.a.kubacki@intel.com>]

FspWrapperPlatformSecLib.c:
* Line 179: Given the other whitespace fixes, there's one missing here after sizeof.
    CopyMem (CopyDestinationPointer, mPeiSecPlatformPpi, sizeof(mPeiSecPlatformPpi));

It does not have to be fixed in this commit.

Reviewed-by: Michael Kubacki <michael.a.kubacki@intel.com>

> -----Original Message-----
> From: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
> Sent: Wednesday, November 13, 2019 10:07 PM
> To: devel@edk2.groups.io
> Cc: Kubacki, Michael A <michael.a.kubacki@intel.com>; Chiu, Chasel
> <chasel.chiu@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: [edk2-platforms] [PATCH V1 09/13] MinPlatformPkg: FSP Dispatch
> Mode Support for PlatformSecLib
> 
> Cc: Michael Kubacki <michael.a.kubacki@intel.com>
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Signed-off-by: Nate DeSimone <nathaniel.l.desimone@intel.com>
> ---
>  .../FspWrapperPlatformSecLib.c                | 34 ++++++++++++---
>  .../SecFspWrapperPlatformSecLib.inf           |  7 +++-
>  .../SecTempRamDone.c                          | 42 +++++++++++++++----
>  .../Intel/MinPlatformPkg/MinPlatformPkg.dec   | 28 ++++++++++++-
>  4 files changed, 95 insertions(+), 16 deletions(-)
> 
> diff --git
> a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> ormSecLib/FspWrapperPlatformSecLib.c
> b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> ormSecLib/FspWrapperPlatformSecLib.c
> index 303f3aac40..876c073fc4 100644
> ---
> a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> ormSecLib/FspWrapperPlatformSecLib.c
> +++
> b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlat
> +++ formSecLib/FspWrapperPlatformSecLib.c
> @@ -1,7 +1,7 @@
>  /** @file   Provide FSP wrapper platform sec related function. -Copyright (c)
> 2017, Intel Corporation. All rights reserved.<BR>+Copyright (c) 2017 - 2019,
> Intel Corporation. All rights reserved.<BR> SPDX-License-Identifier: BSD-2-
> Clause-Patent  **/@@ -12,6 +12,7 @@ SPDX-License-Identifier: BSD-2-
> Clause-Patent
>  #include <Ppi/SecPerformance.h> #include <Ppi/FirmwareVolumeInfo.h>
> #include <Ppi/TopOfTemporaryRam.h>+#include <Ppi/PeiCoreFvLocation.h>
> #include <Guid/FirmwareFileSystem2.h>  #include
> <Library/LocalApicLib.h>@@ -66,6 +67,18 @@ PEI_SEC_PERFORMANCE_PPI
> mSecPerformancePpi = {
>    SecGetPerformance }; +EFI_PEI_CORE_FV_LOCATION_PPI
> mPeiCoreFvLocationPpi = {+  (VOID *) (UINTN) FixedPcdGet32
> (PcdFspmBaseAddress)+};++EFI_PEI_PPI_DESCRIPTOR
> mPeiCoreFvLocationPpiList[] = {+  {+    EFI_PEI_PPI_DESCRIPTOR_PPI,+
> &gEfiPeiCoreFvLocationPpiGuid,+    &mPeiCoreFvLocationPpi+  }+};+
> EFI_PEI_PPI_DESCRIPTOR  mPeiSecPlatformPpi[] = {   {
> EFI_PEI_PPI_DESCRIPTOR_PPI,@@ -129,6 +142,8 @@ SecPlatformMain (
>    ) {   EFI_PEI_PPI_DESCRIPTOR      *PpiList;+  UINT8
> TopOfTemporaryRamPpiIndex;+  UINT8                       *CopyDestinationPointer;
> DEBUG ((DEBUG_INFO, "FSP Wrapper BootFirmwareVolumeBase - 0x%x\n",
> SecCoreData->BootFirmwareVolumeBase));   DEBUG ((DEBUG_INFO, "FSP
> Wrapper BootFirmwareVolumeSize - 0x%x\n", SecCoreData-
> >BootFirmwareVolumeSize));@@ -150,13 +165,22 @@ SecPlatformMain (
>    // Use middle of Heap as temp buffer, it will be copied by caller.   // Do not
> use Stack, because it will cause wrong calculation on stack by PeiCore   //-
> PpiList = (VOID *)((UINTN)SecCoreData->PeiTemporaryRamBase +
> (UINTN)SecCoreData->PeiTemporaryRamSize/2);-  CopyMem (PpiList,
> mPeiSecPlatformPpi, sizeof(mPeiSecPlatformPpi));-+  PpiList = (VOID
> *)((UINTN) SecCoreData->PeiTemporaryRamBase + (UINTN) SecCoreData-
> >PeiTemporaryRamSize/2);+  CopyDestinationPointer = (UINT8 *) PpiList;+
> TopOfTemporaryRamPpiIndex = 0;+  if ((PcdGet8 (PcdFspModeSelection) ==
> 0) && PcdGetBool (PcdFspDispatchModeUseFspPeiMain)) {+    //+    // In
> Dispatch mode, wrapper should provide PeiCoreFvLocationPpi.+    //+
> CopyMem (CopyDestinationPointer, mPeiCoreFvLocationPpiList, sizeof
> (mPeiCoreFvLocationPpiList));+    TopOfTemporaryRamPpiIndex = 1;+
> CopyDestinationPointer += sizeof (mPeiCoreFvLocationPpiList);+  }+
> CopyMem (CopyDestinationPointer, mPeiSecPlatformPpi,
> sizeof(mPeiSecPlatformPpi));   //   // Patch TopOfTemporaryRamPpi   //-
> PpiList[0].Ppi = (VOID *)((UINTN)SecCoreData->TemporaryRamBase +
> SecCoreData->TemporaryRamSize);+
> PpiList[TopOfTemporaryRamPpiIndex].Ppi = (VOID *)((UINTN) SecCoreData-
> >TemporaryRamBase + SecCoreData->TemporaryRamSize);    return PpiList;
> }diff --git
> a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> ormSecLib/SecFspWrapperPlatformSecLib.inf
> b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> ormSecLib/SecFspWrapperPlatformSecLib.inf
> index 3f5a63f273..02c720c73d 100644
> ---
> a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> ormSecLib/SecFspWrapperPlatformSecLib.inf
> +++
> b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlat
> +++ formSecLib/SecFspWrapperPlatformSecLib.inf
> @@ -72,18 +72,20 @@
>    BoardInitLib   SecBoardInitLib   TestPointCheckLib+
> PeiServicesTablePointerLib  [Ppis]   gEfiSecPlatformInformationPpiGuid       ##
> CONSUMES   gPeiSecPerformancePpiGuid               ## CONSUMES
> gTopOfTemporaryRamPpiGuid               ## PRODUCES
> gEfiPeiFirmwareVolumeInfoPpiGuid        ## PRODUCES+
> gFspTempRamExitPpiGuid                  ## CONSUMES  [Pcd]
> gUefiCpuPkgTokenSpaceGuid.PcdPeiTemporaryRamStackSize               ##
> CONSUMES   gIntelFsp2WrapperTokenSpaceGuid.PcdFsptBaseAddress
> ## CONSUMES   gIntelFsp2PkgTokenSpaceGuid.PcdFspTemporaryRamSize
> ## CONSUMES-
> gMinPlatformPkgTokenSpaceGuid.PcdSecSerialPortDebugEnable        ##
> CONSUMES+
> gMinPlatformPkgTokenSpaceGuid.PcdSecSerialPortDebugEnable           ##
> CONSUMES  [FixedPcd]
> gIntelFsp2WrapperTokenSpaceGuid.PcdCpuMicrocodePatchAddress         ##
> CONSUMES@@ -91,3 +93,6 @@
>    gIntelFsp2WrapperTokenSpaceGuid.PcdFlashMicrocodeOffset             ##
> CONSUMES
> gIntelFsp2WrapperTokenSpaceGuid.PcdFlashCodeCacheAddress            ##
> CONSUMES   gIntelFsp2WrapperTokenSpaceGuid.PcdFlashCodeCacheSize
> ## CONSUMES+  gIntelFsp2WrapperTokenSpaceGuid.PcdFspmBaseAddress
> ## CONSUMES+  gIntelFsp2WrapperTokenSpaceGuid.PcdFspModeSelection
> ## CONSUMES+
> gMinPlatformPkgTokenSpaceGuid.PcdFspDispatchModeUseFspPeiMain
> ## CONSUMESdiff --git
> a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> ormSecLib/SecTempRamDone.c
> b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> ormSecLib/SecTempRamDone.c
> index cde8a80a4e..922e4ec204 100644
> ---
> a/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlatf
> ormSecLib/SecTempRamDone.c
> +++
> b/Platform/Intel/MinPlatformPkg/FspWrapper/Library/SecFspWrapperPlat
> +++ formSecLib/SecTempRamDone.c
> @@ -1,7 +1,7 @@
>  /** @file   Provide SecTemporaryRamDone function. -Copyright (c) 2017,
> Intel Corporation. All rights reserved.<BR>+Copyright (c) 2017 - 2019, Intel
> Corporation. All rights reserved.<BR> SPDX-License-Identifier: BSD-2-Clause-
> Patent  **/@@ -9,6 +9,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include <PiPei.h>  #include <Ppi/TemporaryRamDone.h>+#include
> <Ppi/TempRamExitPpi.h>  #include <Library/BaseMemoryLib.h> #include
> <Library/DebugLib.h>@@ -17,6 +18,7 @@ SPDX-License-Identifier: BSD-2-
> Clause-Patent  #include <Library/FspWrapperPlatformLib.h> #include
> <Library/FspWrapperApiLib.h> #include <Library/BoardInitLib.h>+#include
> <Library/PeiServicesTablePointerLib.h>  /** This interface disables
> temporary memory in SEC Phase.@@ -29,17 +31,41 @@
> SecPlatformDisableTemporaryMemory (
>  {   EFI_STATUS                Status;   VOID                      *TempRamExitParam;+
> CONST EFI_PEI_SERVICES    **PeiServices;+  FSP_TEMP_RAM_EXIT_PPI
> *TempRamExitPpi;++  DEBUG ((DEBUG_INFO,
> "SecPlatformDisableTemporaryMemory enter\n")); -  DEBUG((DEBUG_INFO,
> "SecPlatformDisableTemporaryMemory enter\n"));-     Status =
> BoardInitBeforeTempRamExit ();   ASSERT_EFI_ERROR (Status); -
> TempRamExitParam = UpdateTempRamExitParam ();-  Status =
> CallTempRamExit (TempRamExitParam);-  DEBUG((DEBUG_INFO,
> "TempRamExit status: 0x%x\n", Status));-  ASSERT_EFI_ERROR(Status);-  +  if
> (PcdGet8 (PcdFspModeSelection) == 1) {+    //+    // FSP API mode+    //+
> TempRamExitParam = UpdateTempRamExitParam ();+    Status =
> CallTempRamExit (TempRamExitParam);+    DEBUG ((DEBUG_INFO,
> "TempRamExit status: 0x%x\n", Status));+    ASSERT_EFI_ERROR (Status);+  }
> else {+    //+    // FSP Dispatch mode+    //+    PeiServices =
> GetPeiServicesTablePointer ();+    Status = (*PeiServices)->LocatePpi (+
> PeiServices,+                             &gFspTempRamExitPpiGuid,+                             0,+
> NULL,+                             (VOID **) &TempRamExitPpi+                             );+
> ASSERT_EFI_ERROR (Status);+    if (EFI_ERROR (Status)) {+      return;+    }+
> TempRamExitPpi->TempRamExit (NULL);+  }+   Status =
> BoardInitAfterTempRamExit ();   ASSERT_EFI_ERROR (Status); diff --git
> a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
> b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
> index a851021c0b..856c17f737 100644
> --- a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
> +++ b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
> @@ -69,8 +69,6 @@ SetCacheMtrrLib|Include/Library/SetCacheMtrrLib.h
>   [PcdsFixedAtBuild, PcdsPatchableInModule] -
> gMinPlatformPkgTokenSpaceGuid.PcdFspWrapperBootMode|FALSE|BOOLE
> AN|0x80000008-
> gMinPlatformPkgTokenSpaceGuid.PcdFspMaxUpdSize|0x00000000|UINT32|
> 0x80000000
> gMinPlatformPkgTokenSpaceGuid.PcdFspReservedSizeOnStackTop|0x00000
> 040|UINT32|0x80000001
> gMinPlatformPkgTokenSpaceGuid.PcdPeiPhaseStackTop|0x00000000|UINT3
> 2|0x80000002@@ -272,6 +270,32 @@
> gMinPlatformPkgTokenSpaceGuid.PcdPcIoApicEnable|0x0|UINT32|0x90000
> 019
>    #   gMinPlatformPkgTokenSpaceGuid.PcdBootStage|4|UINT8|0xF00000A0
> +  ## FSP Boot Mode Selector+  # FALSE: The board is not a FSP wrapper (FSP
> binary not used)+  # TRUE:  The board is a FSP wrapper (FSP binary is used)+
> #+
> gMinPlatformPkgTokenSpaceGuid.PcdFspWrapperBootMode|FALSE|BOOLE
> AN|0x80000008++  ## FSP Dispatch Mode: Use the PEI Main Binary Included
> in FSP-M+  # FALSE: The PEI Main included in FvPreMemory is used to
> dispatch all PEIMs+  #        (both inside FSP and outside FSP).+  #        Pros:+  #
> * PEI Main is re-built from source and is always the latest version+  #          *
> Platform code can link any desired LibraryClass to PEI Main+  #            (Ex:
> Custom DebugLib instance, SerialPortLib, etc.)+  #        Cons:+  #          * The PEI
> Main being used to execute FSP PEIMs is not the PEI Main+  #            that the
> FSP PEIMs were tested with, adding risk of breakage.+  #          * Two copies of
> PEI Main will exist in the final binary,+  #            #1 in FSP-M, #2 in
> FvPreMemory. The copy in FSP-M is never+  #            executed, wasting
> space.+  #+  # <b>TRUE</b>:  The PEI Main included in FSP is used to dispatch
> all PEIMs+  #        (both inside FSP and outside FSP). PEI Main will not be
> included in+  #        FvPreMemory. This is the default and is the recommended
> choice.+  #+
> gMinPlatformPkgTokenSpaceGuid.PcdFspDispatchModeUseFspPeiMain|TR
> UE|BOOLEAN|0xF00000A8+ [PcdsFeatureFlag]
> gMinPlatformPkgTokenSpaceGuid.PcdStopAfterDebugInit
> |FALSE|BOOLEAN|0xF00000A1--
> 2.23.0.windows.1