Re: [PATCH 0/2] VFIO: Accept IOMMU group (PE) ID

On Mon, 2015-09-21 at 22:11 +1000, Gavin Shan wrote:

On Mon, Sep 21, 2015 at 11:42:28AM +1000, David Gibson wrote:

quoted

On Sat, Sep 19, 2015 at 04:22:47PM +1000, David Gibson wrote:

quoted

On Fri, Sep 18, 2015 at 09:47:32AM -0600, Alex Williamson wrote:

quoted

On Fri, 2015-09-18 at 16:24 +1000, Gavin Shan wrote:

quoted

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.

quoted

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.

quoted

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?

We really need to get away from this mindset of assuming that we know
every user of the code and every dependency it may have.  The reality is
that this is an exposed ABI and we shouldn't break it just because we
don't know of any users.  Thanks,

Alex