| Message ID | 4-v2-f090ae795824+6ad-intel_no_snoop_jgg@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make the iommu driver no-snoop block feature consistent | expand |
> From: Jason Gunthorpe <jgg@nvidia.com> > Sent: Thursday, April 7, 2022 11:24 PM > > 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 IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do > before > allowing an IOMMU backed VFIO device to be created. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/vfio/vfio.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index a4555014bd1e72..9edad767cfdad3 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -815,6 +815,13 @@ static int __vfio_register_dev(struct vfio_device > *device, > > int vfio_register_group_dev(struct vfio_device *device) > { > + /* > + * VFIO always sets IOMMU_CACHE because we offer no way for > userspace to > + * restore cache coherency. > + */ > + if (!iommu_capable(device->dev->bus, > IOMMU_CAP_CACHE_COHERENCY)) > + return -EINVAL; > + One nit. Is it logistically more reasonable to put this patch before changing VFIO to always set IOMMU_CACHE? otherwise, Reviewed-by: Kevin Tian <kevin.tian@intel.com>
On Fri, Apr 08, 2022 at 08:26:10AM +0000, Tian, Kevin wrote: > > From: Jason Gunthorpe <jgg@nvidia.com> > > Sent: Thursday, April 7, 2022 11:24 PM > > > > 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 IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do > > before > > allowing an IOMMU backed VFIO device to be created. > > > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > drivers/vfio/vfio.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > > index a4555014bd1e72..9edad767cfdad3 100644 > > +++ b/drivers/vfio/vfio.c > > @@ -815,6 +815,13 @@ static int __vfio_register_dev(struct vfio_device > > *device, > > > > int vfio_register_group_dev(struct vfio_device *device) > > { > > + /* > > + * VFIO always sets IOMMU_CACHE because we offer no way for > > userspace to > > + * restore cache coherency. > > + */ > > + if (!iommu_capable(device->dev->bus, > > IOMMU_CAP_CACHE_COHERENCY)) > > + return -EINVAL; > > + > > One nit. Is it logistically more reasonable to put this patch before > changing VFIO to always set IOMMU_CACHE? For bisectability it has to be after iommu: Redefine IOMMU_CAP_CACHE_COHERENCY as the cap flag for IOMMU_CACHE Otherwise Intel iommu will stop working with VFIO The ordering is OK as is because no IOMMU that works with VFIO cares about IOMMU_CACHE. Jason
On 2022-04-08 13:22, Jason Gunthorpe wrote: > On Fri, Apr 08, 2022 at 08:26:10AM +0000, Tian, Kevin wrote: >>> From: Jason Gunthorpe <jgg@nvidia.com> >>> Sent: Thursday, April 7, 2022 11:24 PM >>> >>> 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 IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do >>> before >>> allowing an IOMMU backed VFIO device to be created. >>> >>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> >>> drivers/vfio/vfio.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c >>> index a4555014bd1e72..9edad767cfdad3 100644 >>> +++ b/drivers/vfio/vfio.c >>> @@ -815,6 +815,13 @@ static int __vfio_register_dev(struct vfio_device >>> *device, >>> >>> int vfio_register_group_dev(struct vfio_device *device) >>> { >>> + /* >>> + * VFIO always sets IOMMU_CACHE because we offer no way for >>> userspace to >>> + * restore cache coherency. >>> + */ >>> + if (!iommu_capable(device->dev->bus, >>> IOMMU_CAP_CACHE_COHERENCY)) >>> + return -EINVAL; >>> + >> >> One nit. Is it logistically more reasonable to put this patch before >> changing VFIO to always set IOMMU_CACHE? > > For bisectability it has to be after > > iommu: Redefine IOMMU_CAP_CACHE_COHERENCY as the cap flag for IOMMU_CACHE > > Otherwise Intel iommu will stop working with VFIO > > The ordering is OK as is because no IOMMU that works with VFIO cares > about IOMMU_CACHE. The Arm SMMU drivers do (without it even coherent traffic would be downgraded to non-cacheable), but then they also handle IOMMU_CAP_CACHE_COHERENCY nonsensically, and it happens to work out since AFAIK there aren't (yet) any Arm-based systems where you can reasonably try to use VFIO that don't also have hardware-coherent PCI. Thus I don't think there's any risk of regression for us here. Robin.
On Fri, Apr 08, 2022 at 02:28:02PM +0100, Robin Murphy wrote: > > > One nit. Is it logistically more reasonable to put this patch before > > > changing VFIO to always set IOMMU_CACHE? > > > > For bisectability it has to be after > > > > iommu: Redefine IOMMU_CAP_CACHE_COHERENCY as the cap flag for IOMMU_CACHE > > > > Otherwise Intel iommu will stop working with VFIO > > > > The ordering is OK as is because no IOMMU that works with VFIO cares > > about IOMMU_CACHE. > > The Arm SMMU drivers do (without it even coherent traffic would be > downgraded to non-cacheable), but then they also handle > IOMMU_CAP_CACHE_COHERENCY nonsensically, and it happens to work out since > AFAIK there aren't (yet) any Arm-based systems where you can reasonably try > to use VFIO that don't also have hardware-coherent PCI. Thus I don't think > there's any risk of regression for us here. Right, I was unclear, I meant 'requires IOMMU_CACHE to be unset to work with VFIO' Jason
On Thu, 7 Apr 2022 12:23:47 -0300 Jason Gunthorpe <jgg@nvidia.com> wrote: > 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 IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do before > allowing an IOMMU backed VFIO device to be created. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/vfio/vfio.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index a4555014bd1e72..9edad767cfdad3 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -815,6 +815,13 @@ static int __vfio_register_dev(struct vfio_device *device, > > int vfio_register_group_dev(struct vfio_device *device) > { > + /* > + * VFIO always sets IOMMU_CACHE because we offer no way for userspace to > + * restore cache coherency. > + */ > + if (!iommu_capable(device->dev->bus, IOMMU_CAP_CACHE_COHERENCY)) > + return -EINVAL; > + > return __vfio_register_dev(device, > vfio_group_find_or_alloc(device->dev)); > } Acked-by: Alex Williamson <alex.williamson@redhat.com>
On 4/8/22 01:23, Jason Gunthorpe via iommu wrote: > 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 IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do before > allowing an IOMMU backed VFIO device to be created. This just broke VFIO on POWER which does not use iommu_ops. > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/vfio/vfio.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c > index a4555014bd1e72..9edad767cfdad3 100644 > --- a/drivers/vfio/vfio.c > +++ b/drivers/vfio/vfio.c > @@ -815,6 +815,13 @@ static int __vfio_register_dev(struct vfio_device *device, > > int vfio_register_group_dev(struct vfio_device *device) > { > + /* > + * VFIO always sets IOMMU_CACHE because we offer no way for userspace to > + * restore cache coherency. > + */ > + if (!iommu_capable(device->dev->bus, IOMMU_CAP_CACHE_COHERENCY)) > + return -EINVAL; > + > return __vfio_register_dev(device, > vfio_group_find_or_alloc(device->dev)); > }
> From: Alexey Kardashevskiy <aik@ozlabs.ru> > Sent: Friday, July 1, 2022 12:58 PM > > On 4/8/22 01:23, Jason Gunthorpe via iommu wrote: > > 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 IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do > before > > allowing an IOMMU backed VFIO device to be created. > > > This just broke VFIO on POWER which does not use iommu_ops. In this case below check is more reasonable to be put in type1 attach_group(). Do a iommu_group_for_each_dev() to verify CACHE_COHERENCY similar to what Robin did for INTR_REMAP. (sorry no access to my build machine now but I suppose Jason can soon work out a fix once he sees this.
On 7/1/22 16:07, Tian, Kevin wrote: >> From: Alexey Kardashevskiy <aik@ozlabs.ru> >> Sent: Friday, July 1, 2022 12:58 PM >> >> On 4/8/22 01:23, Jason Gunthorpe via iommu wrote: >>> 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 IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do >> before >>> allowing an IOMMU backed VFIO device to be created. >> >> >> This just broke VFIO on POWER which does not use iommu_ops. > > In this case below check is more reasonable to be put in type1 > attach_group(). Do a iommu_group_for_each_dev() to verify > CACHE_COHERENCY similar to what Robin did for INTR_REMAP. ah makes sense. I've posted an RFC with another problem - "[RFC PATCH kernel] vfio: Skip checking for IOMMU_CAP_CACHE_COHERENCY on POWER and more", would be great if both addressed, or I try moving those next week :) Thanks, > > (sorry no access to my build machine now but I suppose Jason > can soon work out a fix once he sees this.
diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index a4555014bd1e72..9edad767cfdad3 100644 --- a/drivers/vfio/vfio.c +++ b/drivers/vfio/vfio.c @@ -815,6 +815,13 @@ static int __vfio_register_dev(struct vfio_device *device, int vfio_register_group_dev(struct vfio_device *device) { + /* + * VFIO always sets IOMMU_CACHE because we offer no way for userspace to + * restore cache coherency. + */ + if (!iommu_capable(device->dev->bus, IOMMU_CAP_CACHE_COHERENCY)) + return -EINVAL; + return __vfio_register_dev(device, vfio_group_find_or_alloc(device->dev)); }
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 IOMMU_CAP_CACHE_COHERENCY like vdpa & usnic do before allowing an IOMMU backed VFIO device to be created. Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> --- drivers/vfio/vfio.c | 7 +++++++ 1 file changed, 7 insertions(+)