| Message ID | 20200224084641.31696-1-yan.y.zhao@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | use vfio_dma_rw to read/write IOVAs from CPU side | expand |
On Mon, 24 Feb 2020 03:46:41 -0500 Yan Zhao <yan.y.zhao@intel.com> 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@redhat.com> > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> > --- > 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. > + > +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. > + > + 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, Alex > extern bool vfio_external_group_match_file(struct vfio_group *group, > struct file *filep); > extern int vfio_external_user_iommu_id(struct vfio_group *group);
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@intel.com> 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@redhat.com> > > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> > > --- > > 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? > > + > > +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); Thanks Yan > > > extern bool vfio_external_group_match_file(struct vfio_group *group, > > struct file *filep); > > extern int vfio_external_user_iommu_id(struct vfio_group *group); >
On Mon, 24 Feb 2020 22:35:42 -0500 Yan Zhao <yan.y.zhao@intel.com> 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@intel.com> 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@redhat.com> > > > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> > > > --- > > > 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
On Fri, Mar 06, 2020 at 03:01:49AM +0800, Alex Williamson wrote: > On Mon, 24 Feb 2020 22:35:42 -0500 > Yan Zhao <yan.y.zhao@intel.com> 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@intel.com> 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@redhat.com> > > > > Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> > > > > --- > > > > 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). ok, thanks for this explanation! > > > > + > > > > +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, > Got it! Thanks! Yan
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. + */ + +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); + + 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); extern bool vfio_external_group_match_file(struct vfio_group *group, struct file *filep); extern int vfio_external_user_iommu_id(struct vfio_group *group);
external user is able to 1. add a device into an vfio group 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@redhat.com> Signed-off-by: Yan Zhao <yan.y.zhao@intel.com> --- drivers/vfio/vfio.c | 37 +++++++++++++++++++++++++++++++++++++ include/linux/vfio.h | 2 ++ 2 files changed, 39 insertions(+)