Date   

Re: PKCS7 Authenticated Variable Enrollment

Guomin Jiang
 

Hi Divneil,

Good News, thanks for your update.

-----Original Message-----
From: Wadhawan, Divneil R <divneil.r.wadhawan@...>
Sent: Tuesday, June 30, 2020 1:49 AM
To: Jiang, Guomin <guomin.jiang@...>; rfc@edk2.groups.io
Cc: Zimmer, Vincent <vincent.zimmer@...>; Wadhawan, Divneil R
<divneil.r.wadhawan@...>
Subject: RE: [edk2-rfc] PKCS7 Authenticated Variable Enrollment

Hi Guomin,

I have moved forward from the last discussion, and the tools work okay for
RSA2048/SHA256.
Once I have the next update, I will notify it here.

Regards,
Divneil

-----Original Message-----
From: Jiang, Guomin <guomin.jiang@...>
Sent: Wednesday, May 27, 2020 4:34 PM
To: rfc@edk2.groups.io; Wadhawan, Divneil R
<divneil.r.wadhawan@...>
Subject: RE: [edk2-rfc] PKCS7 Authenticated Variable Enrollment

I am sorry that I have not the use case, and I plan to investigating it after
August.

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of
divneil.r.wadhawan@...
Sent: Wednesday, May 27, 2020 6:54 PM
To: Jiang, Guomin <guomin.jiang@...>; rfc@edk2.groups.io
Subject: Re: [edk2-rfc] PKCS7 Authenticated Variable Enrollment

Hi Goumin,

I had discussion internally, and I got hold off tools from:
https://git.kernel.org/pub/scm/linux/kernel/git/jejb/efitools.git.
It is generating the correct format as per
EFI_VARIABLE_AUTHENTICATION_2.

So, I thought of first validating RSA2048 Sign verification and it is failing.
I still have to figure out that. Do you have a working use case which
uses
PKCS7 format and PKCS7_verify works?

Regards,
Divneil


Re: UEFI accessibility mandate

Ethin Probst
 

Hello all,
Raising this from (the seeming) silence it's been in since the 30th of
Oct. of 2019. What is the status on this? Also, is there a bit of
source code you guys can share with me for initializing the HDA
controller via MMIO (assume the address is already known) that I can
look at? I have two reasons for asking: (1) I'm writing an little
hobby operating system and would love to get audio in it (but I still
haven't really figured out HDA :)); and (2) I'd like to see about
creating a UEFI application in rust to see if I can embed a speech
synthesizer I know of out there and see if it can talk. If I can get
that working, I'll create a github repo for that and we can see about
turning it into an actual UEFI driver. Intercepting text on the screen
will be a challenge that EDK2 will most likely need to undertake, as I
have never contributed to EDK2 and find its source code layout quite
confusing.

On 10/30/19, Rafael Machado <rafaelrodrigues.machado@...> wrote:
Hi Ethin

I think you have entered the community after the code was sent at this
discussion.
Attached the latest code we have received from Andrew. Not sure if this is
really the latest version, but it's a good starting point for your tests.

Thanks and Regards
Rafael R. Machado

Em ter, 29 de out de 2019 às 20:14, Ethin Probst <harlydavidsen@...>
escreveu:

This is incredible progress! I'd love to help where I can when you
release the src.

On 10/29/19, Andrew Fish <afish@...> wrote:
Ethin,

I've made progress using the EmulatorPkg. I've ported my test C command
line
application into the EmulatorPkg Host (OS C command line application
part
of the EmulatorPkg), so the the host now produces APIs that support
Text
to
Voice PCM files, and playing PCM, also called wave, files. There is a
EmulatorPkg driver to produce Text To PCM, and a driver to play PCM
files. I
also wrote a test application. At this point the test application in
the
emulator can use the two APIs to produce Text To Voice.

Next steps for me are to write up a Bugzilla, cleanup the code, and
make
github branch to share the work.

Next steps for the community... From the EmulatorPkg it should be
possible
to implement the UI work to add voice. it should also be possible to
implement a real EFI Voice to PCM driver. It should also be possible
to
use
the test application to test a real EFI audio driver.

Thanks,

Andrew Fish

On Oct 29, 2019, at 1:17 PM, Ethin Probst <harlydavidsen@...>
wrote:

Has there been any progress on this? I wish I could help, but I have
no experience in developing for EDK2...

On 10/1/19, Rafael Machado <rafaelrodrigues.machado@...> wrote:
Incredible!
I will find some time to try it at linux on the following weeks.
Thanks for the effort Andrew!

Rafael R. Machado

Em seg, 30 de set de 2019 às 22:18, Andrew Fish <afish@...>
escreveu:



On Sep 30, 2019, at 11:50 AM, Rafael Machado <
rafaelrodrigues.machado@...> wrote:

Hi Andrew

As you have mentioned:
" I might find some time to try to get the EmulatorPkg mocking
working."
Answer: This would be amazing! Thanks for trying to help!


Rafael,

I made a little progress this weekend watching American Football on
TV,
and I had some time on an airplane today to add some comments.

I wrote a simple C command line program that does almost all the
primitive
audio functions we need to add Audio/Text-To-Speech to the
EmulatorPkg.
Since ANSI C does not support audio I had to system() out to macOS
command
line tools to get some of the tasks complete. The only feature I'm
missing
is playing the PCM buffer asynchronously, and I may be able to add
that
next weekend. I guessed at the Linux commands, it would be good if
someone
could debug the program on a Linux system, and also make sure I
picked
the
right command line tools to use. I think for Windows there is going
to
be
more coding involved, but I think there is at least a C API to play
audio,
but I only saw a C++ API for speech to text. But the good news is
folks
can
use the simple C program to get the primitives working on Linux and
Windows
systems and it should be easy to port that work to the future
EmulatorPkg
work.

What I've got working on macOS is:
1) Mute/Unmute
2) GetVolume/SetVolume
3) Text to PCD buffer (Wave file format)
a) Supports word per minute rate
b) Supports localizing the voice based on RFC 4646 language code
used
by
EFI (the program speaks in en, zh, and fr as a test)
4) PCD buffer to audio

This is how I was building on my Mac: clang -g -Wall -Werror audio.c

Short term anyone that has some free time if they could look at
testing/debugging audio.c on Linux and send feedback, also maybe
some
one
could start playing around with getting audio.c ported to Windows?

After I figure out getting async audio output working in audio.c I
think
the next steps are working on the protocol/variable definitions.
With
the
protocol/variable definitions we should be able to add the
capability
to
the EmulatorPkg. At that point it should be possible for anyone to
help
work on the accessibility UI. Obviously to get this working on a
real
platform we will need a pure EFI Text to Speech driver (We could
debug
that
in the EmulatorPkg). We would also need Intel HDA Audio drivers, and
Audio
drivers for OVFM, and other Audio hardware that is on peoples
platforms.

Thanks,

Andrew Fish

The plan is to try to get some students to help on this also at the
next
"google summer of code".
Not sure how it works, but since the idea is good, and it is a real
need,
I believe we have chances to have some more hands working on it.

Thanks and Regards
Rafael R. Machado



Em qui, 26 de set de 2019 às 20:44, Andrew Fish <afish@...>
escreveu:



On Sep 26, 2019, at 5:15 AM, Rafael Machado <
rafaelrodrigues.machado@...> wrote:

Hi everyone.

About Ethin's question:
As for the boot manager, yes, that would be covered by the speech
synthesizer protocol. Perhaps we could extend this to the setup
utility
too
(as well as all other things displayed on the screen)?
Answer: This is the target from my point of view. Since we have the
advantages of drivers at UEFI, there is no reason for not having
accessibility at pre-OS anymore. We could also think in future to
add
other
kinds of accessibility, like the magnifying glass (that would be
some
multi-layer version of the GOP driver), that would help low-vision
people.
Just one thing for everyone to think about. The "Convention on the
Rights
of Persons with Disabilities" say that a person has the right to
access
information. So if someone buys a notebook, all information
accessible
to
"common" people must be available to all kinds of people, in a way
they
can
choose to use this information or not.
*" (e) Recognizing that disability is an evolving concept and that
disability results from the interaction between persons with
impairments
and attitudinal and environmental barriers that hinders their full
and
effective participation in society on an equal basis with others,
..."*
Details can be found at the United Nations web site
https://www.un.org/disabilities/documents/convention/convention_accessible_pdf.pdf


About Andrew's comments and questions:
What member functions to you think that EFI_AUDIO_OUTPUT_PROTOCOL
should contain?
Answer: From my perspective, this protocol would need at least the
following functions (names may change after more discussion.)


- EFI_AUDIO_OUTPUT_PROTOCOL.LoadStream()
This function is intended to copy some data to the dma buffers
that
were allocated previously by the AudioController driver. The
initial
idea
I
had was to add, in the case of my MSc, a HdaDriver.efi that is
responsible
to do all the HDA controller initialization (allocating dma
buffers,
configuring the PCIe bus if needed). Not sure if this
initialization
should
we added to the EFI_AUDIO_OUTPUT_PROTOCOL, because this could
create
problems when using other controllers or concepts, like I believe
ARM
platforms do.My understanding is that the platforms should have a
driver
to
initialize the audio controller/codec, so the
EFI_AUDIO_OUTPUT_PROTOCOL
is
just worried with loading and playing information.

- EFI_AUDIO_OUTPUT_PROTOCOL.PlayStream()
This function is responsible to play the stream. This is done by
setting one specific bit at the controllers configuration space
(pointed
by
the bar[0] pcie configuration register at the pcie config space.)
At
this
project I don't think we need to be worried with implementing an
sound
server to enable multiple streams at a time. To the actions done by
this
function are much simpler. Just play a stream.

- EFI_AUDIO_OUTPUT_PROTOCOL.StopStream()
This function is responsible to stop the stream play. The
opposite
of
the previous one. Maybe we could use a single function that
receives
some
parameter to start or stop the play.

- EFI_AUDIO_OUTPUT_PROTOCOL.SetVolume()
This function is responsible to set the volume to be used at all
nodes at the codec (so it depends on a EFI_AUDIO_CODEC_PROTOCOL
that
knows
the commands a given codec needs to receive to process streams.)

- EFI_AUDIO_OUTPUT_PROTOCOL.SetStreamSpeed()
This function is responsible to set the speed a given stream
should
be played on. Just to clarify, visual impaired people have an
amazing
capacity of understanding sound streams in incredibly fast speeds.
So
the
possibility of setting the stream processing speed is what makes
them
be
as
productive as a person with usual vision capacities.

- EFI_AUDIO_OUTPUT_PROTOCOL.UnloadStream()
This function is responsible to clean the buffers and letting
them
prepared to receive other audio streams.

Still about Andrews comments.
This gave me the idea of adding a EFI_AUDIO_OUTPUT_PROTOCOL and
EFI_TEXT_TO_SPEECH_PROTOCOL
driver to the EmulatorPkg that thunk down to the App to do the
work.
With this we could prototype the UI part of accessibility.
Answer: This would be great, because we would split the
development.
One
focusing on the HII and navigation (done normally using the tab
key,
so
each element at the screen would need an tab sequence number and a
accessibility tag with what the speech software should say), and
another
focusing on the drivers/synthesizer/audio files depending on our
future
decisions.

So my understanding is that the list of things is increasing. It is
actually something like this (not ordered by priority):
- EFI_AUDIO_OUTPUT_PROTOCOL: Protocol used to process the streams
- EFI_AUDIO_CODEC_PROTOCOL: Protocol used to communicate with the
codec
- Audio configuration driver (HdaAudio.efi for example). Should
this
create some other protocol, like EFI_AUDIO_CONTROLLER_PROTOCOL
that
is
responsible to configure the controller?


Rafael,

Generally how it works for a PCI device is it gets its Start()
function
called to bind to the device. So the EFI_AUDIO_OUTPUT_PROTOCOL and
EFI_AUDIO_CODEC_PROTOCOL protocols would likely get installed on
the
PCI
IO
handle of the HDA PCI hardware. Some basic init will happen during
the
Start() and the driver could defer some config until one of the
member
functions is called the 1st time.

So for example on a normal boot the Audio driver may not even get
started. So there may have to be an nvram variable to start the
driver.
We
might also need an NVRAM variable for the voice to text driver if
it
is
stored on disk.

- EFI_TEXT_TO_SPEECH_PROTOCOL: Protocol responsible to convert text
to
speech (using the EFI_AUDIO_OUTPUT_PROTOCOL )
- HII changes to add tab navigation capabilities and accessibility
tags
at each component on the screen

Are we aligned with the understanding?


I think so and if we can mock an implementation in the EmulatorPkg
that
would let the HII UI work happen independently of the text to
speech,
and
Audio driver work.

I'm on some air plaines in the next weeks (I'm at the airport right
now),
and my wife is heading to a conference after I get back from my
trips.
Given my dogs are not very talkative I might find some time to try
to
get
the EmulatorPkg mocking working.

Thanks,

Andrew Fish

I believe soon we will be prepared to talk about this on a design
meeting.

Thanks and Regards
Rafael R. Machado



Em qua, 25 de set de 2019 às 22:08, Andrew Fish <afish@...>
escreveu:

Rafael,

What member functions to you think that EFI_AUDIO_OUTPUT_PROTOCOL
should
contain?

I'm thinking if we had an EFI_TEXT_TO_SPEECH_PROTOCOL that driver
could
produce a PCM/Wave buffer, it could then use
EFI_AUDIO_OUTPUT_PROTOCOL
to play the sound.

I poked around my Mac at lunch and I can generate text to speech
from
the command line into a wave file via the `say` command line tool.
I
can
play the wave file from the command line via `afplay`. This gave
me
the
idea of adding a EFI_AUDIO_OUTPUT_PROTOCOL and
EFI_TEXT_TO_SPEECH_PROTOCOL
driver to the EmulatorPkg that thunk down to the App to do the
work.
With this we could prototype the UI part of accessibility. We
could
also
debug the EFI EFI_TEXT_TO_SPEECH_PROTOCOL in this environment.

Thanks,

Andrew Fish

On Sep 21, 2019, at 5:36 AM, Rafael Machado <
rafaelrodrigues.machado@...> wrote:

Hi Everyone
Sorry for the delay on the response, to many things happening at
the
same time.
I will try to answer e-mails to this thread every Saturday or
Sunday
morning at least.
About Andrew's and Laszlo's comments and questions

Please let us know what you find out. I probably don''t have the
time
to help implement this feature, but I happy to help work on the
architecture and design for UEFI accessibility on the edk2 mailing
lists,
and I >>can also represent what ever we come up with at the UEFI
Spec
Work
Group.
During my MSc I had to study a lot the audio and BIOS
architectures.
The
idea was to eliminate the first barrier to the creation of a
screen
reader
for pre-OS environment, that was the lack of some open
implementation
of
audio control and actions at UEFI. To do that I studied the Intel
High
Definition Audio Spec and a lot of UEFI specs to understand better
how
to
do that.
The initial target was to do all this development at OVMF, but as
far
as
I could get, the sound card is not available to OVMF. as Laszlo
mentioned
at this e-mail there are some projects that may help on this, but
at
the
time I was working on my MSc I didn't find this, so I did
everything
on
a
real system (a ASUS notebook).
It took me 2 years of work, because I didn't know a lot of things
and
working on a MSc degree at the same time having a 40hours/week
job,
being a
father and a husband is not an easy task, but it got to what I was
expecting.
The evolution of the project was this:
1 - First tests using some registers named "Immediate Registers",
that
later I noticed that are not mandatory. This is a simple C Major
scale:
https://www.youtube.com/watch?v=I-mgzcOnRCg&feature=youtu.be
2 - Some months later I started to work with the Ring Buffers and
DMA
memory access. For the ones that have good years, it's possible to
listen
some music behing the noise.
https://www.youtube.com/watch?v=6ED2BSc89-Y&feature=youtu.be
3 - Later, wen I was almost giving up, I noticed that the problem
was
that one of my write operations was causing some misunderstanding
between
the audio controller and the audio codec. The controller was
sending
packets with 16bit depth, but the codec was processing them as
8bit
depth
https://www.youtube.com/watch?v=2De9dI9WbwM&feature=youtu.be

So the conclusion is that doing this at UEFI us much easier that
doing
at the OS level.
The reference code, that is just a proof-of-concept, and that has
several things to be improved, can be found here:
https://github.com/RafaelRMachado/Msc_UefiHda_PreOs_Accessibility

Currently it is just an UEFI Application, but we can convert this
to
UEFI drivers after some discussion. Everything is released as BDS
so
companies can use without IP problems.
Just to give some more information about the need of this kind of
solution. There is a lot of blind people that work with hardware
support,
so formatting disk, configuring RAID and booting dual-boot systems
is
always a challenge to them. Even set the BIOS clock. How to do
that
without
the system's feedback?

It would be hard to have a UEFI mandate for accessibility, given
there
is no guideline on how a User Interface (UI) works. If
accessibility
requires some from of hardware abstraction, like audio, then we
could
likely get that into the UEFI Spec. What might be possible is an
EDK2
reference implementation of accessibility. Maybe we could use the
reference
implementation to write a UEFI white paper on design >>for
accessibility? I
there is an open source implementation, and an official design
guide
this
would make it much easier for advocacy groups to lobby for this
feature.
I agree this is the way. Writing a white paper as an official EDK2
papers is one of my targets since the beginning of my MSc almost 5
years
ago.

I've got some experience with accessibility as the macOS EFI OS
Loader
has a UI for the Full Disk Encryption password. If you press the
power
button quickly 3 times at the disk encryption password prompt
accessibility is enabled and Voice Over gets turned on. You then
get
localized voice prompts when you move between UI elements. Since
this
is
the OS loader all the resources are stored on the disk. You
quickly
run
into a range of challenges since, audio is hard, abstracting audio
is
hard
(what codec does firmware have to support), Audio files are not
small
and
firmware is size constrained, the need to localize >>the audio
responses
causes even more size issues, the boot options are usually written
by
an
OS
installer so how would firmware know what to call them?
The solution to this would be the porting of some voice
synthesizer,
so
no audio files would need to be stored. There are some open-source
implementations that are not GPL.
This was described at my MSc as future works that can continue
what
I
have started.

I listed a lot of reasons it is hard but as Kennedy stated in
his
"We
choose to go to the Moon!" speech sometimes we chose to do things
"not
because they are easy, but because they are hard; because that
goal
will
serve to organize and measure the best of our energies and skills,
because
that challenge is one that we are willing to accept". If we have a
design
that means we can break the problem up into >>smaller parts, and
maybe
we
can find people that have expertise in that part to build a chunk
at
a
time. If we could implement the prototype in OVMF that would show
how
it
works, but run on everyone's >>machines, so that would be really
helpful
for demos and design review.
I totally agree. Amazing words that I didn't have heard yet.
Thanks!
As far as I could understand, and with Leif's help, some possible
future
steps could be (not at this specific order):
- 1) Convert proof-of-concept HDA driver to UEFI driver model
with
proper PCI discovery.
- 2) Design a prototype EFI_AUDIO_OUTPUT_PROTOCOL, rework driver
to produce this and application to discover and consume it.
- 3) Implement a USB Audio Class driver also
producing EFI_AUDIO_OUTPUT_PROTOCOL and ensure test application
remains functional.
- 4) Separate controller and codec code by creating
an EFI_AUDIO_CODEC_PROTOCOL, implement this in HDA driver, and
separate
out
the codec support into individual drivers.
- 5) Prototype audio output additions to HII. (using
pre-recorder
audio files)
- 6) Porting of some voice synthesizer to UEFI. (eliminating the
need
of audio files)

Beyond this, there are other things we should look at adding,
like
- EFI_AUDIO_INPUT_PROTOCOL.
- Audio input additions to HII.

It's a lot of work, but I accept the challenge.
It may take a long time, but it is possible.

I am still trying to find some time to finish the translation of
my
thesis to English.
I wrote everything in Portuguese because there was not material
about
UEFI to the Brazilian audience, and another target I have is to
show
companies that we have people that can work at this kind of
projects
in
Brazil, bringing this kind of development to south america. (Yes,
I
have
complicated target, but I like the challenge :) )

Thanks and Regards
Rafael R. Machado

Em qui, 19 de set de 2019 às 14:45, Laszlo Ersek
<lersek@...
escreveu:

On 09/18/19 19:57, Andrew Fish wrote:
Rafael,

Please let us know what you find out. I probably don''t have the
time
to help implement this feature, but I happy to help work on the
architecture and design for UEFI accessibility on the edk2
mailing
lists, and I can also represent what ever we come up with at the
UEFI
Spec Work Group.

It would be hard to have a UEFI mandate for accessibility, given
there is no guideline on how a User Interface (UI) works. If
accessibility requires some from of hardware abstraction, like
audio,
then we could likely get that into the UEFI Spec. What might be
possible is an EDK2 reference implementation of accessibility.
Maybe
we could use the reference implementation to write a UEFI white
paper
on design for accessibility? I there is an open source
implementation, and an official design guide this would make it
much
easier for advocacy groups to lobby for this feature.

I've got some experience with accessibility as the macOS EFI OS
Loader has a UI for the Full Disk Encryption password. If you
press
the power button quickly 3 times at the disk encryption password
prompt accessibility is enabled and Voice Over gets turned on.
You
then get localized voice prompts when you move between UI
elements.
Since this is the OS loader all the resources are stored on the
disk.
You quickly run into a range of challenges since, audio is hard,
abstracting audio is hard (what codec does firmware have to
support),
Audio files are not small and firmware is size constrained, the
need
to localize the audio responses causes even more size issues,
the
boot options are usually written by an OS installer so how would
firmware know what to call them?

I listed a lot of reasons it is hard but as Kennedy stated in
his
"We
choose to go to the Moon!" speech sometimes we chose to do
things
"not because they are easy, but because they are hard; because
that
goal will serve to organize and measure the best of our energies
and
skills, because that challenge is one that we are willing to
accept".
If we have a design that means we can break the problem up into
smaller parts, and maybe we can find people that have expertise
in
that part to build a chunk at a time. If we could implement the
prototype in OVMF that would show how it works, but run on
everyones
machines, so that would be really helpful for demos and design
review.
Somewhat related, in April there was a thread on virtio-dev that
suggests there is interest in a virtio-audio device model:

https://lists.oasis-open.org/archives/virtio-dev/201904/msg00049.html

It looks like the ACRN project already implements a
(non-standard,
as
of
now) virtio-audio device already:

https://lists.oasis-open.org/archives/virtio-dev/201907/msg00061.html

(This is all I can mention right now.)

Thanks
Laszlo


--
Signed,
Ethin D. Probst

--
Signed,
Ethin D. Probst
--
Signed,
Ethin D. Probst


Re: PKCS7 Authenticated Variable Enrollment

Wadhawan, Divneil R
 

Hi Guomin,

I have moved forward from the last discussion, and the tools work okay for RSA2048/SHA256.
Once I have the next update, I will notify it here.

Regards,
Divneil

-----Original Message-----
From: Jiang, Guomin <guomin.jiang@...>
Sent: Wednesday, May 27, 2020 4:34 PM
To: rfc@edk2.groups.io; Wadhawan, Divneil R <divneil.r.wadhawan@...>
Subject: RE: [edk2-rfc] PKCS7 Authenticated Variable Enrollment

I am sorry that I have not the use case, and I plan to investigating it after August.

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of
divneil.r.wadhawan@...
Sent: Wednesday, May 27, 2020 6:54 PM
To: Jiang, Guomin <guomin.jiang@...>; rfc@edk2.groups.io
Subject: Re: [edk2-rfc] PKCS7 Authenticated Variable Enrollment

Hi Goumin,

I had discussion internally, and I got hold off tools from:
https://git.kernel.org/pub/scm/linux/kernel/git/jejb/efitools.git.
It is generating the correct format as per
EFI_VARIABLE_AUTHENTICATION_2.

So, I thought of first validating RSA2048 Sign verification and it is failing.
I still have to figure out that. Do you have a working use case which
uses
PKCS7 format and PKCS7_verify works?

Regards,
Divneil


Re: SubRegionAuthLib RFC

Laszlo Ersek
 

On 06/25/20 21:25, Sukerkar, Amol N wrote:
Hi Laszlo,

Please see my comments below.

Thanks,
Amol

-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: Thursday, June 25, 2020 4:57 AM
To: rfc@edk2.groups.io; Mackay, Curtis A <curtis.a.mackay@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Wang, Jian J <jian.j.wang@...>; Yao, Jiewen <jiewen.yao@...>; Sukerkar, Amol N <amol.n.sukerkar@...>; Agrawal, Sachin <sachin.agrawal@...>
Subject: Re: [edk2-rfc] SubRegionAuthLib RFC

Hi,

On 06/19/20 18:55, Mackay, Curtis A wrote:
Friendly reminder to review and provide feedback on the RFC. We are looking to target a WW34 deadline with this design.
I don't have a stake in this feature, but because the presentation isn't long, I've briefly skimmed it:

- It's unclear whether the public data structures describe a wire format. If they do, they should be packed, IMO.
[ANS] We tries to follow the precedence in other EDK2 implementation. Any suggestions to improve are welcome.
Well as far as I'm concerned it's a simple rule. If a structure is
serialized from RAM into a disk file, or to the network, then the
structure should be packed. If it's only exchanged in RAM, on a single
computer, between modules that all conform to the UEFI spec (which
dictates natural alignment for structure members), then fields in the
structure should not be packed.


- The struct hack (SUB_REGION_VERIFICATION.CertData being an array with
1 element) is not ideal, in my opinion. Especially because SUB_REGION_VERIFICATION is embedded in EFI_SUB_REGION_AUTHENTICATION afterwards. Personally I prefer comments to the struct hack that explain where the CertData starts. Out-of-bounds subscripts for CertData -- having just one element -- are dubious; we shouldn't have more of that (IMO).
[ANS] You can ignore SIGNED_SUB_REGION for now. The only intention here is to show that EFI_SUB_REGION_AUTHENTICATION is the header (with signature) and void *blob is the payload. So, even if you consider SUB_REGION_VERIFICATION as a member of EFI_SUB_REGION_AUTHENTICATION struct, CertData is still at the bottom of the struct stack. And depending on the type of signature the size will vary. But due to the nature of signature as PKCS#7, openssl API will be able to parse it. How do you propose we improve the struct?
The struct hack is not desirable because a snippet like

SUB_REGION_VERIFICATION *Verification;
UINT8 Value;

Verification = ...;
Value = Verification->CertData[1];

is undefined behavior. The compiler knows that CertData has 1 element,
IOW that subscript 1 is out of bounds for the array. Compilers get ever
more aggressive in exploiting undefined behavior, so we should not
introduce more of it.

Simply remove the CertData field from the structure, and replace it with
a comment that states (in natural language) more or less the same. For
an example, refer to the "Description", "FilePathList" and
"OptionalData" fields in EFI_LOAD_OPTION, in file
"MdePkg/Include/Uefi/UefiSpec.h". (Or, in the matching part of the UEFI
2.8 spec itself -- "3.1.3 Load Options".)


- it's unclear whether EFI_SUB_REGION_AUTHENTICATION.Name is supposed to be NUL-terminated, or not.
[ANS] Name is stored in a bounded array. It uses memcmp to validate which is limited to MAX_NAME bytes. If it exceeds and corrupts VendorGuid, the authentication will fail since VendorGuid is part of data over which signature is calculated.
That makes sense, but a comment should document it.


- "char" is not a good element type for
EFI_SUB_REGION_AUTHENTICATION.Name. If we want it to be "text", then it should either be CHAR8 or CHAR16. If we want it to be any binary string, then it should be UINT8.
[ANS] Agree.

- the EFI_ prefix is used inconsistently on the type names (not sure if that's intentional -- I can't tell the principle behind the current use)
[ANS] Could you suggest how we can make it compliant here?
My point was *not* whether the EFI_ prefix should be used, or should not
be used, for the structures introduced for this feature.

Instead, my point was that either *all* of the new structures should use
_EFI, or *none* should.

- SUB_REGION_HEADER: no EFI_ prefix
- SUB_REGION_VERIFICATION: no EFI_ prefix
- EFI_SUB_REGION_AUTHENTICATION: yes EFI_ prefix
- SIGNED_SUB_REGION: no EFI_ prefix

My suggestion is to either prepend EFI_ to SUB_REGION_HEADER,
SUB_REGION_VERIFICATION, and SIGNED_SUB_REGION, *or* to strip the prefix
from EFI_SUB_REGION_AUTHENTICATION. Whichever you choose is up to
whether this feature is going to be standardized. But, either way, the
prefix usage should be consistent.


- in SIGNED_SUB_REGION, the field names are not aligned with each other, and edk2 uses VOID spelling rather than "void". Also "blob" should be CamelCased as "Blob".
[ANS] Agree. Needs to change.

- The size of the entire region is not encoded (in a generic way,
anyway) in the structures. Is this intentional perhaps?
[ANS] SUB_REGION_HEADER.Length = sizeof(SUB_REGION_HEADER) + sizeof (CertData). Do you think anything else needs to be done?
I have three concerns with this:

(1) The expression you provided does not take into account any possible
padding between the Hdr field and the CertData field. (See also my note
on packing.) So minimally, we should say

Verification.Hdr.Length = (OFFSET_OF (
SUB_REGION_VERIFICATION,
CertData
) +
CertDataBytes);

(2) Are we sure a UINT16 length field can cover all CertData fields
encountered in practice? Will there ever be a CertData that's larger
than ~64KB?

(3) But, my question actually relates to the size of the Blob field.
SIGNED_SUB_REGION expresses the base address of the region, but not the
size thereof. Is that intentional?

- in the AuthenticateSubRegion() prototype, SignedSubRegionSize has type UINTN, but (*SubRegionDataSize) has type UINT32. I think these types should be the same.
[ANS] Agree. Needs to change.

- same for SignedSubRegionImage and (*SubRegionDataBuffer): (UINT8*) vs.
(VOID*)
[ANS] Agree.

- the ownership of the output (*SubRegionDataBuffer) is unclear. Is it a pointer into the input buffer? Is it a copy, allocated dynamically?
[ANS] it is a pointer to original sub-region data (without signature header). If authentication is unsuccessful, it will be NULL. But, I agree AuthenticateSubRegion should just authenticate the sub-region. Parsing the data should be left to the caller.
I think the general idea behind the current prototype is fine -- take an
input (base, size) pair, and adjust both components of that pair on
output. That's OK, in my opinion.

It's just that the caller needs to know that the output pointer points
into the same original blob (in other words, that on success,
(*SubRegionDataBuffer) *aliases* SignedSubRegionImage).

This is relevant to avoid memory management errors (double freeing of
buffers, or memory leaks). Every function ever that outputs a pointer,
needs to explain in a comment to the caller, who is responsible for
freeing the pointed-to object.

Thanks
Laszlo


Re: SubRegionAuthLib RFC

Sean
 

Curtis,

Not sure I fully follow your proposal. Can you provide more on the use case? Is the "blob" a FV or is the signed_sub_region a raw section in a ffs file? or something else like binary at flash offset?

The PI spec has a filesystem that describes many options and the DxeCore has support for security validation / authentication state flags associated with FVs and FFS files.

I have also seen many designs that leverage section extraction and doing authentication thru that guided sections.

Can you provide more background as to why it is important to get this into edk2 as a "standard" and why it requires defining new structures and new library abstractions?

Finally a point on the policy. In many products (especially commercial products) you don't see "UEFI secure boot (PK/KEK/DB/DBX)" leveraged for trust prior to EndOfDxe. Since UEFI secure boot is often user controlled this opens up your "platform" to compromise that can be impossible to recover from.

Thanks
Sean

On 6/16/2020 10:50 AM, Mackay, Curtis A wrote:
Hi,
I filed a proposal for a new library to handle UEFI BIOS sub-regions at https://bugzilla.tianocore.org/show_bug.cgi?id=2808. Attached is a slide deck with design overview of the new library.
A UEFI BIOS sub-region is an independent signed FV that can be updated independent of UEFI BIOS on flash and is part of a pre-allocated region on flash that is visible to UEFI BIOS.
The primary use-cases for such a region would be to store independently updateable firmware and large IP configuration data files to be consumed by BIOS.
To maintain the integrity of the BIOS sub-region, this ticket proposes a mechanism that:
- Leverages UEFI Secure Boot to authenticate the BIOS sub-region
- Supports PKCS#7 standard as signing/authentication mechanism to maintain the integrity of sub-region in PEI, DXE or BDS Phase.
Please provide feedback and comments on the design.
Best regards,
Curtis Mackay


Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules

Brian J. Johnson
 

Thanks for the link.  I agree that this change will make the StatusCodeHandler driver more modular, and is a step in the right direction.

But I think it could go further, with almost no additional work, and simplify the overall Status Code mechanism, not just the StatusCodeHandler driver.  Currently, the StatusCodeHandler driver entry routines run some initialization code, register callbacks (eg. for ExitBootServices and SetVirtualAddressMap), and call the RscHandler PPI/Protocol to register the worker routines.  If I'm understanding the proposal correctly, all that code will be moved to the individual NULL libraries, since any particular library may or may not need any of it.  Then the StatusCodeHandler modules will be left with no code of their own at all:  they will only be wrappers for the NULL libraries. Their entry routines will do nothing except return EFI_SUCCESS! (1)

It seems strange and wasteful to keep around empty modules like this.  So I'm suggesting adding the NULL libraries to the StatusCodeRouter modules instead.  They would need to export the protocol/PPI routines to the NULL libraries via a header file, so they could call them directly instead of looking up the protocol/PPI.  But that's a minor change.  Then you could remove the empty StatusCodeHandler modules entirely.  The advantage would be that there would be fewer modules in the build, simplifying the .dsc and .fdf files slightly.  It would also reduce code size a bit by sharing common library routines, such as BaseLib, with the StatusCodeRouter modules.

If those don't seem like worthwhile advantages, that's OK with me.  I don't want to belabor the point, or impede progress.  If others are OK with the proposal as it stands, then I am too.

Thanks,

*Brian J. Johnson
*Enterprise X86 Lab

Hewlett Packard Enterprise


(1) The StatusCodeHandlerRuntimeDxe driver also handles PcdStatusCodeReplayIn as part of its entry code.  That code would probably have to stay in a separate module rather than being linked to StatusCodeRouter as a NULL library.  That way it could be dispatched after the ReportStatusCode protocol is available.

------------------------------------------------------------------------
*From:* Dong, Eric [mailto:eric.dong@...]
*Sent:* Thursday, June 25, 2020, 10:41 AM
*To:* Brian J. Johnson <brian.johnson@...>, Bi, Dandan <dandan.bi@...>, Andrew Fish <afish@...>, edk2-devel-groups-io <devel@edk2.groups.io>
*Cc:* rfc@edk2.groups.io <rfc@edk2.groups.io>, Ni, Ray <ray.ni@...>, Wang, Jian J <jian.j.wang@...>, Wu, Hao A <hao.a.wu@...>, Tan, Ming <ming.tan@...>, Laszlo Ersek <lersek@...>
*Subject:* [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules

Hi Brian,

In this new design, we still use register status code handler Protocol/Ppi to register the handler logic. We just want to change the StatusCodeHandler driver. We try to split the register logic to NULL library to make the code more modularity. We already created sample library in Edk2-Platforms repo https://github.com/tianocore/edk2-platforms/tree/master/Features/Intel/Debugging/PostCodeDebugFeaturePkg/Library/PostCodeStatusCodeHandlerLib. You can check this code to understand more about what we want to do.

Thanks,

Eric

*From:* Brian J. Johnson <brian.johnson@...>
*Sent:* Thursday, June 25, 2020 4:25 AM
*To:* Bi, Dandan <dandan.bi@...>; Andrew Fish <afish@...>; edk2-devel-groups-io <devel@edk2.groups.io>
*Cc:* rfc@edk2.groups.io; Dong, Eric <eric.dong@...>; Ni, Ray <ray.ni@...>; Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Tan, Ming <ming.tan@...>; Laszlo Ersek <lersek@...>
*Subject:* Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules

Dandan,

The Status Code Protocol/PPI is the high-level interface which is being implemented.  The ReportStatusCodeRouter module implements this in terms of the ReportStatusCodeHandler Protocol/PPI.  That allows multiple ReportStatusCodeHandler modules to be used at once, so they can each react to different types of status codes, or report them through multiple channels.  That sort of multiplexing seems like a useful feature.

Now we're considering adding a mechanism which allows registering status code handlers via NULL libraries, rather than via the protocol/PPI. That sounds like exactly what ReportStatusCodeRouter is intended for.  It wouldn't really change its scope, it would just make it more flexible.  Adding this feature via a StatusCodeHandler module wouldn't improve modularity, it would just add complexity.  As an OEM, adding a custom handler would look the same to me either way:  I would have to add the NULL class library to a MdeModulePkg driver's entry in my .dsc file.  It doesn't matter to me whether it's the ReportStatusCodeRouter or StatusCodeHandler module.  And if I can do it in ReportStatusCodeRouter, then I don't need to include any StatusCodeHandler modules in the build at all.  That saves code space and reduces the number of modules in the APRIORI list, which are both good things.

ReportStatusCodeRouterPei already has to track registered handlers in PEI when running from ROM (it uses a HOB.)  Tracking handlers from NULL libraries wouldn't be any harder.  In fact, it looks like it could just export the Register() function to the NULL libraries, and they could call it in their constructors.

I think using NULL libraries for status code handlers is a great idea.  I'd just like to take that opportunity to reduce the complexity of the overall status code stack while we're at it.

Thanks,

*Brian J. Johnson
*Enterprise X86 Lab

Hewlett Packard Enterprise

------------------------------------------------------------------------

*From:* Bi, Dandan [mailto:dandan.bi@...]

*Sent:* Monday, June 22, 2020, 2:27 AM

*To:* Andrew Fish <afish@...> <mailto:afish@...>, edk2-devel-groups-io <devel@edk2.groups.io> <mailto:devel@edk2.groups.io>, brian.johnson@... <mailto:brian.johnson@...> <brian.johnson@...> <mailto:brian.johnson@...>

*Cc:* rfc@edk2.groups.io <mailto:rfc@edk2.groups.io> <rfc@edk2.groups.io> <mailto:rfc@edk2.groups.io>, Dong, Eric <eric.dong@...> <mailto:eric.dong@...>, Ni, Ray <ray.ni@...> <mailto:ray.ni@...>, Wang, Jian J <jian.j.wang@...> <mailto:jian.j.wang@...>, Wu, Hao A <hao.a.wu@...> <mailto:hao.a.wu@...>, Tan, Ming <ming.tan@...> <mailto:ming.tan@...>, Laszlo Ersek <lersek@...> <mailto:lersek@...>, Bi, Dandan <dandan.bi@...> <mailto:dandan.bi@...>

*Subject:* [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules

Hi Brian,

Personally, I prefer to add the NULL class Library to
StatusCodeHandler modules.

1. I think we should make the functionality of each module clear
and separated. It may also be why we added
ReportStatusCodeRouter and StatusCodeHandler modules in edk2
repo before.

ReportStatusCodeRouter modules are responsible for producing
Status Code Protocol/PPI and Report Status Code Handler
Protocol/PPI, and StatusCodeHandler modules are responsible for
producing handlers (Handlers can be provided by NULL class
Libraries in this RFC).

 So, that’s why I don’t want to add the NULL class Library to
ReportStatusCodeRouter modules directly, which change the
functionality scope of existing modules.

2. I agree that we have a lot of layers of indirection now, but
what we may gain is the good modularity. And you also
mentioned that one or more StatusCodeHandler Modules may be
used. We also want to achieve that only the StatusCodeHandler
modules in MdeModulePkg can be used after this separation,
platform can only add its own handler Libs to meet its
requirement.

3. As Andrew mentioned below, if add the libraries to
ReportStatusCodeRouter, there will be some issue we need to
fix, which seems also make the code logic a little tricky to me.

Thanks,

Dandan

*From:* Andrew Fish <afish@...> <mailto:afish@...>
*Sent:* Saturday, June 20, 2020 2:04 AM
*To:* edk2-devel-groups-io <devel@edk2.groups.io>
<mailto:devel@edk2.groups.io>; brian.johnson@...
<mailto:brian.johnson@...>
*Cc:* Bi, Dandan <dandan.bi@...>
<mailto:dandan.bi@...>; rfc@edk2.groups.io
<mailto:rfc@edk2.groups.io>; Dong, Eric <eric.dong@...>
<mailto:eric.dong@...>; Ni, Ray <ray.ni@...>
<mailto:ray.ni@...>; Wang, Jian J <jian.j.wang@...>
<mailto:jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>
<mailto:hao.a.wu@...>; Tan, Ming <ming.tan@...>
<mailto:ming.tan@...>
*Subject:* Re: [edk2-devel] [edk2-rfc]
MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for
Memory and serial handlers from
MdeModulePkg/Universal/StatusCodeHandler modules




On Jun 19, 2020, at 10:29 AM, Brian J. Johnson
<brian.johnson@... <mailto:brian.johnson@...>wrote:

On 6/18/20 2:01 AM, Dandan Bi wrote:

Hi All,

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2816
<https://bugzilla.tianocore.org/show_bug.cgi?id=2816>

We plan to separate two kinds of NULL class libraries for
Memory and serial handlers
from*MdeModulePkg/Universal/StatusCodeHandler/…/
StatusCodeHandlerPei/RuntimeDxe/Smm*modules.

The benefit we want to gain from this separation is to 1)
make the code clear and easy to maintain, 2) make platform
flexible to choose any handler library they need, and it
also can reduce image size since the unused handlers can
be excluded.

If you have any concern or comments for this separation,
please let me know.

We plan to add new separated NULL class
library*MemoryStausCodeHandlerLib*and***SerialStatusCodeHandlerLib*with
different phase implementation
into*MdeModulePkg\Library\*directory.

The main tree structure may like below:

MdeModulePkg\Library

|------*MemoryStausCodeHandlerLib*

|------|------ PeiMemoryStausCodeHandlerLib.inf

|------|------ RuntimeDxeMemoryStatusCodeHandlerLib.inf

|------|------ SmmMemoryStausCodeHandlerLib.inf

|------*SerialStatusCodeHandlerLib*

|------|------ PeiSerialStatusCodeHandlerLib.inf

|------|------ RuntimeDxeSerialStatusCodeHandlerLib.inf

|------|------ SmmSerialStatusCodeHandlerLib.inf

**

**

We will update existing platform use cases in edk2 and
edk2-platform repo to cover the new NULL class library to
make sure this change doesn’t impact any platform.

After this separation, StatusCodeHandler module usage will
like below, and it’s also very flexible for platform to
cover more handler libraries to meet their requirements.

MdeModulePkg/Universal/StatusCodeHandler/Pei/StatusCodeHandlerPei.inf
{

<LibraryClasses>

NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/PeiMemoryStausCodeHandlerLib.inf

NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/PeiSerialStatusCodeHandlerLib.inf



}

MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf
{

<LibraryClasses>

NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/RuntimeDxeMemoryStausCodeHandlerLib.inf

NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/RuntimeDxeSerialStatusCodeHandlerLib.inf



}

MdeModulePkg/Universal/StatusCodeHandler/Smm/StatusCodeHandlerSmm.inf
{

<LibraryClasses>

 NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/SmmMemoryStausCodeHandlerLib.inf

NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/SmmSerialStatusCodeHandlerLib.inf



}

Thanks,

Dandan

Dandan,

We'll have a lot of layers of indirection....  The
ReportStatusCodeRouter modules will call one or more
StatusCodeHandlerModules, and the standard StatusCodeHandler
modules will call multiple StatusCodeHandlerLib libraries.

How about adding StatusCodeHandlerLib support directly to the
ReportStatusCodeRouter modules? Then platforms could omit the
StatusCodeHandler modules if they're only using the
open-source code.  That sounds like less overhead since fewer
modules would be needed.

I think the need to execute from ROM makes this tricky.

It looks to me that it is easy to move from PCD to libs for the
StatusCodeHandler since registration is basically
`RscHandlerPpi->Register (SerialStatusCodeReportWorker);`. The
issue I see is the ReportStatusCodeRouter publishes RscHandlerPpi
after the PEIMs constructor has been called and if the PEIM. Given
globals don’t work when running from ROM you would have to do
something like publish a HOB in the library constructor and then
have the GenericStatusCodePeiEntry() walk the HOBs and install the
handlers. So I guess it is a little easier than I 1st thought when
I started writing this mail, but it would require a new public API.

Thanks,

Andrew Fish


Thanks,

--

*Brian J. Johnson
*Enterprise X86 Lab

Hewlett Packard Enterprise

*hpe.com* <x-msg://64/3D%22hpe.com%22>


-


Re: SubRegionAuthLib RFC

Sukerkar, Amol N
 

Hi Laszlo,

Please see my comments below.

Thanks,
Amol

-----Original Message-----
From: Laszlo Ersek <lersek@...>
Sent: Thursday, June 25, 2020 4:57 AM
To: rfc@edk2.groups.io; Mackay, Curtis A <curtis.a.mackay@...>
Cc: Kinney, Michael D <michael.d.kinney@...>; Wang, Jian J <jian.j.wang@...>; Yao, Jiewen <jiewen.yao@...>; Sukerkar, Amol N <amol.n.sukerkar@...>; Agrawal, Sachin <sachin.agrawal@...>
Subject: Re: [edk2-rfc] SubRegionAuthLib RFC

Hi,

On 06/19/20 18:55, Mackay, Curtis A wrote:
Friendly reminder to review and provide feedback on the RFC. We are looking to target a WW34 deadline with this design.
I don't have a stake in this feature, but because the presentation isn't long, I've briefly skimmed it:

- It's unclear whether the public data structures describe a wire format. If they do, they should be packed, IMO.
[ANS] We tries to follow the precedence in other EDK2 implementation. Any suggestions to improve are welcome.

- The struct hack (SUB_REGION_VERIFICATION.CertData being an array with
1 element) is not ideal, in my opinion. Especially because SUB_REGION_VERIFICATION is embedded in EFI_SUB_REGION_AUTHENTICATION afterwards. Personally I prefer comments to the struct hack that explain where the CertData starts. Out-of-bounds subscripts for CertData -- having just one element -- are dubious; we shouldn't have more of that (IMO).
[ANS] You can ignore SIGNED_SUB_REGION for now. The only intention here is to show that EFI_SUB_REGION_AUTHENTICATION is the header (with signature) and void *blob is the payload. So, even if you consider SUB_REGION_VERIFICATION as a member of EFI_SUB_REGION_AUTHENTICATION struct, CertData is still at the bottom of the struct stack. And depending on the type of signature the size will vary. But due to the nature of signature as PKCS#7, openssl API will be able to parse it. How do you propose we improve the struct?

- it's unclear whether EFI_SUB_REGION_AUTHENTICATION.Name is supposed to be NUL-terminated, or not.
[ANS] Name is stored in a bounded array. It uses memcmp to validate which is limited to MAX_NAME bytes. If it exceeds and corrupts VendorGuid, the authentication will fail since VendorGuid is part of data over which signature is calculated.

- "char" is not a good element type for
EFI_SUB_REGION_AUTHENTICATION.Name. If we want it to be "text", then it should either be CHAR8 or CHAR16. If we want it to be any binary string, then it should be UINT8.
[ANS] Agree.

- the EFI_ prefix is used inconsistently on the type names (not sure if that's intentional -- I can't tell the principle behind the current use)
[ANS] Could you suggest how we can make it compliant here?

- in SIGNED_SUB_REGION, the field names are not aligned with each other, and edk2 uses VOID spelling rather than "void". Also "blob" should be CamelCased as "Blob".
[ANS] Agree. Needs to change.

- The size of the entire region is not encoded (in a generic way,
anyway) in the structures. Is this intentional perhaps?
[ANS] SUB_REGION_HEADER.Length = sizeof(SUB_REGION_HEADER) + sizeof (CertData). Do you think anything else needs to be done?


- in the AuthenticateSubRegion() prototype, SignedSubRegionSize has type UINTN, but (*SubRegionDataSize) has type UINT32. I think these types should be the same.
[ANS] Agree. Needs to change.

- same for SignedSubRegionImage and (*SubRegionDataBuffer): (UINT8*) vs.
(VOID*)
[ANS] Agree.

- the ownership of the output (*SubRegionDataBuffer) is unclear. Is it a pointer into the input buffer? Is it a copy, allocated dynamically?
[ANS] it is a pointer to original sub-region data (without signature header). If authentication is unsuccessful, it will be NULL. But, I agree AuthenticateSubRegion should just authenticate the sub-region. Parsing the data should be left to the caller.


I have no higher-level comments (i.e. on the intended use case).

Thanks
Laszlo


From: Mackay, Curtis A
Sent: Tuesday, June 16, 2020 10:51 AM
To: 'rfc@edk2.groups.io' <rfc@edk2.groups.io>
Cc: Kinney, Michael D <michael.d.kinney@...>; Wang, Jian J
<jian.j.wang@...>; Yao, Jiewen <jiewen.yao@...>; Sukerkar,
Amol N <amol.n.sukerkar@...>; Agrawal, Sachin
<sachin.agrawal@...>
Subject: SubRegionAuthLib RFC

Hi,

I filed a proposal for a new library to handle UEFI BIOS sub-regions at https://bugzilla.tianocore.org/show_bug.cgi?id=2808. Attached is a slide deck with design overview of the new library.

A UEFI BIOS sub-region is an independent signed FV that can be updated independent of UEFI BIOS on flash and is part of a pre-allocated region on flash that is visible to UEFI BIOS.
The primary use-cases for such a region would be to store independently updateable firmware and large IP configuration data files to be consumed by BIOS.

To maintain the integrity of the BIOS sub-region, this ticket proposes a mechanism that:
- Leverages UEFI Secure Boot to authenticate the BIOS sub-region
- Supports PKCS#7 standard as signing/authentication mechanism to maintain the integrity of sub-region in PEI, DXE or BDS Phase.

Please provide feedback and comments on the design.

Best regards,
Curtis Mackay




Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules

Dong, Eric <eric.dong@...>
 

Hi Brian,

In this new design, we still use register status code handler Protocol/Ppi to register the handler logic. We just want to change the StatusCodeHandler driver. We try to split the register logic to NULL library to make the code more modularity. We already created sample library in Edk2-Platforms repo https://github.com/tianocore/edk2-platforms/tree/master/Features/Intel/Debugging/PostCodeDebugFeaturePkg/Library/PostCodeStatusCodeHandlerLib. You can check this code to understand more about what we want to do.

Thanks,
Eric
From: Brian J. Johnson <brian.johnson@...>
Sent: Thursday, June 25, 2020 4:25 AM
To: Bi, Dandan <dandan.bi@...>; Andrew Fish <afish@...>; edk2-devel-groups-io <devel@edk2.groups.io>
Cc: rfc@edk2.groups.io; Dong, Eric <eric.dong@...>; Ni, Ray <ray.ni@...>; Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Tan, Ming <ming.tan@...>; Laszlo Ersek <lersek@...>
Subject: Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules

Dandan,

The Status Code Protocol/PPI is the high-level interface which is being implemented. The ReportStatusCodeRouter module implements this in terms of the ReportStatusCodeHandler Protocol/PPI. That allows multiple ReportStatusCodeHandler modules to be used at once, so they can each react to different types of status codes, or report them through multiple channels. That sort of multiplexing seems like a useful feature.

Now we're considering adding a mechanism which allows registering status code handlers via NULL libraries, rather than via the protocol/PPI. That sounds like exactly what ReportStatusCodeRouter is intended for. It wouldn't really change its scope, it would just make it more flexible. Adding this feature via a StatusCodeHandler module wouldn't improve modularity, it would just add complexity. As an OEM, adding a custom handler would look the same to me either way: I would have to add the NULL class library to a MdeModulePkg driver's entry in my .dsc file. It doesn't matter to me whether it's the ReportStatusCodeRouter or StatusCodeHandler module. And if I can do it in ReportStatusCodeRouter, then I don't need to include any StatusCodeHandler modules in the build at all. That saves code space and reduces the number of modules in the APRIORI list, which are both good things.

ReportStatusCodeRouterPei already has to track registered handlers in PEI when running from ROM (it uses a HOB.) Tracking handlers from NULL libraries wouldn't be any harder. In fact, it looks like it could just export the Register() function to the NULL libraries, and they could call it in their constructors.

I think using NULL libraries for status code handlers is a great idea. I'd just like to take that opportunity to reduce the complexity of the overall status code stack while we're at it.

Thanks,

Brian J. Johnson
Enterprise X86 Lab

Hewlett Packard Enterprise

________________________________
From: Bi, Dandan [mailto:dandan.bi@...]
Sent: Monday, June 22, 2020, 2:27 AM
To: Andrew Fish <afish@...><mailto:afish@...>, edk2-devel-groups-io <devel@edk2.groups.io><mailto:devel@edk2.groups.io>, brian.johnson@...<mailto:brian.johnson@...> <brian.johnson@...><mailto:brian.johnson@...>
Cc: rfc@edk2.groups.io<mailto:rfc@edk2.groups.io> <rfc@edk2.groups.io><mailto:rfc@edk2.groups.io>, Dong, Eric <eric.dong@...><mailto:eric.dong@...>, Ni, Ray <ray.ni@...><mailto:ray.ni@...>, Wang, Jian J <jian.j.wang@...><mailto:jian.j.wang@...>, Wu, Hao A <hao.a.wu@...><mailto:hao.a.wu@...>, Tan, Ming <ming.tan@...><mailto:ming.tan@...>, Laszlo Ersek <lersek@...><mailto:lersek@...>, Bi, Dandan <dandan.bi@...><mailto:dandan.bi@...>
Subject: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules

Hi Brian,

Personally, I prefer to add the NULL class Library to StatusCodeHandler modules.

1. I think we should make the functionality of each module clear and separated. It may also be why we added ReportStatusCodeRouter and StatusCodeHandler modules in edk2 repo before.

ReportStatusCodeRouter modules are responsible for producing Status Code Protocol/PPI and Report Status Code Handler Protocol/PPI, and StatusCodeHandler modules are responsible for producing handlers (Handlers can be provided by NULL class Libraries in this RFC).

So, that’s why I don’t want to add the NULL class Library to ReportStatusCodeRouter modules directly, which change the functionality scope of existing modules.



1. I agree that we have a lot of layers of indirection now, but what we may gain is the good modularity. And you also mentioned that one or more StatusCodeHandler Modules may be used. We also want to achieve that only the StatusCodeHandler modules in MdeModulePkg can be used after this separation, platform can only add its own handler Libs to meet its requirement.



1. As Andrew mentioned below, if add the libraries to ReportStatusCodeRouter, there will be some issue we need to fix, which seems also make the code logic a little tricky to me.



Thanks,
Dandan
From: Andrew Fish <afish@...><mailto:afish@...>
Sent: Saturday, June 20, 2020 2:04 AM
To: edk2-devel-groups-io <devel@edk2.groups.io><mailto:devel@edk2.groups.io>; brian.johnson@...<mailto:brian.johnson@...>
Cc: Bi, Dandan <dandan.bi@...><mailto:dandan.bi@...>; rfc@edk2.groups.io<mailto:rfc@edk2.groups.io>; Dong, Eric <eric.dong@...><mailto:eric.dong@...>; Ni, Ray <ray.ni@...><mailto:ray.ni@...>; Wang, Jian J <jian.j.wang@...><mailto:jian.j.wang@...>; Wu, Hao A <hao.a.wu@...><mailto:hao.a.wu@...>; Tan, Ming <ming.tan@...><mailto:ming.tan@...>
Subject: Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules





On Jun 19, 2020, at 10:29 AM, Brian J. Johnson <brian.johnson@...<mailto:brian.johnson@...>wrote:

On 6/18/20 2:01 AM, Dandan Bi wrote:
Hi All,

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2816

We plan to separate two kinds of NULL class libraries for Memory and serial handlers fromMdeModulePkg/Universal/StatusCodeHandler/…/ StatusCodeHandlerPei/RuntimeDxe/Smm modules.
The benefit we want to gain from this separation is to 1) make the code clear and easy to maintain, 2) make platform flexible to choose any handler library they need, and it also can reduce image size since the unused handlers can be excluded.
If you have any concern or comments for this separation, please let me know.

We plan to add new separated NULL class library MemoryStausCodeHandlerLib and SerialStatusCodeHandlerLib with different phase implementation into MdeModulePkg\Library\ directory.
The main tree structure may like below:
MdeModulePkg\Library
|------MemoryStausCodeHandlerLib
|------|------ PeiMemoryStausCodeHandlerLib.inf
|------|------ RuntimeDxeMemoryStatusCodeHandlerLib.inf
|------|------ SmmMemoryStausCodeHandlerLib.inf
|------SerialStatusCodeHandlerLib
|------|------ PeiSerialStatusCodeHandlerLib.inf
|------|------ RuntimeDxeSerialStatusCodeHandlerLib.inf
|------|------ SmmSerialStatusCodeHandlerLib.inf


We will update existing platform use cases in edk2 and edk2-platform repo to cover the new NULL class library to make sure this change doesn’t impact any platform.
After this separation, StatusCodeHandler module usage will like below, and it’s also very flexible for platform to cover more handler libraries to meet their requirements.
MdeModulePkg/Universal/StatusCodeHandler/Pei/StatusCodeHandlerPei.inf {
<LibraryClasses>
NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/PeiMemoryStausCodeHandlerLib.inf
NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/PeiSerialStatusCodeHandlerLib.inf

}

MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf {
<LibraryClasses>
NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/RuntimeDxeMemoryStausCodeHandlerLib.inf
NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/RuntimeDxeSerialStatusCodeHandlerLib.inf

}

MdeModulePkg/Universal/StatusCodeHandler/Smm/StatusCodeHandlerSmm.inf {
<LibraryClasses>
NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/SmmMemoryStausCodeHandlerLib.inf
NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/SmmSerialStatusCodeHandlerLib.inf

}


Thanks,
Dandan

Dandan,
We'll have a lot of layers of indirection.... The ReportStatusCodeRouter modules will call one or more StatusCodeHandlerModules, and the standard StatusCodeHandler modules will call multiple StatusCodeHandlerLib libraries.
How about adding StatusCodeHandlerLib support directly to the ReportStatusCodeRouter modules? Then platforms could omit the StatusCodeHandler modules if they're only using the open-source code. That sounds like less overhead since fewer modules would be needed.


I think the need to execute from ROM makes this tricky.

It looks to me that it is easy to move from PCD to libs for the StatusCodeHandler since registration is basically `RscHandlerPpi->Register (SerialStatusCodeReportWorker);`. The issue I see is the ReportStatusCodeRouter publishes RscHandlerPpi after the PEIMs constructor has been called and if the PEIM. Given globals don’t work when running from ROM you would have to do something like publish a HOB in the library constructor and then have the GenericStatusCodePeiEntry() walk the HOBs and install the handlers. So I guess it is a little easier than I 1st thought when I started writing this mail, but it would require a new public API.

Thanks,

Andrew Fish


Thanks,
--

Brian J. Johnson
Enterprise X86 Lab

Hewlett Packard Enterprise

hpe.com<x-msg://64/3D%22hpe.com%22>



-


Re: SubRegionAuthLib RFC

Laszlo Ersek
 

Hi,

On 06/19/20 18:55, Mackay, Curtis A wrote:
Friendly reminder to review and provide feedback on the RFC. We are looking to target a WW34 deadline with this design.
I don't have a stake in this feature, but because the presentation isn't
long, I've briefly skimmed it:

- It's unclear whether the public data structures describe a wire
format. If they do, they should be packed, IMO.

- The struct hack (SUB_REGION_VERIFICATION.CertData being an array with
1 element) is not ideal, in my opinion. Especially because
SUB_REGION_VERIFICATION is embedded in EFI_SUB_REGION_AUTHENTICATION
afterwards. Personally I prefer comments to the struct hack that explain
where the CertData starts. Out-of-bounds subscripts for CertData --
having just one element -- are dubious; we shouldn't have more of that
(IMO).

- it's unclear whether EFI_SUB_REGION_AUTHENTICATION.Name is supposed to
be NUL-terminated, or not.

- "char" is not a good element type for
EFI_SUB_REGION_AUTHENTICATION.Name. If we want it to be "text", then it
should either be CHAR8 or CHAR16. If we want it to be any binary string,
then it should be UINT8.

- the EFI_ prefix is used inconsistently on the type names (not sure if
that's intentional -- I can't tell the principle behind the current use)

- in SIGNED_SUB_REGION, the field names are not aligned with each other,
and edk2 uses VOID spelling rather than "void". Also "blob" should be
CamelCased as "Blob".

- The size of the entire region is not encoded (in a generic way,
anyway) in the structures. Is this intentional perhaps?


- in the AuthenticateSubRegion() prototype, SignedSubRegionSize has type
UINTN, but (*SubRegionDataSize) has type UINT32. I think these types
should be the same.

- same for SignedSubRegionImage and (*SubRegionDataBuffer): (UINT8*) vs.
(VOID*)

- the ownership of the output (*SubRegionDataBuffer) is unclear. Is it a
pointer into the input buffer? Is it a copy, allocated dynamically?


I have no higher-level comments (i.e. on the intended use case).

Thanks
Laszlo


From: Mackay, Curtis A
Sent: Tuesday, June 16, 2020 10:51 AM
To: 'rfc@edk2.groups.io' <rfc@edk2.groups.io>
Cc: Kinney, Michael D <michael.d.kinney@...>; Wang, Jian J <jian.j.wang@...>; Yao, Jiewen <jiewen.yao@...>; Sukerkar, Amol N <amol.n.sukerkar@...>; Agrawal, Sachin <sachin.agrawal@...>
Subject: SubRegionAuthLib RFC

Hi,

I filed a proposal for a new library to handle UEFI BIOS sub-regions at https://bugzilla.tianocore.org/show_bug.cgi?id=2808. Attached is a slide deck with design overview of the new library.

A UEFI BIOS sub-region is an independent signed FV that can be updated independent of UEFI BIOS on flash and is part of a pre-allocated region on flash that is visible to UEFI BIOS.
The primary use-cases for such a region would be to store independently updateable firmware and large IP configuration data files to be consumed by BIOS.

To maintain the integrity of the BIOS sub-region, this ticket proposes a mechanism that:
- Leverages UEFI Secure Boot to authenticate the BIOS sub-region
- Supports PKCS#7 standard as signing/authentication mechanism to maintain the integrity of sub-region in PEI, DXE or BDS Phase.

Please provide feedback and comments on the design.

Best regards,
Curtis Mackay




Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules

Brian J. Johnson
 

Dandan,

The Status Code Protocol/PPI is the high-level interface which is being implemented.  The ReportStatusCodeRouter module implements this in terms of the ReportStatusCodeHandler Protocol/PPI.  That allows multiple ReportStatusCodeHandler modules to be used at once, so they can each react to different types of status codes, or report them through multiple channels.  That sort of multiplexing seems like a useful feature.

Now we're considering adding a mechanism which allows registering status code handlers via NULL libraries, rather than via the protocol/PPI.  That sounds like exactly what ReportStatusCodeRouter is intended for.  It wouldn't really change its scope, it would just make it more flexible.  Adding this feature via a StatusCodeHandler module wouldn't improve modularity, it would just add complexity.  As an OEM, adding a custom handler would look the same to me either way:  I would have to add the NULL class library to a MdeModulePkg driver's entry in my .dsc file.  It doesn't matter to me whether it's the ReportStatusCodeRouter or StatusCodeHandler module.  And if I can do it in ReportStatusCodeRouter, then I don't need to include any StatusCodeHandler modules in the build at all.  That saves code space and reduces the number of modules in the APRIORI list, which are both good things.

ReportStatusCodeRouterPei already has to track registered handlers in PEI when running from ROM (it uses a HOB.)  Tracking handlers from NULL libraries wouldn't be any harder.  In fact, it looks like it could just export the Register() function to the NULL libraries, and they could call it in their constructors.

I think using NULL libraries for status code handlers is a great idea.  I'd just like to take that opportunity to reduce the complexity of the overall status code stack while we're at it.

Thanks,

*Brian J. Johnson
*Enterprise X86 Lab

Hewlett Packard Enterprise

------------------------------------------------------------------------
*From:* Bi, Dandan [mailto:dandan.bi@...]
*Sent:* Monday, June 22, 2020, 2:27 AM
*To:* Andrew Fish <afish@...>, edk2-devel-groups-io <devel@edk2.groups.io>, brian.johnson@... <brian.johnson@...>
*Cc:* rfc@edk2.groups.io <rfc@edk2.groups.io>, Dong, Eric <eric.dong@...>, Ni, Ray <ray.ni@...>, Wang, Jian J <jian.j.wang@...>, Wu, Hao A <hao.a.wu@...>, Tan, Ming <ming.tan@...>, Laszlo Ersek <lersek@...>, Bi, Dandan <dandan.bi@...>
*Subject:* [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules

Hi Brian,

Personally, I prefer to add the NULL class Library to StatusCodeHandler modules.

1. I think we should make the functionality of each module clear and
separated. It may also be why we added ReportStatusCodeRouter and
StatusCodeHandler modules in edk2 repo before.

ReportStatusCodeRouter modules are responsible for producing Status Code Protocol/PPI and Report Status Code Handler Protocol/PPI, and StatusCodeHandler modules are responsible for producing handlers (Handlers can be provided by NULL class Libraries in this RFC).

 So, that’s why I don’t want to add the NULL class Library to ReportStatusCodeRouter modules directly, which change the functionality scope of existing modules.

2. I agree that we have a lot of layers of indirection now, but what
we may gain is the good modularity. And you also mentioned that
one or more StatusCodeHandler Modules may be used. We also want to
achieve that only the StatusCodeHandler modules in MdeModulePkg
can be used after this separation, platform can only add its own
handler Libs to meet its requirement.

3. As Andrew mentioned below, if add the libraries to
ReportStatusCodeRouter, there will be some issue we need to fix,
which seems also make the code logic a little tricky to me.

Thanks,

Dandan

*From:* Andrew Fish <afish@...>
*Sent:* Saturday, June 20, 2020 2:04 AM
*To:* edk2-devel-groups-io <devel@edk2.groups.io>; brian.johnson@...
*Cc:* Bi, Dandan <dandan.bi@...>; rfc@edk2.groups.io; Dong, Eric <eric.dong@...>; Ni, Ray <ray.ni@...>; Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Tan, Ming <ming.tan@...>
*Subject:* Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules



On Jun 19, 2020, at 10:29 AM, Brian J. Johnson
<brian.johnson@... <mailto:brian.johnson@...>wrote:

On 6/18/20 2:01 AM, Dandan Bi wrote:

Hi All,

REF:https://bugzilla.tianocore.org/show_bug.cgi?id=2816
<https://bugzilla.tianocore.org/show_bug.cgi?id=2816>

We plan to separate two kinds of NULL class libraries for
Memory and serial handlers
from*MdeModulePkg/Universal/StatusCodeHandler/…/
StatusCodeHandlerPei/RuntimeDxe/Smm*modules.

The benefit we want to gain from this separation is to 1) make
the code clear and easy to maintain, 2) make platform flexible
to choose any handler library they need, and it also can
reduce image size since the unused handlers can be excluded.

If you have any concern or comments for this separation,
please let me know.

We plan to add new separated NULL class
library*MemoryStausCodeHandlerLib*and***SerialStatusCodeHandlerLib*with
different phase implementation
into*MdeModulePkg\Library\*directory.

The main tree structure may like below:

MdeModulePkg\Library

|------*MemoryStausCodeHandlerLib*

|------|------ PeiMemoryStausCodeHandlerLib.inf

|------|------ RuntimeDxeMemoryStatusCodeHandlerLib.inf

|------|------ SmmMemoryStausCodeHandlerLib.inf

|------*SerialStatusCodeHandlerLib*

|------|------ PeiSerialStatusCodeHandlerLib.inf

|------|------ RuntimeDxeSerialStatusCodeHandlerLib.inf

|------|------ SmmSerialStatusCodeHandlerLib.inf

**

**

We will update existing platform use cases in edk2 and
edk2-platform repo to cover the new NULL class library to make
sure this change doesn’t impact any platform.

After this separation, StatusCodeHandler module usage will
like below, and it’s also very flexible for platform to cover
more handler libraries to meet their requirements.

MdeModulePkg/Universal/StatusCodeHandler/Pei/StatusCodeHandlerPei.inf
{

<LibraryClasses>

NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/PeiMemoryStausCodeHandlerLib.inf

NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/PeiSerialStatusCodeHandlerLib.inf



}

MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf
{

<LibraryClasses>

NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/RuntimeDxeMemoryStausCodeHandlerLib.inf

NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/RuntimeDxeSerialStatusCodeHandlerLib.inf



}

MdeModulePkg/Universal/StatusCodeHandler/Smm/StatusCodeHandlerSmm.inf
{

<LibraryClasses>

 NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/SmmMemoryStausCodeHandlerLib.inf

NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/SmmSerialStatusCodeHandlerLib.inf



}

Thanks,

Dandan

Dandan,

We'll have a lot of layers of indirection....  The
ReportStatusCodeRouter modules will call one or more
StatusCodeHandlerModules, and the standard StatusCodeHandler
modules will call multiple StatusCodeHandlerLib libraries.

How about adding StatusCodeHandlerLib support directly to the
ReportStatusCodeRouter modules?  Then platforms could omit the
StatusCodeHandler modules if they're only using the open-source
code.  That sounds like less overhead since fewer modules would be
needed.

I think the need to execute from ROM makes this tricky.

It looks to me that it is easy to move from PCD to libs for the StatusCodeHandler since registration is basically `RscHandlerPpi->Register (SerialStatusCodeReportWorker);`. The issue I see is the ReportStatusCodeRouter publishes RscHandlerPpi after the PEIMs constructor has been called and if the PEIM. Given globals don’t work when running from ROM you would have to do something like publish a HOB in the library constructor and then have the GenericStatusCodePeiEntry() walk the HOBs and install the handlers. So I guess it is a little easier than I 1st thought when I started writing this mail, but it would require a new public API.

Thanks,

Andrew Fish

Thanks,

--

*Brian J. Johnson
*Enterprise X86 Lab

Hewlett Packard Enterprise

*hpe.com* <x-msg://64/3D%22hpe.com%22>

-


Re: SubRegionAuthLib RFC

Mackay, Curtis A <curtis.a.mackay@...>
 

Friendly reminder to review and provide feedback on the RFC. We are looking to target a WW34 deadline with this design.

 

Best,

Curtis

 

From: Mackay, Curtis A
Sent: Tuesday, June 16, 2020 10:51 AM
To: 'rfc@edk2.groups.io' <rfc@edk2.groups.io>
Cc: Kinney, Michael D <michael.d.kinney@...>; Wang, Jian J <jian.j.wang@...>; Yao, Jiewen <jiewen.yao@...>; Sukerkar, Amol N <amol.n.sukerkar@...>; Agrawal, Sachin <sachin.agrawal@...>
Subject: SubRegionAuthLib RFC

 

Hi,

 

I filed a proposal for a new library to handle UEFI BIOS sub-regions at https://bugzilla.tianocore.org/show_bug.cgi?id=2808. Attached is a slide deck with design overview of the new library.

 

A UEFI BIOS sub-region is an independent signed FV that can be updated independent of UEFI BIOS on flash and is part of a pre-allocated region on flash that is visible to UEFI BIOS.

The primary use-cases for such a region would be to store independently updateable firmware and large IP configuration data files to be consumed by BIOS.

 

To maintain the integrity of the BIOS sub-region, this ticket proposes a mechanism that:

- Leverages UEFI Secure Boot to authenticate the BIOS sub-region

- Supports PKCS#7 standard as signing/authentication mechanism to maintain the integrity of sub-region in PEI, DXE or BDS Phase.

 

Please provide feedback and comments on the design.

 

Best regards,

Curtis Mackay


SubRegionAuthLib RFC

Mackay, Curtis A <curtis.a.mackay@...>
 

Hi,

 

I filed a proposal for a new library to handle UEFI BIOS sub-regions at https://bugzilla.tianocore.org/show_bug.cgi?id=2808. Attached is a slide deck with design overview of the new library.

 

A UEFI BIOS sub-region is an independent signed FV that can be updated independent of UEFI BIOS on flash and is part of a pre-allocated region on flash that is visible to UEFI BIOS.

The primary use-cases for such a region would be to store independently updateable firmware and large IP configuration data files to be consumed by BIOS.

 

To maintain the integrity of the BIOS sub-region, this ticket proposes a mechanism that:

- Leverages UEFI Secure Boot to authenticate the BIOS sub-region

- Supports PKCS#7 standard as signing/authentication mechanism to maintain the integrity of sub-region in PEI, DXE or BDS Phase.

 

Please provide feedback and comments on the design.

 

Best regards,

Curtis Mackay


Re: [RFC]: StandAloneMM in OP-TEE

Mayur Gudmeti
 

Hi Sahil,

Your proposal interests us too. I got a question for you. Are you planning to use or leverage SPCI library interfaces for interaction between OP-TEE and Standalone MM?


Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules

Laszlo Ersek
 

On 06/22/20 06:57, Bi, Dandan wrote:
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: Friday, June 19, 2020 8:48 PM
To: rfc@edk2.groups.io; Bi, Dandan <dandan.bi@...>;
devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@...>; Ni, Ray <ray.ni@...>; Wang,
Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Tan,
Ming <ming.tan@...>
Subject: Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler:
Separate NULL class libraries for Memory and serial handlers from
MdeModulePkg/Universal/StatusCodeHandler modules

On 06/18/20 09:01, Dandan Bi wrote:
Hi All,

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2816

We plan to separate two kinds of NULL class libraries for Memory and serial
handlers from MdeModulePkg/Universal/StatusCodeHandler/.../
StatusCodeHandlerPei/RuntimeDxe/Smm modules.
The benefit we want to gain from this separation is to 1) make the code
clear and easy to maintain, 2) make platform flexible to choose any handler
library they need, and it also can reduce image size since the unused
handlers can be excluded.
If you have any concern or comments for this separation, please let me
know.

We plan to add new separated NULL class library
MemoryStausCodeHandlerLib and SerialStatusCodeHandlerLib with different
phase implementation into MdeModulePkg\Library\ directory.
The main tree structure may like below:
MdeModulePkg\Library
|------MemoryStausCodeHandlerLib
|------|------ PeiMemoryStausCodeHandlerLib.inf
|------|------ RuntimeDxeMemoryStatusCodeHandlerLib.inf
|------|------ SmmMemoryStausCodeHandlerLib.inf
|------SerialStatusCodeHandlerLib
|------|------ PeiSerialStatusCodeHandlerLib.inf
|------|------ RuntimeDxeSerialStatusCodeHandlerLib.inf
|------|------ SmmSerialStatusCodeHandlerLib.inf


We will update existing platform use cases in edk2 and edk2-platform repo
to cover the new NULL class library to make sure this change doesn't impact
any platform.
After this separation, StatusCodeHandler module usage will like below, and
it's also very flexible for platform to cover more handler libraries to meet
their requirements.
MdeModulePkg/Universal/StatusCodeHandler/Pei/StatusCodeHandlerPei.in
f {
<LibraryClasses>
NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/PeiMemorySt
ausCode
NULL|HandlerLib.inf
NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/PeiSerialStatusC
o
NULL|deHandlerLib.inf
...
}

MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHan
dlerRuntimeDxe.inf {
<LibraryClasses>
NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/RuntimeDxeM
emorySt
NULL|ausCodeHandlerLib.inf
NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/RuntimeDxeSeri
alS
NULL|tatusCodeHandlerLib.inf
...
}

MdeModulePkg/Universal/StatusCodeHandler/Smm/StatusCodeHandlerSm
m.inf {
<LibraryClasses>

NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/SmmMemory
StausCode
HandlerLib.inf
NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/SmmSerialStatus
Co
NULL|deHandlerLib.inf
...
}
So I assume you're going to remove PcdStatusCodeUseSerial and
PcdStatusCodeUseMemory, and when converting the existent platforms,
the new NULL class resolutions in the DSC files will reflect the specific PCD
values used in those DSC files until then. Is that right?
Thanks for pointing out the PCD part which I miss in this RFC.
This commit https://github.com/tianocore/edk2/commit/45bc28172fbf38ac21e2592c07189b55f57695e3 have updated PcdStatusCodeUseSerial and PcdStatusCodeUseMemory type.
We plan to keep PcdStatusCodeUseSerial and PcdStatusCodeUseMemory. Through NULL class resolutions in the DSC can make the code handler code included or not, then we still can control handler enable/disable through the PCD dynamically if the handler is included.
What do you think of this?
Hm... OK.

Thanks
Laszlo


Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules

Dandan Bi <dandan.bi@...>
 

Hi Brian,

Personally, I prefer to add the NULL class Library to StatusCodeHandler modules.

1. I think we should make the functionality of each module clear and separated. It may also be why we added ReportStatusCodeRouter and StatusCodeHandler modules in edk2 repo before.

ReportStatusCodeRouter modules are responsible for producing Status Code Protocol/PPI and Report Status Code Handler Protocol/PPI, and StatusCodeHandler modules are responsible for producing handlers (Handlers can be provided by NULL class Libraries in this RFC).

So, that’s why I don’t want to add the NULL class Library to ReportStatusCodeRouter modules directly, which change the functionality scope of existing modules.



1. I agree that we have a lot of layers of indirection now, but what we may gain is the good modularity. And you also mentioned that one or more StatusCodeHandler Modules may be used. We also want to achieve that only the StatusCodeHandler modules in MdeModulePkg can be used after this separation, platform can only add its own handler Libs to meet its requirement.



1. As Andrew mentioned below, if add the libraries to ReportStatusCodeRouter, there will be some issue we need to fix, which seems also make the code logic a little tricky to me.



Thanks,
Dandan
From: Andrew Fish <afish@...>
Sent: Saturday, June 20, 2020 2:04 AM
To: edk2-devel-groups-io <devel@edk2.groups.io>; brian.johnson@...
Cc: Bi, Dandan <dandan.bi@...>; rfc@edk2.groups.io; Dong, Eric <eric.dong@...>; Ni, Ray <ray.ni@...>; Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Tan, Ming <ming.tan@...>
Subject: Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules




On Jun 19, 2020, at 10:29 AM, Brian J. Johnson <brian.johnson@...<mailto:brian.johnson@...>> wrote:

On 6/18/20 2:01 AM, Dandan Bi wrote:
Hi All,

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2816

We plan to separate two kinds of NULL class libraries for Memory and serial handlers fromMdeModulePkg/Universal/StatusCodeHandler/…/ StatusCodeHandlerPei/RuntimeDxe/Smm modules.
The benefit we want to gain from this separation is to 1) make the code clear and easy to maintain, 2) make platform flexible to choose any handler library they need, and it also can reduce image size since the unused handlers can be excluded.
If you have any concern or comments for this separation, please let me know.

We plan to add new separated NULL class library MemoryStausCodeHandlerLib and SerialStatusCodeHandlerLib with different phase implementation into MdeModulePkg\Library\ directory.
The main tree structure may like below:
MdeModulePkg\Library
|------MemoryStausCodeHandlerLib
|------|------ PeiMemoryStausCodeHandlerLib.inf
|------|------ RuntimeDxeMemoryStatusCodeHandlerLib.inf
|------|------ SmmMemoryStausCodeHandlerLib.inf
|------SerialStatusCodeHandlerLib
|------|------ PeiSerialStatusCodeHandlerLib.inf
|------|------ RuntimeDxeSerialStatusCodeHandlerLib.inf
|------|------ SmmSerialStatusCodeHandlerLib.inf


We will update existing platform use cases in edk2 and edk2-platform repo to cover the new NULL class library to make sure this change doesn’t impact any platform.
After this separation, StatusCodeHandler module usage will like below, and it’s also very flexible for platform to cover more handler libraries to meet their requirements.
MdeModulePkg/Universal/StatusCodeHandler/Pei/StatusCodeHandlerPei.inf {
<LibraryClasses>
NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/PeiMemoryStausCodeHandlerLib.inf
NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/PeiSerialStatusCodeHandlerLib.inf

}

MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf {
<LibraryClasses>
NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/RuntimeDxeMemoryStausCodeHandlerLib.inf
NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/RuntimeDxeSerialStatusCodeHandlerLib.inf

}

MdeModulePkg/Universal/StatusCodeHandler/Smm/StatusCodeHandlerSmm.inf {
<LibraryClasses>
NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/SmmMemoryStausCodeHandlerLib.inf
NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/SmmSerialStatusCodeHandlerLib.inf

}


Thanks,
Dandan

Dandan,
We'll have a lot of layers of indirection.... The ReportStatusCodeRouter modules will call one or more StatusCodeHandlerModules, and the standard StatusCodeHandler modules will call multiple StatusCodeHandlerLib libraries.
How about adding StatusCodeHandlerLib support directly to the ReportStatusCodeRouter modules? Then platforms could omit the StatusCodeHandler modules if they're only using the open-source code. That sounds like less overhead since fewer modules would be needed.


I think the need to execute from ROM makes this tricky.

It looks to me that it is easy to move from PCD to libs for the StatusCodeHandler since registration is basically `RscHandlerPpi->Register (SerialStatusCodeReportWorker);`. The issue I see is the ReportStatusCodeRouter publishes RscHandlerPpi after the PEIMs constructor has been called and if the PEIM. Given globals don’t work when running from ROM you would have to do something like publish a HOB in the library constructor and then have the GenericStatusCodePeiEntry() walk the HOBs and install the handlers. So I guess it is a little easier than I 1st thought when I started writing this mail, but it would require a new public API.

Thanks,

Andrew Fish

Thanks,
--

Brian J. Johnson
Enterprise X86 Lab

Hewlett Packard Enterprise

hpe.com<x-msg://64/3D%22hpe.com%22>


Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules

Dandan Bi <dandan.bi@...>
 

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
Ersek
Sent: Friday, June 19, 2020 8:48 PM
To: rfc@edk2.groups.io; Bi, Dandan <dandan.bi@...>;
devel@edk2.groups.io
Cc: Dong, Eric <eric.dong@...>; Ni, Ray <ray.ni@...>; Wang,
Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Tan,
Ming <ming.tan@...>
Subject: Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler:
Separate NULL class libraries for Memory and serial handlers from
MdeModulePkg/Universal/StatusCodeHandler modules

On 06/18/20 09:01, Dandan Bi wrote:
Hi All,

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2816

We plan to separate two kinds of NULL class libraries for Memory and serial
handlers from MdeModulePkg/Universal/StatusCodeHandler/.../
StatusCodeHandlerPei/RuntimeDxe/Smm modules.
The benefit we want to gain from this separation is to 1) make the code
clear and easy to maintain, 2) make platform flexible to choose any handler
library they need, and it also can reduce image size since the unused
handlers can be excluded.
If you have any concern or comments for this separation, please let me
know.

We plan to add new separated NULL class library
MemoryStausCodeHandlerLib and SerialStatusCodeHandlerLib with different
phase implementation into MdeModulePkg\Library\ directory.
The main tree structure may like below:
MdeModulePkg\Library
|------MemoryStausCodeHandlerLib
|------|------ PeiMemoryStausCodeHandlerLib.inf
|------|------ RuntimeDxeMemoryStatusCodeHandlerLib.inf
|------|------ SmmMemoryStausCodeHandlerLib.inf
|------SerialStatusCodeHandlerLib
|------|------ PeiSerialStatusCodeHandlerLib.inf
|------|------ RuntimeDxeSerialStatusCodeHandlerLib.inf
|------|------ SmmSerialStatusCodeHandlerLib.inf


We will update existing platform use cases in edk2 and edk2-platform repo
to cover the new NULL class library to make sure this change doesn't impact
any platform.
After this separation, StatusCodeHandler module usage will like below, and
it's also very flexible for platform to cover more handler libraries to meet
their requirements.
MdeModulePkg/Universal/StatusCodeHandler/Pei/StatusCodeHandlerPei.in
f {
<LibraryClasses>
NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/PeiMemorySt
ausCode
NULL|HandlerLib.inf
NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/PeiSerialStatusC
o
NULL|deHandlerLib.inf
...
}

MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHan
dlerRuntimeDxe.inf {
<LibraryClasses>
NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/RuntimeDxeM
emorySt
NULL|ausCodeHandlerLib.inf
NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/RuntimeDxeSeri
alS
NULL|tatusCodeHandlerLib.inf
...
}

MdeModulePkg/Universal/StatusCodeHandler/Smm/StatusCodeHandlerSm
m.inf {
<LibraryClasses>

NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/SmmMemory
StausCode
HandlerLib.inf
NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/SmmSerialStatus
Co
NULL|deHandlerLib.inf
...
}
So I assume you're going to remove PcdStatusCodeUseSerial and
PcdStatusCodeUseMemory, and when converting the existent platforms,
the new NULL class resolutions in the DSC files will reflect the specific PCD
values used in those DSC files until then. Is that right?
Thanks for pointing out the PCD part which I miss in this RFC.
This commit https://github.com/tianocore/edk2/commit/45bc28172fbf38ac21e2592c07189b55f57695e3 have updated PcdStatusCodeUseSerial and PcdStatusCodeUseMemory type.
We plan to keep PcdStatusCodeUseSerial and PcdStatusCodeUseMemory. Through NULL class resolutions in the DSC can make the code handler code included or not, then we still can control handler enable/disable through the PCD dynamically if the handler is included.
What do you think of this?


Thanks,
Dandan
I'm OK with it.

Thanks
Laszlo



Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules

Dong, Eric <eric.dong@...>
 

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Brian J. Johnson
Sent: Saturday, June 20, 2020 1:29 AM
To: devel@edk2.groups.io; Bi, Dandan <dandan.bi@...>; rfc@edk2.groups.io
Cc: Dong, Eric <eric.dong@...>; Ni, Ray <ray.ni@...>; Wang, Jian J <jian.j.wang@...>; Wu, Hao A <hao.a.wu@...>; Tan, Ming <ming.tan@...>
Subject: Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules

On 6/18/20 2:01 AM, Dandan Bi wrote:
Hi All,

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2816

We plan to separate two kinds of NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler/.../ StatusCodeHandlerPei/RuntimeDxe/Smm modules.
The benefit we want to gain from this separation is to 1) make the code clear and easy to maintain, 2) make platform flexible to choose any handler library they need, and it also can reduce image size since the unused handlers can be excluded.
If you have any concern or comments for this separation, please let me know.

We plan to add new separated NULL class library MemoryStausCodeHandlerLib and SerialStatusCodeHandlerLib with different phase implementation into MdeModulePkg\Library\ directory.
The main tree structure may like below:
MdeModulePkg\Library
|------MemoryStausCodeHandlerLib
|------|------ PeiMemoryStausCodeHandlerLib.inf
|------|------ RuntimeDxeMemoryStatusCodeHandlerLib.inf
|------|------ SmmMemoryStausCodeHandlerLib.inf
|------SerialStatusCodeHandlerLib
|------|------ PeiSerialStatusCodeHandlerLib.inf
|------|------ RuntimeDxeSerialStatusCodeHandlerLib.inf
|------|------ SmmSerialStatusCodeHandlerLib.inf


We will update existing platform use cases in edk2 and edk2-platform repo to cover the new NULL class library to make sure this change doesn't impact any platform.
After this separation, StatusCodeHandler module usage will like below, and it's also very flexible for platform to cover more handler libraries to meet their requirements.
MdeModulePkg/Universal/StatusCodeHandler/Pei/StatusCodeHandlerPei.inf {
<LibraryClasses>
NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/PeiMemoryStausCodeHandlerLib.inf
NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/PeiSerialStatusCodeHandlerLib.inf
...
}

MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf {
<LibraryClasses>
NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/RuntimeDxeMemoryStausCodeHandlerLib.inf
NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/RuntimeDxeSerialStatusCodeHandlerLib.inf
...
}

MdeModulePkg/Universal/StatusCodeHandler/Smm/StatusCodeHandlerSmm.inf {
<LibraryClasses>
NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/SmmMemoryStausCodeHandlerLib.inf
NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/SmmSerialStatusCodeHandlerLib.inf
...
}


Thanks,
Dandan



Dandan,

We'll have a lot of layers of indirection.... The ReportStatusCodeRouter modules will call one or more StatusCodeHandlerModules, and the standard StatusCodeHandler modules will call multiple StatusCodeHandlerLib libraries.

How about adding StatusCodeHandlerLib support directly to the ReportStatusCodeRouter modules? Then platforms could omit the StatusCodeHandler modules if they're only using the open-source code. That sounds like less overhead since fewer modules would be needed



Hi Brain,

You are right. Current design truly has a lot of layers. The ReportStatusCodeRouter module provides the register logic and maintain the registered status code handlers. Now the platform may have more than one of drivers used to register the status code handler. This RFC used to resolve the platform has more than one status code handler drivers' issue. We expect the platform only need one wrapper driver in MdeModulePkg to let the status code handler library to register its handler on it.

Thanks,

Eric



Thanks,
--

Brian J. Johnson
Enterprise X86 Lab

Hewlett Packard Enterprise

hpe.com<3D%22hpe.com%22>


Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules

Andrew Fish <afish@...>
 

On Jun 19, 2020, at 10:29 AM, Brian J. Johnson <brian.johnson@...> wrote:

On 6/18/20 2:01 AM, Dandan Bi wrote:
Hi All,

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2816

We plan to separate two kinds of NULL class libraries for Memory and serial handlers fromMdeModulePkg/Universal/StatusCodeHandler/…/ StatusCodeHandlerPei/RuntimeDxe/Smm modules.
The benefit we want to gain from this separation is to 1) make the code clear and easy to maintain, 2) make platform flexible to choose any handler library they need, and it also can reduce image size since the unused handlers can be excluded.
If you have any concern or comments for this separation, please let me know.

We plan to add new separated NULL class library MemoryStausCodeHandlerLib and SerialStatusCodeHandlerLib with different phase implementation into MdeModulePkg\Library\ directory.
The main tree structure may like below:
MdeModulePkg\Library
|------MemoryStausCodeHandlerLib
|------|------ PeiMemoryStausCodeHandlerLib.inf
|------|------ RuntimeDxeMemoryStatusCodeHandlerLib.inf
|------|------ SmmMemoryStausCodeHandlerLib.inf
|------SerialStatusCodeHandlerLib
|------|------ PeiSerialStatusCodeHandlerLib.inf
|------|------ RuntimeDxeSerialStatusCodeHandlerLib.inf
|------|------ SmmSerialStatusCodeHandlerLib.inf


We will update existing platform use cases in edk2 and edk2-platform repo to cover the new NULL class library to make sure this change doesn’t impact any platform.
After this separation, StatusCodeHandler module usage will like below, and it’s also very flexible for platform to cover more handler libraries to meet their requirements.
MdeModulePkg/Universal/StatusCodeHandler/Pei/StatusCodeHandlerPei.inf {
<LibraryClasses>
NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/PeiMemoryStausCodeHandlerLib.inf
NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/PeiSerialStatusCodeHandlerLib.inf

}

MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf {
<LibraryClasses>
NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/RuntimeDxeMemoryStausCodeHandlerLib.inf
NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/RuntimeDxeSerialStatusCodeHandlerLib.inf

}

MdeModulePkg/Universal/StatusCodeHandler/Smm/StatusCodeHandlerSmm.inf {
<LibraryClasses>
NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/SmmMemoryStausCodeHandlerLib.inf
NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/SmmSerialStatusCodeHandlerLib.inf

}


Thanks,
Dandan
Dandan,

We'll have a lot of layers of indirection.... The ReportStatusCodeRouter modules will call one or more StatusCodeHandlerModules, and the standard StatusCodeHandler modules will call multiple StatusCodeHandlerLib libraries.

How about adding StatusCodeHandlerLib support directly to the ReportStatusCodeRouter modules? Then platforms could omit the StatusCodeHandler modules if they're only using the open-source code. That sounds like less overhead since fewer modules would be needed.

I think the need to execute from ROM makes this tricky.

It looks to me that it is easy to move from PCD to libs for the StatusCodeHandler since registration is basically `RscHandlerPpi->Register (SerialStatusCodeReportWorker);`. The issue I see is the ReportStatusCodeRouter publishes RscHandlerPpi after the PEIMs constructor has been called and if the PEIM. Given globals don’t work when running from ROM you would have to do something like publish a HOB in the library constructor and then have the GenericStatusCodePeiEntry() walk the HOBs and install the handlers. So I guess it is a little easier than I 1st thought when I started writing this mail, but it would require a new public API.

Thanks,

Andrew Fish
Thanks,

--
Brian J. Johnson
Enterprise X86 Lab

Hewlett Packard Enterprise

hpe.com <x-msg://64/3D%22hpe.com%22>


Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules

Brian J. Johnson
 

On 6/18/20 2:01 AM, Dandan Bi wrote:

Hi All,

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2816 <https://bugzilla.tianocore.org/show_bug.cgi?id=2816>

We plan to separate two kinds of NULL class libraries for Memory and serial handlers from *MdeModulePkg/Universal/StatusCodeHandler/…/ StatusCodeHandlerPei/RuntimeDxe/Smm* modules.

The benefit we want to gain from this separation is to 1) make the code clear and easy to maintain, 2) make platform flexible to choose any handler library they need, and it also can reduce image size since the unused handlers can be excluded.

If you have any concern or comments for this separation, please let me know.

We plan to add new separated NULL class library *MemoryStausCodeHandlerLib *and*SerialStatusCodeHandlerLib *with different phase implementation into *MdeModulePkg\Library\* directory.

The main tree structure may like below:

MdeModulePkg\Library

|------*MemoryStausCodeHandlerLib*

|------|------ PeiMemoryStausCodeHandlerLib.inf

|------|------ RuntimeDxeMemoryStatusCodeHandlerLib.inf

|------|------ SmmMemoryStausCodeHandlerLib.inf

|------*SerialStatusCodeHandlerLib*

|------|------ PeiSerialStatusCodeHandlerLib.inf

|------|------ RuntimeDxeSerialStatusCodeHandlerLib.inf

|------|------ SmmSerialStatusCodeHandlerLib.inf

**

**

We will update existing platform use cases in edk2 and edk2-platform repo to cover the new NULL class library to make sure this change doesn’t impact any platform.

After this separation, StatusCodeHandler module usage will like below, and it’s also very flexible for platform to cover more handler libraries to meet their requirements.

MdeModulePkg/Universal/StatusCodeHandler/Pei/StatusCodeHandlerPei.inf {

  <LibraryClasses>

NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/PeiMemoryStausCodeHandlerLib.inf

NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/PeiSerialStatusCodeHandlerLib.inf

    …

}

MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf {

  <LibraryClasses>

NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/RuntimeDxeMemoryStausCodeHandlerLib.inf

NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/RuntimeDxeSerialStatusCodeHandlerLib.inf

    …

}

MdeModulePkg/Universal/StatusCodeHandler/Smm/StatusCodeHandlerSmm.inf {

  <LibraryClasses>

 NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/SmmMemoryStausCodeHandlerLib.inf

NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/SmmSerialStatusCodeHandlerLib.inf

    …

}

Thanks,

Dandan
Dandan,

We'll have a lot of layers of indirection....  The ReportStatusCodeRouter modules will call one or more StatusCodeHandlerModules, and the standard StatusCodeHandler modules will call multiple StatusCodeHandlerLib libraries.

How about adding StatusCodeHandlerLib support directly to the ReportStatusCodeRouter modules?  Then platforms could omit the StatusCodeHandler modules if they're only using the open-source code.  That sounds like less overhead since fewer modules would be needed.

Thanks,

--

*Brian J. Johnson
*Enterprise X86 Lab

Hewlett Packard Enterprise

*hpe.com* <3D"hpe.com">


Re: [edk2-devel] [edk2-rfc] MdeModulePkg/StatusCodeHandler: Separate NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler modules

Laszlo Ersek
 

On 06/18/20 09:01, Dandan Bi wrote:
Hi All,

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2816

We plan to separate two kinds of NULL class libraries for Memory and serial handlers from MdeModulePkg/Universal/StatusCodeHandler/.../ StatusCodeHandlerPei/RuntimeDxe/Smm modules.
The benefit we want to gain from this separation is to 1) make the code clear and easy to maintain, 2) make platform flexible to choose any handler library they need, and it also can reduce image size since the unused handlers can be excluded.
If you have any concern or comments for this separation, please let me know.

We plan to add new separated NULL class library MemoryStausCodeHandlerLib and SerialStatusCodeHandlerLib with different phase implementation into MdeModulePkg\Library\ directory.
The main tree structure may like below:
MdeModulePkg\Library
|------MemoryStausCodeHandlerLib
|------|------ PeiMemoryStausCodeHandlerLib.inf
|------|------ RuntimeDxeMemoryStatusCodeHandlerLib.inf
|------|------ SmmMemoryStausCodeHandlerLib.inf
|------SerialStatusCodeHandlerLib
|------|------ PeiSerialStatusCodeHandlerLib.inf
|------|------ RuntimeDxeSerialStatusCodeHandlerLib.inf
|------|------ SmmSerialStatusCodeHandlerLib.inf


We will update existing platform use cases in edk2 and edk2-platform repo to cover the new NULL class library to make sure this change doesn't impact any platform.
After this separation, StatusCodeHandler module usage will like below, and it's also very flexible for platform to cover more handler libraries to meet their requirements.
MdeModulePkg/Universal/StatusCodeHandler/Pei/StatusCodeHandlerPei.inf {
<LibraryClasses>
NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/PeiMemoryStausCodeHandlerLib.inf
NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/PeiSerialStatusCodeHandlerLib.inf
...
}

MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf {
<LibraryClasses>
NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/RuntimeDxeMemoryStausCodeHandlerLib.inf
NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/RuntimeDxeSerialStatusCodeHandlerLib.inf
...
}

MdeModulePkg/Universal/StatusCodeHandler/Smm/StatusCodeHandlerSmm.inf {
<LibraryClasses>
NULL|MdeModulePkg/Library/MemoryStausCodeHandlerLib/SmmMemoryStausCodeHandlerLib.inf
NULL|MdeModulePkg/Library/SerialStatusCodeHandlerLib/SmmSerialStatusCodeHandlerLib.inf
...
}
So I assume you're going to remove PcdStatusCodeUseSerial and
PcdStatusCodeUseMemory, and when converting the existent platforms, the
new NULL class resolutions in the DSC files will reflect the specific
PCD values used in those DSC files until then. Is that right?

I'm OK with it.

Thanks
Laszlo

421 - 440 of 789