On Mon, Sep 21, 2015 at 11:42:28AM +1000, David Gibson wrote: >On Sat, Sep 19, 2015 at 04:22:47PM +1000, David Gibson wrote: >> On Fri, Sep 18, 2015 at 09:47:32AM -0600, Alex Williamson wrote: >> > On Fri, 2015-09-18 at 16:24 +1000, Gavin Shan wrote: >> > > This allows to accept IOMMU group (PE) ID from the parameter from userland >> > > when handling EEH operation so that the operation only affects the target >> > > IOMMU group (PE). If the IOMMU group (PE) ID in the parameter from userland >> > > is invalid, all IOMMU groups (PEs) attached to the specified container are >> > > affected as before. >> > > >> > > Gavin Shan (2): >> > > drivers/vfio: Support EEH API revision >> > > drivers/vfio: Support IOMMU group for EEH operations >> > > >> > > drivers/vfio/vfio_iommu_spapr_tce.c | 50 ++++++++++++++++++++++++++++++++++--- >> > > drivers/vfio/vfio_spapr_eeh.c | 46 ++++++++++++++++++++++------------ >> > > include/linux/vfio.h | 13 +++++++--- >> > > include/uapi/linux/vfio.h | 6 +++++ >> > > 4 files changed, 93 insertions(+), 22 deletions(-) >> > >> > This interface is terrible. A function named foo_enabled() should >> > return a bool, yes or no, don't try to overload it to also return a >> > version. >> >> Sorry, that one's my fault. I suggested that approach to Gavin >> without really thinking it through. >> >> >> > AFAICT, patch 2/2 breaks current users by changing the offset >> > of the union in struct vfio_eeh_pe_err. >> >> Yeah, this one's ugly. We have to preserve the offset, but that means >> putting the group in a very awkward place. Especially since I'm not >> sure if there even are any existing users of the single extant union >> branch. >> >> Sigh. >> >> > Also, we generally pass group >> > file descriptors rather than a group ID because we can prove the >> > ownership of the group through the file descriptor and we don't need to >> > worry about races with the group because we can hold a reference to it. > >Duh. I finally realised the better, simpler, obvious solution. > >Rather than changing the parameter structure, we should move the >ioctl()s so they're on the group fd instead of the container fd. > >Obviously we need to keep it on the container fd for backwards compat, >but I think we should just error out if there is more than one group >in the container there. > >We will need a new capability too, obviously. VFIO_EEH_GROUPFD maybe? > Yeah, the patches should be marked as "RFC" actually as they're actually prototypes. I agree with David that the EEH ioctl commands should be routed through IOMMU group as I proposed long time ago. However, if we're going to do it now, we have to maintain two set the interfaces: one handled by container's ioctl() and another one is handled by IOMMU group's ioctl(). Would it be a problem? Actually, the code change is made based on the fact: nobody is using the union (struct vfio_eeh_pe_err) yet before the QEMU changes to do error injection gets merged by David. So I think it's fine to introduce another field in struct vfio_eeh_pe_op though there is gap? Thanks, Gavin -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
