* Re: [RFC PATCH v5 20/45] KVM: x86/mmu: Allocate/free S-EPT pages using tdx_{alloc,free}_control_page()
From: Huang, Kai @ 2026-02-03 11:16 UTC (permalink / raw)
To: seanjc@google.com, x86@kernel.org, dave.hansen@linux.intel.com,
kas@kernel.org, bp@alien8.de, mingo@redhat.com,
pbonzini@redhat.com, tglx@kernel.org
Cc: Edgecombe, Rick P, ackerleytng@google.com, sagis@google.com,
Annapurve, Vishal, linux-kernel@vger.kernel.org, Zhao, Yan Y,
Li, Xiaoyao, kvm@vger.kernel.org, linux-coco@lists.linux.dev,
Yamahata, Isaku, binbin.wu@linux.intel.com
In-Reply-To: <20260129011517.3545883-21-seanjc@google.com>
On Wed, 2026-01-28 at 17:14 -0800, Sean Christopherson wrote:
> Now that kvm_mmu_memory_cache supports custom page allocators, wire up the
> S-EPT cache to use tdx_{alloc,free}_control_page() (arguably S-EPT pages
> aren't "control" pages, but they're not guest pages either). Using the
> TDX APIs will make S-EPT pages naturally play nice with Dynamic PAMT, by
> virtue of adding/removing PAMT entries when S-EPT pages are allocated and
> freed, as opposed to when they are added/removed from the S-EPT tree.
>
> Inserting into the PAMT entries on allocation does mean KVM will create
> unnecessary PAMT entries, e.g. once a vCPU stops faulting in memory, the
> remaining pages in the MMU cache will go unused. But in practice, odds
> are very good the containing 2MiB page will have other in-use S-EPT pages,
> i.e. will create PAMT entries anyways. And _if_ creating PAMT entries on
> allocation is problematic for memory consumption, that can be resolved by
> tweaking KVM's cache size.
>
> Suggested-by: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
Reviewed-by: Kai Huang <kai.huang@intel.com>
Some nits below ..
[...]
> int (*set_external_spte)(struct kvm *kvm, gfn_t gfn, enum pg_level level,
> u64 mirror_spte);
> -
> - /* Update external page tables for page table about to be freed. */
> void (*reclaim_external_sp)(struct kvm *kvm, gfn_t gfn,
> struct kvm_mmu_page *sp);
> -
> - /* Update external page table from spte getting removed, and flush TLB. */
The above two comments are still useful to me.
Not sure why do you want to remove them, especially in _this_ patch?
> void (*remove_external_spte)(struct kvm *kvm, gfn_t gfn, enum pg_level level,
> u64 mirror_spte);
>
> +
Unintentional change?
> bool (*has_wbinvd_exit)(void);
>
> u64 (*get_l2_tsc_offset)(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 3911ac9bddfd..9b5a6861e2a4 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -6690,11 +6690,13 @@ int kvm_mmu_create(struct kvm_vcpu *vcpu)
> vcpu->arch.mmu_page_header_cache.kmem_cache = mmu_page_header_cache;
> vcpu->arch.mmu_page_header_cache.gfp_zero = __GFP_ZERO;
>
> - vcpu->arch.mmu_shadow_page_cache.init_value =
> - SHADOW_NONPRESENT_VALUE;
> + vcpu->arch.mmu_shadow_page_cache.init_value = SHADOW_NONPRESENT_VALUE;
> if (!vcpu->arch.mmu_shadow_page_cache.init_value)
> vcpu->arch.mmu_shadow_page_cache.gfp_zero = __GFP_ZERO;
Ditto. Not sure this adjustment is intentional?
^ permalink raw reply
* Re: [RFC PATCH v5 19/45] KVM: Allow owner of kvm_mmu_memory_cache to provide a custom page allocator
From: Huang, Kai @ 2026-02-03 10:56 UTC (permalink / raw)
To: seanjc@google.com, x86@kernel.org, dave.hansen@linux.intel.com,
kas@kernel.org, bp@alien8.de, mingo@redhat.com,
pbonzini@redhat.com, tglx@kernel.org
Cc: Edgecombe, Rick P, ackerleytng@google.com, sagis@google.com,
Annapurve, Vishal, linux-kernel@vger.kernel.org, Zhao, Yan Y,
Li, Xiaoyao, kvm@vger.kernel.org, linux-coco@lists.linux.dev,
Yamahata, Isaku, binbin.wu@linux.intel.com
In-Reply-To: <20260129011517.3545883-20-seanjc@google.com>
On Wed, 2026-01-28 at 17:14 -0800, Sean Christopherson wrote:
> Extend "struct kvm_mmu_memory_cache" to support a custom page allocator
> so that x86's TDX can update per-page metadata on allocation and free().
>
> Name the allocator page_get() to align with __get_free_page(), e.g. to
> communicate that it returns an "unsigned long", not a "struct page", and
> to avoid collisions with macros, e.g. with alloc_page.
>
> Suggested-by: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
I thought it could be more generic for allocating an object, but not just a
page.
E.g., I thought we might be able to use it to allocate a structure which has
"pair of DPAMT pages" so it could be assigned to 'struct kvm_mmu_page'. But
it seems you abandoned this idea. May I ask why? Just want to understand
the reasoning here.
Anyway:
Reviewed-by: Kai Huang <kai.huang@intel.com>
> ---
> include/linux/kvm_types.h | 2 ++
> virt/kvm/kvm_main.c | 7 ++++++-
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
> index a568d8e6f4e8..87fa9deffdb7 100644
> --- a/include/linux/kvm_types.h
> +++ b/include/linux/kvm_types.h
> @@ -112,6 +112,8 @@ struct kvm_mmu_memory_cache {
> gfp_t gfp_custom;
> u64 init_value;
> struct kmem_cache *kmem_cache;
> + unsigned long (*page_get)(gfp_t gfp);
> + void (*page_free)(unsigned long addr);
> int capacity;
> int nobjs;
> void **objects;
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 571cf0d6ec01..7015edce5bd8 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -356,7 +356,10 @@ static inline void *mmu_memory_cache_alloc_obj(struct kvm_mmu_memory_cache *mc,
> if (mc->kmem_cache)
> return kmem_cache_alloc(mc->kmem_cache, gfp_flags);
>
> - page = (void *)__get_free_page(gfp_flags);
> + if (mc->page_get)
> + page = (void *)mc->page_get(gfp_flags);
> + else
> + page = (void *)__get_free_page(gfp_flags);
> if (page && mc->init_value)
> memset64(page, mc->init_value, PAGE_SIZE / sizeof(u64));
> return page;
> @@ -416,6 +419,8 @@ void kvm_mmu_free_memory_cache(struct kvm_mmu_memory_cache *mc)
> while (mc->nobjs) {
> if (mc->kmem_cache)
> kmem_cache_free(mc->kmem_cache, mc->objects[--mc->nobjs]);
> + else if (mc->page_free)
> + mc->page_free((unsigned long)mc->objects[--mc->nobjs]);
> else
> free_page((unsigned long)mc->objects[--mc->nobjs]);
> }
> --
> 2.53.0.rc1.217.geba53bf80e-goog
^ permalink raw reply
* Re: [RFC PATCH v5 04/45] KVM: x86: Make "external SPTE" ops that can fail RET0 static calls
From: Huang, Kai @ 2026-02-03 10:44 UTC (permalink / raw)
To: seanjc@google.com, Edgecombe, Rick P
Cc: kvm@vger.kernel.org, Li, Xiaoyao, linux-coco@lists.linux.dev,
Zhao, Yan Y, dave.hansen@linux.intel.com, kas@kernel.org,
binbin.wu@linux.intel.com, mingo@redhat.com,
linux-kernel@vger.kernel.org, pbonzini@redhat.com,
Yamahata, Isaku, ackerleytng@google.com, tglx@kernel.org,
Annapurve, Vishal, bp@alien8.de, sagis@google.com, x86@kernel.org
In-Reply-To: <e3feb0224cf2665a71ba6147e4e3e3bb30f96760.camel@intel.com>
On Fri, 2026-01-30 at 17:32 +0000, Edgecombe, Rick P wrote:
> On Thu, 2026-01-29 at 17:28 -0800, Sean Christopherson wrote:
> >
> > Hmm, that's probably doable, but definitely in a separate patch.
> > E.g. something
> > like:
>
> I think it would be a good change. But after more consideration, I
> think the original patch is good on its own. Better to turn a bug into
> a deterministic thing, than an opportunity to consume stack. Seems to
> be what you intended.
>
> Another idea would be to have a variant that returns an error instead
> of 0 so the callers can have there error logic triggered, but it's all
> incremental value on top of this.
>
> Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
Makes sense.
Reviewed-by: Kai Huang <kai.huang@intel.com>
^ permalink raw reply
* Re: [RFC PATCH v5 03/45] KVM: TDX: Account all non-transient page allocations for per-TD structures
From: Huang, Kai @ 2026-02-03 10:36 UTC (permalink / raw)
To: seanjc@google.com, x86@kernel.org, dave.hansen@linux.intel.com,
kas@kernel.org, bp@alien8.de, mingo@redhat.com,
pbonzini@redhat.com, tglx@kernel.org
Cc: Edgecombe, Rick P, ackerleytng@google.com, sagis@google.com,
Annapurve, Vishal, linux-kernel@vger.kernel.org, Zhao, Yan Y,
Li, Xiaoyao, kvm@vger.kernel.org, linux-coco@lists.linux.dev,
Yamahata, Isaku, binbin.wu@linux.intel.com
In-Reply-To: <20260129011517.3545883-4-seanjc@google.com>
On Wed, 2026-01-28 at 17:14 -0800, Sean Christopherson wrote:
> Account all non-transient allocations associated with a single TD (or its
> vCPUs), as KVM's ABI is that allocations that are active for the lifetime
> of a VM are accounted. Leave temporary allocations, i.e. allocations that
> are freed within a single function/ioctl, unaccounted, to again align with
> KVM's existing behavior, e.g. see commit dd103407ca31 ("KVM: X86: Remove
> unnecessary GFP_KERNEL_ACCOUNT for temporary variables").
>
> Fixes: 8d032b683c29 ("KVM: TDX: create/destroy VM structure")
> Fixes: a50f673f25e0 ("KVM: TDX: Do TDX specific vcpu initialization")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
>
Reviewed-by: Kai Huang <kai.huang@intel.com>
^ permalink raw reply
* Re: [RFC PATCH v5 02/45] KVM: x86/mmu: Update iter->old_spte if cmpxchg64 on mirror SPTE "fails"
From: Huang, Kai @ 2026-02-03 10:30 UTC (permalink / raw)
To: seanjc@google.com, x86@kernel.org, dave.hansen@linux.intel.com,
kas@kernel.org, bp@alien8.de, mingo@redhat.com,
pbonzini@redhat.com, tglx@kernel.org
Cc: Edgecombe, Rick P, ackerleytng@google.com, sagis@google.com,
Annapurve, Vishal, linux-kernel@vger.kernel.org, Zhao, Yan Y,
Li, Xiaoyao, kvm@vger.kernel.org, linux-coco@lists.linux.dev,
Yamahata, Isaku, binbin.wu@linux.intel.com
In-Reply-To: <20260129011517.3545883-3-seanjc@google.com>
On Wed, 2026-01-28 at 17:14 -0800, Sean Christopherson wrote:
> Pass a pointer to iter->old_spte, not simply its value, when setting an
> external SPTE in __tdp_mmu_set_spte_atomic(), so that the iterator's value
> will be updated if the cmpxchg64 to freeze the mirror SPTE fails. The bug
> is currently benign as TDX is mutualy exclusive with all paths that do
> "local" retry", e.g. clear_dirty_gfn_range() and wrprot_gfn_range().
>
> Fixes: 77ac7079e66d ("KVM: x86/tdp_mmu: Propagate building mirror page tables")
> Signed-off-by: Sean Christopherson <seanjc@google.com>
Reviewed-by: Kai Huang <kai.huang@intel.com>
Btw, do we need to cc stable?
^ permalink raw reply
* Re: [RFC PATCH v5 05/45] KVM: TDX: Drop kvm_x86_ops.link_external_spt(), use .set_external_spte() for all
From: Yan Zhao @ 2026-02-03 10:19 UTC (permalink / raw)
To: Sean Christopherson
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
Kiryl Shutsemau, Paolo Bonzini, linux-kernel, linux-coco, kvm,
Kai Huang, Rick Edgecombe, Vishal Annapurve, Ackerley Tng,
Sagi Shahar, Binbin Wu, Xiaoyao Li, Isaku Yamahata
In-Reply-To: <20260129011517.3545883-6-seanjc@google.com>
On Wed, Jan 28, 2026 at 05:14:37PM -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@google.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;
Why not move this check of is_shadow_present_pte() to tdx_sept_set_private_spte()
as well?
Or also check !is_shadow_present_pte(new_spte) in TDP MMU?
> * 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;
Could we remove the KVM_BUG_ON()s, and ...
> + 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);
> + if (!external_spt)
add a KVM_BUG_ON() here?
It could save KVM_BUG_ON()s and have KVM_BUG_ON() match -EIO :)
And as Rick also mentioned, better to remove external in external_spt, e.g.
something like pt_page.
And mirror_spte --> new_spte?
> + return -EIO;
> +
> + err = tdh_mem_sept_add(&to_kvm_tdx(kvm)->td, gpa, level, external_spt,
> + &entry, &level_state);
> + if (unlikely(tdx_operand_busy(err)))
> + return -EBUSY;
> +
> + if (TDX_BUG_ON_2(err, TDH_MEM_SEPT_ADD, entry, level_state, kvm))
> + return -EIO;
> +
> + return 0;
> +}
> +
> static int tdx_sept_set_private_spte(struct kvm *kvm, gfn_t gfn,
> enum pg_level level, u64 mirror_spte)
> {
> struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> kvm_pfn_t pfn = spte_to_pfn(mirror_spte);
>
> + if (KVM_BUG_ON(!is_shadow_present_pte(mirror_spte), kvm))
> + return -EIO;
> +
> + if (!is_last_spte(mirror_spte, level))
> + return tdx_sept_link_private_spt(kvm, gfn, level, mirror_spte);
> +
> /* TODO: handle large pages. */
> if (KVM_BUG_ON(level != PG_LEVEL_4K, kvm))
> return -EIO;
>
> - WARN_ON_ONCE(!is_shadow_present_pte(mirror_spte) ||
> - (mirror_spte & VMX_EPT_RWX_MASK) != VMX_EPT_RWX_MASK);
> + WARN_ON_ONCE((mirror_spte & VMX_EPT_RWX_MASK) != VMX_EPT_RWX_MASK);
Also check this for tdx_sept_link_private_spt()?
> /*
> * Ensure pre_fault_allowed is read by kvm_arch_vcpu_pre_fault_memory()
> @@ -1695,23 +1735,7 @@ static int tdx_sept_set_private_spte(struct kvm *kvm, gfn_t gfn,
> return tdx_mem_page_aug(kvm, gfn, level, pfn);
> }
>
> -static int tdx_sept_link_private_spt(struct kvm *kvm, gfn_t gfn,
> - enum pg_level level, void *private_spt)
> -{
> - gpa_t gpa = gfn_to_gpa(gfn);
> - struct page *page = virt_to_page(private_spt);
> - u64 err, entry, level_state;
>
> - err = tdh_mem_sept_add(&to_kvm_tdx(kvm)->td, gpa, level, page, &entry,
> - &level_state);
> - if (unlikely(tdx_operand_busy(err)))
> - return -EBUSY;
> -
> - if (TDX_BUG_ON_2(err, TDH_MEM_SEPT_ADD, entry, level_state, kvm))
> - return -EIO;
> -
> - return 0;
> -}
>
> /*
> * Ensure shared and private EPTs to be flushed on all vCPUs.
> @@ -3592,7 +3616,6 @@ void __init tdx_hardware_setup(void)
> */
> vt_x86_ops.vm_size = max_t(unsigned int, vt_x86_ops.vm_size, sizeof(struct kvm_tdx));
>
> - vt_x86_ops.link_external_spt = tdx_sept_link_private_spt;
> vt_x86_ops.set_external_spte = tdx_sept_set_private_spte;
> vt_x86_ops.free_external_spt = tdx_sept_free_private_spt;
> vt_x86_ops.remove_external_spte = tdx_sept_remove_private_spte;
> --
> 2.53.0.rc1.217.geba53bf80e-goog
>
^ permalink raw reply
* Re: [RFC PATCH v5 02/45] KVM: x86/mmu: Update iter->old_spte if cmpxchg64 on mirror SPTE "fails"
From: Yan Zhao @ 2026-02-03 8:48 UTC (permalink / raw)
To: Sean Christopherson
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
Kiryl Shutsemau, Paolo Bonzini, linux-kernel, linux-coco, kvm,
Kai Huang, Rick Edgecombe, Vishal Annapurve, Ackerley Tng,
Sagi Shahar, Binbin Wu, Xiaoyao Li, Isaku Yamahata
In-Reply-To: <20260129011517.3545883-3-seanjc@google.com>
On Wed, Jan 28, 2026 at 05:14:34PM -0800, Sean Christopherson wrote:
> Pass a pointer to iter->old_spte, not simply its value, when setting an
> external SPTE in __tdp_mmu_set_spte_atomic(), so that the iterator's value
> will be updated if the cmpxchg64 to freeze the mirror SPTE fails. The bug
> is currently benign as TDX is mutualy exclusive with all paths that do
> "local" retry", e.g. clear_dirty_gfn_range() and wrprot_gfn_range().
>
> Fixes: 77ac7079e66d ("KVM: x86/tdp_mmu: Propagate building mirror page tables")
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> arch/x86/kvm/mmu/tdp_mmu.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 9c26038f6b77..0feda295859a 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -509,10 +509,10 @@ static void *get_external_spt(gfn_t gfn, u64 new_spte, int level)
> }
>
> static int __must_check set_external_spte_present(struct kvm *kvm, tdp_ptep_t sptep,
> - gfn_t gfn, u64 old_spte,
> + gfn_t gfn, u64 *old_spte,
> u64 new_spte, int level)
> {
> - bool was_present = is_shadow_present_pte(old_spte);
> + 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;
> @@ -525,7 +525,7 @@ static int __must_check set_external_spte_present(struct kvm *kvm, tdp_ptep_t sp
> * page table has been modified. Use FROZEN_SPTE similar to
> * the zapping case.
> */
> - if (!try_cmpxchg64(rcu_dereference(sptep), &old_spte, FROZEN_SPTE))
> + if (!try_cmpxchg64(rcu_dereference(sptep), old_spte, FROZEN_SPTE))
> return -EBUSY;
>
> /*
> @@ -541,7 +541,7 @@ static int __must_check set_external_spte_present(struct kvm *kvm, tdp_ptep_t sp
> ret = kvm_x86_call(link_external_spt)(kvm, gfn, level, external_spt);
> }
> if (ret)
> - __kvm_tdp_mmu_write_spte(sptep, old_spte);
> + __kvm_tdp_mmu_write_spte(sptep, *old_spte);
Do we need to add a comment explaining that when the above try_cmpxchg64()
succeeds, the value of *old_spte is unmodified?
> else
> __kvm_tdp_mmu_write_spte(sptep, new_spte);
> return ret;
> @@ -670,7 +670,7 @@ static inline int __must_check __tdp_mmu_set_spte_atomic(struct kvm *kvm,
> return -EBUSY;
>
> ret = set_external_spte_present(kvm, iter->sptep, iter->gfn,
> - iter->old_spte, new_spte, iter->level);
> + &iter->old_spte, new_spte, iter->level);
> if (ret)
> return ret;
> } else {
> --
> 2.53.0.rc1.217.geba53bf80e-goog
>
^ permalink raw reply
* Re: [PATCH 1/2] KVM: SEV: IBPB-on-Entry guest support
From: Borislav Petkov @ 2026-02-02 17:12 UTC (permalink / raw)
To: Tom Lendacky
Cc: Kim Phillips, linux-kernel, kvm, linux-coco, x86,
Sean Christopherson, Paolo Bonzini, K Prateek Nayak,
Nikunj A Dadhania, Michael Roth, Naveen Rao, David Kaplan, stable
In-Reply-To: <3392d89c-ebf5-48f2-b498-a7dc532a0493@amd.com>
On Mon, Feb 02, 2026 at 10:09:19AM -0600, Tom Lendacky wrote:
> But I can see that getting stale because it isn't required to be updated
> for features that don't require an implementation in order for the guest
> to boot successfully. Whereas the SNP_FEATURES_IMPL_REQ is set with
> known values that require an implementation and all the reserved bits
> set. So it takes actual updating to get one of those features to work
> that are represented in that bitmap.
Ok, I guess we can rename that define SNP_FEATURES_IMPL to denote is the
counterpart of SNP_FEATURES_IMPL_REQ, so to speak.
@Kim, you can send a new version with the define renamed.
Due to it being too close to the merge window, it'll wait for after and then
it can go to stable later but I don't think that's a problem.
> That will tell us what the guest is running with, not what it can run with.
hm, ok, let's think about this more then. I don't have a clear use case for
a this-is-what-a-SNP-guest-can-run-with so let's deal with that later...
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH 1/2] KVM: SEV: IBPB-on-Entry guest support
From: Tom Lendacky @ 2026-02-02 16:09 UTC (permalink / raw)
To: Borislav Petkov
Cc: Kim Phillips, linux-kernel, kvm, linux-coco, x86,
Sean Christopherson, Paolo Bonzini, K Prateek Nayak,
Nikunj A Dadhania, Michael Roth, Naveen Rao, David Kaplan, stable
In-Reply-To: <20260202154936.GAaYDHkOMpjFpoBe5m@fat_crate.local>
On 2/2/26 09:49, Borislav Petkov wrote:
> On Mon, Feb 02, 2026 at 09:38:50AM -0600, Tom Lendacky wrote:
>> I guess it really depends on the persons point of view. I agree that
>> renaming the SNP_FEATURES_PRESENT to SNP_FEATURES_IMPL(EMENTED) would
>> match up nicely with SNP_FEATURES_IMPL_REQ. Maybe that's all that is
>> needed...
>
> I guess...
>
> I still think it would be useful to have a common place that says which things
> in SEV_STATUS are supported and present in a guest, no?
But I can see that getting stale because it isn't required to be updated
for features that don't require an implementation in order for the guest
to boot successfully. Whereas the SNP_FEATURES_IMPL_REQ is set with
known values that require an implementation and all the reserved bits
set. So it takes actual updating to get one of those features to work
that are represented in that bitmap.
>
> Or are we going to dump that MSR like Joerg's patch from a while ago and
> that'll tell us what the guest supports?
That will tell us what the guest is running with, not what it can run with.
Thanks,
Tom
>
> Hmm.
>
^ permalink raw reply
* Re: [PATCH 1/1] firmware: smccc: add support for Live Firmware Activation (LFA)
From: Salman Nabi @ 2026-02-02 15:52 UTC (permalink / raw)
To: Trilok Soni, vvidwans, andre.przywara, sudeep.holla, mark.rutland,
lpieralisi
Cc: ardb, chao.gao, linux-arm-kernel, linux-coco, linux-kernel,
sdonthineni, vsethi, vwadekar
In-Reply-To: <b5fd72fa-fcde-4f7a-bfad-17465d7e3841@oss.qualcomm.com>
Hi Trilok,
On 1/31/26 01:35, Trilok Soni wrote:
> On 1/19/2026 4:27 AM, Salman Nabi wrote:
>> The Arm Live Firmware Activation (LFA) is a specification [1] to describe
>> activating firmware components without a reboot. Those components
>> (like TF-A's BL31, EDK-II, TF-RMM, secure paylods) would be updated the
>> usual way: via fwupd, FF-A or other secure storage methods, or via some
>> IMPDEF Out-Of-Bound method. The user can then activate this new firmware,
>> at system runtime, without requiring a reboot.
>> The specification covers the SMCCC interface to list and query available
>> components and eventually trigger the activation.
>>
>> Add a new directory under /sys/firmware to present firmware components
>> capable of live activation. Each of them is a directory under lfa/,
>> and is identified via its GUID. The activation will be triggered by echoing
>> "1" into the "activate" file:
>> ==========================================
>> /sys/firmware/lfa # ls -l . 6c*
>> .:
>> total 0
>> drwxr-xr-x 2 0 0 0 Jan 19 11:33 47d4086d-4cfe-9846-9b95-2950cbbd5a00
>> drwxr-xr-x 2 0 0 0 Jan 19 11:33 6c0762a6-12f2-4b56-92cb-ba8f633606d9
>> drwxr-xr-x 2 0 0 0 Jan 19 11:33 d6d0eea7-fcea-d54b-9782-9934f234b6e4
> Can you please explain or add a note on why we don't have name of the firmware
> as the directory name and why you have selected GUID as top-level
> directory name?
We obtain the GUIDs of firmware components from the LFA agent in TF-A, which does not provide their names. For convenience, we have added a C structure in the driver to associate each firmware GUID with its corresponding name. Because new firmware components may be supported in the LFA agent before their GUID-to-name mapping is added to the driver, we avoid using the firmware name as the directory (kobject) name.
Additionally, sysfs requires directory names to be unique among siblings. Since GUIDs are inherently unique, they provide a convenient, collision-free choice for directory names.
>
> ---Trilok Soni
Many thanks,
Salman
^ permalink raw reply
* Re: [PATCH 1/2] KVM: SEV: IBPB-on-Entry guest support
From: Borislav Petkov @ 2026-02-02 15:49 UTC (permalink / raw)
To: Tom Lendacky
Cc: Kim Phillips, linux-kernel, kvm, linux-coco, x86,
Sean Christopherson, Paolo Bonzini, K Prateek Nayak,
Nikunj A Dadhania, Michael Roth, Naveen Rao, David Kaplan, stable
In-Reply-To: <6556bacb-2e81-4aa8-92e4-0ff8642f4ec9@amd.com>
On Mon, Feb 02, 2026 at 09:38:50AM -0600, Tom Lendacky wrote:
> I guess it really depends on the persons point of view. I agree that
> renaming the SNP_FEATURES_PRESENT to SNP_FEATURES_IMPL(EMENTED) would
> match up nicely with SNP_FEATURES_IMPL_REQ. Maybe that's all that is
> needed...
I guess...
I still think it would be useful to have a common place that says which things
in SEV_STATUS are supported and present in a guest, no?
Or are we going to dump that MSR like Joerg's patch from a while ago and
that'll tell us what the guest supports?
Hmm.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH 1/2] KVM: SEV: IBPB-on-Entry guest support
From: Tom Lendacky @ 2026-02-02 15:38 UTC (permalink / raw)
To: Borislav Petkov
Cc: Kim Phillips, linux-kernel, kvm, linux-coco, x86,
Sean Christopherson, Paolo Bonzini, K Prateek Nayak,
Nikunj A Dadhania, Michael Roth, Naveen Rao, David Kaplan, stable
In-Reply-To: <20260130154534.GCaXzSHgkEFnk5mX14@fat_crate.local>
On 1/30/26 09:45, Borislav Petkov wrote:
> On Fri, Jan 30, 2026 at 08:56:07AM -0600, Tom Lendacky wrote:
>> It can be added. Any of the features added to SNP_FEATURES_PRESENT that
>> aren't set in the SNP_FEATURES_IMPL_REQ bitmap are really a no-op. The
>> SNP_FEATURES_PRESENT bitmap is meant to contain whatever bits are set in
>> SNP_FEATURES_IMPL_REQ when an implementation has been implemented for the
>> guest.
>>
>> But, yeah, we could add all the bits that aren't set in
>> SNP_FEATURES_IMPL_REQ to SNP_FEATURES_PRESENT if it makes it clearer.
>
> Right, that's the question. SNP_FEATURES_PRESENT is used in the masking
> operation to get the unsupported features.
>
> But when we say a SNP feature is present, then, even if it doesn't need guest
> implementation, that feature is still present nonetheless.
>
> So our nomenclature is kinda imprecise here.
>
> I'd say, we can always rename SNP_FEATURES_PRESENT to denote what it is there
> for, i.e., the narrower functionality of the masking.
>
> Or, if we want to gather there *all* features that are present, then we can
> start adding them...
>
>> If we do that, it should probably be a separate patch (?) that also
>> rewords the comment above SNP_FEATURES_PRESENT
>
> ... yes, as a separate patch.
>
> Question is, what do we really wanna do here?
>
> Does it make sense and is it useful to have SNP_FEATURES_PRESENT contain *all*
> guest SNP features...
I guess it really depends on the persons point of view. I agree that
renaming the SNP_FEATURES_PRESENT to SNP_FEATURES_IMPL(EMENTED) would
match up nicely with SNP_FEATURES_IMPL_REQ. Maybe that's all that is
needed...
Thanks,
Tom
>
> Thx.
>
^ permalink raw reply
* Re: [PATCH v3 24/26] x86/virt/seamldr: Extend sigstruct to 16KB
From: Huang, Kai @ 2026-02-02 11:57 UTC (permalink / raw)
To: Gao, Chao
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev,
dave.hansen@linux.intel.com, kas@kernel.org, seanjc@google.com,
Chatre, Reinette, linux-kernel@vger.kernel.org,
tglx@linutronix.de, Verma, Vishal L, nik.borisov@suse.com,
mingo@redhat.com, Weiny, Ira, hpa@zytor.com, Annapurve, Vishal,
sagis@google.com, Duan, Zhenzhong, Edgecombe, Rick P,
paulmck@kernel.org, bp@alien8.de, yilun.xu@linux.intel.com,
x86@kernel.org, Williams, Dan J
In-Reply-To: <aXy/S47ryxy0PwpM@intel.com>
On Fri, 2026-01-30 at 22:25 +0800, Gao, Chao wrote:
> > Let's move the discussion here (from patch 13 -- sorry about that):
> >
> > IIRC this patch just simply re-purposes couple of reserved space in
> > SEAMLDR_PARAMS (which is part of P-SEAMLDR ABI) w/o enumeration, explicit
> > opt-in whatever. The code change here doesn't even bump up its version.
> >
> > IIUC, if this code run on an old platform where SEAMLDR.INSTALL still only
> > works with 4K SIGSTRUCT, the SEAMLDR.INSTALL will only see part of the
> > SIGSTRUCT thus will likely fail.
> >
> > How can we know whether a given 'struct tdx_blob' can work on an platform or
> > not? Or am I missing anything?
>
> Good question.
>
> This is actually userspace's responsibility. The kernel exposes P-SEAMLDR
> version to userspace, and for each module, the mapping file [*] lists the
> module's minimum P-SEAMLDR version requirements. This allows userspace to
> determine whether the existing P-SEAMLDR can load a specific TDX blob.
>
> If the kernel cannot load a module using the current P-SEAMLDR, that's
> userspace's fault.
>
> *: https://github.com/intel/confidential-computing.tdx.tdx-module.binaries/blob/main/mapping_file.json
Thanks for the info.
In this case, I am not sure why do you need to implement code (patch 13) to
firstly support 4K less SIGSTRUCT (with a confusing doc of 'tdx_blob' layout
definition), but here extend it to 16K in a second patch?
How about just merge this one to patch 13 and point out this fact in
changelog if needed?
E.g.,:
For a given TDX blob, not all SEAMLDR and TDX module versions support
runtime update for it. Intel publishes the requirement of the minimal
SEAMLDR and TDX module versions for it.
There's no hardware/firmware interface that the kernel could use to
detect and bail out early if such requirement is not met. It's
userspace's responsibility to make sure such requirement is met before
performing runtime update.
Actually, assuming the new spec which reflects 16KB SIGSTRUCT in
SEAMLDR_PARAMS will be published, I _think_ the fact that "some old SEMALDR
versions only support upto 4K SIGSTRUCT" probably doesn't matter anymore,
especially if we add the above to the changelog.
So I don't quite see why we need to keep this "extend SIGSTRUCT to 16KB" as
a separate patch?
Btw, we also need the updated doc of TDX blob layout too, to reflect what
your code is doing (regarding to how to calculate SIGSTRUCT base/size).
And we need to make sure the TDX blob layout is architectural.
^ permalink raw reply
* [PATCH RESEND] KVM: SEV: Enable SNP AP CPU hotplug
From: Jethro Beekman @ 2026-02-02 10:12 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, kvm,
linux-kernel, linux-coco
The GHCB protocol states that after AP CREATE (as opposed to CREATE_ON_INIT),
the hypervisor must immediately proceed to VMRUN. Update vCPU state on AP
CREATE so this happens.
vCPUs created after SNP_LAUNCH_FINISH don't go through snp_launch_update_vmsa.
Ensure the vCPU state is updated properly during VMCB initialization.
Signed-off-by: Jethro Beekman <jethro@fortanix.com>
---
arch/x86/kvm/svm/sev.c | 43 ++++++++++++++++++++++++------------------
1 file changed, 25 insertions(+), 18 deletions(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index cdaca10b8773..9af1bd5b2071 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -960,6 +960,19 @@ static int sev_es_sync_vmsa(struct vcpu_svm *svm)
return 0;
}
+static void sev_es_finalize_vcpu(struct kvm_vcpu *vcpu)
+{
+ vcpu->arch.guest_state_protected = true;
+ /*
+ * SEV-ES (and thus SNP) guest mandates LBR Virtualization to
+ * be _always_ ON. Enable it only after setting
+ * guest_state_protected because KVM_SET_MSRS allows dynamic
+ * toggling of LBRV (for performance reason) on write access to
+ * MSR_IA32_DEBUGCTLMSR when guest_state_protected is not set.
+ */
+ svm_enable_lbrv(vcpu);
+}
+
static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu,
int *error)
{
@@ -999,15 +1012,9 @@ static int __sev_launch_update_vmsa(struct kvm *kvm, struct kvm_vcpu *vcpu,
* do xsave/xrstor on it.
*/
fpstate_set_confidential(&vcpu->arch.guest_fpu);
- vcpu->arch.guest_state_protected = true;
- /*
- * SEV-ES guest mandates LBR Virtualization to be _always_ ON. Enable it
- * only after setting guest_state_protected because KVM_SET_MSRS allows
- * dynamic toggling of LBRV (for performance reason) on write access to
- * MSR_IA32_DEBUGCTLMSR when guest_state_protected is not set.
- */
- svm_enable_lbrv(vcpu);
+ sev_es_finalize_vcpu(vcpu);
+
return 0;
}
@@ -2480,15 +2487,7 @@ static int snp_launch_update_vmsa(struct kvm *kvm, struct kvm_sev_cmd *argp)
return ret;
}
- svm->vcpu.arch.guest_state_protected = true;
- /*
- * SEV-ES (and thus SNP) guest mandates LBR Virtualization to
- * be _always_ ON. Enable it only after setting
- * guest_state_protected because KVM_SET_MSRS allows dynamic
- * toggling of LBRV (for performance reason) on write access to
- * MSR_IA32_DEBUGCTLMSR when guest_state_protected is not set.
- */
- svm_enable_lbrv(vcpu);
+ sev_es_finalize_vcpu(vcpu);
}
return 0;
@@ -4030,6 +4029,10 @@ static void sev_snp_init_protected_guest_state(struct kvm_vcpu *vcpu)
/* Use the new VMSA */
svm->vmcb->control.vmsa_pa = pfn_to_hpa(pfn);
+ /* vCPU was added after SNP_LAUNCH_FINISH */
+ if (!vcpu->arch.guest_state_protected)
+ sev_es_finalize_vcpu(vcpu);
+
/* Mark the vCPU as runnable */
kvm_set_mp_state(vcpu, KVM_MP_STATE_RUNNABLE);
@@ -4111,8 +4114,12 @@ static int sev_snp_ap_creation(struct vcpu_svm *svm)
* Unless Creation is deferred until INIT, signal the vCPU to update
* its state.
*/
- if (request != SVM_VMGEXIT_AP_CREATE_ON_INIT)
+ if (request != SVM_VMGEXIT_AP_CREATE_ON_INIT) {
+ if (target_vcpu->arch.mp_state == KVM_MP_STATE_UNINITIALIZED ||
+ target_vcpu->arch.mp_state == KVM_MP_STATE_INIT_RECEIVED)
+ kvm_set_mp_state(target_vcpu, KVM_MP_STATE_RUNNABLE);
kvm_make_request_and_kick(KVM_REQ_UPDATE_PROTECTED_GUEST_STATE, target_vcpu);
+ }
return 0;
}
--
2.43.0
^ permalink raw reply related
* [PATCH RESEND] KVM: SEV: Track SNP launch state and disallow invalid userspace interactions
From: Jethro Beekman @ 2026-02-02 10:11 UTC (permalink / raw)
To: Sean Christopherson, Paolo Bonzini, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, H. Peter Anvin, kvm,
linux-kernel, linux-coco
Calling any of the SNP_LAUNCH_ ioctls after SNP_LAUNCH_FINISH results in a
kernel page fault due to RMP violation. Track SNP launch state and exit early.
vCPUs created after SNP_LAUNCH_FINISH won't have a guest VMSA automatically
created during SNP_LAUNCH_FINISH by converting the kernel-allocated VMSA. Don't
allocate a VMSA page, so that the vCPU is in a state similar to what it would
be after SNP AP destroy. This ensures pre_sev_run() prevents the vCPU from
running even if userspace makes the vCPU runnable.
Signed-off-by: Jethro Beekman <jethro@fortanix.com>
---
arch/x86/kvm/svm/sev.c | 43 ++++++++++++++++++++++++++----------------
arch/x86/kvm/svm/svm.h | 1 +
2 files changed, 28 insertions(+), 16 deletions(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index f59c65abe3cf..cdaca10b8773 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2205,6 +2205,9 @@ static int snp_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
if (!sev_snp_guest(kvm))
return -ENOTTY;
+ if (sev->snp_finished)
+ return -EINVAL;
+
if (copy_from_user(¶ms, u64_to_user_ptr(argp->data), sizeof(params)))
return -EFAULT;
@@ -2369,7 +2372,7 @@ static int snp_launch_update(struct kvm *kvm, struct kvm_sev_cmd *argp)
void __user *src;
int ret = 0;
- if (!sev_snp_guest(kvm) || !sev->snp_context)
+ if (!sev_snp_guest(kvm) || !sev->snp_context || sev->snp_finished)
return -EINVAL;
if (copy_from_user(¶ms, u64_to_user_ptr(argp->data), sizeof(params)))
@@ -2502,7 +2505,7 @@ static int snp_launch_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
if (!sev_snp_guest(kvm))
return -ENOTTY;
- if (!sev->snp_context)
+ if (!sev->snp_context || sev->snp_finished)
return -EINVAL;
if (copy_from_user(¶ms, u64_to_user_ptr(argp->data), sizeof(params)))
@@ -2548,13 +2551,15 @@ static int snp_launch_finish(struct kvm *kvm, struct kvm_sev_cmd *argp)
data->gctx_paddr = __psp_pa(sev->snp_context);
ret = sev_issue_cmd(kvm, SEV_CMD_SNP_LAUNCH_FINISH, data, &argp->error);
- /*
- * Now that there will be no more SNP_LAUNCH_UPDATE ioctls, private pages
- * can be given to the guest simply by marking the RMP entry as private.
- * This can happen on first access and also with KVM_PRE_FAULT_MEMORY.
- */
- if (!ret)
+ if (!ret) {
+ sev->snp_finished = true;
+ /*
+ * Now that there will be no more SNP_LAUNCH_UPDATE ioctls, private pages
+ * can be given to the guest simply by marking the RMP entry as private.
+ * This can happen on first access and also with KVM_PRE_FAULT_MEMORY.
+ */
kvm->arch.pre_fault_allowed = true;
+ }
kfree(id_auth);
@@ -3253,6 +3258,9 @@ void sev_free_vcpu(struct kvm_vcpu *vcpu)
svm = to_svm(vcpu);
+ if (!svm->sev_es.vmsa)
+ goto skip_vmsa_free;
+
/*
* If it's an SNP guest, then the VMSA was marked in the RMP table as
* a guest-owned page. Transition the page to hypervisor state before
@@ -4653,6 +4661,7 @@ void sev_init_vmcb(struct vcpu_svm *svm, bool init_event)
int sev_vcpu_create(struct kvm_vcpu *vcpu)
{
+ struct kvm_sev_info *sev = to_kvm_sev_info(vcpu->kvm);
struct vcpu_svm *svm = to_svm(vcpu);
struct page *vmsa_page;
@@ -4661,15 +4670,17 @@ int sev_vcpu_create(struct kvm_vcpu *vcpu)
if (!sev_es_guest(vcpu->kvm))
return 0;
- /*
- * SEV-ES guests require a separate (from the VMCB) VMSA page used to
- * contain the encrypted register state of the guest.
- */
- vmsa_page = snp_safe_alloc_page();
- if (!vmsa_page)
- return -ENOMEM;
+ if (!sev->snp_finished) {
+ /*
+ * SEV-ES guests require a separate (from the VMCB) VMSA page used to
+ * contain the encrypted register state of the guest.
+ */
+ vmsa_page = snp_safe_alloc_page();
+ if (!vmsa_page)
+ return -ENOMEM;
- svm->sev_es.vmsa = page_address(vmsa_page);
+ svm->sev_es.vmsa = page_address(vmsa_page);
+ }
vcpu->arch.guest_tsc_protected = snp_is_secure_tsc_enabled(vcpu->kvm);
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 01be93a53d07..59c328c13b2a 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -96,6 +96,7 @@ struct kvm_sev_info {
bool active; /* SEV enabled guest */
bool es_active; /* SEV-ES enabled guest */
bool need_init; /* waiting for SEV_INIT2 */
+ bool snp_finished; /* SNP guest measurement has been finalized */
unsigned int asid; /* ASID used for this guest */
unsigned int handle; /* SEV firmware handle */
int fd; /* SEV device fd */
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v3 23/26] x86/virt/tdx: Enable TDX Module runtime updates
From: Xu Yilun @ 2026-02-02 7:41 UTC (permalink / raw)
To: Chao Gao
Cc: linux-coco, linux-kernel, kvm, x86, reinette.chatre, ira.weiny,
kai.huang, dan.j.williams, sagis, vannapurve, paulmck,
nik.borisov, zhenzhong.duan, seanjc, rick.p.edgecombe, kas,
dave.hansen, vishal.l.verma, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin
In-Reply-To: <20260123145645.90444-24-chao.gao@intel.com>
> static inline bool tdx_supports_runtime_update(const struct tdx_sys_info *sysinfo)
> {
> - return false; /* To be enabled when kernel is ready */
> + return sysinfo->features.tdx_features0 & TDX_FEATURES0_TD_PRESERVING;
Frankly speak, I don't know why we need endless one-line wrappers like
this, we've already exposed all details of the tdx_sys_info to its
users.
But I'm still OK with them.
Reviewed-by: Xu Yilun <yilun.xu@linux.intel.com>
^ permalink raw reply
* Re: [PATCH v3 22/26] x86/virt/tdx: Update tdx_sysinfo and check features post-update
From: Xu Yilun @ 2026-02-02 7:33 UTC (permalink / raw)
To: Chao Gao
Cc: linux-coco, linux-kernel, kvm, x86, reinette.chatre, ira.weiny,
kai.huang, dan.j.williams, sagis, vannapurve, paulmck,
nik.borisov, zhenzhong.duan, seanjc, rick.p.edgecombe, kas,
dave.hansen, vishal.l.verma, Farrah Chen, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin
In-Reply-To: <20260123145645.90444-23-chao.gao@intel.com>
On Fri, Jan 23, 2026 at 06:55:30AM -0800, Chao Gao wrote:
> tdx_sysinfo contains all metadata of the active TDX module, including
> versions, supported features, and TDMR/TDCS/TDVPS information. These
> elements may change over updates. Blindly refreshing the entire tdx_sysinfo
> could disrupt running software, as it may subtly rely on the previous state
> unless proven otherwise.
>
> Adopt a conservative approach, like microcode updates, by only refreshing
> version information that does not affect functionality, while ignoring
> all other changes. This is acceptable as new modules are required to
> maintain backward compatibility.
>
> Any updates to metadata beyond versions should be justified and reviewed on
> a case-by-case basis.
>
> Note that preallocating a tdx_sys_info buffer before updates is to avoid
> having to handle -ENOMEM when updating tdx_sysinfo after a successful
> update.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Tested-by: Farrah Chen <farrah.chen@intel.com>
Reviewed-by: Xu Yilun <yilun.xu@linux.intel.com>
^ permalink raw reply
* Re: [PATCH v3 20/26] x86/virt/seamldr: Do TDX per-CPU initialization after updates
From: Xu Yilun @ 2026-02-02 7:32 UTC (permalink / raw)
To: Chao Gao
Cc: linux-coco, linux-kernel, kvm, x86, reinette.chatre, ira.weiny,
kai.huang, dan.j.williams, sagis, vannapurve, paulmck,
nik.borisov, zhenzhong.duan, seanjc, rick.p.edgecombe, kas,
dave.hansen, vishal.l.verma, Farrah Chen, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin
In-Reply-To: <20260123145645.90444-21-chao.gao@intel.com>
On Fri, Jan 23, 2026 at 06:55:28AM -0800, Chao Gao wrote:
> After installing the new TDX module, each CPU should be initialized
> again to make the CPU ready to run any other SEAMCALLs. So, call
> tdx_cpu_enable() on all CPUs.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Tested-by: Farrah Chen <farrah.chen@intel.com>
Reviewed-by: Xu Yilun <yilun.xu@linux.intel.com>
^ permalink raw reply
* Re: [PATCH v3 18/26] x86/virt/seamldr: Log TDX Module update failures
From: Xu Yilun @ 2026-02-02 7:11 UTC (permalink / raw)
To: Chao Gao
Cc: linux-coco, linux-kernel, kvm, x86, reinette.chatre, ira.weiny,
kai.huang, dan.j.williams, sagis, vannapurve, paulmck,
nik.borisov, zhenzhong.duan, seanjc, rick.p.edgecombe, kas,
dave.hansen, vishal.l.verma, Farrah Chen, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin
In-Reply-To: <20260123145645.90444-19-chao.gao@intel.com>
> +static void print_update_failure_message(void)
> +{
> + static atomic_t printed = ATOMIC_INIT(0);
> +
> + if (atomic_inc_return(&printed) == 1)
> + pr_err("update failed, SEAMCALLs will report failure until TDs killed\n");
> +}
Not sure why it can't be just pr_err_once()?
> +
> /*
> * See multi_cpu_stop() from where this multi-cpu state-machine was
> * adopted, and the rationale for touch_nmi_watchdog()
> @@ -289,10 +297,13 @@ static int do_seamldr_install_module(void *params)
> break;
> }
>
> - if (ret)
> + if (ret) {
> atomic_inc(&tdp_data.failed);
> - else
> + if (curstate > TDP_SHUTDOWN)
> + print_update_failure_message();
> + } else {
> ack_state();
> + }
> } else {
> touch_nmi_watchdog();
> rcu_momentary_eqs();
> --
> 2.47.3
>
^ permalink raw reply
* Re: [PATCH v3 16/26] x86/virt/seamldr: Shut down the current TDX module
From: Xu Yilun @ 2026-02-02 6:31 UTC (permalink / raw)
To: Chao Gao
Cc: linux-coco, linux-kernel, kvm, x86, reinette.chatre, ira.weiny,
kai.huang, dan.j.williams, sagis, vannapurve, paulmck,
nik.borisov, zhenzhong.duan, seanjc, rick.p.edgecombe, kas,
dave.hansen, vishal.l.verma, Farrah Chen, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin
In-Reply-To: <20260123145645.90444-17-chao.gao@intel.com>
> +static int get_tdx_sys_info_handoff(struct tdx_sys_info_handoff *sysinfo_handoff)
> +{
> + int ret = 0;
> + u64 val;
> +
> + if (tdx_supports_runtime_update(&tdx_sysinfo) &&
> + !(ret = read_sys_metadata_field(0x8900000100000000, &val)))
> + sysinfo_handoff->module_hv = val;
> +
> + return ret;
> +}
> +
> static int get_tdx_sys_info(struct tdx_sys_info *sysinfo)
> {
> int ret = 0;
> @@ -115,6 +127,7 @@ static int get_tdx_sys_info(struct tdx_sys_info *sysinfo)
> ret = ret ?: get_tdx_sys_info_tdmr(&sysinfo->tdmr);
> ret = ret ?: get_tdx_sys_info_td_ctrl(&sysinfo->td_ctrl);
> ret = ret ?: get_tdx_sys_info_td_conf(&sysinfo->td_conf);
> + ret = ret ?: get_tdx_sys_info_handoff(&sysinfo->handoff);
Do all fields in sysinfo_handoff optional or just module_hv, if the
former, is it better:
if (tdx_supports_runtime_update(&tdx_sysinfo)
ret = ret ?: get_tdx_sys_info_handoff(&sysinfo->handoff);
>
> return ret;
> }
> --
> 2.47.3
>
^ permalink raw reply
* Re: [PATCH v3 15/26] x86/virt/seamldr: Abort updates if errors occurred midway
From: Xu Yilun @ 2026-02-02 6:08 UTC (permalink / raw)
To: Chao Gao
Cc: linux-coco, linux-kernel, kvm, x86, reinette.chatre, ira.weiny,
kai.huang, dan.j.williams, sagis, vannapurve, paulmck,
nik.borisov, zhenzhong.duan, seanjc, rick.p.edgecombe, kas,
dave.hansen, vishal.l.verma, Farrah Chen, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin
In-Reply-To: <20260123145645.90444-16-chao.gao@intel.com>
On Fri, Jan 23, 2026 at 06:55:23AM -0800, Chao Gao wrote:
> The TDX Module update process has multiple stages, each of which may
> encounter failures.
>
> The current state machine of updates proceeds to the next stage
> regardless of errors. But continuing updates when errors occur midway
> is pointless.
>
> If a CPU encounters an error, abort the update by setting a flag and
> exiting the execution loop. Note that this CPU doesn't acknowledge the
> current stage. This will keep all other CPUs in the current stage until
> they see the flag and exit the loop as well.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Tested-by: Farrah Chen <farrah.chen@intel.com>
Reviewed-by: Xu Yilun <yilun.xu@linux.intel.com>
^ permalink raw reply
* Re: [PATCH v3 14/26] x86/virt/seamldr: Introduce skeleton for TDX Module updates
From: Xu Yilun @ 2026-02-02 6:01 UTC (permalink / raw)
To: Chao Gao
Cc: linux-coco, linux-kernel, kvm, x86, reinette.chatre, ira.weiny,
kai.huang, dan.j.williams, sagis, vannapurve, paulmck,
nik.borisov, zhenzhong.duan, seanjc, rick.p.edgecombe, kas,
dave.hansen, vishal.l.verma, Farrah Chen, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin
In-Reply-To: <20260123145645.90444-15-chao.gao@intel.com>
On Fri, Jan 23, 2026 at 06:55:22AM -0800, Chao Gao wrote:
> The P-SEAMLDR requires that no TDX Module SEAMCALLs are invoked during a
> runtime TDX Module update.
>
> But currently, TDX Module SEAMCALLs are invoked in various contexts and in
> parallel across CPUs. Additionally, considering the need to force all vCPUs
> out of guest mode, no single lock primitive, except for stop_machine(), can
> meet this requirement.
>
> Perform TDX Module updates within stop_machine() as it achieves the
> P-SEAMLDR requirements and is an existing well understood mechanism.
>
> TDX Module updates consist of several steps: shutting down the old
> module, installing the new module, and initializing the new one and etc.
> Some steps must be executed on a single CPU, others serially across all
> CPUs, and some can be performed concurrently on all CPUs and there are
> ordering requirements between steps. So, all CPUs need to perform the work
> in a step-locked manner.
>
> In preparation for adding concrete steps for TDX Module updates,
> establish the framework by mimicking multi_cpu_stop(). Specifically, use a
> global state machine to control the work done on each CPU and require all
> CPUs to acknowledge completion before proceeding to the next stage.
>
> Potential alternative to stop_machine()
> =======================================
> An alternative approach is to lock all KVM entry points and kick all
> vCPUs. Here, KVM entry points refer to KVM VM/vCPU ioctl entry points,
> implemented in KVM common code (virt/kvm). Adding a locking mechanism
> there would affect all architectures. And to lock only TDX vCPUs, new
> logic would be needed to identify TDX vCPUs, which the common code
> currently lacks. This would add significant complexity and maintenance
> overhead for a TDX-specific use case.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> Tested-by: Farrah Chen <farrah.chen@intel.com>
Reviewed-by: Xu Yilun <yilun.xu@linux.intel.com>
^ permalink raw reply
* Re: [PATCH v3 13/26] x86/virt/seamldr: Allocate and populate a module update request
From: Xu Yilun @ 2026-02-02 3:08 UTC (permalink / raw)
To: Chao Gao
Cc: linux-coco, linux-kernel, kvm, x86, reinette.chatre, ira.weiny,
kai.huang, dan.j.williams, sagis, vannapurve, paulmck,
nik.borisov, zhenzhong.duan, seanjc, rick.p.edgecombe, kas,
dave.hansen, vishal.l.verma, Farrah Chen, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin
In-Reply-To: <20260123145645.90444-14-chao.gao@intel.com>
> +static struct seamldr_params *init_seamldr_params(const u8 *data, u32 size)
> +{
> + const struct tdx_blob *blob = (const void *)data;
> + int module_size, sig_size;
> + const void *sig, *module;
You need to firstly check if size is big enough for the header before
offset into it.
if (size < sizeof(struct tdx_blob))
return XXX;
^ permalink raw reply
* Re: [PATCH v3 11/26] x86/virt/seamldr: Block TDX Module updates if any CPU is offline
From: Xu Yilun @ 2026-02-02 0:31 UTC (permalink / raw)
To: Chao Gao
Cc: linux-coco, linux-kernel, kvm, x86, reinette.chatre, ira.weiny,
kai.huang, dan.j.williams, sagis, vannapurve, paulmck,
nik.borisov, zhenzhong.duan, seanjc, rick.p.edgecombe, kas,
dave.hansen, vishal.l.verma, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin
In-Reply-To: <20260123145645.90444-12-chao.gao@intel.com>
On Fri, Jan 23, 2026 at 06:55:19AM -0800, Chao Gao wrote:
> P-SEAMLDR requires every CPU to call the SEAMLDR.INSTALL SEAMCALL during
> updates. So, every CPU should be online.
>
> Check if all CPUs are online and abort the update if any CPU is offline at
> the very beginning. Without this check, P-SEAMLDR will report failure at a
> later phase where the old TDX module is gone and TDs have to be killed.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
Reviewed-by: Xu Yilun <yilun.xu@linux.intel.com>
^ permalink raw reply
* Re: [PATCH v3 08/26] x86/virt/seamldr: Retrieve P-SEAMLDR information
From: Xu Yilun @ 2026-02-02 0:16 UTC (permalink / raw)
To: Dave Hansen
Cc: Chao Gao, linux-coco, linux-kernel, kvm, x86, reinette.chatre,
ira.weiny, kai.huang, dan.j.williams, sagis, vannapurve, paulmck,
nik.borisov, zhenzhong.duan, seanjc, rick.p.edgecombe, kas,
dave.hansen, vishal.l.verma, Farrah Chen, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, H. Peter Anvin
In-Reply-To: <1b1a2fe7-d225-414c-9055-8ad06938a0bf@intel.com>
> If nothing else, __packed is a good indicator that WYSIWYG for structure
> layout because it's an ABI. I honestly don't see a lot of downsides.
OK. So on x86 I can use it without worry. Thanks.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox