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

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

Thanks for reviewing Michael.
Please see my reply below inline and help to review V4 patch again.

Thanks,
Chasel



> -----Original Message-----
> From: Michael Kubacki <mikuback@linux.microsoft.com>
> Sent: Tuesday, February 7, 2023 12:22 PM
> To: devel@edk2.groups.io; Chiu, Chasel <chasel.chiu@intel.com>
> Cc: 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: Re: [edk2-devel] [edk2-platforms: PATCH v3]
> IntelSiliconPkg/SpiFvbServiceSmm: Rewrite VariableStore header.
> 
> Hi Chasel,
> 
> I agree with the high-level problem. Here's some observations.
> 
> 1. I'm not a big fan of implicitly associating index zero of the
> mPlatformFvBaseAddress array as the variable FV entry. It could be easy for that
> to silently get out of sync in the future.
> 
> Perhaps you could check if the base address at a given index equals the variable
> store address since it is retrieved via
> GetVariableFlashNvStorageInfo() at the beginning of FvbInitialize().
> 


Done in V4


> 2. VariableFlashInfoLib is meant to abstract var store info with
> GetVariableFlashNvStorageInfo().
> 
> Please use that instead of directly getting it with PcdGet32
> (PcdFlashNvStorageVariableSize).
> 


Done in V4

> 3. Suggest a blank link before the following text for readability.
> 
>              //
>              // Write buffer to flash
>              //
> 


Done in V4

> 4. Usually there's a few default variables in the var store FV that form a
> foundation that the variable HOB gets written upon when the HOB gets flushed.
> Does the original var store FV still contain some of those entries? If so, this just
> restores an empty store, so those are known to be lost, right?
> 
> If that's true, please call it out in the patch description.
> 

Since SpiFvbService driver only needs VariableStore information, it should only rewrite FV and VariableStore headers.
I added comments in code and commit message to mention about "the corrupted variable content should be taken care by FaultTolerantWrite driver later".
Please see if this I good enough.



> Thanks,
> Michael
> 
> On 2/7/2023 2:42 PM, Chiu, Chasel wrote:
> > 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
> | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
> --
> >
> Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.inf
> |  4 ++++
> >   Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec                              |  8 ++++++++
> >   3 files changed, 71 insertions(+), 5 deletions(-)
> >
> > diff --git
> > a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> > iceMm.c
> > b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> > iceMm.c
> > index 6b4bcdcfe3..6338442e1a 100644
> > ---
> > a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> > iceMm.c
> > +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvb
> > +++ ServiceMm.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
> >
> > @@ -114,6 +115,10 @@ FvbInitialize (
> >     UINT32                                BytesWritten;
> >
> >     UINTN                                 BytesErased;
> >
> >     UINT64                                NvStorageFvSize;
> >
> > +  UINT32                                ExpectedBytesWritten;
> >
> > +  VARIABLE_STORE_HEADER                 *VariableStoreHeader;
> >
> > +  UINT8                                 VariableStoreType;
> >
> > +  UINT8                                 *NvStoreBuffer;
> >
> >
> >
> >     Status = GetVariableFlashNvStorageInfo (&BaseAddress,
> > &NvStorageFvSize);
> >
> >     if (EFI_ERROR (Status)) {
> >
> > @@ -186,8 +191,57 @@ FvbInitialize (
> >             }
> >
> >             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) {
> >
> > @@ -195,9 +249,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/SpiFvbServ
> > iceSmm.inf
> > b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> > iceSmm.inf
> > index 0cfa3f909b..0485b73679 100644
> > ---
> > a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> > iceSmm.inf
> > +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvb
> > +++ ServiceSmm.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]
> >
> >     TRUE
> >
> > diff --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|
> > + 0x0000000E
> >