| Message ID | 7-v1-33906a626da1+16b0-vfio_kvm_no_group_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Remove vfio_group from the struct file facing VFIO API | expand |
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Friday, April 15, 2022 2:46 AM > > Focus the new op into is_enforced_coherent() which only checks the s/coherent/coherency/ > enforced DMA coherency property of the file. > > Make the new op self contained by properly refcounting the container > before touching it. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/vfio/vfio.c | 27 ++++++++++++++++++++++++--- > include/linux/vfio.h | 3 +-- > virt/kvm/vfio.c | 18 +----------------- > 3 files changed, 26 insertions(+), 22 deletions(-) > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index eb65b4c80ece64..c08093fb6d28d5 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -2003,14 +2003,35 @@ static struct iommu_group > *vfio_file_iommu_group(struct file *filep) > return group->iommu_group; > } > > -long vfio_external_check_extension(struct vfio_group *group, unsigned long > arg) > +/** > + * vfio_file_enforced_coherent - True if the DMA associated with the VFIO > file > + * is always CPU cache coherent > + * @filep: VFIO file > + * > + * Enforced coherent means that the IOMMU ignores things like the PCIe s/coherent/coherency/ > no-snoop > + * bit in DMA transactions. A return of false indicates that the user has > + * rights to access additional instructions such as wbinvd on x86. > + */ > +static bool vfio_file_enforced_coherent(struct file *filep) > { > - return vfio_ioctl_check_extension(group->container, arg); > + struct vfio_group *group = filep->private_data; > + bool ret; > + > + /* > + * Since the coherency state is determined only once a container is > + * attached the user must do so before they can prove they have > + * permission. > + */ > + if (vfio_group_add_container_user(group)) > + return true; I wonder whether it's better to return error here and let KVM to decide whether it wants to allow wbinvd in such case (though likely the conclusion is same) or simply rejects adding the group. Thanks Kevin
On Thu, Apr 14, 2022 at 03:46:06PM -0300, Jason Gunthorpe wrote: > Focus the new op into is_enforced_coherent() which only checks the > enforced DMA coherency property of the file. I have a reall hard time parsing this sentence.
On Fri, Apr 15, 2022 at 04:07:51AM +0000, Tian, Kevin wrote: > > + struct vfio_group *group = filep->private_data; > > + bool ret; > > + > > + /* > > + * Since the coherency state is determined only once a container is > > + * attached the user must do so before they can prove they have > > + * permission. > > + */ > > + if (vfio_group_add_container_user(group)) > > + return true; > > I wonder whether it's better to return error here and let KVM to > decide whether it wants to allow wbinvd in such case (though > likely the conclusion is same) or simply rejects adding the group. Since the new model is to present proof at add it is OK - it just means the user doesn't have a proof to enable wbinvd. The thing that is missing here is a notifier so kvm can track changes in the group's assigned iommu_domain, but I think it is not necessary.. Jason
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Wednesday, April 20, 2022 3:24 AM > > On Fri, Apr 15, 2022 at 04:07:51AM +0000, Tian, Kevin wrote: > > > > + struct vfio_group *group = filep->private_data; > > > + bool ret; > > > + > > > + /* > > > + * Since the coherency state is determined only once a container is > > > + * attached the user must do so before they can prove they have > > > + * permission. > > > + */ > > > + if (vfio_group_add_container_user(group)) > > > + return true; > > > > I wonder whether it's better to return error here and let KVM to > > decide whether it wants to allow wbinvd in such case (though > > likely the conclusion is same) or simply rejects adding the group. > > Since the new model is to present proof at add it is OK - it just > means the user doesn't have a proof to enable wbinvd. it is OK if this interface is called vfio_file_is_wbinvd_required(). But now it is called vfio_file_enforced_coherent() then whether it is a proof for wbinvd is a caller-side knowledge. It is clearer to stick to the fact about cache coherency and if it cannot be determined at this point then simply return error to the caller to handle the situation. Also it sounds more reasonable for KVM to fail adding group if a container hasn't been attached instead of moving forward assuming an incorrect coherency and then getting an error later. Thanks Kevin
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index eb65b4c80ece64..c08093fb6d28d5 100644 --- a/drivers/vfio/vfio.c +++ b/drivers/vfio/vfio.c @@ -2003,14 +2003,35 @@ static struct iommu_group *vfio_file_iommu_group(struct file *filep) return group->iommu_group; } -long vfio_external_check_extension(struct vfio_group *group, unsigned long arg) +/** + * vfio_file_enforced_coherent - True if the DMA associated with the VFIO file + * is always CPU cache coherent + * @filep: VFIO file + * + * Enforced coherent means that the IOMMU ignores things like the PCIe no-snoop + * bit in DMA transactions. A return of false indicates that the user has + * rights to access additional instructions such as wbinvd on x86. + */ +static bool vfio_file_enforced_coherent(struct file *filep) { - return vfio_ioctl_check_extension(group->container, arg); + struct vfio_group *group = filep->private_data; + bool ret; + + /* + * Since the coherency state is determined only once a container is + * attached the user must do so before they can prove they have + * permission. + */ + if (vfio_group_add_container_user(group)) + return true; + ret = vfio_ioctl_check_extension(group->container, VFIO_DMA_CC_IOMMU); + vfio_group_try_dissolve_container(group); + return ret; } -EXPORT_SYMBOL_GPL(vfio_external_check_extension); static const struct vfio_file_ops vfio_file_group_ops = { .get_iommu_group = vfio_file_iommu_group, + .is_enforced_coherent = vfio_file_enforced_coherent, }; /** diff --git a/include/linux/vfio.h b/include/linux/vfio.h index d09a1856d4e5ea..b1583eb80f12e6 100644 --- a/include/linux/vfio.h +++ b/include/linux/vfio.h @@ -140,13 +140,12 @@ int vfio_mig_get_next_state(struct vfio_device *device, */ struct vfio_file_ops { struct iommu_group *(*get_iommu_group)(struct file *filep); + bool (*is_enforced_coherent)(struct file *filep); }; 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 long vfio_external_check_extension(struct vfio_group *group, - unsigned long arg); const struct vfio_file_ops *vfio_file_get_ops(struct file *filep); #define VFIO_PIN_PAGES_MAX_ENTRIES (PAGE_SIZE/sizeof(unsigned long)) diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c index 955cabc0683b29..f5ef78192a97ab 100644 --- a/virt/kvm/vfio.c +++ b/virt/kvm/vfio.c @@ -86,22 +86,6 @@ static void kvm_vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm) symbol_put(vfio_group_set_kvm); } -static bool kvm_vfio_group_is_coherent(struct vfio_group *vfio_group) -{ - long (*fn)(struct vfio_group *, unsigned long); - long ret; - - fn = symbol_get(vfio_external_check_extension); - if (!fn) - return false; - - ret = fn(vfio_group, VFIO_DMA_CC_IOMMU); - - symbol_put(vfio_external_check_extension); - - return ret > 0; -} - static void kvm_spapr_tce_release_vfio_group(struct kvm *kvm, struct kvm_vfio_group *kvg) { @@ -128,7 +112,7 @@ static void kvm_vfio_update_coherency(struct kvm_device *dev) mutex_lock(&kv->lock); list_for_each_entry(kvg, &kv->group_list, node) { - if (!kvm_vfio_group_is_coherent(kvg->vfio_group)) { + if (!kvg->ops->is_enforced_coherent(kvg->filp)) { noncoherent = true; break; }
Focus the new op into is_enforced_coherent() which only checks the enforced DMA coherency property of the file. Make the new op self contained by properly refcounting the container before touching it. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/vfio/vfio.c | 27 ++++++++++++++++++++++++--- include/linux/vfio.h | 3 +-- virt/kvm/vfio.c | 18 +----------------- 3 files changed, 26 insertions(+), 22 deletions(-)