Re: [PATCH v5 0/8] Add Variable Flash Info HOB

["Michael Kubacki" <mikuback@linux.microsoft.com>]

I agree that would be a useful tool and in the case of changes such as 
this that provide backward compatibility with existing functionality, 
particularly helpful.

Some packages such as MdePkg 
(https://github.com/tianocore/edk2/blob/master/MdePkg/MdeLibs.dsc.inc) 
and NetworkPkg 
(https://github.com/tianocore/edk2/blob/master/NetworkPkg/NetworkComponents.dsc.inc) 
provide DSC files that a platform can override if necessary.

However, this does not exist for all edk2 packages. I did not introduce 
such a file in MdeModulePkg because I believe that is an independent 
package design decision outside the scope of this series and, if that 
change was made, it should include libraries other than just this 
instance. That would lead to additional churn and a larger platform 
integration debate, important to that topic, but separate from the 
current state this contribution is based against.

While includes be helpful, it can encourage platform owners to ignore 
potential creep in functionality they should be aware of.

For example, the DSC update is mostly being given to platforms to fix 
their immediate build problem. But, a platform owner might also choose 
to update their FVB driver to use this interface to get flash 
information as opposed to directly using PCDs as many do today. That's a 
decision they need to evaluate and make but they should be aware of the 
interface and make that decision. By directly reviewing/integrating the 
change for their platform, they are more explicitly made aware of this 
new interface to form that decision.

Also, when many include files get involved, platform build complexity 
and developer frustration can increase due to nesting and order of 
include files. Values (library classes, PCDs, etc.) can be overridden 
more than once. Ultimately, this is technically manageable by utilizing 
build reports and understanding the EDK II build output in more detail.

Again, I think this conversation is useful but requires much more time 
to address questions such as the following:

1. Should a different mechanism for default library classes be introduced?

2. Should all packages in edk2 provide such an include file? If so, does 
it only provide the DSC file (like MdePkg) or other files (like 
NetworkPkg which includes FDF)?

3. Which library classes for a given package should be given default 
instances?

4. How would each platform package maintainer like to maintain their 
platform build?

Example: Would MinPlatformPkg prefer to keep its own "core" include 
files 
(https://github.com/tianocore/edk2-platforms/tree/master/Platform/Intel/MinPlatformPkg/Include) 
or reconcile them with include files (possibly introducing an additional 
layer of nesting)? Would others prefer not to use includes at all to 
keep a flat, simpler file?

How would someone updating the platform code due to an edk2 change be 
aware of this per-platform policy?

To my understanding, the answers to these today are:

1. No
2. No / decided on per-change basis
3. Unknown
4. Unknown

(2) adds friction to the edk2 contribution process as expectation is 
unclear.

(3) adds churn into the platform integration process as the integration 
process for existing code is subject to change over time (e.g. realize 
library class is now in DSC and remove from platform DSC to use include 
instance).

(4) adds friction to the edk2 contribution process as expectation is 
unclear. Also, somewhat error prone. There's also a level of due 
diligence needed to confirm if a platform that already has an include is 
overriding that in another DSC file. If so, is that still the correct 
behavior or should it be modified.

Regards,
Michael

On 4/29/2022 9:45 AM, Ard Biesheuvel wrote:
> On Tue, 26 Apr 2022 at 03:29, <mikuback@linux.microsoft.com> wrote:
>>
>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3479
>>
>> Platforms: This series introduces a new library class called
>> VariableFlashInfoLib. Platforms using the variable modules will
>> need to specify these library classes. See the patches at the
>> end of this series for examples of the change needed in the
>> platform DSC file. I have attempted to update all open-source
>> platforms in edk2 and edk2-platforms in this series and
>> https://edk2.groups.io/g/devel/message/89148.
>>
> 
> I will make the same remark here as I made in response to the
> edk2-platforms series:
> 
> Could we please consider introducing a way to define the default
> resolution of a library class? That way, all this churn and
> unnecessary breakage would not be necessary, as defining a resolution
> would only be necessary when deviating from the default. In fact, we
> might be able to clean up some .DSCs considerably by defining defaults
> for library classes that only have one (or very few) implementations.
> 
> 
>> The UEFI variable drivers such as VariableRuntimeDxe, VariableSmm,
>> VariableStandaloneMm, etc. (and their dependent protocol/library
>> stack), typically acquire UEFI variable store flash information
>> with PCDs declared in MdeModulePkg.
>>
>> For example:
>> [Pcd]
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
>>
>> These PCDs work as-is in the StandaloneMm driver if they are not
>> dynamic such as Dynamic or DynamicEx because PCD services are not
>> readily available in the Standalone MM environment. Platforms that
>> use Standalone MM today, must define these PCDs as FixedAtBuild in
>> their platform build. However, the PCDs do allow platforms to treat
>> the PCDs as Dynamic/DynamicEx and being able to support that is
>> currently a gap for Standalone MM.
>>
>> This patch series introduces a HOB that can be produced by the
>> platform to provide the same information. The HOB list is
>> available to Standalone MM.
>>
>> The PCD declarations are left as-is in MdeModulePkg for backward
>> compatibility. This means unless a platform wants to use the HOB,
>> their code will continue to work with no change (they do not need
>> to produce the HOB). Only if the HOB is found, is its value used
>> instead of the PCDs.
>>
>> Due to the large number of consumers of this information, access
>> to the base address and size values is abstracted in a new library
>> class (as requested in the v1 series) called VariableFlashInfoLib.
>>
>> The API of VariableFlashInfoLib does not bind the underlying data
>> structure to the information returned to library users to allow
>> flexibility in the library implementation in the future.
>>
>> V5 changes.
>> 1. Made GetVariableFlashInfoFromHob() in BaseVariableFlashInfoLib.c
>>     STATIC.
>> 2. Added a section in commit v5 [3/8] to explicitly state that the
>>     commit introduces a dependency that requires a change in platform
>>     build.  Please see patches v5 [5/8] - v5 [8/8] for examples of
>>     how to integrate this change (add VariableFlashInfoLib library
>>     to DSC file).
>>
>> V4 changes:
>> 1. Add a UINT32 "Reserved" field to VARIABLE_FLASH_INFO.
>> 2. Add a descriptive comment to VariableFlashInfo.h to explain
>>     HOB usage.
>>
>> V3 changes:
>> 1. To better clarify usage, renamed the members
>>     "NvStorageBaseAddress" and "NvStorageLength" in
>>     "VARIABLE_FLASH_INFO" to "NvVariableBaseAddress" and
>>     "NvVariableLength".
>> 2. Added description comments to the fields in "VARIABLE_FLASH_INFO".
>>
>> V2 changes:
>> 1. Abstracted flash info data access with VariableFlashInfoLib.
>> 2. Updated package builds in the repo that build the variable and
>>     FTW drivers to include VariableFlashInfoLib.
>> 3. Removed a redundant variable assignment in VariableSmm.c.
>> 4. Updated comments in FtwMisc.c and FaultTolerantWritePei.c to
>>     indicate driver assumption is UINTN (not UINT32)
>> 5. Added a version field to the VARIABLE_FLASH_INFO structure.
>>
>> Cc: Abner Chang <abner.chang@hpe.com>
>> Cc: Andrew Fish <afish@apple.com>
>> Cc: Anthony Perard <anthony.perard@citrix.com>
>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>> Cc: Benjamin You <benjamin.you@intel.com>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Cc: Erdem Aktas <erdemaktas@google.com>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Guo Dong <guo.dong@intel.com>
>> Cc: Hao A Wu <hao.a.wu@intel.com>
>> Cc: James Bottomley <jejb@linux.ibm.com>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Julien Grall <julien@xen.org>
>> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Maurice Ma <maurice.ma@intel.com>
>> Cc: Min Xu <min.m.xu@intel.com>
>> Cc: Nickle Wang <nickle.wang@hpe.com>
>> Cc: Peter Grehan <grehan@freebsd.org>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Rebecca Cran <rebecca@bsdio.com>
>> Cc: Sami Mujawar <sami.mujawar@arm.com>
>> Cc: Sean Rhodes <sean@starlabs.systems>
>> Cc: Sebastien Boeuf <sebastien.boeuf@intel.com>
>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> Michael Kubacki (8):
>>    MdeModulePkg: Add Variable Flash Info HOB
>>    MdeModulePkg/VariableFlashInfoLib: Add initial library
>>    MdeModulePkg/Variable: Consume Variable Flash Info
>>    MdeModulePkg/FaultTolerantWrite: Consume Variable Flash Info
>>    ArmVirtPkg/ArmVirt.dsc.inc: Add VariableFlashInfoLib
>>    EmulatorPkg: Add VariableFlashInfoLib
>>    OvmfPkg: Add VariableFlashInfoLib
>>    UefiPayloadPkg: Add VariableFlashInfoLib
>>
>>   MdeModulePkg/Library/BaseVariableFlashInfoLib/BaseVariableFlashInfoLib.c        | 179 ++++++++++++++++++++
>>   MdeModulePkg/Universal/FaultTolerantWriteDxe/FtwMisc.c                          |  41 +++--
>>   MdeModulePkg/Universal/FaultTolerantWriteDxe/UpdateWorkingBlock.c               |   7 +-
>>   MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.c            |  28 +--
>>   MdeModulePkg/Universal/Variable/Pei/Variable.c                                  |  14 +-
>>   MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c                        |  16 +-
>>   MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.c                |  14 +-
>>   MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c                        |  17 +-
>>   ArmVirtPkg/ArmVirt.dsc.inc                                                      |   1 +
>>   EmulatorPkg/EmulatorPkg.dsc                                                     |   1 +
>>   MdeModulePkg/Include/Guid/VariableFlashInfo.h                                   | 111 ++++++++++++
>>   MdeModulePkg/Include/Library/VariableFlashInfoLib.h                             |  68 ++++++++
>>   MdeModulePkg/Library/BaseVariableFlashInfoLib/BaseVariableFlashInfoLib.inf      |  48 ++++++
>>   MdeModulePkg/Library/BaseVariableFlashInfoLib/BaseVariableFlashInfoLib.uni      |  12 ++
>>   MdeModulePkg/MdeModulePkg.dec                                                   |   8 +
>>   MdeModulePkg/MdeModulePkg.dsc                                                   |   2 +
>>   MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWrite.h               |   7 +-
>>   MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteDxe.inf          |  10 +-
>>   MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteSmm.inf          |  10 +-
>>   MdeModulePkg/Universal/FaultTolerantWriteDxe/FaultTolerantWriteStandaloneMm.inf |  10 +-
>>   MdeModulePkg/Universal/FaultTolerantWritePei/FaultTolerantWritePei.inf          |  10 +-
>>   MdeModulePkg/Universal/Variable/Pei/Variable.h                                  |   2 +
>>   MdeModulePkg/Universal/Variable/Pei/VariablePei.inf                             |   5 +-
>>   MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h                           |   7 +-
>>   MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf               |   5 +-
>>   MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf                      |   5 +-
>>   MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf             |   5 +-
>>   OvmfPkg/AmdSev/AmdSevX64.dsc                                                    |   1 +
>>   OvmfPkg/Bhyve/BhyveX64.dsc                                                      |   1 +
>>   OvmfPkg/CloudHv/CloudHvX64.dsc                                                  |   1 +
>>   OvmfPkg/IntelTdx/IntelTdxX64.dsc                                                |   1 +
>>   OvmfPkg/Microvm/MicrovmX64.dsc                                                  |   1 +
>>   OvmfPkg/OvmfPkgIa32.dsc                                                         |   1 +
>>   OvmfPkg/OvmfPkgIa32X64.dsc                                                      |   1 +
>>   OvmfPkg/OvmfPkgX64.dsc                                                          |   1 +
>>   OvmfPkg/OvmfXen.dsc                                                             |   1 +
>>   UefiPayloadPkg/UefiPayloadPkg.dsc                                               |   1 +
>>   37 files changed, 559 insertions(+), 94 deletions(-)
>>   create mode 100644 MdeModulePkg/Library/BaseVariableFlashInfoLib/BaseVariableFlashInfoLib.c
>>   create mode 100644 MdeModulePkg/Include/Guid/VariableFlashInfo.h
>>   create mode 100644 MdeModulePkg/Include/Library/VariableFlashInfoLib.h
>>   create mode 100644 MdeModulePkg/Library/BaseVariableFlashInfoLib/BaseVariableFlashInfoLib.inf
>>   create mode 100644 MdeModulePkg/Library/BaseVariableFlashInfoLib/BaseVariableFlashInfoLib.uni
>>
>> --
>> 2.28.0.windows.1
>>