From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web08.4896.1651801931919728214 for ; Thu, 05 May 2022 18:52:12 -0700 Authentication-Results: mx.groups.io; dkim=fail reason="body hash did not verify" header.i=@linux.microsoft.com header.s=default header.b=rJ2LIFx6; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [192.168.4.22] (unknown [47.195.228.134]) by linux.microsoft.com (Postfix) with ESMTPSA id 80CCC20EB315; Thu, 5 May 2022 18:52:08 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 80CCC20EB315 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1651801931; bh=8u2BMYBo8kfbFZsdEgMw4eHYWt8bgtDQlXVwsK25Wao=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=rJ2LIFx6ZhcwY/S6ZFWI079Pw+XyBuIYYCBspqRZ6yDZJJVkU5BBqBc7TIlKrd03s Cp9vngtL+WyeOtAqSr96Nj1/qELtO4GrXHy81OBbcUhELcmOPf8ybYbEoC329MtGDi +TXSU8b/QKK+b9S19CnUjJ3orEZYos7Gblci73T4= Message-ID: <5e04954f-271e-b7b5-cd58-456310c4030b@linux.microsoft.com> Date: Thu, 5 May 2022 21:52:07 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.8.1 Subject: =?UTF-8?B?UmU6IOWbnuWkjTogW2VkazItZGV2ZWxdIFtQQVRDSCB2NSAwLzhdIEFkZCBWYXJpYWJsZSBGbGFzaCBJbmZvIEhPQg==?= To: gaoliming , devel@edk2.groups.io, '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> <00d401d8601f$3981cc10$ac856430$@byosoft.com.cn> From: "Michael Kubacki" In-Reply-To: <00d401d8601f$3981cc10$ac856430$@byosoft.com.cn> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable I still believe a long term design pattern deserves more focus and=20 documentation than a quick modification to this series. Can you confirm that you envision MdePkg/MdeLibs.dsc.inc serving as a=20 monolithic host of various other default library class instances? That somewhat inverts the package relationships, the code reviewer=20 policy would need to clarify when the original package owners are=20 included on the MdePkg patch (to confirm they agree with the default=20 instance choice), and "core" packages would have to be clearly defined=20 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=20 platform integration thrash. For example, if a new library class=20 instance added to MdePkg/MdeLibs.dsc.inc requires another library class=20 (or multiple others), those might not be added to the DSC include file.=20 They could have been satisfied in the original package DSC (or a test=20 platform DSC) but that doesn't mean they will be in all platform DSC=20 files. So when the MdeLibs.dsc.inc file update occurs, those platforms=20 break and need to add the library class that was already specified in=20 other DSC files. So I request that if this is the preferred approach, that it be agreed=20 upon (e.g. dedicated RFC), documented, and consistently followed by=20 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 an= d PCD from the edk2 core packages, such as MdePkg, MdeModulePkg, CryptoPkg,= SecurirtyPkg and so on. Those packages are required by every platforms. Th= ey can't be separated. So, I think MdePkg/MdeLibs.dsc.inc is for edk2 core = packages, 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 C= hang >> ; 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 I= nfo 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 introduce= d? >> >> 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, 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