From: Marcin Wojtas <mw@semihalf.com>
To: Leif Lindholm <leif.lindholm@linaro.org>
Cc: edk2-devel-01 <edk2-devel@lists.01.org>,
Ard Biesheuvel <ard.biesheuvel@linaro.org>,
nadavh@marvell.com, Neta Zur Hershkovits <neta@marvell.com>,
Kostya Porotchkin <kostap@marvell.com>,
Hua Jing <jinghua@marvell.com>,
semihalf-dabros-jan <jsd@semihalf.com>
Subject: Re: [platforms: PATCH v2 1/4] Platform/Marvell: Introduce MvFvbDxe variable support driver
Date: Sun, 26 Nov 2017 14:38:18 +0100 [thread overview]
Message-ID: <CAPv3WKe-CdvYd+ZfJv-J_P3e295ygL09jTDQvdVoh1PFCb_baA@mail.gmail.com> (raw)
In-Reply-To: <20171125140911.dms6f5qc26oxvfjg@bivouac.eciton.net>
Hi Leif,
2017-11-25 15:09 GMT+01:00 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.
It's two spaces from the beginning of function name. Anyway, I'll
align &gEfiAuthenticatedVariableGuid to &VariableStoreHeader,
>
> > + 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*.
Ok, I'll change it then. I only referred to existing edk2-platforms
implementations (AMD - 'return EFI_SUCCESS', Hisilicon/Socionext -
'return EFI_UNSUPPORTED' - all with doing nothing with the actual
attributes list). Same with ArmPlatformPkg, only Intel seems to
implement this.
>
> > +}
> > +
> > +/**
> > + 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.
Sorry for that. I'll call the GetAttributes routine and compare with
attributes then.
> ---
> (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?
>
> /
As mentioned above, I referred to other edk2-platforms
implementations, which do not support setting the attributes and check
WriteDisable via Media.ReadOnly. I will modify my implementation.
Thanks for your review.
Best regards,
Marcin
next prev parent reply other threads:[~2017-11-26 13:33 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-21 6:46 [platforms: PATCH v2 0/4] Armada 7k/8k variable support Marcin Wojtas
2017-11-21 6:46 ` [platforms: PATCH v2 1/4] Platform/Marvell: Introduce MvFvbDxe variable support driver Marcin Wojtas
2017-11-25 14:09 ` Leif Lindholm
2017-11-26 13:38 ` Marcin Wojtas [this message]
2017-11-26 14:37 ` Leif Lindholm
2017-11-21 6:46 ` [platforms: PATCH v2 2/4] Marvell/Drivers: MvSpiFlash: Enable using driver in RT Marcin Wojtas
2017-11-21 6:46 ` [platforms: PATCH v2 3/4] Marvell/Drivers: MvSpiDxe: " Marcin Wojtas
2017-11-21 6:46 ` [platforms: PATCH v2 4/4] Marvell/Armada: Enable variables support Marcin Wojtas
2017-11-25 14:16 ` [platforms: PATCH v2 0/4] Armada 7k/8k variable support Leif Lindholm
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAPv3WKe-CdvYd+ZfJv-J_P3e295ygL09jTDQvdVoh1PFCb_baA@mail.gmail.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox