Thanks for reviewing Michael.
Please see my reply below inline and help to review V4 patch again.
Thanks,
Chasel
> -----Original Message-----
> From: Michael Kubacki <mikub...@linux.microsoft.com>
> Sent: Tuesday, February 7, 2023 12:22 PM
> To: devel@edk2.groups.io; Chiu, Chasel <chasel.c...@intel.com>
> Cc: S, Ashraf Ali <ashraf.al...@intel.com>; Oram, Isaac W
> <isaac.w.o...@intel.com>; Chaganty, Rangasai V
> <rangasai.v.chaga...@intel.com>; Ni, Ray <ray...@intel.com>; Kubacki,
> Michael <michael.kuba...@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.al...@intel.com>
> > Cc: Isaac Oram <isaac.w.o...@intel.com>
> > Cc: Rangasai V Chaganty <rangasai.v.chaga...@intel.com>
> > Cc: Ray Ni <ray...@intel.com>
> > Cc: Michael Kubacki <michael.kuba...@microsoft.com>
> > Signed-off-by: Chasel Chiu <chasel.c...@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
> >
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#99820): https://edk2.groups.io/g/devel/message/99820
Mute This Topic: https://groups.io/mt/96815037/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
