* [PATCH v2 0/5] OvmfPkg: add FD_SIZE_4MB for Windows HCK SB tests, and for future proofing
@ 2017-05-03 21:39 Laszlo Ersek
2017-05-03 21:39 ` [PATCH v2 1/5] OvmfPkg: introduce the FD_SIZE_IN_KB macro / build flag Laszlo Ersek
` (4 more replies)
0 siblings, 5 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-05-03 21:39 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Gary Ching-Pang Lin, Jordan Justen
This is version 2 of the series posted at
<https://lists.01.org/pipermail/edk2-devel/2017-April/010334.html>.
Changes in v2:
- consolidate FD_SIZE_1MB, FD_SIZE_2MB, FD_SIZE_4MB, basing them all on
the new macro FD_SIZE_IN_KB,
- increase the variable store from 248 KB to 256 KB, and propagate that
change as necessary, at the expense of FVMAIN_COMPACT [Jordan],
- make the 4MB build the general default [Jordan].
V1->v2 changes are also marked on the individual patches.
Thoroughly retested with the usual S3 stuff, the Secure Boot Logo Test,
multiple purposefully triggered reclaims, hexdumps of varstore files,
etc.
Formatted with 13 lines of context for easier review.
Repo: https://github.com/lersek/edk2.git
Branch: fd_size_4mb_v2
Cc: Gary Ching-Pang Lin <glin@suse.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Thanks,
Laszlo
Laszlo Ersek (5):
OvmfPkg: introduce the FD_SIZE_IN_KB macro / build flag
OvmfPkg/OvmfPkg.fdf.inc: extract VARS_LIVE_SIZE and VARS_SPARE_SIZE
macros
OvmfPkg: introduce 4MB flash image (mainly) for Windows HCK
OvmfPkg: raise max variable size (auth & non-auth) to 33KB for
FD_SIZE_4MB
OvmfPkg: make the 4MB flash size the default
OvmfPkg/OvmfPkgIa32.dsc | 25 +++++++++
OvmfPkg/OvmfPkgIa32X64.dsc | 25 +++++++++
OvmfPkg/OvmfPkgX64.dsc | 25 +++++++++
OvmfPkg/OvmfPkg.fdf.inc | 46 ++++++++++++-----
OvmfPkg/VarStore.fdf.inc | 54 +++++++++++++++++++-
5 files changed, 161 insertions(+), 14 deletions(-)
--
2.9.3
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/5] OvmfPkg: introduce the FD_SIZE_IN_KB macro / build flag
2017-05-03 21:39 [PATCH v2 0/5] OvmfPkg: add FD_SIZE_4MB for Windows HCK SB tests, and for future proofing Laszlo Ersek
@ 2017-05-03 21:39 ` Laszlo Ersek
2017-05-03 21:39 ` [PATCH v2 2/5] OvmfPkg/OvmfPkg.fdf.inc: extract VARS_LIVE_SIZE and VARS_SPARE_SIZE macros Laszlo Ersek
` (3 subsequent siblings)
4 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-05-03 21:39 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Gary Ching-Pang Lin, Jordan Justen
FD_SIZE_xMB defines have existed for flash size selection. They can be
passed as "-D FD_SIZE_xMB" on the command line. Passing multiple of them
at the same time has never been supported; earlier settings on the command
line cannot be overridden.
Introduce the integer valued FD_SIZE_IN_KB macro, which provides the
following improvements:
- several instances of it are permitted on the command line, with the last
one taking effect,
- conditional statements in the DSC and FDF files need only check a single
macro, and multiple values can be checked in a single !if with the ||
operator,
- nested !ifdef / !else ladders can be replaced with flat equality tests,
- in the future, flash sizes can be expressed with a finer than MB
granularity, if necessary.
For now, we're going to preserve the FD_SIZE_xMB defines as convenience
wrappers for FD_SIZE_IN_KB.
FD_SIZE_IN_KB is being added to the DSC files because this way we can
depend on it in both the DSC and FDF files.
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:
- new in v2
OvmfPkg/OvmfPkgIa32.dsc | 15 +++++++++++++++
OvmfPkg/OvmfPkgIa32X64.dsc | 15 +++++++++++++++
OvmfPkg/OvmfPkgX64.dsc | 15 +++++++++++++++
OvmfPkg/OvmfPkg.fdf.inc | 14 +++-----------
4 files changed, 48 insertions(+), 11 deletions(-)
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 0796b0db816b..5a21840a55c9 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -30,26 +30,41 @@ [Defines]
SKUID_IDENTIFIER = DEFAULT
FLASH_DEFINITION = OvmfPkg/OvmfPkgIa32.fdf
#
# Defines for default states. These can be changed on the command line.
# -D FLAG=VALUE
#
DEFINE SECURE_BOOT_ENABLE = FALSE
DEFINE NETWORK_IP6_ENABLE = FALSE
DEFINE HTTP_BOOT_ENABLE = FALSE
DEFINE SMM_REQUIRE = FALSE
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
+ DEFINE FD_SIZE_IN_KB = 2048
+!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
GCC:*_*_*_CC_FLAGS = -D DISABLE_NEW_DEPRECATED_INTERFACES
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 71ac62f023b5..11866b7207c7 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -30,26 +30,41 @@ [Defines]
SKUID_IDENTIFIER = DEFAULT
FLASH_DEFINITION = OvmfPkg/OvmfPkgIa32X64.fdf
#
# Defines for default states. These can be changed on the command line.
# -D FLAG=VALUE
#
DEFINE SECURE_BOOT_ENABLE = FALSE
DEFINE NETWORK_IP6_ENABLE = FALSE
DEFINE HTTP_BOOT_ENABLE = FALSE
DEFINE SMM_REQUIRE = FALSE
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
+ DEFINE FD_SIZE_IN_KB = 2048
+!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 2ceb31d7ffd5..2fab544600f5 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -30,26 +30,41 @@ [Defines]
SKUID_IDENTIFIER = DEFAULT
FLASH_DEFINITION = OvmfPkg/OvmfPkgX64.fdf
#
# Defines for default states. These can be changed on the command line.
# -D FLAG=VALUE
#
DEFINE SECURE_BOOT_ENABLE = FALSE
DEFINE NETWORK_IP6_ENABLE = FALSE
DEFINE HTTP_BOOT_ENABLE = FALSE
DEFINE SMM_REQUIRE = FALSE
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
+ DEFINE FD_SIZE_IN_KB = 2048
+!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 9cc0578d6430..f12d61c2b682 100644
--- a/OvmfPkg/OvmfPkg.fdf.inc
+++ b/OvmfPkg/OvmfPkg.fdf.inc
@@ -5,60 +5,52 @@
# 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.
#
##
-#
-# Default flash size is 2MB.
-#
-# Defining FD_SIZE_1MB on the build command line can override this.
-#
-
DEFINE BLOCK_SIZE = 0x1000
DEFINE VARS_SIZE = 0x20000
DEFINE VARS_BLOCKS = 0x20
-!ifdef $(FD_SIZE_1MB)
-
+!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
-!else
-
+!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
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 = 0xE000
SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize = $(BLOCK_SIZE)
SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwWorkingBase = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize
--
2.9.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 2/5] OvmfPkg/OvmfPkg.fdf.inc: extract VARS_LIVE_SIZE and VARS_SPARE_SIZE macros
2017-05-03 21:39 [PATCH v2 0/5] OvmfPkg: add FD_SIZE_4MB for Windows HCK SB tests, and for future proofing Laszlo Ersek
2017-05-03 21:39 ` [PATCH v2 1/5] OvmfPkg: introduce the FD_SIZE_IN_KB macro / build flag Laszlo Ersek
@ 2017-05-03 21:39 ` Laszlo Ersek
2017-05-03 21:39 ` [PATCH v2 3/5] OvmfPkg: introduce 4MB flash image (mainly) for Windows HCK Laszlo Ersek
` (2 subsequent siblings)
4 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-05-03 21:39 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Gary Ching-Pang Lin, Jordan Justen
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:
- no changes
OvmfPkg/OvmfPkg.fdf.inc | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/OvmfPkg/OvmfPkg.fdf.inc b/OvmfPkg/OvmfPkg.fdf.inc
index f12d61c2b682..4e72e35678a2 100644
--- a/OvmfPkg/OvmfPkg.fdf.inc
+++ b/OvmfPkg/OvmfPkg.fdf.inc
@@ -8,26 +8,28 @@
# 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
DEFINE VARS_SIZE = 0x20000
DEFINE VARS_BLOCKS = 0x20
+DEFINE VARS_LIVE_SIZE = 0xE000
+DEFINE VARS_SPARE_SIZE = 0x10000
!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
@@ -38,25 +40,25 @@
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
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 = 0xE000
+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)
SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwSpareBase = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwWorkingBase + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
-SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize = 0x10000
+SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize = $(VARS_SPARE_SIZE)
DEFINE MEMFD_BASE_ADDRESS = 0x800000
--
2.9.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 3/5] OvmfPkg: introduce 4MB flash image (mainly) for Windows HCK
2017-05-03 21:39 [PATCH v2 0/5] OvmfPkg: add FD_SIZE_4MB for Windows HCK SB tests, and for future proofing Laszlo Ersek
2017-05-03 21:39 ` [PATCH v2 1/5] OvmfPkg: introduce the FD_SIZE_IN_KB macro / build flag Laszlo Ersek
2017-05-03 21:39 ` [PATCH v2 2/5] OvmfPkg/OvmfPkg.fdf.inc: extract VARS_LIVE_SIZE and VARS_SPARE_SIZE macros Laszlo Ersek
@ 2017-05-03 21:39 ` Laszlo Ersek
2017-05-05 4:07 ` Laszlo Ersek
2017-05-03 21:39 ` [PATCH v2 4/5] OvmfPkg: raise max variable size (auth & non-auth) to 33KB for FD_SIZE_4MB Laszlo Ersek
2017-05-03 21:39 ` [PATCH v2 5/5] OvmfPkg: make the 4MB flash size the default Laszlo Ersek
4 siblings, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2017-05-03 21:39 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Gary Ching-Pang Lin, Jordan Justen
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 4/5] OvmfPkg: raise max variable size (auth & non-auth) to 33KB for FD_SIZE_4MB
2017-05-03 21:39 [PATCH v2 0/5] OvmfPkg: add FD_SIZE_4MB for Windows HCK SB tests, and for future proofing Laszlo Ersek
` (2 preceding siblings ...)
2017-05-03 21:39 ` [PATCH v2 3/5] OvmfPkg: introduce 4MB flash image (mainly) for Windows HCK Laszlo Ersek
@ 2017-05-03 21:39 ` Laszlo Ersek
2017-05-04 16:36 ` Jordan Justen
2017-05-03 21:39 ` [PATCH v2 5/5] OvmfPkg: make the 4MB flash size the default Laszlo Ersek
4 siblings, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2017-05-03 21:39 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Gary Ching-Pang Lin, Jordan Justen
The "ConfirmSetOfLargeVariable" test case of the Secure Boot Logo Test
("Microsoft.UefiSecureBootLogo.Tests") suite in the Microsoft Hardware
Certification Kit sets a 32 KB large non-authenticated variable.
In the FD_SIZE_4MB build, our live varstore is now 256 KB big, so we can
accommodate this. Set both PcdMaxVariableSize and PcdMaxAuthVariableSize
to 0x8400 -- beyond DataSize=0x8000 from the HCK test, we need some room
for the variable name and attributes as well.
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:
- adjust to FD_SIZE_IN_KB
- update commit msg to state 256 KB for the varstore [Jordan]
OvmfPkg/OvmfPkgIa32.dsc | 6 ++++++
OvmfPkg/OvmfPkgIa32X64.dsc | 6 ++++++
OvmfPkg/OvmfPkgX64.dsc | 6 ++++++
3 files changed, 18 insertions(+)
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 26b807dde9fa..e0779ddaa426 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -404,28 +404,34 @@ [PcdsFeatureFlag]
gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
!endif
!if $(SMM_REQUIRE) == TRUE
gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
!endif
[PcdsFixedAtBuild]
gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
+!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
+!endif
+!if $(FD_SIZE_IN_KB) == 4096
+ gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
+ gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
+!endif
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
# DEBUG_INIT 0x00000001 // Initialization
# DEBUG_WARN 0x00000002 // Warnings
# DEBUG_LOAD 0x00000004 // Load events
# DEBUG_FS 0x00000008 // EFI File system
# DEBUG_POOL 0x00000010 // Alloc & Free (pool)
# DEBUG_PAGE 0x00000020 // Alloc & Free (page)
# DEBUG_INFO 0x00000040 // Informational debug messages
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 41f06a6b6a66..bbe26e2cf452 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -409,28 +409,34 @@ [PcdsFeatureFlag]
gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
!endif
!if $(SMM_REQUIRE) == TRUE
gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
!endif
[PcdsFixedAtBuild]
gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
+!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
+!endif
+!if $(FD_SIZE_IN_KB) == 4096
+ gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
+ gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
+!endif
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
# DEBUG_INIT 0x00000001 // Initialization
# DEBUG_WARN 0x00000002 // Warnings
# DEBUG_LOAD 0x00000004 // Load events
# DEBUG_FS 0x00000008 // EFI File system
# DEBUG_POOL 0x00000010 // Alloc & Free (pool)
# DEBUG_PAGE 0x00000020 // Alloc & Free (page)
# DEBUG_INFO 0x00000040 // Informational debug messages
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 053c84b685c5..ff795815f65f 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -409,28 +409,34 @@ [PcdsFeatureFlag]
gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
!endif
!if $(SMM_REQUIRE) == TRUE
gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
!endif
[PcdsFixedAtBuild]
gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
+!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
+!endif
+!if $(FD_SIZE_IN_KB) == 4096
+ gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
+ gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
+!endif
gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
# DEBUG_INIT 0x00000001 // Initialization
# DEBUG_WARN 0x00000002 // Warnings
# DEBUG_LOAD 0x00000004 // Load events
# DEBUG_FS 0x00000008 // EFI File system
# DEBUG_POOL 0x00000010 // Alloc & Free (pool)
# DEBUG_PAGE 0x00000020 // Alloc & Free (page)
# DEBUG_INFO 0x00000040 // Informational debug messages
--
2.9.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 5/5] OvmfPkg: make the 4MB flash size the default
2017-05-03 21:39 [PATCH v2 0/5] OvmfPkg: add FD_SIZE_4MB for Windows HCK SB tests, and for future proofing Laszlo Ersek
` (3 preceding siblings ...)
2017-05-03 21:39 ` [PATCH v2 4/5] OvmfPkg: raise max variable size (auth & non-auth) to 33KB for FD_SIZE_4MB Laszlo Ersek
@ 2017-05-03 21:39 ` Laszlo Ersek
4 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-05-03 21:39 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Gary Ching-Pang Lin, Jordan Justen
The previously default 2MB can be explicitly selected with
-D FD_SIZE_2MB
or
-D FD_SIZE_IN_KB=2048
Cc: Gary Ching-Pang Lin <glin@suse.com>
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>
---
Notes:
v2:
- new in v2 [Jordan]
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 e0779ddaa426..e0ff7ead034e 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -44,27 +44,27 @@ [Defines]
# 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
+ DEFINE FD_SIZE_IN_KB = 4096
!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.
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index bbe26e2cf452..41922f581c98 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -44,27 +44,27 @@ [Defines]
# 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
+ DEFINE FD_SIZE_IN_KB = 4096
!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
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index ff795815f65f..9ba033956c88 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -44,27 +44,27 @@ [Defines]
# 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
+ DEFINE FD_SIZE_IN_KB = 4096
!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
--
2.9.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 4/5] OvmfPkg: raise max variable size (auth & non-auth) to 33KB for FD_SIZE_4MB
2017-05-03 21:39 ` [PATCH v2 4/5] OvmfPkg: raise max variable size (auth & non-auth) to 33KB for FD_SIZE_4MB Laszlo Ersek
@ 2017-05-04 16:36 ` Jordan Justen
2017-05-04 16:52 ` Laszlo Ersek
0 siblings, 1 reply; 17+ messages in thread
From: Jordan Justen @ 2017-05-04 16:36 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01; +Cc: Gary Ching-Pang Lin
On 2017-05-03 14:39:46, Laszlo Ersek wrote:
> The "ConfirmSetOfLargeVariable" test case of the Secure Boot Logo Test
> ("Microsoft.UefiSecureBootLogo.Tests") suite in the Microsoft Hardware
> Certification Kit sets a 32 KB large non-authenticated variable.
According to
http://www.uefi.org/sites/default/files/resources/UEFI_Plugfest_Security_Microsoft_Fall_2016.pdf
"The maximum supported variable size must be at least 64kB"
Should we just bump the size to match this? We should be able to make
this change later once it is in a test/spec, but for some reason I
thought the requirement was already 64k.
Aside from this question:
Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
> In the FD_SIZE_4MB build, our live varstore is now 256 KB big, so we can
> accommodate this. Set both PcdMaxVariableSize and PcdMaxAuthVariableSize
> to 0x8400 -- beyond DataSize=0x8000 from the HCK test, we need some room
> for the variable name and attributes as well.
>
> 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:
> - adjust to FD_SIZE_IN_KB
> - update commit msg to state 256 KB for the varstore [Jordan]
>
> OvmfPkg/OvmfPkgIa32.dsc | 6 ++++++
> OvmfPkg/OvmfPkgIa32X64.dsc | 6 ++++++
> OvmfPkg/OvmfPkgX64.dsc | 6 ++++++
> 3 files changed, 18 insertions(+)
>
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 26b807dde9fa..e0779ddaa426 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -404,28 +404,34 @@ [PcdsFeatureFlag]
> gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
> !endif
> !if $(SMM_REQUIRE) == TRUE
> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
> !endif
>
> [PcdsFixedAtBuild]
> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
> +!endif
> +!if $(FD_SIZE_IN_KB) == 4096
> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
> +!endif
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>
> gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
>
> # DEBUG_INIT 0x00000001 // Initialization
> # DEBUG_WARN 0x00000002 // Warnings
> # DEBUG_LOAD 0x00000004 // Load events
> # DEBUG_FS 0x00000008 // EFI File system
> # DEBUG_POOL 0x00000010 // Alloc & Free (pool)
> # DEBUG_PAGE 0x00000020 // Alloc & Free (page)
> # DEBUG_INFO 0x00000040 // Informational debug messages
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index 41f06a6b6a66..bbe26e2cf452 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -409,28 +409,34 @@ [PcdsFeatureFlag]
> gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
> !endif
> !if $(SMM_REQUIRE) == TRUE
> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
> !endif
>
> [PcdsFixedAtBuild]
> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
> +!endif
> +!if $(FD_SIZE_IN_KB) == 4096
> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
> +!endif
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>
> gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
>
> # DEBUG_INIT 0x00000001 // Initialization
> # DEBUG_WARN 0x00000002 // Warnings
> # DEBUG_LOAD 0x00000004 // Load events
> # DEBUG_FS 0x00000008 // EFI File system
> # DEBUG_POOL 0x00000010 // Alloc & Free (pool)
> # DEBUG_PAGE 0x00000020 // Alloc & Free (page)
> # DEBUG_INFO 0x00000040 // Informational debug messages
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index 053c84b685c5..ff795815f65f 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -409,28 +409,34 @@ [PcdsFeatureFlag]
> gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
> !endif
> !if $(SMM_REQUIRE) == TRUE
> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
> !endif
>
> [PcdsFixedAtBuild]
> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
> +!endif
> +!if $(FD_SIZE_IN_KB) == 4096
> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
> +!endif
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>
> gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
>
> # DEBUG_INIT 0x00000001 // Initialization
> # DEBUG_WARN 0x00000002 // Warnings
> # DEBUG_LOAD 0x00000004 // Load events
> # DEBUG_FS 0x00000008 // EFI File system
> # DEBUG_POOL 0x00000010 // Alloc & Free (pool)
> # DEBUG_PAGE 0x00000020 // Alloc & Free (page)
> # DEBUG_INFO 0x00000040 // Informational debug messages
> --
> 2.9.3
>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 4/5] OvmfPkg: raise max variable size (auth & non-auth) to 33KB for FD_SIZE_4MB
2017-05-04 16:36 ` Jordan Justen
@ 2017-05-04 16:52 ` Laszlo Ersek
2017-05-04 18:50 ` Jordan Justen
2017-05-04 23:00 ` Laszlo Ersek
0 siblings, 2 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-05-04 16:52 UTC (permalink / raw)
To: Jordan Justen, edk2-devel-01; +Cc: Gary Ching-Pang Lin
On 05/04/17 18:36, Jordan Justen wrote:
> On 2017-05-03 14:39:46, Laszlo Ersek wrote:
>> The "ConfirmSetOfLargeVariable" test case of the Secure Boot Logo Test
>> ("Microsoft.UefiSecureBootLogo.Tests") suite in the Microsoft Hardware
>> Certification Kit sets a 32 KB large non-authenticated variable.
>
> According to
> http://www.uefi.org/sites/default/files/resources/UEFI_Plugfest_Security_Microsoft_Fall_2016.pdf
>
> "The maximum supported variable size must be at least 64kB"
>
> Should we just bump the size to match this? We should be able to make
> this change later once it is in a test/spec, but for some reason I
> thought the requirement was already 64k.
The 32KB requirement comes from the most recent Secure Boot Logo Test. I
installed both the Windows Server 2008 R2 SP1 test controller and the
Windows 2016 Server test client just the other day, together with the
most recent filters, using the following descriptions:
https://msdn.microsoft.com/en-us/library/windows/hardware/jj123537.aspx
https://github.com/daynix/VirtHCK/wiki#Checklist_for_a_New_Controller_VM
https://github.com/daynix/VirtHCK/wiki#Checklist_for_a_New_Client_VM
Given that this limit can be bumped without breaking compatibility, as
you say, I'd like to remain frugal with it, same as we were in James's
commit f5404a3eba1d ("OvmfPkg: Increase the maximum size for
Authenticated variables", 2016-03-24).
I don't understand why the plugfest presentation and the SB Logo Test
require different limits... But, I'm certain our QE will find out in
short order once the SB Logo Test catches up with the presentation, and
I expect I'll submit the corresponding patch soon after.
I dislike the speculation in this series, but breaking compatibility is
even worse. (A lot worse, to me at least.) So I consider the varstore
restructuring the smaller of two wrongs. However, wrt.
PcdMaxVariableSize, it seems we're not being forced to either of those
wrongs (i.e., breaking compat or speculation), so we can delay the increase.
>
> Aside from this question:
>
> Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
Thanks a lot!
I'll await your ACK for the above argument before pushing the series.
Thanks,
Laszlo
>> In the FD_SIZE_4MB build, our live varstore is now 256 KB big, so we can
>> accommodate this. Set both PcdMaxVariableSize and PcdMaxAuthVariableSize
>> to 0x8400 -- beyond DataSize=0x8000 from the HCK test, we need some room
>> for the variable name and attributes as well.
>>
>> 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:
>> - adjust to FD_SIZE_IN_KB
>> - update commit msg to state 256 KB for the varstore [Jordan]
>>
>> OvmfPkg/OvmfPkgIa32.dsc | 6 ++++++
>> OvmfPkg/OvmfPkgIa32X64.dsc | 6 ++++++
>> OvmfPkg/OvmfPkgX64.dsc | 6 ++++++
>> 3 files changed, 18 insertions(+)
>>
>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>> index 26b807dde9fa..e0779ddaa426 100644
>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>> @@ -404,28 +404,34 @@ [PcdsFeatureFlag]
>> gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
>> !endif
>> !if $(SMM_REQUIRE) == TRUE
>> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
>> !endif
>>
>> [PcdsFixedAtBuild]
>> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
>> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
>> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
>> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
>> +!endif
>> +!if $(FD_SIZE_IN_KB) == 4096
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
>> +!endif
>> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
>>
>> gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>>
>> gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
>>
>> # DEBUG_INIT 0x00000001 // Initialization
>> # DEBUG_WARN 0x00000002 // Warnings
>> # DEBUG_LOAD 0x00000004 // Load events
>> # DEBUG_FS 0x00000008 // EFI File system
>> # DEBUG_POOL 0x00000010 // Alloc & Free (pool)
>> # DEBUG_PAGE 0x00000020 // Alloc & Free (page)
>> # DEBUG_INFO 0x00000040 // Informational debug messages
>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>> index 41f06a6b6a66..bbe26e2cf452 100644
>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>> @@ -409,28 +409,34 @@ [PcdsFeatureFlag]
>> gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
>> !endif
>> !if $(SMM_REQUIRE) == TRUE
>> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
>> !endif
>>
>> [PcdsFixedAtBuild]
>> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
>> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
>> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
>> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
>> +!endif
>> +!if $(FD_SIZE_IN_KB) == 4096
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
>> +!endif
>> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
>>
>> gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>>
>> gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
>>
>> # DEBUG_INIT 0x00000001 // Initialization
>> # DEBUG_WARN 0x00000002 // Warnings
>> # DEBUG_LOAD 0x00000004 // Load events
>> # DEBUG_FS 0x00000008 // EFI File system
>> # DEBUG_POOL 0x00000010 // Alloc & Free (pool)
>> # DEBUG_PAGE 0x00000020 // Alloc & Free (page)
>> # DEBUG_INFO 0x00000040 // Informational debug messages
>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>> index 053c84b685c5..ff795815f65f 100644
>> --- a/OvmfPkg/OvmfPkgX64.dsc
>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>> @@ -409,28 +409,34 @@ [PcdsFeatureFlag]
>> gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
>> !endif
>> !if $(SMM_REQUIRE) == TRUE
>> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
>> !endif
>>
>> [PcdsFixedAtBuild]
>> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
>> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
>> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
>> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
>> +!endif
>> +!if $(FD_SIZE_IN_KB) == 4096
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
>> +!endif
>> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
>>
>> gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>>
>> gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
>>
>> # DEBUG_INIT 0x00000001 // Initialization
>> # DEBUG_WARN 0x00000002 // Warnings
>> # DEBUG_LOAD 0x00000004 // Load events
>> # DEBUG_FS 0x00000008 // EFI File system
>> # DEBUG_POOL 0x00000010 // Alloc & Free (pool)
>> # DEBUG_PAGE 0x00000020 // Alloc & Free (page)
>> # DEBUG_INFO 0x00000040 // Informational debug messages
>> --
>> 2.9.3
>>
>>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 4/5] OvmfPkg: raise max variable size (auth & non-auth) to 33KB for FD_SIZE_4MB
2017-05-04 16:52 ` Laszlo Ersek
@ 2017-05-04 18:50 ` Jordan Justen
2017-05-04 19:27 ` Laszlo Ersek
2017-05-04 23:00 ` Laszlo Ersek
1 sibling, 1 reply; 17+ messages in thread
From: Jordan Justen @ 2017-05-04 18:50 UTC (permalink / raw)
To: Laszlo Ersek, edk2-devel-01; +Cc: Gary Ching-Pang Lin
On 2017-05-04 09:52:48, Laszlo Ersek wrote:
> On 05/04/17 18:36, Jordan Justen wrote:
> > On 2017-05-03 14:39:46, Laszlo Ersek wrote:
> >> The "ConfirmSetOfLargeVariable" test case of the Secure Boot Logo Test
> >> ("Microsoft.UefiSecureBootLogo.Tests") suite in the Microsoft Hardware
> >> Certification Kit sets a 32 KB large non-authenticated variable.
> >
> > According to
> > http://www.uefi.org/sites/default/files/resources/UEFI_Plugfest_Security_Microsoft_Fall_2016.pdf
> >
> > "The maximum supported variable size must be at least 64kB"
> >
> > Should we just bump the size to match this? We should be able to make
> > this change later once it is in a test/spec, but for some reason I
> > thought the requirement was already 64k.
>
> The 32KB requirement comes from the most recent Secure Boot Logo Test. I
If the limit is 32k, why go with 33k? Does the test fail with a 32k
limit?
-Jordan
> installed both the Windows Server 2008 R2 SP1 test controller and the
> Windows 2016 Server test client just the other day, together with the
> most recent filters, using the following descriptions:
>
> https://msdn.microsoft.com/en-us/library/windows/hardware/jj123537.aspx
> https://github.com/daynix/VirtHCK/wiki#Checklist_for_a_New_Controller_VM
> https://github.com/daynix/VirtHCK/wiki#Checklist_for_a_New_Client_VM
>
> Given that this limit can be bumped without breaking compatibility, as
> you say, I'd like to remain frugal with it, same as we were in James's
> commit f5404a3eba1d ("OvmfPkg: Increase the maximum size for
> Authenticated variables", 2016-03-24).
>
> I don't understand why the plugfest presentation and the SB Logo Test
> require different limits... But, I'm certain our QE will find out in
> short order once the SB Logo Test catches up with the presentation, and
> I expect I'll submit the corresponding patch soon after.
>
> I dislike the speculation in this series, but breaking compatibility is
> even worse. (A lot worse, to me at least.) So I consider the varstore
> restructuring the smaller of two wrongs. However, wrt.
> PcdMaxVariableSize, it seems we're not being forced to either of those
> wrongs (i.e., breaking compat or speculation), so we can delay the increase.
>
> >
> > Aside from this question:
> >
> > Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
>
> Thanks a lot!
>
> I'll await your ACK for the above argument before pushing the series.
>
> Thanks,
> Laszlo
>
> >> In the FD_SIZE_4MB build, our live varstore is now 256 KB big, so we can
> >> accommodate this. Set both PcdMaxVariableSize and PcdMaxAuthVariableSize
> >> to 0x8400 -- beyond DataSize=0x8000 from the HCK test, we need some room
> >> for the variable name and attributes as well.
> >>
> >> 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:
> >> - adjust to FD_SIZE_IN_KB
> >> - update commit msg to state 256 KB for the varstore [Jordan]
> >>
> >> OvmfPkg/OvmfPkgIa32.dsc | 6 ++++++
> >> OvmfPkg/OvmfPkgIa32X64.dsc | 6 ++++++
> >> OvmfPkg/OvmfPkgX64.dsc | 6 ++++++
> >> 3 files changed, 18 insertions(+)
> >>
> >> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> >> index 26b807dde9fa..e0779ddaa426 100644
> >> --- a/OvmfPkg/OvmfPkgIa32.dsc
> >> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> >> @@ -404,28 +404,34 @@ [PcdsFeatureFlag]
> >> gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
> >> !endif
> >> !if $(SMM_REQUIRE) == TRUE
> >> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
> >> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
> >> !endif
> >>
> >> [PcdsFixedAtBuild]
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
> >> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
> >> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
> >> +!endif
> >> +!if $(FD_SIZE_IN_KB) == 4096
> >> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
> >> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
> >> +!endif
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
> >>
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
> >>
> >> gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
> >>
> >> # DEBUG_INIT 0x00000001 // Initialization
> >> # DEBUG_WARN 0x00000002 // Warnings
> >> # DEBUG_LOAD 0x00000004 // Load events
> >> # DEBUG_FS 0x00000008 // EFI File system
> >> # DEBUG_POOL 0x00000010 // Alloc & Free (pool)
> >> # DEBUG_PAGE 0x00000020 // Alloc & Free (page)
> >> # DEBUG_INFO 0x00000040 // Informational debug messages
> >> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> >> index 41f06a6b6a66..bbe26e2cf452 100644
> >> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> >> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> >> @@ -409,28 +409,34 @@ [PcdsFeatureFlag]
> >> gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
> >> !endif
> >> !if $(SMM_REQUIRE) == TRUE
> >> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
> >> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
> >> !endif
> >>
> >> [PcdsFixedAtBuild]
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
> >> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
> >> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
> >> +!endif
> >> +!if $(FD_SIZE_IN_KB) == 4096
> >> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
> >> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
> >> +!endif
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
> >>
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
> >>
> >> gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
> >>
> >> # DEBUG_INIT 0x00000001 // Initialization
> >> # DEBUG_WARN 0x00000002 // Warnings
> >> # DEBUG_LOAD 0x00000004 // Load events
> >> # DEBUG_FS 0x00000008 // EFI File system
> >> # DEBUG_POOL 0x00000010 // Alloc & Free (pool)
> >> # DEBUG_PAGE 0x00000020 // Alloc & Free (page)
> >> # DEBUG_INFO 0x00000040 // Informational debug messages
> >> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> >> index 053c84b685c5..ff795815f65f 100644
> >> --- a/OvmfPkg/OvmfPkgX64.dsc
> >> +++ b/OvmfPkg/OvmfPkgX64.dsc
> >> @@ -409,28 +409,34 @@ [PcdsFeatureFlag]
> >> gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
> >> !endif
> >> !if $(SMM_REQUIRE) == TRUE
> >> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
> >> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
> >> !endif
> >>
> >> [PcdsFixedAtBuild]
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
> >> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
> >> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
> >> +!endif
> >> +!if $(FD_SIZE_IN_KB) == 4096
> >> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
> >> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
> >> +!endif
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
> >>
> >> gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
> >>
> >> gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
> >>
> >> # DEBUG_INIT 0x00000001 // Initialization
> >> # DEBUG_WARN 0x00000002 // Warnings
> >> # DEBUG_LOAD 0x00000004 // Load events
> >> # DEBUG_FS 0x00000008 // EFI File system
> >> # DEBUG_POOL 0x00000010 // Alloc & Free (pool)
> >> # DEBUG_PAGE 0x00000020 // Alloc & Free (page)
> >> # DEBUG_INFO 0x00000040 // Informational debug messages
> >> --
> >> 2.9.3
> >>
> >>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 4/5] OvmfPkg: raise max variable size (auth & non-auth) to 33KB for FD_SIZE_4MB
2017-05-04 18:50 ` Jordan Justen
@ 2017-05-04 19:27 ` Laszlo Ersek
2017-05-04 19:30 ` Laszlo Ersek
0 siblings, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2017-05-04 19:27 UTC (permalink / raw)
To: Jordan Justen, edk2-devel-01; +Cc: Gary Ching-Pang Lin
On 05/04/17 20:50, Jordan Justen wrote:
> On 2017-05-04 09:52:48, Laszlo Ersek wrote:
>> On 05/04/17 18:36, Jordan Justen wrote:
>>> On 2017-05-03 14:39:46, Laszlo Ersek wrote:
>>>> The "ConfirmSetOfLargeVariable" test case of the Secure Boot Logo Test
>>>> ("Microsoft.UefiSecureBootLogo.Tests") suite in the Microsoft Hardware
>>>> Certification Kit sets a 32 KB large non-authenticated variable.
>>>
>>> According to
>>> http://www.uefi.org/sites/default/files/resources/UEFI_Plugfest_Security_Microsoft_Fall_2016.pdf
>>>
>>> "The maximum supported variable size must be at least 64kB"
>>>
>>> Should we just bump the size to match this? We should be able to make
>>> this change later once it is in a test/spec, but for some reason I
>>> thought the requirement was already 64k.
>>
>> The 32KB requirement comes from the most recent Secure Boot Logo Test. I
>
> If the limit is 32k, why go with 33k? Does the test fail with a 32k
> limit?
Yes, it does. First I tried setting the PCDs to 0x8000 sharp, and then
the test failed.
Then I remembered that the PCDs present a limit on data, name, and
attributes together:
(1) In "MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c", function
InitNonVolatileVariableStore(), we stash the PCDs like this:
> mVariableModuleGlobal->MaxVariableSize = PcdGet32 (PcdMaxVariableSize);
> mVariableModuleGlobal->MaxAuthVariableSize = ((PcdGet32 (PcdMaxAuthVariableSize) != 0) ? PcdGet32 (PcdMaxAuthVariableSize) : mVariableModuleGlobal->MaxVariableSize);
(2) In the same file, function VariableServiceSetVariable(), we have the
following size checks:
> //
> // The size of the VariableName, including the Unicode Null in bytes plus
> // the DataSize is limited to maximum size of PcdGet32 (PcdMaxHardwareErrorVariableSize)
> // bytes for HwErrRec#### variable.
> //
> if ((Attributes & EFI_VARIABLE_HARDWARE_ERROR_RECORD) == EFI_VARIABLE_HARDWARE_ERROR_RECORD) {
> if (StrSize (VariableName) + PayloadSize > PcdGet32 (PcdMaxHardwareErrorVariableSize) - GetVariableHeaderSize ()) {
> return EFI_INVALID_PARAMETER;
> }
> } else {
> //
> // The size of the VariableName, including the Unicode Null in bytes plus
> // the DataSize is limited to maximum size of Max(Auth)VariableSize bytes.
> //
> if ((Attributes & VARIABLE_ATTRIBUTE_AT_AW) != 0) {
> if (StrSize (VariableName) + PayloadSize > mVariableModuleGlobal->MaxAuthVariableSize - GetVariableHeaderSize ()) {
> return EFI_INVALID_PARAMETER;
> }
> } else {
> if (StrSize (VariableName) + PayloadSize > mVariableModuleGlobal->MaxVariableSize - GetVariableHeaderSize ()) {
> return EFI_INVALID_PARAMETER;
> }
> }
> }
That is, the PCDs limit the sum of:
- the variable name (including the terminating L'\0'),
- the variable contents,
- the internal header (which carries the attributes as well). In our
case that's AUTHENTICATED_VARIABLE_HEADER, because we always format
the auth varstore GUID into the template.
Furthermore, the 32KB data size for the contents is not based on
"reverse engineering"; the Secure Boot Logo Test tells you exactly what
it is trying to do. (In fact, there is no other way to fix failures in
any of the tests; you can only rely on the messages in order to see what
the test attempted.)
In this case, we have (see
<https://bugzilla.redhat.com/show_bug.cgi?id=1443351#c27>):
> NOTE: if this testcase fails, try restarting the machine and then
> running this test case on its own, for example: "te.exe
> UefiSecureBootLogo.dll
> /name:Microsoft.UefiSecureBootLogo.Tests.ConfirmSetOfLargeVariable"
>
> Test creation of 1 large variable of size 32768 bytes.
>
> Test creation of testLargeVariable Unexpected Error - SetVariable
> failed with status 0xC000000D VariableName "testLargeVariable"
> VendorGuid 77fa9abd-0359-4d32-bd60-28f4e78f784b Attributes 0x00000007
> DataSize 0x00008000
It reports all the parameters of the gRT->SetVariable() call that
failed, except "Data".
In the successful test (with this patch applied), the messages are (see
<https://bugzilla.redhat.com/attachment.cgi?id=1275172>):
> NOTE: if this testcase fails, try restarting the machine and then
> running this test case on its own, for example: "te.exe
> UefiSecureBootLogo.dll
> /name:Microsoft.UefiSecureBootLogo.Tests.ConfirmSetOfLargeVariable"
>
> Test creation of 1 large variable of size 32768 bytes.
>
> AreEqual(7, 7) - testLargeVariable: Attributes from GetVariable()
> match SetVariable()
>
> AreEqual(32768, 32768) - testLargeVariable: DataSize from
> GetVariable() matches SetVariable()
Thanks
Laszlo
>
>> installed both the Windows Server 2008 R2 SP1 test controller and the
>> Windows 2016 Server test client just the other day, together with the
>> most recent filters, using the following descriptions:
>>
>> https://msdn.microsoft.com/en-us/library/windows/hardware/jj123537.aspx
>> https://github.com/daynix/VirtHCK/wiki#Checklist_for_a_New_Controller_VM
>> https://github.com/daynix/VirtHCK/wiki#Checklist_for_a_New_Client_VM
>>
>> Given that this limit can be bumped without breaking compatibility, as
>> you say, I'd like to remain frugal with it, same as we were in James's
>> commit f5404a3eba1d ("OvmfPkg: Increase the maximum size for
>> Authenticated variables", 2016-03-24).
>>
>> I don't understand why the plugfest presentation and the SB Logo Test
>> require different limits... But, I'm certain our QE will find out in
>> short order once the SB Logo Test catches up with the presentation, and
>> I expect I'll submit the corresponding patch soon after.
>>
>> I dislike the speculation in this series, but breaking compatibility is
>> even worse. (A lot worse, to me at least.) So I consider the varstore
>> restructuring the smaller of two wrongs. However, wrt.
>> PcdMaxVariableSize, it seems we're not being forced to either of those
>> wrongs (i.e., breaking compat or speculation), so we can delay the increase.
>>
>>>
>>> Aside from this question:
>>>
>>> Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
>>
>> Thanks a lot!
>>
>> I'll await your ACK for the above argument before pushing the series.
>>
>> Thanks,
>> Laszlo
>>
>>>> In the FD_SIZE_4MB build, our live varstore is now 256 KB big, so we can
>>>> accommodate this. Set both PcdMaxVariableSize and PcdMaxAuthVariableSize
>>>> to 0x8400 -- beyond DataSize=0x8000 from the HCK test, we need some room
>>>> for the variable name and attributes as well.
>>>>
>>>> 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:
>>>> - adjust to FD_SIZE_IN_KB
>>>> - update commit msg to state 256 KB for the varstore [Jordan]
>>>>
>>>> OvmfPkg/OvmfPkgIa32.dsc | 6 ++++++
>>>> OvmfPkg/OvmfPkgIa32X64.dsc | 6 ++++++
>>>> OvmfPkg/OvmfPkgX64.dsc | 6 ++++++
>>>> 3 files changed, 18 insertions(+)
>>>>
>>>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>>>> index 26b807dde9fa..e0779ddaa426 100644
>>>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>>>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>>>> @@ -404,28 +404,34 @@ [PcdsFeatureFlag]
>>>> gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
>>>> !endif
>>>> !if $(SMM_REQUIRE) == TRUE
>>>> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
>>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
>>>> !endif
>>>>
>>>> [PcdsFixedAtBuild]
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
>>>> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
>>>> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
>>>> +!endif
>>>> +!if $(FD_SIZE_IN_KB) == 4096
>>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
>>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
>>>> +!endif
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
>>>>
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>>>>
>>>> gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
>>>>
>>>> # DEBUG_INIT 0x00000001 // Initialization
>>>> # DEBUG_WARN 0x00000002 // Warnings
>>>> # DEBUG_LOAD 0x00000004 // Load events
>>>> # DEBUG_FS 0x00000008 // EFI File system
>>>> # DEBUG_POOL 0x00000010 // Alloc & Free (pool)
>>>> # DEBUG_PAGE 0x00000020 // Alloc & Free (page)
>>>> # DEBUG_INFO 0x00000040 // Informational debug messages
>>>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>>>> index 41f06a6b6a66..bbe26e2cf452 100644
>>>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>>>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>>>> @@ -409,28 +409,34 @@ [PcdsFeatureFlag]
>>>> gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
>>>> !endif
>>>> !if $(SMM_REQUIRE) == TRUE
>>>> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
>>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
>>>> !endif
>>>>
>>>> [PcdsFixedAtBuild]
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
>>>> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
>>>> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
>>>> +!endif
>>>> +!if $(FD_SIZE_IN_KB) == 4096
>>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
>>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
>>>> +!endif
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
>>>>
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>>>>
>>>> gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
>>>>
>>>> # DEBUG_INIT 0x00000001 // Initialization
>>>> # DEBUG_WARN 0x00000002 // Warnings
>>>> # DEBUG_LOAD 0x00000004 // Load events
>>>> # DEBUG_FS 0x00000008 // EFI File system
>>>> # DEBUG_POOL 0x00000010 // Alloc & Free (pool)
>>>> # DEBUG_PAGE 0x00000020 // Alloc & Free (page)
>>>> # DEBUG_INFO 0x00000040 // Informational debug messages
>>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>>>> index 053c84b685c5..ff795815f65f 100644
>>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>>> @@ -409,28 +409,34 @@ [PcdsFeatureFlag]
>>>> gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
>>>> !endif
>>>> !if $(SMM_REQUIRE) == TRUE
>>>> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
>>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
>>>> !endif
>>>>
>>>> [PcdsFixedAtBuild]
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
>>>> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
>>>> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
>>>> +!endif
>>>> +!if $(FD_SIZE_IN_KB) == 4096
>>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
>>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
>>>> +!endif
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
>>>>
>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>>>>
>>>> gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
>>>>
>>>> # DEBUG_INIT 0x00000001 // Initialization
>>>> # DEBUG_WARN 0x00000002 // Warnings
>>>> # DEBUG_LOAD 0x00000004 // Load events
>>>> # DEBUG_FS 0x00000008 // EFI File system
>>>> # DEBUG_POOL 0x00000010 // Alloc & Free (pool)
>>>> # DEBUG_PAGE 0x00000020 // Alloc & Free (page)
>>>> # DEBUG_INFO 0x00000040 // Informational debug messages
>>>> --
>>>> 2.9.3
>>>>
>>>>
>>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 4/5] OvmfPkg: raise max variable size (auth & non-auth) to 33KB for FD_SIZE_4MB
2017-05-04 19:27 ` Laszlo Ersek
@ 2017-05-04 19:30 ` Laszlo Ersek
0 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-05-04 19:30 UTC (permalink / raw)
To: Jordan Justen, edk2-devel-01; +Cc: Gary Ching-Pang Lin
On 05/04/17 21:27, Laszlo Ersek wrote:
> On 05/04/17 20:50, Jordan Justen wrote:
>> On 2017-05-04 09:52:48, Laszlo Ersek wrote:
>>> On 05/04/17 18:36, Jordan Justen wrote:
>>>> On 2017-05-03 14:39:46, Laszlo Ersek wrote:
>>>>> The "ConfirmSetOfLargeVariable" test case of the Secure Boot Logo Test
>>>>> ("Microsoft.UefiSecureBootLogo.Tests") suite in the Microsoft Hardware
>>>>> Certification Kit sets a 32 KB large non-authenticated variable.
>>>>
>>>> According to
>>>> http://www.uefi.org/sites/default/files/resources/UEFI_Plugfest_Security_Microsoft_Fall_2016.pdf
>>>>
>>>> "The maximum supported variable size must be at least 64kB"
>>>>
>>>> Should we just bump the size to match this? We should be able to make
>>>> this change later once it is in a test/spec, but for some reason I
>>>> thought the requirement was already 64k.
>>>
>>> The 32KB requirement comes from the most recent Secure Boot Logo Test. I
>>
>> If the limit is 32k, why go with 33k? Does the test fail with a 32k
>> limit?
>
> Yes, it does. First I tried setting the PCDs to 0x8000 sharp, and then
> the test failed.
>
> Then I remembered that the PCDs present a limit on data, name, and
> attributes together:
>
> (1) In "MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c", function
> InitNonVolatileVariableStore(), we stash the PCDs like this:
>
>> mVariableModuleGlobal->MaxVariableSize = PcdGet32 (PcdMaxVariableSize);
>> mVariableModuleGlobal->MaxAuthVariableSize = ((PcdGet32 (PcdMaxAuthVariableSize) != 0) ? PcdGet32 (PcdMaxAuthVariableSize) : mVariableModuleGlobal->MaxVariableSize);
>
> (2) In the same file, function VariableServiceSetVariable(), we have the
> following size checks:
>
>> //
>> // The size of the VariableName, including the Unicode Null in bytes plus
>> // the DataSize is limited to maximum size of PcdGet32 (PcdMaxHardwareErrorVariableSize)
>> // bytes for HwErrRec#### variable.
>> //
>> if ((Attributes & EFI_VARIABLE_HARDWARE_ERROR_RECORD) == EFI_VARIABLE_HARDWARE_ERROR_RECORD) {
>> if (StrSize (VariableName) + PayloadSize > PcdGet32 (PcdMaxHardwareErrorVariableSize) - GetVariableHeaderSize ()) {
>> return EFI_INVALID_PARAMETER;
>> }
>> } else {
>> //
>> // The size of the VariableName, including the Unicode Null in bytes plus
>> // the DataSize is limited to maximum size of Max(Auth)VariableSize bytes.
>> //
>> if ((Attributes & VARIABLE_ATTRIBUTE_AT_AW) != 0) {
>> if (StrSize (VariableName) + PayloadSize > mVariableModuleGlobal->MaxAuthVariableSize - GetVariableHeaderSize ()) {
>> return EFI_INVALID_PARAMETER;
>> }
>> } else {
>> if (StrSize (VariableName) + PayloadSize > mVariableModuleGlobal->MaxVariableSize - GetVariableHeaderSize ()) {
>> return EFI_INVALID_PARAMETER;
>> }
>> }
>> }
>
> That is, the PCDs limit the sum of:
> - the variable name (including the terminating L'\0'),
> - the variable contents,
> - the internal header (which carries the attributes as well). In our
> case that's AUTHENTICATED_VARIABLE_HEADER, because we always format
> the auth varstore GUID into the template.
(NB I mentioned this in the commit message, just not in such detail.)
Thanks
Laszlo
>
> Furthermore, the 32KB data size for the contents is not based on
> "reverse engineering"; the Secure Boot Logo Test tells you exactly what
> it is trying to do. (In fact, there is no other way to fix failures in
> any of the tests; you can only rely on the messages in order to see what
> the test attempted.)
>
> In this case, we have (see
> <https://bugzilla.redhat.com/show_bug.cgi?id=1443351#c27>):
>
>> NOTE: if this testcase fails, try restarting the machine and then
>> running this test case on its own, for example: "te.exe
>> UefiSecureBootLogo.dll
>> /name:Microsoft.UefiSecureBootLogo.Tests.ConfirmSetOfLargeVariable"
>>
>> Test creation of 1 large variable of size 32768 bytes.
>>
>> Test creation of testLargeVariable Unexpected Error - SetVariable
>> failed with status 0xC000000D VariableName "testLargeVariable"
>> VendorGuid 77fa9abd-0359-4d32-bd60-28f4e78f784b Attributes 0x00000007
>> DataSize 0x00008000
>
> It reports all the parameters of the gRT->SetVariable() call that
> failed, except "Data".
>
> In the successful test (with this patch applied), the messages are (see
> <https://bugzilla.redhat.com/attachment.cgi?id=1275172>):
>
>> NOTE: if this testcase fails, try restarting the machine and then
>> running this test case on its own, for example: "te.exe
>> UefiSecureBootLogo.dll
>> /name:Microsoft.UefiSecureBootLogo.Tests.ConfirmSetOfLargeVariable"
>>
>> Test creation of 1 large variable of size 32768 bytes.
>>
>> AreEqual(7, 7) - testLargeVariable: Attributes from GetVariable()
>> match SetVariable()
>>
>> AreEqual(32768, 32768) - testLargeVariable: DataSize from
>> GetVariable() matches SetVariable()
>
> Thanks
> Laszlo
>
>>
>>> installed both the Windows Server 2008 R2 SP1 test controller and the
>>> Windows 2016 Server test client just the other day, together with the
>>> most recent filters, using the following descriptions:
>>>
>>> https://msdn.microsoft.com/en-us/library/windows/hardware/jj123537.aspx
>>> https://github.com/daynix/VirtHCK/wiki#Checklist_for_a_New_Controller_VM
>>> https://github.com/daynix/VirtHCK/wiki#Checklist_for_a_New_Client_VM
>>>
>>> Given that this limit can be bumped without breaking compatibility, as
>>> you say, I'd like to remain frugal with it, same as we were in James's
>>> commit f5404a3eba1d ("OvmfPkg: Increase the maximum size for
>>> Authenticated variables", 2016-03-24).
>>>
>>> I don't understand why the plugfest presentation and the SB Logo Test
>>> require different limits... But, I'm certain our QE will find out in
>>> short order once the SB Logo Test catches up with the presentation, and
>>> I expect I'll submit the corresponding patch soon after.
>>>
>>> I dislike the speculation in this series, but breaking compatibility is
>>> even worse. (A lot worse, to me at least.) So I consider the varstore
>>> restructuring the smaller of two wrongs. However, wrt.
>>> PcdMaxVariableSize, it seems we're not being forced to either of those
>>> wrongs (i.e., breaking compat or speculation), so we can delay the increase.
>>>
>>>>
>>>> Aside from this question:
>>>>
>>>> Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
>>>
>>> Thanks a lot!
>>>
>>> I'll await your ACK for the above argument before pushing the series.
>>>
>>> Thanks,
>>> Laszlo
>>>
>>>>> In the FD_SIZE_4MB build, our live varstore is now 256 KB big, so we can
>>>>> accommodate this. Set both PcdMaxVariableSize and PcdMaxAuthVariableSize
>>>>> to 0x8400 -- beyond DataSize=0x8000 from the HCK test, we need some room
>>>>> for the variable name and attributes as well.
>>>>>
>>>>> 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:
>>>>> - adjust to FD_SIZE_IN_KB
>>>>> - update commit msg to state 256 KB for the varstore [Jordan]
>>>>>
>>>>> OvmfPkg/OvmfPkgIa32.dsc | 6 ++++++
>>>>> OvmfPkg/OvmfPkgIa32X64.dsc | 6 ++++++
>>>>> OvmfPkg/OvmfPkgX64.dsc | 6 ++++++
>>>>> 3 files changed, 18 insertions(+)
>>>>>
>>>>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>>>>> index 26b807dde9fa..e0779ddaa426 100644
>>>>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>>>>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>>>>> @@ -404,28 +404,34 @@ [PcdsFeatureFlag]
>>>>> gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
>>>>> !endif
>>>>> !if $(SMM_REQUIRE) == TRUE
>>>>> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
>>>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
>>>>> !endif
>>>>>
>>>>> [PcdsFixedAtBuild]
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
>>>>> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
>>>>> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
>>>>> +!endif
>>>>> +!if $(FD_SIZE_IN_KB) == 4096
>>>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
>>>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
>>>>> +!endif
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
>>>>>
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>>>>>
>>>>> gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
>>>>>
>>>>> # DEBUG_INIT 0x00000001 // Initialization
>>>>> # DEBUG_WARN 0x00000002 // Warnings
>>>>> # DEBUG_LOAD 0x00000004 // Load events
>>>>> # DEBUG_FS 0x00000008 // EFI File system
>>>>> # DEBUG_POOL 0x00000010 // Alloc & Free (pool)
>>>>> # DEBUG_PAGE 0x00000020 // Alloc & Free (page)
>>>>> # DEBUG_INFO 0x00000040 // Informational debug messages
>>>>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>>>>> index 41f06a6b6a66..bbe26e2cf452 100644
>>>>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>>>>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>>>>> @@ -409,28 +409,34 @@ [PcdsFeatureFlag]
>>>>> gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
>>>>> !endif
>>>>> !if $(SMM_REQUIRE) == TRUE
>>>>> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
>>>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
>>>>> !endif
>>>>>
>>>>> [PcdsFixedAtBuild]
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
>>>>> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
>>>>> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
>>>>> +!endif
>>>>> +!if $(FD_SIZE_IN_KB) == 4096
>>>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
>>>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
>>>>> +!endif
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
>>>>>
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>>>>>
>>>>> gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
>>>>>
>>>>> # DEBUG_INIT 0x00000001 // Initialization
>>>>> # DEBUG_WARN 0x00000002 // Warnings
>>>>> # DEBUG_LOAD 0x00000004 // Load events
>>>>> # DEBUG_FS 0x00000008 // EFI File system
>>>>> # DEBUG_POOL 0x00000010 // Alloc & Free (pool)
>>>>> # DEBUG_PAGE 0x00000020 // Alloc & Free (page)
>>>>> # DEBUG_INFO 0x00000040 // Informational debug messages
>>>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>>>>> index 053c84b685c5..ff795815f65f 100644
>>>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>>>> @@ -409,28 +409,34 @@ [PcdsFeatureFlag]
>>>>> gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
>>>>> !endif
>>>>> !if $(SMM_REQUIRE) == TRUE
>>>>> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
>>>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
>>>>> !endif
>>>>>
>>>>> [PcdsFixedAtBuild]
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
>>>>> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
>>>>> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
>>>>> +!endif
>>>>> +!if $(FD_SIZE_IN_KB) == 4096
>>>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
>>>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
>>>>> +!endif
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
>>>>>
>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>>>>>
>>>>> gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
>>>>>
>>>>> # DEBUG_INIT 0x00000001 // Initialization
>>>>> # DEBUG_WARN 0x00000002 // Warnings
>>>>> # DEBUG_LOAD 0x00000004 // Load events
>>>>> # DEBUG_FS 0x00000008 // EFI File system
>>>>> # DEBUG_POOL 0x00000010 // Alloc & Free (pool)
>>>>> # DEBUG_PAGE 0x00000020 // Alloc & Free (page)
>>>>> # DEBUG_INFO 0x00000040 // Informational debug messages
>>>>> --
>>>>> 2.9.3
>>>>>
>>>>>
>>>
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 4/5] OvmfPkg: raise max variable size (auth & non-auth) to 33KB for FD_SIZE_4MB
2017-05-04 16:52 ` Laszlo Ersek
2017-05-04 18:50 ` Jordan Justen
@ 2017-05-04 23:00 ` Laszlo Ersek
1 sibling, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-05-04 23:00 UTC (permalink / raw)
To: edk2-devel-01; +Cc: Jordan Justen, Gary Ching-Pang Lin
On 05/04/17 18:52, Laszlo Ersek wrote:
> On 05/04/17 18:36, Jordan Justen wrote:
>> On 2017-05-03 14:39:46, Laszlo Ersek wrote:
>>> The "ConfirmSetOfLargeVariable" test case of the Secure Boot Logo Test
>>> ("Microsoft.UefiSecureBootLogo.Tests") suite in the Microsoft Hardware
>>> Certification Kit sets a 32 KB large non-authenticated variable.
>>
>> According to
>> http://www.uefi.org/sites/default/files/resources/UEFI_Plugfest_Security_Microsoft_Fall_2016.pdf
>>
>> "The maximum supported variable size must be at least 64kB"
>>
>> Should we just bump the size to match this? We should be able to make
>> this change later once it is in a test/spec, but for some reason I
>> thought the requirement was already 64k.
>
> The 32KB requirement comes from the most recent Secure Boot Logo Test. I
> installed both the Windows Server 2008 R2 SP1 test controller and the
> Windows 2016 Server test client just the other day, together with the
> most recent filters, using the following descriptions:
>
> https://msdn.microsoft.com/en-us/library/windows/hardware/jj123537.aspx
> https://github.com/daynix/VirtHCK/wiki#Checklist_for_a_New_Controller_VM
> https://github.com/daynix/VirtHCK/wiki#Checklist_for_a_New_Client_VM
>
> Given that this limit can be bumped without breaking compatibility, as
> you say, I'd like to remain frugal with it, same as we were in James's
> commit f5404a3eba1d ("OvmfPkg: Increase the maximum size for
> Authenticated variables", 2016-03-24).
>
> I don't understand why the plugfest presentation and the SB Logo Test
> require different limits... But, I'm certain our QE will find out in
> short order once the SB Logo Test catches up with the presentation, and
> I expect I'll submit the corresponding patch soon after.
>
> I dislike the speculation in this series, but breaking compatibility is
> even worse. (A lot worse, to me at least.) So I consider the varstore
> restructuring the smaller of two wrongs. However, wrt.
> PcdMaxVariableSize, it seems we're not being forced to either of those
> wrongs (i.e., breaking compat or speculation), so we can delay the increase.
>
>>
>> Aside from this question:
>>
>> Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>
>
> Thanks a lot!
>
> I'll await your ACK for the above argument before pushing the series.
Based on the ack I got from Jordan on IRC, I pushed the series. Commit
range 4bf3b994e8b2..bba8dfbec3bb.
Thanks!
Laszlo
>>> In the FD_SIZE_4MB build, our live varstore is now 256 KB big, so we can
>>> accommodate this. Set both PcdMaxVariableSize and PcdMaxAuthVariableSize
>>> to 0x8400 -- beyond DataSize=0x8000 from the HCK test, we need some room
>>> for the variable name and attributes as well.
>>>
>>> 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:
>>> - adjust to FD_SIZE_IN_KB
>>> - update commit msg to state 256 KB for the varstore [Jordan]
>>>
>>> OvmfPkg/OvmfPkgIa32.dsc | 6 ++++++
>>> OvmfPkg/OvmfPkgIa32X64.dsc | 6 ++++++
>>> OvmfPkg/OvmfPkgX64.dsc | 6 ++++++
>>> 3 files changed, 18 insertions(+)
>>>
>>> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
>>> index 26b807dde9fa..e0779ddaa426 100644
>>> --- a/OvmfPkg/OvmfPkgIa32.dsc
>>> +++ b/OvmfPkg/OvmfPkgIa32.dsc
>>> @@ -404,28 +404,34 @@ [PcdsFeatureFlag]
>>> gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
>>> !endif
>>> !if $(SMM_REQUIRE) == TRUE
>>> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
>>> !endif
>>>
>>> [PcdsFixedAtBuild]
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
>>> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
>>> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
>>> +!endif
>>> +!if $(FD_SIZE_IN_KB) == 4096
>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
>>> +!endif
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
>>>
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>>>
>>> gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
>>>
>>> # DEBUG_INIT 0x00000001 // Initialization
>>> # DEBUG_WARN 0x00000002 // Warnings
>>> # DEBUG_LOAD 0x00000004 // Load events
>>> # DEBUG_FS 0x00000008 // EFI File system
>>> # DEBUG_POOL 0x00000010 // Alloc & Free (pool)
>>> # DEBUG_PAGE 0x00000020 // Alloc & Free (page)
>>> # DEBUG_INFO 0x00000040 // Informational debug messages
>>> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
>>> index 41f06a6b6a66..bbe26e2cf452 100644
>>> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
>>> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
>>> @@ -409,28 +409,34 @@ [PcdsFeatureFlag]
>>> gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
>>> !endif
>>> !if $(SMM_REQUIRE) == TRUE
>>> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
>>> !endif
>>>
>>> [PcdsFixedAtBuild]
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
>>> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
>>> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
>>> +!endif
>>> +!if $(FD_SIZE_IN_KB) == 4096
>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
>>> +!endif
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
>>>
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>>>
>>> gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
>>>
>>> # DEBUG_INIT 0x00000001 // Initialization
>>> # DEBUG_WARN 0x00000002 // Warnings
>>> # DEBUG_LOAD 0x00000004 // Load events
>>> # DEBUG_FS 0x00000008 // EFI File system
>>> # DEBUG_POOL 0x00000010 // Alloc & Free (pool)
>>> # DEBUG_PAGE 0x00000020 // Alloc & Free (page)
>>> # DEBUG_INFO 0x00000040 // Informational debug messages
>>> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
>>> index 053c84b685c5..ff795815f65f 100644
>>> --- a/OvmfPkg/OvmfPkgX64.dsc
>>> +++ b/OvmfPkg/OvmfPkgX64.dsc
>>> @@ -409,28 +409,34 @@ [PcdsFeatureFlag]
>>> gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootEnable|TRUE
>>> !endif
>>> !if $(SMM_REQUIRE) == TRUE
>>> gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire|TRUE
>>> gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmEnableBspElection|FALSE
>>> !endif
>>>
>>> [PcdsFixedAtBuild]
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize|1
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE
>>> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
>>> +!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
>>> +!endif
>>> +!if $(FD_SIZE_IN_KB) == 4096
>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
>>> + gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
>>> +!endif
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
>>>
>>> gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
>>>
>>> gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x07
>>>
>>> # DEBUG_INIT 0x00000001 // Initialization
>>> # DEBUG_WARN 0x00000002 // Warnings
>>> # DEBUG_LOAD 0x00000004 // Load events
>>> # DEBUG_FS 0x00000008 // EFI File system
>>> # DEBUG_POOL 0x00000010 // Alloc & Free (pool)
>>> # DEBUG_PAGE 0x00000020 // Alloc & Free (page)
>>> # DEBUG_INFO 0x00000040 // Informational debug messages
>>> --
>>> 2.9.3
>>>
>>>
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/5] OvmfPkg: introduce 4MB flash image (mainly) for Windows HCK
2017-05-03 21:39 ` [PATCH v2 3/5] OvmfPkg: introduce 4MB flash image (mainly) for Windows HCK Laszlo Ersek
@ 2017-05-05 4:07 ` Laszlo Ersek
2017-05-05 8:57 ` Jordan Justen
0 siblings, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2017-05-05 4:07 UTC (permalink / raw)
To: Jordan Justen; +Cc: edk2-devel-01, Gary Ching-Pang Lin
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
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/5] OvmfPkg: introduce 4MB flash image (mainly) for Windows HCK
2017-05-05 4:07 ` Laszlo Ersek
@ 2017-05-05 8:57 ` Jordan Justen
2017-05-05 12:07 ` Laszlo Ersek
0 siblings, 1 reply; 17+ messages in thread
From: Jordan Justen @ 2017-05-05 8:57 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Gary Ching-Pang Lin
[-- Attachment #1: Type: text/plain, Size: 1093 bytes --]
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
[-- Attachment #2: 0001-PlatformPei-Force-EmuVariableNvStore-alignment-size-.patch --]
[-- Type: text/x-diff, Size: 1847 bytes --]
>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),
+ 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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/5] OvmfPkg: introduce 4MB flash image (mainly) for Windows HCK
2017-05-05 8:57 ` Jordan Justen
@ 2017-05-05 12:07 ` Laszlo Ersek
2017-05-05 15:43 ` Jordan Justen
0 siblings, 1 reply; 17+ messages in thread
From: Laszlo Ersek @ 2017-05-05 12:07 UTC (permalink / raw)
To: Jordan Justen; +Cc: edk2-devel-01, Gary Ching-Pang Lin
[-- Attachment #1: Type: text/plain, Size: 5024 bytes --]
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
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-OvmfPkg-PlatformPei-handle-non-power-of-two-spare-si.patch --]
[-- Type: text/x-patch; name="0001-OvmfPkg-PlatformPei-handle-non-power-of-two-spare-si.patch", Size: 2833 bytes --]
From 7bd8738556abb86bdbfdd11dd30ad78663925c09 Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Fri, 5 May 2017 02:35:21 +0200
Subject: [PATCH] OvmfPkg/PlatformPei: handle non-power-of-two spare size for
emu variables
In commit b24fca05751f ("OvmfPkg: introduce 4MB flash image (mainly) for
Windows HCK", 2017-04-29), I changed PcdFlashNvStorageFtwSpareSize to
264KB, in the (new default) 4MB build.
While PcdFlashNvStorageFtwSpareSize remains exactly half of the entire
non-volatile store (which is 528KB), 264KB isn't itself a power of two.
This triggers an assertion failure in AllocateAlignedRuntimePages() when
PlatformPei calls it from the ReserveEmuVariableNvStore() function,
passing PcdFlashNvStorageFtwSpareSize as the Alignment parameter:
> ASSERT MdePkg/Library/PeiMemoryAllocationLib/MemoryAllocationLib.c(196):
> (Alignment & (Alignment - 1)) == 0
Round up the alignment to the next power of two if necessary.
Cc: Jordan Justen <jordan.l.justen@intel.com>
Fixes: b24fca05751f8222acf264853709012e0ab7bf49
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/PlatformPei/Platform.c | 17 ++++++++++++++---
1 file changed, 14 insertions(+), 3 deletions(-)
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 77a8a16c15b8..5e983a8dcea9 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -504,6 +504,7 @@ ReserveEmuVariableNvStore (
{
EFI_PHYSICAL_ADDRESS VariableStore;
RETURN_STATUS PcdStatus;
+ UINT32 Alignment;
//
// Allocate storage for NV variables early on so it will be
@@ -511,16 +512,26 @@ 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)),
- PcdGet32 (PcdFlashNvStorageFtwSpareSize)
+ Alignment
);
DEBUG ((EFI_D_INFO,
- "Reserved variable store memory: 0x%lX; size: %dkb\n",
+ "Reserved variable store memory: 0x%lX; size: %dkb, "
+ "alignment: 0x%x\n",
VariableStore,
- (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)) / 1024
+ (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)) / 1024,
+ Alignment
));
PcdStatus = PcdSet64S (PcdEmuVariableNvStoreReserved, VariableStore);
ASSERT_RETURN_ERROR (PcdStatus);
--
2.9.3
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-OvmfPkg-sync-PcdVariableStoreSize-with-PcdFlashNvSto.patch --]
[-- Type: text/x-patch; name="0001-OvmfPkg-sync-PcdVariableStoreSize-with-PcdFlashNvSto.patch", Size: 4236 bytes --]
From 2d6500afb5870e577bab93ef9f54cab9e930703e Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Fri, 5 May 2017 03:31:32 +0200
Subject: [PATCH 1/2] OvmfPkg: sync PcdVariableStoreSize with
PcdFlashNvStorageVariableSize
"MdeModulePkg/MdeModulePkg.dec" declares PcdVariableStoreSize like this:
> The size of volatile buffer. This buffer is used to store VOLATILE
> attribute variables.
There is no inherent reason why the size of the volatile variable store
should match the same of the non-volatile variable store. Indeed flash
variables in the 4MB build work fine without this equality.
However, OvmfPkg/EmuVariableFvbRuntimeDxe uses PcdVariableStoreSize to
initialize the non-volatile VARIABLE_STORE_HEADER too. (Presumably based
on the fact that ultimately that storage will not be permanent.) When
using EmuVariableFvbRuntimeDxe in the 4MB build, the mismatch between the
two mentioned PCDs (which is apparent through EmuVariableFvbRuntimeDxe's
VARIABLE_STORE_HEADER) triggers an assertion in the variable driver:
> ASSERT MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c(3772):
> mNvVariableCache->Size == VariableStoreLength
Bringing PcdVariableStoreSize in sync with PcdFlashNvStorageVariableSize
fixes this. It also happens to ensure a volatile store size in the 4MB
build that equals the non-volatile store size, which likely doesn't hurt
for symmetry.
Cc: Jordan Justen <jordan.l.justen@intel.com>
Fixes: b24fca05751f8222acf264853709012e0ab7bf49
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/OvmfPkgIa32.dsc | 3 ++-
OvmfPkg/OvmfPkgIa32X64.dsc | 3 ++-
OvmfPkg/OvmfPkgX64.dsc | 3 ++-
3 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index e0ff7ead034e..4f518661e3b6 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -417,12 +417,13 @@ [PcdsFixedAtBuild]
!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
+ gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
!endif
!if $(FD_SIZE_IN_KB) == 4096
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
+ gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x40000
!endif
- gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 41922f581c98..4551021f5efa 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -422,12 +422,13 @@ [PcdsFixedAtBuild]
!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
+ gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
!endif
!if $(FD_SIZE_IN_KB) == 4096
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
+ gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x40000
!endif
- gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 9ba033956c88..f993d392ce9c 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -422,12 +422,13 @@ [PcdsFixedAtBuild]
!if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048)
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
+ gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
!endif
!if $(FD_SIZE_IN_KB) == 4096
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
+ gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0x40000
!endif
- gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
--
2.9.3
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0002-wip.patch --]
[-- Type: text/x-patch; name="0002-wip.patch", Size: 5165 bytes --]
From 065349ecf62e0ec8b0cb40384bf43e6af863a4de Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Fri, 5 May 2017 04:26:47 +0200
Subject: [PATCH 2/2] wip
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h | 10 +++++++--
OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c | 23 +++++++++++---------
OvmfPkg/PlatformPei/Platform.c | 5 ++---
3 files changed, 23 insertions(+), 15 deletions(-)
diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.h
index f34fad2cdd48..090b218aaf55 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 dec6d4af50df..990910deb227 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 - 1) {
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 - (UINTN) Lba);
return EFI_SUCCESS;
}
@@ -345,7 +345,9 @@ FvbProtocolEraseBlocks (
//
// Check input parameters
//
- if ((NumOfLba == 0) || (StartingLba > 1) || ((StartingLba + NumOfLba) > 2)) {
+ if ((NumOfLba == 0) ||
+ (StartingLba > EMU_FVB_NUM_TOTAL_BLOCKS - 1) ||
+ ((StartingLba + NumOfLba) > EMU_FVB_NUM_TOTAL_BLOCKS)) {
VA_END (args);
return EFI_INVALID_PARAMETER;
}
@@ -353,7 +355,7 @@ FvbProtocolEraseBlocks (
if (StartingLba == 0) {
Erase = (UINT8) (Erase | BIT0);
}
- if ((StartingLba + NumOfLba) == 2) {
+ if ((StartingLba + NumOfLba) == EMU_FVB_NUM_TOTAL_BLOCKS) {
Erase = (UINT8) (Erase | BIT1);
}
@@ -663,7 +665,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;
}
}
@@ -732,7 +734,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;
}
}
@@ -814,7 +816,7 @@ FvbInitialize (
(PcdGet32 (PcdVariableStoreSize) +
PcdGet32 (PcdFlashNvStorageFtwWorkingSize)
) >
- EMU_FVB_BLOCK_SIZE
+ EMU_FVB_BLOCK_SIZE * EMU_FVB_NUM_SPARE_BLOCKS
) {
DEBUG ((EFI_D_ERROR, "EMU Variable invalid PCD sizes\n"));
return EFI_INVALID_PARAMETER;
@@ -877,7 +879,8 @@ FvbInitialize (
//
// Initialize the Fault Tolerant Write spare block
//
- SubPtr = (VOID*) ((UINT8*) Ptr + EMU_FVB_BLOCK_SIZE);
+ SubPtr = (VOID*) ((UINT8*) Ptr +
+ EMU_FVB_BLOCK_SIZE * EMU_FVB_NUM_SPARE_BLOCKS);
PcdStatus = PcdSet32S (PcdFlashNvStorageFtwSpareBase,
(UINT32)(UINTN) SubPtr);
ASSERT_RETURN_ERROR (PcdStatus);
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index 77a8a16c15b8..c2829dbcf5c7 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -513,9 +513,8 @@ ReserveEmuVariableNvStore (
//
VariableStore =
(EFI_PHYSICAL_ADDRESS)(UINTN)
- AllocateAlignedRuntimePages (
- EFI_SIZE_TO_PAGES (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)),
- PcdGet32 (PcdFlashNvStorageFtwSpareSize)
+ AllocateRuntimePages (
+ EFI_SIZE_TO_PAGES (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize))
);
DEBUG ((EFI_D_INFO,
"Reserved variable store memory: 0x%lX; size: %dkb\n",
--
2.9.3
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/5] OvmfPkg: introduce 4MB flash image (mainly) for Windows HCK
2017-05-05 12:07 ` Laszlo Ersek
@ 2017-05-05 15:43 ` Jordan Justen
2017-05-05 16:46 ` Laszlo Ersek
0 siblings, 1 reply; 17+ messages in thread
From: Jordan Justen @ 2017-05-05 15:43 UTC (permalink / raw)
To: Laszlo Ersek; +Cc: edk2-devel-01, Gary Ching-Pang Lin
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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/5] OvmfPkg: introduce 4MB flash image (mainly) for Windows HCK
2017-05-05 15:43 ` Jordan Justen
@ 2017-05-05 16:46 ` Laszlo Ersek
0 siblings, 0 replies; 17+ messages in thread
From: Laszlo Ersek @ 2017-05-05 16:46 UTC (permalink / raw)
To: Jordan Justen; +Cc: edk2-devel-01, Gary Ching-Pang Lin
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
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-05-05 16:46 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-03 21:39 [PATCH v2 0/5] OvmfPkg: add FD_SIZE_4MB for Windows HCK SB tests, and for future proofing Laszlo Ersek
2017-05-03 21:39 ` [PATCH v2 1/5] OvmfPkg: introduce the FD_SIZE_IN_KB macro / build flag Laszlo Ersek
2017-05-03 21:39 ` [PATCH v2 2/5] OvmfPkg/OvmfPkg.fdf.inc: extract VARS_LIVE_SIZE and VARS_SPARE_SIZE macros Laszlo Ersek
2017-05-03 21:39 ` [PATCH v2 3/5] OvmfPkg: introduce 4MB flash image (mainly) for Windows HCK Laszlo Ersek
2017-05-05 4:07 ` Laszlo Ersek
2017-05-05 8:57 ` Jordan Justen
2017-05-05 12:07 ` Laszlo Ersek
2017-05-05 15:43 ` Jordan Justen
2017-05-05 16:46 ` Laszlo Ersek
2017-05-03 21:39 ` [PATCH v2 4/5] OvmfPkg: raise max variable size (auth & non-auth) to 33KB for FD_SIZE_4MB Laszlo Ersek
2017-05-04 16:36 ` Jordan Justen
2017-05-04 16:52 ` Laszlo Ersek
2017-05-04 18:50 ` Jordan Justen
2017-05-04 19:27 ` Laszlo Ersek
2017-05-04 19:30 ` Laszlo Ersek
2017-05-04 23:00 ` Laszlo Ersek
2017-05-03 21:39 ` [PATCH v2 5/5] OvmfPkg: make the 4MB flash size the default Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox