Re: [v3 4/5] vfio: implement APIs to set/put kvm to/from vfio group

[Alex Williamson <alex.williamson@redhat.com>]

On Mon, 14 Nov 2016 18:19:34 +0800
Jike Song <jike.song@intel.com> wrote:

> On 11/10/2016 01:53 AM, Alex Williamson wrote:
> > On Wed, 09 Nov 2016 20:49:32 +0800
> > Jike Song <jike.song@intel.com> wrote:
> >   
> >> On 11/08/2016 04:45 AM, Paolo Bonzini wrote:  
> >>> On 07/11/2016 19:28, Alex Williamson wrote:    
> >>>>>> Can the reference become invalid?      
> >>>>>
> >>>>> No, this is guaranteed by virt/kvm/vfio.c + the udata.lock mutex (which
> >>>>> probably should be renamed...).    
> >>>>
> >>>> The caller gets a reference to kvm, but there's no guarantee that the
> >>>> association of that kvm reference to the group stays valid.  Once we're
> >>>> outside of that mutex, we might as well consider that kvm:group
> >>>> association stale.
> >>>>      
> >>>>>> The caller may still hold
> >>>>>> a kvm references, but couldn't the group be detached from one kvm
> >>>>>> instance and re-attached to another?      
> >>>>>
> >>>>> Can this be handled by the vendor driver?  Does it get a callback when
> >>>>> it's detached from a KVM instance?    
> >>>>
> >>>> The only release callback through vfio is when the user closes the
> >>>> device, the code in this series is the full extent of vfio awareness of
> >>>> kvm.  Thanks,    
> >>>
> >>> Maybe there should be an mdev callback at the point of association and
> >>> deassociation between VFIO and KVM.  Then the vendor driver can just use
> >>> the same mutex for association, deassociation and usage.  I'm not even
> >>> sure that these patches are necessary once you have that callback.    
> >>
> >> Hi Alex & Paolo,
> >>
> >> So I cooked another draft version of this, there is no kvm pointer saved
> >> in vfio_group in this version, and notifier will be called on attach/detach,
> >> please kindly have a look :-)
> >>
> >>
> >> --
> >> Thanks,
> >> Jike
> >>
> >>
> >> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> >> index ed2361e4..20b5da9 100644
> >> --- a/drivers/vfio/vfio.c
> >> +++ b/drivers/vfio/vfio.c
> >> @@ -34,6 +34,7 @@
> >>  #include <linux/uaccess.h>
> >>  #include <linux/vfio.h>
> >>  #include <linux/wait.h>
> >> +#include <linux/kvm_host.h>
> >>  
> >>  #define DRIVER_VERSION	"0.3"
> >>  #define DRIVER_AUTHOR	"Alex Williamson <alex.williamson@redhat.com>"
> >> @@ -86,6 +87,10 @@ struct vfio_group {
> >>  	struct mutex			unbound_lock;
> >>  	atomic_t			opened;
> >>  	bool				noiommu;
> >> +	struct {
> >> +		struct mutex lock;
> >> +		struct blocking_notifier_head notifier;
> >> +	} udata;
> >>  };
> >>  
> >>  struct vfio_device {
> >> @@ -333,6 +338,7 @@ static struct vfio_group *vfio_create_group(struct iommu_group *iommu_group)
> >>  	mutex_init(&group->device_lock);
> >>  	INIT_LIST_HEAD(&group->unbound_list);
> >>  	mutex_init(&group->unbound_lock);
> >> +	mutex_init(&group->udata.lock);
> >>  	atomic_set(&group->container_users, 0);
> >>  	atomic_set(&group->opened, 0);
> >>  	group->iommu_group = iommu_group;
> >> @@ -414,10 +420,11 @@ static void vfio_group_release(struct kref *kref)
> >>  	iommu_group_put(iommu_group);
> >>  }
> >>  
> >> -static void vfio_group_put(struct vfio_group *group)
> >> +void vfio_group_put(struct vfio_group *group)
> >>  {
> >>  	kref_put_mutex(&group->kref, vfio_group_release, &vfio.group_lock);
> >>  }
> >> +EXPORT_SYMBOL_GPL(vfio_group_put);
> >>  
> >>  /* Assume group_lock or group reference is held */
> >>  static void vfio_group_get(struct vfio_group *group)
> >> @@ -480,7 +487,7 @@ static struct vfio_group *vfio_group_get_from_minor(int minor)
> >>  	return group;
> >>  }
> >>  
> >> -static struct vfio_group *vfio_group_get_from_dev(struct device *dev)
> >> +struct vfio_group *vfio_group_get_from_dev(struct device *dev)
> >>  {
> >>  	struct iommu_group *iommu_group;
> >>  	struct vfio_group *group;
> >> @@ -494,6 +501,7 @@ static struct vfio_group *vfio_group_get_from_dev(struct device *dev)
> >>  
> >>  	return group;
> >>  }
> >> +EXPORT_SYMBOL_GPL(vfio_group_get_from_dev);
> >>  
> >>  /**
> >>   * Device objects - create, release, get, put, search
> >> @@ -1745,6 +1753,44 @@ long vfio_external_check_extension(struct vfio_group *group, unsigned long arg)
> >>  }
> >>  EXPORT_SYMBOL_GPL(vfio_external_check_extension);
> >>  
> >> +int vfio_group_register_notifier(struct vfio_group *group, struct notifier_block *nb)
> >> +{
> >> +	return blocking_notifier_chain_register(&group->udata.notifier, nb);
> >> +}
> >> +EXPORT_SYMBOL_GPL(vfio_group_register_notifier);
> >> +
> >> +int vfio_group_unregister_notifier(struct vfio_group *group, struct notifier_block *nb)
> >> +{
> >> +	return blocking_notifier_chain_unregister(&group->udata.notifier, nb);
> >> +}
> >> +EXPORT_SYMBOL_GPL(vfio_group_unregister_notifier);  
> > 
> > Kirti is already adding vfio_register_notifier &
> > vfio_unregister_notifier, these are not exclusive to the iommu, I
> > clarified that in my question that IOVA range invalidation is just one
> > aspect of what that notifier might be used for.  The mdev framework
> > also automatically registers and unregisters that notifier around
> > open/release.  So, I don't think we want a new notifier, we just want
> > vfio.c to also consume that notifier.
> >   
> 
> Hi Alex,
> 
> Sorry, I have one more question: does combining Kirti's iommu notifier
> and my group notifier mean there should only one blocking_notifier_head?
> If so, where should it be? vfio_container, vfio_group or vfio_iommu?

I suspect the most straightforward approach is to place a
blocking_notifier_head on the vfio_group in addition to the one that
Kirti has placed on the vfio_iommu.  Both will include the same
notifier_block from the vendor driver and call the notifier chain
independently.  Thanks,

Alex