Fuad Tabba <tabba@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> ---
>
> 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?
> +
> 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.
