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

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

[Dmytro Maluka]

Note: compile-tested only. Bug found by code inspection.

X2APIC_MSR(APIC_xxx + APIC_ISR_NR) is incorrect, since APIC_ISR_NR is
0x8, not 0x80, so shifting it in X2APIC_MSR() results in losing those
lower bits, making it simply equal to X2APIC_MSR(APIC_xxx), i.e. making
the entire range consist of APIC_xxx only. So adding APIC_ISR_NR needs
to be outside X2APIC_MSR().

Additionally, since "..." ranges are inclusive, need to subtract 1.

Fixes: dd50294f3e3c ("KVM: TDX: Implement callbacks for MSR operations")
Signed-off-by: Dmytro Maluka <dmaluka@chromium.org>
---
 arch/x86/kvm/vmx/tdx.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index c5065f84b78b..466a7de660c2 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -2136,9 +2136,9 @@ bool tdx_has_emulated_msr(u32 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):
+		case X2APIC_MSR(APIC_ISR) ... X2APIC_MSR(APIC_ISR) + APIC_ISR_NR - 1:
+		case X2APIC_MSR(APIC_TMR) ... X2APIC_MSR(APIC_TMR) + APIC_ISR_NR - 1:
+		case X2APIC_MSR(APIC_IRR) ... X2APIC_MSR(APIC_IRR) + APIC_ISR_NR - 1:
 			return false;
 		default:
 			return true;
-- 
2.53.0.851.ga537e3e6e9-goog

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

[Dave Hansen]

On 3/18/26 12:01, Dmytro Maluka wrote:
> +		case X2APIC_MSR(APIC_ISR) ... X2APIC_MSR(APIC_ISR) + APIC_ISR_NR - 1:
> +		case X2APIC_MSR(APIC_TMR) ... X2APIC_MSR(APIC_TMR) + APIC_ISR_NR - 1:
> +		case X2APIC_MSR(APIC_IRR) ... X2APIC_MSR(APIC_IRR) + APIC_ISR_NR - 1:

Thanks for the patch, Dmytro.

<sigh>

So this code never worked (at least for a big chunk of the ranges.
Isaku, could you please go try to figure out if there are tests for this
somewhere, and why this never bit us?

It might also be handy to have a:

#define X2APIC_LAST_MSR(r)	(X2APIC_MSR(x)+APIC_ISR_NR-1)

so that the resulting code is a bit more readable:

	case X2APIC_MSR(APIC_IRR) ... X2APIC_LAST_MSR(APIC_IRR):

Dmytro, if you feel a burning need to respin this, don't let me stop
you. I can probably just fix this up when it gets applied, or Isaku can
make those changes and resend it too.

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

[Dmytro Maluka]

On Wed, Mar 18, 2026 at 12:42:59PM -0700, Dave Hansen wrote:
> It might also be handy to have a:
> 
> #define X2APIC_LAST_MSR(r)	(X2APIC_MSR(x)+APIC_ISR_NR-1)
> 
> so that the resulting code is a bit more readable:
> 
> 	case X2APIC_MSR(APIC_IRR) ... X2APIC_LAST_MSR(APIC_IRR):
> 
> Dmytro, if you feel a burning need to respin this, don't let me stop
> you. I can probably just fix this up when it gets applied, or Isaku can
> make those changes and resend it too.

Sure, please feel free to take over this. I even have no way to test it
anyway. :)

I'm hesitating whether X2APIC_LAST_MSR would be the best name for it,
given that it is for ISR/IRR/TMR only and is using APIC_ISR_NR (so
maybe, don't know, X2APIC_LAST_ISR_MSR?).

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

[Binbin Wu]

On 3/19/2026 3:42 AM, Dave Hansen wrote:
> On 3/18/26 12:01, Dmytro Maluka wrote:
>> +		case X2APIC_MSR(APIC_ISR) ... X2APIC_MSR(APIC_ISR) + APIC_ISR_NR - 1:
>> +		case X2APIC_MSR(APIC_TMR) ... X2APIC_MSR(APIC_TMR) + APIC_ISR_NR - 1:
>> +		case X2APIC_MSR(APIC_IRR) ... X2APIC_MSR(APIC_IRR) + APIC_ISR_NR - 1:
> 
> Thanks for the patch, Dmytro.
> 
> <sigh>
> 
> So this code never worked (at least for a big chunk of the ranges.
> Isaku, could you please go try to figure out if there are tests for this
> somewhere, and why this never bit us?

The bug doesn't cause problems for TDs because:
- These x2apic MSRs (TASKPRI, PROCPRI, EOI, ISRx, TMRx, IRRx) are virtualized by CPU,
  when a TD accesses these MSRs, it doesn't cause #VE, thus no TDVMCALL from the TD to
  request the emulation of these MSRs.
- The bug make the "false" range of APIC MSRs smaller, so it doesn't impact the result
  for the rest of the APIC MSRs.

The bug could be triggered if a TD issues a TDVMCALL directly to request the
read/write operations for these x2apic MSRs, but a sane TD will not do it. 

Currently, we don't have dedicated KVM selftests code to call TDVMCALL directly to request
the emulation for these x2apic MSRs.

> 
> It might also be handy to have a:
> 
> #define X2APIC_LAST_MSR(r)	(X2APIC_MSR(x)+APIC_ISR_NR-1)
> 
> so that the resulting code is a bit more readable:
> 
> 	case X2APIC_MSR(APIC_IRR) ... X2APIC_LAST_MSR(APIC_IRR):
> 
> Dmytro, if you feel a burning need to respin this, don't let me stop
> you. I can probably just fix this up when it gets applied, or Isaku can
> make those changes and resend it too.
> 

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

[Dave Hansen]

On 3/18/26 18:14, Binbin Wu wrote:
> The bug doesn't cause problems for TDs because:
> - These x2apic MSRs (TASKPRI, PROCPRI, EOI, ISRx, TMRx, IRRx) are virtualized by CPU,
>   when a TD accesses these MSRs, it doesn't cause #VE, thus no TDVMCALL from the TD to
>   request the emulation of these MSRs.
> - The bug make the "false" range of APIC MSRs smaller, so it doesn't impact the result
>   for the rest of the APIC MSRs.

Could we fix this up so that the code that's there is actually usable
and testable, please?

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

[Binbin Wu]

On 3/19/2026 9:48 AM, Dave Hansen wrote:
> On 3/18/26 18:14, Binbin Wu wrote:
>> The bug doesn't cause problems for TDs because:
>> - These x2apic MSRs (TASKPRI, PROCPRI, EOI, ISRx, TMRx, IRRx) are virtualized by CPU,
>>   when a TD accesses these MSRs, it doesn't cause #VE, thus no TDVMCALL from the TD to
>>   request the emulation of these MSRs.
>> - The bug make the "false" range of APIC MSRs smaller, so it doesn't impact the result
>>   for the rest of the APIC MSRs.
> 
> Could we fix this up so that the code that's there is actually usable
> and testable, please?
> 

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?


BTW, besides the bug described by this patch, according to the latest published TDX module
ABI table, MSR IA32_X2APIC_SELF_IPI is native type, but not included in the list.
There are some MSRs, which are reserved for xAPIC MSR, not included in the list, but they
can be covered by the KVM common code.

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

[Edgecombe, Rick P]

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. 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.

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.

> 
> 
> BTW, besides the bug described by this patch, according to the latest published TDX module
> ABI table, MSR IA32_X2APIC_SELF_IPI is native type, but not included in the list.
> There are some MSRs, which are reserved for xAPIC MSR, not included in the list, but they
> can be covered by the KVM common code.