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 <gws...@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 ;).
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html