On Wed, 2014-05-28 at 02:44 +0200, Alexander Graf wrote:
> 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@xxxxxxxxxxxxxxxxxx>
> >>>> ---
> >>>> 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 ;).
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,
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
