* Re: [edk2-devel] [PATCH v1] OvmfPkg: Add build options for 8MB and 16MB X64 OVMF images
2021-05-27 10:12 ` [edk2-devel] " Laszlo Ersek
@ 2021-05-27 10:14 ` Laszlo Ersek
2021-05-27 10:21 ` Laszlo Ersek
2021-05-28 20:49 ` Devon Bautista
2021-05-28 13:33 ` Brian J. Johnson
2021-07-28 19:43 ` Devon Bautista
2 siblings, 2 replies; 8+ messages in thread
From: Laszlo Ersek @ 2021-05-27 10:14 UTC (permalink / raw)
To: devel, dbautista
Cc: Brijesh Singh, Erdem Aktas, James Bottomley, Jiewen Yao, Min Xu,
Tom Lendacky
On 05/27/21 12:12, 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(-)
>>
> 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.
PS: please file a tianocore feature request at <https://bugzilla.tianocore.org/>, and please link the current (v1) posting into it (in a new bugzilla comment), with the following URLs:
https://edk2.groups.io/g/devel/message/75666
https://listman.redhat.com/archives/edk2-devel-archive/2021-May/msg01106.html
http://mid.mail-archive.com/7bfd4b82fc725302beb37e13c4a89d389c34ec34.1622048433.git.dbautista@newmexicoconsortium.org
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH v1] OvmfPkg: Add build options for 8MB and 16MB X64 OVMF images
2021-05-27 10:14 ` Laszlo Ersek
@ 2021-05-27 10:21 ` Laszlo Ersek
2021-05-28 20:49 ` Devon Bautista
1 sibling, 0 replies; 8+ messages in thread
From: Laszlo Ersek @ 2021-05-27 10:21 UTC (permalink / raw)
To: Jordan Justen (Intel address), Ard Biesheuvel (TianoCore)
Cc: devel, dbautista, Brijesh Singh, Erdem Aktas, James Bottomley,
Jiewen Yao, Min Xu, Tom Lendacky
Hi Ard, Jordan,
On 05/27/21 12:14, Laszlo Ersek wrote:
> On 05/27/21 12:12, 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(-)
>>>
>
>> 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.
>
> PS: please file a tianocore feature request at <https://bugzilla.tianocore.org/>, and please link the current (v1) posting into it (in a new bugzilla comment), with the following URLs:
>
> https://edk2.groups.io/g/devel/message/75666
> https://listman.redhat.com/archives/edk2-devel-archive/2021-May/msg01106.html
> http://mid.mail-archive.com/7bfd4b82fc725302beb37e13c4a89d389c34ec34.1622048433.git.dbautista@newmexicoconsortium.org
You weren't CC'd on the patch up-front, and I too forgot adding you
immediately. Please consider checking the thread from the root; please
see the archive URLs above.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH v1] OvmfPkg: Add build options for 8MB and 16MB X64 OVMF images
2021-05-27 10:14 ` Laszlo Ersek
2021-05-27 10:21 ` Laszlo Ersek
@ 2021-05-28 20:49 ` Devon Bautista
1 sibling, 0 replies; 8+ messages in thread
From: Devon Bautista @ 2021-05-28 20:49 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: Brijesh Singh, Erdem Aktas, James Bottomley, Jiewen Yao, Min Xu,
Tom Lendacky
On 5/27/21 3:14 AM, Laszlo Ersek wrote:
> On 05/27/21 12:12, 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(-)
>>>
>> 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.
> PS: please file a tianocore feature request at <https://bugzilla.tianocore.org/>, and please link the current (v1) posting into it (in a new bugzilla comment), with the following URLs:
>
> https://edk2.groups.io/g/devel/message/75666
> https://listman.redhat.com/archives/edk2-devel-archive/2021-May/msg01106.html
> http://mid.mail-archive.com/7bfd4b82fc725302beb37e13c4a89d389c34ec34.1622048433.git.dbautista@newmexicoconsortium.org
>
> Thanks!
> Laszlo
>
Tianocore Feature Request:
https://bugzilla.tianocore.org/show_bug.cgi?id=3420
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH v1] OvmfPkg: Add build options for 8MB and 16MB X64 OVMF images
2021-05-27 10:12 ` [edk2-devel] " Laszlo Ersek
2021-05-27 10:14 ` Laszlo Ersek
@ 2021-05-28 13:33 ` Brian J. Johnson
2021-06-01 12:20 ` Laszlo Ersek
2021-07-28 19:43 ` Devon Bautista
2 siblings, 1 reply; 8+ messages in thread
From: Brian J. Johnson @ 2021-05-28 13:33 UTC (permalink / raw)
To: devel, lersek, dbautista
Cc: Brijesh Singh, Erdem Aktas, James Bottomley, Jiewen Yao, Min Xu,
Tom Lendacky
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [edk2-devel] [PATCH v1] OvmfPkg: Add build options for 8MB and 16MB X64 OVMF images
2021-05-28 13:33 ` Brian J. Johnson
@ 2021-06-01 12:20 ` Laszlo Ersek
0 siblings, 0 replies; 8+ messages in thread
From: Laszlo Ersek @ 2021-06-01 12:20 UTC (permalink / raw)
To: devel, brian.johnson, dbautista
Cc: Brijesh Singh, Erdem Aktas, James Bottomley, Jiewen Yao, Min Xu,
Tom Lendacky
On 05/28/21 15:33, Brian J. Johnson wrote:
> 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(-)
>> (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.
This sounds really nice -- I'd appreciate such a spreadsheet in the
OvmfPkg directory tree somewhere. My concern is that most (all?)
spreadsheet formats are compressed archives one way or another (ZIP
archives of multiple files, or gzipped XML files, or some such), and
such formats are not nice to track in git. I'd like a (structured)
plaintext representation of the spreadsheet to live in edk2 git. Same as
we prefer plaintext SVG files for graphics / diagrams in edk2, to my
knowledge.
Thanks!
Laszlo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [edk2-devel] [PATCH v1] OvmfPkg: Add build options for 8MB and 16MB X64 OVMF images
2021-05-27 10:12 ` [edk2-devel] " Laszlo Ersek
2021-05-27 10:14 ` Laszlo Ersek
2021-05-28 13:33 ` Brian J. Johnson
@ 2021-07-28 19:43 ` Devon Bautista
2 siblings, 0 replies; 8+ messages in thread
From: Devon Bautista @ 2021-07-28 19:43 UTC (permalink / raw)
To: Laszlo Ersek, devel
Cc: Brijesh Singh, Erdem Aktas, James Bottomley, Jiewen Yao, Min Xu,
Tom Lendacky
[-- Attachment #1: Type: text/plain, Size: 10507 bytes --]
Laszlo,
I apologize for the delay. I've been heavily preoccupied in another work
program I am involved in.
On 5/27/21 3: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.
Would you prefer to see a new patchset version addressing points (3)
through (5) and adding the plaintext spreadsheet of the region
offsets/sizes based on Brian's suggestion and discussing this point on
that new thread? Or would you prefer a new discussion on the "discuss'
list? Or use this thread?
I've already started on the next patchset version, but can wait on
submitting for more discussion if desired.
> (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.
I suppose it would be sensible to work this out after this patchset gets
finalized, but your input is greatly welcomed here.
> (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.
That makes sense for logical separation and atomic commits. Will do.
> (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).
In light of Brian's suggestion, would you prefer having such a summary
table both in the commit message /and/ in the OVMF source tree or just
the latter?
> (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.
You're right. I'll update the rest of the DSC files with the changes in
the next patchset version.
> 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 for your feedback. I'm joyed to hear of your support. I, too,
think both Linuxboot and OVMF proper could benefit from this.
Best,
Devon Bautista
[-- Attachment #2: Type: text/html, Size: 11462 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread