[PATCH 0/3] OvmfPkg: add FD_SIZE_4MB for Windows HCK SB tests, and for future proofing

[PATCH 0/3] OvmfPkg: add FD_SIZE_4MB for Windows HCK SB tests, and for future proofing

[Laszlo Ersek]

The patches come with detailed commit messages; for the blurb, the
subject says it all. For (a whole bunch of) details, please refer to
<https://bugzilla.redhat.com/show_bug.cgi?id=1443351>.

Repo:   https://github.com/lersek/edk2.git
Branch: fd_size_4mb

Cc: Gary Ching-Pang Lin <glin@suse.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>

Thanks,
Laszlo

Laszlo Ersek (3):
  OvmfPkg/OvmfPkg.fdf.inc: extract VARS_LIVE_SIZE and VARS_SPARE_SIZE
    macros
  OvmfPkg: introduce FD_SIZE_4MB (mainly) for Windows HCK
  OvmfPkg: raise max variable size (auth & non-auth) to 33KB for
    FD_SIZE_4MB

 OvmfPkg/OvmfPkgIa32.dsc    |  5 +++
 OvmfPkg/OvmfPkgIa32X64.dsc |  5 +++
 OvmfPkg/OvmfPkgX64.dsc     |  5 +++
 OvmfPkg/OvmfPkg.fdf.inc    | 35 ++++++++++++++-
 OvmfPkg/VarStore.fdf.inc   | 46 +++++++++++++++++++-
 5 files changed, 92 insertions(+), 4 deletions(-)

-- 
2.9.3

[PATCH 1/3] OvmfPkg/OvmfPkg.fdf.inc: extract VARS_LIVE_SIZE and VARS_SPARE_SIZE macros

[Laszlo Ersek]

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>
---
 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 9cc0578d6430..fce6c7b8ce64 100644
--- a/OvmfPkg/OvmfPkg.fdf.inc
+++ b/OvmfPkg/OvmfPkg.fdf.inc
@@ -24,6 +24,8 @@
 DEFINE BLOCK_SIZE        = 0x1000
 DEFINE VARS_SIZE         = 0x20000
 DEFINE VARS_BLOCKS       = 0x20
+DEFINE VARS_LIVE_SIZE    = 0xE000
+DEFINE VARS_SPARE_SIZE   = 0x10000
 
 !ifdef $(FD_SIZE_1MB)
 
@@ -56,7 +58,7 @@
 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)
@@ -65,6 +67,6 @@
 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

[PATCH 2/3] OvmfPkg: introduce FD_SIZE_4MB (mainly) for Windows HCK

[Laszlo Ersek]

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, 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 512 KB, and within
  it, the live area from 56 KB to 248 KB.

(For the latter, Hyper-V considers 128 KB generous:

  Re: [edk2] Secure Boot & NV storage size
  http://mid.mail-archive.com/24f63595e68c476eb98cf87e7abfa1bc@BL2PR03MB242.namprd03.prod.outlook.com

so we should be larger than that, for future proofing.)

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 should 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 should occur 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 ->   512 ( +384)
                               data
    Variable store                                 56 ->   248 ( +192)
    Event log                                       4 ->     4 (   +0)
    Working block                                   4 ->     4 (   +0)
    Spare area                                     64 ->   256 ( +192)

  FVMAIN_COMPACT             uncompressed        1712 ->  3376 (+1664)
    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

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>
---
 OvmfPkg/OvmfPkg.fdf.inc  | 29 ++++++++++++
 OvmfPkg/VarStore.fdf.inc | 46 +++++++++++++++++++-
 2 files changed, 73 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/OvmfPkg.fdf.inc b/OvmfPkg/OvmfPkg.fdf.inc
index fce6c7b8ce64..a1eb2d380167 100644
--- a/OvmfPkg/OvmfPkg.fdf.inc
+++ b/OvmfPkg/OvmfPkg.fdf.inc
@@ -20,8 +20,35 @@
 #
 # Defining FD_SIZE_1MB on the build command line can override this.
 #
+# A firmware binary built for the default 2MB flash size, and a firmware binary
+# built with FD_SIZE_1MB defined, use the same variable store layout.
+#
+# Defining FD_SIZE_4MB results in a different (much larger) variable store
+# structure that is incompatible with both the default and the FD_SIZE_1MB
+# firmware binaries.
+#
 
 DEFINE BLOCK_SIZE        = 0x1000
+
+!ifdef $(FD_SIZE_4MB)
+
+DEFINE VARS_SIZE         = 0x80000
+DEFINE VARS_BLOCKS       = 0x80
+DEFINE VARS_LIVE_SIZE    = 0x3E000
+DEFINE VARS_SPARE_SIZE   = 0x40000
+
+DEFINE FW_BASE_ADDRESS   = 0xFFC00000
+DEFINE FW_SIZE           = 0x00400000
+DEFINE FW_BLOCKS         = 0x400
+DEFINE CODE_BASE_ADDRESS = 0xFFC80000
+DEFINE CODE_SIZE         = 0x00380000
+DEFINE CODE_BLOCKS       = 0x380
+DEFINE FVMAIN_SIZE       = 0x0034C000
+DEFINE SECFV_OFFSET      = 0x003CC000
+DEFINE SECFV_SIZE        = 0x34000
+
+!else
+
 DEFINE VARS_SIZE         = 0x20000
 DEFINE VARS_BLOCKS       = 0x20
 DEFINE VARS_LIVE_SIZE    = 0xE000
@@ -53,6 +80,8 @@
 
 !endif
 
+!endif
+
 SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress     = $(FW_BASE_ADDRESS)
 SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize    = $(FW_SIZE)
 SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareBlockSize = $(BLOCK_SIZE)
diff --git a/OvmfPkg/VarStore.fdf.inc b/OvmfPkg/VarStore.fdf.inc
index ce901c0109b1..7325b698157d 100644
--- a/OvmfPkg/VarStore.fdf.inc
+++ b/OvmfPkg/VarStore.fdf.inc
@@ -15,7 +15,11 @@
 #
 ##
 
+!ifdef $(FD_SIZE_4MB)
+0x00000000|0x0003e000
+!else
 0x00000000|0x0000e000
+!endif
 #NV_VARIABLE_STORE
 DATA = {
   ## This is the EFI_FIRMWARE_VOLUME_HEADER
@@ -27,14 +31,33 @@
   #     { 0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50 }}
   0x8D, 0x2B, 0xF1, 0xFF, 0x96, 0x76, 0x8B, 0x4C,
   0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50,
+!ifdef $(FD_SIZE_4MB)
+  # FvLength: 0x80000
+  0x00, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00,
+!else
   # FvLength: 0x20000
   0x00, 0x00, 0x02, 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,
+!ifdef $(FD_SIZE_4MB)
+  # CheckSum
+  0xB3, 0xF8,
+!else
+  # CheckSum
+  0x19, 0xF9,
+!endif
+  # ExtHeaderOffset #Reserved #Revision
+  0x00, 0x00, 0x00, 0x02,
+!ifdef $(FD_SIZE_4MB)
+  # Blockmap[0]: 0x80 Blocks * 0x1000 Bytes / Block
+  0x80, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, 0x00,
+!else
   # Blockmap[0]: 0x20 Blocks * 0x1000 Bytes / Block
   0x20, 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
@@ -44,18 +67,33 @@
   #     { 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92 }}
   0x78, 0x2c, 0xf3, 0xaa, 0x7b, 0x94, 0x9a, 0x43,
   0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92,
+!ifdef $(FD_SIZE_4MB)
+  # Size: 0x3e000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) -
+  #          0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0x3dfb8
+  # This can speed up the Variable Dispatch a bit.
+  0xB8, 0xDF, 0x03, 0x00,
+!else
   # 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
   # FORMATTED: 0x5A #HEALTHY: 0xFE #Reserved: UINT16 #Reserved1: UINT32
   0x5A, 0xFE, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
 }
 
+!ifdef $(FD_SIZE_4MB)
+0x0003e000|0x00001000
+!else
 0x0000e000|0x00001000
+!endif
 #NV_EVENT_LOG
 
+!ifdef $(FD_SIZE_4MB)
+0x0003f000|0x00001000
+!else
 0x0000f000|0x00001000
+!endif
 #NV_FTW_WORKING
 DATA = {
   # EFI_FAULT_TOLERANT_WORKING_BLOCK_HEADER->Signature = gEdkiiWorkingBlockSignatureGuid         =
@@ -68,5 +106,9 @@
   0xE0, 0x0F, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
 }
 
+!ifdef $(FD_SIZE_4MB)
+0x00040000|0x00040000
+!else
 0x00010000|0x00010000
+!endif
 #NV_FTW_SPARE
-- 
2.9.3

[PATCH 3/3] OvmfPkg: raise max variable size (auth & non-auth) to 33KB for FD_SIZE_4MB

[Laszlo Ersek]

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 248 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>
---
 OvmfPkg/OvmfPkgIa32.dsc    | 5 +++++
 OvmfPkg/OvmfPkgIa32X64.dsc | 5 +++++
 OvmfPkg/OvmfPkgX64.dsc     | 5 +++++
 3 files changed, 15 insertions(+)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 0796b0db816b..a87aa669b907 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -395,8 +395,13 @@ [PcdsFixedAtBuild]
   gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
   gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
   gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
+!ifdef $(FD_SIZE_4MB)
+  gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
+  gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
+!else
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
+!endif
   gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
 
   gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 71ac62f023b5..c1e9faf0ef88 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -400,8 +400,13 @@ [PcdsFixedAtBuild]
   gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
   gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
   gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
+!ifdef $(FD_SIZE_4MB)
+  gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
+  gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
+!else
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
+!endif
   gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
 
   gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index 2ceb31d7ffd5..81e06056226f 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -400,8 +400,13 @@ [PcdsFixedAtBuild]
   gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler|0x10
   gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxFvSupported|6
   gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeimPerFv|32
+!ifdef $(FD_SIZE_4MB)
+  gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400
+  gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400
+!else
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x2000
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800
+!endif
   gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000
 
   gEfiMdeModulePkgTokenSpaceGuid.PcdVpdBaseAddress|0x0
-- 
2.9.3

Re: [PATCH 2/3] OvmfPkg: introduce FD_SIZE_4MB (mainly) for Windows HCK

[Jordan Justen]

On 2017-04-29 13:14:59, 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, 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 512 KB, and within
>   it, the live area from 56 KB to 248 KB.
> 
> (For the latter, Hyper-V considers 128 KB generous:

Today, we give 128k to non-volatile storage, which due to various
reasons results in 56k of variable storage. Personally I already
consider this generous, especially when I look at the 128k size of the
seabios bios.bin. :)

>   Re: [edk2] Secure Boot & NV storage size
>   http://mid.mail-archive.com/24f63595e68c476eb98cf87e7abfa1bc@BL2PR03MB242.namprd03.prod.outlook.com
> 
> so we should be larger than that, for future proofing.)

I don't want to try to predict the future of Microsoft requirements,
nor be overly generous in the meantime. :) As pointed out by a
colleague, as of Fall 2016, Microsoft expects 120 KB as the current
requirement:

http://www.uefi.org/sites/default/files/resources/UEFI_Plugfest_Security_Microsoft_Fall_2016.pdf

I tested a 2MB RELEASE build of OvmfIa32X64 on GCC 4.9 with:

 * SECURE_BOOT_ENABLE=1
 * SMM_REQUIRE=1

There was > 600k available.

I also tested a 2MB RELEASE build of OvmfIa32X64 on GCC 4.9 with:

 * SECURE_BOOT_ENABLE=1
 * NETWORK_IP6_ENABLE=1
 * HTTP_BOOT_ENABLE=1
 * SMM_REQUIRE=1
 * TLS_ENABLE=1

I don't think we consider those network items as standard
requirements, yet there was still ~373k available. In order to bump
the variables to 120k, we need to use 2 * (120 - 56) => 128k.

So, what about adjusting our 2MB image to this?

Regarding how to 'upgrade' a system from using the smaller storage
size, I don't think there are any good answers. (Which also applies to
the 4MB case.) Does fw-cfg tell us the rom/flash regions?

-Jordan

> 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 should 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 should occur 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 ->   512 ( +384)
>                                data
>     Variable store                                 56 ->   248 ( +192)
>     Event log                                       4 ->     4 (   +0)
>     Working block                                   4 ->     4 (   +0)
>     Spare area                                     64 ->   256 ( +192)
> 
>   FVMAIN_COMPACT             uncompressed        1712 ->  3376 (+1664)
>     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
> 
> 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>
> ---
>  OvmfPkg/OvmfPkg.fdf.inc  | 29 ++++++++++++
>  OvmfPkg/VarStore.fdf.inc | 46 +++++++++++++++++++-
>  2 files changed, 73 insertions(+), 2 deletions(-)
> 
> diff --git a/OvmfPkg/OvmfPkg.fdf.inc b/OvmfPkg/OvmfPkg.fdf.inc
> index fce6c7b8ce64..a1eb2d380167 100644
> --- a/OvmfPkg/OvmfPkg.fdf.inc
> +++ b/OvmfPkg/OvmfPkg.fdf.inc
> @@ -20,8 +20,35 @@
>  #
>  # Defining FD_SIZE_1MB on the build command line can override this.
>  #
> +# A firmware binary built for the default 2MB flash size, and a firmware binary
> +# built with FD_SIZE_1MB defined, use the same variable store layout.
> +#
> +# Defining FD_SIZE_4MB results in a different (much larger) variable store
> +# structure that is incompatible with both the default and the FD_SIZE_1MB
> +# firmware binaries.
> +#
>  
>  DEFINE BLOCK_SIZE        = 0x1000
> +
> +!ifdef $(FD_SIZE_4MB)
> +
> +DEFINE VARS_SIZE         = 0x80000
> +DEFINE VARS_BLOCKS       = 0x80
> +DEFINE VARS_LIVE_SIZE    = 0x3E000
> +DEFINE VARS_SPARE_SIZE   = 0x40000
> +
> +DEFINE FW_BASE_ADDRESS   = 0xFFC00000
> +DEFINE FW_SIZE           = 0x00400000
> +DEFINE FW_BLOCKS         = 0x400
> +DEFINE CODE_BASE_ADDRESS = 0xFFC80000
> +DEFINE CODE_SIZE         = 0x00380000
> +DEFINE CODE_BLOCKS       = 0x380
> +DEFINE FVMAIN_SIZE       = 0x0034C000
> +DEFINE SECFV_OFFSET      = 0x003CC000
> +DEFINE SECFV_SIZE        = 0x34000
> +
> +!else
> +
>  DEFINE VARS_SIZE         = 0x20000
>  DEFINE VARS_BLOCKS       = 0x20
>  DEFINE VARS_LIVE_SIZE    = 0xE000
> @@ -53,6 +80,8 @@
>  
>  !endif
>  
> +!endif
> +
>  SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress     = $(FW_BASE_ADDRESS)
>  SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize    = $(FW_SIZE)
>  SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareBlockSize = $(BLOCK_SIZE)
> diff --git a/OvmfPkg/VarStore.fdf.inc b/OvmfPkg/VarStore.fdf.inc
> index ce901c0109b1..7325b698157d 100644
> --- a/OvmfPkg/VarStore.fdf.inc
> +++ b/OvmfPkg/VarStore.fdf.inc
> @@ -15,7 +15,11 @@
>  #
>  ##
>  
> +!ifdef $(FD_SIZE_4MB)
> +0x00000000|0x0003e000
> +!else
>  0x00000000|0x0000e000
> +!endif
>  #NV_VARIABLE_STORE
>  DATA = {
>    ## This is the EFI_FIRMWARE_VOLUME_HEADER
> @@ -27,14 +31,33 @@
>    #     { 0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50 }}
>    0x8D, 0x2B, 0xF1, 0xFF, 0x96, 0x76, 0x8B, 0x4C,
>    0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50,
> +!ifdef $(FD_SIZE_4MB)
> +  # FvLength: 0x80000
> +  0x00, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00,
> +!else
>    # FvLength: 0x20000
>    0x00, 0x00, 0x02, 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,
> +!ifdef $(FD_SIZE_4MB)
> +  # CheckSum
> +  0xB3, 0xF8,
> +!else
> +  # CheckSum
> +  0x19, 0xF9,
> +!endif
> +  # ExtHeaderOffset #Reserved #Revision
> +  0x00, 0x00, 0x00, 0x02,
> +!ifdef $(FD_SIZE_4MB)
> +  # Blockmap[0]: 0x80 Blocks * 0x1000 Bytes / Block
> +  0x80, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, 0x00,
> +!else
>    # Blockmap[0]: 0x20 Blocks * 0x1000 Bytes / Block
>    0x20, 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
> @@ -44,18 +67,33 @@
>    #     { 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92 }}
>    0x78, 0x2c, 0xf3, 0xaa, 0x7b, 0x94, 0x9a, 0x43,
>    0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92,
> +!ifdef $(FD_SIZE_4MB)
> +  # Size: 0x3e000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) -
> +  #          0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0x3dfb8
> +  # This can speed up the Variable Dispatch a bit.
> +  0xB8, 0xDF, 0x03, 0x00,
> +!else
>    # 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
>    # FORMATTED: 0x5A #HEALTHY: 0xFE #Reserved: UINT16 #Reserved1: UINT32
>    0x5A, 0xFE, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>  }
>  
> +!ifdef $(FD_SIZE_4MB)
> +0x0003e000|0x00001000
> +!else
>  0x0000e000|0x00001000
> +!endif
>  #NV_EVENT_LOG
>  
> +!ifdef $(FD_SIZE_4MB)
> +0x0003f000|0x00001000
> +!else
>  0x0000f000|0x00001000
> +!endif
>  #NV_FTW_WORKING
>  DATA = {
>    # EFI_FAULT_TOLERANT_WORKING_BLOCK_HEADER->Signature = gEdkiiWorkingBlockSignatureGuid         =
> @@ -68,5 +106,9 @@
>    0xE0, 0x0F, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>  }
>  
> +!ifdef $(FD_SIZE_4MB)
> +0x00040000|0x00040000
> +!else
>  0x00010000|0x00010000
> +!endif
>  #NV_FTW_SPARE
> -- 
> 2.9.3
> 
> 

Re: [PATCH 2/3] OvmfPkg: introduce FD_SIZE_4MB (mainly) for Windows HCK

[Laszlo Ersek]

On 04/30/17 02:48, Jordan Justen wrote:
> On 2017-04-29 13:14:59, 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, 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 512 KB, and within
>>   it, the live area from 56 KB to 248 KB.
>>
>> (For the latter, Hyper-V considers 128 KB generous:
> 
> Today, we give 128k to non-volatile storage, which due to various
> reasons results in 56k of variable storage. Personally I already
> consider this generous, especially when I look at the 128k size of the
> seabios bios.bin. :)

I don't consider our current state generous. In a default setup /
minimal enrollment that is just enough to pass the Secure Boot Logo
Test, the live area usage is approx 0x4300 bytes (~17 KB). MokManager /
shim use a separate set of variables for storing certificate material.
If you enable the efi-pstore backend in Linux, to save crash messages in
efi variables, that can take up a further 10+ KB. DBX is only growing,
never shrinking. I wouldn't like to run into the same issue in a few
years with, say, RHEL-7.9.

> 
>>   Re: [edk2] Secure Boot & NV storage size
>>   http://mid.mail-archive.com/24f63595e68c476eb98cf87e7abfa1bc@BL2PR03MB242.namprd03.prod.outlook.com
>>
>> so we should be larger than that, for future proofing.)
> 
> I don't want to try to predict the future of Microsoft requirements,
> nor be overly generous in the meantime. :)

What's wrong with providing more, and being more robust in the face of
increased future demands?

> As pointed out by a
> colleague, as of Fall 2016, Microsoft expects 120 KB as the current
> requirement:
> 
> http://www.uefi.org/sites/default/files/resources/UEFI_Plugfest_Security_Microsoft_Fall_2016.pdf

Slide 8 also says "there is no maximum NVRAM storage limit".

> 
> I tested a 2MB RELEASE build of OvmfIa32X64 on GCC 4.9 with:

RELEASE builds of virtual UEFI firmware are unsupportable in an
enterprise distribution. On tenths of occasions I've been able to
analyze OVMF and ArmVirtQemu error reports from:
- failed ASSERTs, and
- DEBUG messages
that would have been all lost in a RELEASE build.

QEMU (even upstream QEMU) rejects being built with NDEBUG; they consider
the asserts so important. For example, in "hw/virtio/virtio.c":

#ifdef NDEBUG
#error building with NDEBUG is not supported
#endif

See also this recent thread:

http://lists.nongnu.org/archive/html/qemu-devel/2017-04/msg05499.html

Edk2 in particular still uses ASSERT in a bunch of places for error
checking.

> 
>  * SECURE_BOOT_ENABLE=1
>  * SMM_REQUIRE=1
> 
> There was > 600k available.

Do you mean in FVMAIN_COMPACT?

> 
> I also tested a 2MB RELEASE build of OvmfIa32X64 on GCC 4.9 with:
> 
>  * SECURE_BOOT_ENABLE=1
>  * NETWORK_IP6_ENABLE=1
>  * HTTP_BOOT_ENABLE=1
>  * SMM_REQUIRE=1
>  * TLS_ENABLE=1
> 
> I don't think we consider those network items as standard
> requirements, yet there was still ~373k available. In order to bump
> the variables to 120k, we need to use 2 * (120 - 56) => 128k.

Do I understand correctly that you suggest adding 64K to the live area,
64K to the spare area, and decreasing FVMAIN_COMPACT by 128k?

I think that's a step in the wrong direction.

$ build \
  -b DEBUG \
  -a IA32 -a X64 \
  -p OvmfPkg/OvmfPkgIa32X64.dsc \
  -t GCC48 \
  -D SMM_REQUIRE \
  -D SECURE_BOOT_ENABLE \
  -D HTTP_BOOT_ENABLE \
  -D NETWORK_IP6_ENABLE \
  -D TLS_ENABLE

> FV Space Information
> SECFV [14%Full] 212992 total, 30560 used, 182432 free
> FVMAIN_COMPACT [98%Full] 1753088 total, 1731168 used, 21920 free
> DXEFV [69%Full] 10485760 total, 7292952 used, 3192808 free
> PEIFV [33%Full] 917504 total, 304752 used, 612752 free

21920 bytes left free in FVMAIN_COMPACT, and that's with two thirds of
PEIFV, and one third of DXEFV, left un-utilized (hence almost perfectly
compressible).

After adding -D FD_SIZE_4MB:

> FV Space Information
> SECFV [14%Full] 212992 total, 30560 used, 182432 free
> FVMAIN_COMPACT [50%Full] 3457024 total, 1730864 used, 1726160 free
> DXEFV [69%Full] 10485760 total, 7292952 used, 3192808 free
> PEIFV [33%Full] 917504 total, 304752 used, 612752 free

> 
> So, what about adjusting our 2MB image to this?

I disagree with the idea. I disagree both with bumping the live varstore
size to only 120KB, and with doing that at the expense of
FVMAIN_COMPACT's size.

Changing the live varstore's size from 56 KB to anything else already
breaks compatibility, in itself, between old varstore files and new
firmware binaries. We should benefit as much as possible from that pain;
in other words, push the next such time out as far as possible to the
future. By keeping the final flash image size 2MB, we'd just save 2MB
otherwise fully useless MMIO space under 4GB.

> 
> Regarding how to 'upgrade' a system from using the smaller storage
> size, I don't think there are any good answers. (Which also applies to
> the 4MB case.)

I agree, on both points.

Which is why I think it's time to make the big jump now, and be safe for
the next several years.

In my vocabulary that means the most distant RHEL7 support deadline I
can presently look at, which seems to be 30 June 2024, from
<https://en.wikipedia.org/wiki/Red_Hat_Enterprise_Linux#Product_life_cycle>.

And, I don't see how this change would hurt, rather than benefit, other
downstreams, or any upstream users. The only cost is 2MB of currently
unused MMIO space under 4GB, which I had set aside in QEMU specifically
for this purpose (i.e., future growth), in commit 637a5acb46b3
("hw/i386/pc_sysfw: support two flash drives", 2013-11-28).

It's not like we have to pay for the silicon on the i440fx / Q35
motherboards.

> Does fw-cfg tell us the rom/flash regions?

It doesn't.

Even if it did, I don't see how the varstore template could be built
non-statically.

A statically built varstore template is necessary, because when creating
a new virtual machine, libvirt uses a (firmware binary <-> varstore
template) mapping, to create the private varstore file for the VM, from
the template that matches the selected firmware binary.

The (firmware binary <-> varstore template) mapping in the libvirt
configuration (the "nvram" stanza in /etc/libvirt/qemu.conf)
accommodates arbitrary pairings. FD_SIZE_4MB is just one use case that
puts this flexibility to use. Other examples include the historical case
when an SB-enabled OVMF binary needed a different varstore template from
an SB-disabled binary, and the current case when you have AARCH64 (TCG)
and x86_64 (KVM) VMs on the same x86_64 host, managed by the same
libvirt installation. In that case, the ArmVirtQemu firmware(s) have to
be paired with their matching (independent of OVMF) varstore templates.

The nvram stanza on my laptop looks like this:

nvram = [
  "/usr/share/OVMF/OVMF_CODE.fd:/usr/share/OVMF/OVMF_VARS.fd",
  "/usr/share/OVMF/OVMF_CODE.secboot.fd:/usr/share/OVMF/OVMF_VARS.fd",

  "/home/virt-images/OVMF_CODE.32.fd:/home/virt-images/OVMF_VARS.fd",
  "/home/virt-images/OVMF_CODE.3264.fd:/home/virt-images/OVMF_VARS.fd",
  "/home/virt-images/OVMF_CODE.fd:/home/virt-images/OVMF_VARS.fd",

  "/home/virt-images/OVMF_CODE.4m.32.fd:/home/virt-images/OVMF_VARS.4m.fd",

"/home/virt-images/OVMF_CODE.4m.3264.fd:/home/virt-images/OVMF_VARS.4m.fd",
  "/home/virt-images/OVMF_CODE.4m.fd:/home/virt-images/OVMF_VARS.4m.fd",

  "/usr/share/AAVMF/AAVMF_CODE.fd:/usr/share/-AAVMF/AAVMF_VARS.fd",
  "/usr/share/AAVMF/AAVMF_CODE.verbose.fd:/usr/share/AAVMF/AAVMF_VARS.fd",


"/home/virt-images/arm/QEMU_EFI.fd.padded:/home/virt-images/arm/QEMU_VARS.fd.padded"
]

Thanks
Laszlo

> 
> -Jordan
> 
>> 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 should 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 should occur 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 ->   512 ( +384)
>>                                data
>>     Variable store                                 56 ->   248 ( +192)
>>     Event log                                       4 ->     4 (   +0)
>>     Working block                                   4 ->     4 (   +0)
>>     Spare area                                     64 ->   256 ( +192)
>>
>>   FVMAIN_COMPACT             uncompressed        1712 ->  3376 (+1664)
>>     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
>>
>> 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>
>> ---
>>  OvmfPkg/OvmfPkg.fdf.inc  | 29 ++++++++++++
>>  OvmfPkg/VarStore.fdf.inc | 46 +++++++++++++++++++-
>>  2 files changed, 73 insertions(+), 2 deletions(-)
>>
>> diff --git a/OvmfPkg/OvmfPkg.fdf.inc b/OvmfPkg/OvmfPkg.fdf.inc
>> index fce6c7b8ce64..a1eb2d380167 100644
>> --- a/OvmfPkg/OvmfPkg.fdf.inc
>> +++ b/OvmfPkg/OvmfPkg.fdf.inc
>> @@ -20,8 +20,35 @@
>>  #
>>  # Defining FD_SIZE_1MB on the build command line can override this.
>>  #
>> +# A firmware binary built for the default 2MB flash size, and a firmware binary
>> +# built with FD_SIZE_1MB defined, use the same variable store layout.
>> +#
>> +# Defining FD_SIZE_4MB results in a different (much larger) variable store
>> +# structure that is incompatible with both the default and the FD_SIZE_1MB
>> +# firmware binaries.
>> +#
>>  
>>  DEFINE BLOCK_SIZE        = 0x1000
>> +
>> +!ifdef $(FD_SIZE_4MB)
>> +
>> +DEFINE VARS_SIZE         = 0x80000
>> +DEFINE VARS_BLOCKS       = 0x80
>> +DEFINE VARS_LIVE_SIZE    = 0x3E000
>> +DEFINE VARS_SPARE_SIZE   = 0x40000
>> +
>> +DEFINE FW_BASE_ADDRESS   = 0xFFC00000
>> +DEFINE FW_SIZE           = 0x00400000
>> +DEFINE FW_BLOCKS         = 0x400
>> +DEFINE CODE_BASE_ADDRESS = 0xFFC80000
>> +DEFINE CODE_SIZE         = 0x00380000
>> +DEFINE CODE_BLOCKS       = 0x380
>> +DEFINE FVMAIN_SIZE       = 0x0034C000
>> +DEFINE SECFV_OFFSET      = 0x003CC000
>> +DEFINE SECFV_SIZE        = 0x34000
>> +
>> +!else
>> +
>>  DEFINE VARS_SIZE         = 0x20000
>>  DEFINE VARS_BLOCKS       = 0x20
>>  DEFINE VARS_LIVE_SIZE    = 0xE000
>> @@ -53,6 +80,8 @@
>>  
>>  !endif
>>  
>> +!endif
>> +
>>  SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress     = $(FW_BASE_ADDRESS)
>>  SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize    = $(FW_SIZE)
>>  SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareBlockSize = $(BLOCK_SIZE)
>> diff --git a/OvmfPkg/VarStore.fdf.inc b/OvmfPkg/VarStore.fdf.inc
>> index ce901c0109b1..7325b698157d 100644
>> --- a/OvmfPkg/VarStore.fdf.inc
>> +++ b/OvmfPkg/VarStore.fdf.inc
>> @@ -15,7 +15,11 @@
>>  #
>>  ##
>>  
>> +!ifdef $(FD_SIZE_4MB)
>> +0x00000000|0x0003e000
>> +!else
>>  0x00000000|0x0000e000
>> +!endif
>>  #NV_VARIABLE_STORE
>>  DATA = {
>>    ## This is the EFI_FIRMWARE_VOLUME_HEADER
>> @@ -27,14 +31,33 @@
>>    #     { 0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50 }}
>>    0x8D, 0x2B, 0xF1, 0xFF, 0x96, 0x76, 0x8B, 0x4C,
>>    0xA9, 0x85, 0x27, 0x47, 0x07, 0x5B, 0x4F, 0x50,
>> +!ifdef $(FD_SIZE_4MB)
>> +  # FvLength: 0x80000
>> +  0x00, 0x00, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00,
>> +!else
>>    # FvLength: 0x20000
>>    0x00, 0x00, 0x02, 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,
>> +!ifdef $(FD_SIZE_4MB)
>> +  # CheckSum
>> +  0xB3, 0xF8,
>> +!else
>> +  # CheckSum
>> +  0x19, 0xF9,
>> +!endif
>> +  # ExtHeaderOffset #Reserved #Revision
>> +  0x00, 0x00, 0x00, 0x02,
>> +!ifdef $(FD_SIZE_4MB)
>> +  # Blockmap[0]: 0x80 Blocks * 0x1000 Bytes / Block
>> +  0x80, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, 0x00,
>> +!else
>>    # Blockmap[0]: 0x20 Blocks * 0x1000 Bytes / Block
>>    0x20, 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
>> @@ -44,18 +67,33 @@
>>    #     { 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92 }}
>>    0x78, 0x2c, 0xf3, 0xaa, 0x7b, 0x94, 0x9a, 0x43,
>>    0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92,
>> +!ifdef $(FD_SIZE_4MB)
>> +  # Size: 0x3e000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) -
>> +  #          0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0x3dfb8
>> +  # This can speed up the Variable Dispatch a bit.
>> +  0xB8, 0xDF, 0x03, 0x00,
>> +!else
>>    # 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
>>    # FORMATTED: 0x5A #HEALTHY: 0xFE #Reserved: UINT16 #Reserved1: UINT32
>>    0x5A, 0xFE, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>  }
>>  
>> +!ifdef $(FD_SIZE_4MB)
>> +0x0003e000|0x00001000
>> +!else
>>  0x0000e000|0x00001000
>> +!endif
>>  #NV_EVENT_LOG
>>  
>> +!ifdef $(FD_SIZE_4MB)
>> +0x0003f000|0x00001000
>> +!else
>>  0x0000f000|0x00001000
>> +!endif
>>  #NV_FTW_WORKING
>>  DATA = {
>>    # EFI_FAULT_TOLERANT_WORKING_BLOCK_HEADER->Signature = gEdkiiWorkingBlockSignatureGuid         =
>> @@ -68,5 +106,9 @@
>>    0xE0, 0x0F, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
>>  }
>>  
>> +!ifdef $(FD_SIZE_4MB)
>> +0x00040000|0x00040000
>> +!else
>>  0x00010000|0x00010000
>> +!endif
>>  #NV_FTW_SPARE
>> -- 
>> 2.9.3
>>
>>

Re: [PATCH 2/3] OvmfPkg: introduce FD_SIZE_4MB (mainly) for Windows HCK

[Jordan Justen]

On 2017-04-30 07:42:36, Laszlo Ersek wrote:
> On 04/30/17 02:48, Jordan Justen wrote:
> > 
> > I tested a 2MB RELEASE build of OvmfIa32X64 on GCC 4.9 with:
> 
> RELEASE builds of virtual UEFI firmware are unsupportable in an
> enterprise distribution. On tenths of occasions I've been able to
> analyze OVMF and ArmVirtQemu error reports from:
> - failed ASSERTs, and
> - DEBUG messages
> that would have been all lost in a RELEASE build.
> 
> QEMU (even upstream QEMU) rejects being built with NDEBUG; they consider
> the asserts so important. For example, in "hw/virtio/virtio.c":

We're discussing space issues, and a RELEASE build is considered
unusable? I don't know what that says about the qemu project, but it
certainly is not how EDK II was designed.

Nevertheless, I agree that it is very nice to be able to enable DEBUG
mode with the standard features included. Regarding 'standard
features', see my IP6/HTTP/TLS question below.

> > 
> > I also tested a 2MB RELEASE build of OvmfIa32X64 on GCC 4.9 with:
> > 
> >  * SECURE_BOOT_ENABLE=1
> >  * NETWORK_IP6_ENABLE=1
> >  * HTTP_BOOT_ENABLE=1
> >  * SMM_REQUIRE=1
> >  * TLS_ENABLE=1
> > 
> > I don't think we consider those network items as standard
> > requirements, yet there was still ~373k available. In order to bump
> > the variables to 120k, we need to use 2 * (120 - 56) => 128k.
> 
> Do I understand correctly that you suggest adding 64K to the live area,
> 64K to the spare area, and decreasing FVMAIN_COMPACT by 128k?
> 
> I think that's a step in the wrong direction.

It is starting to look less and less like 1MB is a feasible size for
OVMF. Maybe going forward we'll drop 1MB, and support 2/4MB. If that
happens, then it would be nice if the 2MB image covered the known
requirements.

> $ build \
>   -b DEBUG \
>   -a IA32 -a X64 \
>   -p OvmfPkg/OvmfPkgIa32X64.dsc \
>   -t GCC48 \
>   -D SMM_REQUIRE \
>   -D SECURE_BOOT_ENABLE \
>   -D HTTP_BOOT_ENABLE \
>   -D NETWORK_IP6_ENABLE \
>   -D TLS_ENABLE

Do you enable the last 3 in your production builds? I didn't think it
was the case, but it would change things...

> > 
> > Regarding how to 'upgrade' a system from using the smaller storage
> > size, I don't think there are any good answers. (Which also applies to
> > the 4MB case.)
> 
> I agree, on both points.

How would you plan to support VMs with the old 2MB layout?

I guess it could be easy enough to develop a python script to resize
the old layout to the new layout, but I'm not sure if it is possible
for libvirt to handle the need to launch such an upgrade script..

> Which is why I think it's time to make the big jump now, and be safe for
> the next several years.

Why not just go for 8 MB, and give Microsoft, say 1 MB for variables?
That should be 'future proof', right? :)

The reality is that there's no good way to tell what Microsoft (or Red
Hat) may require in the future. Right now we know that Microsoft
appears to be saying 120k is good for at least the near term.

I'm not against us defining a 4MB image size, but it is frustrating to
be pushed into it by a single test. You did give an example of a crash
dump using 10k of variable space, but even then it is not clear to me
that 56k is insufficient in normal usage.

Regarding your suggested 4MB layout, it seems reasonable. My only
concern is, if the minimum flash size is bumped. What are the chances
that 248k will cover it? Unfortunately (or fortunately), no one I've
asked seem to know of any plans for the requirement to increase beyond
120k.

-Jordan

Re: [PATCH 2/3] OvmfPkg: introduce FD_SIZE_4MB (mainly) for Windows HCK

[Laszlo Ersek]

On 04/30/17 02:48, Jordan Justen wrote:

> Today, we give 128k to non-volatile storage, which due to various
> reasons results in 56k of variable storage. Personally I already
> consider this generous, especially when I look at the 128k size of the
> seabios bios.bin. :)

I'd like to comment on the seabios analogy; I just needed a bit more
time to gather data for this (BZs and build logs, mainly).

In RHEL-6, we shipped (and have been shipping) 128KB SeaBIOS binaries.

Preparing for RHEL-7.0, we increased the SeaBIOS binary size to 256KB,
anticipating future features that would no longer fit into 128KB. This
is the BZ for it:

https://bugzilla.redhat.com/show_bug.cgi?id=1038604

The exact "seabios-1.7.2.2-5.el7" build, dated 2013-Dec-10, in which the
BZ was addressed, is no longer available in our build archives (it was
not a released version, just a development build, so that's OK), but
"seabios-1.7.2.2-6.el7", i.e. the build right after, is available. At
that time, we were still under 128KB:

> Total size: 117472  Fixed: 58792  Free: 144672
> (used 44.8% of 256KiB rom)

Today, looking at the build log of "seabios-1.10.2-2.el7" (built
2017-Mar-30), I find:

> Total size: 149664  Fixed: 75392  Free: 112480
> (used 57.1% of 256KiB rom)

Had we not increased the size in advance (via CONFIG_ROM_SIZE), we'd
have run out of it over the past ~3.5 years.


In OVMF, the address range of the non-volatile area depends on SECFV's
size, FVMAIN_COMPACT's size, and on the NV area's size; they come in
this order downwards from 4GB.

SECFV is safe as-is; it has seen negligible growth in years, and is
currently at about 14% in a DEBUG build.

I'd like to future-proof the other two (FVMAIN_COMPACT and NV) sizes
now, with a large -- approx. seven year -- margin.

Thanks
Laszlo

Re: [PATCH 2/3] OvmfPkg: introduce FD_SIZE_4MB (mainly) for Windows HCK

[Laszlo Ersek]

On 04/30/17 23:16, Jordan Justen wrote:
> On 2017-04-30 07:42:36, Laszlo Ersek wrote:
>> On 04/30/17 02:48, Jordan Justen wrote:
>>>
>>> I tested a 2MB RELEASE build of OvmfIa32X64 on GCC 4.9 with:
>>
>> RELEASE builds of virtual UEFI firmware are unsupportable in an
>> enterprise distribution. On tenths of occasions I've been able to
>> analyze OVMF and ArmVirtQemu error reports from:
>> - failed ASSERTs, and
>> - DEBUG messages
>> that would have been all lost in a RELEASE build.
>>
>> QEMU (even upstream QEMU) rejects being built with NDEBUG; they consider
>> the asserts so important. For example, in "hw/virtio/virtio.c":
> 
> We're discussing space issues, and a RELEASE build is considered
> unusable? I don't know what that says about the qemu project, but it
> certainly is not how EDK II was designed.

I didn't write "unusable". A RELEASE build can certainly be run by end
users. I wrote "unsupportable".

The norm in an enterprise distro is not that a package is given to end
users and they use it happily ever after without encountering bugs.

The norm is that users encounter bugs or just plain non-intuitive
behavior all the time, and they report them. I have to be able to
support them, preferably without asking for access to their systems.
DEBUG logs and working ASSERTs are very important for this.

> 
> Nevertheless, I agree that it is very nice to be able to enable DEBUG
> mode with the standard features included.

It's not "very nice", it is a supportability requirement.

Here's a real life analogy. My work laptop is a Lenovo ThinkPad W541. On
random occasions it locks up during boot, while in the firmware. It
doesn't print anything at all (which is not surprising if the bug hits
before the video card is bound by the GOP driver).

Now, if the laptop had an actual serial port (which could be printed to
even during SEC, via DebugLib / SerialPortLib), *and* the firmware
shipped on the machine were a DEBUG build, I could perhaps file a
sensible bug report somewhere. It's not the case, so any engineer on the
vendor's side would be clueless. I would be instructed to install a
firmware upgrade, which I wouldn't do, because it might or might not
brick the laptop completely.

And this is supposed to be a "mobile workstation" class laptop.

The fact is that the vendor doesn't really intend to support this
laptop, otherwise they'd have built a serial port into it.

I intend to support OVMF in RHEL-7.4+ the best way I can, and for that,
I absolutely need DEBUGs and ASSERTs to be in place.

BTW, status code reporting is also entirely useless. On my APM Mustang
(an aarch64 development box), there used to be serious firmware bugs,
and I figured I'd look into some of those. The firmware by default only
printed status codes to the serial port, which provided zero value. Only
after I rebuilt & reflashed the firmware with DEBUG_VERBOSE enabled, and
could correlate those messages with the kernel logs, did I manage to fix
some issues.


Regarding the design of edk2; I'm not qualified to comment on that (I
wasn't there), but the fact remains that in many places ASSERT()s are
used to catch common errors. Even if that wasn't the case, and ASSERT()s
were only used for catching impossible situations, I do want those
situations caught and logged in a production / customer / partner
environment too.

> Regarding 'standard
> features', see my IP6/HTTP/TLS question below.
> 
>>>
>>> I also tested a 2MB RELEASE build of OvmfIa32X64 on GCC 4.9 with:
>>>
>>>  * SECURE_BOOT_ENABLE=1
>>>  * NETWORK_IP6_ENABLE=1
>>>  * HTTP_BOOT_ENABLE=1
>>>  * SMM_REQUIRE=1
>>>  * TLS_ENABLE=1
>>>
>>> I don't think we consider those network items as standard
>>> requirements, yet there was still ~373k available. In order to bump
>>> the variables to 120k, we need to use 2 * (120 - 56) => 128k.
>>
>> Do I understand correctly that you suggest adding 64K to the live area,
>> 64K to the spare area, and decreasing FVMAIN_COMPACT by 128k?
>>
>> I think that's a step in the wrong direction.
> 
> It is starting to look less and less like 1MB is a feasible size for
> OVMF. Maybe going forward we'll drop 1MB, and support 2/4MB. If that
> happens, then it would be nice if the 2MB image covered the known
> requirements.

If we change the variable store size in the default 2MB build (at the
expense of FVMAIN_COMPACT, or anything else), without any additional
(non-default) build flags, then all downstreams that rebase / rebuild
edk2 after such an upstream patch will see their preexistent VMs break.

Their existent varstore files will not be compatible with the rebuilt
firmware binary.

So, tieing the varstore structure change to a new build flag is required
in any case, in order not to regress current consumers of the upstream
2MB split build.

> 
>> $ build \
>>   -b DEBUG \
>>   -a IA32 -a X64 \
>>   -p OvmfPkg/OvmfPkgIa32X64.dsc \
>>   -t GCC48 \
>>   -D SMM_REQUIRE \
>>   -D SECURE_BOOT_ENABLE \
>>   -D HTTP_BOOT_ENABLE \
>>   -D NETWORK_IP6_ENABLE \
>>   -D TLS_ENABLE
> 
> Do you enable the last 3 in your production builds? I didn't think it
> was the case, but it would change things...

That's a very good question, and I expected it.

Any sane person being responsible for supporting a package will strive
very hard to minimize the features enabled in the package, in order to
minimize the problem surface / support burden. I tend to consider myself
a sane person, so no, HTTP_BOOT_ENABLE, NETWORK_IP6_ENABLE, and
TLS_ENABLE are not turned on.

(TLS_ENABLE carries even more weight, because it increases the security
attack surface, so turning *that* off is very desirable.)

*But*, I certainly want to keep the *ability* to turn these features on
(and maybe later features, in 2-3 years' time) if a customer or a
partner requests it.

In short, I want to increase my safety margin for supportability as much
as possible, both by minimizing the current feature set and by
maximizing the space reserved for future features.

> 
>>>
>>> Regarding how to 'upgrade' a system from using the smaller storage
>>> size, I don't think there are any good answers. (Which also applies to
>>> the 4MB case.)
>>
>> I agree, on both points.
> 
> How would you plan to support VMs with the old 2MB layout?

That's the point: I don't. I can choose not to support it, beacuse that
layout can be left behind with RHEL-7.3, in which OVMF was still Tech
Preview.

This will likely change in RHEL-7.4 -- this shouldn't be considered a
public promise --, and once that happens, any such change will require
us to:

- add another firmware binary to the OVMF package, built for the new layout,
- add a matching varstore template,
- teach all the layered products (libvirt, prominently) about the new
firmware binary (so that new VMs / new machine types would default to that),
- keep the older fw binaries around and updated forever (until the major
release reaches EOL) beause VMs created earlier depend on them.

I think it's understandable why we don't want to do this. Going to 4MB
does not *guarantee* we won't have to do this, but it does add a lot of
safety.

> 
> I guess it could be easy enough to develop a python script to resize
> the old layout to the new layout, but I'm not sure if it is possible
> for libvirt to handle the need to launch such an upgrade script..

Actually, both issues are hard.

(1) The "resizer" would be able to handle three layers of abstractions
in the varstore file (the QEMU flash drive representation, the FTW
layer, and the variable layer). For example, if you "lose power" (i.e.,
forcibly power down a VM) while it is executing Variable Reclaim, your
varstore would be in an inconsistent / unfinished state (the live area,
the FTW working block, and the spare area would reflect this). Now, the
edk2 driver stack handles this *very* robustly (this is why FTW has been
invented in the first place!), so on next VM restart, everything would
recover. *But*, the resizer would have to handle this situation just as
well -- it would have to resize the varstore file in a manner that the
interrupted reclaim can be resumed / restarted after resizing and VM reboot.

I definitely don't want to write or maintain such a program.

(2) Yes, converting varstore files on OVMF / libvirt / etc package
updates is not viable. Such updates (for both OVMF and libvirt) are
generally permitted while virtual machines are running. The OVMF binary
can be replaced without issues (it is not rewritten in-place, but
deleted and recreated as a new file by RPM, AIUI), plus, even if it were
rewritten in place, I'd verified that the pflash chip for the fw binary
is only loaded at QEMU startup.

However, converting an in-use varstore file is plain impossible
(assuming but not conceding that (1) was covered somehow).

> 
>> Which is why I think it's time to make the big jump now, and be safe for
>> the next several years.
> 
> Why not just go for 8 MB, and give Microsoft, say 1 MB for variables?
> That should be 'future proof', right? :)

I expected this example as well :)

I didn't want to go 8MB all at once because I didn't want to exhaust
those 8MBs presently allowed by QEMU all at once. Using 8MB seems
overkill at this point.

Importantly, I don't claim that your point about overkill is *generally*
invalid. I fully agree that overkill exists. I just put the sweet size
elsewhere than you do. To me, 512KB / 4MB appear a comfortable safety
margin for OVMF, considering the RHEL7 lifecycle, while leaving 4MB free
in QEMU for whatever use case might come up in the future.

But, if you really prefer larger than 4MB, I can do that. :)

> The reality is that there's no good way to tell what Microsoft (or Red
> Hat) may require in the future.

Correct. There's no way to guarantee anything, but we can (and should)
plan for the future.

> Right now we know that Microsoft
> appears to be saying 120k is good for at least the near term.

"Near term" is meaningless for an enterprise distro that's been released
for ~3 years and is about to get ~7 more years of support, and is
planning to introduce full support for a new package.

I linked the message
<http://mid.mail-archive.com/24f63595e68c476eb98cf87e7abfa1bc@BL2PR03MB242.namprd03.prod.outlook.com>
earlier, in which Larry Cleeton @ MS stated, in February 2014, that
Hyper-V still considered 128KB generous.

Fast forward cca. *two and half* years (Sept 2016), and Microsoft's UEFI
plugfest presentation (which you linked, thank you) says,

  Windows Hardware Compatibility Requirements for RS2
  ...
  A total of at least 120 KB of non-volatile NVRAM storage
  memory must be available for NV UEFI variables (authenticated and
  unauthenticated, BS and RT) used by UEFI Secure Boot and Windows.
  ...

The ~120KB varstore size got just demoted, from a "generous" amount to a
required minimum, in the time frame of 2.5 years.

We can't guarantee 512KB will be enough for the next 7 years, but
anything less than that seems short-sighted to me.

> 
> I'm not against us defining a 4MB image size, but it is frustrating to
> be pushed into it by a single test.

Tell me about it.

> You did give an example of a crash
> dump using 10k of variable space, but even then it is not clear to me
> that 56k is insufficient in normal usage.

For normal usage, 56K is okay-ish. Not generous by any means, but sort
of acceptable. However, passing SVVP with OVMF is really-really
important to us, and I reckon other enterprise distros that ship OVMF
might feel the same.

> Regarding your suggested 4MB layout, it seems reasonable. My only
> concern is, if the minimum flash size is bumped.

(ITYM "if the minimum varstore size requirement is bumped".)

> What are the chances
> that 248k will cover it? Unfortunately (or fortunately), no one I've
> asked seem to know of any plans for the requirement to increase beyond
> 120k.

Again, I agree this is a valid concern; there's no way to guarantee
248KB will be enough for 7 years (I keep saying 7 years, that's
important to me, others might care about a different time frame).

