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

["Leif Lindholm" <leif@nuviainc.com>]

On Wed, Mar 11, 2020 at 17:14:53 +0100, Laszlo Ersek wrote:
> 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.

I see your point. The generic name suggested to me that it might be
*intended* to hold multiple sections, and currently just happened not
to.

However, to follow a rule of least surprise...

> 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

...could OvmfPkg use a similar naming scheme to this?

That would also remove the drawback of not having the section name as
part of the hunk header, as you'd have it anyway immediately above as
part of the file name?

Regards,

Leif

> 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
> >>
> >>
> >>
> >> 
> >>
> > 
> 
> 
> 
>