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.

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

Update CI to VS2022 #6501

Merged
merged 3 commits into from
Dec 10, 2024
Merged

Update CI to VS2022 #6501

merged 3 commits into from
Dec 10, 2024

Conversation

os-d
Copy link
Contributor

@os-d os-d commented Dec 2, 2024

Description

This PR updates the CI pipelines to use VS2022 instead of VS2019 as that is the latest supported VS toolchain on edk2.

It contains two commits:

BaseTools: Add VS2022 XIPFLAGS

BaseTools has a limitation that modules in FVs that are force rebased
must have the same file and section alignment. This is intended for
XIP modules.

VS2019 and previous VS toolchains did not set 4k section alignment,
but VS2022 does, in order for memory protections to be applied to
images. This causes issues when building SEC and PEI modules on
VS2022 as the file alignment is 0x20 but the section alignment
is 0x1000, so BaseTools will fail to generate the FV. One option
is to set the file alignment to 0x1000 for all of these files, but
that is a large waste of space and is not feasible on some platforms
that have limited flash space. The other option is to selectively
set 0x20 as the section alignment for SEC and PEI modules, which is
the approach GCC ARM/AARCH64 took.

This is only an issue for building 64-bit PEI on x86 currently, as
other architectures are not supported by VS2022 in edk2 yet. For IA32,
the section alignment is set to 0x20 and so it matches the file
alignment, however x64 PEI uses the X64 DLINK flags which have 0x1000
set. For other architectures that don't have the PEI/DXE architecture
split, this is also an issue.

This commit is required to use VS2022 as the default CI in edk2, as
OvmfPkgX64.dsc will fail to build. Any platform with 64-bit PEI also
requires this.

This commit also updates CryptoPkg.dsc and SecurityPkg.dsc as they
are setting custom section alignments.

Update CI to VS2022

This PR updates the CI pipelines to use VS2022 instead of VS2019
as that is the latest supported VS toolchain on edk2.

  • 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

CI change.

Integration Instructions

N/A.

@os-d os-d force-pushed the vs2022_ci branch 4 times, most recently from a2ef798 to 14ab731 Compare December 3, 2024 22:54
@os-d os-d marked this pull request as ready for review December 3, 2024 22:59
@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

@os-d
Copy link
Contributor Author

os-d commented Dec 3, 2024

@mdkinney I originally had moved the three Windows-VS2019.yml files to a new file name, but the edk2 pipelines only will run on that filename and I do not have permission to create a test pipeline. So, to test that this change worked, I left the same Windows-VS2019.yml filename. However, it would be ideal to rename that to Windows-CI.yml or something so that future toolchain updates can simply update that file and we don't either have an outdated name or have to always create new filenames. Open to your suggestion here.

@kraxel
Copy link
Member

kraxel commented Dec 4, 2024

@mdkinney I originally had moved the three Windows-VS2019.yml files to a new file name, but the edk2 pipelines only will run on that filename and I do not have permission to create a test pipeline. So, to test that this change worked, I left the same Windows-VS2019.yml filename. However, it would be ideal to rename that to Windows-CI.yml or something so that future toolchain updates can simply update that file and we don't either have an outdated name or have to always create new filenames. Open to your suggestion here.

GCC CI has a simliar naming issue, it't called UBUNTU_GCC for historical reasons, the tests used to run in ubuntu VMs in the past, nowdays it is fedora containers though. So while being at it maybe also rename those to something more generic with 'Linux' instead of 'Ubuntu'.

@osteffenrh

@makubacki
Copy link
Member

However, it would be ideal to rename that to Windows-CI.yml or something so that future toolchain updates can simply update that file and we don't either have an outdated name or have to always create new filenames. Open to your suggestion here.

We moved our files to be more generic and I agree the same should be done here. The suggestions of "Windows" and "Linux" tend to work well for the variations usually modified over time within each.

@os-d
Copy link
Contributor Author

os-d commented Dec 4, 2024

However, it would be ideal to rename that to Windows-CI.yml or something so that future toolchain updates can simply update that file and we don't either have an outdated name or have to always create new filenames. Open to your suggestion here.

We moved our files to be more generic and I agree the same should be done here. The suggestions of "Windows" and "Linux" tend to work well for the variations usually modified over time within each.

@mdkinney , if you are amenable to it, perhaps the path forward here is to merge this PR and then do a separate PR that just changes the filenames of the YAML files without content changes (or adds copies, let's you switch over the pipelines, then delete the old YAMLs).

Copy link

mergify bot commented Dec 9, 2024

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

@bexcran
Copy link
Contributor

bexcran commented Dec 9, 2024

@os-d Looks like conflicting changes have been added to CryptoPkg.dsc so you'll need to resolve them manually.

@os-d
Copy link
Contributor Author

os-d commented Dec 9, 2024

@mdkinney , did you want to review this? Otherwise it has the approvals it needs to merge from the CI side.

os-d added 3 commits December 10, 2024 14:46
VS2022's DLINK2_FLAGS (containing only /WHOLEARCHIVE) was commented
out during upstreaming, due to some downstream platform issues
when /WHOLEARCHIVE was set. This does not prove an issue for edk2
and is what is used for earlier versions of VS, so is added here
for VS2022.

If platforms see issues, bugs should be filed on edk2 (or fixed in
the platform if applicable).

Signed-off-by: Oliver Smith-Denny <[email protected]>
BaseTools has a limitation that modules in FVs that are force rebased
must have the same file and section alignment. This is intended for
XIP modules.

VS2019 and previous VS toolchains did not set 4k section alignment,
but VS2022 does, in order for memory protections to be applied to
images. This causes issues when building SEC and PEI modules on
VS2022 as the file alignment is 0x20 but the section alignment
is 0x1000, so BaseTools will fail to generate the FV. One option
is to set the file alignment to 0x1000 for all of these files, but
that is a large waste of space and is not feasible on some platforms
that have limited flash space. The other option is to selectively
set 0x20 as the section alignment for SEC and PEI modules, which is
the approach GCC ARM/AARCH64 took.

This is only an issue for building 64-bit PEI on x86 currently, as
other architectures are not supported by VS2022 in edk2 yet. For IA32,
the section alignment is set to 0x20 and so it matches the file
alignment, however x64 PEI uses the X64 DLINK flags which have 0x1000
set. For other architectures that don't have the PEI/DXE architecture
split, this is also an issue.

This commit is required to use VS2022 as the default CI in edk2, as
OvmfPkgX64.dsc will fail to build. Any platform with 64-bit PEI also
requires this.

This commit also updates CryptoPkg.dsc and SecurityPkg.dsc as they
are setting custom section alignments.

Continuous-integration-options: PatchCheck.ignore-multi-package

Signed-off-by: Oliver Smith-Denny <[email protected]>
This PR updates the CI pipelines to use VS2022 instead of VS2019
as that is the latest supported VS toolchain on edk2.

Continuous-integration-options: PatchCheck.ignore-multi-package

Signed-off-by: Oliver Smith-Denny <[email protected]>
@os-d
Copy link
Contributor Author

os-d commented Dec 10, 2024

@mdkinney , I have added a commit adding /WHOLEARCHIVE. Can you please re-review?

@mdkinney mdkinney added the push Auto push patch series in PR if all checks pass label Dec 10, 2024
@mergify mergify bot merged commit 1c5c951 into tianocore:master Dec 10, 2024
126 checks passed
@os-d os-d deleted the vs2022_ci branch December 10, 2024 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
push Auto push patch series in PR if all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants