Re: [PATCH] KVM: TDX: Fix APIC MSR ranges in tdx_has_emulated_msr()

Sean Christopherson
2026-04-03T16:30:48Z

On Thu, Mar 19, 2026, Rick P Edgecombe wrote:
> On Thu, 2026-03-19 at 15:40 +0800, Binbin Wu wrote:
> > tdx_has_emulated_msr() is used by KVM to decide whether to emulate a MSR access from the
> > TDVMCALL or just return the error code.
> >
> > During an off-list discussion, Rick noted that #VE reduction could change the behavior of
> > accessing an MSR (e.g., from #VE to #GP or to be virtualized by the TDX module) without
> > KVM knowing.Because KVM lacks the full context to perfectly decide if an MSR should be
> > emulated, the question was raised: Can we just delete tdx_has_emulated_msr() entirely?
> >
> > However, these native type x2apic MSRs are a special case. Since the TDX module owns the
> > APICv page, KVM cannot emulate these MSRs. If we remove tdx_has_emulated_msr(), a guest
> > directly issuing TDVMCALLs for these native type x2apic MSRs will trigger a silent failure,
> > even though this is the guest's fault.
> >
> > It comes down to a tradeoff. Should we prioritize code simplicity by dropping the function,
> > or keep it to explicitly catch this misbehaving guest corner case?
>
> I think from KVM's perspective it doesn't want to help the guest behave
> correctly.

Uh, yes KVM does does. KVM is responsible for emulating the APIC timer, isn't it?

> So we can ignore that I think. But it does really care to not define
> any specific guest ABI that it has to maintain. So tdx_has_emulated_msr() has
> some value there. And even more, it wants to not allow the guest to hurt the
> host.
>
> On the latter point, another problem with deleting tdx_has_emulated_msr() is the
> current code path skips the checks done in the other MSR paths. So we would need
> to call some appropriate higher up MSR helper to protect the host? And that
> wades into the CPUID bit consistency issues.
>
> So maybe... could we do a more limited version of the deletion where we allow
> all the APIC MSRs through? We'd have to check that it won't cause problems.

What? No. KVM can't get actually read/write most (all?) MSRs, allowing access
is far worse than returning an error, as for all intents and purposes KVM will
silently drop writes, and return garbage on reads.

> Failing that, we should maybe just explicitly list the ones TDX supports rather
> than the current way we define the APIC ones. As you mention below, it's not
> correct in other ways too so it could be more robust.

No? Don't we just want to allow access to MSRs that aren't accelerated? What
the TDX-Module supports is largely irrelevant, I think.

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 1e47c194af53..28e87630870b 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -2116,23 +2116,26 @@ bool tdx_has_emulated_msr(u32 index)
case MSR_IA32_MC0_CTL2 ... MSR_IA32_MCx_CTL2(KVM_MAX_MCE_BANKS) - 1:
/* MSR_IA32_MCx_{CTL, STATUS, ADDR, MISC, CTL2} */
case MSR_KVM_POLL_CONTROL:
+ /*
+ * x2APIC registers that are virtualized by the CPU can't be
+ * emulated, KVM doesn't have access to the virtual APIC page.
+ */
+ case X2APIC_MSR(APIC_ID):
+ case X2APIC_MSR(APIC_LVR):
+ case X2APIC_MSR(APIC_LDR):
+ case X2APIC_MSR(APIC_SPIV):
+ case X2APIC_MSR(APIC_ESR):
+ case X2APIC_MSR(APIC_ICR):
+ case X2APIC_MSR(APIC_LVTT):
+ case X2APIC_MSR(APIC_LVTTHMR):
+ case X2APIC_MSR(APIC_LVTPC):
+ case X2APIC_MSR(APIC_LVT0):
+ case X2APIC_MSR(APIC_LVT1):
+ case X2APIC_MSR(APIC_LVTERR):
+ case X2APIC_MSR(APIC_TMICT):
+ case X2APIC_MSR(APIC_TMCCT):
+ case X2APIC_MSR(APIC_TDCR):
return true;
- case APIC_BASE_MSR ... APIC_BASE_MSR + 0xff:
- /*
- * x2APIC registers that are virtualized by the CPU can't be
- * emulated, KVM doesn't have access to the virtual APIC page.
- */
- switch (index) {
- case X2APIC_MSR(APIC_TASKPRI):
- case X2APIC_MSR(APIC_PROCPRI):
- case X2APIC_MSR(APIC_EOI):
- case X2APIC_MSR(APIC_ISR) ... X2APIC_MSR(APIC_ISR + APIC_ISR_NR):
- case X2APIC_MSR(APIC_TMR) ... X2APIC_MSR(APIC_TMR + APIC_ISR_NR):
- case X2APIC_MSR(APIC_IRR) ... X2APIC_MSR(APIC_IRR + APIC_ISR_NR):
- return false;
- default:
- return true;
- }
default:
return false;
}