Re: [edk2-devel] [PATCH 3/5] OvmfPkg: set fixed FlashNvStorage base addresses with -D SMM_REQUIRE

["Laszlo Ersek" <lersek@redhat.com>]

On 03/11/20 16:44, Leif Lindholm wrote:
> One comment, not on this patch but prompted by it:
> 
> On Tue, Mar 10, 2020 at 23:27:37 +0100, Laszlo Ersek wrote:
>> diff --git a/OvmfPkg/OvmfPkg.fdf.inc b/OvmfPkg/OvmfPkg.fdf.inc
>> index 66e0e4d270f5..35fd454b97ab 100644
>> --- a/OvmfPkg/OvmfPkg.fdf.inc
>> +++ b/OvmfPkg/OvmfPkg.fdf.inc
>> @@ -82,4 +82,10 @@
> 
> I was surprised at not seeing the section header here, so had a look
> at the file, noticed it doesn't have any. And that all files that
> include it do it by:
> 
> [Defines]
> !include OvmfPkg.fdf.inc
> 
> That looks a bit error-prone and inflexible - could we move/copy the
> header into this file?

No, please let us not -- I strive to keep all FDF and DSC include files
under OvmfPkg header-free. It gives more flexibility to the includer.

A recent example of this was my request for NetworkPkg to expose its
include snippets header-less, for DSC files. Please see the "!include
NetworkPkg/..." directives in the OVMF DSC files; those are also by
design header-less:

NetworkPkg/NetworkComponents.dsc.inc
NetworkPkg/NetworkDefines.dsc.inc
NetworkPkg/NetworkLibs.dsc.inc
NetworkPkg/NetworkPcds.dsc.inc

NetworkPkg does offer (as a convenience) more "canned" includes too, but
those were not flexible enough for OVMF.

Same for the other FDF include files under OvmfPkg:
- DecomprScratchEnd.fdf.inc
- VarStore.fdf.inc

An example of where this is actively being put to use is
"VarStore.fdf.inc": it is included both under [FD.OVMF], and [FD.OVMF_VARS].

So the treatment of the include files is consistent (I'd not want some
includes with headers, and some without).


I realize the include files under ArmVirtPkg do not follow this pattern
(they contain headers); however, out of the files:

- ArmVirt.dsc.inc
- ArmVirtQemuFvMain.fdf.inc
- ArmVirtRules.fdf.inc
- VarStore.fdf.inc

the first and the third contain *multiple* headers, so the application
area is arguably different.

Thanks,
Laszlo


> 
> /
>     Leif
> 
>>  SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwSpareBase = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwWorkingBase + gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
>>  SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize = $(VARS_SPARE_SIZE)
>>  
>> +!if $(SMM_REQUIRE) == TRUE
>> +SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase
>> +SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwWorkingBase
>> +SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase   = gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwSpareBase
>> +!endif
>> +
>>  DEFINE MEMFD_BASE_ADDRESS = 0x800000
>> -- 
>> 2.19.1.3.g30247aa5d201
>>
>>
>>
>> 
>>
>