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 <[email protected]>
>
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
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
