Date   

Re: [PATCH 15/22] .mailmap: add entry for Wei6 Xu

Philippe Mathieu-Daudé
 

Hi Laszlo,

On 9/8/20 3:50 AM, Xu, Wei6 wrote:
Hi Laszlo,

Thanks a lot for helping solve this issue.

Reviewed-by: Wei6 Xu <wei6.xu@intel.com>

BR,
Wei
Wei signs as 'Wei', so maybe we should simply use:

Wei Xu <wei6.xu@intel.com>


-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Tuesday, September 8, 2020 3:31 AM
To: edk2-devel-groups-io <devel@edk2.groups.io>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>; Xu, Wei6 <wei6.xu@intel.com>
Subject: [PATCH 15/22] .mailmap: add entry for Wei6 Xu

... for git-shortlog purposes.

Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Wei6 Xu <wei6.xu@intel.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
.mailmap | 1 +
1 file changed, 1 insertion(+)

diff --git a/.mailmap b/.mailmap
index 0c731e74b811..1bdee892fa0c 100644
--- a/.mailmap
+++ b/.mailmap
@@ -82,6 +82,7 @@ Steven Shi <steven.shi@intel.com> Tom Lendacky <thomas.lendacky@amd.com> Vitaly Cheptsov <vit9696@protonmail.com> Vitaly Cheptsov via Groups.Io <vit9696=protonmail.com@groups.io>
Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com> Vladimir Olovyannikov via edk2-devel <edk2-devel@lists.01.org>
+Wei6 Xu <wei6.xu@intel.com>
XiaoyuX Lu <xiaoyux.lu@intel.com>
Yonghong Zhu <yonghong.zhu@intel.com>
Yonghong Zhu <yonghong.zhu@intel.com> <yzhu52@Edk2>
--
2.19.1.3.g30247aa5d201





Re: [PATCH 14/22] .mailmap: add entry for Jiaxin Wu

Philippe Mathieu-Daudé
 

On 9/7/20 9:30 PM, Laszlo Ersek wrote:
... for git-shortlog purposes.

Cc: Jiaxin Wu <jiaxin.wu@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
.mailmap | 1 +
1 file changed, 1 insertion(+)

diff --git a/.mailmap b/.mailmap
index 05887373154a..0c731e74b811 100644
--- a/.mailmap
+++ b/.mailmap
@@ -35,6 +35,7 @@ Hao A Wu <hao.a.wu@intel.com>
Hao A Wu <hao.a.wu@intel.com> <hwu1225@Edk2>
Hot Tian <hot.tian@intel.com>
Hot Tian <hot.tian@intel.com> <hhtian@6f19259b-4bc3-4df7-8a09-765794883524>
+Jiaxin Wu <jiaxin.wu@intel.com>
Also:

Jiaxin Wu <jiaxin.wu@intel.com> <jiaxinwu@Edk2>

Jiewen Yao <jiewen.yao@intel.com>
Jiewen Yao <jiewen.yao@intel.com> <Jiewen.yao@intel.com>
Jiewen Yao <jiewen.yao@intel.com> <Jiewen.Yao@intel.com>


Re: [PATCH 13/22] .mailmap: add entry for Steven Shi

Philippe Mathieu-Daudé
 

On 9/7/20 9:30 PM, Laszlo Ersek wrote:
... for git-shortlog purposes.

Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Steven Shi <steven.shi@intel.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
.mailmap | 1 +
1 file changed, 1 insertion(+)

diff --git a/.mailmap b/.mailmap
index 407fe4559b4c..05887373154a 100644
--- a/.mailmap
+++ b/.mailmap
@@ -77,6 +77,7 @@ Shenglei Zhang <shenglei.zhang@intel.com>
Star Zeng <star.zeng@intel.com>
Star Zeng <star.zeng@intel.com> <lzeng14@6f19259b-4bc3-4df7-8a09-765794883524>
Star Zeng <star.zeng@intel.com> <lzeng14@Edk2>
+Steven Shi <steven.shi@intel.com>
Tom Lendacky <thomas.lendacky@amd.com>
Vitaly Cheptsov <vit9696@protonmail.com> Vitaly Cheptsov via Groups.Io <vit9696=protonmail.com@groups.io>
Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com> Vladimir Olovyannikov via edk2-devel <edk2-devel@lists.01.org>
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>


Re: [PATCH 12/22] .mailmap: add entry for XiaoyuX Lu

Philippe Mathieu-Daudé
 

On 9/7/20 9:30 PM, Laszlo Ersek wrote:
... for git-shortlog purposes.

Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: XiaoyuX Lu <xiaoyux.lu@intel.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
.mailmap | 1 +
1 file changed, 1 insertion(+)

diff --git a/.mailmap b/.mailmap
index 3866642d4dbe..407fe4559b4c 100644
--- a/.mailmap
+++ b/.mailmap
@@ -80,6 +80,7 @@ Star Zeng <star.zeng@intel.com> <lzeng14@Edk2>
Tom Lendacky <thomas.lendacky@amd.com>
Vitaly Cheptsov <vit9696@protonmail.com> Vitaly Cheptsov via Groups.Io <vit9696=protonmail.com@groups.io>
Vladimir Olovyannikov <vladimir.olovyannikov@broadcom.com> Vladimir Olovyannikov via edk2-devel <edk2-devel@lists.01.org>
+XiaoyuX Lu <xiaoyux.lu@intel.com>
I have:

Xiaoyu Lu <xiaoyux.lu@intel.com>

Yonghong Zhu <yonghong.zhu@intel.com>
Yonghong Zhu <yonghong.zhu@intel.com> <yzhu52@Edk2>
Yu-Chen Lin <yuchenlin@synology.com>


Re: [PATCH 10/22] .mailmap: add entry for Derek Lin

Philippe Mathieu-Daudé
 

On 9/7/20 9:30 PM, Laszlo Ersek wrote:
... for git-shortlog purposes.

Cc: Derek Lin <derek.lin2@hpe.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
.mailmap | 1 +
1 file changed, 1 insertion(+)

diff --git a/.mailmap b/.mailmap
index 0d2be4255ff8..33ecbffa5191 100644
--- a/.mailmap
+++ b/.mailmap
@@ -18,6 +18,7 @@ Baraneedharan Anbazhagan <anbazhagan@hp.com>
Chasel Chiu <chasel.chiu@intel.com>
Ching JenX Cheng <ching.jenx.cheng@intel.com>
Christopher J Zurcher <christopher.j.zurcher@intel.com>
+Derek Lin <derek.lin2@hpe.com>
Eric Dong <eric.dong@intel.com>
Eric Dong <eric.dong@intel.com> Eric Dong <eirc.dong@intel.com>
Eric Dong <eric.dong@intel.com> <ydong10@6f19259b-4bc3-4df7-8a09-765794883524>
FWIW:
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>


Re: [PATCH 08/22] .mailmap: add entry for Michael D Kinney

Philippe Mathieu-Daudé
 

On 9/7/20 9:30 PM, Laszlo Ersek wrote:
... for git-shortlog purposes.

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
.mailmap | 1 +
1 file changed, 1 insertion(+)

diff --git a/.mailmap b/.mailmap
index 47608acbf931..2e23a0f43814 100644
--- a/.mailmap
+++ b/.mailmap
@@ -57,6 +57,7 @@ Marvin Häuser <Marvin.Haeuser@outlook.com>
Marvin Häuser <Marvin.Haeuser@outlook.com> edk2-devel <edk2-devel-bounces@lists.01.org>
Marvin Häuser <mhaeuser@outlook.de>
Maurice Ma <maurice.ma@intel.com>
+Michael D Kinney <michael.d.kinney@intel.com>
Also:

Michael Kinney <michael.d.kinney@intel.com>
<mdkinney@6f19259b-4bc3-4df7-8a09-765794883524>
Michael Kinney <michael.d.kinney@intel.com> <mdkinney@Edk2>
Michael Kinney <michael.d.kinney@intel.com>
<mdkinney@mdkinney-desk.amr.corp.intel.com>
Michael Kinney <michael.d.kinney@intel.com> <Michael.d.kinney@intel.com>
Michael Kinney <michael.d.kinney@intel.com>
</o=Intel/ou=Americas01/cn=Workers/cn=Kinney, Michael D>

Michael Kubacki <michael.a.kubacki@intel.com>
Michael Kubacki <michael.a.kubacki@intel.com> </o=Intel/ou=External (FYDIBOHF25SPDLT)/cn=Recipients/cn=3c8b0226e75f4ab08d20c151cb7a8a72>
Ming Tan <ming.tan@intel.com>


Re: [PATCH 07/22] .mailmap: add entry for Eric Jin

Philippe Mathieu-Daudé
 

On 9/7/20 9:30 PM, Laszlo Ersek wrote:
... for git-shortlog purposes.

Cc: Eric Jin <eric.jin@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
.mailmap | 1 +
1 file changed, 1 insertion(+)

diff --git a/.mailmap b/.mailmap
index df05a29ed3bf..47608acbf931 100644
--- a/.mailmap
+++ b/.mailmap
@@ -22,6 +22,7 @@ Eric Dong <eric.dong@intel.com>
Eric Dong <eric.dong@intel.com> Eric Dong <eirc.dong@intel.com>
Eric Dong <eric.dong@intel.com> <ydong10@6f19259b-4bc3-4df7-8a09-765794883524>
Eric Dong <eric.dong@intel.com> <ydong10@Edk2>
+Eric Jin <eric.jin@intel.com>
Also:

Eric Jin <eric.jin@intel.com> <jjin9@6f19259b-4bc3-4df7-8a09-765794883524>

Erik Bjorge <erik.c.bjorge@intel.com> <geekboy15a@6f19259b-4bc3-4df7-8a09-765794883524>
Eugene Cohen <eugene@nuviainc.com>
Eugene Cohen <eugene@nuviainc.com> <eugene@hp.com>


Re: [PATCH 04/22] .mailmap: add entries for Guo Dong

Philippe Mathieu-Daudé
 

On 9/7/20 9:30 PM, Laszlo Ersek wrote:
... for git-shortlog purposes.

Cc: Guo Dong <guo.dong@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
.mailmap | 2 ++
1 file changed, 2 insertions(+)

diff --git a/.mailmap b/.mailmap
index 38cfec35c70e..8ed1d77c9220 100644
--- a/.mailmap
+++ b/.mailmap
@@ -25,6 +25,8 @@ Eric Dong <eric.dong@intel.com> <ydong10@Edk2>
Erik Bjorge <erik.c.bjorge@intel.com> <geekboy15a@6f19259b-4bc3-4df7-8a09-765794883524>
Eugene Cohen <eugene@nuviainc.com>
Eugene Cohen <eugene@nuviainc.com> <eugene@hp.com>
+Guo Dong <guo.dong@intel.com>
+Guo Dong <guo.dong@intel.com> </O=Intel/OU=Pacifica02/cn=Recipients/cn=gdong1>
Also:

Guo Dong <guo.dong@intel.com> <gdong1>
Guo Dong <guo.dong@intel.com> <gdong1@6f19259b-4bc3-4df7-8a09-765794883524>

Hao A Wu <hao.a.wu@intel.com>
Hao A Wu <hao.a.wu@intel.com> <hwu1225@Edk2>
Hot Tian <hot.tian@intel.com>


Re: [PATCH 03/22] .mailmap: add entries for Maggie Chu

Philippe Mathieu-Daudé
 

On 9/7/20 9:30 PM, Laszlo Ersek wrote:
... for git-shortlog purposes.

Cc: Maggie Chu <maggie.chu@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
.mailmap | 2 ++
1 file changed, 2 insertions(+)

diff --git a/.mailmap b/.mailmap
index c2eeee2834e2..38cfec35c70e 100644
--- a/.mailmap
+++ b/.mailmap
@@ -44,6 +44,8 @@ Liming Gao <liming.gao@intel.com> <lgao4@6f19259b-4bc3-4df7-8a09-765794883524>
Liming Gao <liming.gao@intel.com> <lgao4@Edk2>
Liming Gao <liming.gao@intel.com> <liming.gao@intel.com>
Maciej Rabeda <maciej.rabeda@intel.com>
+Maggie Chu <maggie.chu@intel.com>
+Maggie Chu <maggie.chu@intel.com> </o=Intel/ou=External (FYDIBOHF25SPDLT)/cn=Recipients/cn=fe425ca7e5f4401abed22b904fe5d964>
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Marc-André Lureau <marcandre.lureau@redhat.com> <marcandre.lureau@redhat.com>
Marc W Chen <marc.w.chen@intel.com>
Marvin Häuser <Marvin.Haeuser@outlook.com>


Re: [PATCH 02/22] .mailmap: add entry for Ching JenX Cheng

Philippe Mathieu-Daudé
 

On 9/7/20 9:30 PM, Laszlo Ersek wrote:
... for git-shortlog purposes.

Cc: Ching JenX Cheng <ching.jenx.cheng@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
.mailmap | 1 +
1 file changed, 1 insertion(+)

diff --git a/.mailmap b/.mailmap
index f173754d7ee8..c2eeee2834e2 100644
--- a/.mailmap
+++ b/.mailmap
@@ -16,6 +16,7 @@ Ard Biesheuvel <ard.biesheuvel@linaro.org> <abiesheuvel@Edk2>
Ashley DeSimone <ashley.e.desimone@intel.com> <ashdesimone@6f19259b-4bc3-4df7-8a09-765794883524>
Baraneedharan Anbazhagan <anbazhagan@hp.com>
Chasel Chiu <chasel.chiu@intel.com>
+Ching JenX Cheng <ching.jenx.cheng@intel.com>
In my mailmap I have:

Cheng Ching Jen <ching.jenx.cheng@intel.com>

Christopher J Zurcher <christopher.j.zurcher@intel.com>
Eric Dong <eric.dong@intel.com>
Eric Dong <eric.dong@intel.com> Eric Dong <eirc.dong@intel.com>


Re: [PATCH 01/22] .mailmap: add entry for Marc W Chen

Philippe Mathieu-Daudé
 

On 9/7/20 9:30 PM, Laszlo Ersek wrote:
... for git-shortlog purposes.

Cc: Marc W Chen <marc.w.chen@intel.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
.mailmap | 1 +
1 file changed, 1 insertion(+)

diff --git a/.mailmap b/.mailmap
index ba246ff6cd80..f173754d7ee8 100644
--- a/.mailmap
+++ b/.mailmap
@@ -44,6 +44,7 @@ Liming Gao <liming.gao@intel.com> <lgao4@Edk2>
Liming Gao <liming.gao@intel.com> <liming.gao@intel.com>
Maciej Rabeda <maciej.rabeda@intel.com>
Marc-André Lureau <marcandre.lureau@redhat.com> <marcandre.lureau@redhat.com>
+Marc W Chen <marc.w.chen@intel.com>
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Marvin Häuser <Marvin.Haeuser@outlook.com>
Marvin Häuser <Marvin.Haeuser@outlook.com> edk2-devel <edk2-devel-bounces@lists.01.org>
Marvin Häuser <mhaeuser@outlook.de>


Re: [PATCH 1/1] OvmfPkg: drop redundant VendorID check in VirtioMmioDeviceLib

Leif Lindholm
 

On Mon, Sep 14, 2020 at 17:35:13 +0200, Laszlo Ersek wrote:
Hi Leif,

On 09/12/20 18:40, Leif Lindholm wrote:
There is a DEBUG warning printout in VirtioMmioDeviceLib if the current
device's VendorID does not match the traditional 16-bit Red Hat PCIe
vendor ID used with virtio-pci. The virtio-mmio vendor ID is 32-bit and
has no connection to the PCIe registry.

Most specifically, this causes a bunch of noise when booting an AArch64
QEMU platform, since QEMU's virtio-mmio implementation used 'QEMU' as
the vendor ID:
VirtioMmioInit: Warning:
The VendorId (0x554D4551) does not match the VirtIo VendorId (0x1AF4).
Good catch -- in QEMU, this has been the case since initial virtio-mmio
commit 4b52530be987 ("virtio: Implement MMIO based virtio transport",
2013-07-19); and indeed neither the virtio-0.9.5 spec, nor the "Legacy
MMIO" part of the 1.0 spec, require this vendor ID to be 0x1AF4.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks!
Merged as 5648836987ca / #935.

Thanks
Laszlo


Drop the warning message.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
Signed-off-by: Leif Lindholm <leif@nuviainc.com>
---
.../VirtioMmioDeviceLib/VirtioMmioDevice.c | 15 ---------------
1 file changed, 15 deletions(-)

diff --git a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c
index 2f20272c1445..6dbbba008c75 100644
--- a/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c
+++ b/OvmfPkg/Library/VirtioMmioDeviceLib/VirtioMmioDevice.c
@@ -58,7 +58,6 @@ VirtioMmioInit (
)
{
UINT32 MagicValue;
- UINT32 VendorId;
UINT32 Version;

//
@@ -84,20 +83,6 @@ VirtioMmioInit (
return EFI_UNSUPPORTED;
}

- //
- // Double-check MMIO-specific values
- //
- VendorId = VIRTIO_CFG_READ (Device, VIRTIO_MMIO_OFFSET_VENDOR_ID);
- if (VendorId != VIRTIO_VENDOR_ID) {
- //
- // The ARM Base and Foundation Models do not report a valid VirtIo VendorId.
- // They return a value of 0x0 for the VendorId.
- //
- DEBUG((DEBUG_WARN, "VirtioMmioInit: Warning: The VendorId (0x%X) does not "
- "match the VirtIo VendorId (0x%X).\n",
- VendorId, VIRTIO_VENDOR_ID));
- }
-
return EFI_SUCCESS;
}


Re: more development process failure [was: UefiPayloadPkg: Runtime MMCONF]

Laszlo Ersek
 

On 09/17/20 09:31, Yao, Jiewen wrote:
Laszlo
I like your description to compare the process with the programming language and software design. We have to clean up the resource.

Please allow me to point out, this is the exactly the issue we are having today.

1) Take buffer overflow as an example. It has been there for 30 years. We have enough books and papers talking about it. Today, people are still making mistakes. Why? Because C language is not type safe, there is NO enforcement.
That is why many people like other type safe language. If you are trying to make mistake, the compiler will tell you that you are making mistake.
This will sound more convincing once we have Rust (or something similar)
in a mainstream OS kernel or mainstream firmware.

2) Take resource leak as an example. The programming language invented garbage collection. The operating system auto cleaned up application resource after execution.
Same comment as above. I think garbage collection is frequently
considered too opaque for low-level applications (unpredictable
performance and RAM penalties etc).

3) People has wrong check in which may break the system. What is why the world invented smoke test and continuous integration.
Yes, I agree.

*Those are where the inventions come*, to treat human as human being, to prevent people making mistake or teach them automatically. :-)
Thanks, I'm grateful that you use the word "teach" here. I'll reference
it below.

I agree with the "mythical man-month" that there is no silver bulletin.
I tend to agree with you on the attitude.
I am trying to figure if we can do better to help other people (maintainer or contributor). If we really really can do nothing, that is OK.
I am not sure if is a best way to resolve the problem to just complain in the email.
"Just complaining in email" is in fact my attempt to "teach" people. Not
automatically, but by pointing out examples that I consider good.

Automatisms are already in place for broadcasting good practices.
Bugzilla actions and github actions are propagated via email. People
just need to be receptive and look at the list traffic.

I've contributed to Wiki articles. But asking for more documentation and
more automatisms is just too convenient an excuse. People can and
*should* learn by example, especially if they're seasoned in a project
(not newcomers). Asking for more documentation and more automatisms puts
the burden on people *different* from those that need to improve their
discipline.

It basically means, "I refuse to improve my behavior until *you* find
the time to implement the tools and documentations for me to improve my
behavior". Similarly, "I refuse to handle errors until you give me
exceptions and destructors".

I don't think it's fair to *demand* inventions. Because, I perceive the
goals that I'm advocating for as widely-held values. If we accept them
as such, then the burden should be on people that break those values,
not on the advocates.

(If, on the other hand, we do not have a consensus that these values are
universal, that's OK as well: then I can start ignoring the information
content in the PRs that *I* submit, and save myself significant amounts
of time. See again: double standards.)

I think you can understand my point. I will stop here.
Yes, I understand. My general response is that inventions are nice and
they should be utilized if someone comes forward and offers them. On the
other hand, demanding inventions (tooling, documentation etc), i.e.,
demanding that the *other* party spend the effort first, as a condition
for changing our attitude, is not fair.

Laszlo


Re: Open Design Meeting to discuss storage/configuration purge design

Wang, Sunny (HPS SW)
 

Thanks, Ray.

 

Hi All,

If you’re working on option cards’ UEFI driver (especially storage card UEFI driver), welcome to join the meeting and provide your valuable thoughts.

 

Regards,

Sunny Wang

 

From: Ni, Ray <ray.ni@...>
Sent: Thursday, September 17, 2020 5:25 PM
To: devel@edk2.groups.io
Cc: Wang, Sunny (HPS SW) <sunnywang@...>; Rothman, Michael A <michael.a.rothman@...>; Bi, Dandan <dandan.bi@...>; Wu, Hao A <hao.a.wu@...>; Dong, Eric <eric.dong@...>
Subject: Open Design Meeting to discuss storage/configuration purge design

 

All,

There will be one topic in this week’s open design meeting discussing about storage/configuration purge design. (In case you think the meeting is cancelled as usually.)

Sunny will present and I invited Rothman Michael to help review the proposal. Michael is the expert of storage and HII.

 

Dandan, Hao and Eric, since you are the HII/Storage module maintainers. That would be great if you could join to provide the valuable thoughts.

 

NOTE:

There is one minor change: we need to use Webex for the meeting. I suggest you download Webex (www.webex.com/download) and try running it before the meeting.

 

Thanks,

Ray


Open Design Meeting to discuss storage/configuration purge design

Ni, Ray
 

All,

There will be one topic in this week’s open design meeting discussing about storage/configuration purge design. (In case you think the meeting is cancelled as usually.)

Sunny will present and I invited Rothman Michael to help review the proposal. Michael is the expert of storage and HII.

 

Dandan, Hao and Eric, since you are the HII/Storage module maintainers. That would be great if you could join to provide the valuable thoughts.

 

NOTE:

There is one minor change: we need to use Webex for the meeting. I suggest you download Webex (www.webex.com/download) and try running it before the meeting.

 

Thanks,

Ray


Re: [PATCH v2] EmulatorPkg: Enable support for Secure Boot

Ni, Ray
 

Reviewed-by: Ray Ni <ray.ni@...>

 

From: Wadhawan, Divneil R <divneil.r.wadhawan@...>
Sent: Thursday, September 17, 2020 3:43 PM
To: Ni, Ray <ray.ni@...>; devel@edk2.groups.io
Cc: gaoliming <gaoliming@...>; 'Andrew Fish' <afish@...>; Justen, Jordan L <jordan.l.justen@...>; Kinney, Michael D <michael.d.kinney@...>; Wadhawan, Divneil R <divneil.r.wadhawan@...>
Subject: RE: [edk2-devel] [PATCH v2] EmulatorPkg: Enable support for Secure Boot

 

Hi Ray,

 

Yes, I have tested the following:

 

  1. SECURE_BOOT_ENABLE=true
  • Key Enrollment (PK, KEK, db) via custom mode
  • Execution of unit test shell application (signed one works okay, unsigned gives an Access denied)

 

  1. SECURE_BOOT_ENABLE=false (default case)
  • Secure Boot Configuration menu is not visible (Same as existing default case)
  • Execution of Unit Test Application (Signed/Unsigned both works okay)

 

I am planning to post the script in BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2949 in a day or too.

The script generates the full key hierarchy that makes it easy to test this patch.

The patch in BZ requires modifications as per Mike’s comment, so, you can skip the patches in BZ for now.

 

Regards,

Divneil

 

From: Ni, Ray <ray.ni@...>
Sent: Thursday, September 17, 2020 12:49 PM
To: Wadhawan, Divneil R <divneil.r.wadhawan@...>; devel@edk2.groups.io
Cc: gaoliming <gaoliming@...>; 'Andrew Fish' <afish@...>; Justen, Jordan L <jordan.l.justen@...>; Kinney, Michael D <michael.d.kinney@...>
Subject: RE: [edk2-devel] [PATCH v2] EmulatorPkg: Enable support for Secure Boot

 

Divneil,

Just want to double confirm: did you test the secure boot and non-secure boot?

 

Thanks,

Ray

 

From: Wadhawan, Divneil R <divneil.r.wadhawan@...>
Sent: Wednesday, September 16, 2020 11:53 PM
To: devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@...>; gaoliming <gaoliming@...>; 'Andrew Fish' <afish@...>; Justen, Jordan L <jordan.l.justen@...>; Kinney, Michael D <michael.d.kinney@...>; Wadhawan, Divneil R <divneil.r.wadhawan@...>
Subject: [edk2-devel] [PATCH v2] EmulatorPkg: Enable support for Secure Boot

 

SECURE_BOOT_ENABLE feature flag is introduced to enable Secure Boot.

The following gets enabled with this patch:

o Secure Boot Menu in "Device Manager" for enrolling keys

o Storage space for Authenticated Variables

o Authenticated execution of 3rd party images

 

Signed-off-by: Divneil Rai Wadhawan <divneil.r.wadhawan@...>

---

EmulatorPkg/EmulatorPkg.dsc | 37 +++++++++++++++++++++++++++++++++++--

EmulatorPkg/EmulatorPkg.fdf | 14 ++++++++++++++

2 files changed, 49 insertions(+), 2 deletions(-)

 

diff --git a/EmulatorPkg/EmulatorPkg.dsc b/EmulatorPkg/EmulatorPkg.dsc

index 86a6271735..c6e25c745e 100644

--- a/EmulatorPkg/EmulatorPkg.dsc

+++ b/EmulatorPkg/EmulatorPkg.dsc

@@ -32,6 +32,7 @@

   DEFINE NETWORK_TLS_ENABLE       = FALSE

   DEFINE NETWORK_HTTP_BOOT_ENABLE = FALSE

   DEFINE NETWORK_ISCSI_ENABLE     = FALSE

+  DEFINE SECURE_BOOT_ENABLE       = FALSE

 

 [SkuIds]

   0|DEFAULT

@@ -106,12 +107,20 @@

   LockBoxLib|MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.inf

   CpuExceptionHandlerLib|MdeModulePkg/Library/CpuExceptionHandlerLibNull/CpuExceptionHandlerLibNull.inf

   TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf

-  AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf

   VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf

   SortLib|MdeModulePkg/Library/BaseSortLib/BaseSortLib.inf

   ShellLib|ShellPkg/Library/UefiShellLib/UefiShellLib.inf

   FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf

 

+!if $(SECURE_BOOT_ENABLE) == TRUE

+  IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf

+  OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf

+  PlatformSecureLib|SecurityPkg/Library/PlatformSecureLibNull/PlatformSecureLibNull.inf

+  AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf

+!else

+  AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf

+!endif

+

[LibraryClasses.common.SEC]

   PeiServicesLib|EmulatorPkg/Library/SecPeiServicesLib/SecPeiServicesLib.inf

   PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf

@@ -162,6 +171,16 @@

   TimerLib|EmulatorPkg/Library/DxeCoreTimerLib/DxeCoreTimerLib.inf

  EmuThunkLib|EmulatorPkg/Library/DxeEmuLib/DxeEmuLib.inf

 

+[LibraryClasses.common.DXE_DRIVER, LibraryClasses.common.UEFI_DRIVER, LibraryClasses.common.UEFI_APPLICATION]

+!if $(SECURE_BOOT_ENABLE) == TRUE

+  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf

+!endif

+

+[LibraryClasses.common.DXE_RUNTIME_DRIVER]

+!if $(SECURE_BOOT_ENABLE) == TRUE

+  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf

+!endif

+

[LibraryClasses.common.DXE_RUNTIME_DRIVER, LibraryClasses.common.UEFI_DRIVER, LibraryClasses.common.DXE_DRIVER, LibraryClasses.common.UEFI_APPLICATION]

   HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf

   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf

@@ -190,6 +209,10 @@

   gEmulatorPkgTokenSpaceGuid.PcdEmuFirmwareFdSize|0x002a0000

   gEmulatorPkgTokenSpaceGuid.PcdEmuFirmwareBlockSize|0x10000

   gEmulatorPkgTokenSpaceGuid.PcdEmuFirmwareVolume|L"../FV/FV_RECOVERY.fd"

+!if $(SECURE_BOOT_ENABLE) == TRUE

+  gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800

+  gEfiSecurityPkgTokenSpaceGuid.PcdUserPhysicalPresence|TRUE

+!endif

 

   gEmulatorPkgTokenSpaceGuid.PcdEmuMemorySize|L"64!64"

 

@@ -306,7 +329,14 @@

   EmulatorPkg/ResetRuntimeDxe/Reset.inf

   MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf

   EmulatorPkg/FvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf

-  MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf

+

+  MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {

+    <LibraryClasses>

+!if $(SECURE_BOOT_ENABLE) == TRUE

+      NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf

+!endif

+  }

+

   MdeModulePkg/Universal/EbcDxe/EbcDxe.inf

   MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf

   EmulatorPkg/EmuThunkDxe/EmuThunk.inf

@@ -315,6 +345,9 @@

   EmulatorPkg/PlatformSmbiosDxe/PlatformSmbiosDxe.inf

   EmulatorPkg/TimerDxe/Timer.inf

 

+!if $(SECURE_BOOT_ENABLE) == TRUE

+  SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf

+!endif

 

   MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {

     <LibraryClasses>

diff --git a/EmulatorPkg/EmulatorPkg.fdf b/EmulatorPkg/EmulatorPkg.fdf

index 295f6f1db8..b256aa9397 100644

--- a/EmulatorPkg/EmulatorPkg.fdf

+++ b/EmulatorPkg/EmulatorPkg.fdf

@@ -46,10 +46,17 @@ DATA = {

   # Blockmap[1]: End

   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,

   ## This is the VARIABLE_STORE_HEADER

+!if $(SECURE_BOOT_ENABLE) == FALSE

   #Signature: gEfiVariableGuid =

   #  { 0xddcf3616, 0x3275, 0x4164, { 0x98, 0xb6, 0xfe, 0x85, 0x70, 0x7f, 0xfe, 0x7d }}

   0x16, 0x36, 0xcf, 0xdd, 0x75, 0x32, 0x64, 0x41,

   0x98, 0xb6, 0xfe, 0x85, 0x70, 0x7f, 0xfe, 0x7d,

+!else

+  # Signature: gEfiAuthenticatedVariableGuid =

+  #  { 0xaaf32c78, 0x947b, 0x439a, { 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92 }}

+  0x78, 0x2c, 0xf3, 0xaa, 0x7b, 0x94, 0x9a, 0x43,

+  0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92,

+!endif

   #Size: 0xc000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) - 0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0xBFB8

   # This can speed up the Variable Dispatch a bit.

   0xB8, 0xBF, 0x00, 0x00,

@@ -186,6 +193,13 @@ INF  RuleOverride = UI MdeModulePkg/Application/UiApp/UiApp.inf

INF  MdeModulePkg/Application/BootManagerMenuApp/BootManagerMenuApp.inf

INF  MdeModulePkg/Universal/DriverSampleDxe/DriverSampleDxe.inf

 

+#

+# Secure Boot Key Enroll

+#

+!if $(SECURE_BOOT_ENABLE) == TRUE

+INF SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf

+!endif

+

#

# Network stack drivers

#

--

2.24.1.windows.2


Re: [PATCH v2] EmulatorPkg: Enable support for Secure Boot

Wadhawan, Divneil R
 

Hi Ray,

 

Yes, I have tested the following:

 

  1. SECURE_BOOT_ENABLE=true
  • Key Enrollment (PK, KEK, db) via custom mode
  • Execution of unit test shell application (signed one works okay, unsigned gives an Access denied)

 

  1. SECURE_BOOT_ENABLE=false (default case)
  • Secure Boot Configuration menu is not visible (Same as existing default case)
  • Execution of Unit Test Application (Signed/Unsigned both works okay)

 

I am planning to post the script in BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2949 in a day or too.

The script generates the full key hierarchy that makes it easy to test this patch.

The patch in BZ requires modifications as per Mike’s comment, so, you can skip the patches in BZ for now.

 

Regards,

Divneil

 

From: Ni, Ray <ray.ni@...>
Sent: Thursday, September 17, 2020 12:49 PM
To: Wadhawan, Divneil R <divneil.r.wadhawan@...>; devel@edk2.groups.io
Cc: gaoliming <gaoliming@...>; 'Andrew Fish' <afish@...>; Justen, Jordan L <jordan.l.justen@...>; Kinney, Michael D <michael.d.kinney@...>
Subject: RE: [edk2-devel] [PATCH v2] EmulatorPkg: Enable support for Secure Boot

 

Divneil,

Just want to double confirm: did you test the secure boot and non-secure boot?

 

Thanks,

Ray

 

From: Wadhawan, Divneil R <divneil.r.wadhawan@...>
Sent: Wednesday, September 16, 2020 11:53 PM
To: devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@...>; gaoliming <gaoliming@...>; 'Andrew Fish' <afish@...>; Justen, Jordan L <jordan.l.justen@...>; Kinney, Michael D <michael.d.kinney@...>; Wadhawan, Divneil R <divneil.r.wadhawan@...>
Subject: [edk2-devel] [PATCH v2] EmulatorPkg: Enable support for Secure Boot

 

SECURE_BOOT_ENABLE feature flag is introduced to enable Secure Boot.

The following gets enabled with this patch:

o Secure Boot Menu in "Device Manager" for enrolling keys

o Storage space for Authenticated Variables

o Authenticated execution of 3rd party images

 

Signed-off-by: Divneil Rai Wadhawan <divneil.r.wadhawan@...>

---

EmulatorPkg/EmulatorPkg.dsc | 37 +++++++++++++++++++++++++++++++++++--

EmulatorPkg/EmulatorPkg.fdf | 14 ++++++++++++++

2 files changed, 49 insertions(+), 2 deletions(-)

 

diff --git a/EmulatorPkg/EmulatorPkg.dsc b/EmulatorPkg/EmulatorPkg.dsc

index 86a6271735..c6e25c745e 100644

--- a/EmulatorPkg/EmulatorPkg.dsc

+++ b/EmulatorPkg/EmulatorPkg.dsc

@@ -32,6 +32,7 @@

   DEFINE NETWORK_TLS_ENABLE       = FALSE

   DEFINE NETWORK_HTTP_BOOT_ENABLE = FALSE

   DEFINE NETWORK_ISCSI_ENABLE     = FALSE

+  DEFINE SECURE_BOOT_ENABLE       = FALSE

 

 [SkuIds]

   0|DEFAULT

@@ -106,12 +107,20 @@

   LockBoxLib|MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.inf

   CpuExceptionHandlerLib|MdeModulePkg/Library/CpuExceptionHandlerLibNull/CpuExceptionHandlerLibNull.inf

   TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf

-  AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf

   VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf

   SortLib|MdeModulePkg/Library/BaseSortLib/BaseSortLib.inf

   ShellLib|ShellPkg/Library/UefiShellLib/UefiShellLib.inf

   FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf

 

+!if $(SECURE_BOOT_ENABLE) == TRUE

+  IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf

+  OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf

+  PlatformSecureLib|SecurityPkg/Library/PlatformSecureLibNull/PlatformSecureLibNull.inf

+  AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf

+!else

+  AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf

+!endif

+

[LibraryClasses.common.SEC]

   PeiServicesLib|EmulatorPkg/Library/SecPeiServicesLib/SecPeiServicesLib.inf

   PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf

@@ -162,6 +171,16 @@

   TimerLib|EmulatorPkg/Library/DxeCoreTimerLib/DxeCoreTimerLib.inf

  EmuThunkLib|EmulatorPkg/Library/DxeEmuLib/DxeEmuLib.inf

 

+[LibraryClasses.common.DXE_DRIVER, LibraryClasses.common.UEFI_DRIVER, LibraryClasses.common.UEFI_APPLICATION]

+!if $(SECURE_BOOT_ENABLE) == TRUE

+  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf

+!endif

+

+[LibraryClasses.common.DXE_RUNTIME_DRIVER]

+!if $(SECURE_BOOT_ENABLE) == TRUE

+  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf

+!endif

+

[LibraryClasses.common.DXE_RUNTIME_DRIVER, LibraryClasses.common.UEFI_DRIVER, LibraryClasses.common.DXE_DRIVER, LibraryClasses.common.UEFI_APPLICATION]

   HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf

   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf

@@ -190,6 +209,10 @@

   gEmulatorPkgTokenSpaceGuid.PcdEmuFirmwareFdSize|0x002a0000

   gEmulatorPkgTokenSpaceGuid.PcdEmuFirmwareBlockSize|0x10000

   gEmulatorPkgTokenSpaceGuid.PcdEmuFirmwareVolume|L"../FV/FV_RECOVERY.fd"

+!if $(SECURE_BOOT_ENABLE) == TRUE

+  gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800

+  gEfiSecurityPkgTokenSpaceGuid.PcdUserPhysicalPresence|TRUE

+!endif

 

   gEmulatorPkgTokenSpaceGuid.PcdEmuMemorySize|L"64!64"

 

@@ -306,7 +329,14 @@

   EmulatorPkg/ResetRuntimeDxe/Reset.inf

   MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf

   EmulatorPkg/FvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf

-  MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf

+

+  MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {

+    <LibraryClasses>

+!if $(SECURE_BOOT_ENABLE) == TRUE

+      NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf

+!endif

+  }

+

   MdeModulePkg/Universal/EbcDxe/EbcDxe.inf

   MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf

   EmulatorPkg/EmuThunkDxe/EmuThunk.inf

@@ -315,6 +345,9 @@

   EmulatorPkg/PlatformSmbiosDxe/PlatformSmbiosDxe.inf

   EmulatorPkg/TimerDxe/Timer.inf

 

+!if $(SECURE_BOOT_ENABLE) == TRUE

+  SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf

+!endif

 

   MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {

     <LibraryClasses>

diff --git a/EmulatorPkg/EmulatorPkg.fdf b/EmulatorPkg/EmulatorPkg.fdf

index 295f6f1db8..b256aa9397 100644

--- a/EmulatorPkg/EmulatorPkg.fdf

+++ b/EmulatorPkg/EmulatorPkg.fdf

@@ -46,10 +46,17 @@ DATA = {

   # Blockmap[1]: End

   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,

   ## This is the VARIABLE_STORE_HEADER

+!if $(SECURE_BOOT_ENABLE) == FALSE

   #Signature: gEfiVariableGuid =

   #  { 0xddcf3616, 0x3275, 0x4164, { 0x98, 0xb6, 0xfe, 0x85, 0x70, 0x7f, 0xfe, 0x7d }}

   0x16, 0x36, 0xcf, 0xdd, 0x75, 0x32, 0x64, 0x41,

   0x98, 0xb6, 0xfe, 0x85, 0x70, 0x7f, 0xfe, 0x7d,

+!else

+  # Signature: gEfiAuthenticatedVariableGuid =

+  #  { 0xaaf32c78, 0x947b, 0x439a, { 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92 }}

+  0x78, 0x2c, 0xf3, 0xaa, 0x7b, 0x94, 0x9a, 0x43,

+  0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92,

+!endif

   #Size: 0xc000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) - 0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0xBFB8

   # This can speed up the Variable Dispatch a bit.

   0xB8, 0xBF, 0x00, 0x00,

@@ -186,6 +193,13 @@ INF  RuleOverride = UI MdeModulePkg/Application/UiApp/UiApp.inf

INF  MdeModulePkg/Application/BootManagerMenuApp/BootManagerMenuApp.inf

INF  MdeModulePkg/Universal/DriverSampleDxe/DriverSampleDxe.inf

 

+#

+# Secure Boot Key Enroll

+#

+!if $(SECURE_BOOT_ENABLE) == TRUE

+INF SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf

+!endif

+

#

# Network stack drivers

#

--

2.24.1.windows.2


Re: more development process failure [was: UefiPayloadPkg: Runtime MMCONF]

Yao, Jiewen
 

Laszlo
I like your description to compare the process with the programming language and software design. We have to clean up the resource.

Please allow me to point out, this is the exactly the issue we are having today.

1) Take buffer overflow as an example. It has been there for 30 years. We have enough books and papers talking about it. Today, people are still making mistakes. Why? Because C language is not type safe, there is NO enforcement.
That is why many people like other type safe language. If you are trying to make mistake, the compiler will tell you that you are making mistake.

2) Take resource leak as an example. The programming language invented garbage collection. The operating system auto cleaned up application resource after execution.

3) People has wrong check in which may break the system. What is why the world invented smoke test and continuous integration.

*Those are where the inventions come*, to treat human as human being, to prevent people making mistake or teach them automatically. :-)

I agree with the "mythical man-month" that there is no silver bulletin.
I tend to agree with you on the attitude.
I am trying to figure if we can do better to help other people (maintainer or contributor). If we really really can do nothing, that is OK.
I am not sure if is a best way to resolve the problem to just complain in the email.

I think you can understand my point. I will stop here.

Thank you
Yao Jiewen

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo Ersek
Sent: Thursday, September 17, 2020 2:32 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io;
gaoliming@byosoft.com.cn; Dong, Guo <guo.dong@intel.com>
Cc: marcello.bauer@9elements.com; Kinney, Michael D
<michael.d.kinney@intel.com>; 'Leif Lindholm (Nuvia address)'
<leif@nuviainc.com>; Doran, Mark <mark.doran@intel.com>; 'Andrew Fish'
<afish@apple.com>; Guptha, Soumya K <soumya.k.guptha@intel.com>
Subject: Re: [edk2-devel] more development process failure [was:
UefiPayloadPkg: Runtime MMCONF]

Jiewen,

On 09/17/20 04:31, Yao, Jiewen wrote:
Hi Laszlo, Liming, and Mike

I appreciate your effort to setup rule and document this *complex* EDK II
Development Process.

I am thinking if we can have a way (a tool) to mandate these process and
check if there is any violation. If people makes mistake, he/she knows he/she is
making mistake and can correct immediately, instead of letting mistake happens
and getting blame later. In such way, we can prevent issue from happening.

We have old maintainer leaving, new maintainers joining. That is the reality.
We can have training for everyone. But we are still human. There are many bugs
need to be fixed in the code. How can we expect a perfect process that
everyone follows strictly without any violation?

If we only have few violation, it is OK to stay with it.
But if we continuously have violation, we need retrospect to ask, *why*? Why
there is such a process to cause so many violation?
And can we do better? A simpler process? A better tool?
while I agree that the current process is not really simple, I'd like to
point out some things:

- The current complexity exists because we are in a transition period,
and so we get to deal with both the workflow we're leaving (= the
mailing list based review) and the system we're adopting (= github).
This should not last forever. I don't know the exact schedule though.

- I think that lack of attention to detail (on the human side) takes a
relatively large chunk of the blame. The process at the moment is not
simple, but it's exercised every day, every week by some people, so if
somebody *wants*, they can get it right by following examples. Look at
recent patch series threads that have been merged, recent BZs that have
been closed, recent PRs that have been opened and merged.

It's a fallacy that adopting a 100% github.com-native patch review
workflow will solve all of these issues. There is no replacement for
human discipline and attention to detail. In the current process, I
*regularly* find pull requests (personal builds or maintainer push
attempts) on github.com that fail CI (or merging due to conflicts) and
then the submitter never bothers to close or refresh them. I have
cleaned up (closed) a *multitude* of such PRs.

I also feel sorry that Laszlo need check by his eye on every PR and catch the
violation for us. And I also feel sorry to blame some people who is contributing
his/her time to help to maintain the code, review the code, check in the code.
We both feel frustrated. We are all coming her to enable new features or fix
bugs to make EDKII better.

I would like to ask: Is that technically possible to enhance the CI to catch that
earlier, as Laszlo point out:
1) Add patch 0 to PR - can we let CI reject empty description PR?
It won't help.

See the following bug report:

https://bugzilla.tianocore.org/show_bug.cgi?id=2963#c0

While it is technically not empty (the string in comment#0 has nonzero
length), it's practically *devoid of information*.

People that are annoyed that they are required to write sensible
summaries for patch sets and bug reports, will do anything and
everything to wiggle out of that requirement. They will create
single-sentence PR descriptions, which will allow them to pass the CI
check. And the community will be *worse off*, because we will have
complicated our CI logic, but the resultant historical records will be
just as useless.

2) Send email - can we let CI send email automatically? Or remind us to send
email?

github.com *already* sends an email notification when a PR undergoes a
state change; that is, when it is merged, or else CI fails. The email is
*already* there, people just have to *act* upon it -- run a local "git
pull" again, see what the new commit range is, and send a response to
the original thread.

3) update Bugzilla - can we let CI update Bugzilla automatically? Or remind us
to update bugzilla?

Automatically closing tickets is not implemented between github.com and
Bugzilla. It is implemented within github.com (merging a PR can
auto-close issue tracker tickets, if you format the commit message
corectly).

However, auto-closing is *wrong*. It occurs that multiple patch series
relate to a single ticket. In such cases, it's possible that 10+ patches
are merged for a single ticket, and the ticket should *still* not be
closed, because more patches (for the same ticket) are necessary. Only a
human can tell when the fix or the feature is considered complete
(according to their knowledge at that point in time).

4) Unicode char - can we add check in patchchecker, to reject predefined
format violation?

There are many-many classes of unicode code points. It's not easy to
express "accept U+003A for punctuation, but do not accept U+FF1A".

It's easy to express "accept 7-bit ASCII only", but I think some people
might take issue with that, because then their names could not be placed
in subject lines in native form.


I know the new tool/CI cannot be built in one day. And we do improvement
step by step.

The *real* problem is with the attitude. If a developer cares *only*
until their patches are merged, then no tooling will *ever* fix this
issue. People need to start caring about the afterlife of their work.
When you throw a party, or join one, you stay around for the cleanup
afterwards, don't you?

When you call a contractor to fix something in or around the house, do
you expect them to clean up when they're done, or are you happy cleaning
after them?


The exact same bad attitude is the reason that

- we have botched error paths in programming languages like C,

- we have programming languages and libraries that attempt (and *fail*)
to clean up resources on errors, "on behalf" of the programmer -- I'm
referring to exceptions and destructors in C++, for example.

Both of these are symptoms that people *refuse* to deal with the
"boring" aspects of the job.


Just accept that the party isn't finished until the house and the garden
are tidied up, and the furniture is restored to original order.

Laszlo





Re: [PATCH v2] EmulatorPkg: Enable support for Secure Boot

Ni, Ray
 

Divneil,

Just want to double confirm: did you test the secure boot and non-secure boot?

 

Thanks,

Ray

 

From: Wadhawan, Divneil R <divneil.r.wadhawan@...>
Sent: Wednesday, September 16, 2020 11:53 PM
To: devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@...>; gaoliming <gaoliming@...>; 'Andrew Fish' <afish@...>; Justen, Jordan L <jordan.l.justen@...>; Kinney, Michael D <michael.d.kinney@...>; Wadhawan, Divneil R <divneil.r.wadhawan@...>
Subject: [edk2-devel] [PATCH v2] EmulatorPkg: Enable support for Secure Boot

 

SECURE_BOOT_ENABLE feature flag is introduced to enable Secure Boot.

The following gets enabled with this patch:

o Secure Boot Menu in "Device Manager" for enrolling keys

o Storage space for Authenticated Variables

o Authenticated execution of 3rd party images

 

Signed-off-by: Divneil Rai Wadhawan <divneil.r.wadhawan@...>

---

EmulatorPkg/EmulatorPkg.dsc | 37 +++++++++++++++++++++++++++++++++++--

EmulatorPkg/EmulatorPkg.fdf | 14 ++++++++++++++

2 files changed, 49 insertions(+), 2 deletions(-)

 

diff --git a/EmulatorPkg/EmulatorPkg.dsc b/EmulatorPkg/EmulatorPkg.dsc

index 86a6271735..c6e25c745e 100644

--- a/EmulatorPkg/EmulatorPkg.dsc

+++ b/EmulatorPkg/EmulatorPkg.dsc

@@ -32,6 +32,7 @@

   DEFINE NETWORK_TLS_ENABLE       = FALSE

   DEFINE NETWORK_HTTP_BOOT_ENABLE = FALSE

   DEFINE NETWORK_ISCSI_ENABLE     = FALSE

+  DEFINE SECURE_BOOT_ENABLE       = FALSE

 

 [SkuIds]

   0|DEFAULT

@@ -106,12 +107,20 @@

   LockBoxLib|MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.inf

   CpuExceptionHandlerLib|MdeModulePkg/Library/CpuExceptionHandlerLibNull/CpuExceptionHandlerLibNull.inf

   TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf

-  AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf

   VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf

   SortLib|MdeModulePkg/Library/BaseSortLib/BaseSortLib.inf

   ShellLib|ShellPkg/Library/UefiShellLib/UefiShellLib.inf

   FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf

 

+!if $(SECURE_BOOT_ENABLE) == TRUE

+  IntrinsicLib|CryptoPkg/Library/IntrinsicLib/IntrinsicLib.inf

+  OpensslLib|CryptoPkg/Library/OpensslLib/OpensslLibCrypto.inf

+  PlatformSecureLib|SecurityPkg/Library/PlatformSecureLibNull/PlatformSecureLibNull.inf

+  AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf

+!else

+  AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableLibNull.inf

+!endif

+

[LibraryClasses.common.SEC]

   PeiServicesLib|EmulatorPkg/Library/SecPeiServicesLib/SecPeiServicesLib.inf

   PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf

@@ -162,6 +171,16 @@

   TimerLib|EmulatorPkg/Library/DxeCoreTimerLib/DxeCoreTimerLib.inf

  EmuThunkLib|EmulatorPkg/Library/DxeEmuLib/DxeEmuLib.inf

 

+[LibraryClasses.common.DXE_DRIVER, LibraryClasses.common.UEFI_DRIVER, LibraryClasses.common.UEFI_APPLICATION]

+!if $(SECURE_BOOT_ENABLE) == TRUE

+  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/BaseCryptLib.inf

+!endif

+

+[LibraryClasses.common.DXE_RUNTIME_DRIVER]

+!if $(SECURE_BOOT_ENABLE) == TRUE

+  BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf

+!endif

+

[LibraryClasses.common.DXE_RUNTIME_DRIVER, LibraryClasses.common.UEFI_DRIVER, LibraryClasses.common.DXE_DRIVER, LibraryClasses.common.UEFI_APPLICATION]

   HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf

   PcdLib|MdePkg/Library/DxePcdLib/DxePcdLib.inf

@@ -190,6 +209,10 @@

   gEmulatorPkgTokenSpaceGuid.PcdEmuFirmwareFdSize|0x002a0000

   gEmulatorPkgTokenSpaceGuid.PcdEmuFirmwareBlockSize|0x10000

   gEmulatorPkgTokenSpaceGuid.PcdEmuFirmwareVolume|L"../FV/FV_RECOVERY.fd"

+!if $(SECURE_BOOT_ENABLE) == TRUE

+  gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize|0x2800

+  gEfiSecurityPkgTokenSpaceGuid.PcdUserPhysicalPresence|TRUE

+!endif

 

   gEmulatorPkgTokenSpaceGuid.PcdEmuMemorySize|L"64!64"

 

@@ -306,7 +329,14 @@

   EmulatorPkg/ResetRuntimeDxe/Reset.inf

   MdeModulePkg/Core/RuntimeDxe/RuntimeDxe.inf

   EmulatorPkg/FvbServicesRuntimeDxe/FvbServicesRuntimeDxe.inf

-  MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf

+

+  MdeModulePkg/Universal/SecurityStubDxe/SecurityStubDxe.inf {

+    <LibraryClasses>

+!if $(SECURE_BOOT_ENABLE) == TRUE

+      NULL|SecurityPkg/Library/DxeImageVerificationLib/DxeImageVerificationLib.inf

+!endif

+  }

+

   MdeModulePkg/Universal/EbcDxe/EbcDxe.inf

   MdeModulePkg/Universal/MemoryTest/NullMemoryTestDxe/NullMemoryTestDxe.inf

   EmulatorPkg/EmuThunkDxe/EmuThunk.inf

@@ -315,6 +345,9 @@

   EmulatorPkg/PlatformSmbiosDxe/PlatformSmbiosDxe.inf

   EmulatorPkg/TimerDxe/Timer.inf

 

+!if $(SECURE_BOOT_ENABLE) == TRUE

+  SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf

+!endif

 

   MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf {

     <LibraryClasses>

diff --git a/EmulatorPkg/EmulatorPkg.fdf b/EmulatorPkg/EmulatorPkg.fdf

index 295f6f1db8..b256aa9397 100644

--- a/EmulatorPkg/EmulatorPkg.fdf

+++ b/EmulatorPkg/EmulatorPkg.fdf

@@ -46,10 +46,17 @@ DATA = {

   # Blockmap[1]: End

   0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,

   ## This is the VARIABLE_STORE_HEADER

+!if $(SECURE_BOOT_ENABLE) == FALSE

   #Signature: gEfiVariableGuid =

   #  { 0xddcf3616, 0x3275, 0x4164, { 0x98, 0xb6, 0xfe, 0x85, 0x70, 0x7f, 0xfe, 0x7d }}

   0x16, 0x36, 0xcf, 0xdd, 0x75, 0x32, 0x64, 0x41,

   0x98, 0xb6, 0xfe, 0x85, 0x70, 0x7f, 0xfe, 0x7d,

+!else

+  # Signature: gEfiAuthenticatedVariableGuid =

+  #  { 0xaaf32c78, 0x947b, 0x439a, { 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92 }}

+  0x78, 0x2c, 0xf3, 0xaa, 0x7b, 0x94, 0x9a, 0x43,

+  0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3, 0x77, 0x92,

+!endif

   #Size: 0xc000 (gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize) - 0x48 (size of EFI_FIRMWARE_VOLUME_HEADER) = 0xBFB8

   # This can speed up the Variable Dispatch a bit.

   0xB8, 0xBF, 0x00, 0x00,

@@ -186,6 +193,13 @@ INF  RuleOverride = UI MdeModulePkg/Application/UiApp/UiApp.inf

INF  MdeModulePkg/Application/BootManagerMenuApp/BootManagerMenuApp.inf

INF  MdeModulePkg/Universal/DriverSampleDxe/DriverSampleDxe.inf

 

+#

+# Secure Boot Key Enroll

+#

+!if $(SECURE_BOOT_ENABLE) == TRUE

+INF SecurityPkg/VariableAuthenticated/SecureBootConfigDxe/SecureBootConfigDxe.inf

+!endif

+

#

# Network stack drivers

#

--

2.24.1.windows.2


Re: more development process failure [was: UefiPayloadPkg: Runtime MMCONF]

Laszlo Ersek
 

Jiewen,

On 09/17/20 04:31, Yao, Jiewen wrote:
Hi Laszlo, Liming, and Mike

I appreciate your effort to setup rule and document this *complex* EDK II Development Process.

I am thinking if we can have a way (a tool) to mandate these process and check if there is any violation. If people makes mistake, he/she knows he/she is making mistake and can correct immediately, instead of letting mistake happens and getting blame later. In such way, we can prevent issue from happening.

We have old maintainer leaving, new maintainers joining. That is the reality. We can have training for everyone. But we are still human. There are many bugs need to be fixed in the code. How can we expect a perfect process that everyone follows strictly without any violation?

If we only have few violation, it is OK to stay with it.
But if we continuously have violation, we need retrospect to ask, *why*? Why there is such a process to cause so many violation?
And can we do better? A simpler process? A better tool?
while I agree that the current process is not really simple, I'd like to
point out some things:

- The current complexity exists because we are in a transition period,
and so we get to deal with both the workflow we're leaving (= the
mailing list based review) and the system we're adopting (= github).
This should not last forever. I don't know the exact schedule though.

- I think that lack of attention to detail (on the human side) takes a
relatively large chunk of the blame. The process at the moment is not
simple, but it's exercised every day, every week by some people, so if
somebody *wants*, they can get it right by following examples. Look at
recent patch series threads that have been merged, recent BZs that have
been closed, recent PRs that have been opened and merged.

It's a fallacy that adopting a 100% github.com-native patch review
workflow will solve all of these issues. There is no replacement for
human discipline and attention to detail. In the current process, I
*regularly* find pull requests (personal builds or maintainer push
attempts) on github.com that fail CI (or merging due to conflicts) and
then the submitter never bothers to close or refresh them. I have
cleaned up (closed) a *multitude* of such PRs.

I also feel sorry that Laszlo need check by his eye on every PR and catch the violation for us. And I also feel sorry to blame some people who is contributing his/her time to help to maintain the code, review the code, check in the code.
We both feel frustrated. We are all coming her to enable new features or fix bugs to make EDKII better.

I would like to ask: Is that technically possible to enhance the CI to catch that earlier, as Laszlo point out:
1) Add patch 0 to PR - can we let CI reject empty description PR?
It won't help.

See the following bug report:

https://bugzilla.tianocore.org/show_bug.cgi?id=2963#c0

While it is technically not empty (the string in comment#0 has nonzero
length), it's practically *devoid of information*.

People that are annoyed that they are required to write sensible
summaries for patch sets and bug reports, will do anything and
everything to wiggle out of that requirement. They will create
single-sentence PR descriptions, which will allow them to pass the CI
check. And the community will be *worse off*, because we will have
complicated our CI logic, but the resultant historical records will be
just as useless.

2) Send email - can we let CI send email automatically? Or remind us to send email?
github.com *already* sends an email notification when a PR undergoes a
state change; that is, when it is merged, or else CI fails. The email is
*already* there, people just have to *act* upon it -- run a local "git
pull" again, see what the new commit range is, and send a response to
the original thread.

3) update Bugzilla - can we let CI update Bugzilla automatically? Or remind us to update bugzilla?
Automatically closing tickets is not implemented between github.com and
Bugzilla. It is implemented within github.com (merging a PR can
auto-close issue tracker tickets, if you format the commit message
corectly).

However, auto-closing is *wrong*. It occurs that multiple patch series
relate to a single ticket. In such cases, it's possible that 10+ patches
are merged for a single ticket, and the ticket should *still* not be
closed, because more patches (for the same ticket) are necessary. Only a
human can tell when the fix or the feature is considered complete
(according to their knowledge at that point in time).

4) Unicode char - can we add check in patchchecker, to reject predefined format violation?
There are many-many classes of unicode code points. It's not easy to
express "accept U+003A for punctuation, but do not accept U+FF1A".

It's easy to express "accept 7-bit ASCII only", but I think some people
might take issue with that, because then their names could not be placed
in subject lines in native form.


I know the new tool/CI cannot be built in one day. And we do improvement step by step.
The *real* problem is with the attitude. If a developer cares *only*
until their patches are merged, then no tooling will *ever* fix this
issue. People need to start caring about the afterlife of their work.
When you throw a party, or join one, you stay around for the cleanup
afterwards, don't you?

When you call a contractor to fix something in or around the house, do
you expect them to clean up when they're done, or are you happy cleaning
after them?


The exact same bad attitude is the reason that

- we have botched error paths in programming languages like C,

- we have programming languages and libraries that attempt (and *fail*)
to clean up resources on errors, "on behalf" of the programmer -- I'm
referring to exceptions and destructors in C++, for example.

Both of these are symptoms that people *refuse* to deal with the
"boring" aspects of the job.


Just accept that the party isn't finished until the house and the garden
are tidied up, and the furniture is restored to original order.

Laszlo

17401 - 17420 of 82722