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

[Jordan Justen <jordan.l.justen@intel.com>]

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
> 
>