* Re: [edk2-devel] [edk2-platforms: PATCH v4] IntelSiliconPkg/SpiFvbServiceSmm: Rewrite VariableStore header.
2023-02-08 22:17 Chiu, Chasel
@ 2023-02-09 0:40 ` Michael Kubacki
2023-02-09 1:38 ` Chiu, Chasel
2023-02-09 1:39 ` Isaac Oram
0 siblings, 2 replies; 5+ messages in thread
From: Michael Kubacki @ 2023-02-09 0:40 UTC (permalink / raw)
To: devel, chasel.chiu
Cc: Ashraf Ali S, Isaac Oram, Rangasai V Chaganty, Ray Ni,
Michael Kubacki
Reviewed-by: Michael Kubacki <michael.kubacki@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.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/SpiFvbServiceMm.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.inf b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.inf
> index 0cfa3f909b..73049eceb2 100644
> --- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.inf
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.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|BOOLEAN|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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [edk2-platforms: PATCH v4] IntelSiliconPkg/SpiFvbServiceSmm: Rewrite VariableStore header.
2023-02-09 0:40 ` [edk2-devel] " Michael Kubacki
@ 2023-02-09 1:38 ` Chiu, Chasel
2023-02-09 1:39 ` Isaac Oram
1 sibling, 0 replies; 5+ messages in thread
From: Chiu, Chasel @ 2023-02-09 1:38 UTC (permalink / raw)
To: devel@edk2.groups.io, mikuback@linux.microsoft.com
Cc: S, Ashraf Ali, Oram, Isaac W, Chaganty, Rangasai V, Ni, Ray,
Kubacki, Michael
Thanks Michael! I will apply the change during merging!
> -----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.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 v4]
> IntelSiliconPkg/SpiFvbServiceSmm: Rewrite VariableStore header.
>
> Reviewed-by: Michael Kubacki <michael.kubacki@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.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/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
> >
>
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [edk2-platforms: PATCH v4] IntelSiliconPkg/SpiFvbServiceSmm: Rewrite VariableStore header.
2023-02-09 0:40 ` [edk2-devel] " Michael Kubacki
2023-02-09 1:38 ` Chiu, Chasel
@ 2023-02-09 1:39 ` Isaac Oram
2023-02-09 1:46 ` Chiu, Chasel
1 sibling, 1 reply; 5+ messages in thread
From: Isaac Oram @ 2023-02-09 1:39 UTC (permalink / raw)
To: devel@edk2.groups.io, mikuback@linux.microsoft.com, Chiu, Chasel
Cc: S, Ashraf Ali, Chaganty, Rangasai V, Ni, Ray, Kubacki, Michael
Reviewed-by: Isaac Oram <isaac.w.oram@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.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 v4] IntelSiliconPkg/SpiFvbServiceSmm: Rewrite VariableStore header.
Reviewed-by: Michael Kubacki <michael.kubacki@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.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/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|BOOLEAN
> |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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [edk2-platforms: PATCH v4] IntelSiliconPkg/SpiFvbServiceSmm: Rewrite VariableStore header.
2023-02-09 1:39 ` Isaac Oram
@ 2023-02-09 1:46 ` Chiu, Chasel
0 siblings, 0 replies; 5+ messages in thread
From: Chiu, Chasel @ 2023-02-09 1:46 UTC (permalink / raw)
To: Oram, Isaac W, devel@edk2.groups.io, mikuback@linux.microsoft.com
Cc: S, Ashraf Ali, Chaganty, Rangasai V, Ni, Ray, Kubacki, Michael
Thanks Isaac!
> -----Original Message-----
> From: Oram, Isaac W <isaac.w.oram@intel.com>
> Sent: Wednesday, February 8, 2023 5:39 PM
> To: devel@edk2.groups.io; mikuback@linux.microsoft.com; Chiu, Chasel
> <chasel.chiu@intel.com>
> Cc: S, Ashraf Ali <ashraf.ali.s@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 v4]
> IntelSiliconPkg/SpiFvbServiceSmm: Rewrite VariableStore header.
>
> Reviewed-by: Isaac Oram <isaac.w.oram@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.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 v4]
> IntelSiliconPkg/SpiFvbServiceSmm: Rewrite VariableStore header.
>
> Reviewed-by: Michael Kubacki <michael.kubacki@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.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/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
> >
>
>
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [edk2-devel] [edk2-platforms: PATCH v4] IntelSiliconPkg/SpiFvbServiceSmm: Rewrite VariableStore header.
[not found] <1741F951181BEE58.3529@groups.io>
@ 2023-02-09 1:50 ` Chiu, Chasel
0 siblings, 0 replies; 5+ messages in thread
From: Chiu, Chasel @ 2023-02-09 1:50 UTC (permalink / raw)
To: devel@edk2.groups.io, Chiu, Chasel
Cc: S, Ashraf Ali, Oram, Isaac W, Chaganty, Rangasai V, Ni, Ray,
Kubacki, Michael
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] -=-
> =-=-=-=-=
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-02-09 1:50 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1741F951181BEE58.3529@groups.io>
2023-02-09 1:50 ` [edk2-devel] [edk2-platforms: PATCH v4] IntelSiliconPkg/SpiFvbServiceSmm: Rewrite VariableStore header Chiu, Chasel
2023-02-08 22:17 Chiu, Chasel
2023-02-09 0:40 ` [edk2-devel] " Michael Kubacki
2023-02-09 1:38 ` Chiu, Chasel
2023-02-09 1:39 ` Isaac Oram
2023-02-09 1:46 ` Chiu, Chasel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox