* A proposal to reduce incompatible case
@ 2020-11-20 6:51 Zhiguang Liu
2020-11-20 7:20 ` Yao, Jiewen
0 siblings, 1 reply; 11+ messages in thread
From: Zhiguang Liu @ 2020-11-20 6:51 UTC (permalink / raw)
To: devel@edk2.groups.io, michael.kubacki@outlook.com,
awarkentin@vmware.com, Ard Biesheuvel, debtech@gmail.com,
Feng, Bob C, Tian, Hot
Cc: Bret Barkelew, Yao, Jiewen, Bi, Dandan, Chao Zhang, Wang, Jian J,
Wu, Hao A, Liming Gao, Justen, Jordan L, Laszlo Ersek,
Andrew Fish, Ni, Ray, Bret Barkelew, Kinney, Michael D,
Liming Gao
Hi all,
As Michael mentioned, there are some platforms do not build and some is because incompatible code change like this one.
I think it is a burden for both contributor and maintainer to fix platform code when meeting such incompatible change.
I want to proposal one solution to minimum the effort of such code change.
We could add a package library instance dsc include file under each package, like XXXPkgLib.dsc.inc
It will specify the default library instance that will be used by modules in this package.
For example, we add MdeModulePkgLib.dsc.inc file in MdeModulePkg.
Some package already has similar dsc include file, such as ArmVirtPkg/ArmVirt.dsc.inc and NetworkPkg\Network.dsc.inc.
In platform dsc file, we include the XXXPkgLib.dsc.inc file at the beginning, if the platform uses component from the package.
We place the inc file in the beginning because we can override the library instance in other part of the platform dsc file.
Whenever the contributor adds a new library dependency in one module, he should also add a default library instance in the package library instance dsc include file.
For example, in this case,
Contributor will add the below information in UefiPayloadPkgLib.dsc.inc, SecurityPkgLib.dsc.inc and MdeModulePkgLib.dsc.inc
[LibraryClasses]
VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf
VariablePolicyHelperLib|MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.inf
[LibraryClasses.common.DXE_RUNTIME_DRIVER]
VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe.inf
If the platform already includes these inc files, the code change won't break any build.
If the platform wants to choose another library instance, it can specify in the dsc file, and will override the configuration in inc files.
This feature can even reduce the code in platform dsc file if platform choose to use default library instance.
The problem is that it may compiles redundant modules if the
Please give comments about this proposal.
Thanks
Zhiguang
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael
> Kubacki
> Sent: Friday, November 20, 2020 4:16 AM
> To: devel@edk2.groups.io; awarkentin@vmware.com; Ard Biesheuvel
> <ard.biesheuvel@arm.com>; debtech@gmail.com
> Cc: Bret Barkelew <bret@corthon.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> Bi, Dandan <dandan.bi@intel.com>; Chao Zhang <chao.b.zhang@intel.com>;
> Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Liming Gao <liming.gao@intel.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Andrew Fish
> <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Bret Barkelew
> <brbarkel@microsoft.com>
> Subject: Re: [edk2-devel] [PATCH v9 00/13] Add the VariablePolicy feature
>
> While I'm not currently a maintainer in either repo, I believe the current
> process is not ideal. I highlighted some of my observations
> here: https://edk2.groups.io/g/devel/message/65902.
>
> Again, I don't have a strong vested interest in this but I do think some level of a
> more well defined process needs to be reached between repo maintiners to
> ease feature development in the future.
>
> Thanks,
> Michael
>
> On 11/19/2020 12:02 PM, Andrei Warkentin wrote:
> > Hi Bret,
> >
> > To be honest, I don't recall seeing anything. Again, maybe I should
> > have been more proactive, but that's probably the net reality for most
> > people. It would be unreasonable to expect you to test every platform,
> > but it is very reasonable to assume that if you know you're adding
> > build breakage to every platform (that is trivial to fix), that you
> > would be taking care of it... Principle of least surprise. And yes, in
> > some weird corner case perhaps that would be insufficient (again, I
> > don't think anyone would expect you to compile test every platform),
> > but it would take care of 99% of obvious fall-out.
> >
> > For reference, there are occasional clean-ups that happen to the edk2
> > tree, and I've never seen anyone claim "not my problem" to deal with
> > the obvious fall-out resulting from renames and such.
> >
> > A
> > ----------------------------------------------------------------------
> > --
> > *From:* devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Bret
> > Barkelew via groups.io <debtech=gmail.com@groups.io>
> > *Sent:* Thursday, November 19, 2020 10:15 AM
> > *To:* Ard Biesheuvel <ard.biesheuvel@arm.com>
> > *Cc:* Bret Barkelew <bret@corthon.com>; devel@edk2.groups.io
> > <devel@edk2.groups.io>; Jiewen Yao <jiewen.yao@intel.com>; Dandan Bi
> > <dandan.bi@intel.com>; Chao Zhang <chao.b.zhang@intel.com>; Jian J
> > Wang <jian.j.wang@intel.com>; Hao A Wu <hao.a.wu@intel.com>; Liming
> > Gao <liming.gao@intel.com>; Jordan Justen <jordan.l.justen@intel.com>;
> > Laszlo Ersek <lersek@redhat.com>; Andrew Fish <afish@apple.com>; Ray
> > Ni <ray.ni@intel.com>; Bret Barkelew <brbarkel@microsoft.com>
> > *Subject:* Re: [edk2-devel] [PATCH v9 00/13] Add the VariablePolicy
> > feature Those bugs and recommendations were sent out months ago.
> > Several platforms have staged the changes already.
> >
> > You need to add the library class to your DSC.
> >
> > --
> > [ Insert obscure pop-culture reference here. ]
> >
> >> On Nov 19, 2020, at 4:46 AM, Ard Biesheuvel <ard.biesheuvel@arm.com>
> wrote:
> >>
> >> On 11/9/20 7:45 AM, Bret Barkelew wrote:
> >>> The 14 patches in this series add the VariablePolicy feature to the
> >>> core, deprecate Edk2VarLock (while adding a compatibility layer to
> >>> reduce code churn), and integrate the VariablePolicy libraries and
> >>> protocols into Variable Services.
> >>> Since the integration requires multiple changes, including adding
> >>> libraries, a protocol, an SMI communication handler, and
> >>> VariableServices integration, the patches are broken up by
> >>> individual library additions and then a final integration.
> >>> Security-sensitive changes like bypassing Authenticated Variable
> >>> enforcement are also broken out into individual patches so that attention
> can be called directly to them.
> >>> Platform porting instructions are described in this wiki entry:
> >>> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> >>> thub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FVariablePolicy-P
> >>> rotocol---Enhanced-Method-for-Managing-Variables%23platform-porting&
> >>>
> amp;data=04%7C01%7Cawarkentin%40vmware.com%7C594f15b45aaf476bff7e
> 08d
> >>>
> 88cb57390%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C63741405
> 82471
> >>>
> 28819%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> zIiLC
> >>>
> JBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=LLKZ7qeffR0WCvLbYuH
> tQI
> >>> uwJGhXY0mVqB2w9B0q180%3D&reserved=0
> > <https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit
> > hub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FVariablePolicy-Prot
> > ocol---Enhanced-Method-for-Managing-Variables%23platform-
> porting&d
> >
> ata=04%7C01%7Cawarkentin%40vmware.com%7C594f15b45aaf476bff7e08d88
> cb573
> >
> 90%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637414058247128
> 819%7CU
> >
> nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik
> 1ha
> >
> WwiLCJXVCI6Mn0%3D%7C1000&sdata=LLKZ7qeffR0WCvLbYuHtQIuwJGhX
> Y0mVqB2
> > w9B0q180%3D&reserved=0>
> >>> Discussion of the feature can be found in multiple places throughout
> >>> the last year on the RFC channel, staging branches, and in devel.
> >>> Most recently, this subject was discussed in this thread:
> >>> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fed
> >>>
> k2.groups.io%2Fg%2Fdevel%2Fmessage%2F53712&data=04%7C01%7Cawa
> rke
> >>>
> ntin%40vmware.com%7C594f15b45aaf476bff7e08d88cb57390%7Cb39138ca3c
> ee4
> >>>
> b4aa4d6cd83d9dd62f0%7C0%7C0%7C637414058247133820%7CUnknown%7CT
> WFpbGZ
> >>>
> sb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0
> >>> %3D%7C1000&sdata=GYY52rlsPxw07vfdu%2BVbWhzRjtHWXlIGveCTT
> 17mlfc%3
> >>> D&reserved=0
> > <https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk
> >
> 2.groups.io%2Fg%2Fdevel%2Fmessage%2F53712&data=04%7C01%7Cawar
> kenti
> >
> n%40vmware.com%7C594f15b45aaf476bff7e08d88cb57390%7Cb39138ca3cee
> 4b4aa4
> >
> d6cd83d9dd62f0%7C0%7C0%7C637414058247133820%7CUnknown%7CTWFpb
> GZsb3d8ey
> >
> JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7
> C100
> >
> 0&sdata=GYY52rlsPxw07vfdu%2BVbWhzRjtHWXlIGveCTT17mlfc%3D&
> ;reser
> > ved=0>
> >>> (the code branches shared in that discussion are now out of date,
> >>> but the whitepapers and discussion are relevant).
> >>> Cc: Jiewen Yao <jiewen.yao@intel.com>
> >>> Cc: Dandan Bi <dandan.bi@intel.com>
> >>> Cc: Chao Zhang <chao.b.zhang@intel.com>
> >>> Cc: Jian J Wang <jian.j.wang@intel.com>
> >>> Cc: Hao A Wu <hao.a.wu@intel.com>
> >>> Cc: Liming Gao <liming.gao@intel.com>
> >>> Cc: Jordan Justen <jordan.l.justen@intel.com>
> >>> Cc: Laszlo Ersek <lersek@redhat.com>
> >>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> >>> Cc: Andrew Fish <afish@apple.com>
> >>> Cc: Ray Ni <ray.ni@intel.com>
> >>> Cc: Bret Barkelew <brbarkel@microsoft.com>
> >>> Signed-off-by: Bret Barkelew <brbarkel@microsoft.com>
> >>
> >> This series has now made it into edk2, and has subsequently broken every
> single platform in edk2-platforms. Is anyone intending to propose any fixes for
> this?
> >>
> >>
> >>> v9 changes:
> >>> * Rebase
> >>> * Address the event ordering issues around MorLock at EndOfDxe
> >>> * Drop problematic tests
> >>> * Address ECC issues
> >>> v8 changes:
> >>> * Rebase
> >>> * Small tweaks from final PRs
> >>> * Drank a lot
> >>> * Enrolled several members and a steward in CatFacts
> >>> v7 changes:
> >>> * Address comments from Dandan about security of the MM handler
> >>> * Add readme
> >>> * Fix bug around hex characters in BOOT####, etc
> >>> * Add additional testing for hex characters
> >>> * Add additional testing for authenticated variables
> >>> v6 changes:
> >>> * Fix an issue with uninitialized Status in InitVariablePolicyLib()
> >>>and DeinitVariablePolicyLib()
> >>> * Fix GCC building in shell-based functional test
> >>> * Rebase on latest origin/master
> >>> v5 changes:
> >>> * Fix the CONST mismatch in VariablePolicy.h and
> >>>VariablePolicySmmDxe.c
> >>> * Fix EFIAPI mismatches in the functional unittest
> >>> * Rebase on latest origin/master
> >>> v4 changes:
> >>> * Remove Optional PcdAllowVariablePolicyEnforcementDisable PCD from
> >>>platforms
> >>> * Rebase on master
> >>> * Migrate to new MmCommunicate2 protocol
> >>> * Fix an oversight in the default return value for
> >>>InitMmCommonCommBuffer
> >>> * Fix in VariablePolicyLib to allow ExtraInitRuntimeDxe to consume
> >>>variables
> >>> V3 changes:
> >>> * Address all non-unittest issues with ECC
> >>> * Make additional style changes
> >>> * Include section name in hunk headers in "ini-style" files
> >>> * Remove requirement for the EdkiiPiSmmCommunicationsRegionTable
> >>>driver
> >>> (now allocates its own buffer)
> >>> * Change names from VARIABLE_POLICY_PROTOCOL and
> >>>gVariablePolicyProtocolGuid
> >>> to EDKII_VARIABLE_POLICY_PROTOCOL and
> >>>gEdkiiVariablePolicyProtocolGuid
> >>> * Fix GCC warning about initializing externs
> >>> * Add UNI strings for new PCD
> >>> * Add patches for ArmVirtPkg, OvmfXen, and UefiPayloadPkg
> >>> * Reorder patches according to Liming's feedback about adding to
> >>>platforms
> >>> before changing variable driver
> >>> V2 changes:
> >>> * Fixed implementation for RuntimeDxe
> >>> * Add PCD to block DisableVariablePolicy
> >>> * Fix the DumpVariablePolicy pagination in SMM Bret Barkelew (13):
> >>> MdeModulePkg: Define the VariablePolicy protocol interface
> >>> MdeModulePkg: Define the VariablePolicyLib
> >>> MdeModulePkg: Define the VariablePolicyHelperLib
> >>> MdeModulePkg: Define the VarCheckPolicyLib and SMM interface
> >>> OvmfPkg: Add VariablePolicy engine to OvmfPkg platform
> >>> EmulatorPkg: Add VariablePolicy engine to EmulatorPkg platform
> >>> ArmVirtPkg: Add VariablePolicy engine to ArmVirtPkg platform
> >>> UefiPayloadPkg: Add VariablePolicy engine to UefiPayloadPkg
> >>>platform
> >>> MdeModulePkg: Connect VariablePolicy business logic to
> >>> VariableServices
> >>> MdeModulePkg: Allow VariablePolicy state to delete protected
> >>>variables
> >>> SecurityPkg: Allow VariablePolicy state to delete authenticated
> >>> variables
> >>> MdeModulePkg: Change TCG MOR variables to use VariablePolicy
> >>> MdeModulePkg: Drop VarLock from RuntimeDxe variable driver
> >>> MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c
> >>>| 346 ++++++++
> >>>
> >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.
> >>>c | 396 ++++++++++
> >>>
> >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitNull.c
> >>>| 46 ++
> >>>
> >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitRuntime
> >>>Dxe.c | 85 ++
> >>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c
> >>>| 830 ++++++++++++++++++++
> >>> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
>
> >>>| 52 +-
> >>> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
>
> >>>| 60 +-
> >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VarCheck.c
> >>>| 49 +-
> >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
> >>>| 60 ++
> >>>
> >>>MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToLo
> ck.
> >>>c | 71 ++
> >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c
>
> >>>| 573 ++++++++++++++
> >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
>
> >>>| 7 +
> >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx
> e.c
> >>>| 14 +
> >>> SecurityPkg/Library/AuthVariableLib/AuthService.c
> >>>| 30 +-
> >>> ArmVirtPkg/ArmVirt.dsc.inc
> >>>| 4 +
> >>> EmulatorPkg/EmulatorPkg.dsc
> >>>| 3 +
> >>> MdeModulePkg/Include/Guid/VarCheckPolicyMmi.h
> >>>| 54 ++
> >>> MdeModulePkg/Include/Library/VariablePolicyHelperLib.h
> >>>| 164 ++++
> >>> MdeModulePkg/Include/Library/VariablePolicyLib.h
> >>>| 207 +++++
> >>> MdeModulePkg/Include/Protocol/VariablePolicy.h
> >>>| 157 ++++
> >>> MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
> >>>| 42 +
> >>> MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.uni
> >>>| 12 +
> >>>
> >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.
> >>>inf | 35 +
> >>>
> >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.
> >>>uni | 12 +
> >>> MdeModulePkg/Library/VariablePolicyLib/ReadMe.md
> >>>| 406 ++++++++++
> >>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf
> >>>| 48 ++
> >>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.uni
> >>>| 12 +
> >>>
> >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe.in
> >>>f | 51 ++
> >>> MdeModulePkg/MdeModulePkg.ci.yaml
> >>>| 4 +-
> >>> MdeModulePkg/MdeModulePkg.dec
> >>>| 26 +-
> >>> MdeModulePkg/MdeModulePkg.dsc
> >>>| 9 +
> >>> MdeModulePkg/MdeModulePkg.uni
> >>>| 7 +
> >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
>
> >>>| 5 +
> >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
>
> >>>| 4 +
> >>>
> >>>MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.
> inf
> >>>| 11 +
> >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.i
> nf
> >>>| 4 +
> >>> OvmfPkg/OvmfPkgIa32.dsc
> >>>| 5 +
> >>> OvmfPkg/OvmfPkgIa32X64.dsc
> >>>| 5 +
> >>> OvmfPkg/OvmfPkgX64.dsc
> >>>| 5 +
> >>> OvmfPkg/OvmfXen.dsc
> >>>| 4 +
> >>> SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
> >>>| 2 +
> >>> UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> >>>| 4 +
> >>> UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> >>>| 4 +
> >>> 43 files changed, 3845 insertions(+), 80 deletions(-)
> >>> create mode 100644
> >>>MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c
> >>> create mode 100644
> >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.
> >>>c
> >>> create mode 100644
> >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitNull.c
> >>> create mode 100644
> >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitRuntime
> >>>Dxe.c
> >>> create mode 100644
> >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c
> >>> create mode 100644
> >>>MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestToLo
> ck.
> >>>c
> >>> create mode 100644
> >>>MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDxe.c
> >>> create mode 100644 MdeModulePkg/Include/Guid/VarCheckPolicyMmi.h
> >>> create mode 100644
> >>>MdeModulePkg/Include/Library/VariablePolicyHelperLib.h
> >>> create mode 100644 MdeModulePkg/Include/Library/VariablePolicyLib.h
> >>> create mode 100644 MdeModulePkg/Include/Protocol/VariablePolicy.h
> >>> create mode 100644
> >>>MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
> >>> create mode 100644
> >>>MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.uni
> >>> create mode 100644
> >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.
> >>>inf
> >>> create mode 100644
> >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.
> >>>uni
> >>> create mode 100644
> MdeModulePkg/Library/VariablePolicyLib/ReadMe.md
> >>> create mode 100644
> >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf
> >>> create mode 100644
> >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.uni
> >>> create mode 100644
> >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe.in
> >>>f
> >>
> >
> >
> >
> >
> >
> >
>
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: A proposal to reduce incompatible case
2020-11-20 6:51 A proposal to reduce incompatible case Zhiguang Liu
@ 2020-11-20 7:20 ` Yao, Jiewen
2020-11-20 7:24 ` Bret Barkelew
2020-11-20 7:27 ` 回复: [edk2-devel] " gaoliming
0 siblings, 2 replies; 11+ messages in thread
From: Yao, Jiewen @ 2020-11-20 7:20 UTC (permalink / raw)
To: Liu, Zhiguang, devel@edk2.groups.io, michael.kubacki@outlook.com,
awarkentin@vmware.com, Ard Biesheuvel, debtech@gmail.com,
Feng, Bob C, Tian, Hot
Cc: Bret Barkelew, Bi, Dandan, Chao Zhang, Wang, Jian J, Wu, Hao A,
Liming Gao, Justen, Jordan L, Laszlo Ersek, Andrew Fish, Ni, Ray,
Bret Barkelew, Kinney, Michael D, Liming Gao
I like this idea. MinPlatform also adopt the same strategy - define common stuff in a dsc include file @ https://github.com/tianocore/edk2-platforms/tree/master/Platform/Intel/MinPlatformPkg/Include/Dsc
A minor clarification:
For VariablePolicyLib, I think we just need add to MdeModulePkgLib.dsc.inc. We don’t need update UefiPayloadPkgLib.dsc.inc or SecurityPkgLib.dsc.inc, right? They are just consumer, not producer.
Thank you
Yao Jiewen
> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Friday, November 20, 2020 2:52 PM
> To: devel@edk2.groups.io; michael.kubacki@outlook.com;
> awarkentin@vmware.com; Ard Biesheuvel <ard.biesheuvel@arm.com>;
> debtech@gmail.com; Feng, Bob C <bob.c.feng@intel.com>; Tian, Hot
> <hot.tian@intel.com>
> Cc: Bret Barkelew <bret@corthon.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Chao Zhang
> <chao.b.zhang@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao
> A <hao.a.wu@intel.com>; Liming Gao <liming.gao@intel.com>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Bret Barkelew
> <brbarkel@microsoft.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>
> Subject: A proposal to reduce incompatible case
>
> Hi all,
>
> As Michael mentioned, there are some platforms do not build and some is
> because incompatible code change like this one.
> I think it is a burden for both contributor and maintainer to fix platform code
> when meeting such incompatible change.
> I want to proposal one solution to minimum the effort of such code change.
>
> We could add a package library instance dsc include file under each package,
> like XXXPkgLib.dsc.inc
> It will specify the default library instance that will be used by modules in this
> package.
> For example, we add MdeModulePkgLib.dsc.inc file in MdeModulePkg.
> Some package already has similar dsc include file, such as
> ArmVirtPkg/ArmVirt.dsc.inc and NetworkPkg\Network.dsc.inc.
> In platform dsc file, we include the XXXPkgLib.dsc.inc file at the beginning, if
> the platform uses component from the package.
> We place the inc file in the beginning because we can override the library
> instance in other part of the platform dsc file.
>
> Whenever the contributor adds a new library dependency in one module, he
> should also add a default library instance in the package library instance dsc
> include file.
>
> For example, in this case,
> Contributor will add the below information in UefiPayloadPkgLib.dsc.inc,
> SecurityPkgLib.dsc.inc and MdeModulePkgLib.dsc.inc
>
> [LibraryClasses]
>
> VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLi
> b.inf
>
> VariablePolicyHelperLib|MdeModulePkg/Library/VariablePolicyHelperLib/Va
> riablePolicyHelperLib.inf
> [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>
> VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLi
> bRuntimeDxe.inf
>
> If the platform already includes these inc files, the code change won't break
> any build.
> If the platform wants to choose another library instance, it can specify in the
> dsc file, and will override the configuration in inc files.
> This feature can even reduce the code in platform dsc file if platform choose
> to use default library instance.
> The problem is that it may compiles redundant modules if the
>
> Please give comments about this proposal.
>
> Thanks
> Zhiguang
>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Michael
> > Kubacki
> > Sent: Friday, November 20, 2020 4:16 AM
> > To: devel@edk2.groups.io; awarkentin@vmware.com; Ard Biesheuvel
> > <ard.biesheuvel@arm.com>; debtech@gmail.com
> > Cc: Bret Barkelew <bret@corthon.com>; Yao, Jiewen
> <jiewen.yao@intel.com>;
> > Bi, Dandan <dandan.bi@intel.com>; Chao Zhang
> <chao.b.zhang@intel.com>;
> > Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> > Liming Gao <liming.gao@intel.com>; Justen, Jordan L
> > <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Andrew
> Fish
> > <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Bret Barkelew
> > <brbarkel@microsoft.com>
> > Subject: Re: [edk2-devel] [PATCH v9 00/13] Add the VariablePolicy feature
> >
> > While I'm not currently a maintainer in either repo, I believe the current
> > process is not ideal. I highlighted some of my observations
> > here: https://edk2.groups.io/g/devel/message/65902.
> >
> > Again, I don't have a strong vested interest in this but I do think some level
> of a
> > more well defined process needs to be reached between repo maintiners
> to
> > ease feature development in the future.
> >
> > Thanks,
> > Michael
> >
> > On 11/19/2020 12:02 PM, Andrei Warkentin wrote:
> > > Hi Bret,
> > >
> > > To be honest, I don't recall seeing anything. Again, maybe I should
> > > have been more proactive, but that's probably the net reality for most
> > > people. It would be unreasonable to expect you to test every platform,
> > > but it is very reasonable to assume that if you know you're adding
> > > build breakage to every platform (that is trivial to fix), that you
> > > would be taking care of it... Principle of least surprise. And yes, in
> > > some weird corner case perhaps that would be insufficient (again, I
> > > don't think anyone would expect you to compile test every platform),
> > > but it would take care of 99% of obvious fall-out.
> > >
> > > For reference, there are occasional clean-ups that happen to the edk2
> > > tree, and I've never seen anyone claim "not my problem" to deal with
> > > the obvious fall-out resulting from renames and such.
> > >
> > > A
> > > ----------------------------------------------------------------------
> > > --
> > > *From:* devel@edk2.groups.io <devel@edk2.groups.io> on behalf of
> Bret
> > > Barkelew via groups.io <debtech=gmail.com@groups.io>
> > > *Sent:* Thursday, November 19, 2020 10:15 AM
> > > *To:* Ard Biesheuvel <ard.biesheuvel@arm.com>
> > > *Cc:* Bret Barkelew <bret@corthon.com>; devel@edk2.groups.io
> > > <devel@edk2.groups.io>; Jiewen Yao <jiewen.yao@intel.com>; Dandan
> Bi
> > > <dandan.bi@intel.com>; Chao Zhang <chao.b.zhang@intel.com>; Jian J
> > > Wang <jian.j.wang@intel.com>; Hao A Wu <hao.a.wu@intel.com>;
> Liming
> > > Gao <liming.gao@intel.com>; Jordan Justen <jordan.l.justen@intel.com>;
> > > Laszlo Ersek <lersek@redhat.com>; Andrew Fish <afish@apple.com>; Ray
> > > Ni <ray.ni@intel.com>; Bret Barkelew <brbarkel@microsoft.com>
> > > *Subject:* Re: [edk2-devel] [PATCH v9 00/13] Add the VariablePolicy
> > > feature Those bugs and recommendations were sent out months ago.
> > > Several platforms have staged the changes already.
> > >
> > > You need to add the library class to your DSC.
> > >
> > > --
> > > [ Insert obscure pop-culture reference here. ]
> > >
> > >> On Nov 19, 2020, at 4:46 AM, Ard Biesheuvel <ard.biesheuvel@arm.com>
> > wrote:
> > >>
> > >> On 11/9/20 7:45 AM, Bret Barkelew wrote:
> > >>> The 14 patches in this series add the VariablePolicy feature to the
> > >>> core, deprecate Edk2VarLock (while adding a compatibility layer to
> > >>> reduce code churn), and integrate the VariablePolicy libraries and
> > >>> protocols into Variable Services.
> > >>> Since the integration requires multiple changes, including adding
> > >>> libraries, a protocol, an SMI communication handler, and
> > >>> VariableServices integration, the patches are broken up by
> > >>> individual library additions and then a final integration.
> > >>> Security-sensitive changes like bypassing Authenticated Variable
> > >>> enforcement are also broken out into individual patches so that
> attention
> > can be called directly to them.
> > >>> Platform porting instructions are described in this wiki entry:
> > >>>
> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> > >>>
> thub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FVariablePolicy-P
> > >>> rotocol---Enhanced-Method-for-Managing-Variables%23platform-
> porting&
> > >>>
> >
> amp;data=04%7C01%7Cawarkentin%40vmware.com%7C594f15b45aaf476bff
> 7e
> > 08d
> > >>>
> >
> 88cb57390%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C6374140
> 5
> > 82471
> > >>>
> >
> 28819%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2lu
> M
> > zIiLC
> > >>>
> >
> JBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=LLKZ7qeffR0WCvLbY
> uH
> > tQI
> > >>> uwJGhXY0mVqB2w9B0q180%3D&reserved=0
> > >
> <https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit
> > > hub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FVariablePolicy-
> Prot
> > > ocol---Enhanced-Method-for-Managing-Variables%23platform-
> > porting&d
> > >
> >
> ata=04%7C01%7Cawarkentin%40vmware.com%7C594f15b45aaf476bff7e08d
> 88
> > cb573
> > >
> >
> 90%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C63741405824712
> 8
> > 819%7CU
> > >
> >
> nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI
> 6Ik
> > 1ha
> > >
> >
> WwiLCJXVCI6Mn0%3D%7C1000&sdata=LLKZ7qeffR0WCvLbYuHtQIuwJG
> hX
> > Y0mVqB2
> > > w9B0q180%3D&reserved=0>
> > >>> Discussion of the feature can be found in multiple places throughout
> > >>> the last year on the RFC channel, staging branches, and in devel.
> > >>> Most recently, this subject was discussed in this thread:
> > >>>
> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fed
> > >>>
> >
> k2.groups.io%2Fg%2Fdevel%2Fmessage%2F53712&data=04%7C01%7Ca
> wa
> > rke
> > >>>
> >
> ntin%40vmware.com%7C594f15b45aaf476bff7e08d88cb57390%7Cb39138ca
> 3c
> > ee4
> > >>>
> >
> b4aa4d6cd83d9dd62f0%7C0%7C0%7C637414058247133820%7CUnknown%7
> CT
> > WFpbGZ
> > >>>
> >
> sb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6M
> n0
> > >>> %3D%7C1000&sdata=GYY52rlsPxw07vfdu%2BVbWhzRjtHWXlIGve
> CTT
> > 17mlfc%3
> > >>> D&reserved=0
> > >
> <https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk
> > >
> >
> 2.groups.io%2Fg%2Fdevel%2Fmessage%2F53712&data=04%7C01%7Ca
> war
> > kenti
> > >
> >
> n%40vmware.com%7C594f15b45aaf476bff7e08d88cb57390%7Cb39138ca3ce
> e
> > 4b4aa4
> > >
> >
> d6cd83d9dd62f0%7C0%7C0%7C637414058247133820%7CUnknown%7CTWF
> pb
> > GZsb3d8ey
> > >
> >
> JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%
> 7
> > C100
> > >
> >
> 0&sdata=GYY52rlsPxw07vfdu%2BVbWhzRjtHWXlIGveCTT17mlfc%3D&a
> mp
> > ;reser
> > > ved=0>
> > >>> (the code branches shared in that discussion are now out of date,
> > >>> but the whitepapers and discussion are relevant).
> > >>> Cc: Jiewen Yao <jiewen.yao@intel.com>
> > >>> Cc: Dandan Bi <dandan.bi@intel.com>
> > >>> Cc: Chao Zhang <chao.b.zhang@intel.com>
> > >>> Cc: Jian J Wang <jian.j.wang@intel.com>
> > >>> Cc: Hao A Wu <hao.a.wu@intel.com>
> > >>> Cc: Liming Gao <liming.gao@intel.com>
> > >>> Cc: Jordan Justen <jordan.l.justen@intel.com>
> > >>> Cc: Laszlo Ersek <lersek@redhat.com>
> > >>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > >>> Cc: Andrew Fish <afish@apple.com>
> > >>> Cc: Ray Ni <ray.ni@intel.com>
> > >>> Cc: Bret Barkelew <brbarkel@microsoft.com>
> > >>> Signed-off-by: Bret Barkelew <brbarkel@microsoft.com>
> > >>
> > >> This series has now made it into edk2, and has subsequently broken
> every
> > single platform in edk2-platforms. Is anyone intending to propose any fixes
> for
> > this?
> > >>
> > >>
> > >>> v9 changes:
> > >>> * Rebase
> > >>> * Address the event ordering issues around MorLock at EndOfDxe
> > >>> * Drop problematic tests
> > >>> * Address ECC issues
> > >>> v8 changes:
> > >>> * Rebase
> > >>> * Small tweaks from final PRs
> > >>> * Drank a lot
> > >>> * Enrolled several members and a steward in CatFacts
> > >>> v7 changes:
> > >>> * Address comments from Dandan about security of the MM handler
> > >>> * Add readme
> > >>> * Fix bug around hex characters in BOOT####, etc
> > >>> * Add additional testing for hex characters
> > >>> * Add additional testing for authenticated variables
> > >>> v6 changes:
> > >>> * Fix an issue with uninitialized Status in InitVariablePolicyLib()
> > >>>and DeinitVariablePolicyLib()
> > >>> * Fix GCC building in shell-based functional test
> > >>> * Rebase on latest origin/master
> > >>> v5 changes:
> > >>> * Fix the CONST mismatch in VariablePolicy.h and
> > >>>VariablePolicySmmDxe.c
> > >>> * Fix EFIAPI mismatches in the functional unittest
> > >>> * Rebase on latest origin/master
> > >>> v4 changes:
> > >>> * Remove Optional PcdAllowVariablePolicyEnforcementDisable PCD
> from
> > >>>platforms
> > >>> * Rebase on master
> > >>> * Migrate to new MmCommunicate2 protocol
> > >>> * Fix an oversight in the default return value for
> > >>>InitMmCommonCommBuffer
> > >>> * Fix in VariablePolicyLib to allow ExtraInitRuntimeDxe to consume
> > >>>variables
> > >>> V3 changes:
> > >>> * Address all non-unittest issues with ECC
> > >>> * Make additional style changes
> > >>> * Include section name in hunk headers in "ini-style" files
> > >>> * Remove requirement for the
> EdkiiPiSmmCommunicationsRegionTable
> > >>>driver
> > >>> (now allocates its own buffer)
> > >>> * Change names from VARIABLE_POLICY_PROTOCOL and
> > >>>gVariablePolicyProtocolGuid
> > >>> to EDKII_VARIABLE_POLICY_PROTOCOL and
> > >>>gEdkiiVariablePolicyProtocolGuid
> > >>> * Fix GCC warning about initializing externs
> > >>> * Add UNI strings for new PCD
> > >>> * Add patches for ArmVirtPkg, OvmfXen, and UefiPayloadPkg
> > >>> * Reorder patches according to Liming's feedback about adding to
> > >>>platforms
> > >>> before changing variable driver
> > >>> V2 changes:
> > >>> * Fixed implementation for RuntimeDxe
> > >>> * Add PCD to block DisableVariablePolicy
> > >>> * Fix the DumpVariablePolicy pagination in SMM Bret Barkelew (13):
> > >>> MdeModulePkg: Define the VariablePolicy protocol interface
> > >>> MdeModulePkg: Define the VariablePolicyLib
> > >>> MdeModulePkg: Define the VariablePolicyHelperLib
> > >>> MdeModulePkg: Define the VarCheckPolicyLib and SMM interface
> > >>> OvmfPkg: Add VariablePolicy engine to OvmfPkg platform
> > >>> EmulatorPkg: Add VariablePolicy engine to EmulatorPkg platform
> > >>> ArmVirtPkg: Add VariablePolicy engine to ArmVirtPkg platform
> > >>> UefiPayloadPkg: Add VariablePolicy engine to UefiPayloadPkg
> > >>>platform
> > >>> MdeModulePkg: Connect VariablePolicy business logic to
> > >>> VariableServices
> > >>> MdeModulePkg: Allow VariablePolicy state to delete protected
> > >>>variables
> > >>> SecurityPkg: Allow VariablePolicy state to delete authenticated
> > >>> variables
> > >>> MdeModulePkg: Change TCG MOR variables to use VariablePolicy
> > >>> MdeModulePkg: Drop VarLock from RuntimeDxe variable driver
> > >>> MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c
> > >>>| 346 ++++++++
> > >>>
> > >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLi
> b.
> > >>>c | 396 ++++++++++
> > >>>
> > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitNull.c
> > >>>| 46 ++
> > >>>
> > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitRunti
> me
> > >>>Dxe.c | 85 ++
> > >>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c
> > >>>| 830 ++++++++++++++++++++
> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> >
> > >>>| 52 +-
> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> >
> > >>>| 60 +-
> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VarCheck.c
> > >>>| 49 +-
> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
> > >>>| 60 ++
> > >>>
> > >>>MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestT
> oLo
> > ck.
> > >>>c | 71 ++
> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmD
> xe.c
> >
> > >>>| 573 ++++++++++++++
> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> >
> > >>>| 7 +
> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntim
> eDx
> > e.c
> > >>>| 14 +
> > >>> SecurityPkg/Library/AuthVariableLib/AuthService.c
> > >>>| 30 +-
> > >>> ArmVirtPkg/ArmVirt.dsc.inc
> > >>>| 4 +
> > >>> EmulatorPkg/EmulatorPkg.dsc
> > >>>| 3 +
> > >>> MdeModulePkg/Include/Guid/VarCheckPolicyMmi.h
> > >>>| 54 ++
> > >>> MdeModulePkg/Include/Library/VariablePolicyHelperLib.h
> > >>>| 164 ++++
> > >>> MdeModulePkg/Include/Library/VariablePolicyLib.h
> > >>>| 207 +++++
> > >>> MdeModulePkg/Include/Protocol/VariablePolicy.h
> > >>>| 157 ++++
> > >>> MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
> > >>>| 42 +
> > >>> MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.uni
> > >>>| 12 +
> > >>>
> > >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLi
> b.
> > >>>inf | 35 +
> > >>>
> > >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLi
> b.
> > >>>uni | 12 +
> > >>> MdeModulePkg/Library/VariablePolicyLib/ReadMe.md
> > >>>| 406 ++++++++++
> > >>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf
> > >>>| 48 ++
> > >>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.uni
> > >>>| 12 +
> > >>>
> > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe
> .in
> > >>>f | 51 ++
> > >>> MdeModulePkg/MdeModulePkg.ci.yaml
> > >>>| 4 +-
> > >>> MdeModulePkg/MdeModulePkg.dec
> > >>>| 26 +-
> > >>> MdeModulePkg/MdeModulePkg.dsc
> > >>>| 9 +
> > >>> MdeModulePkg/MdeModulePkg.uni
> > >>>| 7 +
> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe
> .inf
> >
> > >>>| 5 +
> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> >
> > >>>| 4 +
> > >>>
> > >>>MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntime
> Dxe.
> > inf
> > >>>| 11 +
> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandalone
> Mm.i
> > nf
> > >>>| 4 +
> > >>> OvmfPkg/OvmfPkgIa32.dsc
> > >>>| 5 +
> > >>> OvmfPkg/OvmfPkgIa32X64.dsc
> > >>>| 5 +
> > >>> OvmfPkg/OvmfPkgX64.dsc
> > >>>| 5 +
> > >>> OvmfPkg/OvmfXen.dsc
> > >>>| 4 +
> > >>> SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
> > >>>| 2 +
> > >>> UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > >>>| 4 +
> > >>> UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > >>>| 4 +
> > >>> 43 files changed, 3845 insertions(+), 80 deletions(-)
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLi
> b.
> > >>>c
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitNull.c
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitRunti
> me
> > >>>Dxe.c
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c
> > >>> create mode 100644
> > >>>MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestT
> oLo
> > ck.
> > >>>c
> > >>> create mode 100644
> > >>>MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDx
> e.c
> > >>> create mode 100644
> MdeModulePkg/Include/Guid/VarCheckPolicyMmi.h
> > >>> create mode 100644
> > >>>MdeModulePkg/Include/Library/VariablePolicyHelperLib.h
> > >>> create mode 100644
> MdeModulePkg/Include/Library/VariablePolicyLib.h
> > >>> create mode 100644
> MdeModulePkg/Include/Protocol/VariablePolicy.h
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.uni
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLi
> b.
> > >>>inf
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLi
> b.
> > >>>uni
> > >>> create mode 100644
> > MdeModulePkg/Library/VariablePolicyLib/ReadMe.md
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.uni
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe
> .in
> > >>>f
> > >>
> > >
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: A proposal to reduce incompatible case
2020-11-20 7:20 ` Yao, Jiewen
@ 2020-11-20 7:24 ` Bret Barkelew
2020-11-20 7:27 ` 回复: [edk2-devel] " gaoliming
1 sibling, 0 replies; 11+ messages in thread
From: Bret Barkelew @ 2020-11-20 7:24 UTC (permalink / raw)
To: Yao, Jiewen, Liu, Zhiguang, devel@edk2.groups.io,
michael.kubacki@outlook.com, awarkentin@vmware.com,
Ard Biesheuvel, debtech@gmail.com, Feng, Bob C, Tian, Hot
Cc: Bret Barkelew, Bi, Dandan, Chao Zhang, Wang, Jian J, Wu, Hao A,
liming.gao, Justen, Jordan L, Laszlo Ersek, Andrew Fish, Ni, Ray,
Kinney, Michael D, Liming Gao
[-- Attachment #1: Type: text/plain, Size: 23516 bytes --]
@Yao, Jiewen<mailto:jiewen.yao@intel.com>, that should be correct.
I’m not so sure that this is a great idea. If a platform is consuming entire core packages en masse, that feels like a recipe for disaster (or a platform that is so inconsequential that it need not be managed). My personal feeling about it is that the platforms should be consuming from EDK2 like… well… platforms. They should snap to stable releases and those EDK2 releases should come with sufficient change notes to explain what the platform will need to change when it snaps.
Either that, or we can come up with a formal way to add platform porting documentation in the commit notes on commits that will have a platfrom-breaking effect.
- Bret
From: Yao, Jiewen<mailto:jiewen.yao@intel.com>
Sent: Thursday, November 19, 2020 11:20 PM
To: Liu, Zhiguang<mailto:zhiguang.liu@intel.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; michael.kubacki@outlook.com<mailto:michael.kubacki@outlook.com>; awarkentin@vmware.com<mailto:awarkentin@vmware.com>; Ard Biesheuvel<mailto:ard.biesheuvel@arm.com>; debtech@gmail.com<mailto:debtech@gmail.com>; Feng, Bob C<mailto:bob.c.feng@intel.com>; Tian, Hot<mailto:hot.tian@intel.com>
Cc: Bret Barkelew<mailto:bret@corthon.com>; Bi, Dandan<mailto:dandan.bi@intel.com>; Chao Zhang<mailto:chao.b.zhang@intel.com>; Wang, Jian J<mailto:jian.j.wang@intel.com>; Wu, Hao A<mailto:hao.a.wu@intel.com>; liming.gao<mailto:liming.gao@intel.com>; Justen, Jordan L<mailto:jordan.l.justen@intel.com>; Laszlo Ersek<mailto:lersek@redhat.com>; Andrew Fish<mailto:afish@apple.com>; Ni, Ray<mailto:ray.ni@intel.com>; Bret Barkelew<mailto:Bret.Barkelew@microsoft.com>; Kinney, Michael D<mailto:michael.d.kinney@intel.com>; Liming Gao<mailto:gaoliming@byosoft.com.cn>
Subject: [EXTERNAL] RE: A proposal to reduce incompatible case
I like this idea. MinPlatform also adopt the same strategy - define common stuff in a dsc include file @ https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftianocore%2Fedk2-platforms%2Ftree%2Fmaster%2FPlatform%2FIntel%2FMinPlatformPkg%2FInclude%2FDsc&data=04%7C01%7CBret.Barkelew%40microsoft.com%7C14c39c430e694a2610d708d88d24c2e3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637414536339327026%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=G0TSIq0nQH5t7%2F1nBsuacydp928NBHBzcmx3KQi%2Fw%2Fc%3D&reserved=0
A minor clarification:
For VariablePolicyLib, I think we just need add to MdeModulePkgLib.dsc.inc. We don’t need update UefiPayloadPkgLib.dsc.inc or SecurityPkgLib.dsc.inc, right? They are just consumer, not producer.
Thank you
Yao Jiewen
> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Friday, November 20, 2020 2:52 PM
> To: devel@edk2.groups.io; michael.kubacki@outlook.com;
> awarkentin@vmware.com; Ard Biesheuvel <ard.biesheuvel@arm.com>;
> debtech@gmail.com; Feng, Bob C <bob.c.feng@intel.com>; Tian, Hot
> <hot.tian@intel.com>
> Cc: Bret Barkelew <bret@corthon.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Chao Zhang
> <chao.b.zhang@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao
> A <hao.a.wu@intel.com>; Liming Gao <liming.gao@intel.com>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Bret Barkelew
> <brbarkel@microsoft.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>
> Subject: A proposal to reduce incompatible case
>
> Hi all,
>
> As Michael mentioned, there are some platforms do not build and some is
> because incompatible code change like this one.
> I think it is a burden for both contributor and maintainer to fix platform code
> when meeting such incompatible change.
> I want to proposal one solution to minimum the effort of such code change.
>
> We could add a package library instance dsc include file under each package,
> like XXXPkgLib.dsc.inc
> It will specify the default library instance that will be used by modules in this
> package.
> For example, we add MdeModulePkgLib.dsc.inc file in MdeModulePkg.
> Some package already has similar dsc include file, such as
> ArmVirtPkg/ArmVirt.dsc.inc and NetworkPkg\Network.dsc.inc.
> In platform dsc file, we include the XXXPkgLib.dsc.inc file at the beginning, if
> the platform uses component from the package.
> We place the inc file in the beginning because we can override the library
> instance in other part of the platform dsc file.
>
> Whenever the contributor adds a new library dependency in one module, he
> should also add a default library instance in the package library instance dsc
> include file.
>
> For example, in this case,
> Contributor will add the below information in UefiPayloadPkgLib.dsc.inc,
> SecurityPkgLib.dsc.inc and MdeModulePkgLib.dsc.inc
>
> [LibraryClasses]
>
> VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLi
> b.inf
>
> VariablePolicyHelperLib|MdeModulePkg/Library/VariablePolicyHelperLib/Va
> riablePolicyHelperLib.inf
> [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>
> VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLi
> bRuntimeDxe.inf
>
> If the platform already includes these inc files, the code change won't break
> any build.
> If the platform wants to choose another library instance, it can specify in the
> dsc file, and will override the configuration in inc files.
> This feature can even reduce the code in platform dsc file if platform choose
> to use default library instance.
> The problem is that it may compiles redundant modules if the
>
> Please give comments about this proposal.
>
> Thanks
> Zhiguang
>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> Michael
> > Kubacki
> > Sent: Friday, November 20, 2020 4:16 AM
> > To: devel@edk2.groups.io; awarkentin@vmware.com; Ard Biesheuvel
> > <ard.biesheuvel@arm.com>; debtech@gmail.com
> > Cc: Bret Barkelew <bret@corthon.com>; Yao, Jiewen
> <jiewen.yao@intel.com>;
> > Bi, Dandan <dandan.bi@intel.com>; Chao Zhang
> <chao.b.zhang@intel.com>;
> > Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> > Liming Gao <liming.gao@intel.com>; Justen, Jordan L
> > <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Andrew
> Fish
> > <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Bret Barkelew
> > <brbarkel@microsoft.com>
> > Subject: Re: [edk2-devel] [PATCH v9 00/13] Add the VariablePolicy feature
> >
> > While I'm not currently a maintainer in either repo, I believe the current
> > process is not ideal. I highlighted some of my observations
> > here: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F65902&data=04%7C01%7CBret.Barkelew%40microsoft.com%7C14c39c430e694a2610d708d88d24c2e3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637414536339327026%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=J6hQ9rFYGEW4tLEXDMTugHKJX7JH%2FPnxHBZpEiU%2FOT4%3D&reserved=0.
> >
> > Again, I don't have a strong vested interest in this but I do think some level
> of a
> > more well defined process needs to be reached between repo maintiners
> to
> > ease feature development in the future.
> >
> > Thanks,
> > Michael
> >
> > On 11/19/2020 12:02 PM, Andrei Warkentin wrote:
> > > Hi Bret,
> > >
> > > To be honest, I don't recall seeing anything. Again, maybe I should
> > > have been more proactive, but that's probably the net reality for most
> > > people. It would be unreasonable to expect you to test every platform,
> > > but it is very reasonable to assume that if you know you're adding
> > > build breakage to every platform (that is trivial to fix), that you
> > > would be taking care of it... Principle of least surprise. And yes, in
> > > some weird corner case perhaps that would be insufficient (again, I
> > > don't think anyone would expect you to compile test every platform),
> > > but it would take care of 99% of obvious fall-out.
> > >
> > > For reference, there are occasional clean-ups that happen to the edk2
> > > tree, and I've never seen anyone claim "not my problem" to deal with
> > > the obvious fall-out resulting from renames and such.
> > >
> > > A
> > > ----------------------------------------------------------------------
> > > --
> > > *From:* devel@edk2.groups.io <devel@edk2.groups.io> on behalf of
> Bret
> > > Barkelew via groups.io <debtech=gmail.com@groups.io>
> > > *Sent:* Thursday, November 19, 2020 10:15 AM
> > > *To:* Ard Biesheuvel <ard.biesheuvel@arm.com>
> > > *Cc:* Bret Barkelew <bret@corthon.com>; devel@edk2.groups.io
> > > <devel@edk2.groups.io>; Jiewen Yao <jiewen.yao@intel.com>; Dandan
> Bi
> > > <dandan.bi@intel.com>; Chao Zhang <chao.b.zhang@intel.com>; Jian J
> > > Wang <jian.j.wang@intel.com>; Hao A Wu <hao.a.wu@intel.com>;
> Liming
> > > Gao <liming.gao@intel.com>; Jordan Justen <jordan.l.justen@intel.com>;
> > > Laszlo Ersek <lersek@redhat.com>; Andrew Fish <afish@apple.com>; Ray
> > > Ni <ray.ni@intel.com>; Bret Barkelew <brbarkel@microsoft.com>
> > > *Subject:* Re: [edk2-devel] [PATCH v9 00/13] Add the VariablePolicy
> > > feature Those bugs and recommendations were sent out months ago.
> > > Several platforms have staged the changes already.
> > >
> > > You need to add the library class to your DSC.
> > >
> > > --
> > > [ Insert obscure pop-culture reference here. ]
> > >
> > >> On Nov 19, 2020, at 4:46 AM, Ard Biesheuvel <ard.biesheuvel@arm.com>
> > wrote:
> > >>
> > >> On 11/9/20 7:45 AM, Bret Barkelew wrote:
> > >>> The 14 patches in this series add the VariablePolicy feature to the
> > >>> core, deprecate Edk2VarLock (while adding a compatibility layer to
> > >>> reduce code churn), and integrate the VariablePolicy libraries and
> > >>> protocols into Variable Services.
> > >>> Since the integration requires multiple changes, including adding
> > >>> libraries, a protocol, an SMI communication handler, and
> > >>> VariableServices integration, the patches are broken up by
> > >>> individual library additions and then a final integration.
> > >>> Security-sensitive changes like bypassing Authenticated Variable
> > >>> enforcement are also broken out into individual patches so that
> attention
> > can be called directly to them.
> > >>> Platform porting instructions are described in this wiki entry:
> > >>>
> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> > >>>
> thub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FVariablePolicy-P
> > >>> rotocol---Enhanced-Method-for-Managing-Variables%23platform-
> porting&
> > >>>
> >
> amp;data=04%7C01%7Cawarkentin%40vmware.com%7C594f15b45aaf476bff
> 7e
> > 08d
> > >>>
> >
> 88cb57390%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C6374140
> 5
> > 82471
> > >>>
> >
> 28819%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2lu
> M
> > zIiLC
> > >>>
> >
> JBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=LLKZ7qeffR0WCvLbY
> uH
> > tQI
> > >>> uwJGhXY0mVqB2w9B0q180%3D&reserved=0
> > >
> <https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit
> > > hub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FVariablePolicy-
> Prot
> > > ocol---Enhanced-Method-for-Managing-Variables%23platform-
> > porting&d
> > >
> >
> ata=04%7C01%7Cawarkentin%40vmware.com%7C594f15b45aaf476bff7e08d
> 88
> > cb573
> > >
> >
> 90%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C63741405824712
> 8
> > 819%7CU
> > >
> >
> nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI
> 6Ik
> > 1ha
> > >
> >
> WwiLCJXVCI6Mn0%3D%7C1000&sdata=LLKZ7qeffR0WCvLbYuHtQIuwJG
> hX
> > Y0mVqB2
> > > w9B0q180%3D&reserved=0>
> > >>> Discussion of the feature can be found in multiple places throughout
> > >>> the last year on the RFC channel, staging branches, and in devel.
> > >>> Most recently, this subject was discussed in this thread:
> > >>>
> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fed
> > >>>
> >
> k2.groups.io%2Fg%2Fdevel%2Fmessage%2F53712&data=04%7C01%7Ca
> wa
> > rke
> > >>>
> >
> ntin%40vmware.com%7C594f15b45aaf476bff7e08d88cb57390%7Cb39138ca
> 3c
> > ee4
> > >>>
> >
> b4aa4d6cd83d9dd62f0%7C0%7C0%7C637414058247133820%7CUnknown%7
> CT
> > WFpbGZ
> > >>>
> >
> sb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6M
> n0
> > >>> %3D%7C1000&sdata=GYY52rlsPxw07vfdu%2BVbWhzRjtHWXlIGve
> CTT
> > 17mlfc%3
> > >>> D&reserved=0
> > >
> <https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk
> > >
> >
> 2.groups.io%2Fg%2Fdevel%2Fmessage%2F53712&data=04%7C01%7Ca
> war
> > kenti
> > >
> >
> n%40vmware.com%7C594f15b45aaf476bff7e08d88cb57390%7Cb39138ca3ce
> e
> > 4b4aa4
> > >
> >
> d6cd83d9dd62f0%7C0%7C0%7C637414058247133820%7CUnknown%7CTWF
> pb
> > GZsb3d8ey
> > >
> >
> JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%
> 7
> > C100
> > >
> >
> 0&sdata=GYY52rlsPxw07vfdu%2BVbWhzRjtHWXlIGveCTT17mlfc%3D&a
> mp
> > ;reser
> > > ved=0>
> > >>> (the code branches shared in that discussion are now out of date,
> > >>> but the whitepapers and discussion are relevant).
> > >>> Cc: Jiewen Yao <jiewen.yao@intel.com>
> > >>> Cc: Dandan Bi <dandan.bi@intel.com>
> > >>> Cc: Chao Zhang <chao.b.zhang@intel.com>
> > >>> Cc: Jian J Wang <jian.j.wang@intel.com>
> > >>> Cc: Hao A Wu <hao.a.wu@intel.com>
> > >>> Cc: Liming Gao <liming.gao@intel.com>
> > >>> Cc: Jordan Justen <jordan.l.justen@intel.com>
> > >>> Cc: Laszlo Ersek <lersek@redhat.com>
> > >>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > >>> Cc: Andrew Fish <afish@apple.com>
> > >>> Cc: Ray Ni <ray.ni@intel.com>
> > >>> Cc: Bret Barkelew <brbarkel@microsoft.com>
> > >>> Signed-off-by: Bret Barkelew <brbarkel@microsoft.com>
> > >>
> > >> This series has now made it into edk2, and has subsequently broken
> every
> > single platform in edk2-platforms. Is anyone intending to propose any fixes
> for
> > this?
> > >>
> > >>
> > >>> v9 changes:
> > >>> * Rebase
> > >>> * Address the event ordering issues around MorLock at EndOfDxe
> > >>> * Drop problematic tests
> > >>> * Address ECC issues
> > >>> v8 changes:
> > >>> * Rebase
> > >>> * Small tweaks from final PRs
> > >>> * Drank a lot
> > >>> * Enrolled several members and a steward in CatFacts
> > >>> v7 changes:
> > >>> * Address comments from Dandan about security of the MM handler
> > >>> * Add readme
> > >>> * Fix bug around hex characters in BOOT####, etc
> > >>> * Add additional testing for hex characters
> > >>> * Add additional testing for authenticated variables
> > >>> v6 changes:
> > >>> * Fix an issue with uninitialized Status in InitVariablePolicyLib()
> > >>>and DeinitVariablePolicyLib()
> > >>> * Fix GCC building in shell-based functional test
> > >>> * Rebase on latest origin/master
> > >>> v5 changes:
> > >>> * Fix the CONST mismatch in VariablePolicy.h and
> > >>>VariablePolicySmmDxe.c
> > >>> * Fix EFIAPI mismatches in the functional unittest
> > >>> * Rebase on latest origin/master
> > >>> v4 changes:
> > >>> * Remove Optional PcdAllowVariablePolicyEnforcementDisable PCD
> from
> > >>>platforms
> > >>> * Rebase on master
> > >>> * Migrate to new MmCommunicate2 protocol
> > >>> * Fix an oversight in the default return value for
> > >>>InitMmCommonCommBuffer
> > >>> * Fix in VariablePolicyLib to allow ExtraInitRuntimeDxe to consume
> > >>>variables
> > >>> V3 changes:
> > >>> * Address all non-unittest issues with ECC
> > >>> * Make additional style changes
> > >>> * Include section name in hunk headers in "ini-style" files
> > >>> * Remove requirement for the
> EdkiiPiSmmCommunicationsRegionTable
> > >>>driver
> > >>> (now allocates its own buffer)
> > >>> * Change names from VARIABLE_POLICY_PROTOCOL and
> > >>>gVariablePolicyProtocolGuid
> > >>> to EDKII_VARIABLE_POLICY_PROTOCOL and
> > >>>gEdkiiVariablePolicyProtocolGuid
> > >>> * Fix GCC warning about initializing externs
> > >>> * Add UNI strings for new PCD
> > >>> * Add patches for ArmVirtPkg, OvmfXen, and UefiPayloadPkg
> > >>> * Reorder patches according to Liming's feedback about adding to
> > >>>platforms
> > >>> before changing variable driver
> > >>> V2 changes:
> > >>> * Fixed implementation for RuntimeDxe
> > >>> * Add PCD to block DisableVariablePolicy
> > >>> * Fix the DumpVariablePolicy pagination in SMM Bret Barkelew (13):
> > >>> MdeModulePkg: Define the VariablePolicy protocol interface
> > >>> MdeModulePkg: Define the VariablePolicyLib
> > >>> MdeModulePkg: Define the VariablePolicyHelperLib
> > >>> MdeModulePkg: Define the VarCheckPolicyLib and SMM interface
> > >>> OvmfPkg: Add VariablePolicy engine to OvmfPkg platform
> > >>> EmulatorPkg: Add VariablePolicy engine to EmulatorPkg platform
> > >>> ArmVirtPkg: Add VariablePolicy engine to ArmVirtPkg platform
> > >>> UefiPayloadPkg: Add VariablePolicy engine to UefiPayloadPkg
> > >>>platform
> > >>> MdeModulePkg: Connect VariablePolicy business logic to
> > >>> VariableServices
> > >>> MdeModulePkg: Allow VariablePolicy state to delete protected
> > >>>variables
> > >>> SecurityPkg: Allow VariablePolicy state to delete authenticated
> > >>> variables
> > >>> MdeModulePkg: Change TCG MOR variables to use VariablePolicy
> > >>> MdeModulePkg: Drop VarLock from RuntimeDxe variable driver
> > >>> MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c
> > >>>| 346 ++++++++
> > >>>
> > >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLi
> b.
> > >>>c | 396 ++++++++++
> > >>>
> > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitNull.c
> > >>>| 46 ++
> > >>>
> > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitRunti
> me
> > >>>Dxe.c | 85 ++
> > >>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c
> > >>>| 830 ++++++++++++++++++++
> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> >
> > >>>| 52 +-
> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> >
> > >>>| 60 +-
> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VarCheck.c
> > >>>| 49 +-
> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
> > >>>| 60 ++
> > >>>
> > >>>MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestT
> oLo
> > ck.
> > >>>c | 71 ++
> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmD
> xe.c
> >
> > >>>| 573 ++++++++++++++
> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> >
> > >>>| 7 +
> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntim
> eDx
> > e.c
> > >>>| 14 +
> > >>> SecurityPkg/Library/AuthVariableLib/AuthService.c
> > >>>| 30 +-
> > >>> ArmVirtPkg/ArmVirt.dsc.inc
> > >>>| 4 +
> > >>> EmulatorPkg/EmulatorPkg.dsc
> > >>>| 3 +
> > >>> MdeModulePkg/Include/Guid/VarCheckPolicyMmi.h
> > >>>| 54 ++
> > >>> MdeModulePkg/Include/Library/VariablePolicyHelperLib.h
> > >>>| 164 ++++
> > >>> MdeModulePkg/Include/Library/VariablePolicyLib.h
> > >>>| 207 +++++
> > >>> MdeModulePkg/Include/Protocol/VariablePolicy.h
> > >>>| 157 ++++
> > >>> MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
> > >>>| 42 +
> > >>> MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.uni
> > >>>| 12 +
> > >>>
> > >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLi
> b.
> > >>>inf | 35 +
> > >>>
> > >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLi
> b.
> > >>>uni | 12 +
> > >>> MdeModulePkg/Library/VariablePolicyLib/ReadMe.md
> > >>>| 406 ++++++++++
> > >>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf
> > >>>| 48 ++
> > >>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.uni
> > >>>| 12 +
> > >>>
> > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe
> .in
> > >>>f | 51 ++
> > >>> MdeModulePkg/MdeModulePkg.ci.yaml
> > >>>| 4 +-
> > >>> MdeModulePkg/MdeModulePkg.dec
> > >>>| 26 +-
> > >>> MdeModulePkg/MdeModulePkg.dsc
> > >>>| 9 +
> > >>> MdeModulePkg/MdeModulePkg.uni
> > >>>| 7 +
> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe
> .inf
> >
> > >>>| 5 +
> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> >
> > >>>| 4 +
> > >>>
> > >>>MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntime
> Dxe.
> > inf
> > >>>| 11 +
> > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandalone
> Mm.i
> > nf
> > >>>| 4 +
> > >>> OvmfPkg/OvmfPkgIa32.dsc
> > >>>| 5 +
> > >>> OvmfPkg/OvmfPkgIa32X64.dsc
> > >>>| 5 +
> > >>> OvmfPkg/OvmfPkgX64.dsc
> > >>>| 5 +
> > >>> OvmfPkg/OvmfXen.dsc
> > >>>| 4 +
> > >>> SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
> > >>>| 2 +
> > >>> UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > >>>| 4 +
> > >>> UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > >>>| 4 +
> > >>> 43 files changed, 3845 insertions(+), 80 deletions(-)
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLi
> b.
> > >>>c
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitNull.c
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitRunti
> me
> > >>>Dxe.c
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c
> > >>> create mode 100644
> > >>>MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequestT
> oLo
> > ck.
> > >>>c
> > >>> create mode 100644
> > >>>MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmDx
> e.c
> > >>> create mode 100644
> MdeModulePkg/Include/Guid/VarCheckPolicyMmi.h
> > >>> create mode 100644
> > >>>MdeModulePkg/Include/Library/VariablePolicyHelperLib.h
> > >>> create mode 100644
> MdeModulePkg/Include/Library/VariablePolicyLib.h
> > >>> create mode 100644
> MdeModulePkg/Include/Protocol/VariablePolicy.h
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.uni
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLi
> b.
> > >>>inf
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLi
> b.
> > >>>uni
> > >>> create mode 100644
> > MdeModulePkg/Library/VariablePolicyLib/ReadMe.md
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.uni
> > >>> create mode 100644
> > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe
> .in
> > >>>f
> > >>
> > >
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> >
[-- Attachment #2: Type: text/html, Size: 35926 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* 回复: [edk2-devel] A proposal to reduce incompatible case
2020-11-20 7:20 ` Yao, Jiewen
2020-11-20 7:24 ` Bret Barkelew
@ 2020-11-20 7:27 ` gaoliming
2020-11-20 9:02 ` Ard Biesheuvel
1 sibling, 1 reply; 11+ messages in thread
From: gaoliming @ 2020-11-20 7:27 UTC (permalink / raw)
To: devel, jiewen.yao, 'Liu, Zhiguang', michael.kubacki,
awarkentin, 'Ard Biesheuvel', debtech,
'Feng, Bob C', 'Tian, Hot'
Cc: 'Bret Barkelew', 'Bi, Dandan',
'Chao Zhang', 'Wang, Jian J', 'Wu, Hao A',
'Liming Gao', 'Justen, Jordan L',
'Laszlo Ersek', 'Andrew Fish', 'Ni, Ray',
'Bret Barkelew', 'Kinney, Michael D'
Zhiguang:
This proposal can reduce the potential library class dependency. Each package has its xxxPkgLib.dsc.inc file that includes the library instances from this package.
Platform DSC can include the required package lib.dsc.inc file for the library instances. Can you work out the code changes to demonstrate this idea?
Thanks
Liming
> -----邮件原件-----
> 发件人: bounce+27952+67752+4905953+8761045@groups.io
> <bounce+27952+67752+4905953+8761045@groups.io> 代表 Yao, Jiewen
> 发送时间: 2020年11月20日 15:20
> 收件人: Liu, Zhiguang <zhiguang.liu@intel.com>; devel@edk2.groups.io;
> michael.kubacki@outlook.com; awarkentin@vmware.com; Ard Biesheuvel
> <ard.biesheuvel@arm.com>; debtech@gmail.com; Feng, Bob C
> <bob.c.feng@intel.com>; Tian, Hot <hot.tian@intel.com>
> 抄送: Bret Barkelew <bret@corthon.com>; Bi, Dandan
> <dandan.bi@intel.com>; Chao Zhang <chao.b.zhang@intel.com>; Wang, Jian
> J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Liming Gao
> <liming.gao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Laszlo
> Ersek <lersek@redhat.com>; Andrew Fish <afish@apple.com>; Ni, Ray
> <ray.ni@intel.com>; Bret Barkelew <brbarkel@microsoft.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>
> 主题: Re: [edk2-devel] A proposal to reduce incompatible case
>
> I like this idea. MinPlatform also adopt the same strategy - define common
> stuff in a dsc include file @
> https://github.com/tianocore/edk2-platforms/tree/master/Platform/Intel/Mi
> nPlatformPkg/Include/Dsc
>
>
> A minor clarification:
> For VariablePolicyLib, I think we just need add to MdeModulePkgLib.dsc.inc.
> We don’t need update UefiPayloadPkgLib.dsc.inc or SecurityPkgLib.dsc.inc,
> right? They are just consumer, not producer.
>
> Thank you
> Yao Jiewen
>
>
> > -----Original Message-----
> > From: Liu, Zhiguang <zhiguang.liu@intel.com>
> > Sent: Friday, November 20, 2020 2:52 PM
> > To: devel@edk2.groups.io; michael.kubacki@outlook.com;
> > awarkentin@vmware.com; Ard Biesheuvel <ard.biesheuvel@arm.com>;
> > debtech@gmail.com; Feng, Bob C <bob.c.feng@intel.com>; Tian, Hot
> > <hot.tian@intel.com>
> > Cc: Bret Barkelew <bret@corthon.com>; Yao, Jiewen
> > <jiewen.yao@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Chao Zhang
> > <chao.b.zhang@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao
> > A <hao.a.wu@intel.com>; Liming Gao <liming.gao@intel.com>; Justen,
> > Jordan L <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> > Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Bret
> Barkelew
> > <brbarkel@microsoft.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>
> > Subject: A proposal to reduce incompatible case
> >
> > Hi all,
> >
> > As Michael mentioned, there are some platforms do not build and some is
> > because incompatible code change like this one.
> > I think it is a burden for both contributor and maintainer to fix platform code
> > when meeting such incompatible change.
> > I want to proposal one solution to minimum the effort of such code change.
> >
> > We could add a package library instance dsc include file under each
> package,
> > like XXXPkgLib.dsc.inc
> > It will specify the default library instance that will be used by modules in this
> > package.
> > For example, we add MdeModulePkgLib.dsc.inc file in MdeModulePkg.
> > Some package already has similar dsc include file, such as
> > ArmVirtPkg/ArmVirt.dsc.inc and NetworkPkg\Network.dsc.inc.
> > In platform dsc file, we include the XXXPkgLib.dsc.inc file at the beginning,
> if
> > the platform uses component from the package.
> > We place the inc file in the beginning because we can override the library
> > instance in other part of the platform dsc file.
> >
> > Whenever the contributor adds a new library dependency in one module, he
> > should also add a default library instance in the package library instance dsc
> > include file.
> >
> > For example, in this case,
> > Contributor will add the below information in UefiPayloadPkgLib.dsc.inc,
> > SecurityPkgLib.dsc.inc and MdeModulePkgLib.dsc.inc
> >
> > [LibraryClasses]
> >
> >
> VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLi
> > b.inf
> >
> >
> VariablePolicyHelperLib|MdeModulePkg/Library/VariablePolicyHelperLib/Va
> > riablePolicyHelperLib.inf
> > [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> >
> >
> VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLi
> > bRuntimeDxe.inf
> >
> > If the platform already includes these inc files, the code change won't break
> > any build.
> > If the platform wants to choose another library instance, it can specify in the
> > dsc file, and will override the configuration in inc files.
> > This feature can even reduce the code in platform dsc file if platform choose
> > to use default library instance.
> > The problem is that it may compiles redundant modules if the
> >
> > Please give comments about this proposal.
> >
> > Thanks
> > Zhiguang
> >
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > Michael
> > > Kubacki
> > > Sent: Friday, November 20, 2020 4:16 AM
> > > To: devel@edk2.groups.io; awarkentin@vmware.com; Ard Biesheuvel
> > > <ard.biesheuvel@arm.com>; debtech@gmail.com
> > > Cc: Bret Barkelew <bret@corthon.com>; Yao, Jiewen
> > <jiewen.yao@intel.com>;
> > > Bi, Dandan <dandan.bi@intel.com>; Chao Zhang
> > <chao.b.zhang@intel.com>;
> > > Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> > > Liming Gao <liming.gao@intel.com>; Justen, Jordan L
> > > <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Andrew
> > Fish
> > > <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Bret Barkelew
> > > <brbarkel@microsoft.com>
> > > Subject: Re: [edk2-devel] [PATCH v9 00/13] Add the VariablePolicy feature
> > >
> > > While I'm not currently a maintainer in either repo, I believe the current
> > > process is not ideal. I highlighted some of my observations
> > > here: https://edk2.groups.io/g/devel/message/65902.
> > >
> > > Again, I don't have a strong vested interest in this but I do think some level
> > of a
> > > more well defined process needs to be reached between repo maintiners
> > to
> > > ease feature development in the future.
> > >
> > > Thanks,
> > > Michael
> > >
> > > On 11/19/2020 12:02 PM, Andrei Warkentin wrote:
> > > > Hi Bret,
> > > >
> > > > To be honest, I don't recall seeing anything. Again, maybe I should
> > > > have been more proactive, but that's probably the net reality for most
> > > > people. It would be unreasonable to expect you to test every platform,
> > > > but it is very reasonable to assume that if you know you're adding
> > > > build breakage to every platform (that is trivial to fix), that you
> > > > would be taking care of it... Principle of least surprise. And yes, in
> > > > some weird corner case perhaps that would be insufficient (again, I
> > > > don't think anyone would expect you to compile test every platform),
> > > > but it would take care of 99% of obvious fall-out.
> > > >
> > > > For reference, there are occasional clean-ups that happen to the edk2
> > > > tree, and I've never seen anyone claim "not my problem" to deal with
> > > > the obvious fall-out resulting from renames and such.
> > > >
> > > > A
> > > > ----------------------------------------------------------------------
> > > > --
> > > > *From:* devel@edk2.groups.io <devel@edk2.groups.io> on behalf of
> > Bret
> > > > Barkelew via groups.io <debtech=gmail.com@groups.io>
> > > > *Sent:* Thursday, November 19, 2020 10:15 AM
> > > > *To:* Ard Biesheuvel <ard.biesheuvel@arm.com>
> > > > *Cc:* Bret Barkelew <bret@corthon.com>; devel@edk2.groups.io
> > > > <devel@edk2.groups.io>; Jiewen Yao <jiewen.yao@intel.com>; Dandan
> > Bi
> > > > <dandan.bi@intel.com>; Chao Zhang <chao.b.zhang@intel.com>; Jian J
> > > > Wang <jian.j.wang@intel.com>; Hao A Wu <hao.a.wu@intel.com>;
> > Liming
> > > > Gao <liming.gao@intel.com>; Jordan Justen
> <jordan.l.justen@intel.com>;
> > > > Laszlo Ersek <lersek@redhat.com>; Andrew Fish <afish@apple.com>;
> Ray
> > > > Ni <ray.ni@intel.com>; Bret Barkelew <brbarkel@microsoft.com>
> > > > *Subject:* Re: [edk2-devel] [PATCH v9 00/13] Add the VariablePolicy
> > > > feature Those bugs and recommendations were sent out months ago.
> > > > Several platforms have staged the changes already.
> > > >
> > > > You need to add the library class to your DSC.
> > > >
> > > > --
> > > > [ Insert obscure pop-culture reference here. ]
> > > >
> > > >> On Nov 19, 2020, at 4:46 AM, Ard Biesheuvel
> <ard.biesheuvel@arm.com>
> > > wrote:
> > > >>
> > > >> On 11/9/20 7:45 AM, Bret Barkelew wrote:
> > > >>> The 14 patches in this series add the VariablePolicy feature to the
> > > >>> core, deprecate Edk2VarLock (while adding a compatibility layer to
> > > >>> reduce code churn), and integrate the VariablePolicy libraries and
> > > >>> protocols into Variable Services.
> > > >>> Since the integration requires multiple changes, including adding
> > > >>> libraries, a protocol, an SMI communication handler, and
> > > >>> VariableServices integration, the patches are broken up by
> > > >>> individual library additions and then a final integration.
> > > >>> Security-sensitive changes like bypassing Authenticated Variable
> > > >>> enforcement are also broken out into individual patches so that
> > attention
> > > can be called directly to them.
> > > >>> Platform porting instructions are described in this wiki entry:
> > > >>>
> > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> > > >>>
> > thub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FVariablePolicy-P
> > > >>> rotocol---Enhanced-Method-for-Managing-Variables%23platform-
> > porting&
> > > >>>
> > >
> >
> amp;data=04%7C01%7Cawarkentin%40vmware.com%7C594f15b45aaf476bf
> f
> > 7e
> > > 08d
> > > >>>
> > >
> >
> 88cb57390%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637414
> 0
> > 5
> > > 82471
> > > >>>
> > >
> >
> 28819%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2l
> u
> > M
> > > zIiLC
> > > >>>
> > >
> >
> JBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=LLKZ7qeffR0WCvLbY
> > uH
> > > tQI
> > > >>> uwJGhXY0mVqB2w9B0q180%3D&reserved=0
> > > >
> > <https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit
> > > > hub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FVariablePolicy-
> > Prot
> > > > ocol---Enhanced-Method-for-Managing-Variables%23platform-
> > > porting&d
> > > >
> > >
> >
> ata=04%7C01%7Cawarkentin%40vmware.com%7C594f15b45aaf476bff7e08d
> > 88
> > > cb573
> > > >
> > >
> >
> 90%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C6374140582471
> 2
> > 8
> > > 819%7CU
> > > >
> > >
> >
> nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI
> > 6Ik
> > > 1ha
> > > >
> > >
> >
> WwiLCJXVCI6Mn0%3D%7C1000&sdata=LLKZ7qeffR0WCvLbYuHtQIuwJG
> > hX
> > > Y0mVqB2
> > > > w9B0q180%3D&reserved=0>
> > > >>> Discussion of the feature can be found in multiple places throughout
> > > >>> the last year on the RFC channel, staging branches, and in devel.
> > > >>> Most recently, this subject was discussed in this thread:
> > > >>>
> > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fed
> > > >>>
> > >
> >
> k2.groups.io%2Fg%2Fdevel%2Fmessage%2F53712&data=04%7C01%7C
> a
> > wa
> > > rke
> > > >>>
> > >
> >
> ntin%40vmware.com%7C594f15b45aaf476bff7e08d88cb57390%7Cb39138ca
> > 3c
> > > ee4
> > > >>>
> > >
> >
> b4aa4d6cd83d9dd62f0%7C0%7C0%7C637414058247133820%7CUnknown%
> 7
> > CT
> > > WFpbGZ
> > > >>>
> > >
> >
> sb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6M
> > n0
> > > >>> %3D%7C1000&sdata=GYY52rlsPxw07vfdu%2BVbWhzRjtHWXlIG
> ve
> > CTT
> > > 17mlfc%3
> > > >>> D&reserved=0
> > > >
> >
> <https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk
> > > >
> > >
> >
> 2.groups.io%2Fg%2Fdevel%2Fmessage%2F53712&data=04%7C01%7Ca
> > war
> > > kenti
> > > >
> > >
> >
> n%40vmware.com%7C594f15b45aaf476bff7e08d88cb57390%7Cb39138ca3c
> e
> > e
> > > 4b4aa4
> > > >
> > >
> >
> d6cd83d9dd62f0%7C0%7C0%7C637414058247133820%7CUnknown%7CTW
> F
> > pb
> > > GZsb3d8ey
> > > >
> > >
> >
> JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
> %
> > 7
> > > C100
> > > >
> > >
> >
> 0&sdata=GYY52rlsPxw07vfdu%2BVbWhzRjtHWXlIGveCTT17mlfc%3D&a
> > mp
> > > ;reser
> > > > ved=0>
> > > >>> (the code branches shared in that discussion are now out of date,
> > > >>> but the whitepapers and discussion are relevant).
> > > >>> Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > >>> Cc: Dandan Bi <dandan.bi@intel.com>
> > > >>> Cc: Chao Zhang <chao.b.zhang@intel.com>
> > > >>> Cc: Jian J Wang <jian.j.wang@intel.com>
> > > >>> Cc: Hao A Wu <hao.a.wu@intel.com>
> > > >>> Cc: Liming Gao <liming.gao@intel.com>
> > > >>> Cc: Jordan Justen <jordan.l.justen@intel.com>
> > > >>> Cc: Laszlo Ersek <lersek@redhat.com>
> > > >>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> > > >>> Cc: Andrew Fish <afish@apple.com>
> > > >>> Cc: Ray Ni <ray.ni@intel.com>
> > > >>> Cc: Bret Barkelew <brbarkel@microsoft.com>
> > > >>> Signed-off-by: Bret Barkelew <brbarkel@microsoft.com>
> > > >>
> > > >> This series has now made it into edk2, and has subsequently broken
> > every
> > > single platform in edk2-platforms. Is anyone intending to propose any fixes
> > for
> > > this?
> > > >>
> > > >>
> > > >>> v9 changes:
> > > >>> * Rebase
> > > >>> * Address the event ordering issues around MorLock at EndOfDxe
> > > >>> * Drop problematic tests
> > > >>> * Address ECC issues
> > > >>> v8 changes:
> > > >>> * Rebase
> > > >>> * Small tweaks from final PRs
> > > >>> * Drank a lot
> > > >>> * Enrolled several members and a steward in CatFacts
> > > >>> v7 changes:
> > > >>> * Address comments from Dandan about security of the MM handler
> > > >>> * Add readme
> > > >>> * Fix bug around hex characters in BOOT####, etc
> > > >>> * Add additional testing for hex characters
> > > >>> * Add additional testing for authenticated variables
> > > >>> v6 changes:
> > > >>> * Fix an issue with uninitialized Status in InitVariablePolicyLib()
> > > >>>and DeinitVariablePolicyLib()
> > > >>> * Fix GCC building in shell-based functional test
> > > >>> * Rebase on latest origin/master
> > > >>> v5 changes:
> > > >>> * Fix the CONST mismatch in VariablePolicy.h and
> > > >>>VariablePolicySmmDxe.c
> > > >>> * Fix EFIAPI mismatches in the functional unittest
> > > >>> * Rebase on latest origin/master
> > > >>> v4 changes:
> > > >>> * Remove Optional PcdAllowVariablePolicyEnforcementDisable PCD
> > from
> > > >>>platforms
> > > >>> * Rebase on master
> > > >>> * Migrate to new MmCommunicate2 protocol
> > > >>> * Fix an oversight in the default return value for
> > > >>>InitMmCommonCommBuffer
> > > >>> * Fix in VariablePolicyLib to allow ExtraInitRuntimeDxe to consume
> > > >>>variables
> > > >>> V3 changes:
> > > >>> * Address all non-unittest issues with ECC
> > > >>> * Make additional style changes
> > > >>> * Include section name in hunk headers in "ini-style" files
> > > >>> * Remove requirement for the
> > EdkiiPiSmmCommunicationsRegionTable
> > > >>>driver
> > > >>> (now allocates its own buffer)
> > > >>> * Change names from VARIABLE_POLICY_PROTOCOL and
> > > >>>gVariablePolicyProtocolGuid
> > > >>> to EDKII_VARIABLE_POLICY_PROTOCOL and
> > > >>>gEdkiiVariablePolicyProtocolGuid
> > > >>> * Fix GCC warning about initializing externs
> > > >>> * Add UNI strings for new PCD
> > > >>> * Add patches for ArmVirtPkg, OvmfXen, and UefiPayloadPkg
> > > >>> * Reorder patches according to Liming's feedback about adding to
> > > >>>platforms
> > > >>> before changing variable driver
> > > >>> V2 changes:
> > > >>> * Fixed implementation for RuntimeDxe
> > > >>> * Add PCD to block DisableVariablePolicy
> > > >>> * Fix the DumpVariablePolicy pagination in SMM Bret Barkelew
> (13):
> > > >>> MdeModulePkg: Define the VariablePolicy protocol interface
> > > >>> MdeModulePkg: Define the VariablePolicyLib
> > > >>> MdeModulePkg: Define the VariablePolicyHelperLib
> > > >>> MdeModulePkg: Define the VarCheckPolicyLib and SMM interface
> > > >>> OvmfPkg: Add VariablePolicy engine to OvmfPkg platform
> > > >>> EmulatorPkg: Add VariablePolicy engine to EmulatorPkg platform
> > > >>> ArmVirtPkg: Add VariablePolicy engine to ArmVirtPkg platform
> > > >>> UefiPayloadPkg: Add VariablePolicy engine to UefiPayloadPkg
> > > >>>platform
> > > >>> MdeModulePkg: Connect VariablePolicy business logic to
> > > >>> VariableServices
> > > >>> MdeModulePkg: Allow VariablePolicy state to delete protected
> > > >>>variables
> > > >>> SecurityPkg: Allow VariablePolicy state to delete authenticated
> > > >>> variables
> > > >>> MdeModulePkg: Change TCG MOR variables to use VariablePolicy
> > > >>> MdeModulePkg: Drop VarLock from RuntimeDxe variable driver
> > > >>> MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c
> > > >>>| 346 ++++++++
> > > >>>
> > > >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelper
> Li
> > b.
> > > >>>c | 396 ++++++++++
> > > >>>
> > > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitNull.
> c
> > > >>>| 46 ++
> > > >>>
> > > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitRunti
> > me
> > > >>>Dxe.c | 85 ++
> > > >>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c
> > > >>>| 830 ++++++++++++++++++++
> > > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
> > >
> > > >>>| 52 +-
> > > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.
> c
> > >
> > > >>>| 60 +-
> > > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VarCheck.c
> > > >>>| 49 +-
> > > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
> > > >>>| 60 ++
> > > >>>
> > > >>>MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequest
> T
> > oLo
> > > ck.
> > > >>>c | 71 ++
> > > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySm
> mD
> > xe.c
> > >
> > > >>>| 573 ++++++++++++++
> > > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> > >
> > > >>>| 7 +
> > > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRunt
> im
> > eDx
> > > e.c
> > > >>>| 14 +
> > > >>> SecurityPkg/Library/AuthVariableLib/AuthService.c
> > > >>>| 30 +-
> > > >>> ArmVirtPkg/ArmVirt.dsc.inc
> > > >>>| 4 +
> > > >>> EmulatorPkg/EmulatorPkg.dsc
> > > >>>| 3 +
> > > >>> MdeModulePkg/Include/Guid/VarCheckPolicyMmi.h
> > > >>>| 54 ++
> > > >>> MdeModulePkg/Include/Library/VariablePolicyHelperLib.h
> > > >>>| 164 ++++
> > > >>> MdeModulePkg/Include/Library/VariablePolicyLib.h
> > > >>>| 207 +++++
> > > >>> MdeModulePkg/Include/Protocol/VariablePolicy.h
> > > >>>| 157 ++++
> > > >>> MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
> > > >>>| 42 +
> > > >>> MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.uni
> > > >>>| 12 +
> > > >>>
> > > >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelper
> Li
> > b.
> > > >>>inf | 35 +
> > > >>>
> > > >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelper
> Li
> > b.
> > > >>>uni | 12 +
> > > >>> MdeModulePkg/Library/VariablePolicyLib/ReadMe.md
> > > >>>| 406 ++++++++++
> > > >>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf
> > > >>>| 48 ++
> > > >>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.uni
> > > >>>| 12 +
> > > >>>
> > > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDx
> e
> > .in
> > > >>>f | 51 ++
> > > >>> MdeModulePkg/MdeModulePkg.ci.yaml
> > > >>>| 4 +-
> > > >>> MdeModulePkg/MdeModulePkg.dec
> > > >>>| 26 +-
> > > >>> MdeModulePkg/MdeModulePkg.dsc
> > > >>>| 9 +
> > > >>> MdeModulePkg/MdeModulePkg.uni
> > > >>>| 7 +
> > > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeD
> xe
> > .inf
> > >
> > > >>>| 5 +
> > > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> > >
> > > >>>| 4 +
> > > >>>
> > > >>>MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntim
> e
> > Dxe.
> > > inf
> > > >>>| 11 +
> > > >>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandalon
> e
> > Mm.i
> > > nf
> > > >>>| 4 +
> > > >>> OvmfPkg/OvmfPkgIa32.dsc
> > > >>>| 5 +
> > > >>> OvmfPkg/OvmfPkgIa32X64.dsc
> > > >>>| 5 +
> > > >>> OvmfPkg/OvmfPkgX64.dsc
> > > >>>| 5 +
> > > >>> OvmfPkg/OvmfXen.dsc
> > > >>>| 4 +
> > > >>> SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
> > > >>>| 2 +
> > > >>> UefiPayloadPkg/UefiPayloadPkgIa32.dsc
> > > >>>| 4 +
> > > >>> UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
> > > >>>| 4 +
> > > >>> 43 files changed, 3845 insertions(+), 80 deletions(-)
> > > >>> create mode 100644
> > > >>>MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c
> > > >>> create mode 100644
> > > >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelper
> Li
> > b.
> > > >>>c
> > > >>> create mode 100644
> > > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitNull.
> c
> > > >>> create mode 100644
> > > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitRunti
> > me
> > > >>>Dxe.c
> > > >>> create mode 100644
> > > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c
> > > >>> create mode 100644
> > > >>>MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequest
> T
> > oLo
> > > ck.
> > > >>>c
> > > >>> create mode 100644
> > > >>>MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmD
> x
> > e.c
> > > >>> create mode 100644
> > MdeModulePkg/Include/Guid/VarCheckPolicyMmi.h
> > > >>> create mode 100644
> > > >>>MdeModulePkg/Include/Library/VariablePolicyHelperLib.h
> > > >>> create mode 100644
> > MdeModulePkg/Include/Library/VariablePolicyLib.h
> > > >>> create mode 100644
> > MdeModulePkg/Include/Protocol/VariablePolicy.h
> > > >>> create mode 100644
> > > >>>MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
> > > >>> create mode 100644
> > > >>>MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.uni
> > > >>> create mode 100644
> > > >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelper
> Li
> > b.
> > > >>>inf
> > > >>> create mode 100644
> > > >>>MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelper
> Li
> > b.
> > > >>>uni
> > > >>> create mode 100644
> > > MdeModulePkg/Library/VariablePolicyLib/ReadMe.md
> > > >>> create mode 100644
> > > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf
> > > >>> create mode 100644
> > > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.uni
> > > >>> create mode 100644
> > > >>>MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDx
> e
> > .in
> > > >>>f
> > > >>
> > > >
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
>
>
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: 回复: [edk2-devel] A proposal to reduce incompatible case
2020-11-20 7:27 ` 回复: [edk2-devel] " gaoliming
@ 2020-11-20 9:02 ` Ard Biesheuvel
2020-11-20 11:17 ` Laszlo Ersek
0 siblings, 1 reply; 11+ messages in thread
From: Ard Biesheuvel @ 2020-11-20 9:02 UTC (permalink / raw)
To: gaoliming, devel, jiewen.yao, 'Liu, Zhiguang',
michael.kubacki, awarkentin, debtech, 'Feng, Bob C',
'Tian, Hot'
Cc: 'Bret Barkelew', 'Bi, Dandan',
'Chao Zhang', 'Wang, Jian J', 'Wu, Hao A',
'Liming Gao', 'Justen, Jordan L',
'Laszlo Ersek', 'Andrew Fish', 'Ni, Ray',
'Bret Barkelew', 'Kinney, Michael D'
On 11/20/20 8:27 AM, gaoliming wrote:
> Zhiguang:
> This proposal can reduce the potential library class dependency. Each package has its xxxPkgLib.dsc.inc file that includes the library instances from this package.
> Platform DSC can include the required package lib.dsc.inc file for the library instances. Can you work out the code changes to demonstrate this idea?
>
+1 for this idea. This would allow us to remove a *lot* of boilerplate
in the .DSC files, and focus on the libraries that actually matter for
the platform at hand.
>> -----邮件原件-----
>> 发件人: bounce+27952+67752+4905953+8761045@groups.io
>> <bounce+27952+67752+4905953+8761045@groups.io> 代表 Yao, Jiewen
>> 发送时间: 2020年11月20日 15:20
>> 收件人: Liu, Zhiguang <zhiguang.liu@intel.com>; devel@edk2.groups.io;
>> michael.kubacki@outlook.com; awarkentin@vmware.com; Ard Biesheuvel
>> <ard.biesheuvel@arm.com>; debtech@gmail.com; Feng, Bob C
>> <bob.c.feng@intel.com>; Tian, Hot <hot.tian@intel.com>
>> 抄送: Bret Barkelew <bret@corthon.com>; Bi, Dandan
>> <dandan.bi@intel.com>; Chao Zhang <chao.b.zhang@intel.com>; Wang, Jian
>> J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Liming Gao
>> <liming.gao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Laszlo
>> Ersek <lersek@redhat.com>; Andrew Fish <afish@apple.com>; Ni, Ray
>> <ray.ni@intel.com>; Bret Barkelew <brbarkel@microsoft.com>; Kinney,
>> Michael D <michael.d.kinney@intel.com>; Liming Gao
>> <gaoliming@byosoft.com.cn>
>> 主题: Re: [edk2-devel] A proposal to reduce incompatible case
>>
>> I like this idea. MinPlatform also adopt the same strategy - define common
>> stuff in a dsc include file @
>> https://github.com/tianocore/edk2-platforms/tree/master/Platform/Intel/Mi
>> nPlatformPkg/Include/Dsc
>>
>>
>> A minor clarification:
>> For VariablePolicyLib, I think we just need add to MdeModulePkgLib.dsc.inc.
>> We don’t need update UefiPayloadPkgLib.dsc.inc or SecurityPkgLib.dsc.inc,
>> right? They are just consumer, not producer.
>>
>> Thank you
>> Yao Jiewen
>>
>>
>>> -----Original Message-----
>>> From: Liu, Zhiguang <zhiguang.liu@intel.com>
>>> Sent: Friday, November 20, 2020 2:52 PM
>>> To: devel@edk2.groups.io; michael.kubacki@outlook.com;
>>> awarkentin@vmware.com; Ard Biesheuvel <ard.biesheuvel@arm.com>;
>>> debtech@gmail.com; Feng, Bob C <bob.c.feng@intel.com>; Tian, Hot
>>> <hot.tian@intel.com>
>>> Cc: Bret Barkelew <bret@corthon.com>; Yao, Jiewen
>>> <jiewen.yao@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Chao Zhang
>>> <chao.b.zhang@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao
>>> A <hao.a.wu@intel.com>; Liming Gao <liming.gao@intel.com>; Justen,
>>> Jordan L <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>;
>>> Andrew Fish <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Bret
>> Barkelew
>>> <brbarkel@microsoft.com>; Kinney, Michael D
>>> <michael.d.kinney@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>
>>> Subject: A proposal to reduce incompatible case
>>>
>>> Hi all,
>>>
>>> As Michael mentioned, there are some platforms do not build and some is
>>> because incompatible code change like this one.
>>> I think it is a burden for both contributor and maintainer to fix platform code
>>> when meeting such incompatible change.
>>> I want to proposal one solution to minimum the effort of such code change.
>>>
>>> We could add a package library instance dsc include file under each
>> package,
>>> like XXXPkgLib.dsc.inc
>>> It will specify the default library instance that will be used by modules in this
>>> package.
>>> For example, we add MdeModulePkgLib.dsc.inc file in MdeModulePkg.
>>> Some package already has similar dsc include file, such as
>>> ArmVirtPkg/ArmVirt.dsc.inc and NetworkPkg\Network.dsc.inc.
>>> In platform dsc file, we include the XXXPkgLib.dsc.inc file at the beginning,
>> if
>>> the platform uses component from the package.
>>> We place the inc file in the beginning because we can override the library
>>> instance in other part of the platform dsc file.
>>>
>>> Whenever the contributor adds a new library dependency in one module, he
>>> should also add a default library instance in the package library instance dsc
>>> include file.
>>>
>>> For example, in this case,
>>> Contributor will add the below information in UefiPayloadPkgLib.dsc.inc,
>>> SecurityPkgLib.dsc.inc and MdeModulePkgLib.dsc.inc
>>>
>>> [LibraryClasses]
>>>
>>>
>> VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLi
>>> b.inf
>>>
>>>
>> VariablePolicyHelperLib|MdeModulePkg/Library/VariablePolicyHelperLib/Va
>>> riablePolicyHelperLib.inf
>>> [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>>>
>>>
>> VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLi
>>> bRuntimeDxe.inf
>>>
>>> If the platform already includes these inc files, the code change won't break
>>> any build.
>>> If the platform wants to choose another library instance, it can specify in the
>>> dsc file, and will override the configuration in inc files.
>>> This feature can even reduce the code in platform dsc file if platform choose
>>> to use default library instance.
>>> The problem is that it may compiles redundant modules if the
>>>
>>> Please give comments about this proposal.
>>>
>>> Thanks
>>> Zhiguang
>>>
>>>
>>>> -----Original Message-----
>>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
>>> Michael
>>>> Kubacki
>>>> Sent: Friday, November 20, 2020 4:16 AM
>>>> To: devel@edk2.groups.io; awarkentin@vmware.com; Ard Biesheuvel
>>>> <ard.biesheuvel@arm.com>; debtech@gmail.com
>>>> Cc: Bret Barkelew <bret@corthon.com>; Yao, Jiewen
>>> <jiewen.yao@intel.com>;
>>>> Bi, Dandan <dandan.bi@intel.com>; Chao Zhang
>>> <chao.b.zhang@intel.com>;
>>>> Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
>>>> Liming Gao <liming.gao@intel.com>; Justen, Jordan L
>>>> <jordan.l.justen@intel.com>; Laszlo Ersek <lersek@redhat.com>; Andrew
>>> Fish
>>>> <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; Bret Barkelew
>>>> <brbarkel@microsoft.com>
>>>> Subject: Re: [edk2-devel] [PATCH v9 00/13] Add the VariablePolicy feature
>>>>
>>>> While I'm not currently a maintainer in either repo, I believe the current
>>>> process is not ideal. I highlighted some of my observations
>>>> here: https://edk2.groups.io/g/devel/message/65902.
>>>>
>>>> Again, I don't have a strong vested interest in this but I do think some level
>>> of a
>>>> more well defined process needs to be reached between repo maintiners
>>> to
>>>> ease feature development in the future.
>>>>
>>>> Thanks,
>>>> Michael
>>>>
>>>> On 11/19/2020 12:02 PM, Andrei Warkentin wrote:
>>>>> Hi Bret,
>>>>>
>>>>> To be honest, I don't recall seeing anything. Again, maybe I should
>>>>> have been more proactive, but that's probably the net reality for most
>>>>> people. It would be unreasonable to expect you to test every platform,
>>>>> but it is very reasonable to assume that if you know you're adding
>>>>> build breakage to every platform (that is trivial to fix), that you
>>>>> would be taking care of it... Principle of least surprise. And yes, in
>>>>> some weird corner case perhaps that would be insufficient (again, I
>>>>> don't think anyone would expect you to compile test every platform),
>>>>> but it would take care of 99% of obvious fall-out.
>>>>>
>>>>> For reference, there are occasional clean-ups that happen to the edk2
>>>>> tree, and I've never seen anyone claim "not my problem" to deal with
>>>>> the obvious fall-out resulting from renames and such.
>>>>>
>>>>> A
>>>>> ----------------------------------------------------------------------
>>>>> --
>>>>> *From:* devel@edk2.groups.io <devel@edk2.groups.io> on behalf of
>>> Bret
>>>>> Barkelew via groups.io <debtech=gmail.com@groups.io>
>>>>> *Sent:* Thursday, November 19, 2020 10:15 AM
>>>>> *To:* Ard Biesheuvel <ard.biesheuvel@arm.com>
>>>>> *Cc:* Bret Barkelew <bret@corthon.com>; devel@edk2.groups.io
>>>>> <devel@edk2.groups.io>; Jiewen Yao <jiewen.yao@intel.com>; Dandan
>>> Bi
>>>>> <dandan.bi@intel.com>; Chao Zhang <chao.b.zhang@intel.com>; Jian J
>>>>> Wang <jian.j.wang@intel.com>; Hao A Wu <hao.a.wu@intel.com>;
>>> Liming
>>>>> Gao <liming.gao@intel.com>; Jordan Justen
>> <jordan.l.justen@intel.com>;
>>>>> Laszlo Ersek <lersek@redhat.com>; Andrew Fish <afish@apple.com>;
>> Ray
>>>>> Ni <ray.ni@intel.com>; Bret Barkelew <brbarkel@microsoft.com>
>>>>> *Subject:* Re: [edk2-devel] [PATCH v9 00/13] Add the VariablePolicy
>>>>> feature Those bugs and recommendations were sent out months ago.
>>>>> Several platforms have staged the changes already.
>>>>>
>>>>> You need to add the library class to your DSC.
>>>>>
>>>>> --
>>>>> [ Insert obscure pop-culture reference here. ]
>>>>>
>>>>>> On Nov 19, 2020, at 4:46 AM, Ard Biesheuvel
>> <ard.biesheuvel@arm.com>
>>>> wrote:
>>>>>>
>>>>>> On 11/9/20 7:45 AM, Bret Barkelew wrote:
>>>>>>> The 14 patches in this series add the VariablePolicy feature to the
>>>>>>> core, deprecate Edk2VarLock (while adding a compatibility layer to
>>>>>>> reduce code churn), and integrate the VariablePolicy libraries and
>>>>>>> protocols into Variable Services.
>>>>>>> Since the integration requires multiple changes, including adding
>>>>>>> libraries, a protocol, an SMI communication handler, and
>>>>>>> VariableServices integration, the patches are broken up by
>>>>>>> individual library additions and then a final integration.
>>>>>>> Security-sensitive changes like bypassing Authenticated Variable
>>>>>>> enforcement are also broken out into individual patches so that
>>> attention
>>>> can be called directly to them.
>>>>>>> Platform porting instructions are described in this wiki entry:
>>>>>>>
>>> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
>>>>>>>
>>> thub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FVariablePolicy-P
>>>>>>> rotocol---Enhanced-Method-for-Managing-Variables%23platform-
>>> porting&
>>>>>>>
>>>>
>>>
>> amp;data=04%7C01%7Cawarkentin%40vmware.com%7C594f15b45aaf476bf
>> f
>>> 7e
>>>> 08d
>>>>>>>
>>>>
>>>
>> 88cb57390%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637414
>> 0
>>> 5
>>>> 82471
>>>>>>>
>>>>
>>>
>> 28819%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2l
>> u
>>> M
>>>> zIiLC
>>>>>>>
>>>>
>>>
>> JBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=LLKZ7qeffR0WCvLbY
>>> uH
>>>> tQI
>>>>>>> uwJGhXY0mVqB2w9B0q180%3D&reserved=0
>>>>>
>>> <https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit
>>>>> hub.com%2Ftianocore%2Ftianocore.github.io%2Fwiki%2FVariablePolicy-
>>> Prot
>>>>> ocol---Enhanced-Method-for-Managing-Variables%23platform-
>>>> porting&d
>>>>>
>>>>
>>>
>> ata=04%7C01%7Cawarkentin%40vmware.com%7C594f15b45aaf476bff7e08d
>>> 88
>>>> cb573
>>>>>
>>>>
>>>
>> 90%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C6374140582471
>> 2
>>> 8
>>>> 819%7CU
>>>>>
>>>>
>>>
>> nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI
>>> 6Ik
>>>> 1ha
>>>>>
>>>>
>>>
>> WwiLCJXVCI6Mn0%3D%7C1000&sdata=LLKZ7qeffR0WCvLbYuHtQIuwJG
>>> hX
>>>> Y0mVqB2
>>>>> w9B0q180%3D&reserved=0>
>>>>>>> Discussion of the feature can be found in multiple places throughout
>>>>>>> the last year on the RFC channel, staging branches, and in devel.
>>>>>>> Most recently, this subject was discussed in this thread:
>>>>>>>
>>> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fed
>>>>>>>
>>>>
>>>
>> k2.groups.io%2Fg%2Fdevel%2Fmessage%2F53712&data=04%7C01%7C
>> a
>>> wa
>>>> rke
>>>>>>>
>>>>
>>>
>> ntin%40vmware.com%7C594f15b45aaf476bff7e08d88cb57390%7Cb39138ca
>>> 3c
>>>> ee4
>>>>>>>
>>>>
>>>
>> b4aa4d6cd83d9dd62f0%7C0%7C0%7C637414058247133820%7CUnknown%
>> 7
>>> CT
>>>> WFpbGZ
>>>>>>>
>>>>
>>>
>> sb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6M
>>> n0
>>>>>>> %3D%7C1000&sdata=GYY52rlsPxw07vfdu%2BVbWhzRjtHWXlIG
>> ve
>>> CTT
>>>> 17mlfc%3
>>>>>>> D&reserved=0
>>>>>
>>>
>> <https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk
>>>>>
>>>>
>>>
>> 2.groups.io%2Fg%2Fdevel%2Fmessage%2F53712&data=04%7C01%7Ca
>>> war
>>>> kenti
>>>>>
>>>>
>>>
>> n%40vmware.com%7C594f15b45aaf476bff7e08d88cb57390%7Cb39138ca3c
>> e
>>> e
>>>> 4b4aa4
>>>>>
>>>>
>>>
>> d6cd83d9dd62f0%7C0%7C0%7C637414058247133820%7CUnknown%7CTW
>> F
>>> pb
>>>> GZsb3d8ey
>>>>>
>>>>
>>>
>> JWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
>> %
>>> 7
>>>> C100
>>>>>
>>>>
>>>
>> 0&sdata=GYY52rlsPxw07vfdu%2BVbWhzRjtHWXlIGveCTT17mlfc%3D&a
>>> mp
>>>> ;reser
>>>>> ved=0>
>>>>>>> (the code branches shared in that discussion are now out of date,
>>>>>>> but the whitepapers and discussion are relevant).
>>>>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>>>>>> Cc: Dandan Bi <dandan.bi@intel.com>
>>>>>>> Cc: Chao Zhang <chao.b.zhang@intel.com>
>>>>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>>>>> Cc: Hao A Wu <hao.a.wu@intel.com>
>>>>>>> Cc: Liming Gao <liming.gao@intel.com>
>>>>>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>>>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>>>>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>>>>>>> Cc: Andrew Fish <afish@apple.com>
>>>>>>> Cc: Ray Ni <ray.ni@intel.com>
>>>>>>> Cc: Bret Barkelew <brbarkel@microsoft.com>
>>>>>>> Signed-off-by: Bret Barkelew <brbarkel@microsoft.com>
>>>>>>
>>>>>> This series has now made it into edk2, and has subsequently broken
>>> every
>>>> single platform in edk2-platforms. Is anyone intending to propose any fixes
>>> for
>>>> this?
>>>>>>
>>>>>>
>>>>>>> v9 changes:
>>>>>>> * Rebase
>>>>>>> * Address the event ordering issues around MorLock at EndOfDxe
>>>>>>> * Drop problematic tests
>>>>>>> * Address ECC issues
>>>>>>> v8 changes:
>>>>>>> * Rebase
>>>>>>> * Small tweaks from final PRs
>>>>>>> * Drank a lot
>>>>>>> * Enrolled several members and a steward in CatFacts
>>>>>>> v7 changes:
>>>>>>> * Address comments from Dandan about security of the MM handler
>>>>>>> * Add readme
>>>>>>> * Fix bug around hex characters in BOOT####, etc
>>>>>>> * Add additional testing for hex characters
>>>>>>> * Add additional testing for authenticated variables
>>>>>>> v6 changes:
>>>>>>> * Fix an issue with uninitialized Status in InitVariablePolicyLib()
>>>>>>> and DeinitVariablePolicyLib()
>>>>>>> * Fix GCC building in shell-based functional test
>>>>>>> * Rebase on latest origin/master
>>>>>>> v5 changes:
>>>>>>> * Fix the CONST mismatch in VariablePolicy.h and
>>>>>>> VariablePolicySmmDxe.c
>>>>>>> * Fix EFIAPI mismatches in the functional unittest
>>>>>>> * Rebase on latest origin/master
>>>>>>> v4 changes:
>>>>>>> * Remove Optional PcdAllowVariablePolicyEnforcementDisable PCD
>>> from
>>>>>>> platforms
>>>>>>> * Rebase on master
>>>>>>> * Migrate to new MmCommunicate2 protocol
>>>>>>> * Fix an oversight in the default return value for
>>>>>>> InitMmCommonCommBuffer
>>>>>>> * Fix in VariablePolicyLib to allow ExtraInitRuntimeDxe to consume
>>>>>>> variables
>>>>>>> V3 changes:
>>>>>>> * Address all non-unittest issues with ECC
>>>>>>> * Make additional style changes
>>>>>>> * Include section name in hunk headers in "ini-style" files
>>>>>>> * Remove requirement for the
>>> EdkiiPiSmmCommunicationsRegionTable
>>>>>>> driver
>>>>>>> (now allocates its own buffer)
>>>>>>> * Change names from VARIABLE_POLICY_PROTOCOL and
>>>>>>> gVariablePolicyProtocolGuid
>>>>>>> to EDKII_VARIABLE_POLICY_PROTOCOL and
>>>>>>> gEdkiiVariablePolicyProtocolGuid
>>>>>>> * Fix GCC warning about initializing externs
>>>>>>> * Add UNI strings for new PCD
>>>>>>> * Add patches for ArmVirtPkg, OvmfXen, and UefiPayloadPkg
>>>>>>> * Reorder patches according to Liming's feedback about adding to
>>>>>>> platforms
>>>>>>> before changing variable driver
>>>>>>> V2 changes:
>>>>>>> * Fixed implementation for RuntimeDxe
>>>>>>> * Add PCD to block DisableVariablePolicy
>>>>>>> * Fix the DumpVariablePolicy pagination in SMM Bret Barkelew
>> (13):
>>>>>>> MdeModulePkg: Define the VariablePolicy protocol interface
>>>>>>> MdeModulePkg: Define the VariablePolicyLib
>>>>>>> MdeModulePkg: Define the VariablePolicyHelperLib
>>>>>>> MdeModulePkg: Define the VarCheckPolicyLib and SMM interface
>>>>>>> OvmfPkg: Add VariablePolicy engine to OvmfPkg platform
>>>>>>> EmulatorPkg: Add VariablePolicy engine to EmulatorPkg platform
>>>>>>> ArmVirtPkg: Add VariablePolicy engine to ArmVirtPkg platform
>>>>>>> UefiPayloadPkg: Add VariablePolicy engine to UefiPayloadPkg
>>>>>>> platform
>>>>>>> MdeModulePkg: Connect VariablePolicy business logic to
>>>>>>> VariableServices
>>>>>>> MdeModulePkg: Allow VariablePolicy state to delete protected
>>>>>>> variables
>>>>>>> SecurityPkg: Allow VariablePolicy state to delete authenticated
>>>>>>> variables
>>>>>>> MdeModulePkg: Change TCG MOR variables to use VariablePolicy
>>>>>>> MdeModulePkg: Drop VarLock from RuntimeDxe variable driver
>>>>>>> MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c
>>>>>>> | 346 ++++++++
>>>>>>>
>>>>>>> MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelper
>> Li
>>> b.
>>>>>>> c | 396 ++++++++++
>>>>>>>
>>>>>>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitNull.
>> c
>>>>>>> | 46 ++
>>>>>>>
>>>>>>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitRunti
>>> me
>>>>>>> Dxe.c | 85 ++
>>>>>>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c
>>>>>>> | 830 ++++++++++++++++++++
>>>>>>> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockDxe.c
>>>>
>>>>>>> | 52 +-
>>>>>>> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.
>> c
>>>>
>>>>>>> | 60 +-
>>>>>>> MdeModulePkg/Universal/Variable/RuntimeDxe/VarCheck.c
>>>>>>> | 49 +-
>>>>>>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
>>>>>>> | 60 ++
>>>>>>>
>>>>>>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequest
>> T
>>> oLo
>>>> ck.
>>>>>>> c | 71 ++
>>>>>>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySm
>> mD
>>> xe.c
>>>>
>>>>>>> | 573 ++++++++++++++
>>>>>>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
>>>>
>>>>>>> | 7 +
>>>>>>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRunt
>> im
>>> eDx
>>>> e.c
>>>>>>> | 14 +
>>>>>>> SecurityPkg/Library/AuthVariableLib/AuthService.c
>>>>>>> | 30 +-
>>>>>>> ArmVirtPkg/ArmVirt.dsc.inc
>>>>>>> | 4 +
>>>>>>> EmulatorPkg/EmulatorPkg.dsc
>>>>>>> | 3 +
>>>>>>> MdeModulePkg/Include/Guid/VarCheckPolicyMmi.h
>>>>>>> | 54 ++
>>>>>>> MdeModulePkg/Include/Library/VariablePolicyHelperLib.h
>>>>>>> | 164 ++++
>>>>>>> MdeModulePkg/Include/Library/VariablePolicyLib.h
>>>>>>> | 207 +++++
>>>>>>> MdeModulePkg/Include/Protocol/VariablePolicy.h
>>>>>>> | 157 ++++
>>>>>>> MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
>>>>>>> | 42 +
>>>>>>> MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.uni
>>>>>>> | 12 +
>>>>>>>
>>>>>>> MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelper
>> Li
>>> b.
>>>>>>> inf | 35 +
>>>>>>>
>>>>>>> MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelper
>> Li
>>> b.
>>>>>>> uni | 12 +
>>>>>>> MdeModulePkg/Library/VariablePolicyLib/ReadMe.md
>>>>>>> | 406 ++++++++++
>>>>>>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf
>>>>>>> | 48 ++
>>>>>>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.uni
>>>>>>> | 12 +
>>>>>>>
>>>>>>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDx
>> e
>>> .in
>>>>>>> f | 51 ++
>>>>>>> MdeModulePkg/MdeModulePkg.ci.yaml
>>>>>>> | 4 +-
>>>>>>> MdeModulePkg/MdeModulePkg.dec
>>>>>>> | 26 +-
>>>>>>> MdeModulePkg/MdeModulePkg.dsc
>>>>>>> | 9 +
>>>>>>> MdeModulePkg/MdeModulePkg.uni
>>>>>>> | 7 +
>>>>>>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeD
>> xe
>>> .inf
>>>>
>>>>>>> | 5 +
>>>>>>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
>>>>
>>>>>>> | 4 +
>>>>>>>
>>>>>>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntim
>> e
>>> Dxe.
>>>> inf
>>>>>>> | 11 +
>>>>>>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandalon
>> e
>>> Mm.i
>>>> nf
>>>>>>> | 4 +
>>>>>>> OvmfPkg/OvmfPkgIa32.dsc
>>>>>>> | 5 +
>>>>>>> OvmfPkg/OvmfPkgIa32X64.dsc
>>>>>>> | 5 +
>>>>>>> OvmfPkg/OvmfPkgX64.dsc
>>>>>>> | 5 +
>>>>>>> OvmfPkg/OvmfXen.dsc
>>>>>>> | 4 +
>>>>>>> SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
>>>>>>> | 2 +
>>>>>>> UefiPayloadPkg/UefiPayloadPkgIa32.dsc
>>>>>>> | 4 +
>>>>>>> UefiPayloadPkg/UefiPayloadPkgIa32X64.dsc
>>>>>>> | 4 +
>>>>>>> 43 files changed, 3845 insertions(+), 80 deletions(-)
>>>>>>> create mode 100644
>>>>>>> MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.c
>>>>>>> create mode 100644
>>>>>>> MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelper
>> Li
>>> b.
>>>>>>> c
>>>>>>> create mode 100644
>>>>>>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitNull.
>> c
>>>>>>> create mode 100644
>>>>>>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyExtraInitRunti
>>> me
>>>>>>> Dxe.c
>>>>>>> create mode 100644
>>>>>>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.c
>>>>>>> create mode 100644
>>>>>>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableLockRequest
>> T
>>> oLo
>>>> ck.
>>>>>>> c
>>>>>>> create mode 100644
>>>>>>> MdeModulePkg/Universal/Variable/RuntimeDxe/VariablePolicySmmD
>> x
>>> e.c
>>>>>>> create mode 100644
>>> MdeModulePkg/Include/Guid/VarCheckPolicyMmi.h
>>>>>>> create mode 100644
>>>>>>> MdeModulePkg/Include/Library/VariablePolicyHelperLib.h
>>>>>>> create mode 100644
>>> MdeModulePkg/Include/Library/VariablePolicyLib.h
>>>>>>> create mode 100644
>>> MdeModulePkg/Include/Protocol/VariablePolicy.h
>>>>>>> create mode 100644
>>>>>>> MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.inf
>>>>>>> create mode 100644
>>>>>>> MdeModulePkg/Library/VarCheckPolicyLib/VarCheckPolicyLib.uni
>>>>>>> create mode 100644
>>>>>>> MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelper
>> Li
>>> b.
>>>>>>> inf
>>>>>>> create mode 100644
>>>>>>> MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelper
>> Li
>>> b.
>>>>>>> uni
>>>>>>> create mode 100644
>>>> MdeModulePkg/Library/VariablePolicyLib/ReadMe.md
>>>>>>> create mode 100644
>>>>>>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.inf
>>>>>>> create mode 100644
>>>>>>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLib.uni
>>>>>>> create mode 100644
>>>>>>> MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDx
>> e
>>> .in
>>>>>>> f
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>>
>>>>
>>
>>
>>
>>
>>
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: 回复: [edk2-devel] A proposal to reduce incompatible case
2020-11-20 9:02 ` Ard Biesheuvel
@ 2020-11-20 11:17 ` Laszlo Ersek
2020-11-21 6:07 ` Zhiguang Liu
0 siblings, 1 reply; 11+ messages in thread
From: Laszlo Ersek @ 2020-11-20 11:17 UTC (permalink / raw)
To: Ard Biesheuvel, gaoliming, devel, jiewen.yao,
'Liu, Zhiguang', michael.kubacki, awarkentin, debtech,
'Feng, Bob C', 'Tian, Hot'
Cc: 'Bret Barkelew', 'Bi, Dandan',
'Chao Zhang', 'Wang, Jian J', 'Wu, Hao A',
'Liming Gao', 'Justen, Jordan L',
'Andrew Fish', 'Ni, Ray', 'Bret Barkelew',
'Kinney, Michael D'
On 11/20/20 10:02, Ard Biesheuvel wrote:
> On 11/20/20 8:27 AM, gaoliming wrote:
>> Zhiguang:
>> This proposal can reduce the potential library class dependency.
>> Each package has its xxxPkgLib.dsc.inc file that includes the library
>> instances from this package.
>> Platform DSC can include the required package lib.dsc.inc file for
>> the library instances. Can you work out the code changes to
>> demonstrate this idea?
>>
>
> +1 for this idea. This would allow us to remove a *lot* of boilerplate
> in the .DSC files, and focus on the libraries that actually matter for
> the platform at hand.
I feel like I'm in *cautious* agreement with this idea.
In particular, I'd *only* like those lib class -> lib instance
resolutions to be extracted, to DSC include files, that *cannot* involve
platform choice. To put it differently, single-instance lib classes.
("Single-instance" can be interpreted per module type / per architecture
of course -- so if a lib class has exactly 1 instance for PEIMs and
exactly 1 (different) instance for DXE_DRIVERs, then those resolutions
qualify for extraction.)
If a library class genuinely has multiple instances in core edk2, then
extracting a "default" resolution would be *wrong*, in my opinion. Every
platform needs to make the choice explicitly, in such cases. This
applies even if a lib class only has two instances, a functional one,
and a Null one. The functional one should not be assumed default.
Example: extracting the UefiLib resolution is valid. Extracting a
SerialPortLib class resolution is invalid.
Basically, I'd like to avoid *overrides*. Overrides are terribly hard to
reason about.
... If someone wants to work on this, they'll have to implement this
with *super small* patches, where every patch extracts resolutions for
just one lib class. I would reject any patch that required me to review
the extraction of two or more lib classes, from an OVMF or ArmVirt DSC
file, at the same time.
There is big potential in this proposal for making platform DSC files
*permanently* more difficult to understand & analyze. I don't
immediately see this proposal as a good solution for avoiding the
current kind of platform DSC breakage. Platform CI would be much better,
in my opinion. The proposal does seem workable, but the implementation
needs to be surgical, IMO.
OK, I'll get off my soap box now.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: 回复: [edk2-devel] A proposal to reduce incompatible case
2020-11-20 11:17 ` Laszlo Ersek
@ 2020-11-21 6:07 ` Zhiguang Liu
2020-11-29 12:31 ` Zhiguang Liu
0 siblings, 1 reply; 11+ messages in thread
From: Zhiguang Liu @ 2020-11-21 6:07 UTC (permalink / raw)
To: Laszlo Ersek, Ard Biesheuvel, gaoliming, devel@edk2.groups.io,
Yao, Jiewen, Feng, Bob C, Tian, Hot, 'Bret Barkelew'
Cc: Bi, Dandan, 'Chao Zhang', Wang, Jian J, Wu, Hao A,
'Liming Gao', Justen, Jordan L, 'Andrew Fish',
Ni, Ray, 'Bret Barkelew', Kinney, Michael D,
debtech@gmail.com, awarkentin@vmware.com,
michael.kubacki@outlook.com
Hi all,
Thanks all for the comments.
Hi Jiewen:
I think we are thinking from the different aspects.
I want the XXPkgLib.dsc.inc to specify the default library instance that the package will consumes.
You may want it to specify the default library instance that the package will produce.
I now think your way makes more sense, and also align with the implement in NetworkPkg.
I will follow your way when working on the demo code.
Hi Bret:
I see you point about that platform should be like platform and should only change in the stable tag.
I partly agree with you, but in fact there are some projects need to keep the Edk2 updated frequently.
And my proposal should also be an optional way to add the library instance.
Platform dsc can also choose the original way. I think it is at least harmless if some platforms choose not to use this way.
Hi Lazlo:
I agree with you that this proposal should only extract "Single-instance".
After all, this proposal is meant to reduce incompatible case like this VaribalePolicyLib.
I want to the platform to be "Seamless update" in such case.
However, if this lib has two instances, it is a platform's choice and platform owner should be aware the change.
Therefore, "Single-instance" and "Multiple-instance" should be totally different case, and we should only enable this proposal to "Single-instance".
Hi Liming and Ard,
Thanks for liking this idea.
I plan to work on the demo code next week and send it here for more comments.
BTW, about the file name, I want to it to follow the existing rule of NetworkPkg and ArmVirtPkg.
I think for MdeModulePkg, "MdeModuleLibs.dsc.inc" will be better.
Thanks
Zhiguang
> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Friday, November 20, 2020 7:18 PM
> To: Ard Biesheuvel <ard.biesheuvel@arm.com>; gaoliming
> <gaoliming@byosoft.com.cn>; devel@edk2.groups.io; Yao, Jiewen
> <jiewen.yao@intel.com>; Liu, Zhiguang <zhiguang.liu@intel.com>;
> michael.kubacki@outlook.com; awarkentin@vmware.com;
> debtech@gmail.com; Feng, Bob C <bob.c.feng@intel.com>; Tian, Hot
> <hot.tian@intel.com>
> Cc: 'Bret Barkelew' <bret@corthon.com>; Bi, Dandan <dandan.bi@intel.com>;
> 'Chao Zhang' <chao.b.zhang@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Wu, Hao A <hao.a.wu@intel.com>; 'Liming Gao' <liming.gao@intel.com>;
> Justen, Jordan L <jordan.l.justen@intel.com>; 'Andrew Fish' <afish@apple.com>;
> Ni, Ray <ray.ni@intel.com>; 'Bret Barkelew' <brbarkel@microsoft.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: 回复: [edk2-devel] A proposal to reduce incompatible case
>
> On 11/20/20 10:02, Ard Biesheuvel wrote:
> > On 11/20/20 8:27 AM, gaoliming wrote:
> >> Zhiguang:
> >> This proposal can reduce the potential library class dependency.
> >> Each package has its xxxPkgLib.dsc.inc file that includes the library
> >> instances from this package.
> >> Platform DSC can include the required package lib.dsc.inc file for
> >> the library instances. Can you work out the code changes to
> >> demonstrate this idea?
> >>
> >
> > +1 for this idea. This would allow us to remove a *lot* of boilerplate
> > in the .DSC files, and focus on the libraries that actually matter for
> > the platform at hand.
>
> I feel like I'm in *cautious* agreement with this idea.
>
> In particular, I'd *only* like those lib class -> lib instance resolutions to be
> extracted, to DSC include files, that *cannot* involve platform choice. To put it
> differently, single-instance lib classes.
>
> ("Single-instance" can be interpreted per module type / per architecture of
> course -- so if a lib class has exactly 1 instance for PEIMs and exactly 1
> (different) instance for DXE_DRIVERs, then those resolutions qualify for
> extraction.)
>
> If a library class genuinely has multiple instances in core edk2, then extracting
> a "default" resolution would be *wrong*, in my opinion. Every platform needs
> to make the choice explicitly, in such cases. This applies even if a lib class only
> has two instances, a functional one, and a Null one. The functional one should
> not be assumed default.
>
> Example: extracting the UefiLib resolution is valid. Extracting a SerialPortLib
> class resolution is invalid.
>
> Basically, I'd like to avoid *overrides*. Overrides are terribly hard to reason
> about.
>
> ... If someone wants to work on this, they'll have to implement this with *super
> small* patches, where every patch extracts resolutions for just one lib class. I
> would reject any patch that required me to review the extraction of two or
> more lib classes, from an OVMF or ArmVirt DSC file, at the same time.
>
> There is big potential in this proposal for making platform DSC files
> *permanently* more difficult to understand & analyze. I don't immediately see
> this proposal as a good solution for avoiding the current kind of platform DSC
> breakage. Platform CI would be much better, in my opinion. The proposal does
> seem workable, but the implementation needs to be surgical, IMO.
>
> OK, I'll get off my soap box now.
>
> Thanks
> Laszlo
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: 回复: [edk2-devel] A proposal to reduce incompatible case
2020-11-21 6:07 ` Zhiguang Liu
@ 2020-11-29 12:31 ` Zhiguang Liu
2020-11-30 16:48 ` Laszlo Ersek
2020-11-30 20:53 ` Michael D Kinney
0 siblings, 2 replies; 11+ messages in thread
From: Zhiguang Liu @ 2020-11-29 12:31 UTC (permalink / raw)
To: Laszlo Ersek, Ard Biesheuvel, gaoliming, devel@edk2.groups.io,
Yao, Jiewen, Feng, Bob C, Tian, Hot, 'Bret Barkelew'
Cc: Bi, Dandan, 'Chao Zhang', Wang, Jian J, Wu, Hao A,
'Liming Gao', Justen, Jordan L, 'Andrew Fish',
Ni, Ray, 'Bret Barkelew', Kinney, Michael D,
debtech@gmail.com, awarkentin@vmware.com,
michael.kubacki@outlook.com
[-- Attachment #1.1: Type: text/plain, Size: 8226 bytes --]
Hi all,
I write a python script to do the work, including classify all the library instance, generating the including lib file.
In the attachment file, the first sheet includes all "Single-instance", which means in edk2 and edk2-platforms repo, only one inf files has the specified library name.
The second sheet includes all "Multi-Single-instance", which means in edk2 and edk2-platforms repo, for a specified module type and arch, this inf is a "Single-instance".
I think the library instance in the first two sheet can be extracted as a XXXLibs.dsc.inc
Here is the draft code
https://github.com/LiuZhiguang001/edk2/commits/extract_lib
Here is the source code of this tool
https://github.com/LiuZhiguang001/MyTool/tree/extract_lib
Please review the excel and the draft code.
If no comments, I will send out a formal patch next week.
Thanks
Zhiguang
> -----Original Message-----
> From: Liu, Zhiguang
> Sent: Saturday, November 21, 2020 2:08 PM
> To: Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel
> <ard.biesheuvel@arm.com>; gaoliming <gaoliming@byosoft.com.cn>;
> devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Feng, Bob C
> <bob.c.feng@intel.com>; Tian, Hot <hot.tian@intel.com>; 'Bret Barkelew'
> <bret@corthon.com>
> Cc: Bi, Dandan <dandan.bi@intel.com>; 'Chao Zhang'
> <chao.b.zhang@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> <hao.a.wu@intel.com>; 'Liming Gao' <liming.gao@intel.com>; Justen, Jordan L
> <jordan.l.justen@intel.com>; 'Andrew Fish' <afish@apple.com>; Ni, Ray
> <ray.ni@intel.com>; 'Bret Barkelew' <brbarkel@microsoft.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; debtech@gmail.com;
> awarkentin@vmware.com; michael.kubacki@outlook.com
> Subject: RE: 回复: [edk2-devel] A proposal to reduce incompatible case
>
> Hi all,
> Thanks all for the comments.
>
> Hi Jiewen:
> I think we are thinking from the different aspects.
> I want the XXPkgLib.dsc.inc to specify the default library instance that the
> package will consumes.
> You may want it to specify the default library instance that the package will
> produce.
> I now think your way makes more sense, and also align with the implement in
> NetworkPkg.
> I will follow your way when working on the demo code.
>
> Hi Bret:
> I see you point about that platform should be like platform and should only
> change in the stable tag.
> I partly agree with you, but in fact there are some projects need to keep the
> Edk2 updated frequently.
> And my proposal should also be an optional way to add the library instance.
> Platform dsc can also choose the original way. I think it is at least harmless if
> some platforms choose not to use this way.
>
> Hi Lazlo:
> I agree with you that this proposal should only extract "Single-instance".
> After all, this proposal is meant to reduce incompatible case like this
> VaribalePolicyLib.
> I want to the platform to be "Seamless update" in such case.
> However, if this lib has two instances, it is a platform's choice and platform
> owner should be aware the change.
> Therefore, "Single-instance" and "Multiple-instance" should be totally different
> case, and we should only enable this proposal to "Single-instance".
>
> Hi Liming and Ard,
> Thanks for liking this idea.
> I plan to work on the demo code next week and send it here for more
> comments.
>
> BTW, about the file name, I want to it to follow the existing rule of NetworkPkg
> and ArmVirtPkg.
> I think for MdeModulePkg, "MdeModuleLibs.dsc.inc" will be better.
>
> Thanks
> Zhiguang
>
> > -----Original Message-----
> > From: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> > Sent: Friday, November 20, 2020 7:18 PM
> > To: Ard Biesheuvel <ard.biesheuvel@arm.com<mailto:ard.biesheuvel@arm.com>>; gaoliming
> > <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Yao, Jiewen
> > <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Liu, Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>;
> > michael.kubacki@outlook.com<mailto:michael.kubacki@outlook.com>; awarkentin@vmware.com<mailto:awarkentin@vmware.com>;
> debtech@gmail.com<mailto:debtech@gmail.com>;
> > Feng, Bob C <bob.c.feng@intel.com<mailto:bob.c.feng@intel.com>>; Tian, Hot <hot.tian@intel.com<mailto:hot.tian@intel.com>>
> > Cc: 'Bret Barkelew' <bret@corthon.com<mailto:bret@corthon.com>>; Bi, Dandan
> > <dandan.bi@intel.com<mailto:dandan.bi@intel.com>>; 'Chao Zhang' <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>; Wang,
> > Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>;
> > 'Liming Gao' <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Justen, Jordan L
> > <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; 'Andrew Fish' <afish@apple.com<mailto:afish@apple.com>>; Ni, Ray
> > <ray.ni@intel.com<mailto:ray.ni@intel.com>>; 'Bret Barkelew' <brbarkel@microsoft.com<mailto:brbarkel@microsoft.com>>; Kinney,
> > Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
> > Subject: Re: 回复: [edk2-devel] A proposal to reduce incompatible case
> >
> > On 11/20/20 10:02, Ard Biesheuvel wrote:
> > > On 11/20/20 8:27 AM, gaoliming wrote:
> > >> Zhiguang:
> > >> This proposal can reduce the potential library class dependency.
> > >> Each package has its xxxPkgLib.dsc.inc file that includes the
> > >> library instances from this package.
> > >> Platform DSC can include the required package lib.dsc.inc file
> > >> for the library instances. Can you work out the code changes to
> > >> demonstrate this idea?
> > >>
> > >
> > > +1 for this idea. This would allow us to remove a *lot* of
> > > +boilerplate
> > > in the .DSC files, and focus on the libraries that actually matter
> > > for the platform at hand.
> >
> > I feel like I'm in *cautious* agreement with this idea.
> >
> > In particular, I'd *only* like those lib class -> lib instance
> > resolutions to be extracted, to DSC include files, that *cannot*
> > involve platform choice. To put it differently, single-instance lib classes.
> >
> > ("Single-instance" can be interpreted per module type / per
> > architecture of course -- so if a lib class has exactly 1 instance for
> > PEIMs and exactly 1
> > (different) instance for DXE_DRIVERs, then those resolutions qualify
> > for
> > extraction.)
> >
> > If a library class genuinely has multiple instances in core edk2, then
> > extracting a "default" resolution would be *wrong*, in my opinion.
> > Every platform needs to make the choice explicitly, in such cases.
> > This applies even if a lib class only has two instances, a functional
> > one, and a Null one. The functional one should not be assumed default.
> >
> > Example: extracting the UefiLib resolution is valid. Extracting a
> > SerialPortLib class resolution is invalid.
> >
> > Basically, I'd like to avoid *overrides*. Overrides are terribly hard
> > to reason about.
> >
> > ... If someone wants to work on this, they'll have to implement this
> > with *super
> > small* patches, where every patch extracts resolutions for just one
> > lib class. I would reject any patch that required me to review the
> > extraction of two or more lib classes, from an OVMF or ArmVirt DSC file, at
> the same time.
> >
> > There is big potential in this proposal for making platform DSC files
> > *permanently* more difficult to understand & analyze. I don't
> > immediately see this proposal as a good solution for avoiding the
> > current kind of platform DSC breakage. Platform CI would be much
> > better, in my opinion. The proposal does seem workable, but the
> implementation needs to be surgical, IMO.
> >
> > OK, I'll get off my soap box now.
> >
> > Thanks
> > Laszlo
[-- Attachment #1.2: Type: text/html, Size: 17396 bytes --]
[-- Attachment #2: Edk2 Library All.xlsx --]
[-- Type: application/vnd.openxmlformats-officedocument.spreadsheetml.sheet, Size: 37609 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: 回复: [edk2-devel] A proposal to reduce incompatible case
2020-11-29 12:31 ` Zhiguang Liu
@ 2020-11-30 16:48 ` Laszlo Ersek
2020-11-30 20:53 ` Michael D Kinney
1 sibling, 0 replies; 11+ messages in thread
From: Laszlo Ersek @ 2020-11-30 16:48 UTC (permalink / raw)
To: Liu, Zhiguang, Ard Biesheuvel, gaoliming, devel@edk2.groups.io,
Yao, Jiewen, Feng, Bob C, Tian, Hot, 'Bret Barkelew'
Cc: Bi, Dandan, 'Chao Zhang', Wang, Jian J, Wu, Hao A,
'Liming Gao', Justen, Jordan L, 'Andrew Fish',
Ni, Ray, 'Bret Barkelew', Kinney, Michael D,
debtech@gmail.com, awarkentin@vmware.com,
michael.kubacki@outlook.com
On 11/29/20 13:31, Liu, Zhiguang wrote:
> Hi all,
>
>
>
> I write a python script to do the work, including classify all the library instance, generating the including lib file.
>
> In the attachment file, the first sheet includes all "Single-instance", which means in edk2 and edk2-platforms repo, only one inf files has the specified library name.
> The second sheet includes all "Multi-Single-instance", which means in edk2 and edk2-platforms repo, for a specified module type and arch, this inf is a "Single-instance".
>
> I think the library instance in the first two sheet can be extracted as a XXXLibs.dsc.inc
>
>
>
> Here is the draft code
>
> https://github.com/LiuZhiguang001/edk2/commits/extract_lib
>
>
>
> Here is the source code of this tool
>
> https://github.com/LiuZhiguang001/MyTool/tree/extract_lib
>
>
>
> Please review the excel and the draft code.
>
> If no comments, I will send out a formal patch next week.
I've briefly checked the first two sheets of the spreadsheet, regarding
OvmfPkg and ArmVirtPkg. Those parts look sensible to me.
Regarding the code, namely the patch "OvmfPkg:Add OvmfLibs.dsc.inc" -- I
really dislike even briefly checking patches that are not on the list,
as we then get into a back-and-forth about them, and then it would just
make more sense to post the patch to the list, and discuss it there.
But, in this case, I did take a quick look.
I don't like the approach. First, the new DSC include file is apparently
not put to use in the OvmfPkg DSC files at all. IIUC, the DSC include
files should not only come as additions, but they should replace
existent lib class resolutions where they can. Second (and I stated this
before), given a new DSC include file, I'd like to see each affected
library class resolution to be migrated *one by one* from the DSC
file(s) to the new DSC include file. If you have 10 affected lib classes
for OVMF, that will take 10 patches for OVMF. I will have to validate
every lib class resolution movement one by one anyway, in isolation (I
won't just blindly trust the python tool that generates these new files,
sorry), and for that kind of review, I need the patches to be broken-out
accordingly.
If this creates too many patches in the series (for example the first
sheet in the spreadsheet has 144 rows), then I suggest writing a
separate series for each package, or something similar.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: 回复: [edk2-devel] A proposal to reduce incompatible case
2020-11-29 12:31 ` Zhiguang Liu
2020-11-30 16:48 ` Laszlo Ersek
@ 2020-11-30 20:53 ` Michael D Kinney
2020-12-01 1:54 ` Zhiguang Liu
1 sibling, 1 reply; 11+ messages in thread
From: Michael D Kinney @ 2020-11-30 20:53 UTC (permalink / raw)
To: Liu, Zhiguang, Laszlo Ersek, Ard Biesheuvel, gaoliming,
devel@edk2.groups.io, Yao, Jiewen, Feng, Bob C, Tian, Hot,
'Bret Barkelew', Kinney, Michael D
Cc: Bi, Dandan, 'Chao Zhang', Wang, Jian J, Wu, Hao A,
'Liming Gao', Justen, Jordan L, 'Andrew Fish',
Ni, Ray, 'Bret Barkelew', debtech@gmail.com,
awarkentin@vmware.com, michael.kubacki@outlook.com
[-- Attachment #1: Type: text/plain, Size: 11449 bytes --]
Hi Zhiguang,
I do not think generating these lists of lib mapping only from evaluation of the edk2 and edk2-platforms DSC usage is correct.
The only way to know for sure that there is a default mapping is if the design and implementation of the library class intended only a single mapping.
This would have to be documented in the lib class and the lib instance.
An example where this will have the incorrect behavior is a platform specific library class where a only a template of that lib class is provided
as a *Null instance. The expectation is that a platform that needs this feature would be required to provide their own instance of that
library class in their platform DSC. There may be features that are not used by the platforms in edk2-platforms. In that case your
tool would identify the *Null instance as the only one. All platforms would build, but would not have the expected behavior. It
would be better for platforms to break at build time with a missing library mapping in this case.
There are a few categories of libraries
1. *Null library instances that are a template for a platform specific lib. Also used for package verification builds.
2. Library class where different library instances are expected to be used in a single platform.
3. Library class with a single lib instance and all platforms are expected to use that one library instance.
I like the idea that packages providing a DSC INC file with default mappings/settings, but we need to make sure it is always correct
and should only be applied to category (3). This may require manual analysis in each package or extra information added to
lib class/lib instance.
Thanks,
Mike
From: Liu, Zhiguang <zhiguang.liu@intel.com>
Sent: Sunday, November 29, 2020 4:32 AM
To: Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@arm.com>; gaoliming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Feng, Bob C <bob.c.feng@intel.com>; Tian, Hot <hot.tian@intel.com>; 'Bret Barkelew' <bret@corthon.com>
Cc: Bi, Dandan <dandan.bi@intel.com>; 'Chao Zhang' <chao.b.zhang@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; 'Liming Gao' <liming.gao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; 'Andrew Fish' <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; 'Bret Barkelew' <brbarkel@microsoft.com>; Kinney, Michael D <michael.d.kinney@intel.com>; debtech@gmail.com; awarkentin@vmware.com; michael.kubacki@outlook.com
Subject: RE: 回复: [edk2-devel] A proposal to reduce incompatible case
Hi all,
I write a python script to do the work, including classify all the library instance, generating the including lib file.
In the attachment file, the first sheet includes all "Single-instance", which means in edk2 and edk2-platforms repo, only one inf files has the specified library name.
The second sheet includes all "Multi-Single-instance", which means in edk2 and edk2-platforms repo, for a specified module type and arch, this inf is a "Single-instance".
I think the library instance in the first two sheet can be extracted as a XXXLibs.dsc.inc
Here is the draft code
https://github.com/LiuZhiguang001/edk2/commits/extract_lib
Here is the source code of this tool
https://github.com/LiuZhiguang001/MyTool/tree/extract_lib
Please review the excel and the draft code.
If no comments, I will send out a formal patch next week.
Thanks
Zhiguang
> -----Original Message-----
> From: Liu, Zhiguang
> Sent: Saturday, November 21, 2020 2:08 PM
> To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Ard Biesheuvel
> <ard.biesheuvel@arm.com<mailto:ard.biesheuvel@arm.com>>; gaoliming <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>;
> devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Feng, Bob C
> <bob.c.feng@intel.com<mailto:bob.c.feng@intel.com>>; Tian, Hot <hot.tian@intel.com<mailto:hot.tian@intel.com>>; 'Bret Barkelew'
> <bret@corthon.com<mailto:bret@corthon.com>>
> Cc: Bi, Dandan <dandan.bi@intel.com<mailto:dandan.bi@intel.com>>; 'Chao Zhang'
> <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
> <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; 'Liming Gao' <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Justen, Jordan L
> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; 'Andrew Fish' <afish@apple.com<mailto:afish@apple.com>>; Ni, Ray
> <ray.ni@intel.com<mailto:ray.ni@intel.com>>; 'Bret Barkelew' <brbarkel@microsoft.com<mailto:brbarkel@microsoft.com>>; Kinney,
> Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; debtech@gmail.com<mailto:debtech@gmail.com>;
> awarkentin@vmware.com<mailto:awarkentin@vmware.com>; michael.kubacki@outlook.com<mailto:michael.kubacki@outlook.com>
> Subject: RE: 回复: [edk2-devel] A proposal to reduce incompatible case
>
> Hi all,
> Thanks all for the comments.
>
> Hi Jiewen:
> I think we are thinking from the different aspects.
> I want the XXPkgLib.dsc.inc to specify the default library instance that the
> package will consumes.
> You may want it to specify the default library instance that the package will
> produce.
> I now think your way makes more sense, and also align with the implement in
> NetworkPkg.
> I will follow your way when working on the demo code.
>
> Hi Bret:
> I see you point about that platform should be like platform and should only
> change in the stable tag.
> I partly agree with you, but in fact there are some projects need to keep the
> Edk2 updated frequently.
> And my proposal should also be an optional way to add the library instance.
> Platform dsc can also choose the original way. I think it is at least harmless if
> some platforms choose not to use this way.
>
> Hi Lazlo:
> I agree with you that this proposal should only extract "Single-instance".
> After all, this proposal is meant to reduce incompatible case like this
> VaribalePolicyLib.
> I want to the platform to be "Seamless update" in such case.
> However, if this lib has two instances, it is a platform's choice and platform
> owner should be aware the change.
> Therefore, "Single-instance" and "Multiple-instance" should be totally different
> case, and we should only enable this proposal to "Single-instance".
>
> Hi Liming and Ard,
> Thanks for liking this idea.
> I plan to work on the demo code next week and send it here for more
> comments.
>
> BTW, about the file name, I want to it to follow the existing rule of NetworkPkg
> and ArmVirtPkg.
> I think for MdeModulePkg, "MdeModuleLibs.dsc.inc" will be better.
>
> Thanks
> Zhiguang
>
> > -----Original Message-----
> > From: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> > Sent: Friday, November 20, 2020 7:18 PM
> > To: Ard Biesheuvel <ard.biesheuvel@arm.com<mailto:ard.biesheuvel@arm.com>>; gaoliming
> > <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Yao, Jiewen
> > <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Liu, Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>;
> > michael.kubacki@outlook.com<mailto:michael.kubacki@outlook.com>; awarkentin@vmware.com<mailto:awarkentin@vmware.com>;
> debtech@gmail.com<mailto:debtech@gmail.com>;
> > Feng, Bob C <bob.c.feng@intel.com<mailto:bob.c.feng@intel.com>>; Tian, Hot <hot.tian@intel.com<mailto:hot.tian@intel.com>>
> > Cc: 'Bret Barkelew' <bret@corthon.com<mailto:bret@corthon.com>>; Bi, Dandan
> > <dandan.bi@intel.com<mailto:dandan.bi@intel.com>>; 'Chao Zhang' <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>; Wang,
> > Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>;
> > 'Liming Gao' <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Justen, Jordan L
> > <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; 'Andrew Fish' <afish@apple.com<mailto:afish@apple.com>>; Ni, Ray
> > <ray.ni@intel.com<mailto:ray.ni@intel.com>>; 'Bret Barkelew' <brbarkel@microsoft.com<mailto:brbarkel@microsoft.com>>; Kinney,
> > Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
> > Subject: Re: 回复: [edk2-devel] A proposal to reduce incompatible case
> >
> > On 11/20/20 10:02, Ard Biesheuvel wrote:
> > > On 11/20/20 8:27 AM, gaoliming wrote:
> > >> Zhiguang:
> > >> This proposal can reduce the potential library class dependency.
> > >> Each package has its xxxPkgLib.dsc.inc file that includes the
> > >> library instances from this package.
> > >> Platform DSC can include the required package lib.dsc.inc file
> > >> for the library instances. Can you work out the code changes to
> > >> demonstrate this idea?
> > >>
> > >
> > > +1 for this idea. This would allow us to remove a *lot* of
> > > +boilerplate
> > > in the .DSC files, and focus on the libraries that actually matter
> > > for the platform at hand.
> >
> > I feel like I'm in *cautious* agreement with this idea.
> >
> > In particular, I'd *only* like those lib class -> lib instance
> > resolutions to be extracted, to DSC include files, that *cannot*
> > involve platform choice. To put it differently, single-instance lib classes.
> >
> > ("Single-instance" can be interpreted per module type / per
> > architecture of course -- so if a lib class has exactly 1 instance for
> > PEIMs and exactly 1
> > (different) instance for DXE_DRIVERs, then those resolutions qualify
> > for
> > extraction.)
> >
> > If a library class genuinely has multiple instances in core edk2, then
> > extracting a "default" resolution would be *wrong*, in my opinion.
> > Every platform needs to make the choice explicitly, in such cases.
> > This applies even if a lib class only has two instances, a functional
> > one, and a Null one. The functional one should not be assumed default.
> >
> > Example: extracting the UefiLib resolution is valid. Extracting a
> > SerialPortLib class resolution is invalid.
> >
> > Basically, I'd like to avoid *overrides*. Overrides are terribly hard
> > to reason about.
> >
> > ... If someone wants to work on this, they'll have to implement this
> > with *super
> > small* patches, where every patch extracts resolutions for just one
> > lib class. I would reject any patch that required me to review the
> > extraction of two or more lib classes, from an OVMF or ArmVirt DSC file, at
> the same time.
> >
> > There is big potential in this proposal for making platform DSC files
> > *permanently* more difficult to understand & analyze. I don't
> > immediately see this proposal as a good solution for avoiding the
> > current kind of platform DSC breakage. Platform CI would be much
> > better, in my opinion. The proposal does seem workable, but the
> implementation needs to be surgical, IMO.
> >
> > OK, I'll get off my soap box now.
> >
> > Thanks
> > Laszlo
[-- Attachment #2: Type: text/html, Size: 67231 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: 回复: [edk2-devel] A proposal to reduce incompatible case
2020-11-30 20:53 ` Michael D Kinney
@ 2020-12-01 1:54 ` Zhiguang Liu
0 siblings, 0 replies; 11+ messages in thread
From: Zhiguang Liu @ 2020-12-01 1:54 UTC (permalink / raw)
To: Kinney, Michael D, Laszlo Ersek, Ard Biesheuvel, gaoliming,
devel@edk2.groups.io, Yao, Jiewen, Feng, Bob C, Tian, Hot,
'Bret Barkelew'
Cc: Bi, Dandan, 'Chao Zhang', Wang, Jian J, Wu, Hao A,
'Liming Gao', Justen, Jordan L, 'Andrew Fish',
Ni, Ray, 'Bret Barkelew', debtech@gmail.com,
awarkentin@vmware.com, michael.kubacki@outlook.com
[-- Attachment #1: Type: text/plain, Size: 13444 bytes --]
Hi Mike and Laszlo,
Thanks for the comments.
I was trying to use the python tool to auto generate the patches for all packages, and this did lack manual verification.
I may mis-classify the library instances and may cause potential misuse of the library instances.
Thanks for your advice and I plan to send the patch for each package one by one and take time to manually check.
Thanks
Zhiguang
From: Kinney, Michael D <michael.d.kinney@intel.com>
Sent: Tuesday, December 1, 2020 4:53 AM
To: Liu, Zhiguang <zhiguang.liu@intel.com>; Laszlo Ersek <lersek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@arm.com>; gaoliming <gaoliming@byosoft.com.cn>; devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Feng, Bob C <bob.c.feng@intel.com>; Tian, Hot <hot.tian@intel.com>; 'Bret Barkelew' <bret@corthon.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Bi, Dandan <dandan.bi@intel.com>; 'Chao Zhang' <chao.b.zhang@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; 'Liming Gao' <liming.gao@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; 'Andrew Fish' <afish@apple.com>; Ni, Ray <ray.ni@intel.com>; 'Bret Barkelew' <brbarkel@microsoft.com>; debtech@gmail.com; awarkentin@vmware.com; michael.kubacki@outlook.com
Subject: RE: 回复: [edk2-devel] A proposal to reduce incompatible case
Hi Zhiguang,
I do not think generating these lists of lib mapping only from evaluation of the edk2 and edk2-platforms DSC usage is correct.
The only way to know for sure that there is a default mapping is if the design and implementation of the library class intended only a single mapping.
This would have to be documented in the lib class and the lib instance.
An example where this will have the incorrect behavior is a platform specific library class where a only a template of that lib class is provided
as a *Null instance. The expectation is that a platform that needs this feature would be required to provide their own instance of that
library class in their platform DSC. There may be features that are not used by the platforms in edk2-platforms. In that case your
tool would identify the *Null instance as the only one. All platforms would build, but would not have the expected behavior. It
would be better for platforms to break at build time with a missing library mapping in this case.
There are a few categories of libraries
1. *Null library instances that are a template for a platform specific lib. Also used for package verification builds.
2. Library class where different library instances are expected to be used in a single platform.
3. Library class with a single lib instance and all platforms are expected to use that one library instance.
I like the idea that packages providing a DSC INC file with default mappings/settings, but we need to make sure it is always correct
and should only be applied to category (3). This may require manual analysis in each package or extra information added to
lib class/lib instance.
Thanks,
Mike
From: Liu, Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>
Sent: Sunday, November 29, 2020 4:32 AM
To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Ard Biesheuvel <ard.biesheuvel@arm.com<mailto:ard.biesheuvel@arm.com>>; gaoliming <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Feng, Bob C <bob.c.feng@intel.com<mailto:bob.c.feng@intel.com>>; Tian, Hot <hot.tian@intel.com<mailto:hot.tian@intel.com>>; 'Bret Barkelew' <bret@corthon.com<mailto:bret@corthon.com>>
Cc: Bi, Dandan <dandan.bi@intel.com<mailto:dandan.bi@intel.com>>; 'Chao Zhang' <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; 'Liming Gao' <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; 'Andrew Fish' <afish@apple.com<mailto:afish@apple.com>>; Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; 'Bret Barkelew' <brbarkel@microsoft.com<mailto:brbarkel@microsoft.com>>; Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; debtech@gmail.com<mailto:debtech@gmail.com>; awarkentin@vmware.com<mailto:awarkentin@vmware.com>; michael.kubacki@outlook.com<mailto:michael.kubacki@outlook.com>
Subject: RE: 回复: [edk2-devel] A proposal to reduce incompatible case
Hi all,
I write a python script to do the work, including classify all the library instance, generating the including lib file.
In the attachment file, the first sheet includes all "Single-instance", which means in edk2 and edk2-platforms repo, only one inf files has the specified library name.
The second sheet includes all "Multi-Single-instance", which means in edk2 and edk2-platforms repo, for a specified module type and arch, this inf is a "Single-instance".
I think the library instance in the first two sheet can be extracted as a XXXLibs.dsc.inc
Here is the draft code
https://github.com/LiuZhiguang001/edk2/commits/extract_lib
Here is the source code of this tool
https://github.com/LiuZhiguang001/MyTool/tree/extract_lib
Please review the excel and the draft code.
If no comments, I will send out a formal patch next week.
Thanks
Zhiguang
> -----Original Message-----
> From: Liu, Zhiguang
> Sent: Saturday, November 21, 2020 2:08 PM
> To: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>; Ard Biesheuvel
> <ard.biesheuvel@arm.com<mailto:ard.biesheuvel@arm.com>>; gaoliming <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>;
> devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Feng, Bob C
> <bob.c.feng@intel.com<mailto:bob.c.feng@intel.com>>; Tian, Hot <hot.tian@intel.com<mailto:hot.tian@intel.com>>; 'Bret Barkelew'
> <bret@corthon.com<mailto:bret@corthon.com>>
> Cc: Bi, Dandan <dandan.bi@intel.com<mailto:dandan.bi@intel.com>>; 'Chao Zhang'
> <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>; Wang, Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A
> <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>; 'Liming Gao' <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Justen, Jordan L
> <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; 'Andrew Fish' <afish@apple.com<mailto:afish@apple.com>>; Ni, Ray
> <ray.ni@intel.com<mailto:ray.ni@intel.com>>; 'Bret Barkelew' <brbarkel@microsoft.com<mailto:brbarkel@microsoft.com>>; Kinney,
> Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; debtech@gmail.com<mailto:debtech@gmail.com>;
> awarkentin@vmware.com<mailto:awarkentin@vmware.com>; michael.kubacki@outlook.com<mailto:michael.kubacki@outlook.com>
> Subject: RE: 回复: [edk2-devel] A proposal to reduce incompatible case
>
> Hi all,
> Thanks all for the comments.
>
> Hi Jiewen:
> I think we are thinking from the different aspects.
> I want the XXPkgLib.dsc.inc to specify the default library instance that the
> package will consumes.
> You may want it to specify the default library instance that the package will
> produce.
> I now think your way makes more sense, and also align with the implement in
> NetworkPkg.
> I will follow your way when working on the demo code.
>
> Hi Bret:
> I see you point about that platform should be like platform and should only
> change in the stable tag.
> I partly agree with you, but in fact there are some projects need to keep the
> Edk2 updated frequently.
> And my proposal should also be an optional way to add the library instance.
> Platform dsc can also choose the original way. I think it is at least harmless if
> some platforms choose not to use this way.
>
> Hi Lazlo:
> I agree with you that this proposal should only extract "Single-instance".
> After all, this proposal is meant to reduce incompatible case like this
> VaribalePolicyLib.
> I want to the platform to be "Seamless update" in such case.
> However, if this lib has two instances, it is a platform's choice and platform
> owner should be aware the change.
> Therefore, "Single-instance" and "Multiple-instance" should be totally different
> case, and we should only enable this proposal to "Single-instance".
>
> Hi Liming and Ard,
> Thanks for liking this idea.
> I plan to work on the demo code next week and send it here for more
> comments.
>
> BTW, about the file name, I want to it to follow the existing rule of NetworkPkg
> and ArmVirtPkg.
> I think for MdeModulePkg, "MdeModuleLibs.dsc.inc" will be better.
>
> Thanks
> Zhiguang
>
> > -----Original Message-----
> > From: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> > Sent: Friday, November 20, 2020 7:18 PM
> > To: Ard Biesheuvel <ard.biesheuvel@arm.com<mailto:ard.biesheuvel@arm.com>>; gaoliming
> > <gaoliming@byosoft.com.cn<mailto:gaoliming@byosoft.com.cn>>; devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Yao, Jiewen
> > <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>; Liu, Zhiguang <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>;
> > michael.kubacki@outlook.com<mailto:michael.kubacki@outlook.com>; awarkentin@vmware.com<mailto:awarkentin@vmware.com>;
> debtech@gmail.com<mailto:debtech@gmail.com>;
> > Feng, Bob C <bob.c.feng@intel.com<mailto:bob.c.feng@intel.com>>; Tian, Hot <hot.tian@intel.com<mailto:hot.tian@intel.com>>
> > Cc: 'Bret Barkelew' <bret@corthon.com<mailto:bret@corthon.com>>; Bi, Dandan
> > <dandan.bi@intel.com<mailto:dandan.bi@intel.com>>; 'Chao Zhang' <chao.b.zhang@intel.com<mailto:chao.b.zhang@intel.com>>; Wang,
> > Jian J <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>>; Wu, Hao A <hao.a.wu@intel.com<mailto:hao.a.wu@intel.com>>;
> > 'Liming Gao' <liming.gao@intel.com<mailto:liming.gao@intel.com>>; Justen, Jordan L
> > <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; 'Andrew Fish' <afish@apple.com<mailto:afish@apple.com>>; Ni, Ray
> > <ray.ni@intel.com<mailto:ray.ni@intel.com>>; 'Bret Barkelew' <brbarkel@microsoft.com<mailto:brbarkel@microsoft.com>>; Kinney,
> > Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>
> > Subject: Re: 回复: [edk2-devel] A proposal to reduce incompatible case
> >
> > On 11/20/20 10:02, Ard Biesheuvel wrote:
> > > On 11/20/20 8:27 AM, gaoliming wrote:
> > >> Zhiguang:
> > >> This proposal can reduce the potential library class dependency.
> > >> Each package has its xxxPkgLib.dsc.inc file that includes the
> > >> library instances from this package.
> > >> Platform DSC can include the required package lib.dsc.inc file
> > >> for the library instances. Can you work out the code changes to
> > >> demonstrate this idea?
> > >>
> > >
> > > +1 for this idea. This would allow us to remove a *lot* of
> > > +boilerplate
> > > in the .DSC files, and focus on the libraries that actually matter
> > > for the platform at hand.
> >
> > I feel like I'm in *cautious* agreement with this idea.
> >
> > In particular, I'd *only* like those lib class -> lib instance
> > resolutions to be extracted, to DSC include files, that *cannot*
> > involve platform choice. To put it differently, single-instance lib classes.
> >
> > ("Single-instance" can be interpreted per module type / per
> > architecture of course -- so if a lib class has exactly 1 instance for
> > PEIMs and exactly 1
> > (different) instance for DXE_DRIVERs, then those resolutions qualify
> > for
> > extraction.)
> >
> > If a library class genuinely has multiple instances in core edk2, then
> > extracting a "default" resolution would be *wrong*, in my opinion.
> > Every platform needs to make the choice explicitly, in such cases.
> > This applies even if a lib class only has two instances, a functional
> > one, and a Null one. The functional one should not be assumed default.
> >
> > Example: extracting the UefiLib resolution is valid. Extracting a
> > SerialPortLib class resolution is invalid.
> >
> > Basically, I'd like to avoid *overrides*. Overrides are terribly hard
> > to reason about.
> >
> > ... If someone wants to work on this, they'll have to implement this
> > with *super
> > small* patches, where every patch extracts resolutions for just one
> > lib class. I would reject any patch that required me to review the
> > extraction of two or more lib classes, from an OVMF or ArmVirt DSC file, at
> the same time.
> >
> > There is big potential in this proposal for making platform DSC files
> > *permanently* more difficult to understand & analyze. I don't
> > immediately see this proposal as a good solution for avoiding the
> > current kind of platform DSC breakage. Platform CI would be much
> > better, in my opinion. The proposal does seem workable, but the
> implementation needs to be surgical, IMO.
> >
> > OK, I'll get off my soap box now.
> >
> > Thanks
> > Laszlo
[-- Attachment #2: Type: text/html, Size: 29511 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-12-01 1:54 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-20 6:51 A proposal to reduce incompatible case Zhiguang Liu
2020-11-20 7:20 ` Yao, Jiewen
2020-11-20 7:24 ` Bret Barkelew
2020-11-20 7:27 ` 回复: [edk2-devel] " gaoliming
2020-11-20 9:02 ` Ard Biesheuvel
2020-11-20 11:17 ` Laszlo Ersek
2020-11-21 6:07 ` Zhiguang Liu
2020-11-29 12:31 ` Zhiguang Liu
2020-11-30 16:48 ` Laszlo Ersek
2020-11-30 20:53 ` Michael D Kinney
2020-12-01 1:54 ` Zhiguang Liu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox