Re: [PATCH V3 7/9] vfio: iommu driver notify callback

[Cornelia Huck <cohuck@redhat.com>]

On Sat, 30 Jan 2021 11:51:41 -0500
Steven Sistare <steven.sistare@oracle.com> wrote:

> On 1/29/2021 4:57 PM, Alex Williamson wrote:
> > On Fri, 29 Jan 2021 08:54:10 -0800
> > Steve Sistare <steven.sistare@oracle.com> wrote:
> >   
> >> Define a vfio_iommu_driver_ops notify callback, for sending events to
> >> the driver.  Drivers are not required to provide the callback, and
> >> may ignore any events.  The handling of events is driver specific.
> >>
> >> Define the CONTAINER_CLOSE event, called when the container's file
> >> descriptor is closed.  This event signifies that no further state changes
> >> will occur via container ioctl's.
> >>
> >> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>
> >> ---
> >>  drivers/vfio/vfio.c  | 5 +++++
> >>  include/linux/vfio.h | 5 +++++
> >>  2 files changed, 10 insertions(+)
> >>
> >> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> >> index 262ab0e..99458fc 100644
> >> --- a/drivers/vfio/vfio.c
> >> +++ b/drivers/vfio/vfio.c
> >> @@ -1220,6 +1220,11 @@ static int vfio_fops_open(struct inode *inode, struct file *filep)
> >>  static int vfio_fops_release(struct inode *inode, struct file *filep)
> >>  {
> >>  	struct vfio_container *container = filep->private_data;
> >> +	struct vfio_iommu_driver *driver = container->iommu_driver;
> >> +
> >> +	if (driver && driver->ops->notify)
> >> +		driver->ops->notify(container->iommu_data,
> >> +				    VFIO_DRIVER_NOTIFY_CONTAINER_CLOSE, NULL);
> >>  
> >>  	filep->private_data = NULL;
> >>  
> >> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> >> index 38d3c6a..9642579 100644
> >> --- a/include/linux/vfio.h
> >> +++ b/include/linux/vfio.h
> >> @@ -57,6 +57,9 @@ extern int vfio_add_group_dev(struct device *dev,
> >>  extern void vfio_device_put(struct vfio_device *device);
> >>  extern void *vfio_device_data(struct vfio_device *device);
> >>  
> >> +/* events for the backend driver notify callback */
> >> +#define VFIO_DRIVER_NOTIFY_CONTAINER_CLOSE	1  
> > 
> > We should use an enum for type checking.  
> 
> Agreed.
> I see you changed the value to 0.  Do you want to reserve 0 for invalid-event?
> (I know, this is internal and can be changed).  Your call.

I'm not sure what we would use an invalid-event event for... the type
checking provided by the enum should be enough?

> 
> >> +
> >>  /**
> >>   * struct vfio_iommu_driver_ops - VFIO IOMMU driver callbacks
> >>   */
> >> @@ -90,6 +93,8 @@ struct vfio_iommu_driver_ops {
> >>  					       struct notifier_block *nb);
> >>  	int		(*dma_rw)(void *iommu_data, dma_addr_t user_iova,
> >>  				  void *data, size_t count, bool write);
> >> +	void		(*notify)(void *iommu_data, unsigned int event,
> >> +				  void *data);  
> > 
> > I'm not sure why we're pre-enabling this 3rd arg, do you have some
> > short term usage in mind that we can't easily add on demand later?
> > This is an internal API, not one we need to keep stable.  Thanks,  
> 
> No short term need, just seems sensible for an event callback.  I was mimic'ing the 
> signature of the callback for vfio_register_notifier. Your call.

I'd drop *data for now, if we don't have a concrete use case.

> 
> - Steve
> 
> > 
> > Alex
> >   
> >>  };
> >>  
> >>  extern int vfio_register_iommu_driver(const struct vfio_iommu_driver_ops *ops);  
> >   
>