Re: [PATCH] KVM: VMX: Explicitly track TDX VMs' root level instead of guessing it from CPUID

[Sean Christopherson <seanjc@google.com>]

On Wed, Aug 19, 2026, Rick P Edgecombe wrote:
> On Wed, 2026-08-19 at 08:56 -0700, Sean Christopherson wrote:
> > FWIW, I don't view keying off gfn_direct_bits as being simpler.  It might be
> > less code, but conceptually it's more complex when reading
> > kvm_mmu_get_tdp_level(). E.g. the comment would need to explain the connection
> > between "direct bits" and the mirror root level, which most non-TDX readers
> > simply won't care about.
> > 
> > Hmm, but the comment I provided isn't very good either, as it too bleeds in
> > details about the S-bit pivot, and at the end of the day that's not the true
> > reason why the mirror root has/needs a predefined level.  The true reason is
> > very simple: KVM needs to mirror the external page tables, and obviously that
> > means using the same number of levels.
> > 
> > 	/*
> > 	 * If the VM has mirror roots, then the root level is predefined as
> > the
> > 	 * mirror root (and by extension the normal root) needs to match the
> > 	 * root level that was configured for the external page tables that
> > are
> > 	 * being mirrored by KVM.
> > 	 */
> > 	if (vcpu->kvm->arch.mirror_root_level)
> > 		return vcpu->kvm->arch.mirror_root_level;
> > 
> > But IMO that's a moot point, because this isn't a matter of simple vs.
> > complex. Keying of gfn_direct_bits is wrong/flawed, so whether or not it's
> > simpler is irrelevant.
> 
> I was just thinking that the patch was kind of doing two things with one change.

Oh, yeah, it kinda is.  More at the bottom.

> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index c519e8e8d646..c78897510a1e 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -5928,19 +5928,21 @@ void __kvm_mmu_refresh_passthrough_bits(struct
> > kvm_vcpu *vcpu,
> >  
> >  static inline int kvm_mmu_get_tdp_level(struct kvm_vcpu *vcpu)
> >  {
> > -	int maxpa;
> > -
> > -	if (vcpu->kvm->arch.vm_type == KVM_X86_TDX_VM)
> > -		maxpa = cpuid_query_maxguestphyaddr(vcpu);
> > -	else
> > -		maxpa = cpuid_maxphyaddr(vcpu);
> > -
> >  	/* tdp_root_level is architecture forced level, use it if nonzero */
> >  	if (tdp_root_level)
> >  		return tdp_root_level;
> >  
> > +	/*
> > +	 * If the VM has mirror roots, then the root level is predefined as
> > the
> > +	 * mirror root (and by extension the normal root) needs to match the
> > +	 * root level that was configured for the external page tables that
> > are
> > +	 * being mirrored by KVM.
> > +	 */
> > +	if (vcpu->kvm->arch.mirror_root_level)
> 
> Elsewhere we use kvm_has_mirrored_tdp(vcpu->kvm) for these kind of checks. Would
> be nice to be consistent and not add any uncertainty of whether
> mirror_root_level can be set without kvm_has_mirrored_tdp() being true.

Hmm, for defense in depth, I want to explicitly check mirror_root_level, because
returning '0' would likely have dire consequences.  How about this?

	if (kvm_has_mirrored_tdp(vcpu->kvm) &&
	    !WARN_ON_ONCE(!vcpu->kvm->arch.mirror_root_level))
		return vcpu->kvm->arch.mirror_root_level;

> > @@ -2760,6 +2754,14 @@ DEFINE_CLASS(tdx_vm_state_guard, tdx_vm_state_guard_t,
> >  	     if (!IS_ERR(_T)) tdx_release_vm_state_locks(_T),
> >  	     tdx_acquire_vm_state_locks(kvm), struct kvm *kvm);
> >  
> > +static __always_inline void tdx_set_mirror_root_level(struct kvm *kvm, int
> > level)
> > +{
> > +	BUILD_BUG_ON(level != 4 && level != 5);
> > +
> > +	kvm->arch.mirror_root_level = level;
> > +	kvm->arch.gfn_direct_bits = gpa_to_gfn(BIT_ULL(level == 4 ? 47 :
> > 51));
> 
> No need to remove TDX_SHARED_BIT_PWL_4/5 in this patch either anymore. Since
> this lives in TDX code.

Killing them off dedups the code, and more importantly makes it all but impossible
for mirror_root_level and the mirror root level to get out of sync.  E.g. with this

	if (td_params->config_flags & TDX_CONFIG_FLAGS_MAX_GPAW) {
		kvm->arch.gfn_direct_bits = TDX_SHARED_BIT_PWL_5;
		tdx_set_mirror_root_level(kvm, 5);
	} else {
		kvm->arch.gfn_direct_bits = TDX_SHARED_BIT_PWL_4;
		tdx_set_mirror_root_level(kvm, 4);
	}

then it's possible we could fat-finger a change and end up with:


	if (td_params->config_flags & TDX_CONFIG_FLAGS_MAX_GPAW) {
		kvm->arch.gfn_direct_bits = TDX_SHARED_BIT_PWL_5;
		tdx_set_mirror_root_level(kvm, 4);
	} else {
		kvm->arch.gfn_direct_bits = TDX_SHARED_BIT_PWL_4;
		tdx_set_mirror_root_level(kvm, 5);
	}

But, as you note above, that can be a separate patch.