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

[Alexander Graf <agraf@suse.de>]

On 28.05.14 02:39, Alex Williamson wrote:
> 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,

Yes, that's the point. I by far prefer to have you push back on anyone 
who introduces useless ioctls rather than have a separate EEH number 
space that people can just throw anything in they like ;).


Alex