Date   

Re: [EXTERNAL] Re: [edk2-devel] EDK2 Host-Based Unit Test RFC (Now with docs!)

Bret Barkelew <bret.barkelew@...>
 

1. I see that MdePkg adds a dependency on UnitTestPkg. This makes UnitTestPkg the root package when building and running host based unit tests. This makes sense, but good to highlight that all packages that use host based tests will introduce a new package dependency on UnitTestPkg.
* Since the dependency only applies to unit tests, can we update the DependencyCheck plugin to support listing dependencies for FW components separate from dependencies for unit tests?
* Can move. Capability is there, but mistakenly added to the wrong section.
2. I see UnitTestPkg declares 6 new lib classes. Are all 6 classes intended to be used directly from a unit test case? If some of these are only intended to be used from the framework inside the UnitTestPkg can we make them private?
* Because there are different instances in multiple places (and conceivably more in the future), we don’t see how we could make them private.
3. In the MdePkg, I see a new top level directory called ‘HostLibrary’. Since these lib instances are only used from a host based test, can they be moved into the ‘Test’ directory?
* Can move.
4. MdePkg/MdePkgTest.dsc.
* Can this DSC file be moved into the ‘Test’ directory?

i. Yes

* I see this DSC file only supports VS today. How much work is it to add GCC support?

i. Don’t know. This is an item on our list, but lower priority and not a blocker.

1. MdePkg/HostLibrary/BaseLibHost
* Why are there 2 INFs. One with ASM and one without ASM? Can we just use the one with ASM and assume NASM is installed?
* I see the purpose of this lib instance is to call the
* SetJump()/LongJump(). So these implementations always work? It looks like it assumes BASE_LIBRARY_JUMP_BUFFER is identical to the host OS user mode application setjmp()/longjmp() state.
* Why are DRx and CRx registers emulated? I would think and code that depends on read/writing these registers would not be compatible with host based testing. Can we change to ASSERT (FALSE)?
* PatchInstructionX86() – I suspect this will not work in a host based environment because it is self modifying code. Should it be ASSERT (FALSE)?
* Libraries were taken directly from the Intel work in HBFA. They worked so we kept them. We’re just as interested in the answers to these questions as you are.
2. MdePkg/HostLibrary/DebugLibHost
* What is ‘#ifndef TEST_WITH_KLEE’
* What is the ‘PatchFormat()’ function? It is always disabled with if (0).
* Are the PCDs to set the debug message levels disabled on purpose? (DebugPrintEnabled(), DebugPrintLevelEnabled(), DebugCodeEnabled())
* Libraries were taken directly from the Intel work in HBFA. They worked so we kept them. We’re just as interested in the answers to these questions as you are.
3. MdePkg/HostLibrary/BaseMemoryLibHost
* Why can’t we use MdePkg/Library/BaseMemoryLib/BaseMemoryLib/inf instead and reduce the number of host specific lib instances?
* Libraries were taken directly from the Intel work in HBFA. They worked so we kept them. We’re just as interested in the answers to these questions as you are.
4. MdePkg/HostLibrary/MemoryAllocationLibHost
* Why is are memcpy(), assert(), memset() used in this lib? I would expect this lib instance to match the UefiMemoryAllocationLib with the only the use of malloc() and free() to replace the UEFI Boot Services calls.
* Libraries were taken directly from the Intel work in HBFA. They worked so we kept them. We’re just as interested in the answers to these questions as you are.
5. Host library instance naming conventions.
* The current naming convention uses the environment as a prefix(e.g. Pei, Smm, Uefi) and the services the lib instance uses as a post fix. Would it make more sense to use ‘Host’ as a prefix instead of a postfix in the lib instance names?
* I don’t know if there’s a 1:1 relationship with these. For some library classes (that you might have host versions of), the class itself is the PeiSomethingLib, and the host version would be the PeiSomethingLibHost. No?
6. Unicode vs ASCII strings
* I see InitUnitTestFramework(), CreateUnitTestSuite(), and AddTestCase() all take Unicode strings for the labels. I also see extra code to convert gEfiCallerBaseName from ASCII to Unicode to use it as a short name of a test framework. I think it would be simpler if the parameters to these APIs were ASCII and the framework can convert to Unicode if needed.
* No strong feelings, but we already have a bunch of tests written this way. Since the UnitTestLib is an abstraction that works as well in Shell as in the Host, going with Unicode strings seemed to match the art for Shell apps (since the framework was written for Shell first).
7. Will InitUnitTestFramework() and CreateUnitTestSuite() always be called in pairs? If so, can we combine these to a single API?
* I see the SafeIntLib example create a single framework and multiple test suites. Perhaps we can have a single CreateUnitTestSuite() API where Framework is an IN/OUT and if it is passed in as NULL, the Framework handle is created.

i. It’s not always in pairs. You would only have a single framework init, but could have multiple suites.

* I see a pattern where the CreateUnitTestSuite() ‘Package’ parameter is used as a prefix to every call to AddTestCase() in the ‘ClassName’ parameter. Can we simplify AddTestCase() so it only need to pass in the name of the unit test case, and the framework can append Package and the unit test case name?

i. Right now, our tests just coincidentally share similar names with the packages, but we don’t feel this is axiomatic and don’t want to force this naming on others, who may be trying to bolt into other reporting structures.

1. I see the use of the ‘Fw’ variable as a shorthand for ‘Framework’. Can we use something other than ‘Fw’. It may be confused with ‘Firmware’.
* No real arguments. Wouldn’t fight a find-replace, but it’ll just be a bunch of touches as we commit.
2. UnitTestPkg/Include/UnitTestTypes.h
* I see several hard coded string lengths. Since this runs in a host environment and strings can always be allocated, can the hard coded lengths be removed? Update the structs to use pointers to strings instead of string arrays, and the framework can manage alloc() and free()?

i. Given that this framework is designed to be nimble enough to work in PEI and SMM and other constrictive environments, we wanted to keep fixed sizing.

* How are Fingerprints used? The idea of using as hash as a unique identifier is a good idea. How is the hash calculated? What unit test code artifacts are used so developers know what parameters must be unique? Can we just decide to use a specific hash algorithm/size and the structure can be a pointer to an allocated buffer instead of a fixed size array to make it easy to change the algorithm/size in the future?

i. Fingerprints are unique IDs to make sure that serialize/unserialized state matches the tests upon re-entry. I’m not married to MD5, but it needs to be predictably unique, and carried with the framework. I would not want to make any requirements on CryptoLib, since these aren’t crypto-strong.

* Update all the strings to be ASCII? See Unicode vs ASCII above.

i. Ideally not, unless there’s a strong use case.

* UNIT_TEST_SAVE_TEST – The last field is a variable sized array, so it can be a formal field.

i. Not opposed, but don’t really want to make the change myself. Is there a style guide that this is breaking?

* UNIT_TEST_SAVE_CONTEXT - – The last field is a variable sized array, so it can be a formal field.
* UNIT_TEST_SAVE_HEADER – Can the last 3 fields be changed to pointers and be formal fields?
* Do the structures really need to be packed? Specially with the changes suggested above? Is the intent to potentially share data stored on disk between different host execution environments that may have different width architectures?

i. That’s an interesting point. Could you draw up your suggestion and submit a PR?

1. UnitTestPkg – UnitTestLib.h
* Can you provide more context for the APIs SaveFrameworkState(), SaveFrameworkStateAndQuit(), SaveFrameworkStateAndReboot(), SetFrameworkBootNextDevice(). I do not see any Load() functions, so how would a set of tests be resumed? If these do not apply to host based tests, should these be split out to a different lib class?

i. I’ll improve the documentation around these functions. I will acknowledge, however, that this is an interface that I expect to evolve as we figure out what kinds of tests the community wants support for “out of the box”. If we want to easily enable tests around – for example – ExitBootServices, we will likely want to tweak this going forward to its simplest form. The version we have here was sufficient to enable the test cases that we’ve currently written.

1. UnitTestPkg/Library/UnitTestLib
* I see an implementation of MD5. We should not do our own. We should use an approved implementation such as OpenSSL.

i. Happy to discuss another implementation, but this is not crypto-strong. It’s just for uniqueness. A sufficiently long CRC would probably work, too.

1. UnitTestPkg/Library/UnitTestTerminationLibTbd
* Do we really need this lib instance now?

i. This is here so that we can figure out what this should look like for a host. Host obviously wouldn’t reset, but could conceivably quit. Or maybe that doesn’t make any sense for a host.



- Bret

________________________________
From: devel@edk2.groups.io <devel@edk2.groups.io> on behalf of Bret Barkelew via Groups.Io <bret.barkelew@...>
Sent: Wednesday, December 4, 2019 10:24:21 AM
To: Andrew Fish <afish@...>; devel@edk2.groups.io <devel@edk2.groups.io>; Kinney, Michael D <michael.d.kinney@...>
Cc: rfc@edk2.groups.io <rfc@edk2.groups.io>
Subject: Re: [EXTERNAL] Re: [edk2-devel] EDK2 Host-Based Unit Test RFC (Now with docs!)


Andrew,

I agree with your points.



Mike,

You’ve got a lot more there. Let me take a look and update the draft. I’ll ping back ASAP.



- Bret



From: Andrew Fish<mailto:afish@...>
Sent: Wednesday, December 4, 2019 9:50 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Kinney, Michael D<mailto:michael.d.kinney@...>
Cc: Bret Barkelew<mailto:Bret.Barkelew@...>; rfc@edk2.groups.io<mailto:rfc@edk2.groups.io>
Subject: [EXTERNAL] Re: [edk2-devel] EDK2 Host-Based Unit Test RFC (Now with docs!)



Mike,



I like and agree with your comments.



On the UnitTestPkg(s) dependency issue I think it would make sense to move as much as possible into the *Pkg/Test/ ( *Pkg/HostLibrary, MdePkg/MdePkgTest.dsc, etc.) directory structure. It looks like for implementation specific tests we skip the Test directory and drop the UnitTest or FuzzTest directly into modules directory. Maybe we should follow the same pattern as *Pkg/Test and have a Test directory? This will help minimize the number of "reserved" directories we need for managing the testing. Have a standardized directory structure will make it easier to differentiate the code from tests while doing `git grep` or other forms of code spelunking.



A Host-Based Unit Test for a Library makes sense as you can link directly against the library and test it. A Host-Based Unit Test for Protocol/PPI/GUID [1] seems a little more complex. It is easy enough to write tests for the interfaces but what APIs do you call to get access to the protocol?



Per our conversation at the Stewards meeting I think make sense to try to roll out the testing of libraries as the 1st phase. The mocking required for drivers could get quite complex and let us not get bogged down in all that to get something working.



[1] *Pkg/Test/UnitTest/[Library|Protocol|Ppi|Guid]



Thanks,



Andrew Fish





On Dec 2, 2019, at 3:12 PM, Michael D Kinney <michael.d.kinney@...<mailto:michael.d.kinney@...>> wrote:



Hi Bret,



Thanks for posting this content. Host based unit testing is a very valuable addition to the CI checks.



I have the following comments:



1. I see that MdePkg adds a dependency on UnitTestPkg. This makes UnitTestPkg the root package when building and running host based unit tests. This makes sense, but good to highlight that all packages that use host based tests will introduce a new package dependency on UnitTestPkg.

* Since the dependency only applies to unit tests, can we update the DependencyCheck plugin to support listing dependencies for FW components separate from dependencies for unit tests?

1. I see UnitTestPkg declares 6 new lib classes. Are all 6 classes intended to be used directly from a unit test case? If some of these are only intended to be used from the framework inside the UnitTestPkg can we make them private?
2. In the MdePkg, I see a new top level directory called ‘HostLibrary’. Since these lib instances are only used from a host based test, can they be moved into the ‘Test’ directory?
3. MdePkg/MdePkgTest.dsc.

* Can this DSC file be moved into the ‘Test’ directory?
* I see this DSC file only supports VS today. How much work is it to add GCC support?

1. MdePkg/HostLibrary/BaseLibHost

* Why are there 2 INFs. One with ASM and one without ASM? Can we just use the one with ASM and assume NASM is installed?
* I see the purpose of this lib instance is to call the
* SetJump()/LongJump(). So these implementations always work? It looks like it assumes BASE_LIBRARY_JUMP_BUFFER is identical to the host OS user mode applicationsetjmp()/longjmp() state.
* Why are DRx and CRx registers emulated? I would think and code that depends on read/writing these registers would not be compatible with host based testing. Can we change to ASSERT (FALSE)?
* PatchInstructionX86() – I suspect this will not work in a host based environment because it is self modifying code. Should it be ASSERT (FALSE)?

1. MdePkg/HostLibrary/DebugLibHost

* What is ‘#ifndef TEST_WITH_KLEE’
* What is the ‘PatchFormat()’ function? It is always disabled with if (0).
* Are the PCDs to set the debug message levels disabled on purpose? (DebugPrintEnabled(), DebugPrintLevelEnabled(), DebugCodeEnabled())

1. MdePkg/HostLibrary/BaseMemoryLibHost

* Why can’t we use MdePkg/Library/BaseMemoryLib/BaseMemoryLib/inf instead and reduce the number of host specific lib instances?

1. MdePkg/HostLibrary/MemoryAllocationLibHost

* Why is are memcpy(), assert(), memset() used in this lib? I would expect this lib instance to match the UefiMemoryAllocationLib with the only the use of malloc() and free() to replace the UEFI Boot Services calls.

1. Host library instance naming conventions.

* The current naming convention uses the environment as a prefix(e.g. Pei, Smm, Uefi) and the services the lib instance uses as a post fix. Would it make more sense to use ‘Host’ as a prefix instead of a postfix in the lib instance names?

1. Unicode vs ASCII strings

* I see InitUnitTestFramework(), CreateUnitTestSuite(), and AddTestCase() all take Unicode strings for the labels. I also see extra code to convert gEfiCallerBaseName from ASCII to Unicode to use it as a short name of a test framework. I think it would be simpler if the parameters to these APIs were ASCII and the framework can convert to Unicode if needed.

1. Will InitUnitTestFramework() and CreateUnitTestSuite() always be called in pairs? If so, can we combine these to a single API?

* I see the SafeIntLib example create a single framework and multiple test suites. Perhaps we can have a single CreateUnitTestSuite() API where Framework is an IN/OUT and if it is passed in as NULL, the Framework handle is created.
* I see a pattern where the CreateUnitTestSuite() ‘Package’ parameter is used as a prefix to every call to AddTestCase() in the ‘ClassName’ parameter. Can we simplify AddTestCase() so it only need to pass in the name of the unit test case, and the framework can append Package and the unit test case name?

1. I see the use of the ‘Fw’ variable as a shorthand for ‘Framework’. Can we use something other than ‘Fw’. It may be confused with ‘Firmware’.
2. UnitTestPkg/Include/UnitTestTypes.h

* I see several hard coded string lengths. Since this runs in a host environment and strings can always be allocated, can the hard coded lengths be removed? Update the structs to use pointers to strings instead of string arrays, and the framework can manage alloc() and free()?
* How are Fingerprints used? The idea of using as hash as a unique identifier is a good idea. How is the hash calculated? What unit test code artifacts are used so developers know what parameters must be unique? Can we just decide to use a specific hash algorithm/size and the structure can be a pointer to an allocated buffer instead of a fixed size array to make it easy to change the algorithm/size in the future?
* Update all the strings to be ASCII? See Unicode vs ASCII above.
* UNIT_TEST_SAVE_TEST – The last field is a variable sized array, so it can be a formal field.
* UNIT_TEST_SAVE_CONTEXT - – The last field is a variable sized array, so it can be a formal field.
* UNIT_TEST_SAVE_HEADER – Can the last 3 fields be changed to pointers and be formal fields?
* Do the structures really need to be packed? Specially with the changes suggested above? Is the intent to potentially share data stored on disk between different host execution environments that may have different width architectures?

1. UnitTestPkg – UnitTestLib.h

* Can you provide more context for the APIs SaveFrameworkState(), SaveFrameworkStateAndQuit(), SaveFrameworkStateAndReboot(), SetFrameworkBootNextDevice(). I do not see any Load() functions, so how would a set of tests be resumed? If these do not apply to host based tests, should these be split out to a different lib class?

1. UnitTestPkg/Library/UnitTestLib

* I see an implementation of MD5. We should not do our own. We should use an approved implementation such as OpenSSL.

1. UnitTestPkg/Library/UnitTestTerminationLibTbd

* Do we really need this lib instance now?



Thanks,



Mike



From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Bret Barkelew via Groups.Io
Sent: Thursday, November 21, 2019 11:39 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: [edk2-devel] EDK2 Host-Based Unit Test RFC (Now with docs!)



Now that CI has landed in edk2/master, we'd like to get the basic framework for unittesting staged as well. Target intercept date would be immediately after the 2019/11 stabilization, so we wanted to go ahead and get comments now.

The host unittest framework consists of five primary pieces:
- The test library (Cmocka)
- Support libraries (Found in UnitTestPkg)
- The test support plugins (HostUnitTestComilerPlugin, HostUnitTestDxeCompleteCheck, HostBasedUnitTestRunner)
- The configuration in the package-based *.ci.yaml file and package-based Test.dsc
- The tests themselves

We have a demo branch set up at:
https://github.com/corthon/edk2-staging/tree/edk2-host-test_v2<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcorthon%2Fedk2-staging%2Ftree%2Fedk2-host-test_v2&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C6eac9932f3f640dd65ac08d778e731e3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637110806683189828&sdata=JQ%2BoWxlEBOK2sH55cRAPVa3OpAxTsm6eHcdbWSo9CVo%3D&reserved=0>
We also have a demo build pipeline at:
https://dev.azure.com/tianocore/edk2-ci-play/_build?definitionId=36&_a=summary<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdev.azure.com%2Ftianocore%2Fedk2-ci-play%2F_build%3FdefinitionId%3D36%26_a%3Dsummary&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C6eac9932f3f640dd65ac08d778e731e3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637110806683189828&sdata=wBwn1ehjyTmYNKVSvlEZSXK5qyeu4EPAL7FzdYntnt4%3D&reserved=0>

The current demo branch contains a single test in MdePkg for SafeIntLib (module file MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.inf). This test is automatically detected by the HostUnitTestComilerPlugin and run by the HostBasedUnitTestRunner as part of the CI process.

A few notes about the current demo:
1) The demo currently only works on Windows build chains, but there's no reason to believe that it can't work equally well on Linux build chains, and are happy to work with anyone to get it there.

2) The demo currently has four failing conditions that can be seen towards the end of MdePkg "Build and Test" log file for this build:
https://dev.azure.com/tianocore/edk2-ci-play/_build/results?buildId=2813<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdev.azure.com%2Ftianocore%2Fedk2-ci-play%2F_build%2Fresults%3FbuildId%3D2590&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C6eac9932f3f640dd65ac08d778e731e3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637110806683199782&sdata=1zd2oq8zRZUn3%2FUiGDuJZEZ5M0srtc2bqoa0%2BLbZB3s%3D&reserved=0>
"WARNING - Test SafeInt16ToChar8 - Status
d:\a\1\s\MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.c:302: error: Failure!
WARNING - TestBaseSafeIntLib.exe Test Failed
WARNING - Test SafeInt32ToChar8 - Status
d:\a\1\s\MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.c:638: error: Failure!
WARNING - TestBaseSafeIntLib.exe Test Failed
WARNING - Test SafeIntnToChar8 - Status
d:\a\1\s\MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.c:1051: error: Failure!
WARNING - TestBaseSafeIntLib.exe Test Failed
WARNING - Test SafeInt64ToChar8 - Status
d:\a\1\s\MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.c:1456: error: Failure!"
These failures seem to be legitimate and further work should be done by the community to decide whether the test case is correct or the library is correct, but one of them needs to change.

3) Current demo pulls in test collateral from a fork of the edk2-test repo. This repo is currently *very* heavy due to the history of the UEFI SCT project and the number of binaries that it pulls down. It's possible that we (the community) need to select a better place for this code to live. Maybe in edk2 primary (though it's not explicitly firmware code, so it seems unnecessary). Maybe in a new edk2-test2 repo or something like that.

There’s an RFC doc here: https://github.com/corthon/edk2-staging/blob/edk2-host-test_v2/Readme-RFC.md<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcorthon%2Fedk2-staging%2Fblob%2Fedk2-host-test_v2%2FReadme-RFC.md&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C6eac9932f3f640dd65ac08d778e731e3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637110806683199782&sdata=1nq1mHjsbSZj4IQM5RBwSrvaQxO5cmDlTvf7VYNDV%2FA%3D&reserved=0>

And a usage guide here: https://github.com/corthon/edk2-staging/blob/edk2-host-test_v2/UnitTestPkg/ReadMe.md<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcorthon%2Fedk2-staging%2Fblob%2Fedk2-host-test_v2%2FUnitTestPkg%2FReadMe.md&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C6eac9932f3f640dd65ac08d778e731e3%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637110806683209750&sdata=0jRQ2Rzr9PWSYJ1YDs7l2aFS3PfAnTbTousYYe8IWTw%3D&reserved=0>

Once again, would love to get this into EDK2 master after stabilization, and most of this has previously been shopped around in other discussion threads. This is just where the rubber meets the road and is the minimal subset of code that needs to land for folks to start contributing unittests against the core libraries that can be run as part of any CI pass.

Thanks!
- Bret


Re: [EXTERNAL] Re: [edk2-devel] EDK2 Host-Based Unit Test RFC (Now with docs!)

Bret Barkelew <bret.barkelew@...>
 

Andrew,
I agree with your points.

Mike,
You’ve got a lot more there. Let me take a look and update the draft. I’ll ping back ASAP.

- Bret

From: Andrew Fish<mailto:afish@...>
Sent: Wednesday, December 4, 2019 9:50 AM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Kinney, Michael D<mailto:michael.d.kinney@...>
Cc: Bret Barkelew<mailto:Bret.Barkelew@...>; rfc@edk2.groups.io<mailto:rfc@edk2.groups.io>
Subject: [EXTERNAL] Re: [edk2-devel] EDK2 Host-Based Unit Test RFC (Now with docs!)

Mike,

I like and agree with your comments.

On the UnitTestPkg(s) dependency issue I think it would make sense to move as much as possible into the *Pkg/Test/ ( *Pkg/HostLibrary, MdePkg/MdePkgTest.dsc, etc.) directory structure. It looks like for implementation specific tests we skip the Test directory and drop the UnitTest or FuzzTest directly into modules directory. Maybe we should follow the same pattern as *Pkg/Test and have a Test directory? This will help minimize the number of "reserved" directories we need for managing the testing. Have a standardized directory structure will make it easier to differentiate the code from tests while doing `git grep` or other forms of code spelunking.

A Host-Based Unit Test for a Library makes sense as you can link directly against the library and test it. A Host-Based Unit Test for Protocol/PPI/GUID [1] seems a little more complex. It is easy enough to write tests for the interfaces but what APIs do you call to get access to the protocol?

Per our conversation at the Stewards meeting I think make sense to try to roll out the testing of libraries as the 1st phase. The mocking required for drivers could get quite complex and let us not get bogged down in all that to get something working.

[1] *Pkg/Test/UnitTest/[Library|Protocol|Ppi|Guid]

Thanks,

Andrew Fish



On Dec 2, 2019, at 3:12 PM, Michael D Kinney <michael.d.kinney@...<mailto:michael.d.kinney@...>> wrote:

Hi Bret,

Thanks for posting this content. Host based unit testing is a very valuable addition to the CI checks.

I have the following comments:


1. I see that MdePkg adds a dependency on UnitTestPkg. This makes UnitTestPkg the root package when building and running host based unit tests. This makes sense, but good to highlight that all packages that use host based tests will introduce a new package dependency on UnitTestPkg.

* Since the dependency only applies to unit tests, can we update the DependencyCheck plugin to support listing dependencies for FW components separate from dependencies for unit tests?

1. I see UnitTestPkg declares 6 new lib classes. Are all 6 classes intended to be used directly from a unit test case? If some of these are only intended to be used from the framework inside the UnitTestPkg can we make them private?
2. In the MdePkg, I see a new top level directory called ‘HostLibrary’. Since these lib instances are only used from a host based test, can they be moved into the ‘Test’ directory?
3. MdePkg/MdePkgTest.dsc.

* Can this DSC file be moved into the ‘Test’ directory?
* I see this DSC file only supports VS today. How much work is it to add GCC support?

1. MdePkg/HostLibrary/BaseLibHost

* Why are there 2 INFs. One with ASM and one without ASM? Can we just use the one with ASM and assume NASM is installed?
* I see the purpose of this lib instance is to call the
* SetJump()/LongJump(). So these implementations always work? It looks like it assumes BASE_LIBRARY_JUMP_BUFFER is identical to the host OS user mode applicationsetjmp()/longjmp() state.
* Why are DRx and CRx registers emulated? I would think and code that depends on read/writing these registers would not be compatible with host based testing. Can we change to ASSERT (FALSE)?
* PatchInstructionX86() – I suspect this will not work in a host based environment because it is self modifying code. Should it be ASSERT (FALSE)?

1. MdePkg/HostLibrary/DebugLibHost

* What is ‘#ifndef TEST_WITH_KLEE’
* What is the ‘PatchFormat()’ function? It is always disabled with if (0).
* Are the PCDs to set the debug message levels disabled on purpose? (DebugPrintEnabled(), DebugPrintLevelEnabled(), DebugCodeEnabled())

1. MdePkg/HostLibrary/BaseMemoryLibHost

* Why can’t we use MdePkg/Library/BaseMemoryLib/BaseMemoryLib/inf instead and reduce the number of host specific lib instances?

1. MdePkg/HostLibrary/MemoryAllocationLibHost

* Why is are memcpy(), assert(), memset() used in this lib? I would expect this lib instance to match the UefiMemoryAllocationLib with the only the use of malloc() and free() to replace the UEFI Boot Services calls.

1. Host library instance naming conventions.

* The current naming convention uses the environment as a prefix(e.g. Pei, Smm, Uefi) and the services the lib instance uses as a post fix. Would it make more sense to use ‘Host’ as a prefix instead of a postfix in the lib instance names?

1. Unicode vs ASCII strings

* I see InitUnitTestFramework(), CreateUnitTestSuite(), and AddTestCase() all take Unicode strings for the labels. I also see extra code to convert gEfiCallerBaseName from ASCII to Unicode to use it as a short name of a test framework. I think it would be simpler if the parameters to these APIs were ASCII and the framework can convert to Unicode if needed.

1. Will InitUnitTestFramework() and CreateUnitTestSuite() always be called in pairs? If so, can we combine these to a single API?

* I see the SafeIntLib example create a single framework and multiple test suites. Perhaps we can have a single CreateUnitTestSuite() API where Framework is an IN/OUT and if it is passed in as NULL, the Framework handle is created.
* I see a pattern where the CreateUnitTestSuite() ‘Package’ parameter is used as a prefix to every call to AddTestCase() in the ‘ClassName’ parameter. Can we simplify AddTestCase() so it only need to pass in the name of the unit test case, and the framework can append Package and the unit test case name?

1. I see the use of the ‘Fw’ variable as a shorthand for ‘Framework’. Can we use something other than ‘Fw’. It may be confused with ‘Firmware’.
2. UnitTestPkg/Include/UnitTestTypes.h

* I see several hard coded string lengths. Since this runs in a host environment and strings can always be allocated, can the hard coded lengths be removed? Update the structs to use pointers to strings instead of string arrays, and the framework can manage alloc() and free()?
* How are Fingerprints used? The idea of using as hash as a unique identifier is a good idea. How is the hash calculated? What unit test code artifacts are used so developers know what parameters must be unique? Can we just decide to use a specific hash algorithm/size and the structure can be a pointer to an allocated buffer instead of a fixed size array to make it easy to change the algorithm/size in the future?
* Update all the strings to be ASCII? See Unicode vs ASCII above.
* UNIT_TEST_SAVE_TEST – The last field is a variable sized array, so it can be a formal field.
* UNIT_TEST_SAVE_CONTEXT - – The last field is a variable sized array, so it can be a formal field.
* UNIT_TEST_SAVE_HEADER – Can the last 3 fields be changed to pointers and be formal fields?
* Do the structures really need to be packed? Specially with the changes suggested above? Is the intent to potentially share data stored on disk between different host execution environments that may have different width architectures?

1. UnitTestPkg – UnitTestLib.h

* Can you provide more context for the APIs SaveFrameworkState(), SaveFrameworkStateAndQuit(), SaveFrameworkStateAndReboot(), SetFrameworkBootNextDevice(). I do not see any Load() functions, so how would a set of tests be resumed? If these do not apply to host based tests, should these be split out to a different lib class?

1. UnitTestPkg/Library/UnitTestLib

* I see an implementation of MD5. We should not do our own. We should use an approved implementation such as OpenSSL.

1. UnitTestPkg/Library/UnitTestTerminationLibTbd

* Do we really need this lib instance now?

Thanks,

Mike

From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Bret Barkelew via Groups.Io
Sent: Thursday, November 21, 2019 11:39 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
Subject: [edk2-devel] EDK2 Host-Based Unit Test RFC (Now with docs!)

Now that CI has landed in edk2/master, we'd like to get the basic framework for unittesting staged as well. Target intercept date would be immediately after the 2019/11 stabilization, so we wanted to go ahead and get comments now.

The host unittest framework consists of five primary pieces:
- The test library (Cmocka)
- Support libraries (Found in UnitTestPkg)
- The test support plugins (HostUnitTestComilerPlugin, HostUnitTestDxeCompleteCheck, HostBasedUnitTestRunner)
- The configuration in the package-based *.ci.yaml file and package-based Test.dsc
- The tests themselves

We have a demo branch set up at:
https://github.com/corthon/edk2-staging/tree/edk2-host-test_v2<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcorthon%2Fedk2-staging%2Ftree%2Fedk2-host-test_v2&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C99a0ca0ef5c047f15d7c08d778e2842f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637110786587150091&sdata=R0cEUBx2R%2F6ppOZf5pwyZ2xHO1vrYWDD4lDP0AhseQw%3D&reserved=0>
We also have a demo build pipeline at:
https://dev.azure.com/tianocore/edk2-ci-play/_build?definitionId=36&_a=summary<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdev.azure.com%2Ftianocore%2Fedk2-ci-play%2F_build%3FdefinitionId%3D36%26_a%3Dsummary&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C99a0ca0ef5c047f15d7c08d778e2842f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637110786587150091&sdata=eVlGMFrTlnBQqIqE76aC12TYPAATe0tGAkg8dbfRfjM%3D&reserved=0>

The current demo branch contains a single test in MdePkg for SafeIntLib (module file MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.inf). This test is automatically detected by the HostUnitTestComilerPlugin and run by the HostBasedUnitTestRunner as part of the CI process.

A few notes about the current demo:
1) The demo currently only works on Windows build chains, but there's no reason to believe that it can't work equally well on Linux build chains, and are happy to work with anyone to get it there.

2) The demo currently has four failing conditions that can be seen towards the end of MdePkg "Build and Test" log file for this build:
https://dev.azure.com/tianocore/edk2-ci-play/_build/results?buildId=2813<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdev.azure.com%2Ftianocore%2Fedk2-ci-play%2F_build%2Fresults%3FbuildId%3D2590&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C99a0ca0ef5c047f15d7c08d778e2842f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637110786587160084&sdata=9DtP1PcX8FiGGtnzQf1%2FKoDjdieM7zMstI1H34Isi6I%3D&reserved=0>
"WARNING - Test SafeInt16ToChar8 - Status
d:\a\1\s\MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.c:302: error: Failure!
WARNING - TestBaseSafeIntLib.exe Test Failed
WARNING - Test SafeInt32ToChar8 - Status
d:\a\1\s\MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.c:638: error: Failure!
WARNING - TestBaseSafeIntLib.exe Test Failed
WARNING - Test SafeIntnToChar8 - Status
d:\a\1\s\MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.c:1051: error: Failure!
WARNING - TestBaseSafeIntLib.exe Test Failed
WARNING - Test SafeInt64ToChar8 - Status
d:\a\1\s\MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.c:1456: error: Failure!"
These failures seem to be legitimate and further work should be done by the community to decide whether the test case is correct or the library is correct, but one of them needs to change.

3) Current demo pulls in test collateral from a fork of the edk2-test repo. This repo is currently *very* heavy due to the history of the UEFI SCT project and the number of binaries that it pulls down. It's possible that we (the community) need to select a better place for this code to live. Maybe in edk2 primary (though it's not explicitly firmware code, so it seems unnecessary). Maybe in a new edk2-test2 repo or something like that.
There’s an RFC doc here: https://github.com/corthon/edk2-staging/blob/edk2-host-test_v2/Readme-RFC.md<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcorthon%2Fedk2-staging%2Fblob%2Fedk2-host-test_v2%2FReadme-RFC.md&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C99a0ca0ef5c047f15d7c08d778e2842f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637110786587170082&sdata=Wk%2FUyw63ja6eYOqidqExBwfMVkwvSDPMyqO%2FcjEMw6Q%3D&reserved=0>
And a usage guide here: https://github.com/corthon/edk2-staging/blob/edk2-host-test_v2/UnitTestPkg/ReadMe.md<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fcorthon%2Fedk2-staging%2Fblob%2Fedk2-host-test_v2%2FUnitTestPkg%2FReadMe.md&data=02%7C01%7Cbret.barkelew%40microsoft.com%7C99a0ca0ef5c047f15d7c08d778e2842f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637110786587170082&sdata=w5oVcKFRt6kWyahwGJjwSgYO6Ii%2B%2FfwjW18rwNcaoQo%3D&reserved=0>

Once again, would love to get this into EDK2 master after stabilization, and most of this has previously been shopped around in other discussion threads. This is just where the rubber meets the road and is the minimal subset of code that needs to land for folks to start contributing unittests against the core libraries that can be run as part of any CI pass.

Thanks!
- Bret


Re: [edk2-devel] EDK2 Host-Based Unit Test RFC (Now with docs!)

Andrew Fish <afish@...>
 

Mike,

I like and agree with your comments.

On the UnitTestPkg(s) dependency issue I think it would make sense to move as much as possible into the *Pkg/Test/ ( *Pkg/HostLibrary, MdePkg/MdePkgTest.dsc, etc.) directory structure. It looks like for implementation specific tests we skip the Test directory and drop the UnitTest or FuzzTest directly into modules directory. Maybe we should follow the same pattern as *Pkg/Test and have a Test directory? This will help minimize the number of "reserved" directories we need for managing the testing. Have a standardized directory structure will make it easier to differentiate the code from tests while doing `git grep` or other forms of code spelunking.

A Host-Based Unit Test for a Library makes sense as you can link directly against the library and test it. A Host-Based Unit Test for Protocol/PPI/GUID [1] seems a little more complex. It is easy enough to write tests for the interfaces but what APIs do you call to get access to the protocol?

Per our conversation at the Stewards meeting I think make sense to try to roll out the testing of libraries as the 1st phase. The mocking required for drivers could get quite complex and let us not get bogged down in all that to get something working.

[1] *Pkg/Test/UnitTest/[Library|Protocol|Ppi|Guid]

Thanks,

Andrew Fish

On Dec 2, 2019, at 3:12 PM, Michael D Kinney <michael.d.kinney@...> wrote:

Hi Bret,

Thanks for posting this content. Host based unit testing is a very valuable addition to the CI checks.

I have the following comments:

I see that MdePkg adds a dependency on UnitTestPkg. This makes UnitTestPkg the root package when building and running host based unit tests. This makes sense, but good to highlight that all packages that use host based tests will introduce a new package dependency on UnitTestPkg.
Since the dependency only applies to unit tests, can we update the DependencyCheck plugin to support listing dependencies for FW components separate from dependencies for unit tests?
I see UnitTestPkg declares 6 new lib classes. Are all 6 classes intended to be used directly from a unit test case? If some of these are only intended to be used from the framework inside the UnitTestPkg can we make them private?
In the MdePkg, I see a new top level directory called ‘HostLibrary’. Since these lib instances are only used from a host based test, can they be moved into the ‘Test’ directory?
MdePkg/MdePkgTest.dsc.
Can this DSC file be moved into the ‘Test’ directory?
I see this DSC file only supports VS today. How much work is it to add GCC support?
MdePkg/HostLibrary/BaseLibHost
Why are there 2 INFs. One with ASM and one without ASM? Can we just use the one with ASM and assume NASM is installed?
I see the purpose of this lib instance is to call the
SetJump()/LongJump(). So these implementations always work? It looks like it assumes BASE_LIBRARY_JUMP_BUFFER is identical to the host OS user mode applicationsetjmp()/longjmp() state.
Why are DRx and CRx registers emulated? I would think and code that depends on read/writing these registers would not be compatible with host based testing. Can we change to ASSERT (FALSE)?
PatchInstructionX86() – I suspect this will not work in a host based environment because it is self modifying code. Should it be ASSERT (FALSE)?
MdePkg/HostLibrary/DebugLibHost
What is ‘#ifndef TEST_WITH_KLEE’
What is the ‘PatchFormat()’ function? It is always disabled with if (0).
Are the PCDs to set the debug message levels disabled on purpose? (DebugPrintEnabled(), DebugPrintLevelEnabled(), DebugCodeEnabled())
MdePkg/HostLibrary/BaseMemoryLibHost
Why can’t we use MdePkg/Library/BaseMemoryLib/BaseMemoryLib/inf instead and reduce the number of host specific lib instances?
MdePkg/HostLibrary/MemoryAllocationLibHost
Why is are memcpy(), assert(), memset() used in this lib? I would expect this lib instance to match the UefiMemoryAllocationLib with the only the use of malloc() and free() to replace the UEFI Boot Services calls.
Host library instance naming conventions.
The current naming convention uses the environment as a prefix(e.g. Pei, Smm, Uefi) and the services the lib instance uses as a post fix. Would it make more sense to use ‘Host’ as a prefix instead of a postfix in the lib instance names?
Unicode vs ASCII strings
I see InitUnitTestFramework(), CreateUnitTestSuite(), and AddTestCase() all take Unicode strings for the labels. I also see extra code to convert gEfiCallerBaseName from ASCII to Unicode to use it as a short name of a test framework. I think it would be simpler if the parameters to these APIs were ASCII and the framework can convert to Unicode if needed.
Will InitUnitTestFramework() and CreateUnitTestSuite() always be called in pairs? If so, can we combine these to a single API?
I see the SafeIntLib example create a single framework and multiple test suites. Perhaps we can have a single CreateUnitTestSuite() API where Framework is an IN/OUT and if it is passed in as NULL, the Framework handle is created.
I see a pattern where the CreateUnitTestSuite() ‘Package’ parameter is used as a prefix to every call to AddTestCase() in the ‘ClassName’ parameter. Can we simplify AddTestCase() so it only need to pass in the name of the unit test case, and the framework can append Package and the unit test case name?
I see the use of the ‘Fw’ variable as a shorthand for ‘Framework’. Can we use something other than ‘Fw’. It may be confused with ‘Firmware’.
UnitTestPkg/Include/UnitTestTypes.h
I see several hard coded string lengths. Since this runs in a host environment and strings can always be allocated, can the hard coded lengths be removed? Update the structs to use pointers to strings instead of string arrays, and the framework can manage alloc() and free()?
How are Fingerprints used? The idea of using as hash as a unique identifier is a good idea. How is the hash calculated? What unit test code artifacts are used so developers know what parameters must be unique? Can we just decide to use a specific hash algorithm/size and the structure can be a pointer to an allocated buffer instead of a fixed size array to make it easy to change the algorithm/size in the future?
Update all the strings to be ASCII? See Unicode vs ASCII above.
UNIT_TEST_SAVE_TEST – The last field is a variable sized array, so it can be a formal field.
UNIT_TEST_SAVE_CONTEXT - – The last field is a variable sized array, so it can be a formal field.
UNIT_TEST_SAVE_HEADER – Can the last 3 fields be changed to pointers and be formal fields?
Do the structures really need to be packed? Specially with the changes suggested above? Is the intent to potentially share data stored on disk between different host execution environments that may have different width architectures?
UnitTestPkg – UnitTestLib.h
Can you provide more context for the APIs SaveFrameworkState(), SaveFrameworkStateAndQuit(), SaveFrameworkStateAndReboot(), SetFrameworkBootNextDevice(). I do not see any Load() functions, so how would a set of tests be resumed? If these do not apply to host based tests, should these be split out to a different lib class?
UnitTestPkg/Library/UnitTestLib
I see an implementation of MD5. We should not do our own. We should use an approved implementation such as OpenSSL.
UnitTestPkg/Library/UnitTestTerminationLibTbd
Do we really need this lib instance now?

Thanks,

Mike

From: devel@edk2.groups.io <mailto:devel@edk2.groups.io> <devel@edk2.groups.io <mailto:devel@edk2.groups.io>> On Behalf Of Bret Barkelew via Groups.Io
Sent: Thursday, November 21, 2019 11:39 PM
To: devel@edk2.groups.io <mailto:devel@edk2.groups.io>
Subject: [edk2-devel] EDK2 Host-Based Unit Test RFC (Now with docs!)

Now that CI has landed in edk2/master, we'd like to get the basic framework for unittesting staged as well. Target intercept date would be immediately after the 2019/11 stabilization, so we wanted to go ahead and get comments now.

The host unittest framework consists of five primary pieces:
- The test library (Cmocka)
- Support libraries (Found in UnitTestPkg)
- The test support plugins (HostUnitTestComilerPlugin, HostUnitTestDxeCompleteCheck, HostBasedUnitTestRunner)
- The configuration in the package-based *.ci.yaml file and package-based Test.dsc
- The tests themselves

We have a demo branch set up at:
https://github.com/corthon/edk2-staging/tree/edk2-host-test_v2
We also have a demo build pipeline at:
https://dev.azure.com/tianocore/edk2-ci-play/_build?definitionId=36&_a=summary <https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdev.azure.com%2Ftianocore%2Fedk2-ci-play%2F_build%3FdefinitionId%3D36%26_a%3Dsummary&;data=02%7C01%7CBret.Barkelew%40microsoft.com%7C3ce0b4eaf6d14de8822808d769f2c5fd%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637094365265670742&amp;sdata=HFBmk%2FdB5pXI3exxB82pTS1oF877fLsrrdcirOzCCw0%3D&amp;reserved=0>

The current demo branch contains a single test in MdePkg for SafeIntLib (module file MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.inf). This test is automatically detected by the HostUnitTestComilerPlugin and run by the HostBasedUnitTestRunner as part of the CI process.

A few notes about the current demo:
1) The demo currently only works on Windows build chains, but there's no reason to believe that it can't work equally well on Linux build chains, and are happy to work with anyone to get it there.

2) The demo currently has four failing conditions that can be seen towards the end of MdePkg "Build and Test" log file for this build:
https://dev.azure.com/tianocore/edk2-ci-play/_build/results?buildId=2813 <https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdev.azure.com%2Ftianocore%2Fedk2-ci-play%2F_build%2Fresults%3FbuildId%3D2590&;data=02%7C01%7CBret.Barkelew%40microsoft.com%7C3ce0b4eaf6d14de8822808d769f2c5fd%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637094365265670742&amp;sdata=HgPEoYg%2Fpx0fv4J5ULO1p0kvLfqcySkJHAYxp9GB598%3D&amp;reserved=0>
"WARNING - Test SafeInt16ToChar8 - Status
d:\a\1\s\MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.c:302: error: Failure!
WARNING - TestBaseSafeIntLib.exe Test Failed
WARNING - Test SafeInt32ToChar8 - Status
d:\a\1\s\MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.c:638: error: Failure!
WARNING - TestBaseSafeIntLib.exe Test Failed
WARNING - Test SafeIntnToChar8 - Status
d:\a\1\s\MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.c:1051: error: Failure!
WARNING - TestBaseSafeIntLib.exe Test Failed
WARNING - Test SafeInt64ToChar8 - Status
d:\a\1\s\MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.c:1456: error: Failure!"
These failures seem to be legitimate and further work should be done by the community to decide whether the test case is correct or the library is correct, but one of them needs to change.

3) Current demo pulls in test collateral from a fork of the edk2-test repo. This repo is currently *very* heavy due to the history of the UEFI SCT project and the number of binaries that it pulls down. It's possible that we (the community) need to select a better place for this code to live. Maybe in edk2 primary (though it's not explicitly firmware code, so it seems unnecessary). Maybe in a new edk2-test2 repo or something like that.

There’s an RFC doc here: https://github.com/corthon/edk2-staging/blob/edk2-host-test_v2/Readme-RFC.md
And a usage guide here: https://github.com/corthon/edk2-staging/blob/edk2-host-test_v2/UnitTestPkg/ReadMe.md

Once again, would love to get this into EDK2 master after stabilization, and most of this has previously been shopped around in other discussion threads. This is just where the rubber meets the road and is the minimal subset of code that needs to land for folks to start contributing unittests against the core libraries that can be run as part of any CI pass.

Thanks!
- Bret


Re: EDK2 Host-Based Unit Test RFC (Now with docs!)

Michael D Kinney
 

Hi Bret,

Thanks for posting this content. Host based unit testing is a very valuable addition to the CI checks.

I have the following comments:


1. I see that MdePkg adds a dependency on UnitTestPkg. This makes UnitTestPkg the root package when building and running host based unit tests. This makes sense, but good to highlight that all packages that use host based tests will introduce a new package dependency on UnitTestPkg.
* Since the dependency only applies to unit tests, can we update the DependencyCheck plugin to support listing dependencies for FW components separate from dependencies for unit tests?
2. I see UnitTestPkg declares 6 new lib classes. Are all 6 classes intended to be used directly from a unit test case? If some of these are only intended to be used from the framework inside the UnitTestPkg can we make them private?
3. In the MdePkg, I see a new top level directory called 'HostLibrary'. Since these lib instances are only used from a host based test, can they be moved into the 'Test' directory?
4. MdePkg/MdePkgTest.dsc.
* Can this DSC file be moved into the 'Test' directory?
* I see this DSC file only supports VS today. How much work is it to add GCC support?
5. MdePkg/HostLibrary/BaseLibHost
* Why are there 2 INFs. One with ASM and one without ASM? Can we just use the one with ASM and assume NASM is installed?
* I see the purpose of this lib instance is to call the
* SetJump()/LongJump(). So these implementations always work? It looks like it assumes BASE_LIBRARY_JUMP_BUFFER is identical to the host OS user mode application setjmp()/longjmp() state.
* Why are DRx and CRx registers emulated? I would think and code that depends on read/writing these registers would not be compatible with host based testing. Can we change to ASSERT (FALSE)?
* PatchInstructionX86() - I suspect this will not work in a host based environment because it is self modifying code. Should it be ASSERT (FALSE)?
6. MdePkg/HostLibrary/DebugLibHost
* What is '#ifndef TEST_WITH_KLEE'
* What is the 'PatchFormat()' function? It is always disabled with if (0).
* Are the PCDs to set the debug message levels disabled on purpose? (DebugPrintEnabled(), DebugPrintLevelEnabled(), DebugCodeEnabled())
7. MdePkg/HostLibrary/BaseMemoryLibHost
* Why can't we use MdePkg/Library/BaseMemoryLib/BaseMemoryLib/inf instead and reduce the number of host specific lib instances?
8. MdePkg/HostLibrary/MemoryAllocationLibHost
* Why is are memcpy(), assert(), memset() used in this lib? I would expect this lib instance to match the UefiMemoryAllocationLib with the only the use of malloc() and free() to replace the UEFI Boot Services calls.
9. Host library instance naming conventions.
* The current naming convention uses the environment as a prefix(e.g. Pei, Smm, Uefi) and the services the lib instance uses as a post fix. Would it make more sense to use 'Host' as a prefix instead of a postfix in the lib instance names?
10. Unicode vs ASCII strings
* I see InitUnitTestFramework(), CreateUnitTestSuite(), and AddTestCase() all take Unicode strings for the labels. I also see extra code to convert gEfiCallerBaseName from ASCII to Unicode to use it as a short name of a test framework. I think it would be simpler if the parameters to these APIs were ASCII and the framework can convert to Unicode if needed.
11. Will InitUnitTestFramework() and CreateUnitTestSuite() always be called in pairs? If so, can we combine these to a single API?
* I see the SafeIntLib example create a single framework and multiple test suites. Perhaps we can have a single CreateUnitTestSuite() API where Framework is an IN/OUT and if it is passed in as NULL, the Framework handle is created.
* I see a pattern where the CreateUnitTestSuite() 'Package' parameter is used as a prefix to every call to AddTestCase() in the 'ClassName' parameter. Can we simplify AddTestCase() so it only need to pass in the name of the unit test case, and the framework can append Package and the unit test case name?
12. I see the use of the 'Fw' variable as a shorthand for 'Framework'. Can we use something other than 'Fw'. It may be confused with 'Firmware'.
13. UnitTestPkg/Include/UnitTestTypes.h
* I see several hard coded string lengths. Since this runs in a host environment and strings can always be allocated, can the hard coded lengths be removed? Update the structs to use pointers to strings instead of string arrays, and the framework can manage alloc() and free()?
* How are Fingerprints used? The idea of using as hash as a unique identifier is a good idea. How is the hash calculated? What unit test code artifacts are used so developers know what parameters must be unique? Can we just decide to use a specific hash algorithm/size and the structure can be a pointer to an allocated buffer instead of a fixed size array to make it easy to change the algorithm/size in the future?
* Update all the strings to be ASCII? See Unicode vs ASCII above.
* UNIT_TEST_SAVE_TEST - The last field is a variable sized array, so it can be a formal field.
* UNIT_TEST_SAVE_CONTEXT - - The last field is a variable sized array, so it can be a formal field.
* UNIT_TEST_SAVE_HEADER - Can the last 3 fields be changed to pointers and be formal fields?
* Do the structures really need to be packed? Specially with the changes suggested above? Is the intent to potentially share data stored on disk between different host execution environments that may have different width architectures?
14. UnitTestPkg - UnitTestLib.h
* Can you provide more context for the APIs SaveFrameworkState(), SaveFrameworkStateAndQuit(), SaveFrameworkStateAndReboot(), SetFrameworkBootNextDevice(). I do not see any Load() functions, so how would a set of tests be resumed? If these do not apply to host based tests, should these be split out to a different lib class?
15. UnitTestPkg/Library/UnitTestLib
* I see an implementation of MD5. We should not do our own. We should use an approved implementation such as OpenSSL.
16. UnitTestPkg/Library/UnitTestTerminationLibTbd
* Do we really need this lib instance now?

Thanks,

Mike

From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Bret Barkelew via Groups.Io
Sent: Thursday, November 21, 2019 11:39 PM
To: devel@edk2.groups.io
Subject: [edk2-devel] EDK2 Host-Based Unit Test RFC (Now with docs!)

Now that CI has landed in edk2/master, we'd like to get the basic framework for unittesting staged as well. Target intercept date would be immediately after the 2019/11 stabilization, so we wanted to go ahead and get comments now.

The host unittest framework consists of five primary pieces:
- The test library (Cmocka)
- Support libraries (Found in UnitTestPkg)
- The test support plugins (HostUnitTestComilerPlugin, HostUnitTestDxeCompleteCheck, HostBasedUnitTestRunner)
- The configuration in the package-based *.ci.yaml file and package-based Test.dsc
- The tests themselves

We have a demo branch set up at:
https://github.com/corthon/edk2-staging/tree/edk2-host-test_v2
We also have a demo build pipeline at:
https://dev.azure.com/tianocore/edk2-ci-play/_build?definitionId=36&_a=summary<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdev.azure.com%2Ftianocore%2Fedk2-ci-play%2F_build%3FdefinitionId%3D36%26_a%3Dsummary&;data=02%7C01%7CBret.Barkelew%40microsoft.com%7C3ce0b4eaf6d14de8822808d769f2c5fd%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637094365265670742&amp;sdata=HFBmk%2FdB5pXI3exxB82pTS1oF877fLsrrdcirOzCCw0%3D&amp;reserved=0>

The current demo branch contains a single test in MdePkg for SafeIntLib (module file MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.inf). This test is automatically detected by the HostUnitTestComilerPlugin and run by the HostBasedUnitTestRunner as part of the CI process.

A few notes about the current demo:
1) The demo currently only works on Windows build chains, but there's no reason to believe that it can't work equally well on Linux build chains, and are happy to work with anyone to get it there.

2) The demo currently has four failing conditions that can be seen towards the end of MdePkg "Build and Test" log file for this build:
https://dev.azure.com/tianocore/edk2-ci-play/_build/results?buildId=2813<https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdev.azure.com%2Ftianocore%2Fedk2-ci-play%2F_build%2Fresults%3FbuildId%3D2590&;data=02%7C01%7CBret.Barkelew%40microsoft.com%7C3ce0b4eaf6d14de8822808d769f2c5fd%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637094365265670742&amp;sdata=HgPEoYg%2Fpx0fv4J5ULO1p0kvLfqcySkJHAYxp9GB598%3D&amp;reserved=0>
"WARNING - Test SafeInt16ToChar8 - Status
d:\a\1\s\MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.c:302: error: Failure!
WARNING - TestBaseSafeIntLib.exe Test Failed
WARNING - Test SafeInt32ToChar8 - Status
d:\a\1\s\MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.c:638: error: Failure!
WARNING - TestBaseSafeIntLib.exe Test Failed
WARNING - Test SafeIntnToChar8 - Status
d:\a\1\s\MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.c:1051: error: Failure!
WARNING - TestBaseSafeIntLib.exe Test Failed
WARNING - Test SafeInt64ToChar8 - Status
d:\a\1\s\MdePkg\Test\UnitTest\Library\BaseSafeIntLib\TestBaseSafeIntLib.c:1456: error: Failure!"
These failures seem to be legitimate and further work should be done by the community to decide whether the test case is correct or the library is correct, but one of them needs to change.

3) Current demo pulls in test collateral from a fork of the edk2-test repo. This repo is currently *very* heavy due to the history of the UEFI SCT project and the number of binaries that it pulls down. It's possible that we (the community) need to select a better place for this code to live. Maybe in edk2 primary (though it's not explicitly firmware code, so it seems unnecessary). Maybe in a new edk2-test2 repo or something like that.
There's an RFC doc here: https://github.com/corthon/edk2-staging/blob/edk2-host-test_v2/Readme-RFC.md
And a usage guide here: https://github.com/corthon/edk2-staging/blob/edk2-host-test_v2/UnitTestPkg/ReadMe.md

Once again, would love to get this into EDK2 master after stabilization, and most of this has previously been shopped around in other discussion threads. This is just where the rubber meets the road and is the minimal subset of code that needs to land for folks to start contributing unittests against the core libraries that can be run as part of any CI pass.

Thanks!
- Bret


Re: [edk2-devel] EDK II Stable Tag edk2-stable201911 will be created based on commit bd85bf54c268204c7a698a96f3ccd96cd77952cd

Laszlo Ersek
 

On 11/29/19 07:18, Liming Gao via Groups.Io wrote:
Hi, all

Today, I review all patches in edk2 mail list. There is no patches for EDK II Stable Tag edk2-stable201911. Based on edk2-stable201911 tag planning, it will be released at 2019-11-29. So, I plan to create edk2-stable201911 based on current edk2 trunk (the latest commit https://github.com/tianocore/edk2/commit/bd85bf54c268204c7a698a96f3ccd96cd77952cd MdeModulePkg/Variable: Initialize local variable "RtPtrTrack"). If you have any comments, please reply the mail. If no concern or objection, I will create tag and send another announce mail that edk2-stable201911 is completed and normal commit is resumed.
No objection from me.

Thank you for managing the SFF / HFF process, and the release, again,
Liming!

Laszlo


EDK II Stable Tag edk2-stable201911 will be created based on commit bd85bf54c268204c7a698a96f3ccd96cd77952cd

Liming Gao
 

Hi, all

Today, I review all patches in edk2 mail list. There is no patches for EDK II Stable Tag edk2-stable201911. Based on edk2-stable201911 tag planning, it will be released at 2019-11-29. So, I plan to create edk2-stable201911 based on current edk2 trunk (the latest commit https://github.com/tianocore/edk2/commit/bd85bf54c268204c7a698a96f3ccd96cd77952cd MdeModulePkg/Variable: Initialize local variable "RtPtrTrack"). If you have any comments, please reply the mail. If no concern or objection, I will create tag and send another announce mail that edk2-stable201911 is completed and normal commit is resumed.

Thanks
Liming


Re: Unified API for Hashing Algorithms in EDK2

Sukerkar, Amol N
 

Hi Mike,

I think the point # 2 you are making is also the implementation we have proposed.

Also, if we can have a two tier approach then we may be in a position to improve code flexibility:
1. Define FixedAtBuild PCD at SecurityPkg DSC file that can be used by each module intending to use system level hashing algorithm.

2. For modules that need a module-specific hashing algorithm, override the PCD at SecurityPkg DSC file with a FIxedAtBuild PCD at platform level DSC file. This can eliminate the need for each module to be forced to define the PCD at platform level.

Thanks,
Amol

-----Original Message-----
From: Kinney, Michael D <michael.d.kinney@...>
Sent: Tuesday, November 26, 2019 6:24 PM
To: Wang, Jian J <jian.j.wang@...>; Sukerkar, Amol N <amol.n.sukerkar@...>; rfc@edk2.groups.io; sean.brogan@...; Desimone, Nathaniel L <nathaniel.l.desimone@...>; Matthew Carlson <macarl@...>; Gao, Liming <liming.gao@...>; Feng, Bob C <bob.c.feng@...>; Lu, XiaoyuX <xiaoyux.lu@...>; Kinney, Michael D <michael.d.kinney@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>
Subject: RE: [edk2-rfc] Unified API for Hashing Algorithms in EDK2

As Sean points out, different components in a platform may have different hash requirements.

If we want to go down the path where we have a few modules that produce Protocols/PPIs for the BaseCryptLib so all modules can share the same crypto services, then each module need to decide which specific hash algorithm it requires.

Instead of defining another protocol/PPI for a unified hash service, we could consider a couple of other
options:

1) Each module that uses hash services could use
a FixedAtBuild PCD to decide which hash algorithm
it should use. Based on this PCD value, different
APIs in BaseCryptLib can be called. Since it is
a FixedAtBuild PCD, the compiler/linker optimizations
will only include the one hash algorithm selected.
by using BaseCryptLib, this is compatible with the
proposal to wrap BaseCryptLib in Protocols/PPIs.

2) If we have many modules that need to make a hash
algorithm selection, then define a new library
class with the unified hash APIs, and the
library instance uses a FixedAtBuild PCD to configure
the hash algorithm used. This is also compatible
with the proposal to wrap BaseCryptLib in Protocols/PPIs
and minimizes the sources in modules that need
a design that supports flexible hash algorithm selection.
If new hash algorithm is added, then only the lib
instance and PCD need to be updated.

Each module sets the FixedAtBuild PCD value in the scope of the module in the platform DSC file. If most or all modules in a platform need the same hash algorithm, then the FixedAtBuild PCD can be set at the platform scope in the platform DSC file.

Best regards,

Mike







-----Original Message-----
From: Wang, Jian J <jian.j.wang@...>
Sent: Monday, November 25, 2019 9:09 PM
To: Sukerkar, Amol N <amol.n.sukerkar@...>; rfc@edk2.groups.io;
sean.brogan@...; Kinney, Michael D
<michael.d.kinney@...>; Desimone, Nathaniel L
<nathaniel.l.desimone@...>; Matthew Carlson
<macarl@...>; Gao, Liming <liming.gao@...>; Feng, Bob
C <bob.c.feng@...>; Lu, XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>
Subject: RE: [edk2-rfc] Unified API for Hashing Algorithms in EDK2

Hi Sean and Amol,

I believe the OBB and FV/PE image verification can still benefit from
the unified API, if we add one more parameter to HashApiIinit() to
force using a hash algorithm or add a PCD to choose one or link only
required HashApiInstanceXxx.

(Just for example)
HashApiInit (
IN EFI_GUID *HashAlg,
OUT HASH_HANDLE *HashHandle
)

In this way, we can avoid hard-code hash API in the verification
module. Although it's slow, I think the OBB verification or FV/PE
image signature algorithm is still subject to change per security
requirement change. During a transition period, we don't have to add a
new code to handle a different algorithm in verification module.
Instead we just need to link a different hash lib or set PCD to use a
different algorithm. I think this makes things simpler and easier to
change.

The BaseCryptLib implemented in PPI/Protocol is a good idea to save
image size. I think the Unified hash API
(BaseHashLib) can make use of it to save extra hash algorithm library
instances (HashApiInstanceSha1, Sha256,...).
The only problem is that the structured PCD, like Mike mentioned
before, doesn't support module override, which prevents different
modules from using different algorithms and then cannot save module
size utter mostly.


Regards,
Jian

-----Original Message-----
From: Sukerkar, Amol N <amol.n.sukerkar@...>
Sent: Tuesday, November 26, 2019 5:53 AM
To: rfc@edk2.groups.io; sean.brogan@...;
Kinney, Michael D
<michael.d.kinney@...>; Desimone, Nathaniel L
<nathaniel.l.desimone@...>; Matthew Carlson
<macarl@...>; Gao, Liming
<liming.gao@...>; Feng, Bob
C <bob.c.feng@...>; Wang, Jian J
<jian.j.wang@...>; Lu,
XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>
Subject: RE: [edk2-rfc] Unified API for Hashing
Algorithms in EDK2

Hi Sean,

A few examples that we want to target are
- HDD Password verification
- Capsule Verification (Platform recovery)
- Bios Guard Platform Data Table
- More importantly, Boot Guard Event Log driver that
has a bunch of
redundant code today calculating various hashes. I
don't think this is open source though.

Currently, the hash verification mechanism in the
above features is
hard-coded to SHA256 which we will need to change to
SHA384 to support
a more robust hashing mechanism. This is a good
opportunity to replace
the existing SHA256 API with an abstracted unified
API. And if
necessary, the abstracted unified API still supports
driver specific
hashing algorithm if needed (as opposed to the
strongest hashing algorithm supported by the system).

We are not targeting verification of OBB or for that
matter any
PE/COFF image with this mechanism, mainly because it
has it's own
wrapper and verification mechanism in
DxeImageVerificationLib.

Thanks,
Amol

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On
Behalf Of Sean via
Groups.Io
Sent: Monday, November 25, 2019 1:24 PM
To: rfc@edk2.groups.io; Sukerkar, Amol N
<amol.n.sukerkar@...>;
Kinney, Michael D <michael.d.kinney@...>;
Desimone, Nathaniel L
<nathaniel.l.desimone@...>; Matthew Carlson
<macarl@...>; Gao, Liming
<liming.gao@...>; Feng, Bob
C <bob.c.feng@...>; Wang, Jian J
<jian.j.wang@...>; Lu,
XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>
Subject: Re: [edk2-rfc] Unified API for Hashing
Algorithms in EDK2

Amol,

I am interested to hear more about the actual use
cases. I like the
idea of abstracting the API so that a calling driver
doesn't have to
change when the hashing algorithm requirements change
but am not
convinced that this will actually be helpful (or at
least worth the
introduction of more complexity in security sensitive
code).

For the TPM and measured boot I agree. Random driver
needs to extend
a measurement then it can use the existing hashlib
and it will be hashed and
extended per platform policy. This is because the
random driver doesn't need
to understand the result. Only the TPM needs to and
that is handled
within the extend operation.

For the other drivers that I have seen using hashing,
they need to
understand the result and make driver specific
decisions based on the
result. For example if I am verifying the OBB or
some FV then I must
make sure the known good and computed match. This
driver must locate
the known good and thus this process generally must
know the format of
the result. For this I think it is best to know what
algorithm I am
using and I don't see a strong use case to change
algorithm to an abstracted algorithm.

Do you have a set of example drivers/use cases that
could use
abstracted hash support?

Thanks
Sean



-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On
Behalf Of Sukerkar,
Amol N via Groups.Io
Sent: Monday, November 25, 2019 12:08 PM
To: rfc@edk2.groups.io; Sukerkar, Amol N
<amol.n.sukerkar@...>;
Kinney, Michael D <michael.d.kinney@...>;
Desimone, Nathaniel L
<nathaniel.l.desimone@...>; Sean Brogan
<sean.brogan@...>; Matthew Carlson
<macarl@...>;
Gao, Liming <liming.gao@...>; Feng, Bob C
<bob.c.feng@...>; Wang, Jian J
<jian.j.wang@...>; Lu,
XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>;
Sukerkar, Amol N
<amol.n.sukerkar@...>
Subject: [EXTERNAL] Re: [edk2-rfc] Unified API for
Hashing Algorithms
in EDK2

Hi Mike and Nate,

With our implementation we are trying to address the
following in EDKII:
1. A common Hashing API for UEFI drivers to consume
instead of the
current API that directly calls into the specific
hashing algorithm,
for instance, SHA1_Init, SHA256_Update, etc. Due to a
stronger hashing
algorithm requirement, certain drivers need to be
upgraded to SHA384
from SHA256 and the unified API is proposed, so,
while upgrading we
remove the hard-coded dependency on each hashing
algorithm API.

2. The size issue as a result of statically linking
openssl/cryptolib
API to the consuming driver. There are two ways we
tried to address
this issue, although, we didn't explicitly defined a
PPI to address it.
a. Introduced a fixed PCD per hashing algorithm to
include/exclude a
particular algorithm from BIOS.
b. Use a register mechanism to link the instance of a
particular
hashing algorithm to Unified API library. This is
configurable using PCD.

Note that 2.a and 2.b implementation is very similar
to
HashLibBaseCryptoRouter used for TPM2 operations.

From the size and static linking perspective, I
believe we can have a
discussion between 2.a and 2.b with Mike's
implementation(https://nam06.safelinks.protection.outlo
ok.com/?url=htt
ps%3A
%2F%2Fgithub.com%2Fmdkinney%2Fedk2%2Ftree%2FCryptoPkg_P
PI_Protocol_
Proposal_V5&amp;data=02%7C01%7Csean.brogan%40microsoft.
com%7Cd110f
db2a91f4e8c2acb08d771e33390%7C72f988bf86f141af91ab2d7cd
011db47%7C1
%7C0%7C637103092953247213&amp;sdata=2acK0dmJZ85aZ3LvOOq
D3e3w44V
EM5dW8TEhs9USyDM%3D&amp;reserved=0) below.

We still need a common hashing API so the hashing
algorithm strength
for a particular driver can be increased without
having to modify the
consumer driver code.

Comments are welcome.

Thanks,
Amol

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On
Behalf Of Sukerkar,
Amol N
Sent: Thursday, November 21, 2019 4:45 PM
To: rfc@edk2.groups.io; Kinney, Michael D
<michael.d.kinney@...>; Desimone, Nathaniel L
<nathaniel.l.desimone@...>;
'sean.brogan@...'
<sean.brogan@...>; Matthew Carlson
<macarl@...>;
Gao, Liming <liming.gao@...>; Feng, Bob C
<bob.c.feng@...>; Wang, Jian J
<jian.j.wang@...>; Lu,
XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>;
Sukerkar, Amol N
<amol.n.sukerkar@...>
Subject: Re: [edk2-rfc] Unified API for Hashing
Algorithms in EDK2

Thanks, Nate and Mike!

I am going through the code and comments and will
respond shortly.

In the meantime, here is the GitHub link to my PoC
for the community
to look at and comment:
https://nam06.safelinks.protection.outlook.com/?url=htt
ps%3A%2F%2Fgith
ub.c
om%2Fansukerk%2Fedk2&amp;data=02%7C01%7Csean.brogan%40m
icrosoft.c
om%7Cd110fdb2a91f4e8c2acb08d771e33390%7C72f988bf86f141a
f91ab2d7cd
011db47%7C1%7C0%7C637103092953247213&amp;sdata=keRdfU%2
BxffkBsyv
YHwvSOIA8ryexOigiKhR%2B%2F1q7DjE%3D&amp;reserved=0.

Best regards,
Amol

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On
Behalf Of Michael D
Kinney
Sent: Thursday, November 21, 2019 4:37 PM
To: rfc@edk2.groups.io; Desimone, Nathaniel L
<nathaniel.l.desimone@...>; Sukerkar, Amol N
<amol.n.sukerkar@...>; Kinney, Michael D
<michael.d.kinney@...>;
'sean.brogan@...'
<sean.brogan@...>; Matthew Carlson
<macarl@...>;
Gao, Liming <liming.gao@...>; Feng, Bob C
<bob.c.feng@...>; Wang, Jian J
<jian.j.wang@...>; Lu,
XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>
Subject: Re: [edk2-rfc] Unified API for Hashing
Algorithms in EDK2

Nate and Amol,

There is some work already started by Sean and Matt
that implements a
PEIM, DXE Driver, and SMM Driver to produce
Protocol/PPI that wraps
the BaseCryptLib services. This content broken out
into its own
package is available
here:


https://nam06.safelinks.protection.outlook.com/?url=htt
ps%3A%2F%2Fgith
ub.c
om%2Fmicrosoft%2Fmu_plus%2Ftree%2Fdev%2F201908%2FShared
CryptoPkg&
amp;data=02%7C01%7Csean.brogan%40microsoft.com%7Cd110fd
b2a91f4e8c2
acb08d771e33390%7C72f988bf86f141af91ab2d7cd011db47%7C1%
7C0%7C637
103092953247213&amp;sdata=88TRhUqsVrj7sx8P4hjbdLy2%2BDK
nrBoi7HAI3G
UhpPo%3D&amp;reserved=0

I have ported and simplified this content into a
proposed set of
patches to the CryptoPkg. It uses a structured PCD
to configure the
services mapped into the Protocols/PPIs and avoids
the issue Nate
notes below with protocols and PPIs including all of
the BaseCryptLib
services. The structured PCD allows families of
crypto services or individual services within a family to be
enabled/disabled.


https://nam06.safelinks.protection.outlook.com/?url=htt
ps%3A%2F%2Fgith
ub.c
om%2Fmdkinney%2Fedk2%2Ftree%2FCryptoPkg_PPI_Protocol_Pr
oposal_V5&a
mp;data=02%7C01%7Csean.brogan%40microsoft.com%7Cd110fdb
2a91f4e8c2a
cb08d771e33390%7C72f988bf86f141af91ab2d7cd011db47%7C1%7
C0%7C6371
03092953247213&amp;sdata=2acK0dmJZ85aZ3LvOOqD3e3w44VEM5
dW8TEhs9
USyDM%3D&amp;reserved=0

For example, the DSC file PCD statements to enable
the SHA1 family and
SHA256 family of hash services with the HashAll
service disabled is:

[PcdsFixedAtBuild]
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnabl
e.Sha1.Family
| PCD_CRYPTO_SERVICE_ENABLE_FAMILY

gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnabl
e.Sha1.Services.Has
hAll | FALSE

gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnabl
e.Sha256.Family
| PCD_CRYPTO_SERVICE_ENABLE_FAMILY

gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnabl
e.Sha256.Services.
HashAll | FALSE

Please take a look at this proposal and let me know
if this can be used to address.


https://nam06.safelinks.protection.outlook.com/?url=htt
ps%3A%2F%2Fbugzilla.
tianocore.org%2Fshow_bug.cgi%3Fid%3D2151&amp;data=02%7C
01%7Csean.b
rogan%40microsoft.com%7Cd110fdb2a91f4e8c2acb08d771e3339
0%7C72f988b
f86f141af91ab2d7cd011db47%7C1%7C0%7C637103092953247213&
amp;sdata
=oTt8U7ZLy3tdb3bfslqeCcDxwR1FEkNv%2F4YVcCWl3Ps%3D&amp;r
eserved=0

There is currently a limitation in the structured PCD
feature that
does not allow the structured PCD field values to be
set in the scope
of a module in a <PcdsFixedAtBuild> section. To work
around this
limitation, the CryptoPkg DSC file has a define
called CRYPTO_SERVICES
that can be set to ALL, NONE, MIN_PEI, or
MIN_DXE_MIN_SMM. The
default is ALL. Building with each of these values
will build the
modules with different sets of enabled services that
matches the
services enabled using multiple modules in the work
from Sean and
Matt. If this limitation is addressed in BaseTools,
then CryptoPkg could remove the CRYPTO_SERVIES define and all !if
statements.

Thanks,

Mike

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On
Behalf Of Nate
DeSimone
Sent: Wednesday, November 20, 2019 10:31 PM
To: rfc@edk2.groups.io; Sukerkar, Amol N
<amol.n.sukerkar@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>;
Wang, Jian J
<jian.j.wang@...>
Subject: Re: [edk2-rfc] Unified API for Hashing
Algorithms in EDK2

Hi Amol,

With regard to verifying code hashes, it makes much
more sense to me
to have a GUID defined that represents the hash
function to use
versus a PCD. The reason for this is the method for
signing Firmware
Volumes typically involves using nested FVs where
the GUID for the
nested FV type indicates the algorithm needed to
"extract" the nested FV.
Extraction in this context is used to verify code
hashes as well as
decompress compressed code. The "extractor" is
usually a statically
linked library attached to either
SignedSectionExtractPei and/or
DxeIpl.

For example, it is very possible for a platform to
use more than one
hashing algorithm, I can think of more than one
closed source
platform at Intel that actually does this.

Now, one thing that I think your document implies
is that we should
have better APIs for crypto operations in general.
If that was the
intent then I totally agree with you. There are
several reasons that
I think this should be done. First, the API for
OpenSSL has a
tendency to change often enough to make integrating
new versions
into older codebases a headache, putting an
abstraction layer on it
would be helpful. Second, the method used to
consume OpenSSL in edk2
causes some technical issues. Specifically, OpenSSL
is statically
linked and it defines a crazy number of global
variables, enough so
that on a debug build I have noticed roughly a 20KB
increase in
binary size even if you don't call a single
function in OpenSSL.
Wrapping the crypto operations in a PPI/Protocol so
that only 1
driver needs to link to OpenSSL would probably help
reduce binary
size for most implementations. Your current
document does not define
a PPI/Protocol, that is something I would
recommend.

One issue that prior attempts of wrapping OpenSSL
in a PPI/Protocol
have run into is that all the crypto functions need
to be linked,
even the ones that a platform does not use, which
results in a net
negative binary size impact. Accordingly, one would
likely need to
break the crypto functionality into multiple
PPIs/Protocols so that
only the functions a platform needs are actually
included. I think
some experiments need to be done to figure out the
most optimal
division of crypto APIs.

Hope that helps,
Nate

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On
Behalf Of Sukerkar,
Amol N
Sent: Wednesday, November 20, 2019 8:28 AM
To: rfc@edk2.groups.io
Cc: Agrawal, Sachin <sachin.agrawal@...>;
Wang, Jian J
<jian.j.wang@...>; Sukerkar, Amol N
<amol.n.sukerkar@...>; Sukerkar, Amol N
<amol.n.sukerkar@...>
Subject: [edk2-rfc] Unified API for Hashing
Algorithms in EDK2
Importance: High

Hello,

Currently the UEFI drivers using the SHA/SM3
hashing algorithms use
hard-coded API to calculate the hash, such as,
sha_256(...), etc.
Since SHA384 and/or SM3 are being increasingly
adopted, it becomes
cumbersome to modify the driver with SHA384 or SM3
calls for each
application.

To better achieve this, we are proposing a unified
API which can be
used by UEFI drivers that provides the drivers with
flexibility to
use the hashing algorithm they desired or the
strongest hashing
algorithm the system supports (with openssl).
Attached is the design
proposal for the same and we request feedback from
the community
before we begin the process of making the changes
to EDK2 repo.

Alternatively, the design document is also attached
to Bugzilla,
https://nam06.safelinks.protection.outlook.com/?url=htt
ps%3A%2F%2Fbugzilla.
tianocore.org%2Fshow_bug.cgi%3Fid%3D2151&amp;data=02%7C
01%7Csean.b
rogan%40microsoft.com%7Cd110fdb2a91f4e8c2acb08d771e3339
0%7C72f988b
f86f141af91ab2d7cd011db47%7C1%7C0%7C637103092953247213&
amp;sdata
=oTt8U7ZLy3tdb3bfslqeCcDxwR1FEkNv%2F4YVcCWl3Ps%3D&amp;r
eserved=0.
You can also provide the feedback in the Bugzilla.

Thanks,
Amol
















Re: Unified API for Hashing Algorithms in EDK2

Michael D Kinney
 

As Sean points out, different components in a platform
may have different hash requirements.

If we want to go down the path where we have a few modules
that produce Protocols/PPIs for the BaseCryptLib so all
modules can share the same crypto services, then each
module need to decide which specific hash algorithm it
requires.

Instead of defining another protocol/PPI for a unified
hash service, we could consider a couple of other
options:

1) Each module that uses hash services could use
a FixedAtBuild PCD to decide which hash algorithm
it should use. Based on this PCD value, different
APIs in BaseCryptLib can be called. Since it is
a FixedAtBuild PCD, the compiler/linker optimizations
will only include the one hash algorithm selected.
by using BaseCryptLib, this is compatible with the
proposal to wrap BaseCryptLib in Protocols/PPIs.

2) If we have many modules that need to make a hash
algorithm selection, then define a new library
class with the unified hash APIs, and the
library instance uses a FixedAtBuild PCD to configure
the hash algorithm used. This is also compatible
with the proposal to wrap BaseCryptLib in Protocols/PPIs
and minimizes the sources in modules that need
a design that supports flexible hash algorithm selection.
If new hash algorithm is added, then only the lib
instance and PCD need to be updated.

Each module sets the FixedAtBuild PCD value in the scope
of the module in the platform DSC file. If most or all
modules in a platform need the same hash algorithm, then
the FixedAtBuild PCD can be set at the platform scope in
the platform DSC file.

Best regards,

Mike

-----Original Message-----
From: Wang, Jian J <jian.j.wang@...>
Sent: Monday, November 25, 2019 9:09 PM
To: Sukerkar, Amol N <amol.n.sukerkar@...>;
rfc@edk2.groups.io; sean.brogan@...; Kinney,
Michael D <michael.d.kinney@...>; Desimone,
Nathaniel L <nathaniel.l.desimone@...>; Matthew
Carlson <macarl@...>; Gao, Liming
<liming.gao@...>; Feng, Bob C
<bob.c.feng@...>; Lu, XiaoyuX
<xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>
Subject: RE: [edk2-rfc] Unified API for Hashing
Algorithms in EDK2

Hi Sean and Amol,

I believe the OBB and FV/PE image verification can
still benefit from the unified API, if we add one more
parameter to HashApiIinit() to force using a hash
algorithm or add a PCD to choose one or link only
required HashApiInstanceXxx.

(Just for example)
HashApiInit (
IN EFI_GUID *HashAlg,
OUT HASH_HANDLE *HashHandle
)

In this way, we can avoid hard-code hash API in the
verification module. Although it's slow, I think the
OBB verification or FV/PE image signature algorithm is
still subject to change per security requirement
change. During a transition period, we don't have to
add a new code to handle a different algorithm in
verification module. Instead we just need to link a
different hash lib or set PCD to use a different
algorithm. I think this makes things simpler and easier
to change.

The BaseCryptLib implemented in PPI/Protocol is a good
idea to save image size. I think the Unified hash API
(BaseHashLib) can make use of it to save extra hash
algorithm library instances (HashApiInstanceSha1,
Sha256,...).
The only problem is that the structured PCD, like Mike
mentioned before, doesn't support module override,
which prevents different modules from using different
algorithms and then cannot save module size utter
mostly.


Regards,
Jian

-----Original Message-----
From: Sukerkar, Amol N <amol.n.sukerkar@...>
Sent: Tuesday, November 26, 2019 5:53 AM
To: rfc@edk2.groups.io; sean.brogan@...;
Kinney, Michael D
<michael.d.kinney@...>; Desimone, Nathaniel L
<nathaniel.l.desimone@...>; Matthew Carlson
<macarl@...>; Gao, Liming
<liming.gao@...>; Feng, Bob
C <bob.c.feng@...>; Wang, Jian J
<jian.j.wang@...>; Lu,
XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>
Subject: RE: [edk2-rfc] Unified API for Hashing
Algorithms in EDK2

Hi Sean,

A few examples that we want to target are
- HDD Password verification
- Capsule Verification (Platform recovery)
- Bios Guard Platform Data Table
- More importantly, Boot Guard Event Log driver that
has a bunch of
redundant code today calculating various hashes. I
don't think this is open source though.

Currently, the hash verification mechanism in the
above features is
hard-coded to SHA256 which we will need to change to
SHA384 to support
a more robust hashing mechanism. This is a good
opportunity to replace
the existing SHA256 API with an abstracted unified
API. And if
necessary, the abstracted unified API still supports
driver specific
hashing algorithm if needed (as opposed to the
strongest hashing algorithm supported by the system).

We are not targeting verification of OBB or for that
matter any
PE/COFF image with this mechanism, mainly because it
has it's own
wrapper and verification mechanism in
DxeImageVerificationLib.

Thanks,
Amol

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On
Behalf Of Sean via
Groups.Io
Sent: Monday, November 25, 2019 1:24 PM
To: rfc@edk2.groups.io; Sukerkar, Amol N
<amol.n.sukerkar@...>;
Kinney, Michael D <michael.d.kinney@...>;
Desimone, Nathaniel L
<nathaniel.l.desimone@...>; Matthew Carlson
<macarl@...>; Gao, Liming
<liming.gao@...>; Feng, Bob
C <bob.c.feng@...>; Wang, Jian J
<jian.j.wang@...>; Lu,
XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>
Subject: Re: [edk2-rfc] Unified API for Hashing
Algorithms in EDK2

Amol,

I am interested to hear more about the actual use
cases. I like the
idea of abstracting the API so that a calling driver
doesn't have to
change when the hashing algorithm requirements change
but am not
convinced that this will actually be helpful (or at
least worth the
introduction of more complexity in security sensitive
code).

For the TPM and measured boot I agree. Random driver
needs to extend
a measurement then it can use the existing hashlib
and it will be hashed and
extended per platform policy. This is because the
random driver doesn't need
to understand the result. Only the TPM needs to and
that is handled
within the extend operation.

For the other drivers that I have seen using hashing,
they need to
understand the result and make driver specific
decisions based on the
result. For example if I am verifying the OBB or
some FV then I must
make sure the known good and computed match. This
driver must locate
the known good and thus this process generally must
know the format of
the result. For this I think it is best to know what
algorithm I am
using and I don't see a strong use case to change
algorithm to an abstracted algorithm.

Do you have a set of example drivers/use cases that
could use
abstracted hash support?

Thanks
Sean



-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On
Behalf Of Sukerkar,
Amol N via Groups.Io
Sent: Monday, November 25, 2019 12:08 PM
To: rfc@edk2.groups.io; Sukerkar, Amol N
<amol.n.sukerkar@...>;
Kinney, Michael D <michael.d.kinney@...>;
Desimone, Nathaniel L
<nathaniel.l.desimone@...>; Sean Brogan
<sean.brogan@...>; Matthew Carlson
<macarl@...>;
Gao, Liming <liming.gao@...>; Feng, Bob C
<bob.c.feng@...>; Wang, Jian J
<jian.j.wang@...>; Lu,
XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>;
Sukerkar, Amol N
<amol.n.sukerkar@...>
Subject: [EXTERNAL] Re: [edk2-rfc] Unified API for
Hashing Algorithms
in EDK2

Hi Mike and Nate,

With our implementation we are trying to address the
following in EDKII:
1. A common Hashing API for UEFI drivers to consume
instead of the
current API that directly calls into the specific
hashing algorithm,
for instance, SHA1_Init, SHA256_Update, etc. Due to a
stronger hashing
algorithm requirement, certain drivers need to be
upgraded to SHA384
from SHA256 and the unified API is proposed, so,
while upgrading we
remove the hard-coded dependency on each hashing
algorithm API.

2. The size issue as a result of statically linking
openssl/cryptolib
API to the consuming driver. There are two ways we
tried to address
this issue, although, we didn't explicitly defined a
PPI to address it.
a. Introduced a fixed PCD per hashing algorithm to
include/exclude a
particular algorithm from BIOS.
b. Use a register mechanism to link the instance of a
particular
hashing algorithm to Unified API library. This is
configurable using PCD.

Note that 2.a and 2.b implementation is very similar
to
HashLibBaseCryptoRouter used for TPM2 operations.

From the size and static linking perspective, I
believe we can have a
discussion between 2.a and 2.b with Mike's
implementation(https://nam06.safelinks.protection.outlo
ok.com/?url=htt
ps%3A
%2F%2Fgithub.com%2Fmdkinney%2Fedk2%2Ftree%2FCryptoPkg_P
PI_Protocol_
Proposal_V5&amp;data=02%7C01%7Csean.brogan%40microsoft.
com%7Cd110f
db2a91f4e8c2acb08d771e33390%7C72f988bf86f141af91ab2d7cd
011db47%7C1
%7C0%7C637103092953247213&amp;sdata=2acK0dmJZ85aZ3LvOOq
D3e3w44V
EM5dW8TEhs9USyDM%3D&amp;reserved=0) below.

We still need a common hashing API so the hashing
algorithm strength
for a particular driver can be increased without
having to modify the
consumer driver code.

Comments are welcome.

Thanks,
Amol

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On
Behalf Of Sukerkar,
Amol N
Sent: Thursday, November 21, 2019 4:45 PM
To: rfc@edk2.groups.io; Kinney, Michael D
<michael.d.kinney@...>; Desimone, Nathaniel L
<nathaniel.l.desimone@...>;
'sean.brogan@...'
<sean.brogan@...>; Matthew Carlson
<macarl@...>;
Gao, Liming <liming.gao@...>; Feng, Bob C
<bob.c.feng@...>; Wang, Jian J
<jian.j.wang@...>; Lu,
XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>;
Sukerkar, Amol N
<amol.n.sukerkar@...>
Subject: Re: [edk2-rfc] Unified API for Hashing
Algorithms in EDK2

Thanks, Nate and Mike!

I am going through the code and comments and will
respond shortly.

In the meantime, here is the GitHub link to my PoC
for the community
to look at and comment:
https://nam06.safelinks.protection.outlook.com/?url=htt
ps%3A%2F%2Fgith
ub.c
om%2Fansukerk%2Fedk2&amp;data=02%7C01%7Csean.brogan%40m
icrosoft.c
om%7Cd110fdb2a91f4e8c2acb08d771e33390%7C72f988bf86f141a
f91ab2d7cd
011db47%7C1%7C0%7C637103092953247213&amp;sdata=keRdfU%2
BxffkBsyv
YHwvSOIA8ryexOigiKhR%2B%2F1q7DjE%3D&amp;reserved=0.

Best regards,
Amol

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On
Behalf Of Michael D
Kinney
Sent: Thursday, November 21, 2019 4:37 PM
To: rfc@edk2.groups.io; Desimone, Nathaniel L
<nathaniel.l.desimone@...>; Sukerkar, Amol N
<amol.n.sukerkar@...>; Kinney, Michael D
<michael.d.kinney@...>;
'sean.brogan@...'
<sean.brogan@...>; Matthew Carlson
<macarl@...>;
Gao, Liming <liming.gao@...>; Feng, Bob C
<bob.c.feng@...>; Wang, Jian J
<jian.j.wang@...>; Lu,
XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>
Subject: Re: [edk2-rfc] Unified API for Hashing
Algorithms in EDK2

Nate and Amol,

There is some work already started by Sean and Matt
that implements a
PEIM, DXE Driver, and SMM Driver to produce
Protocol/PPI that wraps
the BaseCryptLib services. This content broken out
into its own
package is available
here:


https://nam06.safelinks.protection.outlook.com/?url=htt
ps%3A%2F%2Fgith
ub.c
om%2Fmicrosoft%2Fmu_plus%2Ftree%2Fdev%2F201908%2FShared
CryptoPkg&
amp;data=02%7C01%7Csean.brogan%40microsoft.com%7Cd110fd
b2a91f4e8c2
acb08d771e33390%7C72f988bf86f141af91ab2d7cd011db47%7C1%
7C0%7C637
103092953247213&amp;sdata=88TRhUqsVrj7sx8P4hjbdLy2%2BDK
nrBoi7HAI3G
UhpPo%3D&amp;reserved=0

I have ported and simplified this content into a
proposed set of
patches to the CryptoPkg. It uses a structured PCD
to configure the
services mapped into the Protocols/PPIs and avoids
the issue Nate
notes below with protocols and PPIs including all of
the BaseCryptLib
services. The structured PCD allows families of
crypto services or individual services within a family
to be enabled/disabled.


https://nam06.safelinks.protection.outlook.com/?url=htt
ps%3A%2F%2Fgith
ub.c
om%2Fmdkinney%2Fedk2%2Ftree%2FCryptoPkg_PPI_Protocol_Pr
oposal_V5&a
mp;data=02%7C01%7Csean.brogan%40microsoft.com%7Cd110fdb
2a91f4e8c2a
cb08d771e33390%7C72f988bf86f141af91ab2d7cd011db47%7C1%7
C0%7C6371
03092953247213&amp;sdata=2acK0dmJZ85aZ3LvOOqD3e3w44VEM5
dW8TEhs9
USyDM%3D&amp;reserved=0

For example, the DSC file PCD statements to enable
the SHA1 family and
SHA256 family of hash services with the HashAll
service disabled is:

[PcdsFixedAtBuild]
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnabl
e.Sha1.Family
| PCD_CRYPTO_SERVICE_ENABLE_FAMILY

gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnabl
e.Sha1.Services.Has
hAll | FALSE

gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnabl
e.Sha256.Family
| PCD_CRYPTO_SERVICE_ENABLE_FAMILY

gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnabl
e.Sha256.Services.
HashAll | FALSE

Please take a look at this proposal and let me know
if this can be used to address.


https://nam06.safelinks.protection.outlook.com/?url=htt
ps%3A%2F%2Fbugzilla.
tianocore.org%2Fshow_bug.cgi%3Fid%3D2151&amp;data=02%7C
01%7Csean.b
rogan%40microsoft.com%7Cd110fdb2a91f4e8c2acb08d771e3339
0%7C72f988b
f86f141af91ab2d7cd011db47%7C1%7C0%7C637103092953247213&
amp;sdata
=oTt8U7ZLy3tdb3bfslqeCcDxwR1FEkNv%2F4YVcCWl3Ps%3D&amp;r
eserved=0

There is currently a limitation in the structured PCD
feature that
does not allow the structured PCD field values to be
set in the scope
of a module in a <PcdsFixedAtBuild> section. To work
around this
limitation, the CryptoPkg DSC file has a define
called CRYPTO_SERVICES
that can be set to ALL, NONE, MIN_PEI, or
MIN_DXE_MIN_SMM. The
default is ALL. Building with each of these values
will build the
modules with different sets of enabled services that
matches the
services enabled using multiple modules in the work
from Sean and
Matt. If this limitation is addressed in BaseTools,
then CryptoPkg could remove the CRYPTO_SERVIES define
and all !if statements.

Thanks,

Mike

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On
Behalf Of Nate
DeSimone
Sent: Wednesday, November 20, 2019 10:31 PM
To: rfc@edk2.groups.io; Sukerkar, Amol N
<amol.n.sukerkar@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>;
Wang, Jian J
<jian.j.wang@...>
Subject: Re: [edk2-rfc] Unified API for Hashing
Algorithms in EDK2

Hi Amol,

With regard to verifying code hashes, it makes much
more sense to me
to have a GUID defined that represents the hash
function to use
versus a PCD. The reason for this is the method for
signing Firmware
Volumes typically involves using nested FVs where
the GUID for the
nested FV type indicates the algorithm needed to
"extract" the nested FV.
Extraction in this context is used to verify code
hashes as well as
decompress compressed code. The "extractor" is
usually a statically
linked library attached to either
SignedSectionExtractPei and/or
DxeIpl.

For example, it is very possible for a platform to
use more than one
hashing algorithm, I can think of more than one
closed source
platform at Intel that actually does this.

Now, one thing that I think your document implies
is that we should
have better APIs for crypto operations in general.
If that was the
intent then I totally agree with you. There are
several reasons that
I think this should be done. First, the API for
OpenSSL has a
tendency to change often enough to make integrating
new versions
into older codebases a headache, putting an
abstraction layer on it
would be helpful. Second, the method used to
consume OpenSSL in edk2
causes some technical issues. Specifically, OpenSSL
is statically
linked and it defines a crazy number of global
variables, enough so
that on a debug build I have noticed roughly a 20KB
increase in
binary size even if you don't call a single
function in OpenSSL.
Wrapping the crypto operations in a PPI/Protocol so
that only 1
driver needs to link to OpenSSL would probably help
reduce binary
size for most implementations. Your current
document does not define
a PPI/Protocol, that is something I would
recommend.

One issue that prior attempts of wrapping OpenSSL
in a PPI/Protocol
have run into is that all the crypto functions need
to be linked,
even the ones that a platform does not use, which
results in a net
negative binary size impact. Accordingly, one would
likely need to
break the crypto functionality into multiple
PPIs/Protocols so that
only the functions a platform needs are actually
included. I think
some experiments need to be done to figure out the
most optimal
division of crypto APIs.

Hope that helps,
Nate

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On
Behalf Of Sukerkar,
Amol N
Sent: Wednesday, November 20, 2019 8:28 AM
To: rfc@edk2.groups.io
Cc: Agrawal, Sachin <sachin.agrawal@...>;
Wang, Jian J
<jian.j.wang@...>; Sukerkar, Amol N
<amol.n.sukerkar@...>; Sukerkar, Amol N
<amol.n.sukerkar@...>
Subject: [edk2-rfc] Unified API for Hashing
Algorithms in EDK2
Importance: High

Hello,

Currently the UEFI drivers using the SHA/SM3
hashing algorithms use
hard-coded API to calculate the hash, such as,
sha_256(...), etc.
Since SHA384 and/or SM3 are being increasingly
adopted, it becomes
cumbersome to modify the driver with SHA384 or SM3
calls for each
application.

To better achieve this, we are proposing a unified
API which can be
used by UEFI drivers that provides the drivers with
flexibility to
use the hashing algorithm they desired or the
strongest hashing
algorithm the system supports (with openssl).
Attached is the design
proposal for the same and we request feedback from
the community
before we begin the process of making the changes
to EDK2 repo.

Alternatively, the design document is also attached
to Bugzilla,
https://nam06.safelinks.protection.outlook.com/?url=htt
ps%3A%2F%2Fbugzilla.
tianocore.org%2Fshow_bug.cgi%3Fid%3D2151&amp;data=02%7C
01%7Csean.b
rogan%40microsoft.com%7Cd110fdb2a91f4e8c2acb08d771e3339
0%7C72f988b
f86f141af91ab2d7cd011db47%7C1%7C0%7C637103092953247213&
amp;sdata
=oTt8U7ZLy3tdb3bfslqeCcDxwR1FEkNv%2F4YVcCWl3Ps%3D&amp;r
eserved=0.
You can also provide the feedback in the Bugzilla.

Thanks,
Amol
















Re: Unified API for Hashing Algorithms in EDK2

Wang, Jian J
 

Hi Sean and Amol,

I believe the OBB and FV/PE image verification can still benefit from the unified
API, if we add one more parameter to HashApiIinit() to force using a hash algorithm
or add a PCD to choose one or link only required HashApiInstanceXxx.

(Just for example)
HashApiInit (
IN EFI_GUID *HashAlg,
OUT HASH_HANDLE *HashHandle
)

In this way, we can avoid hard-code hash API in the verification module. Although
it's slow, I think the OBB verification or FV/PE image signature algorithm is still
subject to change per security requirement change. During a transition period,
we don't have to add a new code to handle a different algorithm in verification
module. Instead we just need to link a different hash lib or set PCD to use a
different algorithm. I think this makes things simpler and easier to change.

The BaseCryptLib implemented in PPI/Protocol is a good idea to save image
size. I think the Unified hash API (BaseHashLib) can make use of it to save
extra hash algorithm library instances (HashApiInstanceSha1, Sha256,...).
The only problem is that the structured PCD, like Mike mentioned before,
doesn't support module override, which prevents different modules from
using different algorithms and then cannot save module size utter mostly.


Regards,
Jian

-----Original Message-----
From: Sukerkar, Amol N <amol.n.sukerkar@...>
Sent: Tuesday, November 26, 2019 5:53 AM
To: rfc@edk2.groups.io; sean.brogan@...; Kinney, Michael D
<michael.d.kinney@...>; Desimone, Nathaniel L
<nathaniel.l.desimone@...>; Matthew Carlson <macarl@...>;
Gao, Liming <liming.gao@...>; Feng, Bob C <bob.c.feng@...>;
Wang, Jian J <jian.j.wang@...>; Lu, XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>
Subject: RE: [edk2-rfc] Unified API for Hashing Algorithms in EDK2

Hi Sean,

A few examples that we want to target are
- HDD Password verification
- Capsule Verification (Platform recovery)
- Bios Guard Platform Data Table
- More importantly, Boot Guard Event Log driver that has a bunch of redundant
code today calculating various hashes. I don't think this is open source though.

Currently, the hash verification mechanism in the above features is hard-coded
to SHA256 which we will need to change to SHA384 to support a more robust
hashing mechanism. This is a good opportunity to replace the existing SHA256
API with an abstracted unified API. And if necessary, the abstracted unified API
still supports driver specific hashing algorithm if needed (as opposed to the
strongest hashing algorithm supported by the system).

We are not targeting verification of OBB or for that matter any PE/COFF image
with this mechanism, mainly because it has it's own wrapper and verification
mechanism in DxeImageVerificationLib.

Thanks,
Amol

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Sean via
Groups.Io
Sent: Monday, November 25, 2019 1:24 PM
To: rfc@edk2.groups.io; Sukerkar, Amol N <amol.n.sukerkar@...>;
Kinney, Michael D <michael.d.kinney@...>; Desimone, Nathaniel L
<nathaniel.l.desimone@...>; Matthew Carlson <macarl@...>;
Gao, Liming <liming.gao@...>; Feng, Bob C <bob.c.feng@...>;
Wang, Jian J <jian.j.wang@...>; Lu, XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>
Subject: Re: [edk2-rfc] Unified API for Hashing Algorithms in EDK2

Amol,

I am interested to hear more about the actual use cases. I like the idea of
abstracting the API so that a calling driver doesn't have to change when the
hashing algorithm requirements change but am not convinced that this will
actually be helpful (or at least worth the introduction of more complexity in
security sensitive code).

For the TPM and measured boot I agree. Random driver needs to extend a
measurement then it can use the existing hashlib and it will be hashed and
extended per platform policy. This is because the random driver doesn't need
to understand the result. Only the TPM needs to and that is handled within the
extend operation.

For the other drivers that I have seen using hashing, they need to understand the
result and make driver specific decisions based on the result. For example if I am
verifying the OBB or some FV then I must make sure the known good and
computed match. This driver must locate the known good and thus this process
generally must know the format of the result. For this I think it is best to know
what algorithm I am using and I don't see a strong use case to change algorithm
to an abstracted algorithm.

Do you have a set of example drivers/use cases that could use abstracted hash
support?

Thanks
Sean



-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Sukerkar, Amol N
via Groups.Io
Sent: Monday, November 25, 2019 12:08 PM
To: rfc@edk2.groups.io; Sukerkar, Amol N <amol.n.sukerkar@...>;
Kinney, Michael D <michael.d.kinney@...>; Desimone, Nathaniel L
<nathaniel.l.desimone@...>; Sean Brogan
<sean.brogan@...>; Matthew Carlson <macarl@...>;
Gao, Liming <liming.gao@...>; Feng, Bob C <bob.c.feng@...>;
Wang, Jian J <jian.j.wang@...>; Lu, XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>; Sukerkar, Amol N
<amol.n.sukerkar@...>
Subject: [EXTERNAL] Re: [edk2-rfc] Unified API for Hashing Algorithms in EDK2

Hi Mike and Nate,

With our implementation we are trying to address the following in EDKII:
1. A common Hashing API for UEFI drivers to consume instead of the current API
that directly calls into the specific hashing algorithm, for instance, SHA1_Init,
SHA256_Update, etc. Due to a stronger hashing algorithm requirement, certain
drivers need to be upgraded to SHA384 from SHA256 and the unified API is
proposed, so, while upgrading we remove the hard-coded dependency on each
hashing algorithm API.

2. The size issue as a result of statically linking openssl/cryptolib API to the
consuming driver. There are two ways we tried to address this issue, although,
we didn't explicitly defined a PPI to address it.
a. Introduced a fixed PCD per hashing algorithm to include/exclude a particular
algorithm from BIOS.
b. Use a register mechanism to link the instance of a particular hashing
algorithm to Unified API library. This is configurable using PCD.

Note that 2.a and 2.b implementation is very similar to
HashLibBaseCryptoRouter used for TPM2 operations.

From the size and static linking perspective, I believe we can have a discussion
between 2.a and 2.b with Mike's
implementation(https://nam06.safelinks.protection.outlook.com/?url=https%3A
%2F%2Fgithub.com%2Fmdkinney%2Fedk2%2Ftree%2FCryptoPkg_PPI_Protocol_
Proposal_V5&amp;data=02%7C01%7Csean.brogan%40microsoft.com%7Cd110f
db2a91f4e8c2acb08d771e33390%7C72f988bf86f141af91ab2d7cd011db47%7C1
%7C0%7C637103092953247213&amp;sdata=2acK0dmJZ85aZ3LvOOqD3e3w44V
EM5dW8TEhs9USyDM%3D&amp;reserved=0) below.

We still need a common hashing API so the hashing algorithm strength for a
particular driver can be increased without having to modify the consumer driver
code.

Comments are welcome.

Thanks,
Amol

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Sukerkar, Amol N
Sent: Thursday, November 21, 2019 4:45 PM
To: rfc@edk2.groups.io; Kinney, Michael D <michael.d.kinney@...>;
Desimone, Nathaniel L <nathaniel.l.desimone@...>;
'sean.brogan@...' <sean.brogan@...>; Matthew
Carlson <macarl@...>; Gao, Liming <liming.gao@...>; Feng,
Bob C <bob.c.feng@...>; Wang, Jian J <jian.j.wang@...>; Lu,
XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>; Sukerkar, Amol N
<amol.n.sukerkar@...>
Subject: Re: [edk2-rfc] Unified API for Hashing Algorithms in EDK2

Thanks, Nate and Mike!

I am going through the code and comments and will respond shortly.

In the meantime, here is the GitHub link to my PoC for the community to look at
and comment:
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.c
om%2Fansukerk%2Fedk2&amp;data=02%7C01%7Csean.brogan%40microsoft.c
om%7Cd110fdb2a91f4e8c2acb08d771e33390%7C72f988bf86f141af91ab2d7cd
011db47%7C1%7C0%7C637103092953247213&amp;sdata=keRdfU%2BxffkBsyv
YHwvSOIA8ryexOigiKhR%2B%2F1q7DjE%3D&amp;reserved=0.

Best regards,
Amol

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Michael D Kinney
Sent: Thursday, November 21, 2019 4:37 PM
To: rfc@edk2.groups.io; Desimone, Nathaniel L
<nathaniel.l.desimone@...>; Sukerkar, Amol N
<amol.n.sukerkar@...>; Kinney, Michael D
<michael.d.kinney@...>; 'sean.brogan@...'
<sean.brogan@...>; Matthew Carlson <macarl@...>;
Gao, Liming <liming.gao@...>; Feng, Bob C <bob.c.feng@...>;
Wang, Jian J <jian.j.wang@...>; Lu, XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>
Subject: Re: [edk2-rfc] Unified API for Hashing Algorithms in EDK2

Nate and Amol,

There is some work already started by Sean and Matt that implements a PEIM,
DXE Driver, and SMM Driver to produce Protocol/PPI that wraps the
BaseCryptLib services. This content broken out into its own package is available
here:


https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.c
om%2Fmicrosoft%2Fmu_plus%2Ftree%2Fdev%2F201908%2FSharedCryptoPkg&
amp;data=02%7C01%7Csean.brogan%40microsoft.com%7Cd110fdb2a91f4e8c2
acb08d771e33390%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637
103092953247213&amp;sdata=88TRhUqsVrj7sx8P4hjbdLy2%2BDKnrBoi7HAI3G
UhpPo%3D&amp;reserved=0

I have ported and simplified this content into a proposed set of patches to the
CryptoPkg. It uses a structured PCD to configure the services mapped into the
Protocols/PPIs and avoids the issue Nate notes below with protocols and PPIs
including all of the BaseCryptLib services. The structured PCD allows families of
crypto services or individual services within a family to be enabled/disabled.


https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.c
om%2Fmdkinney%2Fedk2%2Ftree%2FCryptoPkg_PPI_Protocol_Proposal_V5&a
mp;data=02%7C01%7Csean.brogan%40microsoft.com%7Cd110fdb2a91f4e8c2a
cb08d771e33390%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6371
03092953247213&amp;sdata=2acK0dmJZ85aZ3LvOOqD3e3w44VEM5dW8TEhs9
USyDM%3D&amp;reserved=0

For example, the DSC file PCD statements to enable the SHA1 family and
SHA256 family of hash services with the HashAll service disabled is:

[PcdsFixedAtBuild]
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha1.Family
| PCD_CRYPTO_SERVICE_ENABLE_FAMILY

gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha1.Services.Has
hAll | FALSE
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha256.Family
| PCD_CRYPTO_SERVICE_ENABLE_FAMILY

gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha256.Services.
HashAll | FALSE

Please take a look at this proposal and let me know if this can be used to address.


https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.
tianocore.org%2Fshow_bug.cgi%3Fid%3D2151&amp;data=02%7C01%7Csean.b
rogan%40microsoft.com%7Cd110fdb2a91f4e8c2acb08d771e33390%7C72f988b
f86f141af91ab2d7cd011db47%7C1%7C0%7C637103092953247213&amp;sdata
=oTt8U7ZLy3tdb3bfslqeCcDxwR1FEkNv%2F4YVcCWl3Ps%3D&amp;reserved=0

There is currently a limitation in the structured PCD feature that does not allow
the structured PCD field values to be set in the scope of a module in a
<PcdsFixedAtBuild> section. To work around this limitation, the CryptoPkg DSC
file has a define called CRYPTO_SERVICES that can be set to ALL, NONE,
MIN_PEI, or MIN_DXE_MIN_SMM. The default is ALL. Building with each of
these values will build the modules with different sets of enabled services that
matches the services enabled using multiple modules in the work from Sean and
Matt. If this limitation is addressed in BaseTools, then CryptoPkg could remove
the CRYPTO_SERVIES define and all !if statements.

Thanks,

Mike

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Nate
DeSimone
Sent: Wednesday, November 20, 2019 10:31 PM
To: rfc@edk2.groups.io; Sukerkar, Amol N <amol.n.sukerkar@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>; Wang, Jian J
<jian.j.wang@...>
Subject: Re: [edk2-rfc] Unified API for Hashing Algorithms in EDK2

Hi Amol,

With regard to verifying code hashes, it makes much more sense to me
to have a GUID defined that represents the hash function to use versus
a PCD. The reason for this is the method for signing Firmware Volumes
typically involves using nested FVs where the GUID for the nested FV
type indicates the algorithm needed to "extract" the nested FV.
Extraction in this context is used to verify code hashes as well as
decompress compressed code. The "extractor" is usually a statically
linked library attached to either SignedSectionExtractPei and/or
DxeIpl.

For example, it is very possible for a platform to use more than one
hashing algorithm, I can think of more than one closed source platform
at Intel that actually does this.

Now, one thing that I think your document implies is that we should
have better APIs for crypto operations in general. If that was the
intent then I totally agree with you. There are several reasons that I
think this should be done. First, the API for OpenSSL has a tendency
to change often enough to make integrating new versions into older
codebases a headache, putting an abstraction layer on it would be
helpful. Second, the method used to consume OpenSSL in edk2 causes
some technical issues. Specifically, OpenSSL is statically linked and
it defines a crazy number of global variables, enough so that on a
debug build I have noticed roughly a 20KB increase in binary size even
if you don't call a single function in OpenSSL. Wrapping the crypto
operations in a PPI/Protocol so that only 1 driver needs to link to
OpenSSL would probably help reduce binary size for most
implementations. Your current document does not define a PPI/Protocol,
that is something I would recommend.

One issue that prior attempts of wrapping OpenSSL in a PPI/Protocol
have run into is that all the crypto functions need to be linked, even
the ones that a platform does not use, which results in a net negative
binary size impact. Accordingly, one would likely need to break the
crypto functionality into multiple PPIs/Protocols so that only the
functions a platform needs are actually included. I think some
experiments need to be done to figure out the most optimal division of
crypto APIs.

Hope that helps,
Nate

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Sukerkar,
Amol N
Sent: Wednesday, November 20, 2019 8:28 AM
To: rfc@edk2.groups.io
Cc: Agrawal, Sachin <sachin.agrawal@...>; Wang, Jian J
<jian.j.wang@...>; Sukerkar, Amol N <amol.n.sukerkar@...>;
Sukerkar, Amol N <amol.n.sukerkar@...>
Subject: [edk2-rfc] Unified API for Hashing Algorithms in EDK2
Importance: High

Hello,

Currently the UEFI drivers using the SHA/SM3 hashing algorithms use
hard-coded API to calculate the hash, such as, sha_256(...), etc.
Since SHA384 and/or SM3 are being increasingly adopted, it becomes
cumbersome to modify the driver with SHA384 or SM3 calls for each
application.

To better achieve this, we are proposing a unified API which can be
used by UEFI drivers that provides the drivers with flexibility to use
the hashing algorithm they desired or the strongest hashing algorithm
the system supports (with openssl). Attached is the design proposal
for the same and we request feedback from the community before we
begin the process of making the changes to EDK2 repo.

Alternatively, the design document is also attached to Bugzilla,
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.
tianocore.org%2Fshow_bug.cgi%3Fid%3D2151&amp;data=02%7C01%7Csean.b
rogan%40microsoft.com%7Cd110fdb2a91f4e8c2acb08d771e33390%7C72f988b
f86f141af91ab2d7cd011db47%7C1%7C0%7C637103092953247213&amp;sdata
=oTt8U7ZLy3tdb3bfslqeCcDxwR1FEkNv%2F4YVcCWl3Ps%3D&amp;reserved=0.
You can also provide the feedback in the Bugzilla.

Thanks,
Amol
















Re: Unified API for Hashing Algorithms in EDK2

Sukerkar, Amol N
 

Hi Sean,

A few examples that we want to target are
- HDD Password verification
- Capsule Verification (Platform recovery)
- Bios Guard Platform Data Table
- More importantly, Boot Guard Event Log driver that has a bunch of redundant code today calculating various hashes. I don't think this is open source though.

Currently, the hash verification mechanism in the above features is hard-coded to SHA256 which we will need to change to SHA384 to support a more robust hashing mechanism. This is a good opportunity to replace the existing SHA256 API with an abstracted unified API. And if necessary, the abstracted unified API still supports driver specific hashing algorithm if needed (as opposed to the strongest hashing algorithm supported by the system).

We are not targeting verification of OBB or for that matter any PE/COFF image with this mechanism, mainly because it has it's own wrapper and verification mechanism in DxeImageVerificationLib.

Thanks,
Amol

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Sean via Groups.Io
Sent: Monday, November 25, 2019 1:24 PM
To: rfc@edk2.groups.io; Sukerkar, Amol N <amol.n.sukerkar@...>; Kinney, Michael D <michael.d.kinney@...>; Desimone, Nathaniel L <nathaniel.l.desimone@...>; Matthew Carlson <macarl@...>; Gao, Liming <liming.gao@...>; Feng, Bob C <bob.c.feng@...>; Wang, Jian J <jian.j.wang@...>; Lu, XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>
Subject: Re: [edk2-rfc] Unified API for Hashing Algorithms in EDK2

Amol,

I am interested to hear more about the actual use cases. I like the idea of abstracting the API so that a calling driver doesn't have to change when the hashing algorithm requirements change but am not convinced that this will actually be helpful (or at least worth the introduction of more complexity in security sensitive code).

For the TPM and measured boot I agree. Random driver needs to extend a measurement then it can use the existing hashlib and it will be hashed and extended per platform policy. This is because the random driver doesn't need to understand the result. Only the TPM needs to and that is handled within the extend operation.

For the other drivers that I have seen using hashing, they need to understand the result and make driver specific decisions based on the result. For example if I am verifying the OBB or some FV then I must make sure the known good and computed match. This driver must locate the known good and thus this process generally must know the format of the result. For this I think it is best to know what algorithm I am using and I don't see a strong use case to change algorithm to an abstracted algorithm.

Do you have a set of example drivers/use cases that could use abstracted hash support?

Thanks
Sean



-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Sukerkar, Amol N via Groups.Io
Sent: Monday, November 25, 2019 12:08 PM
To: rfc@edk2.groups.io; Sukerkar, Amol N <amol.n.sukerkar@...>; Kinney, Michael D <michael.d.kinney@...>; Desimone, Nathaniel L <nathaniel.l.desimone@...>; Sean Brogan <sean.brogan@...>; Matthew Carlson <macarl@...>; Gao, Liming <liming.gao@...>; Feng, Bob C <bob.c.feng@...>; Wang, Jian J <jian.j.wang@...>; Lu, XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>; Sukerkar, Amol N <amol.n.sukerkar@...>
Subject: [EXTERNAL] Re: [edk2-rfc] Unified API for Hashing Algorithms in EDK2

Hi Mike and Nate,

With our implementation we are trying to address the following in EDKII:
1. A common Hashing API for UEFI drivers to consume instead of the current API that directly calls into the specific hashing algorithm, for instance, SHA1_Init, SHA256_Update, etc. Due to a stronger hashing algorithm requirement, certain drivers need to be upgraded to SHA384 from SHA256 and the unified API is proposed, so, while upgrading we remove the hard-coded dependency on each hashing algorithm API.

2. The size issue as a result of statically linking openssl/cryptolib API to the consuming driver. There are two ways we tried to address this issue, although, we didn't explicitly defined a PPI to address it.
a. Introduced a fixed PCD per hashing algorithm to include/exclude a particular algorithm from BIOS.
b. Use a register mechanism to link the instance of a particular hashing algorithm to Unified API library. This is configurable using PCD.

Note that 2.a and 2.b implementation is very similar to HashLibBaseCryptoRouter used for TPM2 operations.

From the size and static linking perspective, I believe we can have a discussion between 2.a and 2.b with Mike's implementation(https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmdkinney%2Fedk2%2Ftree%2FCryptoPkg_PPI_Protocol_Proposal_V5&;data=02%7C01%7Csean.brogan%40microsoft.com%7Cd110fdb2a91f4e8c2acb08d771e33390%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637103092953247213&amp;sdata=2acK0dmJZ85aZ3LvOOqD3e3w44VEM5dW8TEhs9USyDM%3D&amp;reserved=0) below.

We still need a common hashing API so the hashing algorithm strength for a particular driver can be increased without having to modify the consumer driver code.

Comments are welcome.

Thanks,
Amol

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Sukerkar, Amol N
Sent: Thursday, November 21, 2019 4:45 PM
To: rfc@edk2.groups.io; Kinney, Michael D <michael.d.kinney@...>; Desimone, Nathaniel L <nathaniel.l.desimone@...>; 'sean.brogan@...' <sean.brogan@...>; Matthew Carlson <macarl@...>; Gao, Liming <liming.gao@...>; Feng, Bob C <bob.c.feng@...>; Wang, Jian J <jian.j.wang@...>; Lu, XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>; Sukerkar, Amol N <amol.n.sukerkar@...>
Subject: Re: [edk2-rfc] Unified API for Hashing Algorithms in EDK2

Thanks, Nate and Mike!

I am going through the code and comments and will respond shortly.

In the meantime, here is the GitHub link to my PoC for the community to look at and comment: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fansukerk%2Fedk2&;data=02%7C01%7Csean.brogan%40microsoft.com%7Cd110fdb2a91f4e8c2acb08d771e33390%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637103092953247213&amp;sdata=keRdfU%2BxffkBsyvYHwvSOIA8ryexOigiKhR%2B%2F1q7DjE%3D&amp;reserved=0.

Best regards,
Amol

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Michael D Kinney
Sent: Thursday, November 21, 2019 4:37 PM
To: rfc@edk2.groups.io; Desimone, Nathaniel L <nathaniel.l.desimone@...>; Sukerkar, Amol N <amol.n.sukerkar@...>; Kinney, Michael D <michael.d.kinney@...>; 'sean.brogan@...' <sean.brogan@...>; Matthew Carlson <macarl@...>; Gao, Liming <liming.gao@...>; Feng, Bob C <bob.c.feng@...>; Wang, Jian J <jian.j.wang@...>; Lu, XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>
Subject: Re: [edk2-rfc] Unified API for Hashing Algorithms in EDK2

Nate and Amol,

There is some work already started by Sean and Matt that implements a PEIM, DXE Driver, and SMM Driver to produce Protocol/PPI that wraps the BaseCryptLib services. This content broken out into its own package is available here:

https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmicrosoft%2Fmu_plus%2Ftree%2Fdev%2F201908%2FSharedCryptoPkg&;data=02%7C01%7Csean.brogan%40microsoft.com%7Cd110fdb2a91f4e8c2acb08d771e33390%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637103092953247213&amp;sdata=88TRhUqsVrj7sx8P4hjbdLy2%2BDKnrBoi7HAI3GUhpPo%3D&amp;reserved=0

I have ported and simplified this content into a proposed set of patches to the CryptoPkg. It uses a structured PCD to configure the services mapped into the Protocols/PPIs and avoids the issue Nate notes below with protocols and PPIs including all of the BaseCryptLib services. The structured PCD allows families of crypto services or individual services within a family to be enabled/disabled.

https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmdkinney%2Fedk2%2Ftree%2FCryptoPkg_PPI_Protocol_Proposal_V5&;data=02%7C01%7Csean.brogan%40microsoft.com%7Cd110fdb2a91f4e8c2acb08d771e33390%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637103092953247213&amp;sdata=2acK0dmJZ85aZ3LvOOqD3e3w44VEM5dW8TEhs9USyDM%3D&amp;reserved=0

For example, the DSC file PCD statements to enable the SHA1 family and
SHA256 family of hash services with the HashAll service disabled is:

[PcdsFixedAtBuild]
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha1.Family | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha1.Services.HashAll | FALSE
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha256.Family | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha256.Services.HashAll | FALSE

Please take a look at this proposal and let me know if this can be used to address.

https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2151&;data=02%7C01%7Csean.brogan%40microsoft.com%7Cd110fdb2a91f4e8c2acb08d771e33390%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637103092953247213&amp;sdata=oTt8U7ZLy3tdb3bfslqeCcDxwR1FEkNv%2F4YVcCWl3Ps%3D&amp;reserved=0

There is currently a limitation in the structured PCD feature that does not allow the structured PCD field values to be set in the scope of a module in a <PcdsFixedAtBuild> section. To work around this limitation, the CryptoPkg DSC file has a define called CRYPTO_SERVICES that can be set to ALL, NONE, MIN_PEI, or MIN_DXE_MIN_SMM. The default is ALL. Building with each of these values will build the modules with different sets of enabled services that matches the services enabled using multiple modules in the work from Sean and Matt. If this limitation is addressed in BaseTools, then CryptoPkg could remove the CRYPTO_SERVIES define and all !if statements.

Thanks,

Mike

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Nate
DeSimone
Sent: Wednesday, November 20, 2019 10:31 PM
To: rfc@edk2.groups.io; Sukerkar, Amol N <amol.n.sukerkar@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>; Wang, Jian J
<jian.j.wang@...>
Subject: Re: [edk2-rfc] Unified API for Hashing Algorithms in EDK2

Hi Amol,

With regard to verifying code hashes, it makes much more sense to me
to have a GUID defined that represents the hash function to use versus
a PCD. The reason for this is the method for signing Firmware Volumes
typically involves using nested FVs where the GUID for the nested FV
type indicates the algorithm needed to "extract" the nested FV.
Extraction in this context is used to verify code hashes as well as
decompress compressed code. The "extractor" is usually a statically
linked library attached to either SignedSectionExtractPei and/or
DxeIpl.

For example, it is very possible for a platform to use more than one
hashing algorithm, I can think of more than one closed source platform
at Intel that actually does this.

Now, one thing that I think your document implies is that we should
have better APIs for crypto operations in general. If that was the
intent then I totally agree with you. There are several reasons that I
think this should be done. First, the API for OpenSSL has a tendency
to change often enough to make integrating new versions into older
codebases a headache, putting an abstraction layer on it would be
helpful. Second, the method used to consume OpenSSL in edk2 causes
some technical issues. Specifically, OpenSSL is statically linked and
it defines a crazy number of global variables, enough so that on a
debug build I have noticed roughly a 20KB increase in binary size even
if you don't call a single function in OpenSSL. Wrapping the crypto
operations in a PPI/Protocol so that only 1 driver needs to link to
OpenSSL would probably help reduce binary size for most
implementations. Your current document does not define a PPI/Protocol,
that is something I would recommend.

One issue that prior attempts of wrapping OpenSSL in a PPI/Protocol
have run into is that all the crypto functions need to be linked, even
the ones that a platform does not use, which results in a net negative
binary size impact. Accordingly, one would likely need to break the
crypto functionality into multiple PPIs/Protocols so that only the
functions a platform needs are actually included. I think some
experiments need to be done to figure out the most optimal division of
crypto APIs.

Hope that helps,
Nate

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Sukerkar,
Amol N
Sent: Wednesday, November 20, 2019 8:28 AM
To: rfc@edk2.groups.io
Cc: Agrawal, Sachin <sachin.agrawal@...>; Wang, Jian J
<jian.j.wang@...>; Sukerkar, Amol N <amol.n.sukerkar@...>;
Sukerkar, Amol N <amol.n.sukerkar@...>
Subject: [edk2-rfc] Unified API for Hashing Algorithms in EDK2
Importance: High

Hello,

Currently the UEFI drivers using the SHA/SM3 hashing algorithms use
hard-coded API to calculate the hash, such as, sha_256(...), etc.
Since SHA384 and/or SM3 are being increasingly adopted, it becomes
cumbersome to modify the driver with SHA384 or SM3 calls for each
application.

To better achieve this, we are proposing a unified API which can be
used by UEFI drivers that provides the drivers with flexibility to use
the hashing algorithm they desired or the strongest hashing algorithm
the system supports (with openssl). Attached is the design proposal
for the same and we request feedback from the community before we
begin the process of making the changes to EDK2 repo.

Alternatively, the design document is also attached to Bugzilla,
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2151&;data=02%7C01%7Csean.brogan%40microsoft.com%7Cd110fdb2a91f4e8c2acb08d771e33390%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637103092953247213&amp;sdata=oTt8U7ZLy3tdb3bfslqeCcDxwR1FEkNv%2F4YVcCWl3Ps%3D&amp;reserved=0.
You can also provide the feedback in the Bugzilla.

Thanks,
Amol





Re: Unified API for Hashing Algorithms in EDK2

Sean
 

Amol,

I am interested to hear more about the actual use cases. I like the idea of abstracting the API so that a calling driver doesn't have to change when the hashing algorithm requirements change but am not convinced that this will actually be helpful (or at least worth the introduction of more complexity in security sensitive code).

For the TPM and measured boot I agree. Random driver needs to extend a measurement then it can use the existing hashlib and it will be hashed and extended per platform policy. This is because the random driver doesn't need to understand the result. Only the TPM needs to and that is handled within the extend operation.

For the other drivers that I have seen using hashing, they need to understand the result and make driver specific decisions based on the result. For example if I am verifying the OBB or some FV then I must make sure the known good and computed match. This driver must locate the known good and thus this process generally must know the format of the result. For this I think it is best to know what algorithm I am using and I don't see a strong use case to change algorithm to an abstracted algorithm.

Do you have a set of example drivers/use cases that could use abstracted hash support?

Thanks
Sean

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Sukerkar, Amol N via Groups.Io
Sent: Monday, November 25, 2019 12:08 PM
To: rfc@edk2.groups.io; Sukerkar, Amol N <amol.n.sukerkar@...>; Kinney, Michael D <michael.d.kinney@...>; Desimone, Nathaniel L <nathaniel.l.desimone@...>; Sean Brogan <sean.brogan@...>; Matthew Carlson <macarl@...>; Gao, Liming <liming.gao@...>; Feng, Bob C <bob.c.feng@...>; Wang, Jian J <jian.j.wang@...>; Lu, XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>; Sukerkar, Amol N <amol.n.sukerkar@...>
Subject: [EXTERNAL] Re: [edk2-rfc] Unified API for Hashing Algorithms in EDK2

Hi Mike and Nate,

With our implementation we are trying to address the following in EDKII:
1. A common Hashing API for UEFI drivers to consume instead of the current API that directly calls into the specific hashing algorithm, for instance, SHA1_Init, SHA256_Update, etc. Due to a stronger hashing algorithm requirement, certain drivers need to be upgraded to SHA384 from SHA256 and the unified API is proposed, so, while upgrading we remove the hard-coded dependency on each hashing algorithm API.

2. The size issue as a result of statically linking openssl/cryptolib API to the consuming driver. There are two ways we tried to address this issue, although, we didn't explicitly defined a PPI to address it.
a. Introduced a fixed PCD per hashing algorithm to include/exclude a particular algorithm from BIOS.
b. Use a register mechanism to link the instance of a particular hashing algorithm to Unified API library. This is configurable using PCD.

Note that 2.a and 2.b implementation is very similar to HashLibBaseCryptoRouter used for TPM2 operations.

From the size and static linking perspective, I believe we can have a discussion between 2.a and 2.b with Mike's implementation(https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmdkinney%2Fedk2%2Ftree%2FCryptoPkg_PPI_Protocol_Proposal_V5&;data=02%7C01%7Csean.brogan%40microsoft.com%7Cd110fdb2a91f4e8c2acb08d771e33390%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637103092953247213&amp;sdata=2acK0dmJZ85aZ3LvOOqD3e3w44VEM5dW8TEhs9USyDM%3D&amp;reserved=0) below.

We still need a common hashing API so the hashing algorithm strength for a particular driver can be increased without having to modify the consumer driver code.

Comments are welcome.

Thanks,
Amol

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Sukerkar, Amol N
Sent: Thursday, November 21, 2019 4:45 PM
To: rfc@edk2.groups.io; Kinney, Michael D <michael.d.kinney@...>; Desimone, Nathaniel L <nathaniel.l.desimone@...>; 'sean.brogan@...' <sean.brogan@...>; Matthew Carlson <macarl@...>; Gao, Liming <liming.gao@...>; Feng, Bob C <bob.c.feng@...>; Wang, Jian J <jian.j.wang@...>; Lu, XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>; Sukerkar, Amol N <amol.n.sukerkar@...>
Subject: Re: [edk2-rfc] Unified API for Hashing Algorithms in EDK2

Thanks, Nate and Mike!

I am going through the code and comments and will respond shortly.

In the meantime, here is the GitHub link to my PoC for the community to look at and comment: https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fansukerk%2Fedk2&;data=02%7C01%7Csean.brogan%40microsoft.com%7Cd110fdb2a91f4e8c2acb08d771e33390%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637103092953247213&amp;sdata=keRdfU%2BxffkBsyvYHwvSOIA8ryexOigiKhR%2B%2F1q7DjE%3D&amp;reserved=0.

Best regards,
Amol

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Michael D Kinney
Sent: Thursday, November 21, 2019 4:37 PM
To: rfc@edk2.groups.io; Desimone, Nathaniel L <nathaniel.l.desimone@...>; Sukerkar, Amol N <amol.n.sukerkar@...>; Kinney, Michael D <michael.d.kinney@...>; 'sean.brogan@...' <sean.brogan@...>; Matthew Carlson <macarl@...>; Gao, Liming <liming.gao@...>; Feng, Bob C <bob.c.feng@...>; Wang, Jian J <jian.j.wang@...>; Lu, XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>
Subject: Re: [edk2-rfc] Unified API for Hashing Algorithms in EDK2

Nate and Amol,

There is some work already started by Sean and Matt that implements a PEIM, DXE Driver, and SMM Driver to produce Protocol/PPI that wraps the BaseCryptLib services. This content broken out into its own package is available here:

https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmicrosoft%2Fmu_plus%2Ftree%2Fdev%2F201908%2FSharedCryptoPkg&;data=02%7C01%7Csean.brogan%40microsoft.com%7Cd110fdb2a91f4e8c2acb08d771e33390%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637103092953247213&amp;sdata=88TRhUqsVrj7sx8P4hjbdLy2%2BDKnrBoi7HAI3GUhpPo%3D&amp;reserved=0

I have ported and simplified this content into a proposed set of patches to the CryptoPkg. It uses a structured PCD to configure the services mapped into the Protocols/PPIs and avoids the issue Nate notes below with protocols and PPIs including all of the BaseCryptLib services. The structured PCD allows families of crypto services or individual services within a family to be enabled/disabled.

https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmdkinney%2Fedk2%2Ftree%2FCryptoPkg_PPI_Protocol_Proposal_V5&;data=02%7C01%7Csean.brogan%40microsoft.com%7Cd110fdb2a91f4e8c2acb08d771e33390%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637103092953247213&amp;sdata=2acK0dmJZ85aZ3LvOOqD3e3w44VEM5dW8TEhs9USyDM%3D&amp;reserved=0

For example, the DSC file PCD statements to enable the SHA1 family and
SHA256 family of hash services with the HashAll service disabled is:

[PcdsFixedAtBuild]
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha1.Family | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha1.Services.HashAll | FALSE
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha256.Family | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha256.Services.HashAll | FALSE

Please take a look at this proposal and let me know if this can be used to address.

https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2151&;data=02%7C01%7Csean.brogan%40microsoft.com%7Cd110fdb2a91f4e8c2acb08d771e33390%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637103092953247213&amp;sdata=oTt8U7ZLy3tdb3bfslqeCcDxwR1FEkNv%2F4YVcCWl3Ps%3D&amp;reserved=0

There is currently a limitation in the structured PCD feature that does not allow the structured PCD field values to be set in the scope of a module in a <PcdsFixedAtBuild> section. To work around this limitation, the CryptoPkg DSC file has a define called CRYPTO_SERVICES that can be set to ALL, NONE, MIN_PEI, or MIN_DXE_MIN_SMM. The default is ALL. Building with each of these values will build the modules with different sets of enabled services that matches the services enabled using multiple modules in the work from Sean and Matt. If this limitation is addressed in BaseTools, then CryptoPkg could remove the CRYPTO_SERVIES define and all !if statements.

Thanks,

Mike

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Nate
DeSimone
Sent: Wednesday, November 20, 2019 10:31 PM
To: rfc@edk2.groups.io; Sukerkar, Amol N <amol.n.sukerkar@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>; Wang, Jian J
<jian.j.wang@...>
Subject: Re: [edk2-rfc] Unified API for Hashing Algorithms in EDK2

Hi Amol,

With regard to verifying code hashes, it makes much more sense to me
to have a GUID defined that represents the hash function to use versus
a PCD. The reason for this is the method for signing Firmware Volumes
typically involves using nested FVs where the GUID for the nested FV
type indicates the algorithm needed to "extract" the nested FV.
Extraction in this context is used to verify code hashes as well as
decompress compressed code. The "extractor" is usually a statically
linked library attached to either SignedSectionExtractPei and/or
DxeIpl.

For example, it is very possible for a platform to use more than one
hashing algorithm, I can think of more than one closed source platform
at Intel that actually does this.

Now, one thing that I think your document implies is that we should
have better APIs for crypto operations in general. If that was the
intent then I totally agree with you. There are several reasons that I
think this should be done. First, the API for OpenSSL has a tendency
to change often enough to make integrating new versions into older
codebases a headache, putting an abstraction layer on it would be
helpful. Second, the method used to consume OpenSSL in edk2 causes
some technical issues. Specifically, OpenSSL is statically linked and
it defines a crazy number of global variables, enough so that on a
debug build I have noticed roughly a 20KB increase in binary size even
if you don't call a single function in OpenSSL. Wrapping the crypto
operations in a PPI/Protocol so that only 1 driver needs to link to
OpenSSL would probably help reduce binary size for most
implementations. Your current document does not define a PPI/Protocol,
that is something I would recommend.

One issue that prior attempts of wrapping OpenSSL in a PPI/Protocol
have run into is that all the crypto functions need to be linked, even
the ones that a platform does not use, which results in a net negative
binary size impact. Accordingly, one would likely need to break the
crypto functionality into multiple PPIs/Protocols so that only the
functions a platform needs are actually included. I think some
experiments need to be done to figure out the most optimal division of
crypto APIs.

Hope that helps,
Nate

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Sukerkar,
Amol N
Sent: Wednesday, November 20, 2019 8:28 AM
To: rfc@edk2.groups.io
Cc: Agrawal, Sachin <sachin.agrawal@...>; Wang, Jian J
<jian.j.wang@...>; Sukerkar, Amol N <amol.n.sukerkar@...>;
Sukerkar, Amol N <amol.n.sukerkar@...>
Subject: [edk2-rfc] Unified API for Hashing Algorithms in EDK2
Importance: High

Hello,

Currently the UEFI drivers using the SHA/SM3 hashing algorithms use
hard-coded API to calculate the hash, such as, sha_256(...), etc.
Since SHA384 and/or SM3 are being increasingly adopted, it becomes
cumbersome to modify the driver with SHA384 or SM3 calls for each
application.

To better achieve this, we are proposing a unified API which can be
used by UEFI drivers that provides the drivers with flexibility to use
the hashing algorithm they desired or the strongest hashing algorithm
the system supports (with openssl). Attached is the design proposal
for the same and we request feedback from the community before we
begin the process of making the changes to EDK2 repo.

Alternatively, the design document is also attached to Bugzilla,
https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2151&;data=02%7C01%7Csean.brogan%40microsoft.com%7Cd110fdb2a91f4e8c2acb08d771e33390%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637103092953247213&amp;sdata=oTt8U7ZLy3tdb3bfslqeCcDxwR1FEkNv%2F4YVcCWl3Ps%3D&amp;reserved=0.
You can also provide the feedback in the Bugzilla.

Thanks,
Amol





Re: Unified API for Hashing Algorithms in EDK2

Sukerkar, Amol N
 

Hi Mike and Nate,

With our implementation we are trying to address the following in EDKII:
1. A common Hashing API for UEFI drivers to consume instead of the current API that directly calls into the specific hashing algorithm, for instance, SHA1_Init, SHA256_Update, etc. Due to a stronger hashing algorithm requirement, certain drivers need to be upgraded to SHA384 from SHA256 and the unified API is proposed, so, while upgrading we remove the hard-coded dependency on each hashing algorithm API.

2. The size issue as a result of statically linking openssl/cryptolib API to the consuming driver. There are two ways we tried to address this issue, although, we didn't explicitly defined a PPI to address it.
a. Introduced a fixed PCD per hashing algorithm to include/exclude a particular algorithm from BIOS.
b. Use a register mechanism to link the instance of a particular hashing algorithm to Unified API library. This is configurable using PCD.

Note that 2.a and 2.b implementation is very similar to HashLibBaseCryptoRouter used for TPM2 operations.

From the size and static linking perspective, I believe we can have a discussion between 2.a and 2.b with Mike's implementation(https://github.com/mdkinney/edk2/tree/CryptoPkg_PPI_Protocol_Proposal_V5) below.

We still need a common hashing API so the hashing algorithm strength for a particular driver can be increased without having to modify the consumer driver code.

Comments are welcome.

Thanks,
Amol

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Sukerkar, Amol N
Sent: Thursday, November 21, 2019 4:45 PM
To: rfc@edk2.groups.io; Kinney, Michael D <michael.d.kinney@...>; Desimone, Nathaniel L <nathaniel.l.desimone@...>; 'sean.brogan@...' <sean.brogan@...>; Matthew Carlson <macarl@...>; Gao, Liming <liming.gao@...>; Feng, Bob C <bob.c.feng@...>; Wang, Jian J <jian.j.wang@...>; Lu, XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>; Sukerkar, Amol N <amol.n.sukerkar@...>
Subject: Re: [edk2-rfc] Unified API for Hashing Algorithms in EDK2

Thanks, Nate and Mike!

I am going through the code and comments and will respond shortly.

In the meantime, here is the GitHub link to my PoC for the community to look at and comment: https://github.com/ansukerk/edk2.

Best regards,
Amol

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Michael D Kinney
Sent: Thursday, November 21, 2019 4:37 PM
To: rfc@edk2.groups.io; Desimone, Nathaniel L <nathaniel.l.desimone@...>; Sukerkar, Amol N <amol.n.sukerkar@...>; Kinney, Michael D <michael.d.kinney@...>; 'sean.brogan@...' <sean.brogan@...>; Matthew Carlson <macarl@...>; Gao, Liming <liming.gao@...>; Feng, Bob C <bob.c.feng@...>; Wang, Jian J <jian.j.wang@...>; Lu, XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>
Subject: Re: [edk2-rfc] Unified API for Hashing Algorithms in EDK2

Nate and Amol,

There is some work already started by Sean and Matt that implements a PEIM, DXE Driver, and SMM Driver to produce Protocol/PPI that wraps the BaseCryptLib services. This content broken out into its own package is available here:

https://github.com/microsoft/mu_plus/tree/dev/201908/SharedCryptoPkg

I have ported and simplified this content into a proposed set of patches to the CryptoPkg. It uses a structured PCD to configure the services mapped into the Protocols/PPIs and avoids the issue Nate notes below with protocols and PPIs including all of the BaseCryptLib services. The structured PCD allows families of crypto services or individual services within a family to be enabled/disabled.

https://github.com/mdkinney/edk2/tree/CryptoPkg_PPI_Protocol_Proposal_V5

For example, the DSC file PCD statements to enable the SHA1 family and
SHA256 family of hash services with the HashAll service disabled is:

[PcdsFixedAtBuild]
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha1.Family | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha1.Services.HashAll | FALSE
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha256.Family | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha256.Services.HashAll | FALSE

Please take a look at this proposal and let me know if this can be used to address.

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

There is currently a limitation in the structured PCD feature that does not allow the structured PCD field values to be set in the scope of a module in a <PcdsFixedAtBuild> section. To work around this limitation, the CryptoPkg DSC file has a define called CRYPTO_SERVICES that can be set to ALL, NONE, MIN_PEI, or MIN_DXE_MIN_SMM. The default is ALL. Building with each of these values will build the modules with different sets of enabled services that matches the services enabled using multiple modules in the work from Sean and Matt. If this limitation is addressed in BaseTools, then CryptoPkg could remove the CRYPTO_SERVIES define and all !if statements.

Thanks,

Mike

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Nate
DeSimone
Sent: Wednesday, November 20, 2019 10:31 PM
To: rfc@edk2.groups.io; Sukerkar, Amol N <amol.n.sukerkar@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>; Wang, Jian J
<jian.j.wang@...>
Subject: Re: [edk2-rfc] Unified API for Hashing Algorithms in EDK2

Hi Amol,

With regard to verifying code hashes, it makes much more sense to me
to have a GUID defined that represents the hash function to use versus
a PCD. The reason for this is the method for signing Firmware Volumes
typically involves using nested FVs where the GUID for the nested FV
type indicates the algorithm needed to "extract" the nested FV.
Extraction in this context is used to verify code hashes as well as
decompress compressed code. The "extractor" is usually a statically
linked library attached to either SignedSectionExtractPei and/or
DxeIpl.

For example, it is very possible for a platform to use more than one
hashing algorithm, I can think of more than one closed source platform
at Intel that actually does this.

Now, one thing that I think your document implies is that we should
have better APIs for crypto operations in general. If that was the
intent then I totally agree with you. There are several reasons that I
think this should be done. First, the API for OpenSSL has a tendency
to change often enough to make integrating new versions into older
codebases a headache, putting an abstraction layer on it would be
helpful. Second, the method used to consume OpenSSL in edk2 causes
some technical issues. Specifically, OpenSSL is statically linked and
it defines a crazy number of global variables, enough so that on a
debug build I have noticed roughly a 20KB increase in binary size even
if you don't call a single function in OpenSSL. Wrapping the crypto
operations in a PPI/Protocol so that only 1 driver needs to link to
OpenSSL would probably help reduce binary size for most
implementations. Your current document does not define a PPI/Protocol,
that is something I would recommend.

One issue that prior attempts of wrapping OpenSSL in a PPI/Protocol
have run into is that all the crypto functions need to be linked, even
the ones that a platform does not use, which results in a net negative
binary size impact. Accordingly, one would likely need to break the
crypto functionality into multiple PPIs/Protocols so that only the
functions a platform needs are actually included. I think some
experiments need to be done to figure out the most optimal division of
crypto APIs.

Hope that helps,
Nate

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Sukerkar,
Amol N
Sent: Wednesday, November 20, 2019 8:28 AM
To: rfc@edk2.groups.io
Cc: Agrawal, Sachin <sachin.agrawal@...>; Wang, Jian J
<jian.j.wang@...>; Sukerkar, Amol N <amol.n.sukerkar@...>;
Sukerkar, Amol N <amol.n.sukerkar@...>
Subject: [edk2-rfc] Unified API for Hashing Algorithms in EDK2
Importance: High

Hello,

Currently the UEFI drivers using the SHA/SM3 hashing algorithms use
hard-coded API to calculate the hash, such as, sha_256(...), etc.
Since SHA384 and/or SM3 are being increasingly adopted, it becomes
cumbersome to modify the driver with SHA384 or SM3 calls for each
application.

To better achieve this, we are proposing a unified API which can be
used by UEFI drivers that provides the drivers with flexibility to use
the hashing algorithm they desired or the strongest hashing algorithm
the system supports (with openssl). Attached is the design proposal
for the same and we request feedback from the community before we
begin the process of making the changes to EDK2 repo.

Alternatively, the design document is also attached to Bugzilla,
https://bugzilla.tianocore.org/show_bug.cgi?id=2151.
You can also provide the feedback in the Bugzilla.

Thanks,
Amol





Re: Unified API for Hashing Algorithms in EDK2

Sukerkar, Amol N
 

Thanks, Nate and Mike!

I am going through the code and comments and will respond shortly.

In the meantime, here is the GitHub link to my PoC for the community to look at and comment: https://github.com/ansukerk/edk2.

Best regards,
Amol

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Michael D Kinney
Sent: Thursday, November 21, 2019 4:37 PM
To: rfc@edk2.groups.io; Desimone, Nathaniel L <nathaniel.l.desimone@...>; Sukerkar, Amol N <amol.n.sukerkar@...>; Kinney, Michael D <michael.d.kinney@...>; 'sean.brogan@...' <sean.brogan@...>; Matthew Carlson <macarl@...>; Gao, Liming <liming.gao@...>; Feng, Bob C <bob.c.feng@...>; Wang, Jian J <jian.j.wang@...>; Lu, XiaoyuX <xiaoyux.lu@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>
Subject: Re: [edk2-rfc] Unified API for Hashing Algorithms in EDK2

Nate and Amol,

There is some work already started by Sean and Matt that implements a PEIM, DXE Driver, and SMM Driver to produce Protocol/PPI that wraps the BaseCryptLib services. This content broken out into its own package is available here:

https://github.com/microsoft/mu_plus/tree/dev/201908/SharedCryptoPkg

I have ported and simplified this content into a proposed set of patches to the CryptoPkg. It uses a structured PCD to configure the services mapped into the Protocols/PPIs and avoids the issue Nate notes below with protocols and PPIs including all of the BaseCryptLib services. The structured PCD allows families of crypto services or individual services within a family to be enabled/disabled.

https://github.com/mdkinney/edk2/tree/CryptoPkg_PPI_Protocol_Proposal_V5

For example, the DSC file PCD statements to enable the SHA1 family and
SHA256 family of hash services with the HashAll service disabled is:

[PcdsFixedAtBuild]
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha1.Family | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha1.Services.HashAll | FALSE
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha256.Family | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha256.Services.HashAll | FALSE

Please take a look at this proposal and let me know if this can be used to address.

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

There is currently a limitation in the structured PCD feature that does not allow the structured PCD field values to be set in the scope of a module in a <PcdsFixedAtBuild> section. To work around this limitation, the CryptoPkg DSC file has a define called CRYPTO_SERVICES that can be set to ALL, NONE, MIN_PEI, or MIN_DXE_MIN_SMM. The default is ALL. Building with each of these values will build the modules with different sets of enabled services that matches the services enabled using multiple modules in the work from Sean and Matt. If this limitation is addressed in BaseTools, then CryptoPkg could remove the CRYPTO_SERVIES define and all !if statements.

Thanks,

Mike

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Nate
DeSimone
Sent: Wednesday, November 20, 2019 10:31 PM
To: rfc@edk2.groups.io; Sukerkar, Amol N <amol.n.sukerkar@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>; Wang, Jian J
<jian.j.wang@...>
Subject: Re: [edk2-rfc] Unified API for Hashing Algorithms in EDK2

Hi Amol,

With regard to verifying code hashes, it makes much more sense to me
to have a GUID defined that represents the hash function to use versus
a PCD. The reason for this is the method for signing Firmware Volumes
typically involves using nested FVs where the GUID for the nested FV
type indicates the algorithm needed to "extract" the nested FV.
Extraction in this context is used to verify code hashes as well as
decompress compressed code. The "extractor" is usually a statically
linked library attached to either SignedSectionExtractPei and/or
DxeIpl.

For example, it is very possible for a platform to use more than one
hashing algorithm, I can think of more than one closed source platform
at Intel that actually does this.

Now, one thing that I think your document implies is that we should
have better APIs for crypto operations in general. If that was the
intent then I totally agree with you. There are several reasons that I
think this should be done. First, the API for OpenSSL has a tendency
to change often enough to make integrating new versions into older
codebases a headache, putting an abstraction layer on it would be
helpful. Second, the method used to consume OpenSSL in edk2 causes
some technical issues. Specifically, OpenSSL is statically linked and
it defines a crazy number of global variables, enough so that on a
debug build I have noticed roughly a 20KB increase in binary size even
if you don't call a single function in OpenSSL. Wrapping the crypto
operations in a PPI/Protocol so that only 1 driver needs to link to
OpenSSL would probably help reduce binary size for most
implementations. Your current document does not define a PPI/Protocol,
that is something I would recommend.

One issue that prior attempts of wrapping OpenSSL in a PPI/Protocol
have run into is that all the crypto functions need to be linked, even
the ones that a platform does not use, which results in a net negative
binary size impact. Accordingly, one would likely need to break the
crypto functionality into multiple PPIs/Protocols so that only the
functions a platform needs are actually included. I think some
experiments need to be done to figure out the most optimal division of
crypto APIs.

Hope that helps,
Nate

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Sukerkar,
Amol N
Sent: Wednesday, November 20, 2019 8:28 AM
To: rfc@edk2.groups.io
Cc: Agrawal, Sachin <sachin.agrawal@...>; Wang, Jian J
<jian.j.wang@...>; Sukerkar, Amol N <amol.n.sukerkar@...>;
Sukerkar, Amol N <amol.n.sukerkar@...>
Subject: [edk2-rfc] Unified API for Hashing Algorithms in EDK2
Importance: High

Hello,

Currently the UEFI drivers using the SHA/SM3 hashing algorithms use
hard-coded API to calculate the hash, such as, sha_256(...), etc.
Since SHA384 and/or SM3 are being increasingly adopted, it becomes
cumbersome to modify the driver with SHA384 or SM3 calls for each
application.

To better achieve this, we are proposing a unified API which can be
used by UEFI drivers that provides the drivers with flexibility to use
the hashing algorithm they desired or the strongest hashing algorithm
the system supports (with openssl). Attached is the design proposal
for the same and we request feedback from the community before we
begin the process of making the changes to EDK2 repo.

Alternatively, the design document is also attached to Bugzilla,
https://bugzilla.tianocore.org/show_bug.cgi?id=2151.
You can also provide the feedback in the Bugzilla.

Thanks,
Amol





Re: Unified API for Hashing Algorithms in EDK2

Michael D Kinney
 

Nate and Amol,

There is some work already started by Sean and Matt that implements a
PEIM, DXE Driver, and SMM Driver to produce Protocol/PPI that wraps the
BaseCryptLib services. This content broken out into its own package
is available here:

https://github.com/microsoft/mu_plus/tree/dev/201908/SharedCryptoPkg

I have ported and simplified this content into a proposed set of patches
to the CryptoPkg. It uses a structured PCD to configure the services mapped
into the Protocols/PPIs and avoids the issue Nate notes below with
protocols and PPIs including all of the BaseCryptLib services. The
structured PCD allows families of crypto services or individual services
within a family to be enabled/disabled.

https://github.com/mdkinney/edk2/tree/CryptoPkg_PPI_Protocol_Proposal_V5

For example, the DSC file PCD statements to enable the SHA1 family and
SHA256 family of hash services with the HashAll service disabled is:

[PcdsFixedAtBuild]
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha1.Family | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha1.Services.HashAll | FALSE
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha256.Family | PCD_CRYPTO_SERVICE_ENABLE_FAMILY
gEfiCryptoPkgTokenSpaceGuid.PcdCryptoServiceFamilyEnable.Sha256.Services.HashAll | FALSE

Please take a look at this proposal and let me know if this can be used
to address.

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

There is currently a limitation in the structured PCD feature that does not
allow the structured PCD field values to be set in the scope of a module
in a <PcdsFixedAtBuild> section. To work around this limitation, the
CryptoPkg DSC file has a define called CRYPTO_SERVICES that can be set to
ALL, NONE, MIN_PEI, or MIN_DXE_MIN_SMM. The default is ALL. Building with
each of these values will build the modules with different sets of enabled
services that matches the services enabled using multiple modules in the
work from Sean and Matt. If this limitation is addressed in BaseTools,
then CryptoPkg could remove the CRYPTO_SERVIES define and all !if statements.

Thanks,

Mike

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf
Of Nate DeSimone
Sent: Wednesday, November 20, 2019 10:31 PM
To: rfc@edk2.groups.io; Sukerkar, Amol N
<amol.n.sukerkar@...>
Cc: Agrawal, Sachin <sachin.agrawal@...>; Wang,
Jian J <jian.j.wang@...>
Subject: Re: [edk2-rfc] Unified API for Hashing
Algorithms in EDK2

Hi Amol,

With regard to verifying code hashes, it makes much
more sense to me to have a GUID defined that represents
the hash function to use versus a PCD. The reason for
this is the method for signing Firmware Volumes
typically involves using nested FVs where the GUID for
the nested FV type indicates the algorithm needed to
"extract" the nested FV. Extraction in this context is
used to verify code hashes as well as decompress
compressed code. The "extractor" is usually a
statically linked library attached to either
SignedSectionExtractPei and/or DxeIpl.

For example, it is very possible for a platform to use
more than one hashing algorithm, I can think of more
than one closed source platform at Intel that actually
does this.

Now, one thing that I think your document implies is
that we should have better APIs for crypto operations
in general. If that was the intent then I totally agree
with you. There are several reasons that I think this
should be done. First, the API for OpenSSL has a
tendency to change often enough to make integrating new
versions into older codebases a headache, putting an
abstraction layer on it would be helpful. Second, the
method used to consume OpenSSL in edk2 causes some
technical issues. Specifically, OpenSSL is statically
linked and it defines a crazy number of global
variables, enough so that on a debug build I have
noticed roughly a 20KB increase in binary size even if
you don't call a single function in OpenSSL. Wrapping
the crypto operations in a PPI/Protocol so that only 1
driver needs to link to OpenSSL would probably help
reduce binary size for most implementations. Your
current document does not define a PPI/Protocol, that
is something I would recommend.

One issue that prior attempts of wrapping OpenSSL in a
PPI/Protocol have run into is that all the crypto
functions need to be linked, even the ones that a
platform does not use, which results in a net negative
binary size impact. Accordingly, one would likely need
to break the crypto functionality into multiple
PPIs/Protocols so that only the functions a platform
needs are actually included. I think some experiments
need to be done to figure out the most optimal division
of crypto APIs.

Hope that helps,
Nate

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf
Of Sukerkar, Amol N
Sent: Wednesday, November 20, 2019 8:28 AM
To: rfc@edk2.groups.io
Cc: Agrawal, Sachin <sachin.agrawal@...>; Wang,
Jian J <jian.j.wang@...>; Sukerkar, Amol N
<amol.n.sukerkar@...>; Sukerkar, Amol N
<amol.n.sukerkar@...>
Subject: [edk2-rfc] Unified API for Hashing Algorithms
in EDK2
Importance: High

Hello,

Currently the UEFI drivers using the SHA/SM3 hashing
algorithms use hard-coded API to calculate the hash,
such as, sha_256(...), etc. Since SHA384 and/or SM3 are
being increasingly adopted, it becomes cumbersome to
modify the driver with SHA384 or SM3 calls for each
application.

To better achieve this, we are proposing a unified API
which can be used by UEFI drivers that provides the
drivers with flexibility to use the hashing algorithm
they desired or the strongest hashing algorithm the
system supports (with openssl). Attached is the design
proposal for the same and we request feedback from the
community before we begin the process of making the
changes to EDK2 repo.

Alternatively, the design document is also attached to
Bugzilla,
https://bugzilla.tianocore.org/show_bug.cgi?id=2151.
You can also provide the feedback in the Bugzilla.

Thanks,
Amol





Re: Unified API for Hashing Algorithms in EDK2

Nate DeSimone
 

Hi Amol,

With regard to verifying code hashes, it makes much more sense to me to have a GUID defined that represents the hash function to use versus a PCD. The reason for this is the method for signing Firmware Volumes typically involves using nested FVs where the GUID for the nested FV type indicates the algorithm needed to "extract" the nested FV. Extraction in this context is used to verify code hashes as well as decompress compressed code. The "extractor" is usually a statically linked library attached to either SignedSectionExtractPei and/or DxeIpl.

For example, it is very possible for a platform to use more than one hashing algorithm, I can think of more than one closed source platform at Intel that actually does this.

Now, one thing that I think your document implies is that we should have better APIs for crypto operations in general. If that was the intent then I totally agree with you. There are several reasons that I think this should be done. First, the API for OpenSSL has a tendency to change often enough to make integrating new versions into older codebases a headache, putting an abstraction layer on it would be helpful. Second, the method used to consume OpenSSL in edk2 causes some technical issues. Specifically, OpenSSL is statically linked and it defines a crazy number of global variables, enough so that on a debug build I have noticed roughly a 20KB increase in binary size even if you don't call a single function in OpenSSL. Wrapping the crypto operations in a PPI/Protocol so that only 1 driver needs to link to OpenSSL would probably help reduce binary size for most implementations. Your current document does not define a PPI/Protocol, that is something I would recommend.

One issue that prior attempts of wrapping OpenSSL in a PPI/Protocol have run into is that all the crypto functions need to be linked, even the ones that a platform does not use, which results in a net negative binary size impact. Accordingly, one would likely need to break the crypto functionality into multiple PPIs/Protocols so that only the functions a platform needs are actually included. I think some experiments need to be done to figure out the most optimal division of crypto APIs.

Hope that helps,
Nate

-----Original Message-----
From: rfc@edk2.groups.io <rfc@edk2.groups.io> On Behalf Of Sukerkar, Amol N
Sent: Wednesday, November 20, 2019 8:28 AM
To: rfc@edk2.groups.io
Cc: Agrawal, Sachin <sachin.agrawal@...>; Wang, Jian J <jian.j.wang@...>; Sukerkar, Amol N <amol.n.sukerkar@...>; Sukerkar, Amol N <amol.n.sukerkar@...>
Subject: [edk2-rfc] Unified API for Hashing Algorithms in EDK2
Importance: High

Hello,

Currently the UEFI drivers using the SHA/SM3 hashing algorithms use hard-coded API to calculate the hash, such as, sha_256(...), etc. Since SHA384 and/or SM3 are being increasingly adopted, it becomes cumbersome to modify the driver with SHA384 or SM3 calls for each application.

To better achieve this, we are proposing a unified API which can be used by UEFI drivers that provides the drivers with flexibility to use the hashing algorithm they desired or the strongest hashing algorithm the system supports (with openssl). Attached is the design proposal for the same and we request feedback from the community before we begin the process of making the changes to EDK2 repo.

Alternatively, the design document is also attached to Bugzilla, https://bugzilla.tianocore.org/show_bug.cgi?id=2151. You can also provide the feedback in the Bugzilla.

Thanks,
Amol


Unified API for Hashing Algorithms in EDK2

Sukerkar, Amol N
 

Hello,

Currently the UEFI drivers using the SHA/SM3 hashing algorithms use hard-coded API to calculate the hash, such as, sha_256(...), etc. Since SHA384 and/or SM3 are being increasingly adopted, it becomes cumbersome to modify the driver with SHA384 or SM3 calls for each application.

To better achieve this, we are proposing a unified API which can be used by UEFI drivers that provides the drivers with flexibility to use the hashing algorithm they desired or the strongest hashing algorithm the system supports (with openssl). Attached is the design proposal for the same and we request feedback from the community before we begin the process of making the changes to EDK2 repo.

Alternatively, the design document is also attached to Bugzilla, https://bugzilla.tianocore.org/show_bug.cgi?id=2151. You can also provide the feedback in the Bugzilla.

Thanks,
Amol


Re: UEFI accessibility mandate

Rafael Machado <rafaelrodrigues.machado@...>
 

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


Re: UEFI accessibility mandate

Ethin Probst
 

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


Re: UEFI accessibility mandate

Andrew Fish <afish@...>
 

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


Re: UEFI accessibility mandate

Rafael Machado <rafaelrodrigues.machado@...>
 

Hi Ethin

I think you can start by studying the EmulatorPkg at the edk2 repository.

Try to compile and use it, and after that you could try the software
developed by Andrew to check if it works correctly at your system.

About the Uefi technology, in case you want to start studying it before
going to the EmulatorPkg, you can take a look at this training:
https://github.com/tianocore/tianocore.github.io/wiki/UEFI%20EDKII%20Learning%20Dev

Thanks and Regards
Rafael



Em ter, 29 de out de 2019 17:17, Ethin Probst <harlydavidsen@...>
escreveu:

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

581 - 600 of 789