[edk2-devel] A proposal to reduce incompatible case

Zhiguang Liu zhiguang.liu at intel.com
Fri Nov 20 06:51:42 UTC 2020 

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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Michael
> Kubacki
> Sent: Friday, November 20, 2020 4:16 AM
> To: devel at edk2.groups.io; awarkentin at vmware.com; Ard Biesheuvel
> <ard.biesheuvel at arm.com>; debtech at gmail.com
> Cc: Bret Barkelew <bret at corthon.com>; Yao, Jiewen <jiewen.yao at intel.com>;
> Bi, Dandan <dandan.bi at intel.com>; Chao Zhang <chao.b.zhang at intel.com>;
> Wang, Jian J <jian.j.wang at intel.com>; Wu, Hao A <hao.a.wu at intel.com>;
> Liming Gao <liming.gao at intel.com>; Justen, Jordan L
> <jordan.l.justen at intel.com>; Laszlo Ersek <lersek at redhat.com>; Andrew Fish
> <afish at apple.com>; Ni, Ray <ray.ni at intel.com>; Bret Barkelew
> <brbarkel at 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 at edk2.groups.io <devel at edk2.groups.io> on behalf of Bret
> > Barkelew via groups.io <debtech=gmail.com at groups.io>
> > *Sent:* Thursday, November 19, 2020 10:15 AM
> > *To:* Ard Biesheuvel <ard.biesheuvel at arm.com>
> > *Cc:* Bret Barkelew <bret at corthon.com>; devel at edk2.groups.io
> > <devel at edk2.groups.io>; Jiewen Yao <jiewen.yao at intel.com>; Dandan Bi
> > <dandan.bi at intel.com>; Chao Zhang <chao.b.zhang at intel.com>; Jian J
> > Wang <jian.j.wang at intel.com>; Hao A Wu <hao.a.wu at intel.com>; Liming
> > Gao <liming.gao at intel.com>; Jordan Justen <jordan.l.justen at intel.com>;
> > Laszlo Ersek <lersek at redhat.com>; Andrew Fish <afish at apple.com>; Ray
> > Ni <ray.ni at intel.com>; Bret Barkelew <brbarkel at 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 at 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&amp
> ;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 at intel.com>
> >>> Cc: Dandan Bi <dandan.bi at intel.com>
> >>> Cc: Chao Zhang <chao.b.zhang at intel.com>
> >>> Cc: Jian J Wang <jian.j.wang at intel.com>
> >>> Cc: Hao A Wu <hao.a.wu at intel.com>
> >>> Cc: Liming Gao <liming.gao at intel.com>
> >>> Cc: Jordan Justen <jordan.l.justen at intel.com>
> >>> Cc: Laszlo Ersek <lersek at redhat.com>
> >>> Cc: Ard Biesheuvel <ard.biesheuvel at arm.com>
> >>> Cc: Andrew Fish <afish at apple.com>
> >>> Cc: Ray Ni <ray.ni at intel.com>
> >>> Cc: Bret Barkelew <brbarkel at microsoft.com>
> >>> Signed-off-by: Bret Barkelew <brbarkel at 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
> >>
> >
> >
> >
> >
> >
> >
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#67749): https://edk2.groups.io/g/devel/message/67749
Mute This Topic: https://groups.io/mt/78384142/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-

More information about the edk2-devel-archive mailing list