Re: [RFC v3 06/21] vfio: VFIO_IOMMU_BIND_MSI

[Alex Williamson]

On Fri, 11 Jan 2019 16:02:44 -0700
Alex Williamson <alex.williamson@xxxxxxxxxx> wrote:

> On Tue,  8 Jan 2019 11:26:18 +0100
> Eric Auger <eric.auger@xxxxxxxxxx> wrote:
> 
> > This patch adds the VFIO_IOMMU_BIND_MSI ioctl which aims at
> > passing the guest MSI binding to the host.
> > 
> > Signed-off-by: Eric Auger <eric.auger@xxxxxxxxxx>
> > 
> > ---
> > 
> > v2 -> v3:
> > - adapt to new proto of bind_guest_msi
> > - directly use vfio_iommu_for_each_dev
> > 
> > v1 -> v2:
> > - s/vfio_iommu_type1_guest_msi_binding/vfio_iommu_type1_bind_guest_msi
> > ---
> >  drivers/vfio/vfio_iommu_type1.c | 27 +++++++++++++++++++++++++++
> >  include/uapi/linux/vfio.h       |  7 +++++++
> >  2 files changed, 34 insertions(+)
> > 
> > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> > index c3ba3f249438..59229f6e2d84 100644
> > --- a/drivers/vfio/vfio_iommu_type1.c
> > +++ b/drivers/vfio/vfio_iommu_type1.c
> > @@ -1673,6 +1673,15 @@ static int vfio_cache_inv_fn(struct device *dev, void *data)
> >  	return iommu_cache_invalidate(d, dev, &ustruct->info);
> >  }
> >  
> > +static int vfio_bind_guest_msi_fn(struct device *dev, void *data)
> > +{
> > +	struct vfio_iommu_type1_bind_guest_msi *ustruct =
> > +		(struct vfio_iommu_type1_bind_guest_msi *)data;
> > +	struct iommu_domain *d = iommu_get_domain_for_dev(dev);
> > +
> > +	return iommu_bind_guest_msi(d, dev, &ustruct->binding);
> > +}
> > +
> >  static int
> >  vfio_set_pasid_table(struct vfio_iommu *iommu,
> >  		      struct vfio_iommu_type1_set_pasid_table *ustruct)
> > @@ -1792,6 +1801,24 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
> >  					      vfio_cache_inv_fn);
> >  		mutex_unlock(&iommu->lock);
> >  		return ret;
> > +	} else if (cmd == VFIO_IOMMU_BIND_MSI) {
> > +		struct vfio_iommu_type1_bind_guest_msi ustruct;
> > +		int ret;
> > +
> > +		minsz = offsetofend(struct vfio_iommu_type1_bind_guest_msi,
> > +				    binding);
> > +
> > +		if (copy_from_user(&ustruct, (void __user *)arg, minsz))
> > +			return -EFAULT;
> > +
> > +		if (ustruct.argsz < minsz || ustruct.flags)
> > +			return -EINVAL;
> > +
> > +		mutex_lock(&iommu->lock);
> > +		ret = vfio_iommu_for_each_dev(iommu, &ustruct,
> > +					      vfio_bind_guest_msi_fn);  
> 
> The vfio_iommu_for_each_dev() interface is fine for invalidation, where
> a partial failure requires no unwind, but it's not sufficiently robust
> here.

Additionally, what happens as devices are added and removed from the
guest?  Are we designing an interface that specifically precludes
hotplug?  Thanks,

Alex
 
> > +		mutex_unlock(&iommu->lock);
> > +		return ret;
> >  	}
> >  
> >  	return -ENOTTY;
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index 11a07165e7e1..352e795a93c8 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -774,6 +774,13 @@ struct vfio_iommu_type1_cache_invalidate {
> >  };
> >  #define VFIO_IOMMU_CACHE_INVALIDATE      _IO(VFIO_TYPE, VFIO_BASE + 23)
> >  
> > +struct vfio_iommu_type1_bind_guest_msi {
> > +	__u32   argsz;
> > +	__u32   flags;
> > +	struct iommu_guest_msi_binding binding;
> > +};
> > +#define VFIO_IOMMU_BIND_MSI      _IO(VFIO_TYPE, VFIO_BASE + 24)  
> 
> -ENOCOMMENTS  MSIs are setup and torn down, is this only a machine init
> sort of interface?  How does the user un-bind?  Thanks,
> 
> Alex
> 
> > +
> >  /* -------- Additional API for SPAPR TCE (Server POWERPC) IOMMU -------- */
> >  
> >  /*  
>