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.jus...@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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel