public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [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