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 03:33:43PM -0700, Sean Christopherson wrote:
> 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.

True, for SNP at least, but pre-map is nice too, and not really gmem's
place, so most likely KVM_PRE_FAULT_MEMORY would remain regardless.

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

Agreed. My understanding of the current design choice is basically that
it doesn't *need* to happen here, since kvm_gmem_get_pfn() will already
trigger it on-demand when it is actually needed (or not), and that tends
to happen at fault time when other platforms like TDX handle it. To handle
it differently would be to go out of our way to make SNP special in this
flow in exchange for performance, but KVM_PRE_FAULT_MEMORY already offers
a robust solution for folks that want to optimize conversion path or
prealloc host resources prior to guest boot.

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

I wasn't meaning the normal flow for a well-behaved guest, but when you
mentioned "mismatched C-bit vs. RMP status will cause an #NPF(RMP)" I
thought you were considering the case of multiple vCPUs accessing the
same GPA in various ways (that may or may not make sense) and whether not
zapping before conversion would cause any issues with the handling of that
scenario. In that scenario there is potential for any mix of mismatches
between C-bits and RMP tables.

The silly case with C=0 is the specific scenario I was thinking of. E.g.
one vCPU is trying to due memory acceptance/PVALIDATE of a GPA that's
somehow still being used by some other vCPU for some shared memory
workload.

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

Yes I don't see any issue with the normal/well-behaved path, it's just
the funky edge-cases that seem bring about some new considerations, but
it seems like those would be handled okay.

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

Doh, helps if I actually have the right branch checkout out when looking at
code. That does look to be covered.

I'll give this patch a spin on my end as well and report back if I notice
any crazyness.

Thanks,

Mike

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