On Mon, 24 Feb 2020 22:35:42 -0500
Yan Zhao <yan.y.zhao@xxxxxxxxx> wrote:
> On Tue, Feb 25, 2020 at 03:15:04AM +0800, Alex Williamson wrote:
> > On Mon, 24 Feb 2020 03:46:41 -0500
> > Yan Zhao <yan.y.zhao@xxxxxxxxx> wrote:
> >
> > > external user is able to
> > > 1. add a device into an vfio group
> >
> > How so? The device is added via existing mechanisms, the only thing
> > added here is an interface to get a group reference from a struct
> > device.
> >
> > > 2. call vfio_group_get_external_user_from_dev() with the device pointer
> > > to get vfio_group associated with this device and increments the container
> > > user counter to prevent the VFIO group from disposal before KVM exits.
> > > 3. When the external KVM finishes, it calls vfio_group_put_external_user()
> > > to release the VFIO group.
> > >
> > > Suggested-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > > Signed-off-by: Yan Zhao <yan.y.zhao@xxxxxxxxx>
> > > ---
> > > drivers/vfio/vfio.c | 37 +++++++++++++++++++++++++++++++++++++
> > > include/linux/vfio.h | 2 ++
> > > 2 files changed, 39 insertions(+)
> > >
> > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c
> > > index c8482624ca34..914bdf4b9d73 100644
> > > --- a/drivers/vfio/vfio.c
> > > +++ b/drivers/vfio/vfio.c
> > > @@ -1720,6 +1720,43 @@ struct vfio_group *vfio_group_get_external_user(struct file *filep)
> > > }
> > > EXPORT_SYMBOL_GPL(vfio_group_get_external_user);
> > >
> > > +/**
> > > + * External user API, exported by symbols to be linked dynamically.
> > > + *
> > > + * The protocol includes:
> > > + * 1. External user add a device into a vfio group
> > > + *
> > > + * 2. The external user calls vfio_group_get_external_user_from_dev()
> > > + * with the device pointer
> > > + * to verify that:
> > > + * - there's a vfio group associated with it and is initialized;
> > > + * - IOMMU is set for the vfio group.
> > > + * If both checks passed, vfio_group_get_external_user_from_dev()
> > > + * increments the container user counter to prevent
> > > + * the VFIO group from disposal before KVM exits.
> > > + *
> > > + * 3. When the external KVM finishes, it calls
> > > + * vfio_group_put_external_user() to release the VFIO group.
> > > + * This call decrements the container user counter.
> > > + */
> >
> > I don't think we need to duplicate this whole comment block for a
> > _from_dev() version of the existing vfio_group_get_external_user().
> > Please merge the comments.
> ok. but I have a question: for an external user, as it already has group
> fd, it can use vfio_group_get_external_user() directly, is there a
> necessity for it to call vfio_group_get_external_user_from_dev() ?
>
> If an external user wants to call this interface, it needs to first get
> device fd, passes the device fd to kernel and kernel retrieves the pointer
> to struct device, right?
If you have the fd already, then yeah, let's not add a _from_dev()
version, but how would an mdev vendor driver have the fd? IIRC, the
existing interface is designed this way to allow the user to prove
ownership, whereas using a _from_dev() interface would be for trusted
parts of the kernel, where we can theoretically trust that code isn't
simply locating a device in order to perform malicious actions in the
user (because they'd have more direct ways than this to be malicious to
the user already).
> > > +
> > > +struct vfio_group *vfio_group_get_external_user_from_dev(struct device *dev)
> > > +{
> > > + struct vfio_group *group;
> > > + int ret;
> > > +
> > > + group = vfio_group_get_from_dev(dev);
> > > + if (!group)
> > > + return ERR_PTR(-ENODEV);
> > > +
> > > + ret = vfio_group_add_container_user(group);
> > > + if (ret)
> > > + return ERR_PTR(ret);
> >
> > Error path leaks group reference.
> >
> oops, sorry for that.
>
> > > +
> > > + return group;
> > > +}
> > > +EXPORT_SYMBOL_GPL(vfio_group_get_external_user_from_dev);
> > > +
> > > void vfio_group_put_external_user(struct vfio_group *group)
> > > {
> > > vfio_group_try_dissolve_container(group);
> > > diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> > > index e42a711a2800..2e1fa0c7396f 100644
> > > --- a/include/linux/vfio.h
> > > +++ b/include/linux/vfio.h
> > > @@ -94,6 +94,8 @@ extern void vfio_unregister_iommu_driver(
> > > */
> > > extern struct vfio_group *vfio_group_get_external_user(struct file *filep);
> > > extern void vfio_group_put_external_user(struct vfio_group *group);
> > > +extern
> > > +struct vfio_group *vfio_group_get_external_user_from_dev(struct device *dev);
> >
> > Slight cringe at this line wrap, personally would prefer to wrap the
> > args as done repeatedly elsewhere in this file. Thanks,
> >
> yeah, I tried to do in that way, but the name of this interface is too long,
> as well as its return type, it passes 80 characters limit even with just one
> arg...
>
> is it better to wrap in below way?
> extern struct vfio_group *vfio_group_get_external_user_from_dev(struct device
> *dev);
>
> or just a shorter interface name?
> extern struct vfio_group *vfio_group_get_user_from_dev(struct device *dev);
I'd probably tend towards the former, keeping "external" in the name
makes it clear that it belongs to a certain class of functions with
similar conventions. Thanks,
Alex
