Disable additional network components when most are disabled, in all platforms in OvmfPkg and ArmVirtPkg #6092

mikebeaton · 2024-08-22T22:12:28Z

Description

This PR resolves the other issue identified in #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 #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 d933ec1 and 7f17a15).

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 d933ec1 followed by 7f17a15). 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 #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.

tianocore-assign-reviewers · 2024-08-22T22:12:56Z

⚠ 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:

Attn Admins:

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 · 2024-08-22T22:18:48Z

⚠ 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:

Attn Admins:

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 · 2024-08-22T22:29:07Z

⚠ 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:

Attn Admins:

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 · 2024-08-22T22:44:11Z

⚠ 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:

Attn Admins:

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 · 2024-08-22T22:54:10Z

⚠ 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:

Attn Admins:

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 · 2024-08-22T23:07:12Z

⚠ 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:

Attn Admins:

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 · 2024-08-22T23:16:12Z

⚠ 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:

Attn Admins:

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

mikebeaton · 2024-09-12T15:21:53Z

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.)

mikebeaton · 2024-09-12T18:04:13Z

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 · 2024-09-13T04:02:07Z

PR can not be merged due to conflict. Please rebase and resubmit

kraxel · 2024-09-13T05:56:48Z

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.

While fixing tianocore#6092 (the fact that some OvmfPkg and ArmVirtPkg platforms included residual NetworkPkg components even when compiled with -D NETWORK_ENABLE=0), it was noted that OvmfPkg/Include/*/Shell*.inc files which apply the required fix logic are available and already used in some OvmfPkg platforms. This commit applies these files consistently within OvmfPkg. This has the side effect that some platforms now include one or more of HttpDynamicCommand, VariablePolicyDynamicCommand and LinuxInitrdDynamicShellCommand when they previously did not. It is believed that in all cases these changes are neutral (i.e. not necessarily needed, but not harmful, and with the benefit of now using shared code) or positive (i.e. they fix unintentional drift between platforms, and provide additional shell commands which may be useful in some cases). Signed-off-by: Mike Beaton <[email protected]>

While fixing tianocore#6092 (the fact that some OvmfPkg and ArmVirtPkg platforms included residual NetworkPkg components even when compiled with -D NETWORK_ENABLE=0), it was noted that OvmfPkg/Include/*/Shell*.inc files which apply the required fix logic are available and already used in some OvmfPkg platforms. A previous commit applied these files consistently within OvmfPkg. This commit applies these files within ArmVirtPkg. This has the side effect that some platforms now include one or more of HttpDynamicCommand, VariablePolicyDynamicCommand and LinuxInitrdDynamicShellCommand when they previously did not. It is believed that in all cases these changes are neutral (i.e. not necessarily needed, but not harmful, and with the benefit of now using shared code) or positive (i.e. they fix unintentional drift between platforms, and provide additional shell commands which may be useful in some cases). Signed-off-by: Mike Beaton <[email protected]>

While fixing tianocore#6092 (the fact that some OvmfPkg and ArmVirtPkg platforms included residual NetworkPkg components even when compiled with -D NETWORK_ENABLE=0), it was noted that OvmfPkg/Include/*/Shell*.inc files which apply the required fix logic are available and already used in some OvmfPkg platforms. This commit applies these files consistently within OvmfPkg. This has the side effect that some platforms now include one or more of HttpDynamicCommand, VariablePolicyDynamicCommand and LinuxInitrdDynamicShellCommand when they previously did not. It is believed that in all cases these changes are neutral (i.e. not necessarily needed, but not harmful, and with the benefit of now using shared code) or positive (i.e. they fix unintentional drift between platforms, and provide additional shell commands which may be useful in some cases). Signed-off-by: Mike Beaton <[email protected]>

While fixing tianocore#6092 (the fact that some OvmfPkg and ArmVirtPkg platforms included residual NetworkPkg components even when compiled with -D NETWORK_ENABLE=0), it was noted that OvmfPkg/Include/*/Shell*.inc files which apply the required fix logic are available and already used in some OvmfPkg platforms. A previous commit applied these files consistently within OvmfPkg. This commit applies these files within ArmVirtPkg. This has the side effect that some platforms now include one or more of HttpDynamicCommand, VariablePolicyDynamicCommand and LinuxInitrdDynamicShellCommand when they previously did not. It is believed that in all cases these changes are neutral (i.e. not necessarily needed, but not harmful, and with the benefit of now using shared code) or positive (i.e. they fix unintentional drift between platforms, and provide additional shell commands which may be useful in some cases). Signed-off-by: Mike Beaton <[email protected]>

While fixing tianocore#6092 (the fact that some OvmfPkg and ArmVirtPkg platforms included residual NetworkPkg components even when compiled with -D NETWORK_ENABLE=0), it was noted that OvmfPkg/Include/*/Shell*.inc files which apply the required fix logic are available and already used in some OvmfPkg platforms. This commit applies these files consistently within OvmfPkg. This has the side effect that some platforms now include one or more of HttpDynamicCommand, VariablePolicyDynamicCommand and LinuxInitrdDynamicShellCommand when they previously did not. It is believed that in all cases these changes are neutral (i.e. not necessarily needed, but not harmful, and with the benefit of now using shared code) or positive (i.e. they fix unintentional drift between platforms, and provide additional shell commands which may be useful in some cases). Signed-off-by: Mike Beaton <[email protected]>

While fixing tianocore#6092 (the fact that some OvmfPkg and ArmVirtPkg platforms included residual NetworkPkg components even when compiled with -D NETWORK_ENABLE=0), it was noted that OvmfPkg/Include/*/Shell*.inc files which apply the required fix logic are available and already used in some OvmfPkg platforms. A previous commit applied these files consistently within OvmfPkg. This commit applies these files within ArmVirtPkg. This has the side effect that some platforms now include one or more of HttpDynamicCommand, VariablePolicyDynamicCommand and LinuxInitrdDynamicShellCommand when they previously did not. It is believed that in all cases these changes are neutral (i.e. not necessarily needed, but not harmful, and with the benefit of now using shared code) or positive (i.e. they fix unintentional drift between platforms, and provide additional shell commands which may be useful in some cases). Signed-off-by: Mike Beaton <[email protected]>

This issue showed up when addressing https://bugzilla.tianocore.org/show_bug.cgi?id=4829 in tianocore#6087 . Various OvmfPkg and ArmVirtPkg platforms include some residual NetworkPkg components when compiled with -D NETWORK_ENABLE=0, even though they use NetworkPkg includes intended to allow all NetworkPkg components to be disabled on this flag. For the OvmfPkg Intel platforms only, commit d933ec1 started the change of not including these residual NetworkPkg components, and commit 7f17a15 completed it. This commit rolls these changes out to the remaining OvmfPkg platforms where they make sense in the same way. Signed-off-by: Mike Beaton <[email protected]>

This issue showed up when addressing https://bugzilla.tianocore.org/show_bug.cgi?id=4829 in tianocore#6087 . Various OvmfPkg and ArmVirtPkg platforms include some residual NetworkPkg components when compiled with -D NETWORK_ENABLE=0, even though they use NetworkPkg includes intended to allow all NetworkPkg components to be disabled on this flag. For the OvmfPkg Intel platforms only, commit d933ec1 started the change of not including these residual NetworkPkg components, and commit 7f17a15 completed it. This commit rolls these changes out to the ArmVirtPkg platforms where they make sense in the same way. Signed-off-by: Mike Beaton <[email protected]>

While fixing tianocore#6092 (the fact that some OvmfPkg and ArmVirtPkg platforms included residual NetworkPkg components even when compiled with -D NETWORK_ENABLE=0), it was noted that OvmfPkg/Include/*/Shell*.inc files which apply the required fix logic are available and already used in some OvmfPkg platforms. This commit applies these files consistently within OvmfPkg. This has the side effect that some platforms now include one or more of HttpDynamicCommand, VariablePolicyDynamicCommand and LinuxInitrdDynamicShellCommand when they previously did not. It is believed that in all cases these changes are neutral (i.e. not necessarily needed, but not harmful, and with the benefit of now using shared code) or positive (i.e. they fix unintentional drift between platforms, and provide additional shell commands which may be useful in some cases). Signed-off-by: Mike Beaton <[email protected]>

While fixing tianocore#6092 (the fact that some OvmfPkg and ArmVirtPkg platforms included residual NetworkPkg components even when compiled with -D NETWORK_ENABLE=0), it was noted that OvmfPkg/Include/*/Shell*.inc files which apply the required fix logic are available and already used in some OvmfPkg platforms. A previous commit applied these files consistently within OvmfPkg. This commit applies these files within ArmVirtPkg. This has the side effect that some platforms now include one or more of HttpDynamicCommand, VariablePolicyDynamicCommand and LinuxInitrdDynamicShellCommand when they previously did not. It is believed that in all cases these changes are neutral (i.e. not necessarily needed, but not harmful, and with the benefit of now using shared code) or positive (i.e. they fix unintentional drift between platforms, and provide additional shell commands which may be useful in some cases). Signed-off-by: Mike Beaton <[email protected]>

While fixing tianocore#6092 (the fact that some OvmfPkg and ArmVirtPkg platforms included residual NetworkPkg components even when compiled with -D NETWORK_ENABLE=0), it was noted that OvmfPkg/Include/*/Shell*.inc files which apply the required fix logic are available and already used in some OvmfPkg platforms. This commit applies these files consistently within OvmfPkg. This has the side effect that some platforms now include one or more of HttpDynamicCommand, VariablePolicyDynamicCommand and LinuxInitrdDynamicShellCommand when they previously did not. It is believed that in all cases these changes are neutral (i.e. not necessarily needed, but not harmful, and with the benefit of now using shared code) or positive (i.e. they fix unintentional drift between platforms, and provide additional shell commands which may be useful in some cases). Signed-off-by: Mike Beaton <[email protected]>

While fixing tianocore#6092 (the fact that some OvmfPkg and ArmVirtPkg platforms included residual NetworkPkg components even when compiled with -D NETWORK_ENABLE=0), it was noted that OvmfPkg/Include/*/Shell*.inc files which apply the required fix logic are available and already used in some OvmfPkg platforms. A previous commit applied these files consistently within OvmfPkg. This commit applies these files within ArmVirtPkg. This has the side effect that some platforms now include one or more of HttpDynamicCommand, VariablePolicyDynamicCommand and LinuxInitrdDynamicShellCommand when they previously did not. It is believed that in all cases these changes are neutral (i.e. not necessarily needed, but not harmful, and with the benefit of now using shared code) or positive (i.e. they fix unintentional drift between platforms, and provide additional shell commands which may be useful in some cases). Signed-off-by: Mike Beaton <[email protected]>

Place the EFI shell as EFI/BOOT/BOOT{ARCH}.EFI on the virtual drive. This allows the "Run to shell" CI test case to work even in case the shell is not included in the firmware image. This is needed because a follow up patch will exclude the shell from secure boot enabled firmware images. The same update was previously applied to OvmfPkg by 6862b9d. Signed-off-by: Gerd Hoffmann <[email protected]> Signed-off-by: Mike Beaton <[email protected]>

While fixing tianocore#6092 (the fact that some OvmfPkg and ArmVirtPkg platforms included residual NetworkPkg components even when compiled with -D NETWORK_ENABLE=0), it was noted that OvmfPkg/Include/*/Shell*.inc files which apply the required fix logic are available and already used in some OvmfPkg platforms. This commit applies these files consistently within OvmfPkg. This has the side effect that some platforms now include one or more of HttpDynamicCommand, VariablePolicyDynamicCommand and LinuxInitrdDynamicShellCommand when they previously did not. It is believed that in all cases these changes are neutral (i.e. not necessarily needed, but not harmful, and with the benefit of now using shared code) or positive (i.e. they fix unintentional drift between platforms, and provide additional shell commands which may be useful in some cases). Signed-off-by: Mike Beaton <[email protected]>

While fixing tianocore#6092 (the fact that some OvmfPkg and ArmVirtPkg platforms included residual NetworkPkg components even when compiled with -D NETWORK_ENABLE=0), it was noted that OvmfPkg/Include/*/Shell*.inc files which apply the required fix logic are available and already used in some OvmfPkg platforms. A previous commit applied these files consistently within OvmfPkg. This commit applies these files within ArmVirtPkg. This has the side effect that some platforms now include one or more of HttpDynamicCommand, VariablePolicyDynamicCommand and LinuxInitrdDynamicShellCommand when they previously did not. It is believed that in all cases these changes are neutral (i.e. not necessarily needed, but not harmful, and with the benefit of now using shared code) or positive (i.e. they fix unintentional drift between platforms, and provide additional shell commands which may be useful in some cases). Signed-off-by: Mike Beaton <[email protected]>

tianocore-assign-reviewers · 2024-09-13T15:26:53Z

⚠ 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:

Attn Admins:

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

mikebeaton · 2024-09-13T15:43:05Z

@kraxel - I've switched to using the existing approach to handling Secure Boot + UEFI Shell + CI.

@ardbiesheuvel - I've fixed the whitespace issues.

tianocore-assign-reviewers bot requested review from ajfish, andreiw, ardbiesheuvel, bcran, jongwu, jyao1, kilaterlee, kraxel, leiflindholm, mdkinney, SaloniKasbekar, samimujawar, tperard, vlsunil and weltling August 22, 2024 22:12

mikebeaton force-pushed the NetworkEnable0 branch from 5805823 to 1ee4854 Compare August 22, 2024 22:18

tianocore-assign-reviewers bot requested a review from Zclarkwilliams August 22, 2024 22:29

mikebeaton force-pushed the NetworkEnable0 branch 2 times, most recently from 3e966fb to f4e8c3a Compare August 22, 2024 22:44

mikebeaton marked this pull request as draft August 23, 2024 05:57

mikebeaton force-pushed the NetworkEnable0 branch from 994132b to 758b1cf Compare August 23, 2024 06:06

tianocore-assign-reviewers bot requested review from mxu9, ruleof2 and tlendacky September 12, 2024 15:09

mikebeaton marked this pull request as draft September 13, 2024 09:01

mikebeaton added 2 commits September 13, 2024 11:00

mikebeaton force-pushed the NetworkEnable0 branch from bd25067 to 6bed108 Compare September 13, 2024 14:50

mikebeaton added 3 commits September 13, 2024 15:51

mikebeaton force-pushed the NetworkEnable0 branch from 6bed108 to a784f93 Compare September 13, 2024 14:52

mikebeaton marked this pull request as ready for review September 13, 2024 14:55

ardbiesheuvel approved these changes Sep 13, 2024

View reviewed changes

kraxel approved these changes Sep 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable additional network components when most are disabled, in all platforms in OvmfPkg and ArmVirtPkg #6092

Disable additional network components when most are disabled, in all platforms in OvmfPkg and ArmVirtPkg #6092

mikebeaton commented Aug 22, 2024 •

edited

Loading

tianocore-assign-reviewers bot commented Aug 22, 2024

tianocore-assign-reviewers bot commented Aug 22, 2024

tianocore-assign-reviewers bot commented Aug 22, 2024

tianocore-assign-reviewers bot commented Aug 22, 2024

tianocore-assign-reviewers bot commented Aug 22, 2024

tianocore-assign-reviewers bot commented Aug 22, 2024

tianocore-assign-reviewers bot commented Aug 22, 2024

mikebeaton commented Sep 12, 2024

mikebeaton commented Sep 12, 2024

mergify bot commented Sep 13, 2024

kraxel commented Sep 13, 2024

tianocore-assign-reviewers bot commented Sep 13, 2024

mikebeaton commented Sep 13, 2024

Disable additional network components when most are disabled, in all platforms in OvmfPkg and ArmVirtPkg #6092

Are you sure you want to change the base?

Disable additional network components when most are disabled, in all platforms in OvmfPkg and ArmVirtPkg #6092

Conversation

mikebeaton commented Aug 22, 2024 • edited Loading

Description

How This Was Tested

Integration Instructions

tianocore-assign-reviewers bot commented Aug 22, 2024

tianocore-assign-reviewers bot commented Aug 22, 2024

tianocore-assign-reviewers bot commented Aug 22, 2024

tianocore-assign-reviewers bot commented Aug 22, 2024

tianocore-assign-reviewers bot commented Aug 22, 2024

tianocore-assign-reviewers bot commented Aug 22, 2024

tianocore-assign-reviewers bot commented Aug 22, 2024

mikebeaton commented Sep 12, 2024

mikebeaton commented Sep 12, 2024

mergify bot commented Sep 13, 2024

kraxel commented Sep 13, 2024

tianocore-assign-reviewers bot commented Sep 13, 2024

mikebeaton commented Sep 13, 2024

mikebeaton commented Aug 22, 2024 •

edited

Loading