On Thu, May 29, 2014 at 01:38:46AM +0200, Alexander Graf wrote:
>On 29.05.14 01:37, Gavin Shan wrote:
>>On Thu, May 29, 2014 at 12:40:26AM +0200, Alexander Graf wrote:
>>>On 28.05.14 18:17, Alex Williamson wrote:
>>>>On Wed, 2014-05-28 at 13:37 +0200, Alexander Graf wrote:
>>>>>On 28.05.14 02:57, Alex Williamson wrote:
>>>>>>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,
>>>>>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,
>>>Yup, that looks well to me as well :)
>>>
>>s/VFIO_EEH_PE_GET_MODE/VFIO_EEH_PE_GET_STATE.
>>
>>I'll include this in next revision. Thanks, Alex.
>
>Yup, no need for CMD anymore then either :)
>
Yep. Thanks, Guys :)
Thanks,
Gavin
--
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
