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>
---
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 2f89632e5d75..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, UINT32);
-
- //
- // 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 Sat, May 06, 2017 at 09:30:20PM +0200, Laszlo Ersek wrote:
> 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>
> ---
> 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 2f89632e5d75..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, UINT32);
> -
> - //
> - // 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
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 2017-05-06 12:30:20, Laszlo Ersek wrote:
> 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>
> ---
> 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 2f89632e5d75..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, UINT32);
This bug seems mildly concerning. I guess real users are not going to
specify an lba count > 4G. Still I think you should break this fix
out, and perhaps notify other package owners:
$ git grep -e NumOfLba --and -e VA_ARG --and -e UINT32
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c: NumOfLba = VA_ARG (Args, UINT32);
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c: NumOfLba = VA_ARG (Args, UINT32);
DuetPkg/FvbRuntimeService/FWBlockService.c: NumOfLba = VA_ARG (args, UINT32);
DuetPkg/FvbRuntimeService/FWBlockService.c: NumOfLba = VA_ARG (args, UINT32);
EmulatorPkg/FvbServicesRuntimeDxe/FWBlockService.c: NumOfLba = VA_ARG (args, UINT32);
EmulatorPkg/FvbServicesRuntimeDxe/FWBlockService.c: NumOfLba = VA_ARG (args, UINT32);
Nt32Pkg/FvbServicesRuntimeDxe/FWBlockService.c: NumOfLba = VA_ARG (args, UINT32);
Nt32Pkg/FvbServicesRuntimeDxe/FWBlockService.c: NumOfLba = VA_ARG (args, UINT32);
OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c: NumOfLba = VA_ARG (args, UINT32);
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c: NumOfLba = VA_ARG (args, UINT32);
OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c: NumOfLba = VA_ARG (args, UINT32);
QuarkPlatformPkg/Platform/SpiFvbServices/FwBlockService.c: NumOfLba = VA_ARG (args, UINT32);
QuarkPlatformPkg/Platform/SpiFvbServices/FwBlockService.c: NumOfLba = VA_ARG (args, UINT32);
Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbService.c: NumOfLba = VA_ARG (args, UINT32);
Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbService.c: NumOfLba = VA_ARG (args, UINT32);
Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbSmmDxe.c: NumOfLba = VA_ARG (Marker, UINT32);
Vlv2TbltDevicePkg/FvbRuntimeDxe/FvbSmmDxe.c: NumOfLba = VA_ARG (Marker, UINT32);
-Jordan
> -
> - //
> - // 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
© 2016 - 2024 Red Hat, Inc.