Given that 120-128KB has gone from "generous" to "required" in 2.5
years, I feel that providing 248KB for the next 7 years (thereby more
than quadrupling OVMF's current offer) is the low water mark for "prudent".

If you'd like us to go, say, 504KB live varstore, 1MB NV, and 5MB or 6MB
total, I'm game.

Thanks!
Laszlo

Re: [PATCH 2/3] OvmfPkg: introduce FD_SIZE_4MB (mainly) for Windows HCK

[Jordan Justen]

On 2017-05-01 03:51:42, Laszlo Ersek wrote:
> On 04/30/17 23:16, Jordan Justen wrote:
> > What are the chances
> > that 248k will cover it? Unfortunately (or fortunately), no one I've
> > asked seem to know of any plans for the requirement to increase beyond
> > 120k.
> 
> Again, I agree this is a valid concern; there's no way to guarantee
> 248KB will be enough for 7 years (I keep saying 7 years, that's
> important to me, others might care about a different time frame).
> 
> Given that 120-128KB has gone from "generous" to "required" in 2.5
> years, I feel that providing 248KB for the next 7 years (thereby more
> than quadrupling OVMF's current offer) is the low water mark for "prudent".

Since we are trying to predict the future, I don't think we should
stop just shy of a power-of-two. I don't know where the 120k number
comes from, but I don't think we should bet on the next power-of-two
minus 8k.

Regarding the event log, it has been ignored in OVMF, but I'd like to
ask around a bit to see if it could be useful to have more than 4k.

> If you'd like us to go, say, 504KB live varstore, 1MB NV, and 5MB or 6MB
> total, I'm game.

Uh, no. I'd rather go with 128k, and I'd also rather stay with 2MB. :)

-Jordan

Re: [PATCH 2/3] OvmfPkg: introduce FD_SIZE_4MB (mainly) for Windows HCK

[Jordan Justen]

On 2017-05-01 03:51:42, Laszlo Ersek wrote:
> On 04/30/17 23:16, Jordan Justen wrote:
> > On 2017-04-30 07:42:36, Laszlo Ersek wrote:
> > 
> >> $ build \
> >>   -b DEBUG \
> >>   -a IA32 -a X64 \
> >>   -p OvmfPkg/OvmfPkgIa32X64.dsc \
> >>   -t GCC48 \
> >>   -D SMM_REQUIRE \
> >>   -D SECURE_BOOT_ENABLE \
> >>   -D HTTP_BOOT_ENABLE \
> >>   -D NETWORK_IP6_ENABLE \
> >>   -D TLS_ENABLE
> > 
> > Do you enable the last 3 in your production builds? I didn't think it
> > was the case, but it would change things...
> 
> That's a very good question, and I expected it.
> 
> Any sane person being responsible for supporting a package will strive
> very hard to minimize the features enabled in the package, in order to
> minimize the problem surface / support burden. I tend to consider myself
> a sane person, so no, HTTP_BOOT_ENABLE, NETWORK_IP6_ENABLE, and
> TLS_ENABLE are not turned on.
> 
> (TLS_ENABLE carries even more weight, because it increases the security
> attack surface, so turning *that* off is very desirable.)
> 
> *But*, I certainly want to keep the *ability* to turn these features on
> (and maybe later features, in 2-3 years' time) if a customer or a
> partner requests it.

It sounds like you don't expect to 'support' this. At least not to the
same level as the rest of the firmware.

I think it is fine to say, if you want to enable these, you may have
to disable debug on some other features, or remove some other
features.

In other words, at this point I don't think the size of these should
be added into the equation for how 'full' the 2MB image is.

-Jordan

Re: [PATCH 2/3] OvmfPkg: introduce FD_SIZE_4MB (mainly) for Windows HCK

[Laszlo Ersek]

On 05/01/17 19:23, Jordan Justen wrote:
> On 2017-05-01 03:51:42, Laszlo Ersek wrote:
>> On 04/30/17 23:16, Jordan Justen wrote:
>>> On 2017-04-30 07:42:36, Laszlo Ersek wrote:
>>>
>>>> $ build \
>>>>   -b DEBUG \
>>>>   -a IA32 -a X64 \
>>>>   -p OvmfPkg/OvmfPkgIa32X64.dsc \
>>>>   -t GCC48 \
>>>>   -D SMM_REQUIRE \
>>>>   -D SECURE_BOOT_ENABLE \
>>>>   -D HTTP_BOOT_ENABLE \
>>>>   -D NETWORK_IP6_ENABLE \
>>>>   -D TLS_ENABLE
>>>
>>> Do you enable the last 3 in your production builds? I didn't think it
>>> was the case, but it would change things...
>>
>> That's a very good question, and I expected it.
>>
>> Any sane person being responsible for supporting a package will strive
>> very hard to minimize the features enabled in the package, in order to
>> minimize the problem surface / support burden. I tend to consider myself
>> a sane person, so no, HTTP_BOOT_ENABLE, NETWORK_IP6_ENABLE, and
>> TLS_ENABLE are not turned on.
>>
>> (TLS_ENABLE carries even more weight, because it increases the security
>> attack surface, so turning *that* off is very desirable.)
>>
>> *But*, I certainly want to keep the *ability* to turn these features on
>> (and maybe later features, in 2-3 years' time) if a customer or a
>> partner requests it.
> 
> It sounds like you don't expect to 'support' this. At least not to the
> same level as the rest of the firmware.

I hope never to have to support these, but at some point into the future
of RHEL7, I might have no choice.

> I think it is fine to say, if you want to enable these, you may have
> to disable debug on some other features,

As I explained earlier, universal DEBUG coverage is a requirement for
supportability.

(

Example: in parallel to this discussion, our virt QE reported an
independent issue, namely an out-of-SMRAM condition with 240+ VCPUs.

https://bugzilla.redhat.com/show_bug.cgi?id=1447027

The out-of-SMRAM condition is caught by an ASSERT() only. It is in
"UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c", function SetStaticPageTable():

    212           PageDirectoryEntry = AllocatePageTableMemory (1);
    213           ASSERT(PageDirectoryEntry != NULL);
    214           ZeroMem (PageDirectoryEntry, EFI_PAGES_TO_SIZE(1));

I find it plausible that if this memory allocation fails, then the
firmware has really no way to continue -- that's okay. But, the ASSERT()
disappears in a RELEASE build -- not okay. Memory allocation failures
should never be handled with *just* an ASSERT().

I hope you appreciate my insistence on DEBUG a bit more, through this
real-life example.

)

> or remove some other features.

Removing features on purpose can be called "offensive regression" in an
enterprise distro, and it is the antithesis of why people decide to pay
for enterprise support.

> In other words, at this point I don't think the size of these should
> be added into the equation for how 'full' the 2MB image is.

I think at this point it is clear that these patches are not appropriate
for upstream OvmfPkg. That's an acceptable closure for me -- while I
would have preferred to get the patches in, the real moral imperative
for me is to *try* the upstreaming at least, whenever I think the
patches are applicable to upstream. I think we've had an honest and
thorough discussion on this -- thank you very much for taking the time!
(Especially over the weekend!)

The current upstream 2MB build should continue working for everyday
purposes.

Cheers
Laszlo

Re: [PATCH 2/3] OvmfPkg: introduce FD_SIZE_4MB (mainly) for Windows HCK

[Jordan Justen]

On 2017-05-01 11:40:58, Laszlo Ersek wrote:
> On 05/01/17 19:23, Jordan Justen wrote:
> > On 2017-05-01 03:51:42, Laszlo Ersek wrote:
> >> On 04/30/17 23:16, Jordan Justen wrote:
> >>> On 2017-04-30 07:42:36, Laszlo Ersek wrote:
> >>>
> >>>> $ build \
> >>>>   -b DEBUG \
> >>>>   -a IA32 -a X64 \
> >>>>   -p OvmfPkg/OvmfPkgIa32X64.dsc \
> >>>>   -t GCC48 \
> >>>>   -D SMM_REQUIRE \
> >>>>   -D SECURE_BOOT_ENABLE \
> >>>>   -D HTTP_BOOT_ENABLE \
> >>>>   -D NETWORK_IP6_ENABLE \
> >>>>   -D TLS_ENABLE
> >>>
> >>> Do you enable the last 3 in your production builds? I didn't think it
> >>> was the case, but it would change things...
> >>
> >> That's a very good question, and I expected it.
> >>
> >> Any sane person being responsible for supporting a package will strive
> >> very hard to minimize the features enabled in the package, in order to
> >> minimize the problem surface / support burden. I tend to consider myself
> >> a sane person, so no, HTTP_BOOT_ENABLE, NETWORK_IP6_ENABLE, and
> >> TLS_ENABLE are not turned on.
> >>
> >> (TLS_ENABLE carries even more weight, because it increases the security
> >> attack surface, so turning *that* off is very desirable.)
> >>
> >> *But*, I certainly want to keep the *ability* to turn these features on
> >> (and maybe later features, in 2-3 years' time) if a customer or a
> >> partner requests it.
> > 
> > It sounds like you don't expect to 'support' this. At least not to the
> > same level as the rest of the firmware.
> 
> I hope never to have to support these, but at some point into the future
> of RHEL7, I might have no choice.
> 
> > I think it is fine to say, if you want to enable these, you may have
> > to disable debug on some other features,
> 
> As I explained earlier, universal DEBUG coverage is a requirement for
> supportability.
>

The DEBUG for everything is *your* requirement. I'm fine to work with
that on the standard set of features.

But, you've already admitted that these are not features you currently
support, or plan to anytime soon. So, don't hold them up as must have
features when you are not even using them.

At this I'd just like figure out what to do about the 4MB layout, so
can we stop getting worked up over this side show? Obviously you are
going to start using the new 4MB layout, and everything will fit
easily with debug there.

> > In other words, at this point I don't think the size of these should
> > be added into the equation for how 'full' the 2MB image is.
> 
> I think at this point it is clear that these patches are not appropriate
> for upstream OvmfPkg.

I don't know how you came to this conclusion.

I think at this point you've convinced (heh) me that we should figure
out a 4MB layout, and obviously it'd be better to have a single 4MB
layout. (See other email.)

-Jordan

Re: [PATCH 2/3] OvmfPkg: introduce FD_SIZE_4MB (mainly) for Windows HCK

[Laszlo Ersek]

On 05/01/17 21:20, Jordan Justen wrote:
> On 2017-05-01 11:40:58, Laszlo Ersek wrote:
>> On 05/01/17 19:23, Jordan Justen wrote:
>>> On 2017-05-01 03:51:42, Laszlo Ersek wrote:
>>>> On 04/30/17 23:16, Jordan Justen wrote:
>>>>> On 2017-04-30 07:42:36, Laszlo Ersek wrote:
>>>>>
>>>>>> $ build \
>>>>>>   -b DEBUG \
>>>>>>   -a IA32 -a X64 \
>>>>>>   -p OvmfPkg/OvmfPkgIa32X64.dsc \
>>>>>>   -t GCC48 \
>>>>>>   -D SMM_REQUIRE \
>>>>>>   -D SECURE_BOOT_ENABLE \
>>>>>>   -D HTTP_BOOT_ENABLE \
>>>>>>   -D NETWORK_IP6_ENABLE \
>>>>>>   -D TLS_ENABLE
>>>>>
>>>>> Do you enable the last 3 in your production builds? I didn't think it
>>>>> was the case, but it would change things...
>>>>
>>>> That's a very good question, and I expected it.
>>>>
>>>> Any sane person being responsible for supporting a package will strive
>>>> very hard to minimize the features enabled in the package, in order to
>>>> minimize the problem surface / support burden. I tend to consider myself
>>>> a sane person, so no, HTTP_BOOT_ENABLE, NETWORK_IP6_ENABLE, and
>>>> TLS_ENABLE are not turned on.
>>>>
>>>> (TLS_ENABLE carries even more weight, because it increases the security
>>>> attack surface, so turning *that* off is very desirable.)
>>>>
>>>> *But*, I certainly want to keep the *ability* to turn these features on
>>>> (and maybe later features, in 2-3 years' time) if a customer or a
>>>> partner requests it.
>>>
>>> It sounds like you don't expect to 'support' this. At least not to the
>>> same level as the rest of the firmware.
>>
>> I hope never to have to support these, but at some point into the future
>> of RHEL7, I might have no choice.
>>
>>> I think it is fine to say, if you want to enable these, you may have
>>> to disable debug on some other features,
>>
>> As I explained earlier, universal DEBUG coverage is a requirement for
>> supportability.
>>
> 
> The DEBUG for everything is *your* requirement.

Sure.

> I'm fine to work with
> that on the standard set of features.

Above when you wrote

  I think it is fine to say, if you want to enable these, you may have
  to disable debug on some other features

did you not mean that for *my* use case (emphasis yours)?

> 
> But, you've already admitted that these are not features you currently
> support, or plan to anytime soon.

I'm wary of future "offers" in the area that I possibly "can't refuse".
And when talking about area reservations, that is the same thing as
"wanting to support it real hard right now".

(Also, I didn't "admit" it. I confirmed it. I didn't mis-represent or
hide my use case, and you didn't catch me in any such
mis-representation, so there was nothing for me to admit.)

> So, don't hold them up as must have
> features when you are not even using them.

"In two years, I'm possibly buying a really large fridge. I don't want
that fridge, but my kids can eat a lot, and in two years, they are going
to eat even more (growing up!). So, if it's not a big burden, let's make
the kitchen door big enough now while we are remodeling anyway for the
fridge to fit through."

"Don't hold up that fridge as a must have when you are not even using it."

> 
> At this I'd just like figure out what to do about the 4MB layout, so
> can we stop getting worked up over this side show?

Thanks for calling it a side show, real friendly.

Also, I haven't noticed myself getting worked up, but now we're on a
good path to that.

> Obviously you are
> going to start using the new 4MB layout, and everything will fit
> easily with debug there.

Yes. And the one thing you've failed to express clearly until now is why
exactly you dislike the exact size increases in the patch.

> 
>>> In other words, at this point I don't think the size of these should
>>> be added into the equation for how 'full' the 2MB image is.
>>
>> I think at this point it is clear that these patches are not appropriate
>> for upstream OvmfPkg.
> 
> I don't know how you came to this conclusion.

After several rounds of discussion, you still prefer ~128KB of varstore,
which is barely above the latest minimum requirements. Being frugal is
good in general (see e.g. one of my own arguments here:
<http://mid.mail-archive.com/56F40D1A.50801@redhat.com>). *But*, in this
case, every time we're going to grow the varstore, compatibility will
break. So it makes sense to go beyond the exact current requirement and
buy some time until the next breakage. Decrease the frequency of
breakage if you will. The desired amount of time bought differs, of
course. I can't predict the future but would like to aim at a large
margin, with ~7 years. You can't predict the future but would like to
aim at a zero margin. I've come to equate your preference with what's
appropriate for upstream. Therefore the patches I posted aren't. QED.

> I think at this point you've convinced (heh) me that we should figure
> out a 4MB layout, and obviously it'd be better to have a single 4MB
> layout. (See other email.)

The only email (that I can see) in this thread that I haven't reacted to is:

http://mid.mail-archive.com/149365894632.25909.11739243410891079091@jljusten-skl

where you wrote "I'd rather go with 128k, and I'd also rather stay with
2MB".

Laszlo

Re: [PATCH 2/3] OvmfPkg: introduce FD_SIZE_4MB (mainly) for Windows HCK

[Jordan Justen]

On 2017-05-01 16:07:48, Laszlo Ersek wrote:
> On 05/01/17 21:20, Jordan Justen wrote:
> > 
> > At this I'd just like figure out what to do about the 4MB layout, so
> > can we stop getting worked up over this side show?
> 
> Thanks for calling it a side show, real friendly.

Ok, I apologize. The 'side show' comment applies equally to me. Can we
please just move on? We obviously disagree how to determine how full
2MB is, but it doesn't much matter since you'll soon be abandoning it
entirely. We have too much room in the 4MB fridge to be concerned
about this.

> The only email (that I can see) in this thread that I haven't reacted to is:
> 
> http://mid.mail-archive.com/149365894632.25909.11739243410891079091@jljusten-skl
> 
> where you wrote "I'd rather go with 128k, and I'd also rather stay with
> 2MB".

You dropped my ":)" :)

Ok, me adding that was a poor choice, despite including a smiley. I
meant to convey was: "Ok, fine, let's figure out the 4MB layout, but I
still want to whine about it a little more."

-Jordan

Re: [PATCH 2/3] OvmfPkg: introduce FD_SIZE_4MB (mainly) for Windows HCK

[Laszlo Ersek]

On 05/02/17 01:38, Jordan Justen wrote:
> On 2017-05-01 16:07:48, Laszlo Ersek wrote:
>> On 05/01/17 21:20, Jordan Justen wrote:
>>>
>>> At this I'd just like figure out what to do about the 4MB layout, so
>>> can we stop getting worked up over this side show?
>>
>> Thanks for calling it a side show, real friendly.
> 
> Ok, I apologize. The 'side show' comment applies equally to me. Can we
> please just move on? We obviously disagree how to determine how full
> 2MB is, but it doesn't much matter since you'll soon be abandoning it
> entirely. We have too much room in the 4MB fridge to be concerned
> about this.

OK.

I wouldn't mind if we made more room for the varstore in the 2MB build,
even at the expense of FVMAIN_COMPACT, if we also kept the current 2MB
build the default, so that the "new" (incompatible) 2MB build doesn't
come as a surprise to unsuspecting downstreams.

Regarding the 4MB build:
- we can discuss that on top of the above "new" 2MB build,
- we can discuss it instead of the above "new" 2MB build,
- we can postpone it for now, for upstream.

If you do agree that a 4MB build should be offered in upstream, then I'm
proposing my proposal (obviously :) ). If your main focus is the "new"
2MB build, and beause mine is the 4MB build, perhaps we aren't even
disagreeing as much, since this doesn't have to be an either-or. If you
have specific observations for the 4MB structure I proposed, I'd be glad
to hear those as well.

> 
>> The only email (that I can see) in this thread that I haven't reacted to is:
>>
>> http://mid.mail-archive.com/149365894632.25909.11739243410891079091@jljusten-skl
>>
>> where you wrote "I'd rather go with 128k, and I'd also rather stay with
>> 2MB".
> 
> You dropped my ":)" :)
> 
> Ok, me adding that was a poor choice, despite including a smiley. I
> meant to convey was: "Ok, fine, let's figure out the 4MB layout, but I
> still want to whine about it a little more."

I'm sorry that my irony-meter failed (I hope this isn't a poor choice of
words); the downstream deadlines are really close, I'd been working day
and night over the weekend, and I mistook the smiley for "I'll never
agree with you on this, but here's a smiley to take the edge off".

Thanks,
Laszlo

Re: [PATCH 2/3] OvmfPkg: introduce FD_SIZE_4MB (mainly) for Windows HCK

[Jordan Justen]

On 2017-05-02 07:39:04, Laszlo Ersek wrote:
> 
> I wouldn't mind if we made more room for the varstore in the 2MB build,
> even at the expense of FVMAIN_COMPACT, if we also kept the current 2MB
> build the default, so that the "new" (incompatible) 2MB build doesn't
> come as a surprise to unsuspecting downstreams.
> 
> Regarding the 4MB build:
> - we can discuss that on top of the above "new" 2MB build,
> - we can discuss it instead of the above "new" 2MB build,
> - we can postpone it for now, for upstream.

I was hoping there was a way to avoid the need to add 4MB, but you
needing to support the layout until 2024 really adds risk to the 2MB
image. I think there is a decent chance 2MB would work until then, but
I can also see how it adds significant risk.

If we are adding the 4MB layout, then we may as well make it the
default for debug builds. I'm not sure what to do about 2MB then. In
the short term, I say we do nothing. Later, after 4MB is established
as the default, maybe we continue to do nothing, or perhaps resize the
varstore to 120~128k, or perhaps just remove the layout entirely.

> If you do agree that a 4MB build should be offered in upstream, then I'm
> proposing my proposal (obviously :) ). If your main focus is the "new"
> 2MB build, and beause mine is the 4MB build, perhaps we aren't even
> disagreeing as much, since this doesn't have to be an either-or. If you
> have specific observations for the 4MB structure I proposed, I'd be glad
> to hear those as well.

I feel fairly confident of the 4MB image supporting your code size
needs until 2024. What seems less certain in future varstore
requirements. Right now the requirement is 120~128k. I think rather
than 248k in the 4MB layout, we should make it 256k. (Since these
kinds of things often progress in powers-of-two.) It will make for a
couple unfriendly offsets, but it seems worth it to avoid being 8k shy
of the next power-of-two size.

In my other email, I mentioned the event-log. I did ask around a bit
about this, but I didn't find anyone willing to fight for more space.
Therefore, I think we should just keep it at 4k.

-Jordan

Re: [PATCH 2/3] OvmfPkg: introduce FD_SIZE_4MB (mainly) for Windows HCK

[Laszlo Ersek]

On 05/02/17 20:22, Jordan Justen wrote:
> On 2017-05-02 07:39:04, Laszlo Ersek wrote:
>>
>> I wouldn't mind if we made more room for the varstore in the 2MB build,
>> even at the expense of FVMAIN_COMPACT, if we also kept the current 2MB
>> build the default, so that the "new" (incompatible) 2MB build doesn't
>> come as a surprise to unsuspecting downstreams.
>>
>> Regarding the 4MB build:
>> - we can discuss that on top of the above "new" 2MB build,
>> - we can discuss it instead of the above "new" 2MB build,
>> - we can postpone it for now, for upstream.
> 
> I was hoping there was a way to avoid the need to add 4MB, but you
> needing to support the layout until 2024 really adds risk to the 2MB
> image. I think there is a decent chance 2MB would work until then, but
> I can also see how it adds significant risk.
> 
> If we are adding the 4MB layout, then we may as well make it the
> default for debug builds.

OK, I think that's technically doable. Based on your commit e3dca1859b24
("OvmfPkg: Increase default RELEASE build image size to 2MB",
2016-01-29), the $(TARGET) macro can be used in FDF files.

> I'm not sure what to do about 2MB then. In
> the short term, I say we do nothing.

Do you mean "do nothing about 2MB", or "do nothing at all in the fdf.inc"?

(You have to be really specific with me these days, sorry...)

If I understand correctly, we'd have to reinstate FD_SIZE_2MB then, so
that people that want to stick with the 2MB build for DEBUG (and NOOPT)
can select it. Given that 4MB would become the new default for those
targets.

> Later, after 4MB is established
> as the default,

... "for DEBUG/NOOPT", I assume...

> maybe we continue to do nothing,

... "with the 2MB build", I assume...

> or perhaps resize the
> varstore to 120~128k,

... "in the 2MB build", I assume...

> or perhaps just remove the layout entirely.

... "the 2MB build", I assume...

Adding FD_SIZE_4MB as a new default (for DEBUG & NOOPT), but also
permitting an explicit FD_SIZE_2MB selection, would be fine IMO.

I can update the series like this (patch #2 would see only comment
updates, and there would be an addtional patch for the DSC files.)

> 
>> If you do agree that a 4MB build should be offered in upstream, then I'm
>> proposing my proposal (obviously :) ). If your main focus is the "new"
>> 2MB build, and beause mine is the 4MB build, perhaps we aren't even
>> disagreeing as much, since this doesn't have to be an either-or. If you
>> have specific observations for the 4MB structure I proposed, I'd be glad
>> to hear those as well.
> 
> I feel fairly confident of the 4MB image supporting your code size
> needs until 2024. What seems less certain in future varstore
> requirements. Right now the requirement is 120~128k. I think rather
> than 248k in the 4MB layout, we should make it 256k. (Since these
> kinds of things often progress in powers-of-two.) It will make for a
> couple unfriendly offsets, but it seems worth it to avoid being 8k shy
> of the next power-of-two size.
> 
> In my other email, I mentioned the event-log. I did ask around a bit
> about this, but I didn't find anyone willing to fight for more space.
> Therefore, I think we should just keep it at 4k.

