Re: [PATCH 2/5] OvmfPkg/EmuVariableFvbRuntimeDxe: change block size to 4KB

[Jordan Justen <jordan.l.justen@intel.com>]

On 2017-05-06 12:30:20, Laszlo Ersek wrote:
> EmuVariableFvbRuntimeDxe currently produces a Firmware Volume Block
> protocol that is based on a block map of two blocks, each block having
> PcdFlashNvStorageFtwSpareSize for size.
> 
> (The total size is 2 * PcdFlashNvStorageFtwSpareSize.)
> 
> FaultTolerantWriteDxe in turn expects the block size to be a power of two.
> 
> In the 4MB build of OVMF, PcdFlashNvStorageFtwSpareSize is 264KB, which is
> not a power of two. In order to equip EmuVariableFvbRuntimeDxe for this
> build, shrink the block size to 4KB (EFI_PAGE_SIZE), and grow the block
> count from 2 to EFI_SIZE_TO_PAGES(2 * PcdFlashNvStorageFtwSpareSize). The
> total size remains
> 
>   2 * PcdFlashNvStorageFtwSpareSize
>   --------------------------------- * EFI_PAGE_SIZE
>             EFI_PAGE_SIZE
> 
> Right now EmuVariableFvbRuntimeDxe open-codes the block count of 2 in
> various limit checks, so introduce a few new macros:
> - EMU_FVB_NUM_TOTAL_BLOCKS, for the LHS of the above product,
> - EMU_FVB_NUM_SPARE_BLOCKS for the half of that.
> 
> Also rework the FVB protocol members to support an arbitrary count of
> blocks.
> 
> Keep the invariant intact that the first half of the firmware volume hosts
> the variable store and the FTW working block, and that the second half
> maps the FTW spare area.
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=527
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h |  10 +-
>  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c | 147 +++++++++-----------
>  2 files changed, 75 insertions(+), 82 deletions(-)
> 
> diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h
> index 4247d21d72f8..beb11e3f9a90 100644
> --- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h
> +++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h
> @@ -58,8 +58,14 @@ typedef struct {
>  //
>  // Constants
>  //
> -#define EMU_FVB_BLOCK_SIZE (FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize))
> -#define EMU_FVB_SIZE (2 * FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize))
> +#define EMU_FVB_BLOCK_SIZE \
> +  EFI_PAGE_SIZE
> +#define EMU_FVB_NUM_SPARE_BLOCKS \
> +  EFI_SIZE_TO_PAGES ((UINTN)FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize))
> +#define EMU_FVB_NUM_TOTAL_BLOCKS \
> +  (2 * EMU_FVB_NUM_SPARE_BLOCKS)
> +#define EMU_FVB_SIZE \
> +  (EMU_FVB_NUM_TOTAL_BLOCKS * EMU_FVB_BLOCK_SIZE)
>  #define FTW_WRITE_QUEUE_SIZE \
>    (FixedPcdGet32 (PcdFlashNvStorageFtwWorkingSize) - \
>     sizeof (EFI_FAULT_TOLERANT_WORKING_BLOCK_HEADER))
> diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
> index 2f89632e5d75..11c8b1b75cb8 100644
> --- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
> +++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
> @@ -74,8 +74,8 @@ EFI_FW_VOL_BLOCK_DEVICE mEmuVarsFvb = {
>      }
>    },
>    NULL, // BufferPtr
> -  FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize), // BlockSize
> -  2 * FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize), // Size
> +  EMU_FVB_BLOCK_SIZE, // BlockSize
> +  EMU_FVB_SIZE, // Size
>    {     // FwVolBlockInstance
>      FvbProtocolGetAttributes,
>      FvbProtocolSetAttributes,
> @@ -185,14 +185,14 @@ FvbProtocolGetBlockSize (
>  {
>    EFI_FW_VOL_BLOCK_DEVICE *FvbDevice;
>  
> -  if (Lba > 1) {
> +  if (Lba >= EMU_FVB_NUM_TOTAL_BLOCKS) {
>      return EFI_INVALID_PARAMETER;
>    }
>  
>    FvbDevice = FVB_DEVICE_FROM_THIS (This);
>  
>    *BlockSize = FvbDevice->BlockSize;
> -  *NumberOfBlocks = (UINTN) (2 - (UINTN) Lba);
> +  *NumberOfBlocks = (UINTN)(EMU_FVB_NUM_TOTAL_BLOCKS - Lba);
>  
>    return EFI_SUCCESS;
>  }
> @@ -322,68 +322,58 @@ FvbProtocolEraseBlocks (
>    )
>  {
>    EFI_FW_VOL_BLOCK_DEVICE *FvbDevice;
> -  VA_LIST                 args;
> +  VA_LIST                 Args;
>    EFI_LBA                 StartingLba;
>    UINTN                   NumOfLba;
> -  UINT8                   Erase;
> -  VOID                    *ErasePtr;
> +  UINT8                   *ErasePtr;
>    UINTN                   EraseSize;
>  
>    FvbDevice = FVB_DEVICE_FROM_THIS (This);
> -  Erase = 0;
> -
> -  VA_START (args, This);
>  
> +  //
> +  // Check input parameters
> +  //
> +  VA_START (Args, This);
>    do {
> -    StartingLba = VA_ARG (args, EFI_LBA);
> +    StartingLba = VA_ARG (Args, EFI_LBA);
>      if (StartingLba == EFI_LBA_LIST_TERMINATOR) {
>        break;
>      }
> +    NumOfLba = VA_ARG (Args, UINTN);
>  
> -    NumOfLba = VA_ARG (args, UINT32);

This bug seems mildly concerning. I guess real users are not going to
specify an lba count > 4G. Still I think you should break this fix
out, and perhaps notify other package owners:

$ git grep -e NumOfLba --and -e VA_ARG --and -e UINT32
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c:    NumOfLba = VA_ARG (Args, UINT32);
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c:    NumOfLba = VA_ARG (Args, UINT32);
DuetPkg/FvbRuntimeService/FWBlockService.c:    NumOfLba = VA_ARG (args, UINT32);
DuetPkg/FvbRuntimeService/FWBlockService.c:    NumOfLba = VA_ARG (args, UINT32);
EmulatorPkg/FvbServicesRuntimeDxe/FWBlockService.c:    NumOfLba = VA_ARG (args, UINT32);
EmulatorPkg/FvbServicesRuntimeDxe/FWBlockService.c:    NumOfLba = VA_ARG (args, UINT32);
Nt32Pkg/FvbServicesRuntimeDxe/FWBlockService.c:    NumOfLba = VA_ARG (args, UINT32);
Nt32Pkg/FvbServicesRuntimeDxe/FWBlockService.c:    NumOfLba = VA_ARG (args, UINT32);
OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c:    NumOfLba = VA_ARG (args, UINT32);
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c:    NumOfLba = VA_ARG (args, UINT32);
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c:    NumOfLba = VA_ARG (args, UINT32);
QuarkPlatformPkg/Platform/SpiFvbServices/FwBlockService.c:    NumOfLba = VA_ARG (args, UINT32);
QuarkPlatformPkg/Platform/SpiFvbServices/FwBlockService.c:    NumOfLba = VA_ARG (args, UINT32);
Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbService.c:    NumOfLba = VA_ARG (args, UINT32);
Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbService.c:    NumOfLba = VA_ARG (args, UINT32);
Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbSmmDxe.c:    NumOfLba = VA_ARG (Marker, UINT32);
Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbSmmDxe.c:    NumOfLba = VA_ARG (Marker, UINT32);

-Jordan

> -
> -    //
> -    // Check input parameters
> -    //
> -    if ((NumOfLba == 0) || (StartingLba > 1) || ((StartingLba + NumOfLba) > 2)) {
> -      VA_END (args);
> +    if (StartingLba > EMU_FVB_NUM_TOTAL_BLOCKS ||
> +        NumOfLba > EMU_FVB_NUM_TOTAL_BLOCKS - StartingLba) {
> +      VA_END (Args);
>        return EFI_INVALID_PARAMETER;
>      }
> -
> -    if (StartingLba == 0) {
> -      Erase = (UINT8) (Erase | BIT0);
> -    }
> -    if ((StartingLba + NumOfLba) == 2) {
> -      Erase = (UINT8) (Erase | BIT1);
> -    }
> -
>    } while (1);
> +  VA_END (Args);
>  
> -  VA_END (args);
> -
> -  ErasePtr = (UINT8*) FvbDevice->BufferPtr;
> -  EraseSize = 0;
> +  //
> +  // Erase blocks
> +  //
> +  VA_START (Args, This);
> +  do {
> +    StartingLba = VA_ARG (Args, EFI_LBA);
> +    if (StartingLba == EFI_LBA_LIST_TERMINATOR) {
> +      break;
> +    }
> +    NumOfLba = VA_ARG (Args, UINTN);
>  
> -  if ((Erase & BIT0) != 0) {
> -    EraseSize = EraseSize + FvbDevice->BlockSize;
> -  } else {
> -    ErasePtr = (VOID*) ((UINT8*)ErasePtr + FvbDevice->BlockSize);
> -  }
> +    ErasePtr = FvbDevice->BufferPtr;
> +    ErasePtr += (UINTN)StartingLba * FvbDevice->BlockSize;
> +    EraseSize = NumOfLba * FvbDevice->BlockSize;
>  
> -  if ((Erase & BIT1) != 0) {
> -    EraseSize = EraseSize + FvbDevice->BlockSize;
> -  }
> +    SetMem (ErasePtr, EraseSize, ERASED_UINT8);
> +  } while (1);
> +  VA_END (Args);
>  
> -  if (EraseSize != 0) {
> -    SetMem (
> -      (VOID*) ErasePtr,
> -      EraseSize,
> -      ERASED_UINT8
> -      );
> -    VA_START (args, This);
> -    PlatformFvbBlocksErased (This, args);
> -    VA_END (args);
> -  }
> +  //
> +  // Call platform hook
> +  //
> +  VA_START (Args, This);
> +  PlatformFvbBlocksErased (This, Args);
> +  VA_END (Args);
>  
>    return EFI_SUCCESS;
>  }
> @@ -458,31 +448,30 @@ FvbProtocolWrite (
>    IN        UINT8                               *Buffer
>    )
>  {
> -
>    EFI_FW_VOL_BLOCK_DEVICE *FvbDevice;
>    UINT8                   *FvbDataPtr;
> +  EFI_STATUS              Status;
>  
>    FvbDevice = FVB_DEVICE_FROM_THIS (This);
>  
> -  if ((Lba > 1) || (Offset > FvbDevice->BlockSize)) {
> +  if (Lba >= EMU_FVB_NUM_TOTAL_BLOCKS ||
> +      Offset > FvbDevice->BlockSize) {
>      return EFI_INVALID_PARAMETER;
>    }
>  
> -  if ((Offset + *NumBytes) > FvbDevice->BlockSize) {
> +  Status = EFI_SUCCESS;
> +  if (*NumBytes > FvbDevice->BlockSize - Offset) {
>      *NumBytes = FvbDevice->BlockSize - Offset;
> +    Status = EFI_BAD_BUFFER_SIZE;
>    }
>  
> -  FvbDataPtr =
> -    (UINT8*) FvbDevice->BufferPtr +
> -    MultU64x32 (Lba, (UINT32) FvbDevice->BlockSize) +
> -    Offset;
> +  FvbDataPtr = FvbDevice->BufferPtr;
> +  FvbDataPtr += (UINTN)Lba * FvbDevice->BlockSize;
> +  FvbDataPtr += Offset;
>  
> -  if (*NumBytes > 0) {
> -    CopyMem (FvbDataPtr, Buffer, *NumBytes);
> -    PlatformFvbDataWritten (This, Lba, Offset, *NumBytes, Buffer);
> -  }
> -
> -  return EFI_SUCCESS;
> +  CopyMem (FvbDataPtr, Buffer, *NumBytes);
> +  PlatformFvbDataWritten (This, Lba, Offset, *NumBytes, Buffer);
> +  return Status;
>  }
>  
>  
> @@ -545,28 +534,28 @@ FvbProtocolRead (
>  {
>    EFI_FW_VOL_BLOCK_DEVICE *FvbDevice;
>    UINT8                   *FvbDataPtr;
> +  EFI_STATUS              Status;
>  
>    FvbDevice = FVB_DEVICE_FROM_THIS (This);
>  
> -  if ((Lba > 1) || (Offset > FvbDevice->BlockSize)) {
> +  if (Lba >= EMU_FVB_NUM_TOTAL_BLOCKS ||
> +      Offset > FvbDevice->BlockSize) {
>      return EFI_INVALID_PARAMETER;
>    }
>  
> -  if ((Offset + *NumBytes) > FvbDevice->BlockSize) {
> +  Status = EFI_SUCCESS;
> +  if (*NumBytes > FvbDevice->BlockSize - Offset) {
>      *NumBytes = FvbDevice->BlockSize - Offset;
> +    Status = EFI_BAD_BUFFER_SIZE;
>    }
>  
> -  FvbDataPtr =
> -    (UINT8*) FvbDevice->BufferPtr +
> -    MultU64x32 (Lba, (UINT32) FvbDevice->BlockSize) +
> -    Offset;
> +  FvbDataPtr = FvbDevice->BufferPtr;
> +  FvbDataPtr += (UINTN)Lba * FvbDevice->BlockSize;
> +  FvbDataPtr += Offset;
>  
> -  if (*NumBytes > 0) {
> -    CopyMem (Buffer, FvbDataPtr, *NumBytes);
> -    PlatformFvbDataRead (This, Lba, Offset, *NumBytes, Buffer);
> -  }
> -
> -  return EFI_SUCCESS;
> +  CopyMem (Buffer, FvbDataPtr, *NumBytes);
> +  PlatformFvbDataRead (This, Lba, Offset, *NumBytes, Buffer);
> +  return Status;
>  }
>  
>  
> @@ -663,7 +652,7 @@ InitializeFvAndVariableStoreHeaders (
>        // EFI_FV_BLOCK_MAP_ENTRY    BlockMap[1];
>        {
>          {
> -          2, // UINT32 NumBlocks;
> +          EMU_FVB_NUM_TOTAL_BLOCKS, // UINT32 NumBlocks;
>            EMU_FVB_BLOCK_SIZE  // UINT32 Length;
>          }
>        }
> @@ -745,7 +734,7 @@ FvbInitialize (
>         (PcdGet32 (PcdVariableStoreSize) +
>          PcdGet32 (PcdFlashNvStorageFtwWorkingSize)
>         ) >
> -       EMU_FVB_BLOCK_SIZE
> +       EMU_FVB_NUM_SPARE_BLOCKS * EMU_FVB_BLOCK_SIZE
>       ) {
>      DEBUG ((EFI_D_ERROR, "EMU Variable invalid PCD sizes\n"));
>      return EFI_INVALID_PARAMETER;
> @@ -779,10 +768,7 @@ FvbInitialize (
>        Initialize = FALSE;
>      }
>    } else {
> -    Ptr = AllocateAlignedRuntimePages (
> -            EFI_SIZE_TO_PAGES (EMU_FVB_SIZE),
> -            SIZE_64KB
> -            );
> +    Ptr = AllocateRuntimePages (EFI_SIZE_TO_PAGES (EMU_FVB_SIZE));
>    }
>  
>    mEmuVarsFvb.BufferPtr = Ptr;
> @@ -808,7 +794,8 @@ FvbInitialize (
>    //
>    // Initialize the Fault Tolerant Write spare block
>    //
> -  SubPtr = (VOID*) ((UINT8*) Ptr + EMU_FVB_BLOCK_SIZE);
> +  SubPtr = (VOID*) ((UINT8*) Ptr +
> +                    EMU_FVB_NUM_SPARE_BLOCKS * EMU_FVB_BLOCK_SIZE);
>    PcdStatus = PcdSet32S (PcdFlashNvStorageFtwSpareBase,
>                  (UINT32)(UINTN) SubPtr);
>    ASSERT_RETURN_ERROR (PcdStatus);
> -- 
> 2.9.3
> 
>