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

[Laszlo Ersek <lersek@redhat.com>]

On 05/18/17 20:49, Jordan Justen wrote:
> 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.

My first preference would have been

#define SHORT_MACRO_NAME           replacement text 1
#define ANNOYINGLY_LONG_MACRO_NAME replacement text 2

That is, to keep both the macro names and the replacement texts aligned.
However, that way I wouldn't fit into 80 chars on some lines, and then
breaking only *some* macro definitions to multiple lines looked
horrible. Which is why I opted for the current layout: it is uniform,
and it does preserve the alignment for both macro names and replacement
texts separately.

> 
> Could you add to the entry-point an assert:
> 
>   ASSERT(FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize) %
>          EMU_FVB_BLOCK_SIZE == 0);

Should I squash that into this patch?

> 
> 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? :)

Sure :) This patch is more or less a rewrite of the FVB member functions.

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

I feel inclined to commit the first four patches now -- with the OvmfPkg
patches from the prerequisite series -- and pick up patch #5 only later,
when Gary reports the Xen hvmloader fix complete. (I noted this on patch
#5.) Are you OK with that, provided that I add / squash the ASSERT()?

Thanks!
Laszlo