* [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/SpiFvbService: Non-functional cleanup
@ 2023-06-30 2:32 Michael Kubacki
2023-06-30 18:57 ` [edk2-devel] " Ashraf Ali S
0 siblings, 1 reply; 4+ messages in thread
From: Michael Kubacki @ 2023-06-30 2:32 UTC (permalink / raw)
To: devel; +Cc: Ray Ni, Rangasai V Chaganty, Isaac Oram, Ashraf Ali S
From: Michael Kubacki <michael.kubacki@microsoft.com>
During a review of this driver a number of improvements were noted
such as strengthening function input validation, checking return
values, making debug print error levels consistent in certain code
blocks, etc.
These type of changes are made with no explicit change to driver
functionality.
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Ashraf Ali S <ashraf.ali.s@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c | 35 ++-
Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c | 248 ++++++++++++--------
Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c | 54 +++--
Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h | 31 ++-
Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.h | 6 +-
5 files changed, 239 insertions(+), 135 deletions(-)
diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c
index ab1cb2ef1622..ebbd9edaada3 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c
@@ -2,16 +2,17 @@
Defines data structure that is the volume header found.
These data is intent to decouple FVB driver with FV header.
-Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
-Copyright (c) Microsoft Corporation.<BR>
-SPDX-License-Identifier: BSD-2-Clause-Patent
+ Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) Microsoft Corporation.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
**/
#include "SpiFvbServiceCommon.h"
-#define FIRMWARE_BLOCK_SIZE 0x10000
+#define FIRMWARE_BLOCK_SIZE SIZE_64KB
#define FVB_MEDIA_BLOCK_SIZE FIRMWARE_BLOCK_SIZE
+
typedef struct {
EFI_PHYSICAL_ADDRESS BaseAddress;
EFI_FIRMWARE_VOLUME_HEADER FvbInfo;
@@ -19,7 +20,7 @@ typedef struct {
} EFI_FVB2_MEDIA_INFO;
/**
- Returns FVB media information for NV variable storage.
+ Returns FVB media information for a firmware volume.
@return FvbMediaInfo A pointer to an instance of FVB media info produced by this function.
The buffer is allocated internally to this function and it is the caller's
@@ -100,8 +101,21 @@ FVB_MEDIA_INFO_GENERATOR mFvbMediaInfoGenerators[] = {
GenerateNvStorageFvbMediaInfo
};
+/**
+ Returns an empty firmware volume for the firmware volume at the given base address.
+
+ @param[in] FvBaseAddress The base address of the firmware volume requested.
+ @param[out] FvbInfo A pointer that will be set to a buffer for the firmware volume header
+ at the given base address. The buffer is a pool allocation made in this function.
+
+ @retval EFI_SUCCESS The firmware volume was returned successfully.
+ @retval EFI_INVALID_PARAMETER The FvbInfo pointer argument is NULL.
+ @retval EFI_NOT_FOUND The firmware volume was not found for the given base address.
+ @retval EFI_OUT_OF_RESOURCES Insufficient memory to allocate a buffer to the hold the firmware volume.
+
+**/
EFI_STATUS
-GetFvbInfo (
+GetGeneratedFvByAddress (
IN EFI_PHYSICAL_ADDRESS FvBaseAddress,
OUT EFI_FIRMWARE_VOLUME_HEADER **FvbInfo
)
@@ -111,11 +125,19 @@ GetFvbInfo (
UINTN Index;
EFI_FIRMWARE_VOLUME_HEADER *FvHeader;
+ if (FvbInfo == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
for (Index = 0; Index < ARRAY_SIZE (mFvbMediaInfoGenerators); Index++) {
Status = mFvbMediaInfoGenerators[Index](&FvbMediaInfo);
ASSERT_EFI_ERROR (Status);
if (!EFI_ERROR (Status) && (FvbMediaInfo.BaseAddress == FvBaseAddress)) {
FvHeader = AllocateCopyPool (FvbMediaInfo.FvbInfo.HeaderLength, &FvbMediaInfo.FvbInfo);
+ if (FvHeader == NULL) {
+ ASSERT (FvHeader != NULL);
+ return EFI_OUT_OF_RESOURCES;
+ }
//
// Update the checksum value of FV header.
@@ -136,5 +158,6 @@ GetFvbInfo (
return EFI_SUCCESS;
}
}
+
return EFI_NOT_FOUND;
}
diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c
index fcdc715263f2..e21113682f4f 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c
@@ -2,9 +2,9 @@
Common driver source for several Serial Flash devices
which are compliant with the Intel(R) Serial Flash Interface Compatibility Specification.
-Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
-Copyright (c) Microsoft Corporation.<BR>
-SPDX-License-Identifier: BSD-2-Clause-Patent
+ Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) Microsoft Corporation.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -92,13 +92,19 @@ EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL mFvbProtocolTemplate = {
@param[in] FvbInstance The pointer to the EFI_FVB_INSTANCE.
@return Attributes of the FV identified by FvbInstance.
+ Zero is returned if the FvbInstance pointer is NULL.
**/
EFI_FVB_ATTRIBUTES_2
FvbGetVolumeAttributes (
- IN EFI_FVB_INSTANCE *FvbInstance
+ IN CONST EFI_FVB_INSTANCE *FvbInstance
)
{
+ if (FvbInstance == NULL) {
+ ASSERT (FvbInstance != NULL);
+ return 0;
+ }
+
return FvbInstance->FvHeader.Attributes;
}
@@ -109,24 +115,25 @@ FvbGetVolumeAttributes (
@param[in] FvbInstance The pointer to the EFI_FVB_INSTANCE.
@param[in] Lba The logical block address
@param[out] LbaAddress On output, contains the physical starting address
- of the Lba
- @param[out] LbaLength On output, contains the length of the block
+ of the Lba. This pointer is optional and may be NULL.
+ @param[out] LbaLength On output, contains the length of the block.
+ This pointer is optional and may be NULL.
@param[out] NumOfBlocks A pointer to a caller allocated UINTN in which the
number of consecutive blocks starting with Lba is
returned. All blocks in this range have a size of
- BlockSize
+ BlockSize. This pointer is optional and may be NULL.
- @retval EFI_SUCCESS Successfully returns
- @retval EFI_INVALID_PARAMETER Instance not found
+ @retval EFI_SUCCESS Successfully returns
+ @retval EFI_INVALID_PARAMETER FvbInstance is NULL.
**/
EFI_STATUS
FvbGetLbaAddress (
- IN EFI_FVB_INSTANCE *FvbInstance,
+ IN CONST EFI_FVB_INSTANCE *FvbInstance,
IN EFI_LBA Lba,
- OUT UINTN *LbaAddress,
- OUT UINTN *LbaLength,
- OUT UINTN *NumOfBlocks
+ OUT UINTN *LbaAddress OPTIONAL,
+ OUT UINTN *LbaLength OPTIONAL,
+ OUT UINTN *NumOfBlocks OPTIONAL
)
{
UINT32 NumBlocks;
@@ -134,10 +141,15 @@ FvbGetLbaAddress (
UINTN Offset;
EFI_LBA StartLba;
EFI_LBA NextLba;
- EFI_FV_BLOCK_MAP_ENTRY *BlockMap;
+ CONST EFI_FV_BLOCK_MAP_ENTRY *BlockMap;
StartLba = 0;
Offset = 0;
+
+ if (FvbInstance == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
BlockMap = &(FvbInstance->FvHeader.BlockMap[0]);
//
@@ -158,15 +170,15 @@ FvbGetLbaAddress (
//
if (Lba >= StartLba && Lba < NextLba) {
Offset = Offset + (UINTN)MultU64x32((Lba - StartLba), BlockLength);
- if (LbaAddress ) {
+ if (LbaAddress != NULL) {
*LbaAddress = FvbInstance->FvBase + Offset;
}
- if (LbaLength ) {
+ if (LbaLength != NULL) {
*LbaLength = BlockLength;
}
- if (NumOfBlocks ) {
+ if (NumOfBlocks != NULL) {
*NumOfBlocks = (UINTN)(NextLba - Lba);
}
return EFI_SUCCESS;
@@ -190,7 +202,6 @@ FvbGetLbaAddress (
@param[in] Buffer Pointer to a caller allocated buffer that will be
used to hold the data read
-
@retval EFI_SUCCESS The firmware volume was read successfully and
contents are in Buffer
@retval EFI_BAD_BUFFER_SIZE Read attempted across a LBA boundary. On output,
@@ -199,16 +210,16 @@ FvbGetLbaAddress (
@retval EFI_ACCESS_DENIED The firmware volume is in the ReadDisabled state
@retval EFI_DEVICE_ERROR The block device is not functioning correctly and
could not be read
- @retval EFI_INVALID_PARAMETER Instance not found, or NumBytes, Buffer are NULL
+ @retval EFI_INVALID_PARAMETER FvbInstance, NumBytes, and/or Buffer are NULL
**/
EFI_STATUS
FvbReadBlock (
- IN EFI_FVB_INSTANCE *FvbInstance,
- IN EFI_LBA Lba,
- IN UINTN BlockOffset,
- IN OUT UINTN *NumBytes,
- IN UINT8 *Buffer
+ IN CONST EFI_FVB_INSTANCE *FvbInstance,
+ IN EFI_LBA Lba,
+ IN UINTN BlockOffset,
+ IN OUT UINTN *NumBytes,
+ IN UINT8 *Buffer
)
{
EFI_FVB_ATTRIBUTES_2 Attributes;
@@ -217,9 +228,10 @@ FvbReadBlock (
EFI_STATUS Status;
BOOLEAN BadBufferSize = FALSE;
- if ((NumBytes == NULL) || (Buffer == NULL)) {
+ if ((FvbInstance == NULL) || (NumBytes == NULL) || (Buffer == NULL)) {
return EFI_INVALID_PARAMETER;
}
+
if (*NumBytes == 0) {
return EFI_INVALID_PARAMETER;
}
@@ -269,6 +281,7 @@ FvbReadBlock (
of bytes actually written
@param[in] Buffer Pointer to a caller allocated buffer that contains
the source for the write
+
@retval EFI_SUCCESS The firmware volume was written successfully
@retval EFI_BAD_BUFFER_SIZE Write attempted across a LBA boundary. On output,
NumBytes contains the total number of bytes
@@ -276,16 +289,16 @@ FvbReadBlock (
@retval EFI_ACCESS_DENIED The firmware volume is in the WriteDisabled state
@retval EFI_DEVICE_ERROR The block device is not functioning correctly and
could not be written
- @retval EFI_INVALID_PARAMETER Instance not found, or NumBytes, Buffer are NULL
+ @retval EFI_INVALID_PARAMETER FvbInstance, NumBytes, and/or Buffer are NULL
**/
EFI_STATUS
FvbWriteBlock (
- IN EFI_FVB_INSTANCE *FvbInstance,
- IN EFI_LBA Lba,
- IN UINTN BlockOffset,
- IN OUT UINTN *NumBytes,
- IN UINT8 *Buffer
+ IN CONST EFI_FVB_INSTANCE *FvbInstance,
+ IN EFI_LBA Lba,
+ IN UINTN BlockOffset,
+ IN OUT UINTN *NumBytes,
+ IN UINT8 *Buffer
)
{
EFI_FVB_ATTRIBUTES_2 Attributes;
@@ -294,7 +307,7 @@ FvbWriteBlock (
EFI_STATUS Status;
BOOLEAN BadBufferSize = FALSE;
- if ((NumBytes == NULL) || (Buffer == NULL)) {
+ if ((FvbInstance == NULL) || (NumBytes == NULL) || (Buffer == NULL)) {
return EFI_INVALID_PARAMETER;
}
if (*NumBytes == 0) {
@@ -350,8 +363,6 @@ FvbWriteBlock (
}
}
-
-
/**
Erases and initializes a firmware volume block.
@@ -363,13 +374,13 @@ FvbWriteBlock (
@retval EFI_DEVICE_ERROR The block device is not functioning correctly and
could not be written. Firmware device may have been
partially erased
- @retval EFI_INVALID_PARAMETER Instance not found
+ @retval EFI_INVALID_PARAMETER FvbInstance is NULL
**/
EFI_STATUS
FvbEraseBlock (
- IN EFI_FVB_INSTANCE *FvbInstance,
- IN EFI_LBA Lba
+ IN CONST EFI_FVB_INSTANCE *FvbInstance,
+ IN EFI_LBA Lba
)
{
@@ -378,6 +389,10 @@ FvbEraseBlock (
UINTN LbaLength;
EFI_STATUS Status;
+ if (FvbInstance == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
//
// Check if the FV is write enabled
//
@@ -422,9 +437,9 @@ FvbEraseBlock (
@retval EFI_SUCCESS Successfully returns
@retval EFI_ACCESS_DENIED The volume setting is locked and cannot be modified
- @retval EFI_INVALID_PARAMETER Instance not found, or The attributes requested are
- in conflict with the capabilities as declared in the
- firmware volume header
+ @retval EFI_INVALID_PARAMETER FvbInstance or Attributes is NULL.
+ Or the attributes requested are in conflict with the
+ capabilities as declared in the firmware volume header.
**/
EFI_STATUS
@@ -439,6 +454,10 @@ FvbSetVolumeAttributes (
UINT32 Capabilities;
UINT32 OldStatus, NewStatus;
+ if ((FvbInstance == NULL) || (Attributes == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
AttribPtr = (EFI_FVB_ATTRIBUTES_2 *) &(FvbInstance->FvHeader.Attributes);
OldAttributes = *AttribPtr;
Capabilities = OldAttributes & EFI_FVB2_CAPABILITIES;
@@ -554,6 +573,7 @@ GetVariableFvInfo (
ASSERT ((BaseAddress != NULL) && (Length != NULL));
return;
}
+
*BaseAddress = 0;
*Length = 0;
@@ -631,8 +651,9 @@ GetVariableFvInfo (
@param[in] FvHeader A pointer to a firmware volume header
- @retval TRUE The firmware volume is consistent
- @retval FALSE The firmware volume has corrupted.
+ @retval TRUE The firmware volume is consistent.
+ @retval FALSE The firmware volume has corrupted or an invalid firmware
+ volume was provided.
**/
BOOLEAN
@@ -644,6 +665,11 @@ IsFvHeaderValid (
EFI_PHYSICAL_ADDRESS NvStorageFvBaseAddress;
UINT32 NvStorageSize;
+ if (FvHeader == NULL) {
+ ASSERT (FvHeader != NULL);
+ return FALSE;
+ }
+
GetVariableFvInfo (&NvStorageFvBaseAddress, &NvStorageSize);
if (FvBase == NvStorageFvBaseAddress) {
@@ -679,18 +705,23 @@ IsFvHeaderValid (
@param[in] This A pointer to EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL.
@param[out] Address Output buffer containing the address.
- retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
**/
EFI_STATUS
EFIAPI
FvbProtocolGetPhysicalAddress (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- OUT EFI_PHYSICAL_ADDRESS *Address
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ OUT EFI_PHYSICAL_ADDRESS *Address
)
{
EFI_FVB_INSTANCE *FvbInstance;
+ if ((This == NULL) || (Address == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
*Address = FvbInstance->FvBase;
@@ -710,28 +741,34 @@ FvbProtocolGetPhysicalAddress (
returned. All blocks in this range have a size of
BlockSize
- @retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
**/
EFI_STATUS
EFIAPI
FvbProtocolGetBlockSize (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- IN EFI_LBA Lba,
- OUT UINTN *BlockSize,
- OUT UINTN *NumOfBlocks
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ IN EFI_LBA Lba,
+ OUT UINTN *BlockSize,
+ OUT UINTN *NumOfBlocks
)
{
EFI_FVB_INSTANCE *FvbInstance;
+ if ((This == NULL) || (BlockSize == NULL) || (NumOfBlocks == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
- DEBUG((DEBUG_INFO,
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolGetBlockSize: Lba: 0x%lx BlockSize: 0x%x NumOfBlocks: 0x%x\n",
Lba,
- BlockSize,
- NumOfBlocks)
- );
+ *BlockSize,
+ *NumOfBlocks
+ ));
return FvbGetLbaAddress (
FvbInstance,
@@ -748,27 +785,33 @@ FvbProtocolGetBlockSize (
@param[in] This Calling context
@param[out] Attributes Output buffer which contains attributes
- @retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
**/
EFI_STATUS
EFIAPI
FvbProtocolGetAttributes (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- OUT EFI_FVB_ATTRIBUTES_2 *Attributes
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ OUT EFI_FVB_ATTRIBUTES_2 *Attributes
)
{
EFI_FVB_INSTANCE *FvbInstance;
+ if ((This == NULL) || (Attributes == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
*Attributes = FvbGetVolumeAttributes (FvbInstance);
- DEBUG ((DEBUG_INFO,
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolGetAttributes: This: 0x%x Attributes: 0x%x\n",
This,
- *Attributes)
- );
+ *Attributes
+ ));
return EFI_SUCCESS;
}
@@ -785,28 +828,34 @@ FvbProtocolGetAttributes (
EFI_STATUS
EFIAPI
FvbProtocolSetAttributes (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- IN OUT EFI_FVB_ATTRIBUTES_2 *Attributes
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ IN OUT EFI_FVB_ATTRIBUTES_2 *Attributes
)
{
EFI_STATUS Status;
EFI_FVB_INSTANCE *FvbInstance;
- DEBUG((DEBUG_INFO,
+ if ((This == NULL) || (Attributes == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolSetAttributes: Before SET - This: 0x%x Attributes: 0x%x\n",
This,
- *Attributes)
- );
+ *Attributes
+ ));
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
Status = FvbSetVolumeAttributes (FvbInstance, Attributes);
- DEBUG((DEBUG_INFO,
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolSetAttributes: After SET - This: 0x%x Attributes: 0x%x\n",
This,
- *Attributes)
- );
+ *Attributes
+ ));
return Status;
}
@@ -823,17 +872,18 @@ FvbProtocolSetAttributes (
@param[in] ... Starting LBA followed by Number of Lba to erase.
a -1 to terminate the list.
- @retval EFI_SUCCESS The erase request was successfully completed
- @retval EFI_ACCESS_DENIED The firmware volume is in the WriteDisabled state
- @retval EFI_DEVICE_ERROR The block device is not functioning correctly and
- could not be written. Firmware device may have been
- partially erased
+ @retval EFI_SUCCESS The erase request was successfully completed
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
+ @retval EFI_ACCESS_DENIED The firmware volume is in the WriteDisabled state
+ @retval EFI_DEVICE_ERROR The block device is not functioning correctly and
+ could not be written. Firmware device may have been
+ partially erased
**/
EFI_STATUS
EFIAPI
FvbProtocolEraseBlocks (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
...
)
{
@@ -846,6 +896,10 @@ FvbProtocolEraseBlocks (
DEBUG((DEBUG_INFO, "FvbProtocolEraseBlocks: \n"));
+ if (This == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
NumOfBlocks = FvbInstance->NumOfBlocks;
@@ -923,30 +977,35 @@ FvbProtocolEraseBlocks (
@retval EFI_ACCESS_DENIED The firmware volume is in the WriteDisabled state
@retval EFI_DEVICE_ERROR The block device is not functioning correctly and
could not be written
- @retval EFI_INVALID_PARAMETER NumBytes or Buffer are NULL
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
**/
EFI_STATUS
EFIAPI
FvbProtocolWrite (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- IN EFI_LBA Lba,
- IN UINTN Offset,
- IN OUT UINTN *NumBytes,
- IN UINT8 *Buffer
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ IN EFI_LBA Lba,
+ IN UINTN Offset,
+ IN OUT UINTN *NumBytes,
+ IN UINT8 *Buffer
)
{
EFI_FVB_INSTANCE *FvbInstance;
+ if ((This == NULL) || (NumBytes == NULL) || (Buffer == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
- DEBUG((DEBUG_INFO,
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolWrite: Lba: 0x%lx Offset: 0x%x NumBytes: 0x%x, Buffer: 0x%x\n",
Lba,
Offset,
*NumBytes,
- Buffer)
- );
+ Buffer
+ ));
return FvbWriteBlock (FvbInstance, Lba, Offset, NumBytes, Buffer);
}
@@ -974,31 +1033,36 @@ FvbProtocolWrite (
@retval EFI_ACCESS_DENIED The firmware volume is in the ReadDisabled state
@retval EFI_DEVICE_ERROR The block device is not functioning correctly and
could not be read
- @retval EFI_INVALID_PARAMETER NumBytes or Buffer are NULL
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
**/
EFI_STATUS
EFIAPI
FvbProtocolRead (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- IN EFI_LBA Lba,
- IN UINTN Offset,
- IN OUT UINTN *NumBytes,
- OUT UINT8 *Buffer
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ IN EFI_LBA Lba,
+ IN UINTN Offset,
+ IN OUT UINTN *NumBytes,
+ OUT UINT8 *Buffer
)
{
EFI_FVB_INSTANCE *FvbInstance;
EFI_STATUS Status;
+ if ((This == NULL) || (NumBytes == NULL) || (Buffer == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
Status = FvbReadBlock (FvbInstance, Lba, Offset, NumBytes, Buffer);
- DEBUG((DEBUG_INFO,
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolRead: Lba: 0x%lx Offset: 0x%x NumBytes: 0x%x, Buffer: 0x%x\n",
Lba,
Offset,
*NumBytes,
- Buffer)
- );
+ Buffer
+ ));
return Status;
}
diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c
index f9543c5ba677..5d7591480319 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c
@@ -15,14 +15,11 @@
#include <Guid/VariableFormat.h>
/**
- The function installs EFI_FIRMWARE_VOLUME_BLOCK protocol
- for each FV in the system.
+ The function installs the given EFI_FIRMWARE_VOLUME_BLOCK protocol instance.
@param[in] FvbInstance The pointer to a FW volume instance structure,
which contains the information about one FV.
- @retval VOID
-
**/
VOID
InstallFvbProtocol (
@@ -33,8 +30,8 @@ InstallFvbProtocol (
EFI_STATUS Status;
EFI_HANDLE FvbHandle;
- ASSERT (FvbInstance != NULL);
if (FvbInstance == NULL) {
+ ASSERT (FvbInstance != NULL);
return;
}
@@ -48,14 +45,14 @@ InstallFvbProtocol (
//
// Set up the devicepath
//
- DEBUG ((DEBUG_INFO, "FwBlockService.c: Setting up DevicePath for 0x%lx:\n", FvbInstance->FvBase));
+ DEBUG ((DEBUG_INFO, "SpiFvbServiceSmm.c: Setting up DevicePath for 0x%lx:\n", FvbInstance->FvBase));
if (FvHeader->ExtHeaderOffset == 0) {
//
// FV does not contains extension header, then produce MEMMAP_DEVICE_PATH
//
FvbInstance->DevicePath = (EFI_DEVICE_PATH_PROTOCOL *) AllocateRuntimeCopyPool (sizeof (FV_MEMMAP_DEVICE_PATH), &mFvMemmapDevicePathTemplate);
if (FvbInstance->DevicePath == NULL) {
- DEBUG ((DEBUG_INFO, "SpiFvbServiceSmm.c: Memory allocation for MEMMAP_DEVICE_PATH failed\n"));
+ DEBUG ((DEBUG_ERROR, "SpiFvbServiceSmm.c: Memory allocation for MEMMAP_DEVICE_PATH failed\n"));
return;
}
((FV_MEMMAP_DEVICE_PATH *) FvbInstance->DevicePath)->MemMapDevPath.StartingAddress = FvbInstance->FvBase;
@@ -63,7 +60,7 @@ InstallFvbProtocol (
} else {
FvbInstance->DevicePath = (EFI_DEVICE_PATH_PROTOCOL *) AllocateRuntimeCopyPool (sizeof (FV_PIWG_DEVICE_PATH), &mFvPIWGDevicePathTemplate);
if (FvbInstance->DevicePath == NULL) {
- DEBUG ((DEBUG_INFO, "SpiFvbServiceSmm.c: Memory allocation for FV_PIWG_DEVICE_PATH failed\n"));
+ DEBUG ((DEBUG_ERROR, "SpiFvbServiceSmm.c: Memory allocation for FV_PIWG_DEVICE_PATH failed\n"));
return;
}
CopyGuid (
@@ -96,7 +93,7 @@ InstallFvbProtocol (
/**
The function does the necessary initialization work for
- Firmware Volume Block Driver.
+ the Firmware Volume Block Driver.
**/
VOID
@@ -148,10 +145,6 @@ FvbInitialize (
mPlatformFvBaseAddress[1].FvBase = PcdGet32(PcdFlashMicrocodeFvBase);
mPlatformFvBaseAddress[1].FvSize = PcdGet32(PcdFlashMicrocodeFvSize);
- //
- // We will only continue with FVB installation if the
- // SPI is the active BIOS state
- //
{
//
// Make sure all FVB are valid and/or fix if possible
@@ -167,11 +160,20 @@ FvbInitialize (
if (!IsFvHeaderValid (BaseAddress, FvHeader)) {
BytesWritten = 0;
BytesErased = 0;
- DEBUG ((DEBUG_ERROR, "ERROR - The FV in 0x%x is invalid!\n", FvHeader));
+ DEBUG ((DEBUG_ERROR, "ERROR - The FV at 0x%x is invalid!\n", FvHeader));
+
+ //
+ // Attempt to recover the FV
+ //
FvHeader = NULL;
- Status = GetFvbInfo (BaseAddress, &FvHeader);
+ Status = GetGeneratedFvByAddress (BaseAddress, &FvHeader);
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_WARN, "ERROR - Can't recovery FV header at 0x%x. GetFvbInfo Status %r\n", BaseAddress, Status));
+ DEBUG ((
+ DEBUG_WARN | DEBUG_ERROR,
+ "ERROR - Can't recover FV header at 0x%x. GetGeneratedFvByAddress Status %r\n",
+ BaseAddress,
+ Status
+ ));
continue;
}
DEBUG ((DEBUG_INFO, "Rewriting FV header at 0x%X with static data\n", BaseAddress));
@@ -181,15 +183,15 @@ FvbInitialize (
BytesErased = (UINTN) FvHeader->BlockMap->Length;
Status = SpiFlashBlockErase( (UINTN) BaseAddress, &BytesErased);
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_WARN, "ERROR - SpiFlashBlockErase Error %r\n", Status));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - SpiFlashBlockErase Error %r\n", Status));
if (FvHeader != NULL) {
FreePool (FvHeader);
}
continue;
}
if (BytesErased != FvHeader->BlockMap->Length) {
- DEBUG ((DEBUG_WARN, "ERROR - BytesErased != FvHeader->BlockMap->Length\n"));
- DEBUG ((DEBUG_INFO, " BytesErased = 0x%X\n Length = 0x%X\n", BytesErased, FvHeader->BlockMap->Length));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - BytesErased != FvHeader->BlockMap->Length\n"));
+ DEBUG ((DEBUG_ERROR, " BytesErased = 0x%X\n Length = 0x%X\n", BytesErased, FvHeader->BlockMap->Length));
if (FvHeader != NULL) {
FreePool (FvHeader);
}
@@ -249,15 +251,15 @@ FvbInitialize (
}
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_WARN, "ERROR - SpiFlashWrite Error %r\n", Status));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - SpiFlashWrite Error %r\n", Status));
if (FvHeader != NULL) {
FreePool (FvHeader);
}
continue;
}
if (BytesWritten != ExpectedBytesWritten) {
- DEBUG ((DEBUG_WARN, "ERROR - BytesWritten != ExpectedBytesWritten\n"));
- DEBUG ((DEBUG_INFO, " BytesWritten = 0x%X\n ExpectedBytesWritten = 0x%X\n", BytesWritten, ExpectedBytesWritten));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - BytesWritten != ExpectedBytesWritten\n"));
+ DEBUG ((DEBUG_ERROR, " BytesWritten = 0x%X\n ExpectedBytesWritten = 0x%X\n", BytesWritten, ExpectedBytesWritten));
if (FvHeader != NULL) {
FreePool (FvHeader);
}
@@ -265,13 +267,13 @@ FvbInitialize (
}
Status = SpiFlashLock ();
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_WARN, "ERROR - SpiFlashLock Error %r\n", Status));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - SpiFlashLock Error %r\n", Status));
if (FvHeader != NULL) {
FreePool (FvHeader);
}
continue;
}
- DEBUG ((DEBUG_INFO, "FV Header @ 0x%X restored with static data\n", BaseAddress));
+ DEBUG ((DEBUG_ERROR, "FV Header @ 0x%X restored with static data\n", BaseAddress));
//
// Clear cache for this range.
//
@@ -294,7 +296,7 @@ FvbInitialize (
FvHeader = (EFI_FIRMWARE_VOLUME_HEADER *) (UINTN) BaseAddress;
if (!IsFvHeaderValid (BaseAddress, FvHeader)) {
- DEBUG ((DEBUG_WARN, "ERROR - The FV in 0x%x is invalid!\n", FvHeader));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - The FV in 0x%x is invalid!\n", FvHeader));
continue;
}
@@ -322,7 +324,7 @@ FvbInitialize (
FvHeader = (EFI_FIRMWARE_VOLUME_HEADER *) (UINTN) BaseAddress;
if (!IsFvHeaderValid (BaseAddress, FvHeader)) {
- DEBUG ((DEBUG_WARN, "ERROR - The FV in 0x%x is invalid!\n", FvHeader));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - The FV in 0x%x is invalid!\n", FvHeader));
continue;
}
diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h
index 7b0908b03fdc..7664670457a2 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h
@@ -1,14 +1,14 @@
/** @file
Common source definitions used in serial flash drivers
-Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
-Copyright (c) Microsoft Corporation.<BR>
-SPDX-License-Identifier: BSD-2-Clause-Patent
+ Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) Microsoft Corporation.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
**/
-#ifndef _SPI_FVB_SERVICE_COMMON_H
-#define _SPI_FVB_SERVICE_COMMON_H
+#ifndef SPI_FVB_SERVICE_COMMON_H_
+#define SPI_FVB_SERVICE_COMMON_H_
#include <Guid/EventGroup.h>
#include <Guid/FirmwareFileSystem2.h>
@@ -61,7 +61,7 @@ typedef struct {
} FVB_GLOBAL;
//
-// Fvb Protocol instance data
+// Firmware Volume Block (FVB) Protocol instance data
//
#define FVB_INSTANCE_FROM_THIS(a) CR(a, EFI_FVB_INSTANCE, FvbProtocol, FVB_INSTANCE_SIGNATURE)
@@ -81,7 +81,7 @@ typedef struct {
} FV_INFO;
//
-// Protocol APIs
+// Firmware Volume Block (FVB) Protocol APIs
//
EFI_STATUS
EFIAPI
@@ -146,8 +146,23 @@ IsFvHeaderValid (
IN CONST EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader
);
+//
+// Module Local Functions
+//
+
+/**
+ Returns an empty firmware volume for the firmware volume at the given base address.
+
+ @param[in] FvBaseAddress The base address of the firmware volume requested.
+ @param[out] FvbInfo A pointer that will be set to a buffer for the firmware volume header
+ at the given base address.
+
+ @retval EFI_SUCCESS The firmware volume was returned successfully.
+ @retval EFI_NOT_FOUND The firmware volume was not found for the given base address.
+
+**/
EFI_STATUS
-GetFvbInfo (
+GetGeneratedFvByAddress (
IN EFI_PHYSICAL_ADDRESS FvBaseAddress,
OUT EFI_FIRMWARE_VOLUME_HEADER **FvbInfo
);
diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.h b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.h
index 36af1130c8ee..512844197a4d 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.h
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.h
@@ -6,12 +6,12 @@
**/
-#ifndef _SPI_FVB_SERVICE_MM_H_
-#define _SPI_FVB_SERVICE_MM_H_
+#ifndef SPI_FVB_SERVICE_MM_H_
+#define SPI_FVB_SERVICE_MM_H_
/**
The function does the necessary initialization work for
- Firmware Volume Block Driver.
+ the Firmware Volume Block Driver.
**/
VOID
--
2.41.0.windows.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/SpiFvbService: Non-functional cleanup
2023-06-30 2:32 [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/SpiFvbService: Non-functional cleanup Michael Kubacki
@ 2023-06-30 18:57 ` Ashraf Ali S
2023-06-30 20:21 ` Isaac Oram
[not found] ` <176D896904216960.17093@groups.io>
0 siblings, 2 replies; 4+ messages in thread
From: Ashraf Ali S @ 2023-06-30 18:57 UTC (permalink / raw)
To: devel@edk2.groups.io, mikuback@linux.microsoft.com
Cc: Ni, Ray, Chaganty, Rangasai V, Oram, Isaac W
Reviewed-by: Ashraf Ali S <ashraf.ali.s@intel.com>
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
Sent: Friday, June 30, 2023 8:03 AM
To: devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; S, Ashraf Ali <ashraf.ali.s@intel.com>
Subject: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/SpiFvbService: Non-functional cleanup
From: Michael Kubacki <michael.kubacki@microsoft.com>
During a review of this driver a number of improvements were noted such as strengthening function input validation, checking return values, making debug print error levels consistent in certain code blocks, etc.
These type of changes are made with no explicit change to driver functionality.
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Ashraf Ali S <ashraf.ali.s@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c | 35 ++-
Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c | 248 ++++++++++++--------
Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c | 54 +++--
Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h | 31 ++-
Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.h | 6 +-
5 files changed, 239 insertions(+), 135 deletions(-)
diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c
index ab1cb2ef1622..ebbd9edaada3 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.
+++ c
@@ -2,16 +2,17 @@
Defines data structure that is the volume header found.
These data is intent to decouple FVB driver with FV header.
-Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> -Copyright (c) Microsoft Corporation.<BR>
-SPDX-License-Identifier: BSD-2-Clause-Patent
+ Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) Microsoft Corporation.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
**/
#include "SpiFvbServiceCommon.h"
-#define FIRMWARE_BLOCK_SIZE 0x10000
+#define FIRMWARE_BLOCK_SIZE SIZE_64KB
#define FVB_MEDIA_BLOCK_SIZE FIRMWARE_BLOCK_SIZE
+
typedef struct {
EFI_PHYSICAL_ADDRESS BaseAddress;
EFI_FIRMWARE_VOLUME_HEADER FvbInfo;
@@ -19,7 +20,7 @@ typedef struct {
} EFI_FVB2_MEDIA_INFO;
/**
- Returns FVB media information for NV variable storage.
+ Returns FVB media information for a firmware volume.
@return FvbMediaInfo A pointer to an instance of FVB media info produced by this function.
The buffer is allocated internally to this function and it is the caller's @@ -100,8 +101,21 @@ FVB_MEDIA_INFO_GENERATOR mFvbMediaInfoGenerators[] = {
GenerateNvStorageFvbMediaInfo
};
+/**
+ Returns an empty firmware volume for the firmware volume at the given base address.
+
+ @param[in] FvBaseAddress The base address of the firmware volume requested.
+ @param[out] FvbInfo A pointer that will be set to a buffer for the firmware volume header
+ at the given base address. The buffer is a pool allocation made in this function.
+
+ @retval EFI_SUCCESS The firmware volume was returned successfully.
+ @retval EFI_INVALID_PARAMETER The FvbInfo pointer argument is NULL.
+ @retval EFI_NOT_FOUND The firmware volume was not found for the given base address.
+ @retval EFI_OUT_OF_RESOURCES Insufficient memory to allocate a buffer to the hold the firmware volume.
+
+**/
EFI_STATUS
-GetFvbInfo (
+GetGeneratedFvByAddress (
IN EFI_PHYSICAL_ADDRESS FvBaseAddress,
OUT EFI_FIRMWARE_VOLUME_HEADER **FvbInfo
)
@@ -111,11 +125,19 @@ GetFvbInfo (
UINTN Index;
EFI_FIRMWARE_VOLUME_HEADER *FvHeader;
+ if (FvbInfo == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
for (Index = 0; Index < ARRAY_SIZE (mFvbMediaInfoGenerators); Index++) {
Status = mFvbMediaInfoGenerators[Index](&FvbMediaInfo);
ASSERT_EFI_ERROR (Status);
if (!EFI_ERROR (Status) && (FvbMediaInfo.BaseAddress == FvBaseAddress)) {
FvHeader = AllocateCopyPool (FvbMediaInfo.FvbInfo.HeaderLength, &FvbMediaInfo.FvbInfo);
+ if (FvHeader == NULL) {
+ ASSERT (FvHeader != NULL);
+ return EFI_OUT_OF_RESOURCES;
+ }
//
// Update the checksum value of FV header.
@@ -136,5 +158,6 @@ GetFvbInfo (
return EFI_SUCCESS;
}
}
+
return EFI_NOT_FOUND;
}
diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c
index fcdc715263f2..e21113682f4f 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbSe
+++ rviceCommon.c
@@ -2,9 +2,9 @@
Common driver source for several Serial Flash devices
which are compliant with the Intel(R) Serial Flash Interface Compatibility Specification.
-Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> -Copyright (c) Microsoft Corporation.<BR>
-SPDX-License-Identifier: BSD-2-Clause-Patent
+ Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) Microsoft Corporation.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -92,13 +92,19 @@ EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL mFvbProtocolTemplate = {
@param[in] FvbInstance The pointer to the EFI_FVB_INSTANCE.
@return Attributes of the FV identified by FvbInstance.
+ Zero is returned if the FvbInstance pointer is NULL.
**/
EFI_FVB_ATTRIBUTES_2
FvbGetVolumeAttributes (
- IN EFI_FVB_INSTANCE *FvbInstance
+ IN CONST EFI_FVB_INSTANCE *FvbInstance
)
{
+ if (FvbInstance == NULL) {
+ ASSERT (FvbInstance != NULL);
+ return 0;
+ }
+
return FvbInstance->FvHeader.Attributes; }
@@ -109,24 +115,25 @@ FvbGetVolumeAttributes (
@param[in] FvbInstance The pointer to the EFI_FVB_INSTANCE.
@param[in] Lba The logical block address
@param[out] LbaAddress On output, contains the physical starting address
- of the Lba
- @param[out] LbaLength On output, contains the length of the block
+ of the Lba. This pointer is optional and may be NULL.
+ @param[out] LbaLength On output, contains the length of the block.
+ This pointer is optional and may be NULL.
@param[out] NumOfBlocks A pointer to a caller allocated UINTN in which the
number of consecutive blocks starting with Lba is
returned. All blocks in this range have a size of
- BlockSize
+ BlockSize. This pointer is optional and may be NULL.
- @retval EFI_SUCCESS Successfully returns
- @retval EFI_INVALID_PARAMETER Instance not found
+ @retval EFI_SUCCESS Successfully returns
+ @retval EFI_INVALID_PARAMETER FvbInstance is NULL.
**/
EFI_STATUS
FvbGetLbaAddress (
- IN EFI_FVB_INSTANCE *FvbInstance,
+ IN CONST EFI_FVB_INSTANCE *FvbInstance,
IN EFI_LBA Lba,
- OUT UINTN *LbaAddress,
- OUT UINTN *LbaLength,
- OUT UINTN *NumOfBlocks
+ OUT UINTN *LbaAddress OPTIONAL,
+ OUT UINTN *LbaLength OPTIONAL,
+ OUT UINTN *NumOfBlocks OPTIONAL
)
{
UINT32 NumBlocks;
@@ -134,10 +141,15 @@ FvbGetLbaAddress (
UINTN Offset;
EFI_LBA StartLba;
EFI_LBA NextLba;
- EFI_FV_BLOCK_MAP_ENTRY *BlockMap;
+ CONST EFI_FV_BLOCK_MAP_ENTRY *BlockMap;
StartLba = 0;
Offset = 0;
+
+ if (FvbInstance == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
BlockMap = &(FvbInstance->FvHeader.BlockMap[0]);
//
@@ -158,15 +170,15 @@ FvbGetLbaAddress (
//
if (Lba >= StartLba && Lba < NextLba) {
Offset = Offset + (UINTN)MultU64x32((Lba - StartLba), BlockLength);
- if (LbaAddress ) {
+ if (LbaAddress != NULL) {
*LbaAddress = FvbInstance->FvBase + Offset;
}
- if (LbaLength ) {
+ if (LbaLength != NULL) {
*LbaLength = BlockLength;
}
- if (NumOfBlocks ) {
+ if (NumOfBlocks != NULL) {
*NumOfBlocks = (UINTN)(NextLba - Lba);
}
return EFI_SUCCESS;
@@ -190,7 +202,6 @@ FvbGetLbaAddress (
@param[in] Buffer Pointer to a caller allocated buffer that will be
used to hold the data read
-
@retval EFI_SUCCESS The firmware volume was read successfully and
contents are in Buffer
@retval EFI_BAD_BUFFER_SIZE Read attempted across a LBA boundary. On output,
@@ -199,16 +210,16 @@ FvbGetLbaAddress (
@retval EFI_ACCESS_DENIED The firmware volume is in the ReadDisabled state
@retval EFI_DEVICE_ERROR The block device is not functioning correctly and
could not be read
- @retval EFI_INVALID_PARAMETER Instance not found, or NumBytes, Buffer are NULL
+ @retval EFI_INVALID_PARAMETER FvbInstance, NumBytes, and/or Buffer are NULL
**/
EFI_STATUS
FvbReadBlock (
- IN EFI_FVB_INSTANCE *FvbInstance,
- IN EFI_LBA Lba,
- IN UINTN BlockOffset,
- IN OUT UINTN *NumBytes,
- IN UINT8 *Buffer
+ IN CONST EFI_FVB_INSTANCE *FvbInstance,
+ IN EFI_LBA Lba,
+ IN UINTN BlockOffset,
+ IN OUT UINTN *NumBytes,
+ IN UINT8 *Buffer
)
{
EFI_FVB_ATTRIBUTES_2 Attributes;
@@ -217,9 +228,10 @@ FvbReadBlock (
EFI_STATUS Status;
BOOLEAN BadBufferSize = FALSE;
- if ((NumBytes == NULL) || (Buffer == NULL)) {
+ if ((FvbInstance == NULL) || (NumBytes == NULL) || (Buffer == NULL))
+ {
return EFI_INVALID_PARAMETER;
}
+
if (*NumBytes == 0) {
return EFI_INVALID_PARAMETER;
}
@@ -269,6 +281,7 @@ FvbReadBlock (
of bytes actually written
@param[in] Buffer Pointer to a caller allocated buffer that contains
the source for the write
+
@retval EFI_SUCCESS The firmware volume was written successfully
@retval EFI_BAD_BUFFER_SIZE Write attempted across a LBA boundary. On output,
NumBytes contains the total number of bytes @@ -276,16 +289,16 @@ FvbReadBlock (
@retval EFI_ACCESS_DENIED The firmware volume is in the WriteDisabled state
@retval EFI_DEVICE_ERROR The block device is not functioning correctly and
could not be written
- @retval EFI_INVALID_PARAMETER Instance not found, or NumBytes, Buffer are NULL
+ @retval EFI_INVALID_PARAMETER FvbInstance, NumBytes, and/or Buffer are NULL
**/
EFI_STATUS
FvbWriteBlock (
- IN EFI_FVB_INSTANCE *FvbInstance,
- IN EFI_LBA Lba,
- IN UINTN BlockOffset,
- IN OUT UINTN *NumBytes,
- IN UINT8 *Buffer
+ IN CONST EFI_FVB_INSTANCE *FvbInstance,
+ IN EFI_LBA Lba,
+ IN UINTN BlockOffset,
+ IN OUT UINTN *NumBytes,
+ IN UINT8 *Buffer
)
{
EFI_FVB_ATTRIBUTES_2 Attributes;
@@ -294,7 +307,7 @@ FvbWriteBlock (
EFI_STATUS Status;
BOOLEAN BadBufferSize = FALSE;
- if ((NumBytes == NULL) || (Buffer == NULL)) {
+ if ((FvbInstance == NULL) || (NumBytes == NULL) || (Buffer == NULL))
+ {
return EFI_INVALID_PARAMETER;
}
if (*NumBytes == 0) {
@@ -350,8 +363,6 @@ FvbWriteBlock (
}
}
-
-
/**
Erases and initializes a firmware volume block.
@@ -363,13 +374,13 @@ FvbWriteBlock (
@retval EFI_DEVICE_ERROR The block device is not functioning correctly and
could not be written. Firmware device may have been
partially erased
- @retval EFI_INVALID_PARAMETER Instance not found
+ @retval EFI_INVALID_PARAMETER FvbInstance is NULL
**/
EFI_STATUS
FvbEraseBlock (
- IN EFI_FVB_INSTANCE *FvbInstance,
- IN EFI_LBA Lba
+ IN CONST EFI_FVB_INSTANCE *FvbInstance,
+ IN EFI_LBA Lba
)
{
@@ -378,6 +389,10 @@ FvbEraseBlock (
UINTN LbaLength;
EFI_STATUS Status;
+ if (FvbInstance == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
//
// Check if the FV is write enabled
//
@@ -422,9 +437,9 @@ FvbEraseBlock (
@retval EFI_SUCCESS Successfully returns
@retval EFI_ACCESS_DENIED The volume setting is locked and cannot be modified
- @retval EFI_INVALID_PARAMETER Instance not found, or The attributes requested are
- in conflict with the capabilities as declared in the
- firmware volume header
+ @retval EFI_INVALID_PARAMETER FvbInstance or Attributes is NULL.
+ Or the attributes requested are in conflict with the
+ capabilities as declared in the firmware volume header.
**/
EFI_STATUS
@@ -439,6 +454,10 @@ FvbSetVolumeAttributes (
UINT32 Capabilities;
UINT32 OldStatus, NewStatus;
+ if ((FvbInstance == NULL) || (Attributes == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
AttribPtr = (EFI_FVB_ATTRIBUTES_2 *) &(FvbInstance->FvHeader.Attributes);
OldAttributes = *AttribPtr;
Capabilities = OldAttributes & EFI_FVB2_CAPABILITIES; @@ -554,6 +573,7 @@ GetVariableFvInfo (
ASSERT ((BaseAddress != NULL) && (Length != NULL));
return;
}
+
*BaseAddress = 0;
*Length = 0;
@@ -631,8 +651,9 @@ GetVariableFvInfo (
@param[in] FvHeader A pointer to a firmware volume header
- @retval TRUE The firmware volume is consistent
- @retval FALSE The firmware volume has corrupted.
+ @retval TRUE The firmware volume is consistent.
+ @retval FALSE The firmware volume has corrupted or an invalid firmware
+ volume was provided.
**/
BOOLEAN
@@ -644,6 +665,11 @@ IsFvHeaderValid (
EFI_PHYSICAL_ADDRESS NvStorageFvBaseAddress;
UINT32 NvStorageSize;
+ if (FvHeader == NULL) {
+ ASSERT (FvHeader != NULL);
+ return FALSE;
+ }
+
GetVariableFvInfo (&NvStorageFvBaseAddress, &NvStorageSize);
if (FvBase == NvStorageFvBaseAddress) { @@ -679,18 +705,23 @@ IsFvHeaderValid (
@param[in] This A pointer to EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL.
@param[out] Address Output buffer containing the address.
- retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
**/
EFI_STATUS
EFIAPI
FvbProtocolGetPhysicalAddress (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- OUT EFI_PHYSICAL_ADDRESS *Address
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ OUT EFI_PHYSICAL_ADDRESS *Address
)
{
EFI_FVB_INSTANCE *FvbInstance;
+ if ((This == NULL) || (Address == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
*Address = FvbInstance->FvBase;
@@ -710,28 +741,34 @@ FvbProtocolGetPhysicalAddress (
returned. All blocks in this range have a size of
BlockSize
- @retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
**/
EFI_STATUS
EFIAPI
FvbProtocolGetBlockSize (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- IN EFI_LBA Lba,
- OUT UINTN *BlockSize,
- OUT UINTN *NumOfBlocks
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ IN EFI_LBA Lba,
+ OUT UINTN *BlockSize,
+ OUT UINTN *NumOfBlocks
)
{
EFI_FVB_INSTANCE *FvbInstance;
+ if ((This == NULL) || (BlockSize == NULL) || (NumOfBlocks == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
- DEBUG((DEBUG_INFO,
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolGetBlockSize: Lba: 0x%lx BlockSize: 0x%x NumOfBlocks: 0x%x\n",
Lba,
- BlockSize,
- NumOfBlocks)
- );
+ *BlockSize,
+ *NumOfBlocks
+ ));
return FvbGetLbaAddress (
FvbInstance,
@@ -748,27 +785,33 @@ FvbProtocolGetBlockSize (
@param[in] This Calling context
@param[out] Attributes Output buffer which contains attributes
- @retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
**/
EFI_STATUS
EFIAPI
FvbProtocolGetAttributes (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- OUT EFI_FVB_ATTRIBUTES_2 *Attributes
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ OUT EFI_FVB_ATTRIBUTES_2 *Attributes
)
{
EFI_FVB_INSTANCE *FvbInstance;
+ if ((This == NULL) || (Attributes == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
*Attributes = FvbGetVolumeAttributes (FvbInstance);
- DEBUG ((DEBUG_INFO,
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolGetAttributes: This: 0x%x Attributes: 0x%x\n",
This,
- *Attributes)
- );
+ *Attributes
+ ));
return EFI_SUCCESS;
}
@@ -785,28 +828,34 @@ FvbProtocolGetAttributes ( EFI_STATUS EFIAPI FvbProtocolSetAttributes (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- IN OUT EFI_FVB_ATTRIBUTES_2 *Attributes
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ IN OUT EFI_FVB_ATTRIBUTES_2 *Attributes
)
{
EFI_STATUS Status;
EFI_FVB_INSTANCE *FvbInstance;
- DEBUG((DEBUG_INFO,
+ if ((This == NULL) || (Attributes == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolSetAttributes: Before SET - This: 0x%x Attributes: 0x%x\n",
This,
- *Attributes)
- );
+ *Attributes
+ ));
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
Status = FvbSetVolumeAttributes (FvbInstance, Attributes);
- DEBUG((DEBUG_INFO,
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolSetAttributes: After SET - This: 0x%x Attributes: 0x%x\n",
This,
- *Attributes)
- );
+ *Attributes
+ ));
return Status;
}
@@ -823,17 +872,18 @@ FvbProtocolSetAttributes (
@param[in] ... Starting LBA followed by Number of Lba to erase.
a -1 to terminate the list.
- @retval EFI_SUCCESS The erase request was successfully completed
- @retval EFI_ACCESS_DENIED The firmware volume is in the WriteDisabled state
- @retval EFI_DEVICE_ERROR The block device is not functioning correctly and
- could not be written. Firmware device may have been
- partially erased
+ @retval EFI_SUCCESS The erase request was successfully completed
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
+ @retval EFI_ACCESS_DENIED The firmware volume is in the WriteDisabled state
+ @retval EFI_DEVICE_ERROR The block device is not functioning correctly and
+ could not be written. Firmware device may have been
+ partially erased
**/
EFI_STATUS
EFIAPI
FvbProtocolEraseBlocks (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
...
)
{
@@ -846,6 +896,10 @@ FvbProtocolEraseBlocks (
DEBUG((DEBUG_INFO, "FvbProtocolEraseBlocks: \n"));
+ if (This == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
NumOfBlocks = FvbInstance->NumOfBlocks; @@ -923,30 +977,35 @@ FvbProtocolEraseBlocks (
@retval EFI_ACCESS_DENIED The firmware volume is in the WriteDisabled state
@retval EFI_DEVICE_ERROR The block device is not functioning correctly and
could not be written
- @retval EFI_INVALID_PARAMETER NumBytes or Buffer are NULL
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
**/
EFI_STATUS
EFIAPI
FvbProtocolWrite (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- IN EFI_LBA Lba,
- IN UINTN Offset,
- IN OUT UINTN *NumBytes,
- IN UINT8 *Buffer
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ IN EFI_LBA Lba,
+ IN UINTN Offset,
+ IN OUT UINTN *NumBytes,
+ IN UINT8 *Buffer
)
{
EFI_FVB_INSTANCE *FvbInstance;
+ if ((This == NULL) || (NumBytes == NULL) || (Buffer == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
- DEBUG((DEBUG_INFO,
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolWrite: Lba: 0x%lx Offset: 0x%x NumBytes: 0x%x, Buffer: 0x%x\n",
Lba,
Offset,
*NumBytes,
- Buffer)
- );
+ Buffer
+ ));
return FvbWriteBlock (FvbInstance, Lba, Offset, NumBytes, Buffer); } @@ -974,31 +1033,36 @@ FvbProtocolWrite (
@retval EFI_ACCESS_DENIED The firmware volume is in the ReadDisabled state
@retval EFI_DEVICE_ERROR The block device is not functioning correctly and
could not be read
- @retval EFI_INVALID_PARAMETER NumBytes or Buffer are NULL
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
**/
EFI_STATUS
EFIAPI
FvbProtocolRead (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- IN EFI_LBA Lba,
- IN UINTN Offset,
- IN OUT UINTN *NumBytes,
- OUT UINT8 *Buffer
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ IN EFI_LBA Lba,
+ IN UINTN Offset,
+ IN OUT UINTN *NumBytes,
+ OUT UINT8 *Buffer
)
{
EFI_FVB_INSTANCE *FvbInstance;
EFI_STATUS Status;
+ if ((This == NULL) || (NumBytes == NULL) || (Buffer == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
Status = FvbReadBlock (FvbInstance, Lba, Offset, NumBytes, Buffer);
- DEBUG((DEBUG_INFO,
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolRead: Lba: 0x%lx Offset: 0x%x NumBytes: 0x%x, Buffer: 0x%x\n",
Lba,
Offset,
*NumBytes,
- Buffer)
- );
+ Buffer
+ ));
return Status;
}
diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c
index f9543c5ba677..5d7591480319 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbSe
+++ rviceMm.c
@@ -15,14 +15,11 @@
#include <Guid/VariableFormat.h>
/**
- The function installs EFI_FIRMWARE_VOLUME_BLOCK protocol
- for each FV in the system.
+ The function installs the given EFI_FIRMWARE_VOLUME_BLOCK protocol instance.
@param[in] FvbInstance The pointer to a FW volume instance structure,
which contains the information about one FV.
- @retval VOID
-
**/
VOID
InstallFvbProtocol (
@@ -33,8 +30,8 @@ InstallFvbProtocol (
EFI_STATUS Status;
EFI_HANDLE FvbHandle;
- ASSERT (FvbInstance != NULL);
if (FvbInstance == NULL) {
+ ASSERT (FvbInstance != NULL);
return;
}
@@ -48,14 +45,14 @@ InstallFvbProtocol (
//
// Set up the devicepath
//
- DEBUG ((DEBUG_INFO, "FwBlockService.c: Setting up DevicePath for 0x%lx:\n", FvbInstance->FvBase));
+ DEBUG ((DEBUG_INFO, "SpiFvbServiceSmm.c: Setting up DevicePath for
+ 0x%lx:\n", FvbInstance->FvBase));
if (FvHeader->ExtHeaderOffset == 0) {
//
// FV does not contains extension header, then produce MEMMAP_DEVICE_PATH
//
FvbInstance->DevicePath = (EFI_DEVICE_PATH_PROTOCOL *) AllocateRuntimeCopyPool (sizeof (FV_MEMMAP_DEVICE_PATH), &mFvMemmapDevicePathTemplate);
if (FvbInstance->DevicePath == NULL) {
- DEBUG ((DEBUG_INFO, "SpiFvbServiceSmm.c: Memory allocation for MEMMAP_DEVICE_PATH failed\n"));
+ DEBUG ((DEBUG_ERROR, "SpiFvbServiceSmm.c: Memory allocation for
+ MEMMAP_DEVICE_PATH failed\n"));
return;
}
((FV_MEMMAP_DEVICE_PATH *) FvbInstance->DevicePath)->MemMapDevPath.StartingAddress = FvbInstance->FvBase; @@ -63,7 +60,7 @@ InstallFvbProtocol (
} else {
FvbInstance->DevicePath = (EFI_DEVICE_PATH_PROTOCOL *) AllocateRuntimeCopyPool (sizeof (FV_PIWG_DEVICE_PATH), &mFvPIWGDevicePathTemplate);
if (FvbInstance->DevicePath == NULL) {
- DEBUG ((DEBUG_INFO, "SpiFvbServiceSmm.c: Memory allocation for FV_PIWG_DEVICE_PATH failed\n"));
+ DEBUG ((DEBUG_ERROR, "SpiFvbServiceSmm.c: Memory allocation for
+ FV_PIWG_DEVICE_PATH failed\n"));
return;
}
CopyGuid (
@@ -96,7 +93,7 @@ InstallFvbProtocol (
/**
The function does the necessary initialization work for
- Firmware Volume Block Driver.
+ the Firmware Volume Block Driver.
**/
VOID
@@ -148,10 +145,6 @@ FvbInitialize (
mPlatformFvBaseAddress[1].FvBase = PcdGet32(PcdFlashMicrocodeFvBase);
mPlatformFvBaseAddress[1].FvSize = PcdGet32(PcdFlashMicrocodeFvSize);
- //
- // We will only continue with FVB installation if the
- // SPI is the active BIOS state
- //
{
//
// Make sure all FVB are valid and/or fix if possible @@ -167,11 +160,20 @@ FvbInitialize (
if (!IsFvHeaderValid (BaseAddress, FvHeader)) {
BytesWritten = 0;
BytesErased = 0;
- DEBUG ((DEBUG_ERROR, "ERROR - The FV in 0x%x is invalid!\n", FvHeader));
+ DEBUG ((DEBUG_ERROR, "ERROR - The FV at 0x%x is invalid!\n",
+ FvHeader));
+
+ //
+ // Attempt to recover the FV
+ //
FvHeader = NULL;
- Status = GetFvbInfo (BaseAddress, &FvHeader);
+ Status = GetGeneratedFvByAddress (BaseAddress, &FvHeader);
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_WARN, "ERROR - Can't recovery FV header at 0x%x. GetFvbInfo Status %r\n", BaseAddress, Status));
+ DEBUG ((
+ DEBUG_WARN | DEBUG_ERROR,
+ "ERROR - Can't recover FV header at 0x%x. GetGeneratedFvByAddress Status %r\n",
+ BaseAddress,
+ Status
+ ));
continue;
}
DEBUG ((DEBUG_INFO, "Rewriting FV header at 0x%X with static data\n", BaseAddress)); @@ -181,15 +183,15 @@ FvbInitialize (
BytesErased = (UINTN) FvHeader->BlockMap->Length;
Status = SpiFlashBlockErase( (UINTN) BaseAddress, &BytesErased);
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_WARN, "ERROR - SpiFlashBlockErase Error %r\n", Status));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - SpiFlashBlockErase
+ Error %r\n", Status));
if (FvHeader != NULL) {
FreePool (FvHeader);
}
continue;
}
if (BytesErased != FvHeader->BlockMap->Length) {
- DEBUG ((DEBUG_WARN, "ERROR - BytesErased != FvHeader->BlockMap->Length\n"));
- DEBUG ((DEBUG_INFO, " BytesErased = 0x%X\n Length = 0x%X\n", BytesErased, FvHeader->BlockMap->Length));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - BytesErased != FvHeader->BlockMap->Length\n"));
+ DEBUG ((DEBUG_ERROR, " BytesErased = 0x%X\n Length = 0x%X\n",
+ BytesErased, FvHeader->BlockMap->Length));
if (FvHeader != NULL) {
FreePool (FvHeader);
}
@@ -249,15 +251,15 @@ FvbInitialize (
}
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_WARN, "ERROR - SpiFlashWrite Error %r\n", Status));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - SpiFlashWrite
+ Error %r\n", Status));
if (FvHeader != NULL) {
FreePool (FvHeader);
}
continue;
}
if (BytesWritten != ExpectedBytesWritten) {
- DEBUG ((DEBUG_WARN, "ERROR - BytesWritten != ExpectedBytesWritten\n"));
- DEBUG ((DEBUG_INFO, " BytesWritten = 0x%X\n ExpectedBytesWritten = 0x%X\n", BytesWritten, ExpectedBytesWritten));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - BytesWritten != ExpectedBytesWritten\n"));
+ DEBUG ((DEBUG_ERROR, " BytesWritten = 0x%X\n
+ ExpectedBytesWritten = 0x%X\n", BytesWritten, ExpectedBytesWritten));
if (FvHeader != NULL) {
FreePool (FvHeader);
}
@@ -265,13 +267,13 @@ FvbInitialize (
}
Status = SpiFlashLock ();
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_WARN, "ERROR - SpiFlashLock Error %r\n", Status));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - SpiFlashLock Error
+ %r\n", Status));
if (FvHeader != NULL) {
FreePool (FvHeader);
}
continue;
}
- DEBUG ((DEBUG_INFO, "FV Header @ 0x%X restored with static data\n", BaseAddress));
+ DEBUG ((DEBUG_ERROR, "FV Header @ 0x%X restored with static
+ data\n", BaseAddress));
//
// Clear cache for this range.
//
@@ -294,7 +296,7 @@ FvbInitialize (
FvHeader = (EFI_FIRMWARE_VOLUME_HEADER *) (UINTN) BaseAddress;
if (!IsFvHeaderValid (BaseAddress, FvHeader)) {
- DEBUG ((DEBUG_WARN, "ERROR - The FV in 0x%x is invalid!\n", FvHeader));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - The FV in 0x%x is
+ invalid!\n", FvHeader));
continue;
}
@@ -322,7 +324,7 @@ FvbInitialize (
FvHeader = (EFI_FIRMWARE_VOLUME_HEADER *) (UINTN) BaseAddress;
if (!IsFvHeaderValid (BaseAddress, FvHeader)) {
- DEBUG ((DEBUG_WARN, "ERROR - The FV in 0x%x is invalid!\n", FvHeader));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - The FV in 0x%x is
+ invalid!\n", FvHeader));
continue;
}
diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h
index 7b0908b03fdc..7664670457a2 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbSe
+++ rviceCommon.h
@@ -1,14 +1,14 @@
/** @file
Common source definitions used in serial flash drivers
-Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> -Copyright (c) Microsoft Corporation.<BR>
-SPDX-License-Identifier: BSD-2-Clause-Patent
+ Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) Microsoft Corporation.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
**/
-#ifndef _SPI_FVB_SERVICE_COMMON_H
-#define _SPI_FVB_SERVICE_COMMON_H
+#ifndef SPI_FVB_SERVICE_COMMON_H_
+#define SPI_FVB_SERVICE_COMMON_H_
#include <Guid/EventGroup.h>
#include <Guid/FirmwareFileSystem2.h>
@@ -61,7 +61,7 @@ typedef struct {
} FVB_GLOBAL;
//
-// Fvb Protocol instance data
+// Firmware Volume Block (FVB) Protocol instance data
//
#define FVB_INSTANCE_FROM_THIS(a) CR(a, EFI_FVB_INSTANCE, FvbProtocol, FVB_INSTANCE_SIGNATURE)
@@ -81,7 +81,7 @@ typedef struct {
} FV_INFO;
//
-// Protocol APIs
+// Firmware Volume Block (FVB) Protocol APIs
//
EFI_STATUS
EFIAPI
@@ -146,8 +146,23 @@ IsFvHeaderValid (
IN CONST EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader
);
+//
+// Module Local Functions
+//
+
+/**
+ Returns an empty firmware volume for the firmware volume at the given base address.
+
+ @param[in] FvBaseAddress The base address of the firmware volume requested.
+ @param[out] FvbInfo A pointer that will be set to a buffer for the firmware volume header
+ at the given base address.
+
+ @retval EFI_SUCCESS The firmware volume was returned successfully.
+ @retval EFI_NOT_FOUND The firmware volume was not found for the given base address.
+
+**/
EFI_STATUS
-GetFvbInfo (
+GetGeneratedFvByAddress (
IN EFI_PHYSICAL_ADDRESS FvBaseAddress,
OUT EFI_FIRMWARE_VOLUME_HEADER **FvbInfo
);
diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.h b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.h
index 36af1130c8ee..512844197a4d 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.h
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbSe
+++ rviceMm.h
@@ -6,12 +6,12 @@
**/
-#ifndef _SPI_FVB_SERVICE_MM_H_
-#define _SPI_FVB_SERVICE_MM_H_
+#ifndef SPI_FVB_SERVICE_MM_H_
+#define SPI_FVB_SERVICE_MM_H_
/**
The function does the necessary initialization work for
- Firmware Volume Block Driver.
+ the Firmware Volume Block Driver.
**/
VOID
--
2.41.0.windows.1
-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106553): https://edk2.groups.io/g/devel/message/106553
Mute This Topic: https://groups.io/mt/99866031/6226280
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [ashraf.ali.s@intel.com]
-=-=-=-=-=-=
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/SpiFvbService: Non-functional cleanup
2023-06-30 18:57 ` [edk2-devel] " Ashraf Ali S
@ 2023-06-30 20:21 ` Isaac Oram
[not found] ` <176D896904216960.17093@groups.io>
1 sibling, 0 replies; 4+ messages in thread
From: Isaac Oram @ 2023-06-30 20:21 UTC (permalink / raw)
To: S, Ashraf Ali, devel@edk2.groups.io, mikuback@linux.microsoft.com
Cc: Ni, Ray, Chaganty, Rangasai V
Reviewed-by: Isaac Oram <isaac.w.oram@intel.com>
Mixing DEBUG_WARN | DEBUG_ERROR confuses me a bit. I would prefer simpler definitions were errors are "something is wrong" and warnings are "something might be wrong".
But it is a pretty minor nit and cleanup is good, so don't worry about changing it.
Regards,
Isaac
-----Original Message-----
From: S, Ashraf Ali <ashraf.ali.s@intel.com>
Sent: Friday, June 30, 2023 11:57 AM
To: devel@edk2.groups.io; mikuback@linux.microsoft.com
Cc: Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>
Subject: RE: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/SpiFvbService: Non-functional cleanup
Reviewed-by: Ashraf Ali S <ashraf.ali.s@intel.com>
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
Sent: Friday, June 30, 2023 8:03 AM
To: devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; S, Ashraf Ali <ashraf.ali.s@intel.com>
Subject: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/SpiFvbService: Non-functional cleanup
From: Michael Kubacki <michael.kubacki@microsoft.com>
During a review of this driver a number of improvements were noted such as strengthening function input validation, checking return values, making debug print error levels consistent in certain code blocks, etc.
These type of changes are made with no explicit change to driver functionality.
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Ashraf Ali S <ashraf.ali.s@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c | 35 ++-
Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c | 248 ++++++++++++--------
Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c | 54 +++--
Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h | 31 ++-
Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.h | 6 +-
5 files changed, 239 insertions(+), 135 deletions(-)
diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c
index ab1cb2ef1622..ebbd9edaada3 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.
+++ c
@@ -2,16 +2,17 @@
Defines data structure that is the volume header found.
These data is intent to decouple FVB driver with FV header.
-Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> -Copyright (c) Microsoft Corporation.<BR>
-SPDX-License-Identifier: BSD-2-Clause-Patent
+ Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) Microsoft Corporation.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
**/
#include "SpiFvbServiceCommon.h"
-#define FIRMWARE_BLOCK_SIZE 0x10000
+#define FIRMWARE_BLOCK_SIZE SIZE_64KB
#define FVB_MEDIA_BLOCK_SIZE FIRMWARE_BLOCK_SIZE
+
typedef struct {
EFI_PHYSICAL_ADDRESS BaseAddress;
EFI_FIRMWARE_VOLUME_HEADER FvbInfo;
@@ -19,7 +20,7 @@ typedef struct {
} EFI_FVB2_MEDIA_INFO;
/**
- Returns FVB media information for NV variable storage.
+ Returns FVB media information for a firmware volume.
@return FvbMediaInfo A pointer to an instance of FVB media info produced by this function.
The buffer is allocated internally to this function and it is the caller's @@ -100,8 +101,21 @@ FVB_MEDIA_INFO_GENERATOR mFvbMediaInfoGenerators[] = {
GenerateNvStorageFvbMediaInfo
};
+/**
+ Returns an empty firmware volume for the firmware volume at the given base address.
+
+ @param[in] FvBaseAddress The base address of the firmware volume requested.
+ @param[out] FvbInfo A pointer that will be set to a buffer for the firmware volume header
+ at the given base address. The buffer is a pool allocation made in this function.
+
+ @retval EFI_SUCCESS The firmware volume was returned successfully.
+ @retval EFI_INVALID_PARAMETER The FvbInfo pointer argument is NULL.
+ @retval EFI_NOT_FOUND The firmware volume was not found for the given base address.
+ @retval EFI_OUT_OF_RESOURCES Insufficient memory to allocate a buffer to the hold the firmware volume.
+
+**/
EFI_STATUS
-GetFvbInfo (
+GetGeneratedFvByAddress (
IN EFI_PHYSICAL_ADDRESS FvBaseAddress,
OUT EFI_FIRMWARE_VOLUME_HEADER **FvbInfo
)
@@ -111,11 +125,19 @@ GetFvbInfo (
UINTN Index;
EFI_FIRMWARE_VOLUME_HEADER *FvHeader;
+ if (FvbInfo == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
for (Index = 0; Index < ARRAY_SIZE (mFvbMediaInfoGenerators); Index++) {
Status = mFvbMediaInfoGenerators[Index](&FvbMediaInfo);
ASSERT_EFI_ERROR (Status);
if (!EFI_ERROR (Status) && (FvbMediaInfo.BaseAddress == FvBaseAddress)) {
FvHeader = AllocateCopyPool (FvbMediaInfo.FvbInfo.HeaderLength, &FvbMediaInfo.FvbInfo);
+ if (FvHeader == NULL) {
+ ASSERT (FvHeader != NULL);
+ return EFI_OUT_OF_RESOURCES;
+ }
//
// Update the checksum value of FV header.
@@ -136,5 +158,6 @@ GetFvbInfo (
return EFI_SUCCESS;
}
}
+
return EFI_NOT_FOUND;
}
diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c
index fcdc715263f2..e21113682f4f 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbSe
+++ rviceCommon.c
@@ -2,9 +2,9 @@
Common driver source for several Serial Flash devices
which are compliant with the Intel(R) Serial Flash Interface Compatibility Specification.
-Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> -Copyright (c) Microsoft Corporation.<BR>
-SPDX-License-Identifier: BSD-2-Clause-Patent
+ Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) Microsoft Corporation.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -92,13 +92,19 @@ EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL mFvbProtocolTemplate = {
@param[in] FvbInstance The pointer to the EFI_FVB_INSTANCE.
@return Attributes of the FV identified by FvbInstance.
+ Zero is returned if the FvbInstance pointer is NULL.
**/
EFI_FVB_ATTRIBUTES_2
FvbGetVolumeAttributes (
- IN EFI_FVB_INSTANCE *FvbInstance
+ IN CONST EFI_FVB_INSTANCE *FvbInstance
)
{
+ if (FvbInstance == NULL) {
+ ASSERT (FvbInstance != NULL);
+ return 0;
+ }
+
return FvbInstance->FvHeader.Attributes; }
@@ -109,24 +115,25 @@ FvbGetVolumeAttributes (
@param[in] FvbInstance The pointer to the EFI_FVB_INSTANCE.
@param[in] Lba The logical block address
@param[out] LbaAddress On output, contains the physical starting address
- of the Lba
- @param[out] LbaLength On output, contains the length of the block
+ of the Lba. This pointer is optional and may be NULL.
+ @param[out] LbaLength On output, contains the length of the block.
+ This pointer is optional and may be NULL.
@param[out] NumOfBlocks A pointer to a caller allocated UINTN in which the
number of consecutive blocks starting with Lba is
returned. All blocks in this range have a size of
- BlockSize
+ BlockSize. This pointer is optional and may be NULL.
- @retval EFI_SUCCESS Successfully returns
- @retval EFI_INVALID_PARAMETER Instance not found
+ @retval EFI_SUCCESS Successfully returns
+ @retval EFI_INVALID_PARAMETER FvbInstance is NULL.
**/
EFI_STATUS
FvbGetLbaAddress (
- IN EFI_FVB_INSTANCE *FvbInstance,
+ IN CONST EFI_FVB_INSTANCE *FvbInstance,
IN EFI_LBA Lba,
- OUT UINTN *LbaAddress,
- OUT UINTN *LbaLength,
- OUT UINTN *NumOfBlocks
+ OUT UINTN *LbaAddress OPTIONAL,
+ OUT UINTN *LbaLength OPTIONAL,
+ OUT UINTN *NumOfBlocks OPTIONAL
)
{
UINT32 NumBlocks;
@@ -134,10 +141,15 @@ FvbGetLbaAddress (
UINTN Offset;
EFI_LBA StartLba;
EFI_LBA NextLba;
- EFI_FV_BLOCK_MAP_ENTRY *BlockMap;
+ CONST EFI_FV_BLOCK_MAP_ENTRY *BlockMap;
StartLba = 0;
Offset = 0;
+
+ if (FvbInstance == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
BlockMap = &(FvbInstance->FvHeader.BlockMap[0]);
//
@@ -158,15 +170,15 @@ FvbGetLbaAddress (
//
if (Lba >= StartLba && Lba < NextLba) {
Offset = Offset + (UINTN)MultU64x32((Lba - StartLba), BlockLength);
- if (LbaAddress ) {
+ if (LbaAddress != NULL) {
*LbaAddress = FvbInstance->FvBase + Offset;
}
- if (LbaLength ) {
+ if (LbaLength != NULL) {
*LbaLength = BlockLength;
}
- if (NumOfBlocks ) {
+ if (NumOfBlocks != NULL) {
*NumOfBlocks = (UINTN)(NextLba - Lba);
}
return EFI_SUCCESS;
@@ -190,7 +202,6 @@ FvbGetLbaAddress (
@param[in] Buffer Pointer to a caller allocated buffer that will be
used to hold the data read
-
@retval EFI_SUCCESS The firmware volume was read successfully and
contents are in Buffer
@retval EFI_BAD_BUFFER_SIZE Read attempted across a LBA boundary. On output,
@@ -199,16 +210,16 @@ FvbGetLbaAddress (
@retval EFI_ACCESS_DENIED The firmware volume is in the ReadDisabled state
@retval EFI_DEVICE_ERROR The block device is not functioning correctly and
could not be read
- @retval EFI_INVALID_PARAMETER Instance not found, or NumBytes, Buffer are NULL
+ @retval EFI_INVALID_PARAMETER FvbInstance, NumBytes, and/or Buffer are NULL
**/
EFI_STATUS
FvbReadBlock (
- IN EFI_FVB_INSTANCE *FvbInstance,
- IN EFI_LBA Lba,
- IN UINTN BlockOffset,
- IN OUT UINTN *NumBytes,
- IN UINT8 *Buffer
+ IN CONST EFI_FVB_INSTANCE *FvbInstance,
+ IN EFI_LBA Lba,
+ IN UINTN BlockOffset,
+ IN OUT UINTN *NumBytes,
+ IN UINT8 *Buffer
)
{
EFI_FVB_ATTRIBUTES_2 Attributes;
@@ -217,9 +228,10 @@ FvbReadBlock (
EFI_STATUS Status;
BOOLEAN BadBufferSize = FALSE;
- if ((NumBytes == NULL) || (Buffer == NULL)) {
+ if ((FvbInstance == NULL) || (NumBytes == NULL) || (Buffer == NULL))
+ {
return EFI_INVALID_PARAMETER;
}
+
if (*NumBytes == 0) {
return EFI_INVALID_PARAMETER;
}
@@ -269,6 +281,7 @@ FvbReadBlock (
of bytes actually written
@param[in] Buffer Pointer to a caller allocated buffer that contains
the source for the write
+
@retval EFI_SUCCESS The firmware volume was written successfully
@retval EFI_BAD_BUFFER_SIZE Write attempted across a LBA boundary. On output,
NumBytes contains the total number of bytes @@ -276,16 +289,16 @@ FvbReadBlock (
@retval EFI_ACCESS_DENIED The firmware volume is in the WriteDisabled state
@retval EFI_DEVICE_ERROR The block device is not functioning correctly and
could not be written
- @retval EFI_INVALID_PARAMETER Instance not found, or NumBytes, Buffer are NULL
+ @retval EFI_INVALID_PARAMETER FvbInstance, NumBytes, and/or Buffer are NULL
**/
EFI_STATUS
FvbWriteBlock (
- IN EFI_FVB_INSTANCE *FvbInstance,
- IN EFI_LBA Lba,
- IN UINTN BlockOffset,
- IN OUT UINTN *NumBytes,
- IN UINT8 *Buffer
+ IN CONST EFI_FVB_INSTANCE *FvbInstance,
+ IN EFI_LBA Lba,
+ IN UINTN BlockOffset,
+ IN OUT UINTN *NumBytes,
+ IN UINT8 *Buffer
)
{
EFI_FVB_ATTRIBUTES_2 Attributes;
@@ -294,7 +307,7 @@ FvbWriteBlock (
EFI_STATUS Status;
BOOLEAN BadBufferSize = FALSE;
- if ((NumBytes == NULL) || (Buffer == NULL)) {
+ if ((FvbInstance == NULL) || (NumBytes == NULL) || (Buffer == NULL))
+ {
return EFI_INVALID_PARAMETER;
}
if (*NumBytes == 0) {
@@ -350,8 +363,6 @@ FvbWriteBlock (
}
}
-
-
/**
Erases and initializes a firmware volume block.
@@ -363,13 +374,13 @@ FvbWriteBlock (
@retval EFI_DEVICE_ERROR The block device is not functioning correctly and
could not be written. Firmware device may have been
partially erased
- @retval EFI_INVALID_PARAMETER Instance not found
+ @retval EFI_INVALID_PARAMETER FvbInstance is NULL
**/
EFI_STATUS
FvbEraseBlock (
- IN EFI_FVB_INSTANCE *FvbInstance,
- IN EFI_LBA Lba
+ IN CONST EFI_FVB_INSTANCE *FvbInstance,
+ IN EFI_LBA Lba
)
{
@@ -378,6 +389,10 @@ FvbEraseBlock (
UINTN LbaLength;
EFI_STATUS Status;
+ if (FvbInstance == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
//
// Check if the FV is write enabled
//
@@ -422,9 +437,9 @@ FvbEraseBlock (
@retval EFI_SUCCESS Successfully returns
@retval EFI_ACCESS_DENIED The volume setting is locked and cannot be modified
- @retval EFI_INVALID_PARAMETER Instance not found, or The attributes requested are
- in conflict with the capabilities as declared in the
- firmware volume header
+ @retval EFI_INVALID_PARAMETER FvbInstance or Attributes is NULL.
+ Or the attributes requested are in conflict with the
+ capabilities as declared in the firmware volume header.
**/
EFI_STATUS
@@ -439,6 +454,10 @@ FvbSetVolumeAttributes (
UINT32 Capabilities;
UINT32 OldStatus, NewStatus;
+ if ((FvbInstance == NULL) || (Attributes == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
AttribPtr = (EFI_FVB_ATTRIBUTES_2 *) &(FvbInstance->FvHeader.Attributes);
OldAttributes = *AttribPtr;
Capabilities = OldAttributes & EFI_FVB2_CAPABILITIES; @@ -554,6 +573,7 @@ GetVariableFvInfo (
ASSERT ((BaseAddress != NULL) && (Length != NULL));
return;
}
+
*BaseAddress = 0;
*Length = 0;
@@ -631,8 +651,9 @@ GetVariableFvInfo (
@param[in] FvHeader A pointer to a firmware volume header
- @retval TRUE The firmware volume is consistent
- @retval FALSE The firmware volume has corrupted.
+ @retval TRUE The firmware volume is consistent.
+ @retval FALSE The firmware volume has corrupted or an invalid firmware
+ volume was provided.
**/
BOOLEAN
@@ -644,6 +665,11 @@ IsFvHeaderValid (
EFI_PHYSICAL_ADDRESS NvStorageFvBaseAddress;
UINT32 NvStorageSize;
+ if (FvHeader == NULL) {
+ ASSERT (FvHeader != NULL);
+ return FALSE;
+ }
+
GetVariableFvInfo (&NvStorageFvBaseAddress, &NvStorageSize);
if (FvBase == NvStorageFvBaseAddress) { @@ -679,18 +705,23 @@ IsFvHeaderValid (
@param[in] This A pointer to EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL.
@param[out] Address Output buffer containing the address.
- retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
**/
EFI_STATUS
EFIAPI
FvbProtocolGetPhysicalAddress (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- OUT EFI_PHYSICAL_ADDRESS *Address
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ OUT EFI_PHYSICAL_ADDRESS *Address
)
{
EFI_FVB_INSTANCE *FvbInstance;
+ if ((This == NULL) || (Address == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
*Address = FvbInstance->FvBase;
@@ -710,28 +741,34 @@ FvbProtocolGetPhysicalAddress (
returned. All blocks in this range have a size of
BlockSize
- @retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
**/
EFI_STATUS
EFIAPI
FvbProtocolGetBlockSize (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- IN EFI_LBA Lba,
- OUT UINTN *BlockSize,
- OUT UINTN *NumOfBlocks
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ IN EFI_LBA Lba,
+ OUT UINTN *BlockSize,
+ OUT UINTN *NumOfBlocks
)
{
EFI_FVB_INSTANCE *FvbInstance;
+ if ((This == NULL) || (BlockSize == NULL) || (NumOfBlocks == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
- DEBUG((DEBUG_INFO,
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolGetBlockSize: Lba: 0x%lx BlockSize: 0x%x NumOfBlocks: 0x%x\n",
Lba,
- BlockSize,
- NumOfBlocks)
- );
+ *BlockSize,
+ *NumOfBlocks
+ ));
return FvbGetLbaAddress (
FvbInstance,
@@ -748,27 +785,33 @@ FvbProtocolGetBlockSize (
@param[in] This Calling context
@param[out] Attributes Output buffer which contains attributes
- @retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
**/
EFI_STATUS
EFIAPI
FvbProtocolGetAttributes (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- OUT EFI_FVB_ATTRIBUTES_2 *Attributes
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ OUT EFI_FVB_ATTRIBUTES_2 *Attributes
)
{
EFI_FVB_INSTANCE *FvbInstance;
+ if ((This == NULL) || (Attributes == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
*Attributes = FvbGetVolumeAttributes (FvbInstance);
- DEBUG ((DEBUG_INFO,
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolGetAttributes: This: 0x%x Attributes: 0x%x\n",
This,
- *Attributes)
- );
+ *Attributes
+ ));
return EFI_SUCCESS;
}
@@ -785,28 +828,34 @@ FvbProtocolGetAttributes ( EFI_STATUS EFIAPI FvbProtocolSetAttributes (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- IN OUT EFI_FVB_ATTRIBUTES_2 *Attributes
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ IN OUT EFI_FVB_ATTRIBUTES_2 *Attributes
)
{
EFI_STATUS Status;
EFI_FVB_INSTANCE *FvbInstance;
- DEBUG((DEBUG_INFO,
+ if ((This == NULL) || (Attributes == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolSetAttributes: Before SET - This: 0x%x Attributes: 0x%x\n",
This,
- *Attributes)
- );
+ *Attributes
+ ));
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
Status = FvbSetVolumeAttributes (FvbInstance, Attributes);
- DEBUG((DEBUG_INFO,
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolSetAttributes: After SET - This: 0x%x Attributes: 0x%x\n",
This,
- *Attributes)
- );
+ *Attributes
+ ));
return Status;
}
@@ -823,17 +872,18 @@ FvbProtocolSetAttributes (
@param[in] ... Starting LBA followed by Number of Lba to erase.
a -1 to terminate the list.
- @retval EFI_SUCCESS The erase request was successfully completed
- @retval EFI_ACCESS_DENIED The firmware volume is in the WriteDisabled state
- @retval EFI_DEVICE_ERROR The block device is not functioning correctly and
- could not be written. Firmware device may have been
- partially erased
+ @retval EFI_SUCCESS The erase request was successfully completed
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
+ @retval EFI_ACCESS_DENIED The firmware volume is in the WriteDisabled state
+ @retval EFI_DEVICE_ERROR The block device is not functioning correctly and
+ could not be written. Firmware device may have been
+ partially erased
**/
EFI_STATUS
EFIAPI
FvbProtocolEraseBlocks (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
...
)
{
@@ -846,6 +896,10 @@ FvbProtocolEraseBlocks (
DEBUG((DEBUG_INFO, "FvbProtocolEraseBlocks: \n"));
+ if (This == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
NumOfBlocks = FvbInstance->NumOfBlocks; @@ -923,30 +977,35 @@ FvbProtocolEraseBlocks (
@retval EFI_ACCESS_DENIED The firmware volume is in the WriteDisabled state
@retval EFI_DEVICE_ERROR The block device is not functioning correctly and
could not be written
- @retval EFI_INVALID_PARAMETER NumBytes or Buffer are NULL
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
**/
EFI_STATUS
EFIAPI
FvbProtocolWrite (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- IN EFI_LBA Lba,
- IN UINTN Offset,
- IN OUT UINTN *NumBytes,
- IN UINT8 *Buffer
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ IN EFI_LBA Lba,
+ IN UINTN Offset,
+ IN OUT UINTN *NumBytes,
+ IN UINT8 *Buffer
)
{
EFI_FVB_INSTANCE *FvbInstance;
+ if ((This == NULL) || (NumBytes == NULL) || (Buffer == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
- DEBUG((DEBUG_INFO,
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolWrite: Lba: 0x%lx Offset: 0x%x NumBytes: 0x%x, Buffer: 0x%x\n",
Lba,
Offset,
*NumBytes,
- Buffer)
- );
+ Buffer
+ ));
return FvbWriteBlock (FvbInstance, Lba, Offset, NumBytes, Buffer); } @@ -974,31 +1033,36 @@ FvbProtocolWrite (
@retval EFI_ACCESS_DENIED The firmware volume is in the ReadDisabled state
@retval EFI_DEVICE_ERROR The block device is not functioning correctly and
could not be read
- @retval EFI_INVALID_PARAMETER NumBytes or Buffer are NULL
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
**/
EFI_STATUS
EFIAPI
FvbProtocolRead (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- IN EFI_LBA Lba,
- IN UINTN Offset,
- IN OUT UINTN *NumBytes,
- OUT UINT8 *Buffer
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ IN EFI_LBA Lba,
+ IN UINTN Offset,
+ IN OUT UINTN *NumBytes,
+ OUT UINT8 *Buffer
)
{
EFI_FVB_INSTANCE *FvbInstance;
EFI_STATUS Status;
+ if ((This == NULL) || (NumBytes == NULL) || (Buffer == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
Status = FvbReadBlock (FvbInstance, Lba, Offset, NumBytes, Buffer);
- DEBUG((DEBUG_INFO,
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolRead: Lba: 0x%lx Offset: 0x%x NumBytes: 0x%x, Buffer: 0x%x\n",
Lba,
Offset,
*NumBytes,
- Buffer)
- );
+ Buffer
+ ));
return Status;
}
diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c
index f9543c5ba677..5d7591480319 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbSe
+++ rviceMm.c
@@ -15,14 +15,11 @@
#include <Guid/VariableFormat.h>
/**
- The function installs EFI_FIRMWARE_VOLUME_BLOCK protocol
- for each FV in the system.
+ The function installs the given EFI_FIRMWARE_VOLUME_BLOCK protocol instance.
@param[in] FvbInstance The pointer to a FW volume instance structure,
which contains the information about one FV.
- @retval VOID
-
**/
VOID
InstallFvbProtocol (
@@ -33,8 +30,8 @@ InstallFvbProtocol (
EFI_STATUS Status;
EFI_HANDLE FvbHandle;
- ASSERT (FvbInstance != NULL);
if (FvbInstance == NULL) {
+ ASSERT (FvbInstance != NULL);
return;
}
@@ -48,14 +45,14 @@ InstallFvbProtocol (
//
// Set up the devicepath
//
- DEBUG ((DEBUG_INFO, "FwBlockService.c: Setting up DevicePath for 0x%lx:\n", FvbInstance->FvBase));
+ DEBUG ((DEBUG_INFO, "SpiFvbServiceSmm.c: Setting up DevicePath for
+ 0x%lx:\n", FvbInstance->FvBase));
if (FvHeader->ExtHeaderOffset == 0) {
//
// FV does not contains extension header, then produce MEMMAP_DEVICE_PATH
//
FvbInstance->DevicePath = (EFI_DEVICE_PATH_PROTOCOL *) AllocateRuntimeCopyPool (sizeof (FV_MEMMAP_DEVICE_PATH), &mFvMemmapDevicePathTemplate);
if (FvbInstance->DevicePath == NULL) {
- DEBUG ((DEBUG_INFO, "SpiFvbServiceSmm.c: Memory allocation for MEMMAP_DEVICE_PATH failed\n"));
+ DEBUG ((DEBUG_ERROR, "SpiFvbServiceSmm.c: Memory allocation for
+ MEMMAP_DEVICE_PATH failed\n"));
return;
}
((FV_MEMMAP_DEVICE_PATH *) FvbInstance->DevicePath)->MemMapDevPath.StartingAddress = FvbInstance->FvBase; @@ -63,7 +60,7 @@ InstallFvbProtocol (
} else {
FvbInstance->DevicePath = (EFI_DEVICE_PATH_PROTOCOL *) AllocateRuntimeCopyPool (sizeof (FV_PIWG_DEVICE_PATH), &mFvPIWGDevicePathTemplate);
if (FvbInstance->DevicePath == NULL) {
- DEBUG ((DEBUG_INFO, "SpiFvbServiceSmm.c: Memory allocation for FV_PIWG_DEVICE_PATH failed\n"));
+ DEBUG ((DEBUG_ERROR, "SpiFvbServiceSmm.c: Memory allocation for
+ FV_PIWG_DEVICE_PATH failed\n"));
return;
}
CopyGuid (
@@ -96,7 +93,7 @@ InstallFvbProtocol (
/**
The function does the necessary initialization work for
- Firmware Volume Block Driver.
+ the Firmware Volume Block Driver.
**/
VOID
@@ -148,10 +145,6 @@ FvbInitialize (
mPlatformFvBaseAddress[1].FvBase = PcdGet32(PcdFlashMicrocodeFvBase);
mPlatformFvBaseAddress[1].FvSize = PcdGet32(PcdFlashMicrocodeFvSize);
- //
- // We will only continue with FVB installation if the
- // SPI is the active BIOS state
- //
{
//
// Make sure all FVB are valid and/or fix if possible @@ -167,11 +160,20 @@ FvbInitialize (
if (!IsFvHeaderValid (BaseAddress, FvHeader)) {
BytesWritten = 0;
BytesErased = 0;
- DEBUG ((DEBUG_ERROR, "ERROR - The FV in 0x%x is invalid!\n", FvHeader));
+ DEBUG ((DEBUG_ERROR, "ERROR - The FV at 0x%x is invalid!\n",
+ FvHeader));
+
+ //
+ // Attempt to recover the FV
+ //
FvHeader = NULL;
- Status = GetFvbInfo (BaseAddress, &FvHeader);
+ Status = GetGeneratedFvByAddress (BaseAddress, &FvHeader);
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_WARN, "ERROR - Can't recovery FV header at 0x%x. GetFvbInfo Status %r\n", BaseAddress, Status));
+ DEBUG ((
+ DEBUG_WARN | DEBUG_ERROR,
+ "ERROR - Can't recover FV header at 0x%x. GetGeneratedFvByAddress Status %r\n",
+ BaseAddress,
+ Status
+ ));
continue;
}
DEBUG ((DEBUG_INFO, "Rewriting FV header at 0x%X with static data\n", BaseAddress)); @@ -181,15 +183,15 @@ FvbInitialize (
BytesErased = (UINTN) FvHeader->BlockMap->Length;
Status = SpiFlashBlockErase( (UINTN) BaseAddress, &BytesErased);
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_WARN, "ERROR - SpiFlashBlockErase Error %r\n", Status));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - SpiFlashBlockErase
+ Error %r\n", Status));
if (FvHeader != NULL) {
FreePool (FvHeader);
}
continue;
}
if (BytesErased != FvHeader->BlockMap->Length) {
- DEBUG ((DEBUG_WARN, "ERROR - BytesErased != FvHeader->BlockMap->Length\n"));
- DEBUG ((DEBUG_INFO, " BytesErased = 0x%X\n Length = 0x%X\n", BytesErased, FvHeader->BlockMap->Length));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - BytesErased != FvHeader->BlockMap->Length\n"));
+ DEBUG ((DEBUG_ERROR, " BytesErased = 0x%X\n Length = 0x%X\n",
+ BytesErased, FvHeader->BlockMap->Length));
if (FvHeader != NULL) {
FreePool (FvHeader);
}
@@ -249,15 +251,15 @@ FvbInitialize (
}
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_WARN, "ERROR - SpiFlashWrite Error %r\n", Status));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - SpiFlashWrite
+ Error %r\n", Status));
if (FvHeader != NULL) {
FreePool (FvHeader);
}
continue;
}
if (BytesWritten != ExpectedBytesWritten) {
- DEBUG ((DEBUG_WARN, "ERROR - BytesWritten != ExpectedBytesWritten\n"));
- DEBUG ((DEBUG_INFO, " BytesWritten = 0x%X\n ExpectedBytesWritten = 0x%X\n", BytesWritten, ExpectedBytesWritten));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - BytesWritten != ExpectedBytesWritten\n"));
+ DEBUG ((DEBUG_ERROR, " BytesWritten = 0x%X\n
+ ExpectedBytesWritten = 0x%X\n", BytesWritten, ExpectedBytesWritten));
if (FvHeader != NULL) {
FreePool (FvHeader);
}
@@ -265,13 +267,13 @@ FvbInitialize (
}
Status = SpiFlashLock ();
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_WARN, "ERROR - SpiFlashLock Error %r\n", Status));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - SpiFlashLock Error
+ %r\n", Status));
if (FvHeader != NULL) {
FreePool (FvHeader);
}
continue;
}
- DEBUG ((DEBUG_INFO, "FV Header @ 0x%X restored with static data\n", BaseAddress));
+ DEBUG ((DEBUG_ERROR, "FV Header @ 0x%X restored with static
+ data\n", BaseAddress));
//
// Clear cache for this range.
//
@@ -294,7 +296,7 @@ FvbInitialize (
FvHeader = (EFI_FIRMWARE_VOLUME_HEADER *) (UINTN) BaseAddress;
if (!IsFvHeaderValid (BaseAddress, FvHeader)) {
- DEBUG ((DEBUG_WARN, "ERROR - The FV in 0x%x is invalid!\n", FvHeader));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - The FV in 0x%x is
+ invalid!\n", FvHeader));
continue;
}
@@ -322,7 +324,7 @@ FvbInitialize (
FvHeader = (EFI_FIRMWARE_VOLUME_HEADER *) (UINTN) BaseAddress;
if (!IsFvHeaderValid (BaseAddress, FvHeader)) {
- DEBUG ((DEBUG_WARN, "ERROR - The FV in 0x%x is invalid!\n", FvHeader));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - The FV in 0x%x is
+ invalid!\n", FvHeader));
continue;
}
diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h
index 7b0908b03fdc..7664670457a2 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbSe
+++ rviceCommon.h
@@ -1,14 +1,14 @@
/** @file
Common source definitions used in serial flash drivers
-Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> -Copyright (c) Microsoft Corporation.<BR>
-SPDX-License-Identifier: BSD-2-Clause-Patent
+ Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) Microsoft Corporation.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
**/
-#ifndef _SPI_FVB_SERVICE_COMMON_H
-#define _SPI_FVB_SERVICE_COMMON_H
+#ifndef SPI_FVB_SERVICE_COMMON_H_
+#define SPI_FVB_SERVICE_COMMON_H_
#include <Guid/EventGroup.h>
#include <Guid/FirmwareFileSystem2.h>
@@ -61,7 +61,7 @@ typedef struct {
} FVB_GLOBAL;
//
-// Fvb Protocol instance data
+// Firmware Volume Block (FVB) Protocol instance data
//
#define FVB_INSTANCE_FROM_THIS(a) CR(a, EFI_FVB_INSTANCE, FvbProtocol, FVB_INSTANCE_SIGNATURE)
@@ -81,7 +81,7 @@ typedef struct {
} FV_INFO;
//
-// Protocol APIs
+// Firmware Volume Block (FVB) Protocol APIs
//
EFI_STATUS
EFIAPI
@@ -146,8 +146,23 @@ IsFvHeaderValid (
IN CONST EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader
);
+//
+// Module Local Functions
+//
+
+/**
+ Returns an empty firmware volume for the firmware volume at the given base address.
+
+ @param[in] FvBaseAddress The base address of the firmware volume requested.
+ @param[out] FvbInfo A pointer that will be set to a buffer for the firmware volume header
+ at the given base address.
+
+ @retval EFI_SUCCESS The firmware volume was returned successfully.
+ @retval EFI_NOT_FOUND The firmware volume was not found for the given base address.
+
+**/
EFI_STATUS
-GetFvbInfo (
+GetGeneratedFvByAddress (
IN EFI_PHYSICAL_ADDRESS FvBaseAddress,
OUT EFI_FIRMWARE_VOLUME_HEADER **FvbInfo
);
diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.h b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.h
index 36af1130c8ee..512844197a4d 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.h
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbSe
+++ rviceMm.h
@@ -6,12 +6,12 @@
**/
-#ifndef _SPI_FVB_SERVICE_MM_H_
-#define _SPI_FVB_SERVICE_MM_H_
+#ifndef SPI_FVB_SERVICE_MM_H_
+#define SPI_FVB_SERVICE_MM_H_
/**
The function does the necessary initialization work for
- Firmware Volume Block Driver.
+ the Firmware Volume Block Driver.
**/
VOID
--
2.41.0.windows.1
-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106553): https://edk2.groups.io/g/devel/message/106553
Mute This Topic: https://groups.io/mt/99866031/6226280
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [ashraf.ali.s@intel.com] -=-=-=-=-=-=
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/SpiFvbService: Non-functional cleanup
[not found] ` <176D896904216960.17093@groups.io>
@ 2023-06-30 20:47 ` Isaac Oram
0 siblings, 0 replies; 4+ messages in thread
From: Isaac Oram @ 2023-06-30 20:47 UTC (permalink / raw)
To: devel@edk2.groups.io, Oram, Isaac W, S, Ashraf Ali,
mikuback@linux.microsoft.com
Cc: Ni, Ray, Chaganty, Rangasai V
Pushed as 1befeabcc8..dd09609ec9
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Isaac Oram
Sent: Friday, June 30, 2023 1:22 PM
To: S, Ashraf Ali <ashraf.ali.s@intel.com>; devel@edk2.groups.io; mikuback@linux.microsoft.com
Cc: Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>
Subject: Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/SpiFvbService: Non-functional cleanup
Reviewed-by: Isaac Oram <isaac.w.oram@intel.com>
Mixing DEBUG_WARN | DEBUG_ERROR confuses me a bit. I would prefer simpler definitions were errors are "something is wrong" and warnings are "something might be wrong".
But it is a pretty minor nit and cleanup is good, so don't worry about changing it.
Regards,
Isaac
-----Original Message-----
From: S, Ashraf Ali <ashraf.ali.s@intel.com>
Sent: Friday, June 30, 2023 11:57 AM
To: devel@edk2.groups.io; mikuback@linux.microsoft.com
Cc: Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>
Subject: RE: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/SpiFvbService: Non-functional cleanup
Reviewed-by: Ashraf Ali S <ashraf.ali.s@intel.com>
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
Sent: Friday, June 30, 2023 8:03 AM
To: devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; S, Ashraf Ali <ashraf.ali.s@intel.com>
Subject: [edk2-devel] [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/SpiFvbService: Non-functional cleanup
From: Michael Kubacki <michael.kubacki@microsoft.com>
During a review of this driver a number of improvements were noted such as strengthening function input validation, checking return values, making debug print error levels consistent in certain code blocks, etc.
These type of changes are made with no explicit change to driver functionality.
Cc: Ray Ni <ray.ni@intel.com>
Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Ashraf Ali S <ashraf.ali.s@intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
---
Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c | 35 ++-
Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c | 248 ++++++++++++--------
Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c | 54 +++--
Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h | 31 ++-
Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.h | 6 +-
5 files changed, 239 insertions(+), 135 deletions(-)
diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c
index ab1cb2ef1622..ebbd9edaada3 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.c
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/FvbInfo.
+++ c
@@ -2,16 +2,17 @@
Defines data structure that is the volume header found.
These data is intent to decouple FVB driver with FV header.
-Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> -Copyright (c) Microsoft Corporation.<BR>
-SPDX-License-Identifier: BSD-2-Clause-Patent
+ Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) Microsoft Corporation.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
**/
#include "SpiFvbServiceCommon.h"
-#define FIRMWARE_BLOCK_SIZE 0x10000
+#define FIRMWARE_BLOCK_SIZE SIZE_64KB
#define FVB_MEDIA_BLOCK_SIZE FIRMWARE_BLOCK_SIZE
+
typedef struct {
EFI_PHYSICAL_ADDRESS BaseAddress;
EFI_FIRMWARE_VOLUME_HEADER FvbInfo;
@@ -19,7 +20,7 @@ typedef struct {
} EFI_FVB2_MEDIA_INFO;
/**
- Returns FVB media information for NV variable storage.
+ Returns FVB media information for a firmware volume.
@return FvbMediaInfo A pointer to an instance of FVB media info produced by this function.
The buffer is allocated internally to this function and it is the caller's @@ -100,8 +101,21 @@ FVB_MEDIA_INFO_GENERATOR mFvbMediaInfoGenerators[] = {
GenerateNvStorageFvbMediaInfo
};
+/**
+ Returns an empty firmware volume for the firmware volume at the given base address.
+
+ @param[in] FvBaseAddress The base address of the firmware volume requested.
+ @param[out] FvbInfo A pointer that will be set to a buffer for the firmware volume header
+ at the given base address. The buffer is a pool allocation made in this function.
+
+ @retval EFI_SUCCESS The firmware volume was returned successfully.
+ @retval EFI_INVALID_PARAMETER The FvbInfo pointer argument is NULL.
+ @retval EFI_NOT_FOUND The firmware volume was not found for the given base address.
+ @retval EFI_OUT_OF_RESOURCES Insufficient memory to allocate a buffer to the hold the firmware volume.
+
+**/
EFI_STATUS
-GetFvbInfo (
+GetGeneratedFvByAddress (
IN EFI_PHYSICAL_ADDRESS FvBaseAddress,
OUT EFI_FIRMWARE_VOLUME_HEADER **FvbInfo
)
@@ -111,11 +125,19 @@ GetFvbInfo (
UINTN Index;
EFI_FIRMWARE_VOLUME_HEADER *FvHeader;
+ if (FvbInfo == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
for (Index = 0; Index < ARRAY_SIZE (mFvbMediaInfoGenerators); Index++) {
Status = mFvbMediaInfoGenerators[Index](&FvbMediaInfo);
ASSERT_EFI_ERROR (Status);
if (!EFI_ERROR (Status) && (FvbMediaInfo.BaseAddress == FvBaseAddress)) {
FvHeader = AllocateCopyPool (FvbMediaInfo.FvbInfo.HeaderLength, &FvbMediaInfo.FvbInfo);
+ if (FvHeader == NULL) {
+ ASSERT (FvHeader != NULL);
+ return EFI_OUT_OF_RESOURCES;
+ }
//
// Update the checksum value of FV header.
@@ -136,5 +158,6 @@ GetFvbInfo (
return EFI_SUCCESS;
}
}
+
return EFI_NOT_FOUND;
}
diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c
index fcdc715263f2..e21113682f4f 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.c
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbSe
+++ rviceCommon.c
@@ -2,9 +2,9 @@
Common driver source for several Serial Flash devices
which are compliant with the Intel(R) Serial Flash Interface Compatibility Specification.
-Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> -Copyright (c) Microsoft Corporation.<BR>
-SPDX-License-Identifier: BSD-2-Clause-Patent
+ Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) Microsoft Corporation.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
**/
@@ -92,13 +92,19 @@ EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL mFvbProtocolTemplate = {
@param[in] FvbInstance The pointer to the EFI_FVB_INSTANCE.
@return Attributes of the FV identified by FvbInstance.
+ Zero is returned if the FvbInstance pointer is NULL.
**/
EFI_FVB_ATTRIBUTES_2
FvbGetVolumeAttributes (
- IN EFI_FVB_INSTANCE *FvbInstance
+ IN CONST EFI_FVB_INSTANCE *FvbInstance
)
{
+ if (FvbInstance == NULL) {
+ ASSERT (FvbInstance != NULL);
+ return 0;
+ }
+
return FvbInstance->FvHeader.Attributes; }
@@ -109,24 +115,25 @@ FvbGetVolumeAttributes (
@param[in] FvbInstance The pointer to the EFI_FVB_INSTANCE.
@param[in] Lba The logical block address
@param[out] LbaAddress On output, contains the physical starting address
- of the Lba
- @param[out] LbaLength On output, contains the length of the block
+ of the Lba. This pointer is optional and may be NULL.
+ @param[out] LbaLength On output, contains the length of the block.
+ This pointer is optional and may be NULL.
@param[out] NumOfBlocks A pointer to a caller allocated UINTN in which the
number of consecutive blocks starting with Lba is
returned. All blocks in this range have a size of
- BlockSize
+ BlockSize. This pointer is optional and may be NULL.
- @retval EFI_SUCCESS Successfully returns
- @retval EFI_INVALID_PARAMETER Instance not found
+ @retval EFI_SUCCESS Successfully returns
+ @retval EFI_INVALID_PARAMETER FvbInstance is NULL.
**/
EFI_STATUS
FvbGetLbaAddress (
- IN EFI_FVB_INSTANCE *FvbInstance,
+ IN CONST EFI_FVB_INSTANCE *FvbInstance,
IN EFI_LBA Lba,
- OUT UINTN *LbaAddress,
- OUT UINTN *LbaLength,
- OUT UINTN *NumOfBlocks
+ OUT UINTN *LbaAddress OPTIONAL,
+ OUT UINTN *LbaLength OPTIONAL,
+ OUT UINTN *NumOfBlocks OPTIONAL
)
{
UINT32 NumBlocks;
@@ -134,10 +141,15 @@ FvbGetLbaAddress (
UINTN Offset;
EFI_LBA StartLba;
EFI_LBA NextLba;
- EFI_FV_BLOCK_MAP_ENTRY *BlockMap;
+ CONST EFI_FV_BLOCK_MAP_ENTRY *BlockMap;
StartLba = 0;
Offset = 0;
+
+ if (FvbInstance == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
BlockMap = &(FvbInstance->FvHeader.BlockMap[0]);
//
@@ -158,15 +170,15 @@ FvbGetLbaAddress (
//
if (Lba >= StartLba && Lba < NextLba) {
Offset = Offset + (UINTN)MultU64x32((Lba - StartLba), BlockLength);
- if (LbaAddress ) {
+ if (LbaAddress != NULL) {
*LbaAddress = FvbInstance->FvBase + Offset;
}
- if (LbaLength ) {
+ if (LbaLength != NULL) {
*LbaLength = BlockLength;
}
- if (NumOfBlocks ) {
+ if (NumOfBlocks != NULL) {
*NumOfBlocks = (UINTN)(NextLba - Lba);
}
return EFI_SUCCESS;
@@ -190,7 +202,6 @@ FvbGetLbaAddress (
@param[in] Buffer Pointer to a caller allocated buffer that will be
used to hold the data read
-
@retval EFI_SUCCESS The firmware volume was read successfully and
contents are in Buffer
@retval EFI_BAD_BUFFER_SIZE Read attempted across a LBA boundary. On output,
@@ -199,16 +210,16 @@ FvbGetLbaAddress (
@retval EFI_ACCESS_DENIED The firmware volume is in the ReadDisabled state
@retval EFI_DEVICE_ERROR The block device is not functioning correctly and
could not be read
- @retval EFI_INVALID_PARAMETER Instance not found, or NumBytes, Buffer are NULL
+ @retval EFI_INVALID_PARAMETER FvbInstance, NumBytes, and/or Buffer are NULL
**/
EFI_STATUS
FvbReadBlock (
- IN EFI_FVB_INSTANCE *FvbInstance,
- IN EFI_LBA Lba,
- IN UINTN BlockOffset,
- IN OUT UINTN *NumBytes,
- IN UINT8 *Buffer
+ IN CONST EFI_FVB_INSTANCE *FvbInstance,
+ IN EFI_LBA Lba,
+ IN UINTN BlockOffset,
+ IN OUT UINTN *NumBytes,
+ IN UINT8 *Buffer
)
{
EFI_FVB_ATTRIBUTES_2 Attributes;
@@ -217,9 +228,10 @@ FvbReadBlock (
EFI_STATUS Status;
BOOLEAN BadBufferSize = FALSE;
- if ((NumBytes == NULL) || (Buffer == NULL)) {
+ if ((FvbInstance == NULL) || (NumBytes == NULL) || (Buffer == NULL))
+ {
return EFI_INVALID_PARAMETER;
}
+
if (*NumBytes == 0) {
return EFI_INVALID_PARAMETER;
}
@@ -269,6 +281,7 @@ FvbReadBlock (
of bytes actually written
@param[in] Buffer Pointer to a caller allocated buffer that contains
the source for the write
+
@retval EFI_SUCCESS The firmware volume was written successfully
@retval EFI_BAD_BUFFER_SIZE Write attempted across a LBA boundary. On output,
NumBytes contains the total number of bytes @@ -276,16 +289,16 @@ FvbReadBlock (
@retval EFI_ACCESS_DENIED The firmware volume is in the WriteDisabled state
@retval EFI_DEVICE_ERROR The block device is not functioning correctly and
could not be written
- @retval EFI_INVALID_PARAMETER Instance not found, or NumBytes, Buffer are NULL
+ @retval EFI_INVALID_PARAMETER FvbInstance, NumBytes, and/or Buffer are NULL
**/
EFI_STATUS
FvbWriteBlock (
- IN EFI_FVB_INSTANCE *FvbInstance,
- IN EFI_LBA Lba,
- IN UINTN BlockOffset,
- IN OUT UINTN *NumBytes,
- IN UINT8 *Buffer
+ IN CONST EFI_FVB_INSTANCE *FvbInstance,
+ IN EFI_LBA Lba,
+ IN UINTN BlockOffset,
+ IN OUT UINTN *NumBytes,
+ IN UINT8 *Buffer
)
{
EFI_FVB_ATTRIBUTES_2 Attributes;
@@ -294,7 +307,7 @@ FvbWriteBlock (
EFI_STATUS Status;
BOOLEAN BadBufferSize = FALSE;
- if ((NumBytes == NULL) || (Buffer == NULL)) {
+ if ((FvbInstance == NULL) || (NumBytes == NULL) || (Buffer == NULL))
+ {
return EFI_INVALID_PARAMETER;
}
if (*NumBytes == 0) {
@@ -350,8 +363,6 @@ FvbWriteBlock (
}
}
-
-
/**
Erases and initializes a firmware volume block.
@@ -363,13 +374,13 @@ FvbWriteBlock (
@retval EFI_DEVICE_ERROR The block device is not functioning correctly and
could not be written. Firmware device may have been
partially erased
- @retval EFI_INVALID_PARAMETER Instance not found
+ @retval EFI_INVALID_PARAMETER FvbInstance is NULL
**/
EFI_STATUS
FvbEraseBlock (
- IN EFI_FVB_INSTANCE *FvbInstance,
- IN EFI_LBA Lba
+ IN CONST EFI_FVB_INSTANCE *FvbInstance,
+ IN EFI_LBA Lba
)
{
@@ -378,6 +389,10 @@ FvbEraseBlock (
UINTN LbaLength;
EFI_STATUS Status;
+ if (FvbInstance == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
//
// Check if the FV is write enabled
//
@@ -422,9 +437,9 @@ FvbEraseBlock (
@retval EFI_SUCCESS Successfully returns
@retval EFI_ACCESS_DENIED The volume setting is locked and cannot be modified
- @retval EFI_INVALID_PARAMETER Instance not found, or The attributes requested are
- in conflict with the capabilities as declared in the
- firmware volume header
+ @retval EFI_INVALID_PARAMETER FvbInstance or Attributes is NULL.
+ Or the attributes requested are in conflict with the
+ capabilities as declared in the firmware volume header.
**/
EFI_STATUS
@@ -439,6 +454,10 @@ FvbSetVolumeAttributes (
UINT32 Capabilities;
UINT32 OldStatus, NewStatus;
+ if ((FvbInstance == NULL) || (Attributes == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
AttribPtr = (EFI_FVB_ATTRIBUTES_2 *) &(FvbInstance->FvHeader.Attributes);
OldAttributes = *AttribPtr;
Capabilities = OldAttributes & EFI_FVB2_CAPABILITIES; @@ -554,6 +573,7 @@ GetVariableFvInfo (
ASSERT ((BaseAddress != NULL) && (Length != NULL));
return;
}
+
*BaseAddress = 0;
*Length = 0;
@@ -631,8 +651,9 @@ GetVariableFvInfo (
@param[in] FvHeader A pointer to a firmware volume header
- @retval TRUE The firmware volume is consistent
- @retval FALSE The firmware volume has corrupted.
+ @retval TRUE The firmware volume is consistent.
+ @retval FALSE The firmware volume has corrupted or an invalid firmware
+ volume was provided.
**/
BOOLEAN
@@ -644,6 +665,11 @@ IsFvHeaderValid (
EFI_PHYSICAL_ADDRESS NvStorageFvBaseAddress;
UINT32 NvStorageSize;
+ if (FvHeader == NULL) {
+ ASSERT (FvHeader != NULL);
+ return FALSE;
+ }
+
GetVariableFvInfo (&NvStorageFvBaseAddress, &NvStorageSize);
if (FvBase == NvStorageFvBaseAddress) { @@ -679,18 +705,23 @@ IsFvHeaderValid (
@param[in] This A pointer to EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL.
@param[out] Address Output buffer containing the address.
- retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
**/
EFI_STATUS
EFIAPI
FvbProtocolGetPhysicalAddress (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- OUT EFI_PHYSICAL_ADDRESS *Address
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ OUT EFI_PHYSICAL_ADDRESS *Address
)
{
EFI_FVB_INSTANCE *FvbInstance;
+ if ((This == NULL) || (Address == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
*Address = FvbInstance->FvBase;
@@ -710,28 +741,34 @@ FvbProtocolGetPhysicalAddress (
returned. All blocks in this range have a size of
BlockSize
- @retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
**/
EFI_STATUS
EFIAPI
FvbProtocolGetBlockSize (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- IN EFI_LBA Lba,
- OUT UINTN *BlockSize,
- OUT UINTN *NumOfBlocks
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ IN EFI_LBA Lba,
+ OUT UINTN *BlockSize,
+ OUT UINTN *NumOfBlocks
)
{
EFI_FVB_INSTANCE *FvbInstance;
+ if ((This == NULL) || (BlockSize == NULL) || (NumOfBlocks == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
- DEBUG((DEBUG_INFO,
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolGetBlockSize: Lba: 0x%lx BlockSize: 0x%x NumOfBlocks: 0x%x\n",
Lba,
- BlockSize,
- NumOfBlocks)
- );
+ *BlockSize,
+ *NumOfBlocks
+ ));
return FvbGetLbaAddress (
FvbInstance,
@@ -748,27 +785,33 @@ FvbProtocolGetBlockSize (
@param[in] This Calling context
@param[out] Attributes Output buffer which contains attributes
- @retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_SUCCESS The function always return successfully.
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
**/
EFI_STATUS
EFIAPI
FvbProtocolGetAttributes (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- OUT EFI_FVB_ATTRIBUTES_2 *Attributes
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ OUT EFI_FVB_ATTRIBUTES_2 *Attributes
)
{
EFI_FVB_INSTANCE *FvbInstance;
+ if ((This == NULL) || (Attributes == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
*Attributes = FvbGetVolumeAttributes (FvbInstance);
- DEBUG ((DEBUG_INFO,
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolGetAttributes: This: 0x%x Attributes: 0x%x\n",
This,
- *Attributes)
- );
+ *Attributes
+ ));
return EFI_SUCCESS;
}
@@ -785,28 +828,34 @@ FvbProtocolGetAttributes ( EFI_STATUS EFIAPI FvbProtocolSetAttributes (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- IN OUT EFI_FVB_ATTRIBUTES_2 *Attributes
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ IN OUT EFI_FVB_ATTRIBUTES_2 *Attributes
)
{
EFI_STATUS Status;
EFI_FVB_INSTANCE *FvbInstance;
- DEBUG((DEBUG_INFO,
+ if ((This == NULL) || (Attributes == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolSetAttributes: Before SET - This: 0x%x Attributes: 0x%x\n",
This,
- *Attributes)
- );
+ *Attributes
+ ));
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
Status = FvbSetVolumeAttributes (FvbInstance, Attributes);
- DEBUG((DEBUG_INFO,
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolSetAttributes: After SET - This: 0x%x Attributes: 0x%x\n",
This,
- *Attributes)
- );
+ *Attributes
+ ));
return Status;
}
@@ -823,17 +872,18 @@ FvbProtocolSetAttributes (
@param[in] ... Starting LBA followed by Number of Lba to erase.
a -1 to terminate the list.
- @retval EFI_SUCCESS The erase request was successfully completed
- @retval EFI_ACCESS_DENIED The firmware volume is in the WriteDisabled state
- @retval EFI_DEVICE_ERROR The block device is not functioning correctly and
- could not be written. Firmware device may have been
- partially erased
+ @retval EFI_SUCCESS The erase request was successfully completed
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
+ @retval EFI_ACCESS_DENIED The firmware volume is in the WriteDisabled state
+ @retval EFI_DEVICE_ERROR The block device is not functioning correctly and
+ could not be written. Firmware device may have been
+ partially erased
**/
EFI_STATUS
EFIAPI
FvbProtocolEraseBlocks (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
...
)
{
@@ -846,6 +896,10 @@ FvbProtocolEraseBlocks (
DEBUG((DEBUG_INFO, "FvbProtocolEraseBlocks: \n"));
+ if (This == NULL) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
NumOfBlocks = FvbInstance->NumOfBlocks; @@ -923,30 +977,35 @@ FvbProtocolEraseBlocks (
@retval EFI_ACCESS_DENIED The firmware volume is in the WriteDisabled state
@retval EFI_DEVICE_ERROR The block device is not functioning correctly and
could not be written
- @retval EFI_INVALID_PARAMETER NumBytes or Buffer are NULL
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
**/
EFI_STATUS
EFIAPI
FvbProtocolWrite (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- IN EFI_LBA Lba,
- IN UINTN Offset,
- IN OUT UINTN *NumBytes,
- IN UINT8 *Buffer
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ IN EFI_LBA Lba,
+ IN UINTN Offset,
+ IN OUT UINTN *NumBytes,
+ IN UINT8 *Buffer
)
{
EFI_FVB_INSTANCE *FvbInstance;
+ if ((This == NULL) || (NumBytes == NULL) || (Buffer == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
- DEBUG((DEBUG_INFO,
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolWrite: Lba: 0x%lx Offset: 0x%x NumBytes: 0x%x, Buffer: 0x%x\n",
Lba,
Offset,
*NumBytes,
- Buffer)
- );
+ Buffer
+ ));
return FvbWriteBlock (FvbInstance, Lba, Offset, NumBytes, Buffer); } @@ -974,31 +1033,36 @@ FvbProtocolWrite (
@retval EFI_ACCESS_DENIED The firmware volume is in the ReadDisabled state
@retval EFI_DEVICE_ERROR The block device is not functioning correctly and
could not be read
- @retval EFI_INVALID_PARAMETER NumBytes or Buffer are NULL
+ @retval EFI_INVALID_PARAMETER A pointer argument provided is NULL.
**/
EFI_STATUS
EFIAPI
FvbProtocolRead (
- IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
- IN EFI_LBA Lba,
- IN UINTN Offset,
- IN OUT UINTN *NumBytes,
- OUT UINT8 *Buffer
+ IN CONST EFI_FIRMWARE_VOLUME_BLOCK_PROTOCOL *This,
+ IN EFI_LBA Lba,
+ IN UINTN Offset,
+ IN OUT UINTN *NumBytes,
+ OUT UINT8 *Buffer
)
{
EFI_FVB_INSTANCE *FvbInstance;
EFI_STATUS Status;
+ if ((This == NULL) || (NumBytes == NULL) || (Buffer == NULL)) {
+ return EFI_INVALID_PARAMETER;
+ }
+
FvbInstance = FVB_INSTANCE_FROM_THIS (This);
Status = FvbReadBlock (FvbInstance, Lba, Offset, NumBytes, Buffer);
- DEBUG((DEBUG_INFO,
+ DEBUG ((
+ DEBUG_INFO,
"FvbProtocolRead: Lba: 0x%lx Offset: 0x%x NumBytes: 0x%x, Buffer: 0x%x\n",
Lba,
Offset,
*NumBytes,
- Buffer)
- );
+ Buffer
+ ));
return Status;
}
diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c
index f9543c5ba677..5d7591480319 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.c
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbSe
+++ rviceMm.c
@@ -15,14 +15,11 @@
#include <Guid/VariableFormat.h>
/**
- The function installs EFI_FIRMWARE_VOLUME_BLOCK protocol
- for each FV in the system.
+ The function installs the given EFI_FIRMWARE_VOLUME_BLOCK protocol instance.
@param[in] FvbInstance The pointer to a FW volume instance structure,
which contains the information about one FV.
- @retval VOID
-
**/
VOID
InstallFvbProtocol (
@@ -33,8 +30,8 @@ InstallFvbProtocol (
EFI_STATUS Status;
EFI_HANDLE FvbHandle;
- ASSERT (FvbInstance != NULL);
if (FvbInstance == NULL) {
+ ASSERT (FvbInstance != NULL);
return;
}
@@ -48,14 +45,14 @@ InstallFvbProtocol (
//
// Set up the devicepath
//
- DEBUG ((DEBUG_INFO, "FwBlockService.c: Setting up DevicePath for 0x%lx:\n", FvbInstance->FvBase));
+ DEBUG ((DEBUG_INFO, "SpiFvbServiceSmm.c: Setting up DevicePath for
+ 0x%lx:\n", FvbInstance->FvBase));
if (FvHeader->ExtHeaderOffset == 0) {
//
// FV does not contains extension header, then produce MEMMAP_DEVICE_PATH
//
FvbInstance->DevicePath = (EFI_DEVICE_PATH_PROTOCOL *) AllocateRuntimeCopyPool (sizeof (FV_MEMMAP_DEVICE_PATH), &mFvMemmapDevicePathTemplate);
if (FvbInstance->DevicePath == NULL) {
- DEBUG ((DEBUG_INFO, "SpiFvbServiceSmm.c: Memory allocation for MEMMAP_DEVICE_PATH failed\n"));
+ DEBUG ((DEBUG_ERROR, "SpiFvbServiceSmm.c: Memory allocation for
+ MEMMAP_DEVICE_PATH failed\n"));
return;
}
((FV_MEMMAP_DEVICE_PATH *) FvbInstance->DevicePath)->MemMapDevPath.StartingAddress = FvbInstance->FvBase; @@ -63,7 +60,7 @@ InstallFvbProtocol (
} else {
FvbInstance->DevicePath = (EFI_DEVICE_PATH_PROTOCOL *) AllocateRuntimeCopyPool (sizeof (FV_PIWG_DEVICE_PATH), &mFvPIWGDevicePathTemplate);
if (FvbInstance->DevicePath == NULL) {
- DEBUG ((DEBUG_INFO, "SpiFvbServiceSmm.c: Memory allocation for FV_PIWG_DEVICE_PATH failed\n"));
+ DEBUG ((DEBUG_ERROR, "SpiFvbServiceSmm.c: Memory allocation for
+ FV_PIWG_DEVICE_PATH failed\n"));
return;
}
CopyGuid (
@@ -96,7 +93,7 @@ InstallFvbProtocol (
/**
The function does the necessary initialization work for
- Firmware Volume Block Driver.
+ the Firmware Volume Block Driver.
**/
VOID
@@ -148,10 +145,6 @@ FvbInitialize (
mPlatformFvBaseAddress[1].FvBase = PcdGet32(PcdFlashMicrocodeFvBase);
mPlatformFvBaseAddress[1].FvSize = PcdGet32(PcdFlashMicrocodeFvSize);
- //
- // We will only continue with FVB installation if the
- // SPI is the active BIOS state
- //
{
//
// Make sure all FVB are valid and/or fix if possible @@ -167,11 +160,20 @@ FvbInitialize (
if (!IsFvHeaderValid (BaseAddress, FvHeader)) {
BytesWritten = 0;
BytesErased = 0;
- DEBUG ((DEBUG_ERROR, "ERROR - The FV in 0x%x is invalid!\n", FvHeader));
+ DEBUG ((DEBUG_ERROR, "ERROR - The FV at 0x%x is invalid!\n",
+ FvHeader));
+
+ //
+ // Attempt to recover the FV
+ //
FvHeader = NULL;
- Status = GetFvbInfo (BaseAddress, &FvHeader);
+ Status = GetGeneratedFvByAddress (BaseAddress, &FvHeader);
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_WARN, "ERROR - Can't recovery FV header at 0x%x. GetFvbInfo Status %r\n", BaseAddress, Status));
+ DEBUG ((
+ DEBUG_WARN | DEBUG_ERROR,
+ "ERROR - Can't recover FV header at 0x%x. GetGeneratedFvByAddress Status %r\n",
+ BaseAddress,
+ Status
+ ));
continue;
}
DEBUG ((DEBUG_INFO, "Rewriting FV header at 0x%X with static data\n", BaseAddress)); @@ -181,15 +183,15 @@ FvbInitialize (
BytesErased = (UINTN) FvHeader->BlockMap->Length;
Status = SpiFlashBlockErase( (UINTN) BaseAddress, &BytesErased);
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_WARN, "ERROR - SpiFlashBlockErase Error %r\n", Status));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - SpiFlashBlockErase
+ Error %r\n", Status));
if (FvHeader != NULL) {
FreePool (FvHeader);
}
continue;
}
if (BytesErased != FvHeader->BlockMap->Length) {
- DEBUG ((DEBUG_WARN, "ERROR - BytesErased != FvHeader->BlockMap->Length\n"));
- DEBUG ((DEBUG_INFO, " BytesErased = 0x%X\n Length = 0x%X\n", BytesErased, FvHeader->BlockMap->Length));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - BytesErased != FvHeader->BlockMap->Length\n"));
+ DEBUG ((DEBUG_ERROR, " BytesErased = 0x%X\n Length = 0x%X\n",
+ BytesErased, FvHeader->BlockMap->Length));
if (FvHeader != NULL) {
FreePool (FvHeader);
}
@@ -249,15 +251,15 @@ FvbInitialize (
}
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_WARN, "ERROR - SpiFlashWrite Error %r\n", Status));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - SpiFlashWrite
+ Error %r\n", Status));
if (FvHeader != NULL) {
FreePool (FvHeader);
}
continue;
}
if (BytesWritten != ExpectedBytesWritten) {
- DEBUG ((DEBUG_WARN, "ERROR - BytesWritten != ExpectedBytesWritten\n"));
- DEBUG ((DEBUG_INFO, " BytesWritten = 0x%X\n ExpectedBytesWritten = 0x%X\n", BytesWritten, ExpectedBytesWritten));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - BytesWritten != ExpectedBytesWritten\n"));
+ DEBUG ((DEBUG_ERROR, " BytesWritten = 0x%X\n
+ ExpectedBytesWritten = 0x%X\n", BytesWritten, ExpectedBytesWritten));
if (FvHeader != NULL) {
FreePool (FvHeader);
}
@@ -265,13 +267,13 @@ FvbInitialize (
}
Status = SpiFlashLock ();
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_WARN, "ERROR - SpiFlashLock Error %r\n", Status));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - SpiFlashLock Error
+ %r\n", Status));
if (FvHeader != NULL) {
FreePool (FvHeader);
}
continue;
}
- DEBUG ((DEBUG_INFO, "FV Header @ 0x%X restored with static data\n", BaseAddress));
+ DEBUG ((DEBUG_ERROR, "FV Header @ 0x%X restored with static
+ data\n", BaseAddress));
//
// Clear cache for this range.
//
@@ -294,7 +296,7 @@ FvbInitialize (
FvHeader = (EFI_FIRMWARE_VOLUME_HEADER *) (UINTN) BaseAddress;
if (!IsFvHeaderValid (BaseAddress, FvHeader)) {
- DEBUG ((DEBUG_WARN, "ERROR - The FV in 0x%x is invalid!\n", FvHeader));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - The FV in 0x%x is
+ invalid!\n", FvHeader));
continue;
}
@@ -322,7 +324,7 @@ FvbInitialize (
FvHeader = (EFI_FIRMWARE_VOLUME_HEADER *) (UINTN) BaseAddress;
if (!IsFvHeaderValid (BaseAddress, FvHeader)) {
- DEBUG ((DEBUG_WARN, "ERROR - The FV in 0x%x is invalid!\n", FvHeader));
+ DEBUG ((DEBUG_WARN | DEBUG_ERROR, "ERROR - The FV in 0x%x is
+ invalid!\n", FvHeader));
continue;
}
diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h
index 7b0908b03fdc..7664670457a2 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceCommon.h
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbSe
+++ rviceCommon.h
@@ -1,14 +1,14 @@
/** @file
Common source definitions used in serial flash drivers
-Copyright (c) 2017, Intel Corporation. All rights reserved.<BR> -Copyright (c) Microsoft Corporation.<BR>
-SPDX-License-Identifier: BSD-2-Clause-Patent
+ Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) Microsoft Corporation.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
**/
-#ifndef _SPI_FVB_SERVICE_COMMON_H
-#define _SPI_FVB_SERVICE_COMMON_H
+#ifndef SPI_FVB_SERVICE_COMMON_H_
+#define SPI_FVB_SERVICE_COMMON_H_
#include <Guid/EventGroup.h>
#include <Guid/FirmwareFileSystem2.h>
@@ -61,7 +61,7 @@ typedef struct {
} FVB_GLOBAL;
//
-// Fvb Protocol instance data
+// Firmware Volume Block (FVB) Protocol instance data
//
#define FVB_INSTANCE_FROM_THIS(a) CR(a, EFI_FVB_INSTANCE, FvbProtocol, FVB_INSTANCE_SIGNATURE)
@@ -81,7 +81,7 @@ typedef struct {
} FV_INFO;
//
-// Protocol APIs
+// Firmware Volume Block (FVB) Protocol APIs
//
EFI_STATUS
EFIAPI
@@ -146,8 +146,23 @@ IsFvHeaderValid (
IN CONST EFI_FIRMWARE_VOLUME_HEADER *FwVolHeader
);
+//
+// Module Local Functions
+//
+
+/**
+ Returns an empty firmware volume for the firmware volume at the given base address.
+
+ @param[in] FvBaseAddress The base address of the firmware volume requested.
+ @param[out] FvbInfo A pointer that will be set to a buffer for the firmware volume header
+ at the given base address.
+
+ @retval EFI_SUCCESS The firmware volume was returned successfully.
+ @retval EFI_NOT_FOUND The firmware volume was not found for the given base address.
+
+**/
EFI_STATUS
-GetFvbInfo (
+GetGeneratedFvByAddress (
IN EFI_PHYSICAL_ADDRESS FvBaseAddress,
OUT EFI_FIRMWARE_VOLUME_HEADER **FvbInfo
);
diff --git a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.h b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.h
index 36af1130c8ee..512844197a4d 100644
--- a/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbServiceMm.h
+++ b/Silicon/Intel/IntelSiliconPkg/Feature/Flash/SpiFvbService/SpiFvbSe
+++ rviceMm.h
@@ -6,12 +6,12 @@
**/
-#ifndef _SPI_FVB_SERVICE_MM_H_
-#define _SPI_FVB_SERVICE_MM_H_
+#ifndef SPI_FVB_SERVICE_MM_H_
+#define SPI_FVB_SERVICE_MM_H_
/**
The function does the necessary initialization work for
- Firmware Volume Block Driver.
+ the Firmware Volume Block Driver.
**/
VOID
--
2.41.0.windows.1
-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106553): https://edk2.groups.io/g/devel/message/106553
Mute This Topic: https://groups.io/mt/99866031/6226280
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [ashraf.ali.s@intel.com] -=-=-=-=-=-=
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-06-30 20:47 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-30 2:32 [edk2-platforms][PATCH v1 1/1] IntelSiliconPkg/SpiFvbService: Non-functional cleanup Michael Kubacki
2023-06-30 18:57 ` [edk2-devel] " Ashraf Ali S
2023-06-30 20:21 ` Isaac Oram
[not found] ` <176D896904216960.17093@groups.io>
2023-06-30 20:47 ` Isaac Oram
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox