Re: [PATCH RFC v1 1/5] KVM: guest_memfd: Remove preparation tracking

Yan Zhao
2025-11-07T13:07:25Z

Hi Michael,
Have you posted a newer version of this patch?

I also have a question about this patch:

Suppose there's a 2MB huge folio A, where
A1 and A2 are 4KB pages belonging to folio A.

(1) kvm_gmem_populate() invokes __kvm_gmem_get_pfn() and gets folio A.
It adds page A1 and invokes folio_mark_uptodate() on folio A.

(2) kvm_gmem_get_pfn() later faults in page A2.
As folio A is uptodate, clear_highpage() is not invoked on page A2.
kvm_gmem_prepare_folio() is invoked on the whole folio A.

(2) could occur at least in TDX when only a part the 2MB page is added as guest
initial memory.

My questions:
- Would (2) occur on SEV?
- If it does, is the lack of clear_highpage() on A2 a problem ?
- Is invoking gmem_prepare on page A1 a problem?

Thanks
Yan

On Thu, Jun 12, 2025 at 07:53:56PM -0500, Michael Roth wrote:
> guest_memfd currently uses the folio uptodate flag to track:
>
> 1) whether or not a page had been cleared before initial usage
> 2) whether or not the architecture hooks have been issued to put the
> page in a private state as defined by the architecture
>
> In practice, 2) is only actually being tracked for SEV-SNP VMs, and
> there do not seem to be any plans/reasons that would suggest this will
> change in the future, so this additional tracking/complexity is not
> really providing any general benefit to guest_memfd users. Future plans
> around in-place conversion and hugepage support, where the per-folio
> uptodate flag is planned to be used purely to track the initial clearing
> of folios, whereas conversion operations could trigger multiple
> transitions between 'prepared' and 'unprepared' and thus need separate
> tracking, will make the burden of tracking this information within
> guest_memfd even more complex, since preparation generally happens
> during fault time, on the "read-side" of any global locks that might
> protect state tracked by guest_memfd, and so may require more complex
> locking schemes to allow for concurrent handling of page faults for
> multiple vCPUs where the "preparedness" state tracked by guest_memfd
> might need to be updated as part of handling the fault.
>
> Instead of keeping this current/future complexity within guest_memfd for
> what is essentially just SEV-SNP, just drop the tracking for 2) and have
> the arch-specific preparation hooks get triggered unconditionally on
> every fault so the arch-specific hooks can check the preparation state
> directly and decide whether or not a folio still needs additional
> preparation. In the case of SEV-SNP, the preparation state is already
> checked again via the preparation hooks to avoid double-preparation, so
> nothing extra needs to be done to update the handling of things there.
>
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> ---
> virt/kvm/guest_memfd.c | 47 ++++++++++++++----------------------------
> 1 file changed, 15 insertions(+), 32 deletions(-)
>
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index 35f94a288e52..cc93c502b5d8 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -421,11 +421,6 @@ static int __kvm_gmem_prepare_folio(struct kvm *kvm, struct kvm_memory_slot *slo
> return 0;
> }
>
> -static inline void kvm_gmem_mark_prepared(struct folio *folio)
> -{
> - folio_mark_uptodate(folio);
> -}
> -
> /*
> * Process @folio, which contains @gfn, so that the guest can use it.
> * The folio must be locked and the gfn must be contained in @slot.
> @@ -435,13 +430,7 @@ static inline void kvm_gmem_mark_prepared(struct folio *folio)
> 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;
> - int r;
> -
> - nr_pages = folio_nr_pages(folio);
> - for (i = 0; i < nr_pages; i++)
> - clear_highpage(folio_page(folio, i));
>
> /*
> * Preparing huge folios should always be safe, since it should
> @@ -459,11 +448,8 @@ static int kvm_gmem_prepare_folio(struct kvm *kvm, struct kvm_memory_slot *slot,
> WARN_ON(!IS_ALIGNED(slot->gmem.pgoff, 1 << folio_order(folio)));
> index = gfn - slot->base_gfn + slot->gmem.pgoff;
> index = ALIGN_DOWN(index, 1 << folio_order(folio));
> - r = __kvm_gmem_prepare_folio(kvm, slot, index, folio);
> - if (!r)
> - kvm_gmem_mark_prepared(folio);
>
> - return r;
> + return __kvm_gmem_prepare_folio(kvm, slot, index, folio);
> }
>
> static int __kvm_gmem_filemap_add_folio(struct address_space *mapping,
> @@ -808,7 +794,7 @@ static vm_fault_t kvm_gmem_fault_shared(struct vm_fault *vmf)
>
> if (!folio_test_uptodate(folio)) {
> clear_highpage(folio_page(folio, 0));
> - kvm_gmem_mark_prepared(folio);
> + folio_mark_uptodate(folio);
> }
>
> vmf->page = folio_file_page(folio, vmf->pgoff);
> @@ -1306,7 +1292,7 @@ void kvm_gmem_unbind(struct kvm_memory_slot *slot)
> static struct folio *__kvm_gmem_get_pfn(struct file *file,
> struct kvm_memory_slot *slot,
> pgoff_t index, kvm_pfn_t *pfn,
> - bool *is_prepared, int *max_order)
> + int *max_order)
> {
> struct file *gmem_file = READ_ONCE(slot->gmem.file);
> struct kvm_gmem *gmem = file->private_data;
> @@ -1337,7 +1323,6 @@ static struct folio *__kvm_gmem_get_pfn(struct file *file,
> if (max_order)
> *max_order = 0;
>
> - *is_prepared = folio_test_uptodate(folio);
> return folio;
> }
>
> @@ -1348,7 +1333,6 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
> pgoff_t index = kvm_gmem_get_index(slot, gfn);
> struct file *file = kvm_gmem_get_file(slot);
> struct folio *folio;
> - bool is_prepared = false;
> int r = 0;
>
> if (!file)
> @@ -1356,14 +1340,21 @@ int kvm_gmem_get_pfn(struct kvm *kvm, struct kvm_memory_slot *slot,
>
> filemap_invalidate_lock_shared(file_inode(file)->i_mapping);
>
> - 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)) {
> r = PTR_ERR(folio);
> goto out;
> }
>
> - 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);
> + }
> +
> + r = kvm_gmem_prepare_folio(kvm, slot, gfn, folio);
> folio_unlock(folio);
>
> @@ -1420,7 +1411,6 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
> struct folio *folio;
> gfn_t gfn = start_gfn + i;
> pgoff_t index = kvm_gmem_get_index(slot, gfn);
> - bool is_prepared = false;
> kvm_pfn_t pfn;
>
> if (signal_pending(current)) {
> @@ -1428,19 +1418,12 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
> break;
> }
>
> - 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)) {
> ret = PTR_ERR(folio);
> break;
> }
>
> - if (is_prepared) {
> - folio_unlock(folio);
> - folio_put(folio);
> - ret = -EEXIST;
> - break;
> - }
> -
> folio_unlock(folio);
> WARN_ON(!IS_ALIGNED(gfn, 1 << max_order) ||
> (npages - i) < (1 << max_order));
> @@ -1457,7 +1440,7 @@ long kvm_gmem_populate(struct kvm *kvm, gfn_t start_gfn, void __user *src, long
> p = src ? src + i * PAGE_SIZE : NULL;
> ret = post_populate(kvm, gfn, pfn, p, max_order, opaque);
> if (!ret)
> - kvm_gmem_mark_prepared(folio);
> + folio_mark_uptodate(folio);
>
> put_folio_and_exit:
> folio_put(folio);
> --
> 2.25.1
>