Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

OVMF fix building without network support #6061

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

OVMF fix building without network support #6061

wants to merge 6 commits into from

Conversation

Joursoir
Copy link
Contributor

@Joursoir Joursoir commented Aug 7, 2024

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

@tianocore-assign-reviewers
Copy link

⚠ 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

@kraxel
Copy link
Member

kraxel commented Aug 21, 2024

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.

@tianocore-assign-reviewers
Copy link

⚠ 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

@Joursoir
Copy link
Contributor Author

Joursoir commented Sep 8, 2024

@kraxel please take a look, I tried to move PCDs to a separate include file

@kraxel
Copy link
Member

kraxel commented Sep 9, 2024

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?

@Joursoir
Copy link
Contributor Author

@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 Mergify
Copy link

mergify bot commented Sep 12, 2024

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

@Joursoir
NetworkPkg: introduce include file for dynamic PCDs
15c5b21 
Introduce an include file with dynamic PCDs to simplify the usage of
the network stage. This allows us to stop manually adding
`PcdIPv4PXESupport` and `PcdIPv6PXESupport` to the DSC file.

`NETWORK_IP4_ENABLE` and `NETWORK_IP6_ENABLE` are not used because
PXEv4 and PXEv6 boot support can be controlled from the QEMU command
line.

Cc: Saloni Kasbekar <[email protected]>
Cc: Zachary Clark-williams <[email protected]>
Signed-off-by: Aleksandr Goncharov <[email protected]>
@Joursoir
Copy link
Contributor Author

A similar patch has already been merged #6087

However, I don't believe it's the right approach to deal with network stuff just by adding a lot of DEFINEs IMHO

@Joursoir
OvmfPkg: use NetworkDynamicPcds include file
159996c 
Start using the include file in the OvmfPkg package to manage dynamic
network-related PCDs. This change removes the manual addition of
`PcdIPv4PXESupport` and `PcdIPv6PXESupport` from the DSC file,
relying instead on the centralized include file introduced in
NetworkPkg.

Cc: Ard Biesheuvel <[email protected]>
Cc: Jiewen Yao <[email protected]>
Cc: Gerd Hoffmann <[email protected]>
Cc: Jianyong Wu <[email protected]>
Cc: Anatol Belski <[email protected]>
Cc: Sunil V L <[email protected]>
Cc: Andrei Warkentin <[email protected]>
Cc: Chao Li <[email protected]>
Cc: Bibo Mao <[email protected]>
Cc: Xianglai Li <[email protected]>
Signed-off-by: Aleksandr Goncharov <[email protected]>
@Joursoir
ArmVirtPkg: use NetworkDynamicPcds include file
fac2482 
Start using the include file in the ArmVirtPkg package to manage
dynamic network-related PCDs. This change removes the manual addition
of `PcdIPv4PXESupport` and `PcdIPv6PXESupport` from the DSC file,
relying instead on the centralized include file introduced in
NetworkPkg.

Cc: Ard Biesheuvel <[email protected]>
Cc: Leif Lindholm <[email protected]>
Cc: Sami Mujawar <[email protected]>
Cc: Gerd Hoffmann <[email protected]>
Signed-off-by: Aleksandr Goncharov <[email protected]>
@Joursoir
NetworkPkg: mark fixed network PCDs
eba4547 
Rename `NetworkPcds` to `NetworkFixedPcds` to avoid confusion with
dynamic PCDs. The next patches in the chain will update all references
across the codebase to use the new name.

Cc: Saloni Kasbekar <[email protected]>
Cc: Zachary Clark-williams <[email protected]>
Signed-off-by: Aleksandr Goncharov <[email protected]>
@Joursoir
OvmfPkg: mark fixed network PCDs
bbdfbb9 
Rename `NetworkPcds` to `NetworkFixedPcds` to avoid confusion with
dynamic PCDs

Cc: Jiewen Yao <[email protected]>
Cc: Gerd Hoffmann <[email protected]>
Cc: Jianyong Wu <[email protected]>
Cc: Anatol Belski <[email protected]>
Cc: Sunil V L <[email protected]>
Cc: Andrei Warkentin <[email protected]>
Cc: Chao Li <[email protected]>
Cc: Bibo Mao <[email protected]>
Cc: Xianglai Li <[email protected]>
Signed-off-by: Aleksandr Goncharov <[email protected]>
@Joursoir
ArmVirtPkg: mark fixed network PCDs
2530c1f 
Rename `NetworkPcds` to `NetworkFixedPcds` to avoid confusion with
dynamic PCDs.

Cc: Leif Lindholm <[email protected]>
Cc: Sami Mujawar <[email protected]>
Cc: Gerd Hoffmann <[email protected]>
Signed-off-by: Aleksandr Goncharov <[email protected]>
@tianocore-assign-reviewers
Copy link

⚠ 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bexcran bexcran bexcran approved these changes

@SaloniKasbekar SaloniKasbekar SaloniKasbekar approved these changes

@kraxel kraxel kraxel approved these changes

@ajfish ajfish Awaiting requested review from ajfish

@ardbiesheuvel ardbiesheuvel Awaiting requested review from ardbiesheuvel

@jyao1 jyao1 Awaiting requested review from jyao1

@leiflindholm leiflindholm Awaiting requested review from leiflindholm

@mdkinney mdkinney Awaiting requested review from mdkinney

@Zclarkwilliams Zclarkwilliams Awaiting requested review from Zclarkwilliams

@andreiw andreiw Awaiting requested review from andreiw

@jongwu jongwu Awaiting requested review from jongwu

@kilaterlee kilaterlee Awaiting requested review from kilaterlee

@samimujawar samimujawar Awaiting requested review from samimujawar

@tperard tperard Awaiting requested review from tperard

@vlsunil vlsunil Awaiting requested review from vlsunil

@weltling weltling Awaiting requested review from weltling

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Joursoir @kraxel @SaloniKasbekar @bexcran