Re: [PATCH 1/2] VFIO-AER: Vfio-pci driver changes for supporting AER

[Marcelo Tosatti <mtosatti@redhat.com>]

On Fri, Jan 11, 2013 at 08:46:35AM -0700, Alex Williamson wrote:
> On Fri, 2013-01-11 at 08:45 +0000, Pandarathil, Vijaymohan R wrote:
> > 
> > > -----Original Message-----
> > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > Sent: Wednesday, January 09, 2013 11:05 AM
> > > To: Pandarathil, Vijaymohan R
> > > Cc: Bjorn Helgaas; Gleb Natapov; kvm@vger.kernel.org; qemu-
> > > devel@nongnu.org; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org
> > > Subject: Re: [PATCH 1/2] VFIO-AER: Vfio-pci driver changes for supporting
> > > AER
> > > 
> > > On Wed, 2013-01-09 at 10:52 -0700, Alex Williamson wrote:
> > > > On Wed, 2013-01-09 at 06:26 +0000, Pandarathil, Vijaymohan R wrote:
> > > > > 	- New ioctl which is used to pass the eventfd that is signaled when
> > > > >           an error occurs in the vfio_pci_device
> > > > >
> > > > > 	- Register pci_error_handler for the vfio_pci driver
> > > > >
> > > > > 	- When the device encounters an error, the error handler registered
> > > by
> > > > >           the vfio_pci driver gets invoked by the AER infrastructure
> > > > >
> > > > > 	- In the error handler, signal the eventfd registered for the device.
> > > > >
> > > > > 	- This results in the qemu eventfd handler getting invoked and
> > > > >           appropriate action taken for the guest.
> > > > >
> > > > > Signed-off-by: Vijay Mohan Pandarathil <vijaymohan.pandarathil@hp.com>
> > > > > ---
> > > > >  drivers/vfio/pci/vfio_pci.c         | 29 +++++++++++++++++++++++++++++
> > > > >  drivers/vfio/pci/vfio_pci_private.h |  1 +
> > > > >  drivers/vfio/vfio.c                 |  8 ++++++++
> > > > >  include/linux/vfio.h                |  1 +
> > > > >  include/uapi/linux/vfio.h           |  9 +++++++++
> > > > >  5 files changed, 48 insertions(+)
> > > > >
> > > > > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> > > > > index 6c11994..4ae9526 100644
> > > > > --- a/drivers/vfio/pci/vfio_pci.c
> > > > > +++ b/drivers/vfio/pci/vfio_pci.c
> > > > > @@ -207,6 +207,8 @@ static long vfio_pci_ioctl(void *device_data,
> > > > >  		if (vdev->reset_works)
> > > > >  			info.flags |= VFIO_DEVICE_FLAGS_RESET;
> > > > >
> > > > > +		info.flags |= VFIO_DEVICE_FLAGS_AER_NOTIFY;
> > > > > +
> > > >
> > > > This appears to be a PCI specific flag, so the name should include
> > > > _PCI_.  We also support non-PCIe devices and it seems like it would be
> > > > possible to not have AER support available, so shouldn't this be
> > > > conditional?
> > 
> > Will do that.
> > 
> > > >
> > > > >  		info.num_regions = VFIO_PCI_NUM_REGIONS;
> > > > >  		info.num_irqs = VFIO_PCI_NUM_IRQS;
> > > > >
> > > > > @@ -348,6 +350,19 @@ static long vfio_pci_ioctl(void *device_data,
> > > > >
> > > > >  		return ret;
> > > > >
> > > > > +	} else if (cmd == VFIO_DEVICE_SET_ERRFD) {
> > > > > +		int32_t fd = (int32_t)arg;
> > > > > +
> > > > > +		if (fd < 0)
> > > > > +			return -EINVAL;
> > > > > +
> > > > > +		vdev->err_trigger = eventfd_ctx_fdget(fd);
> > > > > +
> > > > > +		if (IS_ERR(vdev->err_trigger))
> > > > > +			return PTR_ERR(vdev->err_trigger);
> > > > > +
> > > > > +		return 0;
> > > > > +
> > > >
> > > > I'm not sure why we wouldn't describe this as just another interrupt
> > > > from the device and configure it via SET_IRQ.  This ioctl has very
> > > > limited use and doesn't follow any of the conventions of all the other
> > > > vfio ioctls.
> > 
> > I thought this was a fairly simple ioctl to implement this way. Moreover, 
> > there is no device interrupt involved. Let me know if you really want to 
> > model it as a SET_IRQ ioctl.
> 
> I'd like to see something else for sure.  We're trying to do AER
> piecemeal but this ioctl leaves no room for anything else.  Seems like a
> soon to be wasted ioctl number.  Note that there isn't even a de-assign
> interface, nor a flag to set to indicate de-assignment.  The SET_IRQ
> ioctl already handles these in a very similar way to how we'd want to
> handle it for error signaling.  It doesn't seem like that much of a
> stretch to me to include it there, but I'd also entertain other ideas.
> Thanks,
> 
> Alex

Yes it would be good to have a picture of the dependencies necessary to
implement passthrough, before introducing an incompatible interface.