On Fri, Nov 15, 2024 at 11:58:59AM +0100, David Hildenbrand wrote: > On 15.11.24 11:58, David Hildenbrand wrote: > > On 13.11.24 23:34, Elliot Berman wrote: > > > When guest_memfd becomes a library, a callback will need to be made to > > > the owner (KVM SEV) to transition pages back to hypervisor-owned/shared > > > state. This is currently being done as part of .free_folio() address > > > space op, but this callback shouldn't assume that the mapping still > > > exists. guest_memfd library will need the mapping to still exist to look > > > up its operations table. > > > > I assume you mean, that the mapping is no longer set for the folio (it > > sure still exists, because we are getting a callback from it :) )? > > > > Staring at filemap_remove_folio(), this is exactly what happens: > > > > We remember folio->mapping, call __filemap_remove_folio(), and then call > > filemap_free_folio() where we zap folio->mapping via page_cache_delete(). > > > > Maybe it's easier+cleaner to also forward the mapping to the > > free_folio() callback, just like we do with filemap_free_folio()? Would > > that help? > > > > CCing Willy if that would be reasonable extension of the free_folio > > callback. > > I like this approach too. It would avoid the checks we have to do in the invalidate_folio() callback and is cleaner. - Elliot > > > > > > .release_folio() and .invalidate_folio() address space ops can serve the > > > same purpose here. The key difference between release_folio() and > > > free_folio() is whether the mapping is still valid at time of the > > > callback. This approach was discussed in the link in the footer, but not > > > taken because free_folio() was easier to implement. > > > > > > Link: https://lore.kernel.org/kvm/20231016115028.996656-1-michael.roth@xxxxxxx/ > > > Signed-off-by: Elliot Berman <quic_eberman@xxxxxxxxxxx> > > > --- > > > virt/kvm/guest_memfd.c | 19 ++++++++++++++++--- > > > 1 file changed, 16 insertions(+), 3 deletions(-) > > > > > > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c > > > index 47a9f68f7b247f4cba0c958b4c7cd9458e7c46b4..13f83ad8a4c26ba82aca4f2684f22044abb4bc19 100644 > > > --- a/virt/kvm/guest_memfd.c > > > +++ b/virt/kvm/guest_memfd.c > > > @@ -358,22 +358,35 @@ static int kvm_gmem_error_folio(struct address_space *mapping, struct folio *fol > > > } > > > #ifdef CONFIG_HAVE_KVM_ARCH_GMEM_INVALIDATE > > > -static void kvm_gmem_free_folio(struct folio *folio) > > > +static bool kvm_gmem_release_folio(struct folio *folio, gfp_t gfp) > > > { > > > struct page *page = folio_page(folio, 0); > > > kvm_pfn_t pfn = page_to_pfn(page); > > > int order = folio_order(folio); > > > kvm_arch_gmem_invalidate(pfn, pfn + (1ul << order)); > > > + > > > + return true; > > > +} > > > + > > > +static void kvm_gmem_invalidate_folio(struct folio *folio, size_t offset, > > > + size_t len) > > > +{ > > > + WARN_ON_ONCE(offset != 0); > > > + WARN_ON_ONCE(len != folio_size(folio)); > > > + > > > + if (offset == 0 && len == folio_size(folio)) > > > + filemap_release_folio(folio, 0); > > > } > > > #endif > > > static const struct address_space_operations kvm_gmem_aops = { > > > .dirty_folio = noop_dirty_folio, > > > - .migrate_folio = kvm_gmem_migrate_folio, > > > + .migrate_folio = kvm_gmem_migrate_folio, > > > .error_remove_folio = kvm_gmem_error_folio, > > > #ifdef CONFIG_HAVE_KVM_ARCH_GMEM_INVALIDATE > > > - .free_folio = kvm_gmem_free_folio, > > > + .release_folio = kvm_gmem_release_folio, > > > + .invalidate_folio = kvm_gmem_invalidate_folio, > > > #endif > > > }; > > > > > > > > > > -- > Cheers, > > David / dhildenb >
