* Re: [RFC PATCH v5 20/45] KVM: x86/mmu: Allocate/free S-EPT pages using tdx_{alloc,free}_control_page()
From: Sean Christopherson @ 2026-02-09 23:20 UTC (permalink / raw)
To: Yan Zhao
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: <aYmoIaFwgR6+hnGp@yzhao56-desk.sh.intel.com>
On Mon, Feb 09, 2026, Yan Zhao wrote:
> On Fri, Feb 06, 2026 at 07:01:14AM -0800, Sean Christopherson wrote:
> > @@ -2348,7 +2348,7 @@ void __tdx_pamt_put(u64 pfn)
> > if (!atomic_dec_and_test(pamt_refcount))
> > return;
> >
> > - scoped_guard(spinlock, &pamt_lock) {
> > + scoped_guard(raw_spinlock_irqsave, &pamt_lock) {
> > /* Lost race with tdx_pamt_get(). */
> > if (atomic_read(pamt_refcount))
> > return;
>
> This option can get rid of the warning.
>
> However, given the pamt_lock is a global lock, which may be acquired even in the
> softirq context, not sure if this irq disabled version is good.
FWIW, the SEAMCALL itself disables IRQs (and everything else), so it's not _that_
big of a change. But yeah, waiting on the spinlock with IRQs disabled isn't
exactly idea.
> For your reference, I measured some test data by concurrently launching and
> destroying 4 TDs for 3 rounds:
>
> t0 ---------------------
> scoped_guard(spinlock, &pamt_lock) { |->T1=t1-t0 |
> t1 ---------- |
> ... |
> t2 ---------- |->T3=t4-t0
> tdh_phymem_pamt_add/remove() |->T2=t3-t2 |
> t3 ---------- |
> ... |
> t4 ---------------------
> }
>
> (1) for __tdx_pamt_get()
>
> avg us min us max us
> ------|---------------------------
> T1 | 4 0 69
> T2 | 4 2 18
> T3 | 10 3 83
>
>
> (2) for__tdx_pamt_put()
>
> avg us min us max us
> ------|---------------------------
> T1 | 0 0 5
> T2 | 2 1 11
> T3 | 3 2 15
>
>
> > Option #2 would be to immediately free the page in tdx_sept_reclaim_private_sp(),
> > so that pages that freed via handle_removed_pt() don't defer freeing the S-EPT
> > page table (which, IIUC, is safe since the TDX-Module forces TLB flushes and exits).
> >
> > I really, really don't like this option (if it even works).
> I don't like its asymmetry with tdx_sept_link_private_spt().
>
> However, do you think it would be good to have the PAMT pages of the sept pages
> allocated from (*topup_private_mapping_cache) [1]?
Hrm, dunno about "good", but it's definitely not terrible. To get the cache
management right, it means adding yet another use of kvm_get_running_vcpu(), which
I really dislike.
On the other hand, if we combine that with TDX freeing in-use S-EPT page tables,
unless I'm overly simplifying things, it would avoid having to extend
kvm_mmu_memory_cache with the page_{get,free}() hook, and would then eliminate
two kvm_x86_ops hooks, because the alloc/free of _unused_ S-EPT page tables is
no different than regular page tables.
As a bonus, we could keep the topup_external_cache() name and just clarify that
the parameter specifies the number of page table pages, i.e. account for the +1
for the mapping page in TDX code.
All in all, I'm kinda leaning in this direction, because as much as I dislike
kvm_get_running_vcpu(), it does minimize the number of kvm_x86_ops hooks.
Something like this? Also pushed to
https://github.com/sean-jc/linux.git x86/tdx_huge_sept_alt
if it doesn't apply.
---
arch/x86/include/asm/kvm-x86-ops.h | 6 +--
arch/x86/include/asm/kvm_host.h | 15 ++------
arch/x86/kvm/mmu/mmu.c | 3 --
arch/x86/kvm/mmu/tdp_mmu.c | 23 +++++++-----
arch/x86/kvm/vmx/tdx.c | 60 ++++++++++++++++++++----------
5 files changed, 61 insertions(+), 46 deletions(-)
diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
index 6083fb07cd3b..4b865617a421 100644
--- a/arch/x86/include/asm/kvm-x86-ops.h
+++ b/arch/x86/include/asm/kvm-x86-ops.h
@@ -94,11 +94,9 @@ 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(alloc_external_sp)
-KVM_X86_OP_OPTIONAL(free_external_sp)
-KVM_X86_OP_OPTIONAL_RET0(set_external_spte)
-KVM_X86_OP_OPTIONAL(reclaim_external_sp)
+KVM_X86_OP_OPTIONAL(reclaim_external_spt)
KVM_X86_OP_OPTIONAL_RET0(topup_external_cache)
+KVM_X86_OP_OPTIONAL_RET0(set_external_spte)
KVM_X86_OP(has_wbinvd_exit)
KVM_X86_OP(get_l2_tsc_offset)
KVM_X86_OP(get_l2_tsc_multiplier)
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index cd3e7dc6ab9b..d3c31eaf18b1 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1850,19 +1850,12 @@ struct kvm_x86_ops {
void (*load_mmu_pgd)(struct kvm_vcpu *vcpu, hpa_t root_hpa,
int root_level);
- /*
- * Callbacks to allocate and free external page tables, a.k.a. S-EPT,
- * and to propagate changes in mirror page tables to the external page
- * tables.
- */
- unsigned long (*alloc_external_sp)(gfp_t gfp);
- void (*free_external_sp)(unsigned long addr);
+ void (*reclaim_external_spt)(struct kvm *kvm, gfn_t gfn,
+ struct kvm_mmu_page *sp);
+ int (*topup_external_cache)(struct kvm *kvm, struct kvm_vcpu *vcpu,
+ int min_nr_spts);
int (*set_external_spte)(struct kvm *kvm, gfn_t gfn, u64 old_spte,
u64 new_spte, enum pg_level level);
- void (*reclaim_external_sp)(struct kvm *kvm, gfn_t gfn,
- struct kvm_mmu_page *sp);
- int (*topup_external_cache)(struct kvm *kvm, struct kvm_vcpu *vcpu, int min);
-
bool (*has_wbinvd_exit)(void);
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 62bf6bec2df2..f7cf456d9404 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -6714,9 +6714,6 @@ int kvm_mmu_create(struct kvm_vcpu *vcpu)
if (!vcpu->arch.mmu_shadow_page_cache.init_value)
vcpu->arch.mmu_shadow_page_cache.gfp_zero = __GFP_ZERO;
- vcpu->arch.mmu_external_spt_cache.page_get = kvm_x86_ops.alloc_external_sp;
- vcpu->arch.mmu_external_spt_cache.page_free = kvm_x86_ops.free_external_sp;
-
vcpu->arch.mmu = &vcpu->arch.root_mmu;
vcpu->arch.walk_mmu = &vcpu->arch.root_mmu;
diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index fef856323821..732548a678d8 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -53,14 +53,18 @@ void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm)
rcu_barrier();
}
-static void tdp_mmu_free_sp(struct kvm_mmu_page *sp)
+static void __tdp_mmu_free_sp(struct kvm_mmu_page *sp)
{
- if (sp->external_spt)
- kvm_x86_call(free_external_sp)((unsigned long)sp->external_spt);
free_page((unsigned long)sp->spt);
kmem_cache_free(mmu_page_header_cache, sp);
}
+static void tdp_mmu_free_unused_sp(struct kvm_mmu_page *sp)
+{
+ free_page((unsigned long)sp->external_spt);
+ __tdp_mmu_free_sp(sp);
+}
+
/*
* This is called through call_rcu in order to free TDP page table memory
* safely with respect to other kernel threads that may be operating on
@@ -74,7 +78,8 @@ static void tdp_mmu_free_sp_rcu_callback(struct rcu_head *head)
struct kvm_mmu_page *sp = container_of(head, struct kvm_mmu_page,
rcu_head);
- tdp_mmu_free_sp(sp);
+ WARN_ON_ONCE(sp->external_spt);
+ __tdp_mmu_free_sp(sp);
}
void kvm_tdp_mmu_put_root(struct kvm *kvm, struct kvm_mmu_page *root)
@@ -458,7 +463,7 @@ static void handle_removed_pt(struct kvm *kvm, tdp_ptep_t pt, bool shared)
}
if (is_mirror_sp(sp))
- kvm_x86_call(reclaim_external_sp)(kvm, base_gfn, sp);
+ kvm_x86_call(reclaim_external_spt)(kvm, base_gfn, sp);
call_rcu(&sp->rcu_head, tdp_mmu_free_sp_rcu_callback);
}
@@ -1266,7 +1271,7 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
* failed, e.g. because a different task modified the SPTE.
*/
if (r) {
- tdp_mmu_free_sp(sp);
+ tdp_mmu_free_unused_sp(sp);
goto retry;
}
@@ -1461,7 +1466,7 @@ static struct kvm_mmu_page *tdp_mmu_alloc_sp_for_split(struct kvm *kvm,
goto err_spt;
if (is_mirror_sp) {
- sp->external_spt = (void *)kvm_x86_call(alloc_external_sp)(GFP_KERNEL_ACCOUNT);
+ sp->external_spt = (void *)__get_free_page(GFP_KERNEL_ACCOUNT);
if (!sp->external_spt)
goto err_external_spt;
@@ -1472,7 +1477,7 @@ static struct kvm_mmu_page *tdp_mmu_alloc_sp_for_split(struct kvm *kvm,
return sp;
err_external_split:
- kvm_x86_call(free_external_sp)((unsigned long)sp->external_spt);
+ free_page((unsigned long)sp->external_spt);
err_external_spt:
free_page((unsigned long)sp->spt);
err_spt:
@@ -1594,7 +1599,7 @@ static int tdp_mmu_split_huge_pages_root(struct kvm *kvm,
* installs its own sp in place of the last sp we tried to split.
*/
if (sp)
- tdp_mmu_free_sp(sp);
+ tdp_mmu_free_unused_sp(sp);
return 0;
}
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index ae7b9beb3249..b0fc17baa1fc 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -1701,7 +1701,7 @@ static struct tdx_pamt_cache *tdx_get_pamt_cache(struct kvm *kvm,
}
static int tdx_topup_external_pamt_cache(struct kvm *kvm,
- struct kvm_vcpu *vcpu, int min)
+ struct kvm_vcpu *vcpu, int min_nr_spts)
{
struct tdx_pamt_cache *pamt_cache;
@@ -1712,7 +1712,11 @@ static int tdx_topup_external_pamt_cache(struct kvm *kvm,
if (!pamt_cache)
return -EIO;
- return tdx_topup_pamt_cache(pamt_cache, min);
+ /*
+ * Each S-EPT page tables requires a DPAMT pair, plus one more for the
+ * memory being mapped into the guest.
+ */
+ return tdx_topup_pamt_cache(pamt_cache, min_nr_spts + 1);
}
static int tdx_mem_page_add(struct kvm *kvm, gfn_t gfn, enum pg_level level,
@@ -1911,23 +1915,41 @@ static int tdx_sept_split_private_spte(struct kvm *kvm, gfn_t gfn, u64 old_spte,
static int tdx_sept_link_private_spt(struct kvm *kvm, gfn_t gfn, u64 new_spte,
enum pg_level level)
{
+ struct tdx_pamt_cache *pamt_cache;
gpa_t gpa = gfn_to_gpa(gfn);
u64 err, entry, level_state;
struct page *external_spt;
+ int r;
external_spt = tdx_spte_to_external_spt(kvm, gfn, new_spte, level);
if (!external_spt)
return -EIO;
+ pamt_cache = tdx_get_pamt_cache(kvm, kvm_get_running_vcpu());
+ if (!pamt_cache)
+ return -EIO;
+
+ r = tdx_pamt_get(page_to_pfn(external_spt), PG_LEVEL_4K, pamt_cache);
+ if (r)
+ return r;
+
err = tdh_mem_sept_add(&to_kvm_tdx(kvm)->td, gpa, level, external_spt,
&entry, &level_state);
- if (unlikely(IS_TDX_OPERAND_BUSY(err)))
- return -EBUSY;
+ if (unlikely(IS_TDX_OPERAND_BUSY(err))) {
+ r = -EBUSY;
+ goto err;
+ }
- if (TDX_BUG_ON_2(err, TDH_MEM_SEPT_ADD, entry, level_state, kvm))
- return -EIO;
+ if (TDX_BUG_ON_2(err, TDH_MEM_SEPT_ADD, entry, level_state, kvm)) {
+ r = -EIO;
+ goto err;
+ }
return 0;
+
+err:
+ tdx_pamt_put(page_to_pfn(external_spt), PG_LEVEL_4K);
+ return r;
}
static int tdx_sept_remove_private_spte(struct kvm *kvm, gfn_t gfn,
@@ -1995,8 +2017,8 @@ static int tdx_sept_set_private_spte(struct kvm *kvm, gfn_t gfn, u64 old_spte,
return tdx_sept_map_leaf_spte(kvm, gfn, new_spte, level);
}
-static void tdx_sept_reclaim_private_sp(struct kvm *kvm, gfn_t gfn,
- struct kvm_mmu_page *sp)
+static void tdx_sept_reclaim_private_spt(struct kvm *kvm, gfn_t gfn,
+ struct kvm_mmu_page *sp)
{
/*
* KVM doesn't (yet) zap page table pages in mirror page table while
@@ -2014,7 +2036,16 @@ static void tdx_sept_reclaim_private_sp(struct kvm *kvm, gfn_t gfn,
*/
if (KVM_BUG_ON(is_hkid_assigned(to_kvm_tdx(kvm)), kvm) ||
tdx_reclaim_page(virt_to_page(sp->external_spt)))
- sp->external_spt = NULL;
+ goto out;
+
+ /*
+ * Immediately free the S-EPT page as the TDX subsystem doesn't support
+ * freeing pages from RCU callbacks, and more importantly because
+ * TDH.PHYMEM.PAGE.RECLAIM ensures there are no outstanding readers.
+ */
+ tdx_free_control_page((unsigned long)sp->external_spt);
+out:
+ sp->external_spt = NULL;
}
void tdx_deliver_interrupt(struct kvm_lapic *apic, int delivery_mode,
@@ -3869,17 +3900,8 @@ void __init tdx_hardware_setup(void)
*/
vt_x86_ops.vm_size = max_t(unsigned int, vt_x86_ops.vm_size, sizeof(struct kvm_tdx));
- /*
- * TDX uses the external_spt cache to allocate S-EPT page table pages,
- * which (a) don't need to be initialized by KVM as the TDX-Module will
- * initialize the page (using the guest's encryption key), and (b) need
- * to use a custom allocator to be compatible with Dynamic PAMT.
- */
- vt_x86_ops.alloc_external_sp = tdx_alloc_control_page;
- vt_x86_ops.free_external_sp = tdx_free_control_page;
-
+ vt_x86_ops.reclaim_external_spt = tdx_sept_reclaim_private_spt;
vt_x86_ops.set_external_spte = tdx_sept_set_private_spte;
- vt_x86_ops.reclaim_external_sp = tdx_sept_reclaim_private_sp;
vt_x86_ops.gmem_convert = tdx_gmem_convert;
base-commit: 7adb9e428488cf7873a122043385a50dc1eebc8f
--
^ permalink raw reply related
* Re: [RFC PATCH v5 20/45] KVM: x86/mmu: Allocate/free S-EPT pages using tdx_{alloc,free}_control_page()
From: Dave Hansen @ 2026-02-09 23:40 UTC (permalink / raw)
To: Sean Christopherson, Yan Zhao
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: <aYYCOiMvWfSJR1AL@google.com>
On 2/6/26 07:01, Sean Christopherson wrote:
> /* Bump PAMT refcount for the given page and allocate PAMT memory if needed */
> int __tdx_pamt_get(u64 pfn, struct tdx_pamt_cache *cache)
> @@ -2272,7 +2272,7 @@ int __tdx_pamt_get(u64 pfn, struct tdx_pamt_cache *cache)
> if (ret)
> goto out_free;
>
> - scoped_guard(spinlock, &pamt_lock) {
> + scoped_guard(raw_spinlock_irqsave, &pamt_lock) {
> /*
> * Lost race to other tdx_pamt_add(). Other task has already allocated
> * PAMT memory for the HPA.
> @@ -2348,7 +2348,7 @@ void __tdx_pamt_put(u64 pfn)
Why does this need to be a raw spinlock? irqsave, sure, but raw?
The page allocator locks are used in this context and aren't raw.
^ permalink raw reply
* Re: [RFC PATCH v5 20/45] KVM: x86/mmu: Allocate/free S-EPT pages using tdx_{alloc,free}_control_page()
From: Sean Christopherson @ 2026-02-10 0:03 UTC (permalink / raw)
To: Dave Hansen
Cc: Yan Zhao, 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: <e81151ce-6e67-48e8-a722-da9ff03d686b@intel.com>
On Mon, Feb 09, 2026, Dave Hansen wrote:
> On 2/6/26 07:01, Sean Christopherson wrote:
> > /* Bump PAMT refcount for the given page and allocate PAMT memory if needed */
> > int __tdx_pamt_get(u64 pfn, struct tdx_pamt_cache *cache)
> > @@ -2272,7 +2272,7 @@ int __tdx_pamt_get(u64 pfn, struct tdx_pamt_cache *cache)
> > if (ret)
> > goto out_free;
> >
> > - scoped_guard(spinlock, &pamt_lock) {
> > + scoped_guard(raw_spinlock_irqsave, &pamt_lock) {
> > /*
> > * Lost race to other tdx_pamt_add(). Other task has already allocated
> > * PAMT memory for the HPA.
> > @@ -2348,7 +2348,7 @@ void __tdx_pamt_put(u64 pfn)
>
> Why does this need to be a raw spinlock? irqsave, sure, but raw?
Huh, TIL. (And just when I thought I finally had my head wrapped around RT "spinlocks"):
The hard interrupt related suffixes for spin_lock / spin_unlock operations
(_irq, _irqsave / _irqrestore) do not affect the CPU’s interrupt disabled state.
Ah, and running RCU callbacks from soft IRQ context is straight up disallowed for
PREEMPT_RT.
/* By default, use RCU_SOFTIRQ instead of rcuc kthreads. */
static bool use_softirq = !IS_ENABLED(CONFIG_PREEMPT_RT);
#ifndef CONFIG_PREEMPT_RT
module_param(use_softirq, bool, 0444);
#endif
So yeah, just spinlock_irqsave should be fine. Though the way things are trending,
it'll be a moot point if KVM ends up freeing S-EPT page tables from task context.
> The page allocator locks are used in this context and aren't raw.
^ permalink raw reply
* Re: [RFC PATCH v5 20/45] KVM: x86/mmu: Allocate/free S-EPT pages using tdx_{alloc,free}_control_page()
From: Dave Hansen @ 2026-02-10 0:07 UTC (permalink / raw)
To: Yan Zhao, 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: <aYmoIaFwgR6+hnGp@yzhao56-desk.sh.intel.com>
On 2/9/26 01:25, Yan Zhao wrote:
> However, given the pamt_lock is a global lock, which may be acquired
> even in the softirq context, not sure if this irq disabled version
> is good.
Generally, we try to avoid crap that's not scalable because it's hard to
retrofit. But in this case, I'm just not sure how much of a bottleneck
this lock is going to be in the real world.
Let's be honest: starting and shutting down VMs in a loop doesn't mint
money for cloud providers like running VMs does, so it's not exactly a
real-world thing.
That said, if this global lock _actually_ ever starts to bite anyone for
real, it's not going to be rocket science to turn the single lock into 5
or 10 or NR_CPUs, or whatever. So I think we can just keep it as-is and
avert our eyes for the time being.
^ permalink raw reply
* Re: [PATCH 4/5] dma-buf: heaps: allow heap to specify valid heap flags
From: Jason Gunthorpe @ 2026-02-10 0:29 UTC (permalink / raw)
To: John Stultz
Cc: Jiri Pirko, dri-devel, linaro-mm-sig, iommu, linux-media,
sumit.semwal, benjamin.gaignard, Brian.Starkey, tjmercier,
christian.koenig, m.szyprowski, robin.murphy, leon, sean.anderson,
ptesarik, catalin.marinas, aneesh.kumar, suzuki.poulose,
steven.price, thomas.lendacky, john.allen, ashish.kalra,
suravee.suthikulpanit, linux-coco
In-Reply-To: <CANDhNCoHEZsNRmU+3z5AbeAy05H7PTtUdTq1apNd5k0f9hWW8A@mail.gmail.com>
On Mon, Feb 09, 2026 at 12:08:03PM -0800, John Stultz wrote:
> On Mon, Feb 9, 2026 at 7:38 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >
> > From: Jiri Pirko <jiri@nvidia.com>
> >
> > Currently the flags, which are unused, are validated for all heaps.
> > Since the follow-up patch introduces a flag valid for only one of the
> > heaps, allow to specify the valid flags per-heap.
>
> I'm not really in this space anymore, so take my feedback with a grain of salt.
>
> While the heap allocate flags argument is unused, it was intended to
> be used for generic allocation flags that would apply to all or at
> least a wide majority of heaps.
>
> It was definitely not added to allow for per-heap or heap specific
> flags (as this patch tries to utilize it). That was the mess we had
> with ION driver that we were trying to avoid.
I don't know alot about DMA heaps..
On a CC VM system the shared/private property is universal and applies
to every physical address. Not every address can dynamically change
between shared and private, but every address does have a
shared/private state.
By default userspace process generally run exclusively in private
memory and there are very few ways for userspace to even access shared
memory.
From a heaps perspective the API would be very strange, and perhaps
even security dangerous, if it is returning shared memory to userspace
without userspace knowing this is happening.
I'd advocate that the right design is for userspace to positively
signal via this flag that it wants/accepts shared memory and without
the flag shared memory should never be returned.
Even if the underyling heap only has shared memory in it (eg it is
mmio or something).
Otherwise making it implicit, perhaps based on heap name, sounds very
tricky for userspace to actually use fully securely.
Again, I don't know alot about heaps, but perhaps the missing part
here is that on a CC system all existing heaps, other than the one
using normal system pages, should be disabled for now. They can come
back once they are audited as to their shared/private state and
respect the new flag.
Another view is to ignore this affirmative handshake and just make it
implicit on something like the heap name and hope userspace lucks into
something that works for it, and doesn't accidently place, or become
tricked into placing, sensitive information into shared heap memory.
Again I know nothing about heaps, but this is a fuller picture of the
security sensitivity and what to think about with heaps and CC VM
systems.
> Now, there has been many discussions around "protected buffers" (which
> doesn't seem to map exactly to this confidental computing primitive,
> but sounds like it might be related)
I'm not sure what protected buffers are, but this CC VM shared/private
(or encrypted/decrypted) is a core kernel property that applies to
every physical address in the CC VM.
I assume protected buffers are something more platform specific and
hidden?
> But, it seems like the use case here is still far too narrow for a top
> level allocation flag.
CC certainly is a narrow use case, but within CC I don't think it is
narrow at all..
Jason
^ permalink raw reply
* Re: [RFC PATCH v5 20/45] KVM: x86/mmu: Allocate/free S-EPT pages using tdx_{alloc,free}_control_page()
From: Yan Zhao @ 2026-02-10 1:40 UTC (permalink / raw)
To: Dave Hansen
Cc: Sean Christopherson, 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: <6a5e3e9f-69b2-4416-9465-92a859034391@intel.com>
On Mon, Feb 09, 2026 at 04:07:08PM -0800, Dave Hansen wrote:
> On 2/9/26 01:25, Yan Zhao wrote:
> > However, given the pamt_lock is a global lock, which may be acquired
> > even in the softirq context, not sure if this irq disabled version
> > is good.
>
> Generally, we try to avoid crap that's not scalable because it's hard to
> retrofit. But in this case, I'm just not sure how much of a bottleneck
> this lock is going to be in the real world.
>
> Let's be honest: starting and shutting down VMs in a loop doesn't mint
> money for cloud providers like running VMs does, so it's not exactly a
> real-world thing.
>
> That said, if this global lock _actually_ ever starts to bite anyone for
> real, it's not going to be rocket science to turn the single lock into 5
> or 10 or NR_CPUs, or whatever. So I think we can just keep it as-is and
> avert our eyes for the time being.
Hmm. One clarification: I'm not concerned about the global spinlock. My
concern is the attempt in the #1 solution [1] to turn off irq before acquiring
spinlock (spin_lock_irqsave()) to address the deadlock issue reported in [2].
[1] https://lore.kernel.org/all/aYYCOiMvWfSJR1AL@google.com/
[2] https://lore.kernel.org/all/aYW5CbUvZrLogsWF@yzhao56-desk.sh.intel.com/
^ permalink raw reply
* Re: [RFC PATCH v5 20/45] KVM: x86/mmu: Allocate/free S-EPT pages using tdx_{alloc,free}_control_page()
From: Yan Zhao @ 2026-02-10 8:30 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: <aYprxnSHKHUtk7pt@google.com>
On Mon, Feb 09, 2026 at 03:20:38PM -0800, Sean Christopherson wrote:
> On Mon, Feb 09, 2026, Yan Zhao wrote:
> > On Fri, Feb 06, 2026 at 07:01:14AM -0800, Sean Christopherson wrote:
> > > @@ -2348,7 +2348,7 @@ void __tdx_pamt_put(u64 pfn)
> > > if (!atomic_dec_and_test(pamt_refcount))
> > > return;
> > >
> > > - scoped_guard(spinlock, &pamt_lock) {
> > > + scoped_guard(raw_spinlock_irqsave, &pamt_lock) {
> > > /* Lost race with tdx_pamt_get(). */
> > > if (atomic_read(pamt_refcount))
> > > return;
> >
> > This option can get rid of the warning.
> >
> > However, given the pamt_lock is a global lock, which may be acquired even in the
> > softirq context, not sure if this irq disabled version is good.
>
> FWIW, the SEAMCALL itself disables IRQs (and everything else), so it's not _that_
> big of a change. But yeah, waiting on the spinlock with IRQs disabled isn't
> exactly idea.
Right. Though the SEAMCALL itself disables IRQs (which is no more than 18us from
my measurement), the time spent waiting for acquiring the spinlock with IRQs
disabled may scale with the number of contending threads. e.g.
When there're 4 threads trying to acquire the spinlock, the most unlucky thread
needs to wait with IRQs disabled for 3x18us=54us in the worst case.
> > For your reference, I measured some test data by concurrently launching and
> > destroying 4 TDs for 3 rounds:
> >
> > t0 ---------------------
> > scoped_guard(spinlock, &pamt_lock) { |->T1=t1-t0 |
> > t1 ---------- |
> > ... |
> > t2 ---------- |->T3=t4-t0
> > tdh_phymem_pamt_add/remove() |->T2=t3-t2 |
> > t3 ---------- |
> > ... |
> > t4 ---------------------
> > }
> >
> > (1) for __tdx_pamt_get()
> >
> > avg us min us max us
> > ------|---------------------------
> > T1 | 4 0 69
> > T2 | 4 2 18
> > T3 | 10 3 83
> >
> >
> > (2) for__tdx_pamt_put()
> >
> > avg us min us max us
> > ------|---------------------------
> > T1 | 0 0 5
> > T2 | 2 1 11
> > T3 | 3 2 15
> >
> >
> > > Option #2 would be to immediately free the page in tdx_sept_reclaim_private_sp(),
> > > so that pages that freed via handle_removed_pt() don't defer freeing the S-EPT
> > > page table (which, IIUC, is safe since the TDX-Module forces TLB flushes and exits).
> > >
> > > I really, really don't like this option (if it even works).
> > I don't like its asymmetry with tdx_sept_link_private_spt().
> >
> > However, do you think it would be good to have the PAMT pages of the sept pages
> > allocated from (*topup_private_mapping_cache) [1]?
>
> Hrm, dunno about "good", but it's definitely not terrible. To get the cache
> management right, it means adding yet another use of kvm_get_running_vcpu(), which
> I really dislike.
>
> On the other hand, if we combine that with TDX freeing in-use S-EPT page tables,
> unless I'm overly simplifying things, it would avoid having to extend
> kvm_mmu_memory_cache with the page_{get,free}() hook, and would then eliminate
> two kvm_x86_ops hooks, because the alloc/free of _unused_ S-EPT page tables is
> no different than regular page tables.
>
> As a bonus, we could keep the topup_external_cache() name and just clarify that
> the parameter specifies the number of page table pages, i.e. account for the +1
> for the mapping page in TDX code.
>
> All in all, I'm kinda leaning in this direction, because as much as I dislike
> kvm_get_running_vcpu(), it does minimize the number of kvm_x86_ops hooks.
>
> Something like this? Also pushed to
>
> https://github.com/sean-jc/linux.git x86/tdx_huge_sept_alt
>
It lacks the following change in tdx_sept_split_private_spte().
@@ -1836,46 +1841,70 @@ static int tdx_sept_split_private_spte(struct kvm *kvm, gfn_t gfn, u64 old_spte,
if (!pamt_cache)
return -EIO;
+ r = tdx_pamt_get(page_to_pfn(external_spt), PG_LEVEL_4K, pamt_cache);
+ if (r)
+ return r;
+
err = tdh_do_no_vcpus(tdh_mem_range_block, kvm, &kvm_tdx->td, gpa,
level, &entry, &level_state);
- if (TDX_BUG_ON_2(err, TDH_MEM_RANGE_BLOCK, entry, level_state, kvm))
- return -EIO;
+ if (TDX_BUG_ON_2(err, TDH_MEM_RANGE_BLOCK, entry, level_state, kvm)) {
+ r = -EIO;
+ goto err;
+ }
tdx_track(kvm);
err = tdh_do_no_vcpus(tdh_mem_page_demote, kvm, &kvm_tdx->td, gpa,
level, spte_to_pfn(old_spte), external_spt,
pamt_cache, &entry, &level_state);
- if (TDX_BUG_ON_2(err, TDH_MEM_PAGE_DEMOTE, entry, level_state, kvm))
- return -EIO;
+ if (TDX_BUG_ON_2(err, TDH_MEM_PAGE_DEMOTE, entry, level_state, kvm)) {
+ r = -EIO;
+ goto err;
+ }
return 0;
+err:
+ tdx_pamt_put(page_to_pfn(external_spt), PG_LEVEL_4K);
+ return r;
}
Otherwise, LGTM except for the nits below.
> ---
> arch/x86/include/asm/kvm-x86-ops.h | 6 +--
> arch/x86/include/asm/kvm_host.h | 15 ++------
> arch/x86/kvm/mmu/mmu.c | 3 --
> arch/x86/kvm/mmu/tdp_mmu.c | 23 +++++++-----
> arch/x86/kvm/vmx/tdx.c | 60 ++++++++++++++++++++----------
> 5 files changed, 61 insertions(+), 46 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
> index 6083fb07cd3b..4b865617a421 100644
> --- a/arch/x86/include/asm/kvm-x86-ops.h
> +++ b/arch/x86/include/asm/kvm-x86-ops.h
> @@ -94,11 +94,9 @@ 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(alloc_external_sp)
> -KVM_X86_OP_OPTIONAL(free_external_sp)
> -KVM_X86_OP_OPTIONAL_RET0(set_external_spte)
> -KVM_X86_OP_OPTIONAL(reclaim_external_sp)
> +KVM_X86_OP_OPTIONAL(reclaim_external_spt)
> KVM_X86_OP_OPTIONAL_RET0(topup_external_cache)
> +KVM_X86_OP_OPTIONAL_RET0(set_external_spte)
> KVM_X86_OP(has_wbinvd_exit)
> KVM_X86_OP(get_l2_tsc_offset)
> KVM_X86_OP(get_l2_tsc_multiplier)
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index cd3e7dc6ab9b..d3c31eaf18b1 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1850,19 +1850,12 @@ struct kvm_x86_ops {
> void (*load_mmu_pgd)(struct kvm_vcpu *vcpu, hpa_t root_hpa,
> int root_level);
>
> - /*
> - * Callbacks to allocate and free external page tables, a.k.a. S-EPT,
> - * and to propagate changes in mirror page tables to the external page
> - * tables.
> - */
> - unsigned long (*alloc_external_sp)(gfp_t gfp);
> - void (*free_external_sp)(unsigned long addr);
> + void (*reclaim_external_spt)(struct kvm *kvm, gfn_t gfn,
> + struct kvm_mmu_page *sp);
> + int (*topup_external_cache)(struct kvm *kvm, struct kvm_vcpu *vcpu,
> + int min_nr_spts);
> int (*set_external_spte)(struct kvm *kvm, gfn_t gfn, u64 old_spte,
> u64 new_spte, enum pg_level level);
> - void (*reclaim_external_sp)(struct kvm *kvm, gfn_t gfn,
> - struct kvm_mmu_page *sp);
> - int (*topup_external_cache)(struct kvm *kvm, struct kvm_vcpu *vcpu, int min);
> -
>
> bool (*has_wbinvd_exit)(void);
>
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 62bf6bec2df2..f7cf456d9404 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -6714,9 +6714,6 @@ int kvm_mmu_create(struct kvm_vcpu *vcpu)
> if (!vcpu->arch.mmu_shadow_page_cache.init_value)
> vcpu->arch.mmu_shadow_page_cache.gfp_zero = __GFP_ZERO;
>
> - vcpu->arch.mmu_external_spt_cache.page_get = kvm_x86_ops.alloc_external_sp;
> - vcpu->arch.mmu_external_spt_cache.page_free = kvm_x86_ops.free_external_sp;
> -
> vcpu->arch.mmu = &vcpu->arch.root_mmu;
> vcpu->arch.walk_mmu = &vcpu->arch.root_mmu;
>
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index fef856323821..732548a678d8 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -53,14 +53,18 @@ void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm)
> rcu_barrier();
> }
>
> -static void tdp_mmu_free_sp(struct kvm_mmu_page *sp)
> +static void __tdp_mmu_free_sp(struct kvm_mmu_page *sp)
> {
> - if (sp->external_spt)
> - kvm_x86_call(free_external_sp)((unsigned long)sp->external_spt);
> free_page((unsigned long)sp->spt);
> kmem_cache_free(mmu_page_header_cache, sp);
> }
>
> +static void tdp_mmu_free_unused_sp(struct kvm_mmu_page *sp)
> +{
> + free_page((unsigned long)sp->external_spt);
> + __tdp_mmu_free_sp(sp);
> +}
> +
> /*
> * This is called through call_rcu in order to free TDP page table memory
> * safely with respect to other kernel threads that may be operating on
> @@ -74,7 +78,8 @@ static void tdp_mmu_free_sp_rcu_callback(struct rcu_head *head)
> struct kvm_mmu_page *sp = container_of(head, struct kvm_mmu_page,
> rcu_head);
>
> - tdp_mmu_free_sp(sp);
> + WARN_ON_ONCE(sp->external_spt);
> + __tdp_mmu_free_sp(sp);
> }
>
> void kvm_tdp_mmu_put_root(struct kvm *kvm, struct kvm_mmu_page *root)
> @@ -458,7 +463,7 @@ static void handle_removed_pt(struct kvm *kvm, tdp_ptep_t pt, bool shared)
> }
>
> if (is_mirror_sp(sp))
> - kvm_x86_call(reclaim_external_sp)(kvm, base_gfn, sp);
> + kvm_x86_call(reclaim_external_spt)(kvm, base_gfn, sp);
>
> call_rcu(&sp->rcu_head, tdp_mmu_free_sp_rcu_callback);
> }
> @@ -1266,7 +1271,7 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
> * failed, e.g. because a different task modified the SPTE.
> */
> if (r) {
> - tdp_mmu_free_sp(sp);
> + tdp_mmu_free_unused_sp(sp);
> goto retry;
> }
>
> @@ -1461,7 +1466,7 @@ static struct kvm_mmu_page *tdp_mmu_alloc_sp_for_split(struct kvm *kvm,
> goto err_spt;
>
> if (is_mirror_sp) {
> - sp->external_spt = (void *)kvm_x86_call(alloc_external_sp)(GFP_KERNEL_ACCOUNT);
> + sp->external_spt = (void *)__get_free_page(GFP_KERNEL_ACCOUNT);
> if (!sp->external_spt)
> goto err_external_spt;
>
> @@ -1472,7 +1477,7 @@ static struct kvm_mmu_page *tdp_mmu_alloc_sp_for_split(struct kvm *kvm,
> return sp;
>
> err_external_split:
> - kvm_x86_call(free_external_sp)((unsigned long)sp->external_spt);
> + free_page((unsigned long)sp->external_spt);
> err_external_spt:
> free_page((unsigned long)sp->spt);
> err_spt:
> @@ -1594,7 +1599,7 @@ static int tdp_mmu_split_huge_pages_root(struct kvm *kvm,
> * installs its own sp in place of the last sp we tried to split.
> */
> if (sp)
> - tdp_mmu_free_sp(sp);
> + tdp_mmu_free_unused_sp(sp);
>
> return 0;
> }
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index ae7b9beb3249..b0fc17baa1fc 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -1701,7 +1701,7 @@ static struct tdx_pamt_cache *tdx_get_pamt_cache(struct kvm *kvm,
> }
>
> static int tdx_topup_external_pamt_cache(struct kvm *kvm,
> - struct kvm_vcpu *vcpu, int min)
> + struct kvm_vcpu *vcpu, int min_nr_spts)
> {
> struct tdx_pamt_cache *pamt_cache;
>
> @@ -1712,7 +1712,11 @@ static int tdx_topup_external_pamt_cache(struct kvm *kvm,
> if (!pamt_cache)
> return -EIO;
>
> - return tdx_topup_pamt_cache(pamt_cache, min);
> + /*
> + * Each S-EPT page tables requires a DPAMT pair, plus one more for the
> + * memory being mapped into the guest.
> + */
> + return tdx_topup_pamt_cache(pamt_cache, min_nr_spts + 1);
Nit:
S-EPT root page is a control page and it has no corresponding sp->external_spt.
So, do you think it would be good to check the root level?
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index ae8b8438ae99..fff05052de27 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -1643,16 +1643,23 @@ static struct tdx_pamt_cache *tdx_get_pamt_cache(struct kvm *kvm,
static int tdx_topup_external_pamt_cache(struct kvm *kvm,
struct kvm_vcpu *vcpu, int min_nr_spts)
{
+ int root_level = (kvm_gfn_direct_bits(kvm) == TDX_SHARED_BIT_PWL_5) ? 5 :4;
struct tdx_pamt_cache *pamt_cache;
if (!tdx_supports_dynamic_pamt(tdx_sysinfo))
return 0;
pamt_cache = tdx_get_pamt_cache(kvm, vcpu);
if (!pamt_cache)
return -EIO;
+ /*
+ * S-EPT root page is one of tdcs_pages whose PAMT pages have been installed in
+ * __tdx_td_init().
+ */
+ if (min_nr_spts == root_level)
+ min_nr_spts--;
+
/*
* Each S-EPT page tables requires a DPAMT pair, plus one more for the
* memory being mapped into the guest.
> }
>
> static int tdx_mem_page_add(struct kvm *kvm, gfn_t gfn, enum pg_level level,
> @@ -1911,23 +1915,41 @@ static int tdx_sept_split_private_spte(struct kvm *kvm, gfn_t gfn, u64 old_spte,
> static int tdx_sept_link_private_spt(struct kvm *kvm, gfn_t gfn, u64 new_spte,
> enum pg_level level)
> {
> + struct tdx_pamt_cache *pamt_cache;
> gpa_t gpa = gfn_to_gpa(gfn);
> u64 err, entry, level_state;
> struct page *external_spt;
> + int r;
>
> external_spt = tdx_spte_to_external_spt(kvm, gfn, new_spte, level);
> if (!external_spt)
> return -EIO;
>
> + pamt_cache = tdx_get_pamt_cache(kvm, kvm_get_running_vcpu());
> + if (!pamt_cache)
> + return -EIO;
> +
> + r = tdx_pamt_get(page_to_pfn(external_spt), PG_LEVEL_4K, pamt_cache);
> + if (r)
> + return r;
> +
> err = tdh_mem_sept_add(&to_kvm_tdx(kvm)->td, gpa, level, external_spt,
> &entry, &level_state);
> - if (unlikely(IS_TDX_OPERAND_BUSY(err)))
> - return -EBUSY;
> + if (unlikely(IS_TDX_OPERAND_BUSY(err))) {
> + r = -EBUSY;
> + goto err;
> + }
>
> - if (TDX_BUG_ON_2(err, TDH_MEM_SEPT_ADD, entry, level_state, kvm))
> - return -EIO;
> + if (TDX_BUG_ON_2(err, TDH_MEM_SEPT_ADD, entry, level_state, kvm)) {
> + r = -EIO;
> + goto err;
> + }
>
> return 0;
> +
> +err:
> + tdx_pamt_put(page_to_pfn(external_spt), PG_LEVEL_4K);
> + return r;
> }
>
> static int tdx_sept_remove_private_spte(struct kvm *kvm, gfn_t gfn,
> @@ -1995,8 +2017,8 @@ static int tdx_sept_set_private_spte(struct kvm *kvm, gfn_t gfn, u64 old_spte,
> return tdx_sept_map_leaf_spte(kvm, gfn, new_spte, level);
> }
>
> -static void tdx_sept_reclaim_private_sp(struct kvm *kvm, gfn_t gfn,
> - struct kvm_mmu_page *sp)
> +static void tdx_sept_reclaim_private_spt(struct kvm *kvm, gfn_t gfn,
> + struct kvm_mmu_page *sp)
> {
> /*
> * KVM doesn't (yet) zap page table pages in mirror page table while
> @@ -2014,7 +2036,16 @@ static void tdx_sept_reclaim_private_sp(struct kvm *kvm, gfn_t gfn,
> */
> if (KVM_BUG_ON(is_hkid_assigned(to_kvm_tdx(kvm)), kvm) ||
> tdx_reclaim_page(virt_to_page(sp->external_spt)))
> - sp->external_spt = NULL;
> + goto out;
> +
> + /*
> + * Immediately free the S-EPT page as the TDX subsystem doesn't support
> + * freeing pages from RCU callbacks, and more importantly because
> + * TDH.PHYMEM.PAGE.RECLAIM ensures there are no outstanding readers.
> + */
> + tdx_free_control_page((unsigned long)sp->external_spt);
This creates another asymmetry, where there's nowhere to invoke
tdx_alloc_control_page() for the sp->external_spt.
Calling tdx_free_control_page() here could be confusing because:
- tdx_sept_reclaim_private_spt() is called only for non-root sps, whose
sp->external_spt is not allocated via tdx_alloc_control_page().
- The S-EPT root page is allocated via __tdx_alloc_control_page() by
__tdx_td_init(), but has no corresponding sp->external_spt.
So, could we just invoke
"__tdx_pamt_put(page_to_pfn(virt_to_page(sp->external_spt)))" in
tdx_sept_reclaim_private_sp()?
After tdx_sept_reclaim_private_spt() returns, sp goes back to unused by the
external page table. So, TDP MMU can invoke tdp_mmu_free_sp() without needing to
differentiate whether it's unused or not.
Something like below?
diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 732548a678d8..d621e94d73c2 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -53,18 +53,15 @@ void kvm_mmu_uninit_tdp_mmu(struct kvm *kvm)
rcu_barrier();
}
-static void __tdp_mmu_free_sp(struct kvm_mmu_page *sp)
+static void tdp_mmu_free_sp(struct kvm_mmu_page *sp)
{
+ free_page((unsigned long)sp->external_spt);
free_page((unsigned long)sp->spt);
kmem_cache_free(mmu_page_header_cache, sp);
}
-static void tdp_mmu_free_unused_sp(struct kvm_mmu_page *sp)
-{
- free_page((unsigned long)sp->external_spt);
- __tdp_mmu_free_sp(sp);
-}
-
/*
* This is called through call_rcu in order to free TDP page table memory
* safely with respect to other kernel threads that may be operating on
@@ -78,8 +75,7 @@ static void tdp_mmu_free_sp_rcu_callback(struct rcu_head *head)
struct kvm_mmu_page *sp = container_of(head, struct kvm_mmu_page,
rcu_head);
- WARN_ON_ONCE(sp->external_spt);
- __tdp_mmu_free_sp(sp);
+ tdp_mmu_free_sp(sp);
}
void kvm_tdp_mmu_put_root(struct kvm *kvm, struct kvm_mmu_page *root)
@@ -1271,7 +1267,7 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
* failed, e.g. because a different task modified the SPTE.
*/
if (r) {
- tdp_mmu_free_unused_sp(sp);
+ tdp_mmu_free_sp(sp);
goto retry;
}
@@ -1599,7 +1595,7 @@ static int tdp_mmu_split_huge_pages_root(struct kvm *kvm,
* installs its own sp in place of the last sp we tried to split.
*/
if (sp)
- tdp_mmu_free_unused_sp(sp);
+ tdp_mmu_free_sp(sp);
return 0;
}
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index b0fc17baa1fc..fbaf43b8cd46 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -2035,17 +2035,12 @@ static void tdx_sept_reclaim_private_spt(struct kvm *kvm, gfn_t gfn,
* removal of the still-used PAMT entry.
*/
if (KVM_BUG_ON(is_hkid_assigned(to_kvm_tdx(kvm)), kvm) ||
- tdx_reclaim_page(virt_to_page(sp->external_spt)))
- goto out;
+ tdx_reclaim_page(virt_to_page(sp->external_spt))) {
+ sp->external_spt = NULL;
+ return;
+ }
- /*
- * Immediately free the S-EPT page as the TDX subsystem doesn't support
- * freeing pages from RCU callbacks, and more importantly because
- * TDH.PHYMEM.PAGE.RECLAIM ensures there are no outstanding readers.
- */
- tdx_free_control_page((unsigned long)sp->external_spt);
-out:
- sp->external_spt = NULL;
+ __tdx_pamt_put(page_to_pfn(virt_to_page(sp->external_spt)));
}
void tdx_deliver_interrupt(struct kvm_lapic *apic, int delivery_mode,
^ permalink raw reply related
* Re: [PATCH 4/5] dma-buf: heaps: allow heap to specify valid heap flags
From: Jiri Pirko @ 2026-02-10 9:05 UTC (permalink / raw)
To: John Stultz
Cc: dri-devel, linaro-mm-sig, iommu, linux-media, sumit.semwal,
benjamin.gaignard, Brian.Starkey, tjmercier, christian.koenig,
m.szyprowski, robin.murphy, jgg, leon, sean.anderson, ptesarik,
catalin.marinas, aneesh.kumar, suzuki.poulose, steven.price,
thomas.lendacky, john.allen, ashish.kalra, suravee.suthikulpanit,
linux-coco
In-Reply-To: <CANDhNCoHEZsNRmU+3z5AbeAy05H7PTtUdTq1apNd5k0f9hWW8A@mail.gmail.com>
Mon, Feb 09, 2026 at 09:08:03PM +0100, jstultz@google.com wrote:
>On Mon, Feb 9, 2026 at 7:38 AM Jiri Pirko <jiri@resnulli.us> wrote:
>>
>> From: Jiri Pirko <jiri@nvidia.com>
>>
>> Currently the flags, which are unused, are validated for all heaps.
>> Since the follow-up patch introduces a flag valid for only one of the
>> heaps, allow to specify the valid flags per-heap.
>
>I'm not really in this space anymore, so take my feedback with a grain of salt.
>
>While the heap allocate flags argument is unused, it was intended to
>be used for generic allocation flags that would apply to all or at
>least a wide majority of heaps.
>
>It was definitely not added to allow for per-heap or heap specific
>flags (as this patch tries to utilize it). That was the mess we had
>with ION driver that we were trying to avoid.
>
>The intent of dma-buf heaps is to try to abstract all the different
>device memory constraints so there only needs to be a [usage] ->
>[heap] mapping, and otherwise userland can be generalized so that it
>doesn't need to be re-written to work with different devices/memory
>types. Adding heap-specific allocation flags prevents that
>generalization.
>
>So instead of adding heap specific flags, the general advice has been
>to add a separate heap name for the flag property.
Right, my original idea was to add a separate heap. Then I spotted the
flags and seemed like a great fit. Was not aware or the history or
original intention. Would be probably good to document it for
future generations.
So instead of flag, I will add heap named something
like "system_cc_decrypted" to implement this.
Thanks!
^ permalink raw reply
* Re: [PATCH 4/5] dma-buf: heaps: allow heap to specify valid heap flags
From: Jiri Pirko @ 2026-02-10 9:14 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: John Stultz, dri-devel, linaro-mm-sig, iommu, linux-media,
sumit.semwal, benjamin.gaignard, Brian.Starkey, tjmercier,
christian.koenig, m.szyprowski, robin.murphy, leon, sean.anderson,
ptesarik, catalin.marinas, aneesh.kumar, suzuki.poulose,
steven.price, thomas.lendacky, john.allen, ashish.kalra,
suravee.suthikulpanit, linux-coco
In-Reply-To: <20260210002927.GC943673@ziepe.ca>
Tue, Feb 10, 2026 at 01:29:27AM +0100, jgg@ziepe.ca wrote:
>On Mon, Feb 09, 2026 at 12:08:03PM -0800, John Stultz wrote:
>> On Mon, Feb 9, 2026 at 7:38 AM Jiri Pirko <jiri@resnulli.us> wrote:
>> >
>> > From: Jiri Pirko <jiri@nvidia.com>
>> >
>> > Currently the flags, which are unused, are validated for all heaps.
>> > Since the follow-up patch introduces a flag valid for only one of the
>> > heaps, allow to specify the valid flags per-heap.
>>
>> I'm not really in this space anymore, so take my feedback with a grain of salt.
>>
>> While the heap allocate flags argument is unused, it was intended to
>> be used for generic allocation flags that would apply to all or at
>> least a wide majority of heaps.
>>
>> It was definitely not added to allow for per-heap or heap specific
>> flags (as this patch tries to utilize it). That was the mess we had
>> with ION driver that we were trying to avoid.
>
>I don't know alot about DMA heaps..
>
>On a CC VM system the shared/private property is universal and applies
>to every physical address. Not every address can dynamically change
>between shared and private, but every address does have a
>shared/private state.
>
>By default userspace process generally run exclusively in private
>memory and there are very few ways for userspace to even access shared
>memory.
>
>From a heaps perspective the API would be very strange, and perhaps
>even security dangerous, if it is returning shared memory to userspace
>without userspace knowing this is happening.
>
>I'd advocate that the right design is for userspace to positively
>signal via this flag that it wants/accepts shared memory and without
>the flag shared memory should never be returned.
We can have the same behaviour with the separate heap, can't we?
Userpace positively signals it wants/accepts the shared memory by
choosing "system_cc_decrypted" heap name.
[...]
^ permalink raw reply
* 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-10 10:54 UTC (permalink / raw)
To: seanjc@google.com
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Li, Xiaoyao,
Zhao, Yan Y, dave.hansen@linux.intel.com,
linux-kernel@vger.kernel.org, kas@kernel.org,
binbin.wu@linux.intel.com, pbonzini@redhat.com, mingo@redhat.com,
Yamahata, Isaku, ackerleytng@google.com, tglx@kernel.org,
sagis@google.com, Edgecombe, Rick P, bp@alien8.de,
Annapurve, Vishal, x86@kernel.org
In-Reply-To: <aYpjNrtGmogNzqwT@google.com>
>
> Doesn't work, because sp->external_spt will be non-NULL when KVM is freeing
> unused pages in tdp_mmu_split_huge_pages_root() and kvm_tdp_mmu_map(). That's
> solvable, but it's part of the asymmetry I don't love. AFAICT, unless we do
> something truly awful, there's no way to avoid having common KVM free unused
> S-EPT pages.
>
> That said, while I don't love the asymmetry, it's not a deal breaker, especially
> if we make the asymmetry super obvious and cleanly delineated. Specifically, if
> we differentiate between freeing unused page tables and freeing used (linked at
> any point) page tables.
>
> This would also allow us to address the naming than Yan doesn't like around
> reclaim_external_sp(), because we could have both free_external_sp() and
> free_unused_external_spt(), where the lack of "unused" gives the reader a hint
> that there's interesting work to be done for in-use external page tables.
>
> This won't apply cleanly due to other fixups. It's also at:
That's a bit unfortunate.
I hate to say, but should we just use option 1? :-)
As Dave mentioned, we can always improve the locking when there's real
performance issue.
^ permalink raw reply
* Re: [RFC PATCH v5 08/45] KVM: x86/mmu: Propagate mirror SPTE removal to S-EPT in handle_changed_spte()
From: Yan Zhao @ 2026-02-10 10:54 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: <aYYn0nf2cayYu8e7@google.com>
On Fri, Feb 06, 2026 at 09:41:38AM -0800, Sean Christopherson wrote:
> On Fri, Feb 06, 2026, Yan Zhao wrote:
> > On Thu, Feb 05, 2026 at 02:33:16PM -0800, Sean Christopherson wrote:
> > > On Thu, Feb 05, 2026, Yan Zhao wrote:
> > > > > > > if (was_present && !was_leaf &&
> > > > > > > (is_leaf || !is_present || WARN_ON_ONCE(pfn_changed)))
> > > > > > > handle_removed_pt(kvm, spte_to_child_pt(old_spte, level), shared);
> > > > > > > + else if (was_leaf && is_mirror_sptep(sptep) && !is_leaf)
> > > > > > Should we check !is_present instead of !is_leaf?
> > > > > > e.g. a transition from a present leaf entry to a present non-leaf entry could
> > > > > > also trigger this if case.
> > > > >
> > > > > No, the !is_leaf check is very intentional. At this point in the series, S-EPT
> > > > > doesn't support hugepages. If KVM manages to install a leaf SPTE and replaces
> > > > > that SPTE with a non-leaf SPTE, then we absolutely want the KVM_BUG_ON() in
> > > > > tdx_sept_remove_private_spte() to fire:
> > > > >
> > > > > /* TODO: handle large pages. */
> > > > > if (KVM_BUG_ON(level != PG_LEVEL_4K, kvm))
> > > > > return -EIO;
> > > > But the op is named remove_external_spte().
> > > > And the check of "level != PG_LEVEL_4K" is for removing large leaf entries.
> > >
> > > I agree that the naming at this point in the series is unfortunate, but I don't
> > > see it as outright wrong. That the TDP MMU could theoretically replace the leaf
> > > SPTE with a non-leaf SPTE doesn't change the fact that the old leaf SPTE *is*
> > > being removed.
> > Hmm, I can't agree with that. But I won't insist if you think it's ok :)
>
> If the code is read through a TDX lens, then I agree, it's seems wrong. Because
> then you *know* that TDX doesn't support back-to-back remove()=>add() operations
> to handle a page split.
>
> But from a TDP MMU perspective, this is entirely logical (ignoring that
> link_external_spt() is gone at this point in the series).
>
> else if (was_leaf && is_mirror_sptep(sptep) && !is_leaf) {
> kvm_x86_call(remove_external_spte)(kvm, gfn, level, old_spte);
>
> /*
> * Link the new page table if a hugepage is being split, i.e.
> * if a leaf SPTE is being replaced with a non-leaf SPTE.
> */
> if (is_present)
> kvm_x86_call(link_external_spt)(kvm, gfn, level, ...);
> }
>
> And that is *exactly* why I want to get rid of the hyper-specific kvm_x86_ops
> hooks. They bleed all kinds of implementation details all over the TDP MMU, which
> makes it difficult to read and understand the relevant TDP MMU code if you don't
> already know the TDX rules. And I absolutely do not want to effectively require
> others to understand TDX's rules to be able to make changes to the TDP MMU.
Ok. I can understand your reasoning of checking !is_leaf now.
Thanks for the explanation!
Though I still think checking !is_present before calling op remove_external_spte()
in this patch is better, I have no strong opinion :)
...
> Nope, as above, 100% the opposite. Over ~3 patches, e.g.
>
> 1. Drop the KVM_BUG_ON()s or move them to TDX
> 2. Morph the !is_frozen_spte() check into a KVM_MMU_WARN_ON()
> 3. Rework the code to rely on __handle_changed_spte() to propagate writes to S-EPT
>
> That way, the _only_ path that updates external SPTEs is this:
>
> if (was_present && !was_leaf &&
> (is_leaf || !is_present || WARN_ON_ONCE(pfn_changed)))
> handle_removed_pt(kvm, spte_to_child_pt(old_spte, level), shared);
> else if (is_mirror_sptep(sptep))
> return kvm_x86_call(set_external_spte)(kvm, gfn, old_spte,
> new_spte, level);
>
> which is fully aligned with handle_changed_spte()'s role for !mirror roots: it
> exists to react to changes (the sole exception to that being aging SPTEs, which
> is a special case).
>
> Compile-tested only.
LGTM overall.
> arch/x86/kvm/mmu/tdp_mmu.c | 118 ++++++++++++++++++-------------------
> 1 file changed, 59 insertions(+), 59 deletions(-)
>
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 847f2fcb6740..33a321aedac0 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -464,7 +464,7 @@ static void handle_removed_pt(struct kvm *kvm, tdp_ptep_t pt, bool shared)
> }
>
> /**
> - * handle_changed_spte - handle bookkeeping associated with an SPTE change
> + * __handle_changed_spte - handle bookkeeping associated with an SPTE change
> * @kvm: kvm instance
> * @as_id: the address space of the paging structure the SPTE was a part of
> * @sptep: pointer to the SPTE
> @@ -480,9 +480,9 @@ static void handle_removed_pt(struct kvm *kvm, tdp_ptep_t pt, bool shared)
> * dirty logging updates are handled in common code, not here (see make_spte()
> * and fast_pf_fix_direct_spte()).
> */
> -static void handle_changed_spte(struct kvm *kvm, int as_id, tdp_ptep_t sptep,
> - gfn_t gfn, u64 old_spte, u64 new_spte,
> - int level, bool shared)
> +static int __handle_changed_spte(struct kvm *kvm, int as_id, tdp_ptep_t sptep,
> + gfn_t gfn, u64 old_spte, u64 new_spte,
> + int level, bool shared)
> {
> bool was_present = is_shadow_present_pte(old_spte);
> bool is_present = is_shadow_present_pte(new_spte);
> @@ -518,7 +518,7 @@ static void handle_changed_spte(struct kvm *kvm, int as_id, tdp_ptep_t sptep,
> }
>
> if (old_spte == new_spte)
> - return;
> + return 0;
>
> trace_kvm_tdp_mmu_spte_changed(as_id, gfn, level, old_spte, new_spte);
>
> @@ -547,7 +547,7 @@ static void handle_changed_spte(struct kvm *kvm, int as_id, tdp_ptep_t sptep,
> "a temporary frozen SPTE.\n"
> "as_id: %d gfn: %llx old_spte: %llx new_spte: %llx level: %d",
> as_id, gfn, old_spte, new_spte, level);
> - return;
> + return 0;
> }
>
> if (is_leaf != was_leaf)
> @@ -559,30 +559,31 @@ static void handle_changed_spte(struct kvm *kvm, int as_id, tdp_ptep_t sptep,
> * SPTE being converted to a hugepage (leaf) or being zapped. Shadow
> * pages are kernel allocations and should never be migrated.
> *
> - * When modifying leaf entries in mirrored page tables, propagate the
> - * changes to the external SPTE. Bug the VM on failure, as callers
> - * aren't prepared to handle errors, e.g. due to lock contention in the
> - * TDX-Module. Note, changes to non-leaf mirror SPTEs are handled by
> - * handle_removed_pt() (the TDX-Module requires that child entries are
> - * removed before the parent SPTE), and changes to non-present mirror
> - * SPTEs are handled by __tdp_mmu_set_spte_atomic() (KVM needs to set
> - * the external SPTE while the mirror SPTE is frozen so that installing
> - * a new SPTE is effectively an atomic operation).
> + * When modifying leaf entries in mirrored page tables, propagate all
> + * changes to the external SPTE.
> */
> if (was_present && !was_leaf &&
> (is_leaf || !is_present || WARN_ON_ONCE(pfn_changed)))
> handle_removed_pt(kvm, spte_to_child_pt(old_spte, level), shared);
> - else if (was_leaf && is_mirror_sptep(sptep))
> - KVM_BUG_ON(kvm_x86_call(set_external_spte)(kvm, gfn, old_spte,
> - new_spte, level), kvm);
> + else if (is_mirror_sptep(sptep))
> + return kvm_x86_call(set_external_spte)(kvm, gfn, old_spte,
> + new_spte, level);
For TDX's future implementation of set_external_spte() for split splitting,
could we add a new param "bool shared" to op set_external_spte() in the
future? i.e.,
- when tdx_sept_split_private_spte() is invoked under write mmu_lock, it calls
tdh_do_no_vcpus() to retry BUSY error, and TDX_BUG_ON_2() then.
- when tdx_sept_split_private_spte() is invoked under read mmu_lock
(in the future when calling tdh_mem_range_block() in unnecessary), it could
directly return BUSY to TDP MMU on contention.
> + return 0;
> +}
> +
> +static void handle_changed_spte(struct kvm *kvm, int as_id, tdp_ptep_t sptep,
> + gfn_t gfn, u64 old_spte, u64 new_spte,
> + int level, bool shared)
> +{
Do we need "WARN_ON_ONCE(is_mirror_sptep(sptep) && shared)" here ?
> + KVM_BUG_ON(__handle_changed_spte(kvm, as_id, sptep, gfn, old_spte,
> + new_spte, level, shared), kvm);
> }
>
> static inline int __must_check __tdp_mmu_set_spte_atomic(struct kvm *kvm,
> struct tdp_iter *iter,
> u64 new_spte)
> {
> - u64 *raw_sptep = rcu_dereference(iter->sptep);
> -
> /*
> * The caller is responsible for ensuring the old SPTE is not a FROZEN
> * SPTE. KVM should never attempt to zap or manipulate a FROZEN SPTE,
> @@ -591,40 +592,6 @@ static inline int __must_check __tdp_mmu_set_spte_atomic(struct kvm *kvm,
> */
> WARN_ON_ONCE(iter->yielded || is_frozen_spte(iter->old_spte));
>
> - if (is_mirror_sptep(iter->sptep) && !is_frozen_spte(new_spte)) {
> - int ret;
> -
> - /*
> - * KVM doesn't currently support zapping or splitting mirror
> - * SPTEs while holding mmu_lock for read.
> - */
> - if (KVM_BUG_ON(is_shadow_present_pte(iter->old_spte), kvm) ||
> - KVM_BUG_ON(!is_shadow_present_pte(new_spte), kvm))
> - return -EBUSY;
> -
> - /*
> - * Temporarily freeze the SPTE until the external PTE operation
> - * has completed, e.g. so that concurrent faults don't attempt
> - * to install a child PTE in the external page table before the
> - * parent PTE has been written.
> - */
> - if (!try_cmpxchg64(raw_sptep, &iter->old_spte, FROZEN_SPTE))
> - return -EBUSY;
> -
> - /*
> - * Update the external PTE. On success, set the mirror SPTE to
> - * the desired value. On failure, restore the old SPTE so that
> - * the SPTE isn't frozen in perpetuity.
> - */
> - ret = kvm_x86_call(set_external_spte)(kvm, iter->gfn, iter->old_spte,
> - new_spte, iter->level);
> - if (ret)
> - __kvm_tdp_mmu_write_spte(iter->sptep, iter->old_spte);
> - else
> - __kvm_tdp_mmu_write_spte(iter->sptep, new_spte);
> - return ret;
> - }
> -
> /*
> * Note, fast_pf_fix_direct_spte() can also modify TDP MMU SPTEs and
> * does not hold the mmu_lock. On failure, i.e. if a different logical
> @@ -632,7 +599,7 @@ static inline int __must_check __tdp_mmu_set_spte_atomic(struct kvm *kvm,
> * the current value, so the caller operates on fresh data, e.g. if it
> * retries tdp_mmu_set_spte_atomic()
> */
> - if (!try_cmpxchg64(raw_sptep, &iter->old_spte, new_spte))
> + if (!try_cmpxchg64(rcu_dereference(iter->sptep), &iter->old_spte, new_spte))
> return -EBUSY;
>
> return 0;
> @@ -663,14 +630,44 @@ static inline int __must_check tdp_mmu_set_spte_atomic(struct kvm *kvm,
>
> lockdep_assert_held_read(&kvm->mmu_lock);
>
> - ret = __tdp_mmu_set_spte_atomic(kvm, iter, new_spte);
>
> + /* KVM should never freeze SPTEs using higher level APIs. */
> + KVM_MMU_WARN_ON(is_frozen_spte(new_spte));
What about
KVM_MMU_WARN_ON(is_frozen_spte(new_spte) ||
is_frozen_spte(iter->old_spte) || iter->yielded);
> + /*
> + * Temporarily freeze the SPTE until the external PTE operation has
> + * completed (unless the new SPTE itself will be frozen), e.g. so that
> + * concurrent faults don't attempt to install a child PTE in the
> + * external page table before the parent PTE has been written, or try
> + * to re-install a page table before the old one was removed.
> + */
> + if (is_mirror_sptep(iter->sptep))
> + ret = __tdp_mmu_set_spte_atomic(kvm, iter, FROZEN_SPTE);
> + else
> + ret = __tdp_mmu_set_spte_atomic(kvm, iter, new_spte);
and invoking open code try_cmpxchg64() directly?
> if (ret)
> return ret;
>
> - handle_changed_spte(kvm, iter->as_id, iter->sptep, iter->gfn,
> - iter->old_spte, new_spte, iter->level, true);
> + ret = __handle_changed_spte(kvm, iter->as_id, iter->sptep, iter->gfn,
> + iter->old_spte, new_spte, iter->level, true);
>
> - return 0;
> + /*
> + * Unfreeze the mirror SPTE. If updating the external SPTE failed,
> + * restore the old SPTE so that the SPTE isn't frozen in perpetuity,
> + * otherwise set the mirror SPTE to the new desired value.
> + */
> + if (is_mirror_sptep(iter->sptep)) {
> + if (ret)
> + __kvm_tdp_mmu_write_spte(iter->sptep, iter->old_spte);
> + else
> + __kvm_tdp_mmu_write_spte(iter->sptep, new_spte);
> + } else {
> + /*
> + * Bug the VM if handling the change failed, as failure is only
> + * allowed if KVM couldn't update the external SPTE.
> + */
> + KVM_BUG_ON(ret, kvm);
> + }
> + return ret;
> }
One concern for tdp_mmu_set_spte_atomic() to handle mirror SPTEs:
- Previously
1. set *iter->sptep to FROZEN_SPTE.
2. kvm_x86_call(set_external_spte)(old_spte, new_spte)
3. set *iter->sptep to new_spte
- Now with this diff
1. set *iter->sptep to FROZEN_SPTE.
2. __handle_changed_spte()
--> kvm_x86_call(set_external_spte)(iter->sptep, old_spte, new_spte)
3. set *iter->sptep to new_spte
what if __handle_changed_spte() reads *iter->sptep in step 2?
Passing in "bool is_mirror_sp" to __handle_changed_spte() instead?
^ permalink raw reply
* Re: [PATCH 5/5] dma-buf: heaps: system: add an option to allocate explicitly decrypted memory
From: kernel test robot @ 2026-02-10 12:02 UTC (permalink / raw)
To: Jiri Pirko, dri-devel, linaro-mm-sig, iommu, linux-media
Cc: llvm, oe-kbuild-all, sumit.semwal, benjamin.gaignard,
Brian.Starkey, jstultz, tjmercier, christian.koenig, m.szyprowski,
robin.murphy, jgg, leon, sean.anderson, ptesarik, catalin.marinas,
aneesh.kumar, suzuki.poulose, steven.price, thomas.lendacky,
john.allen, ashish.kalra, suravee.suthikulpanit, linux-coco
In-Reply-To: <20260209153809.250835-6-jiri@resnulli.us>
Hi Jiri,
kernel test robot noticed the following build errors:
[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on drm-tip/drm-tip trace/for-next linus/master v6.19]
[cannot apply to next-20260209]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jiri-Pirko/dma-mapping-avoid-random-addr-value-print-out-on-error-path/20260209-234013
base: https://gitlab.freedesktop.org/drm/misc/kernel.git drm-misc-next
patch link: https://lore.kernel.org/r/20260209153809.250835-6-jiri%40resnulli.us
patch subject: [PATCH 5/5] dma-buf: heaps: system: add an option to allocate explicitly decrypted memory
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20260210/202602101926.lsquJdb1-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260210/202602101926.lsquJdb1-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202602101926.lsquJdb1-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/dma-buf/heaps/system_heap.c:66:8: error: call to undeclared function 'set_memory_decrypted'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
66 | ret = set_memory_decrypted(addr, nr_pages);
| ^
>> drivers/dma-buf/heaps/system_heap.c:80:8: error: call to undeclared function 'set_memory_encrypted'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
80 | ret = set_memory_encrypted(addr, nr_pages);
| ^
2 errors generated.
vim +/set_memory_decrypted +66 drivers/dma-buf/heaps/system_heap.c
59
60 static int system_heap_set_page_decrypted(struct page *page)
61 {
62 unsigned long addr = (unsigned long)page_address(page);
63 unsigned int nr_pages = 1 << compound_order(page);
64 int ret;
65
> 66 ret = set_memory_decrypted(addr, nr_pages);
67 if (ret)
68 pr_warn_ratelimited("dma-buf system heap: failed to decrypt page at %p\n",
69 page_address(page));
70
71 return ret;
72 }
73
74 static int system_heap_set_page_encrypted(struct page *page)
75 {
76 unsigned long addr = (unsigned long)page_address(page);
77 unsigned int nr_pages = 1 << compound_order(page);
78 int ret;
79
> 80 ret = set_memory_encrypted(addr, nr_pages);
81 if (ret)
82 pr_warn_ratelimited("dma-buf system heap: failed to re-encrypt page at %p, leaking memory\n",
83 page_address(page));
84
85 return ret;
86 }
87
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH 4/5] dma-buf: heaps: allow heap to specify valid heap flags
From: Jason Gunthorpe @ 2026-02-10 12:43 UTC (permalink / raw)
To: Jiri Pirko
Cc: John Stultz, dri-devel, linaro-mm-sig, iommu, linux-media,
sumit.semwal, benjamin.gaignard, Brian.Starkey, tjmercier,
christian.koenig, m.szyprowski, robin.murphy, leon, sean.anderson,
ptesarik, catalin.marinas, aneesh.kumar, suzuki.poulose,
steven.price, thomas.lendacky, john.allen, ashish.kalra,
suravee.suthikulpanit, linux-coco
In-Reply-To: <tgvdjszwxggr53digbmddcbxvupzl4xcoprofkgrs2kgf6rknx@44ebljjpghjm>
On Tue, Feb 10, 2026 at 10:14:08AM +0100, Jiri Pirko wrote:
> >I'd advocate that the right design is for userspace to positively
> >signal via this flag that it wants/accepts shared memory and without
> >the flag shared memory should never be returned.
>
> We can have the same behaviour with the separate heap, can't we?
> Userpace positively signals it wants/accepts the shared memory by
> choosing "system_cc_decrypted" heap name.
So what do the other heap names do? Always private? Do you ever get
heaps that are unknowably private or shared (eg MMIO backed?)
Jason
^ permalink raw reply
* Re: [PATCH 4/5] dma-buf: heaps: allow heap to specify valid heap flags
From: Leon Romanovsky @ 2026-02-10 12:48 UTC (permalink / raw)
To: Jiri Pirko
Cc: John Stultz, dri-devel, linaro-mm-sig, iommu, linux-media,
sumit.semwal, benjamin.gaignard, Brian.Starkey, tjmercier,
christian.koenig, m.szyprowski, robin.murphy, jgg, sean.anderson,
ptesarik, catalin.marinas, aneesh.kumar, suzuki.poulose,
steven.price, thomas.lendacky, john.allen, ashish.kalra,
suravee.suthikulpanit, linux-coco
In-Reply-To: <hwdezwktndbm6hoko3rz5lffgfljodegcygzf6rbdf2ferokj6@ftk2uk3rqfdq>
On Tue, Feb 10, 2026 at 10:05:14AM +0100, Jiri Pirko wrote:
> Mon, Feb 09, 2026 at 09:08:03PM +0100, jstultz@google.com wrote:
> >On Mon, Feb 9, 2026 at 7:38 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >>
> >> From: Jiri Pirko <jiri@nvidia.com>
> >>
> >> Currently the flags, which are unused, are validated for all heaps.
> >> Since the follow-up patch introduces a flag valid for only one of the
> >> heaps, allow to specify the valid flags per-heap.
> >
> >I'm not really in this space anymore, so take my feedback with a grain of salt.
> >
> >While the heap allocate flags argument is unused, it was intended to
> >be used for generic allocation flags that would apply to all or at
> >least a wide majority of heaps.
> >
> >It was definitely not added to allow for per-heap or heap specific
> >flags (as this patch tries to utilize it). That was the mess we had
> >with ION driver that we were trying to avoid.
> >
> >The intent of dma-buf heaps is to try to abstract all the different
> >device memory constraints so there only needs to be a [usage] ->
> >[heap] mapping, and otherwise userland can be generalized so that it
> >doesn't need to be re-written to work with different devices/memory
> >types. Adding heap-specific allocation flags prevents that
> >generalization.
> >
> >So instead of adding heap specific flags, the general advice has been
> >to add a separate heap name for the flag property.
>
> Right, my original idea was to add a separate heap. Then I spotted the
> flags and seemed like a great fit. Was not aware or the history or
> original intention. Would be probably good to document it for
> future generations.
>
> So instead of flag, I will add heap named something
> like "system_cc_decrypted" to implement this.
It is problematic to expose a user‑visible API that depends on a name.
Such a design limits our ability to extend the functionality in the
future, should new use cases arise.
Thanks
>
> Thanks!
^ permalink raw reply
* Re: [PATCH 4/5] dma-buf: heaps: allow heap to specify valid heap flags
From: Jiri Pirko @ 2026-02-10 14:49 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: John Stultz, dri-devel, linaro-mm-sig, iommu, linux-media,
sumit.semwal, benjamin.gaignard, Brian.Starkey, tjmercier,
christian.koenig, m.szyprowski, robin.murphy, leon, sean.anderson,
ptesarik, catalin.marinas, aneesh.kumar, suzuki.poulose,
steven.price, thomas.lendacky, john.allen, ashish.kalra,
suravee.suthikulpanit, linux-coco
In-Reply-To: <20260210124357.GD943673@ziepe.ca>
Tue, Feb 10, 2026 at 01:43:57PM +0100, jgg@ziepe.ca wrote:
>On Tue, Feb 10, 2026 at 10:14:08AM +0100, Jiri Pirko wrote:
>
>> >I'd advocate that the right design is for userspace to positively
>> >signal via this flag that it wants/accepts shared memory and without
>> >the flag shared memory should never be returned.
>>
>> We can have the same behaviour with the separate heap, can't we?
>> Userpace positively signals it wants/accepts the shared memory by
>> choosing "system_cc_decrypted" heap name.
>
>So what do the other heap names do? Always private? Do you ever get
>heaps that are unknowably private or shared (eg MMIO backed?)
If I understand the code correctly, you may get something like this:
$ ls /dev/dma_heap/
default_cma_region
protected,secure-video
protected,secure-video-record
protected,trusted-ui
system
The "protected*" ones are created by tee. I believe they handle
memory that is inaccesible to CPU.
^ permalink raw reply
* Re: [PATCH 4/5] dma-buf: heaps: allow heap to specify valid heap flags
From: Jason Gunthorpe @ 2026-02-10 14:54 UTC (permalink / raw)
To: Jiri Pirko
Cc: John Stultz, dri-devel, linaro-mm-sig, iommu, linux-media,
sumit.semwal, benjamin.gaignard, Brian.Starkey, tjmercier,
christian.koenig, m.szyprowski, robin.murphy, leon, sean.anderson,
ptesarik, catalin.marinas, aneesh.kumar, suzuki.poulose,
steven.price, thomas.lendacky, john.allen, ashish.kalra,
suravee.suthikulpanit, linux-coco
In-Reply-To: <y7gvezflidmma7odnut2rmlecsbxahrcwpmoevfnhzjveusuwj@6qxqogin45j3>
On Tue, Feb 10, 2026 at 03:49:02PM +0100, Jiri Pirko wrote:
> Tue, Feb 10, 2026 at 01:43:57PM +0100, jgg@ziepe.ca wrote:
> >On Tue, Feb 10, 2026 at 10:14:08AM +0100, Jiri Pirko wrote:
> >
> >> >I'd advocate that the right design is for userspace to positively
> >> >signal via this flag that it wants/accepts shared memory and without
> >> >the flag shared memory should never be returned.
> >>
> >> We can have the same behaviour with the separate heap, can't we?
> >> Userpace positively signals it wants/accepts the shared memory by
> >> choosing "system_cc_decrypted" heap name.
> >
> >So what do the other heap names do? Always private? Do you ever get
> >heaps that are unknowably private or shared (eg MMIO backed?)
>
> If I understand the code correctly, you may get something like this:
> $ ls /dev/dma_heap/
> default_cma_region
> protected,secure-video
> protected,secure-video-record
> protected,trusted-ui
> system
>
> The "protected*" ones are created by tee. I believe they handle
> memory that is inaccesible to CPU.
If that is the only list of options then maybe just the name will work
Ok.
I *think* CMA and system should be reliably CC private.
The protected ones seem to have their own internal definition, and
probably can't exist on CC VM systems..
Meaning we don't have any shared things leaking through which would be
the point.
Jason
^ permalink raw reply
* SVSM Development Call February 11, 2026
From: Jörg Rödel @ 2026-02-10 15:56 UTC (permalink / raw)
To: coconut-svsm, linux-coco
Hi,
Here is the call for agenda items for this weeks SVSM development call. Please
send any agenda items you have in mind as a reply to this email or raise them
in the meeting.
We will use the LF Zoom instance. Details of the meeting can be found in our
governance repository at:
https://github.com/coconut-svsm/governance
The link to the COCONUT-SVSM calendar is:
https://zoom-lfx.platform.linuxfoundation.org/meetings/coconut-svsm?view=week
The meeting will be recorded and the recording eventually published.
Regards,
Jörg
^ permalink raw reply
* Re: [RFC PATCH v5 16/45] x86/virt/tdx: Add tdx_alloc/free_control_page() helpers
From: Dave Hansen @ 2026-02-10 17:44 UTC (permalink / raw)
To: Sean Christopherson, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, Dave Hansen, x86, Kiryl Shutsemau, Paolo Bonzini
Cc: linux-kernel, linux-coco, kvm, Kai Huang, Rick Edgecombe,
Yan Zhao, Vishal Annapurve, Ackerley Tng, Sagi Shahar, Binbin Wu,
Xiaoyao Li, Isaku Yamahata
In-Reply-To: <20260129011517.3545883-17-seanjc@google.com>
On 1/28/26 17:14, Sean Christopherson wrote:
> +static void tdx_pamt_put(struct page *page)
> +{
> + u64 pamt_pa_array[MAX_NR_DPAMT_ARGS];
> + atomic_t *pamt_refcount;
> + u64 tdx_status;
> +
> + if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> + return;
> +
> + pamt_refcount = tdx_find_pamt_refcount(page_to_pfn(page));
> +
> + scoped_guard(spinlock, &pamt_lock) {
> + /*
> + * If the there are more than 1 references on the pamt page,
> + * don't remove it yet. Just decrement the refcount.
> + */
> + if (atomic_read(pamt_refcount) > 1) {
> + atomic_dec(pamt_refcount);
> + return;
> + }
> +
> + /* Try to remove the pamt page and take the refcount 1->0. */
> + tdx_status = tdh_phymem_pamt_remove(page, pamt_pa_array);
> +
> + /*
> + * Don't free pamt_pa_array as it could hold garbage when
> + * tdh_phymem_pamt_remove() fails. Don't panic/BUG_ON(), as
> + * there is no risk of data corruption, but do yell loudly as
> + * failure indicates a kernel bug, memory is being leaked, and
> + * the dangling PAMT entry may cause future operations to fail.
> + */
> + if (WARN_ON_ONCE(!IS_TDX_SUCCESS(tdx_status)))
> + return;
> +
> + atomic_dec(pamt_refcount);
> + }
> +
> + /*
> + * pamt_pa_array is populated up to tdx_dpamt_entry_pages() by the TDX
> + * module with pages, or remains zero inited. free_pamt_array() can
> + * handle either case. Just pass it unconditionally.
> + */
> + free_pamt_array(pamt_pa_array);
> +}
This looks funky.
Right now, this is:
spin_lock(pamt_lock)
atomic_inc/dec(fine-grained-refcount)
tdcall_blah_blah()
spin_unlock(pamt_lock)
Where it *always* acquires the global lock when DPAMT is supported.
Couldn't we optimize it so that it only acquires it when it has to keep
the refcount stable at zero?
Roughly:
slow_path = atomic_dec_and_lock(fine-grained-refcount,
pamt_lock)
if (!slow_path)
goto out;
// fine-grained-refcount==0 and must stay that way with
// pamt_lock held. Remove the DPAMT pages:
tdh_phymem_pamt_remove(page, pamt_pa_array)
out:
spin_unlock(pamt_lock)
On the acquire side, you do:
fast_path = atomic_inc_not_zero(fine-grained-refcount)
if (fast_path)
return;
// slow path:
spin_lock(pamt_lock)
// Was the race lost with another 0=>1 increment?
if (atomic_read(fine-grained-refcount) > 0)
goto out_inc
tdh_phymem_pamt_add(page, pamt_pa_array)
// Inc after the TDCALL so another thread won't race ahead of us
// and try to use a non-existent PAMT entry
out_inc:
atomic_inc(fine-grained-refcount)
spin_unlock(pamt_lock)
Then, at least only the 0=>1 and 1=>0 transitions need the global lock.
The fast paths only touch the refcount which isn't shared nearly as much
as the global lock.
BTW, this probably still needs to be spin_lock_irq(), not what I wrote
above, but that's not a big deal to add.
I've stared at this for a bit and don't see any holes. Does anyone else
see any?
^ permalink raw reply
* Re: [PATCH 5/5] dma-buf: heaps: system: add an option to allocate explicitly decrypted memory
From: kernel test robot @ 2026-02-10 18:03 UTC (permalink / raw)
To: Jiri Pirko, dri-devel, linaro-mm-sig, iommu, linux-media
Cc: oe-kbuild-all, sumit.semwal, benjamin.gaignard, Brian.Starkey,
jstultz, tjmercier, christian.koenig, m.szyprowski, robin.murphy,
jgg, leon, sean.anderson, ptesarik, catalin.marinas, aneesh.kumar,
suzuki.poulose, steven.price, thomas.lendacky, john.allen,
ashish.kalra, suravee.suthikulpanit, linux-coco
In-Reply-To: <20260209153809.250835-6-jiri@resnulli.us>
Hi Jiri,
kernel test robot noticed the following build errors:
[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on drm-tip/drm-tip trace/for-next linus/master v6.19]
[cannot apply to next-20260209]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jiri-Pirko/dma-mapping-avoid-random-addr-value-print-out-on-error-path/20260209-234013
base: https://gitlab.freedesktop.org/drm/misc/kernel.git drm-misc-next
patch link: https://lore.kernel.org/r/20260209153809.250835-6-jiri%40resnulli.us
patch subject: [PATCH 5/5] dma-buf: heaps: system: add an option to allocate explicitly decrypted memory
config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20260211/202602110149.tBUPP0bh-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 15.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260211/202602110149.tBUPP0bh-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202602110149.tBUPP0bh-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/dma-buf/heaps/system_heap.c: In function 'system_heap_set_page_decrypted':
>> drivers/dma-buf/heaps/system_heap.c:66:15: error: implicit declaration of function 'set_memory_decrypted' [-Wimplicit-function-declaration]
66 | ret = set_memory_decrypted(addr, nr_pages);
| ^~~~~~~~~~~~~~~~~~~~
drivers/dma-buf/heaps/system_heap.c: In function 'system_heap_set_page_encrypted':
>> drivers/dma-buf/heaps/system_heap.c:80:15: error: implicit declaration of function 'set_memory_encrypted' [-Wimplicit-function-declaration]
80 | ret = set_memory_encrypted(addr, nr_pages);
| ^~~~~~~~~~~~~~~~~~~~
vim +/set_memory_decrypted +66 drivers/dma-buf/heaps/system_heap.c
59
60 static int system_heap_set_page_decrypted(struct page *page)
61 {
62 unsigned long addr = (unsigned long)page_address(page);
63 unsigned int nr_pages = 1 << compound_order(page);
64 int ret;
65
> 66 ret = set_memory_decrypted(addr, nr_pages);
67 if (ret)
68 pr_warn_ratelimited("dma-buf system heap: failed to decrypt page at %p\n",
69 page_address(page));
70
71 return ret;
72 }
73
74 static int system_heap_set_page_encrypted(struct page *page)
75 {
76 unsigned long addr = (unsigned long)page_address(page);
77 unsigned int nr_pages = 1 << compound_order(page);
78 int ret;
79
> 80 ret = set_memory_encrypted(addr, nr_pages);
81 if (ret)
82 pr_warn_ratelimited("dma-buf system heap: failed to re-encrypt page at %p, leaking memory\n",
83 page_address(page));
84
85 return ret;
86 }
87
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [RFC PATCH v5 08/45] KVM: x86/mmu: Propagate mirror SPTE removal to S-EPT in handle_changed_spte()
From: Sean Christopherson @ 2026-02-10 19:52 UTC (permalink / raw)
To: Yan Zhao
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: <aYsOV7Q5FTWo+6/x@yzhao56-desk.sh.intel.com>
On Tue, Feb 10, 2026, Yan Zhao wrote:
> On Fri, Feb 06, 2026 at 09:41:38AM -0800, Sean Christopherson wrote:
> > On Fri, Feb 06, 2026, Yan Zhao wrote:
> > @@ -559,30 +559,31 @@ static void handle_changed_spte(struct kvm *kvm, int as_id, tdp_ptep_t sptep,
> > * SPTE being converted to a hugepage (leaf) or being zapped. Shadow
> > * pages are kernel allocations and should never be migrated.
> > *
> > - * When modifying leaf entries in mirrored page tables, propagate the
> > - * changes to the external SPTE. Bug the VM on failure, as callers
> > - * aren't prepared to handle errors, e.g. due to lock contention in the
> > - * TDX-Module. Note, changes to non-leaf mirror SPTEs are handled by
> > - * handle_removed_pt() (the TDX-Module requires that child entries are
> > - * removed before the parent SPTE), and changes to non-present mirror
> > - * SPTEs are handled by __tdp_mmu_set_spte_atomic() (KVM needs to set
> > - * the external SPTE while the mirror SPTE is frozen so that installing
> > - * a new SPTE is effectively an atomic operation).
> > + * When modifying leaf entries in mirrored page tables, propagate all
> > + * changes to the external SPTE.
> > */
> > if (was_present && !was_leaf &&
> > (is_leaf || !is_present || WARN_ON_ONCE(pfn_changed)))
> > handle_removed_pt(kvm, spte_to_child_pt(old_spte, level), shared);
> > - else if (was_leaf && is_mirror_sptep(sptep))
> > - KVM_BUG_ON(kvm_x86_call(set_external_spte)(kvm, gfn, old_spte,
> > - new_spte, level), kvm);
> > + else if (is_mirror_sptep(sptep))
> > + return kvm_x86_call(set_external_spte)(kvm, gfn, old_spte,
> > + new_spte, level);
> For TDX's future implementation of set_external_spte() for split splitting,
> could we add a new param "bool shared" to op set_external_spte() in the
> future? i.e.,
> - when tdx_sept_split_private_spte() is invoked under write mmu_lock, it calls
> tdh_do_no_vcpus() to retry BUSY error, and TDX_BUG_ON_2() then.
> - when tdx_sept_split_private_spte() is invoked under read mmu_lock
> (in the future when calling tdh_mem_range_block() in unnecessary), it could
> directly return BUSY to TDP MMU on contention.
Yeah, I have no objection to using @shared for things like that.
> > + return 0;
> > +}
> > +
> > +static void handle_changed_spte(struct kvm *kvm, int as_id, tdp_ptep_t sptep,
> > + gfn_t gfn, u64 old_spte, u64 new_spte,
> > + int level, bool shared)
> > +{
> Do we need "WARN_ON_ONCE(is_mirror_sptep(sptep) && shared)" here ?
No, because I want to call this code for all paths, including the fault path.
> > + KVM_BUG_ON(__handle_changed_spte(kvm, as_id, sptep, gfn, old_spte,
> > + new_spte, level, shared), kvm);
> > }
>
>
>
> >
> > static inline int __must_check __tdp_mmu_set_spte_atomic(struct kvm *kvm,
> > struct tdp_iter *iter,
> > u64 new_spte)
> > {
> > - u64 *raw_sptep = rcu_dereference(iter->sptep);
> > -
> > /*
> > * The caller is responsible for ensuring the old SPTE is not a FROZEN
> > * SPTE. KVM should never attempt to zap or manipulate a FROZEN SPTE,
> > @@ -591,40 +592,6 @@ static inline int __must_check __tdp_mmu_set_spte_atomic(struct kvm *kvm,
> > */
> > WARN_ON_ONCE(iter->yielded || is_frozen_spte(iter->old_spte));
> >
> > - if (is_mirror_sptep(iter->sptep) && !is_frozen_spte(new_spte)) {
> > - int ret;
> > -
> > - /*
> > - * KVM doesn't currently support zapping or splitting mirror
> > - * SPTEs while holding mmu_lock for read.
> > - */
> > - if (KVM_BUG_ON(is_shadow_present_pte(iter->old_spte), kvm) ||
> > - KVM_BUG_ON(!is_shadow_present_pte(new_spte), kvm))
> > - return -EBUSY;
> > -
> > - /*
> > - * Temporarily freeze the SPTE until the external PTE operation
> > - * has completed, e.g. so that concurrent faults don't attempt
> > - * to install a child PTE in the external page table before the
> > - * parent PTE has been written.
> > - */
> > - if (!try_cmpxchg64(raw_sptep, &iter->old_spte, FROZEN_SPTE))
> > - return -EBUSY;
> > -
> > - /*
> > - * Update the external PTE. On success, set the mirror SPTE to
> > - * the desired value. On failure, restore the old SPTE so that
> > - * the SPTE isn't frozen in perpetuity.
> > - */
> > - ret = kvm_x86_call(set_external_spte)(kvm, iter->gfn, iter->old_spte,
> > - new_spte, iter->level);
> > - if (ret)
> > - __kvm_tdp_mmu_write_spte(iter->sptep, iter->old_spte);
> > - else
> > - __kvm_tdp_mmu_write_spte(iter->sptep, new_spte);
> > - return ret;
> > - }
> > -
> > /*
> > * Note, fast_pf_fix_direct_spte() can also modify TDP MMU SPTEs and
> > * does not hold the mmu_lock. On failure, i.e. if a different logical
> > @@ -632,7 +599,7 @@ static inline int __must_check __tdp_mmu_set_spte_atomic(struct kvm *kvm,
> > * the current value, so the caller operates on fresh data, e.g. if it
> > * retries tdp_mmu_set_spte_atomic()
> > */
> > - if (!try_cmpxchg64(raw_sptep, &iter->old_spte, new_spte))
> > + if (!try_cmpxchg64(rcu_dereference(iter->sptep), &iter->old_spte, new_spte))
> > return -EBUSY;
> >
> > return 0;
> > @@ -663,14 +630,44 @@ static inline int __must_check tdp_mmu_set_spte_atomic(struct kvm *kvm,
> >
> > lockdep_assert_held_read(&kvm->mmu_lock);
> >
> > - ret = __tdp_mmu_set_spte_atomic(kvm, iter, new_spte);
> >
> > + /* KVM should never freeze SPTEs using higher level APIs. */
> > + KVM_MMU_WARN_ON(is_frozen_spte(new_spte));
> What about
> KVM_MMU_WARN_ON(is_frozen_spte(new_spte) ||
> is_frozen_spte(iter->old_spte) || iter->yielded);
>
> > + /*
> > + * Temporarily freeze the SPTE until the external PTE operation has
> > + * completed (unless the new SPTE itself will be frozen), e.g. so that
> > + * concurrent faults don't attempt to install a child PTE in the
> > + * external page table before the parent PTE has been written, or try
> > + * to re-install a page table before the old one was removed.
> > + */
> > + if (is_mirror_sptep(iter->sptep))
> > + ret = __tdp_mmu_set_spte_atomic(kvm, iter, FROZEN_SPTE);
> > + else
> > + ret = __tdp_mmu_set_spte_atomic(kvm, iter, new_spte);
> and invoking open code try_cmpxchg64() directly?
No, because __tdp_mmu_set_spte_atomic() is still used by kvm_tdp_mmu_age_spte(),
and the yielded/frozen rules apply there as well.
> > + /*
> > + * Unfreeze the mirror SPTE. If updating the external SPTE failed,
> > + * restore the old SPTE so that the SPTE isn't frozen in perpetuity,
> > + * otherwise set the mirror SPTE to the new desired value.
> > + */
> > + if (is_mirror_sptep(iter->sptep)) {
> > + if (ret)
> > + __kvm_tdp_mmu_write_spte(iter->sptep, iter->old_spte);
> > + else
> > + __kvm_tdp_mmu_write_spte(iter->sptep, new_spte);
> > + } else {
> > + /*
> > + * Bug the VM if handling the change failed, as failure is only
> > + * allowed if KVM couldn't update the external SPTE.
> > + */
> > + KVM_BUG_ON(ret, kvm);
> > + }
> > + return ret;
> > }
> One concern for tdp_mmu_set_spte_atomic() to handle mirror SPTEs:
> - Previously
> 1. set *iter->sptep to FROZEN_SPTE.
> 2. kvm_x86_call(set_external_spte)(old_spte, new_spte)
> 3. set *iter->sptep to new_spte
>
> - Now with this diff
> 1. set *iter->sptep to FROZEN_SPTE.
> 2. __handle_changed_spte()
> --> kvm_x86_call(set_external_spte)(iter->sptep, old_spte, new_spte)
Note, iter->sptep isn't passed to set_external_spte(), the invocation for that is:
return kvm_x86_call(set_external_spte)(kvm, gfn, old_spte,
new_spte, level);
> 3. set *iter->sptep to new_spte
>
> what if __handle_changed_spte() reads *iter->sptep in step 2?
For the most part, "don't do that". There are an infinite number of "what ifs".
I agree that re-reading iter->sptep is slightly more likely than other "what ifs",
but then if we convert to a boolean it creates the "what if we swap the order of
@as_id and @is_mirror_sp"? Given that @old_spte is provided, IMO re-reading the
SPTE from memory will stand out.
That said, I think we can have the best of both worlds. Rather than pass @as_id
and @sptep, pass the @sp, i.e. the owning kvm_mmu_page. That would address your
concern about re-reading the sptep, without needing another boolean.
E.g. slotted in as a cleanup somewhere earlier:
---
arch/x86/kvm/mmu/tdp_mmu.c | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
index 732548a678d8..d395da35d5e4 100644
--- a/arch/x86/kvm/mmu/tdp_mmu.c
+++ b/arch/x86/kvm/mmu/tdp_mmu.c
@@ -326,7 +326,7 @@ void kvm_tdp_mmu_alloc_root(struct kvm_vcpu *vcpu, bool mirror)
}
}
-static void handle_changed_spte(struct kvm *kvm, int as_id, tdp_ptep_t sptep,
+static void handle_changed_spte(struct kvm *kvm, struct kvm_mmu_page *sp,
gfn_t gfn, u64 old_spte, u64 new_spte,
int level, bool shared);
@@ -458,8 +458,7 @@ static void handle_removed_pt(struct kvm *kvm, tdp_ptep_t pt, bool shared)
old_spte = kvm_tdp_mmu_write_spte(sptep, old_spte,
FROZEN_SPTE, level);
}
- handle_changed_spte(kvm, kvm_mmu_page_as_id(sp), sptep, gfn,
- old_spte, FROZEN_SPTE, level, shared);
+ handle_changed_spte(kvm, sp, gfn, old_spte, FROZEN_SPTE, level, shared);
}
if (is_mirror_sp(sp))
@@ -471,8 +470,7 @@ static void handle_removed_pt(struct kvm *kvm, tdp_ptep_t pt, bool shared)
/**
* __handle_changed_spte - handle bookkeeping associated with an SPTE change
* @kvm: kvm instance
- * @as_id: the address space of the paging structure the SPTE was a part of
- * @sptep: pointer to the SPTE
+ * @sp: the page table in which the SPTE resides
* @gfn: the base GFN that was mapped by the SPTE
* @old_spte: The value of the SPTE before the change
* @new_spte: The value of the SPTE after the change
@@ -485,7 +483,7 @@ static void handle_removed_pt(struct kvm *kvm, tdp_ptep_t pt, bool shared)
* dirty logging updates are handled in common code, not here (see make_spte()
* and fast_pf_fix_direct_spte()).
*/
-static int __handle_changed_spte(struct kvm *kvm, int as_id, tdp_ptep_t sptep,
+static int __handle_changed_spte(struct kvm *kvm, struct kvm_mmu_page *sp,
gfn_t gfn, u64 old_spte, u64 new_spte,
int level, bool shared)
{
@@ -494,6 +492,7 @@ static int __handle_changed_spte(struct kvm *kvm, int as_id, tdp_ptep_t sptep,
bool was_leaf = was_present && is_last_spte(old_spte, level);
bool is_leaf = is_present && is_last_spte(new_spte, level);
bool pfn_changed = spte_to_pfn(old_spte) != spte_to_pfn(new_spte);
+ int as_id = kvm_mmu_page_as_id(sp);
WARN_ON_ONCE(level > PT64_ROOT_MAX_LEVEL);
WARN_ON_ONCE(level < PG_LEVEL_4K);
@@ -570,19 +569,19 @@ static int __handle_changed_spte(struct kvm *kvm, int as_id, tdp_ptep_t sptep,
if (was_present && !was_leaf &&
(is_leaf || !is_present || WARN_ON_ONCE(pfn_changed)))
handle_removed_pt(kvm, spte_to_child_pt(old_spte, level), shared);
- else if (is_mirror_sptep(sptep))
+ else if (is_mirror_sp(sp))
return kvm_x86_call(set_external_spte)(kvm, gfn, old_spte,
new_spte, level);
return 0;
}
-static void handle_changed_spte(struct kvm *kvm, int as_id, tdp_ptep_t sptep,
+static void handle_changed_spte(struct kvm *kvm, struct kvm_mmu_page *sp,
gfn_t gfn, u64 old_spte, u64 new_spte,
int level, bool shared)
{
- KVM_BUG_ON(__handle_changed_spte(kvm, as_id, sptep, gfn, old_spte,
- new_spte, level, shared), kvm);
+ KVM_BUG_ON(__handle_changed_spte(kvm, sp, gfn, old_spte, new_spte,
+ level, shared), kvm);
}
static inline int __must_check __tdp_mmu_set_spte_atomic(struct kvm *kvm,
@@ -631,6 +630,7 @@ static inline int __must_check tdp_mmu_set_spte_atomic(struct kvm *kvm,
struct tdp_iter *iter,
u64 new_spte)
{
+ struct kvm_mmu_page *sp = sptep_to_sp(rcu_dereference(iter->sptep));
int ret;
lockdep_assert_held_read(&kvm->mmu_lock);
@@ -652,8 +652,8 @@ static inline int __must_check tdp_mmu_set_spte_atomic(struct kvm *kvm,
if (ret)
return ret;
- ret = __handle_changed_spte(kvm, iter->as_id, iter->sptep, iter->gfn,
- iter->old_spte, new_spte, iter->level, true);
+ ret = __handle_changed_spte(kvm, sp, iter->gfn, iter->old_spte,
+ new_spte, iter->level, true);
/*
* Unfreeze the mirror SPTE. If updating the external SPTE failed,
@@ -678,7 +678,6 @@ static inline int __must_check tdp_mmu_set_spte_atomic(struct kvm *kvm,
/*
* tdp_mmu_set_spte - Set a TDP MMU SPTE and handle the associated bookkeeping
* @kvm: KVM instance
- * @as_id: Address space ID, i.e. regular vs. SMM
* @sptep: Pointer to the SPTE
* @old_spte: The current value of the SPTE
* @new_spte: The new value that will be set for the SPTE
@@ -691,6 +690,8 @@ static inline int __must_check tdp_mmu_set_spte_atomic(struct kvm *kvm,
static u64 tdp_mmu_set_spte(struct kvm *kvm, int as_id, tdp_ptep_t sptep,
u64 old_spte, u64 new_spte, gfn_t gfn, int level)
{
+ struct kvm_mmu_page *sp = sptep_to_sp(rcu_dereference(sptep));
+
lockdep_assert_held_write(&kvm->mmu_lock);
/*
@@ -704,7 +705,7 @@ static u64 tdp_mmu_set_spte(struct kvm *kvm, int as_id, tdp_ptep_t sptep,
old_spte = kvm_tdp_mmu_write_spte(sptep, old_spte, new_spte, level);
- handle_changed_spte(kvm, as_id, sptep, gfn, old_spte, new_spte, level, false);
+ handle_changed_spte(kvm, sp, gfn, old_spte, new_spte, level, false);
return old_spte;
}
base-commit: f9d48449fbf9aff6cdced4703cdfdfc1d2e49efe
--
> Passing in "bool is_mirror_sp" to __handle_changed_spte() instead?
^ permalink raw reply related
* Re: [PATCH 4/5] dma-buf: heaps: allow heap to specify valid heap flags
From: John Stultz @ 2026-02-10 20:05 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Jiri Pirko, dri-devel, linaro-mm-sig, iommu, linux-media,
sumit.semwal, benjamin.gaignard, Brian.Starkey, tjmercier,
christian.koenig, m.szyprowski, robin.murphy, jgg, sean.anderson,
ptesarik, catalin.marinas, aneesh.kumar, suzuki.poulose,
steven.price, thomas.lendacky, john.allen, ashish.kalra,
suravee.suthikulpanit, linux-coco
In-Reply-To: <20260210124819.GC12887@unreal>
On Tue, Feb 10, 2026 at 4:48 AM Leon Romanovsky <leon@kernel.org> wrote:
> On Tue, Feb 10, 2026 at 10:05:14AM +0100, Jiri Pirko wrote:
> > Mon, Feb 09, 2026 at 09:08:03PM +0100, jstultz@google.com wrote:
> > >On Mon, Feb 9, 2026 at 7:38 AM Jiri Pirko <jiri@resnulli.us> wrote:
> > >>
> > >> From: Jiri Pirko <jiri@nvidia.com>
> > >>
> > >> Currently the flags, which are unused, are validated for all heaps.
> > >> Since the follow-up patch introduces a flag valid for only one of the
> > >> heaps, allow to specify the valid flags per-heap.
> > >
> > >I'm not really in this space anymore, so take my feedback with a grain of salt.
> > >
> > >While the heap allocate flags argument is unused, it was intended to
> > >be used for generic allocation flags that would apply to all or at
> > >least a wide majority of heaps.
> > >
> > >It was definitely not added to allow for per-heap or heap specific
> > >flags (as this patch tries to utilize it). That was the mess we had
> > >with ION driver that we were trying to avoid.
> > >
> > >The intent of dma-buf heaps is to try to abstract all the different
> > >device memory constraints so there only needs to be a [usage] ->
> > >[heap] mapping, and otherwise userland can be generalized so that it
> > >doesn't need to be re-written to work with different devices/memory
> > >types. Adding heap-specific allocation flags prevents that
> > >generalization.
> > >
> > >So instead of adding heap specific flags, the general advice has been
> > >to add a separate heap name for the flag property.
> >
> > Right, my original idea was to add a separate heap. Then I spotted the
> > flags and seemed like a great fit. Was not aware or the history or
> > original intention. Would be probably good to document it for
> > future generations.
> >
> > So instead of flag, I will add heap named something
> > like "system_cc_decrypted" to implement this.
>
> It is problematic to expose a user‑visible API that depends on a name.
> Such a design limits our ability to extend the functionality in the
> future, should new use cases arise.
Yes, how userland chooses a heap name is an open problem.
The difficulty is that userland is the only thing that knows what
devices the buffer will be shared (and this knowledge may be
incomplete if userland passes a buffer between processes) with, so it
has to pick. But the kernel doesn't give it a way to solve the
constraints of what memory types work with what devices. There have
been some proposals for device sysfs directories to have links to heap
types they support, but that also requires every driver to understand
every heap type. And then you get to the fact that performance is what
folks really want, not compatibility and that may require some system
specific knowledge to decide.
The working solution right now is to have the system provide a [use]
-> [heap] mapping for a specific system.
I think of this as similar to the vfs and /etc/fstab. So /home/ might
be /dev/sdb1 on one device or dev/sda1 on another. You need some
system specific configuration.
In Android, this mapping is done by Gralloc, so buffers are requested
for a use and then Gralloc decides which heap to allocated from.
Unfortunately there doesn't seem to be a similar standard convention
elsewhere. And I'll admit even then the enumeration of uses/pipelines
in some general form is also difficult problem (and is somewhat more
bounded for Android).
thanks
-john
^ permalink raw reply
* Re: [RFC PATCH v5 16/45] x86/virt/tdx: Add tdx_alloc/free_control_page() helpers
From: Edgecombe, Rick P @ 2026-02-10 22:15 UTC (permalink / raw)
To: Hansen, Dave, seanjc@google.com, x86@kernel.org,
dave.hansen@linux.intel.com, kas@kernel.org, bp@alien8.de,
mingo@redhat.com, tglx@kernel.org, pbonzini@redhat.com
Cc: Huang, Kai, 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: <655724f8-0098-40ee-a097-ce4c0249933d@intel.com>
On Tue, 2026-02-10 at 09:44 -0800, Dave Hansen wrote:
> This looks funky.
>
> Right now, this is:
>
> spin_lock(pamt_lock)
> atomic_inc/dec(fine-grained-refcount)
> tdcall_blah_blah()
> spin_unlock(pamt_lock)
>
> Where it *always* acquires the global lock when DPAMT is supported.
> Couldn't we optimize it so that it only acquires it when it has to keep
> the refcount stable at zero?
>
> Roughly:
>
> slow_path = atomic_dec_and_lock(fine-grained-refcount,
> pamt_lock)
> if (!slow_path)
> goto out;
>
> // fine-grained-refcount==0 and must stay that way with
> // pamt_lock held. Remove the DPAMT pages:
> tdh_phymem_pamt_remove(page, pamt_pa_array)
> out:
> spin_unlock(pamt_lock)
>
> On the acquire side, you do:
>
> fast_path = atomic_inc_not_zero(fine-grained-refcount)
> if (fast_path)
> return;
>
> // slow path:
> spin_lock(pamt_lock)
>
> // Was the race lost with another 0=>1 increment?
> if (atomic_read(fine-grained-refcount) > 0)
> goto out_inc
>
> tdh_phymem_pamt_add(page, pamt_pa_array)
> // Inc after the TDCALL so another thread won't race ahead of us
> // and try to use a non-existent PAMT entry
> out_inc:
> atomic_inc(fine-grained-refcount)
> spin_unlock(pamt_lock)
>
> Then, at least only the 0=>1 and 1=>0 transitions need the global lock.
> The fast paths only touch the refcount which isn't shared nearly as much
> as the global lock.
This is pretty much what the next patch does "x86/virt/tdx: Optimize
tdx_alloc/free_control_page() helpers", although it doesn't use the
atomic_dec_and_lock() helpers. There are a few extra considerations. The get/put
fast paths can race, so inside the lock it has to double check that work or
otherwise handle the race. This lead the code to be complex enough that it was
split into too patches "dumb and correct" and "smart and complicated".
I'm wasn't familiar with atomic_dec_and_lock(). I'm guess the atomic part
doesn't cover both decrementing *and* taking the lock?
^ permalink raw reply
* Re: [RFC PATCH v5 16/45] x86/virt/tdx: Add tdx_alloc/free_control_page() helpers
From: Dave Hansen @ 2026-02-10 22:19 UTC (permalink / raw)
To: Edgecombe, Rick P, seanjc@google.com, x86@kernel.org,
dave.hansen@linux.intel.com, kas@kernel.org, bp@alien8.de,
mingo@redhat.com, tglx@kernel.org, pbonzini@redhat.com
Cc: Huang, Kai, 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: <ebd424718bb0b2754b7cbacb277746a3076faea3.camel@intel.com>
On 2/10/26 14:15, Edgecombe, Rick P wrote:
> I'm wasn't familiar with atomic_dec_and_lock(). I'm guess the atomic
> part doesn't cover both decrementing *and* taking the lock?
Right. Only 1=>0 is under the lock. All other decs are outside the lock.
It doesn't do the atomic and the lock "atomically together" somehow.
^ permalink raw reply
* Re: [PATCH 1/1] firmware: smccc: add support for Live Firmware Activation (LFA)
From: Vedashree Vidwans @ 2026-02-10 22:20 UTC (permalink / raw)
To: Salman Nabi, andre.przywara, sudeep.holla, mark.rutland,
lpieralisi
Cc: ardb, chao.gao, linux-arm-kernel, linux-coco, linux-kernel,
sdonthineni, vsethi, vwadekar
In-Reply-To: <20260119122729.287522-2-salman.nabi@arm.com>
Hello,
I have tested this change on Nvidia server platform using Linux kernel
6.16. My testing was done on the final integrated version, which
includes a platform driver interface built on top of this patch. I will
be posting the platform driver patch to LKML separately.
The final driver has been validated on target platform, except for the
sysfs interface which was not excercised during testing.
Tested-by: Vedashree Vidwans <vvidwans@nvidia.com>
Best,
Veda
On 1/19/26 04:27, 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
>
> 6c0762a6-12f2-4b56-92cb-ba8f633606d9:
> total 0
> --w------- 1 0 0 4096 Jan 19 11:33 activate
> -r--r--r-- 1 0 0 4096 Jan 19 11:33 activation_capable
> -r--r--r-- 1 0 0 4096 Jan 19 11:33 activation_pending
> --w------- 1 0 0 4096 Jan 19 11:33 cancel
> -r--r--r-- 1 0 0 4096 Jan 19 11:33 cpu_rendezvous
> -r--r--r-- 1 0 0 4096 Jan 19 11:33 current_version
> -rw-r--r-- 1 0 0 4096 Jan 19 11:33 force_cpu_rendezvous
> -r--r--r-- 1 0 0 4096 Jan 19 11:33 may_reset_cpu
> -r--r--r-- 1 0 0 4096 Jan 19 11:33 name
> -r--r--r-- 1 0 0 4096 Jan 19 11:33 pending_version
> /sys/firmware/lfa/6c0762a6-12f2-4b56-92cb-ba8f633606d9 # grep . *
> grep: activate: Permission denied
> activation_capable:1
> activation_pending:1
> grep: cancel: Permission denied
> cpu_rendezvous:1
> current_version:0.0
> force_cpu_rendezvous:1
> may_reset_cpu:0
> name:TF-RMM
> pending_version:0.0
> /sys/firmware/lfa/6c0762a6-12f2-4b56-92cb-ba8f633606d9 # echo 1 > activate
> [ 2825.797871] Arm LFA: firmware activation succeeded.
> /sys/firmware/lfa/6c0762a6-12f2-4b56-92cb-ba8f633606d9 #
> ==========================================
>
> [1] https://developer.arm.com/documentation/den0147/latest/
>
> Signed-off-by: Salman Nabi <salman.nabi@arm.com>
> ---
> drivers/firmware/smccc/Kconfig | 8 +
> drivers/firmware/smccc/Makefile | 1 +
> drivers/firmware/smccc/lfa_fw.c | 668 ++++++++++++++++++++++++++++++++
> 3 files changed, 677 insertions(+)
> create mode 100644 drivers/firmware/smccc/lfa_fw.c
>
> diff --git a/drivers/firmware/smccc/Kconfig b/drivers/firmware/smccc/Kconfig
> index 15e7466179a6..ff7ca49486b0 100644
> --- a/drivers/firmware/smccc/Kconfig
> +++ b/drivers/firmware/smccc/Kconfig
> @@ -23,3 +23,11 @@ config ARM_SMCCC_SOC_ID
> help
> Include support for the SoC bus on the ARM SMCCC firmware based
> platforms providing some sysfs information about the SoC variant.
> +
> +config ARM_LFA
> + tristate "Arm Live Firmware activation support"
> + depends on HAVE_ARM_SMCCC_DISCOVERY
> + default y
> + help
> + Include support for triggering Live Firmware Activation, which
> + allows to upgrade certain firmware components without a reboot.
> diff --git a/drivers/firmware/smccc/Makefile b/drivers/firmware/smccc/Makefile
> index 40d19144a860..a6dd01558a94 100644
> --- a/drivers/firmware/smccc/Makefile
> +++ b/drivers/firmware/smccc/Makefile
> @@ -2,3 +2,4 @@
> #
> obj-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY) += smccc.o kvm_guest.o
> obj-$(CONFIG_ARM_SMCCC_SOC_ID) += soc_id.o
> +obj-$(CONFIG_ARM_LFA) += lfa_fw.o
> diff --git a/drivers/firmware/smccc/lfa_fw.c b/drivers/firmware/smccc/lfa_fw.c
> new file mode 100644
> index 000000000000..ce54049b7190
> --- /dev/null
> +++ b/drivers/firmware/smccc/lfa_fw.c
> @@ -0,0 +1,668 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2025 Arm Limited
> + */
> +
> +#include <linux/fs.h>
> +#include <linux/init.h>
> +#include <linux/kobject.h>
> +#include <linux/module.h>
> +#include <linux/stop_machine.h>
> +#include <linux/string.h>
> +#include <linux/sysfs.h>
> +#include <linux/arm-smccc.h>
> +#include <linux/psci.h>
> +#include <uapi/linux/psci.h>
> +#include <linux/uuid.h>
> +#include <linux/array_size.h>
> +#include <linux/list.h>
> +#include <linux/mutex.h>
> +
> +#undef pr_fmt
> +#define pr_fmt(fmt) "Arm LFA: " fmt
> +
> +/* LFA v1.0b0 specification */
> +#define LFA_1_0_FN_BASE 0xc40002e0
> +#define LFA_1_0_FN(n) (LFA_1_0_FN_BASE + (n))
> +
> +#define LFA_1_0_FN_GET_VERSION LFA_1_0_FN(0)
> +#define LFA_1_0_FN_CHECK_FEATURE LFA_1_0_FN(1)
> +#define LFA_1_0_FN_GET_INFO LFA_1_0_FN(2)
> +#define LFA_1_0_FN_GET_INVENTORY LFA_1_0_FN(3)
> +#define LFA_1_0_FN_PRIME LFA_1_0_FN(4)
> +#define LFA_1_0_FN_ACTIVATE LFA_1_0_FN(5)
> +#define LFA_1_0_FN_CANCEL LFA_1_0_FN(6)
> +
> +/* CALL_AGAIN flags (returned by SMC) */
> +#define LFA_PRIME_CALL_AGAIN BIT(0)
> +#define LFA_ACTIVATE_CALL_AGAIN BIT(0)
> +
> +/* LFA return values */
> +#define LFA_SUCCESS 0
> +#define LFA_NOT_SUPPORTED 1
> +#define LFA_BUSY 2
> +#define LFA_AUTH_ERROR 3
> +#define LFA_NO_MEMORY 4
> +#define LFA_CRITICAL_ERROR 5
> +#define LFA_DEVICE_ERROR 6
> +#define LFA_WRONG_STATE 7
> +#define LFA_INVALID_PARAMETERS 8
> +#define LFA_COMPONENT_WRONG_STATE 9
> +#define LFA_INVALID_ADDRESS 10
> +#define LFA_ACTIVATION_FAILED 11
> +
> +#define LFA_ERROR_STRING(name) \
> + [name] = #name
> +
> +static const char * const lfa_error_strings[] = {
> + LFA_ERROR_STRING(LFA_SUCCESS),
> + LFA_ERROR_STRING(LFA_NOT_SUPPORTED),
> + LFA_ERROR_STRING(LFA_BUSY),
> + LFA_ERROR_STRING(LFA_AUTH_ERROR),
> + LFA_ERROR_STRING(LFA_NO_MEMORY),
> + LFA_ERROR_STRING(LFA_CRITICAL_ERROR),
> + LFA_ERROR_STRING(LFA_DEVICE_ERROR),
> + LFA_ERROR_STRING(LFA_WRONG_STATE),
> + LFA_ERROR_STRING(LFA_INVALID_PARAMETERS),
> + LFA_ERROR_STRING(LFA_COMPONENT_WRONG_STATE),
> + LFA_ERROR_STRING(LFA_INVALID_ADDRESS),
> + LFA_ERROR_STRING(LFA_ACTIVATION_FAILED)
> +};
> +
> +enum image_attr_names {
> + LFA_ATTR_NAME,
> + LFA_ATTR_CURRENT_VERSION,
> + LFA_ATTR_PENDING_VERSION,
> + LFA_ATTR_ACT_CAPABLE,
> + LFA_ATTR_ACT_PENDING,
> + LFA_ATTR_MAY_RESET_CPU,
> + LFA_ATTR_CPU_RENDEZVOUS,
> + LFA_ATTR_FORCE_CPU_RENDEZVOUS,
> + LFA_ATTR_ACTIVATE,
> + LFA_ATTR_CANCEL,
> + LFA_ATTR_NR_IMAGES
> +};
> +
> +struct image_props {
> + struct list_head image_node;
> + const char *image_name;
> + int fw_seq_id;
> + u64 current_version;
> + u64 pending_version;
> + bool activation_capable;
> + bool activation_pending;
> + bool may_reset_cpu;
> + bool cpu_rendezvous;
> + bool cpu_rendezvous_forced;
> + struct kobject *image_dir;
> + struct kobj_attribute image_attrs[LFA_ATTR_NR_IMAGES];
> +};
> +static LIST_HEAD(lfa_fw_images);
> +
> +/* A UUID split over two 64-bit registers */
> +struct uuid_regs {
> + u64 uuid_lo;
> + u64 uuid_hi;
> +};
> +
> +static const struct fw_image_uuid {
> + const char *name;
> + const char *uuid;
> +} fw_images_uuids[] = {
> + {
> + .name = "TF-A BL31 runtime",
> + .uuid = "47d4086d-4cfe-9846-9b95-2950cbbd5a00",
> + },
> + {
> + .name = "BL33 non-secure payload",
> + .uuid = "d6d0eea7-fcea-d54b-9782-9934f234b6e4",
> + },
> + {
> + .name = "TF-RMM",
> + .uuid = "6c0762a6-12f2-4b56-92cb-ba8f633606d9",
> + },
> +};
> +
> +static struct kobject *lfa_dir;
> +static DEFINE_MUTEX(lfa_lock);
> +static struct workqueue_struct *fw_images_update_wq;
> +static struct work_struct fw_images_update_work;
> +
> +static int update_fw_images_tree(void);
> +
> +static void delete_fw_image_node(struct image_props *attrs)
> +{
> + int i;
> +
> + for (i = 0; i < LFA_ATTR_NR_IMAGES; i++)
> + sysfs_remove_file(attrs->image_dir, &attrs->image_attrs[i].attr);
> +
> + kobject_put(attrs->image_dir);
> + list_del(&attrs->image_node);
> + kfree(attrs);
> +}
> +
> +static void remove_invalid_fw_images(struct work_struct *work)
> +{
> + struct image_props *attrs, *tmp;
> +
> + mutex_lock(&lfa_lock);
> +
> + /*
> + * Remove firmware images including directories that are no longer
> + * present in the LFA agent after updating the existing ones.
> + */
> + list_for_each_entry_safe(attrs, tmp, &lfa_fw_images, image_node) {
> + if (attrs->fw_seq_id == -1)
> + delete_fw_image_node(attrs);
> + }
> +
> + mutex_unlock(&lfa_lock);
> +}
> +
> +static void set_image_flags(struct image_props *attrs, int seq_id,
> + u32 image_flags, u64 reg_current_ver,
> + u64 reg_pending_ver)
> +{
> + attrs->fw_seq_id = seq_id;
> + attrs->current_version = reg_current_ver;
> + attrs->pending_version = reg_pending_ver;
> + attrs->activation_capable = !!(image_flags & BIT(0));
> + attrs->activation_pending = !!(image_flags & BIT(1));
> + attrs->may_reset_cpu = !!(image_flags & BIT(2));
> + /* cpu_rendezvous_optional bit has inverse logic in the spec */
> + attrs->cpu_rendezvous = !(image_flags & BIT(3));
> +}
> +
> +static unsigned long get_nr_lfa_components(void)
> +{
> + struct arm_smccc_1_2_regs reg = { 0 };
> +
> + reg.a0 = LFA_1_0_FN_GET_INFO;
> + reg.a1 = 0; /* lfa_info_selector = 0 */
> +
> + arm_smccc_1_2_invoke(®, ®);
> + if (reg.a0 != LFA_SUCCESS)
> + return reg.a0;
> +
> + return reg.a1;
> +}
> +
> +static int lfa_cancel(void *data)
> +{
> + struct image_props *attrs = data;
> + struct arm_smccc_1_2_regs reg = { 0 };
> +
> + reg.a0 = LFA_1_0_FN_CANCEL;
> + reg.a1 = attrs->fw_seq_id;
> + arm_smccc_1_2_invoke(®, ®);
> +
> + /*
> + * When firmware activation is called with "skip_cpu_rendezvous=1",
> + * LFA_CANCEL can fail with LFA_BUSY if the activation could not be
> + * cancelled.
> + */
> + if (reg.a0 == LFA_SUCCESS) {
> + pr_info("Activation cancelled for image %s\n",
> + attrs->image_name);
> + } else {
> + pr_err("Firmware activation could not be cancelled: %s\n",
> + lfa_error_strings[-reg.a0]);
> + return -EINVAL;
> + }
> +
> + return reg.a0;
> +}
> +
> +static int call_lfa_activate(void *data)
> +{
> + struct image_props *attrs = data;
> + struct arm_smccc_1_2_regs reg = { 0 };
> +
> + reg.a0 = LFA_1_0_FN_ACTIVATE;
> + reg.a1 = attrs->fw_seq_id; /* fw_seq_id under consideration */
> + /*
> + * As we do not support updates requiring a CPU reset (yet),
> + * we pass 0 in reg.a3 and reg.a4, holding the entry point and context
> + * ID respectively.
> + * cpu_rendezvous_forced is set by the administrator, via sysfs,
> + * cpu_rendezvous is dictated by each firmware component.
> + */
> + reg.a2 = !(attrs->cpu_rendezvous_forced || attrs->cpu_rendezvous);
> +
> + for (;;) {
> + arm_smccc_1_2_invoke(®, ®);
> +
> + if ((long)reg.a0 < 0) {
> + pr_err("ACTIVATE for image %s failed: %s\n",
> + attrs->image_name, lfa_error_strings[-reg.a0]);
> + return reg.a0;
> + }
> + if (!(reg.a1 & LFA_ACTIVATE_CALL_AGAIN))
> + break; /* ACTIVATE successful */
> + }
> +
> + return reg.a0;
> +}
> +
> +static int activate_fw_image(struct image_props *attrs)
> +{
> + int ret;
> +
> + mutex_lock(&lfa_lock);
> + if (attrs->cpu_rendezvous_forced || attrs->cpu_rendezvous)
> + ret = stop_machine(call_lfa_activate, attrs, cpu_online_mask);
> + else
> + ret = call_lfa_activate(attrs);
> +
> + if (ret != 0) {
> + mutex_unlock(&lfa_lock);
> + return lfa_cancel(attrs);
> + }
> +
> + /*
> + * Invalidate fw_seq_ids (-1) for all images as the seq_ids and the
> + * number of firmware images in the LFA agent may change after a
> + * successful activation attempt. Negate all image flags as well.
> + */
> + attrs = NULL;
> + list_for_each_entry(attrs, &lfa_fw_images, image_node) {
> + set_image_flags(attrs, -1, 0b1000, 0, 0);
> + }
> +
> + update_fw_images_tree();
> +
> + /*
> + * Removing non-valid image directories at the end of an activation.
> + * We can't remove the sysfs attributes while in the respective
> + * _store() handler, so have to postpone the list removal to a
> + * workqueue.
> + */
> + INIT_WORK(&fw_images_update_work, remove_invalid_fw_images);
> + queue_work(fw_images_update_wq, &fw_images_update_work);
> + mutex_unlock(&lfa_lock);
> +
> + return ret;
> +}
> +
> +static int prime_fw_image(struct image_props *attrs)
> +{
> + struct arm_smccc_1_2_regs reg = { 0 };
> + int ret;
> +
> + mutex_lock(&lfa_lock);
> + /* Avoid SMC calls on invalid firmware images */
> + if (attrs->fw_seq_id == -1) {
> + pr_err("Arm LFA: Invalid firmware sequence id\n");
> + mutex_unlock(&lfa_lock);
> +
> + return -ENODEV;
> + }
> +
> + if (attrs->may_reset_cpu) {
> + pr_err("CPU reset not supported by kernel driver\n");
> + mutex_unlock(&lfa_lock);
> +
> + return -EINVAL;
> + }
> +
> + /*
> + * LFA_PRIME/ACTIVATE will return 1 in reg.a1 if the firmware
> + * priming/activation is still in progress. In that case
> + * LFA_PRIME/ACTIVATE will need to be called again.
> + * reg.a1 will become 0 once the prime/activate process completes.
> + */
> + reg.a0 = LFA_1_0_FN_PRIME;
> + reg.a1 = attrs->fw_seq_id; /* fw_seq_id under consideration */
> + for (;;) {
> + arm_smccc_1_2_invoke(®, ®);
> +
> + if ((long)reg.a0 < 0) {
> + pr_err("LFA_PRIME for image %s failed: %s\n",
> + attrs->image_name, lfa_error_strings[-reg.a0]);
> + mutex_unlock(&lfa_lock);
> +
> + return reg.a0;
> + }
> + if (!(reg.a1 & LFA_PRIME_CALL_AGAIN)) {
> + ret = 0;
> + break; /* PRIME successful */
> + }
> + }
> +
> + mutex_unlock(&lfa_lock);
> + return ret;
> +}
> +
> +static ssize_t name_show(struct kobject *kobj, struct kobj_attribute *attr,
> + char *buf)
> +{
> + struct image_props *attrs = container_of(attr, struct image_props,
> + image_attrs[LFA_ATTR_NAME]);
> +
> + return sysfs_emit(buf, "%s\n", attrs->image_name);
> +}
> +
> +static ssize_t activation_capable_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct image_props *attrs = container_of(attr, struct image_props,
> + image_attrs[LFA_ATTR_ACT_CAPABLE]);
> +
> + return sysfs_emit(buf, "%d\n", attrs->activation_capable);
> +}
> +
> +static ssize_t activation_pending_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct image_props *attrs = container_of(attr, struct image_props,
> + image_attrs[LFA_ATTR_ACT_PENDING]);
> + struct arm_smccc_1_2_regs reg = { 0 };
> +
> + /*
> + * Activation pending status can change anytime thus we need to update
> + * and return its current value
> + */
> + reg.a0 = LFA_1_0_FN_GET_INVENTORY;
> + reg.a1 = attrs->fw_seq_id;
> + arm_smccc_1_2_invoke(®, ®);
> + if (reg.a0 == LFA_SUCCESS)
> + attrs->activation_pending = !!(reg.a3 & BIT(1));
> +
> + return sysfs_emit(buf, "%d\n", attrs->activation_pending);
> +}
> +
> +static ssize_t may_reset_cpu_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct image_props *attrs = container_of(attr, struct image_props,
> + image_attrs[LFA_ATTR_MAY_RESET_CPU]);
> +
> + return sysfs_emit(buf, "%d\n", attrs->may_reset_cpu);
> +}
> +
> +static ssize_t cpu_rendezvous_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct image_props *attrs = container_of(attr, struct image_props,
> + image_attrs[LFA_ATTR_CPU_RENDEZVOUS]);
> +
> + return sysfs_emit(buf, "%d\n", attrs->cpu_rendezvous);
> +}
> +
> +static ssize_t force_cpu_rendezvous_store(struct kobject *kobj,
> + struct kobj_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct image_props *attrs = container_of(attr, struct image_props,
> + image_attrs[LFA_ATTR_FORCE_CPU_RENDEZVOUS]);
> + int ret;
> +
> + ret = kstrtobool(buf, &attrs->cpu_rendezvous_forced);
> + if (ret)
> + return ret;
> +
> + return count;
> +}
> +
> +static ssize_t force_cpu_rendezvous_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct image_props *attrs = container_of(attr, struct image_props,
> + image_attrs[LFA_ATTR_FORCE_CPU_RENDEZVOUS]);
> +
> + return sysfs_emit(buf, "%d\n", attrs->cpu_rendezvous_forced);
> +}
> +
> +static ssize_t current_version_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct image_props *attrs = container_of(attr, struct image_props,
> + image_attrs[LFA_ATTR_CURRENT_VERSION]);
> + u32 maj, min;
> +
> + maj = attrs->current_version >> 32;
> + min = attrs->current_version & 0xffffffff;
> + return sysfs_emit(buf, "%u.%u\n", maj, min);
> +}
> +
> +static ssize_t pending_version_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + struct image_props *attrs = container_of(attr, struct image_props,
> + image_attrs[LFA_ATTR_ACT_PENDING]);
> + struct arm_smccc_1_2_regs reg = { 0 };
> + u32 maj, min;
> +
> + /*
> + * Similar to activation pending, this value can change following an
> + * update, we need to retrieve fresh info instead of stale information.
> + */
> + reg.a0 = LFA_1_0_FN_GET_INVENTORY;
> + reg.a1 = attrs->fw_seq_id;
> + arm_smccc_1_2_invoke(®, ®);
> + if (reg.a0 == LFA_SUCCESS) {
> + if (reg.a5 != 0 && attrs->activation_pending)
> + {
> + attrs->pending_version = reg.a5;
> + maj = reg.a5 >> 32;
> + min = reg.a5 & 0xffffffff;
> + }
> + }
> +
> + return sysfs_emit(buf, "%u.%u\n", maj, min);
> +}
> +
> +static ssize_t activate_store(struct kobject *kobj, struct kobj_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct image_props *attrs = container_of(attr, struct image_props,
> + image_attrs[LFA_ATTR_ACTIVATE]);
> + int ret;
> +
> + ret = prime_fw_image(attrs);
> + if (ret) {
> + pr_err("Firmware prime failed: %s\n",
> + lfa_error_strings[-ret]);
> + return -ECANCELED;
> + }
> +
> + ret = activate_fw_image(attrs);
> + if (ret) {
> + pr_err("Firmware activation failed: %s\n",
> + lfa_error_strings[-ret]);
> + return -ECANCELED;
> + }
> +
> + pr_info("Firmware activation succeeded\n");
> +
> + return count;
> +}
> +
> +static ssize_t cancel_store(struct kobject *kobj, struct kobj_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct image_props *attrs = container_of(attr, struct image_props,
> + image_attrs[LFA_ATTR_CANCEL]);
> + int ret;
> +
> + ret = lfa_cancel(attrs);
> + if (ret != 0)
> + return ret;
> +
> + return count;
> +}
> +
> +static struct kobj_attribute image_attrs_group[LFA_ATTR_NR_IMAGES] = {
> + [LFA_ATTR_NAME] = __ATTR_RO(name),
> + [LFA_ATTR_CURRENT_VERSION] = __ATTR_RO(current_version),
> + [LFA_ATTR_PENDING_VERSION] = __ATTR_RO(pending_version),
> + [LFA_ATTR_ACT_CAPABLE] = __ATTR_RO(activation_capable),
> + [LFA_ATTR_ACT_PENDING] = __ATTR_RO(activation_pending),
> + [LFA_ATTR_MAY_RESET_CPU] = __ATTR_RO(may_reset_cpu),
> + [LFA_ATTR_CPU_RENDEZVOUS] = __ATTR_RO(cpu_rendezvous),
> + [LFA_ATTR_FORCE_CPU_RENDEZVOUS] = __ATTR_RW(force_cpu_rendezvous),
> + [LFA_ATTR_ACTIVATE] = __ATTR_WO(activate),
> + [LFA_ATTR_CANCEL] = __ATTR_WO(cancel)
> +};
> +
> +static void clean_fw_images_tree(void)
> +{
> + struct image_props *attrs, *tmp;
> +
> + list_for_each_entry_safe(attrs, tmp, &lfa_fw_images, image_node)
> + delete_fw_image_node(attrs);
> +}
> +
> +static int update_fw_image_node(char *fw_uuid, int seq_id,
> + u32 image_flags, u64 reg_current_ver,
> + u64 reg_pending_ver)
> +{
> + const char *image_name = "(unknown)";
> + struct image_props *attrs;
> + int ret;
> +
> + /*
> + * If a fw_image is already in the images list then we just update
> + * its flags and seq_id instead of trying to recreate it.
> + */
> + list_for_each_entry(attrs, &lfa_fw_images, image_node) {
> + if (!strcmp(attrs->image_dir->name, fw_uuid)) {
> + set_image_flags(attrs, seq_id, image_flags,
> + reg_current_ver, reg_pending_ver);
> + return 0;
> + }
> + }
> +
> + attrs = kzalloc(sizeof(*attrs), GFP_KERNEL);
> + if (!attrs)
> + return -ENOMEM;
> +
> + for (int i = 0; i < ARRAY_SIZE(fw_images_uuids); i++) {
> + if (!strcmp(fw_images_uuids[i].uuid, fw_uuid))
> + image_name = fw_images_uuids[i].name;
> + }
> +
> + attrs->image_dir = kobject_create_and_add(fw_uuid, lfa_dir);
> + if (!attrs->image_dir)
> + return -ENOMEM;
> +
> + INIT_LIST_HEAD(&attrs->image_node);
> + attrs->image_name = image_name;
> + attrs->cpu_rendezvous_forced = 1;
> + set_image_flags(attrs, seq_id, image_flags, reg_current_ver,
> + reg_pending_ver);
> +
> + /*
> + * The attributes for each sysfs file are constant (handler functions,
> + * name and permissions are the same within each directory), but we
> + * need a per-directory copy regardless, to get a unique handle
> + * for each directory, so that container_of can do its magic.
> + * Also this requires an explicit sysfs_attr_init(), since it's a new
> + * copy, to make LOCKDEP happy.
> + */
> + memcpy(attrs->image_attrs, image_attrs_group,
> + sizeof(attrs->image_attrs));
> + for (int i = 0; i < LFA_ATTR_NR_IMAGES; i++) {
> + struct attribute *attr = &attrs->image_attrs[i].attr;
> +
> + sysfs_attr_init(attr);
> + ret = sysfs_create_file(attrs->image_dir, attr);
> + if (ret) {
> + pr_err("creating sysfs file for uuid %s: %d\n",
> + fw_uuid, ret);
> + clean_fw_images_tree();
> +
> + return ret;
> + }
> + }
> + list_add(&attrs->image_node, &lfa_fw_images);
> +
> + return ret;
> +}
> +
> +static int update_fw_images_tree(void)
> +{
> + struct arm_smccc_1_2_regs reg = { 0 };
> + struct uuid_regs image_uuid;
> + char image_id_str[40];
> + int ret, num_of_components;
> +
> + num_of_components = get_nr_lfa_components();
> + if (num_of_components <= 0) {
> + pr_err("Error getting number of LFA components\n");
> + return -ENODEV;
> + }
> +
> + for (int i = 0; i < num_of_components; i++) {
> + reg.a0 = LFA_1_0_FN_GET_INVENTORY;
> + reg.a1 = i; /* fw_seq_id under consideration */
> + arm_smccc_1_2_invoke(®, ®);
> + if (reg.a0 == LFA_SUCCESS) {
> + image_uuid.uuid_lo = reg.a1;
> + image_uuid.uuid_hi = reg.a2;
> +
> + snprintf(image_id_str, sizeof(image_id_str), "%pUb",
> + &image_uuid);
> + ret = update_fw_image_node(image_id_str, i,
> + reg.a3, reg.a4, reg.a5);
> + if (ret)
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static int __init lfa_init(void)
> +{
> + struct arm_smccc_1_2_regs reg = { 0 };
> + int err;
> +
> + reg.a0 = LFA_1_0_FN_GET_VERSION;
> + arm_smccc_1_2_invoke(®, ®);
> + if (reg.a0 == -LFA_NOT_SUPPORTED) {
> + pr_info("Live Firmware activation: no firmware agent found\n");
> + return -ENODEV;
> + }
> +
> + fw_images_update_wq = alloc_workqueue("fw_images_update_wq",
> + WQ_UNBOUND | WQ_MEM_RECLAIM, 1);
> + if (!fw_images_update_wq) {
> + pr_err("Live Firmware Activation: Failed to allocate workqueue.\n");
> +
> + return -ENOMEM;
> + }
> +
> + pr_info("Live Firmware Activation: detected v%ld.%ld\n",
> + reg.a0 >> 16, reg.a0 & 0xffff);
> +
> + lfa_dir = kobject_create_and_add("lfa", firmware_kobj);
> + if (!lfa_dir)
> + return -ENOMEM;
> +
> + mutex_lock(&lfa_lock);
> + err = update_fw_images_tree();
> + if (err != 0)
> + kobject_put(lfa_dir);
> +
> + mutex_unlock(&lfa_lock);
> + return err;
> +}
> +module_init(lfa_init);
> +
> +static void __exit lfa_exit(void)
> +{
> + flush_workqueue(fw_images_update_wq);
> + destroy_workqueue(fw_images_update_wq);
> +
> + mutex_lock(&lfa_lock);
> + clean_fw_images_tree();
> + mutex_unlock(&lfa_lock);
> +
> + kobject_put(lfa_dir);
> +}
> +module_exit(lfa_exit);
> +
> +MODULE_DESCRIPTION("ARM Live Firmware Activation (LFA)");
> +MODULE_LICENSE("GPL");
^ permalink raw reply
* [PATCH 0/2] Arm LFA: timeout and ACPI platform driver support
From: Vedashree Vidwans @ 2026-02-10 22:40 UTC (permalink / raw)
To: salman.nabi, sudeep.holla, andre.przywara, lpieralisi,
mark.rutland, trilokkumar.soni
Cc: ardb, chao.gao, linux-arm-kernel, linux-coco, linux-kernel,
sdonthineni, vsethi, vwadekar, Vedashree Vidwans
Hello,
(This is an updated version of the [RFC PATCH 0/5] Arm LFA: Improvements
and interrupt support [1], which builds on top of the latest [PATCH 0/1]
Arm Live Firmware activation (LFA) support [2].)
The latest LFA specification [3] updates the interface requirements for
ACPI-based platforms to use the ACPI Notify() signal, and
device-tree-based interface is unspecified. This series focuses on
reworking the LFA driver as an ACPI-backed platform driver, using ACPI
Notify() instead of a dedicated interrupt handler. The LFA core behavior
and sysfs layout remain as implemented by the base driver.
This series contains two incremental changes:
1. Add a timeout and watchdog touch during LFA operations, to make the
driver more robust in cases where firmware-side prime/activation phases
take longer than expected.
2. Register the LFA implementation as a platform driver, layering a
platform driver interface on top of the existing LFA core logic so the
functionality can be instantiated via a platform device.
Note:
This posting focuses on architectural and implementation improvements
for the LFA driver itself. It assumes that the bugs and issues raised
during review of the original "[PATCH 0/1] Arm Live Firmware activation
(LFA) support” [2] will be addressed directly by the author in that base
series. Once those fixes are in place, this series is intended to layer
on top cleanly.
Testing:
The final integrated driver (base LFA + these additions) has been tested
on Nvidia server platform with Linux kernel v6.16. The sysfs interface
was not exercised as part of this testing.
Regards,
Veda
[1] https://lore.kernel.org/linux-arm-kernel/20251208221319.1524888-1-vvidwans@nvidia.com/
[2] https://lore.kernel.org/linux-arm-kernel/20260119122729.287522-2-salman.nabi@arm.com/
[3] https://developer.arm.com/documentation/den0147/latest/
Vedashree Vidwans (2):
firmware: smccc: add timeout, touch wdt
firmware: smccc: register as platform driver
drivers/firmware/smccc/lfa_fw.c | 193 ++++++++++++++++++++++++++++----
1 file changed, 174 insertions(+), 19 deletions(-)
--
2.43.0
^ 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