| Message ID | 20250312175824.1809636-4-tabba@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: Mapping guest_memfd backed memory at the host for software protected VMs | expand |
Hi Ackerley, On Thu, 13 Mar 2025 at 13:49, Ackerley Tng <ackerleytng@google.com> wrote: > > Fuad Tabba <tabba@google.com> writes: > > > In some architectures, KVM could be defined as a module. If there is a > > pending folio_put() while KVM is unloaded, the system could crash. By > > having a helper check for that and call the function only if it's > > available, we are able to handle that case more gracefully. > > > > Signed-off-by: Fuad Tabba <tabba@google.com> > > > > --- > > > > This patch could be squashed with the previous one of the maintainers > > think it would be better. > > --- > > include/linux/kvm_host.h | 5 +---- > > mm/swap.c | 20 +++++++++++++++++++- > > virt/kvm/guest_memfd.c | 8 ++++++++ > > 3 files changed, 28 insertions(+), 5 deletions(-) > > > > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h > > index 7788e3625f6d..3ad0719bfc4f 100644 > > --- a/include/linux/kvm_host.h > > +++ b/include/linux/kvm_host.h > > @@ -2572,10 +2572,7 @@ long kvm_arch_vcpu_pre_fault_memory(struct kvm_vcpu *vcpu, > > #endif > > > > #ifdef CONFIG_KVM_GMEM_SHARED_MEM > > -static inline void kvm_gmem_handle_folio_put(struct folio *folio) > > -{ > > - WARN_ONCE(1, "A placeholder that shouldn't trigger. Work in progress."); > > -} > > +void kvm_gmem_handle_folio_put(struct folio *folio); > > #endif > > > > #endif > > diff --git a/mm/swap.c b/mm/swap.c > > index 241880a46358..27dfd75536c8 100644 > > --- a/mm/swap.c > > +++ b/mm/swap.c > > @@ -98,6 +98,24 @@ static void page_cache_release(struct folio *folio) > > unlock_page_lruvec_irqrestore(lruvec, flags); > > } > > > > +#ifdef CONFIG_KVM_GMEM_SHARED_MEM > > +static void gmem_folio_put(struct folio *folio) > > +{ > > +#if IS_MODULE(CONFIG_KVM) > > + void (*fn)(struct folio *folio); > > + > > + fn = symbol_get(kvm_gmem_handle_folio_put); > > + if (WARN_ON_ONCE(!fn)) > > + return; > > + > > + fn(folio); > > + symbol_put(kvm_gmem_handle_folio_put); > > +#else > > + kvm_gmem_handle_folio_put(folio); > > +#endif > > +} > > +#endif > > Sorry about the premature sending earlier! > > I was thinking about having a static function pointer in mm/swap.c that > will be filled in when KVM is loaded and cleared when KVM is unloaded. > > One benefit I see is that it'll avoid the lookup that symbol_get() does > on every folio_put(), but some other pinning on KVM would have to be > done to prevent KVM from being unloaded in the middle of > kvm_gmem_handle_folio_put() call. > > Do you/anyone else see pros/cons either way? To be honest, I didn't do it that way (or even think of doing it that way) since that's how vfio does it, e.g.,: https://elixir.bootlin.com/linux/v6.14-rc6/source/virt/kvm/vfio.c#L38 Like you said, your suggestion would require something like try_module_get()/put() to ensure that it's not unloaded. Is that better? I don't know... Maybe someone with more experience could chime in. Thanks! /fuad > > + > > static void free_typed_folio(struct folio *folio) > > { > > switch (folio_get_type(folio)) { > > @@ -108,7 +126,7 @@ static void free_typed_folio(struct folio *folio) > > #endif > > #ifdef CONFIG_KVM_GMEM_SHARED_MEM > > case PGTY_guestmem: > > - kvm_gmem_handle_folio_put(folio); > > + gmem_folio_put(folio); > > return; > > #endif > > default: > > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c > > index b2aa6bf24d3a..5fc414becae5 100644 > > --- a/virt/kvm/guest_memfd.c > > +++ b/virt/kvm/guest_memfd.c > > @@ -13,6 +13,14 @@ struct kvm_gmem { > > struct list_head entry; > > }; > > > > +#ifdef CONFIG_KVM_GMEM_SHARED_MEM > > +void kvm_gmem_handle_folio_put(struct folio *folio) > > +{ > > + WARN_ONCE(1, "A placeholder that shouldn't trigger. Work in progress."); > > +} > > +EXPORT_SYMBOL_GPL(kvm_gmem_handle_folio_put); > > +#endif /* CONFIG_KVM_GMEM_SHARED_MEM */ > > + > > /** > > * folio_file_pfn - like folio_file_page, but return a pfn. > > * @folio: The folio which contains this index.
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 7788e3625f6d..3ad0719bfc4f 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -2572,10 +2572,7 @@ long kvm_arch_vcpu_pre_fault_memory(struct kvm_vcpu *vcpu, #endif #ifdef CONFIG_KVM_GMEM_SHARED_MEM -static inline void kvm_gmem_handle_folio_put(struct folio *folio) -{ - WARN_ONCE(1, "A placeholder that shouldn't trigger. Work in progress."); -} +void kvm_gmem_handle_folio_put(struct folio *folio); #endif #endif diff --git a/mm/swap.c b/mm/swap.c index 241880a46358..27dfd75536c8 100644 --- a/mm/swap.c +++ b/mm/swap.c @@ -98,6 +98,24 @@ static void page_cache_release(struct folio *folio) unlock_page_lruvec_irqrestore(lruvec, flags); } +#ifdef CONFIG_KVM_GMEM_SHARED_MEM +static void gmem_folio_put(struct folio *folio) +{ +#if IS_MODULE(CONFIG_KVM) + void (*fn)(struct folio *folio); + + fn = symbol_get(kvm_gmem_handle_folio_put); + if (WARN_ON_ONCE(!fn)) + return; + + fn(folio); + symbol_put(kvm_gmem_handle_folio_put); +#else + kvm_gmem_handle_folio_put(folio); +#endif +} +#endif + static void free_typed_folio(struct folio *folio) { switch (folio_get_type(folio)) { @@ -108,7 +126,7 @@ static void free_typed_folio(struct folio *folio) #endif #ifdef CONFIG_KVM_GMEM_SHARED_MEM case PGTY_guestmem: - kvm_gmem_handle_folio_put(folio); + gmem_folio_put(folio); return; #endif default: diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c index b2aa6bf24d3a..5fc414becae5 100644 --- a/virt/kvm/guest_memfd.c +++ b/virt/kvm/guest_memfd.c @@ -13,6 +13,14 @@ struct kvm_gmem { struct list_head entry; }; +#ifdef CONFIG_KVM_GMEM_SHARED_MEM +void kvm_gmem_handle_folio_put(struct folio *folio) +{ + WARN_ONCE(1, "A placeholder that shouldn't trigger. Work in progress."); +} +EXPORT_SYMBOL_GPL(kvm_gmem_handle_folio_put); +#endif /* CONFIG_KVM_GMEM_SHARED_MEM */ + /** * folio_file_pfn - like folio_file_page, but return a pfn. * @folio: The folio which contains this index.
In some architectures, KVM could be defined as a module. If there is a pending folio_put() while KVM is unloaded, the system could crash. By having a helper check for that and call the function only if it's available, we are able to handle that case more gracefully. Signed-off-by: Fuad Tabba <tabba@google.com> --- This patch could be squashed with the previous one of the maintainers think it would be better. --- include/linux/kvm_host.h | 5 +---- mm/swap.c | 20 +++++++++++++++++++- virt/kvm/guest_memfd.c | 8 ++++++++ 3 files changed, 28 insertions(+), 5 deletions(-)