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

[Sean Christopherson <seanjc@google.com>]

On Wed, Aug 26, 2026, Michael Roth wrote:
> 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.

Or another way to look at it, eager conversion would allow QEMU to drop its
workaround.

To be clear, I'm a-ok with the code as-is, I just want to make sure we document
exactly why we're choosing this implementation.

> 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,

Why would they trigger conversions back to shared?  Assuming the guest isn't
being silly and accessing the memory with C-bit=0, the #NPF will be tagged ENC
and KVM will treat it as a private access.

	if (is_sev_snp_guest(vcpu) && (error_code & PFERR_GUEST_ENC_MASK))
		error_code |= PFERR_PRIVATE_ACCESS;

kvm_mmu_faultin_pfn() will see "fault->is_private == kvm_is_private_gfn()" as
true, i.e. won't kick out to userspace.  Same for __kvm_mmu_faultin_pfn(), which
will call into kvm_mmu_faultin_pfn_gmem() => kvm_gmem_get_pfn(), see that the
gfn is private, and call kvm_arch_gmem_make_private() as needed.

I don't see how #NPFs due to the RMP being SHARED would be handled differently
than !PRESENT #NPFs.

> 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.

No, because PUNCH_HOLE uses kvm_gmem_get_all_gfns_filter(), which does:

	if (gmem_in_place_conversion)
		return KVM_FILTER_SHARED | KVM_FILTER_PRIVATE;

i.e. won't get short-circuited.  Though I agree with the implication that this
is super fragile/subtle.