[edk2 rebase - task 1] Cherry-pick required commits on top of recent edk2

miczyg1 commented 1 year ago

Here is a list containing a diff against the upstream edk2: https://paste.dasharo.com/?cc6b3b260b093ad6#4TfxUFhyJtCrZJ3ARy3EabzFb7UK7tMRUKFzndCZ7utU

I have marked each commit (on the begining of the line) with a symbol:

+ means a commit that has to be cherry-picked onto a rebased edk2 branch (to be migrated)
- measn a commit has to be skipped/dropped
?+ means a commit that SHOULD be cherry-picked onto a rebased edk2 branch (to be migrated), however there are some concerns which have to be checked first before deciding (the concerns are at the end of the line in parentheses)
?- means a commit that SHOULD be skipped/dropped, however there are some concerns which have to be checked first before deciding (the concerns are at the end of the line in parentheses)
U means upstreamable fix to edk2-devel ML

There are also commits marked for quashing or to be migrated to DasharoModulePkg to make future rebases easier.

First task of the migration to rebased edk2 is as follows:

Items in the above paste that are marked for migration to DasahroModulePkg, should be migrated first and tested on current codebase. Migrating a module will basically mean changing the paths in DSC/FDF files and copying the sources. If the module still works as expected after migration, go to the next one until all items designated for migration on the commit list are done.

Also to ease the future rebases further, we should create DSC and FDF inc files that will already cotnain all the necessary DasharoModulePkg entries specific to Dasharo and reduce the modifications of UefiPayloadPkg FDF and DSC file. We should create something similar to NetworkPkg/NetworkDefines.dsc.inc, NetworkPkg/NetworkLibs.dsc.inc, NetworkPkg/NetworkPcds.dsc.inc, NetworkPkg/NetworkComponents.dsc.inc and NetworkPkg/Network.fdf.inc.

At the end we should create a new paste which has the migrated items either removed or marked as DONE.

miczyg1 commented 9 months ago

By comparing the UefiPayloadPkg directory structure and the edk2 logs from running upstream EDK2 we will have to:

Migrate UefiPayloadPkg/AcpiPlatformDxe to DasharoModulePkg.
Upstream EDK2 i PEI-less so everything for PEI will have to be removed. All HOBs produced in BlSupportPei will have to be published in the UefiPayloadPkg/UefiPayloadEntry
There is a MULTIPLE_DEBUG_PORT_SUPPORT but it is incompatible with cbmem debug output.
Many libs and modules are dependent on UNIVERSAL_PAYLOAD, which does not make sense in all cases (e.g. the BaseCpuTimerLib).
MtrrLibNull can be dropped in favor of PcdCpuDisableMtrrProgramming on MrChromebox repo
LaptopBatterryLib should be migrated to DasharoModulePkg
SmmStore can also be migrated to DasharoModulePkg
System76EcLib should be migrated to DasharoModulePkg
PciPlatformDxe should be migrated to DasharoModulePkg
Our custom Logo processing should be migrated to DasharoModulePkg
There is no TPM support at all in upstream UefiPayload. Detection of TPM must be done somewhere, because there is no way to execute TCG PEI modules now.
UefiPayloadPkg/SecureBootDefaultKeys should be migrated to DasharoModulePkg
Our UefiPayloadPkg/PlatformBootManagerLib has many custom changes. It should be migrated to DasharoModulePkg
UefiPayloadPkg/Tcg2PhysicalPresencePlatformLibUefipayload should be migrated to DasharoModulePkg
Migrate the filesystem drivers to DasharoModulePkg

Some PCDs will have to be moved to DasharoModulePkg.dec and updated inside the modules' INFs.

macpijan commented 3 months ago

Updated paste (commits to be picked) from @miczyg1 https://paste.dasharo.com/?cc6b3b260b093ad6#4TfxUFhyJtCrZJ3ARy3EabzFb7UK7tMRUKFzndCZ7utU

macpijan commented 3 months ago

After discussions, we have adjusted this migration plan. I will close it and link here a follow-up issue.

macpijan commented 3 months ago

Actually, I have just updated the title, as the task description and the list of commits has not really changed. What has changed is the approach we want to follow. So we have decided to:

Start on the edk2-stable202402 tag
Create DasharoPayloadPkg based on the UefiPayloadPkg
- upstream UefiPayloadPkg is already much different than what we have and we do not benefit much from upstream fixes in the module
- we more benefit from fixes in other upstream modules, which we still need to rely on
Cherry-pick the required commits (with + in the lists - this task) and analyze the commits with concerns (in the other task: https://github.com/Dasharo/dasharo-issues/issues/433).
Finally, we also want to migrate the DasharoModulePkg from a separate submodule into this repository.

We have concluded that the previous attempt was not the best solution and we will not continue these PRs:

SergiiDmytruk commented 3 months ago

To make sure I got it right:

Starting from edk2-stable202402 tag.
Create DasharoPayloadPkg as a copy of UefiPayloadPkg as it was in 2019 (commit dd7523b which is both in upstream and in Dasharo/edk2).
Apply + commits from dasharo/edk2 taking care of the rename.
MergeDasharoModulePkg repo into DasharoModulePkg/ of Dasharo/edk2.

Like that? Step 2 in particular.

macpijan commented 3 months ago

Create DasharoPayloadPkg as a copy of UefiPayloadPkg as it was in 2019 (commit dd7523b which is both in upstream and in Dasharo/edk2).

I think it would be preferable to start this way and apply our changes we have done to UefiPayloadPkg into DasharoPayloadPkg instead. I am not sure how practical would that be, though.

Definitely, the end result is that we'd like to keep the DasharoPaylaodPkg as a separate package. This should also ease future rebasing, as we will avoid conflicts in the UefiPayloadPkgs, where we want to do stuff in our way anyway.

If it is easier / more practical to create DasharoPayloadPkg based on the most recent UefiPayloadPkg in Dasharo fork, we can also consider this approach. Or Move UefiPayloadPkg into DasharoPaylaodPkg after we are done with rebase? Ideas welcome, after you try to approach this, I am sure the ideas will come to mind.

@miczyg1 @krystian-hebel thoughts ?

Apply + commits from dasharo/edk2 taking care of the rename.

Some cleanup in general, as renaming and squashing. It has been suggested by @miczyg1 in the list, but opinions are welcome after reviewing it.

MergeDasharoModulePkg repo into DasharoModulePkg/ of Dasharo/edk2.

Correct, we have concluded that in the end it is easier to keep it in one repo. And keeping it in a separate directory should not make much problems when rebasing.

miczyg1 commented 3 months ago

I think it would be preferable to start this way and apply our changes we have done to UefiPayloadPkg into DasharoPayloadPkg instead. I am not sure how practical would that be, though.

I guess we have to do the step 2 proposed by @SergiiDmytruk , but skip renaming/copying the UefiPayloadPkg to DasharoPayloadPkg yet. Then apply the commits cleanly with all the squashing etc. When finished, copy such UefiPayloadPkg as DasharoPayloadPkg onto edk2-stable202402.

We may of course pick up some improvements and changes from upstream UefiPayloadPkg if anything seems beneficial.

krystian-hebel commented 3 months ago

When finished, copy such UefiPayloadPkg as DasharoPayloadPkg onto edk2-stable202402.

This wouldn't copy the git history, so I'm not sure if this would give any benefit over copying UefiPayloadPkg from current version, unless I misunderstood something.

SergiiDmytruk commented 3 months ago

I guess we have to do the step 2 proposed by @SergiiDmytruk , but skip renaming/copying the UefiPayloadPkg to DasharoPayloadPkg yet. Then apply the commits cleanly with all the squashing etc. When finished, copy such UefiPayloadPkg as DasharoPayloadPkg onto edk2-stable202402.

The result of such manipulations would then need a rebase for all other files, but it should be more convenient to clean up changes first and then rebase instead of cherry-picking commits with fixes to fixes on top of a new base.

This wouldn't copy the git history, so I'm not sure if this would give any benefit over copying UefiPayloadPkg from current version, unless I misunderstood something.

I thought the main reason for bothering with DasharoPayloadPkg is that we can't apply our changes on top of current UefiPayloadPkg without reworking them.

Preserving history should still be possible if using UefiPayloadPkg for rebasing, but will require filter-repo in a separate clone to update paths in commits and drop everything but that directory which can then be merged back along with the full history of changes.

Anyhow, I understood that this is something to figure out in the process.

SergiiDmytruk commented 3 months ago

@miczyg1 416768c16b..9132842d3b range appears twice in the paste with the comment

commits from 416768c16b to 9132842d3b are duplicates of the below commits, probably created by mistake in git history after rebase, since the above commit are newer, cherry-picking below commits is discouraged

I don't see duplicate commits in git history, so maybe you were looking at two branches at the same time or something. Those two versions in the paste aren't completely identical. Top version:

- 1e5a8bb8aa Add filesystem drivers (will be migrated to DasharoModulePkg)
- e0e6f307b5 HACK: BmMisc: Remove S4 memory check until VariableStore is implemented (verify if S4 works as intended when not cherry-picked)
+ 56313ac56f MdeModulePkg/Usb/Keyboard.c: don't request protocol before setting
+ 143d9fea48 MdeModulePkg/Usb/Keyboard.c: remove Get/SetConfig calls

while the bottom one says:

?+ 1e5a8bb8aa Add filesystem drivers (check if upstream does not include it already, EXT4 is included in edk2-platforms/Features/Ext4Pkg)
?- e0e6f307b5 HACK: BmMisc: Remove S4 memory check until VariableStore is implemented
?- 56313ac56f MdeModulePkg/Usb/Keyboard.c: don't request protocol before setting
?- 143d9fea48 MdeModulePkg/Usb/Keyboard.c: remove Get/SetConfig calls

Do we need those keyboard changes which have + and ?- in different versions?

Edit: also, all comments to the effect of "will be migrated to DasharoModulePkg" are outdated and those - are + now, right?

miczyg1 commented 3 months ago

Do we need those keyboard changes which have + and ?- in different versions?

When I looked at theKB commits, there were some duplications in the Get/SetConfig calls. I think we may skip them and keep the upstream version and see what happens.

I don't see duplicate commits in git history, so maybe you were looking at two branches at the same time or something. Those two versions in the paste aren't completely identical. Top version:

I think this was generate with git log --oneline and it showed me the duplicated commits. Or maybe it was my mistake by accidentally touching the mouse scroll (scrolling git log will cause such duplicates to appear, but not happening when using page up and page down).

Edit: also, all comments to the effect of "will be migrated to DasharoModulePkg" are outdated and those - are + now, right?

Yes, those should be a part of the resulting DasharoPayloadPkg.

SergiiDmytruk commented 3 months ago

After going through bottom half of the paste decided to give history rewrite a try to see how well it works. Hit https://github.com/newren/git-filter-repo/issues/551 but otherwise this script seems to do the job:

git clone --no-local Dasharo/ tmp

git -C tmp filter-repo --paths-from-file ../renames \
                       --replace-text ../replacements \
                       --replace-message ../replacements

git -C tmp remote add origin ../Dasharo/
git -C tmp fetch origin
git -C tmp rebase --onto origin/rebase-target ea0f14d9085e3cf6e16879d0688e972832cdbcab rebase

Unlike I wrote above, it works by first rewriting history to correct all paths (handling UefiPayloadPkg/ alone would decouple its changes from changes of all other files), commit messages and commit data and then by rebasing all interesting commits onto edk2-stable202402 tag after the commit that creates DasharoPayloadPkg/. Letting git-filter-repo handle data as well to correct paths/variables automatically here, will see if it works. Obviously, the rebase command above doesn't finish successfully because of conflicts in files outside of DasharoPayloadPkg/, but some commits do apply and the history looks as intended, so the overall approach is sound.

Also realized that all comments in the paste which say a change isn't needed because upstream UefiPayloadPkg has it don't apply either, but there aren't that many of those, so not trying to add for now in case upstream versions could be used instead, but that requires finding corresponding changes and assessing them first.

SergiiDmytruk commented 3 months ago

Finishing going through the whole list and doing most of the changes. Several commits are still pending rebase onto edk2-stable202402 because they require upstream commits.

Also applied those upstream UefiPayloadPkg commits which we now need as we might not get upstream version. They applied with some trivial conflicts, so it won't be a problem to replace them with something else if need be.

Can now use the script above and start resolving conflicts outside of DasharoPayloadPkg/.

SergiiDmytruk commented 3 months ago

Upstream ran uncrustify on sources in 2021 which now causes conflicts simply due to formatting changes. No real hard conflicts so far. Trying to check that things build after conflicts, sometimes things don't build due to commits which didn't conflict, like at the moment:

/.../tmp/DasharoPayloadPkg/DasharoPayloadPkgIa32X64.dsc(...): error F002: Library [/.../tmp/MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf] with constructors has a cycle
                consumed by /.../tmp/DasharoPayloadPkg/Library/BasePciExpressLib/BasePciExpressLib.inf

SergiiDmytruk commented 3 months ago

So upstream added an empty constructor for BaseSerialPortLib16550 so that constructors of other libraries are executed in correct order, which is why dependency cycles appear now related to DebugLib which uses serial.

SergiiDmytruk commented 3 months ago

Switched to PeiDxeDebugLibReportStatusCode where use of BaseDebugLibSerialPort caused dependency loop to fix the issue. Upstream switched to PeiDxeDebugLibReportStatusCode in [LibraryClasses], although not sure if for related reason. The problem is https://github.com/Dasharo/edk2/commit/6be227e28d introduces such cycle as well, so it's undone for now on a new branch as I'm not yet sure how to handle it properly.

Finished rebasing on the new tag and started testing build with coreboot and given the change of payload's name, that entails changes to its Makefiles. Had to fix several things in EDK but it got built for MSI, although then failed in a way seemingly unrelated to payload:

E: Could not add [build/Fsp_S.fd, 283692 bytes (277 KB)@0x0]; too big?
E: Failed to add 'build/Fsp_S.fd' into ROM image.
E: Failed while operating on 'COREBOOT' region!

Haven't yet tried merging in DasharoModulePkg. There are some more improvements in history to make, in particular UefiPayloadPkg is gone as a result of moving commits from a clone on which I ran git filter-repo and need to sort that out.

SergiiDmytruk commented 3 months ago

The problem is Dasharo/edk2@6be227e28d introduces such cycle as well, so it's undone for now on a new branch as I'm not yet sure how to handle it properly.

EDK can still be built by coreboot if I drop that "revert", so maybe it's not an issue when all proper variables are set.

E: Could not add [build/Fsp_S.fd, 283692 bytes (277 KB)@0x0]; too big?
E: Failed to add 'build/Fsp_S.fd' into ROM image.
E: Failed while operating on 'COREBOOT' region!

This is actually caused by payload being larger than before. It gained about 350 KiB when compressed and fsps.bin really doesn't fit now for an MSI board (need maybe 50 KiB). Some modules increased in size significantly:

FILE                                             OLD    NEW
====                                             ===    ===
CpuDxe.efi                                       53K   109K  !!
DxeCore.efi                                     287K   321K  !
HddPasswordDxe.efi                              171K   624K  !!!
Pkcs7VerifyDxe.efi                              481K   738K  !!
SecureBootConfigDxe.efi                         450K   688K  !!
SecureBootDefaultKeysDxe.efi                    370K   600K  !!
SecurityStubDxe.efi                             518K   785K  !!
UserAuthenticationDxe.efi                       162K   605K  !!!
VariableRuntimeDxe.efi                          531K   803K  !!

This might be caused by a switch to OpenSSL 3. Its static library is now 20 MiB instead of 13 MiB. It's not just binary data, there is LTO involved, but still something in the code is causing this inflation. Build uses LTO and -Os, so it doesn't look like a configuration issue. Need to either reduce the size or allocate more for payload, which might not work out well on each of the supported platforms.

As for DasharoModulePkg:

merged it into EDK2
dropped merge commits from it
cleaned up its history a bit (the obvious commits)
edited its commit titles to have DasharoModulePkg with git-filter-repo
merged its pre-commit configuration with that of EDK2

macpijan commented 3 months ago

@mkopec @miczyg1 @krystian-hebel do you have any feedback on this, especially the size?

SergiiDmytruk commented 3 months ago

I have feedback on the size :) I reverted commits that switch to OpenSSL 3 to verify my guess and payload size became almost the same as before. In the process, I glanced over the 27 commits it took upstream to perform the switch and saw that they did some adjustments to reduce size of OpenSSL 3.

The conclusion is that we might not able to generally reduce its footprint by much. https://github.com/Dasharo/edk2/commit/dfa6147a79ac792e964cb36da0f53dd99e973dcf looks promising though. Don't know how helpful it would be to use those *Null libraries and where it's safe to switch to them, but will try to see whether it helps and what's the gain. EDIT: looks like those aren't libraries which can be included to reduce size because they are already applied.

Upstream must have done the switch in August 2023 because OpenSSL 1 is no longer maintained after September 2023, so reverting to it is probably not the best idea, but is an option if we have to.

SergiiDmytruk commented 2 months ago

Hm, I commented out OpenSSL-related stuff in HddPasswordDxe, its size went from 623.94 KiB to 47.94 KiB, but compressed size of the payload decreased by mere 6 KiB. UserAuthenticationDxe had very similar results. Looking a bit more at this, I found

which all boil down to "yes, OpenSSL3 is just larger (due to providers design)". There were some advices on decreasing the size and it looks like they were taken into account by upstream. This larger size is something we'll have to deal with by making more space, dropping features or maybe using a library that's more suitable for constrained environments (some seem to provide OpenSSL-compatible interface), but that might be unfeasible or not more secure than keeping OpenSSL1.

Dealt with some more TODO notes which I had:

got rid of EFI driver blobs for network which were introduced and then dropped
added several commits which were postponed until rebase is done
squashed some more commits with fixes
correctly fixed build issue of OpenSSL due to -Werror

@miczyg, should there be one more else branch at https://github.com/Dasharo/edk2/commit/f090a90394#diff-f34d17b28035e13540f82219722db7462c65b3d0e0f3061f2224836bae315b5cR314 like the one below?

} else {
 DEBUG ((DEBUG_INFO, "PS/2 keyboard not connected\n"));
 // Remove PS/2 Keyboard from ConIn
 EfiBootManagerUpdateConsoleVariable (ConIn, NULL, DevicePath);
}

I've noticed the inconsistency compared to other conditional statements nearby, but not sure whether this needs to change.

Sent https://github.com/Dasharo/coreboot/pull/486 to run CI and all failures seem to be a result of running out of space: 7 failing, 5 cancelled, and 8 successful checks (I guess some tasks got cancelled automatically).

I think the rebase is pretty much over. OpenSSL issue is a side effect of it, but I don't know if we want to deal with it here or elsewhere. One workaround is to keep using OpenSSL1 for now (just revert 27 commits in one commit) to not get blocked and handle it later.

Updated paste is here. Could be used for reference as to what was or wasn't squashed, because some commits are gone, some changed their titles, there were more things to squash and some of the original comments weren't applicable in which case I tried to provide information on that.

Also sent https://github.com/Dasharo/edk2/pull/129 for review, target branch is just to be able to open this PR.

SergiiDmytruk commented 2 months ago

[...] or maybe using a library that's more suitable for constrained environments (some seem to provide OpenSSL-compatible interface), but that might be unfeasible [...]

I wanted to revert switch to OpenSSL3 on an up-to-date branch to be able to see if it runs on hardware and ended up noticing that upstream has added and integrated MbedTls about the same time they switched to OpenSSL3! I must have been too fixated on OpenSSL to notice it earlier. Switching to CryptoPkgMbedTls results in shrinking of the compressed payload by 300 KiB compared to earlier releases of MSI, so that everything more than fits now.

SergiiDmytruk commented 2 months ago

Instead of booting an OS rebased EDK rebooted like this on Z790-P DDR5

Memory  Previous  Current    Next
 Type    Pages     Pages     Pages
======  ========  ========  ========
  09    00000008  00000014  00000019
  0A    00000004  00000019  0000001F
  00    00000004  0000004D  00000060
  06    000000C0  00000199  000001FF
  05    00000080  00000031  0000003D
Memory Type Information settings change.
...Warm Reset!!!

until I added

gEfiMdeModulePkgTokenSpaceGuid.PcdResetOnMemoryTypeInformationChange|FALSE

to DasharoPayloadPkg which was added to UefiPayloadPkg in 2021, so I guess it's required now, although I don't know why.

While looking at that log, I've also noticed asserts related to missing functionality in MbedTLS:

SM3 hash
RsaGetPublicKeyFromX509()

In fact, it seems to be missing quite a few API calls:

Cipher/CryptAeadAesGcmNull.c
Hash/CryptParallelHashNull.c
Hash/CryptSm3Null.c
Pk/CryptRsaExtNull.c
Pk/CryptRsaPssSignNull.c
Bn/CryptBnNull.c
Pem/CryptPemNull.c
Pk/CryptDhNull.c
Pk/CryptEcNull.c
Pk/CryptPkcs1OaepNull.c
Pk/CryptPkcs5Pbkdf2Null.c
Pk/CryptPkcs7SignNull.c
Pk/CryptPkcs7VerifyNull.c
Pk/CryptPkcs7VerifyEkuNull.c
Pk/CryptX509Null.c
Pk/CryptAuthenticodeNull.c
Pk/CryptTsNull.c
Rand/CryptRandNull.c

which could be a problem, but there are open PRs to add missing functionality:

The problem is Dasharo/edk2@6be227e28d introduces such cycle as well, so it's undone for now on a new branch as I'm not yet sure how to handle it properly.

EDK can still be built by coreboot if I drop that "revert", so maybe it's not an issue when all proper variables are set.

Well, actually debug version doesn't build. I was thinking that maybe DxeRuntimeDebugLibSerialPort should only be used in releases, but given that release builds, maybe this library doesn't build in release at all and thus currently has no effect?

macpijan commented 2 months ago

Switching to CryptoPkgMbedTls results in shrinking of the compressed payload by 300 KiB compared to earlier releases of MSI, so that everything more than fits now.

That sounds much better now :smile:

In fact, it seems to be missing quite a few API calls:

I wonder if it may be missing something that we need. We need to run some tests to find out.

We definitely will need crypto features required for UEFI Secure Boot.

SergiiDmytruk commented 2 months ago

I wonder if it may be missing something that we need. We need to run some tests to find out.

Tried enabling SecureBoot, found a related rebase issue, but SecureBoot still doesn't work even with OpenSSL. It can't set any secure variables (this is in QEmu). The issue seems to be related to signatures in secure variables, but don't yet know why the check doesn't pass after rebase.

SergiiDmytruk commented 2 months ago

OK, fix for enrolling dbx default needs to be done in reverse now: drop time based payload header for dbx instead of adding the header to everything else. With it, I see no asserts when enabling Secure Boot or trying to boot in QEMU, could be different on hardware though. Updated PRs and added short description to EDK2's PR on things to pay attention for.

Wenxing-hou commented 2 months ago

Hi all, I am upstreaming the missing features for CryptoPkgMbedTls.

SergiiDmytruk commented 1 month ago

Hi all, I am upstreaming the missing features for CryptoPkgMbedTls.

Hi. Thanks for that. I can see PRs are closed and commits merged (https://github.com/tianocore/edk2/commit/08281572aab5b1f7e05bf26de4148af19eddc8b7 and parents).

Because of some assertions related to missing functionality in MbedTLS with the latest release of EDK, I want to cherry-pick all these changes to reduce the probability of something not working in a non-obvious way.

One thing looks weird though, doesn't https://github.com/tianocore/edk2/commit/08281572aab5b1f7e05bf26de4148af19eddc8b7 bring OpenSSL dependency back for SM3 thus defeating the whole purpose of using MbedTLS? https://github.com/tianocore/edk2/pull/5573 just used some of OpenSSL sources without the whole library.

SergiiDmytruk commented 1 month ago

Used OpenSSL-less version in https://github.com/Dasharo/edk2/pull/129/commits/766b5a9292f276588c6cd4141d0e1a6b61bc1c9f. Also https://github.com/Dasharo/edk2/pull/129/commits/bd05440cf1c04003a06098b238af9822bb20ec27 could be of interest as it fixes some LTO warnings produced by the build.

Dasharo / dasharo-issues

[edk2 rebase - task 1] Cherry-pick required commits on top of recent edk2 #432