Re: [edk2-devel] [PATCH v1] OvmfPkg: Add build options for 8MB and 16MB X64 OVMF images

["Brian J. Johnson" <brian.johnson@hpe.com>]

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 <dbautista@newmexicoconsortium.org>
>> ---
>>   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