[edk2-devel] [PATCH v1] OvmfPkg: Add build options for 8MB and 16MB X64 OVMF images
Brian J. Johnson
brian.johnson at hpe.com
Fri May 28 13:33:01 UTC 2021
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 at 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75838): https://edk2.groups.io/g/devel/message/75838
Mute This Topic: https://groups.io/mt/83106841/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list