That means 256K for the varstore, 4K for the event log, 4K for the FTW
working block.

How much for the spare area? Currently the spare area equals the sum of
the former three. The spare area is used both while reclaiming the
varstore, and while reclaiming the FTW working block. (Not sure about
the event log.) So I'd say we should stick with our tradition, and make
the spare area 256K + 4K + 4K = 264K in size. That would result in a
528K NVRAM. (Which is 16K larger than in the current patch.)

In turn, I wouldn't increase FVMAIN_COMPACT by 1664K, to 3376K, but by
16K less (1648K) to 3360K. The full FD size would remain 4M.

Does this sound okay?

Thanks
Laszlo

Re: [PATCH 2/3] OvmfPkg: introduce FD_SIZE_4MB (mainly) for Windows HCK

[Jordan Justen]

On 2017-05-02 12:31:39, Laszlo Ersek wrote:
> On 05/02/17 20:22, Jordan Justen wrote:
> > On 2017-05-02 07:39:04, Laszlo Ersek wrote:
> >>
> >> I wouldn't mind if we made more room for the varstore in the 2MB build,
> >> even at the expense of FVMAIN_COMPACT, if we also kept the current 2MB
> >> build the default, so that the "new" (incompatible) 2MB build doesn't
> >> come as a surprise to unsuspecting downstreams.
> >>
> >> Regarding the 4MB build:
> >> - we can discuss that on top of the above "new" 2MB build,
> >> - we can discuss it instead of the above "new" 2MB build,
> >> - we can postpone it for now, for upstream.
> > 
> > I was hoping there was a way to avoid the need to add 4MB, but you
> > needing to support the layout until 2024 really adds risk to the 2MB
> > image. I think there is a decent chance 2MB would work until then, but
> > I can also see how it adds significant risk.
> > 
> > If we are adding the 4MB layout, then we may as well make it the
> > default for debug builds.
> 
> OK, I think that's technically doable. Based on your commit e3dca1859b24
> ("OvmfPkg: Increase default RELEASE build image size to 2MB",
> 2016-01-29), the $(TARGET) macro can be used in FDF files.
> 
> > I'm not sure what to do about 2MB then. In
> > the short term, I say we do nothing.
> 
> Do you mean "do nothing about 2MB", or "do nothing at all in the fdf.inc"?
> 
> (You have to be really specific with me these days, sorry...)
> 
> If I understand correctly, we'd have to reinstate FD_SIZE_2MB then, so
> that people that want to stick with the 2MB build for DEBUG (and NOOPT)
> can select it. Given that 4MB would become the new default for those
> targets.

