Re: 回复: [edk2-devel] [PATCH v5 0/8] Add Variable Flash Info HOB
Michael Kubacki
mikuback at linux.microsoft.com
Tue May 10 15:01:44 UTC 2022
What's the plan for next steps? The v5 PR has been up for two weeks with
no changes.
Are we going to try to define a long-term pattern for how to include new
library classes in core packages or merge the patch series?
Thanks,
Michael
On 5/5/2022 9:52 PM, Michael Kubacki wrote:
> I still believe a long term design pattern deserves more focus and
> documentation than a quick modification to this series.
>
> Can you confirm that you envision MdePkg/MdeLibs.dsc.inc serving as a
> monolithic host of various other default library class instances?
>
> That somewhat inverts the package relationships, the code reviewer
> policy would need to clarify when the original package owners are
> included on the MdePkg patch (to confirm they agree with the default
> instance choice), and "core" packages would have to be clearly defined
> in this context for developers to know what packages are allowed.
>
> In addition, this does not mean there still won't be some level of
> platform integration thrash. For example, if a new library class
> instance added to MdePkg/MdeLibs.dsc.inc requires another library class
> (or multiple others), those might not be added to the DSC include file.
> They could have been satisfied in the original package DSC (or a test
> platform DSC) but that doesn't mean they will be in all platform DSC
> files. So when the MdeLibs.dsc.inc file update occurs, those platforms
> break and need to add the library class that was already specified in
> other DSC files.
>
> So I request that if this is the preferred approach, that it be agreed
> upon (e.g. dedicated RFC), documented, and consistently followed by
> other contributions as well.
>
> Regards,
> Michael
>
> On 5/4/2022 9:27 PM, gaoliming wrote:
>> Michael:
>> I would suggest to reuse MdePkg/MdeLibs.dsc.inc to list the library
>> and PCD from the edk2 core packages, such as MdePkg, MdeModulePkg,
>> CryptoPkg, SecurirtyPkg and so on. Those packages are required by
>> every platforms. They can't be separated. So, I think
>> MdePkg/MdeLibs.dsc.inc is for edk2 core packages, not only for MdePkg.
>>
>> Thanks
>> Liming
>>> -----邮件原件-----
>>> 发件人: devel at edk2.groups.io <devel at edk2.groups.io> 代表 Michael
>>> Kubacki
>>> 发送时间: 2022年4月29日 23:48
>>> 收件人: Ard Biesheuvel <ardb at kernel.org>
>>> 抄送: edk2-devel-groups-io <devel at edk2.groups.io>; Abner Chang
>>> <abner.chang at hpe.com>; Andrew Fish <afish at apple.com>; Anthony Perard
>>> <anthony.perard at citrix.com>; Ard Biesheuvel <ardb+tianocore at kernel.org>;
>>> Benjamin You <benjamin.you at intel.com>; Brijesh Singh
>>> <brijesh.singh at amd.com>; Erdem Aktas <erdemaktas at google.com>; Gerd
>>> Hoffmann <kraxel at redhat.com>; Guo Dong <guo.dong at intel.com>; Hao A
>>> Wu <hao.a.wu at intel.com>; James Bottomley <jejb at linux.ibm.com>; Jian J
>>> Wang <jian.j.wang at intel.com>; Jiewen Yao <jiewen.yao at intel.com>; Jordan
>>> Justen <jordan.l.justen at intel.com>; Julien Grall <julien at xen.org>; Leif
>>> Lindholm <quic_llindhol at quicinc.com>; Liming Gao
>>> <gaoliming at byosoft.com.cn>; Maurice Ma <maurice.ma at intel.com>; Min Xu
>>> <min.m.xu at intel.com>; Nickle Wang <nickle.wang at hpe.com>; Peter Grehan
>>> <grehan at freebsd.org>; Ray Ni <ray.ni at intel.com>; Rebecca Cran
>>> <rebecca at bsdio.com>; Sami Mujawar <sami.mujawar at arm.com>; Sean
>>> Rhodes <sean at starlabs.systems>; Sebastien Boeuf
>>> <sebastien.boeuf at intel.com>; Tom Lendacky <thomas.lendacky at amd.com>
>>> 主题: Re: [edk2-devel] [PATCH v5 0/8] Add Variable Flash Info HOB
>>>
>>> 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/NetworkCom
>>> ponents.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/Mi
>>>
>>> nPlatformPkg/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 at linux.microsoft.com> wrote:
>>>>>
>>>>> From: Michael Kubacki <michael.kubacki at 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 at hpe.com>
>>>>> Cc: Andrew Fish <afish at apple.com>
>>>>> Cc: Anthony Perard <anthony.perard at citrix.com>
>>>>> Cc: Ard Biesheuvel <ardb+tianocore at kernel.org>
>>>>> Cc: Benjamin You <benjamin.you at intel.com>
>>>>> Cc: Brijesh Singh <brijesh.singh at amd.com>
>>>>> Cc: Erdem Aktas <erdemaktas at google.com>
>>>>> Cc: Gerd Hoffmann <kraxel at redhat.com>
>>>>> Cc: Guo Dong <guo.dong at intel.com>
>>>>> Cc: Hao A Wu <hao.a.wu at intel.com>
>>>>> Cc: James Bottomley <jejb at linux.ibm.com>
>>>>> Cc: Jian J Wang <jian.j.wang at intel.com>
>>>>> Cc: Jiewen Yao <jiewen.yao at intel.com>
>>>>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>>>>> Cc: Julien Grall <julien at xen.org>
>>>>> Cc: Leif Lindholm <quic_llindhol at quicinc.com>
>>>>> Cc: Liming Gao <gaoliming at byosoft.com.cn>
>>>>> Cc: Maurice Ma <maurice.ma at intel.com>
>>>>> Cc: Min Xu <min.m.xu at intel.com>
>>>>> Cc: Nickle Wang <nickle.wang at hpe.com>
>>>>> Cc: Peter Grehan <grehan at freebsd.org>
>>>>> Cc: Ray Ni <ray.ni at intel.com>
>>>>> Cc: Rebecca Cran <rebecca at bsdio.com>
>>>>> Cc: Sami Mujawar <sami.mujawar at arm.com>
>>>>> Cc: Sean Rhodes <sean at starlabs.systems>
>>>>> Cc: Sebastien Boeuf <sebastien.boeuf at intel.com>
>>>>> Cc: Tom Lendacky <thomas.lendacky at amd.com>
>>>>> Signed-off-by: Michael Kubacki <michael.kubacki at 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.i
>>> nf | 48 ++++++
>>>>>
>>> MdeModulePkg/Library/BaseVariableFlashInfoLib/BaseVariableFlashInfoLib.u
>>> ni | 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/FaultTolerantWriteStandal
>>> oneMm.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.i
>>> nf
>>>>> create mode 100644
>>> MdeModulePkg/Library/BaseVariableFlashInfoLib/BaseVariableFlashInfoLib.u
>>> ni
>>>>>
>>>>> --
>>>>> 2.28.0.windows.1
>>>>>
>>>
>>>
>>>
>>>
>>
>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89664): https://edk2.groups.io/g/devel/message/89664
Mute This Topic: https://groups.io/mt/90903015/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list