The "Confirm64KilobytesOfUnauthenticatedVariableStorage" test case of the
Secure Boot Logo Test ("Microsoft.UefiSecureBootLogo.Tests") suite in the
Microsoft Hardware Certification Kit expects to be able to populate the
variable store up to roughly 64 KB, with a series of 1 KB sized,
unauthenticated variables. OVMF's current live varstore area is too small
for this: 56 KB.
Introduce the FD_SIZE_4MB build macro (equivalently, FD_SIZE_IN_KB=4096),
which
- enlarges the full flash image to 4MB -- QEMU supports up to 8MB, see
FLASH_MAP_BASE_MIN in "hw/i386/pc_sysfw.c" --,
- inside that, grows the varstore area / pflash chip to 528 KB, and within
it, the live area from 56 KB to 256 KB.
Importantly, a firmware binary built with -D FD_SIZE_4MB will *not* be
compatible with a variable store that originates from a variable store
template built *without* -D FD_SIZE_4MB. This is the reason for the large
increase, as every such change breaks compatibility between a new firmware
binary and old varstore files.
Enlarging the varstore does not impact the performance of normal
operations, as we keep the varstore block size 4KB. The performance of
reclaim is affected, but that is expected (since reclaim has to rework the
full live area). And, reclaim occurs proportionally less frequently.
While at it, the FVMAIN_COMPACT volume (with the compressed FFS file in
it) is also enlarged significantly, so that we have plenty of room for
future DXEFV (and perhaps PEIFV) increments -- DXEFV has been growing
steadily, and that increase shows through compression too. Right now the
PEIFV and DXEFV volumes need no resizing.
Here's a summary:
Description Compression type Size [KB]
------------------------- ----------------- ----------------------
Non-volatile data storage open-coded binary 128 -> 528 ( +400)
data
Variable store 56 -> 256 ( +200)
Event log 4 -> 4 ( +0)
Working block 4 -> 4 ( +0)
Spare area 64 -> 264 ( +200)
FVMAIN_COMPACT uncompressed 1712 -> 3360 (+1648)
FV FFS file LZMA compressed
PEIFV uncompressed 896 -> 896 ( +0)
individual PEI uncompressed
modules
DXEFV uncompressed 10240 -> 10240 ( +0)
individual DXE uncompressed
modules
SECFV uncompressed 208 -> 208 ( +0)
SEC driver
reset vector code
For now, the 2MB flash image remains the default.
Cc: Gary Ching-Pang Lin <glin@suse.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
Notes:
v2:
- use $(FD_SIZE_IN_KB) in conditional statements
- Raise VARS_LIVE_SIZE by 8KB to 256KB, VARS_SPARE_SIZE by 8KB to 264KB,
thereby raising the containing VARS_SIZE by 16KB to 528KB. To
compensate, raise CODE_BASE_ADDRESS by 16KB, and shrink both
FVMAIN_SIZE and the containing CODE_SIZE by 16KB. No change to
FW_BASE_ADDRESS, FW_SIZE, SECFV_OFFSET, SECFV_SIZE. [Jordan]
OvmfPkg/OvmfPkgIa32.dsc | 4 ++
OvmfPkg/OvmfPkgIa32X64.dsc | 4 ++
OvmfPkg/OvmfPkgX64.dsc | 4 ++
OvmfPkg/OvmfPkg.fdf.inc | 28 ++++++++++
OvmfPkg/VarStore.fdf.inc | 54 +++++++++++++++++++-
5 files changed, 92 insertions(+), 2 deletions(-)
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 5a21840a55c9..26b807dde9fa 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -41,29 +41,33 @@ [Defines]
DEFINE TLS_ENABLE = FALSE
#
# Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
# one of the supported values, in place of any of the convenience macros, is
# permitted.
#
!ifdef $(FD_SIZE_1MB)
DEFINE FD_SIZE_IN_KB = 1024
!else
!ifdef $(FD_SIZE_2MB)
DEFINE FD_SIZE_IN_KB = 2048
!else
+!ifdef $(FD_SIZE_4MB)
+ DEFINE FD_SIZE_IN_KB = 4096
+!else
DEFINE FD_SIZE_IN_KB = 2048
!endif
!endif
+!endif
[BuildOptions]
GCC:*_UNIXGCC_*_CC_FLAGS = -DMDEPKG_NDEBUG
GCC:RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG
INTEL:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG
MSFT:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG
GCC:*_*_*_CC_FLAGS = -mno-mmx -mno-sse
#
# Disable deprecated APIs.
#
MSFT:*_*_*_CC_FLAGS = /D DISABLE_NEW_DEPRECATED_INTERFACES
INTEL:*_*_*_CC_FLAGS = /D DISABLE_NEW_DEPRECATED_INTERFACES
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 11866b7207c7..41f06a6b6a66 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -41,29 +41,33 @@ [Defines]
DEFINE TLS_ENABLE = FALSE
#
# Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
# one of the supported values, in place of any of the convenience macros, is
# permitted.
#
!ifdef $(FD_SIZE_1MB)
DEFINE FD_SIZE_IN_KB = 1024
!else
!ifdef $(FD_SIZE_2MB)
DEFINE FD_SIZE_IN_KB = 2048
!else
+!ifdef $(FD_SIZE_4MB)
+ DEFINE FD_SIZE_IN_KB = 4096
+!else
DEFINE FD_SIZE_IN_KB = 2048
!endif
!endif
+!endif
[BuildOptions]
GCC:*_UNIXGCC_*_CC_FLAGS = -DMDEPKG_NDEBUG
GCC:RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG
INTEL:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG
MSFT:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG
GCC:*_*_*_CC_FLAGS = -mno-mmx -mno-sse
!ifdef $(SOURCE_DEBUG_ENABLE)
MSFT:*_*_X64_GENFW_FLAGS = --keepexceptiontable
GCC:*_*_X64_GENFW_FLAGS = --keepexceptiontable
INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
!endif
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 2fab544600f5..053c84b685c5 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -41,29 +41,33 @@ [Defines]
DEFINE TLS_ENABLE = FALSE
#
# Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
# one of the supported values, in place of any of the convenience macros, is
# permitted.
#
!ifdef $(FD_SIZE_1MB)
DEFINE FD_SIZE_IN_KB = 1024
!else
!ifdef $(FD_SIZE_2MB)
DEFINE FD_SIZE_IN_KB = 2048
!else
+!ifdef $(FD_SIZE_4MB)
+ DEFINE FD_SIZE_IN_KB = 4096
+!else
DEFINE FD_SIZE_IN_KB = 2048
!endif
!endif
+!endif
[BuildOptions]
GCC:*_UNIXGCC_*_CC_FLAGS = -DMDEPKG_NDEBUG
GCC:RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG
INTEL:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG
MSFT:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG
GCC:*_*_*_CC_FLAGS = -mno-mmx -mno-sse
!ifdef $(SOURCE_DEBUG_ENABLE)
MSFT:*_*_X64_GENFW_FLAGS = --keepexceptiontable
GCC:*_*_X64_GENFW_FLAGS = --keepexceptiontable
INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
!endif
diff --git a/OvmfPkg/OvmfPkg.fdf.inc b/OvmfPkg/OvmfPkg.fdf.inc
index 4e72e35678a2..b3e0c472a1a8 100644
--- a/OvmfPkg/OvmfPkg.fdf.inc
+++ b/OvmfPkg/OvmfPkg.fdf.inc
@@ -6,55 +6,83 @@
#
# 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.
#
##
DEFINE BLOCK_SIZE = 0x1000
+
+#
+# A firmware binary built with FD_SIZE_IN_KB=1024, and a firmware binary built
+# with FD_SIZE_IN_KB=2048, use the same variable store layout.
+#
+# Setting FD_SIZE_IN_KB to 4096 results in a different (much larger) variable
+# store structure that is incompatible with both of the above-mentioned
+# firmware binaries.
+#
+!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
DEFINE VARS_SIZE = 0x20000
DEFINE VARS_BLOCKS = 0x20
DEFINE VARS_LIVE_SIZE = 0xE000
DEFINE VARS_SPARE_SIZE = 0x10000
+!endif
!if $(FD_SIZE_IN_KB) == 1024
DEFINE FW_BASE_ADDRESS = 0xFFF00000
DEFINE FW_SIZE = 0x00100000
DEFINE FW_BLOCKS = 0x100
DEFINE CODE_BASE_ADDRESS = 0xFFF20000
DEFINE CODE_SIZE = 0x000E0000
DEFINE CODE_BLOCKS = 0xE0
DEFINE FVMAIN_SIZE = 0x000CC000
DEFINE SECFV_OFFSET = 0x000EC000
DEFINE SECFV_SIZE = 0x14000
!endif
!if $(FD_SIZE_IN_KB) == 2048
DEFINE FW_BASE_ADDRESS = 0xFFE00000
DEFINE FW_SIZE = 0x00200000
DEFINE FW_BLOCKS = 0x200
DEFINE CODE_BASE_ADDRESS = 0xFFE20000
DEFINE CODE_SIZE = 0x001E0000
DEFINE CODE_BLOCKS = 0x1E0
DEFINE FVMAIN_SIZE = 0x001AC000
DEFINE SECFV_OFFSET = 0x001CC000
DEFINE SECFV_SIZE = 0x34000
!endif
+!if $(FD_SIZE_IN_KB) == 4096
+DEFINE VARS_SIZE = 0x84000
+DEFINE VARS_BLOCKS = 0x84
+DEFINE VARS_LIVE_SIZE = 0x40000
+DEFINE VARS_SPARE_SIZE = 0x42000
+
+DEFINE FW_BASE_ADDRESS = 0xFFC00000
+DEFINE FW_SIZE = 0x00400000
+DEFINE FW_BLOCKS = 0x400
+DEFINE CODE_BASE_ADDRESS = 0xFFC84000
+DEFINE CODE_SIZE = 0x0037C000
+DEFINE CODE_BLOCKS = 0x37C
+DEFINE FVMAIN_SIZE = 0x00348000
+DEFINE SECFV_OFFSET = 0x003CC000
+DEFINE SECFV_SIZE = 0x34000
+!endif
+
SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress = $(FW_BASE_ADDRESS)
SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize = $(FW_SIZE)
SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareBlockSize = $(BLOCK_SIZE)
SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase = $(FW_BASE_ADDRESS)
SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize = $(VARS_LIVE_SIZE)
SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize = $(BLOCK_SIZE)
SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwWorkingBase = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize
SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize = $(BLOCK_SIZE)
diff --git a/OvmfPkg/VarStore.fdf.inc b/OvmfPkg/VarStore.fdf.inc
index ce901c0109b1..742fed105334 100644
--- a/OvmfPkg/VarStore.fdf.inc
+++ b/OvmfPkg/VarStore.fdf.inc
@@ -5,68 +5,118 @@
# Copyright (c) 2006 - 2013, 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.
#
##
+!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
0x00000000|0x0000e000
+!endif
+!if $(FD_SIZE_IN_KB) == 4096
+0x00000000|0x00040000
+!endif
#NV_VARIABLE_STORE
DATA = {
## This is the EFI_FIRMWARE_VOLUME_HEADER
# ZeroVector []
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
# FileSystemGuid: gEfiSystemNvDataFvGuid =
# { 0xFFF12B8D, 0x7696, 0x4C8B,
# { 0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50 }}
0x8D, 0x2B, 0xF1, 0xFF, 0x96, 0x76, 0x8B, 0x4C,
0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50,
+!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
# FvLength: 0x20000
0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00,
+!endif
+!if $(FD_SIZE_IN_KB) == 4096
+ # FvLength: 0x84000
+ 0x00, 0x40, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00,
+!endif
# Signature "_FVH" # Attributes
0x5f, 0x46, 0x56, 0x48, 0xff, 0xfe, 0x04, 0x00,
- # HeaderLength # CheckSum # ExtHeaderOffset #Reserved #Revision
- 0x48, 0x00, 0x19, 0xF9, 0x00, 0x00, 0x00, 0x02,
+ # HeaderLength
+ 0x48, 0x00,
+!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
+ # CheckSum
+ 0x19, 0xF9,
+!endif
+!if $(FD_SIZE_IN_KB) == 4096
+ # CheckSum
+ 0xAF, 0xB8,
+!endif
+ # ExtHeaderOffset #Reserved #Revision
+ 0x00, 0x00, 0x00, 0x02,
+!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
# Blockmap[0]: 0x20 Blocks * 0x1000 Bytes / Block
0x20, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, 0x00,
+!endif
+!if $(FD_SIZE_IN_KB) == 4096
+ # Blockmap[0]: 0x84 Blocks * 0x1000 Bytes / Block
+ 0x84, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, 0x00,
+!endif
# Blockmap[1]: End
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
## This is the VARIABLE_STORE_HEADER
# It is compatible with SECURE_BOOT_ENABLE == FALSE as well.
# Signature: gEfiAuthenticatedVariableGuid =
# { 0xaaf32c78, 0x947b, 0x439a,
# { 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92 }}
0x78, 0x2c, 0xf3, 0xaa, 0x7b, 0x94, 0x9a, 0x43,
0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92,
+!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
# Size: 0xe000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) -
# 0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0xdfb8
# This can speed up the Variable Dispatch a bit.
0xB8, 0xDF, 0x00, 0x00,
+!endif
+!if $(FD_SIZE_IN_KB) == 4096
+ # Size: 0x40000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) -
+ # 0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0x3ffb8
+ # This can speed up the Variable Dispatch a bit.
+ 0xB8, 0xFF, 0x03, 0x00,
+!endif
# FORMATTED: 0x5A #HEALTHY: 0xFE #Reserved: UINT16 #Reserved1: UINT32
0x5A, 0xFE, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
}
+!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
0x0000e000|0x00001000
+!endif
+!if $(FD_SIZE_IN_KB) == 4096
+0x00040000|0x00001000
+!endif
#NV_EVENT_LOG
+!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
0x0000f000|0x00001000
+!endif
+!if $(FD_SIZE_IN_KB) == 4096
+0x00041000|0x00001000
+!endif
#NV_FTW_WORKING
DATA = {
# EFI_FAULT_TOLERANT_WORKING_BLOCK_HEADER->Signature = gEdkiiWorkingBlockSignatureGuid =
# { 0x9e58292b, 0x7c68, 0x497d, { 0xa0, 0xce, 0x65, 0x0, 0xfd, 0x9f, 0x1b, 0x95 }}
0x2b, 0x29, 0x58, 0x9e, 0x68, 0x7c, 0x7d, 0x49,
0xa0, 0xce, 0x65, 0x0, 0xfd, 0x9f, 0x1b, 0x95,
# Crc:UINT32 #WorkingBlockValid:1, WorkingBlockInvalid:1, Reserved
0x2c, 0xaf, 0x2c, 0x64, 0xFE, 0xFF, 0xFF, 0xFF,
# WriteQueueSize: UINT64
0xE0, 0x0F, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
}
+!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
0x00010000|0x00010000
+!endif
+!if $(FD_SIZE_IN_KB) == 4096
+0x00042000|0x00042000
+!endif
#NV_FTW_SPARE
--
2.9.3
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 05/03/17 23:39, Laszlo Ersek wrote:
> The "Confirm64KilobytesOfUnauthenticatedVariableStorage" test case of the
> Secure Boot Logo Test ("Microsoft.UefiSecureBootLogo.Tests") suite in the
> Microsoft Hardware Certification Kit expects to be able to populate the
> variable store up to roughly 64 KB, with a series of 1 KB sized,
> unauthenticated variables. OVMF's current live varstore area is too small
> for this: 56 KB.
>
> Introduce the FD_SIZE_4MB build macro (equivalently, FD_SIZE_IN_KB=4096),
> which
>
> - enlarges the full flash image to 4MB -- QEMU supports up to 8MB, see
> FLASH_MAP_BASE_MIN in "hw/i386/pc_sysfw.c" --,
>
> - inside that, grows the varstore area / pflash chip to 528 KB, and within
> it, the live area from 56 KB to 256 KB.
>
> Importantly, a firmware binary built with -D FD_SIZE_4MB will *not* be
> compatible with a variable store that originates from a variable store
> template built *without* -D FD_SIZE_4MB. This is the reason for the large
> increase, as every such change breaks compatibility between a new firmware
> binary and old varstore files.
>
> Enlarging the varstore does not impact the performance of normal
> operations, as we keep the varstore block size 4KB. The performance of
> reclaim is affected, but that is expected (since reclaim has to rework the
> full live area). And, reclaim occurs proportionally less frequently.
>
> While at it, the FVMAIN_COMPACT volume (with the compressed FFS file in
> it) is also enlarged significantly, so that we have plenty of room for
> future DXEFV (and perhaps PEIFV) increments -- DXEFV has been growing
> steadily, and that increase shows through compression too. Right now the
> PEIFV and DXEFV volumes need no resizing.
>
> Here's a summary:
>
> Description Compression type Size [KB]
> ------------------------- ----------------- ----------------------
> Non-volatile data storage open-coded binary 128 -> 528 ( +400)
> data
> Variable store 56 -> 256 ( +200)
> Event log 4 -> 4 ( +0)
> Working block 4 -> 4 ( +0)
> Spare area 64 -> 264 ( +200)
>
> FVMAIN_COMPACT uncompressed 1712 -> 3360 (+1648)
> FV FFS file LZMA compressed
> PEIFV uncompressed 896 -> 896 ( +0)
> individual PEI uncompressed
> modules
> DXEFV uncompressed 10240 -> 10240 ( +0)
> individual DXE uncompressed
> modules
>
> SECFV uncompressed 208 -> 208 ( +0)
> SEC driver
> reset vector code
>
> For now, the 2MB flash image remains the default.
>
> Cc: Gary Ching-Pang Lin <glin@suse.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
> v2:
> - use $(FD_SIZE_IN_KB) in conditional statements
>
> - Raise VARS_LIVE_SIZE by 8KB to 256KB, VARS_SPARE_SIZE by 8KB to 264KB,
Unfortunately, we have a terrible regression here. :(
It has to do with the emulated variable store.
The OvmfPkg/EmuVariableFvbRuntimeDxe driver exposes a two-block flash
device (FVB protocol) that uses the NV spare area size as internal block
size. And thereby covers the full NV area with just two blocks in total.
In addition, that driver keeps cutting corners everywhere, exploiting
very heavily that there are just two blocks. This assumption is
open-coded all over it.
This is also why we have the exact allocation logic in
ReserveEmuVariableNvStore() that we have, in PlatformPei.
The AllocateAlignedRuntimePages() call in ReserveEmuVariableNvStore()
depends on the NV spare area size being a power of two, because only
such values are accepted as Alignment.
And the FTW driver that consumes the FVB from EmuVariableFvbRuntimeDxe
also expects the FVB block size to be a power of two, which again
translates to the NV spare area size having to be a power of two.
(Because that's what EmuVariableFvbRuntimeDxe exposes as FVB block size,
for "all two" of its blocks.)
Changing the spare area size to 264KB breaks this extremely.
Booting the 4MB image with -bios is completely broken. I've been working
for a few hours on it now, but it's not just about refining the
granularity of the internal block size of the EmuVariableFvbRuntimeDxe
driver, to 4KB. As I said, the driver open-codes the two-block
assumption everywhere. For example, in the FvbProtocolEraseBlocks()
function, a complete loop is missing. :(
Adapting EmuVariableFvbRuntimeDxe to a non-power-of-two NV spare area
size is hopeless. (Definitely hopeless for the time frame & resources
I'm looking at.)
Worse, even -pflash is broken in the 4MB build, actually. The
non-power-of-two NV spare area size, when used as Alignment for
AllocateAlignedRuntimePages() in ReserveEmuVariableNvStore(), triggers
an assertion. And this path is taken for the -pflash boot as well.
So why didn't I notice this earlier? This is why: I tested v1 (which had
a 248KB NV varstore size and a 256KB NV spare area size) against the
pure upstream master branch, as well as against some of my personal
development (long term in progresss) patches. Because the spare area
size was a power of two, v1 worked fine on the master branch (with pflash).
Unfortunately, when testing v2, I failed to test on the master branch
*without* also heaping my personal dev patches on top. And some of those
patches skip the ReserveEmuVariableNvStore() call, hence the
non-power-of-two spare area size didn't cause any problems. Obviously, I
didn't test the emu variable case, because I haven't used that in years.
I apologize for not noticing how hard the 264KB spare would conflict
with the EMU variable driver -- my only excuse is that I haven't used
the latter in years, and I've been carrying private patches that disable
ReserveEmuVariableNvStore() so that I don't even lose memory to a
(misleading) feature that I otherwise never use. This is why the error
in v2 on the common code path remained hidden from me.
So here's the only thing I can propose now. Flip the NV layout back to
the one seen in v1 -- 248KB varstore, 256KB spare, 3376KB
FVMAIN_COMPACT. That will keep all the assumptions about the spare area
being a power of two satisfied, in PlatformPei, and in the FTW driver
through EmuVariableFvbRuntimeDxe's FVB protocol.
This can be done with one additional patch.
Another patch is necessary for synchronizing PcdVariableStoreSize with
PcdFlashNvStorageVariableSize. The former gives the varstore size for
*volatile* variables, and it normally doesn't have to match the size of
the non-volatile varstore (as evidenced by the -pflash case, with v1).
But, EmuVariableFvbRuntimeDxe abuses PcdVariableStoreSize for setting
the size field in the NV varstore header that it produces, and if both
PCDs aren't in sync, then in the -bios case, the variable driver blows
up with an assertion failure.
Jordan, does this plan look acceptable to you?
The 2MB build continues to work with -bios; at least I don't seem to
have broken that.
Thanks
Laszlo
> thereby raising the containing VARS_SIZE by 16KB to 528KB. To
> compensate, raise CODE_BASE_ADDRESS by 16KB, and shrink both
> FVMAIN_SIZE and the containing CODE_SIZE by 16KB. No change to
> FW_BASE_ADDRESS, FW_SIZE, SECFV_OFFSET, SECFV_SIZE. [Jordan]
>
> OvmfPkg/OvmfPkgIa32.dsc | 4 ++
> OvmfPkg/OvmfPkgIa32X64.dsc | 4 ++
> OvmfPkg/OvmfPkgX64.dsc | 4 ++
> OvmfPkg/OvmfPkg.fdf.inc | 28 ++++++++++
> OvmfPkg/VarStore.fdf.inc | 54 +++++++++++++++++++-
> 5 files changed, 92 insertions(+), 2 deletions(-)
>
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 5a21840a55c9..26b807dde9fa 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -41,29 +41,33 @@ [Defines]
> DEFINE TLS_ENABLE = FALSE
>
> #
> # Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
> # one of the supported values, in place of any of the convenience macros, is
> # permitted.
> #
> !ifdef $(FD_SIZE_1MB)
> DEFINE FD_SIZE_IN_KB = 1024
> !else
> !ifdef $(FD_SIZE_2MB)
> DEFINE FD_SIZE_IN_KB = 2048
> !else
> +!ifdef $(FD_SIZE_4MB)
> + DEFINE FD_SIZE_IN_KB = 4096
> +!else
> DEFINE FD_SIZE_IN_KB = 2048
> !endif
> !endif
> +!endif
>
> [BuildOptions]
> GCC:*_UNIXGCC_*_CC_FLAGS = -DMDEPKG_NDEBUG
> GCC:RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG
> INTEL:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG
> MSFT:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG
> GCC:*_*_*_CC_FLAGS = -mno-mmx -mno-sse
>
> #
> # Disable deprecated APIs.
> #
> MSFT:*_*_*_CC_FLAGS = /D DISABLE_NEW_DEPRECATED_INTERFACES
> INTEL:*_*_*_CC_FLAGS = /D DISABLE_NEW_DEPRECATED_INTERFACES
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 11866b7207c7..41f06a6b6a66 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -41,29 +41,33 @@ [Defines]
> DEFINE TLS_ENABLE = FALSE
>
> #
> # Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
> # one of the supported values, in place of any of the convenience macros, is
> # permitted.
> #
> !ifdef $(FD_SIZE_1MB)
> DEFINE FD_SIZE_IN_KB = 1024
> !else
> !ifdef $(FD_SIZE_2MB)
> DEFINE FD_SIZE_IN_KB = 2048
> !else
> +!ifdef $(FD_SIZE_4MB)
> + DEFINE FD_SIZE_IN_KB = 4096
> +!else
> DEFINE FD_SIZE_IN_KB = 2048
> !endif
> !endif
> +!endif
>
> [BuildOptions]
> GCC:*_UNIXGCC_*_CC_FLAGS = -DMDEPKG_NDEBUG
> GCC:RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG
> INTEL:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG
> MSFT:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG
> GCC:*_*_*_CC_FLAGS = -mno-mmx -mno-sse
> !ifdef $(SOURCE_DEBUG_ENABLE)
> MSFT:*_*_X64_GENFW_FLAGS = --keepexceptiontable
> GCC:*_*_X64_GENFW_FLAGS = --keepexceptiontable
> INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
> !endif
>
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 2fab544600f5..053c84b685c5 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -41,29 +41,33 @@ [Defines]
> DEFINE TLS_ENABLE = FALSE
>
> #
> # Flash size selection. Setting FD_SIZE_IN_KB on the command line directly to
> # one of the supported values, in place of any of the convenience macros, is
> # permitted.
> #
> !ifdef $(FD_SIZE_1MB)
> DEFINE FD_SIZE_IN_KB = 1024
> !else
> !ifdef $(FD_SIZE_2MB)
> DEFINE FD_SIZE_IN_KB = 2048
> !else
> +!ifdef $(FD_SIZE_4MB)
> + DEFINE FD_SIZE_IN_KB = 4096
> +!else
> DEFINE FD_SIZE_IN_KB = 2048
> !endif
> !endif
> +!endif
>
> [BuildOptions]
> GCC:*_UNIXGCC_*_CC_FLAGS = -DMDEPKG_NDEBUG
> GCC:RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG
> INTEL:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG
> MSFT:RELEASE_*_*_CC_FLAGS = /D MDEPKG_NDEBUG
> GCC:*_*_*_CC_FLAGS = -mno-mmx -mno-sse
> !ifdef $(SOURCE_DEBUG_ENABLE)
> MSFT:*_*_X64_GENFW_FLAGS = --keepexceptiontable
> GCC:*_*_X64_GENFW_FLAGS = --keepexceptiontable
> INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
> !endif
>
> diff --git a/OvmfPkg/OvmfPkg.fdf.inc b/OvmfPkg/OvmfPkg.fdf.inc
> index 4e72e35678a2..b3e0c472a1a8 100644
> --- a/OvmfPkg/OvmfPkg.fdf.inc
> +++ b/OvmfPkg/OvmfPkg.fdf.inc
> @@ -6,55 +6,83 @@
> #
> # 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.
> #
> ##
>
> DEFINE BLOCK_SIZE = 0x1000
> +
> +#
> +# A firmware binary built with FD_SIZE_IN_KB=1024, and a firmware binary built
> +# with FD_SIZE_IN_KB=2048, use the same variable store layout.
> +#
> +# Setting FD_SIZE_IN_KB to 4096 results in a different (much larger) variable
> +# store structure that is incompatible with both of the above-mentioned
> +# firmware binaries.
> +#
> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> DEFINE VARS_SIZE = 0x20000
> DEFINE VARS_BLOCKS = 0x20
> DEFINE VARS_LIVE_SIZE = 0xE000
> DEFINE VARS_SPARE_SIZE = 0x10000
> +!endif
>
> !if $(FD_SIZE_IN_KB) == 1024
> DEFINE FW_BASE_ADDRESS = 0xFFF00000
> DEFINE FW_SIZE = 0x00100000
> DEFINE FW_BLOCKS = 0x100
> DEFINE CODE_BASE_ADDRESS = 0xFFF20000
> DEFINE CODE_SIZE = 0x000E0000
> DEFINE CODE_BLOCKS = 0xE0
> DEFINE FVMAIN_SIZE = 0x000CC000
> DEFINE SECFV_OFFSET = 0x000EC000
> DEFINE SECFV_SIZE = 0x14000
> !endif
>
> !if $(FD_SIZE_IN_KB) == 2048
> DEFINE FW_BASE_ADDRESS = 0xFFE00000
> DEFINE FW_SIZE = 0x00200000
> DEFINE FW_BLOCKS = 0x200
> DEFINE CODE_BASE_ADDRESS = 0xFFE20000
> DEFINE CODE_SIZE = 0x001E0000
> DEFINE CODE_BLOCKS = 0x1E0
> DEFINE FVMAIN_SIZE = 0x001AC000
> DEFINE SECFV_OFFSET = 0x001CC000
> DEFINE SECFV_SIZE = 0x34000
> !endif
>
> +!if $(FD_SIZE_IN_KB) == 4096
> +DEFINE VARS_SIZE = 0x84000
> +DEFINE VARS_BLOCKS = 0x84
> +DEFINE VARS_LIVE_SIZE = 0x40000
> +DEFINE VARS_SPARE_SIZE = 0x42000
> +
> +DEFINE FW_BASE_ADDRESS = 0xFFC00000
> +DEFINE FW_SIZE = 0x00400000
> +DEFINE FW_BLOCKS = 0x400
> +DEFINE CODE_BASE_ADDRESS = 0xFFC84000
> +DEFINE CODE_SIZE = 0x0037C000
> +DEFINE CODE_BLOCKS = 0x37C
> +DEFINE FVMAIN_SIZE = 0x00348000
> +DEFINE SECFV_OFFSET = 0x003CC000
> +DEFINE SECFV_SIZE = 0x34000
> +!endif
> +
> SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress = $(FW_BASE_ADDRESS)
> SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize = $(FW_SIZE)
> SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareBlockSize = $(BLOCK_SIZE)
>
> SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase = $(FW_BASE_ADDRESS)
> SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize = $(VARS_LIVE_SIZE)
>
> SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
> SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize = $(BLOCK_SIZE)
>
> SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwWorkingBase = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize
> SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize = $(BLOCK_SIZE)
>
> diff --git a/OvmfPkg/VarStore.fdf.inc b/OvmfPkg/VarStore.fdf.inc
> index ce901c0109b1..742fed105334 100644
> --- a/OvmfPkg/VarStore.fdf.inc
> +++ b/OvmfPkg/VarStore.fdf.inc
> @@ -5,68 +5,118 @@
> # Copyright (c) 2006 - 2013, 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.
> #
> ##
>
> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> 0x00000000|0x0000e000
> +!endif
> +!if $(FD_SIZE_IN_KB) == 4096
> +0x00000000|0x00040000
> +!endif
> #NV_VARIABLE_STORE
> DATA = {
> ## This is the EFI_FIRMWARE_VOLUME_HEADER
> # ZeroVector []
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> # FileSystemGuid: gEfiSystemNvDataFvGuid =
> # { 0xFFF12B8D, 0x7696, 0x4C8B,
> # { 0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50 }}
> 0x8D, 0x2B, 0xF1, 0xFF, 0x96, 0x76, 0x8B, 0x4C,
> 0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50,
> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> # FvLength: 0x20000
> 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00,
> +!endif
> +!if $(FD_SIZE_IN_KB) == 4096
> + # FvLength: 0x84000
> + 0x00, 0x40, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00,
> +!endif
> # Signature "_FVH" # Attributes
> 0x5f, 0x46, 0x56, 0x48, 0xff, 0xfe, 0x04, 0x00,
> - # HeaderLength # CheckSum # ExtHeaderOffset #Reserved #Revision
> - 0x48, 0x00, 0x19, 0xF9, 0x00, 0x00, 0x00, 0x02,
> + # HeaderLength
> + 0x48, 0x00,
> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> + # CheckSum
> + 0x19, 0xF9,
> +!endif
> +!if $(FD_SIZE_IN_KB) == 4096
> + # CheckSum
> + 0xAF, 0xB8,
> +!endif
> + # ExtHeaderOffset #Reserved #Revision
> + 0x00, 0x00, 0x00, 0x02,
> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> # Blockmap[0]: 0x20 Blocks * 0x1000 Bytes / Block
> 0x20, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, 0x00,
> +!endif
> +!if $(FD_SIZE_IN_KB) == 4096
> + # Blockmap[0]: 0x84 Blocks * 0x1000 Bytes / Block
> + 0x84, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, 0x00,
> +!endif
> # Blockmap[1]: End
> 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
> ## This is the VARIABLE_STORE_HEADER
> # It is compatible with SECURE_BOOT_ENABLE == FALSE as well.
> # Signature: gEfiAuthenticatedVariableGuid =
> # { 0xaaf32c78, 0x947b, 0x439a,
> # { 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92 }}
> 0x78, 0x2c, 0xf3, 0xaa, 0x7b, 0x94, 0x9a, 0x43,
> 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92,
> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> # Size: 0xe000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) -
> # 0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0xdfb8
> # This can speed up the Variable Dispatch a bit.
> 0xB8, 0xDF, 0x00, 0x00,
> +!endif
> +!if $(FD_SIZE_IN_KB) == 4096
> + # Size: 0x40000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) -
> + # 0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0x3ffb8
> + # This can speed up the Variable Dispatch a bit.
> + 0xB8, 0xFF, 0x03, 0x00,
> +!endif
> # FORMATTED: 0x5A #HEALTHY: 0xFE #Reserved: UINT16 #Reserved1: UINT32
> 0x5A, 0xFE, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> }
>
> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> 0x0000e000|0x00001000
> +!endif
> +!if $(FD_SIZE_IN_KB) == 4096
> +0x00040000|0x00001000
> +!endif
> #NV_EVENT_LOG
>
> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> 0x0000f000|0x00001000
> +!endif
> +!if $(FD_SIZE_IN_KB) == 4096
> +0x00041000|0x00001000
> +!endif
> #NV_FTW_WORKING
> DATA = {
> # EFI_FAULT_TOLERANT_WORKING_BLOCK_HEADER->Signature = gEdkiiWorkingBlockSignatureGuid =
> # { 0x9e58292b, 0x7c68, 0x497d, { 0xa0, 0xce, 0x65, 0x0, 0xfd, 0x9f, 0x1b, 0x95 }}
> 0x2b, 0x29, 0x58, 0x9e, 0x68, 0x7c, 0x7d, 0x49,
> 0xa0, 0xce, 0x65, 0x0, 0xfd, 0x9f, 0x1b, 0x95,
> # Crc:UINT32 #WorkingBlockValid:1, WorkingBlockInvalid:1, Reserved
> 0x2c, 0xaf, 0x2c, 0x64, 0xFE, 0xFF, 0xFF, 0xFF,
> # WriteQueueSize: UINT64
> 0xE0, 0x0F, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
> }
>
> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> 0x00010000|0x00010000
> +!endif
> +!if $(FD_SIZE_IN_KB) == 4096
> +0x00042000|0x00042000
> +!endif
> #NV_FTW_SPARE
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 2017-05-04 21:07:24, Laszlo Ersek wrote: > On 05/03/17 23:39, Laszlo Ersek wrote: > > - Raise VARS_LIVE_SIZE by 8KB to 256KB, VARS_SPARE_SIZE by 8KB to 264KB, > > Unfortunately, we have a terrible regression here. :( > <snip> > Adapting EmuVariableFvbRuntimeDxe to a non-power-of-two NV spare area > size is hopeless. (Definitely hopeless for the time frame & resources > I'm looking at.) > > Worse, even -pflash is broken in the 4MB build, actually. The > non-power-of-two NV spare area size, when used as Alignment for > AllocateAlignedRuntimePages() in ReserveEmuVariableNvStore(), triggers > an assertion. And this path is taken for the -pflash boot as well. For a short term fix, would something like this work? 1. Force emu fvb buffer alignment to next power-of-two Something like the attachment, but I'm guessing you already wrote something similar. 2. Revert 4MB by default patch This should allow you to start using the 4MB layout for your builds, and we can fix the non-flash path before re-enabling 4MB as the default. -Jordan _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 05/05/17 10:57, Jordan Justen wrote:
> On 2017-05-04 21:07:24, Laszlo Ersek wrote:
>> On 05/03/17 23:39, Laszlo Ersek wrote:
>>> - Raise VARS_LIVE_SIZE by 8KB to 256KB, VARS_SPARE_SIZE by 8KB to 264KB,
>> Unfortunately, we have a terrible regression here. :(
>>
> <snip>
>
>> Adapting EmuVariableFvbRuntimeDxe to a non-power-of-two NV spare area
>> size is hopeless. (Definitely hopeless for the time frame & resources
>> I'm looking at.)
>>
>> Worse, even -pflash is broken in the 4MB build, actually. The
>> non-power-of-two NV spare area size, when used as Alignment for
>> AllocateAlignedRuntimePages() in ReserveEmuVariableNvStore(), triggers
>> an assertion. And this path is taken for the -pflash boot as well.
> For a short term fix, would something like this work?
Absolutely. I didn't dare ask for it.
> 1. Force emu fvb buffer alignment to next power-of-two
>
> Something like the attachment, but I'm guessing you already wrote
> something similar.
Yes, I have almost exactly that; please see the attached
"OvmfPkg/PlatformPei: handle non-power-of-two spare size for emu variables".
The difference is that your patch uses HighBitSet32() directly, which
mine uses through GetPowerOfTwo32(). Also yours starts with the full
size, and then subtracts one for the shift in the alignment; mine starts
with half the size (i.e., the spare area size) and uses that in the
alignment.
One other comment on the patch:
> 0001-PlatformPei-Force-EmuVariableNvStore-alignment-size-.patch
>
>
> From 58ba393adf60caf806b26dec9aa56d936743f595 Mon Sep 17 00:00:00 2001
> From: Jordan Justen <jordan.l.justen@intel.com>
> Date: Fri, 5 May 2017 01:43:31 -0700
> Subject: [PATCH] PlatformPei: Force EmuVariableNvStore alignment size to power
> of two
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jordan Justen <jordan.l.justen@intel.com>
> ---
> OvmfPkg/PlatformPei/Platform.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 77a8a16c15..97dce8de92 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -504,6 +504,14 @@ ReserveEmuVariableNvStore (
> {
> EFI_PHYSICAL_ADDRESS VariableStore;
> RETURN_STATUS PcdStatus;
> + UINT32 EmuFvbSize;
> + INTN SizeHighBit;
> +
> + EmuFvbSize = 2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize);
> + SizeHighBit = HighBitSet32 (EmuFvbSize);
> + if ((EmuFvbSize & (EmuFvbSize - 1)) != 0) {
> + SizeHighBit++;
> + }
>
> //
> // Allocate storage for NV variables early on so it will be
> @@ -514,13 +522,13 @@ ReserveEmuVariableNvStore (
> VariableStore =
> (EFI_PHYSICAL_ADDRESS)(UINTN)
> AllocateAlignedRuntimePages (
> - EFI_SIZE_TO_PAGES (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)),
> - PcdGet32 (PcdFlashNvStorageFtwSpareSize)
> + EFI_SIZE_TO_PAGES (EmuFvbSize),
I think we should cast EmuFvbSize to UINTN first; EFI_SIZE_TO_PAGES()
likes the "Size" parameter to be UINTN.
> + 1 << (SizeHighBit - 1)
> );
> DEBUG ((EFI_D_INFO,
> "Reserved variable store memory: 0x%lX; size: %dkb\n",
> VariableStore,
> - (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)) / 1024
> + EmuFvbSize / 1024
> ));
> PcdStatus = PcdSet64S (PcdEmuVariableNvStoreReserved, VariableStore);
> ASSERT_RETURN_ERROR (PcdStatus);
> -- 2.11.0
>
Which one do you prefer:
(1) I can take your patch, stick in the UINTN cast, and expand the
commit message a bit (similarly to what's on my patch),
(2) we can go with my patch as well.
I'm tempted to do (1) and commit it with my R-b immediately, but I
realize that "rushing it" is the root of all evil. So I won't rush it.
On 05/05/17 10:57, Jordan Justen wrote:
> 2. Revert 4MB by default patch
>
> This should allow you to start using the 4MB layout for your builds,
> and we can fix the non-flash path before re-enabling 4MB as the
> default.
This works for me, yes. Thank you.
Regarding the non-flash path, I have the attached work-in-progress patches:
- "OvmfPkg: sync PcdVariableStoreSize with PcdFlashNvStorageVariableSize",
- and "wip".
I think that the "wip" patch does all the "simple" fixes in
EmuVariableFvbRuntimeDxe, and what remains is to add code so that the
FVB protocol members actually do their job. Also, the "wip" patch
eliminates any special alignment in the AllocateRuntimePages() call of
PlatformPei, since after the "block size = page size" change, no such
alignment will be necessary. What do you think of this?
So here's the plan:
- based on what you prefer for (1) vs. (2), I'll post that patch as
first patch,
- I'll post the revert of the 4MB default as second patch,
- I think I'll immediately post the patch that syncs
PcdVariableStoreSize as well, as third patch, because it is a small
improvement for the pflash case too.
Does this work for you?
Thank you,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
On 2017-05-05 05:07:25, Laszlo Ersek wrote: > > Which one do you prefer: > (1) I can take your patch, stick in the UINTN cast, and expand the > commit message a bit (similarly to what's on my patch), > (2) we can go with my patch as well. I prefer your patch. Re: "0001-OvmfPkg-PlatformPei-handle-non-power-of-two-spare-si.patch" > + Alignment = GetPowerOfTwo32 (Alignment) << 1; Do you think this might end up needing a UINT32 cast with 64-bit PEI? Reviewed-by: Jordan Justen <jordan.l.justen@intel.com> > Regarding the non-flash path, I have the attached work-in-progress patches: > > - "OvmfPkg: sync PcdVariableStoreSize with PcdFlashNvStorageVariableSize", > - and "wip". I looked over the non-wip patch, and it seems good, but maybe we should just revert the default size for now. For a revert of bba8dfbec3bbc4fba7fa6398ba3cf76593e0725e: 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/05/17 17:43, Jordan Justen wrote:
> On 2017-05-05 05:07:25, Laszlo Ersek wrote:
>>
>> Which one do you prefer:
>> (1) I can take your patch, stick in the UINTN cast, and expand the
>> commit message a bit (similarly to what's on my patch),
>> (2) we can go with my patch as well.
>
> I prefer your patch.
>
> Re: "0001-OvmfPkg-PlatformPei-handle-non-power-of-two-spare-si.patch"
>
>> + Alignment = GetPowerOfTwo32 (Alignment) << 1;
>
> Do you think this might end up needing a UINT32 cast with 64-bit PEI?
No, the prototype is
UINT32
EFIAPI
GetPowerOfTwo32 (
IN UINT32 Operand
)
and we can safely shift the returned UINT32 in both 32-bit PEI and
64-bit PEI with the << operator. The result of the shift will also have
type UINT32, which we can safely re-assign to the UINT32 Alignment variable.
Shifting the result of GetPowerOfTwo64() with << would be problematic in
32-bit PEI.
>
> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
Thanks!
I verified this in the (now again default 2MB build) with the -bios
command line like this:
(1) boot to the UEFI shell normally. The log says:
> Reserved variable store memory: 0x7F50000; size: 128kb,
> alignment: 0x10000
> ...
> EMU Variable FVB Started
> EMU Variable FVB: Using pre-reserved block at 7F50000
> EMU Variable FVB: Basic FV headers were invalid
> Installing FVB for EMU Variable support
> ...
> Ftw: FtwWorkSpaceLba - 0x0, WorkBlockSize - 0x10000,
> FtwWorkSpaceBase - 0xE000
> Ftw: FtwSpareLba - 0x1, SpareBlockSize - 0x10000
> Ftw: NumberOfWorkBlock - 0x1, FtwWorkBlockLba - 0x0
> Ftw: WorkSpaceLbaInSpare - 0x0, WorkSpaceBaseInSpare - 0xE000
> Ftw: Remaining work space size - FE0
> Ftw: Work block header check mismatch
> Ftw: Work block header check mismatch
> Ftw: Both working and spare blocks are invalid, init workspace
> Ftw: start to reclaim work space
> Ftw: reclaim work space successfully
(2) run the following shell commands:
Shell> setvar TestVar0000 -guid ebe29c42-f3d1-4f96-a7c2-5585ce88f056
-bs -nv
="0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
Shell> reset
(3) after reboot, the log says:
> Reserved variable store memory: 0x7F50000; size: 128kb,
> alignment: 0x10000
> ...
> EMU Variable FVB Started
> EMU Variable FVB: Using pre-reserved block at 7F50000
> EMU Variable FVB: Found valid pre-existing FV
> ...
> Ftw: FtwWorkSpaceLba - 0x0, WorkBlockSize - 0x10000,
> FtwWorkSpaceBase - 0xE000
> Ftw: FtwSpareLba - 0x1, SpareBlockSize - 0x10000
> Ftw: NumberOfWorkBlock - 0x1, FtwWorkBlockLba - 0x0
> Ftw: WorkSpaceLbaInSpare - 0x0, WorkSpaceBaseInSpare - 0xE000
> Ftw: Remaining work space size - FE0
(4) run the following shell command:
Shell> setvar TestVar0000 -guid ebe29c42-f3d1-4f96-a7c2-5585ce88f056
EBE29C42-F3D1-4F96-A7C2-5585CE88F056 - TestVar0000 - 0040 Bytes
01 23 45 67 89 AB CD EF 01 23 45 67 89 AB CD EF 01 23 45 67 89 AB CD EF
01 23 45 67 89 AB CD EF 01 23 45 67 89 AB CD EF 01 23 45 67 89 AB CD EF
01 23 45 67 89 AB CD EF 01 23 45 67 89 AB CD EF
Also tested this with the 4MB build, using pflash. The log says
> Reserved variable store memory: 0xBFE80000; size: 528kb,
> alignment: 0x80000
The alignment is 512KB, which is the next power of two after 264KB.
>
>> Regarding the non-flash path, I have the attached work-in-progress patches:
>>
>> - "OvmfPkg: sync PcdVariableStoreSize with PcdFlashNvStorageVariableSize",
>> - and "wip".
>
> I looked over the non-wip patch, and it seems good, but maybe we
> should just revert the default size for now.
Works for me.
> For a revert of bba8dfbec3bbc4fba7fa6398ba3cf76593e0725e:
>
> Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
>
Thanks!
Commits:
1 6e49d01cfb43 Revert "OvmfPkg: make the 4MB flash size the default"
2 0c79471d6a98 OvmfPkg/PlatformPei: handle non-power-of-two spare
size for emu variables
I've also filed the following TianoCore Feature Request:
https://bugzilla.tianocore.org/show_bug.cgi?id=527
If you or Gary feel inclined to work on this, that would be awesome. I
have no idea when I'll get to it; my TODO list hasn't been this long in
ages. Of course I cannot really *request* that you guys please pick up
my slack, just sayin' that you please feel free to... :)
Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
© 2016 - 2024 Red Hat, Inc.