Hello,

I have sent a V3 patch to remove Uncrustify coding style changes. We will do coding style changes separately later.
Please help to review V3 patch.

Thanks,
Chasel


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chiu, Chasel
> Sent: Monday, February 6, 2023 10:50 AM
> To: devel@edk2.groups.io
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; S, Ashraf Ali <ashraf.ali.s@intel.com>;
> Oram, Isaac W <isaac.w.oram@intel.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty@intel.com>; Ni, Ray <ray.ni@intel.com>; Kubacki,
> Michael <michael.kubacki@microsoft.com>
> Subject: [edk2-devel] [edk2-platforms: PATCH v2]
> IntelSiliconPkg/SpiFvbServiceSmm: Rewrite VariableStore header.
> 
> When invalid VariableStore FV header detected, current SpiFvbService will erase
> both FV and VariableStore headers from flash, however, it will only rewrite FV
> header back and cause invalid VariableStore header.
> 
> This patch adding the support for rewriting both FV header and VariableStore
> header when VariableStore corruption happened.
> 
> Platform has to set PcdFlashVariableStoreType to inform SpiFvbService which
> VariableStoreType should be rewritten.
> 
> Cc: Ashraf Ali S <ashraf.ali.s@intel.com>
> Cc: Isaac Oram <isaac.w.oram@intel.com>
> Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Michael Kubacki <michael.kubacki@microsoft.com>
> Signed-off-by: Chasel Chiu <chasel.chiu@intel.com>
> ---
>  Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c    |
> 174
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------
> ----------------------------------------
>  Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.inf
> |   4 ++++
>  Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec                              |   8 ++++++++
>  3 files changed, 134 insertions(+), 52 deletions(-)
> 
> diff --git
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c
> index 6b4bcdcfe3..6af2dfac10 100644
> ---
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbSe
> +++ rviceMm.c
> @@ -12,6 +12,7 @@
>  #include <Library/MmServicesTableLib.h> #include
> <Library/UefiDriverEntryPoint.h> #include
> <Protocol/SmmFirmwareVolumeBlock.h>+#include <Guid/VariableFormat.h>
> /**   The function installs EFI_FIRMWARE_VOLUME_BLOCK protocol@@ -25,12
> +26,12 @@
>  **/ VOID InstallFvbProtocol (-  IN  EFI_FVB_INSTANCE               *FvbInstance+  IN
> EFI_FVB_INSTANCE  *FvbInstance   ) {-  EFI_FIRMWARE_VOLUME_HEADER
> *FvHeader;-  EFI_STATUS                            Status;-  EFI_HANDLE
> FvbHandle;+  EFI_FIRMWARE_VOLUME_HEADER  *FvHeader;+  EFI_STATUS
> Status;+  EFI_HANDLE                  FvbHandle;    ASSERT (FvbInstance != NULL);   if
> (FvbInstance == NULL) {@@ -52,19 +53,21 @@ InstallFvbProtocol (
>      //     // FV does not contains extension header, then produce
> MEMMAP_DEVICE_PATH     //-    FvbInstance->DevicePath =
> (EFI_DEVICE_PATH_PROTOCOL *) AllocateRuntimeCopyPool (sizeof
> (FV_MEMMAP_DEVICE_PATH), &mFvMemmapDevicePathTemplate);+
> FvbInstance->DevicePath = (EFI_DEVICE_PATH_PROTOCOL
> *)AllocateRuntimeCopyPool (sizeof (FV_MEMMAP_DEVICE_PATH),
> &mFvMemmapDevicePathTemplate);     if (FvbInstance->DevicePath == NULL)
> {       DEBUG ((DEBUG_INFO, "SpiFvbServiceSmm.c: Memory allocation for
> MEMMAP_DEVICE_PATH failed\n"));       return;     }-
> ((FV_MEMMAP_DEVICE_PATH *) FvbInstance->DevicePath)-
> >MemMapDevPath.StartingAddress = FvbInstance->FvBase;-
> ((FV_MEMMAP_DEVICE_PATH *) FvbInstance->DevicePath)-
> >MemMapDevPath.EndingAddress   = FvbInstance->FvBase + FvHeader-
> >FvLength - 1;++    ((FV_MEMMAP_DEVICE_PATH *)FvbInstance->DevicePath)-
> >MemMapDevPath.StartingAddress = FvbInstance->FvBase;+
> ((FV_MEMMAP_DEVICE_PATH *)FvbInstance->DevicePath)-
> >MemMapDevPath.EndingAddress   = FvbInstance->FvBase + FvHeader-
> >FvLength - 1;   } else {-    FvbInstance->DevicePath =
> (EFI_DEVICE_PATH_PROTOCOL *) AllocateRuntimeCopyPool (sizeof
> (FV_PIWG_DEVICE_PATH), &mFvPIWGDevicePathTemplate);+    FvbInstance-
> >DevicePath = (EFI_DEVICE_PATH_PROTOCOL *)AllocateRuntimeCopyPool
> (sizeof (FV_PIWG_DEVICE_PATH), &mFvPIWGDevicePathTemplate);     if
> (FvbInstance->DevicePath == NULL) {       DEBUG ((DEBUG_INFO,
> "SpiFvbServiceSmm.c: Memory allocation for FV_PIWG_DEVICE_PATH
> failed\n"));       return;     }+     CopyGuid (       &((FV_PIWG_DEVICE_PATH
> *)FvbInstance->DevicePath)->FvDevPath.FvName,       (GUID
> *)(UINTN)(FvbInstance->FvBase + FvHeader->ExtHeaderOffset)@@ -103,17
> +106,21 @@ FvbInitialize (
>    VOID   ) {-  EFI_FVB_INSTANCE                      *FvbInstance;-
> EFI_FIRMWARE_VOLUME_HEADER            *FvHeader;-
> EFI_FV_BLOCK_MAP_ENTRY                *PtrBlockMapEntry;-
> EFI_PHYSICAL_ADDRESS                  BaseAddress;-  EFI_STATUS
> Status;-  UINTN                                 BufferSize;-  UINTN                                 Idx;-
> UINT32                                MaxLbaSize;-  UINT32                                BytesWritten;-
> UINTN                                 BytesErased;-  UINT64
> NvStorageFvSize;+  EFI_FVB_INSTANCE            *FvbInstance;+
> EFI_FIRMWARE_VOLUME_HEADER  *FvHeader;+  EFI_FV_BLOCK_MAP_ENTRY
> *PtrBlockMapEntry;+  EFI_PHYSICAL_ADDRESS        BaseAddress;+  EFI_STATUS
> Status;+  UINTN                       BufferSize;+  UINTN                       Idx;+  UINT32
> MaxLbaSize;+  UINT32                      BytesWritten;+  UINTN
> BytesErased;+  UINT64                      NvStorageFvSize;+  UINT32
> ExpectedBytesWritten;+  VARIABLE_STORE_HEADER       *VariableStoreHeader;+
> UINT8                       VariableStoreType;+  UINT8                       *NvStoreBuffer;
> Status = GetVariableFlashNvStorageInfo (&BaseAddress, &NvStorageFvSize);   if
> (EFI_ERROR (Status)) {@@ -129,6 +136,7 @@ FvbInitialize (
>      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);@@ -136,8 +144,8 @@ FvbInitialize (
>      return;   } -  mPlatformFvBaseAddress[1].FvBase =
> PcdGet32(PcdFlashMicrocodeFvBase);-  mPlatformFvBaseAddress[1].FvSize =
> PcdGet32(PcdFlashMicrocodeFvSize);+  mPlatformFvBaseAddress[1].FvBase =
> PcdGet32 (PcdFlashMicrocodeFvBase);+  mPlatformFvBaseAddress[1].FvSize =
> PcdGet32 (PcdFlashMicrocodeFvSize);    //   // We will only continue with FVB
> installation if the@@ -147,17 +155,17 @@ FvbInitialize (
>      //     // Make sure all FVB are valid and/or fix if possible     //-    for (Idx = 0;;
> Idx++) {-      if (mPlatformFvBaseAddress[Idx].FvSize == 0 &&
> mPlatformFvBaseAddress[Idx].FvBase == 0) {+    for (Idx = 0; ; Idx++) {+      if
> ((mPlatformFvBaseAddress[Idx].FvSize == 0) &&
> (mPlatformFvBaseAddress[Idx].FvBase == 0)) {         break;       }        BaseAddress
> = mPlatformFvBaseAddress[Idx].FvBase;-      FvHeader =
> (EFI_FIRMWARE_VOLUME_HEADER *) (UINTN) BaseAddress;+      FvHeader    =
> (EFI_FIRMWARE_VOLUME_HEADER *)(UINTN)BaseAddress;        if
> (!IsFvHeaderValid (BaseAddress, FvHeader)) {         BytesWritten = 0;-
> BytesErased = 0;+        BytesErased  = 0;         DEBUG ((DEBUG_ERROR, "ERROR -
> The FV in 0x%x is invalid!\n", FvHeader));         FvHeader = NULL;         Status   =
> GetFvbInfo (BaseAddress, &FvHeader);@@ -165,57 +173,116 @@ FvbInitialize (
>            DEBUG ((DEBUG_WARN, "ERROR - Can't recovery FV header at 0x%x.
> GetFvbInfo Status %r\n", BaseAddress, Status));           continue;         }+
> DEBUG ((DEBUG_INFO, "Rewriting FV header at 0x%X with static data\n",
> BaseAddress));         //         // Spi erase         //-        BytesErased = (UINTN)
> FvHeader->BlockMap->Length;-        Status = SpiFlashBlockErase( (UINTN)
> BaseAddress, &BytesErased);+        BytesErased = (UINTN)FvHeader->BlockMap-
> >Length;+        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);++        BytesWritten         = FvHeader-
> >HeaderLength;+        ExpectedBytesWritten = BytesWritten;+        if (Idx != 0) {+
> Status = SpiFlashWrite ((UINTN)BaseAddress, &BytesWritten, (UINT8
> *)FvHeader);+        } else {+          //+          // This is Variable Store, rewrite both
> EFI_FIRMWARE_VOLUME_HEADER and VARIABLE_STORE_HEADER+          //+
> NvStoreBuffer = NULL;+          NvStoreBuffer = AllocateZeroPool (sizeof
> (VARIABLE_STORE_HEADER) + FvHeader->HeaderLength);+          if
> (NvStoreBuffer != NULL) {+            //+            // Combine FV header and
> VariableStore header into the buffer.+            //+            CopyMem (NvStoreBuffer,
> FvHeader, FvHeader->HeaderLength);+            VariableStoreHeader =
> (VARIABLE_STORE_HEADER *)(NvStoreBuffer + FvHeader->HeaderLength);+
> VariableStoreType   = PcdGet8 (PcdFlashVariableStoreType);+            switch
> (VariableStoreType) {+              case 0:+                DEBUG ((DEBUG_ERROR, "Type:
> gEfiVariableGuid\n"));+                CopyGuid (&VariableStoreHeader->Signature,
> &gEfiVariableGuid);+                break;+              case 1:+                DEBUG
> ((DEBUG_ERROR, "Type: gEfiAuthenticatedVariableGuid\n"));+                CopyGuid
> (&VariableStoreHeader->Signature, &gEfiAuthenticatedVariableGuid);+
> break;+              default:+                break;+            }++            //+            // Initialize
> common VariableStore header fields+            //+            VariableStoreHeader-
> >Size      = PcdGet32 (PcdFlashNvStorageVariableSize) - FvHeader-
> >HeaderLength;+            VariableStoreHeader->Format    =
> VARIABLE_STORE_FORMATTED;+            VariableStoreHeader->State     =
> VARIABLE_STORE_HEALTHY;+            VariableStoreHeader->Reserved  = 0;+
> VariableStoreHeader->Reserved1 = 0;+            //+            // Write buffer to flash+
> //+            BytesWritten         = FvHeader->HeaderLength + sizeof
> (VARIABLE_STORE_HEADER);+            ExpectedBytesWritten = BytesWritten;+
> Status               = SpiFlashWrite ((UINTN)BaseAddress, &BytesWritten,
> NvStoreBuffer);+            FreePool (NvStoreBuffer);+          } else {+            Status =
> EFI_OUT_OF_RESOURCES;+          }+        }+         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 (BytesWritten != ExpectedBytesWritten) {+
> DEBUG ((DEBUG_WARN, "ERROR - BytesWritten != ExpectedBytesWritten\n"));+
> DEBUG ((DEBUG_INFO, " BytesWritten = 0x%X\n ExpectedBytesWritten =
> 0x%X\n", BytesWritten, ExpectedBytesWritten));           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));
> //         // Clear cache for this range.         //-
> WriteBackInvalidateDataCacheRange ( (VOID *) (UINTN) BaseAddress,
> FvHeader->BlockMap->Length);+        WriteBackInvalidateDataCacheRange
> ((VOID *)(UINTN)BaseAddress, FvHeader->BlockMap->Length);         if
> (FvHeader != NULL) {           FreePool (FvHeader);         }@@ -227,11 +294,12 @@
> FvbInitialize (
>      //     BufferSize = 0;     for (Idx = 0; ; Idx++) {-      if
> (mPlatformFvBaseAddress[Idx].FvSize == 0 &&
> mPlatformFvBaseAddress[Idx].FvBase == 0) {+      if
> ((mPlatformFvBaseAddress[Idx].FvSize == 0) &&
> (mPlatformFvBaseAddress[Idx].FvBase == 0)) {         break;       }+       BaseAddress
> = mPlatformFvBaseAddress[Idx].FvBase;-      FvHeader =
> (EFI_FIRMWARE_VOLUME_HEADER *) (UINTN) BaseAddress;+      FvHeader    =
> (EFI_FIRMWARE_VOLUME_HEADER *)(UINTN)BaseAddress;        if
> (!IsFvHeaderValid (BaseAddress, FvHeader)) {         DEBUG ((DEBUG_WARN,
> "ERROR - The FV in 0x%x is invalid!\n", FvHeader));@@ -239,27 +307,28 @@
> FvbInitialize (
>        }        BufferSize += (FvHeader->HeaderLength +-                    sizeof
> (EFI_FVB_INSTANCE) --                    sizeof (EFI_FIRMWARE_VOLUME_HEADER)-
>                     );+                     sizeof (EFI_FVB_INSTANCE) -+                     sizeof
> (EFI_FIRMWARE_VOLUME_HEADER)+                     );     } -
> mFvbModuleGlobal.FvbInstance =  (EFI_FVB_INSTANCE *)
> AllocateRuntimeZeroPool (BufferSize);+    mFvbModuleGlobal.FvbInstance =
> (EFI_FVB_INSTANCE *)AllocateRuntimeZeroPool (BufferSize);     if
> (mFvbModuleGlobal.FvbInstance == NULL) {       ASSERT (FALSE);       return;     } -
> MaxLbaSize      = 0;-    FvbInstance     = mFvbModuleGlobal.FvbInstance;-
> mFvbModuleGlobal.NumFv   = 0;+    MaxLbaSize             = 0;+    FvbInstance
> = mFvbModuleGlobal.FvbInstance;+    mFvbModuleGlobal.NumFv = 0;      for (Idx
> = 0; ; Idx++) {-      if (mPlatformFvBaseAddress[Idx].FvSize == 0 &&
> mPlatformFvBaseAddress[Idx].FvBase == 0) {+      if
> ((mPlatformFvBaseAddress[Idx].FvSize == 0) &&
> (mPlatformFvBaseAddress[Idx].FvBase == 0)) {         break;       }+       BaseAddress
> = mPlatformFvBaseAddress[Idx].FvBase;-      FvHeader =
> (EFI_FIRMWARE_VOLUME_HEADER *) (UINTN) BaseAddress;+      FvHeader    =
> (EFI_FIRMWARE_VOLUME_HEADER *)(UINTN)BaseAddress;        if
> (!IsFvHeaderValid (BaseAddress, FvHeader)) {         DEBUG ((DEBUG_WARN,
> "ERROR - The FV in 0x%x is invalid!\n", FvHeader));@@ -269,22 +338,24 @@
> FvbInitialize (
>        FvbInstance->Signature = FVB_INSTANCE_SIGNATURE;       CopyMem
> (&(FvbInstance->FvHeader), FvHeader, FvHeader->HeaderLength); -      FvHeader
> = &(FvbInstance->FvHeader);+      FvHeader            = &(FvbInstance->FvHeader);
> FvbInstance->FvBase = (UINTN)BaseAddress;        //       // Process the block map
> for each FV       //-      FvbInstance->NumOfBlocks   = 0;+      FvbInstance-
> >NumOfBlocks = 0;       for (PtrBlockMapEntry = FvHeader->BlockMap;
> PtrBlockMapEntry->NumBlocks != 0;-           PtrBlockMapEntry++) {+
> PtrBlockMapEntry++)+      {         //         // Get the maximum size of a block.
> //         if (MaxLbaSize < PtrBlockMapEntry->Length) {-          MaxLbaSize  =
> PtrBlockMapEntry->Length;+          MaxLbaSize = PtrBlockMapEntry-
> >Length;         }+         FvbInstance->NumOfBlocks += PtrBlockMapEntry-
> >NumBlocks;       } @@ -297,10 +368,9 @@ FvbInitialize (
>        //       // Move on to the next FvbInstance       //-      FvbInstance =
> (EFI_FVB_INSTANCE *) ((UINTN)((UINT8 *)FvbInstance) +-
> FvHeader->HeaderLength +-                                            (sizeof (EFI_FVB_INSTANCE)
> - sizeof (EFI_FIRMWARE_VOLUME_HEADER)));-+      FvbInstance =
> (EFI_FVB_INSTANCE *)((UINTN)((UINT8 *)FvbInstance) ++
> FvHeader->HeaderLength ++                                         (sizeof (EFI_FVB_INSTANCE) -
> sizeof (EFI_FIRMWARE_VOLUME_HEADER)));     }   } }diff --git
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.in
> f
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.in
> f
> index 0cfa3f909b..0485b73679 100644
> ---
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.in
> f
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbSe
> +++ rviceSmm.inf
> @@ -45,6 +45,8 @@
>  [Pcd]   gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvBase         ##
> CONSUMES   gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvSize         ##
> CONSUMES+
> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize   ##
> SOMETIMES_CONSUMES+
> gIntelSiliconPkgTokenSpaceGuid.PcdFlashVariableStoreType       ##
> SOMETIMES_CONSUMES  [Sources]   FvbInfo.c@@ -61,6 +63,8 @@
>  [Guids]   gEfiFirmwareFileSystem2Guid                   ## CONSUMES
> gEfiSystemNvDataFvGuid                        ## CONSUMES+  gEfiVariableGuid
> ## SOMETIMES_CONSUMES+  gEfiAuthenticatedVariableGuid                 ##
> SOMETIMES_CONSUMES  [Depex]   TRUEdiff --git
> a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
> b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
> index 485cb3e80a..63dae756ad 100644
> --- a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
> +++ b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec
> @@ -186,3 +186,11 @@
>    # @Prompt VTd abort DMA mode support.
> gIntelSiliconPkgTokenSpaceGuid.PcdVTdSupportAbortDmaMode|FALSE|BOOLE
> AN|0x0000000C +  ## Define Flash Variable Store type.<BR><BR>+  #  When
> Flash Variable Store corruption happened, the SpiFvbService will recreate
> Variable Store+  #  with valid header information provided by this PCD
> value.<BR>+  #  0: Variable Store is gEfiVariableGuid type.<BR>+  #  1: Variable
> Store is gEfiAuthenticatedVariableGuid type.<BR>+  #  Other value: reserved for
> future use.<BR>+  # @Prompt Flash Variable Store type.+
> gIntelSiliconPkgTokenSpaceGuid.PcdFlashVariableStoreType|0x00|UINT8|0x000
> 0000E--
> 2.35.0.windows.1
> 
> 
> 
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#99697): https://edk2.groups.io/g/devel/message/99697
> Mute This Topic: https://groups.io/mt/96790455/1777047
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [chasel.chiu@intel.com] -=-
> =-=-=-=-=
>