From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id A4F222195407C for ; Sun, 30 Apr 2017 07:42:39 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id F141781138; Sun, 30 Apr 2017 14:42:38 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com F141781138 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=lersek@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com F141781138 Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-53.phx2.redhat.com [10.3.116.53]) by smtp.corp.redhat.com (Postfix) with ESMTP id 57FCE19E3D; Sun, 30 Apr 2017 14:42:37 +0000 (UTC) To: Jordan Justen , edk2-devel-01 References: <20170429201500.18496-1-lersek@redhat.com> <20170429201500.18496-3-lersek@redhat.com> <149351328512.20670.1563878734495138189@jljusten-skl> Cc: Gary Ching-Pang Lin From: Laszlo Ersek Message-ID: <030f8312-35ce-5c86-205c-2ee6c0b5ab8b@redhat.com> Date: Sun, 30 Apr 2017 16:42:36 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <149351328512.20670.1563878734495138189@jljusten-skl> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Sun, 30 Apr 2017 14:42:39 +0000 (UTC) Subject: Re: [PATCH 2/3] OvmfPkg: introduce FD_SIZE_4MB (mainly) for Windows HCK X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 30 Apr 2017 14:42:39 -0000 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit 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 . 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 >> Cc: Jordan Justen >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek >> --- >> 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 >> >>