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

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

On 2017-05-18 12:40:30, Laszlo Ersek wrote:
> 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.

I don't think you would make a block of function calls all multiline
if one call required it. I see your point and I agree that aligning
things can be nice if it works out. It seems like it doesn't in this
case.

Could FTW_SPARE_SIZE and FTW_WORKING_SIZE macros help?

If you feel strongly about this current format, then keep it, as I
don't feel too strongly about it.

> > 
> > Could you add to the entry-point an assert:
> > 
> >   ASSERT(FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize) %
> >          EMU_FVB_BLOCK_SIZE == 0);
> 
> Should I squash that into this patch?

Yeah. No need for resend.

-Jordan