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

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

On 03/12/20 15:20, Leif Lindholm wrote:
> On Wed, Mar 11, 2020 at 17:41:04 +0100, Laszlo Ersek wrote:
>>>> 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?
>>
>> There are three FDF include files:
>>
>> (1) DecomprScratchEnd.fdf.inc (included under [FV.FVMAIN_COMPACT], per
>>     commit 9beac0d847bf9):
>>
>> ## @file
>> #  This FDF include file computes the end of the scratch buffer used in
>> #  DecompressMemFvs() [OvmfPkg/Sec/SecMain.c]. It is based on the decompressed
>> #  (ie. original) size of the LZMA-compressed section of the one FFS file in
>> #  the FVMAIN_COMPACT firmware volume.
>> #
>> #  Copyright (C) 2015, Red Hat, Inc.
>> #
>> #  SPDX-License-Identifier: BSD-2-Clause-Patent
>> ##
>>
>> This include file contains DEFINE and SET statements (for setting macros
>> and PCDs, accordingly). I don't think either a "Defines" or "Pcds"
>> suffix would apply.
> 
> Would FvmainCompactScratchEnd.fdf.inc be an option?
> 
>> (2) OvmfPkg.fdf.inc (included under [Defines]):
>>
>> ## @file
>> #  FDF include file that defines the main macros and sets the dependent PCDs.
>> #
>> #  Copyright (C) 2014, Red Hat, Inc.
>> #  Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.<BR>
>> #
>> #  SPDX-License-Identifier: BSD-2-Clause-Patent
>> #
>> ##
>>
>> Same situation (both DEFINE and SET statements).
> 
> However, it *needs* to be included inside the [Defines] section, so
> there is no need to get philosofical:
> It's OvmfPkgDefines.fdf.inc (or OvmfDefines.fdf.inc).
> This aligns 100% with the NetworkPkg example.
> 
>> (3) VarStore.fdf.inc (included under [FD.OVMF] and [FD.OVMF_VARS]):
>>
>> ## @file
>> #  FDF include file with Layout Regions that define an empty variable store.
>> #
>> #  Copyright (C) 2014, Red Hat, Inc.
>> #  Copyright (c) 2006 - 2013, Intel Corporation. All rights reserved.<BR>
>> #
>> #  SPDX-License-Identifier: BSD-2-Clause-Patent
>> #
>> ##
>>
>> I guess we could rename this to "VarStoreLayoutRegions.fdf.inc" -- but
>> would that really help?
> 
> OK, so this one is different because it can be included in more than
> one location. Also, it's not the sort of thing you need to touch if
> you're not already in the sixth circle of hell.
> 
>> I'm not against introducing helpful name suffixes, I just can't find
>> anything that really fits and significantly improves upon the current
>> names. I vaguely remember racking my brain when I was introducing these
>> files, for better names, and this is what I had come up with. :)
> 
> Sure, this all predates the NetworkPkg fragments (or really any
> fragments). It would just be nice to be able to start aligning this
> across packages. Splitting up the ARM fragment files is also totally
> on the table here.
> 
> /
>     Leif
> 

I'll send a patch for (1) and (2).

Thanks
Laszlo