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 17:20, Leif Lindholm wrote:
> 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?

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.


(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).


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

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. :)

Thanks,
Laszlo