Linux Virtualization - Jan 2021 - [RFC 3/3] vfio: Share the KVM instance with Vdmabuf

On Wed, 20 Jan 2021 00:14:49 +0000
"Kasireddy, Vivek" <vivek.kasireddy at intel.com> wrote:
> Hi Alex,
> 
> > -----Original Message-----
> > From: Alex Williamson <alex.williamson at redhat.com>
> > Sent: Tuesday, January 19, 2021 7:40 AM
> > To: Kasireddy, Vivek <vivek.kasireddy at intel.com>
> > Cc: virtualization at lists.linux-foundation.org; Kim, Dongwon
<dongwon.kim at intel.com>
> > Subject: Re: [RFC 3/3] vfio: Share the KVM instance with Vdmabuf
> > 
> > On Tue, 19 Jan 2021 00:28:12 -0800
> > Vivek Kasireddy <vivek.kasireddy at intel.com> wrote:
> >   
> > > Getting a copy of the KVM instance is necessary for mapping Guest
> > > pages in the Host.
> > >
> > > TODO: Instead of invoking the symbol directly, there needs to be
a
> > > better way of getting a copy of the KVM instance probably by
using
> > > other notifiers. However, currently, KVM shares its instance only
> > > with VFIO and therefore we are compelled to bind the
passthrough'd
> > > device to vfio-pci.  
> > 
> > Yeah, this is a bad solution, sorry, vfio is not going to gratuitously
> > call out to vhost to share a kvm pointer.  I'd prefer to get rid
of
> > vfio having any knowledge or visibility of the kvm pointer.  Thanks,  
> 
> [Kasireddy, Vivek] I agree that this is definitely not ideal as I recognize
it
> in the TODO. However, it looks like VFIO also gets a copy of the KVM 
> pointer in a similar manner:
> 
> virt/kvm/vfio.c
> 
> static void kvm_vfio_group_set_kvm(struct vfio_group *group, struct kvm
*kvm)
> {
>         void (*fn)(struct vfio_group *, struct kvm *);
> 
>         fn = symbol_get(vfio_group_set_kvm);
>         if (!fn)
>                 return;
> 
>         fn(group, kvm);
> 
>         symbol_put(vfio_group_set_kvm);
> }
You're equating the mechanism with the architecture.  We use symbols
here to avoid module dependencies between kvm and vfio, but this is
just propagating data that userspace is specifically registering
between kvm and vfio.  vhost doesn't get to piggyback on that channel.
> With this patch, I am not suggesting that this is a precedent that should
be followed
> but it appears there doesn't seem to be an alternative way of getting a
copy of the KVM
> pointer that is clean and elegant -- unless I have not looked hard enough.
I guess we
> could create a notifier chain with callbacks for VFIO and Vhost that KVM
would call
> but this would mean modifying KVM.
> 
> Also, if I understand correctly, if VFIO does not want to share the KVM
pointer with
> VFIO groups, then I think it would break stuff like mdev which counts on
it.
Only kvmgt requires the kvm pointer and the use case there is pretty
questionable, I wonder if it actually still exists now that we have the
DMA r/w interface through vfio.  Thanks,

Alex

> From: Alex Williamson
> Sent: Wednesday, January 20, 2021 8:51 AM
> 
> On Wed, 20 Jan 2021 00:14:49 +0000
> "Kasireddy, Vivek" <vivek.kasireddy at intel.com> wrote:
> 
> > Hi Alex,
> >
> > > -----Original Message-----
> > > From: Alex Williamson <alex.williamson at redhat.com>
> > > Sent: Tuesday, January 19, 2021 7:40 AM
> > > To: Kasireddy, Vivek <vivek.kasireddy at intel.com>
> > > Cc: virtualization at lists.linux-foundation.org; Kim, Dongwon
> <dongwon.kim at intel.com>
> > > Subject: Re: [RFC 3/3] vfio: Share the KVM instance with Vdmabuf
> > >
> > > On Tue, 19 Jan 2021 00:28:12 -0800
> > > Vivek Kasireddy <vivek.kasireddy at intel.com> wrote:
> > >
> > > > Getting a copy of the KVM instance is necessary for mapping
Guest
> > > > pages in the Host.
> > > >
> > > > TODO: Instead of invoking the symbol directly, there needs
to be a
> > > > better way of getting a copy of the KVM instance probably by
using
> > > > other notifiers. However, currently, KVM shares its instance
only
> > > > with VFIO and therefore we are compelled to bind the
passthrough'd
> > > > device to vfio-pci.
> > >
> > > Yeah, this is a bad solution, sorry, vfio is not going to
gratuitously
> > > call out to vhost to share a kvm pointer.  I'd prefer to get
rid of
> > > vfio having any knowledge or visibility of the kvm pointer. 
Thanks,
> >
> > [Kasireddy, Vivek] I agree that this is definitely not ideal as I
recognize it
> > in the TODO. However, it looks like VFIO also gets a copy of the KVM
> > pointer in a similar manner:
> >
> > virt/kvm/vfio.c
> >
> > static void kvm_vfio_group_set_kvm(struct vfio_group *group, struct
kvm
> *kvm)
> > {
> >         void (*fn)(struct vfio_group *, struct kvm *);
> >
> >         fn = symbol_get(vfio_group_set_kvm);
> >         if (!fn)
> >                 return;
> >
> >         fn(group, kvm);
> >
> >         symbol_put(vfio_group_set_kvm);
> > }
> 
> You're equating the mechanism with the architecture.  We use symbols
> here to avoid module dependencies between kvm and vfio, but this is
> just propagating data that userspace is specifically registering
> between kvm and vfio.  vhost doesn't get to piggyback on that channel.
> 
> > With this patch, I am not suggesting that this is a precedent that
should be
> followed
> > but it appears there doesn't seem to be an alternative way of
getting a copy
> of the KVM
> > pointer that is clean and elegant -- unless I have not looked hard
enough. I
> guess we
> > could create a notifier chain with callbacks for VFIO and Vhost that
KVM
> would call
> > but this would mean modifying KVM.
> >
> > Also, if I understand correctly, if VFIO does not want to share the
KVM
> pointer with
> > VFIO groups, then I think it would break stuff like mdev which counts
on it.
> 
> Only kvmgt requires the kvm pointer and the use case there is pretty
> questionable, I wonder if it actually still exists now that we have the
> DMA r/w interface through vfio.  Thanks,
> 
IIRC, kvmgt still needs the kvm pointer to use kvm page tracking interface 
for write-protecting guest pgtable.

Thanks
Kevin