From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.byosoft.com.cn (mail.byosoft.com.cn [58.240.74.242]) by mx.groups.io with SMTP id smtpd.web09.5695.1651714064035625928 for ; Wed, 04 May 2022 18:27:47 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=none, err=permanent DNS error (domain: byosoft.com.cn, ip: 58.240.74.242, mailfrom: gaoliming@byosoft.com.cn) Received: from DESKTOPS6D0PVI ([101.224.116.119]) (envelope-sender ) by 192.168.6.13 with ESMTP for ; Thu, 05 May 2022 09:26:59 +0800 X-WM-Sender: gaoliming@byosoft.com.cn X-Originating-IP: 101.224.116.119 X-WM-AuthFlag: YES X-WM-AuthUser: gaoliming@byosoft.com.cn From: "gaoliming" To: , , "'Ard Biesheuvel'" Cc: "'Abner Chang'" , "'Andrew Fish'" , "'Anthony Perard'" , "'Ard Biesheuvel'" , "'Benjamin You'" , "'Brijesh Singh'" , "'Erdem Aktas'" , "'Gerd Hoffmann'" , "'Guo Dong'" , "'Hao A Wu'" , "'James Bottomley'" , "'Jian J Wang'" , "'Jiewen Yao'" , "'Jordan Justen'" , "'Julien Grall'" , "'Leif Lindholm'" , "'Maurice Ma'" , "'Min Xu'" , "'Nickle Wang'" , "'Peter Grehan'" , "'Ray Ni'" , "'Rebecca Cran'" , "'Sami Mujawar'" , "'Sean Rhodes'" , "'Sebastien Boeuf'" , "'Tom Lendacky'" References: <20220426012918.1216-1-mikuback@linux.microsoft.com> In-Reply-To: Subject: =?UTF-8?B?5Zue5aSNOiBbZWRrMi1kZXZlbF0gW1BBVENIIHY1IDAvOF0gQWRkIFZhcmlhYmxlIEZsYXNoIEluZm8gSE9C?= Date: Thu, 5 May 2022 09:27:02 +0800 Message-ID: <00d401d8601f$3981cc10$ac856430$@byosoft.com.cn> MIME-Version: 1.0 X-Mailer: Microsoft Outlook 16.0 Thread-Index: AQI0Yy49haEnV8mscETCKlnDeBIkewJk2TNWAXn9Ob+sOIB5YA== Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Content-Language: zh-cn Michael: I would suggest to reuse MdePkg/MdeLibs.dsc.inc to list the library and P= CD from the edk2 core packages, such as MdePkg, MdeModulePkg, CryptoPkg, Se= curirtyPkg 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 pac= kages, not only for MdePkg.=20 Thanks Liming > -----=E9=82=AE=E4=BB=B6=E5=8E=9F=E4=BB=B6----- > =E5=8F=91=E4=BB=B6=E4=BA=BA: devel@edk2.groups.io = =E4=BB=A3=E8=A1=A8 Michael > Kubacki > =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: 2022=E5=B9=B44=E6=9C=8829=E6=97=A5 = 23:48 > =E6=94=B6=E4=BB=B6=E4=BA=BA: Ard Biesheuvel > =E6=8A=84=E9=80=81: edk2-devel-groups-io ; Abner Ch= ang > ; Andrew Fish ; Anthony Perard > ; Ard Biesheuvel ; > Benjamin You ; Brijesh Singh > ; Erdem Aktas ; Gerd > Hoffmann ; Guo Dong ; Hao A > Wu ; James Bottomley ; Jian J > Wang ; Jiewen Yao ; Jordan > Justen ; Julien Grall ; Leif > Lindholm ; Liming Gao > ; Maurice Ma ; Min Xu > ; Nickle Wang ; Peter Grehan > ; Ray Ni ; Rebecca Cran > ; Sami Mujawar ; Sean > Rhodes ; Sebastien Boeuf > ; Tom Lendacky > =E4=B8=BB=E9=A2=98: Re: [edk2-devel] [PATCH v5 0/8] Add Variable Flash In= fo HOB >=20 > 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. >=20 > 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. >=20 > 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. >=20 > While includes be helpful, it can encourage platform owners to ignore > potential creep in functionality they should be aware of. >=20 > 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. >=20 > 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. >=20 > Again, I think this conversation is useful but requires much more time > to address questions such as the following: >=20 > 1. Should a different mechanism for default library classes be introduced= ? >=20 > 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)? >=20 > 3. Which library classes for a given package should be given default > instances? >=20 > 4. How would each platform package maintainer like to maintain their > platform build? >=20 > Example: Would MinPlatformPkg prefer to keep its own "core" include > files > (https://github.com/tianocore/edk2-platforms/tree/master/Platform/Intel/M= i > 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? >=20 > How would someone updating the platform code due to an edk2 change be > aware of this per-platform policy? >=20 > To my understanding, the answers to these today are: >=20 > 1. No > 2. No / decided on per-change basis > 3. Unknown > 4. Unknown >=20 > (2) adds friction to the edk2 contribution process as expectation is > unclear. >=20 > (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). >=20 > (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. >=20 > Regards, > Michael >=20 > On 4/29/2022 9:45 AM, Ard Biesheuvel wrote: > > On Tue, 26 Apr 2022 at 03:29, wrote: > >> > >> From: Michael Kubacki > >> > >> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3D3479 > >> > >> 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 > >> Cc: Andrew Fish > >> Cc: Anthony Perard > >> Cc: Ard Biesheuvel > >> Cc: Benjamin You > >> Cc: Brijesh Singh > >> Cc: Erdem Aktas > >> Cc: Gerd Hoffmann > >> Cc: Guo Dong > >> Cc: Hao A Wu > >> Cc: James Bottomley > >> Cc: Jian J Wang > >> Cc: Jiewen Yao > >> Cc: Jordan Justen > >> Cc: Julien Grall > >> Cc: Leif Lindholm > >> Cc: Liming Gao > >> Cc: Maurice Ma > >> Cc: Min Xu > >> Cc: Nickle Wang > >> Cc: Peter Grehan > >> Cc: Ray Ni > >> Cc: Rebecca Cran > >> Cc: Sami Mujawar > >> Cc: Sean Rhodes > >> Cc: Sebastien Boeuf > >> Cc: Tom Lendacky > >> Signed-off-by: Michael Kubacki > >> > >> 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 > >> >=20 >=20 >=20 >=20