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

[Sean Christopherson <seanjc@google.com>]

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;
        }