From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx0a-002e3701.pphosted.com (mx0a-002e3701.pphosted.com [148.163.147.86]) by mx.groups.io with SMTP id smtpd.web12.8208.1622208787002287489 for ; Fri, 28 May 2021 06:33:07 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@hpe.com header.s=pps0720 header.b=Mr/WgDBh; spf=permerror, err=parse error for token &{10 18 %{ir}.%{v}.%{d}.spf.has.pphosted.com}: invalid domain name (domain: hpe.com, ip: 148.163.147.86, mailfrom: prvs=0782424dcb=brian.johnson@hpe.com) Received: from pps.filterd (m0134422.ppops.net [127.0.0.1]) by mx0b-002e3701.pphosted.com (8.16.0.43/8.16.0.43) with SMTP id 14SDWbOc008231; Fri, 28 May 2021 13:33:05 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=hpe.com; h=subject : to : cc : references : from : message-id : date : in-reply-to : content-type : content-transfer-encoding : mime-version; s=pps0720; bh=fWbob9TAYbsYSkssN8ruouozPHDQRggd7r1TNq2vZDU=; b=Mr/WgDBhLbUxdZYbENs31+jbK0amnHohgCWU1Ej+SqNUWZQolcQyCaP92PdloVh2SCdf bEpTkD4Z1vw3tEeONDxVaY5DFoj9V7djluERIB5AQFNPpdIa0Xsysp4ubhE5RSu6BrrN kjHSuYseGvXlRMp84cveH2G7v8ZoSx2miHP++IJiYxgAuyBRi3XTUtjqy25Yd5+evLfc FtFtZbxdmBcuZONsorxSlJh9kra6jDUQrvMub/CkEgPpOu4hwnYRc91FCPjX3CDxKwQI CslBjQlQZ4afaJIW4jtN0uWkPi7uKOHrhz054NsFu/yuhefTK+OaqtV9eN4gImW00wuX hA== Received: from g9t5008.houston.hpe.com (g9t5008.houston.hpe.com [15.241.48.72]) by mx0b-002e3701.pphosted.com with ESMTP id 38tpw3d5ex-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Fri, 28 May 2021 13:33:05 +0000 Received: from g9t2301.houston.hpecorp.net (g9t2301.houston.hpecorp.net [16.220.97.129]) by g9t5008.houston.hpe.com (Postfix) with ESMTP id 9C1A162; Fri, 28 May 2021 13:33:03 +0000 (UTC) Received: from [16.99.169.214] (unknown [16.99.169.214]) by g9t2301.houston.hpecorp.net (Postfix) with ESMTP id 74C3D4B; Fri, 28 May 2021 13:33:02 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v1] OvmfPkg: Add build options for 8MB and 16MB X64 OVMF images To: devel@edk2.groups.io, lersek@redhat.com, dbautista@newmexicoconsortium.org Cc: Brijesh Singh , Erdem Aktas , James Bottomley , Jiewen Yao , Min Xu , Tom Lendacky References: <7bfd4b82fc725302beb37e13c4a89d389c34ec34.1622048433.git.dbautista@newmexicoconsortium.org> From: "Brian J. Johnson" Message-ID: <44af78f5-dc76-3d86-6021-488dcfaff52c@hpe.com> Date: Fri, 28 May 2021 08:33:01 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 In-Reply-To: X-Proofpoint-GUID: FG_Dd1wtUwVvyd4aOoR6gLcG3iWgD43w X-Proofpoint-ORIG-GUID: FG_Dd1wtUwVvyd4aOoR6gLcG3iWgD43w X-Proofpoint-UnRewURL: 3 URL's were un-rewritten MIME-Version: 1.0 X-HPE-SCL: -1 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391,18.0.761 definitions=2021-05-28_05:2021-05-27,2021-05-28 signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 impostorscore=0 suspectscore=0 mlxscore=0 phishscore=0 bulkscore=0 priorityscore=1501 malwarescore=0 adultscore=0 spamscore=0 lowpriorityscore=0 mlxlogscore=999 clxscore=1011 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2104190000 definitions=main-2105280090 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit On 5/27/21 5:12 AM, Laszlo Ersek wrote: > On 05/26/21 19:08, Devon Bautista wrote: >> Currently, the largest volume size for building OVMF images is 4MB. With >> the growth of the Linuxboot project, maintainers have had to maintain a >> fork containing this patch which allows larger image sizes in order for >> Linuxboot developers/users to have enough space to experiment with >> and test including their own Linux kernel in the DXE section of OVMF >> firmware. Testing using OVMF is valuable since it allows testing in QEMU >> and thus does not require any hardware to do so. >> >> This patch allows specifying '-D FD_SIZE_8MB' or '-D FD_SIZE_16MB' to >> the OVMF build script in order to add the ability to build 8MB or 16MB >> x86_64 (X64) OVMF images, respectively. >> >> Signed-off-by: Devon Bautista >> --- >> OvmfPkg/OvmfPkgDefines.fdf.inc | 34 ++++++++++++++++++++++++++++++++++ >> OvmfPkg/OvmfPkgX64.dsc | 10 +++++++++- >> OvmfPkg/VarStore.fdf.inc | 16 ++++++++-------- >> 3 files changed, 51 insertions(+), 9 deletions(-) >> >> diff --git a/OvmfPkg/OvmfPkgDefines.fdf.inc b/OvmfPkg/OvmfPkgDefines.fdf.inc >> index 35fd454b97..da37758934 100644 >> --- a/OvmfPkg/OvmfPkgDefines.fdf.inc >> +++ b/OvmfPkg/OvmfPkgDefines.fdf.inc >> @@ -66,6 +66,40 @@ DEFINE SECFV_OFFSET = 0x003CC000 >> DEFINE SECFV_SIZE = 0x34000 >> !endif >> >> +!if $(FD_SIZE_IN_KB) == 8192 >> +DEFINE VARS_SIZE = 0x84000 >> +DEFINE VARS_BLOCKS = 0x84 >> +DEFINE VARS_LIVE_SIZE = 0x40000 >> +DEFINE VARS_SPARE_SIZE = 0x42000 >> + >> +DEFINE FW_BASE_ADDRESS = 0xFF800000 >> +DEFINE FW_SIZE = 0x00800000 >> +DEFINE FW_BLOCKS = 0x800 >> +DEFINE CODE_BASE_ADDRESS = 0xFF884000 >> +DEFINE CODE_SIZE = 0x0077C000 >> +DEFINE CODE_BLOCKS = 0x77C >> +DEFINE FVMAIN_SIZE = 0x00748000 >> +DEFINE SECFV_OFFSET = 0x007CC000 >> +DEFINE SECFV_SIZE = 0x34000 >> +!endif >> + >> +!if $(FD_SIZE_IN_KB) == 16384 >> +DEFINE VARS_SIZE = 0x84000 >> +DEFINE VARS_BLOCKS = 0x84 >> +DEFINE VARS_LIVE_SIZE = 0x40000 >> +DEFINE VARS_SPARE_SIZE = 0x42000 >> + >> +DEFINE FW_BASE_ADDRESS = 0xFF000000 >> +DEFINE FW_SIZE = 0x01000000 >> +DEFINE FW_BLOCKS = 0x1000 >> +DEFINE CODE_BASE_ADDRESS = 0xFF084000 >> +DEFINE CODE_SIZE = 0x00F7C000 >> +DEFINE CODE_BLOCKS = 0xF7C >> +DEFINE FVMAIN_SIZE = 0x00F48000 >> +DEFINE SECFV_OFFSET = 0x00FCC000 >> +DEFINE SECFV_SIZE = 0x34000 >> +!endif >> + >> SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFdBaseAddress = $(FW_BASE_ADDRESS) >> SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize = $(FW_SIZE) >> SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareBlockSize = $(BLOCK_SIZE) >> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc >> index 999738dc39..28351e2f56 100644 >> --- a/OvmfPkg/OvmfPkgX64.dsc >> +++ b/OvmfPkg/OvmfPkgX64.dsc >> @@ -66,11 +66,19 @@ >> !else >> !ifdef $(FD_SIZE_4MB) >> DEFINE FD_SIZE_IN_KB = 4096 >> +!else >> +!ifdef $(FD_SIZE_8MB) >> + DEFINE FD_SIZE_IN_KB = 8192 >> +!else >> +!ifdef $(FD_SIZE_16MB) >> + DEFINE FD_SIZE_IN_KB = 16384 >> !else >> DEFINE FD_SIZE_IN_KB = 4096 >> !endif >> !endif >> !endif >> +!endif >> +!endif >> >> [BuildOptions] >> GCC:RELEASE_*_*_CC_FLAGS = -DMDEPKG_NDEBUG >> @@ -501,7 +509,7 @@ >> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize|0xe000 >> !endif >> !endif >> -!if $(FD_SIZE_IN_KB) == 4096 >> +!if $(FD_SIZE_IN_KB) == 4096 || $(FD_SIZE_IN_KB) == 8196 || $(FD_SIZE_IN_KB) == 16384 >> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize|0x8400 >> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x8400 >> !if $(NETWORK_TLS_ENABLE) == FALSE >> diff --git a/OvmfPkg/VarStore.fdf.inc b/OvmfPkg/VarStore.fdf.inc >> index a1e524e393..70db929478 100644 >> --- a/OvmfPkg/VarStore.fdf.inc >> +++ b/OvmfPkg/VarStore.fdf.inc >> @@ -11,7 +11,7 @@ >> !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048) >> 0x00000000|0x0000e000 >> !endif >> -!if $(FD_SIZE_IN_KB) == 4096 >> +!if ($(FD_SIZE_IN_KB) == 4096) || ($(FD_SIZE_IN_KB) == 8192) || ($(FD_SIZE_IN_KB) == 16384) >> 0x00000000|0x00040000 >> !endif >> #NV_VARIABLE_STORE >> @@ -29,7 +29,7 @@ DATA = { >> # FvLength: 0x20000 >> 0x00, 0x00, 0x02, 0x00, 0x00, 0x00, 0x00, 0x00, >> !endif >> -!if $(FD_SIZE_IN_KB) == 4096 >> +!if ($(FD_SIZE_IN_KB) == 4096) || ($(FD_SIZE_IN_KB) == 8192) || ($(FD_SIZE_IN_KB) == 16384) >> # FvLength: 0x84000 >> 0x00, 0x40, 0x08, 0x00, 0x00, 0x00, 0x00, 0x00, >> !endif >> @@ -41,7 +41,7 @@ DATA = { >> # CheckSum >> 0x19, 0xF9, >> !endif >> -!if $(FD_SIZE_IN_KB) == 4096 >> +!if ($(FD_SIZE_IN_KB) == 4096) || ($(FD_SIZE_IN_KB) == 8192) || ($(FD_SIZE_IN_KB) == 16384) >> # CheckSum >> 0xAF, 0xB8, >> !endif >> @@ -51,7 +51,7 @@ DATA = { >> # Blockmap[0]: 0x20 Blocks * 0x1000 Bytes / Block >> 0x20, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, 0x00, >> !endif >> -!if $(FD_SIZE_IN_KB) == 4096 >> +!if ($(FD_SIZE_IN_KB) == 4096) || ($(FD_SIZE_IN_KB) == 8192) || ($(FD_SIZE_IN_KB) == 16384) >> # Blockmap[0]: 0x84 Blocks * 0x1000 Bytes / Block >> 0x84, 0x00, 0x00, 0x00, 0x00, 0x10, 0x00, 0x00, >> !endif >> @@ -70,7 +70,7 @@ DATA = { >> # This can speed up the Variable Dispatch a bit. >> 0xB8, 0xDF, 0x00, 0x00, >> !endif >> -!if $(FD_SIZE_IN_KB) == 4096 >> +!if ($(FD_SIZE_IN_KB) == 4096) || ($(FD_SIZE_IN_KB) == 8192) || ($(FD_SIZE_IN_KB) == 16384) >> # Size: 0x40000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) - >> # 0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0x3ffb8 >> # This can speed up the Variable Dispatch a bit. >> @@ -83,7 +83,7 @@ DATA = { >> !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048) >> 0x0000e000|0x00001000 >> !endif >> -!if $(FD_SIZE_IN_KB) == 4096 >> +!if ($(FD_SIZE_IN_KB) == 4096) || ($(FD_SIZE_IN_KB) == 8192) || ($(FD_SIZE_IN_KB) == 16384) >> 0x00040000|0x00001000 >> !endif >> #NV_EVENT_LOG >> @@ -91,7 +91,7 @@ DATA = { >> !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048) >> 0x0000f000|0x00001000 >> !endif >> -!if $(FD_SIZE_IN_KB) == 4096 >> +!if ($(FD_SIZE_IN_KB) == 4096) || ($(FD_SIZE_IN_KB) == 8192) || ($(FD_SIZE_IN_KB) == 16384) >> 0x00041000|0x00001000 >> !endif >> #NV_FTW_WORKING >> @@ -109,7 +109,7 @@ DATA = { >> !if ($(FD_SIZE_IN_KB) == 1024) || ($(FD_SIZE_IN_KB) == 2048) >> 0x00010000|0x00010000 >> !endif >> -!if $(FD_SIZE_IN_KB) == 4096 >> +!if ($(FD_SIZE_IN_KB) == 4096) || ($(FD_SIZE_IN_KB) == 8192) || ($(FD_SIZE_IN_KB) == 16384) >> 0x00042000|0x00042000 >> !endif >> #NV_FTW_SPARE >> > > I'm providing minimal feedback here just to get this review off my plate > as quickly as possible. Sorry, I'm collapsing under my TODO list. > > > (1) Every such change is compatibility breaking, so we *must* use the > opportunity at once to *significantly increase* the non-volatile > variable store size as well. > > We need to discuss this question with OS vendors and hardware platform > vendors on this list, to see what physical flash sizes are expected in > the future, and we must add a good safety margin on top of that. > > The primary concern is with the dbx variable growing without bounds over > time. > > Once we introduce a new FD_SIZE_IN_KB option, we're stuck with its > varstore layout forever, so we'd better get it right and future-proof at > once. > > > (2) [FD.MEMFD] should immediately benefit from this change, even if your > downstream populates FVMAIN_COMPACT with something else than PEIFV and > DXEFV. First, we're almost out of (uncompressed) DXEFV space again. > Second, especially the confidential computing technologies have been > gobbling up the nice, low, free space in FD.MEMFD the way a kid with a > sweet tooth empties a cookie jar. This change is already compat > breaking, so I'd like to see *some* proposal (separate patches) for > enlarging *and pushing up* PEIFV and DXEFV. > > > (3) Unfortunately, I have to agree that introducing *both* a 8MB option > *and* a 16MB option is justified, per QEMU commit 0657c657eb37 > ("hw/i386/pc: add max combined fw size as machine configuration option", > 2020-12-09). > > However, please add each option in a separate patch. > > > (4) Dumping a bunch of magic numbers on reviewers is unhelpful. I'll > need to sit down with a calculator and go through the patch with a > magnifying glass. Please support that work by creating a commit message > (summary table) similar to the one in commit b24fca05751f ("OvmfPkg: > introduce 4MB flash image (mainly) for Windows HCK", 2017-05-05). > I've found it very helpful to create a spreadsheet to calculate the offsets based on the region sizes, and add it to the tree. It can also calculate the related PCDs, if any. That makes it a lot easier to verify the numbers, and to make changes in the future. > > (5) Modifying *only* "OvmfPkg/OvmfPkgX64.dsc" doesn't seem right, there > are other DSC files (platforms) in OvmfPkg that would benefit. Without > much thinking for now, I'd say the new options should be available in > each DSC (platform description), even the 32-bit ones. > > > I'm extremely annoyed by the general trend that the firmware (the OS > under the OS) keeps growing. Because of that, Linuxboot is a fantastic > project. I'd like OVMF to support the development of Linuxboot. I > welcome this patch for that reason. > > But I'd also like OVMF to benefit from this change even when it is built > with a traditional -- and regrettably, ever-growing -- DXE phase. I > welcome this patch for that reason too. > > Thank you, > Laszlo > > > > > > -- Brian J. Johnson Enterprise X86 Lab Hewlett Packard Enterprise