edk2
edk2 copied to clipboard
tianocore
OVMF fix building without network support
Description
It's currently not possible to compile OVMF x64 without the network stack. My patch fixes that.
The second commit fixes the build issue specifically for X64 QEMU. However, similar problems occur on other QEMU platforms (IA32, MicrovmX64, etc.) and ArmVirtPkg. I am interested in fixing these platforms as well. Moving PCDs to conditional expressions across all platforms doesn't seem like an ideal solution. Instead, I propose moving them to NetworkPcds.dsc.inc. Any thoughts?
- [ ] Breaking change?
- Breaking change - Does this PR cause a break in build or boot behavior?
- Examples: Does it add a new library class or move a module to a different repo.
- [ ] Impacts security?
- Security - Does this PR have a direct security impact?
- Examples: Crypto algorithm change or buffer overflow fix.
- [ ] Includes tests?
- Tests - Does this PR include any explicit test code?
- Examples: Unit tests or integration tests.
How This Was Tested
Tested on QEMU 9.0.2 with NETWORK_ENABLE=FALSE and NETWORK_ENABLE=TRUE builds.
Integration Instructions
N/A
⚠ 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:
- @Zclarkwilliams
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}
Creating a NetworkPcds.dsc.inc makes sense to me. I'd also suggest to add checks for NETWORK_IP4_ENABLE and NETWORK_IP6_ENABLE, following existing practice of the existing NetworkComponents.dsc.inc file.
⚠ 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 please take a look, I tried to move PCDs to a separate include file
Oh, I didn't realize there already is a NetworkPkg/NetworkPcds.dsc.inc file.
Any specific reason why you have created a new file instead of adding the PCDs to the existing file?
@kraxel Yeah, there's a specific reason why I created a new file rather than added the PCDs to the existing NetworkPkg/NetworkPcds.dsc.inc file.
At the moment, the NetworkPcds.dsc.inc file contains gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections without being associated with a specific section. However, this PCD is included in the [PcdsFixed*] section in the DSC files.
If I add new PCDs to this include file, I would need to make sure they are in the correct sections. This would mean changing the file to include different sections like:
[PcdsFixedAtBuild]
gEfiNetworkPkgTokenSpaceGuid.PcdAllowHttpConnections|TRUE
[PcdsDynamicDefault]
gEfiNetworkPkgTokenSpaceGuid.PcdIPv4PXESupport|0x01
gEfiNetworkPkgTokenSpaceGuid.PcdIPv6PXESupport|0x01
Then, I would have to modify the DSC files that use this include file, as they reference NetworkPcds.dsc.inc and continue PCD declarations afterward.
Therefore, I found that separating the PCDs into Dynamic and Fixed files is a cleaner and more manageable approach
![mergify[bot] avatar](https://avatars.githubusercontent.com/in/10562?v=4 "Published by a pub.dev verified publisher"){kind=small}
A similar patch has already been merged https://github.com/tianocore/edk2/pull/6087
However, I don't believe it's the right approach to deal with network stuff just by adding a lot of DEFINEs IMHO
⚠ 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
- @bcran
- @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
@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.
@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.
I think merging this is fine.
