* Re: [PATCH 1/2] x86/split_lock: Don't try to handle user split lock in TDX guest
From: Kiryl Shutsemau @ 2025-11-27 16:16 UTC (permalink / raw)
To: Xiaoyao Li
Cc: Dave Hansen, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin, Rick Edgecombe, linux-kernel, linux-coco, kvm,
Reinette Chatre, Chenyi Qiang, chao.p.peng
In-Reply-To: <0f8983e9-0e23-4a05-8015-de6e2218d8a5@intel.com>
On Thu, Nov 27, 2025 at 10:00:58AM +0800, Xiaoyao Li wrote:
> On 11/26/2025 9:35 PM, Kiryl Shutsemau wrote:
> > On Wed, Nov 26, 2025 at 08:17:18PM +0800, Xiaoyao Li wrote:
> > > On 11/26/2025 7:25 PM, Kiryl Shutsemau wrote:
> > > > On Wed, Nov 26, 2025 at 06:02:03PM +0800, Xiaoyao Li wrote:
> > > > > When the host enables split lock detection feature, the split lock from
> > > > > guests (normal or TDX) triggers #AC. The #AC caused by split lock access
> > > > > within a normal guest triggers a VM Exit and is handled in the host.
> > > > > The #AC caused by split lock access within a TDX guest does not trigger
> > > > > a VM Exit and instead it's delivered to the guest self.
> > > > >
> > > > > The default "warning" mode of handling split lock depends on being able
> > > > > to temporarily disable detection to recover from the split lock event.
> > > > > But the MSR that disables detection is not accessible to a guest.
> > > > >
> > > > > This means that TDX guests today can not disable the feature or use
> > > > > the "warning" mode (which is the default). But, they can use the "fatal"
> > > > > mode.
> > > > >
> > > > > Force TDX guests to use the "fatal" mode.
> > > > >
> > > > > Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
> > > > > ---
> > > > > arch/x86/kernel/cpu/bus_lock.c | 17 ++++++++++++++++-
> > > > > 1 file changed, 16 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/arch/x86/kernel/cpu/bus_lock.c b/arch/x86/kernel/cpu/bus_lock.c
> > > > > index 981f8b1f0792..f278e4ea3dd4 100644
> > > > > --- a/arch/x86/kernel/cpu/bus_lock.c
> > > > > +++ b/arch/x86/kernel/cpu/bus_lock.c
> > > > > @@ -315,9 +315,24 @@ void bus_lock_init(void)
> > > > > wrmsrq(MSR_IA32_DEBUGCTLMSR, val);
> > > > > }
> > > > > +static bool split_lock_fatal(void)
> > > > > +{
> > > > > + if (sld_state == sld_fatal)
> > > > > + return true;
> > > > > +
> > > > > + /*
> > > > > + * TDX guests can not disable split lock detection.
> > > > > + * Force them into the fatal behavior.
> > > > > + */
> > > > > + if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
> > > > > + return true;
> > > > > +
> > > > > + return false;
> > > > > +}
> > > > > +
> > > > > bool handle_user_split_lock(struct pt_regs *regs, long error_code)
> > > > > {
> > > > > - if ((regs->flags & X86_EFLAGS_AC) || sld_state == sld_fatal)
> > > > > + if ((regs->flags & X86_EFLAGS_AC) || split_lock_fatal())
> > > > > return false;
> > > >
> > > > Maybe it would be cleaner to make it conditional on
> > > > cpu_model_supports_sld instead of special-casing TDX guest?
> > > >
> > > > #AC on any platfrom when we didn't asked for it suppose to be fatal, no?
> > >
> > > But TDX is the only one has such special non-architectural behavior.
> > >
> > > For example, for normal VMs under KVM, the behavior is x86 architectural.
> > > MSR_TEST_CTRL is not accessible to normal VMs, and no split lock #AC will be
> > > delivered to the normal VMs because it's handled by KVM.
> >
> > How does it contradict what I suggested?
> >
> > For both normal VMs and TDX guest, cpu_model_supports_sld will not be
> > set to true. So check for cpu_model_supports_sld here is going to be
> > NOP, unless #AC actually delivered, like we have in TDX case. Handling
> > it as fatal is sane behaviour in such case regardless if it TDX.
> >
> > And we don't need to make the check explicitly about TDX guest.
>
> Well, it depends on how defensive we would like to be, and whether to
> specialize or commonize the issue.
>
> Either can work. If the preference and agreement are to commonize the issue,
> I can do it in v2. And in this direction, what should we do with the patch
> 2? just drop it since it's specialized for TDX ?
I am not sure. Leaving it as produces produces false messages which is
not good, but not critical.
Maybe just clear X86_FEATURE_BUS_LOCK_DETECT and stop pretending we
control split-lock behaviour from the guest?
--
Kiryl Shutsemau / Kirill A. Shutemov
^ permalink raw reply
* Re: [PATCH v4 07/16] x86/virt/tdx: Add tdx_alloc/free_page() helpers
From: Nikolay Borisov @ 2025-11-27 16:11 UTC (permalink / raw)
To: Rick Edgecombe, bp, chao.gao, dave.hansen, isaku.yamahata,
kai.huang, kas, kvm, linux-coco, linux-kernel, mingo, pbonzini,
seanjc, tglx, vannapurve, x86, yan.y.zhao, xiaoyao.li, binbin.wu
In-Reply-To: <20251121005125.417831-8-rick.p.edgecombe@intel.com>
On 21.11.25 г. 2:51 ч., Rick Edgecombe wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>
> Add helpers to use when allocating or preparing pages that need DPAMT
> backing. Make them handle races internally for the case of multiple
> callers trying operate on the same 2MB range simultaneously.
>
> While the TDX initialization code in arch/x86 uses pages with 2MB
> alignment, KVM will need to hand 4KB pages for it to use. Under DPAMT,
> these pages will need DPAMT backing 4KB backing.
>
> Add tdx_alloc_page() and tdx_free_page() to handle both page allocation
> and DPAMT installation. Make them behave like normal alloc/free functions
> where allocation can fail in the case of no memory, but free (with any
> necessary DPAMT release) always succeeds. Do this so they can support the
> existing TDX flows that require cleanups to succeed. Also create
> tdx_pamt_put()/tdx_pamt_get() to handle installing DPAMT 4KB backing for
> pages that are already allocated (such as external page tables, or S-EPT
> pages).
>
> Allocate the pages as GFP_KERNEL_ACCOUNT based on that the allocations
> will be easily user triggerable.
>
> Since the source of these pages is the page allocator, multiple TDs could
> each get 4KB pages that are covered by the same 2MB range. When this
> happens only one page pair needs to be installed to cover the 2MB range.
> Similarly, when one page is freed, the DPAMT backing cannot be freed until
> all TDX pages in the range are no longer in use. Have the helpers manage
> these races internally.
>
> So the requirements are that:
>
> 1. Free path cannot fail (i.e. no TDX module BUSY errors).
> 2. Allocation paths need to handle finding that DPAMT backing is already
> installed, and only return an error in the case of no memory, not in the
> case of losing races with other’s trying to operate on the same DPAMT
> range.
> 3. Free paths cannot fail, and also need to clean up the DPAMT backing
> when the last page in the 2MB range is no longer needed by TDX.
>
> Previous changes allocated refcounts to be used to track how many 4KB
> pages are in use by TDX for each 2MB region. So update those inside the
> helpers and use them to decide when to actually install the DPAMT backing
> pages.
>
> tdx_pamt_put() needs to guarantee the DPAMT is installed before returning
> so that racing threads don’t tell the TDX module to operate on the page
> before it’s installed. Take a lock while adjusting the refcount and doing
> the actual TDH.PHYMEM.PAMT.ADD/REMOVE to make sure these happen
> atomically. The lock is heavyweight, but will be optimized in future
> changes. Just do the simple solution before any complex improvements.
>
> TDH.PHYMEM.PAMT.ADD/REMOVE take exclusive locks at the granularity each
> 2MB range. A simultaneous attempt to operate on the same 2MB region would
> result in a BUSY error code returned from the SEAMCALL. Since the
> invocation of SEAMCALLs are behind a lock, this won’t conflict.
>
> Besides the contention between TDH.PHYMEM.PAMT.ADD/REMOVE, many other
> SEAMCALLs take the same 2MB granularity locks as shared. This means any
> attempt to operate on the page by the TDX module while simultaneously
> doing PAMT.ADD/REMOVE will result in a BUSY error. This should not happen,
> as the PAMT pages always has to be installed before giving the pages to
> the TDX module anyway.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> [Add feedback, update log]
> Signed-off-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
> v4:
> - Update tdx_find_pamt_refcount() calls to pass PFN and rely on
> internal PMD bucket calculations. Based on changes in previous patch.
> - Pull calculation TDX DPAMT 2MB range arg into helper.
> - Fix alloc_pamt_array() doesn't zero array on allocation failure (Yan)
> - Move "prealloc" comment to future patch. (Kai)
> - Use union for dpamt page array. (Dave)
> - Use sizeof(*args_array) everywhere instead of sizeof(u64) in some
> places. (Dave)
> - Fix refcount inc/dec cases. (Xiaoyao)
> - Rearrange error handling in tdx_pamt_get()/tdx_pamt_put() to remove
> some indented lines.
> - Make alloc_pamt_array() use GFP_KERNEL_ACCOUNT like the pre-fault
> path does later.
>
> v3:
> - Fix hard to follow iteration over struct members.
> - Simplify the code by removing the intermediate lists of pages.
> - Clear PAMT pages before freeing. (Adrian)
> - Rename tdx_nr_pamt_pages(). (Dave)
> - Add comments some comments, but thought the simpler code needed
> less. So not as much as seem to be requested. (Dave)
> - Fix asymmetry in which level of the add/remove helpers global lock is
> held.
> - Split out optimization.
> - Write log.
> - Flatten call hierarchies and adjust errors accordingly.
> ---
> arch/x86/include/asm/shared/tdx.h | 7 +
> arch/x86/include/asm/tdx.h | 8 +-
> arch/x86/virt/vmx/tdx/tdx.c | 258 ++++++++++++++++++++++++++++++
> arch/x86/virt/vmx/tdx/tdx.h | 2 +
> 4 files changed, 274 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
> index 6a1646fc2b2f..cc2f251cb791 100644
> --- a/arch/x86/include/asm/shared/tdx.h
> +++ b/arch/x86/include/asm/shared/tdx.h
> @@ -145,6 +145,13 @@ struct tdx_module_args {
> u64 rsi;
> };
>
> +struct tdx_module_array_args {
> + union {
> + struct tdx_module_args args;
> + u64 args_array[sizeof(struct tdx_module_args) / sizeof(u64)];
> + };
> +};
> +
> /* Used to communicate with the TDX module */
> u64 __tdcall(u64 fn, struct tdx_module_args *args);
> u64 __tdcall_ret(u64 fn, struct tdx_module_args *args);
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index cf51ccd16194..914213123d94 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -135,11 +135,17 @@ static inline bool tdx_supports_dynamic_pamt(const struct tdx_sys_info *sysinfo)
> return false; /* To be enabled when kernel is ready */
> }
>
> +void tdx_quirk_reset_page(struct page *page);
> +
> int tdx_guest_keyid_alloc(void);
> u32 tdx_get_nr_guest_keyids(void);
> void tdx_guest_keyid_free(unsigned int keyid);
>
> -void tdx_quirk_reset_page(struct page *page);
> +int tdx_pamt_get(struct page *page);
> +void tdx_pamt_put(struct page *page);
> +
> +struct page *tdx_alloc_page(void);
> +void tdx_free_page(struct page *page);
>
> struct tdx_td {
> /* TD root structure: */
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index edf9182ed86d..745b308785d6 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -2009,6 +2009,264 @@ u64 tdh_phymem_page_wbinvd_hkid(u64 hkid, struct page *page)
> }
> EXPORT_SYMBOL_GPL(tdh_phymem_page_wbinvd_hkid);
>
> +/* Number PAMT pages to be provided to TDX module per 2M region of PA */
> +static int tdx_dpamt_entry_pages(void)
> +{
> + if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> + return 0;
> +
> + return tdx_sysinfo.tdmr.pamt_4k_entry_size * PTRS_PER_PTE / PAGE_SIZE;
> +}
Isn't this guaranteed to return 2 always as per the ABI? Can't the
allocation of the 2 pages be moved closer to where it's used - in
tdh_phymem_pamt_add which will simplify things a bit?
<snip>
^ permalink raw reply
* Re: [PATCH v4 06/16] x86/virt/tdx: Improve PAMT refcounts allocation for sparse memory
From: Kiryl Shutsemau @ 2025-11-27 16:04 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Li, Xiaoyao, Hansen, Dave, Zhao, Yan Y, Wu, Binbin,
seanjc@google.com, mingo@redhat.com, linux-kernel@vger.kernel.org,
tglx@linutronix.de, Yamahata, Isaku, nik.borisov@suse.com,
pbonzini@redhat.com, Annapurve, Vishal, Gao, Chao, bp@alien8.de,
x86@kernel.org, kirill.shutemov@linux.intel.com
In-Reply-To: <7dd848e5735105ac3bf01b2f2db8b595045f47ad.camel@intel.com>
On Wed, Nov 26, 2025 at 08:47:07PM +0000, Edgecombe, Rick P wrote:
> Kiryl, curious if you have any comments on the below...
>
> On Wed, 2025-11-26 at 16:45 +0200, Nikolay Borisov wrote:
> > > +static int pamt_refcount_populate(pte_t *pte, unsigned long addr, void
> > > *data)
> > > +{
> > > + struct page *page;
> > > + pte_t entry;
> > > +
> > > + page = alloc_page(GFP_KERNEL | __GFP_ZERO);
> > > + if (!page)
> > > return -ENOMEM;
> > >
> > > + entry = mk_pte(page, PAGE_KERNEL);
> > > +
> > > + spin_lock(&init_mm.page_table_lock);
> > > + /*
> > > + * PAMT refcount populations can overlap due to rounding of the
> > > + * start/end pfn. Make sure the PAMT range is only populated once.
> > > + */
> > > + if (pte_none(ptep_get(pte)))
> > > + set_pte_at(&init_mm, addr, pte, entry);
> > > + else
> > > + __free_page(page);
> > > + spin_unlock(&init_mm.page_table_lock);
> >
> > nit: Wouldn't it be better to perform the pte_none() check before doing
> > the allocation thus avoiding needless allocations? I.e do the
> > alloc/mk_pte only after we are 100% sure we are going to use this entry.
>
> Yes, but I'm also wondering why it needs init_mm.page_table_lock at all. Here is
> my reasoning for why it doesn't:
>
> apply_to_page_range() takes init_mm.page_table_lock internally when it modified
> page tables in the address range (vmalloc). It needs to do this to avoid races
> with other allocations that share the upper level page tables, which could be on
> the ends of area that TDX reserves.
>
> But pamt_refcount_populate() is only operating on the PTE's for the address
> range that TDX code already controls. Vmalloc should not free the PMD underneath
> the PTE operation because there is an allocation in any page tables it covers.
> So we can skip the lock and also do the pte_none() check before the page
> allocation as Nikolay suggests.
>
> Same for the depopulate path.
I cannot remember/find a good reason to keep the locking around.
--
Kiryl Shutsemau / Kirill A. Shutemov
^ permalink raw reply
* Re: [PATCH v4 06/16] x86/virt/tdx: Improve PAMT refcounts allocation for sparse memory
From: Kiryl Shutsemau @ 2025-11-27 15:57 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: binbin.wu@linux.intel.com, kvm@vger.kernel.org,
linux-coco@lists.linux.dev, Huang, Kai, Li, Xiaoyao, Hansen, Dave,
Zhao, Yan Y, Wu, Binbin, seanjc@google.com, mingo@redhat.com,
linux-kernel@vger.kernel.org, pbonzini@redhat.com,
Yamahata, Isaku, tglx@linutronix.de, Annapurve, Vishal, Gao, Chao,
bp@alien8.de, x86@kernel.org
In-Reply-To: <21a759efdcfb4429ed952303f7d7143263220b22.camel@intel.com>
On Wed, Nov 26, 2025 at 08:47:19PM +0000, Edgecombe, Rick P wrote:
> On Tue, 2025-11-25 at 11:15 +0800, Binbin Wu wrote:
> > On 11/21/2025 8:51 AM, Rick Edgecombe wrote:
> > [...]
> > > +
> > > +/* Unmap a page from the PAMT refcount vmalloc region */
> > > +static int pamt_refcount_depopulate(pte_t *pte, unsigned long addr, void *data)
> > > +{
> > > + struct page *page;
> > > + pte_t entry;
> > > +
> > > + spin_lock(&init_mm.page_table_lock);
> > > +
> > > + entry = ptep_get(pte);
> > > + /* refcount allocation is sparse, may not be populated */
> >
> > Not sure this comment about "sparse" is accurate since this function is called via
> > apply_to_existing_page_range().
> >
> > And the check for not present just for sanity check?
>
> Yes, I don't see what that comment is referring to. But we do need it, because
> hypothetically the refcount mapping could have failed halfway. So we will have
> pte_none()s for the ranges that didn't get populated. I'll use:
>
> /* Refcount mapping could have failed part way, handle aborted mappings. */
It is possible that we can have holes in physical address space between
0 and max_pfn. You need the check even outside of "failed halfway"
scenario.
--
Kiryl Shutsemau / Kirill A. Shutemov
^ permalink raw reply
* Re: [PATCH v4 07/16] x86/virt/tdx: Add tdx_alloc/free_page() helpers
From: Nikolay Borisov @ 2025-11-27 12:29 UTC (permalink / raw)
To: Rick Edgecombe, bp, chao.gao, dave.hansen, isaku.yamahata,
kai.huang, kas, kvm, linux-coco, linux-kernel, mingo, pbonzini,
seanjc, tglx, vannapurve, x86, yan.y.zhao, xiaoyao.li, binbin.wu
Cc: Kirill A. Shutemov
In-Reply-To: <20251121005125.417831-8-rick.p.edgecombe@intel.com>
On 21.11.25 г. 2:51 ч., Rick Edgecombe wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
>
> Add helpers to use when allocating or preparing pages that need DPAMT
> backing. Make them handle races internally for the case of multiple
> callers trying operate on the same 2MB range simultaneously.
>
> While the TDX initialization code in arch/x86 uses pages with 2MB
> alignment, KVM will need to hand 4KB pages for it to use. Under DPAMT,
> these pages will need DPAMT backing 4KB backing.
That paragraph is rather hard to parse. KVM will need to hand 4k pages
to whom? The tdx init code? Also the last sentence with the 2 "backing"
words is hard to parse. Does it say that the 4k pages that KVM need to
pass must be backed by DPAMT pages i.e like a chicken and egg problem?
>
> Add tdx_alloc_page() and tdx_free_page() to handle both page allocation
> and DPAMT installation. Make them behave like normal alloc/free functions
> where allocation can fail in the case of no memory, but free (with any
> necessary DPAMT release) always succeeds. Do this so they can support the
> existing TDX flows that require cleanups to succeed. Also create
> tdx_pamt_put()/tdx_pamt_get() to handle installing DPAMT 4KB backing for
> pages that are already allocated (such as external page tables, or S-EPT
> pages).
>
<snip>
> +
> +/* Serializes adding/removing PAMT memory */
> +static DEFINE_SPINLOCK(pamt_lock);
> +
> +/* Bump PAMT refcount for the given page and allocate PAMT memory if needed */
> +int tdx_pamt_get(struct page *page)
> +{
> + u64 pamt_pa_array[MAX_TDX_ARG_SIZE(rdx)];
> + atomic_t *pamt_refcount;
> + u64 tdx_status;
> + int ret;
> +
> + if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> + return 0;
> +
> + ret = alloc_pamt_array(pamt_pa_array);
> + if (ret)
> + goto out_free;
> +
> + pamt_refcount = tdx_find_pamt_refcount(page_to_pfn(page));
> +
> + scoped_guard(spinlock, &pamt_lock) {
> + /*
> + * If the pamt page is already added (i.e. refcount >= 1),
> + * then just increment the refcount.
> + */
> + if (atomic_read(pamt_refcount)) {
> + atomic_inc(pamt_refcount);
> + goto out_free;
> + }
Replace this pair of read/inc with a single call to atomic_inc_not_zero()
> +
> + /* Try to add the pamt page and take the refcount 0->1. */
> +
> + tdx_status = tdh_phymem_pamt_add(page, pamt_pa_array);
> + if (!IS_TDX_SUCCESS(tdx_status)) {
> + pr_err("TDH_PHYMEM_PAMT_ADD failed: %#llx\n", tdx_status);
> + goto out_free;
> + }
> +
> + atomic_inc(pamt_refcount);
> + }
> +
> + return ret;
> +out_free:
> + /*
> + * pamt_pa_array is populated or zeroed up to tdx_dpamt_entry_pages()
> + * above. free_pamt_array() can handle either case.
> + */
> + free_pamt_array(pamt_pa_array);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(tdx_pamt_get);
> +
> +/*
> + * Drop PAMT refcount for the given page and free PAMT memory if it is no
> + * longer needed.
> + */
> +void tdx_pamt_put(struct page *page)
> +{
> + u64 pamt_pa_array[MAX_TDX_ARG_SIZE(rdx)];
> + 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;
> + }
nit: Could be replaced with : atomic_add_unless(pamt_refcount, -1, 1);
Probably it would have been better to simply use atomic64_dec_and_test
and if it returns true do the phymem_pamt_remove, but I suspect you
can't do it because in case it fails you don't want to decrement the
last refcount, though that could be remedied by an extra atomic_int in
the failure path. I guess it might be worth simplifying since the extra
inc will only be needed in exceptional cases (we don't expect failure ot
be the usual path) and freeing is not a fast path.
> +
> + /* Try to remove the pamt page and take the refcount 1->0. */
> +
> + tdx_status = tdh_phymem_pamt_remove(page, pamt_pa_array);
> + if (!IS_TDX_SUCCESS(tdx_status)) {
> + pr_err("TDH_PHYMEM_PAMT_REMOVE failed: %#llx\n", tdx_status);
> +
> + /*
> + * Don't free pamt_pa_array as it could hold garbage
> + * when tdh_phymem_pamt_remove() fails.
> + */
> + 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);
> +}
> +EXPORT_SYMBOL_GPL(tdx_pamt_put);
<snip>
^ permalink raw reply
* COCONUT-SVSM Development Release v2025.11-devel
From: Jörg Rödel @ 2025-11-27 10:56 UTC (permalink / raw)
To: coconut-svsm, linux-coco
Hi,
The November development release of COCONUT-SVSM now ready and tagged. This
release features 101 non-merge commits since the October release. Some
highlights of the changes (in no particular order) include:
* Consuming MADT table via IGVM instead of GWCGF on QEMU. This needs an
updated QEMU, so please make sure to use the latest one from the
COCONUT-SVSM github project.
* Some reorganization of repository structure.
* Build system improvements around building the C parts.
* IGVM memory map for OVMF support.
* Improvements to the allocator to allow stage2 to allocate in the
kernel heap before kernel is launched (WIP).
* Updated development plan document.
* Attestation and verification updates.
* Other fixes and documentation updates.
Overall this is a pretty solid release, a big Thanks goes out to all
contributors.
As in the previous release, formal verification does still not work. The
reasons it did not work in the October release have been fixed (Thanks!), but
other changes broke it again. We are working on fixing this for the next
release.
Have fun!
Regards,
Joerg
^ permalink raw reply
* Re: SVSM Development Call November 26th, 2025
From: Jörg Rödel @ 2025-11-27 8:42 UTC (permalink / raw)
To: coconut-svsm, linux-coco
In-Reply-To: <tqu2fjqbhrwsmritqthp7f3qucqljxc3ivxm2wmtdbgdrpt6hm@7qj7ocuwoksw>
Meeting minutes are ready:
https://github.com/coconut-svsm/governance/pull/88
Regards,
Joerg
^ permalink raw reply
* Re: [PATCH v2 11/21] x86/virt/seamldr: Allocate and populate a module update request
From: Binbin Wu @ 2025-11-27 8:39 UTC (permalink / raw)
To: Chao Gao
Cc: linux-coco, linux-kernel, x86, reinette.chatre, ira.weiny,
kai.huang, dan.j.williams, yilun.xu, sagis, vannapurve, paulmck,
nik.borisov, Farrah Chen, Kirill A. Shutemov, Dave Hansen,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin
In-Reply-To: <a8073390-b952-43a1-8e63-18c2a10bb721@linux.intel.com>
On 11/27/2025 4:30 PM, Binbin Wu wrote:
>
>
> On 10/1/2025 10:52 AM, Chao Gao wrote:
> [...]
>> +
>> +/* Allocate and populate a seamldr_params */
>> +static struct seamldr_params *alloc_seamldr_params(const void *module, int module_size,
>> + const void *sig, int sig_size)
>> +{
>> + struct seamldr_params *params;
>> + const u8 *ptr;
>> + int i;
>> +
>> + BUILD_BUG_ON(sizeof(struct seamldr_params) != SZ_4K);
>> + if (module_size > SEAMLDR_MAX_NR_MODULE_4KB_PAGES * SZ_4K)
>> + return ERR_PTR(-EINVAL);
>> +
>> + if (!IS_ALIGNED(module_size, SZ_4K) || !IS_ALIGNED(sig_size, SZ_4K) ||
>> + !IS_ALIGNED((unsigned long)module, SZ_4K) ||
>> + !IS_ALIGNED((unsigned long)sig, SZ_4K))
>> + return ERR_PTR(-EINVAL);
>> +
>> + /* seamldr_params accepts one 4KB-page for sigstruct */
>> + if (sig_size != SZ_4K)
> According to the link [2] you provided above, it seems that the layout of
> tdx_blob as following:
> tdx_blob
> |- u16 version
> |- u16 checksum
> |- u32 offset_of_module --------------------------------------|
> |- u8 signature[8] |
> |- u32 len 8KB + (N * 4KB) |
> |- u32 resv1 |
> |- u64 resv2[509] |
> |- u8 data[] |
> |- _u64 sigstruct[256] //2KB sigstruct |
> |- _u64 reserved2[256] |
> |- _u64 reserved3[N*512] //4KB aligned, optional, N >=0 |
> |- _u8 module[] //<-----------------------------|
Sorry about the mess.
tdx_blob
|- u16 version
|- u16 checksum
|- u32 offset_of_module --------------------------------------|
|- u8 signature[8] |
|- u32 len 8KB + (N * 4KB) |
|- u32 resv1 |
|- u64 resv2[509] |
|- u8 data[] |
|- _u64 sigstruct[256] //2KB sigstruct |
|- _u64 reserved2[256] |
|- _u64 reserved3[N*512] //4KB aligned, optional, N >=0 |
|- _u8 module[] //<-----------------------------|
>
> If N is not 0 for reserved3, then the sig_size passed will not be 4KB.
>
>
^ permalink raw reply
* Re: [PATCH v2 11/21] x86/virt/seamldr: Allocate and populate a module update request
From: Binbin Wu @ 2025-11-27 8:30 UTC (permalink / raw)
To: Chao Gao
Cc: linux-coco, linux-kernel, x86, reinette.chatre, ira.weiny,
kai.huang, dan.j.williams, yilun.xu, sagis, vannapurve, paulmck,
nik.borisov, Farrah Chen, Kirill A. Shutemov, Dave Hansen,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin
In-Reply-To: <20251001025442.427697-12-chao.gao@intel.com>
On 10/1/2025 10:52 AM, Chao Gao wrote:
[...]
> +
> +/* Allocate and populate a seamldr_params */
> +static struct seamldr_params *alloc_seamldr_params(const void *module, int module_size,
> + const void *sig, int sig_size)
> +{
> + struct seamldr_params *params;
> + const u8 *ptr;
> + int i;
> +
> + BUILD_BUG_ON(sizeof(struct seamldr_params) != SZ_4K);
> + if (module_size > SEAMLDR_MAX_NR_MODULE_4KB_PAGES * SZ_4K)
> + return ERR_PTR(-EINVAL);
> +
> + if (!IS_ALIGNED(module_size, SZ_4K) || !IS_ALIGNED(sig_size, SZ_4K) ||
> + !IS_ALIGNED((unsigned long)module, SZ_4K) ||
> + !IS_ALIGNED((unsigned long)sig, SZ_4K))
> + return ERR_PTR(-EINVAL);
> +
> + /* seamldr_params accepts one 4KB-page for sigstruct */
> + if (sig_size != SZ_4K)
According to the link [2] you provided above, it seems that the layout of
tdx_blob as following:
tdx_blob
|- u16 version
|- u16 checksum
|- u32 offset_of_module --------------------------------------|
|- u8 signature[8] |
|- u32 len 8KB + (N * 4KB) |
|- u32 resv1 |
|- u64 resv2[509] |
|- u8 data[] |
|- _u64 sigstruct[256] //2KB sigstruct |
|- _u64 reserved2[256] |
|- _u64 reserved3[N*512] //4KB aligned, optional, N >=0 |
|- _u8 module[] //<-----------------------------|
If N is not 0 for reserved3, then the sig_size passed will not be 4KB.
> + return ERR_PTR(-EINVAL);
> +
> + params = (struct seamldr_params *)get_zeroed_page(GFP_KERNEL);
> + if (!params)
> + return ERR_PTR(-ENOMEM);
> +
> + params->scenario = SEAMLDR_SCENARIO_UPDATE;
> + params->sigstruct_pa = (vmalloc_to_pfn(sig) << PAGE_SHIFT) +
> + ((unsigned long)sig & ~PAGE_MASK);
Since sig is 4KB aligned, is ((unsigned long)sig & ~PAGE_MASK) needed?
> + params->num_module_pages = module_size / SZ_4K;
> +
> + ptr = module;
> + for (i = 0; i < params->num_module_pages; i++) {
> + params->mod_pages_pa_list[i] = (vmalloc_to_pfn(ptr) << PAGE_SHIFT) +
> + ((unsigned long)ptr & ~PAGE_MASK);
Ditto for ptr, very ptr is 4KB aligned.
> + ptr += SZ_4K;
> + }
> +
> + return params;
> +}
> +
> +/*
> + * Intel TDX Module blob. Its format is defined at:
> + * https://github.com/intel/tdx-module-binaries/blob/main/blob_structure.txt
> + */
> +struct tdx_blob {
> + u16 version;
> + u16 checksum;
> + u32 offset_of_module;
> + u8 signature[8];
> + u32 len;
> + u32 resv1;
> + u64 resv2[509];
> + u8 data[];
> +} __packed;
> +
> +/*
> + * Verify that the checksum of the entire blob is zero. The checksum is
> + * calculated by summing up all 16-bit words, with carry bits dropped.
> + */
> +static bool verify_checksum(const struct tdx_blob *blob)
> +{
> + u32 size = blob->len;
> + u16 checksum = 0;
> + const u16 *p;
> + int i;
> +
> + /* Handle the last byte if the size is odd */
> + if (size % 2) {
> + checksum += *((const u8 *)blob + size - 1);
> + size--;
> + }
> +
> + p = (const u16 *)blob;
> + for (i = 0; i < size; i += 2) {
> + checksum += *p;
> + p++;
> + }
> +
> + return !checksum;
> +}
> +
> +static struct seamldr_params *init_seamldr_params(const u8 *data, u32 size)
> +{
> + const struct tdx_blob *blob = (const void *)data;
> + int module_size, sig_size;
> + const void *sig, *module;
> +
> + if (blob->version != 0x100) {
> + pr_err("unsupported blob version: %u\n", blob->version);
Based on the link [2], 0x100 stands for version 1.0, Using hexadecimal seems
more readable.
> + return ERR_PTR(-EINVAL);
> + }
> +
> + if (blob->resv1 || memchr_inv(blob->resv2, 0, sizeof(blob->resv2))) {
> + pr_err("non-zero reserved fields\n");
> + return ERR_PTR(-EINVAL);
> + }
> +
> + /* Split the given blob into a sigstruct and a module */
> + sig = blob->data;
> + sig_size = blob->offset_of_module - sizeof(struct tdx_blob);
> + module = data + blob->offset_of_module;
> + module_size = size - blob->offset_of_module;
> +
> + if (sig_size <= 0 || module_size <= 0 || blob->len != size)
> + return ERR_PTR(-EINVAL);
> +
> + if (memcmp(blob->signature, "TDX-BLOB", 8)) {
> + pr_err("invalid signature\n");
> + return ERR_PTR(-EINVAL);
> + }
> +
> + if (!verify_checksum(blob)) {
> + pr_err("invalid checksum\n");
> + return ERR_PTR(-EINVAL);
> + }
> +
> + return alloc_seamldr_params(module, module_size, sig, sig_size);
> +}
> +
> +DEFINE_FREE(free_seamldr_params, struct seamldr_params *,
> + if (!IS_ERR_OR_NULL(_T)) free_seamldr_params(_T))
> +
> int seamldr_install_module(const u8 *data, u32 size)
> {
> const struct seamldr_info *info = seamldr_get_info();
> @@ -82,6 +232,11 @@ int seamldr_install_module(const u8 *data, u32 size)
> if (!info->num_remaining_updates)
> return -ENOSPC;
>
> + struct seamldr_params *params __free(free_seamldr_params) =
> + init_seamldr_params(data, size);
> + if (IS_ERR(params))
> + return PTR_ERR(params);
> +
> guard(cpus_read_lock)();
> if (!cpumask_equal(cpu_online_mask, cpu_present_mask)) {
> pr_err("Cannot update TDX module if any CPU is offline\n");
^ permalink raw reply
* Re: [PATCH v4 06/16] x86/virt/tdx: Improve PAMT refcounts allocation for sparse memory
From: Nikolay Borisov @ 2025-11-27 7:36 UTC (permalink / raw)
To: Edgecombe, Rick P, kvm@vger.kernel.org,
linux-coco@lists.linux.dev, Huang, Kai, Li, Xiaoyao, Hansen, Dave,
Zhao, Yan Y, Wu, Binbin, kas@kernel.org, seanjc@google.com,
mingo@redhat.com, linux-kernel@vger.kernel.org,
tglx@linutronix.de, Yamahata, Isaku, pbonzini@redhat.com,
Annapurve, Vishal, Gao, Chao, bp@alien8.de, x86@kernel.org
Cc: kirill.shutemov@linux.intel.com
In-Reply-To: <7dd848e5735105ac3bf01b2f2db8b595045f47ad.camel@intel.com>
On 26.11.25 г. 22:47 ч., Edgecombe, Rick P wrote:
> Kiryl, curious if you have any comments on the below...
>
> On Wed, 2025-11-26 at 16:45 +0200, Nikolay Borisov wrote:
>>> +static int pamt_refcount_populate(pte_t *pte, unsigned long addr, void
>>> *data)
>>> +{
>>> + struct page *page;
>>> + pte_t entry;
>>> +
>>> + page = alloc_page(GFP_KERNEL | __GFP_ZERO);
>>> + if (!page)
>>> return -ENOMEM;
>>>
>>> + entry = mk_pte(page, PAGE_KERNEL);
>>> +
>>> + spin_lock(&init_mm.page_table_lock);
>>> + /*
>>> + * PAMT refcount populations can overlap due to rounding of the
>>> + * start/end pfn. Make sure the PAMT range is only populated once.
>>> + */
>>> + if (pte_none(ptep_get(pte)))
>>> + set_pte_at(&init_mm, addr, pte, entry);
>>> + else
>>> + __free_page(page);
>>> + spin_unlock(&init_mm.page_table_lock);
>>
>> nit: Wouldn't it be better to perform the pte_none() check before doing
>> the allocation thus avoiding needless allocations? I.e do the
>> alloc/mk_pte only after we are 100% sure we are going to use this entry.
>
> Yes, but I'm also wondering why it needs init_mm.page_table_lock at all. Here is
> my reasoning for why it doesn't:
>
> apply_to_page_range() takes init_mm.page_table_lock internally when it modified
> page tables in the address range (vmalloc). It needs to do this to avoid races
> with other allocations that share the upper level page tables, which could be on
> the ends of area that TDX reserves.
> > But pamt_refcount_populate() is only operating on the PTE's for the
address
> range that TDX code already controls. Vmalloc should not free the PMD underneath
> the PTE operation because there is an allocation in any page tables it covers.
> So we can skip the lock and also do the pte_none() check before the page
> allocation as Nikolay suggests.
I agree with your analysis but this needs to be described not only in
the commit message but also as a code comment because you intentionally
omit locking since that particular pte (at that point) can only have a
single user so no race conditions are possible.
>
> Same for the depopulate path.
^ permalink raw reply
* Re: [PATCH v4 12/16] x86/virt/tdx: Add helpers to allow for pre-allocating pages
From: Binbin Wu @ 2025-11-27 2:38 UTC (permalink / raw)
To: Edgecombe, Rick P
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Li, Xiaoyao, Hansen, Dave, Zhao, Yan Y, Wu, Binbin,
kas@kernel.org, seanjc@google.com, mingo@redhat.com,
linux-kernel@vger.kernel.org, pbonzini@redhat.com,
Yamahata, Isaku, tglx@linutronix.de, Annapurve, Vishal, Gao, Chao,
bp@alien8.de, x86@kernel.org
In-Reply-To: <af0e05e39229adb814601eb9ce720a0278be3c2f.camel@intel.com>
On 11/27/2025 6:33 AM, Edgecombe, Rick P wrote:
>>>
>>> static int tdx_topup_external_fault_cache(struct kvm_vcpu *vcpu, unsigned int cnt)
>>> {
>>> - struct vcpu_tdx *tdx = to_tdx(vcpu);
>>> + struct tdx_prealloc *prealloc = &to_tdx(vcpu)->prealloc;
>>> + int min_fault_cache_size;
>>>
>>> - return kvm_mmu_topup_memory_cache(&tdx->mmu_external_spt_cache, cnt);
>>> + /* External page tables */
>>> + min_fault_cache_size = cnt;
>>> + /* Dynamic PAMT pages (if enabled) */
>>> + min_fault_cache_size += tdx_dpamt_entry_pages() * PT64_ROOT_MAX_LEVEL;
>> Is the value PT64_ROOT_MAX_LEVEL intended, since dynamic PAMT pages are only
>> needed for 4KB level?
> I'm not sure I follow. We need DPAMT backing for each S-EPT page table.
Oh, right!
IIUIC, PT64_ROOT_MAX_LEVEL is actually
- PT64_ROOT_MAX_LEVEL - 1 for S-ETP pages since root page is not needed.
- 1 for TD private memory page
It's better to add a comment about it.
^ permalink raw reply
* Re: [PATCH 1/2] x86/split_lock: Don't try to handle user split lock in TDX guest
From: Xiaoyao Li @ 2025-11-27 2:00 UTC (permalink / raw)
To: Kiryl Shutsemau
Cc: Dave Hansen, Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin, Rick Edgecombe, linux-kernel, linux-coco, kvm,
Reinette Chatre, Chenyi Qiang, chao.p.peng
In-Reply-To: <qvsi3xht4kn3iwkx5xw2p7zsq4cvpg4xhq3ra52fe34xjpixfo@fsgchsobc343>
On 11/26/2025 9:35 PM, Kiryl Shutsemau wrote:
> On Wed, Nov 26, 2025 at 08:17:18PM +0800, Xiaoyao Li wrote:
>> On 11/26/2025 7:25 PM, Kiryl Shutsemau wrote:
>>> On Wed, Nov 26, 2025 at 06:02:03PM +0800, Xiaoyao Li wrote:
>>>> When the host enables split lock detection feature, the split lock from
>>>> guests (normal or TDX) triggers #AC. The #AC caused by split lock access
>>>> within a normal guest triggers a VM Exit and is handled in the host.
>>>> The #AC caused by split lock access within a TDX guest does not trigger
>>>> a VM Exit and instead it's delivered to the guest self.
>>>>
>>>> The default "warning" mode of handling split lock depends on being able
>>>> to temporarily disable detection to recover from the split lock event.
>>>> But the MSR that disables detection is not accessible to a guest.
>>>>
>>>> This means that TDX guests today can not disable the feature or use
>>>> the "warning" mode (which is the default). But, they can use the "fatal"
>>>> mode.
>>>>
>>>> Force TDX guests to use the "fatal" mode.
>>>>
>>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>>> ---
>>>> arch/x86/kernel/cpu/bus_lock.c | 17 ++++++++++++++++-
>>>> 1 file changed, 16 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/arch/x86/kernel/cpu/bus_lock.c b/arch/x86/kernel/cpu/bus_lock.c
>>>> index 981f8b1f0792..f278e4ea3dd4 100644
>>>> --- a/arch/x86/kernel/cpu/bus_lock.c
>>>> +++ b/arch/x86/kernel/cpu/bus_lock.c
>>>> @@ -315,9 +315,24 @@ void bus_lock_init(void)
>>>> wrmsrq(MSR_IA32_DEBUGCTLMSR, val);
>>>> }
>>>> +static bool split_lock_fatal(void)
>>>> +{
>>>> + if (sld_state == sld_fatal)
>>>> + return true;
>>>> +
>>>> + /*
>>>> + * TDX guests can not disable split lock detection.
>>>> + * Force them into the fatal behavior.
>>>> + */
>>>> + if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
>>>> + return true;
>>>> +
>>>> + return false;
>>>> +}
>>>> +
>>>> bool handle_user_split_lock(struct pt_regs *regs, long error_code)
>>>> {
>>>> - if ((regs->flags & X86_EFLAGS_AC) || sld_state == sld_fatal)
>>>> + if ((regs->flags & X86_EFLAGS_AC) || split_lock_fatal())
>>>> return false;
>>>
>>> Maybe it would be cleaner to make it conditional on
>>> cpu_model_supports_sld instead of special-casing TDX guest?
>>>
>>> #AC on any platfrom when we didn't asked for it suppose to be fatal, no?
>>
>> But TDX is the only one has such special non-architectural behavior.
>>
>> For example, for normal VMs under KVM, the behavior is x86 architectural.
>> MSR_TEST_CTRL is not accessible to normal VMs, and no split lock #AC will be
>> delivered to the normal VMs because it's handled by KVM.
>
> How does it contradict what I suggested?
>
> For both normal VMs and TDX guest, cpu_model_supports_sld will not be
> set to true. So check for cpu_model_supports_sld here is going to be
> NOP, unless #AC actually delivered, like we have in TDX case. Handling
> it as fatal is sane behaviour in such case regardless if it TDX.
>
> And we don't need to make the check explicitly about TDX guest.
Well, it depends on how defensive we would like to be, and whether to
specialize or commonize the issue.
Either can work. If the preference and agreement are to commonize the
issue, I can do it in v2. And in this direction, what should we do with
the patch 2? just drop it since it's specialized for TDX ?
^ permalink raw reply
* Re: [PATCH v4 02/16] x86/tdx: Add helpers to check return status codes
From: Edgecombe, Rick P @ 2025-11-26 23:26 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Li, Xiaoyao, Hansen, Dave, Zhao, Yan Y, Wu, Binbin,
kas@kernel.org, seanjc@google.com, mingo@redhat.com,
pbonzini@redhat.com, tglx@linutronix.de, Yamahata, Isaku,
linux-kernel@vger.kernel.org, Annapurve, Vishal, Gao, Chao,
bp@alien8.de, x86@kernel.org
Cc: kirill.shutemov@linux.intel.com
In-Reply-To: <a2ac55888596ddf081084804c0276e3686515cd4.camel@intel.com>
On Tue, 2025-11-25 at 23:07 +0000, Huang, Kai wrote:
> On Thu, 2025-11-20 at 16:51 -0800, Rick Edgecombe wrote:
> > diff --git a/arch/x86/include/asm/shared/tdx_errno.h b/arch/x86/include/asm/shared/tdx_errno.h
> > index 3aa74f6a6119..e302aed31b50 100644
> > --- a/arch/x86/include/asm/shared/tdx_errno.h
> > +++ b/arch/x86/include/asm/shared/tdx_errno.h
> > @@ -5,7 +5,7 @@
> > #include <asm/trapnr.h>
> >
> > /* Upper 32 bit of the TDX error code encodes the status */
> > -#define TDX_SEAMCALL_STATUS_MASK 0xFFFFFFFF00000000ULL
> > +#define TDX_STATUS_MASK 0xFFFFFFFF00000000ULL
> >
> > /*
> > * TDX SEAMCALL Status Codes
> > @@ -54,4 +54,49 @@
> > #define TDX_OPERAND_ID_SEPT 0x92
> > #define TDX_OPERAND_ID_TD_EPOCH 0xa9
> >
> > +#ifndef __ASSEMBLER__
> > +#include <linux/bits.h>
> > +#include <linux/types.h>
>
> IMHO:
>
> You might want to move #include <linux/bits.h> out of __ASSEMBLER__ to the top
> of this file since macros like GENMASK_ULL() are used by SW-defined error codes
> already. And you might want to move the inclusion of this header to the
> previous patch when these error codes were moved to <asm/shared/tdx_errno.h>.
Yea that makes sense.
>
> You may also move <linux/types.h> out of __ASSEMBLER__ since AFAICT this file is
> assembly inclusion safe.
Sure.
^ permalink raw reply
* Re: [PATCH v4 01/16] x86/tdx: Move all TDX error defines into <asm/shared/tdx_errno.h>
From: Edgecombe, Rick P @ 2025-11-26 23:15 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Li, Xiaoyao, Hansen, Dave, Zhao, Yan Y, Wu, Binbin,
kas@kernel.org, seanjc@google.com, mingo@redhat.com,
pbonzini@redhat.com, tglx@linutronix.de, Yamahata, Isaku,
linux-kernel@vger.kernel.org, Annapurve, Vishal, Gao, Chao,
bp@alien8.de, x86@kernel.org
Cc: kirill.shutemov@linux.intel.com
In-Reply-To: <af7c8f3ec86688709cce550a2fc17110e3fd12b7.camel@intel.com>
On Tue, 2025-11-25 at 22:44 +0000, Huang, Kai wrote:
> Also forgot to say, AFAICT these error codes will also be used by TDX guest,
> therefore you might want to drop the "SEAMCALL" part from the status codes.
Oh, yes. That comment tweak is actually relevent to this patch.
^ permalink raw reply
* Re: [PATCH v4 01/16] x86/tdx: Move all TDX error defines into <asm/shared/tdx_errno.h>
From: Edgecombe, Rick P @ 2025-11-26 23:14 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Li, Xiaoyao, Hansen, Dave, Zhao, Yan Y, Wu, Binbin,
kas@kernel.org, seanjc@google.com, mingo@redhat.com,
pbonzini@redhat.com, tglx@linutronix.de, Yamahata, Isaku,
linux-kernel@vger.kernel.org, Annapurve, Vishal, Gao, Chao,
bp@alien8.de, x86@kernel.org
Cc: kirill.shutemov@linux.intel.com
In-Reply-To: <6968dcb446fb857b3f254030e487d889b464d7ce.camel@intel.com>
On Tue, 2025-11-25 at 22:30 +0000, Huang, Kai wrote:
> > --- a/arch/x86/kvm/vmx/tdx_errno.h
> > +++ b/arch/x86/include/asm/shared/tdx_errno.h
> > @@ -1,14 +1,16 @@
> > /* SPDX-License-Identifier: GPL-2.0 */
> > -/* architectural status code for SEAMCALL */
> > +#ifndef _X86_SHARED_TDX_ERRNO_H
> > +#define _X86_SHARED_TDX_ERRNO_H
>
> AFAICT other files under asm/shared/ all use
>
> #ifdef _ASM_X86_SHARED_xx_H
> ...
>
> I guess it's better to follow this pattern.
Oh, wow you're right. Will update it.
>
> >
> > -#ifndef __KVM_X86_TDX_ERRNO_H
> > -#define __KVM_X86_TDX_ERRNO_H
> > +#include <asm/trapnr.h>
> >
> > +/* Upper 32 bit of the TDX error code encodes the status */
> > #define TDX_SEAMCALL_STATUS_MASK 0xFFFFFFFF00000000ULL
> >
> > /*
> > - * TDX SEAMCALL Status Codes (returned in RAX)
> > + * TDX SEAMCALL Status Codes
>
> Nit:
>
> I don't quite follow this change. Just curious: is it because "returned in
> RAX"
> doesn't apply to all error codes any more?
This change was originally from Kiryl. I'm not sure. AFAIK they should all be
RAX. I'd be fine to remove it, but I do think the RAX part is a bit extraneous.
But this patch doesn't require it, so happy to shrink the diff.
>
> > */
> > +#define TDX_SUCCESS 0ULL
> > #define TDX_NON_RECOVERABLE_VCPU 0x4000000100000000ULL
> > #define TDX_NON_RECOVERABLE_TD 0x4000000200000000ULL
> > #define TDX_NON_RECOVERABLE_TD_NON_ACCESSIBLE 0x6000000500000000ULL
> > @@ -17,6 +19,7 @@
> > #define TDX_OPERAND_INVALID 0xC000010000000000ULL
> > #define TDX_OPERAND_BUSY 0x8000020000000000ULL
> > #define TDX_PREVIOUS_TLB_EPOCH_BUSY 0x8000020100000000ULL
> > +#define TDX_RND_NO_ENTROPY 0x8000020300000000ULL
> > #define TDX_PAGE_METADATA_INCORRECT 0xC000030000000000ULL
> > #define
> > TDX_VCPU_NOT_ASSOCIATED 0x8000070200000000ULL
> > #define TDX_KEY_GENERATION_FAILED 0x8000080000000000ULL
> > @@ -28,6 +31,20 @@
> > #define TDX_EPT_ENTRY_STATE_INCORRECT 0xC0000B0D00000000ULL
> > #define
> > TDX_METADATA_FIELD_NOT_READABLE 0xC0000C0200000000ULL
> >
> > +/*
> > + * SW-defined error codes.
> > + *
> > + * Bits 47:40 == 0xFF indicate Reserved status code class that never used
> > by
> > + * TDX module.
> > + */
> > +#define TDX_ERROR _BITULL(63)
> > +#define TDX_NON_RECOVERABLE _BITULL(62)
> > +#define TDX_SW_ERROR (TDX_ERROR | GENMASK_ULL(47, 40))
> > +#define TDX_SEAMCALL_VMFAILINVALID (TDX_SW_ERROR | _ULL(0xFFFF0000))
> > +
> > +#define TDX_SEAMCALL_GP (TDX_SW_ERROR |
> > X86_TRAP_GP)
> > +#define TDX_SEAMCALL_UD (TDX_SW_ERROR |
> > X86_TRAP_UD)
> > +
> > /*
> > * TDX module operand ID, appears in 31:0 part of error code as
> > * detail information
> > @@ -37,4 +54,4 @@
> > #define TDX_OPERAND_ID_SEPT 0x92
> > #define TDX_OPERAND_ID_TD_EPOCH 0xa9
> >
> > -#endif /* __KVM_X86_TDX_ERRNO_H */
> > +#endif /* _X86_SHARED_TDX_ERRNO_H */
> > diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> > index 6b338d7f01b7..2f3e16b93b4c 100644
> > --- a/arch/x86/include/asm/tdx.h
> > +++ b/arch/x86/include/asm/tdx.h
> > @@ -12,26 +12,6 @@
> > #include <asm/trapnr.h>
>
> I think you can remove <asm/trapnr.h> here since now <asm/tdx.h> is no longer
> using any definitions from it. And <asm/shared/tdx_errno.h> now includes it
> directly.
Yep, you're right.
^ permalink raw reply
* Re: [PATCH v4 14/16] KVM: TDX: Reclaim PAMT memory
From: Edgecombe, Rick P @ 2025-11-26 22:58 UTC (permalink / raw)
To: binbin.wu@linux.intel.com
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Li, Xiaoyao, Hansen, Dave, Zhao, Yan Y, Wu, Binbin,
kas@kernel.org, seanjc@google.com, mingo@redhat.com,
linux-kernel@vger.kernel.org, pbonzini@redhat.com,
Yamahata, Isaku, tglx@linutronix.de, Annapurve, Vishal, Gao, Chao,
bp@alien8.de, x86@kernel.org
In-Reply-To: <7fd9ebae-58f9-4908-87db-4317f30deae6@linux.intel.com>
On Wed, 2025-11-26 at 16:53 +0800, Binbin Wu wrote:
> > Add tdx_pamt_put() for memory that comes from guest_memfd and use
> > tdx_free_page() for the rest.
>
> External page table pages are not from guest_memfd, but tdx_pamt_put() is
> used in tdx_sept_free_private_spt() for them.
Right, this could be "...for pages that are passed from outside of TDX code...
^ permalink raw reply
* Re: [PATCH v4 13/16] KVM: TDX: Handle PAMT allocation in fault path
From: Edgecombe, Rick P @ 2025-11-26 22:33 UTC (permalink / raw)
To: binbin.wu@linux.intel.com
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Li, Xiaoyao, Hansen, Dave, Zhao, Yan Y, Wu, Binbin,
kas@kernel.org, seanjc@google.com, mingo@redhat.com,
linux-kernel@vger.kernel.org, pbonzini@redhat.com,
Yamahata, Isaku, tglx@linutronix.de, Annapurve, Vishal, Gao, Chao,
bp@alien8.de, x86@kernel.org
In-Reply-To: <8d5c0f57-bf91-4ea3-bd7e-5a02bcb5cc09@linux.intel.com>
On Wed, 2025-11-26 at 13:56 +0800, Binbin Wu wrote:
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -683,6 +683,8 @@ int tdx_vcpu_create(struct kvm_vcpu *vcpu)
> > if (!irqchip_split(vcpu->kvm))
> > return -EINVAL;
> >
> > + INIT_LIST_HEAD(&tdx->prealloc.page_list);
> > +
>
> Should this change be moved to patch 12?
> Because the pre-alloc page list has started to be used in patch 12 for
> external
> page tables even without enabling dynamic PAMT.
Oh, yes. thanks.
^ permalink raw reply
* Re: [PATCH v4 12/16] x86/virt/tdx: Add helpers to allow for pre-allocating pages
From: Edgecombe, Rick P @ 2025-11-26 22:33 UTC (permalink / raw)
To: binbin.wu@linux.intel.com
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Li, Xiaoyao, Hansen, Dave, Zhao, Yan Y, Wu, Binbin,
kas@kernel.org, seanjc@google.com, mingo@redhat.com,
linux-kernel@vger.kernel.org, pbonzini@redhat.com,
Yamahata, Isaku, tglx@linutronix.de, Annapurve, Vishal, Gao, Chao,
bp@alien8.de, x86@kernel.org
In-Reply-To: <7a6f5b4e-ad7b-4ad0-95fd-e1698f9b4e06@linux.intel.com>
On Wed, 2025-11-26 at 11:40 +0800, Binbin Wu wrote:
>
> On 11/21/2025 8:51 AM, Rick Edgecombe wrote:
> > In the KVM fault path page, tables and private pages need to be
> "In the KVM fault path page, tables ..." should be
> "In the KVM fault path, page tables ..."
Right, thanks.
>
> > installed under a spin lock. This means that the operations around
> > installing PAMT pages for them will not be able to allocate pages.
> >
> [...]
> > @@ -141,7 +142,46 @@ int tdx_guest_keyid_alloc(void);
> > u32 tdx_get_nr_guest_keyids(void);
> > void tdx_guest_keyid_free(unsigned int keyid);
> >
> > -int tdx_pamt_get(struct page *page);
> > +int tdx_dpamt_entry_pages(void);
> > +
> > +/*
> > + * Simple structure for pre-allocating Dynamic
> > + * PAMT pages outside of locks.
>
> It's not just for Dynamic PAMT pages, but also external page table pages.
I'll adjust.
>
> > + */
> > +struct tdx_prealloc {
> > + struct list_head page_list;
> > + int cnt;
> > +};
> > +
> > +static inline struct page *get_tdx_prealloc_page(struct tdx_prealloc *prealloc)
> > +{
> > + struct page *page;
> > +
> > + page = list_first_entry_or_null(&prealloc->page_list, struct page, lru);
> > + if (page) {
> > + list_del(&page->lru);
> > + prealloc->cnt--;
> > + }
> > +
> > + return page;
> > +}
> > +
> > +static inline int topup_tdx_prealloc_page(struct tdx_prealloc *prealloc, unsigned int min_size)
> > +{
> > + while (prealloc->cnt < min_size) {
> > + struct page *page = alloc_page(GFP_KERNEL_ACCOUNT);
> > +
> > + if (!page)
> > + return -ENOMEM;
> > +
> > + list_add(&page->lru, &prealloc->page_list);
> > + prealloc->cnt++;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +int tdx_pamt_get(struct page *page, struct tdx_prealloc *prealloc);
> > void tdx_pamt_put(struct page *page);
> >
> > struct page *tdx_alloc_page(void);
> > @@ -219,6 +259,7 @@ static inline int tdx_enable(void) { return -ENODEV; }
> > static inline u32 tdx_get_nr_guest_keyids(void) { return 0; }
> > static inline const char *tdx_dump_mce_info(struct mce *m) { return NULL; }
> > static inline const struct tdx_sys_info *tdx_get_sysinfo(void) { return NULL; }
> > +static inline int tdx_dpamt_entry_pages(void) { return 0; }
> > #endif /* CONFIG_INTEL_TDX_HOST */
> >
> > #ifdef CONFIG_KEXEC_CORE
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index 260bb0e6eb44..61a058a8f159 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -1644,23 +1644,34 @@ static int tdx_mem_page_add(struct kvm *kvm, gfn_t gfn, enum pg_level level,
> >
> > static void *tdx_alloc_external_fault_cache(struct kvm_vcpu *vcpu)
> > {
> > - struct vcpu_tdx *tdx = to_tdx(vcpu);
> > + struct page *page = get_tdx_prealloc_page(&to_tdx(vcpu)->prealloc);
> >
> > - return kvm_mmu_memory_cache_alloc(&tdx->mmu_external_spt_cache);
> > + if (WARN_ON_ONCE(!page))
> > + return (void *)__get_free_page(GFP_ATOMIC | __GFP_ACCOUNT);
>
> kvm_mmu_memory_cache_alloc() calls BUG_ON() if the atomic allocation failed.
> Do we want to follow?
Ignoring, per your other comment. But we don't want to add BUG_ON()s in general.
>
> > +
> > + return page_address(page);
> > }
> >
> > static int tdx_topup_external_fault_cache(struct kvm_vcpu *vcpu, unsigned int cnt)
> > {
> > - struct vcpu_tdx *tdx = to_tdx(vcpu);
> > + struct tdx_prealloc *prealloc = &to_tdx(vcpu)->prealloc;
> > + int min_fault_cache_size;
> >
> > - return kvm_mmu_topup_memory_cache(&tdx->mmu_external_spt_cache, cnt);
> > + /* External page tables */
> > + min_fault_cache_size = cnt;
> > + /* Dynamic PAMT pages (if enabled) */
> > + min_fault_cache_size += tdx_dpamt_entry_pages() * PT64_ROOT_MAX_LEVEL;
>
> Is the value PT64_ROOT_MAX_LEVEL intended, since dynamic PAMT pages are only
> needed for 4KB level?
I'm not sure I follow. We need DPAMT backing for each S-EPT page table.
^ permalink raw reply
* Re: [RFC PATCH 2/4] mm: Add support for unaccepted memory hotplug
From: Borislav Petkov @ 2025-11-26 22:31 UTC (permalink / raw)
To: Kiryl Shutsemau
Cc: Pratik R. Sampat, linux-mm, linux-coco, linux-efi, x86,
linux-kernel, tglx, mingo, dave.hansen, ardb, akpm, david,
osalvador, thomas.lendacky, michael.roth
In-Reply-To: <66ylzwknm4ftd6utn3nqr63jmhl2ccvcdvyi5fechfnvmfxivu@37pckhjixayh>
On Wed, Nov 26, 2025 at 11:12:13AM +0000, Kiryl Shutsemau wrote:
> > diff --git a/arch/x86/boot/compressed/efi.h b/arch/x86/boot/compressed/efi.h
> > index 4f7027f33def..a220a1966cae 100644
> > --- a/arch/x86/boot/compressed/efi.h
> > +++ b/arch/x86/boot/compressed/efi.h
> > @@ -102,6 +102,7 @@ struct efi_unaccepted_memory {
> > u32 unit_size;
> > u64 phys_base;
> > u64 size;
> > + bool mem_reserved;
> > unsigned long *bitmap;
> > };
> >
>
> Again, this is ABI break for kexec.
ABI break for kexec? Is that a thing?
Since when do we enforce ABI compatibility for kexec and where are we
documenting that?
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH v4 07/16] x86/virt/tdx: Add tdx_alloc/free_page() helpers
From: Edgecombe, Rick P @ 2025-11-26 22:28 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Li, Xiaoyao, Hansen, Dave, Zhao, Yan Y, Wu, Binbin,
kas@kernel.org, seanjc@google.com, mingo@redhat.com,
pbonzini@redhat.com, tglx@linutronix.de, Yamahata, Isaku,
linux-kernel@vger.kernel.org, Annapurve, Vishal, Gao, Chao,
bp@alien8.de, x86@kernel.org
Cc: kirill.shutemov@linux.intel.com
In-Reply-To: <c345d734e111eb0c3a98dbc517ed25071852106b.camel@intel.com>
On Wed, 2025-11-26 at 01:21 +0000, Huang, Kai wrote:
> > }
> >
> > +void tdx_quirk_reset_page(struct page *page);
> > +
> > int tdx_guest_keyid_alloc(void);
> > u32 tdx_get_nr_guest_keyids(void);
> > void tdx_guest_keyid_free(unsigned int keyid);
> >
> > -void tdx_quirk_reset_page(struct page *page);
>
> I don't think it's mandatory to move the declaration of
> tdx_quirk_reset_page()?
Sure, I'll fix it.
^ permalink raw reply
* Re: [PATCH v4 07/16] x86/virt/tdx: Add tdx_alloc/free_page() helpers
From: Edgecombe, Rick P @ 2025-11-26 22:28 UTC (permalink / raw)
To: binbin.wu@linux.intel.com
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Li, Xiaoyao, Hansen, Dave, Zhao, Yan Y, Wu, Binbin,
kas@kernel.org, seanjc@google.com, mingo@redhat.com,
linux-kernel@vger.kernel.org, pbonzini@redhat.com,
Yamahata, Isaku, tglx@linutronix.de, Annapurve, Vishal, Gao, Chao,
bp@alien8.de, x86@kernel.org
In-Reply-To: <12144256-b71a-4331-8309-2e805dc120d1@linux.intel.com>
On Tue, 2025-11-25 at 16:09 +0800, Binbin Wu wrote:
>
> On 11/21/2025 8:51 AM, Rick Edgecombe wrote:
> [...]
> >
> > +/* Number PAMT pages to be provided to TDX module per 2M region of PA */
> ^ ^
> of 2MB
Sounds good.
> > +static int tdx_dpamt_entry_pages(void)
> > +{
> > + if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> > + return 0;
> > +
> > + return tdx_sysinfo.tdmr.pamt_4k_entry_size * PTRS_PER_PTE / PAGE_SIZE;
> > +}
> > +
> > +/*
> > + * The TDX spec treats the registers like an array, as they are ordered
> > + * in the struct. The array size is limited by the number or registers,
> > + * so define the max size it could be for worst case allocations and sanity
> > + * checking.
> > + */
> > +#define MAX_TDX_ARG_SIZE(reg) (sizeof(struct tdx_module_args) - \
> > + offsetof(struct tdx_module_args, reg))
>
> This should be the maximum number of registers could be used to pass the
> addresses of the pages (or other information), it needs to be divided by sizeof(u64).
Oops, right.
>
> Also, "SIZE" in the name could be confusing.
Yes LENGTH is probably better.
>
> > +#define TDX_ARG_INDEX(reg) (offsetof(struct tdx_module_args, reg) / \
> > + sizeof(u64))
> > +
> > +/*
> > + * Treat struct the registers like an array that starts at RDX, per
> > + * TDX spec. Do some sanitychecks, and return an indexable type.
> sanitychecks -> sanity checks
>
> > + */
> [...]
> > +/* Serializes adding/removing PAMT memory */
> > +static DEFINE_SPINLOCK(pamt_lock);
> > +
> > +/* Bump PAMT refcount for the given page and allocate PAMT memory if needed */
> > +int tdx_pamt_get(struct page *page)
> > +{
> > + u64 pamt_pa_array[MAX_TDX_ARG_SIZE(rdx)];
> > + atomic_t *pamt_refcount;
> > + u64 tdx_status;
> > + int ret;
> > +
> > + if (!tdx_supports_dynamic_pamt(&tdx_sysinfo))
> > + return 0;
> > +
> > + ret = alloc_pamt_array(pamt_pa_array);
> > + if (ret)
> > + goto out_free;
> > +
> > + pamt_refcount = tdx_find_pamt_refcount(page_to_pfn(page));
> > +
> > + scoped_guard(spinlock, &pamt_lock) {
> > + /*
> > + * If the pamt page is already added (i.e. refcount >= 1),
> > + * then just increment the refcount.
> > + */
> > + if (atomic_read(pamt_refcount)) {
> > + atomic_inc(pamt_refcount);
>
> So far, all atomic operations are inside the spinlock.
> May be better to add some info in the change log that atomic is needed due to
> the optimization in the later patch?
Yea, I really debated this. Changing to atomics is more churn, but doing part of
the optimizations early is weird too. I think I might go with the more churn
approach.
>
> > + goto out_free;
> > + }
> > +
> > + /* Try to add the pamt page and take the refcount 0->1. */
> > +
> > + tdx_status = tdh_phymem_pamt_add(page, pamt_pa_array);
> > + if (!IS_TDX_SUCCESS(tdx_status)) {
> > + pr_err("TDH_PHYMEM_PAMT_ADD failed: %#llx\n", tdx_status);
>
> Can use pr_tdx_error().
Not in arch/x86, plus it became TDX_BUG_ON() in the cleanup series.
>
> Aslo, so for in this patch, when this SEAMCALL failed, does it indicate a bug?
Yes. This should set ret to an error value too. Yes SEAMCALL failure indicates a
bug, but it should be harmless to the host. tdx_pamt_get() returns a failure so
the calling code can handle it. The tdx_pamt_put() is a little weirder. We can
have the option to keep the refcount elevated if remove fails. This means DPAMT
stays addded for this 2MB range. I think it is ok for 4KB guest memory. It
basically means a DPAMT page will stay in place forever in the case of a bug,
which is the current non-dynamic PAMT situation. So I'm not sure if a full WARN
is needed.
But I'm just realizing that the page it is backing could reform it's way into a
2MB page. Then, after the TDX huge pages series, it could end up getting mapped
as 2MB private memory. In which case the PAMT.REMOVE needs to succeed before
adding the page as 2MB. Hmm, need to check TDX huge page series, because I think
this is not handled. So when TDX huge pages comes, a WARN might be more
appropriate.
We might need to make tdx_pamt_put() return an error for huge pages, and treat
failure like the other TDX remove failures, or maybe just a WARN. I'd lean
towards just the WARN. Do you have any better ideas?
>
> > + goto out_free;
> > + }
> > +
> > + atomic_inc(pamt_refcount);
> > + }
> > +
> > + return ret;
>
> Maybe just return 0 here since all error paths must be directed to out_free.
Yes.
>
> > +out_free:
> > + /*
> > + * pamt_pa_array is populated or zeroed up to tdx_dpamt_entry_pages()
> > + * above. free_pamt_array() can handle either case.
> > + */
> > + free_pamt_array(pamt_pa_array);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(tdx_pamt_get);
> > +
> >
> [...]
^ permalink raw reply
* Re: [RFC PATCH 2/4] mm: Add support for unaccepted memory hotplug
From: Pratik R. Sampat @ 2025-11-26 22:27 UTC (permalink / raw)
To: Kiryl Shutsemau
Cc: linux-mm, linux-coco, linux-efi, x86, linux-kernel, tglx, mingo,
bp, dave.hansen, ardb, akpm, david, osalvador, thomas.lendacky,
michael.roth
In-Reply-To: <66ylzwknm4ftd6utn3nqr63jmhl2ccvcdvyi5fechfnvmfxivu@37pckhjixayh>
On 11/26/25 5:12 AM, Kiryl Shutsemau wrote:
> On Tue, Nov 25, 2025 at 11:57:51AM -0600, Pratik R. Sampat wrote:
>> The unaccepted memory structure currently only supports accepting memory
>> present at boot time. The unaccepted table uses a fixed-size bitmap
>> reserved in memblock based on the initial memory layout, preventing
>> dynamic addition of memory ranges after boot. This causes guest
>> termination when memory is hot-added in a secure virtual machine due to
>> accessing pages that have not transitioned to private before use.
>
> How does the hot-pluggable memory look in EFI memory map? I thought
> hot-pluggable ranges suppose to be declared thare. The cleanest solution
> would be to have hot-pluggable and unaccepted indicated in EFI memory,
> so we can size bitmap accordingly upfront.
>
I'm not quite sure if I fully understand. Do you mean to refer to the
EFI_MEMORY_HOT_PLUGGABLE attribute that is used for cold plugged boot
memory? If so, wouldn't it still be desirable to increase the size of
the bitmap to what was marked as hotpluggable initially?
>> Extend the unaccepted memory framework to handle hotplugged memory by
>> dynamically managing the unaccepted bitmap. Allocate a new bitmap when
>> hotplugged ranges exceed the reserved bitmap capacity and switch to
>> kernel-managed allocation.
>>
>> Hotplugged memory also follows the same acceptance policy using the
>> accept_memory=[eager|lazy] kernel parameter to accept memory either
>> up-front when added or before first use.
>>
>> Signed-off-by: Pratik R. Sampat <prsampat@amd.com>
>> ---
>> arch/x86/boot/compressed/efi.h | 1 +
>> .../firmware/efi/libstub/unaccepted_memory.c | 1 +
>> drivers/firmware/efi/unaccepted_memory.c | 83 +++++++++++++++++++
>> include/linux/efi.h | 1 +
>> include/linux/mm.h | 11 +++
>> mm/memory_hotplug.c | 7 ++
>> mm/page_alloc.c | 2 +
>> 7 files changed, 106 insertions(+)
>>
>> diff --git a/arch/x86/boot/compressed/efi.h b/arch/x86/boot/compressed/efi.h
>> index 4f7027f33def..a220a1966cae 100644
>> --- a/arch/x86/boot/compressed/efi.h
>> +++ b/arch/x86/boot/compressed/efi.h
>> @@ -102,6 +102,7 @@ struct efi_unaccepted_memory {
>> u32 unit_size;
>> u64 phys_base;
>> u64 size;
>> + bool mem_reserved;
>> unsigned long *bitmap;
>> };
>>
>
> Again, this is ABI break for kexec.
>
Right, maybe I could just use memblock_is_reserved() instead to prevent
this ABI breakage.
Thanks!
--
Pratik
^ permalink raw reply
* Re: [RFC PATCH 1/4] efi/libstub: Decouple memory bitmap from the unaccepted table
From: Pratik R. Sampat @ 2025-11-26 22:27 UTC (permalink / raw)
To: Kiryl Shutsemau
Cc: linux-mm, linux-coco, linux-efi, x86, linux-kernel, tglx, mingo,
bp, dave.hansen, ardb, akpm, david, osalvador, thomas.lendacky,
michael.roth
In-Reply-To: <phywn74ghpsqncoqsgdqjoxeqdxskibzt3yltn6bmtc25sm46g@emad5n6wp5l2>
Hi Kiryl,
Thanks for you comments.
On 11/26/25 5:08 AM, Kiryl Shutsemau wrote:
> On Tue, Nov 25, 2025 at 11:57:50AM -0600, Pratik R. Sampat wrote:
>> Memory hotplug in secure environments requires the unaccepted memory
>> bitmap to grow as new memory is added. Currently, the bitmap is
>> implemented as a flexible array member at the end of struct
>> efi_unaccepted_memory, which is reserved by memblock at boot and cannot
>> be resized without reallocating the entire structure.
>>
>> Replace the flexible array member with a pointer.
>
> Well, it break interoperability between kernel before and after the
> patch. Consider kexec from kernel without the patch to the kernel with
> the patch and then back to older kernel. It is ABI break.
>
> Is re-allocating the entire structure such a big pain?
>
We could do that. My concern is that we would then need to protect the
entire table instead of just the bitmap, which may add an additional
overhead?
--
Pratik
^ permalink raw reply
* Re: [PATCH v4 06/16] x86/virt/tdx: Improve PAMT refcounts allocation for sparse memory
From: Edgecombe, Rick P @ 2025-11-26 20:47 UTC (permalink / raw)
To: binbin.wu@linux.intel.com
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Li, Xiaoyao, Hansen, Dave, Zhao, Yan Y, Wu, Binbin,
kas@kernel.org, seanjc@google.com, mingo@redhat.com,
linux-kernel@vger.kernel.org, pbonzini@redhat.com,
Yamahata, Isaku, tglx@linutronix.de, Annapurve, Vishal, Gao, Chao,
bp@alien8.de, x86@kernel.org
In-Reply-To: <468165b7-46aa-4321-a47f-a97befaa993f@linux.intel.com>
On Tue, 2025-11-25 at 11:15 +0800, Binbin Wu wrote:
> On 11/21/2025 8:51 AM, Rick Edgecombe wrote:
> [...]
> > +
> > +/* Unmap a page from the PAMT refcount vmalloc region */
> > +static int pamt_refcount_depopulate(pte_t *pte, unsigned long addr, void *data)
> > +{
> > + struct page *page;
> > + pte_t entry;
> > +
> > + spin_lock(&init_mm.page_table_lock);
> > +
> > + entry = ptep_get(pte);
> > + /* refcount allocation is sparse, may not be populated */
>
> Not sure this comment about "sparse" is accurate since this function is called via
> apply_to_existing_page_range().
>
> And the check for not present just for sanity check?
Yes, I don't see what that comment is referring to. But we do need it, because
hypothetically the refcount mapping could have failed halfway. So we will have
pte_none()s for the ranges that didn't get populated. I'll use:
/* Refcount mapping could have failed part way, handle aborted mappings. */
^ permalink raw reply
* Re: [PATCH v4 06/16] x86/virt/tdx: Improve PAMT refcounts allocation for sparse memory
From: Edgecombe, Rick P @ 2025-11-26 20:47 UTC (permalink / raw)
To: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Li, Xiaoyao, Hansen, Dave, Zhao, Yan Y, Wu, Binbin,
kas@kernel.org, seanjc@google.com, mingo@redhat.com,
linux-kernel@vger.kernel.org, tglx@linutronix.de, Yamahata, Isaku,
nik.borisov@suse.com, pbonzini@redhat.com, Annapurve, Vishal,
Gao, Chao, bp@alien8.de, x86@kernel.org
Cc: kirill.shutemov@linux.intel.com
In-Reply-To: <b3b465e6-cfa0-44d0-bdef-6d37bb26e6e0@suse.com>
Kiryl, curious if you have any comments on the below...
On Wed, 2025-11-26 at 16:45 +0200, Nikolay Borisov wrote:
> > +static int pamt_refcount_populate(pte_t *pte, unsigned long addr, void
> > *data)
> > +{
> > + struct page *page;
> > + pte_t entry;
> > +
> > + page = alloc_page(GFP_KERNEL | __GFP_ZERO);
> > + if (!page)
> > return -ENOMEM;
> >
> > + entry = mk_pte(page, PAGE_KERNEL);
> > +
> > + spin_lock(&init_mm.page_table_lock);
> > + /*
> > + * PAMT refcount populations can overlap due to rounding of the
> > + * start/end pfn. Make sure the PAMT range is only populated once.
> > + */
> > + if (pte_none(ptep_get(pte)))
> > + set_pte_at(&init_mm, addr, pte, entry);
> > + else
> > + __free_page(page);
> > + spin_unlock(&init_mm.page_table_lock);
>
> nit: Wouldn't it be better to perform the pte_none() check before doing
> the allocation thus avoiding needless allocations? I.e do the
> alloc/mk_pte only after we are 100% sure we are going to use this entry.
Yes, but I'm also wondering why it needs init_mm.page_table_lock at all. Here is
my reasoning for why it doesn't:
apply_to_page_range() takes init_mm.page_table_lock internally when it modified
page tables in the address range (vmalloc). It needs to do this to avoid races
with other allocations that share the upper level page tables, which could be on
the ends of area that TDX reserves.
But pamt_refcount_populate() is only operating on the PTE's for the address
range that TDX code already controls. Vmalloc should not free the PMD underneath
the PTE operation because there is an allocation in any page tables it covers.
So we can skip the lock and also do the pte_none() check before the page
allocation as Nikolay suggests.
Same for the depopulate path.
^ 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