From: "Michael Kubacki" <mikuback@linux.microsoft.com>
To: devel@edk2.groups.io, chasel.chiu@intel.com
Cc: Ashraf Ali S <ashraf.ali.s@intel.com>,
Isaac Oram <isaac.w.oram@intel.com>,
Rangasai V Chaganty <rangasai.v.chaganty@intel.com>,
Ray Ni <ray.ni@intel.com>,
Michael Kubacki <michael.kubacki@microsoft.com>
Subject: Re: [edk2-devel] [edk2-platforms: PATCH v4] IntelSiliconPkg/SpiFvbServiceSmm: Rewrite VariableStore header.
Date: Wed, 8 Feb 2023 19:40:37 -0500 [thread overview]
Message-ID: <379f9b9e-de97-ffd5-c313-6246604d9c95@linux.microsoft.com> (raw)
In-Reply-To: <20230208221723.917-1-chasel.chiu@intel.com>
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
>
next prev parent reply other threads:[~2023-02-09 0:40 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-08 22:17 [edk2-platforms: PATCH v4] IntelSiliconPkg/SpiFvbServiceSmm: Rewrite VariableStore header Chiu, Chasel
2023-02-09 0:40 ` Michael Kubacki [this message]
2023-02-09 1:38 ` [edk2-devel] " Chiu, Chasel
2023-02-09 1:39 ` Isaac Oram
2023-02-09 1:46 ` Chiu, Chasel
[not found] <1741F951181BEE58.3529@groups.io>
2023-02-09 1:50 ` Chiu, Chasel
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=379f9b9e-de97-ffd5-c313-6246604d9c95@linux.microsoft.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox