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

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

On 2017-05-18 08:14:33, Laszlo Ersek wrote:
> 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))

In the cases where we don't exceed 80 columns, I don't see the excess
newlines as helping here, style-wise.

Could you add to the entry-point an assert:

  ASSERT(FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize) %
         EMU_FVB_BLOCK_SIZE == 0);

We should tweak VERIFY_SIZE_OF to make a STATIC_ASSERT macro, because
I guess this check should be possible at compile time.

> @@ -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;

Stealth bug fix? :)

With the understanding that we're holding off on the final patch for
now to coordinate with Xen:

Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>