From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web11.2507.1675903240276027881 for ; Wed, 08 Feb 2023 16:40:40 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=RYcfL+y7; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [192.168.4.22] (unknown [47.201.8.94]) by linux.microsoft.com (Postfix) with ESMTPSA id ECB9A20C8AD6; Wed, 8 Feb 2023 16:40:38 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com ECB9A20C8AD6 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1675903239; bh=9WNWOO4k3pgjeTIOWTtozDovU53oFS6Tw/kZDnkUFFk=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=RYcfL+y7JBYyjNGbwYxM6Zb/0FZogOgD1LfMq2QJEUWVJSfo7U1+sSwgEzlVn5Ce/ IjyeQdMJGeZ4tM7Pw7cclgx9coHCV+JFQS8fTmnCtcvPktLdI0EdKxuazgU4eL4tQ7 XcBfPOGvmhfh7Yofyv3pqm3sd+OaCdCLZkmn3VJU= Message-ID: <379f9b9e-de97-ffd5-c313-6246604d9c95@linux.microsoft.com> Date: Wed, 8 Feb 2023 19:40:37 -0500 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.13.1 Subject: Re: [edk2-devel] [edk2-platforms: PATCH v4] IntelSiliconPkg/SpiFvbServiceSmm: Rewrite VariableStore header. To: devel@edk2.groups.io, chasel.chiu@intel.com Cc: Ashraf Ali S , Isaac Oram , Rangasai V Chaganty , Ray Ni , Michael Kubacki References: <20230208221723.917-1-chasel.chiu@intel.com> From: "Michael Kubacki" In-Reply-To: <20230208221723.917-1-chasel.chiu@intel.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Reviewed-by: Michael Kubacki 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 > Cc: Isaac Oram > Cc: Rangasai V Chaganty > Cc: Ray Ni > Cc: Michael Kubacki > Signed-off-by: Chasel Chiu > --- > 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 > > #include > > #include > > +#include > > > > /** > > 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.

> > + # When Flash Variable Store corruption happened, the SpiFvbService will recreate Variable Store > > + # with valid header information provided by this PCD value.
> > + # 0: Variable Store is gEfiVariableGuid type.
> > + # 1: Variable Store is gEfiAuthenticatedVariableGuid type.
> > + # Other value: reserved for future use.
> > + # @Prompt Flash Variable Store type. > > + gIntelSiliconPkgTokenSpaceGuid.PcdFlashVariableStoreType|0x00|UINT8|0x0000000E >