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

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.

mikebeaton · 2024-09-24T13:29:32Z

Is this PR waiting for further approvals?

mdkinney · 2024-09-27T15:31:44Z

Need some OvmfPkg approvals

tlendacky · 2024-09-27T20:08:41Z

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.

tperard

The changes seems to work fine for OvmfXen. Thanks.

vlsunil

RiscVVirt looks good to me.

lgao4 · 2024-11-11T02:26:01Z

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

tianocore-assign-reviewers bot requested review from ajfish, andreiw, ardbiesheuvel, a user, 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

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

tperard approved these changes Sep 30, 2024

View reviewed changes

This comment was marked as duplicate.

Sign in to view

vlsunil approved these changes Oct 9, 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

tianocore-assign-reviewers bot commented Sep 13, 2024

mikebeaton commented Sep 13, 2024

mikebeaton commented Sep 24, 2024

mdkinney commented Sep 27, 2024

tlendacky commented Sep 27, 2024

tperard left a comment

This comment was marked as duplicate.

vlsunil left a comment

lgao4 commented Nov 11, 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

tianocore-assign-reviewers bot commented Sep 13, 2024

mikebeaton commented Sep 13, 2024

mikebeaton commented Sep 24, 2024

mdkinney commented Sep 27, 2024

tlendacky commented Sep 27, 2024

tperard left a comment

Choose a reason for hiding this comment

This comment was marked as duplicate.

vlsunil left a comment

Choose a reason for hiding this comment

lgao4 commented Nov 11, 2024

mikebeaton commented Aug 22, 2024 •

edited

Loading