Re: [PATCH v8 08/13] KVM: guest_memfd: Allow host to map guest_memfd() pages

[Fuad Tabba <tabba@google.com>]

Hi Ira,

On Mon, 5 May 2025 at 22:05, Ira Weiny <ira.weiny@intel.com> wrote:
>
> Fuad Tabba wrote:
> > Add support for mmap() and fault() for guest_memfd backed memory
> > in the host for VMs that support in-place conversion between
> > shared and private. To that end, this patch adds the ability to
> > check whether the VM type supports in-place conversion, and only
> > allows mapping its memory if that's the case.
> >
> > This patch introduces the configuration option KVM_GMEM_SHARED_MEM,
> > which enables support for in-place shared memory.
> >
> > It also introduces the KVM capability KVM_CAP_GMEM_SHARED_MEM, which
> > indicates that the host can create VMs that support shared memory.
> > Supporting shared memory implies that memory can be mapped when shared
> > with the host.
> >
> > Signed-off-by: Fuad Tabba <tabba@google.com>
> > ---
> >  include/linux/kvm_host.h | 15 ++++++-
> >  include/uapi/linux/kvm.h |  1 +
> >  virt/kvm/Kconfig         |  5 +++
> >  virt/kvm/guest_memfd.c   | 92 ++++++++++++++++++++++++++++++++++++++++
> >  virt/kvm/kvm_main.c      |  4 ++
> >  5 files changed, 116 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index 9419fb99f7c2..f3af6bff3232 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -729,6 +729,17 @@ static inline bool kvm_arch_supports_gmem(struct kvm *kvm)
> >  }
> >  #endif
> >
> > +/*
> > + * Arch code must define kvm_arch_gmem_supports_shared_mem if support for
> > + * private memory is enabled and it supports in-place shared/private conversion.
> > + */
> > +#if !defined(kvm_arch_gmem_supports_shared_mem) && !IS_ENABLED(CONFIG_KVM_GMEM_SHARED_MEM)
>
> Perhaps the bots already caught this?
>
> I just tried enabling KVM_GMEM_SHARED_MEM on x86 with this patch and it fails with:
>
> || In file included from arch/x86/kvm/../../../virt/kvm/binary_stats.c:8:
> || ./include/linux/kvm_host.h: In function ‘kvm_mem_from_gmem’:
> include/linux/kvm_host.h|2530 col 13| error: implicit declaration of function ‘kvm_arch_gmem_supports_shared_mem’ [-Wimplicit-function-declaration]
> ||  2530 |         if (kvm_arch_gmem_supports_shared_mem(kvm))
> ||       |             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> || make[4]: *** Waiting for unfinished jobs....
>
>
> I think the predicate on !CONFIG_KVM_GMEM_SHARED_MEM is wrong.
>
> Shouldn't this always default off?  I __think__ this then gets enabled in
> 11/13?

You're right. With the other comments from David and Ackerley, this
functions is gone, replaced by checking a per-vm flag.

Thanks,
/fuad

> IOW
>
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index f3af6bff3232..577674e95c09 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -733,7 +733,7 @@ static inline bool kvm_arch_supports_gmem(struct kvm *kvm)
>   * Arch code must define kvm_arch_gmem_supports_shared_mem if support for
>   * private memory is enabled and it supports in-place shared/private conversion.
>   */
> -#if !defined(kvm_arch_gmem_supports_shared_mem) && !IS_ENABLED(CONFIG_KVM_GMEM_SHARED_MEM)
> +#if !defined(kvm_arch_gmem_supports_shared_mem)
>  static inline bool kvm_arch_gmem_supports_shared_mem(struct kvm *kvm)
>  {
>         return false;