-----邮件原件-----
发件人: Michael Kubacki <mikub...@linux.microsoft.com>
发送时间: 2022年5月14日 2:24
收件人: gaoliming <gaolim...@byosoft.com.cn>; devel@edk2.groups.io; 'Ard
Biesheuvel' <a...@kernel.org>
抄送: 'Abner Chang' <abner.ch...@hpe.com>; 'Andrew Fish'
<af...@apple.com>; 'Anthony Perard' <anthony.per...@citrix.com>; 'Ard
Biesheuvel' <ardb+tianoc...@kernel.org>; 'Benjamin You'
<benjamin....@intel.com>; 'Brijesh Singh' <brijesh.si...@amd.com>; 'Erdem
Aktas' <erdemak...@google.com>; 'Gerd Hoffmann' <kra...@redhat.com>;
'Guo Dong' <guo.d...@intel.com>; 'Hao A Wu' <hao.a...@intel.com>;
'James Bottomley' <j...@linux.ibm.com>; 'Jian J Wang'
<jian.j.w...@intel.com>; 'Jiewen Yao' <jiewen....@intel.com>; 'Jordan
Justen' <jordan.l.jus...@intel.com>; 'Julien Grall' <jul...@xen.org>; 'Leif
Lindholm' <quic_llind...@quicinc.com>; 'Maurice Ma'
<maurice...@intel.com>; 'Min Xu' <min.m...@intel.com>; 'Nickle Wang'
<nickle.w...@hpe.com>; 'Peter Grehan' <gre...@freebsd.org>; 'Ray Ni'
<ray...@intel.com>; 'Rebecca Cran' <rebe...@bsdio.com>; 'Sami Mujawar'
<sami.muja...@arm.com>; 'Sean Rhodes' <sean@starlabs.systems>;
'Sebastien Boeuf' <sebastien.bo...@intel.com>; 'Tom Lendacky'
<thomas.lenda...@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 <a...@kernel.org>
抄送: edk2-devel-groups-io <devel@edk2.groups.io>; Abner Chang
<abner.ch...@hpe.com>; Andrew Fish <af...@apple.com>; Anthony
Perard
<anthony.per...@citrix.com>; Ard Biesheuvel
<ardb+tianoc...@kernel.org>;
Benjamin You <benjamin....@intel.com>; Brijesh Singh
<brijesh.si...@amd.com>; Erdem Aktas <erdemak...@google.com>;
Gerd
Hoffmann <kra...@redhat.com>; Guo Dong <guo.d...@intel.com>; Hao
A
Wu <hao.a...@intel.com>; James Bottomley <j...@linux.ibm.com>;
Jian J
Wang <jian.j.w...@intel.com>; Jiewen Yao <jiewen....@intel.com>;
Jordan
Justen <jordan.l.jus...@intel.com>; Julien Grall <jul...@xen.org>; Leif
Lindholm <quic_llind...@quicinc.com>; Liming Gao
<gaolim...@byosoft.com.cn>; Maurice Ma <maurice...@intel.com>;
Min Xu
<min.m...@intel.com>; Nickle Wang <nickle.w...@hpe.com>; Peter
Grehan
<gre...@freebsd.org>; Ray Ni <ray...@intel.com>; Rebecca Cran
<rebe...@bsdio.com>; Sami Mujawar <sami.muja...@arm.com>;
Sean
Rhodes <sean@starlabs.systems>; Sebastien Boeuf
<sebastien.bo...@intel.com>; Tom Lendacky
<thomas.lenda...@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, <mikub...@linux.microsoft.com>
wrote:
From: Michael Kubacki <michael.kuba...@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.ch...@hpe.com>
Cc: Andrew Fish <af...@apple.com>
Cc: Anthony Perard <anthony.per...@citrix.com>
Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>
Cc: Benjamin You <benjamin....@intel.com>
Cc: Brijesh Singh <brijesh.si...@amd.com>
Cc: Erdem Aktas <erdemak...@google.com>
Cc: Gerd Hoffmann <kra...@redhat.com>
Cc: Guo Dong <guo.d...@intel.com>
Cc: Hao A Wu <hao.a...@intel.com>
Cc: James Bottomley <j...@linux.ibm.com>
Cc: Jian J Wang <jian.j.w...@intel.com>
Cc: Jiewen Yao <jiewen....@intel.com>
Cc: Jordan Justen <jordan.l.jus...@intel.com>
Cc: Julien Grall <jul...@xen.org>
Cc: Leif Lindholm <quic_llind...@quicinc.com>
Cc: Liming Gao <gaolim...@byosoft.com.cn>
Cc: Maurice Ma <maurice...@intel.com>
Cc: Min Xu <min.m...@intel.com>
Cc: Nickle Wang <nickle.w...@hpe.com>
Cc: Peter Grehan <gre...@freebsd.org>
Cc: Ray Ni <ray...@intel.com>
Cc: Rebecca Cran <rebe...@bsdio.com>
Cc: Sami Mujawar <sami.muja...@arm.com>
Cc: Sean Rhodes <sean@starlabs.systems>
Cc: Sebastien Boeuf <sebastien.bo...@intel.com>
Cc: Tom Lendacky <thomas.lenda...@amd.com>
Signed-off-by: Michael Kubacki <michael.kuba...@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
