* [edk2-platforms: PATCH v3] IntelSiliconPkg/SpiFvbServiceSmm: Rewrite VariableStore header.
@ 2023-02-07 19:42 Chiu, Chasel
2023-02-07 20:22 ` [edk2-devel] " Michael Kubacki
0 siblings, 1 reply; 3+ messages in thread
From: Chiu, Chasel @ 2023-02-07 19:42 UTC (permalink / raw)
To: devel
Cc: Chasel Chiu, Ashraf Ali S, Isaac Oram, Rangasai V Chaganty,
Ray Ni, Michael Kubacki
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/SpiFvbServiceMm.c b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c
index 6b4bcdcfe3..6338442e1a 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
@@ -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/SpiFvbServiceSmm.inf b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.inf
index 0cfa3f909b..0485b73679 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.inf
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.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|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
--
2.35.0.windows.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [edk2-devel] [edk2-platforms: PATCH v3] IntelSiliconPkg/SpiFvbServiceSmm: Rewrite VariableStore header.
2023-02-07 19:42 [edk2-platforms: PATCH v3] IntelSiliconPkg/SpiFvbServiceSmm: Rewrite VariableStore header Chiu, Chasel
@ 2023-02-07 20:22 ` Michael Kubacki
2023-02-08 22:24 ` Chiu, Chasel
0 siblings, 1 reply; 3+ messages in thread
From: Michael Kubacki @ 2023-02-07 20:22 UTC (permalink / raw)
To: devel, chasel.chiu
Cc: Ashraf Ali S, Isaac Oram, Rangasai V Chaganty, Ray Ni,
Michael Kubacki
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().
2. VariableFlashInfoLib is meant to abstract var store info with
GetVariableFlashNvStorageInfo().
Please use that instead of directly getting it with PcdGet32
(PcdFlashNvStorageVariableSize).
3. Suggest a blank link before the following text for readability.
//
// Write buffer to flash
//
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.
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/SpiFvbServiceMm.c b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c
> index 6b4bcdcfe3..6338442e1a 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
>
> @@ -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/SpiFvbServiceSmm.inf b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.inf
> index 0cfa3f909b..0485b73679 100644
> --- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.inf
> +++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceSmm.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|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] 3+ messages in thread
* Re: [edk2-devel] [edk2-platforms: PATCH v3] IntelSiliconPkg/SpiFvbServiceSmm: Rewrite VariableStore header.
2023-02-07 20:22 ` [edk2-devel] " Michael Kubacki
@ 2023-02-08 22:24 ` Chiu, Chasel
0 siblings, 0 replies; 3+ messages in thread
From: Chiu, Chasel @ 2023-02-08 22:24 UTC (permalink / raw)
To: Michael Kubacki, devel@edk2.groups.io
Cc: S, Ashraf Ali, Oram, Isaac W, Chaganty, Rangasai V, Ni, Ray,
Kubacki, Michael
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
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-02-08 22:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-07 19:42 [edk2-platforms: PATCH v3] IntelSiliconPkg/SpiFvbServiceSmm: Rewrite VariableStore header Chiu, Chasel
2023-02-07 20:22 ` [edk2-devel] " Michael Kubacki
2023-02-08 22:24 ` Chiu, Chasel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox