Re: [PATCH v11 15/46] KVM: guest_memfd: Call arch make_shared callback for to-shared conversion

[Michael Roth]

On Wed, Aug 26, 2026 at 12:44:40PM -0700, Sean Christopherson wrote:
> On Wed, Aug 26, 2026, Ackerley Tng wrote:
> > Omit support for calling the arch hook to make private, since SNP, the only
> > implementer of the arch make-private hook today, would actually prefer
> > making private only just before faulting memory into the NPTs.
> > 
> > Calling the make-private arch hook would require iterating both bindings
> > and the filemap to find the intersection of bindings and allocated
> > folios.
> 
> Why would KVM need to iterate over the bindings?  Only the RMP needs to be updated,
> whether or not the RMP is currently reachable is irrelevant, no?
> 
> Subsequent calls to kvm_arch_gmem_make_private() from kvm_gmem_get_pfn() would be
> superfluous, but that's already possible, e.g. if an NPT mappings is removed for
> whatever reason.
> 
> > On top of that, SNP would need to figure out whether to actually
> > make private based on whether the memory is about to be faulted, or
> > whether it is a conversion.
> 
> This is a non-issue, no?  As above, sev_gmem_make_private() already bails early
> if the page is already assigned in the RMP.
> 
> I don't care terribly about how SNP handles this, but I do want accurate reasoning
> and justification so that if/when we revisit any of this in the future, we can make
> informed decisions.  Because unless I'm missing something, this is an optimization
> choice (eager vs. lazy to-private conversions), not a complexity tradeoff, and it's
> not clear to me how we decided the lazy approach would provide better performance.

I'm not sure it was discussed in this context, but there was some past
discussion around preallocation (i.e. "should we call make-private arch
hooks at allocation time to allow for faster boot for prealloc guests"
and then that ran into the TDX side of things where that would
necessarily entail pre-mapping into the sEPT as well, so
KVM_PRE_FAULT_MEMORY ended up being the interface we adopted for this
purpose.

Since then, KVM_PRE_FAULT_MEMORY was added on the QEMU side and gets
called after all conversions for both SNP/TDX, and even without
preallocation it's a decent performance boost to SNP. If we were to
switch to pre-calling the make-private arch hook then the
KVM_PRE_FAULT_MEMORY call because partly redundant and in practice we'd
probably see a small performance loss.

So there's real performance differences here but it's sort of been
addressed through a solution that offers additional performance
benefits on top so there's no longer as much to be gained here I think.

But I guess that's a moot point...

> 
> > Calling the make-shared arch hook and not the make-private arch hook does
> > leak SNP-specific details into guest_memfd (as in, why only make-shared
> > during conversions but not make-private?), but the additional complexity is
> > not worth taking on until guest_memfd has a user actually requiring an arch
> > make-private call.
> 
> ...
> 
> > +#ifdef CONFIG_HAVE_KVM_ARCH_GMEM_CONVERT
> > +static void kvm_gmem_make_shared(struct inode *inode, pgoff_t start, pgoff_t end)
> > +{
> > +	struct folio_batch fbatch;
> > +	pgoff_t next = start;
> > +	int i;
> > +
> > +	folio_batch_init(&fbatch);
> > +	while (filemap_get_folios(inode->i_mapping, &next, end - 1, &fbatch)) {
> > +		for (i = 0; i < folio_batch_count(&fbatch); ++i) {
> > +			struct folio *folio = fbatch.folios[i];
> > +			pgoff_t start_index, end_index;
> > +			kvm_pfn_t start_pfn;
> > +			kvm_pfn_t nr_pages;
> > +
> > +			start_index = max(start, folio->index);
> > +			end_index = min(end, folio_next_index(folio));
> > +			/*
> > +			 * end_index is either in folio or points to
> > +			 * the first page of the next folio. Hence,
> > +			 * all pages in range [start_index, end_index)
> > +			 * are contiguous.
> > +			 */
> > +			start_pfn = folio_file_pfn(folio, start_index);
> > +			nr_pages = end_index - start_index;
> > +
> > +			kvm_arch_gmem_make_shared(start_pfn, nr_pages);
> > +		}
> > +
> > +		folio_batch_release(&fbatch);
> > +		cond_resched();
> > +	}
> > +}
> > +#else
> > +static void kvm_gmem_make_shared(struct inode *inode, pgoff_t start, pgoff_t end) {}
> > +#endif
> > +
> >  static int __kvm_gmem_set_attributes(struct inode *inode, pgoff_t start,
> >  				     size_t nr_pages, uint64_t attrs,
> >  				     pgoff_t *err_index)
> > @@ -624,7 +661,12 @@ static int __kvm_gmem_set_attributes(struct inode *inode, pgoff_t start,
> >  
> >  	filter = to_private ? KVM_FILTER_SHARED : KVM_FILTER_PRIVATE;
> >  	kvm_gmem_invalidate_start(inode, start, end, filter);
> > +
> > +	if (!to_private && kvm_arch_has_gmem_convert())
> > +		kvm_gmem_make_shared(inode, start, end);
> > +
> >  	mas_store_prealloc(&mas, xa_mk_value(attrs));
> > +
> >  	kvm_gmem_invalidate_end(inode, start, end);
> 
> The real reason I responded...
> 
> Thinking about the Secure AVIC mess made me realize zapping NPTs for SNP VMs isn't
> strictly necessary in this path.  The PFN isn't changing, just the attributes, and
> that's (obviously) tracked in the RMP.  KVM doesn't need to zap SPTEs to induce a
> fault, because the mismatched C-bit vs. RMP status will cause an #NPF(RMP), and
> AFAICT kvm_mmu_page_fault() will do the right thing.  A misbehaving guest could
> continue to access the shared data (assuming we stick with lazy conversions), but
> that should be fine?  E.g. it's not really any different than implicit conversions.

I think this should work in theory...

zapping NPTs means the vCPUs will keep retrying until they get the page first
vCPU that faulted is trying to grab from gmem. If we don't zap, then they will
instead be racing with the first vCPU, and if they lose they will be
generating implicit page faults that trigger conversions back to shared,
and most likely the first vCPU will re-trigger an implicit shared->private
conversion when it does PVALIDATE. Worst case, the guest fails PVALIDATE
due to racing with itself.

It's a bit chaotic, but it shouldn't break anything other than the guest, and
it's only something we'd generally expect for buggy/malicious guests anyway.

However...

> 
> In other words, couldn't we do this (as an on-top optimization)?  The only wrinkle
> I can think of is that it could delay reconstituion of a hugepage, especially if
> we opted for eager conversion (because the guest wouldn't hit #NPFs to trigger the
> hugepage promotion).
> 
> diff --git arch/x86/kvm/mmu/mmu.c arch/x86/kvm/mmu/mmu.c
> index 62f751952ad8..61f3e270ab61 100644
> --- arch/x86/kvm/mmu/mmu.c
> +++ arch/x86/kvm/mmu/mmu.c
> @@ -1670,6 +1670,7 @@ static bool __kvm_rmap_zap_gfn_range(struct kvm *kvm,
>  
>  bool kvm_unmap_gfn_range(struct kvm *kvm, struct kvm_gfn_range *range)
>  {
> +       unsigned long shared_private =  KVM_FILTER_SHARED | KVM_FILTER_PRIVATE;
>         bool flush = false;
>  
>         /*
> @@ -1683,6 +1684,10 @@ bool kvm_unmap_gfn_range(struct kvm *kvm, struct kvm_gfn_range *range)
>         lockdep_assert_once(kvm->mmu_invalidate_in_progress ||
>                             lockdep_is_held(&kvm->slots_lock));
>  
> +       if (gmem_in_place_conversion && !kvm_has_mirrored_tdp(kvm) &&
> +           ((range->attr_filter & shared_private) != shared_private))
> +               return false;
> +

This path would also trigger for hole-punching, where we would want to
zap the NPT entries. So we might need to adjust the logic for more than
just shared vs. private to account for that.

-Mike

>         if (kvm_memslots_have_rmaps(kvm))
>                 flush = __kvm_rmap_zap_gfn_range(kvm, range->slot,
>                                                  range->start, range->end,