Re: [platforms: PATCH v2 1/4] Platform/Marvell: Introduce MvFvbDxe variable support driver

[Leif Lindholm <leif.lindholm@linaro.org>]

On Tue, Nov 21, 2017 at 07:46:18AM +0100, Marcin Wojtas wrote:
> MvFvbDxe driver introduces non-volatile EFI variable support
> for Armada platforms. It relies on memory-mapped SPI read access.
> Implementation of EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL
> is done with using existing Marvell SPI infrastructure
> (SpiMasterProtocol and SpiFlashProtocol), thanks to which
> this driver will be able to support various combinations of
> flash devices and host controllers.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marcin Wojtas <mw@semihalf.com>
> ---
>  Platform/Marvell/Drivers/Spi/Variables/MvFvbDxe.c   | 1056 ++++++++++++++++++++
>  Platform/Marvell/Drivers/Spi/Variables/MvFvbDxe.h   |  114 +++
>  Platform/Marvell/Drivers/Spi/Variables/MvFvbDxe.inf |   91 ++
>  Platform/Marvell/Marvell.dec                        |    1 +
>  4 files changed, 1262 insertions(+)
> 
> diff --git a/Platform/Marvell/Drivers/Spi/Variables/MvFvbDxe.c b/Platform/Marvell/Drivers/Spi/Variables/MvFvbDxe.c
> new file mode 100644
> index 0000000..7a44f80
> --- /dev/null
> +++ b/Platform/Marvell/Drivers/Spi/Variables/MvFvbDxe.c
> @@ -0,0 +1,1056 @@
> +/*++ @file  MvFvbDxe.c
> +
> + Copyright (c) 2011 - 2014, ARM Ltd. All rights reserved.<BR>
> + Copyright (c) 2017 Marvell International Ltd.<BR>
> +
> + This program and the accompanying materials are licensed and made available
> + under the terms and conditions of the BSD License which accompanies this
> + distribution.  The full text of the license may be found at
> + http://opensource.org/licenses/bsd-license.php
> +
> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> + --*/
> +
> +#include <PiDxe.h>
> +
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/DxeServicesTableLib.h>
> +#include <Library/HobLib.h>
> +#include <Library/MemoryAllocationLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/UefiLib.h>
> +#include <Library/UefiRuntimeLib.h>
> +
> +#include <Guid/SystemNvDataGuid.h>
> +#include <Guid/VariableFormat.h>
> +
> +#include "MvFvbDxe.h"
> +
> +STATIC EFI_EVENT     mFvbVirtualAddrChangeEvent;
> +STATIC FVB_DEVICE    *mFvbDevice;
> +
> +STATIC CONST FVB_DEVICE mMvFvbFlashInstanceTemplate = {
> +  {
> +    0,    // SpiFlash Chip Select ... NEED TO BE FILLED
> +    0,    // SpiFlash Maximum Frequency ... NEED TO BE FILLED
> +    0,    // SpiFlash Transfer Mode ... NEED TO BE FILLED
> +    0,    // SpiFlash Address Size ... NEED TO BE FILLED
> +    NULL, // SpiFlash detailed information ... NEED TO BE FILLED
> +    0,    // HostRegisterBaseAddress ... NEED TO BE FILLED
> +    0,    // CoreClock ... NEED TO BE FILLED
> +  }, // SpiDevice
> +
> +  NULL, // SpiFlashProtocol ... NEED TO BE FILLED
> +  NULL, // SpiMasterProtocol ... NEED TO BE FILLED
> +  NULL, // Handle ... NEED TO BE FILLED
> +
> +  FVB_FLASH_SIGNATURE, // Signature
> +
> +  0, // DeviceBaseAddress ... NEED TO BE FILLED
> +  0, // RegionBaseAddress ... NEED TO BE FILLED
> +  SIZE_256KB, // Size
> +  0, // FvbOffset ... NEED TO BE FILLED
> +  0, // FvbSize ... NEED TO BE FILLED
> +  0, // StartLba
> +
> +  {
> +    0,     // MediaId ... NEED TO BE FILLED
> +    FALSE, // RemovableMedia
> +    TRUE,  // MediaPresent
> +    FALSE, // LogicalPartition
> +    FALSE, // ReadOnly
> +    FALSE, // WriteCaching;
> +    0,     // BlockSize ... NEED TO BE FILLED
> +    4,     // IoAlign
> +    0,     // LastBlock ... NEED TO BE FILLED
> +    0,     // LowestAlignedLba
> +    1,     // LogicalBlocksPerPhysicalBlock
> +  }, //Media;
> +
> +  {
> +    MvFvbGetAttributes,       // GetAttributes
> +    MvFvbSetAttributes,       // SetAttributes
> +    MvFvbGetPhysicalAddress,  // GetPhysicalAddress
> +    MvFvbGetBlockSize,        // GetBlockSize
> +    MvFvbRead,                // Read
> +    MvFvbWrite,               // Write
> +    MvFvbEraseBlocks,         // EraseBlocks
> +    NULL,                     // ParentHandle
> +  }, //  FvbProtocol;
> +
> +  {
> +    {
> +      {
> +        HARDWARE_DEVICE_PATH,
> +        HW_VENDOR_DP,
> +        {
> +          (UINT8)sizeof (VENDOR_DEVICE_PATH),
> +          (UINT8)((sizeof (VENDOR_DEVICE_PATH)) >> 8)
> +        }
> +      },
> +      { 0xfc0cb972, 0x21df, 0x44d2, { 0x92, 0xa5, 0x78, 0x98, 0x99, 0xcb, 0xf6, 0x61 } }
> +    },
> +    {
> +      END_DEVICE_PATH_TYPE,
> +      END_ENTIRE_DEVICE_PATH_SUBTYPE,
> +      { sizeof (EFI_DEVICE_PATH_PROTOCOL), 0 }
> +    }
> +  } // DevicePath
> +};
> +
> +//
> +// The Firmware Volume Block Protocol is the low-level interface
> +// to a firmware volume. File-level access to a firmware volume
> +// should not be done using the Firmware Volume Block Protocol.
> +// Normal access to a firmware volume must use the Firmware
> +// Volume Protocol. Typically, only the file system driver that
> +// produces the Firmware Volume Protocol will bind to the
> +// Firmware Volume Block Protocol.
> +//
> +
> +/**
> +  Initialises the FV Header and Variable Store Header
> +  to support variable operations.
> +
> +  @param[in]  Ptr - Location to initialise the headers
> +
> +**/
> +STATIC
> +EFI_STATUS
> +MvFvbInitFvAndVariableStoreHeaders (
> +  IN FVB_DEVICE *FlashInstance
> +  )
> +{
> +  EFI_FIRMWARE_VOLUME_HEADER  *FirmwareVolumeHeader;
> +  VARIABLE_STORE_HEADER       *VariableStoreHeader;
> +  EFI_STATUS                  Status;
> +  VOID*                       Headers;
> +  UINTN                       HeadersLength;
> +  UINTN                       BlockSize;
> +
> +  HeadersLength = sizeof (EFI_FIRMWARE_VOLUME_HEADER) +
> +                  sizeof (EFI_FV_BLOCK_MAP_ENTRY) +
> +                  sizeof (VARIABLE_STORE_HEADER);
> +  Headers = AllocateZeroPool (HeadersLength);
> +
> +  BlockSize = FlashInstance->Media.BlockSize;
> +
> +  //
> +  // FirmwareVolumeHeader->FvLength is declared to have the Variable area
> +  // AND the FTW working area AND the FTW Spare contiguous.
> +  //
> +  ASSERT (PcdGet32 (PcdFlashNvStorageVariableBase) +
> +    PcdGet32 (PcdFlashNvStorageVariableSize) ==
> +    PcdGet32 (PcdFlashNvStorageFtwWorkingBase));
> +  ASSERT (PcdGet32 (PcdFlashNvStorageFtwWorkingBase) +
> +    PcdGet32 (PcdFlashNvStorageFtwWorkingSize) ==
> +    PcdGet32 (PcdFlashNvStorageFtwSpareBase));
> +
> +  // Check if the size of the area is at least one block size
> +  ASSERT ((PcdGet32 (PcdFlashNvStorageVariableSize) > 0) &&
> +    (PcdGet32 (PcdFlashNvStorageVariableSize) / BlockSize > 0));
> +  ASSERT ((PcdGet32 (PcdFlashNvStorageFtwWorkingSize) > 0) &&
> +    (PcdGet32 (PcdFlashNvStorageFtwWorkingSize) / BlockSize > 0));
> +  ASSERT ((PcdGet32 (PcdFlashNvStorageFtwSpareSize) > 0) &&
> +    (PcdGet32 (PcdFlashNvStorageFtwSpareSize) / BlockSize > 0));
> +
> +  // Ensure the Variable areas are aligned on block size boundaries
> +  ASSERT ((PcdGet32 (PcdFlashNvStorageVariableBase) % BlockSize) == 0);
> +  ASSERT ((PcdGet32 (PcdFlashNvStorageFtwWorkingBase) % BlockSize) == 0);
> +  ASSERT ((PcdGet32 (PcdFlashNvStorageFtwSpareBase) % BlockSize) == 0);
> +
> +  //
> +  // EFI_FIRMWARE_VOLUME_HEADER
> +  //
> +  FirmwareVolumeHeader = (EFI_FIRMWARE_VOLUME_HEADER*)Headers;
> +  CopyGuid (&FirmwareVolumeHeader->FileSystemGuid, &gEfiSystemNvDataFvGuid);
> +  FirmwareVolumeHeader->FvLength = FlashInstance->FvbSize;
> +  FirmwareVolumeHeader->Signature = EFI_FVH_SIGNATURE;
> +  FirmwareVolumeHeader->Attributes = EFI_FVB2_READ_ENABLED_CAP |
> +                                     EFI_FVB2_READ_STATUS |
> +                                     EFI_FVB2_STICKY_WRITE |
> +                                     EFI_FVB2_MEMORY_MAPPED |
> +                                     EFI_FVB2_ERASE_POLARITY |
> +                                     EFI_FVB2_WRITE_STATUS |
> +                                     EFI_FVB2_WRITE_ENABLED_CAP;
> +
> +  FirmwareVolumeHeader->HeaderLength = sizeof (EFI_FIRMWARE_VOLUME_HEADER) +
> +                                       sizeof (EFI_FV_BLOCK_MAP_ENTRY);
> +  FirmwareVolumeHeader->Revision = EFI_FVH_REVISION;
> +  FirmwareVolumeHeader->BlockMap[0].NumBlocks = FlashInstance->Media.LastBlock + 1;
> +  FirmwareVolumeHeader->BlockMap[0].Length = FlashInstance->Media.BlockSize;
> +  FirmwareVolumeHeader->BlockMap[1].NumBlocks = 0;
> +  FirmwareVolumeHeader->BlockMap[1].Length = 0;
> +  FirmwareVolumeHeader->Checksum = CalculateCheckSum16 (
> +                                     (UINT16 *)FirmwareVolumeHeader,
> +                                     FirmwareVolumeHeader->HeaderLength);
> +
> +  //
> +  // VARIABLE_STORE_HEADER
> +  //
> +  VariableStoreHeader = (VOID *)((UINTN)Headers +
> +                                 FirmwareVolumeHeader->HeaderLength);
> +  CopyGuid (&VariableStoreHeader->Signature, &gEfiAuthenticatedVariableGuid);
> +  VariableStoreHeader->Size = PcdGet32(PcdFlashNvStorageVariableSize) -
> +                              FirmwareVolumeHeader->HeaderLength;
> +  VariableStoreHeader->Format = VARIABLE_STORE_FORMATTED;
> +  VariableStoreHeader->State = VARIABLE_STORE_HEALTHY;
> +
> +  // Install the combined super-header in the flash device
> +  Status = MvFvbWrite (&FlashInstance->FvbProtocol, 0, 0, &HeadersLength, Headers);
> +
> +  FreePool (Headers);
> +
> +  return Status;
> +}
> +
> +/**
> +  Check the integrity of firmware volume header.
> +
> +  @param[in] FwVolHeader - A pointer to a firmware volume header
> +
> +  @retval  EFI_SUCCESS   - The firmware volume is consistent
> +  @retval  EFI_NOT_FOUND - The firmware volume has been corrupted.
> +
> +**/
> +STATIC
> +EFI_STATUS
> +MvFvbValidateFvHeader (
> +  IN  FVB_DEVICE *FlashInstance
> +  )
> +{
> +  UINT16                      Checksum;
> +  EFI_FIRMWARE_VOLUME_HEADER  *FwVolHeader;
> +  VARIABLE_STORE_HEADER       *VariableStoreHeader;
> +  UINTN                       VariableStoreLength;
> +
> +  FwVolHeader = (EFI_FIRMWARE_VOLUME_HEADER *)FlashInstance->RegionBaseAddress;
> +
> +  // Verify the header revision, header signature, length
> +  if ((FwVolHeader->Revision  != EFI_FVH_REVISION) ||
> +      (FwVolHeader->Signature != EFI_FVH_SIGNATURE) ||
> +      (FwVolHeader->FvLength  != FlashInstance->FvbSize)) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: No Firmware Volume header present\n",
> +      __FUNCTION__));
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  // Check the Firmware Volume Guid
> +  if (!CompareGuid (&FwVolHeader->FileSystemGuid, &gEfiSystemNvDataFvGuid)) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: Firmware Volume Guid non-compatible\n",
> +      __FUNCTION__));
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  // Verify the header checksum
> +  Checksum = CalculateSum16 ((UINT16 *)FwVolHeader, FwVolHeader->HeaderLength);
> +  if (Checksum != 0) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: FV checksum is invalid (Checksum:0x%x)\n",
> +      __FUNCTION__,
> +      Checksum));
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  VariableStoreHeader = (VOID *)((UINTN)FwVolHeader + FwVolHeader->HeaderLength);
> +
> +  // Check the Variable Store Guid
> +  if (!CompareGuid (&VariableStoreHeader->Signature, &gEfiVariableGuid) &&
> +      !CompareGuid (&VariableStoreHeader->Signature,
> +         &gEfiAuthenticatedVariableGuid)) {

Still spurious indentation. (If it's meant to indicate continuation,
that's fine, but that's two spaces, not three. Although I would find
      !CompareGuid (&VariableStoreHeader->Signature,
                    &gEfiAuthenticatedVariableGuid)) {
more clear.

> +    DEBUG ((DEBUG_ERROR,
> +      "%a: Variable Store Guid non-compatible\n",
> +      __FUNCTION__));
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  VariableStoreLength = PcdGet32 (PcdFlashNvStorageVariableSize) -
> +                        FwVolHeader->HeaderLength;
> +  if (VariableStoreHeader->Size != VariableStoreLength) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: Variable Store Length does not match\n",
> +      __FUNCTION__));
> +    return EFI_NOT_FOUND;
> +  }
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> + The GetAttributes() function retrieves the attributes and
> + current settings of the block.
> +
> + @param This         Indicates the EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL instance.
> +
> + @param Attributes   Pointer to EFI_FVB_ATTRIBUTES_2 in which the attributes and
> +                     current settings are returned.
> +                     Type EFI_FVB_ATTRIBUTES_2 is defined in
> +                     EFI_FIRMWARE_VOLUME_HEADER.
> +
> + @retval EFI_SUCCESS The firmware volume attributes were returned.
> +
> + **/
> +EFI_STATUS
> +EFIAPI
> +MvFvbGetAttributes (
> +  IN CONST  EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL    *This,
> +  OUT       EFI_FVB_ATTRIBUTES_2                   *Attributes
> +  )
> +{
> +  EFI_FVB_ATTRIBUTES_2  FlashFvbAttributes;
> +  FVB_DEVICE            *FlashInstance;
> +
> +  FlashInstance = INSTANCE_FROM_FVB_THIS (This);
> +
> +  FlashFvbAttributes = EFI_FVB2_READ_ENABLED_CAP | EFI_FVB2_READ_STATUS |
> +                       EFI_FVB2_STICKY_WRITE | EFI_FVB2_MEMORY_MAPPED |
> +                       EFI_FVB2_ERASE_POLARITY;
> +
> +  // Check if it is write protected
> +  if (!FlashInstance->Media.ReadOnly) {
> +    FlashFvbAttributes |= EFI_FVB2_WRITE_STATUS | EFI_FVB2_WRITE_ENABLED_CAP;
> +  }
> +
> +  *Attributes = FlashFvbAttributes;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> + The SetAttributes() function sets configurable firmware volume attributes
> + and returns the new settings of the firmware volume.
> +
> +
> + @param This                     EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL instance.
> +
> + @param Attributes               On input, Attributes is a pointer to
> +                                 EFI_FVB_ATTRIBUTES_2 that contains the desired
> +                                 firmware volume settings.
> +                                 On successful return, it contains the new
> +                                 settings of the firmware volume.
> +
> + @retval EFI_SUCCESS             The firmware volume attributes were returned.
> +
> + @retval EFI_INVALID_PARAMETER   The attributes requested are in conflict with
> +                                 the capabilities as declared in the firmware
> +                                 volume header.
> +
> + **/
> +EFI_STATUS
> +EFIAPI
> +MvFvbSetAttributes (
> +  IN CONST  EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL  *This,
> +  IN OUT    EFI_FVB_ATTRIBUTES_2                 *Attributes
> +  )
> +{
> +  DEBUG ((DEBUG_BLKIO,
> +    "%a: Operation not supported, keep default set of attributes\n",
> +    __FUNCTION__));
> +
> +  return MvFvbGetAttributes (This, Attributes);

I'm still not completely thrilled about this.
Sure, the return value is always SUCCESS now.

But the implicit meaning of the specification is that
*this*support*is*not*optional*.

> +}
> +
> +/**
> + The GetPhysicalAddress() function retrieves the base address of
> + a memory-mapped firmware volume. This function should be called
> + only for memory-mapped firmware volumes.
> +
> + @param This               EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL instance.
> +
> + @param Address            Pointer to a caller-allocated
> +                           EFI_PHYSICAL_ADDRESS that, on successful
> +                           return from GetPhysicalAddress(), contains the
> +                           base address of the firmware volume.
> +
> + @retval EFI_SUCCESS       The firmware volume base address was returned.
> +
> + @retval EFI_NOT_SUPPORTED The firmware volume is not memory mapped.
> +
> + **/
> +EFI_STATUS
> +EFIAPI
> +MvFvbGetPhysicalAddress (
> +  IN CONST  EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL  *This,
> +  OUT       EFI_PHYSICAL_ADDRESS                 *Address
> +  )
> +{
> +  FVB_DEVICE *FlashInstance;
> +
> +  ASSERT (Address != NULL);
> +
> +  FlashInstance = INSTANCE_FROM_FVB_THIS (This);
> +
> +  *Address = FlashInstance->RegionBaseAddress;
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> + The GetBlockSize() function retrieves the size of the requested
> + block. It also returns the number of additional blocks with
> + the identical size. The GetBlockSize() function is used to
> + retrieve the block map (see EFI_FIRMWARE_VOLUME_HEADER).
> +
> +
> + @param This                     EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL instance.
> +
> + @param Lba                      Indicates the block whose size to return
> +
> + @param BlockSize                Pointer to a caller-allocated UINTN in which
> +                                 the size of the block is returned.
> +
> + @param NumberOfBlocks           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.
> +
> +
> + @retval EFI_SUCCESS             The firmware volume base address was returned.
> +
> + @retval EFI_INVALID_PARAMETER   The requested LBA is out of range.
> +
> + **/
> +EFI_STATUS
> +EFIAPI
> +MvFvbGetBlockSize (
> +  IN CONST  EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL  *This,
> +  IN        EFI_LBA                              Lba,
> +  OUT       UINTN                                *BlockSize,
> +  OUT       UINTN                                *NumberOfBlocks
> +  )
> +{
> +  FVB_DEVICE *FlashInstance;
> +
> +  FlashInstance = INSTANCE_FROM_FVB_THIS (This);
> +
> +  if (Lba > FlashInstance->Media.LastBlock) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: Error: Requested LBA %ld is beyond the last available LBA (%ld).\n",
> +      __FUNCTION__,
> +      Lba,
> +      FlashInstance->Media.LastBlock));
> +    return EFI_INVALID_PARAMETER;
> +  } else {
> +    // Assume equal sized blocks in all flash devices
> +    *BlockSize = (UINTN)FlashInstance->Media.BlockSize;
> +    *NumberOfBlocks = (UINTN)(FlashInstance->Media.LastBlock - Lba + 1);
> +
> +    return EFI_SUCCESS;
> +  }
> +}
> +
> +/**
> + Reads the specified number of bytes into a buffer from the specified block.
> +
> + The Read() function reads the requested number of bytes from the
> + requested block and stores them in the provided buffer.
> + Implementations should be mindful that the firmware volume
> + might be in the ReadDisabled state. If it is in this state,
> + the Read() function must return the status code
> + EFI_ACCESS_DENIED without modifying the contents of the
> + buffer. The Read() function must also prevent spanning block
> + boundaries. If a read is requested that would span a block
> + boundary, the read must read up to the boundary but not
> + beyond. The output parameter NumBytes must be set to correctly
> + indicate the number of bytes actually read. The caller must be
> + aware that a read may be partially completed.
> +
> + @param This                 EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL instance.
> +
> + @param Lba                  The starting logical block index from which to read
> +
> + @param Offset               Offset into the block at which to begin reading.
> +
> + @param NumBytes             Pointer to a UINTN.
> +                             At entry, *NumBytes contains the total size of the
> +                             buffer.
> +                             At exit, *NumBytes contains the total number of
> +                             bytes read.
> +
> + @param Buffer               Pointer to a caller-allocated buffer that will be
> +                             used to hold the data that is read.
> +
> + @retval EFI_SUCCESS         The firmware volume was read successfully, and
> +                             contents are in Buffer.
> +
> + @retval EFI_BAD_BUFFER_SIZE Read attempted across an LBA boundary.
> +                             On output, NumBytes contains the total number of
> +                             bytes returned in Buffer.
> +
> + @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.
> +
> + **/
> +EFI_STATUS
> +EFIAPI
> +MvFvbRead (
> +  IN CONST  EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL   *This,
> +  IN        EFI_LBA                               Lba,
> +  IN        UINTN                                 Offset,
> +  IN OUT    UINTN                                 *NumBytes,
> +  IN OUT    UINT8                                 *Buffer
> +  )
> +{
> +  FVB_DEVICE   *FlashInstance;
> +  UINTN         BlockSize;
> +  UINTN         DataOffset;
> +
> +  FlashInstance = INSTANCE_FROM_FVB_THIS (This);
> +
> +
> +  // Cache the block size to avoid de-referencing pointers all the time
> +  BlockSize = FlashInstance->Media.BlockSize;
> +
> +  //
> +  // The read must not span block boundaries.
> +  // We need to check each variable individually because adding two large
> +  // values together overflows.
> +  //
> +  if (Offset               >= BlockSize ||
> +      *NumBytes            >  BlockSize ||
> +      (Offset + *NumBytes) >  BlockSize) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: Wrong buffer size: (Offset=0x%x + NumBytes=0x%x) > BlockSize=0x%x\n",
> +      __FUNCTION__,
> +      Offset,
> +      *NumBytes,
> +      BlockSize));
> +    return EFI_BAD_BUFFER_SIZE;
> +  }
> +
> +  // No bytes to read
> +  if (*NumBytes == 0) {
> +    return EFI_SUCCESS;
> +  }
> +
> +  DataOffset = GET_DATA_OFFSET (FlashInstance->RegionBaseAddress + Offset,
> +                 FlashInstance->StartLba + Lba,
> +                 FlashInstance->Media.BlockSize);
> +
> +  // Read the memory-mapped data
> +  CopyMem (Buffer, (UINTN *)DataOffset, *NumBytes);
> +
> +  return EFI_SUCCESS;
> +}
> +
> +/**
> + Writes the specified number of bytes from the input buffer to the block.
> +
> + The Write() function writes the specified number of bytes from
> + the provided buffer to the specified block and offset. If the
> + firmware volume is sticky write, the caller must ensure that
> + all the bits of the specified range to write are in the
> + EFI_FVB_ERASE_POLARITY state before calling the Write()
> + function, or else the result will be unpredictable. This
> + unpredictability arises because, for a sticky-write firmware
> + volume, a write may negate a bit in the EFI_FVB_ERASE_POLARITY
> + state but cannot flip it back again.  Before calling the
> + Write() function,  it is recommended for the caller to first call
> + the EraseBlocks() function to erase the specified block to
> + write. A block erase cycle will transition bits from the
> + (NOT)EFI_FVB_ERASE_POLARITY state back to the
> + EFI_FVB_ERASE_POLARITY state. Implementations should be
> + mindful that the firmware volume might be in the WriteDisabled
> + state. If it is in this state, the Write() function must
> + return the status code EFI_ACCESS_DENIED without modifying the
> + contents of the firmware volume. The Write() function must
> + also prevent spanning block boundaries. If a write is
> + requested that spans a block boundary, the write must store up
> + to the boundary but not beyond. The output parameter NumBytes
> + must be set to correctly indicate the number of bytes actually
> + written. The caller must be aware that a write may be
> + partially completed. All writes, partial or otherwise, must be
> + fully flushed to the hardware before the Write() service
> + returns.
> +
> + @param This                 EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL instance.
> +
> + @param Lba                  The starting logical block index to write to.
> +
> + @param Offset               Offset into the block at which to begin writing.
> +
> + @param NumBytes             The pointer to a UINTN.
> +                             At entry, *NumBytes contains the total size of the
> +                             buffer.
> +                             At exit, *NumBytes contains the total number of
> +                             bytes actually written.
> +
> + @param Buffer               The 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 The write was attempted across an LBA boundary.
> +                             On output, NumBytes contains the total number of
> +                             bytes actually written.
> +
> + @retval EFI_ACCESS_DENIED   The firmware volume is in the WriteDisabled state.
> +
> + @retval EFI_DEVICE_ERROR    The block device is malfunctioning and could not be
> +                             written.
> +
> +
> + **/
> +EFI_STATUS
> +EFIAPI
> +MvFvbWrite (
> +  IN CONST  EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL   *This,
> +  IN        EFI_LBA                               Lba,
> +  IN        UINTN                                 Offset,
> +  IN OUT    UINTN                                 *NumBytes,
> +  IN        UINT8                                 *Buffer
> +  )
> +{
> +  FVB_DEVICE   *FlashInstance;
> +  UINTN         DataOffset;
> +
> +  FlashInstance = INSTANCE_FROM_FVB_THIS (This);
> +
> +  DataOffset = GET_DATA_OFFSET (FlashInstance->FvbOffset + Offset,
> +                 FlashInstance->StartLba + Lba,
> +                 FlashInstance->Media.BlockSize);
> +
> +  return FlashInstance->SpiFlashProtocol->Write (&FlashInstance->SpiDevice,
> +                                            DataOffset,
> +                                            *NumBytes,
> +                                            Buffer);
> +}
> +
> +/**
> + Erases and initialises a firmware volume block.
> +
> + The EraseBlocks() function erases one or more blocks as denoted
> + by the variable argument list. The entire parameter list of
> + blocks must be verified before erasing any blocks. If a block is
> + requested that does not exist within the associated firmware
> + volume (it has a larger index than the last block of the
> + firmware volume), the EraseBlocks() function must return the
> + status code EFI_INVALID_PARAMETER without modifying the contents
> + of the firmware volume. Implementations should be mindful that
> + the firmware volume might be in the WriteDisabled state. If it
> + is in this state, the EraseBlocks() function must return the
> + status code EFI_ACCESS_DENIED without modifying the contents of
> + the firmware volume. All calls to EraseBlocks() must be fully
> + flushed to the hardware before the EraseBlocks() service
> + returns.
> +
> + @param This                     EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL
> + instance.
> +
> + @param ...                      The variable argument list is a list of tuples.
> +                                 Each tuple describes a range of LBAs to erase
> +                                 and consists of the following:
> +                                 - An EFI_LBA that indicates the starting LBA
> +                                 - A UINTN that indicates the number of blocks
> +                                   to erase.
> +
> +                                 The list is terminated with an
> +                                 EFI_LBA_LIST_TERMINATOR.
> +
> + @retval EFI_SUCCESS             The erase request 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.
> +                                 The firmware device may have been partially
> +                                 erased.
> +
> + @retval EFI_INVALID_PARAMETER   One or more of the LBAs listed in the variable
> +                                 argument list do not exist in the firmware
> +                                 volume.
> +
> + **/
> +EFI_STATUS
> +EFIAPI
> +MvFvbEraseBlocks (
> +  IN CONST EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL *This,
> +  ...
> +  )
> +{
> +  EFI_STATUS   Status;
> +  VA_LIST      Args;
> +  UINTN        BlockAddress; // Physical address of Lba to erase
> +  EFI_LBA      StartingLba;  // Lba from which we start erasing
> +  UINTN        NumOfLba;     // Number of Lba blocks to erase
> +  FVB_DEVICE  *FlashInstance;
> +
> +  FlashInstance = INSTANCE_FROM_FVB_THIS (This);
> +
> +  Status = EFI_SUCCESS;
> +
> +  // Detect WriteDisabled state
> +  if (FlashInstance->Media.ReadOnly) {
> +    DEBUG ((DEBUG_ERROR,
> +      "%a: Device is in WriteDisabled state.\n",
> +      __FUNCTION__));
> +    return EFI_ACCESS_DENIED;
> +  }

No response to comment:
---
I see other implementations doing
  if ((Attributes & EFI_FVB2_WRITE_STATUS) == 0) {
as well, or instead.
---
(although the messages have been improved)

Together with my comment above, it basically looks like this code is
implementing its own layer of Attributes instead of implementing
attribute handling. Am I misunderstanding this?

/
    Leif