EmuVariableFvbRuntimeDxe currently produces a Firmware Volume Block
protocol that is based on a block map of two blocks, each block having
PcdFlashNvStorageFtwSpareSize for size.

(The total size is 2 * PcdFlashNvStorageFtwSpareSize.)

FaultTolerantWriteDxe in turn expects the block size to be a power of two.

In the 4MB build of OVMF, PcdFlashNvStorageFtwSpareSize is 264KB, which is
not a power of two. In order to equip EmuVariableFvbRuntimeDxe for this
build, shrink the block size to 4KB (EFI_PAGE_SIZE), and grow the block
count from 2 to EFI_SIZE_TO_PAGES(2 * PcdFlashNvStorageFtwSpareSize). The
total size remains

  2 * PcdFlashNvStorageFtwSpareSize
  --------------------------------- * EFI_PAGE_SIZE
            EFI_PAGE_SIZE

Right now EmuVariableFvbRuntimeDxe open-codes the block count of 2 in
various limit checks, so introduce a few new macros:
- EMU_FVB_NUM_TOTAL_BLOCKS, for the LHS of the above product,
- EMU_FVB_NUM_SPARE_BLOCKS for the half of that.

Also rework the FVB protocol members to support an arbitrary count of
blocks.

Keep the invariant intact that the first half of the firmware volume hosts
the variable store and the FTW working block, and that the second half
maps the FTW spare area.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=527
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Regression-tested-by: Gary Lin <glin@suse.com>
---

Notes:
    v2:
    - rebase to separated-out patch "OvmfPkg/EmuVariableFvbRuntimeDxe:
      correct NumOfLba vararg type in EraseBlocks()"; end result is
      identical to v1 [Jordan]
    - add Gary's R-t-b

 OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h |  10 +-
 OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c | 147 +++++++++-----------
 2 files changed, 75 insertions(+), 82 deletions(-)

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))
diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
index 30f69b999ab0..11c8b1b75cb8 100644
--- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
+++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
@@ -74,8 +74,8 @@ EFI_FW_VOL_BLOCK_DEVICE mEmuVarsFvb = {
     }
   },
   NULL, // BufferPtr
-  FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize), // BlockSize
-  2 * FixedPcdGet32 (PcdFlashNvStorageFtwSpareSize), // Size
+  EMU_FVB_BLOCK_SIZE, // BlockSize
+  EMU_FVB_SIZE, // Size
   {     // FwVolBlockInstance
     FvbProtocolGetAttributes,
     FvbProtocolSetAttributes,
@@ -185,14 +185,14 @@ FvbProtocolGetBlockSize (
 {
   EFI_FW_VOL_BLOCK_DEVICE *FvbDevice;
 
-  if (Lba > 1) {
+  if (Lba >= EMU_FVB_NUM_TOTAL_BLOCKS) {
     return EFI_INVALID_PARAMETER;
   }
 
   FvbDevice = FVB_DEVICE_FROM_THIS (This);
 
   *BlockSize = FvbDevice->BlockSize;
-  *NumberOfBlocks = (UINTN) (2 - (UINTN) Lba);
+  *NumberOfBlocks = (UINTN)(EMU_FVB_NUM_TOTAL_BLOCKS - Lba);
 
   return EFI_SUCCESS;
 }
@@ -322,68 +322,58 @@ FvbProtocolEraseBlocks (
   )
 {
   EFI_FW_VOL_BLOCK_DEVICE *FvbDevice;
-  VA_LIST                 args;
+  VA_LIST                 Args;
   EFI_LBA                 StartingLba;
   UINTN                   NumOfLba;
-  UINT8                   Erase;
-  VOID                    *ErasePtr;
+  UINT8                   *ErasePtr;
   UINTN                   EraseSize;
 
   FvbDevice = FVB_DEVICE_FROM_THIS (This);
-  Erase = 0;
-
-  VA_START (args, This);
 
+  //
+  // Check input parameters
+  //
+  VA_START (Args, This);
   do {
-    StartingLba = VA_ARG (args, EFI_LBA);
+    StartingLba = VA_ARG (Args, EFI_LBA);
     if (StartingLba == EFI_LBA_LIST_TERMINATOR) {
       break;
     }
+    NumOfLba = VA_ARG (Args, UINTN);
 
-    NumOfLba = VA_ARG (args, UINTN);
-
-    //
-    // Check input parameters
-    //
-    if ((NumOfLba == 0) || (StartingLba > 1) || ((StartingLba + NumOfLba) > 2)) {
-      VA_END (args);
+    if (StartingLba > EMU_FVB_NUM_TOTAL_BLOCKS ||
+        NumOfLba > EMU_FVB_NUM_TOTAL_BLOCKS - StartingLba) {
+      VA_END (Args);
       return EFI_INVALID_PARAMETER;
     }
-
-    if (StartingLba == 0) {
-      Erase = (UINT8) (Erase | BIT0);
-    }
-    if ((StartingLba + NumOfLba) == 2) {
-      Erase = (UINT8) (Erase | BIT1);
-    }
-
   } while (1);
+  VA_END (Args);
 
-  VA_END (args);
-
-  ErasePtr = (UINT8*) FvbDevice->BufferPtr;
-  EraseSize = 0;
+  //
+  // Erase blocks
+  //
+  VA_START (Args, This);
+  do {
+    StartingLba = VA_ARG (Args, EFI_LBA);
+    if (StartingLba == EFI_LBA_LIST_TERMINATOR) {
+      break;
+    }
+    NumOfLba = VA_ARG (Args, UINTN);
 
-  if ((Erase & BIT0) != 0) {
-    EraseSize = EraseSize + FvbDevice->BlockSize;
-  } else {
-    ErasePtr = (VOID*) ((UINT8*)ErasePtr + FvbDevice->BlockSize);
-  }
+    ErasePtr = FvbDevice->BufferPtr;
+    ErasePtr += (UINTN)StartingLba * FvbDevice->BlockSize;
+    EraseSize = NumOfLba * FvbDevice->BlockSize;
 
-  if ((Erase & BIT1) != 0) {
-    EraseSize = EraseSize + FvbDevice->BlockSize;
-  }
+    SetMem (ErasePtr, EraseSize, ERASED_UINT8);
+  } while (1);
+  VA_END (Args);
 
-  if (EraseSize != 0) {
-    SetMem (
-      (VOID*) ErasePtr,
-      EraseSize,
-      ERASED_UINT8
-      );
-    VA_START (args, This);
-    PlatformFvbBlocksErased (This, args);
-    VA_END (args);
-  }
+  //
+  // Call platform hook
+  //
+  VA_START (Args, This);
+  PlatformFvbBlocksErased (This, Args);
+  VA_END (Args);
 
   return EFI_SUCCESS;
 }
@@ -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;
   }
 
-  FvbDataPtr =
-    (UINT8*) FvbDevice->BufferPtr +
-    MultU64x32 (Lba, (UINT32) FvbDevice->BlockSize) +
-    Offset;
+  FvbDataPtr = FvbDevice->BufferPtr;
+  FvbDataPtr += (UINTN)Lba * FvbDevice->BlockSize;
+  FvbDataPtr += Offset;
 
-  if (*NumBytes > 0) {
-    CopyMem (FvbDataPtr, Buffer, *NumBytes);
-    PlatformFvbDataWritten (This, Lba, Offset, *NumBytes, Buffer);
-  }
-
-  return EFI_SUCCESS;
+  CopyMem (FvbDataPtr, Buffer, *NumBytes);
+  PlatformFvbDataWritten (This, Lba, Offset, *NumBytes, Buffer);
+  return Status;
 }
 
 
@@ -545,28 +534,28 @@ FvbProtocolRead (
 {
   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;
   }
 
-  FvbDataPtr =
-    (UINT8*) FvbDevice->BufferPtr +
-    MultU64x32 (Lba, (UINT32) FvbDevice->BlockSize) +
-    Offset;
+  FvbDataPtr = FvbDevice->BufferPtr;
+  FvbDataPtr += (UINTN)Lba * FvbDevice->BlockSize;
+  FvbDataPtr += Offset;
 
-  if (*NumBytes > 0) {
-    CopyMem (Buffer, FvbDataPtr, *NumBytes);
-    PlatformFvbDataRead (This, Lba, Offset, *NumBytes, Buffer);
-  }
-
-  return EFI_SUCCESS;
+  CopyMem (Buffer, FvbDataPtr, *NumBytes);
+  PlatformFvbDataRead (This, Lba, Offset, *NumBytes, Buffer);
+  return Status;
 }
 
 
@@ -663,7 +652,7 @@ InitializeFvAndVariableStoreHeaders (
       // EFI_FV_BLOCK_MAP_ENTRY    BlockMap[1];
       {
         {
-          2, // UINT32 NumBlocks;
+          EMU_FVB_NUM_TOTAL_BLOCKS, // UINT32 NumBlocks;
           EMU_FVB_BLOCK_SIZE  // UINT32 Length;
         }
       }
@@ -745,7 +734,7 @@ FvbInitialize (
        (PcdGet32 (PcdVariableStoreSize) +
         PcdGet32 (PcdFlashNvStorageFtwWorkingSize)
        ) >
-       EMU_FVB_BLOCK_SIZE
+       EMU_FVB_NUM_SPARE_BLOCKS * EMU_FVB_BLOCK_SIZE
      ) {
     DEBUG ((EFI_D_ERROR, "EMU Variable invalid PCD sizes\n"));
     return EFI_INVALID_PARAMETER;
@@ -779,10 +768,7 @@ FvbInitialize (
       Initialize = FALSE;
     }
   } else {
-    Ptr = AllocateAlignedRuntimePages (
-            EFI_SIZE_TO_PAGES (EMU_FVB_SIZE),
-            SIZE_64KB
-            );
+    Ptr = AllocateRuntimePages (EFI_SIZE_TO_PAGES (EMU_FVB_SIZE));
   }
 
   mEmuVarsFvb.BufferPtr = Ptr;
@@ -808,7 +794,8 @@ FvbInitialize (
   //
   // Initialize the Fault Tolerant Write spare block
   //
-  SubPtr = (VOID*) ((UINT8*) Ptr + EMU_FVB_BLOCK_SIZE);
+  SubPtr = (VOID*) ((UINT8*) Ptr +
+                    EMU_FVB_NUM_SPARE_BLOCKS * EMU_FVB_BLOCK_SIZE);
   PcdStatus = PcdSet32S (PcdFlashNvStorageFtwSpareBase,
                 (UINT32)(UINTN) SubPtr);
   ASSERT_RETURN_ERROR (PcdStatus);
-- 
2.9.3


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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

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

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

On 05/18/17 22:56, Jordan Justen wrote:
> 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?

Assuming you mean those as shorthands for the FixedPcdGet32() macro
invocations, they wouldn't (fully); FTW_WRITE_QUEUE_SIZE would remain
overlong even after such a replacement.

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

I don't feel strongly about this layout, so if (when) you have an
incremental patch, I'll be glad to review it. What I do feel strongly
about :) is not wanting to retest the -bios scenarios, which is sort of
required once these macros are touched. (The ASSERT() below is a lot
easier / quicker to test.) Due to the testing impact, I prefer to keep
the current layout.

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

Thanks, I'll squash it then.

Cheers,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 05/18/17 23:09, Laszlo Ersek wrote:
> On 05/18/17 22:56, Jordan Justen wrote:
>> 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?
> 
> Assuming you mean those as shorthands for the FixedPcdGet32() macro
> invocations, they wouldn't (fully); FTW_WRITE_QUEUE_SIZE would remain
> overlong even after such a replacement.
> 
>>
>> If you feel strongly about this current format, then keep it, as I
>> don't feel too strongly about it.
> 
> I don't feel strongly about this layout, so if (when) you have an
> incremental patch, I'll be glad to review it. What I do feel strongly
> about :) is not wanting to retest the -bios scenarios, which is sort of
> required once these macros are touched. (The ASSERT() below is a lot
> easier / quicker to test.) Due to the testing impact, I prefer to keep
> the current layout.
> 
>>
>>>>
>>>> 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.
> 
> Thanks, I'll squash it then.

Commit 7e8329267ecb.
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel