Re: [PATCH v7 3/3] drivers/vfio: EEH support for VFIO PCI device

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

On Wed, 2014-05-28 at 00:49 +0200, Alexander Graf wrote:
> On 27.05.14 20:15, Alex Williamson wrote:
> > On Tue, 2014-05-27 at 18:40 +1000, Gavin Shan wrote:
> >> The patch adds new IOCTL commands for sPAPR VFIO container device
> >> to support EEH functionality for PCI devices, which have been passed
> >> through from host to somebody else via VFIO.
> >>
> >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com>
> >> ---
> >>   Documentation/vfio.txt              | 92 ++++++++++++++++++++++++++++++++++++-
> >>   drivers/vfio/pci/Makefile           |  1 +
> >>   drivers/vfio/pci/vfio_pci.c         | 20 +++++---
> >>   drivers/vfio/pci/vfio_pci_eeh.c     | 46 +++++++++++++++++++
> >>   drivers/vfio/pci/vfio_pci_private.h |  5 ++
> >>   drivers/vfio/vfio_iommu_spapr_tce.c | 85 ++++++++++++++++++++++++++++++++++
> >>   include/uapi/linux/vfio.h           | 66 ++++++++++++++++++++++++++
> >>   7 files changed, 308 insertions(+), 7 deletions(-)
> >>   create mode 100644 drivers/vfio/pci/vfio_pci_eeh.c
> 
> [...]
> 
> >> +
> >> +	return ret;
> >> +}
> >> +
> >>   static long tce_iommu_ioctl(void *iommu_data,
> >>   				 unsigned int cmd, unsigned long arg)
> >>   {
> >> @@ -283,6 +363,11 @@ static long tce_iommu_ioctl(void *iommu_data,
> >>   		tce_iommu_disable(container);
> >>   		mutex_unlock(&container->lock);
> >>   		return 0;
> >> +	case VFIO_EEH_PE_SET_OPTION:
> >> +	case VFIO_EEH_PE_GET_STATE:
> >> +	case VFIO_EEH_PE_RESET:
> >> +	case VFIO_EEH_PE_CONFIGURE:
> >> +		return tce_iommu_eeh_ioctl(iommu_data, cmd, arg);
> > This is where it would have really made sense to have a single
> > VFIO_EEH_OP ioctl with a data structure passed to indicate the sub-op.
> > AlexG, are you really attached to splitting these out into separate
> > ioctls?
> 
> I don't see the problem. We need to forward 4 ioctls to a separate piece 
> of code, so we forward 4 ioctls to a separate piece of code :). Putting 
> them into one ioctl just moves the switch() into another function.

And uses an extra 3 ioctl numbers and gives us extra things to update if
we ever need to add more ioctls, etc.  ioctl numbers are an address
space, how much address space do we really want to give to EEH?  It's
not a big difference, but I don't think it's completely even either.
Thanks,

Alex