Skip to content

Conversation

smalis-msft
Copy link
Contributor

@smalis-msft smalis-msft commented Oct 10, 2025

This fixes a regression with newer TDX firmware modules.

Closes #2131

@Copilot Copilot AI review requested due to automatic review settings October 10, 2025 18:18
@smalis-msft smalis-msft requested a review from a team as a code owner October 10, 2025 18:18
@smalis-msft smalis-msft added the backport_2505 Change should be backported to the release/2505 branch label Oct 10, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a regression with newer TDX firmware modules by ensuring that specific CPU features (MCE, MCA, and MTRR) are always reported as supported to guests, regardless of what the underlying hardware reports.

  • Always sets MCE, MCA, and MTRR feature bits in CPUID responses for TDX guests
  • Adds fixup logic for both VersionAndFeatures and ExtendedVersionAndFeatures CPUID functions

Copy link
Member

@chris-oo chris-oo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but let's make sure we test it before we merge.

@chris-oo chris-oo changed the title TDX: Always report that MCE,MCA,MTRR CPUID bits are set, regardless of what the hardware says openhcl/virt_mshv_vtl: On TDX, always report that MCE,MCA,MTRR CPUID bits are set regardless of what the hardware says Oct 10, 2025
@chris-oo
Copy link
Member

One thing Ben raised was questions around MSR handling. I think we already emulated the associated MSRs in usermode (IE the hardware didn't do anything), but can we confirm? Wondering if we need to try some guest accesses?

@smalis-msft
Copy link
Contributor Author

What MSRs would that be?

@chris-oo
Copy link
Member

Ben thinks that booting windows should exercise MTRRs and MCA/MCE, lets double check with Alex & Ben.

@smalis-msft smalis-msft added the release-ci-required Add to a PR to trigger PR gates in release mode label Oct 10, 2025
@benpye
Copy link

benpye commented Oct 10, 2025

The TDX spec just states "applicable MSRs" -

For MTRR I expect they would be MTRR_CAP, MTRR_PHYSMASK* and MTRR_FIX*

The machine check registers are MC*_CTL/STATUS/ADDR/MISC

Copy link

@vibhutet
Copy link
Contributor

The kernel side change will be needed. In case of the availability and opt-in of feature (TDCS.REDUCE_VE = 1), setting of the TDCS.FEATURE_PARAVIRT_CTLS bits will in turn dictate the tdx module behavior when guest accesses the MTRRs and MCA/MCE registers.
Ref: TDX ABI spec: 4.1.3.2 TDCS.FEATURE_PARAVIRT_CTRL
The table in the spec provides more details on the various conditions.
Linux kernel changes: microsoft/OHCL-Linux-Kernel#105

@chris-oo
Copy link
Member

Could you clarify - does this mean the TDX module won't even exit to the L1 if the L2 accesses these MSRs when the CTLS bit is 0-ed?

@smalis-msft
Copy link
Contributor Author

I have successfully booted a TDX Windows VM with this patch and the new TDX module that did not boot without this patch.

@smalis-msft smalis-msft enabled auto-merge (squash) October 10, 2025 22:46
@smmalis37
Copy link
Contributor

According to wikipedia the duplicated bits in ExtendedInfo here are only present on AMD cpus, not Intel. We may want to remove them?

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

Labels

backport_2505 Change should be backported to the release/2505 branch release-ci-required Add to a PR to trigger PR gates in release mode

Projects

None yet

Development

Successfully merging this pull request may close these issues.

openhcl/tdx: Add support for MCA, MCE, MTRR via tdx/Feature_paravirt_ctls bitmap

5 participants