Linux Confidential Computing Development
 help / color / mirror / Atom feed
* [PATCH] KVM: VMX: Explicitly track TDX VMs' root level instead of guessing it from CPUID
@ 2026-08-14 22:45 Sean Christopherson
  2026-08-19  0:53 ` Edgecombe, Rick P
  0 siblings, 1 reply; 9+ messages in thread
From: Sean Christopherson @ 2026-08-14 22:45 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini, Kiryl Shutsemau,
	Rick Edgecombe
  Cc: Dave Hansen, kvm, x86, linux-coco, linux-kernel, Xiaoyao Li,
	Binbin Wu, Kai Huang, Yan Zhao

Explicitly track the root level for TDX VMs instead of trying to infer the
depth of the paging tree based on an individual vCPU's CPUID information.
Applying KVM's existing logic to select the root level to TDX is flawed as
nothing *requires* userspace to fill in the correct guest.MAXPHYADDR for a
vCPU's CPUID.  Guessing at the correct root level is also ridiculous given
that userspace has already told KVM the root level during TD initialization.

Relying on userspace to set the expected/correct CPUID lets a misbehaving
userspace trip the KVM_BUG_ON() in tdx_load_mmu_pgd() by configuring guest
CPUID to use an "incorrect" guest.MAXPHYADDR.

Keep gfn_direct_bits even though it can be trivially derived from
mirror_root_level as saving a whole eight bytes per VM is meaningless, and
the value is queried fairly often and in hot paths.

Cc: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: Xiaoyao Li <xiaoyao.li@intel.com>
Cc: Binbin Wu <binbin.wu@linux.intel.com>
Cc: Kai Huang <kai.huang@intel.com>
Cc: Yan Zhao <yan.y.zhao@intel.com>
Fixes: 20d913729c11 ("KVM: x86/mmu: Taking guest pa into consideration when calculate tdp level")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---

Compile-tested only, and found by inspection, i.e. I haven't proven that this
works, or that there's actually a bug.  But I'm pretty sure there's a bug.

Came across this when looking at our out-of-tree intrahost migration code, and
wanted to assert that could be at most one mirror root per TDX VM.

 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/cpuid.c            | 14 --------------
 arch/x86/kvm/cpuid.h            |  1 -
 arch/x86/kvm/mmu.h              |  9 +++++++++
 arch/x86/kvm/mmu/mmu.c          | 16 ++++++++--------
 arch/x86/kvm/vmx/tdx.c          | 12 +++---------
 6 files changed, 21 insertions(+), 32 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 283847619ff8..b2a74c69cc4a 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1406,6 +1406,7 @@ struct kvm_arch {
 	struct kvm_mmu_memory_cache split_desc_cache;
 
 	gfn_t gfn_direct_bits;
+	int mirror_root_level;
 
 	/*
 	 * Size of the CPU's dirty log buffer, i.e. VMX's PML buffer. A Zero
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index ddb022cb203a..34c609a60eef 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -483,20 +483,6 @@ int cpuid_query_maxphyaddr(struct kvm_vcpu *vcpu)
 	return 36;
 }
 
-int cpuid_query_maxguestphyaddr(struct kvm_vcpu *vcpu)
-{
-	struct kvm_cpuid_entry2 *best;
-
-	best = kvm_find_cpuid_entry(vcpu, 0x80000000);
-	if (!best || best->eax < 0x80000008)
-		goto not_found;
-	best = kvm_find_cpuid_entry(vcpu, 0x80000008);
-	if (best)
-		return (best->eax >> 16) & 0xff;
-not_found:
-	return 0;
-}
-
 /*
  * This "raw" version returns the reserved GPA bits without any adjustments for
  * encryption technologies that usurp bits.  The raw mask should be used if and
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index 8d863f45585d..46bfe8699e67 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -68,7 +68,6 @@ void __init kvm_init_xstate_sizes(void);
 u32 xstate_required_size(u64 xstate_bv, bool compacted);
 
 int cpuid_query_maxphyaddr(struct kvm_vcpu *vcpu);
-int cpuid_query_maxguestphyaddr(struct kvm_vcpu *vcpu);
 u64 kvm_vcpu_reserved_gpa_bits_raw(struct kvm_vcpu *vcpu);
 
 static inline int cpuid_maxphyaddr(struct kvm_vcpu *vcpu)
diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index 2ae7f9ed4cf8..cc37e210787c 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -389,6 +389,15 @@ static inline gpa_t kvm_translate_gpa(struct kvm_vcpu *vcpu,
 						     exception, pte_access);
 }
 
+static __always_inline void kvm_mmu_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));
+}
+
 static inline bool kvm_has_mirrored_tdp(const struct kvm *kvm)
 {
 	return kvm->arch.vm_type == KVM_X86_TDX_VM;
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index c519e8e8d646..a51a852c1bec 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -5928,19 +5928,19 @@ 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 fixed as the gfn
+	 * used to select between the normal and mirror root must be covered.
+	 */
+	if (vcpu->kvm->arch.mirror_root_level)
+		return vcpu->kvm->arch.mirror_root_level;
+
 	/* Use 5-level TDP if and only if it's useful/necessary. */
-	if (max_tdp_level == 5 && maxpa <= 48)
+	if (max_tdp_level == 5 && cpuid_maxphyaddr(vcpu) <= 48)
 		return 4;
 
 	return max_tdp_level;
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index b272c20586a7..6d8e9befd9e6 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -56,9 +56,6 @@
 bool enable_tdx __ro_after_init;
 module_param_named(tdx, enable_tdx, bool, 0444);
 
-#define TDX_SHARED_BIT_PWL_5 gpa_to_gfn(BIT_ULL(51))
-#define TDX_SHARED_BIT_PWL_4 gpa_to_gfn(BIT_ULL(47))
-
 static const struct tdx_sys_info *tdx_sysinfo;
 
 void tdh_vp_rd_failed(struct vcpu_tdx *tdx, char *uclass, u32 field, u64 err)
@@ -1609,10 +1606,7 @@ static int handle_tdvmcall(struct kvm_vcpu *vcpu)
 
 void tdx_load_mmu_pgd(struct kvm_vcpu *vcpu, hpa_t root_hpa, int pgd_level)
 {
-	u64 shared_bit = (pgd_level == 5) ? TDX_SHARED_BIT_PWL_5 :
-			  TDX_SHARED_BIT_PWL_4;
-
-	if (KVM_BUG_ON(shared_bit != kvm_gfn_direct_bits(vcpu->kvm), vcpu->kvm))
+	if (KVM_BUG_ON(pgd_level != vcpu->kvm->arch.mirror_root_level, vcpu->kvm))
 		return;
 
 	td_vmcs_write64(to_tdx(vcpu), SHARED_EPT_POINTER, root_hpa);
@@ -2823,9 +2817,9 @@ static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
 	kvm_tdx->xfam = td_params->xfam;
 
 	if (td_params->config_flags & TDX_CONFIG_FLAGS_MAX_GPAW)
-		kvm->arch.gfn_direct_bits = TDX_SHARED_BIT_PWL_5;
+		kvm_mmu_set_mirror_root_level(kvm, 5);
 	else
-		kvm->arch.gfn_direct_bits = TDX_SHARED_BIT_PWL_4;
+		kvm_mmu_set_mirror_root_level(kvm, 4);
 
 	kvm_tdx->state = TD_STATE_INITIALIZED;
 out:

base-commit: 1b731e5ded480bd1e5546aed35584238661ce72e
-- 
2.55.0.691.gc56d675ccc-goog


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: VMX: Explicitly track TDX VMs' root level instead of guessing it from CPUID
  2026-08-14 22:45 [PATCH] KVM: VMX: Explicitly track TDX VMs' root level instead of guessing it from CPUID Sean Christopherson
@ 2026-08-19  0:53 ` Edgecombe, Rick P
  2026-08-19  0:59   ` Sean Christopherson
  0 siblings, 1 reply; 9+ messages in thread
From: Edgecombe, Rick P @ 2026-08-19  0:53 UTC (permalink / raw)
  To: pbonzini@redhat.com, kas@kernel.org, seanjc@google.com
  Cc: dave.hansen@linux.intel.com, Huang, Kai,
	binbin.wu@linux.intel.com, Li, Xiaoyao,
	linux-kernel@vger.kernel.org, Zhao, Yan Y, x86@kernel.org,
	kvm@vger.kernel.org, linux-coco@lists.linux.dev

On Fri, 2026-08-14 at 15:45 -0700, Sean Christopherson wrote:
> Explicitly track the root level for TDX VMs instead of trying to infer the
> depth of the paging tree based on an individual vCPU's CPUID information.
> Applying KVM's existing logic to select the root level to TDX is flawed as
> nothing *requires* userspace to fill in the correct guest.MAXPHYADDR for a
> vCPU's CPUID.  Guessing at the correct root level is also ridiculous given
> that userspace has already told KVM the root level during TD initialization.
> 
> Relying on userspace to set the expected/correct CPUID lets a misbehaving
> userspace trip the KVM_BUG_ON() in tdx_load_mmu_pgd() by configuring guest
> CPUID to use an "incorrect" guest.MAXPHYADDR.

Hmm, yea. But to me the text is a little ambiguous what "configuring guest
CPUID" means. There are the two configurations of CPUID that happen and the goof
was due to forgetting that there is no enforcement between the first "directly
configurable bits" (where "userspace has already told KVM the root level during
TD initialization" happens), and the second that happens via normal SET_CPUID.

Doing a KVM_BUG_ON() if tdx code sees a different level than what was processed
in setup_tdparams_eptp_controls() seems good to me.

> 
> Keep gfn_direct_bits even though it can be trivially derived from
> mirror_root_level as saving a whole eight bytes per VM is meaningless, and
> the value is queried fairly often and in hot paths.

gfn_direct_bits comes directly from the the initial configuration, so why do we
need to add mirror_root_level in this patch? The old code calculated shared bit
with a conditional, so we could easily compute level with an inverted
conditional. The mirror_root_level caching is then a separate
cleanup/enhancments.

Ohhh, because to calculate it kvm_mmu_get_tdp_level() would embed some TDX
specifics there.

Wait, no, this knowledge embeds in kvm_mmu_set_mirror_root_level() anyway. So
I'd think to just have the below. If comparing gfn_direct_bits to
gfn_direct_bits doesn't make sense, then let's just drop the KVM_BUG_ON().


 arch/x86/kvm/cpuid.c   | 14 --------------
 arch/x86/kvm/cpuid.h   |  1 -
 arch/x86/kvm/mmu.h     |  5 +++++
 arch/x86/kvm/mmu/mmu.c | 16 ++++++++--------
 4 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index ddb022cb203a..34c609a60eef 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -483,20 +483,6 @@ int cpuid_query_maxphyaddr(struct kvm_vcpu *vcpu)
        return 36;
 }
 
-int cpuid_query_maxguestphyaddr(struct kvm_vcpu *vcpu)
-{
-       struct kvm_cpuid_entry2 *best;
-
-       best = kvm_find_cpuid_entry(vcpu, 0x80000000);
-       if (!best || best->eax < 0x80000008)
-               goto not_found;
-       best = kvm_find_cpuid_entry(vcpu, 0x80000008);
-       if (best)
-               return (best->eax >> 16) & 0xff;
-not_found:
-       return 0;
-}
-
 /*
  * This "raw" version returns the reserved GPA bits without any adjustments for
  * encryption technologies that usurp bits.  The raw mask should be used if and
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index 8d863f45585d..46bfe8699e67 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -68,7 +68,6 @@ void __init kvm_init_xstate_sizes(void);
 u32 xstate_required_size(u64 xstate_bv, bool compacted);
 
 int cpuid_query_maxphyaddr(struct kvm_vcpu *vcpu);
-int cpuid_query_maxguestphyaddr(struct kvm_vcpu *vcpu);
 u64 kvm_vcpu_reserved_gpa_bits_raw(struct kvm_vcpu *vcpu);
 
 static inline int cpuid_maxphyaddr(struct kvm_vcpu *vcpu)
diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index 2ae7f9ed4cf8..e950f656007e 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -389,6 +389,11 @@ static inline gpa_t kvm_translate_gpa(struct kvm_vcpu
*vcpu,
                                                     exception, pte_access);
 }
 
+static inline unsigned int kvm_mmu_get_mirror_root_level(struct kvm *kvm)
+{
+       return kvm->arch.gfn_direct_bits == BIT_ULL(47) ? 4 : 5;
+}
+
 static inline bool kvm_has_mirrored_tdp(const struct kvm *kvm)
 {
        return kvm->arch.vm_type == KVM_X86_TDX_VM;
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 064ecc33b926..025871403597 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -5953,19 +5953,19 @@ 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 fixed as the gfn
+        * used to select between the normal and mirror root must be covered.
+        */
+       if (vcpu->kvm->arch.gfn_direct_bits)
+               return kvm_mmu_get_mirror_root_level(vcpu->kvm);
+
        /* Use 5-level TDP if and only if it's useful/necessary. */
-       if (max_tdp_level == 5 && maxpa <= 48)
+       if (max_tdp_level == 5 && cpuid_maxphyaddr(vcpu) <= 48)
                return 4;
 
        return max_tdp_level;


> 
> Cc: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Cc: Xiaoyao Li <xiaoyao.li@intel.com>
> Cc: Binbin Wu <binbin.wu@linux.intel.com>
> Cc: Kai Huang <kai.huang@intel.com>
> Cc: Yan Zhao <yan.y.zhao@intel.com>
> Fixes: 20d913729c11 ("KVM: x86/mmu: Taking guest pa into consideration when
> calculate tdp level")
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> 
> Compile-tested only, and found by inspection, i.e. I haven't proven that this
> works, or that there's actually a bug.  But I'm pretty sure there's a bug.
> 
> Came across this when looking at our out-of-tree intrahost migration code, and
> wanted to assert that could be at most one mirror root per TDX VM.

I regression tested it in our CI. I don't have a TDX setup the past two days to
try to reproduce it specifically. By inspection it also looks like a bug to me.


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: VMX: Explicitly track TDX VMs' root level instead of guessing it from CPUID
  2026-08-19  0:53 ` Edgecombe, Rick P
@ 2026-08-19  0:59   ` Sean Christopherson
  2026-08-19 14:35     ` Edgecombe, Rick P
  0 siblings, 1 reply; 9+ messages in thread
From: Sean Christopherson @ 2026-08-19  0:59 UTC (permalink / raw)
  To: Rick P Edgecombe
  Cc: pbonzini@redhat.com, kas@kernel.org, dave.hansen@linux.intel.com,
	Kai Huang, binbin.wu@linux.intel.com, Xiaoyao Li,
	linux-kernel@vger.kernel.org, Yan Y Zhao, x86@kernel.org,
	kvm@vger.kernel.org, linux-coco@lists.linux.dev

On Wed, Aug 19, 2026, Rick P Edgecombe wrote:
> On Fri, 2026-08-14 at 15:45 -0700, Sean Christopherson wrote:
> > Explicitly track the root level for TDX VMs instead of trying to infer the
> > depth of the paging tree based on an individual vCPU's CPUID information.
> > Applying KVM's existing logic to select the root level to TDX is flawed as
> > nothing *requires* userspace to fill in the correct guest.MAXPHYADDR for a
> > vCPU's CPUID.  Guessing at the correct root level is also ridiculous given
> > that userspace has already told KVM the root level during TD initialization.
> > 
> > Relying on userspace to set the expected/correct CPUID lets a misbehaving
> > userspace trip the KVM_BUG_ON() in tdx_load_mmu_pgd() by configuring guest
> > CPUID to use an "incorrect" guest.MAXPHYADDR.
> 
> Hmm, yea. But to me the text is a little ambiguous what "configuring guest
> CPUID" means. There are the two configurations of CPUID that happen and the goof
> was due to forgetting that there is no enforcement between the first "directly
> configurable bits" (where "userspace has already told KVM the root level during
> TD initialization" happens), and the second that happens via normal SET_CPUID.
> 
> Doing a KVM_BUG_ON() if tdx code sees a different level than what was processed
> in setup_tdparams_eptp_controls() seems good to me.
> 
> > 
> > Keep gfn_direct_bits even though it can be trivially derived from
> > mirror_root_level as saving a whole eight bytes per VM is meaningless, and
> > the value is queried fairly often and in hot paths.
> 
> gfn_direct_bits comes directly from the the initial configuration, so why do we
> need to add mirror_root_level in this patch? The old code calculated shared bit
> with a conditional, so we could easily compute level with an inverted
> conditional. The mirror_root_level caching is then a separate
> cleanup/enhancments.
> 
> Ohhh, because to calculate it kvm_mmu_get_tdp_level() would embed some TDX
> specifics there.
> 
> Wait, no, this knowledge embeds in kvm_mmu_set_mirror_root_level() anyway. So
> I'd think to just have the below. 

I started with that, but I didn't like bleeding that level of detail into the
MMU.  Or rather, I didn't like baking in the assumption that there is exactly
one "direct bits", that the one bit is a pivot between normal and mirror root,
and that the pivot bit is the most significant bit of the effective GPA space.

On the other hand, the MMU already knows about mirror roots, and needs to know
that mirror roots can have predetermined levels, so explicitly storing that level
doesn't add new assumptions.

> If comparing gfn_direct_bits to gfn_direct_bits doesn't make sense, then
> let's just drop the KVM_BUG_ON().

Why?  Defense in depth is often useful.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: VMX: Explicitly track TDX VMs' root level instead of guessing it from CPUID
  2026-08-19  0:59   ` Sean Christopherson
@ 2026-08-19 14:35     ` Edgecombe, Rick P
  2026-08-19 15:56       ` Sean Christopherson
  0 siblings, 1 reply; 9+ messages in thread
From: Edgecombe, Rick P @ 2026-08-19 14:35 UTC (permalink / raw)
  To: seanjc@google.com
  Cc: dave.hansen@linux.intel.com, binbin.wu@linux.intel.com,
	kas@kernel.org, Li, Xiaoyao, linux-kernel@vger.kernel.org,
	Zhao, Yan Y, Huang, Kai, pbonzini@redhat.com, kvm@vger.kernel.org,
	linux-coco@lists.linux.dev, x86@kernel.org

On Tue, 2026-08-18 at 17:59 -0700, Sean Christopherson wrote:
> > Wait, no, this knowledge embeds in kvm_mmu_set_mirror_root_level() anyway.
> > So I'd think to just have the below. 
> 
> I started with that, but I didn't like bleeding that level of detail into the
> MMU.  Or rather, I didn't like baking in the assumption that there is exactly
> one "direct bits", that the one bit is a pivot between normal and mirror root,
> and that the pivot bit is the most significant bit of the effective GPA space.
> 
> On the other hand, the MMU already knows about mirror roots, and needs to know
> that mirror roots can have predetermined levels, so explicitly storing that
> level doesn't add new assumptions.

Yea.

And with the log as is, the exact reasoning to add a mirror_root_level is not
clear. To me at least. So if we want to keep it, some extra justification would
help. But for a bug fix, I'd think to keep it simple and then do
mirror_root_level as a separate change.

> 
> > If comparing gfn_direct_bits to gfn_direct_bits doesn't make sense, then
> > let's just drop the KVM_BUG_ON().
> 
> Why?  Defense in depth is often useful.

It's fair. And especially TDX code is tucked away enough away that leaning
towards more checks is probably good.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: VMX: Explicitly track TDX VMs' root level instead of guessing it from CPUID
  2026-08-19 14:35     ` Edgecombe, Rick P
@ 2026-08-19 15:56       ` Sean Christopherson
  2026-08-19 17:35         ` Edgecombe, Rick P
  0 siblings, 1 reply; 9+ messages in thread
From: Sean Christopherson @ 2026-08-19 15:56 UTC (permalink / raw)
  To: Rick P Edgecombe
  Cc: dave.hansen@linux.intel.com, binbin.wu@linux.intel.com,
	kas@kernel.org, Xiaoyao Li, linux-kernel@vger.kernel.org,
	Yan Y Zhao, Kai Huang, pbonzini@redhat.com, kvm@vger.kernel.org,
	linux-coco@lists.linux.dev, x86@kernel.org

On Wed, Aug 19, 2026, Rick P Edgecombe wrote:
> On Tue, 2026-08-18 at 17:59 -0700, Sean Christopherson wrote:
> > > Wait, no, this knowledge embeds in kvm_mmu_set_mirror_root_level() anyway.
> > > So I'd think to just have the below. 
> > 
> > I started with that, but I didn't like bleeding that level of detail into the
> > MMU.  Or rather, I didn't like baking in the assumption that there is exactly
> > one "direct bits", that the one bit is a pivot between normal and mirror root,
> > and that the pivot bit is the most significant bit of the effective GPA space.
> > 
> > On the other hand, the MMU already knows about mirror roots, and needs to know
> > that mirror roots can have predetermined levels, so explicitly storing that
> > level doesn't add new assumptions.
> 
> Yea.

Hmm, but an argument against this exact patch is that this code:

static __always_inline void kvm_mmu_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));
}

completely undermines the argument that exactly how TDX uses gfn_direct_bits is
an implementation detail that we don't want to bleed into the MMU.  That's easy
enough to address though, just move the helper into tdx.c.

> And with the log as is, the exact reasoning to add a mirror_root_level is not
> clear. To me at least. So if we want to keep it, some extra justification would
> help.

How about this (to replace the existing one that talks about gfn_direct_bits)?

    Don't use kvm_gfn_direct_bits() to infer the mirror root level, as the
    connection between TDX's one and only "direct" bit and the predetermined
    root level is a TDX implementation detail.  I.e. avoid baking in the
    assumption that there is exactly one "direct bits", that the one bit is a
    pivot between normal and mirror roots, and that the pivot bit is the most
    significant bit of the effective GPA space.  For the same reason, set the
    root level and direct bits in TDX code, i.e. don't provide a helper in the
    MMU, because from the MMU's perspective, they are two separate concepts.
    
    Keep gfn_direct_bits even though it can be trivially derived from
    mirror_root_level as saving a whole eight bytes per VM is meaningless,
    keeping the TDX details buring in TDX would require a kvm_x86_ops hook, and
    the value is queried fairly often and in hot paths.

> But for a bug fix, I'd think to keep it simple and then do mirror_root_level
> as a separate change.

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.

All in all, this?

From: Sean Christopherson <seanjc@google.com>
Date: Fri, 14 Aug 2026 07:50:32 -0700
Subject: [PATCH] KVM: VMX: Explicitly track TDX VMs' root level instead of
 guessing it from CPUID

Explicitly track the root level for TDX VMs instead of trying to infer the
depth of the paging tree based on an individual vCPU's CPUID information.
Applying KVM's existing logic to select the root level to TDX is flawed as
nothing *requires* userspace to fill in the correct guest.MAXPHYADDR for a
vCPU's CPUID.  Guessing at the correct root level is also ridiculous given
that userspace has already told KVM the root level during TD initialization.

Relying on userspace to set the expected/correct CPUID lets a misbehaving
userspace trip the KVM_BUG_ON() in tdx_load_mmu_pgd() by configuring guest
CPUID to use an "incorrect" guest.MAXPHYADDR.

Don't use kvm_gfn_direct_bits() to infer the mirror root level, as the
connection between TDX's one and only "direct" bit and the predetermined
root level is a TDX implementation detail.  I.e. avoid baking in the
assumption that there is exactly one "direct bits", that the one bit is a
pivot between normal and mirror roots, and that the pivot bit is the most
significant bit of the effective GPA space.  For the same reason, set the
root level and direct bits in TDX code, i.e. don't provide a helper in the
MMU, because from the MMU's perspective, they are two separate concepts.

Keep gfn_direct_bits even though it can be trivially derived from
mirror_root_level as saving a whole eight bytes per VM is meaningless,
keeping the TDX details buring in TDX would require a kvm_x86_ops hook, and
the value is queried fairly often and in hot paths.

Cc: Rick Edgecombe <rick.p.edgecombe@intel.com>
Cc: Xiaoyao Li <xiaoyao.li@intel.com>
Cc: Binbin Wu <binbin.wu@linux.intel.com>
Cc: Kai Huang <kai.huang@intel.com>
Cc: Yan Zhao <yan.y.zhao@intel.com>
Fixes: 20d913729c11 ("KVM: x86/mmu: Taking guest pa into consideration when calculate tdp level")
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/kvm_host.h |  1 +
 arch/x86/kvm/cpuid.c            | 14 --------------
 arch/x86/kvm/cpuid.h            |  1 -
 arch/x86/kvm/mmu/mmu.c          | 18 ++++++++++--------
 arch/x86/kvm/vmx/tdx.c          | 20 +++++++++++---------
 5 files changed, 22 insertions(+), 32 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 283847619ff8..b2a74c69cc4a 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1406,6 +1406,7 @@ struct kvm_arch {
 	struct kvm_mmu_memory_cache split_desc_cache;
 
 	gfn_t gfn_direct_bits;
+	int mirror_root_level;
 
 	/*
 	 * Size of the CPU's dirty log buffer, i.e. VMX's PML buffer. A Zero
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index ddb022cb203a..34c609a60eef 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -483,20 +483,6 @@ int cpuid_query_maxphyaddr(struct kvm_vcpu *vcpu)
 	return 36;
 }
 
-int cpuid_query_maxguestphyaddr(struct kvm_vcpu *vcpu)
-{
-	struct kvm_cpuid_entry2 *best;
-
-	best = kvm_find_cpuid_entry(vcpu, 0x80000000);
-	if (!best || best->eax < 0x80000008)
-		goto not_found;
-	best = kvm_find_cpuid_entry(vcpu, 0x80000008);
-	if (best)
-		return (best->eax >> 16) & 0xff;
-not_found:
-	return 0;
-}
-
 /*
  * This "raw" version returns the reserved GPA bits without any adjustments for
  * encryption technologies that usurp bits.  The raw mask should be used if and
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index 8d863f45585d..46bfe8699e67 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -68,7 +68,6 @@ void __init kvm_init_xstate_sizes(void);
 u32 xstate_required_size(u64 xstate_bv, bool compacted);
 
 int cpuid_query_maxphyaddr(struct kvm_vcpu *vcpu);
-int cpuid_query_maxguestphyaddr(struct kvm_vcpu *vcpu);
 u64 kvm_vcpu_reserved_gpa_bits_raw(struct kvm_vcpu *vcpu);
 
 static inline int cpuid_maxphyaddr(struct kvm_vcpu *vcpu)
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)
+		return vcpu->kvm->arch.mirror_root_level;
+
 	/* Use 5-level TDP if and only if it's useful/necessary. */
-	if (max_tdp_level == 5 && maxpa <= 48)
+	if (max_tdp_level == 5 && cpuid_maxphyaddr(vcpu) <= 48)
 		return 4;
 
 	return max_tdp_level;
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index b272c20586a7..a0bc9f818f43 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -56,9 +56,6 @@
 bool enable_tdx __ro_after_init;
 module_param_named(tdx, enable_tdx, bool, 0444);
 
-#define TDX_SHARED_BIT_PWL_5 gpa_to_gfn(BIT_ULL(51))
-#define TDX_SHARED_BIT_PWL_4 gpa_to_gfn(BIT_ULL(47))
-
 static const struct tdx_sys_info *tdx_sysinfo;
 
 void tdh_vp_rd_failed(struct vcpu_tdx *tdx, char *uclass, u32 field, u64 err)
@@ -1609,10 +1606,7 @@ static int handle_tdvmcall(struct kvm_vcpu *vcpu)
 
 void tdx_load_mmu_pgd(struct kvm_vcpu *vcpu, hpa_t root_hpa, int pgd_level)
 {
-	u64 shared_bit = (pgd_level == 5) ? TDX_SHARED_BIT_PWL_5 :
-			  TDX_SHARED_BIT_PWL_4;
-
-	if (KVM_BUG_ON(shared_bit != kvm_gfn_direct_bits(vcpu->kvm), vcpu->kvm))
+	if (KVM_BUG_ON(pgd_level != vcpu->kvm->arch.mirror_root_level, vcpu->kvm))
 		return;
 
 	td_vmcs_write64(to_tdx(vcpu), SHARED_EPT_POINTER, root_hpa);
@@ -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));
+}
+
 static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
 {
 	struct kvm_tdx_init_vm __user *user_data = u64_to_user_ptr(cmd->data);
@@ -2823,9 +2825,9 @@ static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
 	kvm_tdx->xfam = td_params->xfam;
 
 	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);
 
 	kvm_tdx->state = TD_STATE_INITIALIZED;
 out:

base-commit: 1b731e5ded480bd1e5546aed35584238661ce72e
--

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: VMX: Explicitly track TDX VMs' root level instead of guessing it from CPUID
  2026-08-19 15:56       ` Sean Christopherson
@ 2026-08-19 17:35         ` Edgecombe, Rick P
  2026-08-19 18:45           ` Sean Christopherson
  0 siblings, 1 reply; 9+ messages in thread
From: Edgecombe, Rick P @ 2026-08-19 17:35 UTC (permalink / raw)
  To: seanjc@google.com
  Cc: dave.hansen@linux.intel.com, kas@kernel.org,
	binbin.wu@linux.intel.com, Li, Xiaoyao,
	linux-kernel@vger.kernel.org, Zhao, Yan Y, Huang, Kai,
	pbonzini@redhat.com, kvm@vger.kernel.org,
	linux-coco@lists.linux.dev, x86@kernel.org

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.

> 
> All in all, this?
> 
> From: Sean Christopherson <seanjc@google.com>
> Date: Fri, 14 Aug 2026 07:50:32 -0700
> Subject: [PATCH] KVM: VMX: Explicitly track TDX VMs' root level instead of
>  guessing it from CPUID
> 
> Explicitly track the root level for TDX VMs instead of trying to infer the
> depth of the paging tree based on an individual vCPU's CPUID information.
> Applying KVM's existing logic to select the root level to TDX is flawed as
> nothing *requires* userspace to fill in the correct guest.MAXPHYADDR for a
> vCPU's CPUID.  Guessing at the correct root level is also ridiculous given
> that userspace has already told KVM the root level during TD initialization.
> 
> Relying on userspace to set the expected/correct CPUID lets a misbehaving
> userspace trip the KVM_BUG_ON() in tdx_load_mmu_pgd() by configuring guest
> CPUID to use an "incorrect" guest.MAXPHYADDR.
> 
> Don't use kvm_gfn_direct_bits() to infer the mirror root level, as the
> connection between TDX's one and only "direct" bit and the predetermined
> root level is a TDX implementation detail.  I.e. avoid baking in the
> assumption that there is exactly one "direct bits", that the one bit is a
> pivot between normal and mirror roots, and that the pivot bit is the most
> significant bit of the effective GPA space.  For the same reason, set the
> root level and direct bits in TDX code, i.e. don't provide a helper in the
> MMU, because from the MMU's perspective, they are two separate concepts.
> 
> Keep gfn_direct_bits even though it can be trivially derived from
> mirror_root_level as saving a whole eight bytes per VM is meaningless,
> keeping the TDX details buring in TDX would require a kvm_x86_ops hook, and
> the value is queried fairly often and in hot paths.
> 
> Cc: Rick Edgecombe <rick.p.edgecombe@intel.com>
> Cc: Xiaoyao Li <xiaoyao.li@intel.com>
> Cc: Binbin Wu <binbin.wu@linux.intel.com>
> Cc: Kai Huang <kai.huang@intel.com>
> Cc: Yan Zhao <yan.y.zhao@intel.com>
> Fixes: 20d913729c11 ("KVM: x86/mmu: Taking guest pa into consideration when
> calculate tdp level")
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  1 +
>  arch/x86/kvm/cpuid.c            | 14 --------------
>  arch/x86/kvm/cpuid.h            |  1 -
>  arch/x86/kvm/mmu/mmu.c          | 18 ++++++++++--------
>  arch/x86/kvm/vmx/tdx.c          | 20 +++++++++++---------
>  5 files changed, 22 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 283847619ff8..b2a74c69cc4a 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1406,6 +1406,7 @@ struct kvm_arch {
>  	struct kvm_mmu_memory_cache split_desc_cache;
>  
>  	gfn_t gfn_direct_bits;
> +	int mirror_root_level;
>  
>  	/*
>  	 * Size of the CPU's dirty log buffer, i.e. VMX's PML buffer. A Zero
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index ddb022cb203a..34c609a60eef 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -483,20 +483,6 @@ int cpuid_query_maxphyaddr(struct kvm_vcpu *vcpu)
>  	return 36;
>  }
>  
> -int cpuid_query_maxguestphyaddr(struct kvm_vcpu *vcpu)
> -{
> -	struct kvm_cpuid_entry2 *best;
> -
> -	best = kvm_find_cpuid_entry(vcpu, 0x80000000);
> -	if (!best || best->eax < 0x80000008)
> -		goto not_found;
> -	best = kvm_find_cpuid_entry(vcpu, 0x80000008);
> -	if (best)
> -		return (best->eax >> 16) & 0xff;
> -not_found:
> -	return 0;
> -}
> -
>  /*
>   * This "raw" version returns the reserved GPA bits without any adjustments
> for
>   * encryption technologies that usurp bits.  The raw mask should be used if
> and
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index 8d863f45585d..46bfe8699e67 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -68,7 +68,6 @@ void __init kvm_init_xstate_sizes(void);
>  u32 xstate_required_size(u64 xstate_bv, bool compacted);
>  
>  int cpuid_query_maxphyaddr(struct kvm_vcpu *vcpu);
> -int cpuid_query_maxguestphyaddr(struct kvm_vcpu *vcpu);
>  u64 kvm_vcpu_reserved_gpa_bits_raw(struct kvm_vcpu *vcpu);
>  
>  static inline int cpuid_maxphyaddr(struct kvm_vcpu *vcpu)
> 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.

> +		return vcpu->kvm->arch.mirror_root_level;
> +
>  	/* Use 5-level TDP if and only if it's useful/necessary. */
> -	if (max_tdp_level == 5 && maxpa <= 48)
> +	if (max_tdp_level == 5 && cpuid_maxphyaddr(vcpu) <= 48)
>  		return 4;
>  
>  	return max_tdp_level;
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index b272c20586a7..a0bc9f818f43 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -56,9 +56,6 @@
>  bool enable_tdx __ro_after_init;
>  module_param_named(tdx, enable_tdx, bool, 0444);
>  
> -#define TDX_SHARED_BIT_PWL_5 gpa_to_gfn(BIT_ULL(51))
> -#define TDX_SHARED_BIT_PWL_4 gpa_to_gfn(BIT_ULL(47))
> -
>  static const struct tdx_sys_info *tdx_sysinfo;
>  
>  void tdh_vp_rd_failed(struct vcpu_tdx *tdx, char *uclass, u32 field, u64 err)
> @@ -1609,10 +1606,7 @@ static int handle_tdvmcall(struct kvm_vcpu *vcpu)
>  
>  void tdx_load_mmu_pgd(struct kvm_vcpu *vcpu, hpa_t root_hpa, int pgd_level)
>  {
> -	u64 shared_bit = (pgd_level == 5) ? TDX_SHARED_BIT_PWL_5 :
> -			  TDX_SHARED_BIT_PWL_4;
> -
> -	if (KVM_BUG_ON(shared_bit != kvm_gfn_direct_bits(vcpu->kvm), vcpu-
> >kvm))
> +	if (KVM_BUG_ON(pgd_level != vcpu->kvm->arch.mirror_root_level, vcpu-
> >kvm))
>  		return;
>  
>  	td_vmcs_write64(to_tdx(vcpu), SHARED_EPT_POINTER, root_hpa);
> @@ -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.

Otherwise,

Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>

> +}
> +
>  static int tdx_td_init(struct kvm *kvm, struct kvm_tdx_cmd *cmd)
>  {
>  	struct kvm_tdx_init_vm __user *user_data = u64_to_user_ptr(cmd-
> >data);
> @@ -2823,9 +2825,9 @@ static int tdx_td_init(struct kvm *kvm, struct
> kvm_tdx_cmd *cmd)
>  	kvm_tdx->xfam = td_params->xfam;
>  
>  	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);
>  
>  	kvm_tdx->state = TD_STATE_INITIALIZED;
>  out:
> 
> base-commit: 1b731e5ded480bd1e5546aed35584238661ce72e
> --


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: VMX: Explicitly track TDX VMs' root level instead of guessing it from CPUID
  2026-08-19 17:35         ` Edgecombe, Rick P
@ 2026-08-19 18:45           ` Sean Christopherson
  2026-08-19 18:56             ` Edgecombe, Rick P
  0 siblings, 1 reply; 9+ messages in thread
From: Sean Christopherson @ 2026-08-19 18:45 UTC (permalink / raw)
  To: Rick P Edgecombe
  Cc: dave.hansen@linux.intel.com, kas@kernel.org,
	binbin.wu@linux.intel.com, Xiaoyao Li,
	linux-kernel@vger.kernel.org, Yan Y Zhao, Kai Huang,
	pbonzini@redhat.com, kvm@vger.kernel.org,
	linux-coco@lists.linux.dev, x86@kernel.org

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.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: VMX: Explicitly track TDX VMs' root level instead of guessing it from CPUID
  2026-08-19 18:45           ` Sean Christopherson
@ 2026-08-19 18:56             ` Edgecombe, Rick P
  2026-08-19 19:41               ` Sean Christopherson
  0 siblings, 1 reply; 9+ messages in thread
From: Edgecombe, Rick P @ 2026-08-19 18:56 UTC (permalink / raw)
  To: seanjc@google.com
  Cc: dave.hansen@linux.intel.com, kas@kernel.org,
	binbin.wu@linux.intel.com, Li, Xiaoyao,
	linux-kernel@vger.kernel.org, Zhao, Yan Y, Huang, Kai,
	pbonzini@redhat.com, kvm@vger.kernel.org,
	linux-coco@lists.linux.dev, x86@kernel.org

On Wed, 2026-08-19 at 11:45 -0700, Sean Christopherson wrote:
> > 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?

:) Sure.

Yan and I were discussing what might be a new level of defense on MMU checking.
We were basically trying to work out your thinking on some of the defensive
patches lately. It seems there has also been a new level of activity on the bugs
front so we want to adapt to any learnings you had. I actually planned to bring
it up in PUCK, but...

Can you share any thoughts? Should we be more paranoid in general, or same as
always? Or more specifically paranoid where issues hit?

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

Eh, I can see it. I weigh it against "magic numbers" though.


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] KVM: VMX: Explicitly track TDX VMs' root level instead of guessing it from CPUID
  2026-08-19 18:56             ` Edgecombe, Rick P
@ 2026-08-19 19:41               ` Sean Christopherson
  0 siblings, 0 replies; 9+ messages in thread
From: Sean Christopherson @ 2026-08-19 19:41 UTC (permalink / raw)
  To: Rick P Edgecombe
  Cc: dave.hansen@linux.intel.com, kas@kernel.org,
	binbin.wu@linux.intel.com, Xiaoyao Li,
	linux-kernel@vger.kernel.org, Yan Y Zhao, Kai Huang,
	pbonzini@redhat.com, kvm@vger.kernel.org,
	linux-coco@lists.linux.dev, x86@kernel.org

On Wed, Aug 19, 2026, Rick P Edgecombe wrote:
> On Wed, 2026-08-19 at 11:45 -0700, Sean Christopherson wrote:
> > Hmm, for defense in depth, I want to explicitly check mirror_root_level,
> > because returning '0' would likely have dire consequences.  How about this?
> 
> :) Sure.
> 
> Yan and I were discussing what might be a new level of defense on MMU checking.
> We were basically trying to work out your thinking on some of the defensive
> patches lately. It seems there has also been a new level of activity on the bugs
> front so we want to adapt to any learnings you had. I actually planned to bring
> it up in PUCK, but...
> 
> Can you share any thoughts? Should we be more paranoid in general, or same as
> always? Or more specifically paranoid where issues hit?

The big learning I've had is that simply detecting bugs doesn't help protect the
host unless KVM also takes evasive action when the bug is detected.  E.g. a WARN
will (hopefully) be super helpful in root causing what went wrong, but it doesn't
do anything to mitigate the bug in real time.

A theme common to several (not all, but several) of the recent guest-exploitable
vulnerabilities is that KVM *did* have relevant sanity checks, but KVM didn't
actually do anything meaningful when a check failed and/or an assumption didn't
hold true.  It's not always possible/desirable to take evasive action (see below),
but in most cases it is.

Other than that, I don't think there's anything "new" per se, just a bit more of
a sense of urgency.  E.g. avoid BUG() and BUG_ON() unless there's a *very* high
probability the alternative is worse (this is why I said above that doing more
than WARNing may not be desirable).  If you fix a bug that could be applicable to
other code, look for ways to (practically) eliminate the potential source of bugs
(much of the guard() stuff falls into this category; the cleanup behavior makes
it a lot hard to end up with deadlock due to forgetting an unlock in a rare path).
And so on and so forth.

As for this exact sanity check, the reason why I think it's worth keeping is that
we've already messed it up once, the code pretty much only runs once per VM boot,
and if KVM configures the wrong level, the *best* case scenario is probably that
the host panics.  I.e. my past statements along the lines of "at some point we have
to not screw up" still hold true, but as with many kernel rules and guidlines, it
needs to be applied with a healthy dose of critical thinking.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2026-08-19 19:41 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-14 22:45 [PATCH] KVM: VMX: Explicitly track TDX VMs' root level instead of guessing it from CPUID Sean Christopherson
2026-08-19  0:53 ` Edgecombe, Rick P
2026-08-19  0:59   ` Sean Christopherson
2026-08-19 14:35     ` Edgecombe, Rick P
2026-08-19 15:56       ` Sean Christopherson
2026-08-19 17:35         ` Edgecombe, Rick P
2026-08-19 18:45           ` Sean Christopherson
2026-08-19 18:56             ` Edgecombe, Rick P
2026-08-19 19:41               ` Sean Christopherson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox