| Message ID | 2-v1-ef02c60ddb76+12ca2-intel_no_snoop_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Make the iommu driver no-snoop block feature consistent | expand |
On Tue, 5 Apr 2022 13:16:01 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote: > dev_is_dma_coherent() is the control to determine if IOMMU_CACHE can be > supported. > > IOMMU_CACHE means that normal DMAs do not require any additional coherency > mechanism and is the basic uAPI that VFIO exposes to userspace. For > instance VFIO applications like DPDK will not work if additional coherency > operations are required. > > Therefore check dev_is_dma_coherent() before allowing a device to join a > domain. This will block device/platform/iommu combinations from using VFIO > that do not support cache coherent DMA. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/vfio/vfio.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index a4555014bd1e72..2a3aa3e742d943 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -32,6 +32,7 @@ > #include <linux/vfio.h> > #include <linux/wait.h> > #include <linux/sched/signal.h> > +#include <linux/dma-map-ops.h> > #include "vfio.h" > > #define DRIVER_VERSION "0.3" > @@ -1348,6 +1349,11 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf) > if (IS_ERR(device)) > return PTR_ERR(device); > > + if (group->type == VFIO_IOMMU && !dev_is_dma_coherent(device->dev)) { > + ret = -ENODEV; > + goto err_device_put; > + } > + Failing at the point where the user is trying to gain access to the device seems a little late in the process and opaque, wouldn't we rather have vfio bus drivers fail to probe such devices? I'd expect this to occur in the vfio_register_group_dev() path. Thanks, Alex > if (!try_module_get(device->dev->driver->owner)) { > ret = -ENODEV; > goto err_device_put;
On Tue, Apr 05, 2022 at 01:10:44PM -0600, Alex Williamson wrote: > On Tue, 5 Apr 2022 13:16:01 -0300 > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > dev_is_dma_coherent() is the control to determine if IOMMU_CACHE can be > > supported. > > > > IOMMU_CACHE means that normal DMAs do not require any additional coherency > > mechanism and is the basic uAPI that VFIO exposes to userspace. For > > instance VFIO applications like DPDK will not work if additional coherency > > operations are required. > > > > Therefore check dev_is_dma_coherent() before allowing a device to join a > > domain. This will block device/platform/iommu combinations from using VFIO > > that do not support cache coherent DMA. > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > drivers/vfio/vfio.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > > index a4555014bd1e72..2a3aa3e742d943 100644 > > +++ b/drivers/vfio/vfio.c > > @@ -32,6 +32,7 @@ > > #include <linux/vfio.h> > > #include <linux/wait.h> > > #include <linux/sched/signal.h> > > +#include <linux/dma-map-ops.h> > > #include "vfio.h" > > > > #define DRIVER_VERSION "0.3" > > @@ -1348,6 +1349,11 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf) > > if (IS_ERR(device)) > > return PTR_ERR(device); > > > > + if (group->type == VFIO_IOMMU && !dev_is_dma_coherent(device->dev)) { > > + ret = -ENODEV; > > + goto err_device_put; > > + } > > + > > Failing at the point where the user is trying to gain access to the > device seems a little late in the process and opaque, wouldn't we > rather have vfio bus drivers fail to probe such devices? I'd expect > this to occur in the vfio_register_group_dev() path. Thanks, Yes, that is a good point. So like this: int vfio_register_group_dev(struct vfio_device *device) { + if (!dev_is_dma_coherent(device->dev)) + return -EINVAL; + return __vfio_register_dev(device, vfio_group_find_or_alloc(device->dev)); } I fixed it up. Thanks, Jason
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Wednesday, April 6, 2022 3:29 AM > > On Tue, Apr 05, 2022 at 01:10:44PM -0600, Alex Williamson wrote: > > On Tue, 5 Apr 2022 13:16:01 -0300 > > Jason Gunthorpe <jgg@nvidia.com> wrote: > > > > > dev_is_dma_coherent() is the control to determine if IOMMU_CACHE can > be > > > supported. > > > > > > IOMMU_CACHE means that normal DMAs do not require any additional > coherency > > > mechanism and is the basic uAPI that VFIO exposes to userspace. For > > > instance VFIO applications like DPDK will not work if additional coherency > > > operations are required. > > > > > > Therefore check dev_is_dma_coherent() before allowing a device to join > a > > > domain. This will block device/platform/iommu combinations from using > VFIO > > > that do not support cache coherent DMA. > > > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > > drivers/vfio/vfio.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > > > index a4555014bd1e72..2a3aa3e742d943 100644 > > > +++ b/drivers/vfio/vfio.c > > > @@ -32,6 +32,7 @@ > > > #include <linux/vfio.h> > > > #include <linux/wait.h> > > > #include <linux/sched/signal.h> > > > +#include <linux/dma-map-ops.h> > > > #include "vfio.h" > > > > > > #define DRIVER_VERSION "0.3" > > > @@ -1348,6 +1349,11 @@ static int vfio_group_get_device_fd(struct > vfio_group *group, char *buf) > > > if (IS_ERR(device)) > > > return PTR_ERR(device); > > > > > > + if (group->type == VFIO_IOMMU && !dev_is_dma_coherent(device- > >dev)) { > > > + ret = -ENODEV; > > > + goto err_device_put; > > > + } > > > + > > > > Failing at the point where the user is trying to gain access to the > > device seems a little late in the process and opaque, wouldn't we > > rather have vfio bus drivers fail to probe such devices? I'd expect > > this to occur in the vfio_register_group_dev() path. Thanks, > > Yes, that is a good point. > > So like this: > > int vfio_register_group_dev(struct vfio_device *device) > { > + if (!dev_is_dma_coherent(device->dev)) > + return -EINVAL; > + > return __vfio_register_dev(device, > vfio_group_find_or_alloc(device->dev)); > } > > I fixed it up. > if that is the case should it also apply to usnic and vdpa in the first patch (i.e. fail the probe)?
On Wed, Apr 06, 2022 at 07:02:36AM +0000, Tian, Kevin wrote: > > So like this: > > > > int vfio_register_group_dev(struct vfio_device *device) > > { > > + if (!dev_is_dma_coherent(device->dev)) > > + return -EINVAL; > > + > > return __vfio_register_dev(device, > > vfio_group_find_or_alloc(device->dev)); > > } > > > > I fixed it up. > > > > if that is the case should it also apply to usnic and vdpa in the first > patch (i.e. fail the probe)? Ideally, but I don't want to mess with existing logic in these drivers.. Thanks, Jason
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index a4555014bd1e72..2a3aa3e742d943 100644 --- a/drivers/vfio/vfio.c +++ b/drivers/vfio/vfio.c @@ -32,6 +32,7 @@ #include <linux/vfio.h> #include <linux/wait.h> #include <linux/sched/signal.h> +#include <linux/dma-map-ops.h> #include "vfio.h" #define DRIVER_VERSION "0.3" @@ -1348,6 +1349,11 @@ static int vfio_group_get_device_fd(struct vfio_group *group, char *buf) if (IS_ERR(device)) return PTR_ERR(device); + if (group->type == VFIO_IOMMU && !dev_is_dma_coherent(device->dev)) { + ret = -ENODEV; + goto err_device_put; + } + if (!try_module_get(device->dev->driver->owner)) { ret = -ENODEV; goto err_device_put;
dev_is_dma_coherent() is the control to determine if IOMMU_CACHE can be supported. IOMMU_CACHE means that normal DMAs do not require any additional coherency mechanism and is the basic uAPI that VFIO exposes to userspace. For instance VFIO applications like DPDK will not work if additional coherency operations are required. Therefore check dev_is_dma_coherent() before allowing a device to join a domain. This will block device/platform/iommu combinations from using VFIO that do not support cache coherent DMA. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/vfio/vfio.c | 6 ++++++ 1 file changed, 6 insertions(+)