On Wed, 2014-05-28 at 13:37 +0200, Alexander Graf wrote:

On 28.05.14 02:57, Alex Williamson wrote:

quoted

On Wed, 2014-05-28 at 02:44 +0200, Alexander Graf wrote:

quoted

On 28.05.14 02:39, Alex Williamson wrote:

quoted

On Wed, 2014-05-28 at 00:49 +0200, Alexander Graf wrote:

quoted

On 27.05.14 20:15, Alex Williamson wrote:

quoted

On Tue, 2014-05-27 at 18:40 +1000, Gavin Shan wrote:

quoted

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 <redacted>
---
    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

[...]

quoted

quoted

+
+	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 ;).

Well, I appreciate that, but having them as separate ioctls doesn't
really prevent that either.  Any one of these 4 could be set to take a
sub-option to extend and contort the EEH interface.  The only way to
prevent that would be to avoid the argsz+flags hack that make the ioctl
extendable.  Thanks,

Sure, that's what patch review is about. I'm really more concerned about 
whose court the number space is in - you or Gavin. If we're talking 
about top level ioctls you will care a lot more.

But I'm not religious about this. You're the VFIO maintainer, so it's 
your call. I just personally cringe when I see an ioctl that gets an 
"opcode" and a "parameter" argument where the "parameter" argument is a 
union with one struct for each opcode.

Well, what would it look like...

struct vfio_eeh_pe_op {
	__u32 argsz;
	__u32 flags;
	__u32 op;
};

Couldn't every single one of these be a separate "op"?  Are there any
cases where we can't use the ioctl return value?

VFIO_EEH_PE_DISABLE
VFIO_EEH_PE_ENABLE
VFIO_EEH_PE_UNFREEZE_IO
VFIO_EEH_PE_UNFREEZE_DMA
VFIO_EEH_PE_GET_MODE
VFIO_EEH_PE_RESET_DEACTIVATE
VFIO_EEH_PE_RESET_HOT
VFIO_EEH_PE_RESET_FUNDAMENTAL
VFIO_EEH_PE_CONFIGURE

It doesn't look that bad to me, what am I missing?  Thanks,

Alex