| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <0855f4db39b58fdea6f6304fc97863241e509620.camel@intel.com>
Date: Fri, 30 Jan 2026 23:55:43 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "seanjc@...gle.com" <seanjc@...gle.com>, "x86@...nel.org"
<x86@...nel.org>, "dave.hansen@...ux.intel.com"
<dave.hansen@...ux.intel.com>, "kas@...nel.org" <kas@...nel.org>,
"bp@...en8.de" <bp@...en8.de>, "mingo@...hat.com" <mingo@...hat.com>,
"pbonzini@...hat.com" <pbonzini@...hat.com>, "tglx@...nel.org"
<tglx@...nel.org>
CC: "Huang, Kai" <kai.huang@...el.com>, "ackerleytng@...gle.com"
<ackerleytng@...gle.com>, "sagis@...gle.com" <sagis@...gle.com>, "Annapurve,
Vishal" <vannapurve@...gle.com>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>, "Zhao, Yan Y" <yan.y.zhao@...el.com>, "Li,
Xiaoyao" <xiaoyao.li@...el.com>, "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
"linux-coco@...ts.linux.dev" <linux-coco@...ts.linux.dev>, "Yamahata, Isaku"
<isaku.yamahata@...el.com>, "binbin.wu@...ux.intel.com"
<binbin.wu@...ux.intel.com>
Subject: Re: [RFC PATCH v5 05/45] KVM: TDX: Drop
kvm_x86_ops.link_external_spt(), use .set_external_spte() for all
On Wed, 2026-01-28 at 17:14 -0800, Sean Christopherson wrote:
> Drop the dedicated .link_external_spt() for linking non-leaf S-EPT
> pages, and instead funnel everything through .set_external_spte().
> Using separate hooks doesn't help prevent TDP MMU details from
> bleeding into TDX, and vice versa; to the contrary, dedicated
> callbacks will result in _more_ pollution when hugepage support is
> added, e.g. will require the TDP MMU to know details about the
> splitting rules for TDX that aren't all that relevant to
> the TDP MMU.
>
> Ideally, KVM would provide a single pair of hooks to set S-EPT
> entries, one hook for setting SPTEs under write-lock and another for
> settings SPTEs under read-lock (e.g. to ensure the entire operation
> is "atomic", to allow for failure, etc.). Sadly, TDX's requirement
> that all child S-EPT entries are removed before the parent makes that
> impractical: the TDP MMU deliberately prunes non-leaf SPTEs and
> _then_ processes its children, thus making it quite important for the
> TDP MMU to differentiate between zapping leaf and non-leaf S-EPT
> entries.
>
> However, that's the _only_ case that's truly special, and even that
> case could be shoehorned into a single hook; it's just wouldn't be a
> net positive.
>
> Signed-off-by: Sean Christopherson <seanjc@...gle.com>
It has better handling of the external_spt == NULL case too, by
actually returning an error, but one naming nit below to take or leave.
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@...el.com>
> ---
> arch/x86/include/asm/kvm-x86-ops.h | 1 -
> arch/x86/include/asm/kvm_host.h | 3 --
> arch/x86/kvm/mmu/tdp_mmu.c | 37 +++---------------
> arch/x86/kvm/vmx/tdx.c | 61 ++++++++++++++++++++--------
> --
> 4 files changed, 48 insertions(+), 54 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm-x86-ops.h
> b/arch/x86/include/asm/kvm-x86-ops.h
> index c18a033bee7e..57eb1f4832ae 100644
> --- a/arch/x86/include/asm/kvm-x86-ops.h
> +++ b/arch/x86/include/asm/kvm-x86-ops.h
> @@ -94,7 +94,6 @@ KVM_X86_OP_OPTIONAL_RET0(set_tss_addr)
> KVM_X86_OP_OPTIONAL_RET0(set_identity_map_addr)
> KVM_X86_OP_OPTIONAL_RET0(get_mt_mask)
> KVM_X86_OP(load_mmu_pgd)
> -KVM_X86_OP_OPTIONAL_RET0(link_external_spt)
> KVM_X86_OP_OPTIONAL_RET0(set_external_spte)
> KVM_X86_OP_OPTIONAL_RET0(free_external_spt)
> KVM_X86_OP_OPTIONAL(remove_external_spte)
> diff --git a/arch/x86/include/asm/kvm_host.h
> b/arch/x86/include/asm/kvm_host.h
> index e441f270f354..d12ca0f8a348 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1853,9 +1853,6 @@ struct kvm_x86_ops {
> void (*load_mmu_pgd)(struct kvm_vcpu *vcpu, hpa_t root_hpa,
> int root_level);
>
> - /* Update external mapping with page table link. */
> - int (*link_external_spt)(struct kvm *kvm, gfn_t gfn, enum
> pg_level level,
> - void *external_spt);
> /* Update the external page table from spte getting set. */
> int (*set_external_spte)(struct kvm *kvm, gfn_t gfn, enum
> pg_level level,
> u64 mirror_spte);
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 0feda295859a..56ad056e6042 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -495,31 +495,17 @@ static void handle_removed_pt(struct kvm *kvm,
> tdp_ptep_t pt, bool shared)
> call_rcu(&sp->rcu_head, tdp_mmu_free_sp_rcu_callback);
> }
>
> -static void *get_external_spt(gfn_t gfn, u64 new_spte, int level)
> -{
> - if (is_shadow_present_pte(new_spte) &&
> !is_last_spte(new_spte, level)) {
> - struct kvm_mmu_page *sp =
> spte_to_child_sp(new_spte);
> -
> - WARN_ON_ONCE(sp->role.level + 1 != level);
> - WARN_ON_ONCE(sp->gfn != gfn);
> - return sp->external_spt;
> - }
> -
> - return NULL;
> -}
> -
> static int __must_check set_external_spte_present(struct kvm *kvm,
> tdp_ptep_t sptep,
> gfn_t gfn, u64
> *old_spte,
> u64 new_spte, int
> level)
> {
> - bool was_present = is_shadow_present_pte(*old_spte);
> - bool is_present = is_shadow_present_pte(new_spte);
> - bool is_leaf = is_present && is_last_spte(new_spte, level);
> - int ret = 0;
> -
> - KVM_BUG_ON(was_present, kvm);
> + int ret;
>
> lockdep_assert_held(&kvm->mmu_lock);
> +
> + if (KVM_BUG_ON(is_shadow_present_pte(*old_spte), kvm))
> + return -EIO;
> +
> /*
> * We need to lock out other updates to the SPTE until the
> external
> * page table has been modified. Use FROZEN_SPTE similar to
> @@ -528,18 +514,7 @@ static int __must_check
> set_external_spte_present(struct kvm *kvm, tdp_ptep_t sp
> if (!try_cmpxchg64(rcu_dereference(sptep), old_spte,
> FROZEN_SPTE))
> return -EBUSY;
>
> - /*
> - * Use different call to either set up middle level
> - * external page table, or leaf.
> - */
> - if (is_leaf) {
> - ret = kvm_x86_call(set_external_spte)(kvm, gfn,
> level, new_spte);
> - } else {
> - void *external_spt = get_external_spt(gfn, new_spte,
> level);
> -
> - KVM_BUG_ON(!external_spt, kvm);
> - ret = kvm_x86_call(link_external_spt)(kvm, gfn,
> level, external_spt);
> - }
> + ret = kvm_x86_call(set_external_spte)(kvm, gfn, level,
> new_spte);
> if (ret)
> __kvm_tdp_mmu_write_spte(sptep, *old_spte);
> else
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 5688c77616e3..30494f9ceb31 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -1664,18 +1664,58 @@ static int tdx_mem_page_aug(struct kvm *kvm,
> gfn_t gfn,
> return 0;
> }
>
> +static struct page *tdx_spte_to_external_spt(struct kvm *kvm, gfn_t
> gfn,
> + u64 new_spte, enum
> pg_level level)
> +{
> + struct kvm_mmu_page *sp = spte_to_child_sp(new_spte);
> +
> + if (KVM_BUG_ON(!sp->external_spt, kvm) ||
> + KVM_BUG_ON(sp->role.level + 1 != level, kvm) ||
> + KVM_BUG_ON(sp->gfn != gfn, kvm))
> + return NULL;
> +
> + return virt_to_page(sp->external_spt);
> +}
> +
> +static int tdx_sept_link_private_spt(struct kvm *kvm, gfn_t gfn,
> + enum pg_level level, u64
> mirror_spte)
> +{
> + gpa_t gpa = gfn_to_gpa(gfn);
> + u64 err, entry, level_state;
> + struct page *external_spt;
> +
> + external_spt = tdx_spte_to_external_spt(kvm, gfn,
> mirror_spte, level);
The "external" abstraction wraps the "S-EPT" knowledge and naming (for
maybe increasingly dubious reasons), but in the TDX code, inside the
abstraction, it uses the sept naming. so I might have called it
sept_pt.
Powered by blists - more mailing lists