public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* Re: [edk2-devel] [edk2-platforms][PATCH v2 1/1] IntelSiliconPkg/SpiFvbService: Add support for VariableFlashInfoLib
       [not found] <16FDBB890A0E8B8C.7323@groups.io>
@ 2022-07-12 23:04 ` Michael Kubacki
  2022-07-13 15:51   ` Oram, Isaac W
  2022-07-13 16:49   ` Chaganty, Rangasai V
  0 siblings, 2 replies; 4+ messages in thread
From: Michael Kubacki @ 2022-07-12 23:04 UTC (permalink / raw)
  To: devel; +Cc: Rangasai V Chaganty, Ray Ni, Isaac Oram, Nate DeSimone

Reminder about this patch.

Thanks,
Michael

On 7/1/2022 10:39 AM, Michael Kubacki wrote:
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3478
> 
> Adds support for getting the variable flash information from
> VariableFlashInfoLib. This library abstracts the source of flash
> information so platforms could expect the information to come
> from a different source in the library implementation than the
> PCDs previously used as the information source in this module.
> 
> In particular, the library allows Standalone MM platforms to
> dynamically pass the information behind the library API.
> 
> Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Isaac Oram <isaac.w.oram@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
> 
> Notes:
>      V2 changes:
>      
>      1. Updated the buffer that is returned from
>         GetFvbInfo() to be allocated from pool.
>      2. Updated calls to that function in FvbInitialize()
>         to free the new buffer after usage.
>      3. Updated the multiplication operation to determine
>         the FV length to use safe multiplication in
>         GenerateNvStorageFvbMediaInfo().
> 
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c                     | 128 +++++++++++++-------
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c         |  93 ++++++++++++--
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c             |  49 ++++++--
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h         |  18 ++-
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.inf          |   6 +-
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceStandaloneMm.inf |   6 +-
>   Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc                                       |   7 ++
>   7 files changed, 243 insertions(+), 64 deletions(-)
> 
> diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c
> index 7f2678fa9e5a..634a44218c7a 100644
> --- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c
> @@ -3,6 +3,7 @@
>     These data is intent to decouple FVB driver with FV header.
>   
>   Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) Microsoft Corporation.<BR>
>   SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>   **/
> @@ -11,51 +12,92 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>   #define FIRMWARE_BLOCK_SIZE         0x10000
>   #define FVB_MEDIA_BLOCK_SIZE        FIRMWARE_BLOCK_SIZE
> -
> -#define NV_STORAGE_BASE_ADDRESS     FixedPcdGet32(PcdFlashNvStorageVariableBase)
> -#define SYSTEM_NV_BLOCK_NUM         ((FixedPcdGet32(PcdFlashNvStorageVariableSize)+ FixedPcdGet32(PcdFlashNvStorageFtwWorkingSize) + FixedPcdGet32(PcdFlashNvStorageFtwSpareSize))/ FVB_MEDIA_BLOCK_SIZE)
> -
>   typedef struct {
>     EFI_PHYSICAL_ADDRESS        BaseAddress;
>     EFI_FIRMWARE_VOLUME_HEADER  FvbInfo;
>     EFI_FV_BLOCK_MAP_ENTRY      End[1];
>   } EFI_FVB2_MEDIA_INFO;
>   
> -//
> -// This data structure contains a template of all correct FV headers, which is used to restore
> -// Fv header if it's corrupted.
> -//
> -EFI_FVB2_MEDIA_INFO mPlatformFvbMediaInfo[] = {
> -  //
> -  // Systen NvStorage FVB
> -  //
> -  {
> -    NV_STORAGE_BASE_ADDRESS,
> -    {
> -      {0,}, //ZeroVector[16]
> -      EFI_SYSTEM_NV_DATA_FV_GUID,
> -      FVB_MEDIA_BLOCK_SIZE * SYSTEM_NV_BLOCK_NUM,
> -      EFI_FVH_SIGNATURE,
> -      0x0004feff, // check MdePkg/Include/Pi/PiFirmwareVolume.h for details on EFI_FVB_ATTRIBUTES_2
> -      sizeof (EFI_FIRMWARE_VOLUME_HEADER) + sizeof (EFI_FV_BLOCK_MAP_ENTRY),
> -      0,    //CheckSum which will be calucated dynamically.
> -      0,    //ExtHeaderOffset
> -      {0,}, //Reserved[1]
> -      2,    //Revision
> -      {
> -        {
> -          SYSTEM_NV_BLOCK_NUM,
> -          FVB_MEDIA_BLOCK_SIZE,
> -        }
> -      }
> -    },
> -    {
> -      {
> -        0,
> -        0
> -      }
> -    }
> +/**
> +  Returns FVB media information for NV variable storage.
> +
> +  @return       FvbMediaInfo          A pointer to an instance of FVB media info produced by this function.
> +                                      The buffer is allocated internally to this function and it is the caller's
> +                                      responsibility to free the memory
> +
> +**/
> +typedef
> +EFI_STATUS
> +(*FVB_MEDIA_INFO_GENERATOR)(
> +  OUT EFI_FVB2_MEDIA_INFO     *FvbMediaInfo
> +  );
> +
> +/**
> +  Returns FVB media information for NV variable storage.
> +
> +  @param[out]   FvbMediaInfo          A pointer to an instance of FVB media info produced by this function.
> +
> +  @retval       EFI_SUCCESS           A structure was successfully written to the FvbMediaInfo buffer.
> +  @retval       EFI_INVALID_PARAMETER The FvbMediaInfo parameter is NULL.
> +  @retval       EFI_UNSUPPORTED       An error occurred retrieving variable FV information.
> +  @retval       EFI_BAD_BUFFER_SIZE   An overflow or underflow of the FV buffer occurred with the information found.
> +
> +**/
> +EFI_STATUS
> +GenerateNvStorageFvbMediaInfo (
> +  OUT EFI_FVB2_MEDIA_INFO     *FvbMediaInfo
> +  )
> +{
> +  EFI_STATUS                  Status;
> +  UINT32                      NvBlockNum;
> +  UINT32                      TotalNvVariableStorageSize;
> +  EFI_PHYSICAL_ADDRESS        NvStorageBaseAddress;
> +  EFI_FIRMWARE_VOLUME_HEADER  FvbInfo = {
> +                                          {0,},                                   //ZeroVector[16]
> +                                          EFI_SYSTEM_NV_DATA_FV_GUID,             //FileSystemGuid
> +                                          0,                                      //FvLength
> +                                          EFI_FVH_SIGNATURE,                      //Signature
> +                                          0x0004feff,                             //Attributes
> +                                          sizeof (EFI_FIRMWARE_VOLUME_HEADER) +   //HeaderLength
> +                                            sizeof (EFI_FV_BLOCK_MAP_ENTRY),
> +                                          0,                                      //Checksum
> +                                          0,                                      //ExtHeaderOffset
> +                                          {0,},                                   //Reserved[1]
> +                                          2,                                      //Revision
> +                                          {                                       //BlockMap[1]
> +                                            {0,0}
> +                                          }
> +                                        };
> +
> +  if (FvbMediaInfo == NULL) {
> +    return EFI_INVALID_PARAMETER;
>     }
> +
> +  ZeroMem (FvbMediaInfo, sizeof (*FvbMediaInfo));
> +
> +  GetVariableFvInfo (&NvStorageBaseAddress, &TotalNvVariableStorageSize);
> +  if ((NvStorageBaseAddress == 0) || (TotalNvVariableStorageSize == 0)) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  NvBlockNum = TotalNvVariableStorageSize / FVB_MEDIA_BLOCK_SIZE;
> +
> +  Status = SafeUint64Mult ((UINT64)NvBlockNum, FVB_MEDIA_BLOCK_SIZE, &FvbInfo.FvLength);
> +  if (EFI_ERROR (Status)) {
> +    return EFI_BAD_BUFFER_SIZE;
> +  }
> +
> +  FvbInfo.BlockMap[0].NumBlocks = NvBlockNum;
> +  FvbInfo.BlockMap[0].Length = FVB_MEDIA_BLOCK_SIZE;
> +
> +  FvbMediaInfo->BaseAddress = NvStorageBaseAddress;
> +  CopyMem (&FvbMediaInfo->FvbInfo, &FvbInfo, sizeof (FvbInfo));
> +
> +  return EFI_SUCCESS;
> +}
> +
> +FVB_MEDIA_INFO_GENERATOR mFvbMediaInfoGenerators[] = {
> +  GenerateNvStorageFvbMediaInfo
>   };
>   
>   EFI_STATUS
> @@ -64,12 +106,16 @@ GetFvbInfo (
>     OUT EFI_FIRMWARE_VOLUME_HEADER   **FvbInfo
>     )
>   {
> +  EFI_STATUS                  Status;
> +  EFI_FVB2_MEDIA_INFO         FvbMediaInfo;
>     UINTN                       Index;
>     EFI_FIRMWARE_VOLUME_HEADER  *FvHeader;
>   
> -  for (Index = 0; Index < sizeof (mPlatformFvbMediaInfo) / sizeof (EFI_FVB2_MEDIA_INFO); Index++) {
> -    if (mPlatformFvbMediaInfo[Index].BaseAddress == FvBaseAddress) {
> -      FvHeader = &mPlatformFvbMediaInfo[Index].FvbInfo;
> +  for (Index = 0; Index < ARRAY_SIZE (mFvbMediaInfoGenerators); Index++) {
> +    Status = mFvbMediaInfoGenerators[Index](&FvbMediaInfo);
> +    ASSERT_EFI_ERROR (Status);
> +    if (!EFI_ERROR (Status) && (FvbMediaInfo.BaseAddress == FvBaseAddress)) {
> +      FvHeader = AllocateCopyPool (sizeof (EFI_FIRMWARE_VOLUME_HEADER), &FvbMediaInfo.FvbInfo);
>   
>         //
>         // Update the checksum value of FV header.
> diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c
> index dab818e98087..942abf95a64f 100644
> --- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c
> @@ -3,6 +3,7 @@
>     which are compliant with the Intel(R) Serial Flash Interface Compatibility Specification.
>   
>   Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) Microsoft Corporation.<BR>
>   SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>   **/
> @@ -25,12 +26,6 @@ FV_INFO mPlatformFvBaseAddress[] = {
>     {0, 0}
>   };
>   
> -FV_INFO mPlatformDefaultBaseAddress[] = {
> -  {0, 0}, // {FixedPcdGet32(PcdFlashNvStorageVariableBase), FixedPcdGet32(PcdFlashNvStorageVariableSize)},
> -  {0, 0}, // {FixedPcdGet32(PcdFlashMicrocodeFvBase), FixedPcdGet32(PcdFlashMicrocodeFvSize)},
> -  {0, 0}
> -};
> -
>   FV_MEMMAP_DEVICE_PATH mFvMemmapDevicePathTemplate = {
>     {
>       {
> @@ -530,6 +525,85 @@ FvbSetVolumeAttributes (
>     return EFI_SUCCESS;
>   }
>   
> +/**
> +  Get the total size of the firmware volume on flash used for variable store operations.
> +
> +  @param[out] BaseAddress         Base address of the variable store firmware volume.
> +  @param[out] Length              Length in bytes of the firmware volume used for variable store operations.
> +
> +**/
> +VOID
> +GetVariableFvInfo (
> +  OUT EFI_PHYSICAL_ADDRESS      *BaseAddress,
> +  OUT UINT32                    *Length
> +  )
> +{
> +  EFI_STATUS                    Status;
> +  EFI_PHYSICAL_ADDRESS          NvBaseAddress;
> +  EFI_PHYSICAL_ADDRESS          NvVariableBaseAddress;
> +  UINT64                        Length64;
> +  UINT32                        NvStoreLength;
> +  UINT32                        FtwSpareLength;
> +  UINT32                        FtwWorkingLength;
> +  UINT32                        TotalLength;
> +
> +  TotalLength = 0;
> +  Status = EFI_SUCCESS;
> +
> +  if ((BaseAddress == NULL) || (Length == NULL)) {
> +    ASSERT ((BaseAddress != NULL) && (Length != NULL));
> +    return;
> +  }
> +  *BaseAddress = 0;
> +  *Length = 0;
> +
> +  Status = GetVariableFlashNvStorageInfo (&NvBaseAddress, &Length64);
> +  if (!EFI_ERROR (Status)) {
> +    NvVariableBaseAddress = NvBaseAddress;
> +    // Stay within the current UINT32 size assumptions in the variable stack.
> +    Status = SafeUint64ToUint32 (Length64, &NvStoreLength);
> +  }
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return;
> +  }
> +
> +  Status = GetVariableFlashFtwSpareInfo (&NvBaseAddress, &Length64);
> +  if (!EFI_ERROR (Status)) {
> +    // Stay within the current UINT32 size assumptions in the variable stack.
> +    Status = SafeUint64ToUint32 (Length64, &FtwSpareLength);
> +  }
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return;
> +  }
> +
> +  Status = GetVariableFlashFtwWorkingInfo (&NvBaseAddress, &Length64);
> +  if (!EFI_ERROR (Status)) {
> +    // Stay within the current UINT32 size assumptions in the variable stack.
> +    Status = SafeUint64ToUint32 (Length64, &FtwWorkingLength);
> +  }
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return;
> +  }
> +
> +  Status = SafeUint32Add (NvStoreLength, FtwSpareLength, &TotalLength);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return;
> +  }
> +
> +  Status = SafeUint32Add (TotalLength, FtwWorkingLength, &TotalLength);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return;
> +  }
> +
> +  *BaseAddress = NvVariableBaseAddress;
> +  *Length = TotalLength;
> +}
> +
>   /**
>     Check the integrity of firmware volume header
>   
> @@ -545,7 +619,12 @@ IsFvHeaderValid (
>     IN CONST EFI_FIRMWARE_VOLUME_HEADER    *FvHeader
>     )
>   {
> -  if (FvBase == PcdGet32(PcdFlashNvStorageVariableBase)) {
> +  EFI_PHYSICAL_ADDRESS    NvStorageFvBaseAddress;
> +  UINT32                  NvStorageSize;
> +
> +  GetVariableFvInfo (&NvStorageFvBaseAddress, &NvStorageSize);
> +
> +  if (FvBase == NvStorageFvBaseAddress) {
>       if (CompareMem (&FvHeader->FileSystemGuid, &gEfiSystemNvDataFvGuid, sizeof(EFI_GUID)) != 0 ) {
>         return FALSE;
>       }
> diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c
> index 42a0828c6fae..6b4bcdcfe3c4 100644
> --- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c
> @@ -113,15 +113,31 @@ FvbInitialize (
>     UINT32                                MaxLbaSize;
>     UINT32                                BytesWritten;
>     UINTN                                 BytesErased;
> +  UINT64                                NvStorageFvSize;
> +
> +  Status = GetVariableFlashNvStorageInfo (&BaseAddress, &NvStorageFvSize);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    DEBUG ((DEBUG_ERROR, "[%a] - An error ocurred getting variable info - %r.\n", __FUNCTION__, Status));
> +    return;
> +  }
> +
> +  // Stay within the current UINT32 size assumptions in the variable stack.
> +  Status = SafeUint64ToUint32 (BaseAddress, &mPlatformFvBaseAddress[0].FvBase);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    DEBUG ((DEBUG_ERROR, "[%a] - 64-bit variable storage base address not supported.\n", __FUNCTION__));
> +    return;
> +  }
> +  Status = SafeUint64ToUint32 (NvStorageFvSize, &mPlatformFvBaseAddress[0].FvSize);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    DEBUG ((DEBUG_ERROR, "[%a] - 64-bit variable storage size not supported.\n", __FUNCTION__));
> +    return;
> +  }
>   
> -  mPlatformFvBaseAddress[0].FvBase = PcdGet32(PcdFlashNvStorageVariableBase);
> -  mPlatformFvBaseAddress[0].FvSize = PcdGet32(PcdFlashNvStorageVariableSize);
>     mPlatformFvBaseAddress[1].FvBase = PcdGet32(PcdFlashMicrocodeFvBase);
>     mPlatformFvBaseAddress[1].FvSize = PcdGet32(PcdFlashMicrocodeFvSize);
> -  mPlatformDefaultBaseAddress[0].FvBase = PcdGet32(PcdFlashNvStorageVariableBase);
> -  mPlatformDefaultBaseAddress[0].FvSize = PcdGet32(PcdFlashNvStorageVariableSize);
> -  mPlatformDefaultBaseAddress[1].FvBase = PcdGet32(PcdFlashMicrocodeFvBase);
> -  mPlatformDefaultBaseAddress[1].FvSize = PcdGet32(PcdFlashMicrocodeFvSize);
>   
>     //
>     // We will only continue with FVB installation if the
> @@ -143,7 +159,8 @@ FvbInitialize (
>           BytesWritten = 0;
>           BytesErased = 0;
>           DEBUG ((DEBUG_ERROR, "ERROR - The FV in 0x%x is invalid!\n", FvHeader));
> -        Status = GetFvbInfo (BaseAddress, &FvHeader);
> +        FvHeader = NULL;
> +        Status   = GetFvbInfo (BaseAddress, &FvHeader);
>           if (EFI_ERROR (Status)) {
>             DEBUG ((DEBUG_WARN, "ERROR - Can't recovery FV header at 0x%x.  GetFvbInfo Status %r\n", BaseAddress, Status));
>             continue;
> @@ -156,27 +173,42 @@ FvbInitialize (
>           Status = SpiFlashBlockErase( (UINTN) BaseAddress, &BytesErased);
>           if (EFI_ERROR (Status)) {
>             DEBUG ((DEBUG_WARN, "ERROR - SpiFlashBlockErase Error  %r\n", Status));
> +          if (FvHeader != NULL) {
> +            FreePool (FvHeader);
> +          }
>             continue;
>           }
>           if (BytesErased != FvHeader->BlockMap->Length) {
>             DEBUG ((DEBUG_WARN, "ERROR - BytesErased != FvHeader->BlockMap->Length\n"));
>             DEBUG ((DEBUG_INFO, " BytesErased = 0x%X\n Length = 0x%X\n", BytesErased, FvHeader->BlockMap->Length));
> +          if (FvHeader != NULL) {
> +            FreePool (FvHeader);
> +          }
>             continue;
>           }
>           BytesWritten = FvHeader->HeaderLength;
>           Status = SpiFlashWrite ((UINTN)BaseAddress, &BytesWritten, (UINT8*)FvHeader);
>           if (EFI_ERROR (Status)) {
>             DEBUG ((DEBUG_WARN, "ERROR - SpiFlashWrite Error  %r\n", Status));
> +          if (FvHeader != NULL) {
> +            FreePool (FvHeader);
> +          }
>             continue;
>           }
>           if (BytesWritten != FvHeader->HeaderLength) {
>             DEBUG ((DEBUG_WARN, "ERROR - BytesWritten != HeaderLength\n"));
>             DEBUG ((DEBUG_INFO, " BytesWritten = 0x%X\n HeaderLength = 0x%X\n", BytesWritten, FvHeader->HeaderLength));
> +          if (FvHeader != NULL) {
> +            FreePool (FvHeader);
> +          }
>             continue;
>           }
>           Status = SpiFlashLock ();
>           if (EFI_ERROR (Status)) {
>             DEBUG ((DEBUG_WARN, "ERROR - SpiFlashLock Error  %r\n", Status));
> +          if (FvHeader != NULL) {
> +            FreePool (FvHeader);
> +          }
>             continue;
>           }
>           DEBUG ((DEBUG_INFO, "FV Header @ 0x%X restored with static data\n", BaseAddress));
> @@ -184,6 +216,9 @@ FvbInitialize (
>           // Clear cache for this range.
>           //
>           WriteBackInvalidateDataCacheRange ( (VOID *) (UINTN) BaseAddress, FvHeader->BlockMap->Length);
> +        if (FvHeader != NULL) {
> +          FreePool (FvHeader);
> +        }
>         }
>       }
>   
> diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h
> index e9d69e985814..7b0908b03fdc 100644
> --- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h
> @@ -2,6 +2,7 @@
>     Common source definitions used in serial flash drivers
>   
>   Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) Microsoft Corporation.<BR>
>   SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>   **/
> @@ -12,6 +13,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>   #include <Guid/EventGroup.h>
>   #include <Guid/FirmwareFileSystem2.h>
>   #include <Guid/SystemNvDataGuid.h>
> +#include <Pi/PiFirmwareVolume.h>
>   #include <Protocol/DevicePath.h>
>   #include <Protocol/FirmwareVolumeBlock.h>
>   
> @@ -24,6 +26,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>   #include <Library/PcdLib.h>
>   #include <Library/DevicePathLib.h>
>   #include <Library/HobLib.h>
> +#include <Library/VariableFlashInfoLib.h>
> +#include <Library/SafeIntLib.h>
>   
>   #include <Library/SpiFlashCommonLib.h>
>   
> @@ -148,11 +152,23 @@ GetFvbInfo (
>     OUT EFI_FIRMWARE_VOLUME_HEADER   **FvbInfo
>     );
>   
> +/**
> +  Get the total size of the firmware volume on flash used for variable store operations.
> +
> +  @param[out] BaseAddress         Base address of the variable store firmware volume.
> +  @param[out] Length              Length in bytes of the firmware volume used for variable store operations.
> +
> +**/
> +VOID
> +GetVariableFvInfo (
> +  OUT EFI_PHYSICAL_ADDRESS      *BaseAddress,
> +  OUT UINT32                    *Length
> +  );
> +
>   extern FVB_GLOBAL                         mFvbModuleGlobal;
>   extern FV_MEMMAP_DEVICE_PATH              mFvMemmapDevicePathTemplate;
>   extern FV_PIWG_DEVICE_PATH                mFvPIWGDevicePathTemplate;
>   extern EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL mFvbProtocolTemplate;
>   extern FV_INFO                            mPlatformFvBaseAddress[];
> -extern FV_INFO                            mPlatformDefaultBaseAddress[];
>   
>   #endif
> diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.inf b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.inf
> index bf1306f00201..0cfa3f909bdf 100644
> --- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.inf
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.inf
> @@ -32,8 +32,10 @@ [LibraryClasses]
>     BaseLib
>     UefiBootServicesTableLib
>     UefiDriverEntryPoint
> +  SafeIntLib
>     SpiFlashCommonLib
>     MmServicesTableLib
> +  VariableFlashInfoLib
>   
>   [Packages]
>     MdePkg/MdePkg.dec
> @@ -41,10 +43,6 @@ [Packages]
>     IntelSiliconPkg/IntelSiliconPkg.dec
>   
>   [Pcd]
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase   ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize   ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize   ## CONSUMES
>     gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvBase         ## CONSUMES
>     gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvSize         ## CONSUMES
>   
> diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceStandaloneMm.inf b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceStandaloneMm.inf
> index b66233968247..152cf0036fdb 100644
> --- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceStandaloneMm.inf
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceStandaloneMm.inf
> @@ -31,8 +31,10 @@ [LibraryClasses]
>     MemoryAllocationLib
>     PcdLib
>     MmServicesTableLib
> +  SafeIntLib
>     SpiFlashCommonLib
>     StandaloneMmDriverEntryPoint
> +  VariableFlashInfoLib
>   
>   [Packages]
>     MdePkg/MdePkg.dec
> @@ -40,10 +42,6 @@ [Packages]
>     IntelSiliconPkg/IntelSiliconPkg.dec
>   
>   [Pcd]
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase   ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize   ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize   ## CONSUMES
>     gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvBase         ## CONSUMES
>     gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvSize         ## CONSUMES
>   
> diff --git a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
> index 1e826a080f28..170eb480ada1 100644
> --- a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
> +++ b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
> @@ -40,9 +40,11 @@ [LibraryClasses]
>     PeiGetVtdPmrAlignmentLib|IntelSiliconPkg/Library/PeiGetVtdPmrAlignmentLib/PeiGetVtdPmrAlignmentLib.inf
>     TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
>     MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
> +  SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
>     SpiFlashCommonLib|IntelSiliconPkg/Library/SpiFlashCommonLibNull/SpiFlashCommonLibNull.inf
>     UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf
>     UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryPoint.inf
> +  VariableFlashInfoLib|MdeModulePkg/Library/BaseVariableFlashInfoLib/BaseVariableFlashInfoLib.inf
>   
>   [LibraryClasses.common.PEIM]
>     PeimEntryPoint|MdePkg/Library/PeimEntryPoint/PeimEntryPoint.inf
> @@ -63,11 +65,16 @@ [LibraryClasses.common.DXE_DRIVER]
>     MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
>   
>   [LibraryClasses.common.DXE_SMM_DRIVER]
> +  DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
> +  HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
>     MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLib/SmmMemoryAllocationLib.inf
>     MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf
>     SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesTableLib.inf
> +  UefiLib|MdePkg/Library/UefiLib/UefiLib.inf
> +  UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
>   
>   [LibraryClasses.common.MM_STANDALONE]
> +  HobLib|StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib.inf
>     MemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMemoryAllocationLib.inf
>     MmServicesTableLib|MdePkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.inf
>     StandaloneMmDriverEntryPoint|MdePkg/Library/StandaloneMmDriverEntryPoint/StandaloneMmDriverEntryPoint.inf

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [edk2-devel] [edk2-platforms][PATCH v2 1/1] IntelSiliconPkg/SpiFvbService: Add support for VariableFlashInfoLib
  2022-07-12 23:04 ` [edk2-devel] [edk2-platforms][PATCH v2 1/1] IntelSiliconPkg/SpiFvbService: Add support for VariableFlashInfoLib Michael Kubacki
@ 2022-07-13 15:51   ` Oram, Isaac W
  2022-07-13 16:49   ` Chaganty, Rangasai V
  1 sibling, 0 replies; 4+ messages in thread
From: Oram, Isaac W @ 2022-07-13 15:51 UTC (permalink / raw)
  To: devel@edk2.groups.io, mikuback@linux.microsoft.com
  Cc: Chaganty, Rangasai V, Ni, Ray, Desimone, Nathaniel L

Reviewed-by: Isaac Oram <isaac.w.oram@intel.com>

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
Sent: Tuesday, July 12, 2022 4:04 PM
To: devel@edk2.groups.io
Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Ni, Ray <ray.ni@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Subject: Re: [edk2-devel] [edk2-platforms][PATCH v2 1/1] IntelSiliconPkg/SpiFvbService: Add support for VariableFlashInfoLib

Reminder about this patch.

Thanks,
Michael

On 7/1/2022 10:39 AM, Michael Kubacki wrote:
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3478
> 
> Adds support for getting the variable flash information from 
> VariableFlashInfoLib. This library abstracts the source of flash 
> information so platforms could expect the information to come from a 
> different source in the library implementation than the PCDs 
> previously used as the information source in this module.
> 
> In particular, the library allows Standalone MM platforms to 
> dynamically pass the information behind the library API.
> 
> Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Isaac Oram <isaac.w.oram@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
> 
> Notes:
>      V2 changes:
>      
>      1. Updated the buffer that is returned from
>         GetFvbInfo() to be allocated from pool.
>      2. Updated calls to that function in FvbInitialize()
>         to free the new buffer after usage.
>      3. Updated the multiplication operation to determine
>         the FV length to use safe multiplication in
>         GenerateNvStorageFvbMediaInfo().
> 
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c                     | 128 +++++++++++++-------
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c         |  93 ++++++++++++--
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c             |  49 ++++++--
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h         |  18 ++-
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.inf          |   6 +-
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceStandaloneMm.inf |   6 +-
>   Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc                                       |   7 ++
>   7 files changed, 243 insertions(+), 64 deletions(-)
> 
> diff --git 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c 
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c
> index 7f2678fa9e5a..634a44218c7a 100644
> --- 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInf
> +++ o.c
> @@ -3,6 +3,7 @@
>     These data is intent to decouple FVB driver with FV header.
>   
>   Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) Microsoft Corporation.<BR>
>   SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>   **/
> @@ -11,51 +12,92 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>   #define FIRMWARE_BLOCK_SIZE         0x10000
>   #define FVB_MEDIA_BLOCK_SIZE        FIRMWARE_BLOCK_SIZE
> -
> -#define NV_STORAGE_BASE_ADDRESS     FixedPcdGet32(PcdFlashNvStorageVariableBase)
> -#define SYSTEM_NV_BLOCK_NUM         ((FixedPcdGet32(PcdFlashNvStorageVariableSize)+ FixedPcdGet32(PcdFlashNvStorageFtwWorkingSize) + FixedPcdGet32(PcdFlashNvStorageFtwSpareSize))/ FVB_MEDIA_BLOCK_SIZE)
> -
>   typedef struct {
>     EFI_PHYSICAL_ADDRESS        BaseAddress;
>     EFI_FIRMWARE_VOLUME_HEADER  FvbInfo;
>     EFI_FV_BLOCK_MAP_ENTRY      End[1];
>   } EFI_FVB2_MEDIA_INFO;
>   
> -//
> -// This data structure contains a template of all correct FV headers, 
> which is used to restore -// Fv header if it's corrupted.
> -//
> -EFI_FVB2_MEDIA_INFO mPlatformFvbMediaInfo[] = {
> -  //
> -  // Systen NvStorage FVB
> -  //
> -  {
> -    NV_STORAGE_BASE_ADDRESS,
> -    {
> -      {0,}, //ZeroVector[16]
> -      EFI_SYSTEM_NV_DATA_FV_GUID,
> -      FVB_MEDIA_BLOCK_SIZE * SYSTEM_NV_BLOCK_NUM,
> -      EFI_FVH_SIGNATURE,
> -      0x0004feff, // check MdePkg/Include/Pi/PiFirmwareVolume.h for details on EFI_FVB_ATTRIBUTES_2
> -      sizeof (EFI_FIRMWARE_VOLUME_HEADER) + sizeof (EFI_FV_BLOCK_MAP_ENTRY),
> -      0,    //CheckSum which will be calucated dynamically.
> -      0,    //ExtHeaderOffset
> -      {0,}, //Reserved[1]
> -      2,    //Revision
> -      {
> -        {
> -          SYSTEM_NV_BLOCK_NUM,
> -          FVB_MEDIA_BLOCK_SIZE,
> -        }
> -      }
> -    },
> -    {
> -      {
> -        0,
> -        0
> -      }
> -    }
> +/**
> +  Returns FVB media information for NV variable storage.
> +
> +  @return       FvbMediaInfo          A pointer to an instance of FVB media info produced by this function.
> +                                      The buffer is allocated internally to this function and it is the caller's
> +                                      responsibility to free the 
> + memory
> +
> +**/
> +typedef
> +EFI_STATUS
> +(*FVB_MEDIA_INFO_GENERATOR)(
> +  OUT EFI_FVB2_MEDIA_INFO     *FvbMediaInfo
> +  );
> +
> +/**
> +  Returns FVB media information for NV variable storage.
> +
> +  @param[out]   FvbMediaInfo          A pointer to an instance of FVB media info produced by this function.
> +
> +  @retval       EFI_SUCCESS           A structure was successfully written to the FvbMediaInfo buffer.
> +  @retval       EFI_INVALID_PARAMETER The FvbMediaInfo parameter is NULL.
> +  @retval       EFI_UNSUPPORTED       An error occurred retrieving variable FV information.
> +  @retval       EFI_BAD_BUFFER_SIZE   An overflow or underflow of the FV buffer occurred with the information found.
> +
> +**/
> +EFI_STATUS
> +GenerateNvStorageFvbMediaInfo (
> +  OUT EFI_FVB2_MEDIA_INFO     *FvbMediaInfo
> +  )
> +{
> +  EFI_STATUS                  Status;
> +  UINT32                      NvBlockNum;
> +  UINT32                      TotalNvVariableStorageSize;
> +  EFI_PHYSICAL_ADDRESS        NvStorageBaseAddress;
> +  EFI_FIRMWARE_VOLUME_HEADER  FvbInfo = {
> +                                          {0,},                                   //ZeroVector[16]
> +                                          EFI_SYSTEM_NV_DATA_FV_GUID,             //FileSystemGuid
> +                                          0,                                      //FvLength
> +                                          EFI_FVH_SIGNATURE,                      //Signature
> +                                          0x0004feff,                             //Attributes
> +                                          sizeof (EFI_FIRMWARE_VOLUME_HEADER) +   //HeaderLength
> +                                            sizeof (EFI_FV_BLOCK_MAP_ENTRY),
> +                                          0,                                      //Checksum
> +                                          0,                                      //ExtHeaderOffset
> +                                          {0,},                                   //Reserved[1]
> +                                          2,                                      //Revision
> +                                          {                                       //BlockMap[1]
> +                                            {0,0}
> +                                          }
> +                                        };
> +
> +  if (FvbMediaInfo == NULL) {
> +    return EFI_INVALID_PARAMETER;
>     }
> +
> +  ZeroMem (FvbMediaInfo, sizeof (*FvbMediaInfo));
> +
> +  GetVariableFvInfo (&NvStorageBaseAddress, 
> + &TotalNvVariableStorageSize);  if ((NvStorageBaseAddress == 0) || (TotalNvVariableStorageSize == 0)) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  NvBlockNum = TotalNvVariableStorageSize / FVB_MEDIA_BLOCK_SIZE;
> +
> +  Status = SafeUint64Mult ((UINT64)NvBlockNum, FVB_MEDIA_BLOCK_SIZE, 
> + &FvbInfo.FvLength);  if (EFI_ERROR (Status)) {
> +    return EFI_BAD_BUFFER_SIZE;
> +  }
> +
> +  FvbInfo.BlockMap[0].NumBlocks = NvBlockNum;  
> + FvbInfo.BlockMap[0].Length = FVB_MEDIA_BLOCK_SIZE;
> +
> +  FvbMediaInfo->BaseAddress = NvStorageBaseAddress;  CopyMem 
> + (&FvbMediaInfo->FvbInfo, &FvbInfo, sizeof (FvbInfo));
> +
> +  return EFI_SUCCESS;
> +}
> +
> +FVB_MEDIA_INFO_GENERATOR mFvbMediaInfoGenerators[] = {
> +  GenerateNvStorageFvbMediaInfo
>   };
>   
>   EFI_STATUS
> @@ -64,12 +106,16 @@ GetFvbInfo (
>     OUT EFI_FIRMWARE_VOLUME_HEADER   **FvbInfo
>     )
>   {
> +  EFI_STATUS                  Status;
> +  EFI_FVB2_MEDIA_INFO         FvbMediaInfo;
>     UINTN                       Index;
>     EFI_FIRMWARE_VOLUME_HEADER  *FvHeader;
>   
> -  for (Index = 0; Index < sizeof (mPlatformFvbMediaInfo) / sizeof (EFI_FVB2_MEDIA_INFO); Index++) {
> -    if (mPlatformFvbMediaInfo[Index].BaseAddress == FvBaseAddress) {
> -      FvHeader = &mPlatformFvbMediaInfo[Index].FvbInfo;
> +  for (Index = 0; Index < ARRAY_SIZE (mFvbMediaInfoGenerators); Index++) {
> +    Status = mFvbMediaInfoGenerators[Index](&FvbMediaInfo);
> +    ASSERT_EFI_ERROR (Status);
> +    if (!EFI_ERROR (Status) && (FvbMediaInfo.BaseAddress == FvBaseAddress)) {
> +      FvHeader = AllocateCopyPool (sizeof 
> + (EFI_FIRMWARE_VOLUME_HEADER), &FvbMediaInfo.FvbInfo);
>   
>         //
>         // Update the checksum value of FV header.
> diff --git 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceCommon.c 
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceCommon.c index dab818e98087..942abf95a64f 100644
> --- 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceCommon.c
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvb
> +++ ServiceCommon.c
> @@ -3,6 +3,7 @@
>     which are compliant with the Intel(R) Serial Flash Interface Compatibility Specification.
>   
>   Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) Microsoft Corporation.<BR>
>   SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>   **/
> @@ -25,12 +26,6 @@ FV_INFO mPlatformFvBaseAddress[] = {
>     {0, 0}
>   };
>   
> -FV_INFO mPlatformDefaultBaseAddress[] = {
> -  {0, 0}, // {FixedPcdGet32(PcdFlashNvStorageVariableBase), 
> FixedPcdGet32(PcdFlashNvStorageVariableSize)},
> -  {0, 0}, // {FixedPcdGet32(PcdFlashMicrocodeFvBase), 
> FixedPcdGet32(PcdFlashMicrocodeFvSize)},
> -  {0, 0}
> -};
> -
>   FV_MEMMAP_DEVICE_PATH mFvMemmapDevicePathTemplate = {
>     {
>       {
> @@ -530,6 +525,85 @@ FvbSetVolumeAttributes (
>     return EFI_SUCCESS;
>   }
>   
> +/**
> +  Get the total size of the firmware volume on flash used for variable store operations.
> +
> +  @param[out] BaseAddress         Base address of the variable store firmware volume.
> +  @param[out] Length              Length in bytes of the firmware volume used for variable store operations.
> +
> +**/
> +VOID
> +GetVariableFvInfo (
> +  OUT EFI_PHYSICAL_ADDRESS      *BaseAddress,
> +  OUT UINT32                    *Length
> +  )
> +{
> +  EFI_STATUS                    Status;
> +  EFI_PHYSICAL_ADDRESS          NvBaseAddress;
> +  EFI_PHYSICAL_ADDRESS          NvVariableBaseAddress;
> +  UINT64                        Length64;
> +  UINT32                        NvStoreLength;
> +  UINT32                        FtwSpareLength;
> +  UINT32                        FtwWorkingLength;
> +  UINT32                        TotalLength;
> +
> +  TotalLength = 0;
> +  Status = EFI_SUCCESS;
> +
> +  if ((BaseAddress == NULL) || (Length == NULL)) {
> +    ASSERT ((BaseAddress != NULL) && (Length != NULL));
> +    return;
> +  }
> +  *BaseAddress = 0;
> +  *Length = 0;
> +
> +  Status = GetVariableFlashNvStorageInfo (&NvBaseAddress, &Length64);  
> + if (!EFI_ERROR (Status)) {
> +    NvVariableBaseAddress = NvBaseAddress;
> +    // Stay within the current UINT32 size assumptions in the variable stack.
> +    Status = SafeUint64ToUint32 (Length64, &NvStoreLength);  }  if 
> + (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return;
> +  }
> +
> +  Status = GetVariableFlashFtwSpareInfo (&NvBaseAddress, &Length64);  
> + if (!EFI_ERROR (Status)) {
> +    // Stay within the current UINT32 size assumptions in the variable stack.
> +    Status = SafeUint64ToUint32 (Length64, &FtwSpareLength);  }  if 
> + (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return;
> +  }
> +
> +  Status = GetVariableFlashFtwWorkingInfo (&NvBaseAddress, 
> + &Length64);  if (!EFI_ERROR (Status)) {
> +    // Stay within the current UINT32 size assumptions in the variable stack.
> +    Status = SafeUint64ToUint32 (Length64, &FtwWorkingLength);  }  if 
> + (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return;
> +  }
> +
> +  Status = SafeUint32Add (NvStoreLength, FtwSpareLength, 
> + &TotalLength);  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return;
> +  }
> +
> +  Status = SafeUint32Add (TotalLength, FtwWorkingLength, 
> + &TotalLength);  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return;
> +  }
> +
> +  *BaseAddress = NvVariableBaseAddress;
> +  *Length = TotalLength;
> +}
> +
>   /**
>     Check the integrity of firmware volume header
>   
> @@ -545,7 +619,12 @@ IsFvHeaderValid (
>     IN CONST EFI_FIRMWARE_VOLUME_HEADER    *FvHeader
>     )
>   {
> -  if (FvBase == PcdGet32(PcdFlashNvStorageVariableBase)) {
> +  EFI_PHYSICAL_ADDRESS    NvStorageFvBaseAddress;
> +  UINT32                  NvStorageSize;
> +
> +  GetVariableFvInfo (&NvStorageFvBaseAddress, &NvStorageSize);
> +
> +  if (FvBase == NvStorageFvBaseAddress) {
>       if (CompareMem (&FvHeader->FileSystemGuid, &gEfiSystemNvDataFvGuid, sizeof(EFI_GUID)) != 0 ) {
>         return FALSE;
>       }
> diff --git 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceMm.c 
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceMm.c index 42a0828c6fae..6b4bcdcfe3c4 100644
> --- 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceMm.c
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvb
> +++ ServiceMm.c
> @@ -113,15 +113,31 @@ FvbInitialize (
>     UINT32                                MaxLbaSize;
>     UINT32                                BytesWritten;
>     UINTN                                 BytesErased;
> +  UINT64                                NvStorageFvSize;
> +
> +  Status = GetVariableFlashNvStorageInfo (&BaseAddress, 
> + &NvStorageFvSize);  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    DEBUG ((DEBUG_ERROR, "[%a] - An error ocurred getting variable info - %r.\n", __FUNCTION__, Status));
> +    return;
> +  }
> +
> +  // Stay within the current UINT32 size assumptions in the variable stack.
> +  Status = SafeUint64ToUint32 (BaseAddress, 
> + &mPlatformFvBaseAddress[0].FvBase);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    DEBUG ((DEBUG_ERROR, "[%a] - 64-bit variable storage base address not supported.\n", __FUNCTION__));
> +    return;
> +  }
> +  Status = SafeUint64ToUint32 (NvStorageFvSize, 
> + &mPlatformFvBaseAddress[0].FvSize);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    DEBUG ((DEBUG_ERROR, "[%a] - 64-bit variable storage size not supported.\n", __FUNCTION__));
> +    return;
> +  }
>   
> -  mPlatformFvBaseAddress[0].FvBase = 
> PcdGet32(PcdFlashNvStorageVariableBase);
> -  mPlatformFvBaseAddress[0].FvSize = PcdGet32(PcdFlashNvStorageVariableSize);
>     mPlatformFvBaseAddress[1].FvBase = PcdGet32(PcdFlashMicrocodeFvBase);
>     mPlatformFvBaseAddress[1].FvSize = 
> PcdGet32(PcdFlashMicrocodeFvSize);
> -  mPlatformDefaultBaseAddress[0].FvBase = 
> PcdGet32(PcdFlashNvStorageVariableBase);
> -  mPlatformDefaultBaseAddress[0].FvSize = 
> PcdGet32(PcdFlashNvStorageVariableSize);
> -  mPlatformDefaultBaseAddress[1].FvBase = 
> PcdGet32(PcdFlashMicrocodeFvBase);
> -  mPlatformDefaultBaseAddress[1].FvSize = 
> PcdGet32(PcdFlashMicrocodeFvSize);
>   
>     //
>     // We will only continue with FVB installation if the @@ -143,7 
> +159,8 @@ FvbInitialize (
>           BytesWritten = 0;
>           BytesErased = 0;
>           DEBUG ((DEBUG_ERROR, "ERROR - The FV in 0x%x is invalid!\n", FvHeader));
> -        Status = GetFvbInfo (BaseAddress, &FvHeader);
> +        FvHeader = NULL;
> +        Status   = GetFvbInfo (BaseAddress, &FvHeader);
>           if (EFI_ERROR (Status)) {
>             DEBUG ((DEBUG_WARN, "ERROR - Can't recovery FV header at 0x%x.  GetFvbInfo Status %r\n", BaseAddress, Status));
>             continue;
> @@ -156,27 +173,42 @@ FvbInitialize (
>           Status = SpiFlashBlockErase( (UINTN) BaseAddress, &BytesErased);
>           if (EFI_ERROR (Status)) {
>             DEBUG ((DEBUG_WARN, "ERROR - SpiFlashBlockErase Error  
> %r\n", Status));
> +          if (FvHeader != NULL) {
> +            FreePool (FvHeader);
> +          }
>             continue;
>           }
>           if (BytesErased != FvHeader->BlockMap->Length) {
>             DEBUG ((DEBUG_WARN, "ERROR - BytesErased != FvHeader->BlockMap->Length\n"));
>             DEBUG ((DEBUG_INFO, " BytesErased = 0x%X\n Length = 
> 0x%X\n", BytesErased, FvHeader->BlockMap->Length));
> +          if (FvHeader != NULL) {
> +            FreePool (FvHeader);
> +          }
>             continue;
>           }
>           BytesWritten = FvHeader->HeaderLength;
>           Status = SpiFlashWrite ((UINTN)BaseAddress, &BytesWritten, (UINT8*)FvHeader);
>           if (EFI_ERROR (Status)) {
>             DEBUG ((DEBUG_WARN, "ERROR - SpiFlashWrite Error  %r\n", 
> Status));
> +          if (FvHeader != NULL) {
> +            FreePool (FvHeader);
> +          }
>             continue;
>           }
>           if (BytesWritten != FvHeader->HeaderLength) {
>             DEBUG ((DEBUG_WARN, "ERROR - BytesWritten != HeaderLength\n"));
>             DEBUG ((DEBUG_INFO, " BytesWritten = 0x%X\n HeaderLength = 
> 0x%X\n", BytesWritten, FvHeader->HeaderLength));
> +          if (FvHeader != NULL) {
> +            FreePool (FvHeader);
> +          }
>             continue;
>           }
>           Status = SpiFlashLock ();
>           if (EFI_ERROR (Status)) {
>             DEBUG ((DEBUG_WARN, "ERROR - SpiFlashLock Error  %r\n", 
> Status));
> +          if (FvHeader != NULL) {
> +            FreePool (FvHeader);
> +          }
>             continue;
>           }
>           DEBUG ((DEBUG_INFO, "FV Header @ 0x%X restored with static 
> data\n", BaseAddress)); @@ -184,6 +216,9 @@ FvbInitialize (
>           // Clear cache for this range.
>           //
>           WriteBackInvalidateDataCacheRange ( (VOID *) (UINTN) 
> BaseAddress, FvHeader->BlockMap->Length);
> +        if (FvHeader != NULL) {
> +          FreePool (FvHeader);
> +        }
>         }
>       }
>   
> diff --git 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceCommon.h 
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceCommon.h index e9d69e985814..7b0908b03fdc 100644
> --- 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceCommon.h
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvb
> +++ ServiceCommon.h
> @@ -2,6 +2,7 @@
>     Common source definitions used in serial flash drivers
>   
>   Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) Microsoft Corporation.<BR>
>   SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>   **/
> @@ -12,6 +13,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>   #include <Guid/EventGroup.h>
>   #include <Guid/FirmwareFileSystem2.h>
>   #include <Guid/SystemNvDataGuid.h>
> +#include <Pi/PiFirmwareVolume.h>
>   #include <Protocol/DevicePath.h>
>   #include <Protocol/FirmwareVolumeBlock.h>
>   
> @@ -24,6 +26,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>   #include <Library/PcdLib.h>
>   #include <Library/DevicePathLib.h>
>   #include <Library/HobLib.h>
> +#include <Library/VariableFlashInfoLib.h> #include 
> +<Library/SafeIntLib.h>
>   
>   #include <Library/SpiFlashCommonLib.h>
>   
> @@ -148,11 +152,23 @@ GetFvbInfo (
>     OUT EFI_FIRMWARE_VOLUME_HEADER   **FvbInfo
>     );
>   
> +/**
> +  Get the total size of the firmware volume on flash used for variable store operations.
> +
> +  @param[out] BaseAddress         Base address of the variable store firmware volume.
> +  @param[out] Length              Length in bytes of the firmware volume used for variable store operations.
> +
> +**/
> +VOID
> +GetVariableFvInfo (
> +  OUT EFI_PHYSICAL_ADDRESS      *BaseAddress,
> +  OUT UINT32                    *Length
> +  );
> +
>   extern FVB_GLOBAL                         mFvbModuleGlobal;
>   extern FV_MEMMAP_DEVICE_PATH              mFvMemmapDevicePathTemplate;
>   extern FV_PIWG_DEVICE_PATH                mFvPIWGDevicePathTemplate;
>   extern EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL mFvbProtocolTemplate;
>   extern FV_INFO                            mPlatformFvBaseAddress[];
> -extern FV_INFO                            mPlatformDefaultBaseAddress[];
>   
>   #endif
> diff --git 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceSmm.inf 
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceSmm.inf index bf1306f00201..0cfa3f909bdf 100644
> --- 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceSmm.inf
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvb
> +++ ServiceSmm.inf
> @@ -32,8 +32,10 @@ [LibraryClasses]
>     BaseLib
>     UefiBootServicesTableLib
>     UefiDriverEntryPoint
> +  SafeIntLib
>     SpiFlashCommonLib
>     MmServicesTableLib
> +  VariableFlashInfoLib
>   
>   [Packages]
>     MdePkg/MdePkg.dec
> @@ -41,10 +43,6 @@ [Packages]
>     IntelSiliconPkg/IntelSiliconPkg.dec
>   
>   [Pcd]
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase   ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize   ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize   ## CONSUMES
>     gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvBase         ## CONSUMES
>     gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvSize         ## CONSUMES
>   
> diff --git 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceStandaloneMm.inf 
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceStandaloneMm.inf index b66233968247..152cf0036fdb 100644
> --- 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceStandaloneMm.inf
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvb
> +++ ServiceStandaloneMm.inf
> @@ -31,8 +31,10 @@ [LibraryClasses]
>     MemoryAllocationLib
>     PcdLib
>     MmServicesTableLib
> +  SafeIntLib
>     SpiFlashCommonLib
>     StandaloneMmDriverEntryPoint
> +  VariableFlashInfoLib
>   
>   [Packages]
>     MdePkg/MdePkg.dec
> @@ -40,10 +42,6 @@ [Packages]
>     IntelSiliconPkg/IntelSiliconPkg.dec
>   
>   [Pcd]
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase   ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize   ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize   ## CONSUMES
>     gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvBase         ## CONSUMES
>     gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvSize         ## CONSUMES
>   
> diff --git a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc 
> b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
> index 1e826a080f28..170eb480ada1 100644
> --- a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
> +++ b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
> @@ -40,9 +40,11 @@ [LibraryClasses]
>     PeiGetVtdPmrAlignmentLib|IntelSiliconPkg/Library/PeiGetVtdPmrAlignmentLib/PeiGetVtdPmrAlignmentLib.inf
>     TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
>     MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
> +  SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
>     SpiFlashCommonLib|IntelSiliconPkg/Library/SpiFlashCommonLibNull/SpiFlashCommonLibNull.inf
>     UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf
>     
> UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEnt
> ryPoint.inf
> +  
> + VariableFlashInfoLib|MdeModulePkg/Library/BaseVariableFlashInfoLib/B
> + aseVariableFlashInfoLib.inf
>   
>   [LibraryClasses.common.PEIM]
>     PeimEntryPoint|MdePkg/Library/PeimEntryPoint/PeimEntryPoint.inf
> @@ -63,11 +65,16 @@ [LibraryClasses.common.DXE_DRIVER]
>     
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryA
> llocationLib.inf
>   
>   [LibraryClasses.common.DXE_SMM_DRIVER]
> +  
> + DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
> +  HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
>     MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLib/SmmMemoryAllocationLib.inf
>     MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf
>     
> SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesTabl
> eLib.inf
> +  UefiLib|MdePkg/Library/UefiLib/UefiLib.inf
> +  
> + UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableL
> + ib/UefiRuntimeServicesTableLib.inf
>   
>   [LibraryClasses.common.MM_STANDALONE]
> +  
> + HobLib|StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib
> + .inf
>     MemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMemoryAllocationLib.inf
>     MmServicesTableLib|MdePkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.inf
>     
> StandaloneMmDriverEntryPoint|MdePkg/Library/StandaloneMmDriverEntryPoi
> nt/StandaloneMmDriverEntryPoint.inf






^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [edk2-devel] [edk2-platforms][PATCH v2 1/1] IntelSiliconPkg/SpiFvbService: Add support for VariableFlashInfoLib
  2022-07-12 23:04 ` [edk2-devel] [edk2-platforms][PATCH v2 1/1] IntelSiliconPkg/SpiFvbService: Add support for VariableFlashInfoLib Michael Kubacki
  2022-07-13 15:51   ` Oram, Isaac W
@ 2022-07-13 16:49   ` Chaganty, Rangasai V
  2022-07-13 20:59     ` Oram, Isaac W
  1 sibling, 1 reply; 4+ messages in thread
From: Chaganty, Rangasai V @ 2022-07-13 16:49 UTC (permalink / raw)
  To: devel@edk2.groups.io, mikuback@linux.microsoft.com
  Cc: Ni, Ray, Oram, Isaac W, Desimone, Nathaniel L

Reviewed-by: Sai Chaganty <rangasai.v.chaganty@intel.com>

-----Original Message-----
From: Michael Kubacki <mikuback@linux.microsoft.com> 
Sent: Tuesday, July 12, 2022 4:04 PM
To: devel@edk2.groups.io
Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Ni, Ray <ray.ni@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Subject: Re: [edk2-devel] [edk2-platforms][PATCH v2 1/1] IntelSiliconPkg/SpiFvbService: Add support for VariableFlashInfoLib

Reminder about this patch.

Thanks,
Michael

On 7/1/2022 10:39 AM, Michael Kubacki wrote:
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3478
> 
> Adds support for getting the variable flash information from 
> VariableFlashInfoLib. This library abstracts the source of flash 
> information so platforms could expect the information to come from a 
> different source in the library implementation than the PCDs 
> previously used as the information source in this module.
> 
> In particular, the library allows Standalone MM platforms to 
> dynamically pass the information behind the library API.
> 
> Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Isaac Oram <isaac.w.oram@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
> 
> Notes:
>      V2 changes:
>      
>      1. Updated the buffer that is returned from
>         GetFvbInfo() to be allocated from pool.
>      2. Updated calls to that function in FvbInitialize()
>         to free the new buffer after usage.
>      3. Updated the multiplication operation to determine
>         the FV length to use safe multiplication in
>         GenerateNvStorageFvbMediaInfo().
> 
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c                     | 128 +++++++++++++-------
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c         |  93 ++++++++++++--
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c             |  49 ++++++--
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h         |  18 ++-
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.inf          |   6 +-
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceStandaloneMm.inf |   6 +-
>   Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc                                       |   7 ++
>   7 files changed, 243 insertions(+), 64 deletions(-)
> 
> diff --git 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c 
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c
> index 7f2678fa9e5a..634a44218c7a 100644
> --- 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInf
> +++ o.c
> @@ -3,6 +3,7 @@
>     These data is intent to decouple FVB driver with FV header.
>   
>   Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) Microsoft Corporation.<BR>
>   SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>   **/
> @@ -11,51 +12,92 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>   #define FIRMWARE_BLOCK_SIZE         0x10000
>   #define FVB_MEDIA_BLOCK_SIZE        FIRMWARE_BLOCK_SIZE
> -
> -#define NV_STORAGE_BASE_ADDRESS     FixedPcdGet32(PcdFlashNvStorageVariableBase)
> -#define SYSTEM_NV_BLOCK_NUM         ((FixedPcdGet32(PcdFlashNvStorageVariableSize)+ FixedPcdGet32(PcdFlashNvStorageFtwWorkingSize) + FixedPcdGet32(PcdFlashNvStorageFtwSpareSize))/ FVB_MEDIA_BLOCK_SIZE)
> -
>   typedef struct {
>     EFI_PHYSICAL_ADDRESS        BaseAddress;
>     EFI_FIRMWARE_VOLUME_HEADER  FvbInfo;
>     EFI_FV_BLOCK_MAP_ENTRY      End[1];
>   } EFI_FVB2_MEDIA_INFO;
>   
> -//
> -// This data structure contains a template of all correct FV headers, 
> which is used to restore -// Fv header if it's corrupted.
> -//
> -EFI_FVB2_MEDIA_INFO mPlatformFvbMediaInfo[] = {
> -  //
> -  // Systen NvStorage FVB
> -  //
> -  {
> -    NV_STORAGE_BASE_ADDRESS,
> -    {
> -      {0,}, //ZeroVector[16]
> -      EFI_SYSTEM_NV_DATA_FV_GUID,
> -      FVB_MEDIA_BLOCK_SIZE * SYSTEM_NV_BLOCK_NUM,
> -      EFI_FVH_SIGNATURE,
> -      0x0004feff, // check MdePkg/Include/Pi/PiFirmwareVolume.h for details on EFI_FVB_ATTRIBUTES_2
> -      sizeof (EFI_FIRMWARE_VOLUME_HEADER) + sizeof (EFI_FV_BLOCK_MAP_ENTRY),
> -      0,    //CheckSum which will be calucated dynamically.
> -      0,    //ExtHeaderOffset
> -      {0,}, //Reserved[1]
> -      2,    //Revision
> -      {
> -        {
> -          SYSTEM_NV_BLOCK_NUM,
> -          FVB_MEDIA_BLOCK_SIZE,
> -        }
> -      }
> -    },
> -    {
> -      {
> -        0,
> -        0
> -      }
> -    }
> +/**
> +  Returns FVB media information for NV variable storage.
> +
> +  @return       FvbMediaInfo          A pointer to an instance of FVB media info produced by this function.
> +                                      The buffer is allocated internally to this function and it is the caller's
> +                                      responsibility to free the 
> + memory
> +
> +**/
> +typedef
> +EFI_STATUS
> +(*FVB_MEDIA_INFO_GENERATOR)(
> +  OUT EFI_FVB2_MEDIA_INFO     *FvbMediaInfo
> +  );
> +
> +/**
> +  Returns FVB media information for NV variable storage.
> +
> +  @param[out]   FvbMediaInfo          A pointer to an instance of FVB media info produced by this function.
> +
> +  @retval       EFI_SUCCESS           A structure was successfully written to the FvbMediaInfo buffer.
> +  @retval       EFI_INVALID_PARAMETER The FvbMediaInfo parameter is NULL.
> +  @retval       EFI_UNSUPPORTED       An error occurred retrieving variable FV information.
> +  @retval       EFI_BAD_BUFFER_SIZE   An overflow or underflow of the FV buffer occurred with the information found.
> +
> +**/
> +EFI_STATUS
> +GenerateNvStorageFvbMediaInfo (
> +  OUT EFI_FVB2_MEDIA_INFO     *FvbMediaInfo
> +  )
> +{
> +  EFI_STATUS                  Status;
> +  UINT32                      NvBlockNum;
> +  UINT32                      TotalNvVariableStorageSize;
> +  EFI_PHYSICAL_ADDRESS        NvStorageBaseAddress;
> +  EFI_FIRMWARE_VOLUME_HEADER  FvbInfo = {
> +                                          {0,},                                   //ZeroVector[16]
> +                                          EFI_SYSTEM_NV_DATA_FV_GUID,             //FileSystemGuid
> +                                          0,                                      //FvLength
> +                                          EFI_FVH_SIGNATURE,                      //Signature
> +                                          0x0004feff,                             //Attributes
> +                                          sizeof (EFI_FIRMWARE_VOLUME_HEADER) +   //HeaderLength
> +                                            sizeof (EFI_FV_BLOCK_MAP_ENTRY),
> +                                          0,                                      //Checksum
> +                                          0,                                      //ExtHeaderOffset
> +                                          {0,},                                   //Reserved[1]
> +                                          2,                                      //Revision
> +                                          {                                       //BlockMap[1]
> +                                            {0,0}
> +                                          }
> +                                        };
> +
> +  if (FvbMediaInfo == NULL) {
> +    return EFI_INVALID_PARAMETER;
>     }
> +
> +  ZeroMem (FvbMediaInfo, sizeof (*FvbMediaInfo));
> +
> +  GetVariableFvInfo (&NvStorageBaseAddress, 
> + &TotalNvVariableStorageSize);  if ((NvStorageBaseAddress == 0) || (TotalNvVariableStorageSize == 0)) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  NvBlockNum = TotalNvVariableStorageSize / FVB_MEDIA_BLOCK_SIZE;
> +
> +  Status = SafeUint64Mult ((UINT64)NvBlockNum, FVB_MEDIA_BLOCK_SIZE, 
> + &FvbInfo.FvLength);  if (EFI_ERROR (Status)) {
> +    return EFI_BAD_BUFFER_SIZE;
> +  }
> +
> +  FvbInfo.BlockMap[0].NumBlocks = NvBlockNum;  
> + FvbInfo.BlockMap[0].Length = FVB_MEDIA_BLOCK_SIZE;
> +
> +  FvbMediaInfo->BaseAddress = NvStorageBaseAddress;  CopyMem 
> + (&FvbMediaInfo->FvbInfo, &FvbInfo, sizeof (FvbInfo));
> +
> +  return EFI_SUCCESS;
> +}
> +
> +FVB_MEDIA_INFO_GENERATOR mFvbMediaInfoGenerators[] = {
> +  GenerateNvStorageFvbMediaInfo
>   };
>   
>   EFI_STATUS
> @@ -64,12 +106,16 @@ GetFvbInfo (
>     OUT EFI_FIRMWARE_VOLUME_HEADER   **FvbInfo
>     )
>   {
> +  EFI_STATUS                  Status;
> +  EFI_FVB2_MEDIA_INFO         FvbMediaInfo;
>     UINTN                       Index;
>     EFI_FIRMWARE_VOLUME_HEADER  *FvHeader;
>   
> -  for (Index = 0; Index < sizeof (mPlatformFvbMediaInfo) / sizeof (EFI_FVB2_MEDIA_INFO); Index++) {
> -    if (mPlatformFvbMediaInfo[Index].BaseAddress == FvBaseAddress) {
> -      FvHeader = &mPlatformFvbMediaInfo[Index].FvbInfo;
> +  for (Index = 0; Index < ARRAY_SIZE (mFvbMediaInfoGenerators); Index++) {
> +    Status = mFvbMediaInfoGenerators[Index](&FvbMediaInfo);
> +    ASSERT_EFI_ERROR (Status);
> +    if (!EFI_ERROR (Status) && (FvbMediaInfo.BaseAddress == FvBaseAddress)) {
> +      FvHeader = AllocateCopyPool (sizeof 
> + (EFI_FIRMWARE_VOLUME_HEADER), &FvbMediaInfo.FvbInfo);
>   
>         //
>         // Update the checksum value of FV header.
> diff --git 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceCommon.c 
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceCommon.c index dab818e98087..942abf95a64f 100644
> --- 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceCommon.c
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvb
> +++ ServiceCommon.c
> @@ -3,6 +3,7 @@
>     which are compliant with the Intel(R) Serial Flash Interface Compatibility Specification.
>   
>   Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) Microsoft Corporation.<BR>
>   SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>   **/
> @@ -25,12 +26,6 @@ FV_INFO mPlatformFvBaseAddress[] = {
>     {0, 0}
>   };
>   
> -FV_INFO mPlatformDefaultBaseAddress[] = {
> -  {0, 0}, // {FixedPcdGet32(PcdFlashNvStorageVariableBase), 
> FixedPcdGet32(PcdFlashNvStorageVariableSize)},
> -  {0, 0}, // {FixedPcdGet32(PcdFlashMicrocodeFvBase), 
> FixedPcdGet32(PcdFlashMicrocodeFvSize)},
> -  {0, 0}
> -};
> -
>   FV_MEMMAP_DEVICE_PATH mFvMemmapDevicePathTemplate = {
>     {
>       {
> @@ -530,6 +525,85 @@ FvbSetVolumeAttributes (
>     return EFI_SUCCESS;
>   }
>   
> +/**
> +  Get the total size of the firmware volume on flash used for variable store operations.
> +
> +  @param[out] BaseAddress         Base address of the variable store firmware volume.
> +  @param[out] Length              Length in bytes of the firmware volume used for variable store operations.
> +
> +**/
> +VOID
> +GetVariableFvInfo (
> +  OUT EFI_PHYSICAL_ADDRESS      *BaseAddress,
> +  OUT UINT32                    *Length
> +  )
> +{
> +  EFI_STATUS                    Status;
> +  EFI_PHYSICAL_ADDRESS          NvBaseAddress;
> +  EFI_PHYSICAL_ADDRESS          NvVariableBaseAddress;
> +  UINT64                        Length64;
> +  UINT32                        NvStoreLength;
> +  UINT32                        FtwSpareLength;
> +  UINT32                        FtwWorkingLength;
> +  UINT32                        TotalLength;
> +
> +  TotalLength = 0;
> +  Status = EFI_SUCCESS;
> +
> +  if ((BaseAddress == NULL) || (Length == NULL)) {
> +    ASSERT ((BaseAddress != NULL) && (Length != NULL));
> +    return;
> +  }
> +  *BaseAddress = 0;
> +  *Length = 0;
> +
> +  Status = GetVariableFlashNvStorageInfo (&NvBaseAddress, &Length64);  
> + if (!EFI_ERROR (Status)) {
> +    NvVariableBaseAddress = NvBaseAddress;
> +    // Stay within the current UINT32 size assumptions in the variable stack.
> +    Status = SafeUint64ToUint32 (Length64, &NvStoreLength);  }  if 
> + (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return;
> +  }
> +
> +  Status = GetVariableFlashFtwSpareInfo (&NvBaseAddress, &Length64);  
> + if (!EFI_ERROR (Status)) {
> +    // Stay within the current UINT32 size assumptions in the variable stack.
> +    Status = SafeUint64ToUint32 (Length64, &FtwSpareLength);  }  if 
> + (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return;
> +  }
> +
> +  Status = GetVariableFlashFtwWorkingInfo (&NvBaseAddress, 
> + &Length64);  if (!EFI_ERROR (Status)) {
> +    // Stay within the current UINT32 size assumptions in the variable stack.
> +    Status = SafeUint64ToUint32 (Length64, &FtwWorkingLength);  }  if 
> + (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return;
> +  }
> +
> +  Status = SafeUint32Add (NvStoreLength, FtwSpareLength, 
> + &TotalLength);  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return;
> +  }
> +
> +  Status = SafeUint32Add (TotalLength, FtwWorkingLength, 
> + &TotalLength);  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return;
> +  }
> +
> +  *BaseAddress = NvVariableBaseAddress;
> +  *Length = TotalLength;
> +}
> +
>   /**
>     Check the integrity of firmware volume header
>   
> @@ -545,7 +619,12 @@ IsFvHeaderValid (
>     IN CONST EFI_FIRMWARE_VOLUME_HEADER    *FvHeader
>     )
>   {
> -  if (FvBase == PcdGet32(PcdFlashNvStorageVariableBase)) {
> +  EFI_PHYSICAL_ADDRESS    NvStorageFvBaseAddress;
> +  UINT32                  NvStorageSize;
> +
> +  GetVariableFvInfo (&NvStorageFvBaseAddress, &NvStorageSize);
> +
> +  if (FvBase == NvStorageFvBaseAddress) {
>       if (CompareMem (&FvHeader->FileSystemGuid, &gEfiSystemNvDataFvGuid, sizeof(EFI_GUID)) != 0 ) {
>         return FALSE;
>       }
> diff --git 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceMm.c 
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceMm.c index 42a0828c6fae..6b4bcdcfe3c4 100644
> --- 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceMm.c
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvb
> +++ ServiceMm.c
> @@ -113,15 +113,31 @@ FvbInitialize (
>     UINT32                                MaxLbaSize;
>     UINT32                                BytesWritten;
>     UINTN                                 BytesErased;
> +  UINT64                                NvStorageFvSize;
> +
> +  Status = GetVariableFlashNvStorageInfo (&BaseAddress, 
> + &NvStorageFvSize);  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    DEBUG ((DEBUG_ERROR, "[%a] - An error ocurred getting variable info - %r.\n", __FUNCTION__, Status));
> +    return;
> +  }
> +
> +  // Stay within the current UINT32 size assumptions in the variable stack.
> +  Status = SafeUint64ToUint32 (BaseAddress, 
> + &mPlatformFvBaseAddress[0].FvBase);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    DEBUG ((DEBUG_ERROR, "[%a] - 64-bit variable storage base address not supported.\n", __FUNCTION__));
> +    return;
> +  }
> +  Status = SafeUint64ToUint32 (NvStorageFvSize, 
> + &mPlatformFvBaseAddress[0].FvSize);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    DEBUG ((DEBUG_ERROR, "[%a] - 64-bit variable storage size not supported.\n", __FUNCTION__));
> +    return;
> +  }
>   
> -  mPlatformFvBaseAddress[0].FvBase = 
> PcdGet32(PcdFlashNvStorageVariableBase);
> -  mPlatformFvBaseAddress[0].FvSize = PcdGet32(PcdFlashNvStorageVariableSize);
>     mPlatformFvBaseAddress[1].FvBase = PcdGet32(PcdFlashMicrocodeFvBase);
>     mPlatformFvBaseAddress[1].FvSize = 
> PcdGet32(PcdFlashMicrocodeFvSize);
> -  mPlatformDefaultBaseAddress[0].FvBase = 
> PcdGet32(PcdFlashNvStorageVariableBase);
> -  mPlatformDefaultBaseAddress[0].FvSize = 
> PcdGet32(PcdFlashNvStorageVariableSize);
> -  mPlatformDefaultBaseAddress[1].FvBase = 
> PcdGet32(PcdFlashMicrocodeFvBase);
> -  mPlatformDefaultBaseAddress[1].FvSize = 
> PcdGet32(PcdFlashMicrocodeFvSize);
>   
>     //
>     // We will only continue with FVB installation if the @@ -143,7 
> +159,8 @@ FvbInitialize (
>           BytesWritten = 0;
>           BytesErased = 0;
>           DEBUG ((DEBUG_ERROR, "ERROR - The FV in 0x%x is invalid!\n", FvHeader));
> -        Status = GetFvbInfo (BaseAddress, &FvHeader);
> +        FvHeader = NULL;
> +        Status   = GetFvbInfo (BaseAddress, &FvHeader);
>           if (EFI_ERROR (Status)) {
>             DEBUG ((DEBUG_WARN, "ERROR - Can't recovery FV header at 0x%x.  GetFvbInfo Status %r\n", BaseAddress, Status));
>             continue;
> @@ -156,27 +173,42 @@ FvbInitialize (
>           Status = SpiFlashBlockErase( (UINTN) BaseAddress, &BytesErased);
>           if (EFI_ERROR (Status)) {
>             DEBUG ((DEBUG_WARN, "ERROR - SpiFlashBlockErase Error  
> %r\n", Status));
> +          if (FvHeader != NULL) {
> +            FreePool (FvHeader);
> +          }
>             continue;
>           }
>           if (BytesErased != FvHeader->BlockMap->Length) {
>             DEBUG ((DEBUG_WARN, "ERROR - BytesErased != FvHeader->BlockMap->Length\n"));
>             DEBUG ((DEBUG_INFO, " BytesErased = 0x%X\n Length = 
> 0x%X\n", BytesErased, FvHeader->BlockMap->Length));
> +          if (FvHeader != NULL) {
> +            FreePool (FvHeader);
> +          }
>             continue;
>           }
>           BytesWritten = FvHeader->HeaderLength;
>           Status = SpiFlashWrite ((UINTN)BaseAddress, &BytesWritten, (UINT8*)FvHeader);
>           if (EFI_ERROR (Status)) {
>             DEBUG ((DEBUG_WARN, "ERROR - SpiFlashWrite Error  %r\n", 
> Status));
> +          if (FvHeader != NULL) {
> +            FreePool (FvHeader);
> +          }
>             continue;
>           }
>           if (BytesWritten != FvHeader->HeaderLength) {
>             DEBUG ((DEBUG_WARN, "ERROR - BytesWritten != HeaderLength\n"));
>             DEBUG ((DEBUG_INFO, " BytesWritten = 0x%X\n HeaderLength = 
> 0x%X\n", BytesWritten, FvHeader->HeaderLength));
> +          if (FvHeader != NULL) {
> +            FreePool (FvHeader);
> +          }
>             continue;
>           }
>           Status = SpiFlashLock ();
>           if (EFI_ERROR (Status)) {
>             DEBUG ((DEBUG_WARN, "ERROR - SpiFlashLock Error  %r\n", 
> Status));
> +          if (FvHeader != NULL) {
> +            FreePool (FvHeader);
> +          }
>             continue;
>           }
>           DEBUG ((DEBUG_INFO, "FV Header @ 0x%X restored with static 
> data\n", BaseAddress)); @@ -184,6 +216,9 @@ FvbInitialize (
>           // Clear cache for this range.
>           //
>           WriteBackInvalidateDataCacheRange ( (VOID *) (UINTN) 
> BaseAddress, FvHeader->BlockMap->Length);
> +        if (FvHeader != NULL) {
> +          FreePool (FvHeader);
> +        }
>         }
>       }
>   
> diff --git 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceCommon.h 
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceCommon.h index e9d69e985814..7b0908b03fdc 100644
> --- 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceCommon.h
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvb
> +++ ServiceCommon.h
> @@ -2,6 +2,7 @@
>     Common source definitions used in serial flash drivers
>   
>   Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) Microsoft Corporation.<BR>
>   SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>   **/
> @@ -12,6 +13,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>   #include <Guid/EventGroup.h>
>   #include <Guid/FirmwareFileSystem2.h>
>   #include <Guid/SystemNvDataGuid.h>
> +#include <Pi/PiFirmwareVolume.h>
>   #include <Protocol/DevicePath.h>
>   #include <Protocol/FirmwareVolumeBlock.h>
>   
> @@ -24,6 +26,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>   #include <Library/PcdLib.h>
>   #include <Library/DevicePathLib.h>
>   #include <Library/HobLib.h>
> +#include <Library/VariableFlashInfoLib.h> #include 
> +<Library/SafeIntLib.h>
>   
>   #include <Library/SpiFlashCommonLib.h>
>   
> @@ -148,11 +152,23 @@ GetFvbInfo (
>     OUT EFI_FIRMWARE_VOLUME_HEADER   **FvbInfo
>     );
>   
> +/**
> +  Get the total size of the firmware volume on flash used for variable store operations.
> +
> +  @param[out] BaseAddress         Base address of the variable store firmware volume.
> +  @param[out] Length              Length in bytes of the firmware volume used for variable store operations.
> +
> +**/
> +VOID
> +GetVariableFvInfo (
> +  OUT EFI_PHYSICAL_ADDRESS      *BaseAddress,
> +  OUT UINT32                    *Length
> +  );
> +
>   extern FVB_GLOBAL                         mFvbModuleGlobal;
>   extern FV_MEMMAP_DEVICE_PATH              mFvMemmapDevicePathTemplate;
>   extern FV_PIWG_DEVICE_PATH                mFvPIWGDevicePathTemplate;
>   extern EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL mFvbProtocolTemplate;
>   extern FV_INFO                            mPlatformFvBaseAddress[];
> -extern FV_INFO                            mPlatformDefaultBaseAddress[];
>   
>   #endif
> diff --git 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceSmm.inf 
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceSmm.inf index bf1306f00201..0cfa3f909bdf 100644
> --- 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceSmm.inf
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvb
> +++ ServiceSmm.inf
> @@ -32,8 +32,10 @@ [LibraryClasses]
>     BaseLib
>     UefiBootServicesTableLib
>     UefiDriverEntryPoint
> +  SafeIntLib
>     SpiFlashCommonLib
>     MmServicesTableLib
> +  VariableFlashInfoLib
>   
>   [Packages]
>     MdePkg/MdePkg.dec
> @@ -41,10 +43,6 @@ [Packages]
>     IntelSiliconPkg/IntelSiliconPkg.dec
>   
>   [Pcd]
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase   ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize   ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize   ## CONSUMES
>     gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvBase         ## CONSUMES
>     gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvSize         ## CONSUMES
>   
> diff --git 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceStandaloneMm.inf 
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceStandaloneMm.inf index b66233968247..152cf0036fdb 100644
> --- 
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceStandaloneMm.inf
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvb
> +++ ServiceStandaloneMm.inf
> @@ -31,8 +31,10 @@ [LibraryClasses]
>     MemoryAllocationLib
>     PcdLib
>     MmServicesTableLib
> +  SafeIntLib
>     SpiFlashCommonLib
>     StandaloneMmDriverEntryPoint
> +  VariableFlashInfoLib
>   
>   [Packages]
>     MdePkg/MdePkg.dec
> @@ -40,10 +42,6 @@ [Packages]
>     IntelSiliconPkg/IntelSiliconPkg.dec
>   
>   [Pcd]
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase   ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize   ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize   ## CONSUMES
>     gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvBase         ## CONSUMES
>     gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvSize         ## CONSUMES
>   
> diff --git a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc 
> b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
> index 1e826a080f28..170eb480ada1 100644
> --- a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
> +++ b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
> @@ -40,9 +40,11 @@ [LibraryClasses]
>     PeiGetVtdPmrAlignmentLib|IntelSiliconPkg/Library/PeiGetVtdPmrAlignmentLib/PeiGetVtdPmrAlignmentLib.inf
>     TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
>     MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
> +  SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
>     SpiFlashCommonLib|IntelSiliconPkg/Library/SpiFlashCommonLibNull/SpiFlashCommonLibNull.inf
>     UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiBootServicesTableLib.inf
>     
> UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEnt
> ryPoint.inf
> +  
> + VariableFlashInfoLib|MdeModulePkg/Library/BaseVariableFlashInfoLib/B
> + aseVariableFlashInfoLib.inf
>   
>   [LibraryClasses.common.PEIM]
>     PeimEntryPoint|MdePkg/Library/PeimEntryPoint/PeimEntryPoint.inf
> @@ -63,11 +65,16 @@ [LibraryClasses.common.DXE_DRIVER]
>     
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryA
> llocationLib.inf
>   
>   [LibraryClasses.common.DXE_SMM_DRIVER]
> +  
> + DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
> +  HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
>     MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLib/SmmMemoryAllocationLib.inf
>     MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf
>     
> SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesTabl
> eLib.inf
> +  UefiLib|MdePkg/Library/UefiLib/UefiLib.inf
> +  
> + UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableL
> + ib/UefiRuntimeServicesTableLib.inf
>   
>   [LibraryClasses.common.MM_STANDALONE]
> +  
> + HobLib|StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib
> + .inf
>     MemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMemoryAllocationLib.inf
>     MmServicesTableLib|MdePkg/Library/StandaloneMmServicesTableLib/StandaloneMmServicesTableLib.inf
>     
> StandaloneMmDriverEntryPoint|MdePkg/Library/StandaloneMmDriverEntryPoi
> nt/StandaloneMmDriverEntryPoint.inf

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [edk2-devel] [edk2-platforms][PATCH v2 1/1] IntelSiliconPkg/SpiFvbService: Add support for VariableFlashInfoLib
  2022-07-13 16:49   ` Chaganty, Rangasai V
@ 2022-07-13 20:59     ` Oram, Isaac W
  0 siblings, 0 replies; 4+ messages in thread
From: Oram, Isaac W @ 2022-07-13 20:59 UTC (permalink / raw)
  To: Chaganty, Rangasai V, devel@edk2.groups.io,
	mikuback@linux.microsoft.com
  Cc: Ni, Ray, Desimone, Nathaniel L

Pushed as 9935472b19..bc93dea9b9

-----Original Message-----
From: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com> 
Sent: Wednesday, July 13, 2022 9:49 AM
To: devel@edk2.groups.io; mikuback@linux.microsoft.com
Cc: Ni, Ray <ray.ni@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Subject: RE: [edk2-devel] [edk2-platforms][PATCH v2 1/1] IntelSiliconPkg/SpiFvbService: Add support for VariableFlashInfoLib

Reviewed-by: Sai Chaganty <rangasai.v.chaganty@intel.com>

-----Original Message-----
From: Michael Kubacki <mikuback@linux.microsoft.com>
Sent: Tuesday, July 12, 2022 4:04 PM
To: devel@edk2.groups.io
Cc: Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Ni, Ray <ray.ni@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Subject: Re: [edk2-devel] [edk2-platforms][PATCH v2 1/1] IntelSiliconPkg/SpiFvbService: Add support for VariableFlashInfoLib

Reminder about this patch.

Thanks,
Michael

On 7/1/2022 10:39 AM, Michael Kubacki wrote:
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3478
> 
> Adds support for getting the variable flash information from 
> VariableFlashInfoLib. This library abstracts the source of flash 
> information so platforms could expect the information to come from a 
> different source in the library implementation than the PCDs 
> previously used as the information source in this module.
> 
> In particular, the library allows Standalone MM platforms to 
> dynamically pass the information behind the library API.
> 
> Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Isaac Oram <isaac.w.oram@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
> 
> Notes:
>      V2 changes:
>      
>      1. Updated the buffer that is returned from
>         GetFvbInfo() to be allocated from pool.
>      2. Updated calls to that function in FvbInitialize()
>         to free the new buffer after usage.
>      3. Updated the multiplication operation to determine
>         the FV length to use safe multiplication in
>         GenerateNvStorageFvbMediaInfo().
> 
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c                     | 128 +++++++++++++-------
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c         |  93 ++++++++++++--
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c             |  49 ++++++--
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h         |  18 ++-
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.inf          |   6 +-
>   Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceStandaloneMm.inf |   6 +-
>   Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc                                       |   7 ++
>   7 files changed, 243 insertions(+), 64 deletions(-)
> 
> diff --git
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c
> index 7f2678fa9e5a..634a44218c7a 100644
> ---
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInf
> +++ o.c
> @@ -3,6 +3,7 @@
>     These data is intent to decouple FVB driver with FV header.
>   
>   Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) Microsoft Corporation.<BR>
>   SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>   **/
> @@ -11,51 +12,92 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>   #define FIRMWARE_BLOCK_SIZE         0x10000
>   #define FVB_MEDIA_BLOCK_SIZE        FIRMWARE_BLOCK_SIZE
> -
> -#define NV_STORAGE_BASE_ADDRESS     FixedPcdGet32(PcdFlashNvStorageVariableBase)
> -#define SYSTEM_NV_BLOCK_NUM         ((FixedPcdGet32(PcdFlashNvStorageVariableSize)+ FixedPcdGet32(PcdFlashNvStorageFtwWorkingSize) + FixedPcdGet32(PcdFlashNvStorageFtwSpareSize))/ FVB_MEDIA_BLOCK_SIZE)
> -
>   typedef struct {
>     EFI_PHYSICAL_ADDRESS        BaseAddress;
>     EFI_FIRMWARE_VOLUME_HEADER  FvbInfo;
>     EFI_FV_BLOCK_MAP_ENTRY      End[1];
>   } EFI_FVB2_MEDIA_INFO;
>   
> -//
> -// This data structure contains a template of all correct FV headers, 
> which is used to restore -// Fv header if it's corrupted.
> -//
> -EFI_FVB2_MEDIA_INFO mPlatformFvbMediaInfo[] = {
> -  //
> -  // Systen NvStorage FVB
> -  //
> -  {
> -    NV_STORAGE_BASE_ADDRESS,
> -    {
> -      {0,}, //ZeroVector[16]
> -      EFI_SYSTEM_NV_DATA_FV_GUID,
> -      FVB_MEDIA_BLOCK_SIZE * SYSTEM_NV_BLOCK_NUM,
> -      EFI_FVH_SIGNATURE,
> -      0x0004feff, // check MdePkg/Include/Pi/PiFirmwareVolume.h for details on EFI_FVB_ATTRIBUTES_2
> -      sizeof (EFI_FIRMWARE_VOLUME_HEADER) + sizeof (EFI_FV_BLOCK_MAP_ENTRY),
> -      0,    //CheckSum which will be calucated dynamically.
> -      0,    //ExtHeaderOffset
> -      {0,}, //Reserved[1]
> -      2,    //Revision
> -      {
> -        {
> -          SYSTEM_NV_BLOCK_NUM,
> -          FVB_MEDIA_BLOCK_SIZE,
> -        }
> -      }
> -    },
> -    {
> -      {
> -        0,
> -        0
> -      }
> -    }
> +/**
> +  Returns FVB media information for NV variable storage.
> +
> +  @return       FvbMediaInfo          A pointer to an instance of FVB media info produced by this function.
> +                                      The buffer is allocated internally to this function and it is the caller's
> +                                      responsibility to free the 
> + memory
> +
> +**/
> +typedef
> +EFI_STATUS
> +(*FVB_MEDIA_INFO_GENERATOR)(
> +  OUT EFI_FVB2_MEDIA_INFO     *FvbMediaInfo
> +  );
> +
> +/**
> +  Returns FVB media information for NV variable storage.
> +
> +  @param[out]   FvbMediaInfo          A pointer to an instance of FVB media info produced by this function.
> +
> +  @retval       EFI_SUCCESS           A structure was successfully written to the FvbMediaInfo buffer.
> +  @retval       EFI_INVALID_PARAMETER The FvbMediaInfo parameter is NULL.
> +  @retval       EFI_UNSUPPORTED       An error occurred retrieving variable FV information.
> +  @retval       EFI_BAD_BUFFER_SIZE   An overflow or underflow of the FV buffer occurred with the information found.
> +
> +**/
> +EFI_STATUS
> +GenerateNvStorageFvbMediaInfo (
> +  OUT EFI_FVB2_MEDIA_INFO     *FvbMediaInfo
> +  )
> +{
> +  EFI_STATUS                  Status;
> +  UINT32                      NvBlockNum;
> +  UINT32                      TotalNvVariableStorageSize;
> +  EFI_PHYSICAL_ADDRESS        NvStorageBaseAddress;
> +  EFI_FIRMWARE_VOLUME_HEADER  FvbInfo = {
> +                                          {0,},                                   //ZeroVector[16]
> +                                          EFI_SYSTEM_NV_DATA_FV_GUID,             //FileSystemGuid
> +                                          0,                                      //FvLength
> +                                          EFI_FVH_SIGNATURE,                      //Signature
> +                                          0x0004feff,                             //Attributes
> +                                          sizeof (EFI_FIRMWARE_VOLUME_HEADER) +   //HeaderLength
> +                                            sizeof (EFI_FV_BLOCK_MAP_ENTRY),
> +                                          0,                                      //Checksum
> +                                          0,                                      //ExtHeaderOffset
> +                                          {0,},                                   //Reserved[1]
> +                                          2,                                      //Revision
> +                                          {                                       //BlockMap[1]
> +                                            {0,0}
> +                                          }
> +                                        };
> +
> +  if (FvbMediaInfo == NULL) {
> +    return EFI_INVALID_PARAMETER;
>     }
> +
> +  ZeroMem (FvbMediaInfo, sizeof (*FvbMediaInfo));
> +
> +  GetVariableFvInfo (&NvStorageBaseAddress, 
> + &TotalNvVariableStorageSize);  if ((NvStorageBaseAddress == 0) || (TotalNvVariableStorageSize == 0)) {
> +    return EFI_UNSUPPORTED;
> +  }
> +
> +  NvBlockNum = TotalNvVariableStorageSize / FVB_MEDIA_BLOCK_SIZE;
> +
> +  Status = SafeUint64Mult ((UINT64)NvBlockNum, FVB_MEDIA_BLOCK_SIZE, 
> + &FvbInfo.FvLength);  if (EFI_ERROR (Status)) {
> +    return EFI_BAD_BUFFER_SIZE;
> +  }
> +
> +  FvbInfo.BlockMap[0].NumBlocks = NvBlockNum; 
> + FvbInfo.BlockMap[0].Length = FVB_MEDIA_BLOCK_SIZE;
> +
> +  FvbMediaInfo->BaseAddress = NvStorageBaseAddress;  CopyMem 
> + (&FvbMediaInfo->FvbInfo, &FvbInfo, sizeof (FvbInfo));
> +
> +  return EFI_SUCCESS;
> +}
> +
> +FVB_MEDIA_INFO_GENERATOR mFvbMediaInfoGenerators[] = {
> +  GenerateNvStorageFvbMediaInfo
>   };
>   
>   EFI_STATUS
> @@ -64,12 +106,16 @@ GetFvbInfo (
>     OUT EFI_FIRMWARE_VOLUME_HEADER   **FvbInfo
>     )
>   {
> +  EFI_STATUS                  Status;
> +  EFI_FVB2_MEDIA_INFO         FvbMediaInfo;
>     UINTN                       Index;
>     EFI_FIRMWARE_VOLUME_HEADER  *FvHeader;
>   
> -  for (Index = 0; Index < sizeof (mPlatformFvbMediaInfo) / sizeof (EFI_FVB2_MEDIA_INFO); Index++) {
> -    if (mPlatformFvbMediaInfo[Index].BaseAddress == FvBaseAddress) {
> -      FvHeader = &mPlatformFvbMediaInfo[Index].FvbInfo;
> +  for (Index = 0; Index < ARRAY_SIZE (mFvbMediaInfoGenerators); Index++) {
> +    Status = mFvbMediaInfoGenerators[Index](&FvbMediaInfo);
> +    ASSERT_EFI_ERROR (Status);
> +    if (!EFI_ERROR (Status) && (FvbMediaInfo.BaseAddress == FvBaseAddress)) {
> +      FvHeader = AllocateCopyPool (sizeof 
> + (EFI_FIRMWARE_VOLUME_HEADER), &FvbMediaInfo.FvbInfo);
>   
>         //
>         // Update the checksum value of FV header.
> diff --git
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceCommon.c
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceCommon.c index dab818e98087..942abf95a64f 100644
> ---
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceCommon.c
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvb
> +++ ServiceCommon.c
> @@ -3,6 +3,7 @@
>     which are compliant with the Intel(R) Serial Flash Interface Compatibility Specification.
>   
>   Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) Microsoft Corporation.<BR>
>   SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>   **/
> @@ -25,12 +26,6 @@ FV_INFO mPlatformFvBaseAddress[] = {
>     {0, 0}
>   };
>   
> -FV_INFO mPlatformDefaultBaseAddress[] = {
> -  {0, 0}, // {FixedPcdGet32(PcdFlashNvStorageVariableBase),
> FixedPcdGet32(PcdFlashNvStorageVariableSize)},
> -  {0, 0}, // {FixedPcdGet32(PcdFlashMicrocodeFvBase),
> FixedPcdGet32(PcdFlashMicrocodeFvSize)},
> -  {0, 0}
> -};
> -
>   FV_MEMMAP_DEVICE_PATH mFvMemmapDevicePathTemplate = {
>     {
>       {
> @@ -530,6 +525,85 @@ FvbSetVolumeAttributes (
>     return EFI_SUCCESS;
>   }
>   
> +/**
> +  Get the total size of the firmware volume on flash used for variable store operations.
> +
> +  @param[out] BaseAddress         Base address of the variable store firmware volume.
> +  @param[out] Length              Length in bytes of the firmware volume used for variable store operations.
> +
> +**/
> +VOID
> +GetVariableFvInfo (
> +  OUT EFI_PHYSICAL_ADDRESS      *BaseAddress,
> +  OUT UINT32                    *Length
> +  )
> +{
> +  EFI_STATUS                    Status;
> +  EFI_PHYSICAL_ADDRESS          NvBaseAddress;
> +  EFI_PHYSICAL_ADDRESS          NvVariableBaseAddress;
> +  UINT64                        Length64;
> +  UINT32                        NvStoreLength;
> +  UINT32                        FtwSpareLength;
> +  UINT32                        FtwWorkingLength;
> +  UINT32                        TotalLength;
> +
> +  TotalLength = 0;
> +  Status = EFI_SUCCESS;
> +
> +  if ((BaseAddress == NULL) || (Length == NULL)) {
> +    ASSERT ((BaseAddress != NULL) && (Length != NULL));
> +    return;
> +  }
> +  *BaseAddress = 0;
> +  *Length = 0;
> +
> +  Status = GetVariableFlashNvStorageInfo (&NvBaseAddress, &Length64); 
> + if (!EFI_ERROR (Status)) {
> +    NvVariableBaseAddress = NvBaseAddress;
> +    // Stay within the current UINT32 size assumptions in the variable stack.
> +    Status = SafeUint64ToUint32 (Length64, &NvStoreLength);  }  if 
> + (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return;
> +  }
> +
> +  Status = GetVariableFlashFtwSpareInfo (&NvBaseAddress, &Length64); 
> + if (!EFI_ERROR (Status)) {
> +    // Stay within the current UINT32 size assumptions in the variable stack.
> +    Status = SafeUint64ToUint32 (Length64, &FtwSpareLength);  }  if 
> + (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return;
> +  }
> +
> +  Status = GetVariableFlashFtwWorkingInfo (&NvBaseAddress, 
> + &Length64);  if (!EFI_ERROR (Status)) {
> +    // Stay within the current UINT32 size assumptions in the variable stack.
> +    Status = SafeUint64ToUint32 (Length64, &FtwWorkingLength);  }  if 
> + (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return;
> +  }
> +
> +  Status = SafeUint32Add (NvStoreLength, FtwSpareLength, 
> + &TotalLength);  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return;
> +  }
> +
> +  Status = SafeUint32Add (TotalLength, FtwWorkingLength, 
> + &TotalLength);  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    return;
> +  }
> +
> +  *BaseAddress = NvVariableBaseAddress;
> +  *Length = TotalLength;
> +}
> +
>   /**
>     Check the integrity of firmware volume header
>   
> @@ -545,7 +619,12 @@ IsFvHeaderValid (
>     IN CONST EFI_FIRMWARE_VOLUME_HEADER    *FvHeader
>     )
>   {
> -  if (FvBase == PcdGet32(PcdFlashNvStorageVariableBase)) {
> +  EFI_PHYSICAL_ADDRESS    NvStorageFvBaseAddress;
> +  UINT32                  NvStorageSize;
> +
> +  GetVariableFvInfo (&NvStorageFvBaseAddress, &NvStorageSize);
> +
> +  if (FvBase == NvStorageFvBaseAddress) {
>       if (CompareMem (&FvHeader->FileSystemGuid, &gEfiSystemNvDataFvGuid, sizeof(EFI_GUID)) != 0 ) {
>         return FALSE;
>       }
> diff --git
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceMm.c
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceMm.c index 42a0828c6fae..6b4bcdcfe3c4 100644
> ---
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceMm.c
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvb
> +++ ServiceMm.c
> @@ -113,15 +113,31 @@ FvbInitialize (
>     UINT32                                MaxLbaSize;
>     UINT32                                BytesWritten;
>     UINTN                                 BytesErased;
> +  UINT64                                NvStorageFvSize;
> +
> +  Status = GetVariableFlashNvStorageInfo (&BaseAddress, 
> + &NvStorageFvSize);  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    DEBUG ((DEBUG_ERROR, "[%a] - An error ocurred getting variable info - %r.\n", __FUNCTION__, Status));
> +    return;
> +  }
> +
> +  // Stay within the current UINT32 size assumptions in the variable stack.
> +  Status = SafeUint64ToUint32 (BaseAddress, 
> + &mPlatformFvBaseAddress[0].FvBase);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    DEBUG ((DEBUG_ERROR, "[%a] - 64-bit variable storage base address not supported.\n", __FUNCTION__));
> +    return;
> +  }
> +  Status = SafeUint64ToUint32 (NvStorageFvSize, 
> + &mPlatformFvBaseAddress[0].FvSize);
> +  if (EFI_ERROR (Status)) {
> +    ASSERT_EFI_ERROR (Status);
> +    DEBUG ((DEBUG_ERROR, "[%a] - 64-bit variable storage size not supported.\n", __FUNCTION__));
> +    return;
> +  }
>   
> -  mPlatformFvBaseAddress[0].FvBase = 
> PcdGet32(PcdFlashNvStorageVariableBase);
> -  mPlatformFvBaseAddress[0].FvSize = PcdGet32(PcdFlashNvStorageVariableSize);
>     mPlatformFvBaseAddress[1].FvBase = PcdGet32(PcdFlashMicrocodeFvBase);
>     mPlatformFvBaseAddress[1].FvSize = 
> PcdGet32(PcdFlashMicrocodeFvSize);
> -  mPlatformDefaultBaseAddress[0].FvBase = 
> PcdGet32(PcdFlashNvStorageVariableBase);
> -  mPlatformDefaultBaseAddress[0].FvSize = 
> PcdGet32(PcdFlashNvStorageVariableSize);
> -  mPlatformDefaultBaseAddress[1].FvBase = 
> PcdGet32(PcdFlashMicrocodeFvBase);
> -  mPlatformDefaultBaseAddress[1].FvSize = 
> PcdGet32(PcdFlashMicrocodeFvSize);
>   
>     //
>     // We will only continue with FVB installation if the @@ -143,7
> +159,8 @@ FvbInitialize (
>           BytesWritten = 0;
>           BytesErased = 0;
>           DEBUG ((DEBUG_ERROR, "ERROR - The FV in 0x%x is invalid!\n", FvHeader));
> -        Status = GetFvbInfo (BaseAddress, &FvHeader);
> +        FvHeader = NULL;
> +        Status   = GetFvbInfo (BaseAddress, &FvHeader);
>           if (EFI_ERROR (Status)) {
>             DEBUG ((DEBUG_WARN, "ERROR - Can't recovery FV header at 0x%x.  GetFvbInfo Status %r\n", BaseAddress, Status));
>             continue;
> @@ -156,27 +173,42 @@ FvbInitialize (
>           Status = SpiFlashBlockErase( (UINTN) BaseAddress, &BytesErased);
>           if (EFI_ERROR (Status)) {
>             DEBUG ((DEBUG_WARN, "ERROR - SpiFlashBlockErase Error 
> %r\n", Status));
> +          if (FvHeader != NULL) {
> +            FreePool (FvHeader);
> +          }
>             continue;
>           }
>           if (BytesErased != FvHeader->BlockMap->Length) {
>             DEBUG ((DEBUG_WARN, "ERROR - BytesErased != FvHeader->BlockMap->Length\n"));
>             DEBUG ((DEBUG_INFO, " BytesErased = 0x%X\n Length = 
> 0x%X\n", BytesErased, FvHeader->BlockMap->Length));
> +          if (FvHeader != NULL) {
> +            FreePool (FvHeader);
> +          }
>             continue;
>           }
>           BytesWritten = FvHeader->HeaderLength;
>           Status = SpiFlashWrite ((UINTN)BaseAddress, &BytesWritten, (UINT8*)FvHeader);
>           if (EFI_ERROR (Status)) {
>             DEBUG ((DEBUG_WARN, "ERROR - SpiFlashWrite Error  %r\n", 
> Status));
> +          if (FvHeader != NULL) {
> +            FreePool (FvHeader);
> +          }
>             continue;
>           }
>           if (BytesWritten != FvHeader->HeaderLength) {
>             DEBUG ((DEBUG_WARN, "ERROR - BytesWritten != HeaderLength\n"));
>             DEBUG ((DEBUG_INFO, " BytesWritten = 0x%X\n HeaderLength = 
> 0x%X\n", BytesWritten, FvHeader->HeaderLength));
> +          if (FvHeader != NULL) {
> +            FreePool (FvHeader);
> +          }
>             continue;
>           }
>           Status = SpiFlashLock ();
>           if (EFI_ERROR (Status)) {
>             DEBUG ((DEBUG_WARN, "ERROR - SpiFlashLock Error  %r\n", 
> Status));
> +          if (FvHeader != NULL) {
> +            FreePool (FvHeader);
> +          }
>             continue;
>           }
>           DEBUG ((DEBUG_INFO, "FV Header @ 0x%X restored with static 
> data\n", BaseAddress)); @@ -184,6 +216,9 @@ FvbInitialize (
>           // Clear cache for this range.
>           //
>           WriteBackInvalidateDataCacheRange ( (VOID *) (UINTN) 
> BaseAddress, FvHeader->BlockMap->Length);
> +        if (FvHeader != NULL) {
> +          FreePool (FvHeader);
> +        }
>         }
>       }
>   
> diff --git
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceCommon.h
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceCommon.h index e9d69e985814..7b0908b03fdc 100644
> ---
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceCommon.h
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvb
> +++ ServiceCommon.h
> @@ -2,6 +2,7 @@
>     Common source definitions used in serial flash drivers
>   
>   Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) Microsoft Corporation.<BR>
>   SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>   **/
> @@ -12,6 +13,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>   #include <Guid/EventGroup.h>
>   #include <Guid/FirmwareFileSystem2.h>
>   #include <Guid/SystemNvDataGuid.h>
> +#include <Pi/PiFirmwareVolume.h>
>   #include <Protocol/DevicePath.h>
>   #include <Protocol/FirmwareVolumeBlock.h>
>   
> @@ -24,6 +26,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>   #include <Library/PcdLib.h>
>   #include <Library/DevicePathLib.h>
>   #include <Library/HobLib.h>
> +#include <Library/VariableFlashInfoLib.h> #include 
> +<Library/SafeIntLib.h>
>   
>   #include <Library/SpiFlashCommonLib.h>
>   
> @@ -148,11 +152,23 @@ GetFvbInfo (
>     OUT EFI_FIRMWARE_VOLUME_HEADER   **FvbInfo
>     );
>   
> +/**
> +  Get the total size of the firmware volume on flash used for variable store operations.
> +
> +  @param[out] BaseAddress         Base address of the variable store firmware volume.
> +  @param[out] Length              Length in bytes of the firmware volume used for variable store operations.
> +
> +**/
> +VOID
> +GetVariableFvInfo (
> +  OUT EFI_PHYSICAL_ADDRESS      *BaseAddress,
> +  OUT UINT32                    *Length
> +  );
> +
>   extern FVB_GLOBAL                         mFvbModuleGlobal;
>   extern FV_MEMMAP_DEVICE_PATH              mFvMemmapDevicePathTemplate;
>   extern FV_PIWG_DEVICE_PATH                mFvPIWGDevicePathTemplate;
>   extern EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL mFvbProtocolTemplate;
>   extern FV_INFO                            mPlatformFvBaseAddress[];
> -extern FV_INFO                            mPlatformDefaultBaseAddress[];
>   
>   #endif
> diff --git
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceSmm.inf
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceSmm.inf index bf1306f00201..0cfa3f909bdf 100644
> ---
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceSmm.inf
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvb
> +++ ServiceSmm.inf
> @@ -32,8 +32,10 @@ [LibraryClasses]
>     BaseLib
>     UefiBootServicesTableLib
>     UefiDriverEntryPoint
> +  SafeIntLib
>     SpiFlashCommonLib
>     MmServicesTableLib
> +  VariableFlashInfoLib
>   
>   [Packages]
>     MdePkg/MdePkg.dec
> @@ -41,10 +43,6 @@ [Packages]
>     IntelSiliconPkg/IntelSiliconPkg.dec
>   
>   [Pcd]
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase   ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize   ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize   ## CONSUMES
>     gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvBase         ## CONSUMES
>     gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvSize         ## CONSUMES
>   
> diff --git
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceStandaloneMm.inf
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceStandaloneMm.inf index b66233968247..152cf0036fdb 100644
> ---
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> iceStandaloneMm.inf
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvb
> +++ ServiceStandaloneMm.inf
> @@ -31,8 +31,10 @@ [LibraryClasses]
>     MemoryAllocationLib
>     PcdLib
>     MmServicesTableLib
> +  SafeIntLib
>     SpiFlashCommonLib
>     StandaloneMmDriverEntryPoint
> +  VariableFlashInfoLib
>   
>   [Packages]
>     MdePkg/MdePkg.dec
> @@ -40,10 +42,6 @@ [Packages]
>     IntelSiliconPkg/IntelSiliconPkg.dec
>   
>   [Pcd]
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase   ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize   ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize ## CONSUMES
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize   ## CONSUMES
>     gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvBase         ## CONSUMES
>     gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvSize         ## CONSUMES
>   
> diff --git a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
> b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
> index 1e826a080f28..170eb480ada1 100644
> --- a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
> +++ b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dsc
> @@ -40,9 +40,11 @@ [LibraryClasses]
>     PeiGetVtdPmrAlignmentLib|IntelSiliconPkg/Library/PeiGetVtdPmrAlignmentLib/PeiGetVtdPmrAlignmentLib.inf
>     TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
>     MicrocodeLib|UefiCpuPkg/Library/MicrocodeLib/MicrocodeLib.inf
> +  SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
>     SpiFlashCommonLib|IntelSiliconPkg/Library/SpiFlashCommonLibNull/SpiFlashCommonLibNull.inf
>     
> UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiB
> ootServicesTableLib.inf
>     
> UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEnt
> ryPoint.inf
> +  
> + VariableFlashInfoLib|MdeModulePkg/Library/BaseVariableFlashInfoLib/B
> + aseVariableFlashInfoLib.inf
>   
>   [LibraryClasses.common.PEIM]
>     PeimEntryPoint|MdePkg/Library/PeimEntryPoint/PeimEntryPoint.inf
> @@ -63,11 +65,16 @@ [LibraryClasses.common.DXE_DRIVER]
>     
> MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryA
> llocationLib.inf
>   
>   [LibraryClasses.common.DXE_SMM_DRIVER]
> +  
> + DevicePathLib|MdePkg/Library/UefiDevicePathLib/UefiDevicePathLib.inf
> +  HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf
>     MemoryAllocationLib|MdePkg/Library/SmmMemoryAllocationLib/SmmMemoryAllocationLib.inf
>     
> MmServicesTableLib|MdePkg/Library/MmServicesTableLib/MmServicesTableLi
> b.inf
>     
> SmmServicesTableLib|MdePkg/Library/SmmServicesTableLib/SmmServicesTabl
> eLib.inf
> +  UefiLib|MdePkg/Library/UefiLib/UefiLib.inf
> +  
> + UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableL
> + ib/UefiRuntimeServicesTableLib.inf
>   
>   [LibraryClasses.common.MM_STANDALONE]
> +  
> + HobLib|StandaloneMmPkg/Library/StandaloneMmHobLib/StandaloneMmHobLib
> + .inf
>     MemoryAllocationLib|StandaloneMmPkg/Library/StandaloneMmMemoryAllocationLib/StandaloneMmMemoryAllocationLib.inf
>     
> MmServicesTableLib|MdePkg/Library/StandaloneMmServicesTableLib/Standal
> oneMmServicesTableLib.inf
>     
> StandaloneMmDriverEntryPoint|MdePkg/Library/StandaloneMmDriverEntryPoi
> nt/StandaloneMmDriverEntryPoint.inf

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2022-07-13 20:59 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
     [not found] <16FDBB890A0E8B8C.7323@groups.io>
2022-07-12 23:04 ` [edk2-devel] [edk2-platforms][PATCH v2 1/1] IntelSiliconPkg/SpiFvbService: Add support for VariableFlashInfoLib Michael Kubacki
2022-07-13 15:51   ` Oram, Isaac W
2022-07-13 16:49   ` Chaganty, Rangasai V
2022-07-13 20:59     ` Oram, Isaac W

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox