edk2
edk2 copied to clipboard
tianocore
Disable additional network components when most are disabled, in all platforms in OvmfPkg and ArmVirtPkg
Description
This PR resolves the other issue identified in https://github.com/tianocore/edk2/pull/6087 - namely that various other platforms were only not showing the same problem, of failing to build when -D NETWORK_ENABLE=0 is specified, because they were incorrectly still including some NetworkPkg components when most were disabled by -D NETWORK_ENABLE=0 (which masks the issue in https://github.com/tianocore/edk2/pull/6087). To identify the changes to make, it was necessary to identify which NetworkPkg components were being unnecessarily included, and then to add conditional include statements for the affected components (all of which were already conditionally included in the Intel OvmfPkg platforms, after changes introduced in d933ec115bdf9be1d8dfe6a818414a14973cc0d3 and 7f17a155640a2a9e1f7b0f3522628ee2c6f62624).
EDIT: I have now split the original, single cross-package commit into two. On the request/suggestion below, I have now also added ~two~ further commits which implement the change of using the OvmfPkg/Include/*/Shell*.inc files (which already had this logic and were used in some platforms), instead of just copying the logic from them. ~These changes may well be worthwhile, though (as I somewhat suspected) they were slightly less straightforward to implement than might have been expected. Additional comments on these extra commits below.~ (After a pointer from @kraxel as to how the shell+secure boot+CI thing worked in OvmfPkg, things became more straightforward. :-))
- [ ] Breaking change? NO
- [ ] Impacts security? NO
- [ ] Includes tests? NO
How This Was Tested
Identify all NetworkPkg components still being included when compiled with -D NETWORK_ENABLE=0, in platforms which were already using NetworkPkg includes intended to allow all NetworkPkg components to be disabled via -D NETWORK_ENABLE=0.
After the changes to avoid these residual NetworkPkg includes, test that all the platforms affected would not build with -D NETWORK_ENABLE=0 (as already happened in the Intel Ovmf platforms, on which similar changes to avoid residual NetworkPkg includes had been made by d933ec115bdf9be1d8dfe6a818414a14973cc0d3 followed by 7f17a155640a2a9e1f7b0f3522628ee2c6f62624). This confirms that no NetworkPkg components are being included any more. Confirm that these platforms still build the same as before with -D NETWORK_ENABLE=1 (default), and still pass CI.
Integration Instructions
The paired change https://github.com/tianocore/edk2/pull/6087 (which can be applied before or after this) resolves the remaining issue of access to undefined Pcds and allows the platforms changed here to build again with -D NETWORK_ENABLE=0.
⚠ WARNING: Cannot add some reviewers: A user specified as a reviewer for this PR is not a collaborator of the repository. Please add them as a collaborator to the repository so they can be requested in the future.
Non-collaborators requested:
- @bibo-mao
- @corvink
- @lixianglai
Attn Admins:
- @jljusten
- @lgao4
- @mdkinney
- @vincent-j-zimmer
- @jkmathews
- @miki-intel-work
Admin Instructions:
- Add the non-collaborators as collaborators to the appropriate team(s) listed in teams
- If they are no longer needed as reviewers, remove them from
Maintainers.txt
![tianocore-assign-reviewers[bot] avatar](https://avatars.githubusercontent.com/u/352162?v=4 "Published by a pub.dev verified publisher"){kind=small}
⚠ WARNING: Cannot add some reviewers: A user specified as a reviewer for this PR is not a collaborator of the repository. Please add them as a collaborator to the repository so they can be requested in the future.
Non-collaborators requested:
- @bibo-mao
- @corvink
- @lixianglai
Attn Admins:
- @jljusten
- @lgao4
- @mdkinney
- @vincent-j-zimmer
- @jkmathews
- @miki-intel-work
Admin Instructions:
- Add the non-collaborators as collaborators to the appropriate team(s) listed in teams
- If they are no longer needed as reviewers, remove them from
Maintainers.txt
⚠ WARNING: Cannot add some reviewers: A user specified as a reviewer for this PR is not a collaborator of the repository. Please add them as a collaborator to the repository so they can be requested in the future.
Non-collaborators requested:
- @bibo-mao
- @corvink
- @lixianglai
Attn Admins:
- @jljusten
- @lgao4
- @mdkinney
- @vincent-j-zimmer
- @jkmathews
- @miki-intel-work
Admin Instructions:
- Add the non-collaborators as collaborators to the appropriate team(s) listed in teams
- If they are no longer needed as reviewers, remove them from
Maintainers.txt
⚠ WARNING: Cannot add some reviewers: A user specified as a reviewer for this PR is not a collaborator of the repository. Please add them as a collaborator to the repository so they can be requested in the future.
Non-collaborators requested:
- @bibo-mao
- @corvink
- @lixianglai
Attn Admins:
- @jljusten
- @lgao4
- @mdkinney
- @vincent-j-zimmer
- @jkmathews
- @miki-intel-work
Admin Instructions:
- Add the non-collaborators as collaborators to the appropriate team(s) listed in teams
- If they are no longer needed as reviewers, remove them from
Maintainers.txt
⚠ WARNING: Cannot add some reviewers: A user specified as a reviewer for this PR is not a collaborator of the repository. Please add them as a collaborator to the repository so they can be requested in the future.
Non-collaborators requested:
- @bibo-mao
- @corvink
- @lixianglai
Attn Admins:
- @jljusten
- @lgao4
- @mdkinney
- @vincent-j-zimmer
- @jkmathews
- @miki-intel-work
Admin Instructions:
- Add the non-collaborators as collaborators to the appropriate team(s) listed in teams
- If they are no longer needed as reviewers, remove them from
Maintainers.txt
⚠ WARNING: Cannot add some reviewers: A user specified as a reviewer for this PR is not a collaborator of the repository. Please add them as a collaborator to the repository so they can be requested in the future.
Non-collaborators requested:
- @bibo-mao
- @corvink
- @lixianglai
Attn Admins:
- @jljusten
- @lgao4
- @mdkinney
- @vincent-j-zimmer
- @jkmathews
- @miki-intel-work
Admin Instructions:
- Add the non-collaborators as collaborators to the appropriate team(s) listed in teams
- If they are no longer needed as reviewers, remove them from
Maintainers.txt
⚠ WARNING: Cannot add some reviewers: A user specified as a reviewer for this PR is not a collaborator of the repository. Please add them as a collaborator to the repository so they can be requested in the future.
Non-collaborators requested:
- @bibo-mao
- @corvink
- @lixianglai
Attn Admins:
- @jljusten
- @lgao4
- @mdkinney
- @vincent-j-zimmer
- @jkmathews
- @miki-intel-work
Admin Instructions:
- Add the non-collaborators as collaborators to the appropriate team(s) listed in teams
- If they are no longer needed as reviewers, remove them from
Maintainers.txt
⚠ WARNING: Cannot add some reviewers: A user specified as a reviewer for this PR is not a collaborator of the repository. Please add them as a collaborator to the repository so they can be requested in the future.
Non-collaborators requested:
- @bibo-mao
- @corvink
- @lixianglai
Attn Admins:
- @jljusten
- @lgao4
- @mdkinney
- @vincent-j-zimmer
- @jkmathews
- @miki-intel-work
Admin Instructions:
- Add the non-collaborators as collaborators to the appropriate team(s) listed in teams
- If they are no longer needed as reviewers, remove them from
Maintainers.txt
⚠ WARNING: Cannot add some reviewers: A user specified as a reviewer for this PR is not a collaborator of the repository. Please add them as a collaborator to the repository so they can be requested in the future.
Non-collaborators requested:
- @bibo-mao
- @corvink
- @lixianglai
Attn Admins:
- @jljusten
- @lgao4
- @mdkinney
- @vincent-j-zimmer
- @jkmathews
- @miki-intel-work
Admin Instructions:
- Add the non-collaborators as collaborators to the appropriate team(s) listed in teams
- If they are no longer needed as reviewers, remove them from
Maintainers.txt
I have realised it makes more sense for the change here to be a separate PR, so have put this back to just one commit and have re-opened https://github.com/tianocore/edk2/pull/6087 (with the minor update there to fix LoongArchVirtQemu as well since it is also affected, as discovered while preparing this commit).
⚠ WARNING: Cannot add some reviewers: A user specified as a reviewer for this PR is not a collaborator of the repository. Please add them as a collaborator to the repository so they can be requested in the future.
Non-collaborators requested:
- @bibo-mao
- @corvink
- @lixianglai
Attn Admins:
- @jljusten
- @lgao4
- @mdkinney
- @vincent-j-zimmer
- @jkmathews
- @miki-intel-work
Admin Instructions:
- Add the non-collaborators as collaborators to the appropriate team(s) listed in teams
- If they are no longer needed as reviewers, remove them from
Maintainers.txt
⚠ WARNING: Cannot add some reviewers: A user specified as a reviewer for this PR is not a collaborator of the repository. Please add them as a collaborator to the repository so they can be requested in the future.
Non-collaborators requested:
- @bibo-mao
- @corvink
- @lixianglai
Attn Admins:
- @jljusten
- @lgao4
- @mdkinney
- @vincent-j-zimmer
- @jkmathews
- @miki-intel-work
Admin Instructions:
- Add the non-collaborators as collaborators to the appropriate team(s) listed in teams
- If they are no longer needed as reviewers, remove them from
Maintainers.txt
How about switching the builds over to use the OvmfPkg/Include/*/Shell*.inc include files (which already have this logic)?
@kraxel - The existing includes are not applicable everywhere. These platforms all have slight variants of what's included, so switching to those includes in the obvious places wouldn't be like-for-like.
https://github.com/tianocore/edk2/blob/master/OvmfPkg/Include/Dsc/NetworkComponents.dsc.inc#L18-L34 has additional library and entry point settings for Ip4Dxe.inf and Ip6Dxe.inf. These settings aren't currently applied at any of the other places where we might otherwise want to include the top lines from this file (search for UefiPxeBcDxe in Files Changed of this PR).
https://github.com/tianocore/edk2/blob/master/OvmfPkg/Include/Fdf/ShellDxe.fdf.inc matches what is included in some but not all of the places where we might want to use it (search for ShellPkg/DynamicCommand/TftpDynamicCommand/TftpDynamicCommand.inf within .fdf files in Files Changed). Might also require BUILD_SHELL or SECURE_BOOT_ENABLE defines to be present, if they are not already in the other packages?
https://github.com/tianocore/edk2/blob/master/OvmfPkg/Include/Dsc/ShellComponents.dsc.inc similar to the above: some of the places where we might approximately want to use it only include TftpDynamicCommand (not both TftpDynamicCommand and HttpDynamicCommand), so not like-for-like. Also, this file is the full Shell components list, so again even where it is clearly more or less the same thing as what it would (approximately) replace, the exact components imported (and possibly the defines present, to trigger the correct behaviour of the include file?) are not exactly the same between the packages.
With some updates and care (and preferably local running versions of all the platforms being changed, to be sure it's all working; I tend to use OvmfPkg Intel variants, and have dabbled with running ArmVirtPkg, so far!), this could be done, but not sure if this is enough to render all this possibly more suitable for future work?
I don't think the ShellPkg/DynamicCommand/* differences are intentional, they probably sneaked in over time as commands have been added and not all *.dsc files have been updated.
How about switching the builds over to use the
OvmfPkg/Include/*/Shell*.incinclude files (which already have this logic)?
@kraxel This is an initial attempt to do the above correctly. I am not yet quite sure why a few CI tests are failing, if you have any pointers? If I build and run X64 GCC ArmVirtQemu by hand, from that commit, then it reaches the UEFI Shell correctly after pausing in PXE boot for a bit - I believe as expected.
EDIT: Actually I can see in the CI logs that the right boot options are not there in the CI run. I guess I should be able to recreate the CI situation at home, eventually - unless you can spot anything?! :-)
EDIT 2: Right, I believe the issue is that the build matrix defines SECURE_BOOT_ENABLE, for AARCH64 only (https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=149296&view=logs&j=a3cbe7a7-5fa9-5965-2615-b39fbc4e581a and expand 'Job preparation parameters'). Since the include files (more or less correctly, I'd say) don't include UEFI Shell in the FDF when SECURE_BOOT_ENABLE is TRUE, the 'Run to shell' test for AARCH64 now fails.
@kraxel @ardbiesheuvel -
I have split the previous cross-package commit into two.
I have also implemented the suggested change of using the OvmfPkg/Include/*/Shell*.inc files (which already had this logic and were used in some platforms), in two further commits. As can be seen, this was not as trivial as it might perhaps have sounded - which is the main reason that it seems right to keep the initial changes (which actually stop unwanted network components from being included, without otherwise changing anything) separate from the the somewhat more invasive changes involved in sharing the include files.
EDIT 2: Right, I believe the issue is that the build matrix defines SECURE_BOOT_ENABLE, for AARCH64 only (https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=149296&view=logs&j=a3cbe7a7-5fa9-5965-2615-b39fbc4e581a and expand 'Job preparation parameters'). Since the include files (more or less correctly, I'd say) don't include UEFI Shell in the FDF when SECURE_BOOT_ENABLE is TRUE, the 'Run to shell' test for AARCH64 now fails.
See https://github.com/tianocore/edk2/commit/bc982869dd3e how this works for OVMF.
⚠ WARNING: Cannot add some reviewers: A user specified as a reviewer for this PR is not a collaborator of the repository. Please add them as a collaborator to the repository so they can be requested in the future.
Non-collaborators requested:
- @bibo-mao
- @corvink
- @lixianglai
Attn Admins:
- @jljusten
- @lgao4
- @mdkinney
- @vincent-j-zimmer
- @jkmathews
- @miki-intel-work
Admin Instructions:
- Add the non-collaborators as collaborators to the appropriate team(s) listed in teams
- If they are no longer needed as reviewers, remove them from
Maintainers.txt
EDIT 2: Right, I believe the issue is that the build matrix defines SECURE_BOOT_ENABLE, for AARCH64 only (https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=149296&view=logs&j=a3cbe7a7-5fa9-5965-2615-b39fbc4e581a and expand 'Job preparation parameters'). Since the include files (more or less correctly, I'd say) don't include UEFI Shell in the FDF when SECURE_BOOT_ENABLE is TRUE, the 'Run to shell' test for AARCH64 now fails.
See bc982869dd3e how this works for OVMF.
Ah, right, I didn't spot this until just now and came up with my own different solution - which I think is actually reasonable. Possibly even worth considering using instead? (&/or of course, I can start up again, and switch to using this method in the places where I had problems.)
EDIT 2: Right, I believe the issue is that the build matrix defines SECURE_BOOT_ENABLE, for AARCH64 only (https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=149296&view=logs&j=a3cbe7a7-5fa9-5965-2615-b39fbc4e581a and expand 'Job preparation parameters'). Since the include files (more or less correctly, I'd say) don't include UEFI Shell in the FDF when SECURE_BOOT_ENABLE is TRUE, the 'Run to shell' test for AARCH64 now fails.
See bc982869dd3e how this works for OVMF.
@kraxel - I think I get it. When secure boot is enabled the shell is built, even though not bundled into firmware, as much as anything to allow the above approach to work in CI? I'll look into not breaking that(!), and adding it to ArmVirtPkg as needed.
![mergify[bot] avatar](https://avatars.githubusercontent.com/in/10562?v=4 "Published by a pub.dev verified publisher"){kind=small}
See bc982869dd3e how this works for OVMF.
@kraxel - I think I get it. When secure boot is enabled the shell is built, even though not bundled into firmware, as much as anything to allow the above approach to work in CI? I'll look into not breaking that(!), and adding it to ArmVirtPkg as needed.
Exactly, the idea is to build the shell unconditionally so CI can use it, but exclude it from the firmware images if secure boot is enabled.
⚠ WARNING: Cannot add some reviewers: A user specified as a reviewer for this PR is not a collaborator of the repository. Please add them as a collaborator to the repository so they can be requested in the future.
Non-collaborators requested:
- @bibo-mao
- @corvink
- @lixianglai
Attn Admins:
- @jljusten
- @lgao4
- @mdkinney
- @vincent-j-zimmer
- @jkmathews
- @miki-intel-work
Admin Instructions:
- Add the non-collaborators as collaborators to the appropriate team(s) listed in teams
- If they are no longer needed as reviewers, remove them from
Maintainers.txt
@kraxel - I've switched to using the existing approach to handling Secure Boot + UEFI Shell + CI.
@ardbiesheuvel - I've fixed the whitespace issues.
I'm not the owner / reviewer of the OvmfPkg files that were changed, so not sure why I would be a required reviewer. But if you really need my review approval, I guess I can do that, or you can remove me from the list.
@ardbiesheuvel , is there the quality risk to merge them for this table tag 202411? This patch has been reviewed before soft feature feature freeze. It can be merged for 202411.
Updated to fix minor merge conflicts with b3b3cfab7eb52acd77558a9727196e30d05d1f2a (cc @kraxel) and with addition of NETWORK_PXE_BOOT_ENABLE (https://github.com/tianocore/edk2/commits?author=xpahos cc @xpahos).
Have also updated the commit messages of the final two commits to be slightly more confident in tone.