Re: [PATCH 1/5] iommu: Replace uses of IOMMU_CAP_CACHE_COHERENCY with dev_is_dma_coherent()

Robin Murphy
2022-04-06T16:37:06Z

On 2022-04-05 17:16, Jason Gunthorpe wrote:
> vdpa and usnic are trying to test if IOMMU_CACHE is supported. The correct
> way to do this is via dev_is_dma_coherent()

Not necessarily...

Disregarding the complete disaster of PCIe No Snoop on Arm-Based
systems, there's the more interesting effectively-opposite scenario
where an SMMU bridges non-coherent devices to a coherent interconnect.
It's not something we take advantage of yet in Linux, and it can only be
properly described in ACPI, but there do exist situations where
IOMMU_CACHE is capable of making the device's traffic snoop, but
dev_is_dma_coherent() - and device_get_dma_attr() for external users -
would still say non-coherent because they can't assume that the SMMU is
enabled and programmed in just the right way.

I've also not thought too much about how things might look with S2FWB
thrown into the mix in future...

Robin.

> like the DMA API does. If
> IOMMU_CACHE is not supported then these drivers won't work as they don't
> call any coherency-restoring routines around their DMAs.
>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
> drivers/infiniband/hw/usnic/usnic_uiom.c | 16 +++++++---------
> drivers/vhost/vdpa.c | 3 ++-
> 2 files changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/infiniband/hw/usnic/usnic_uiom.c b/drivers/infiniband/hw/usnic/usnic_uiom.c
> index 760b254ba42d6b..24d118198ac756 100644
> --- a/drivers/infiniband/hw/usnic/usnic_uiom.c
> +++ b/drivers/infiniband/hw/usnic/usnic_uiom.c
> @@ -42,6 +42,7 @@
> #include <linux/list.h>
> #include <linux/pci.h>
> #include <rdma/ib_verbs.h>
> +#include <linux/dma-map-ops.h>
>
> #include "usnic_log.h"
> #include "usnic_uiom.h"
> @@ -474,6 +475,12 @@ int usnic_uiom_attach_dev_to_pd(struct usnic_uiom_pd *pd, struct device *dev)
> struct usnic_uiom_dev *uiom_dev;
> int err;
>
> + if (!dev_is_dma_coherent(dev)) {
> + usnic_err("IOMMU of %s does not support cache coherency\n",
> + dev_name(dev));
> + return -EINVAL;
> + }
> +
> uiom_dev = kzalloc(sizeof(*uiom_dev), GFP_ATOMIC);
> if (!uiom_dev)
> return -ENOMEM;
> @@ -483,13 +490,6 @@ int usnic_uiom_attach_dev_to_pd(struct usnic_uiom_pd *pd, struct device *dev)
> if (err)
> goto out_free_dev;
>
> - if (!iommu_capable(dev->bus, IOMMU_CAP_CACHE_COHERENCY)) {
> - usnic_err("IOMMU of %s does not support cache coherency\n",
> - dev_name(dev));
> - err = -EINVAL;
> - goto out_detach_device;
> - }
> -
> spin_lock(&pd->lock);
> list_add_tail(&uiom_dev->link, &pd->devs);
> pd->dev_cnt++;
> @@ -497,8 +497,6 @@ int usnic_uiom_attach_dev_to_pd(struct usnic_uiom_pd *pd, struct device *dev)
>
> return 0;
>
> -out_detach_device:
> - iommu_detach_device(pd->domain, dev);
> out_free_dev:
> kfree(uiom_dev);
> return err;
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 4c2f0bd062856a..05ea5800febc37 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -22,6 +22,7 @@
> #include <linux/vdpa.h>
> #include <linux/nospec.h>
> #include <linux/vhost.h>
> +#include <linux/dma-map-ops.h>
>
> #include "vhost.h"
>
> @@ -929,7 +930,7 @@ static int vhost_vdpa_alloc_domain(struct vhost_vdpa *v)
> if (!bus)
> return -EFAULT;
>
> - if (!iommu_capable(bus, IOMMU_CAP_CACHE_COHERENCY))
> + if (!dev_is_dma_coherent(dma_dev))
> return -ENOTSUPP;
>
> v->domain = iommu_domain_alloc(bus);