On Tue, Mar 02, 2021 at 09:13:19AM -0800, Jacob Pan wrote:
> Hi Jason,
>
> On Tue, 2 Mar 2021 08:56:28 -0400, Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
>
> > On Wed, Mar 03, 2021 at 04:35:39AM +0800, Liu Yi L wrote:
> > >
> > > +static int vfio_dev_bind_gpasid_fn(struct device *dev, void *data)
> > > +{
> > > + struct domain_capsule *dc = (struct domain_capsule *)data;
> > > + unsigned long arg = *(unsigned long *)dc->data;
> > > +
> > > + return iommu_uapi_sva_bind_gpasid(dc->domain, dev,
> > > + (void __user *)arg);
> >
> > This arg buisness is really tortured. The type should be set at the
> > ioctl, not constantly passed down as unsigned long or worse void *.
> >
> > And why is this passing a __user pointer deep into an iommu_* API??
> >
> The idea was that IOMMU UAPI (not API) is independent of VFIO or other user
> driver frameworks. The design is documented here:
> Documentation/userspace-api/iommu.rst
> IOMMU UAPI handles the type and sanitation of user provided data.
Why? If it is uapi it has defined types and those types should be
completely clear from the C code, not obfuscated.
I haven't looked at the design doc yet, but this is a just a big red
flag, you shouldn't be tunneling one subsytems uAPI through another
subsystem.
If you need to hook two subsystems together it should be more
directly, like VFIO takes in the IOMMU FD and 'registers' itself in
some way with the IOMMU then you can do the IOMMU actions through the
IOMMU FD and it can call back to VFIO as needed.
At least in this way we can swap VFIO for other things in the API.
Having every subsystem that wants to implement IOMMU also implement
tunneled ops seems very backwards.
> Could you be more specific about your concerns?
Avoid using unsigned long, void * and flex arrays to describe
concretely typed things.
Jason
