Re: A proposal to reduce incompatible case


Yao, Jiewen
 

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@...>
Sent: Friday, November 20, 2020 2:52 PM
To: devel@edk2.groups.io; michael.kubacki@...;
awarkentin@...; Ard Biesheuvel <ard.biesheuvel@...>;
debtech@...; Feng, Bob C <bob.c.feng@...>; Tian, Hot
<hot.tian@...>
Cc: Bret Barkelew <bret@...>; Yao, Jiewen
<jiewen.yao@...>; Bi, Dandan <dandan.bi@...>; Chao Zhang
<chao.b.zhang@...>; Wang, Jian J <jian.j.wang@...>; Wu, Hao
A <hao.a.wu@...>; Liming Gao <liming.gao@...>; Justen,
Jordan L <jordan.l.justen@...>; Laszlo Ersek <lersek@...>;
Andrew Fish <afish@...>; Ni, Ray <ray.ni@...>; Bret Barkelew
<brbarkel@...>; Kinney, Michael D
<michael.d.kinney@...>; Liming Gao <gaoliming@...>
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@...; Ard Biesheuvel
<ard.biesheuvel@...>; debtech@...
Cc: Bret Barkelew <bret@...>; Yao, Jiewen
<jiewen.yao@...>;
Bi, Dandan <dandan.bi@...>; Chao Zhang
<chao.b.zhang@...>;
Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>;
Liming Gao <liming.gao@...>; Justen, Jordan L
<jordan.l.justen@...>; Laszlo Ersek <lersek@...>; Andrew
Fish
<afish@...>; Ni, Ray <ray.ni@...>; Bret Barkelew
<brbarkel@...>
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@...>
*Sent:* Thursday, November 19, 2020 10:15 AM
*To:* Ard Biesheuvel <ard.biesheuvel@...>
*Cc:* Bret Barkelew <bret@...>; devel@edk2.groups.io
<devel@edk2.groups.io>; Jiewen Yao <jiewen.yao@...>; Dandan
Bi
<dandan.bi@...>; Chao Zhang <chao.b.zhang@...>; Jian J
Wang <jian.j.wang@...>; Hao A Wu <hao.a.wu@...>;
Liming
Gao <liming.gao@...>; Jordan Justen <jordan.l.justen@...>;
Laszlo Ersek <lersek@...>; Andrew Fish <afish@...>; Ray
Ni <ray.ni@...>; Bret Barkelew <brbarkel@...>
*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@...>
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&amp;sdata=LLKZ7qeffR0WCvLbY
uH
tQI
uwJGhXY0mVqB2w9B0q180%3D&amp;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&amp;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&amp;sdata=LLKZ7qeffR0WCvLbYuHtQIuwJG
hX
Y0mVqB2
w9B0q180%3D&amp;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&amp;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&amp;sdata=GYY52rlsPxw07vfdu%2BVbWhzRjtHWXlIGve
CTT
17mlfc%3
D&amp;reserved=0
<https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk
2.groups.io%2Fg%2Fdevel%2Fmessage%2F53712&amp;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&amp;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@...>
Cc: Dandan Bi <dandan.bi@...>
Cc: Chao Zhang <chao.b.zhang@...>
Cc: Jian J Wang <jian.j.wang@...>
Cc: Hao A Wu <hao.a.wu@...>
Cc: Liming Gao <liming.gao@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Laszlo Ersek <lersek@...>
Cc: Ard Biesheuvel <ard.biesheuvel@...>
Cc: Andrew Fish <afish@...>
Cc: Ray Ni <ray.ni@...>
Cc: Bret Barkelew <brbarkel@...>
Signed-off-by: Bret Barkelew <brbarkel@...>
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






Join devel@edk2.groups.io to automatically receive all group messages.