Topics

回复: [edk2-devel] A proposal to reduce incompatible case


gaoliming
 

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@...>; devel@edk2.groups.io;
@michael.kubacki; awarkentin@...; Ard Biesheuvel
<ard.biesheuvel@...>; debtech@...; Feng, Bob C
<bob.c.feng@...>; Tian, Hot <@Hot>
抄送: Bret Barkelew <bret@...>; 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@...>
主题: 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@...>
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>
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=gmail.com@groups.io>
*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%7C594f15b45aaf476bf
f
7e
08d
88cb57390%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637414
0
5
82471
28819%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2l
u
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%7C6374140582471
2
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%7C
a
wa
rke
ntin%40vmware.com%7C594f15b45aaf476bff7e08d88cb57390%7Cb39138ca
3c
ee4
b4aa4d6cd83d9dd62f0%7C0%7C0%7C637414058247133820%7CUnknown%
7
CT
WFpbGZ
sb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6M
n0
%3D%7C1000&amp;sdata=GYY52rlsPxw07vfdu%2BVbWhzRjtHWXlIG
ve
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%7Cb39138ca3c
e
e
4b4aa4
d6cd83d9dd62f0%7C0%7C0%7C637414058247133820%7CUnknown%7CTW
F
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/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










Ard Biesheuvel
 

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@...>; devel@edk2.groups.io;
@michael.kubacki; awarkentin@...; Ard Biesheuvel
<ard.biesheuvel@...>; debtech@...; Feng, Bob C
<bob.c.feng@...>; Tian, Hot <@Hot>
抄送: Bret Barkelew <bret@...>; 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@...>
主题: 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@...>
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>
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=gmail.com@groups.io>
*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%7C594f15b45aaf476bf
f
7e
08d
88cb57390%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637414
0
5
82471
28819%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2l
u
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%7C6374140582471
2
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%7C
a
wa
rke
ntin%40vmware.com%7C594f15b45aaf476bff7e08d88cb57390%7Cb39138ca
3c
ee4
b4aa4d6cd83d9dd62f0%7C0%7C0%7C637414058247133820%7CUnknown%
7
CT
WFpbGZ
sb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6M
n0
%3D%7C1000&amp;sdata=GYY52rlsPxw07vfdu%2BVbWhzRjtHWXlIG
ve
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%7Cb39138ca3c
e
e
4b4aa4
d6cd83d9dd62f0%7C0%7C0%7C637414058247133820%7CUnknown%7CTW
F
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/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









Laszlo Ersek
 

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


Zhiguang Liu
 

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@...>
Sent: Friday, November 20, 2020 7:18 PM
To: Ard Biesheuvel <ard.biesheuvel@...>; gaoliming
<gaoliming@...>; devel@edk2.groups.io; Yao, Jiewen
<jiewen.yao@...>; Liu, Zhiguang <zhiguang.liu@...>;
@michael.kubacki; awarkentin@...;
debtech@...; Feng, Bob C <bob.c.feng@...>; Tian, Hot
<@Hot>
Cc: 'Bret Barkelew' <bret@...>; 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@...>; 'Andrew Fish' <afish@...>;
Ni, Ray <ray.ni@...>; 'Bret Barkelew' <brbarkel@...>;
Kinney, Michael D <michael.d.kinney@...>
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


Zhiguang Liu
 

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@...>; Ard Biesheuvel

> <ard.biesheuvel@...>; gaoliming <gaoliming@...>;

> devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@...>; Feng, Bob C

> <bob.c.feng@...>; Tian, Hot <hot.tian@...>; 'Bret Barkelew'

> <bret@...>

> Cc: 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@...>; 'Andrew Fish' <afish@...>; Ni, Ray

> <ray.ni@...>; 'Bret Barkelew' <brbarkel@...>; Kinney,

> Michael D <michael.d.kinney@...>; debtech@...;

> awarkentin@...; michael.kubacki@...

> 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@...>

> > Sent: Friday, November 20, 2020 7:18 PM

> > To: Ard Biesheuvel <ard.biesheuvel@...>; gaoliming

> > <gaoliming@...>; devel@edk2.groups.io; Yao, Jiewen

> > <jiewen.yao@...>; Liu, Zhiguang <zhiguang.liu@...>;

> > michael.kubacki@...; awarkentin@...;

> debtech@...;

> > Feng, Bob C <bob.c.feng@...>; Tian, Hot <hot.tian@...>

> > Cc: 'Bret Barkelew' <bret@...>; 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@...>; 'Andrew Fish' <afish@...>; Ni, Ray

> > <ray.ni@...>; 'Bret Barkelew' <brbarkel@...>; Kinney,

> > Michael D <michael.d.kinney@...>

> > 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