Date: Tue, 15 Nov 2016 08:19:08 -0700
From: Alex Williamson <alex.williamson@redhat.com>
To: Jike Song <jike.song@intel.com>
Cc: Kirti Wankhede <kwankhede@nvidia.com>, pbonzini@redhat.com,
        kraxel@redhat.com, cjia@nvidia.com, qemu-devel@nongnu.org,
        kvm@vger.kernel.org, kevin.tian@intel.com, bjsdjshi@linux.vnet.ibm.com,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v12 12/22] vfio: Add notifier callback to parent's ops
 structure of mdev
Message-ID: <20161115081908.070b10ae@t450s.home>
In-Reply-To: <582AAF16.5080906@intel.com>
References: <1479138156-28905-1-git-send-email-kwankhede@nvidia.com>
        <1479138156-28905-13-git-send-email-kwankhede@nvidia.com>
        <582AAF16.5080906@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4676
Lines: 123

On Tue, 15 Nov 2016 14:45:42 +0800
Jike Song <jike.song@intel.com> wrote:

> On 11/14/2016 11:42 PM, Kirti Wankhede wrote:
> > Add a notifier calback to parent's ops structure of mdev device so that per
> > device notifer for vfio module is registered through vfio_mdev module.
> > 
> > Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> > Signed-off-by: Neo Jia <cjia@nvidia.com>
> > Change-Id: Iafa6f1721aecdd6e50eb93b153b5621e6d29b637
> > ---
> >  drivers/vfio/mdev/vfio_mdev.c | 19 +++++++++++++++++++
> >  include/linux/mdev.h          |  9 +++++++++
> >  2 files changed, 28 insertions(+)
> > 
> > diff --git a/drivers/vfio/mdev/vfio_mdev.c b/drivers/vfio/mdev/vfio_mdev.c
> > index ffc36758cb84..1694b1635607 100644
> > --- a/drivers/vfio/mdev/vfio_mdev.c
> > +++ b/drivers/vfio/mdev/vfio_mdev.c
> > @@ -24,6 +24,15 @@
> >  #define DRIVER_AUTHOR   "NVIDIA Corporation"
> >  #define DRIVER_DESC     "VFIO based driver for Mediated device"
> >  
> > +static int vfio_mdev_notifier(struct notifier_block *nb, unsigned long action,
> > +			      void *data)
> > +{
> > +	struct mdev_device *mdev = container_of(nb, struct mdev_device, nb);
> > +	struct parent_device *parent = mdev->parent;
> > +
> > +	return parent->ops->notifier(mdev, action, data);
> > +}
> > +
> >  static int vfio_mdev_open(void *device_data)
> >  {
> >  	struct mdev_device *mdev = device_data;
> > @@ -40,6 +49,11 @@ static int vfio_mdev_open(void *device_data)
> >  	if (ret)
> >  		module_put(THIS_MODULE);
> >  
> > +	if (likely(parent->ops->notifier)) {
> > +		mdev->nb.notifier_call = vfio_mdev_notifier;
> > +		if (vfio_register_notifier(&mdev->dev, &mdev->nb))
> > +			pr_err("Failed to register notifier for mdev\n");
> > +	}  
> 
> Hi Kirti,
> 
> Could you please move the notifier registration before parent->ops->open()?
> as you might know, I'm extending your vfio_register_notifier to also include
> the attaching/detaching events of vfio_group and kvm.  Basically if vfio_group
> not attached to any kvm instance, the parent->ops->open() should return -ENODEV
> to indicate the failure, but to know whether kvm is available in open(), the
> notifier registration should be earlier.

It seems like you're giving general guidance for how a vendor driver
open() function should work, yet a hard dependency on KVM should be
discouraged.  You're making a choice for your vendor driver alone.

I would also be very cautious about the coherency of signaling the KVM
association relative to the user of the group.  Is it possible that the
association of one KVM instance by a user of the group can leak to the
next user?  Does vfio need to seen a gratuitous un-set of the KVM
association on group close()? etc.  Thanks,

Alex
 
> Of course I can call vfio_register_notifier() from an earlier place to
> workaround it, but it doesn't seem a canonical way.
> 
> --
> Thanks,
> Jike
> 
> >  	return ret;
> >  }
> >  
> > @@ -48,6 +62,11 @@ static void vfio_mdev_release(void *device_data)
> >  	struct mdev_device *mdev = device_data;
> >  	struct parent_device *parent = mdev->parent;
> >  
> > +	if (likely(parent->ops->notifier)) {
> > +		if (vfio_unregister_notifier(&mdev->dev, &mdev->nb))
> > +			pr_err("Failed to unregister notifier for mdev\n");
> > +	}
> > +
> >  	if (likely(parent->ops->release))
> >  		parent->ops->release(mdev);
> >  
> > diff --git a/include/linux/mdev.h b/include/linux/mdev.h
> > index 4900cc472364..665afe0a4c31 100644
> > --- a/include/linux/mdev.h
> > +++ b/include/linux/mdev.h
> > @@ -37,6 +37,7 @@ struct mdev_device {
> >  	struct kref		ref;
> >  	struct list_head	next;
> >  	struct kobject		*type_kobj;
> > +	struct notifier_block	nb;
> >  };
> >  
> >  /**
> > @@ -85,6 +86,12 @@ struct mdev_device {
> >   * @mmap:		mmap callback
> >   *			@mdev: mediated device structure
> >   *			@vma: vma structure
> > + * @notifer:		Notifier callback, currently only for
> > + *			VFIO_IOMMU_NOTIFY_DMA_UNMAP action notified duing
> > + *			DMA_UNMAP call on mapped iova range.
> > + *			@mdev: mediated device structure
> > + *			@action: Action for which notifier is called
> > + *			@data: Data associated with the notifier
> >   * Parent device that support mediated device should be registered with mdev
> >   * module with parent_ops structure.
> >   **/
> > @@ -106,6 +113,8 @@ struct parent_ops {
> >  	ssize_t (*ioctl)(struct mdev_device *mdev, unsigned int cmd,
> >  			 unsigned long arg);
> >  	int	(*mmap)(struct mdev_device *mdev, struct vm_area_struct *vma);
> > +	int	(*notifier)(struct mdev_device *mdev, unsigned long action,
> > +			    void *data);
> >  };
> >  
> >  /* interface for exporting mdev supported type attributes */
> >