* Re: [PATCH v4 07/16] x86/virt/tdx: Add tdx_alloc/free_page() helpers
From: Edgecombe, Rick P @ 2025-12-03 21:39 UTC (permalink / raw)
To: Hansen, Dave, nik.borisov@suse.com, kas@kernel.org
Cc: kvm@vger.kernel.org, Li, Xiaoyao, Huang, Kai,
linux-coco@lists.linux.dev, Zhao, Yan Y, Wu, Binbin,
linux-kernel@vger.kernel.org, seanjc@google.com, mingo@redhat.com,
pbonzini@redhat.com, tglx@linutronix.de, Yamahata, Isaku,
Annapurve, Vishal, Gao, Chao, bp@alien8.de, x86@kernel.org
In-Reply-To: <408079db-c488-492e-b6e7-063dea3cb861@intel.com>
On Wed, 2025-12-03 at 12:13 -0800, Dave Hansen wrote:
> BTW, I *DO* expect the DPAMT pages to be mostly allocated forever. Maybe
> I'm just a pessimist, but you can't get them back for compaction or
> reclaim, so they're basically untouchable. Sure, if you kill all the TDX
> guests you get them back, but that's a very different kind of kernel
> memory from stuff that's truly reclaimable under pressure.
If we want to improve it (later) we could topup via a single higher order
allocation. So like if we need 15 pages we could round up to an order-4, then
hand them out individually. But I suspect someone would want to see some proof
before that kind of tweak.
So I take it you are ok to leave the general approach, but worth trying to
remove the dynamic sizing.
^ permalink raw reply
* Re: [PATCH 3/3] KVM: guest_memfd: GUP source pages prior to populating guest memory
From: FirstName LastName @ 2025-12-03 20:59 UTC (permalink / raw)
To: michael.roth
Cc: ackerleytng, aik, ashish.kalra, david, ira.weiny, kvm,
liam.merwick, linux-coco, linux-kernel, linux-mm, pbonzini,
seanjc, thomas.lendacky, vannapurve, vbabka, yan.y.zhao
In-Reply-To: <20251203142648.trx6sslxvxr26yzd@amd.com>
> >
> > > but it makes a lot more sense to make those restrictions and changes in
> > > the context of hugepage support, rather than this series which is trying
> > > very hard to not do hugepage enablement, but simply keep what's partially
> > > there intact while reworking other things that have proven to be
> > > continued impediments to both in-place conversion and hugepage
> > > enablement.
> > Not sure how fixing the warning in this series could impede hugepage enabling :)
> >
> > But if you prefer, I don't mind keeping it locally for longer.
>
> It's the whole burden of needing to anticipate hugepage design, while it
> is in a state of potentially massive flux just before LPC, in order to
> make tiny incremental progress toward enabling in-place conversion,
> which is something I think we can get upstream much sooner. Look at your
> changelog for the change above, for instance: it has no relevance in the
> context of this series. What do I put in its place? Bug reports about
> my experimental tree? It's just not the right place to try to justify
> these changes.
>
> And most of this weirdness stems from the fact that we prematurely added
> partial hugepage enablement to begin with. Let's not repeat these mistakes,
> and address changes in the proper context where we know they make sense.
>
> I considered stripping out the existing hugepage support as a pre-patch
> to avoid leaving these uncertainties in place while we are reworking
> things, but it felt like needless churn. But that's where I'm coming
I think simplifying this implementation to handle populate at 4K pages is worth
considering at this stage and we could optimize for huge page granularity
population in future based on the need.
e.g. 4K page based population logic will keep things simple and can be
further simplified if we can add PAGE_ALIGNED(params.uaddr) restriction.
Extending Sean's suggestion earlier, compile tested only.
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index f59c65abe3cf..224e79ab8f86 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2267,66 +2267,56 @@ struct sev_gmem_populate_args {
int fw_error;
};
-static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn_start, kvm_pfn_t pfn,
- void __user *src, int order, void *opaque)
+static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
+ struct page *src_page, void *opaque)
{
struct sev_gmem_populate_args *sev_populate_args = opaque;
struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
- int n_private = 0, ret, i;
- int npages = (1 << order);
- gfn_t gfn;
+ int ret;
- if (WARN_ON_ONCE(sev_populate_args->type != KVM_SEV_SNP_PAGE_TYPE_ZERO && !src))
+ if (WARN_ON_ONCE(sev_populate_args->type != KVM_SEV_SNP_PAGE_TYPE_ZERO && !src_page))
return -EINVAL;
- for (gfn = gfn_start, i = 0; gfn < gfn_start + npages; gfn++, i++) {
- struct sev_data_snp_launch_update fw_args = {0};
- bool assigned = false;
- int level;
-
- ret = snp_lookup_rmpentry((u64)pfn + i, &assigned, &level);
- if (ret || assigned) {
- pr_debug("%s: Failed to ensure GFN 0x%llx RMP entry is initial shared state, ret: %d assigned: %d\n",
- __func__, gfn, ret, assigned);
- ret = ret ? -EINVAL : -EEXIST;
- goto err;
- }
+ struct sev_data_snp_launch_update fw_args = {0};
+ bool assigned = false;
+ int level;
- if (src) {
- void *vaddr = kmap_local_pfn(pfn + i);
+ ret = snp_lookup_rmpentry((u64)pfn, &assigned, &level);
+ if (ret || assigned) {
+ pr_debug("%s: Failed to ensure GFN 0x%llx RMP entry is initial shared state, ret: %d assigned: %d\n",
+ __func__, gfn, ret, assigned);
+ ret = ret ? -EINVAL : -EEXIST;
+ goto err;
+ }
- if (copy_from_user(vaddr, src + i * PAGE_SIZE, PAGE_SIZE)) {
- ret = -EFAULT;
- goto err;
- }
- kunmap_local(vaddr);
- }
+ if (src_page) {
+ void *vaddr = kmap_local_pfn(pfn);
- ret = rmp_make_private(pfn + i, gfn << PAGE_SHIFT, PG_LEVEL_4K,
- sev_get_asid(kvm), true);
- if (ret)
- goto err;
+ memcpy(vaddr, page_address(src_page), PAGE_SIZE);
+ kunmap_local(vaddr);
+ }
- n_private++;
+ ret = rmp_make_private(pfn, gfn << PAGE_SHIFT, PG_LEVEL_4K,
+ sev_get_asid(kvm), true);
+ if (ret)
+ goto err;
- fw_args.gctx_paddr = __psp_pa(sev->snp_context);
- fw_args.address = __sme_set(pfn_to_hpa(pfn + i));
- fw_args.page_size = PG_LEVEL_TO_RMP(PG_LEVEL_4K);
- fw_args.page_type = sev_populate_args->type;
+ fw_args.gctx_paddr = __psp_pa(sev->snp_context);
+ fw_args.address = __sme_set(pfn_to_hpa(pfn));
+ fw_args.page_size = PG_LEVEL_TO_RMP(PG_LEVEL_4K);
+ fw_args.page_type = sev_populate_args->type;
- ret = __sev_issue_cmd(sev_populate_args->sev_fd, SEV_CMD_SNP_LAUNCH_UPDATE,
- &fw_args, &sev_populate_args->fw_error);
- if (ret)
- goto fw_err;
- }
+ ret = __sev_issue_cmd(sev_populate_args->sev_fd, SEV_CMD_SNP_LAUNCH_UPDATE,
+ &fw_args, &sev_populate_args->fw_error);
+ if (ret)
+ goto fw_err;
return 0;
fw_err:
/*
* If the firmware command failed handle the reclaim and cleanup of that
- * PFN specially vs. prior pages which can be cleaned up below without
- * needing to reclaim in advance.
+ * PFN specially.
*
* Additionally, when invalid CPUID function entries are detected,
* firmware writes the expected values into the page and leaves it
@@ -2336,25 +2326,18 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn_start, kvm_pfn_t pf
* information to provide information on which CPUID leaves/fields
* failed CPUID validation.
*/
- if (!snp_page_reclaim(kvm, pfn + i) &&
+ if (!snp_page_reclaim(kvm, pfn) &&
sev_populate_args->type == KVM_SEV_SNP_PAGE_TYPE_CPUID &&
sev_populate_args->fw_error == SEV_RET_INVALID_PARAM) {
- void *vaddr = kmap_local_pfn(pfn + i);
-
- if (copy_to_user(src + i * PAGE_SIZE, vaddr, PAGE_SIZE))
- pr_debug("Failed to write CPUID page back to userspace\n");
+ void *vaddr = kmap_local_pfn(pfn);
+ memcpy(page_address(src_page), vaddr, PAGE_SIZE);
kunmap_local(vaddr);
}
- /* pfn + i is hypervisor-owned now, so skip below cleanup for it. */
- n_private--;
-
err:
- pr_debug("%s: exiting with error ret %d (fw_error %d), restoring %d gmem PFNs to shared.\n",
- __func__, ret, sev_populate_args->fw_error, n_private);
- for (i = 0; i < n_private; i++)
- kvm_rmp_make_shared(kvm, pfn + i, PG_LEVEL_4K);
+ pr_debug("%s: exiting with error ret %d (fw_error %d)\n",
+ __func__, ret, sev_populate_args->fw_error);
return ret;
}
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 2d7a4d52ccfb..acdcb802d9f2 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -3118,34 +3118,21 @@ struct tdx_gmem_post_populate_arg {
};
static int tdx_gmem_post_populate(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
- void __user *src, int order, void *_arg)
+ struct page *src_page, void *_arg)
{
struct tdx_gmem_post_populate_arg *arg = _arg;
struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
u64 err, entry, level_state;
gpa_t gpa = gfn_to_gpa(gfn);
- struct page *src_page;
- int ret, i;
+ int ret;
if (KVM_BUG_ON(kvm_tdx->page_add_src, kvm))
return -EIO;
- /*
- * Get the source page if it has been faulted in. Return failure if the
- * source page has been swapped out or unmapped in primary memory.
- */
- ret = get_user_pages_fast((unsigned long)src, 1, 0, &src_page);
- if (ret < 0)
- return ret;
- if (ret != 1)
- return -ENOMEM;
-
kvm_tdx->page_add_src = src_page;
ret = kvm_tdp_mmu_map_private_pfn(arg->vcpu, gfn, pfn);
kvm_tdx->page_add_src = NULL;
- put_page(src_page);
-
if (ret || !(arg->flags & KVM_TDX_MEASURE_MEMORY_REGION))
return ret;
@@ -3156,11 +3143,9 @@ static int tdx_gmem_post_populate(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
* mmu_notifier events can't reach S-EPT entries, and KVM's internal
* zapping flows are mutually exclusive with S-EPT mappings.
*/
- for (i = 0; i < PAGE_SIZE; i += TDX_EXTENDMR_CHUNKSIZE) {
- err = tdh_mr_extend(&kvm_tdx->td, gpa + i, &entry, &level_state);
- if (TDX_BUG_ON_2(err, TDH_MR_EXTEND, entry, level_state, kvm))
- return -EIO;
- }
+ err = tdh_mr_extend(&kvm_tdx->td, gpa, &entry, &level_state);
+ if (TDX_BUG_ON_2(err, TDH_MR_EXTEND, entry, level_state, kvm))
+ return -EIO;
return 0;
}
@@ -3196,38 +3181,26 @@ static int tdx_vcpu_init_mem_region(struct kvm_vcpu *vcpu, struct kvm_tdx_cmd *c
return -EINVAL;
ret = 0;
- while (region.nr_pages) {
- if (signal_pending(current)) {
- ret = -EINTR;
- break;
- }
-
- arg = (struct tdx_gmem_post_populate_arg) {
- .vcpu = vcpu,
- .flags = cmd->flags,
- };
- gmem_ret = kvm_gmem_populate(kvm, gpa_to_gfn(region.gpa),
- u64_to_user_ptr(region.source_addr),
- 1, tdx_gmem_post_populate, &arg);
- if (gmem_ret < 0) {
- ret = gmem_ret;
- break;
- }
+ arg = (struct tdx_gmem_post_populate_arg) {
+ .vcpu = vcpu,
+ .flags = cmd->flags,
+ };
+ gmem_ret = kvm_gmem_populate(kvm, gpa_to_gfn(region.gpa),
+ u64_to_user_ptr(region.source_addr),
+ region.nr_pages, tdx_gmem_post_populate, &arg);
+ if (gmem_ret < 0)
+ ret = gmem_ret;
- if (gmem_ret != 1) {
- ret = -EIO;
- break;
- }
+ if (gmem_ret != region.nr_pages)
+ ret = -EIO;
- region.source_addr += PAGE_SIZE;
- region.gpa += PAGE_SIZE;
- region.nr_pages--;
+ if (gmem_ret >= 0) {
+ region.source_addr += gmem_ret * PAGE_SIZE;
+ region.gpa += gmem_ret * PAGE_SIZE;
- cond_resched();
+ if (copy_to_user(u64_to_user_ptr(cmd->data), ®ion, sizeof(region)))
+ ret = -EFAULT;
}
-
- if (copy_to_user(u64_to_user_ptr(cmd->data), ®ion, sizeof(region)))
- ret = -EFAULT;
return ret;
}
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index d93f75b05ae2..263e75f90e91 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -2581,7 +2581,7 @@ int kvm_arch_gmem_prepare(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn, int max_ord
* Returns the number of pages that were populated.
*/
typedef int (*kvm_gmem_populate_cb)(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
- void __user *src, int order, void *opaque);
+ struct page *src_page, void *opaque);
long kvm_gmem_populate(struct kvm *kvm, gfn_t gfn, void __user *src, long npages,
kvm_gmem_populate_cb post_populate, void *opaque);
diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
index 2e62bf882aa8..550dc818748b 100644
--- a/virt/kvm/guest_memfd.c
+++ b/virt/kvm/guest_memfd.c
@@ -85,7 +85,6 @@ static int __kvm_gmem_prepare_folio(struct kvm *kvm, struct kvm_memory_slot *slo
static int kvm_gmem_prepare_folio(struct kvm *kvm, struct kvm_memory_slot *slot,
gfn_t gfn, struct folio *folio)
{
- unsigned long nr_pages, i;
pgoff_t index;
/*
@@ -794,7 +793,7 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
return PTR_ERR(folio);
if (!folio_test_uptodate(folio)) {
- clear_huge_page(&folio->page, 0, folio_nr_pages(folio));
+ clear_highpage(folio_page(folio, 0));
folio_mark_uptodate(folio);
}
@@ -812,13 +811,54 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_gmem_get_pfn);
#ifdef CONFIG_HAVE_KVM_ARCH_GMEM_POPULATE
+static long __kvm_gmem_populate(struct kvm *kvm, struct kvm_memory_slot *slot,
+ struct file *file, gfn_t gfn, struct page *src_page,
+ kvm_gmem_populate_cb post_populate, void *opaque)
+{
+ pgoff_t index = kvm_gmem_get_index(slot, gfn);
+ struct gmem_inode *gi;
+ struct folio *folio;
+ int ret, max_order;
+ kvm_pfn_t pfn;
+
+ gi = GMEM_I(file_inode(file));
+
+ filemap_invalidate_lock(file->f_mapping);
+
+ folio = __kvm_gmem_get_pfn(file, slot, index, &pfn, &max_order);
+ if (IS_ERR(folio)) {
+ ret = PTR_ERR(folio);
+ goto out_unlock;
+ }
+
+ folio_unlock(folio);
+
+ if (!kvm_range_has_memory_attributes(kvm, gfn, gfn + 1,
+ KVM_MEMORY_ATTRIBUTE_PRIVATE,
+ KVM_MEMORY_ATTRIBUTE_PRIVATE)) {
+ ret = -EINVAL;
+ goto out_put_folio;
+ }
+
+ ret = post_populate(kvm, gfn, pfn, src_page, opaque);
+ if (!ret)
+ folio_mark_uptodate(folio);
+
+out_put_folio:
+ folio_put(folio);
+out_unlock:
+ filemap_invalidate_unlock(file->f_mapping);
+ return ret;
+}
+
long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long npages,
kvm_gmem_populate_cb post_populate, void *opaque)
{
+ struct page *src_aligned_page = NULL;
struct kvm_memory_slot *slot;
+ struct page *src_page = NULL;
void __user *p;
-
- int ret = 0, max_order;
+ int ret = 0;
long i;
lockdep_assert_held(&kvm->slots_lock);
@@ -834,52 +874,50 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
if (!file)
return -EFAULT;
- filemap_invalidate_lock(file->f_mapping);
+ if (src && !PAGE_ALIGNED(src)) {
+ src_page = alloc_page(GFP_KERNEL_ACCOUNT);
+ if (!src_page)
+ return -ENOMEM;
+ }
npages = min_t(ulong, slot->npages - (start_gfn - slot->base_gfn), npages);
- for (i = 0; i < npages; i += (1 << max_order)) {
- struct folio *folio;
- gfn_t gfn = start_gfn + i;
- pgoff_t index = kvm_gmem_get_index(slot, gfn);
- kvm_pfn_t pfn;
-
+ for (i = 0; i < npages; i++) {
if (signal_pending(current)) {
ret = -EINTR;
break;
}
- folio = __kvm_gmem_get_pfn(file, slot, index, &pfn, &max_order);
- if (IS_ERR(folio)) {
- ret = PTR_ERR(folio);
- break;
- }
-
- folio_unlock(folio);
- WARN_ON(!IS_ALIGNED(gfn, 1 << max_order) ||
- (npages - i) < (1 << max_order));
+ p = src ? src + i * PAGE_SIZE : NULL;
- ret = -EINVAL;
- while (!kvm_range_has_memory_attributes(kvm, gfn, gfn + (1 << max_order),
- KVM_MEMORY_ATTRIBUTE_PRIVATE,
- KVM_MEMORY_ATTRIBUTE_PRIVATE)) {
- if (!max_order)
- goto put_folio_and_exit;
- max_order--;
+ if (p) {
+ if (src_page) {
+ if (copy_from_user(page_address(src_page), p, PAGE_SIZE)) {
+ ret = -EFAULT;
+ break;
+ }
+ src_aligned_page = src_page;
+ } else {
+ ret = get_user_pages((unsigned long)p, 1, 0, &src_aligned_page);
+ if (ret < 0)
+ break;
+ if (ret != 1) {
+ ret = -ENOMEM;
+ break;
+ }
+ }
}
- p = src ? src + i * PAGE_SIZE : NULL;
- ret = post_populate(kvm, gfn, pfn, p, max_order, opaque);
- if (!ret)
- folio_mark_uptodate(folio);
+ ret = __kvm_gmem_populate(kvm, slot, file, start_gfn + i, src_aligned_page,
+ post_populate, opaque);
+ if (p && !src_page)
+ put_page(src_aligned_page);
-put_folio_and_exit:
- folio_put(folio);
if (ret)
break;
}
- filemap_invalidate_unlock(file->f_mapping);
-
+ if (src_page)
+ __free_page(src_page);
return ret && !i ? ret : i;
}
EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_gmem_populate);
--
2.52.0.177.g9f829587af-goog
^ permalink raw reply related
* Re: [PATCH 3/3] KVM: guest_memfd: GUP source pages prior to populating guest memory
From: Ira Weiny @ 2025-12-03 21:01 UTC (permalink / raw)
To: Michael Roth, Yan Zhao
Cc: kvm, linux-coco, linux-mm, linux-kernel, thomas.lendacky,
pbonzini, seanjc, vbabka, ashish.kalra, liam.merwick, david,
vannapurve, ackerleytng, aik, ira.weiny
In-Reply-To: <20251203142648.trx6sslxvxr26yzd@amd.com>
Michael Roth wrote:
> On Wed, Dec 03, 2025 at 10:46:27AM +0800, Yan Zhao wrote:
> > On Mon, Dec 01, 2025 at 04:13:55PM -0600, Michael Roth wrote:
> > > On Mon, Nov 24, 2025 at 05:31:46PM +0800, Yan Zhao wrote:
> > > > On Fri, Nov 21, 2025 at 07:01:44AM -0600, Michael Roth wrote:
> > > > > On Thu, Nov 20, 2025 at 05:11:48PM +0800, Yan Zhao wrote:
> > > > > > On Thu, Nov 13, 2025 at 05:07:59PM -0600, Michael Roth wrote:
[snip]
> > > > > > > ---
> > > > > > > arch/x86/kvm/svm/sev.c | 40 ++++++++++++++++++++++++++------------
> > > > > > > arch/x86/kvm/vmx/tdx.c | 21 +++++---------------
> > > > > > > include/linux/kvm_host.h | 3 ++-
> > > > > > > virt/kvm/guest_memfd.c | 42 ++++++++++++++++++++++++++++++++++------
> > > > > > > 4 files changed, 71 insertions(+), 35 deletions(-)
> > > > > > >
> > > > > > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > > > > > > index 0835c664fbfd..d0ac710697a2 100644
> > > > > > > --- a/arch/x86/kvm/svm/sev.c
> > > > > > > +++ b/arch/x86/kvm/svm/sev.c
> > > > > > > @@ -2260,7 +2260,8 @@ struct sev_gmem_populate_args {
> > > > > > > };
> > > > > > >
> > > > > > > static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn_start, kvm_pfn_t pfn,
> > > > > > > - void __user *src, int order, void *opaque)
> > > > > > > + struct page **src_pages, loff_t src_offset,
> > > > > > > + int order, void *opaque)
> > > > > > > {
> > > > > > > struct sev_gmem_populate_args *sev_populate_args = opaque;
> > > > > > > struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
> > > > > > > @@ -2268,7 +2269,7 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn_start, kvm_pfn_t pf
> > > > > > > int npages = (1 << order);
> > > > > > > gfn_t gfn;
> > > > > > >
> > > > > > > - if (WARN_ON_ONCE(sev_populate_args->type != KVM_SEV_SNP_PAGE_TYPE_ZERO && !src))
> > > > > > > + if (WARN_ON_ONCE(sev_populate_args->type != KVM_SEV_SNP_PAGE_TYPE_ZERO && !src_pages))
> > > > > > > return -EINVAL;
> > > > > > >
> > > > > > > for (gfn = gfn_start, i = 0; gfn < gfn_start + npages; gfn++, i++) {
> > > > > > > @@ -2284,14 +2285,21 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn_start, kvm_pfn_t pf
> > > > > > > goto err;
> > > > > > > }
> > > > > > >
> > > > > > > - if (src) {
> > > > > > > - void *vaddr = kmap_local_pfn(pfn + i);
> > > > > > > + if (src_pages) {
> > > > > > > + void *src_vaddr = kmap_local_pfn(page_to_pfn(src_pages[i]));
> > > > > > > + void *dst_vaddr = kmap_local_pfn(pfn + i);
> > > > > > >
> > > > > > > - if (copy_from_user(vaddr, src + i * PAGE_SIZE, PAGE_SIZE)) {
> > > > > > > - ret = -EFAULT;
> > > > > > > - goto err;
> > > > > > > + memcpy(dst_vaddr, src_vaddr + src_offset, PAGE_SIZE - src_offset);
> > > > > > > + kunmap_local(src_vaddr);
> > > > > > > +
> > > > > > > + if (src_offset) {
> > > > > > > + src_vaddr = kmap_local_pfn(page_to_pfn(src_pages[i + 1]));
> > > > > > > +
> > > > > > > + memcpy(dst_vaddr + PAGE_SIZE - src_offset, src_vaddr, src_offset);
> > > > > > > + kunmap_local(src_vaddr);
> > > > > > IIUC, src_offset is the src's offset from the first page. e.g.,
> > > > > > src could be 0x7fea82684100, with src_offset=0x100, while npages could be 512.
> > > > > >
> > > > > > Then it looks like the two memcpy() calls here only work when npages == 1 ?
> > > > >
> > > > > src_offset ends up being the offset into the pair of src pages that we
> > > > > are using to fully populate a single dest page with each iteration. So
> > > > > if we start at src_offset, read a page worth of data, then we are now at
> > > > > src_offset in the next src page and the loop continues that way even if
> > > > > npages > 1.
> > > > >
> > > > > If src_offset is 0 we never have to bother with straddling 2 src pages so
> > > > > the 2nd memcpy is skipped on every iteration.
> > > > >
> > > > > That's the intent at least. Is there a flaw in the code/reasoning that I
> > > > > missed?
> > > > Oh, I got you. SNP expects a single src_offset applies for each src page.
> > > >
> > > > So if npages = 2, there're 4 memcpy() calls.
> > > >
> > > > src: |---------|---------|---------| (VA contiguous)
> > > > ^ ^ ^
> > > > | | |
> > > > dst: |---------|---------| (PA contiguous)
> > > >
> > > >
> > > > I previously incorrectly thought kvm_gmem_populate() should pass in src_offset
> > > > as 0 for the 2nd src page.
> > > >
> > > > Would you consider checking if params.uaddr is PAGE_ALIGNED() in
> > > > snp_launch_update() to simplify the design?
> > >
> > > This was an option mentioned in the cover letter and during PUCK. I am
> > > not opposed if that's the direction we decide, but I also don't think
> > > it makes big difference since:
> > >
> > > int (*kvm_gmem_populate_cb)(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
> > > struct page **src_pages, loff_t src_offset,
> > > int order, void *opaque);
> > >
> > > basically reduces to Sean's originally proposed:
> > >
> > > int (*kvm_gmem_populate_cb)(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
> > > struct page *src_pages, int order,
> > > void *opaque);
> >
> > Hmm, the requirement of having each copy to dst_page account for src_offset
> > (which actually results in 2 copies) is quite confusing. I initially thought the
> > src_offset only applied to the first dst_page.
>
> What I'm wondering though is if I'd done a better job of documenting
> this aspect, e.g. with the following comment added above
> kvm_gmem_populate_cb:
>
> /*
> * ...
> * 'src_pages': array of GUP'd struct page pointers corresponding to
> * the pages that store the data that is to be copied
> * into the HPA corresponding to 'pfn'
> * 'src_offset': byte offset, relative to the first page in the array
> * of pages pointed to by 'src_pages', to begin copying
> * the data from.
> *
> * NOTE: if the caller of kvm_gmem_populate() enforces that 'src' is
> * page-aligned, then 'src_offset' will always be zero, and src_pages
> * will contain only 1 page to copy from, beginning at byte offset 0.
> * In this case, callers can assume src_offset is 0.
> */
> int (*kvm_gmem_populate_cb)(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
> struct page **src_pages, loff_t src_offset,
> int order, void *opaque);
>
> could the confusion have been avoided, or is it still unwieldly?
>
> I don't mind that users like SNP need to deal with the extra bits, but
> I'm hoping for users like TDX it isn't too cludgy.
FWIW I don't think the TDX code was a problem. I was trying to review the
SNP code for correctness and it was confusing enough that I was concerned
the investment is not worth the cost.
I'll re-iterate that the in-place conversion _use_ _case_ requires user
space to keep the 'source' (ie the page) aligned because it is all getting
converted anyway. So I'm not seeing a good use case for supporting this.
But Vishal seemed to think there was so...
Given this potential use case; the above comment is more clear.
FWIW, I think this is going to get even more complex if the src/dest page
sizes are miss-matched. But that algorithm can be reviewed at that time,
not now.
> >
> > This will also cause kvm_gmem_populate() to allocate 1 more src_npages than
> > npages for dst pages.
>
> That's more of a decision on the part of userspace deciding to use
> non-page-aligned 'src' pointer to begin with.
IIRC this is where I think there might be an issue with the code. The
code used PAGE_SIZE for the memcpy's. Is it clear that user space must
have a buffer >= PAGE_SIZE when src_offset != 0?
I did not see that check; and/or I was not clear how that works.
[snip]
> > > > >
> > > > > This was necessarily chosen in prep for hugepages, but more about my
> > > > > unease at letting userspace GUP arbitrarilly large ranges. PMD_ORDER
> > > > > happens to align with 2MB hugepages while seeming like a reasonable
> > > > > batching value so that's why I chose it.
> > > > >
> > > > > Even with 1GB support, I wasn't really planning to increase it. SNP
> > > > > doesn't really make use of RMP sizes >2MB, and it sounds like TDX
> > > > > handles promotion in a completely different path. So atm I'm leaning
> > > > > toward just letting GMEM_GUP_NPAGES be the cap for the max page size we
> > > > > support for kvm_gmem_populate() path and not bothering to change it
> > > > > until a solid use-case arises.
> > > > The problem is that with hugetlb-based guest_memfd, the folio itself could be
> > > > of 1GB, though SNP and TDX can force mapping at only 4KB.
> > >
> > > If TDX wants to unload handling of page-clearing to its per-page
> > > post-populate callback and tie that its shared/private tracking that's
> > > perfectly fine by me.
> > >
> > > *How* TDX tells gmem it wants this different behavior is a topic for a
> > > follow-up patchset, Vishal suggested kernel-internal flags to
> > > kvm_gmem_create(), which seemed reasonable to me. In that case, uptodate
> > Not sure which flag you are referring to with "Vishal suggested kernel-internal
> > flags to kvm_gmem_create()".
> >
> > However, my point is that when the backend folio is 1GB in size (leading to
> > max_order being PUD_ORDER), even if SNP only maps at 2MB to RMP, it may hit the
> > warning of "!IS_ALIGNED(gfn, 1 << max_order)".
>
> I think I've had to remove that warning every time I start working on
> some new spin of THP/hugetlbfs-based SNP. I'm not objecting to that. But it's
> obvious there, in those contexts, and I can explain exactly why it's being
> removed.
>
> It's not obvious in this series, where all we have are hand-wavy thoughts
> about what hugepages will look like. For all we know we might decide that
> kvm_gmem_populate() path should just pre-split hugepages to make all the
> logic easier, or we decide to lock it in at 4K-only and just strip all the
> hugepage stuff out.
Yea don't do that.
> I don't really know, and this doesn't seem like the place
> to try to hash all that out when nothing in this series will cause this
> existing WARN_ON to be tripped.
Agreed.
[snip]
>
> >
> > > but it makes a lot more sense to make those restrictions and changes in
> > > the context of hugepage support, rather than this series which is trying
> > > very hard to not do hugepage enablement, but simply keep what's partially
> > > there intact while reworking other things that have proven to be
> > > continued impediments to both in-place conversion and hugepage
> > > enablement.
> > Not sure how fixing the warning in this series could impede hugepage enabling :)
> >
> > But if you prefer, I don't mind keeping it locally for longer.
>
> It's the whole burden of needing to anticipate hugepage design, while it
> is in a state of potentially massive flux just before LPC, in order to
> make tiny incremental progress toward enabling in-place conversion,
> which is something I think we can get upstream much sooner. Look at your
> changelog for the change above, for instance: it has no relevance in the
> context of this series. What do I put in its place? Bug reports about
> my experimental tree? It's just not the right place to try to justify
> these changes.
>
> And most of this weirdness stems from the fact that we prematurely added
> partial hugepage enablement to begin with. Let's not repeat these mistakes,
> and address changes in the proper context where we know they make sense.
>
> I considered stripping out the existing hugepage support as a pre-patch
> to avoid leaving these uncertainties in place while we are reworking
> things, but it felt like needless churn. But that's where I'm coming
> from with this series: let's get in-place conversion landed, get the API
> fleshed out, get it working, and then re-assess hugepages with all these
> common/intersecting bits out of the way. If we can remove some obstacles
> for hugepages as part of that, great, but that is not the main intent
> here.
I'd like to second what Mike is saying here. The entire discussion about
hugepage support is premature for this series.
Ira
[snip]
^ permalink raw reply
* Re: [PATCH v4] page_alloc: allow migration of smaller hugepages during contig_alloc
From: David Hildenbrand (Red Hat) @ 2025-12-03 20:14 UTC (permalink / raw)
To: Gregory Price
Cc: Frank van der Linden, Johannes Weiner, linux-mm, kernel-team,
linux-kernel, akpm, vbabka, surenb, mhocko, jackmanb, ziy, kas,
dave.hansen, rick.p.edgecombe, muchun.song, osalvador, x86,
linux-coco, kvm, Wei Yang, David Rientjes, Joshua Hahn
In-Reply-To: <aTCZEcJqcgGv8Zir@gourry-fedora-PF4VCD3F>
On 12/3/25 21:09, Gregory Price wrote:
> On Wed, Dec 03, 2025 at 08:43:29PM +0100, David Hildenbrand (Red Hat) wrote:
>> On 12/3/25 19:01, Frank van der Linden wrote:
>>>
>>> The PageHuge() check seems a bit out of place there, if you just
>>> removed it altogether you'd get the same results, right? The isolation
>>> code will deal with it. But sure, it does potentially avoid doing some
>>> unnecessary work.
>>
>> commit 4d73ba5fa710fe7d432e0b271e6fecd252aef66e
>> Author: Mel Gorman <mgorman@techsingularity.net>
>> Date: Fri Apr 14 15:14:29 2023 +0100
>>
>> mm: page_alloc: skip regions with hugetlbfs pages when allocating 1G pages
>> A bug was reported by Yuanxi Liu where allocating 1G pages at runtime is
>> taking an excessive amount of time for large amounts of memory. Further
>> testing allocating huge pages that the cost is linear i.e. if allocating
>> 1G pages in batches of 10 then the time to allocate nr_hugepages from
>> 10->20->30->etc increases linearly even though 10 pages are allocated at
>> each step. Profiles indicated that much of the time is spent checking the
>> validity within already existing huge pages and then attempting a
>> migration that fails after isolating the range, draining pages and a whole
>> lot of other useless work.
>> Commit eb14d4eefdc4 ("mm,page_alloc: drop unnecessary checks from
>> pfn_range_valid_contig") removed two checks, one which ignored huge pages
>> for contiguous allocations as huge pages can sometimes migrate. While
>> there may be value on migrating a 2M page to satisfy a 1G allocation, it's
>> potentially expensive if the 1G allocation fails and it's pointless to try
>> moving a 1G page for a new 1G allocation or scan the tail pages for valid
>> PFNs.
>> Reintroduce the PageHuge check and assume any contiguous region with
>> hugetlbfs pages is unsuitable for a new 1G allocation.
>>
>
> Worth noting that because this check really only applies to gigantic
> page *reservation* (not faulting), this isn't necessarily incurred in a
> time critical path. So, maybe i'm biased here, the reliability increase
> feels like a win even if the operation can take a very long time under
> memory pressure scenarios (which seems like an outliar anyway).
Not sure I understand correctly. I think the fix from Mel was the right
thing to do.
It does not make sense to try migrating a 1GB page when allocating a 1GB
page. Ever.
--
Cheers
David
^ permalink raw reply
* Re: [PATCH v4 07/16] x86/virt/tdx: Add tdx_alloc/free_page() helpers
From: Dave Hansen @ 2025-12-03 20:13 UTC (permalink / raw)
To: Edgecombe, Rick P, nik.borisov@suse.com, kas@kernel.org
Cc: kvm@vger.kernel.org, Li, Xiaoyao, Huang, Kai,
linux-coco@lists.linux.dev, Zhao, Yan Y, Wu, Binbin,
linux-kernel@vger.kernel.org, seanjc@google.com, mingo@redhat.com,
pbonzini@redhat.com, tglx@linutronix.de, Yamahata, Isaku,
Annapurve, Vishal, Gao, Chao, bp@alien8.de, x86@kernel.org
In-Reply-To: <8bd4850b0c74fbed531232a4a69603882a5562a1.camel@intel.com>
On 12/3/25 11:59, Edgecombe, Rick P wrote:
> On Wed, 2025-12-03 at 10:21 -0800, Dave Hansen wrote:
>>> Thanks Dave. Yes, let's stick to the spec. I'm going to try to pull the
>>> loops
>>> out too because we can get rid of the union array thing too.
>>
>> Also, I honestly don't see the problem with just allocating an order-1
>> page for this. Yeah, the TDX modules doesn't need physically contiguous
>> pages, but it's easier for _us_ to lug them around if they are
>> physically contiguous.
>
> We have two spin locks to contend with for these allocations. One is the global
> spin lock on the arch/x86 side. In this case, the the pages don't have to be
> passed far, like:
>
> tdx_pamt_get(some_page, NULL)
> page1 = alloc()
> page2 = alloc()
>
> scoped_guard(spinlock, &pamt_lock) {
> tdh_phymem_pamt_add(.., page1, page2)
> /* Pack into struct */
> seamcall()
> }
>
> I think it's not too bad?
No, that's not bad.
The thing I thought was annoying was in the past when there were a bunch
of functions with two explicit page pointers plumbed in to them.
> So if we decide to pass a single order-1 page into tdx_pamt_get() instead of
> order_0_cache, we can stop passing the cache between KVM and arch/x86, but we
> then need two cache's instead of one. One for order-0 S-EPT page tables and one
> for order-1 DPAMT page pairs.
>
> Also, if we have to allocate the order-1 page in each caller, it simplifies the
> arch/x86 code, but duplicates the allocation in the KVM callers (only 2 today
> though).
>
> So I'm suspicious it's not going to be a big win, but I'll give it a try.
Yeah, the value of doing order-1 is super low if it means managing a
second cache.
>> Plus, if you permanently allocate 2 order-0 pages, you are _probably_
>> going to permanently destroy 2 potential future 2MB pages. The order-1
>> allocation will only destroy 1.
>
> Doesn't the buddy allocator try to avoid splitting larger blocks? I guess you
> mean in the worst case, but the DPAMT should also not be allocated forever
> either. So I think it's only at the intersection of two worst cases? Worth it?
It's not splitting them in this case. They're *already* split:
# cat /proc/buddyinfo
...
Node 0, zone Normal 32903 33566 ...
See, there are already ~33,000 4k pages sitting there. Those will be
consumed first on any 4k allocation. So, yeah, it'll avoid splitting an
8k block to get 4k pages normally.
BTW, I *DO* expect the DPAMT pages to be mostly allocated forever. Maybe
I'm just a pessimist, but you can't get them back for compaction or
reclaim, so they're basically untouchable. Sure, if you kill all the TDX
guests you get them back, but that's a very different kind of kernel
memory from stuff that's truly reclaimable under pressure.
^ permalink raw reply
* Re: [PATCH v4] page_alloc: allow migration of smaller hugepages during contig_alloc
From: Gregory Price @ 2025-12-03 20:09 UTC (permalink / raw)
To: David Hildenbrand (Red Hat)
Cc: Frank van der Linden, Johannes Weiner, linux-mm, kernel-team,
linux-kernel, akpm, vbabka, surenb, mhocko, jackmanb, ziy, kas,
dave.hansen, rick.p.edgecombe, muchun.song, osalvador, x86,
linux-coco, kvm, Wei Yang, David Rientjes, Joshua Hahn
In-Reply-To: <b5a925c5-523e-41e1-a3ce-0bb51ce0e995@kernel.org>
On Wed, Dec 03, 2025 at 08:43:29PM +0100, David Hildenbrand (Red Hat) wrote:
> On 12/3/25 19:01, Frank van der Linden wrote:
> >
> > The PageHuge() check seems a bit out of place there, if you just
> > removed it altogether you'd get the same results, right? The isolation
> > code will deal with it. But sure, it does potentially avoid doing some
> > unnecessary work.
>
> commit 4d73ba5fa710fe7d432e0b271e6fecd252aef66e
> Author: Mel Gorman <mgorman@techsingularity.net>
> Date: Fri Apr 14 15:14:29 2023 +0100
>
> mm: page_alloc: skip regions with hugetlbfs pages when allocating 1G pages
> A bug was reported by Yuanxi Liu where allocating 1G pages at runtime is
> taking an excessive amount of time for large amounts of memory. Further
> testing allocating huge pages that the cost is linear i.e. if allocating
> 1G pages in batches of 10 then the time to allocate nr_hugepages from
> 10->20->30->etc increases linearly even though 10 pages are allocated at
> each step. Profiles indicated that much of the time is spent checking the
> validity within already existing huge pages and then attempting a
> migration that fails after isolating the range, draining pages and a whole
> lot of other useless work.
> Commit eb14d4eefdc4 ("mm,page_alloc: drop unnecessary checks from
> pfn_range_valid_contig") removed two checks, one which ignored huge pages
> for contiguous allocations as huge pages can sometimes migrate. While
> there may be value on migrating a 2M page to satisfy a 1G allocation, it's
> potentially expensive if the 1G allocation fails and it's pointless to try
> moving a 1G page for a new 1G allocation or scan the tail pages for valid
> PFNs.
> Reintroduce the PageHuge check and assume any contiguous region with
> hugetlbfs pages is unsuitable for a new 1G allocation.
>
Worth noting that because this check really only applies to gigantic
page *reservation* (not faulting), this isn't necessarily incurred in a
time critical path. So, maybe i'm biased here, the reliability increase
feels like a win even if the operation can take a very long time under
memory pressure scenarios (which seems like an outliar anyway).
~Gregory
^ permalink raw reply
* Re: [PATCH v4 07/16] x86/virt/tdx: Add tdx_alloc/free_page() helpers
From: Edgecombe, Rick P @ 2025-12-03 19:59 UTC (permalink / raw)
To: Hansen, Dave, nik.borisov@suse.com, kas@kernel.org
Cc: kvm@vger.kernel.org, Li, Xiaoyao, Huang, Kai,
linux-coco@lists.linux.dev, Zhao, Yan Y, Wu, Binbin,
linux-kernel@vger.kernel.org, seanjc@google.com, mingo@redhat.com,
pbonzini@redhat.com, tglx@linutronix.de, Yamahata, Isaku,
Annapurve, Vishal, Gao, Chao, bp@alien8.de, x86@kernel.org
In-Reply-To: <474f5ace-e237-4c01-b0bc-d3e68ecc937b@intel.com>
On Wed, 2025-12-03 at 10:21 -0800, Dave Hansen wrote:
> > Thanks Dave. Yes, let's stick to the spec. I'm going to try to pull the
> > loops
> > out too because we can get rid of the union array thing too.
>
> Also, I honestly don't see the problem with just allocating an order-1
> page for this. Yeah, the TDX modules doesn't need physically contiguous
> pages, but it's easier for _us_ to lug them around if they are
> physically contiguous.
We have two spin locks to contend with for these allocations. One is the global
spin lock on the arch/x86 side. In this case, the the pages don't have to be
passed far, like:
tdx_pamt_get(some_page, NULL)
page1 = alloc()
page2 = alloc()
scoped_guard(spinlock, &pamt_lock) {
tdh_phymem_pamt_add(.., page1, page2)
/* Pack into struct */
seamcall()
}
I think it's not too bad?
Then there is the KVM MMU spin lock during the fault path. This lock happens way
up the call chain. It goes something like:
topup_tdx_pages_cache() /* Add order-0 pages for S-EPT page tables and dpamt */
spin_lock()
... many calls ...
order_0_s_ept_page table = alloc_from_order_0_cache();
tdx_sept_link_private_spt(order_0_s_ept_page)
tdx_pamt_get(order_0_s_ept_page, order_0_cache)
/* alloc two pages from order_0_cache for dpamt */
tdx_sept_set_private_spte(guest_page)
tdx_pamt_get(guest_page, order_0_cache)
/* alloc two pages from order_0_cache for dpamt*/
spin_unlock()
So if we decide to pass a single order-1 page into tdx_pamt_get() instead of
order_0_cache, we can stop passing the cache between KVM and arch/x86, but we
then need two cache's instead of one. One for order-0 S-EPT page tables and one
for order-1 DPAMT page pairs.
Also, if we have to allocate the order-1 page in each caller, it simplifies the
arch/x86 code, but duplicates the allocation in the KVM callers (only 2 today
though).
So I'm suspicious it's not going to be a big win, but I'll give it a try.
>
> Plus, if you permanently allocate 2 order-0 pages, you are _probably_
> going to permanently destroy 2 potential future 2MB pages. The order-1
> allocation will only destroy 1.
Doesn't the buddy allocator try to avoid splitting larger blocks? I guess you
mean in the worst case, but the DPAMT should also not be allocated forever
either. So I think it's only at the intersection of two worst cases? Worth it?
^ permalink raw reply
* Re: [PATCH v4] page_alloc: allow migration of smaller hugepages during contig_alloc
From: David Hildenbrand (Red Hat) @ 2025-12-03 19:43 UTC (permalink / raw)
To: Frank van der Linden, Gregory Price
Cc: Johannes Weiner, linux-mm, kernel-team, linux-kernel, akpm,
vbabka, surenb, mhocko, jackmanb, ziy, kas, dave.hansen,
rick.p.edgecombe, muchun.song, osalvador, x86, linux-coco, kvm,
Wei Yang, David Rientjes, Joshua Hahn
In-Reply-To: <CAPTztWar1KqLyUOknkf0XVnO9JOBbMxov6pHZjEm4Ou0wtSJTA@mail.gmail.com>
On 12/3/25 19:01, Frank van der Linden wrote:
> On Wed, Dec 3, 2025 at 9:53 AM Gregory Price <gourry@gourry.net> wrote:
>>
>> On Wed, Dec 03, 2025 at 12:32:09PM -0500, Johannes Weiner wrote:
>>> On Wed, Dec 03, 2025 at 01:30:04AM -0500, Gregory Price wrote:
>>>> - if (PageHuge(page))
>>>> - return false;
>>>> + /*
>>>> + * Only consider ranges containing hugepages if those pages are
>>>> + * smaller than the requested contiguous region. e.g.:
>>>> + * Move 2MB pages to free up a 1GB range.
>>>
>>> This one makes sense to me.
>>>
>>>> + * Don't move 1GB pages to free up a 2MB range.
>>>
>>> This one I might be missing something. We don't use cma for 2M pages,
>>> so I don't see how we can end up in this path for 2M allocations.
>>>
>>
>> I used 2MB as an example, but the other users (listed in the changelog)
>> would run into these as well. The contiguous order size seemed
>> different between each of the 4 users (memtrace, tx, kfence, thp debug).
>>
>>> The reason I'm bringing this up is because this function overall looks
>>> kind of unnecessary. Page isolation checks all of these conditions
>>> already, and arbitrates huge pages on hugepage_migration_supported() -
>>> which seems to be the semantics you also desire here.
>>>
>>> Would it make sense to just remove pfn_range_valid_contig()?
>>
>> This seems like a pretty clear optimization that was added at some point
>> to prevent incurring the cost of starting to isolate 512MB of pages and
>> then having to go undo it because it ran into a single huge page.
>>
>> for_each_zone_zonelist_nodemask(zone, z, zonelist,
>> gfp_zone(gfp_mask), nodemask) {
>>
>> spin_lock_irqsave(&zone->lock, flags);
>> pfn = ALIGN(zone->zone_start_pfn, nr_pages);
>> while (zone_spans_last_pfn(zone, pfn, nr_pages)) {
>> if (pfn_range_valid_contig(zone, pfn, nr_pages)) {
>>
>> spin_unlock_irqrestore(&zone->lock, flags);
>> ret = __alloc_contig_pages(pfn, nr_pages,
>> gfp_mask);
>> spin_lock_irqsave(&zone->lock, flags);
>>
>> }
>> pfn += nr_pages;
>> }
>> spin_unlock_irqrestore(&zone->lock, flags);
>> }
>>
>> and then
>>
>> __alloc_contig_pages
>> ret = start_isolate_page_range(start, end, mode);
>>
>> This is called without pre-checking the range for unmovable pages.
>>
>> Seems dangerous to remove without significant data.
>>
>> ~Gregory
>
> Yeah, the function itself makes sense: "check if this is actually a
> contiguous range available within this zone, so no holes and/or
> reserved pages".
>
> The PageHuge() check seems a bit out of place there, if you just
> removed it altogether you'd get the same results, right? The isolation
> code will deal with it. But sure, it does potentially avoid doing some
> unnecessary work.
commit 4d73ba5fa710fe7d432e0b271e6fecd252aef66e
Author: Mel Gorman <mgorman@techsingularity.net>
Date: Fri Apr 14 15:14:29 2023 +0100
mm: page_alloc: skip regions with hugetlbfs pages when allocating 1G pages
A bug was reported by Yuanxi Liu where allocating 1G pages at runtime is
taking an excessive amount of time for large amounts of memory. Further
testing allocating huge pages that the cost is linear i.e. if allocating
1G pages in batches of 10 then the time to allocate nr_hugepages from
10->20->30->etc increases linearly even though 10 pages are allocated at
each step. Profiles indicated that much of the time is spent checking the
validity within already existing huge pages and then attempting a
migration that fails after isolating the range, draining pages and a whole
lot of other useless work.
Commit eb14d4eefdc4 ("mm,page_alloc: drop unnecessary checks from
pfn_range_valid_contig") removed two checks, one which ignored huge pages
for contiguous allocations as huge pages can sometimes migrate. While
there may be value on migrating a 2M page to satisfy a 1G allocation, it's
potentially expensive if the 1G allocation fails and it's pointless to try
moving a 1G page for a new 1G allocation or scan the tail pages for valid
PFNs.
Reintroduce the PageHuge check and assume any contiguous region with
hugetlbfs pages is unsuitable for a new 1G allocation.
...
--
Cheers
David
^ permalink raw reply
* Re: [PATCH v4 07/16] x86/virt/tdx: Add tdx_alloc/free_page() helpers
From: Dave Hansen @ 2025-12-03 18:21 UTC (permalink / raw)
To: Edgecombe, Rick P, nik.borisov@suse.com, kas@kernel.org
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Li, Xiaoyao, Zhao, Yan Y, Wu, Binbin,
linux-kernel@vger.kernel.org, seanjc@google.com, mingo@redhat.com,
pbonzini@redhat.com, tglx@linutronix.de, Yamahata, Isaku,
Annapurve, Vishal, Gao, Chao, bp@alien8.de, x86@kernel.org
In-Reply-To: <bb174006cbe969fc71fe71a3e12003ab9052213c.camel@intel.com>
On 12/3/25 10:15, Edgecombe, Rick P wrote:
> On Wed, 2025-12-03 at 07:41 -0800, Dave Hansen wrote:
>> It's fixed at a "page pair" in the ABI documentation, no?
>>
>> If Intel wants anything else, it should have documented that as the ABI.
>> So, as far as the code goes, it's a "page pair" now and forever. Linux
>> does not need to go out of its way to make it inflexible, but there's no
>> need to add complexity now for some future that may never come.
>>
>> I agree with Nikolay: KISS.
> Thanks Dave. Yes, let's stick to the spec. I'm going to try to pull the loops
> out too because we can get rid of the union array thing too.
Also, I honestly don't see the problem with just allocating an order-1
page for this. Yeah, the TDX modules doesn't need physically contiguous
pages, but it's easier for _us_ to lug them around if they are
physically contiguous.
Plus, if you permanently allocate 2 order-0 pages, you are _probably_
going to permanently destroy 2 potential future 2MB pages. The order-1
allocation will only destroy 1.
^ permalink raw reply
* Re: [PATCH v4] page_alloc: allow migration of smaller hugepages during contig_alloc
From: Johannes Weiner @ 2025-12-03 18:19 UTC (permalink / raw)
To: Gregory Price
Cc: linux-mm, kernel-team, linux-kernel, akpm, vbabka, surenb, mhocko,
jackmanb, ziy, kas, dave.hansen, rick.p.edgecombe, muchun.song,
osalvador, david, x86, linux-coco, kvm, Wei Yang, David Rientjes,
Joshua Hahn
In-Reply-To: <aTB5CJ0oFfPjavGx@gourry-fedora-PF4VCD3F>
On Wed, Dec 03, 2025 at 12:53:12PM -0500, Gregory Price wrote:
> On Wed, Dec 03, 2025 at 12:32:09PM -0500, Johannes Weiner wrote:
> > The reason I'm bringing this up is because this function overall looks
> > kind of unnecessary. Page isolation checks all of these conditions
> > already, and arbitrates huge pages on hugepage_migration_supported() -
> > which seems to be the semantics you also desire here.
> >
> > Would it make sense to just remove pfn_range_valid_contig()?
>
> This seems like a pretty clear optimization that was added at some point
> to prevent incurring the cost of starting to isolate 512MB of pages and
> then having to go undo it because it ran into a single huge page.
>
> for_each_zone_zonelist_nodemask(zone, z, zonelist,
> gfp_zone(gfp_mask), nodemask) {
>
> spin_lock_irqsave(&zone->lock, flags);
> pfn = ALIGN(zone->zone_start_pfn, nr_pages);
> while (zone_spans_last_pfn(zone, pfn, nr_pages)) {
> if (pfn_range_valid_contig(zone, pfn, nr_pages)) {
>
> spin_unlock_irqrestore(&zone->lock, flags);
> ret = __alloc_contig_pages(pfn, nr_pages,
> gfp_mask);
> spin_lock_irqsave(&zone->lock, flags);
>
> }
> pfn += nr_pages;
> }
> spin_unlock_irqrestore(&zone->lock, flags);
> }
>
> and then
>
> __alloc_contig_pages
> ret = start_isolate_page_range(start, end, mode);
>
> This is called without pre-checking the range for unmovable pages.
>
> Seems dangerous to remove without significant data.
Fair enough. It just caught my eye that the page allocator is running
all the same checks as page isolation itself.
I agree that a quick up front check is useful before updating hundreds
of page blocks, then failing and unrolling on the last one. Arguably
that should just be part of the isolation code, though, not a random
callsite. But that move is better done in a separate patch.
^ permalink raw reply
* Re: [PATCH v5] virt: tdx-guest: Handle GetQuote request error code
From: Dave Hansen @ 2025-12-03 18:16 UTC (permalink / raw)
To: Sathyanarayanan Kuppuswamy, Kirill A . Shutemov
Cc: Rick Edgecombe, Dave Hansen, Dan Williams, x86, linux-kernel,
linux-coco
In-Reply-To: <48b4ec49-76f2-4b82-8551-02dd876d1da0@linux.intel.com>
On 12/3/25 10:04, Sathyanarayanan Kuppuswamy wrote:
> Got it. How about the following version?
Still way too wordy and flowery for my taste.
^ permalink raw reply
* Re: [PATCH v4 07/16] x86/virt/tdx: Add tdx_alloc/free_page() helpers
From: Edgecombe, Rick P @ 2025-12-03 18:15 UTC (permalink / raw)
To: Hansen, Dave, nik.borisov@suse.com, kas@kernel.org
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Li, Xiaoyao, Zhao, Yan Y, Wu, Binbin,
linux-kernel@vger.kernel.org, seanjc@google.com, mingo@redhat.com,
pbonzini@redhat.com, tglx@linutronix.de, Yamahata, Isaku,
Annapurve, Vishal, Gao, Chao, bp@alien8.de, x86@kernel.org
In-Reply-To: <04c51f1d-b79b-4ff8-b141-5888407a318e@intel.com>
On Wed, 2025-12-03 at 07:41 -0800, Dave Hansen wrote:
> It's fixed at a "page pair" in the ABI documentation, no?
>
> If Intel wants anything else, it should have documented that as the ABI.
> So, as far as the code goes, it's a "page pair" now and forever. Linux
> does not need to go out of its way to make it inflexible, but there's no
> need to add complexity now for some future that may never come.
>
> I agree with Nikolay: KISS.
Thanks Dave. Yes, let's stick to the spec. I'm going to try to pull the loops
out too because we can get rid of the union array thing too.
Thanks for the catch Nikolay.
^ permalink raw reply
* Re: [PATCH v5] virt: tdx-guest: Handle GetQuote request error code
From: Sathyanarayanan Kuppuswamy @ 2025-12-03 18:04 UTC (permalink / raw)
To: Dave Hansen, Kirill A . Shutemov
Cc: Rick Edgecombe, Dave Hansen, Dan Williams, x86, linux-kernel,
linux-coco
In-Reply-To: <56c01575-c0e3-43f3-a6c6-bf62fcbe4f5d@intel.com>
Hi Dave,
On 12/2/25 4:03 PM, Dave Hansen wrote:
> On 12/2/25 16:00, Kuppuswamy Sathyanarayanan wrote:
>> The reason the kernel must parse the status field is that the failure
>> code is only available in the header portion of the shared GPA buffer
>> populated by the VMM. Userspace currently does not have access to this
>> header since we only expose the Quote payload itself. Because userspace
>> cannot directly interpret the VMM status codes, the kernel needs to parse
>> them and return appropriate generic error codes.
> That's kinda the key to this.
Agreed. I have reworked the commit message to make this the primary
motivation.
>
> Users are poking at sysfs and expect (near) universal explicit errors.
> Are they even doing this from shell scripts most of the time?
Yes, many users validate the GetQuote flow using simple shell scripts or other
minimal tooling. Since there is no common userspace library for this interface,
each vendor or user typically has their own implementation.
>
> Also, please don't just keep tacking gunk onto the changelog. Start
> cutting out the cruft, please.
Got it. How about the following version?
virt: tdx-guest: Return explicit errors for GetQuote failures
TD users often retrieve the Quote through simple libraries or shell
scripts over the configfs interface. In such cases, direct error
returns from the kernel for Quote failures are preferred and simplify
failure detection. Prior to this patch, certain VMM reported GetQuote
failures, such as Quote service unavailability or VMM processing
errors, were silently reported as success with a zero length Quote
buffer. This behavior is ambiguous and makes failure detection
complex.
The VMM reports these failures through the status Code in the header
portion of the shared GPA buffer (refer to GHCI specification v1.5
March 2023, sec titled TDG.VP.VMCALL<GetQuote>, Table 3-10 and Table
3-11 for GPA format and status code details). Userspace does not have
access to this header because only the Quote payload is exposed
through configfs. Therefore, the kernel must parse the status and
translate VMM failures into proper error codes.
Update the TDX guest driver to return explicit kernel errors for all
VMM reported GetQuote failure cases. This preserves existing ABI
behavior because userspace already treats a zero length Quote as a
failure indication. The only change is that such failures now return
explicit error codes instead of silently succeeding.
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply
* Re: [PATCH v4] page_alloc: allow migration of smaller hugepages during contig_alloc
From: Frank van der Linden @ 2025-12-03 18:01 UTC (permalink / raw)
To: Gregory Price
Cc: Johannes Weiner, linux-mm, kernel-team, linux-kernel, akpm,
vbabka, surenb, mhocko, jackmanb, ziy, kas, dave.hansen,
rick.p.edgecombe, muchun.song, osalvador, david, x86, linux-coco,
kvm, Wei Yang, David Rientjes, Joshua Hahn
In-Reply-To: <aTB5CJ0oFfPjavGx@gourry-fedora-PF4VCD3F>
On Wed, Dec 3, 2025 at 9:53 AM Gregory Price <gourry@gourry.net> wrote:
>
> On Wed, Dec 03, 2025 at 12:32:09PM -0500, Johannes Weiner wrote:
> > On Wed, Dec 03, 2025 at 01:30:04AM -0500, Gregory Price wrote:
> > > - if (PageHuge(page))
> > > - return false;
> > > + /*
> > > + * Only consider ranges containing hugepages if those pages are
> > > + * smaller than the requested contiguous region. e.g.:
> > > + * Move 2MB pages to free up a 1GB range.
> >
> > This one makes sense to me.
> >
> > > + * Don't move 1GB pages to free up a 2MB range.
> >
> > This one I might be missing something. We don't use cma for 2M pages,
> > so I don't see how we can end up in this path for 2M allocations.
> >
>
> I used 2MB as an example, but the other users (listed in the changelog)
> would run into these as well. The contiguous order size seemed
> different between each of the 4 users (memtrace, tx, kfence, thp debug).
>
> > The reason I'm bringing this up is because this function overall looks
> > kind of unnecessary. Page isolation checks all of these conditions
> > already, and arbitrates huge pages on hugepage_migration_supported() -
> > which seems to be the semantics you also desire here.
> >
> > Would it make sense to just remove pfn_range_valid_contig()?
>
> This seems like a pretty clear optimization that was added at some point
> to prevent incurring the cost of starting to isolate 512MB of pages and
> then having to go undo it because it ran into a single huge page.
>
> for_each_zone_zonelist_nodemask(zone, z, zonelist,
> gfp_zone(gfp_mask), nodemask) {
>
> spin_lock_irqsave(&zone->lock, flags);
> pfn = ALIGN(zone->zone_start_pfn, nr_pages);
> while (zone_spans_last_pfn(zone, pfn, nr_pages)) {
> if (pfn_range_valid_contig(zone, pfn, nr_pages)) {
>
> spin_unlock_irqrestore(&zone->lock, flags);
> ret = __alloc_contig_pages(pfn, nr_pages,
> gfp_mask);
> spin_lock_irqsave(&zone->lock, flags);
>
> }
> pfn += nr_pages;
> }
> spin_unlock_irqrestore(&zone->lock, flags);
> }
>
> and then
>
> __alloc_contig_pages
> ret = start_isolate_page_range(start, end, mode);
>
> This is called without pre-checking the range for unmovable pages.
>
> Seems dangerous to remove without significant data.
>
> ~Gregory
Yeah, the function itself makes sense: "check if this is actually a
contiguous range available within this zone, so no holes and/or
reserved pages".
The PageHuge() check seems a bit out of place there, if you just
removed it altogether you'd get the same results, right? The isolation
code will deal with it. But sure, it does potentially avoid doing some
unnecessary work.
- Frank
^ permalink raw reply
* Re: [PATCH v4] page_alloc: allow migration of smaller hugepages during contig_alloc
From: Gregory Price @ 2025-12-03 17:53 UTC (permalink / raw)
To: Johannes Weiner
Cc: linux-mm, kernel-team, linux-kernel, akpm, vbabka, surenb, mhocko,
jackmanb, ziy, kas, dave.hansen, rick.p.edgecombe, muchun.song,
osalvador, david, x86, linux-coco, kvm, Wei Yang, David Rientjes,
Joshua Hahn
In-Reply-To: <20251203173209.GA478168@cmpxchg.org>
On Wed, Dec 03, 2025 at 12:32:09PM -0500, Johannes Weiner wrote:
> On Wed, Dec 03, 2025 at 01:30:04AM -0500, Gregory Price wrote:
> > - if (PageHuge(page))
> > - return false;
> > + /*
> > + * Only consider ranges containing hugepages if those pages are
> > + * smaller than the requested contiguous region. e.g.:
> > + * Move 2MB pages to free up a 1GB range.
>
> This one makes sense to me.
>
> > + * Don't move 1GB pages to free up a 2MB range.
>
> This one I might be missing something. We don't use cma for 2M pages,
> so I don't see how we can end up in this path for 2M allocations.
>
I used 2MB as an example, but the other users (listed in the changelog)
would run into these as well. The contiguous order size seemed
different between each of the 4 users (memtrace, tx, kfence, thp debug).
> The reason I'm bringing this up is because this function overall looks
> kind of unnecessary. Page isolation checks all of these conditions
> already, and arbitrates huge pages on hugepage_migration_supported() -
> which seems to be the semantics you also desire here.
>
> Would it make sense to just remove pfn_range_valid_contig()?
This seems like a pretty clear optimization that was added at some point
to prevent incurring the cost of starting to isolate 512MB of pages and
then having to go undo it because it ran into a single huge page.
for_each_zone_zonelist_nodemask(zone, z, zonelist,
gfp_zone(gfp_mask), nodemask) {
spin_lock_irqsave(&zone->lock, flags);
pfn = ALIGN(zone->zone_start_pfn, nr_pages);
while (zone_spans_last_pfn(zone, pfn, nr_pages)) {
if (pfn_range_valid_contig(zone, pfn, nr_pages)) {
spin_unlock_irqrestore(&zone->lock, flags);
ret = __alloc_contig_pages(pfn, nr_pages,
gfp_mask);
spin_lock_irqsave(&zone->lock, flags);
}
pfn += nr_pages;
}
spin_unlock_irqrestore(&zone->lock, flags);
}
and then
__alloc_contig_pages
ret = start_isolate_page_range(start, end, mode);
This is called without pre-checking the range for unmovable pages.
Seems dangerous to remove without significant data.
~Gregory
^ permalink raw reply
* Re: [PATCH v4] page_alloc: allow migration of smaller hugepages during contig_alloc
From: Johannes Weiner @ 2025-12-03 17:32 UTC (permalink / raw)
To: Gregory Price
Cc: linux-mm, kernel-team, linux-kernel, akpm, vbabka, surenb, mhocko,
jackmanb, ziy, kas, dave.hansen, rick.p.edgecombe, muchun.song,
osalvador, david, x86, linux-coco, kvm, Wei Yang, David Rientjes,
Joshua Hahn
In-Reply-To: <20251203063004.185182-1-gourry@gourry.net>
On Wed, Dec 03, 2025 at 01:30:04AM -0500, Gregory Price wrote:
> We presently skip regions with hugepages entirely when trying to do
> contiguous page allocation. This will cause otherwise-movable
> 2MB HugeTLB pages to be considered unmovable, and will make 1GB
> hugepages more difficult to allocate on systems utilizing both.
>
> Instead, if hugepage migration is enabled, consider regions with
> hugepages smaller than the target contiguous allocation request
> as valid targets for allocation.
>
> isolate_migrate_pages_block() has similar logic, and the hugetlb code
> does a migratable check in folio_isolate_hugetlb() during isolation.
> So the code servicing the subsequent allocaiton and migration already
> supports this exact use case (it's just unreachable).
>
> To test, allocate a bunch of 2MB HugeTLB pages (in this case 48GB)
> and then attempt to allocate some 1G HugeTLB pages (in this case 4GB)
> (Scale to your machine's memory capacity).
>
> echo 24576 > .../hugepages-2048kB/nr_hugepages
> echo 4 > .../hugepages-1048576kB/nr_hugepages
>
> Prior to this patch, the 1GB page allocation can fail if no contiguous
> 1GB pages remain. After this patch, the kernel will try to move 2MB
> pages and successfully allocate the 1GB pages (assuming overall
> sufficient memory is available).
>
> folio_alloc_gigantic() is the primary user of alloc_contig_pages(),
> other users are debug or init-time allocations and largely unaffected.
> - ppc/memtrace is a debugfs interface
> - x86/tdx memory allocation occurs once on module-init
> - kfence/core happens once on module (late) init
> - THP uses it in debug_vm_pgtable_alloc_huge_page at __init time
>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Link: https://lore.kernel.org/linux-mm/6fe3562d-49b2-4975-aa86-e139c535ad00@redhat.com/
> Signed-off-by: Gregory Price <gourry@gourry.net>
> Reviewed-by: Zi Yan <ziy@nvidia.com>
> Reviewed-by: Wei Yang <richard.weiyang@gmail.com>
> Reviewed-by: Oscar Salvador <osalvador@suse.de>
> Acked-by: David Rientjes <rientjes@google.com>
> Acked-by: David Hildenbrand <david@redhat.com>
> Tested-by: Joshua Hahn <joshua.hahnjy@gmail.com>
> ---
> mm/page_alloc.c | 23 +++++++++++++++++++++--
> 1 file changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 95d8b812efd0..8ca3273f734a 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -7069,8 +7069,27 @@ static bool pfn_range_valid_contig(struct zone *z, unsigned long start_pfn,
> if (PageReserved(page))
> return false;
>
> - if (PageHuge(page))
> - return false;
> + /*
> + * Only consider ranges containing hugepages if those pages are
> + * smaller than the requested contiguous region. e.g.:
> + * Move 2MB pages to free up a 1GB range.
This one makes sense to me.
> + * Don't move 1GB pages to free up a 2MB range.
This one I might be missing something. We don't use cma for 2M pages,
so I don't see how we can end up in this path for 2M allocations.
The reason I'm bringing this up is because this function overall looks
kind of unnecessary. Page isolation checks all of these conditions
already, and arbitrates huge pages on hugepage_migration_supported() -
which seems to be the semantics you also desire here.
Would it make sense to just remove pfn_range_valid_contig()?
^ permalink raw reply
* Re: [RFC PATCH 2/4] mm: Add support for unaccepted memory hotplug
From: Borislav Petkov @ 2025-12-03 15:58 UTC (permalink / raw)
To: Kiryl Shutsemau
Cc: David Hildenbrand (Red Hat), ardb, Pratik R. Sampat, linux-mm,
linux-coco, linux-efi, x86, linux-kernel, tglx, mingo,
dave.hansen, akpm, osalvador, thomas.lendacky, michael.roth,
torvalds
In-Reply-To: <etd7r45wmnuoftpckrzkzithr443ru6akgwqjgzw2vgmzqi7cs@camfecdmef74>
On Wed, Dec 03, 2025 at 02:46:23PM +0000, Kiryl Shutsemau wrote:
> There is also the #1 Kernel Rule: "we do not break users."
Do I need to point you to that too:
Documentation/process/stable-api-nonsense.rst
?
> Booting a different version of the kernel is a core functionality of
> kexec. It is widely used to deploy new kernels or revert to older ones.
> Breaking this functionality is a show-stopper for most, if not all,
> hyperscalers.
>
> This specific change may not be a show-stopper as CoCo deployment is not
> widespread enough to be noticed yet.
>
> The notion that nobody promised that you can kexec into a different kernel
> is absurd. It is used everywhere.
Dude, can you please stop handwaving and say what you really wanna say: you
want different kernels to kexec. And it has worked so far but nothing
guarantees that. And we should all agree on some strategy going forward and
enforce it.
I don't care if different kernels can kexec or not. If I need to kexec, then
I simply build the same kernel.
So I'd take a patch which breaks that and when the submitter gets stopped by
you or someone else, I'll go tell him: "well, actually, I can't take your
patch because Kiryl said so but that's his opinion."
Do you see how absurd this is?!
Geez, I'm tired of typing the same shit over and over again on this thread.
Feel free to propose to make kexec'ing different kernels a rule and let's all
discuss it but let's stop this nonsense of what worked and so on. The kernel
gets complicated constantly, grows things here and there and without such
a rule, are you going to sit around and guard that kexec works?
Pfff.
> I am not involved in the deployment of CoCo VMs, but I don't believe it
> is specifically about CoCo or the kexec ABI. I think it is more about
> the boot protocol. Kexec is one way to boot the kernel.
>
> Should we consider the EFI configuration tables format as part of the
> boot protocol?
You're basically proving my point: this needs to be discussed and agreed upon.
It doesn't matter if it used to work implicitly in the past.
Thx.
--
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: Dave Hansen @ 2025-12-03 15:41 UTC (permalink / raw)
To: Nikolay Borisov, Kiryl Shutsemau, Edgecombe, Rick P
Cc: kvm@vger.kernel.org, linux-coco@lists.linux.dev, Huang, Kai,
Li, Xiaoyao, Zhao, Yan Y, Wu, Binbin, 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
In-Reply-To: <89d5876f-625b-43a6-bcad-d8caa4cbda2b@suse.com>
On 12/3/25 05:48, Nikolay Borisov wrote:
>> There was a plan to future prove DPAMT by allowing PAMT descriptor to
>> grow in the future. The concrete approach was not settled last time I
>> checked. This code was my attempt to accommodate it. I don't know if it
>> fits the current plan.
>
> Considering this, I'd opt for the simplest possible approach that works
> _now_. If in the future there are changes to the ABI let's introduce
> them incrementally when their time comes.
It's fixed at a "page pair" in the ABI documentation, no?
If Intel wants anything else, it should have documented that as the ABI.
So, as far as the code goes, it's a "page pair" now and forever. Linux
does not need to go out of its way to make it inflexible, but there's no
need to add complexity now for some future that may never come.
I agree with Nikolay: KISS.
^ permalink raw reply
* Re: [RFC PATCH 2/4] mm: Add support for unaccepted memory hotplug
From: Rik van Riel @ 2025-12-03 15:00 UTC (permalink / raw)
To: David Hildenbrand (Red Hat), Borislav Petkov, Kiryl Shutsemau
Cc: Pratik R. Sampat, linux-mm, linux-coco, linux-efi, x86,
linux-kernel, tglx, mingo, dave.hansen, ardb, akpm, osalvador,
thomas.lendacky, michael.roth
In-Reply-To: <beec5651-5c23-4f5e-a0a3-d1cc01a8490a@kernel.org>
On Fri, 2025-11-28 at 10:30 +0100, David Hildenbrand (Red Hat) wrote:
> On 11/27/25 19:12, Borislav Petkov wrote:
> >
> > None of that matters if you kexec the same kernels.
> >
> > IOW, for some reason you want to be able to kexec different
> > kernels. The
> > question is why do we care?
>
> kexecing the same kernel is typically used for kdump purposes.
>
> kexecing different kernels is used for all sorts of things
> (live-upgrade, grub-emu come to mind). It's quite common to kexec
> different kernels, or maybe I misunderstood the question here?
>
Even for kdump it is not unusual to use a different
kernel.
When working on kernel code, getting a proper crash
dump can really help figure out where my code went
wrong.
It helps if the kdump kernel doesn't have the same
broken code that my test kernel does :)
--
All Rights Reversed.
^ permalink raw reply
* Re: [RFC PATCH 2/4] mm: Add support for unaccepted memory hotplug
From: Kiryl Shutsemau @ 2025-12-03 14:46 UTC (permalink / raw)
To: David Hildenbrand (Red Hat), ardb
Cc: Borislav Petkov, Pratik R. Sampat, linux-mm, linux-coco,
linux-efi, x86, linux-kernel, tglx, mingo, dave.hansen, akpm,
osalvador, thomas.lendacky, michael.roth, torvalds
In-Reply-To: <420865fb-34cc-43a8-820c-b15b5f24a27c@kernel.org>
On Mon, Dec 01, 2025 at 09:36:58PM +0100, David Hildenbrand (Red Hat) wrote:
> On 12/1/25 21:25, Borislav Petkov wrote:
> > On Mon, Dec 01, 2025 at 09:10:26PM +0100, David Hildenbrand (Red Hat) wrote:
> > > Just to be clear, I don't think it exist and also I don't think that it
> > > should exist.
> >
> > By that logic if it doesn't exist and someone sends a patch, I should simply
> > ignore a review comment about that patch breaking some non-existent ABI and
> > simply take it.
>
> Well, we can always discuss and see if there is a way to not break a
> specific use case, independent of any ABI stability guarantees.
There is also the #1 Kernel Rule: "we do not break users."
Booting a different version of the kernel is a core functionality of
kexec. It is widely used to deploy new kernels or revert to older ones.
Breaking this functionality is a show-stopper for most, if not all,
hyperscalers.
This specific change may not be a show-stopper as CoCo deployment is not
widespread enough to be noticed yet.
The notion that nobody promised that you can kexec into a different kernel
is absurd. It is used everywhere.
> >
> > Well, it certainly works for me.
> >
> > Unless you folks come-a-runnin' later screaming it broke some use case of
> > yours.
>
> Heh, not me, but likely some of the CoCo folks regarding this specific use
> case (kexec in a confidential VM).
>
> > And then we're back to what I've been preaching on this thread from the
> > very beginning: having a common agreement on what ABI Linux enforces.
>
> Right. Maybe Kiryl knows more about this specific case as he brought up that
> these structures are versioned.
I am not involved in the deployment of CoCo VMs, but I don't believe it
is specifically about CoCo or the kexec ABI. I think it is more about
the boot protocol. Kexec is one way to boot the kernel.
Should we consider the EFI configuration tables format as part of the
boot protocol? I believe the answer is "yes," at least for some of them,
like LINUX_EFI_INITRD_MEDIA_GUID.
I also think LINUX_EFI_UNACCEPTED_MEM_TABLE_GUID should be considered in
the same way.
Ard, do you have any comments on this?
--
Kiryl Shutsemau / Kirill A. Shutemov
^ permalink raw reply
* Re: [PATCH 3/3] KVM: guest_memfd: GUP source pages prior to populating guest memory
From: Michael Roth @ 2025-12-03 14:26 UTC (permalink / raw)
To: Yan Zhao
Cc: kvm, linux-coco, linux-mm, linux-kernel, thomas.lendacky,
pbonzini, seanjc, vbabka, ashish.kalra, liam.merwick, david,
vannapurve, ackerleytng, aik, ira.weiny
In-Reply-To: <aS+kg+sHJ0lveupH@yzhao56-desk.sh.intel.com>
On Wed, Dec 03, 2025 at 10:46:27AM +0800, Yan Zhao wrote:
> On Mon, Dec 01, 2025 at 04:13:55PM -0600, Michael Roth wrote:
> > On Mon, Nov 24, 2025 at 05:31:46PM +0800, Yan Zhao wrote:
> > > On Fri, Nov 21, 2025 at 07:01:44AM -0600, Michael Roth wrote:
> > > > On Thu, Nov 20, 2025 at 05:11:48PM +0800, Yan Zhao wrote:
> > > > > On Thu, Nov 13, 2025 at 05:07:59PM -0600, Michael Roth wrote:
> > > > > > Currently the post-populate callbacks handle copying source pages into
> > > > > > private GPA ranges backed by guest_memfd, where kvm_gmem_populate()
> > > > > > acquires the filemap invalidate lock, then calls a post-populate
> > > > > > callback which may issue a get_user_pages() on the source pages prior to
> > > > > > copying them into the private GPA (e.g. TDX).
> > > > > >
> > > > > > This will not be compatible with in-place conversion, where the
> > > > > > userspace page fault path will attempt to acquire filemap invalidate
> > > > > > lock while holding the mm->mmap_lock, leading to a potential ABBA
> > > > > > deadlock[1].
> > > > > >
> > > > > > Address this by hoisting the GUP above the filemap invalidate lock so
> > > > > > that these page faults path can be taken early, prior to acquiring the
> > > > > > filemap invalidate lock.
> > > > > >
> > > > > > It's not currently clear whether this issue is reachable with the
> > > > > > current implementation of guest_memfd, which doesn't support in-place
> > > > > > conversion, however it does provide a consistent mechanism to provide
> > > > > > stable source/target PFNs to callbacks rather than punting to
> > > > > > vendor-specific code, which allows for more commonality across
> > > > > > architectures, which may be worthwhile even without in-place conversion.
> > > > > >
> > > > > > Suggested-by: Sean Christopherson <seanjc@google.com>
> > > > > > Signed-off-by: Michael Roth <michael.roth@amd.com>
> > > > > > ---
> > > > > > arch/x86/kvm/svm/sev.c | 40 ++++++++++++++++++++++++++------------
> > > > > > arch/x86/kvm/vmx/tdx.c | 21 +++++---------------
> > > > > > include/linux/kvm_host.h | 3 ++-
> > > > > > virt/kvm/guest_memfd.c | 42 ++++++++++++++++++++++++++++++++++------
> > > > > > 4 files changed, 71 insertions(+), 35 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > > > > > index 0835c664fbfd..d0ac710697a2 100644
> > > > > > --- a/arch/x86/kvm/svm/sev.c
> > > > > > +++ b/arch/x86/kvm/svm/sev.c
> > > > > > @@ -2260,7 +2260,8 @@ struct sev_gmem_populate_args {
> > > > > > };
> > > > > >
> > > > > > static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn_start, kvm_pfn_t pfn,
> > > > > > - void __user *src, int order, void *opaque)
> > > > > > + struct page **src_pages, loff_t src_offset,
> > > > > > + int order, void *opaque)
> > > > > > {
> > > > > > struct sev_gmem_populate_args *sev_populate_args = opaque;
> > > > > > struct kvm_sev_info *sev = to_kvm_sev_info(kvm);
> > > > > > @@ -2268,7 +2269,7 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn_start, kvm_pfn_t pf
> > > > > > int npages = (1 << order);
> > > > > > gfn_t gfn;
> > > > > >
> > > > > > - if (WARN_ON_ONCE(sev_populate_args->type != KVM_SEV_SNP_PAGE_TYPE_ZERO && !src))
> > > > > > + if (WARN_ON_ONCE(sev_populate_args->type != KVM_SEV_SNP_PAGE_TYPE_ZERO && !src_pages))
> > > > > > return -EINVAL;
> > > > > >
> > > > > > for (gfn = gfn_start, i = 0; gfn < gfn_start + npages; gfn++, i++) {
> > > > > > @@ -2284,14 +2285,21 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn_start, kvm_pfn_t pf
> > > > > > goto err;
> > > > > > }
> > > > > >
> > > > > > - if (src) {
> > > > > > - void *vaddr = kmap_local_pfn(pfn + i);
> > > > > > + if (src_pages) {
> > > > > > + void *src_vaddr = kmap_local_pfn(page_to_pfn(src_pages[i]));
> > > > > > + void *dst_vaddr = kmap_local_pfn(pfn + i);
> > > > > >
> > > > > > - if (copy_from_user(vaddr, src + i * PAGE_SIZE, PAGE_SIZE)) {
> > > > > > - ret = -EFAULT;
> > > > > > - goto err;
> > > > > > + memcpy(dst_vaddr, src_vaddr + src_offset, PAGE_SIZE - src_offset);
> > > > > > + kunmap_local(src_vaddr);
> > > > > > +
> > > > > > + if (src_offset) {
> > > > > > + src_vaddr = kmap_local_pfn(page_to_pfn(src_pages[i + 1]));
> > > > > > +
> > > > > > + memcpy(dst_vaddr + PAGE_SIZE - src_offset, src_vaddr, src_offset);
> > > > > > + kunmap_local(src_vaddr);
> > > > > IIUC, src_offset is the src's offset from the first page. e.g.,
> > > > > src could be 0x7fea82684100, with src_offset=0x100, while npages could be 512.
> > > > >
> > > > > Then it looks like the two memcpy() calls here only work when npages == 1 ?
> > > >
> > > > src_offset ends up being the offset into the pair of src pages that we
> > > > are using to fully populate a single dest page with each iteration. So
> > > > if we start at src_offset, read a page worth of data, then we are now at
> > > > src_offset in the next src page and the loop continues that way even if
> > > > npages > 1.
> > > >
> > > > If src_offset is 0 we never have to bother with straddling 2 src pages so
> > > > the 2nd memcpy is skipped on every iteration.
> > > >
> > > > That's the intent at least. Is there a flaw in the code/reasoning that I
> > > > missed?
> > > Oh, I got you. SNP expects a single src_offset applies for each src page.
> > >
> > > So if npages = 2, there're 4 memcpy() calls.
> > >
> > > src: |---------|---------|---------| (VA contiguous)
> > > ^ ^ ^
> > > | | |
> > > dst: |---------|---------| (PA contiguous)
> > >
> > >
> > > I previously incorrectly thought kvm_gmem_populate() should pass in src_offset
> > > as 0 for the 2nd src page.
> > >
> > > Would you consider checking if params.uaddr is PAGE_ALIGNED() in
> > > snp_launch_update() to simplify the design?
> >
> > This was an option mentioned in the cover letter and during PUCK. I am
> > not opposed if that's the direction we decide, but I also don't think
> > it makes big difference since:
> >
> > int (*kvm_gmem_populate_cb)(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
> > struct page **src_pages, loff_t src_offset,
> > int order, void *opaque);
> >
> > basically reduces to Sean's originally proposed:
> >
> > int (*kvm_gmem_populate_cb)(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
> > struct page *src_pages, int order,
> > void *opaque);
>
> Hmm, the requirement of having each copy to dst_page account for src_offset
> (which actually results in 2 copies) is quite confusing. I initially thought the
> src_offset only applied to the first dst_page.
What I'm wondering though is if I'd done a better job of documenting
this aspect, e.g. with the following comment added above
kvm_gmem_populate_cb:
/*
* ...
* 'src_pages': array of GUP'd struct page pointers corresponding to
* the pages that store the data that is to be copied
* into the HPA corresponding to 'pfn'
* 'src_offset': byte offset, relative to the first page in the array
* of pages pointed to by 'src_pages', to begin copying
* the data from.
*
* NOTE: if the caller of kvm_gmem_populate() enforces that 'src' is
* page-aligned, then 'src_offset' will always be zero, and src_pages
* will contain only 1 page to copy from, beginning at byte offset 0.
* In this case, callers can assume src_offset is 0.
*/
int (*kvm_gmem_populate_cb)(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
struct page **src_pages, loff_t src_offset,
int order, void *opaque);
could the confusion have been avoided, or is it still unwieldly?
I don't mind that users like SNP need to deal with the extra bits, but
I'm hoping for users like TDX it isn't too cludgy.
>
> This will also cause kvm_gmem_populate() to allocate 1 more src_npages than
> npages for dst pages.
That's more of a decision on the part of userspace deciding to use
non-page-aligned 'src' pointer to begin with.
>
> > for any platform that enforces that the src is page-aligned, which
> > doesn't seem like a huge technical burden, IMO, despite me initially
> > thinking it would be gross when I brought this up during the PUCK call
> > that preceeding this posting.
> > >
> > > > >
> > > > > > }
> > > > > > - kunmap_local(vaddr);
> > > > > > +
> > > > > > + kunmap_local(dst_vaddr);
> > > > > > }
> > > > > >
> > > > > > ret = rmp_make_private(pfn + i, gfn << PAGE_SHIFT, PG_LEVEL_4K,
> > > > > > @@ -2331,12 +2339,20 @@ static int sev_gmem_post_populate(struct kvm *kvm, gfn_t gfn_start, kvm_pfn_t pf
> > > > > > if (!snp_page_reclaim(kvm, pfn + i) &&
> > > > > > sev_populate_args->type == KVM_SEV_SNP_PAGE_TYPE_CPUID &&
> > > > > > sev_populate_args->fw_error == SEV_RET_INVALID_PARAM) {
> > > > > > - void *vaddr = kmap_local_pfn(pfn + i);
> > > > > > + void *src_vaddr = kmap_local_pfn(page_to_pfn(src_pages[i]));
> > > > > > + void *dst_vaddr = kmap_local_pfn(pfn + i);
> > > > > >
> > > > > > - if (copy_to_user(src + i * PAGE_SIZE, vaddr, PAGE_SIZE))
> > > > > > - pr_debug("Failed to write CPUID page back to userspace\n");
> > > > > > + memcpy(src_vaddr + src_offset, dst_vaddr, PAGE_SIZE - src_offset);
> > > > > > + kunmap_local(src_vaddr);
> > > > > > +
> > > > > > + if (src_offset) {
> > > > > > + src_vaddr = kmap_local_pfn(page_to_pfn(src_pages[i + 1]));
> > > > > > +
> > > > > > + memcpy(src_vaddr, dst_vaddr + PAGE_SIZE - src_offset, src_offset);
> > > > > > + kunmap_local(src_vaddr);
> > > > > > + }
> > > > > >
> > > > > > - kunmap_local(vaddr);
> > > > > > + kunmap_local(dst_vaddr);
> > > > > > }
> > > > > >
> > > > > > /* pfn + i is hypervisor-owned now, so skip below cleanup for it. */
> > > > > > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > > > > > index 57ed101a1181..dd5439ec1473 100644
> > > > > > --- a/arch/x86/kvm/vmx/tdx.c
> > > > > > +++ b/arch/x86/kvm/vmx/tdx.c
> > > > > > @@ -3115,37 +3115,26 @@ struct tdx_gmem_post_populate_arg {
> > > > > > };
> > > > > >
> > > > > > static int tdx_gmem_post_populate(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
> > > > > > - void __user *src, int order, void *_arg)
> > > > > > + struct page **src_pages, loff_t src_offset,
> > > > > > + int order, void *_arg)
> > > > > > {
> > > > > > struct tdx_gmem_post_populate_arg *arg = _arg;
> > > > > > struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> > > > > > u64 err, entry, level_state;
> > > > > > gpa_t gpa = gfn_to_gpa(gfn);
> > > > > > - struct page *src_page;
> > > > > > int ret, i;
> > > > > >
> > > > > > if (KVM_BUG_ON(kvm_tdx->page_add_src, kvm))
> > > > > > return -EIO;
> > > > > >
> > > > > > - if (KVM_BUG_ON(!PAGE_ALIGNED(src), kvm))
> > > > > > + /* Source should be page-aligned, in which case src_offset will be 0. */
> > > > > > + if (KVM_BUG_ON(src_offset))
> > > > > if (KVM_BUG_ON(src_offset, kvm))
> > > > >
> > > > > > return -EINVAL;
> > > > > >
> > > > > > - /*
> > > > > > - * Get the source page if it has been faulted in. Return failure if the
> > > > > > - * source page has been swapped out or unmapped in primary memory.
> > > > > > - */
> > > > > > - ret = get_user_pages_fast((unsigned long)src, 1, 0, &src_page);
> > > > > > - if (ret < 0)
> > > > > > - return ret;
> > > > > > - if (ret != 1)
> > > > > > - return -ENOMEM;
> > > > > > -
> > > > > > - kvm_tdx->page_add_src = src_page;
> > > > > > + kvm_tdx->page_add_src = src_pages[i];
> > > > > src_pages[0] ? i is not initialized.
> > > >
> > > > Sorry, I switched on TDX options for compile testing but I must have done a
> > > > sloppy job confirming it actually built. I'll re-test push these and squash
> > > > in the fixes in the github tree.
> > > >
> > > > >
> > > > > Should there also be a KVM_BUG_ON(order > 0, kvm) ?
> > > >
> > > > Seems reasonable, but I'm not sure this is the right patch. Maybe I
> > > > could squash it into the preceeding documentation patch so as to not
> > > > give the impression this patch changes those expectations in any way.
> > > I don't think it should be documented as a user requirement.
> >
> > I didn't necessarily mean in the documentation, but mainly some patch
> > other than this. If we add that check here as part of this patch, we
> > give the impression that the order expectations are changing as a result
> > of the changes here, when in reality they are exactly the same as
> > before.
> >
> > If not the documentation patch here, then I don't think it really fits
> > in this series at all and would be more of a standalone patch against
> > kvm/next.
> >
> > The change here:
> >
> > - if (KVM_BUG_ON(!PAGE_ALIGNED(src), kvm))
> > + /* Source should be page-aligned, in which case src_offset will be 0. */
> > + if (KVM_BUG_ON(src_offset))
> >
> > made sense as part of this patch, because now that we are passing struct
> > page *src_pages, we can no longer infer alignment from 'src' field, and
> > instead need to infer it from src_offset being 0.
> >
> > >
> > > However, we need to comment out that this assertion is due to that
> > > tdx_vcpu_init_mem_region() passes npages as 1 to kvm_gmem_populate().
> >
> > You mean for the KVM_BUG_ON(order > 0, kvm) you're proposing to add?
> > Again, if feels awkward to address this as part of this series since it
> > is an existing/unchanged behavior and not really the intent of this
> > patchset.
> That's true. src_pages[0] just makes it more eye-catching.
> What about just adding a comment for src_pages[0] instead of KVM_BUG_ON()?
That seems fair/relevant for this series.
>
> > > > > > ret = kvm_tdp_mmu_map_private_pfn(arg->vcpu, gfn, pfn);
> > > > > > kvm_tdx->page_add_src = NULL;
> > > > > >
> > > > > > - put_page(src_page);
> > > > > > -
> > > > > > if (ret || !(arg->flags & KVM_TDX_MEASURE_MEMORY_REGION))
> > > > > > return ret;
> > > > > >
> > > > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > > > > > index d93f75b05ae2..7e9d2403c61f 100644
> > > > > > --- a/include/linux/kvm_host.h
> > > > > > +++ b/include/linux/kvm_host.h
> > > > > > @@ -2581,7 +2581,8 @@ int kvm_arch_gmem_prepare(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn, int max_ord
> > > > > > * Returns the number of pages that were populated.
> > > > > > */
> > > > > > typedef int (*kvm_gmem_populate_cb)(struct kvm *kvm, gfn_t gfn, kvm_pfn_t pfn,
> > > > > > - void __user *src, int order, void *opaque);
> > > > > > + struct page **src_pages, loff_t src_offset,
> > > > > > + int order, void *opaque);
> > > > > >
> > > > > > long kvm_gmem_populate(struct kvm *kvm, gfn_t gfn, void __user *src, long npages,
> > > > > > kvm_gmem_populate_cb post_populate, void *opaque);
> > > > > > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > > > > > index 9160379df378..e9ac3fd4fd8f 100644
> > > > > > --- a/virt/kvm/guest_memfd.c
> > > > > > +++ b/virt/kvm/guest_memfd.c
> > > > > > @@ -814,14 +814,17 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
> > > > > > EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_gmem_get_pfn);
> > > > > >
> > > > > > #ifdef CONFIG_HAVE_KVM_ARCH_GMEM_POPULATE
> > > > > > +
> > > > > > +#define GMEM_GUP_NPAGES (1UL << PMD_ORDER)
> > > > > Limiting GMEM_GUP_NPAGES to PMD_ORDER may only work when the max_order of a huge
> > > > > folio is 2MB. What if the max_order returned from __kvm_gmem_get_pfn() is 1GB
> > > > > when src_pages[] can only hold up to 512 pages?
> > > >
> > > > This was necessarily chosen in prep for hugepages, but more about my
> > > > unease at letting userspace GUP arbitrarilly large ranges. PMD_ORDER
> > > > happens to align with 2MB hugepages while seeming like a reasonable
> > > > batching value so that's why I chose it.
> > > >
> > > > Even with 1GB support, I wasn't really planning to increase it. SNP
> > > > doesn't really make use of RMP sizes >2MB, and it sounds like TDX
> > > > handles promotion in a completely different path. So atm I'm leaning
> > > > toward just letting GMEM_GUP_NPAGES be the cap for the max page size we
> > > > support for kvm_gmem_populate() path and not bothering to change it
> > > > until a solid use-case arises.
> > > The problem is that with hugetlb-based guest_memfd, the folio itself could be
> > > of 1GB, though SNP and TDX can force mapping at only 4KB.
> >
> > If TDX wants to unload handling of page-clearing to its per-page
> > post-populate callback and tie that its shared/private tracking that's
> > perfectly fine by me.
> >
> > *How* TDX tells gmem it wants this different behavior is a topic for a
> > follow-up patchset, Vishal suggested kernel-internal flags to
> > kvm_gmem_create(), which seemed reasonable to me. In that case, uptodate
> Not sure which flag you are referring to with "Vishal suggested kernel-internal
> flags to kvm_gmem_create()".
>
> However, my point is that when the backend folio is 1GB in size (leading to
> max_order being PUD_ORDER), even if SNP only maps at 2MB to RMP, it may hit the
> warning of "!IS_ALIGNED(gfn, 1 << max_order)".
I think I've had to remove that warning every time I start working on
some new spin of THP/hugetlbfs-based SNP. I'm not objecting to that. But it's
obvious there, in those contexts, and I can explain exactly why it's being
removed.
It's not obvious in this series, where all we have are hand-wavy thoughts
about what hugepages will look like. For all we know we might decide that
kvm_gmem_populate() path should just pre-split hugepages to make all the
logic easier, or we decide to lock it in at 4K-only and just strip all the
hugepage stuff out. I don't really know, and this doesn't seem like the place
to try to hash all that out when nothing in this series will cause this
existing WARN_ON to be tripped.
>
> For TDX, it's worse because it always passes npages as 1, so it will also hit
> the warning of "(npages - i) < (1 << max_order)".
>
> Given that this patch already considers huge pages for SNP, it feels half-baked
> to leave the WARN_ON() for future handling.
> WARN_ON(!IS_ALIGNED(gfn, 1 << max_order) ||
> (npages - i) < (1 << max_order));
>
> > flag would probably just default to set and punt to post-populate/prep
> > hooks, because we absolutely *do not* want to have to re-introduce per-4K
> > tracking of this type of state within gmem, since getting rid of that sort
> > of tracking requirement within gmem is the entire motivation of this
> > series. And since, within this series, the uptodate flag and
> > prep-tracking both have the same 4K granularity, it seems unecessary to
> > address this here.
> >
> > If you were to send a patchset on top of this (or even independently) that
> > introduces said kernel-internal gmem flag to offload uptodate-tracking to
> > post-populate/prep hooks, and utilize it to optimize the current 4K-only
> > TDX implementation by letting TDX module handle the initial
> > page-clearing, then I think that change/discussion can progress without
> > being blocked in any major way by this series.
> >
> > But I don't think we need to flesh all that out here, so long as we are
> > aware of this as a future change/requirement and have reasonable
> > indication that it is compatible with this series.
> >
> > >
> > > Then since max_order = folio_order(folio) (at least in the tree for [1]),
> > > WARN_ON() in kvm_gmem_populate() could still be hit:
> > >
> > > folio = __kvm_gmem_get_pfn(file, slot, index, &pfn, &max_order);
> > > WARN_ON(!IS_ALIGNED(gfn, 1 << max_order) ||
> > > (npages - i) < (1 << max_order));
> >
> > Yes, in the SNP implementation of hugetlb I ended up removing this
> > warning, and in that case I also ended up forcing kvm_gmem_populate() to
> > be 4K-only:
> >
> > https://github.com/AMDESE/linux/blob/snp-hugetlb-v2-wip0/virt/kvm/guest_memfd.c#L2372
>
> For 1G (aka HugeTLB) page, this fix is also needed, which was missed in [1] and
> I pointed out to Ackerley at [2].
>
> [1] https://github.com/googleprodkernel/linux-cc/tree/gmem-1g-page-support-rfc-v2
> [2] https://lore.kernel.org/all/aFPGPVbzo92t565h@yzhao56-desk.sh.intel.com/
Yes, we'll likely need some kind of change here.
I think, if we're trying to find common ground to build hugepage support
on, you can assume this will be removed. But I just don't think we need
to squash that into this series in order to make progress on those ends.
>
> > but it makes a lot more sense to make those restrictions and changes in
> > the context of hugepage support, rather than this series which is trying
> > very hard to not do hugepage enablement, but simply keep what's partially
> > there intact while reworking other things that have proven to be
> > continued impediments to both in-place conversion and hugepage
> > enablement.
> Not sure how fixing the warning in this series could impede hugepage enabling :)
>
> But if you prefer, I don't mind keeping it locally for longer.
It's the whole burden of needing to anticipate hugepage design, while it
is in a state of potentially massive flux just before LPC, in order to
make tiny incremental progress toward enabling in-place conversion,
which is something I think we can get upstream much sooner. Look at your
changelog for the change above, for instance: it has no relevance in the
context of this series. What do I put in its place? Bug reports about
my experimental tree? It's just not the right place to try to justify
these changes.
And most of this weirdness stems from the fact that we prematurely added
partial hugepage enablement to begin with. Let's not repeat these mistakes,
and address changes in the proper context where we know they make sense.
I considered stripping out the existing hugepage support as a pre-patch
to avoid leaving these uncertainties in place while we are reworking
things, but it felt like needless churn. But that's where I'm coming
from with this series: let's get in-place conversion landed, get the API
fleshed out, get it working, and then re-assess hugepages with all these
common/intersecting bits out of the way. If we can remove some obstacles
for hugepages as part of that, great, but that is not the main intent
here.
-Mike
>
> > Also, there's talk now of enabling hugepages even without in-place
> > conversion for hugetlbfs, and that will likely be the same path we
> > follow for THP to remain in alignment. Rather than anticipating what all
> > these changes will mean WRT hugepage implementation/requirements, I
> > think it will be fruitful to remove some of the baggage that will
> > complicate that process/discussion like this patchset attempts.
> >
> > -Mike
> >
> > >
> > > TDX is even easier to hit this warning because it always passes npages as 1.
> > >
> > > [1] https://lore.kernel.org/all/cover.1747264138.git.ackerleytng@google.com
> > >
> > >
> > > > > Increasing GMEM_GUP_NPAGES to (1UL << PUD_ORDER) is probabaly not a good idea.
> > > > >
> > > > > Given both TDX/SNP map at 4KB granularity, why not just invoke post_populate()
> > > > > per 4KB while removing the max_order from post_populate() parameters, as done
> > > > > in Sean's sketch patch [1]?
> > > >
> > > > That's an option too, but SNP can make use of 2MB pages in the
> > > > post-populate callback so I don't want to shut the door on that option
> > > > just yet if it's not too much of a pain to work in. Given the guest BIOS
> > > > lives primarily in 1 or 2 of these 2MB regions the benefits might be
> > > > worthwhile, and SNP doesn't have a post-post-populate promotion path
> > > > like TDX (at least, not one that would help much for guest boot times)
> > > I see.
> > >
> > > So, what about below change?
> > >
> > > --- a/virt/kvm/guest_memfd.c
> > > +++ b/virt/kvm/guest_memfd.c
> > > @@ -878,11 +878,10 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
> > > }
> > >
> > > folio_unlock(folio);
> > > - WARN_ON(!IS_ALIGNED(gfn, 1 << max_order) ||
> > > - (npages - i) < (1 << max_order));
> > >
> > > ret = -EINVAL;
> > > - while (!kvm_range_has_memory_attributes(kvm, gfn, gfn + (1 << max_order),
> > > + while (!IS_ALIGNED(gfn, 1 << max_order) || (npages - i) < (1 << max_order) ||
> > > + !kvm_range_has_memory_attributes(kvm, gfn, gfn + (1 << max_order),
> > > KVM_MEMORY_ATTRIBUTE_PRIVATE,
> > > KVM_MEMORY_ATTRIBUTE_PRIVATE)) {
> > > if (!max_order)
> > >
> > >
> > >
> > > >
> > > > >
> > > > > Then the WARN_ON() in kvm_gmem_populate() can be removed, which would be easily
> > > > > triggered by TDX when max_order > 0 && npages == 1:
> > > > >
> > > > > WARN_ON(!IS_ALIGNED(gfn, 1 << max_order) ||
> > > > > (npages - i) < (1 << max_order));
> > > > >
> > > > >
> > > > > [1] https://lore.kernel.org/all/aHEwT4X0RcfZzHlt@google.com/
> > > > >
> > > > > > long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long npages,
> > > > > > kvm_gmem_populate_cb post_populate, void *opaque)
> > > > > > {
> > > > > > struct kvm_memory_slot *slot;
> > > > > > - void __user *p;
> > > > > > -
> > > > > > + struct page **src_pages;
> > > > > > int ret = 0, max_order;
> > > > > > - long i;
> > > > > > + loff_t src_offset = 0;
> > > > > > + long i, src_npages;
> > > > > >
> > > > > > lockdep_assert_held(&kvm->slots_lock);
> > > > > >
> > > > > > @@ -836,9 +839,28 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
> > > > > > if (!file)
> > > > > > return -EFAULT;
> > > > > >
> > > > > > + npages = min_t(ulong, slot->npages - (start_gfn - slot->base_gfn), npages);
> > > > > > + npages = min_t(ulong, npages, GMEM_GUP_NPAGES);
> > > > > > +
> > > > > > + if (src) {
> > > > > > + src_npages = IS_ALIGNED((unsigned long)src, PAGE_SIZE) ? npages : npages + 1;
> > > > > > +
> > > > > > + src_pages = kmalloc_array(src_npages, sizeof(struct page *), GFP_KERNEL);
> > > > > > + if (!src_pages)
> > > > > > + return -ENOMEM;
> > > > > > +
> > > > > > + ret = get_user_pages_fast((unsigned long)src, src_npages, 0, src_pages);
> > > > > > + if (ret < 0)
> > > > > > + return ret;
> > > > > > +
> > > > > > + if (ret != src_npages)
> > > > > > + return -ENOMEM;
> > > > > > +
> > > > > > + src_offset = (loff_t)(src - PTR_ALIGN_DOWN(src, PAGE_SIZE));
> > > > > > + }
> > > > > > +
> > > > > > filemap_invalidate_lock(file->f_mapping);
> > > > > >
> > > > > > - npages = min_t(ulong, slot->npages - (start_gfn - slot->base_gfn), npages);
> > > > > > for (i = 0; i < npages; i += (1 << max_order)) {
> > > > > > struct folio *folio;
> > > > > > gfn_t gfn = start_gfn + i;
> > > > > > @@ -869,8 +891,8 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
> > > > > > max_order--;
> > > > > > }
> > > > > >
> > > > > > - p = src ? src + i * PAGE_SIZE : NULL;
> > > > > > - ret = post_populate(kvm, gfn, pfn, p, max_order, opaque);
> > > > > > + ret = post_populate(kvm, gfn, pfn, src ? &src_pages[i] : NULL,
> > > > > > + src_offset, max_order, opaque);
> > > > > Why src_offset is not 0 starting from the 2nd page?
> > > > >
> > > > > > if (!ret)
> > > > > > folio_mark_uptodate(folio);
> > > > > >
> > > > > > @@ -882,6 +904,14 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
> > > > > >
> > > > > > filemap_invalidate_unlock(file->f_mapping);
> > > > > >
> > > > > > + if (src) {
> > > > > > + long j;
> > > > > > +
> > > > > > + for (j = 0; j < src_npages; j++)
> > > > > > + put_page(src_pages[j]);
> > > > > > + kfree(src_pages);
> > > > > > + }
> > > > > > +
> > > > > > return ret && !i ? ret : i;
> > > > > > }
> > > > > > EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_gmem_populate);
> > > > > > --
> > > > > > 2.25.1
> > > > > >
^ permalink raw reply
* Re: [PATCH 1/3] KVM: guest_memfd: Remove preparation tracking
From: Michael Roth @ 2025-12-03 13:47 UTC (permalink / raw)
To: Yan Zhao
Cc: kvm, linux-coco, linux-mm, linux-kernel, thomas.lendacky,
pbonzini, seanjc, vbabka, ashish.kalra, liam.merwick, david,
vannapurve, ackerleytng, aik, ira.weiny
In-Reply-To: <aS6uoFyqF+SdGWlI@yzhao56-desk.sh.intel.com>
On Tue, Dec 02, 2025 at 05:17:20PM +0800, Yan Zhao wrote:
> On Mon, Dec 01, 2025 at 05:44:47PM -0600, Michael Roth wrote:
> > On Tue, Nov 25, 2025 at 11:13:25AM +0800, Yan Zhao wrote:
> > > On Fri, Nov 21, 2025 at 06:43:14AM -0600, Michael Roth wrote:
> > > > On Thu, Nov 20, 2025 at 05:12:55PM +0800, Yan Zhao wrote:
> > > > > On Thu, Nov 13, 2025 at 05:07:57PM -0600, Michael Roth wrote:
> > > > > > @@ -797,19 +782,25 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
> > > > > > {
> > > > > > pgoff_t index = kvm_gmem_get_index(slot, gfn);
> > > > > > struct folio *folio;
> > > > > > - bool is_prepared = false;
> > > > > > int r = 0;
> > > > > >
> > > > > > CLASS(gmem_get_file, file)(slot);
> > > > > > if (!file)
> > > > > > return -EFAULT;
> > > > > >
> > > > > > - folio = __kvm_gmem_get_pfn(file, slot, index, pfn, &is_prepared, max_order);
> > > > > > + folio = __kvm_gmem_get_pfn(file, slot, index, pfn, max_order);
> > > > > > if (IS_ERR(folio))
> > > > > > return PTR_ERR(folio);
> > > > > >
> > > > > > - if (!is_prepared)
> > > > > > - r = kvm_gmem_prepare_folio(kvm, slot, gfn, folio);
> > > > > > + if (!folio_test_uptodate(folio)) {
> > > > > > + unsigned long i, nr_pages = folio_nr_pages(folio);
> > > > > > +
> > > > > > + for (i = 0; i < nr_pages; i++)
> > > > > > + clear_highpage(folio_page(folio, i));
> > > > > > + folio_mark_uptodate(folio);
> > > > > Here, the entire folio is cleared only when the folio is not marked uptodate.
> > > > > Then, please check my questions at the bottom
> > > >
> > > > Yes, in this patch at least where I tried to mirror the current logic. I
> > > > would not be surprised if we need to rework things for inplace/hugepage
> > > > support though, but decoupling 'preparation' from the uptodate flag is
> > > > the main goal here.
> > > Could you elaborate a little why the decoupling is needed if it's not for
> > > hugepage?
> >
> > For instance, for in-place conversion:
> >
> > 1. initial allocation: clear, set uptodate, fault in as private
> > 2. private->shared: call invalidate hook, fault in as shared
> > 3. shared->private: call prep hook, fault in as private
> >
> > Here, 2/3 need to track where the current state is shared/private in
> > order to make appropriate architecture-specific changes (e.g. RMP table
> > updates). But we want to allow for non-destructive in-place conversion,
> > where a page is 'uptodate', but not in the desired shared/private state.
> > So 'uptodate' becomes a separate piece of state, which is still
> > reasonable for gmem to track in the current 4K-only implementation, and
> > provides for a reasonable approach to upstreaming in-place conversion,
> > which isn't far off for either SNP or TDX.
> To me, "1. initial allocation: clear, set uptodate" is more appropriate to
> be done in kvm_gmem_get_folio(), instead of in kvm_gmem_get_pfn().
The downside is that preallocating originally involved just
preallocating, and zero'ing would happen lazily during fault time. (or
upfront via KVM_PRE_FAULT_MEMORY).
But in the context of the hugetlb RFC, it certainly looks cleaner to do
it there, since it could be done before any potential splitting activity,
so then all the tail pages can inherit that initial uptodate flag.
We'd probably want to weigh that these trade-offs based on how it will
affect hugepages, but that would be clearer in the context of a new
posting of hugepage support on top of these changes. So I think it's
better to address that change as a follow-up so we can consider it with
more context.
>
> With it, below looks reasonable to me.
> > For hugepages, we'll have other things to consider, but those things are
> > probably still somewhat far off, and so we shouldn't block steps toward
> > in-place conversion based on uncertainty around hugepages. I think it's
> > gotten enough attention at least that we know it *can* work, e.g. even
> > if we take the inefficient/easy route of zero'ing the whole folio on
> > initial access, setting it uptodate, and never doing anything with
> > uptodate again, it's still a usable implementation.
To me as well. But in the context of this series, against kvm/next, it
creates userspace-visible changes to pre-allocation behavior that we
can't justify in the context of this series alone, so as above I think
that's something to save for hugepage follow-up.
FWIW though, I ended up taking this approach for the hugetlb-based test
branch, to address the fact (after you reminded me) that I wasn't fully
zero'ing folio's in the kvm_gmem_populate() path:
https://github.com/AMDESE/linux/commit/253fb30b076d81232deba0b02db070d5bc2b90b5
So maybe for your testing you could do similar. For newer hugepage
support I'll likely do similar, but I don't think any of this reasoning
or changes makes sense to people reviewing this series without already
being aware of hugepage plans/development, so that's why I'm trying to
keep this series more focused on in-place conversion enablement, because
hugepage plans might be massively reworked for next posting based on LPC
talks and changes in direction mentioned in recent guest_memfd calls and
we are basically just hand-waving about what it will look like at this
point while blocking other efforts.
-Mike
>
> <...>
> > > > Assuming this patch goes upstream in some form, we will now have the
> > > > following major differences versus previous code:
> > > >
> > > > 1) uptodate flag only tracks whether a folio has been cleared
> > > > 2) gmem always calls kvm_arch_gmem_prepare() via kvm_gmem_get_pfn() and
> > > > the architecture can handle it's own tracking at whatever granularity
> > > > it likes.
> > > 2) looks good to me.
> > >
> > > > My hope is that 1) can similarly be done in such a way that gmem does not
> > > > need to track things at sub-hugepage granularity and necessitate the need
> > > > for some new data structure/state/flag to track sub-page status.
> > > I actually don't understand what uptodate flag helps gmem to track.
> > > Why can't clear_highpage() be done inside arch specific code? TDX doesn't need
> > > this clearing after all.
> >
> > It could. E.g. via the kernel-internal gmem flag that I mentioned in my
> > earlier reply, or some alternative.
> >
> > In the context of this series, uptodate flag continues to instruct
> > kvm_gmem_get_pfn() that it doesn't not need to re-clear pages, because
> > a prior kvm_gmem_get_pfn() or kvm_gmem_populate() already initialized
> > the folio, and it is no longer tied to any notion of
> > preparedness-tracking.
> >
> > What use uptodate will have in the context of hugepages: I'm not sure.
> > For non-in-place conversion, it's tempting to just let it continue to be
> > per-folio and require clearing the whole folio on initial access, but
> > it's not efficient. It may make sense to farm it out to
> > post-populate/prep hooks instead, as you're suggesting for TDX.
> >
> > But then, for in-place conversion, you have to deal with pages initially
> > faulted in as shared. They might be split prior to initial access as a
> > private page, where we can't assume TDX will have scrubbed things. So in
> > that case it might still make sense to rely on it.
> >
> > Definitely things that require some more thought. But having it inextricably
> > tied to preparedness just makes preparation tracking similarly more
> > complicated as it pulls it back into gmem when that does not seem to be
> > the direction any architectures other SNP have/want to go.
> >
> > >
> > > > My understanding based on prior discussion in guest_memfd calls was that
> > > > it would be okay to go ahead and clear the entire folio at initial allocation
> > > > time, and basically never mess with it again. It was also my understanding
> > > That's where I don't follow in this patch.
> > > I don't see where the entire folio A is cleared if it's only partially mapped by
> > > kvm_gmem_populate(). kvm_gmem_get_pfn() won't clear folio A either due to
> > > kvm_gmem_populate() has set the uptodate flag.
> > >
> > > > that for TDX it might even be optimal to completely skip clearing the folio
> > > > if it is getting mapped into SecureEPT as a hugepage since the TDX module
> > > > would handle that, but that maybe conversely after private->shared there
> > > > would be some need to reclear... I'll try to find that discussion and
> > > > refresh. Vishal I believe suggested some flags to provide more control over
> > > > this behavior.
> > > >
> > > > >
> > > > > It's possible (at least for TDX) that a huge folio is only partially populated
> > > > > by kvm_gmem_populate(). Then kvm_gmem_get_pfn() faults in another part of the
> > > > > huge folio. For example, in TDX, GFN 0x81f belongs to the init memory region,
> > > > > while GFN 0x820 is faulted after TD is running. However, these two GFNs can
> > > > > belong to the same folio of order 9.
> > > >
> > > > Would the above scheme of clearing the entire folio up front and not re-clearing
> > > > at fault time work for this case?
> > > This case doesn't affect TDX, because TDX clearing private pages internally in
> > > SEAM APIs. So, as long as kvm_gmem_get_pfn() does not invoke clear_highpage()
> > > after making a folio private, it works fine for TDX.
> > >
> > > I was just trying to understand why SNP needs the clearing of entire folio in
> > > kvm_gmem_get_pfn() while I don't see how the entire folio is cleared when it's
> > > partially mapped in kvm_gmem_populate().
> > > Also, I'm wondering if it would be better if SNP could move the clearing of
> > > folio into something like kvm_arch_gmem_clear(), just as kvm_arch_gmem_prepare()
> > > which is always invoked by kvm_gmem_get_pfn() and the architecture can handle
> > > it's own tracking at whatever granularity.
> >
> > Possibly, but I touched elsewhere on where in-place conversion might
> > trip up this approach. At least decoupling them allows for the prep side
> > of things to be moved to architecture-specific tracking. We can deal
> > with uptodate separately I think.
> >
> > -Mike
> >
> > >
> > >
> > > > > Note: the current code should not impact TDX. I'm just asking out of curiosity:)
> > > > >
> > > > > [1] https://lore.kernel.org/all/aQ3uj4BZL6uFQzrD@yzhao56-desk.sh.intel.com/
> > > > >
> > > > >
>
^ permalink raw reply
* Re: [PATCH v4 07/16] x86/virt/tdx: Add tdx_alloc/free_page() helpers
From: Nikolay Borisov @ 2025-12-03 13:48 UTC (permalink / raw)
To: Kiryl Shutsemau, 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, pbonzini@redhat.com,
tglx@linutronix.de, Yamahata, Isaku, linux-kernel@vger.kernel.org,
Annapurve, Vishal, Gao, Chao, bp@alien8.de, x86@kernel.org
In-Reply-To: <7xbqq2uplwkc36q6jyorxe6u3fboka3snwar6parado5ysz25o@qrstyzh3okgh>
On 3.12.25 г. 15:46 ч., Kiryl Shutsemau wrote:
> On Tue, Dec 02, 2025 at 08:02:38PM +0000, Edgecombe, Rick P wrote:
>> On Tue, 2025-12-02 at 09:38 +0200, Nikolay Borisov wrote:
>>>> Yea, it could be simpler if it was always guaranteed to be 2 pages. But it
>>>> was
>>>> my understanding that it would not be a fixed size. Can you point to what
>>>> docs
>>>> makes you think that?
>>>
>>> Looking at the PHYMEM.PAMT.ADD ABI spec the pages being added are always
>>> put into pair in rdx/r8. So e.g. looking into tdh_phymem_pamt_add rcx is
>>> set to a 2mb page, and subsequently we have the memcpy which simply sets
>>> the rdx/r8 input argument registers, no ? Or am I misunderstanding the
>>> code?
>>
>> Hmm, you are totally right. The docs specify the size of the 4k entries, but
>> doesn't specify that Dynamic PAMT is supposed to provide larger sizes in the
>> other registers. A reasonable reading could assume 2 pages always, and the usage
>> of the other registers seems like an assumption.
>>
>> Kirill, any history here?
>
> There was a plan to future prove DPAMT by allowing PAMT descriptor to
> grow in the future. The concrete approach was not settled last time I
> checked. This code was my attempt to accommodate it. I don't know if it
> fits the current plan.
>
Considering this, I'd opt for the simplest possible approach that works
_now_. If in the future there are changes to the ABI let's introduce
them incrementally when their time comes.
^ permalink raw reply
* Re: [PATCH v4 07/16] x86/virt/tdx: Add tdx_alloc/free_page() helpers
From: Kiryl Shutsemau @ 2025-12-03 13:46 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, pbonzini@redhat.com,
tglx@linutronix.de, Yamahata, Isaku, nik.borisov@suse.com,
linux-kernel@vger.kernel.org, Annapurve, Vishal, Gao, Chao,
bp@alien8.de, x86@kernel.org
In-Reply-To: <0274cee22d90cbfd2b26c52b864cde6dba04fc60.camel@intel.com>
On Tue, Dec 02, 2025 at 08:02:38PM +0000, Edgecombe, Rick P wrote:
> On Tue, 2025-12-02 at 09:38 +0200, Nikolay Borisov wrote:
> > > Yea, it could be simpler if it was always guaranteed to be 2 pages. But it
> > > was
> > > my understanding that it would not be a fixed size. Can you point to what
> > > docs
> > > makes you think that?
> >
> > Looking at the PHYMEM.PAMT.ADD ABI spec the pages being added are always
> > put into pair in rdx/r8. So e.g. looking into tdh_phymem_pamt_add rcx is
> > set to a 2mb page, and subsequently we have the memcpy which simply sets
> > the rdx/r8 input argument registers, no ? Or am I misunderstanding the
> > code?
>
> Hmm, you are totally right. The docs specify the size of the 4k entries, but
> doesn't specify that Dynamic PAMT is supposed to provide larger sizes in the
> other registers. A reasonable reading could assume 2 pages always, and the usage
> of the other registers seems like an assumption.
>
> Kirill, any history here?
There was a plan to future prove DPAMT by allowing PAMT descriptor to
grow in the future. The concrete approach was not settled last time I
checked. This code was my attempt to accommodate it. I don't know if it
fits the current plan.
--
Kiryl Shutsemau / Kirill A. Shutemov
^ permalink raw reply
* Re: [PATCH v2 20/21] x86/virt/tdx: Update tdx_sysinfo and check features post-update
From: Binbin Wu @ 2025-12-03 7:41 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-21-chao.gao@intel.com>
On 10/1/2025 10:53 AM, Chao Gao wrote:
[...]
>
> +/*
> + * Update tdx_sysinfo and check if any TDX module features changed after
> + * updates
> + */
> +int tdx_module_post_update(struct tdx_sys_info *info)
> +{
> + struct tdx_sys_info_version *cur, *new;
> + int ret;
> +
> + /* Shouldn't fail as the update has succeeded */
> + ret = get_tdx_sys_info(info);
> + if (ret) {
> + WARN_ONCE(1, "version retrieval failed after update, replace TDX Module\n");
Nit:
Could be if (WARN_ONCE(ret, "..."))
> + return ret;
> + }
> +
> + guard(mutex)(&tdx_module_lock);
> +
> + cur = &tdx_sysinfo.version;
Nit:
After update, the current TDX module is the new TDX module already, may be
better to use old instead of cur.
> + new = &info->version;
> + pr_info("version %u.%u.%02u -> %u.%u.%02u\n", cur->major_version,
> + cur->minor_version,
> + cur->update_version,
> + new->major_version,
> + new->minor_version,
> + new->update_version);
> +
> + /*
> + * Blindly refreshing the entire tdx_sysinfo could disrupt running
> + * software, as it may subtly rely on the previous state unless
> + * proven otherwise.
> + *
> + * Only refresh version information (including handoff version)
> + * that does not affect functionality, and ignore all other
> + * changes.
> + */
> + tdx_sysinfo.version = info->version;
> + tdx_sysinfo.handoff = info->handoff;
> +
> + if (!memcmp(&tdx_sysinfo, info, sizeof(*info)))
> + return 0;
> +
> + pr_info("TDX module features have changed after updates, but might not take effect.\n");
> + pr_info("Please consider a potential BIOS update.\n");
BIOS update?
I guess it's "TDX module update via BIOS"?
Does it mean after a system reboot, the change done by TD preserving update will
be gone? If we want the TDX module upgrade to be permanent, it needs to replace
the TDX module binary the BIOS will load, right?
So the scenario of TD preserving update seems to be limited to security fixes?
(I guess the security fixes will take effect directly after TD preserving
update?)
> + return 0;
> +}
> +
> static bool is_pamt_page(unsigned long phys)
> {
> struct tdmr_info_list *tdmr_list = &tdx_tdmr_list;
> diff --git a/arch/x86/virt/vmx/tdx/tdx.h b/arch/x86/virt/vmx/tdx/tdx.h
> index 983c01c6949a..ca76126880ee 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.h
> +++ b/arch/x86/virt/vmx/tdx/tdx.h
> @@ -3,6 +3,7 @@
> #define _X86_VIRT_TDX_H
>
> #include <linux/bits.h>
> +#include <asm/tdx_global_metadata.h>
>
> /*
> * This file contains both macros and data structures defined by the TDX
> @@ -124,5 +125,6 @@ int tdx_module_shutdown(void);
> void tdx_module_set_error(void);
> int tdx_cpu_enable(void);
> int tdx_module_run_update(void);
> +int tdx_module_post_update(struct tdx_sys_info *info);
>
> #endif
^ 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