* [PATCH v2 1/5] OvmfPkg/EmuVariableFvbRuntimeDxe: strip trailing whitespace
2017-05-18 15:14 [PATCH v2 0/5] OvmfPkg: complete the 4MB flash image support ("-bios" / emulated variables) Laszlo Ersek
@ 2017-05-18 15:14 ` Laszlo Ersek
2017-05-18 22:12 ` Laszlo Ersek
2017-05-18 15:14 ` [PATCH v2 2/5] OvmfPkg/EmuVariableFvbRuntimeDxe: change block size to 4KB Laszlo Ersek
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2017-05-18 15:14 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
Cc: Jordan Justen <jordan.l.justen@intel.com>
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:
- no changes
- add Gary's R-t-b
OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h | 16 +++----
OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c | 44 ++++++++++----------
2 files changed, 30 insertions(+), 30 deletions(-)
diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h
index f34fad2cdd48..4247d21d72f8 100644
--- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h
+++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h
@@ -1,18 +1,18 @@
/*++
Copyright (c) 2006, Intel Corporation. All rights reserved.<BR>
-This program and the accompanying materials
-are licensed and made available under the terms and conditions of the BSD License
-which accompanies this distribution. The full text of the license may be found at
-http://opensource.org/licenses/bsd-license.php
-
-THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
-WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+This program and the accompanying materials
+are licensed and made available under the terms and conditions of the BSD License
+which accompanies this distribution. The full text of the license may be found at
+http://opensource.org/licenses/bsd-license.php
+
+THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
Module Name:
FwBlockService.h
-
+
Abstract:
Firmware volume block driver for Intel Firmware Hub (FWH) device
diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
index 9f9babc9cc33..30f69b999ab0 100644
--- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
+++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
@@ -120,14 +120,14 @@ FvbVirtualAddressChangeEvent (
only for memory-mapped firmware volumes.
@param This Indicates the EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL instance.
-
+
@param Address Pointer to a caller-allocated
EFI_PHYSICAL_ADDRESS that, on successful
return from GetPhysicalAddress(), contains the
base address of the firmware volume.
-
+
@retval EFI_SUCCESS The firmware volume base address is returned.
-
+
@retval EFI_NOT_SUPPORTED The firmware volume is not memory mapped.
**/
@@ -168,9 +168,9 @@ FvbProtocolGetPhysicalAddress (
blocks in this range have a size of
BlockSize.
-
+
@retval EFI_SUCCESS The firmware volume base address is returned.
-
+
@retval EFI_INVALID_PARAMETER The requested LBA is out of range.
**/
@@ -246,7 +246,7 @@ FvbProtocolGetAttributes (
settings of the firmware volume. Type
EFI_FVB_ATTRIBUTES_2 is defined in
EFI_FIRMWARE_VOLUME_HEADER.
-
+
@retval EFI_SUCCESS The firmware volume attributes were returned.
@retval EFI_INVALID_PARAMETER The attributes requested are in
@@ -302,7 +302,7 @@ FvbProtocolSetAttributes (
@retval EFI_SUCCESS The erase request was successfully
completed.
-
+
@retval EFI_ACCESS_DENIED The firmware volume is in the
WriteDisabled state.
@retval EFI_DEVICE_ERROR The block device is not functioning
@@ -311,7 +311,7 @@ FvbProtocolSetAttributes (
partially erased.
@retval EFI_INVALID_PARAMETER One or more of the LBAs listed
in the variable argument list do
- not exist in the firmware volume.
+ not exist in the firmware volume.
**/
EFI_STATUS
@@ -420,29 +420,29 @@ FvbProtocolEraseBlocks (
returns.
@param This Indicates the EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL instance.
-
+
@param Lba The starting logical block index to write to.
-
+
@param Offset Offset into the block at which to begin writing.
-
+
@param NumBytes Pointer to a UINTN. At entry, *NumBytes
contains the total size of the buffer. At
exit, *NumBytes contains the total number of
bytes actually written.
-
+
@param Buffer Pointer to a caller-allocated buffer that
contains the source for the write.
-
+
@retval EFI_SUCCESS The firmware volume was written successfully.
-
+
@retval EFI_BAD_BUFFER_SIZE The write was attempted across an
LBA boundary. On output, NumBytes
contains the total number of bytes
actually written.
-
+
@retval EFI_ACCESS_DENIED The firmware volume is in the
WriteDisabled state.
-
+
@retval EFI_DEVICE_ERROR The block device is malfunctioning
and could not be written.
@@ -503,7 +503,7 @@ FvbProtocolWrite (
aware that a read may be partially completed.
@param This Indicates the EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL instance.
-
+
@param Lba The starting logical block index
from which to read.
@@ -519,15 +519,15 @@ FvbProtocolWrite (
@retval EFI_SUCCESS The firmware volume was read successfully
and contents are in Buffer.
-
+
@retval EFI_BAD_BUFFER_SIZE Read attempted across an LBA
boundary. On output, NumBytes
contains the total number of bytes
returned in Buffer.
-
+
@retval EFI_ACCESS_DENIED The firmware volume is in the
ReadDisabled state.
-
+
@retval EFI_DEVICE_ERROR The block device is not
functioning correctly and could
not be read.
@@ -715,9 +715,9 @@ InitializeFvAndVariableStoreHeaders (
/**
Main entry point.
- @param[in] ImageHandle The firmware allocated handle for the EFI image.
+ @param[in] ImageHandle The firmware allocated handle for the EFI image.
@param[in] SystemTable A pointer to the EFI System Table.
-
+
@retval EFI_SUCCESS Successfully initialized.
**/
--
2.9.3
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 1/5] OvmfPkg/EmuVariableFvbRuntimeDxe: strip trailing whitespace
2017-05-18 15:14 ` [PATCH v2 1/5] OvmfPkg/EmuVariableFvbRuntimeDxe: strip trailing whitespace Laszlo Ersek
@ 2017-05-18 22:12 ` Laszlo Ersek
0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2017-05-18 22:12 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
On 05/18/17 17:14, Laszlo Ersek wrote:
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> 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:
> - no changes
> - add Gary's R-t-b
>
> OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h | 16 +++----
> OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c | 44 ++++++++++----------
> 2 files changed, 30 insertions(+), 30 deletions(-)
>
> diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h
> index f34fad2cdd48..4247d21d72f8 100644
> --- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h
> +++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h
> @@ -1,18 +1,18 @@
> /*++
>
> Copyright (c) 2006, Intel Corporation. All rights reserved.<BR>
> -This program and the accompanying materials
> -are licensed and made available under the terms and conditions of the BSD License
> -which accompanies this distribution. The full text of the license may be found at
> -http://opensource.org/licenses/bsd-license.php
> -
> -THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> -WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +This program and the accompanying materials
> +are licensed and made available under the terms and conditions of the BSD License
> +which accompanies this distribution. The full text of the license may be found at
> +http://opensource.org/licenses/bsd-license.php
> +
> +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>
> Module Name:
>
> FwBlockService.h
> -
> +
> Abstract:
>
> Firmware volume block driver for Intel Firmware Hub (FWH) device
> diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
> index 9f9babc9cc33..30f69b999ab0 100644
> --- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
> +++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
> @@ -120,14 +120,14 @@ FvbVirtualAddressChangeEvent (
> only for memory-mapped firmware volumes.
>
> @param This Indicates the EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL instance.
> -
> +
> @param Address Pointer to a caller-allocated
> EFI_PHYSICAL_ADDRESS that, on successful
> return from GetPhysicalAddress(), contains the
> base address of the firmware volume.
> -
> +
> @retval EFI_SUCCESS The firmware volume base address is returned.
> -
> +
> @retval EFI_NOT_SUPPORTED The firmware volume is not memory mapped.
>
> **/
> @@ -168,9 +168,9 @@ FvbProtocolGetPhysicalAddress (
> blocks in this range have a size of
> BlockSize.
>
> -
> +
> @retval EFI_SUCCESS The firmware volume base address is returned.
> -
> +
> @retval EFI_INVALID_PARAMETER The requested LBA is out of range.
>
> **/
> @@ -246,7 +246,7 @@ FvbProtocolGetAttributes (
> settings of the firmware volume. Type
> EFI_FVB_ATTRIBUTES_2 is defined in
> EFI_FIRMWARE_VOLUME_HEADER.
> -
> +
> @retval EFI_SUCCESS The firmware volume attributes were returned.
>
> @retval EFI_INVALID_PARAMETER The attributes requested are in
> @@ -302,7 +302,7 @@ FvbProtocolSetAttributes (
>
> @retval EFI_SUCCESS The erase request was successfully
> completed.
> -
> +
> @retval EFI_ACCESS_DENIED The firmware volume is in the
> WriteDisabled state.
> @retval EFI_DEVICE_ERROR The block device is not functioning
> @@ -311,7 +311,7 @@ FvbProtocolSetAttributes (
> partially erased.
> @retval EFI_INVALID_PARAMETER One or more of the LBAs listed
> in the variable argument list do
> - not exist in the firmware volume.
> + not exist in the firmware volume.
>
> **/
> EFI_STATUS
> @@ -420,29 +420,29 @@ FvbProtocolEraseBlocks (
> returns.
>
> @param This Indicates the EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL instance.
> -
> +
> @param Lba The starting logical block index to write to.
> -
> +
> @param Offset Offset into the block at which to begin writing.
> -
> +
> @param NumBytes Pointer to a UINTN. At entry, *NumBytes
> contains the total size of the buffer. At
> exit, *NumBytes contains the total number of
> bytes actually written.
> -
> +
> @param Buffer Pointer to a caller-allocated buffer that
> contains the source for the write.
> -
> +
> @retval EFI_SUCCESS The firmware volume was written successfully.
> -
> +
> @retval EFI_BAD_BUFFER_SIZE The write was attempted across an
> LBA boundary. On output, NumBytes
> contains the total number of bytes
> actually written.
> -
> +
> @retval EFI_ACCESS_DENIED The firmware volume is in the
> WriteDisabled state.
> -
> +
> @retval EFI_DEVICE_ERROR The block device is malfunctioning
> and could not be written.
>
> @@ -503,7 +503,7 @@ FvbProtocolWrite (
> aware that a read may be partially completed.
>
> @param This Indicates the EFI_FIRMWARE_VOLUME_BLOCK2_PROTOCOL instance.
> -
> +
> @param Lba The starting logical block index
> from which to read.
>
> @@ -519,15 +519,15 @@ FvbProtocolWrite (
>
> @retval EFI_SUCCESS The firmware volume was read successfully
> and contents are in Buffer.
> -
> +
> @retval EFI_BAD_BUFFER_SIZE Read attempted across an LBA
> boundary. On output, NumBytes
> contains the total number of bytes
> returned in Buffer.
> -
> +
> @retval EFI_ACCESS_DENIED The firmware volume is in the
> ReadDisabled state.
> -
> +
> @retval EFI_DEVICE_ERROR The block device is not
> functioning correctly and could
> not be read.
> @@ -715,9 +715,9 @@ InitializeFvAndVariableStoreHeaders (
> /**
> Main entry point.
>
> - @param[in] ImageHandle The firmware allocated handle for the EFI image.
> + @param[in] ImageHandle The firmware allocated handle for the EFI image.
> @param[in] SystemTable A pointer to the EFI System Table.
> -
> +
> @retval EFI_SUCCESS Successfully initialized.
>
> **/
>
Commit 89f385ce0ad7.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 2/5] OvmfPkg/EmuVariableFvbRuntimeDxe: change block size to 4KB
2017-05-18 15:14 [PATCH v2 0/5] OvmfPkg: complete the 4MB flash image support ("-bios" / emulated variables) Laszlo Ersek
2017-05-18 15:14 ` [PATCH v2 1/5] OvmfPkg/EmuVariableFvbRuntimeDxe: strip trailing whitespace Laszlo Ersek
@ 2017-05-18 15:14 ` Laszlo Ersek
2017-05-18 18:49 ` Jordan Justen
2017-05-18 15:14 ` [PATCH v2 3/5] OvmfPkg/PlatformPei: align EmuVariableNvStore at any page boundary Laszlo Ersek
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2017-05-18 15:14 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 2/5] OvmfPkg/EmuVariableFvbRuntimeDxe: change block size to 4KB
2017-05-18 15:14 ` [PATCH v2 2/5] OvmfPkg/EmuVariableFvbRuntimeDxe: change block size to 4KB Laszlo Ersek
@ 2017-05-18 18:49 ` Jordan Justen
2017-05-18 19:40 ` Laszlo Ersek
0 siblings, 1 reply; 15+ messages in thread
From: Jordan Justen @ 2017-05-18 18:49 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
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>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 2/5] OvmfPkg/EmuVariableFvbRuntimeDxe: change block size to 4KB
2017-05-18 18:49 ` Jordan Justen
@ 2017-05-18 19:40 ` Laszlo Ersek
2017-05-18 20:56 ` Jordan Justen
0 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2017-05-18 19:40 UTC (permalink / raw)
To: Jordan Justen, edk2-devel-01
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 2/5] OvmfPkg/EmuVariableFvbRuntimeDxe: change block size to 4KB
2017-05-18 19:40 ` Laszlo Ersek
@ 2017-05-18 20:56 ` Jordan Justen
2017-05-18 21:09 ` Laszlo Ersek
0 siblings, 1 reply; 15+ messages in thread
From: Jordan Justen @ 2017-05-18 20:56 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 2/5] OvmfPkg/EmuVariableFvbRuntimeDxe: change block size to 4KB
2017-05-18 20:56 ` Jordan Justen
@ 2017-05-18 21:09 ` Laszlo Ersek
2017-05-18 22:13 ` Laszlo Ersek
0 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2017-05-18 21:09 UTC (permalink / raw)
To: Jordan Justen, edk2-devel-01
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH v2 2/5] OvmfPkg/EmuVariableFvbRuntimeDxe: change block size to 4KB
2017-05-18 21:09 ` Laszlo Ersek
@ 2017-05-18 22:13 ` Laszlo Ersek
0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2017-05-18 22:13 UTC (permalink / raw)
To: Jordan Justen, edk2-devel-01
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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 3/5] OvmfPkg/PlatformPei: align EmuVariableNvStore at any page boundary
2017-05-18 15:14 [PATCH v2 0/5] OvmfPkg: complete the 4MB flash image support ("-bios" / emulated variables) Laszlo Ersek
2017-05-18 15:14 ` [PATCH v2 1/5] OvmfPkg/EmuVariableFvbRuntimeDxe: strip trailing whitespace Laszlo Ersek
2017-05-18 15:14 ` [PATCH v2 2/5] OvmfPkg/EmuVariableFvbRuntimeDxe: change block size to 4KB Laszlo Ersek
@ 2017-05-18 15:14 ` Laszlo Ersek
2017-05-18 22:13 ` Laszlo Ersek
2017-05-18 15:14 ` [PATCH v2 4/5] OvmfPkg/README: document 4MB flash layout Laszlo Ersek
2017-05-18 15:14 ` [PATCH v2 5/5] OvmfPkg: make the 4MB flash size the default (again) Laszlo Ersek
4 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2017-05-18 15:14 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
EmuVariableFvbRuntimeDxe now uses a 4KB (EFI_PAGE_SIZE) block size.
Cc: Jordan Justen <jordan.l.justen@intel.com>
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:
- no changes
- add Gary's R-t-b
OvmfPkg/PlatformPei/Platform.c | 20 ++++----------------
1 file changed, 4 insertions(+), 16 deletions(-)
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 1b4dc00b0180..3e9fda7c7ab0 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -504,7 +504,6 @@ ReserveEmuVariableNvStore (
{
EFI_PHYSICAL_ADDRESS VariableStore;
RETURN_STATUS PcdStatus;
- UINT32 Alignment;
//
// Allocate storage for NV variables early on so it will be
@@ -512,26 +511,15 @@ ReserveEmuVariableNvStore (
// across reboots, this allows the NV variable storage to survive
// a VM reboot.
//
- Alignment = PcdGet32 (PcdFlashNvStorageFtwSpareSize);
- if ((Alignment & (Alignment - 1)) != 0) {
- //
- // Round up Alignment to the next power of two.
- //
- Alignment = GetPowerOfTwo32 (Alignment) << 1;
- }
-
VariableStore =
(EFI_PHYSICAL_ADDRESS)(UINTN)
- AllocateAlignedRuntimePages (
- EFI_SIZE_TO_PAGES (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)),
- Alignment
+ AllocateRuntimePages (
+ EFI_SIZE_TO_PAGES (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize))
);
DEBUG ((EFI_D_INFO,
- "Reserved variable store memory: 0x%lX; size: %dkb, "
- "alignment: 0x%x\n",
+ "Reserved variable store memory: 0x%lX; size: %dkb\n",
VariableStore,
- (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)) / 1024,
- Alignment
+ (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)) / 1024
));
PcdStatus = PcdSet64S (PcdEmuVariableNvStoreReserved, VariableStore);
ASSERT_RETURN_ERROR (PcdStatus);
--
2.9.3
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 3/5] OvmfPkg/PlatformPei: align EmuVariableNvStore at any page boundary
2017-05-18 15:14 ` [PATCH v2 3/5] OvmfPkg/PlatformPei: align EmuVariableNvStore at any page boundary Laszlo Ersek
@ 2017-05-18 22:13 ` Laszlo Ersek
0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2017-05-18 22:13 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
On 05/18/17 17:14, Laszlo Ersek wrote:
> EmuVariableFvbRuntimeDxe now uses a 4KB (EFI_PAGE_SIZE) block size.
>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> 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:
> - no changes
> - add Gary's R-t-b
>
> OvmfPkg/PlatformPei/Platform.c | 20 ++++----------------
> 1 file changed, 4 insertions(+), 16 deletions(-)
>
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 1b4dc00b0180..3e9fda7c7ab0 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -504,7 +504,6 @@ ReserveEmuVariableNvStore (
> {
> EFI_PHYSICAL_ADDRESS VariableStore;
> RETURN_STATUS PcdStatus;
> - UINT32 Alignment;
>
> //
> // Allocate storage for NV variables early on so it will be
> @@ -512,26 +511,15 @@ ReserveEmuVariableNvStore (
> // across reboots, this allows the NV variable storage to survive
> // a VM reboot.
> //
> - Alignment = PcdGet32 (PcdFlashNvStorageFtwSpareSize);
> - if ((Alignment & (Alignment - 1)) != 0) {
> - //
> - // Round up Alignment to the next power of two.
> - //
> - Alignment = GetPowerOfTwo32 (Alignment) << 1;
> - }
> -
> VariableStore =
> (EFI_PHYSICAL_ADDRESS)(UINTN)
> - AllocateAlignedRuntimePages (
> - EFI_SIZE_TO_PAGES (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)),
> - Alignment
> + AllocateRuntimePages (
> + EFI_SIZE_TO_PAGES (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize))
> );
> DEBUG ((EFI_D_INFO,
> - "Reserved variable store memory: 0x%lX; size: %dkb, "
> - "alignment: 0x%x\n",
> + "Reserved variable store memory: 0x%lX; size: %dkb\n",
> VariableStore,
> - (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)) / 1024,
> - Alignment
> + (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)) / 1024
> ));
> PcdStatus = PcdSet64S (PcdEmuVariableNvStoreReserved, VariableStore);
> ASSERT_RETURN_ERROR (PcdStatus);
>
Commit c9e7907d09ea.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 4/5] OvmfPkg/README: document 4MB flash layout
2017-05-18 15:14 [PATCH v2 0/5] OvmfPkg: complete the 4MB flash image support ("-bios" / emulated variables) Laszlo Ersek
` (2 preceding siblings ...)
2017-05-18 15:14 ` [PATCH v2 3/5] OvmfPkg/PlatformPei: align EmuVariableNvStore at any page boundary Laszlo Ersek
@ 2017-05-18 15:14 ` Laszlo Ersek
2017-05-18 22:13 ` Laszlo Ersek
2017-05-18 15:14 ` [PATCH v2 5/5] OvmfPkg: make the 4MB flash size the default (again) Laszlo Ersek
4 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2017-05-18 15:14 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
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>
---
Notes:
v2:
- no changes
OvmfPkg/README | 39 ++++++++++++++++----
1 file changed, 31 insertions(+), 8 deletions(-)
diff --git a/OvmfPkg/README b/OvmfPkg/README
index 304e69fbe545..33ff9432bb3e 100644
--- a/OvmfPkg/README
+++ b/OvmfPkg/README
@@ -245,15 +245,20 @@ longer.)
=== OVMF Flash Layout ===
-Like all current IA32/X64 system designs, OVMF's firmware
-device (rom/flash) appears in QEMU's physical address space
-just below 4GB (0x100000000).
+Like all current IA32/X64 system designs, OVMF's firmware device (rom/flash)
+appears in QEMU's physical address space just below 4GB (0x100000000).
-The layout of the firmware device in memory looks like:
+OVMF supports building a 1MB, 2MB or 4MB flash image (see the DSC files for the
+FD_SIZE_1MB, FD_SIZE_2MB, FD_SIZE_4MB build defines). The base address for the
+1MB image in QEMU physical memory is 0xfff00000. The base address for the 2MB
+image is 0xffe00000. The base address for the 4MB image is 0xffc00000.
+
+Using the 1MB or 2MB image, the layout of the firmware device in memory looks
+like:
+--------------------------------------- 4GB (0x100000000)
| VTF0 (16-bit reset code) and OVMF SEC
-| (SECFV)
+| (SECFV, 208KB/0x34000)
+--------------------------------------- varies based on flash size
|
| Compressed main firmware image
@@ -271,9 +276,27 @@ The layout of the firmware device in memory looks like:
| area (56KB/0xe000)
+--------------------------------------- base address
-OVMF supports building a 1MB or a 2MB flash image. The base address for
-a 1MB image in QEMU physical memory is 0xfff00000. The base address for
-a 2MB image is 0xffe00000.
+Using the 4MB image, the layout of the firmware device in memory looks like:
+
++--------------------------------------- base + 0x400000 (4GB/0x100000000)
+| VTF0 (16-bit reset code) and OVMF SEC
+| (SECFV, 208KB/0x34000)
++--------------------------------------- base + 0x3cc000
+|
+| Compressed main firmware image
+| (FVMAIN_COMPACT, 3360KB/0x348000)
+|
++--------------------------------------- base + 0x84000
+| Fault-tolerant write (FTW)
+| Spare blocks (264KB/0x42000)
++--------------------------------------- base + 0x42000
+| FTW Work block (4KB/0x1000)
++--------------------------------------- base + 0x41000
+| Event log area (4KB/0x1000)
++--------------------------------------- base + 0x40000
+| Non-volatile variable storage
+| area (256KB/0x40000)
++--------------------------------------- base address (0xffc00000)
The code in SECFV locates FVMAIN_COMPACT, and decompresses the
main firmware (MAINFV) into RAM memory at address 0x800000. The
--
2.9.3
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 4/5] OvmfPkg/README: document 4MB flash layout
2017-05-18 15:14 ` [PATCH v2 4/5] OvmfPkg/README: document 4MB flash layout Laszlo Ersek
@ 2017-05-18 22:13 ` Laszlo Ersek
0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2017-05-18 22:13 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
On 05/18/17 17:14, Laszlo Ersek wrote:
> 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>
> ---
>
> Notes:
> v2:
> - no changes
>
> OvmfPkg/README | 39 ++++++++++++++++----
> 1 file changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/OvmfPkg/README b/OvmfPkg/README
> index 304e69fbe545..33ff9432bb3e 100644
> --- a/OvmfPkg/README
> +++ b/OvmfPkg/README
> @@ -245,15 +245,20 @@ longer.)
>
> === OVMF Flash Layout ===
>
> -Like all current IA32/X64 system designs, OVMF's firmware
> -device (rom/flash) appears in QEMU's physical address space
> -just below 4GB (0x100000000).
> +Like all current IA32/X64 system designs, OVMF's firmware device (rom/flash)
> +appears in QEMU's physical address space just below 4GB (0x100000000).
>
> -The layout of the firmware device in memory looks like:
> +OVMF supports building a 1MB, 2MB or 4MB flash image (see the DSC files for the
> +FD_SIZE_1MB, FD_SIZE_2MB, FD_SIZE_4MB build defines). The base address for the
> +1MB image in QEMU physical memory is 0xfff00000. The base address for the 2MB
> +image is 0xffe00000. The base address for the 4MB image is 0xffc00000.
> +
> +Using the 1MB or 2MB image, the layout of the firmware device in memory looks
> +like:
>
> +--------------------------------------- 4GB (0x100000000)
> | VTF0 (16-bit reset code) and OVMF SEC
> -| (SECFV)
> +| (SECFV, 208KB/0x34000)
> +--------------------------------------- varies based on flash size
> |
> | Compressed main firmware image
> @@ -271,9 +276,27 @@ The layout of the firmware device in memory looks like:
> | area (56KB/0xe000)
> +--------------------------------------- base address
>
> -OVMF supports building a 1MB or a 2MB flash image. The base address for
> -a 1MB image in QEMU physical memory is 0xfff00000. The base address for
> -a 2MB image is 0xffe00000.
> +Using the 4MB image, the layout of the firmware device in memory looks like:
> +
> ++--------------------------------------- base + 0x400000 (4GB/0x100000000)
> +| VTF0 (16-bit reset code) and OVMF SEC
> +| (SECFV, 208KB/0x34000)
> ++--------------------------------------- base + 0x3cc000
> +|
> +| Compressed main firmware image
> +| (FVMAIN_COMPACT, 3360KB/0x348000)
> +|
> ++--------------------------------------- base + 0x84000
> +| Fault-tolerant write (FTW)
> +| Spare blocks (264KB/0x42000)
> ++--------------------------------------- base + 0x42000
> +| FTW Work block (4KB/0x1000)
> ++--------------------------------------- base + 0x41000
> +| Event log area (4KB/0x1000)
> ++--------------------------------------- base + 0x40000
> +| Non-volatile variable storage
> +| area (256KB/0x40000)
> ++--------------------------------------- base address (0xffc00000)
>
> The code in SECFV locates FVMAIN_COMPACT, and decompresses the
> main firmware (MAINFV) into RAM memory at address 0x800000. The
>
Commit f78c8bf2c64f.
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2 5/5] OvmfPkg: make the 4MB flash size the default (again)
2017-05-18 15:14 [PATCH v2 0/5] OvmfPkg: complete the 4MB flash image support ("-bios" / emulated variables) Laszlo Ersek
` (3 preceding siblings ...)
2017-05-18 15:14 ` [PATCH v2 4/5] OvmfPkg/README: document 4MB flash layout Laszlo Ersek
@ 2017-05-18 15:14 ` Laszlo Ersek
2017-05-23 16:05 ` Laszlo Ersek
4 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2017-05-18 15:14 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
The previously default 2MB can be explicitly selected with
-D FD_SIZE_2MB
or
-D FD_SIZE_IN_KB=2048
Cc: Jordan Justen <jordan.l.justen@intel.com>
Suggested-by: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
(cherry picked from commit bba8dfbec3bbc4fba7fa6398ba3cf76593e0725e)
---
Notes:
v2:
- no changes
- we can postpone this one patch if we want to wait for Xen to update
hvmloader
OvmfPkg/OvmfPkgIa32.dsc | 2 +-
OvmfPkg/OvmfPkgIa32X64.dsc | 2 +-
OvmfPkg/OvmfPkgX64.dsc | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index bd115c9ced93..8f0ab9f721fe 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -54,7 +54,7 @@ [Defines]
!ifdef $(FD_SIZE_4MB)
DEFINE FD_SIZE_IN_KB = 4096
!else
- DEFINE FD_SIZE_IN_KB = 2048
+ DEFINE FD_SIZE_IN_KB = 4096
!endif
!endif
!endif
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 9727db842922..4b2136262865 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -54,7 +54,7 @@ [Defines]
!ifdef $(FD_SIZE_4MB)
DEFINE FD_SIZE_IN_KB = 4096
!else
- DEFINE FD_SIZE_IN_KB = 2048
+ DEFINE FD_SIZE_IN_KB = 4096
!endif
!endif
!endif
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 61aaed761657..2ee30fb00a28 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -54,7 +54,7 @@ [Defines]
!ifdef $(FD_SIZE_4MB)
DEFINE FD_SIZE_IN_KB = 4096
!else
- DEFINE FD_SIZE_IN_KB = 2048
+ DEFINE FD_SIZE_IN_KB = 4096
!endif
!endif
!endif
--
2.9.3
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH v2 5/5] OvmfPkg: make the 4MB flash size the default (again)
2017-05-18 15:14 ` [PATCH v2 5/5] OvmfPkg: make the 4MB flash size the default (again) Laszlo Ersek
@ 2017-05-23 16:05 ` Laszlo Ersek
0 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2017-05-23 16:05 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen
On 05/18/17 17:14, Laszlo Ersek wrote:
> The previously default 2MB can be explicitly selected with
>
> -D FD_SIZE_2MB
>
> or
>
> -D FD_SIZE_IN_KB=2048
>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Suggested-by: Jordan Justen <jordan.l.justen@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> (cherry picked from commit bba8dfbec3bbc4fba7fa6398ba3cf76593e0725e)
> ---
>
> Notes:
> v2:
> - no changes
> - we can postpone this one patch if we want to wait for Xen to update
> hvmloader
>
> OvmfPkg/OvmfPkgIa32.dsc | 2 +-
> OvmfPkg/OvmfPkgIa32X64.dsc | 2 +-
> OvmfPkg/OvmfPkgX64.dsc | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index bd115c9ced93..8f0ab9f721fe 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -54,7 +54,7 @@ [Defines]
> !ifdef $(FD_SIZE_4MB)
> DEFINE FD_SIZE_IN_KB = 4096
> !else
> - DEFINE FD_SIZE_IN_KB = 2048
> + DEFINE FD_SIZE_IN_KB = 4096
> !endif
> !endif
> !endif
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 9727db842922..4b2136262865 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -54,7 +54,7 @@ [Defines]
> !ifdef $(FD_SIZE_4MB)
> DEFINE FD_SIZE_IN_KB = 4096
> !else
> - DEFINE FD_SIZE_IN_KB = 2048
> + DEFINE FD_SIZE_IN_KB = 4096
> !endif
> !endif
> !endif
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 61aaed761657..2ee30fb00a28 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -54,7 +54,7 @@ [Defines]
> !ifdef $(FD_SIZE_4MB)
> DEFINE FD_SIZE_IN_KB = 4096
> !else
> - DEFINE FD_SIZE_IN_KB = 2048
> + DEFINE FD_SIZE_IN_KB = 4096
> !endif
> !endif
> !endif
>
This patch has been now committed (completing the series); please refer
to <https://lists.01.org/pipermail/edk2-devel/2017-May/010911.html>.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 15+ messages in thread