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
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. 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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89296): https://edk2.groups.io/g/devel/message/89296 Mute This Topic: https://groups.io/mt/90699662/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89413): https://edk2.groups.io/g/devel/message/89413 Mute This Topic: https://groups.io/mt/90699662/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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 >> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89417): https://edk2.groups.io/g/devel/message/89417 Mute This Topic: https://groups.io/mt/90699662/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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@edk2.groups.io <devel@edk2.groups.io> 代表 Michael > Kubacki > 发送时间: 2022年4月29日 23:48 > 收件人: Ard Biesheuvel <ardb@kernel.org> > 抄送: edk2-devel-groups-io <devel@edk2.groups.io>; Abner Chang > <abner.chang@hpe.com>; Andrew Fish <afish@apple.com>; Anthony Perard > <anthony.perard@citrix.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; > Benjamin You <benjamin.you@intel.com>; Brijesh Singh > <brijesh.singh@amd.com>; Erdem Aktas <erdemaktas@google.com>; Gerd > Hoffmann <kraxel@redhat.com>; Guo Dong <guo.dong@intel.com>; Hao A > Wu <hao.a.wu@intel.com>; James Bottomley <jejb@linux.ibm.com>; Jian J > Wang <jian.j.wang@intel.com>; Jiewen Yao <jiewen.yao@intel.com>; Jordan > Justen <jordan.l.justen@intel.com>; Julien Grall <julien@xen.org>; Leif > Lindholm <quic_llindhol@quicinc.com>; Liming Gao > <gaoliming@byosoft.com.cn>; Maurice Ma <maurice.ma@intel.com>; Min Xu > <min.m.xu@intel.com>; Nickle Wang <nickle.wang@hpe.com>; Peter Grehan > <grehan@freebsd.org>; Ray Ni <ray.ni@intel.com>; Rebecca Cran > <rebecca@bsdio.com>; Sami Mujawar <sami.mujawar@arm.com>; Sean > Rhodes <sean@starlabs.systems>; Sebastien Boeuf > <sebastien.boeuf@intel.com>; Tom Lendacky <thomas.lendacky@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@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.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 (#89519): https://edk2.groups.io/g/devel/message/89519 Mute This Topic: https://groups.io/mt/90903015/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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@edk2.groups.io <devel@edk2.groups.io> 代表 Michael >> Kubacki >> 发送时间: 2022年4月29日 23:48 >> 收件人: Ard Biesheuvel <ardb@kernel.org> >> 抄送: edk2-devel-groups-io <devel@edk2.groups.io>; Abner Chang >> <abner.chang@hpe.com>; Andrew Fish <afish@apple.com>; Anthony Perard >> <anthony.perard@citrix.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; >> Benjamin You <benjamin.you@intel.com>; Brijesh Singh >> <brijesh.singh@amd.com>; Erdem Aktas <erdemaktas@google.com>; Gerd >> Hoffmann <kraxel@redhat.com>; Guo Dong <guo.dong@intel.com>; Hao A >> Wu <hao.a.wu@intel.com>; James Bottomley <jejb@linux.ibm.com>; Jian J >> Wang <jian.j.wang@intel.com>; Jiewen Yao <jiewen.yao@intel.com>; Jordan >> Justen <jordan.l.justen@intel.com>; Julien Grall <julien@xen.org>; Leif >> Lindholm <quic_llindhol@quicinc.com>; Liming Gao >> <gaoliming@byosoft.com.cn>; Maurice Ma <maurice.ma@intel.com>; Min Xu >> <min.m.xu@intel.com>; Nickle Wang <nickle.wang@hpe.com>; Peter Grehan >> <grehan@freebsd.org>; Ray Ni <ray.ni@intel.com>; Rebecca Cran >> <rebecca@bsdio.com>; Sami Mujawar <sami.mujawar@arm.com>; Sean >> Rhodes <sean@starlabs.systems>; Sebastien Boeuf >> <sebastien.boeuf@intel.com>; Tom Lendacky <thomas.lendacky@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@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.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 (#89546): https://edk2.groups.io/g/devel/message/89546 Mute This Topic: https://groups.io/mt/90903015/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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@edk2.groups.io <devel@edk2.groups.io> 代表 Michael >>> Kubacki >>> 发送时间: 2022年4月29日 23:48 >>> 收件人: Ard Biesheuvel <ardb@kernel.org> >>> 抄送: edk2-devel-groups-io <devel@edk2.groups.io>; Abner Chang >>> <abner.chang@hpe.com>; Andrew Fish <afish@apple.com>; Anthony Perard >>> <anthony.perard@citrix.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; >>> Benjamin You <benjamin.you@intel.com>; Brijesh Singh >>> <brijesh.singh@amd.com>; Erdem Aktas <erdemaktas@google.com>; Gerd >>> Hoffmann <kraxel@redhat.com>; Guo Dong <guo.dong@intel.com>; Hao A >>> Wu <hao.a.wu@intel.com>; James Bottomley <jejb@linux.ibm.com>; Jian J >>> Wang <jian.j.wang@intel.com>; Jiewen Yao <jiewen.yao@intel.com>; Jordan >>> Justen <jordan.l.justen@intel.com>; Julien Grall <julien@xen.org>; Leif >>> Lindholm <quic_llindhol@quicinc.com>; Liming Gao >>> <gaoliming@byosoft.com.cn>; Maurice Ma <maurice.ma@intel.com>; Min Xu >>> <min.m.xu@intel.com>; Nickle Wang <nickle.wang@hpe.com>; Peter Grehan >>> <grehan@freebsd.org>; Ray Ni <ray.ni@intel.com>; Rebecca Cran >>> <rebecca@bsdio.com>; Sami Mujawar <sami.mujawar@arm.com>; Sean >>> Rhodes <sean@starlabs.systems>; Sebastien Boeuf >>> <sebastien.boeuf@intel.com>; Tom Lendacky <thomas.lendacky@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@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.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/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Can you please respond with your preference? I am ready to do this but if it is required now, it should be documented so it becomes a consistent pattern for future changes. Thanks, Michael On 5/10/2022 11:01 AM, Michael Kubacki wrote: > 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@edk2.groups.io <devel@edk2.groups.io> 代表 Michael >>>> Kubacki >>>> 发送时间: 2022年4月29日 23:48 >>>> 收件人: Ard Biesheuvel <ardb@kernel.org> >>>> 抄送: edk2-devel-groups-io <devel@edk2.groups.io>; Abner Chang >>>> <abner.chang@hpe.com>; Andrew Fish <afish@apple.com>; Anthony Perard >>>> <anthony.perard@citrix.com>; Ard Biesheuvel >>>> <ardb+tianocore@kernel.org>; >>>> Benjamin You <benjamin.you@intel.com>; Brijesh Singh >>>> <brijesh.singh@amd.com>; Erdem Aktas <erdemaktas@google.com>; Gerd >>>> Hoffmann <kraxel@redhat.com>; Guo Dong <guo.dong@intel.com>; Hao A >>>> Wu <hao.a.wu@intel.com>; James Bottomley <jejb@linux.ibm.com>; Jian J >>>> Wang <jian.j.wang@intel.com>; Jiewen Yao <jiewen.yao@intel.com>; Jordan >>>> Justen <jordan.l.justen@intel.com>; Julien Grall <julien@xen.org>; Leif >>>> Lindholm <quic_llindhol@quicinc.com>; Liming Gao >>>> <gaoliming@byosoft.com.cn>; Maurice Ma <maurice.ma@intel.com>; Min Xu >>>> <min.m.xu@intel.com>; Nickle Wang <nickle.wang@hpe.com>; Peter Grehan >>>> <grehan@freebsd.org>; Ray Ni <ray.ni@intel.com>; Rebecca Cran >>>> <rebecca@bsdio.com>; Sami Mujawar <sami.mujawar@arm.com>; Sean >>>> Rhodes <sean@starlabs.systems>; Sebastien Boeuf >>>> <sebastien.boeuf@intel.com>; Tom Lendacky <thomas.lendacky@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@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.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 (#89731): https://edk2.groups.io/g/devel/message/89731 Mute This Topic: https://groups.io/mt/90903015/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Michael: This is my suggestion to resolve such compatible issue. As you say, it needs more review and discussion. So, it may not be applied immediately. If this patch set needs to catch this table tag, it has to take current way to update each DSC file. Have the patch set got reviewed-by from Package maintainers? If yes, I think this patch set can still be merged for this stable tag. Thanks Liming > -----邮件原件----- > 发件人: Michael Kubacki <mikuback@linux.microsoft.com> > 发送时间: 2022年5月14日 2:24 > 收件人: gaoliming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io; 'Ard > Biesheuvel' <ardb@kernel.org> > 抄送: 'Abner Chang' <abner.chang@hpe.com>; 'Andrew Fish' > <afish@apple.com>; 'Anthony Perard' <anthony.perard@citrix.com>; 'Ard > Biesheuvel' <ardb+tianocore@kernel.org>; 'Benjamin You' > <benjamin.you@intel.com>; 'Brijesh Singh' <brijesh.singh@amd.com>; 'Erdem > Aktas' <erdemaktas@google.com>; 'Gerd Hoffmann' <kraxel@redhat.com>; > 'Guo Dong' <guo.dong@intel.com>; 'Hao A Wu' <hao.a.wu@intel.com>; > 'James Bottomley' <jejb@linux.ibm.com>; 'Jian J Wang' > <jian.j.wang@intel.com>; 'Jiewen Yao' <jiewen.yao@intel.com>; 'Jordan > Justen' <jordan.l.justen@intel.com>; 'Julien Grall' <julien@xen.org>; 'Leif > Lindholm' <quic_llindhol@quicinc.com>; 'Maurice Ma' > <maurice.ma@intel.com>; 'Min Xu' <min.m.xu@intel.com>; 'Nickle Wang' > <nickle.wang@hpe.com>; 'Peter Grehan' <grehan@freebsd.org>; 'Ray Ni' > <ray.ni@intel.com>; 'Rebecca Cran' <rebecca@bsdio.com>; 'Sami Mujawar' > <sami.mujawar@arm.com>; 'Sean Rhodes' <sean@starlabs.systems>; > 'Sebastien Boeuf' <sebastien.boeuf@intel.com>; 'Tom Lendacky' > <thomas.lendacky@amd.com> > 主题: Re: 回复: [edk2-devel] [PATCH v5 0/8] Add Variable Flash Info HOB > > Can you please respond with your preference? > > I am ready to do this but if it is required now, it should be documented > so it becomes a consistent pattern for future changes. > > Thanks, > Michael > > On 5/10/2022 11:01 AM, Michael Kubacki wrote: > > 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@edk2.groups.io <devel@edk2.groups.io> 代表 > Michael > >>>> Kubacki > >>>> 发送时间: 2022年4月29日 23:48 > >>>> 收件人: Ard Biesheuvel <ardb@kernel.org> > >>>> 抄送: edk2-devel-groups-io <devel@edk2.groups.io>; Abner Chang > >>>> <abner.chang@hpe.com>; Andrew Fish <afish@apple.com>; Anthony > Perard > >>>> <anthony.perard@citrix.com>; Ard Biesheuvel > >>>> <ardb+tianocore@kernel.org>; > >>>> Benjamin You <benjamin.you@intel.com>; Brijesh Singh > >>>> <brijesh.singh@amd.com>; Erdem Aktas <erdemaktas@google.com>; > Gerd > >>>> Hoffmann <kraxel@redhat.com>; Guo Dong <guo.dong@intel.com>; Hao > A > >>>> Wu <hao.a.wu@intel.com>; James Bottomley <jejb@linux.ibm.com>; > Jian J > >>>> Wang <jian.j.wang@intel.com>; Jiewen Yao <jiewen.yao@intel.com>; > Jordan > >>>> Justen <jordan.l.justen@intel.com>; Julien Grall <julien@xen.org>; Leif > >>>> Lindholm <quic_llindhol@quicinc.com>; Liming Gao > >>>> <gaoliming@byosoft.com.cn>; Maurice Ma <maurice.ma@intel.com>; > Min Xu > >>>> <min.m.xu@intel.com>; Nickle Wang <nickle.wang@hpe.com>; Peter > Grehan > >>>> <grehan@freebsd.org>; Ray Ni <ray.ni@intel.com>; Rebecca Cran > >>>> <rebecca@bsdio.com>; Sami Mujawar <sami.mujawar@arm.com>; > Sean > >>>> Rhodes <sean@starlabs.systems>; Sebastien Boeuf > >>>> <sebastien.boeuf@intel.com>; Tom Lendacky > <thomas.lendacky@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@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.PcdFlashNvStorageVariabl > eBase > >>>>>> > >>>> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 > >>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariabl > eSize > >>>>>> > >>>>>> 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/VariableNonVo > latile.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/FaultTolerant > Write.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.i > nf > >>>> | 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 (#89734): https://edk2.groups.io/g/devel/message/89734 Mute This Topic: https://groups.io/mt/91094643/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Yes, it has been reviewed by all maintainers. An announcement of the change was sent to edk2 on April 29th: https://edk2.groups.io/g/announce/message/291 The series for edk2-platforms has also been out since April 25th: https://edk2.groups.io/g/devel/message/89308 Thanks, Michael On 5/13/2022 9:16 PM, gaoliming wrote: > Michael: > This is my suggestion to resolve such compatible issue. As you say, it needs more review and discussion. So, it may not be applied immediately. > > If this patch set needs to catch this table tag, it has to take current way to update each DSC file. Have the patch set got reviewed-by from Package maintainers? If yes, I think this patch set can still be merged for this stable tag. > > Thanks > Liming >> -----邮件原件----- >> 发件人: Michael Kubacki <mikuback@linux.microsoft.com> >> 发送时间: 2022年5月14日 2:24 >> 收件人: gaoliming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io; 'Ard >> Biesheuvel' <ardb@kernel.org> >> 抄送: 'Abner Chang' <abner.chang@hpe.com>; 'Andrew Fish' >> <afish@apple.com>; 'Anthony Perard' <anthony.perard@citrix.com>; 'Ard >> Biesheuvel' <ardb+tianocore@kernel.org>; 'Benjamin You' >> <benjamin.you@intel.com>; 'Brijesh Singh' <brijesh.singh@amd.com>; 'Erdem >> Aktas' <erdemaktas@google.com>; 'Gerd Hoffmann' <kraxel@redhat.com>; >> 'Guo Dong' <guo.dong@intel.com>; 'Hao A Wu' <hao.a.wu@intel.com>; >> 'James Bottomley' <jejb@linux.ibm.com>; 'Jian J Wang' >> <jian.j.wang@intel.com>; 'Jiewen Yao' <jiewen.yao@intel.com>; 'Jordan >> Justen' <jordan.l.justen@intel.com>; 'Julien Grall' <julien@xen.org>; 'Leif >> Lindholm' <quic_llindhol@quicinc.com>; 'Maurice Ma' >> <maurice.ma@intel.com>; 'Min Xu' <min.m.xu@intel.com>; 'Nickle Wang' >> <nickle.wang@hpe.com>; 'Peter Grehan' <grehan@freebsd.org>; 'Ray Ni' >> <ray.ni@intel.com>; 'Rebecca Cran' <rebecca@bsdio.com>; 'Sami Mujawar' >> <sami.mujawar@arm.com>; 'Sean Rhodes' <sean@starlabs.systems>; >> 'Sebastien Boeuf' <sebastien.boeuf@intel.com>; 'Tom Lendacky' >> <thomas.lendacky@amd.com> >> 主题: Re: 回复: [edk2-devel] [PATCH v5 0/8] Add Variable Flash Info HOB >> >> Can you please respond with your preference? >> >> I am ready to do this but if it is required now, it should be documented >> so it becomes a consistent pattern for future changes. >> >> Thanks, >> Michael >> >> On 5/10/2022 11:01 AM, Michael Kubacki wrote: >>> 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@edk2.groups.io <devel@edk2.groups.io> 代表 >> Michael >>>>>> Kubacki >>>>>> 发送时间: 2022年4月29日 23:48 >>>>>> 收件人: Ard Biesheuvel <ardb@kernel.org> >>>>>> 抄送: edk2-devel-groups-io <devel@edk2.groups.io>; Abner Chang >>>>>> <abner.chang@hpe.com>; Andrew Fish <afish@apple.com>; Anthony >> Perard >>>>>> <anthony.perard@citrix.com>; Ard Biesheuvel >>>>>> <ardb+tianocore@kernel.org>; >>>>>> Benjamin You <benjamin.you@intel.com>; Brijesh Singh >>>>>> <brijesh.singh@amd.com>; Erdem Aktas <erdemaktas@google.com>; >> Gerd >>>>>> Hoffmann <kraxel@redhat.com>; Guo Dong <guo.dong@intel.com>; Hao >> A >>>>>> Wu <hao.a.wu@intel.com>; James Bottomley <jejb@linux.ibm.com>; >> Jian J >>>>>> Wang <jian.j.wang@intel.com>; Jiewen Yao <jiewen.yao@intel.com>; >> Jordan >>>>>> Justen <jordan.l.justen@intel.com>; Julien Grall <julien@xen.org>; Leif >>>>>> Lindholm <quic_llindhol@quicinc.com>; Liming Gao >>>>>> <gaoliming@byosoft.com.cn>; Maurice Ma <maurice.ma@intel.com>; >> Min Xu >>>>>> <min.m.xu@intel.com>; Nickle Wang <nickle.wang@hpe.com>; Peter >> Grehan >>>>>> <grehan@freebsd.org>; Ray Ni <ray.ni@intel.com>; Rebecca Cran >>>>>> <rebecca@bsdio.com>; Sami Mujawar <sami.mujawar@arm.com>; >> Sean >>>>>> Rhodes <sean@starlabs.systems>; Sebastien Boeuf >>>>>> <sebastien.boeuf@intel.com>; Tom Lendacky >> <thomas.lendacky@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@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.PcdFlashNvStorageVariabl >> eBase >>>>>>>> >>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 >>>>>>>> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariabl >> eSize >>>>>>>> >>>>>>>> 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/VariableNonVo >> latile.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/FaultTolerant >> Write.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.i >> nf >>>>>> | 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 (#89771): https://edk2.groups.io/g/devel/message/89771 Mute This Topic: https://groups.io/mt/91094643/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On Mon, 16 May 2022 at 17:27, Michael Kubacki <mikuback@linux.microsoft.com> wrote: > > Yes, it has been reviewed by all maintainers. An announcement of the > change was sent to edk2 on April 29th: > https://edk2.groups.io/g/announce/message/291 > > The series for edk2-platforms has also been out since April 25th: > https://edk2.groups.io/g/devel/message/89308 > > Thanks, > Michael > I am fine with merging this as is, but please merge the edk2-platforms changes as soon as the edk2 changes are in, so platforms there are not left in a broken state. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89778): https://edk2.groups.io/g/devel/message/89778 Mute This Topic: https://groups.io/mt/91094643/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Liming, will you push both? I put a rebased edk2-platforms with all the review tags for that series here: https://github.com/makubacki/edk2-platforms/commits/add_variableflashinfolib_to_platforms Thanks, Michael On 5/16/2022 1:36 PM, Ard Biesheuvel wrote: > On Mon, 16 May 2022 at 17:27, Michael Kubacki > <mikuback@linux.microsoft.com> wrote: >> >> Yes, it has been reviewed by all maintainers. An announcement of the >> change was sent to edk2 on April 29th: >> https://edk2.groups.io/g/announce/message/291 >> >> The series for edk2-platforms has also been out since April 25th: >> https://edk2.groups.io/g/devel/message/89308 >> >> Thanks, >> Michael >> > > I am fine with merging this as is, but please merge the edk2-platforms > changes as soon as the edk2 changes are in, so platforms there are not > left in a broken state. > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89794): https://edk2.groups.io/g/devel/message/89794 Mute This Topic: https://groups.io/mt/91094643/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Michael: Yes. I can. Where is the change for Edk2? Thanks Liming > -----邮件原件----- > 发件人: Michael Kubacki <mikuback@linux.microsoft.com> > 发送时间: 2022年5月17日 12:14 > 收件人: devel@edk2.groups.io; ardb@kernel.org > 抄送: gaoliming <gaoliming@byosoft.com.cn>; Abner Chang > <abner.chang@hpe.com>; Andrew Fish <afish@apple.com>; Anthony Perard > <anthony.perard@citrix.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; > Benjamin You <benjamin.you@intel.com>; Brijesh Singh > <brijesh.singh@amd.com>; Erdem Aktas <erdemaktas@google.com>; Gerd > Hoffmann <kraxel@redhat.com>; Guo Dong <guo.dong@intel.com>; Hao A > Wu <hao.a.wu@intel.com>; James Bottomley <jejb@linux.ibm.com>; Jian J > Wang <jian.j.wang@intel.com>; Jiewen Yao <jiewen.yao@intel.com>; Jordan > Justen <jordan.l.justen@intel.com>; Julien Grall <julien@xen.org>; Leif > Lindholm <quic_llindhol@quicinc.com>; Maurice Ma > <maurice.ma@intel.com>; Min Xu <min.m.xu@intel.com>; Nickle Wang > <nickle.wang@hpe.com>; Peter Grehan <grehan@freebsd.org>; Ray Ni > <ray.ni@intel.com>; Rebecca Cran <rebecca@bsdio.com>; Sami Mujawar > <sami.mujawar@arm.com>; Sean Rhodes <sean@starlabs.systems>; > Sebastien Boeuf <sebastien.boeuf@intel.com>; Tom Lendacky > <thomas.lendacky@amd.com> > 主题: Re: 回复: 回复: [edk2-devel] [PATCH v5 0/8] Add Variable Flash Info > HOB > > Liming, will you push both? > > I put a rebased edk2-platforms with all the review tags for that series > here: > https://github.com/makubacki/edk2-platforms/commits/add_variableflashinf > olib_to_platforms > > Thanks, > Michael > > On 5/16/2022 1:36 PM, Ard Biesheuvel wrote: > > On Mon, 16 May 2022 at 17:27, Michael Kubacki > > <mikuback@linux.microsoft.com> wrote: > >> > >> Yes, it has been reviewed by all maintainers. An announcement of the > >> change was sent to edk2 on April 29th: > >> https://edk2.groups.io/g/announce/message/291 > >> > >> The series for edk2-platforms has also been out since April 25th: > >> https://edk2.groups.io/g/devel/message/89308 > >> > >> Thanks, > >> Michael > >> > > > > I am fine with merging this as is, but please merge the edk2-platforms > > changes as soon as the edk2 changes are in, so platforms there are not > > left in a broken state. > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89800): https://edk2.groups.io/g/devel/message/89800 Mute This Topic: https://groups.io/mt/91157764/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
It is here: https://github.com/tianocore/edk2/pull/2828 I just rebased and pushed the branch. Thanks, Michael On 5/17/2022 1:22 AM, gaoliming wrote: > Michael: > Yes. I can. Where is the change for Edk2? > > Thanks > Liming >> -----邮件原件----- >> 发件人: Michael Kubacki <mikuback@linux.microsoft.com> >> 发送时间: 2022年5月17日 12:14 >> 收件人: devel@edk2.groups.io; ardb@kernel.org >> 抄送: gaoliming <gaoliming@byosoft.com.cn>; Abner Chang >> <abner.chang@hpe.com>; Andrew Fish <afish@apple.com>; Anthony Perard >> <anthony.perard@citrix.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; >> Benjamin You <benjamin.you@intel.com>; Brijesh Singh >> <brijesh.singh@amd.com>; Erdem Aktas <erdemaktas@google.com>; Gerd >> Hoffmann <kraxel@redhat.com>; Guo Dong <guo.dong@intel.com>; Hao A >> Wu <hao.a.wu@intel.com>; James Bottomley <jejb@linux.ibm.com>; Jian J >> Wang <jian.j.wang@intel.com>; Jiewen Yao <jiewen.yao@intel.com>; Jordan >> Justen <jordan.l.justen@intel.com>; Julien Grall <julien@xen.org>; Leif >> Lindholm <quic_llindhol@quicinc.com>; Maurice Ma >> <maurice.ma@intel.com>; Min Xu <min.m.xu@intel.com>; Nickle Wang >> <nickle.wang@hpe.com>; Peter Grehan <grehan@freebsd.org>; Ray Ni >> <ray.ni@intel.com>; Rebecca Cran <rebecca@bsdio.com>; Sami Mujawar >> <sami.mujawar@arm.com>; Sean Rhodes <sean@starlabs.systems>; >> Sebastien Boeuf <sebastien.boeuf@intel.com>; Tom Lendacky >> <thomas.lendacky@amd.com> >> 主题: Re: 回复: 回复: [edk2-devel] [PATCH v5 0/8] Add Variable Flash Info >> HOB >> >> Liming, will you push both? >> >> I put a rebased edk2-platforms with all the review tags for that series >> here: >> https://github.com/makubacki/edk2-platforms/commits/add_variableflashinf >> olib_to_platforms >> >> Thanks, >> Michael >> >> On 5/16/2022 1:36 PM, Ard Biesheuvel wrote: >>> On Mon, 16 May 2022 at 17:27, Michael Kubacki >>> <mikuback@linux.microsoft.com> wrote: >>>> >>>> Yes, it has been reviewed by all maintainers. An announcement of the >>>> change was sent to edk2 on April 29th: >>>> https://edk2.groups.io/g/announce/message/291 >>>> >>>> The series for edk2-platforms has also been out since April 25th: >>>> https://edk2.groups.io/g/devel/message/89308 >>>> >>>> Thanks, >>>> Michael >>>> >>> >>> I am fine with merging this as is, but please merge the edk2-platforms >>> changes as soon as the edk2 changes are in, so platforms there are not >>> left in a broken state. >>> >>> >>> >>> > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89821): https://edk2.groups.io/g/devel/message/89821 Mute This Topic: https://groups.io/mt/91157764/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
Michael: I create PR https://github.com/tianocore/edk2/pull/2908 for this patch set. Once it is merged, I will immediately update edk2-platforms. Thanks Liming > -----邮件原件----- > 发件人: Michael Kubacki <mikuback@linux.microsoft.com> > 发送时间: 2022年5月17日 21:07 > 收件人: devel@edk2.groups.io; gaoliming@byosoft.com.cn; ardb@kernel.org > 抄送: 'Abner Chang' <abner.chang@hpe.com>; 'Andrew Fish' > <afish@apple.com>; 'Anthony Perard' <anthony.perard@citrix.com>; 'Ard > Biesheuvel' <ardb+tianocore@kernel.org>; 'Benjamin You' > <benjamin.you@intel.com>; 'Brijesh Singh' <brijesh.singh@amd.com>; 'Erdem > Aktas' <erdemaktas@google.com>; 'Gerd Hoffmann' <kraxel@redhat.com>; > 'Guo Dong' <guo.dong@intel.com>; 'Hao A Wu' <hao.a.wu@intel.com>; > 'James Bottomley' <jejb@linux.ibm.com>; 'Jian J Wang' > <jian.j.wang@intel.com>; 'Jiewen Yao' <jiewen.yao@intel.com>; 'Jordan > Justen' <jordan.l.justen@intel.com>; 'Julien Grall' <julien@xen.org>; 'Leif > Lindholm' <quic_llindhol@quicinc.com>; 'Maurice Ma' > <maurice.ma@intel.com>; 'Min Xu' <min.m.xu@intel.com>; 'Nickle Wang' > <nickle.wang@hpe.com>; 'Peter Grehan' <grehan@freebsd.org>; 'Ray Ni' > <ray.ni@intel.com>; 'Rebecca Cran' <rebecca@bsdio.com>; 'Sami Mujawar' > <sami.mujawar@arm.com>; 'Sean Rhodes' <sean@starlabs.systems>; > 'Sebastien Boeuf' <sebastien.boeuf@intel.com>; 'Tom Lendacky' > <thomas.lendacky@amd.com> > 主题: Re: 回复: 回复: 回复: [edk2-devel] [PATCH v5 0/8] Add Variable > Flash Info HOB > > It is here: > https://github.com/tianocore/edk2/pull/2828 > > I just rebased and pushed the branch. > > Thanks, > Michael > > On 5/17/2022 1:22 AM, gaoliming wrote: > > Michael: > > Yes. I can. Where is the change for Edk2? > > > > Thanks > > Liming > >> -----邮件原件----- > >> 发件人: Michael Kubacki <mikuback@linux.microsoft.com> > >> 发送时间: 2022年5月17日 12:14 > >> 收件人: devel@edk2.groups.io; ardb@kernel.org > >> 抄送: gaoliming <gaoliming@byosoft.com.cn>; Abner Chang > >> <abner.chang@hpe.com>; Andrew Fish <afish@apple.com>; Anthony > Perard > >> <anthony.perard@citrix.com>; Ard Biesheuvel > <ardb+tianocore@kernel.org>; > >> Benjamin You <benjamin.you@intel.com>; Brijesh Singh > >> <brijesh.singh@amd.com>; Erdem Aktas <erdemaktas@google.com>; > Gerd > >> Hoffmann <kraxel@redhat.com>; Guo Dong <guo.dong@intel.com>; Hao A > >> Wu <hao.a.wu@intel.com>; James Bottomley <jejb@linux.ibm.com>; Jian > J > >> Wang <jian.j.wang@intel.com>; Jiewen Yao <jiewen.yao@intel.com>; > Jordan > >> Justen <jordan.l.justen@intel.com>; Julien Grall <julien@xen.org>; Leif > >> Lindholm <quic_llindhol@quicinc.com>; Maurice Ma > >> <maurice.ma@intel.com>; Min Xu <min.m.xu@intel.com>; Nickle Wang > >> <nickle.wang@hpe.com>; Peter Grehan <grehan@freebsd.org>; Ray Ni > >> <ray.ni@intel.com>; Rebecca Cran <rebecca@bsdio.com>; Sami Mujawar > >> <sami.mujawar@arm.com>; Sean Rhodes <sean@starlabs.systems>; > >> Sebastien Boeuf <sebastien.boeuf@intel.com>; Tom Lendacky > >> <thomas.lendacky@amd.com> > >> 主题: Re: 回复: 回复: [edk2-devel] [PATCH v5 0/8] Add Variable Flash > Info > >> HOB > >> > >> Liming, will you push both? > >> > >> I put a rebased edk2-platforms with all the review tags for that series > >> here: > >> > https://github.com/makubacki/edk2-platforms/commits/add_variableflashinf > >> olib_to_platforms > >> > >> Thanks, > >> Michael > >> > >> On 5/16/2022 1:36 PM, Ard Biesheuvel wrote: > >>> On Mon, 16 May 2022 at 17:27, Michael Kubacki > >>> <mikuback@linux.microsoft.com> wrote: > >>>> > >>>> Yes, it has been reviewed by all maintainers. An announcement of the > >>>> change was sent to edk2 on April 29th: > >>>> https://edk2.groups.io/g/announce/message/291 > >>>> > >>>> The series for edk2-platforms has also been out since April 25th: > >>>> https://edk2.groups.io/g/devel/message/89308 > >>>> > >>>> Thanks, > >>>> Michael > >>>> > >>> > >>> I am fine with merging this as is, but please merge the edk2-platforms > >>> changes as soon as the edk2 changes are in, so platforms there are not > >>> left in a broken state. > >>> > >>> > >>> > >>> > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89899): https://edk2.groups.io/g/devel/message/89899 Mute This Topic: https://groups.io/mt/91201685/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
A lot of new spelling errors began appearing in various packages in that PR (unrelated to this change). As a community service to unblock edk2 PRs, I have sent the following series to address these newly reported errors: https://edk2.groups.io/g/devel/message/89833 The PR with the CI results is available here: https://github.com/tianocore/edk2/pull/2903 Please help prioritize reviewing this so we can unblock changes. Thanks, Michael On 5/17/2022 9:06 AM, Michael Kubacki wrote: > It is here: > https://github.com/tianocore/edk2/pull/2828 > > I just rebased and pushed the branch. > > Thanks, > Michael > > On 5/17/2022 1:22 AM, gaoliming wrote: >> Michael: >> Yes. I can. Where is the change for Edk2? >> >> Thanks >> Liming >>> -----邮件原件----- >>> 发件人: Michael Kubacki <mikuback@linux.microsoft.com> >>> 发送时间: 2022年5月17日 12:14 >>> 收件人: devel@edk2.groups.io; ardb@kernel.org >>> 抄送: gaoliming <gaoliming@byosoft.com.cn>; Abner Chang >>> <abner.chang@hpe.com>; Andrew Fish <afish@apple.com>; Anthony Perard >>> <anthony.perard@citrix.com>; Ard Biesheuvel <ardb+tianocore@kernel.org>; >>> Benjamin You <benjamin.you@intel.com>; Brijesh Singh >>> <brijesh.singh@amd.com>; Erdem Aktas <erdemaktas@google.com>; Gerd >>> Hoffmann <kraxel@redhat.com>; Guo Dong <guo.dong@intel.com>; Hao A >>> Wu <hao.a.wu@intel.com>; James Bottomley <jejb@linux.ibm.com>; Jian J >>> Wang <jian.j.wang@intel.com>; Jiewen Yao <jiewen.yao@intel.com>; Jordan >>> Justen <jordan.l.justen@intel.com>; Julien Grall <julien@xen.org>; Leif >>> Lindholm <quic_llindhol@quicinc.com>; Maurice Ma >>> <maurice.ma@intel.com>; Min Xu <min.m.xu@intel.com>; Nickle Wang >>> <nickle.wang@hpe.com>; Peter Grehan <grehan@freebsd.org>; Ray Ni >>> <ray.ni@intel.com>; Rebecca Cran <rebecca@bsdio.com>; Sami Mujawar >>> <sami.mujawar@arm.com>; Sean Rhodes <sean@starlabs.systems>; >>> Sebastien Boeuf <sebastien.boeuf@intel.com>; Tom Lendacky >>> <thomas.lendacky@amd.com> >>> 主题: Re: 回复: 回复: [edk2-devel] [PATCH v5 0/8] Add Variable Flash >>> Info >>> HOB >>> >>> Liming, will you push both? >>> >>> I put a rebased edk2-platforms with all the review tags for that series >>> here: >>> https://github.com/makubacki/edk2-platforms/commits/add_variableflashinf >>> olib_to_platforms >>> >>> Thanks, >>> Michael >>> >>> On 5/16/2022 1:36 PM, Ard Biesheuvel wrote: >>>> On Mon, 16 May 2022 at 17:27, Michael Kubacki >>>> <mikuback@linux.microsoft.com> wrote: >>>>> >>>>> Yes, it has been reviewed by all maintainers. An announcement of the >>>>> change was sent to edk2 on April 29th: >>>>> https://edk2.groups.io/g/announce/message/291 >>>>> >>>>> The series for edk2-platforms has also been out since April 25th: >>>>> https://edk2.groups.io/g/devel/message/89308 >>>>> >>>>> Thanks, >>>>> Michael >>>>> >>>> >>>> I am fine with merging this as is, but please merge the edk2-platforms >>>> changes as soon as the edk2 changes are in, so platforms there are not >>>> left in a broken state. >>>> >>>> >>>> >>>> >> >> >> >> >> >> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#89843): https://edk2.groups.io/g/devel/message/89843 Mute This Topic: https://groups.io/mt/91157764/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.