Ah. I guess I dropped FD_SIZE_2MB in e3dca1859b24, which I don't think
I should have done. Going forward, I think we should allow
FD_SIZE_1/2/4MB.

Regarding RELEASE builds, I'm not sure what we should do. Should we
just change it to 4MB as well? In the past, I preferred to allow
release builds to use the smaller size, since it fit. But, in this
case we also know that leaving 2MB size will mean a known test will
fail. The test failing doesn't mean a real user is likely to be
impacted, but I guess Microsoft feels the larger size may be required
in some scenarios.

What do you think? (Maybe not a fair question since you don't use the
release build.) I guess the safe option is to just bump the default
for both the debug and release builds to the ridiculously large (er, I
mean luxuriously spacious :) 4MB image.

> > I feel fairly confident of the 4MB image supporting your code size
> > needs until 2024. What seems less certain in future varstore
> > requirements. Right now the requirement is 120~128k. I think rather
> > than 248k in the 4MB layout, we should make it 256k. (Since these
> > kinds of things often progress in powers-of-two.) It will make for a
> > couple unfriendly offsets, but it seems worth it to avoid being 8k shy
> > of the next power-of-two size.
> > 
> > In my other email, I mentioned the event-log. I did ask around a bit
> > about this, but I didn't find anyone willing to fight for more space.
> > Therefore, I think we should just keep it at 4k.
> 
> That means 256K for the varstore, 4K for the event log, 4K for the FTW
> working block.
> 
> How much for the spare area? Currently the spare area equals the sum of
> the former three. The spare area is used both while reclaiming the
> varstore, and while reclaiming the FTW working block. (Not sure about
> the event log.) So I'd say we should stick with our tradition, and make
> the spare area 256K + 4K + 4K = 264K in size. That would result in a
> 528K NVRAM. (Which is 16K larger than in the current patch.)
> 
> In turn, I wouldn't increase FVMAIN_COMPACT by 1664K, to 3376K, but by
> 16K less (1648K) to 3360K. The full FD size would remain 4M.
> 
> Does this sound okay?

Yes.

This will leave the split rom sizes being a multiple of 16k rather
than 512k. Today they are a multiple of 128k. I don't expect this
would be an issue for qemu/kvm. Do you agree?

-Jordan

Re: [PATCH 2/3] OvmfPkg: introduce FD_SIZE_4MB (mainly) for Windows HCK

[Laszlo Ersek]

On 05/02/17 23:45, Jordan Justen wrote:
> On 2017-05-02 12:31:39, Laszlo Ersek wrote:
>> On 05/02/17 20:22, Jordan Justen wrote:
>>> On 2017-05-02 07:39:04, Laszlo Ersek wrote:
>>>>
>>>> I wouldn't mind if we made more room for the varstore in the 2MB build,
>>>> even at the expense of FVMAIN_COMPACT, if we also kept the current 2MB
>>>> build the default, so that the "new" (incompatible) 2MB build doesn't
>>>> come as a surprise to unsuspecting downstreams.
>>>>
>>>> Regarding the 4MB build:
>>>> - we can discuss that on top of the above "new" 2MB build,
>>>> - we can discuss it instead of the above "new" 2MB build,
>>>> - we can postpone it for now, for upstream.
>>>
>>> I was hoping there was a way to avoid the need to add 4MB, but you
>>> needing to support the layout until 2024 really adds risk to the 2MB
>>> image. I think there is a decent chance 2MB would work until then, but
>>> I can also see how it adds significant risk.
>>>
>>> If we are adding the 4MB layout, then we may as well make it the
>>> default for debug builds.
>>
>> OK, I think that's technically doable. Based on your commit e3dca1859b24
>> ("OvmfPkg: Increase default RELEASE build image size to 2MB",
>> 2016-01-29), the $(TARGET) macro can be used in FDF files.
>>
>>> I'm not sure what to do about 2MB then. In
>>> the short term, I say we do nothing.
>>
>> Do you mean "do nothing about 2MB", or "do nothing at all in the fdf.inc"?
>>
>> (You have to be really specific with me these days, sorry...)
>>
>> If I understand correctly, we'd have to reinstate FD_SIZE_2MB then, so
>> that people that want to stick with the 2MB build for DEBUG (and NOOPT)
>> can select it. Given that 4MB would become the new default for those
>> targets.
> 
> Ah. I guess I dropped FD_SIZE_2MB in e3dca1859b24, which I don't think
> I should have done. Going forward, I think we should allow
> FD_SIZE_1/2/4MB.

Agreed.

> 
> Regarding RELEASE builds, I'm not sure what we should do. Should we
> just change it to 4MB as well? In the past, I preferred to allow
> release builds to use the smaller size, since it fit. But, in this
> case we also know that leaving 2MB size will mean a known test will
> fail. The test failing doesn't mean a real user is likely to be
> impacted, but I guess Microsoft feels the larger size may be required
> in some scenarios.
> 
> What do you think? (Maybe not a fair question since you don't use the
> release build.) I guess the safe option is to just bump the default
> for both the debug and release builds to the ridiculously large (er, I
> mean luxuriously spacious :) 4MB image.

Will do that in v2. (Maybe we should call the 4MB build
FD_SIZE_LUXURIOUS_FRIDGE. :) )

> 
>>> I feel fairly confident of the 4MB image supporting your code size
>>> needs until 2024. What seems less certain in future varstore
>>> requirements. Right now the requirement is 120~128k. I think rather
>>> than 248k in the 4MB layout, we should make it 256k. (Since these
>>> kinds of things often progress in powers-of-two.) It will make for a
>>> couple unfriendly offsets, but it seems worth it to avoid being 8k shy
>>> of the next power-of-two size.
>>>
>>> In my other email, I mentioned the event-log. I did ask around a bit
>>> about this, but I didn't find anyone willing to fight for more space.
>>> Therefore, I think we should just keep it at 4k.
>>
>> That means 256K for the varstore, 4K for the event log, 4K for the FTW
>> working block.
>>
>> How much for the spare area? Currently the spare area equals the sum of
>> the former three. The spare area is used both while reclaiming the
>> varstore, and while reclaiming the FTW working block. (Not sure about
>> the event log.) So I'd say we should stick with our tradition, and make
>> the spare area 256K + 4K + 4K = 264K in size. That would result in a
>> 528K NVRAM. (Which is 16K larger than in the current patch.)
>>
>> In turn, I wouldn't increase FVMAIN_COMPACT by 1664K, to 3376K, but by
>> 16K less (1648K) to 3360K. The full FD size would remain 4M.
>>
>> Does this sound okay?
> 
> Yes.

Yay! \o/

> This will leave the split rom sizes being a multiple of 16k rather
> than 512k. Today they are a multiple of 128k. I don't expect this
> would be an issue for qemu/kvm. Do you agree?

I agree. In "hw/i386/pc_sysfw.c", QEMU expects the pflash chip sizes to
be multiples of 4KB (1 << 12).

I'll attempt to write, test and post v2 today.

Thank you,
Laszlo