Re: [PATCH v3 kvm/queue 06/16] KVM: Implement fd-based memory using MEMFD_OPS interfaces

[Chao Peng]

On Thu, Dec 23, 2021 at 06:34:22PM +0000, Sean Christopherson wrote:
> On Thu, Dec 23, 2021, Chao Peng wrote:
> > diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig
> > index 03b2ce34e7f4..86655cd660ca 100644
> > --- a/arch/x86/kvm/Kconfig
> > +++ b/arch/x86/kvm/Kconfig
> > @@ -46,6 +46,7 @@ config KVM
> >     select SRCU
> >     select INTERVAL_TREE
> >     select HAVE_KVM_PM_NOTIFIER if PM
> > +   select MEMFD_OPS
> 
> MEMFD_OPS is a weird Kconfig name given that it's not just memfd() that can
> implement the ops.
> 
> >     help
> >       Support hosting fully virtualized guest machines using hardware
> >       virtualization extensions.  You will need a fairly recent
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index 3bd875f9669f..21f8b1880723 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -806,6 +806,12 @@ static inline void kvm_irqfd_exit(void)
> >  {
> >  }
> >  #endif
> > +
> > +int kvm_memfd_register(struct kvm *kvm, struct kvm_memory_slot *slot);
> > +void kvm_memfd_unregister(struct kvm_memory_slot *slot);
> > +long kvm_memfd_get_pfn(struct kvm_memory_slot *slot, gfn_t gfn, int 
> > *order);
> > +void kvm_memfd_put_pfn(kvm_pfn_t pfn);
> > +
> >  int kvm_init(void *opaque, unsigned vcpu_size, unsigned vcpu_align,
> >               struct module *module);
> >  void kvm_exit(void);
> > diff --git a/virt/kvm/Makefile.kvm b/virt/kvm/Makefile.kvm
> > index ffdcad3cc97a..8842128d8429 100644
> > --- a/virt/kvm/Makefile.kvm
> > +++ b/virt/kvm/Makefile.kvm
> > @@ -5,7 +5,7 @@
> >  
> >  KVM ?= ../../../virt/kvm
> >  
> > -kvm-y := $(KVM)/kvm_main.o $(KVM)/eventfd.o $(KVM)/binary_stats.o
> > +kvm-y := $(KVM)/kvm_main.o $(KVM)/eventfd.o $(KVM)/binary_stats.o 
> > $(KVM)/memfd.o
> 
> This should be
> 
>    kvm-$(CONFIG_MEMFD_OPS) += $(KVM)/memfd.o
> 
> with stubs provided in a header file as needed.  I also really dislike naming 
> KVM's
> file memfd.c, though I don't have a good alternative off the top of my head.
> 
> >  kvm-$(CONFIG_KVM_VFIO) += $(KVM)/vfio.o
> >  kvm-$(CONFIG_KVM_MMIO) += $(KVM)/coalesced_mmio.o
> >  kvm-$(CONFIG_KVM_ASYNC_PF) += $(KVM)/async_pf.o
> 
> 
> > +#ifdef CONFIG_MEMFD_OPS
> > +static const struct memfd_pfn_ops *memfd_ops;
> 
> memfd_ops needs to be associated with the slot, e.g. userspace should be able 
> to
> map multiple types of a backing stores into a single VM.

I considered this but gave up as I'm not so confident that we will
support other memory backends than memfd in the forthcoming future. 

>This doesn't even allow
> that for multiple VMs, and there are all kinds of ordering issues.

Current memfd kAPI actually returns the same set of callback pointer for
all the VMs. It supports multiple VMs via callback parameter inode,
assume one inode can be associated with only one VM.

> 
> > +void kvm_memfd_unregister(struct kvm_memory_slot *slot)
> > +{
> > +#ifdef CONFIG_MEMFD_OPS
> > +   if (slot->file) {
> > +           fput(slot->file);
> 
> Needs to actually unregister.

Good catch, thanks.
> 
> > +           slot->file = NULL;
> > +   }
> > +#endif
> > +}
> > -- 
> > 2.17.1
> >