Re: [edk2-devel] [edk2-platforms: PATCH v4] IntelSiliconPkg/SpiFvbServiceSmm: Rewrite VariableStore header.

["Chiu, Chasel" <chasel.chiu@intel.com>]

Bug fix patch has been merged: https://github.com/tianocore/edk2-platforms/commit/e95c7988994c73918ffa282e2d2f5af11f8addc4

Thanks,
Chasel


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chiu, Chasel
> Sent: Wednesday, February 8, 2023 2:17 PM
> 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 v4]
> 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.
> The Corrupted variable content should be taken care by FaultTolerantWrite
> driver later.
> 
> 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    |
> 69
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> ---
>  Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.inf
> |  3 +++
>  Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec                              |  8 ++++++++
>  3 files changed, 75 insertions(+), 5 deletions(-)
> 
> diff --git
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c
> index 6b4bcdcfe3..052be97872 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@@ -113,7
> +114,12 @@ FvbInitialize (
>    UINT32                                MaxLbaSize;   UINT32                                BytesWritten;
> UINTN                                 BytesErased;+  EFI_PHYSICAL_ADDRESS
> NvStorageBaseAddress;   UINT64                                NvStorageFvSize;+  UINT32
> ExpectedBytesWritten;+  VARIABLE_STORE_HEADER
> *VariableStoreHeader;+  UINT8                                 VariableStoreType;+  UINT8
> *NvStoreBuffer;    Status = GetVariableFlashNvStorageInfo (&BaseAddress,
> &NvStorageFvSize);   if (EFI_ERROR (Status)) {@@ -124,12 +130,14 @@
> FvbInitialize (
>     // Stay within the current UINT32 size assumptions in the variable stack.
> Status = SafeUint64ToUint32 (BaseAddress,
> &mPlatformFvBaseAddress[0].FvBase);+  NvStorageBaseAddress =
> 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);+  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__));@@ -186,8 +194,59
> @@ FvbInitialize (
>            }           continue;         }-        BytesWritten = FvHeader->HeaderLength;-
> Status = SpiFlashWrite ((UINTN)BaseAddress, &BytesWritten,
> (UINT8*)FvHeader);++        BytesWritten         = FvHeader->HeaderLength;+
> ExpectedBytesWritten = BytesWritten;+        if (BaseAddress !=
> NvStorageBaseAddress) {+          Status = SpiFlashWrite ((UINTN)BaseAddress,
> &BytesWritten, (UINT8 *)FvHeader);+        } else {+          //+          // This is
> Variable Store, rewrite both EFI_FIRMWARE_VOLUME_HEADER and
> VARIABLE_STORE_HEADER.+          // The corrupted Variable content should be
> taken care by FaultTolerantWrite driver later.+          //+          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      = (UINT32) (NvStorageFvSize - 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) {@@ -195,9 +254,9 @@ FvbInitialize (
>            }           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);           }diff --git
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.in
> f
> b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.in
> f
> index 0cfa3f909b..73049eceb2 100644
> ---
> a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.in
> f
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbSe
> +++ rviceSmm.inf
> @@ -45,6 +45,7 @@
>  [Pcd]   gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvBase         ##
> CONSUMES   gIntelSiliconPkgTokenSpaceGuid.PcdFlashMicrocodeFvSize         ##
> CONSUMES+  gIntelSiliconPkgTokenSpaceGuid.PcdFlashVariableStoreType       ##
> SOMETIMES_CONSUMES  [Sources]   FvbInfo.c@@ -61,6 +62,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 (#99818): https://edk2.groups.io/g/devel/message/99818
> Mute This Topic: https://groups.io/mt/96841486/1777047
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [chasel.chiu@intel.com] -=-
> =-=-=-=-=
>