Thanks Isaac!
> -----Original Message-----
> From: Oram, Isaac W <isaac.w.o...@intel.com>
> Sent: Wednesday, February 8, 2023 5:39 PM
> To: devel@edk2.groups.io; mikub...@linux.microsoft.com; Chiu, Chasel
> <chasel.c...@intel.com>
> Cc: S, Ashraf Ali <ashraf.al...@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 v4]
> IntelSiliconPkg/SpiFvbServiceSmm: Rewrite VariableStore header.
>
> Reviewed-by: Isaac Oram <isaac.w.o...@intel.com>
>
> At some point, we should work to comment the related flows better so that
> code is clear on the different responsibilities for the different paths
> through first
> boots, normal scenarios, reclaims, and error remediation. For now though,
> this
> is fine.
>
> Regards,
> Isaac
>
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael
> Kubacki
> Sent: Wednesday, February 8, 2023 4:41 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 v4]
> IntelSiliconPkg/SpiFvbServiceSmm: Rewrite VariableStore header.
>
> Reviewed-by: Michael Kubacki <michael.kuba...@microsoft.com>
>
> On the following lines, I recommend moving the assignment until after the if
> block. It seems unnecessary to assign a potentially invalid value to a local
> variable before checking the validation result.
>
> 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;
> }
>
> ---
>
> (similar for NvStorageFvSize)
>
> Thanks,
> Michael
>
> On 2/8/2023 5:17 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.
> > 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.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
> | 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/SpiFvbServ
> > iceMm.c
> > b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> > iceMm.c
> > index 6b4bcdcfe3..052be97872 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
> >
> > @@ -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/SpiFvbServ
> > iceSmm.inf
> > b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> > iceSmm.inf
> > index 0cfa3f909b..73049eceb2 100644
> > ---
> > a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServ
> > iceSmm.inf
> > +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvb
> > +++ ServiceSmm.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]
> >
> > 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 (#99838): https://edk2.groups.io/g/devel/message/99838
Mute This Topic: https://groups.io/mt/96841486/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
