On Tue, Mar 30, 2021 at 05:28:19PM +0100, Robin Murphy wrote:

On 2021-03-30 14:58, Will Deacon wrote:

quoted

On Tue, Mar 30, 2021 at 02:19:38PM +0100, Robin Murphy wrote:

quoted

On 2021-03-30 14:11, Will Deacon wrote:

quoted

On Tue, Mar 16, 2021 at 04:38:22PM +0100, Christoph Hellwig wrote:

quoted

From: Robin Murphy <robin.murphy@arm.com>

Instead make the global iommu_dma_strict paramete in iommu.c canonical by
exporting helpers to get and set it and use those directly in the drivers.

This make sure that the iommu.strict parameter also works for the AMD and
Intel IOMMU drivers on x86.  As those default to lazy flushing a new
IOMMU_CMD_LINE_STRICT is used to turn the value into a tristate to
represent the default if not overriden by an explicit parameter.

Signed-off-by: Robin Murphy <robin.murphy@arm.com>.
[ported on top of the other iommu_attr changes and added a few small
   missing bits]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
   drivers/iommu/amd/iommu.c                   | 23 +-------
   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 50 +---------------
   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  1 -
   drivers/iommu/arm/arm-smmu/arm-smmu.c       | 27 +--------
   drivers/iommu/dma-iommu.c                   |  9 +--
   drivers/iommu/intel/iommu.c                 | 64 ++++-----------------
   drivers/iommu/iommu.c                       | 27 ++++++---
   include/linux/iommu.h                       |  4 +-
   8 files changed, 40 insertions(+), 165 deletions(-)

I really like this cleanup, but I can't help wonder if it's going in the
wrong direction. With SoCs often having multiple IOMMU instances and a
distinction between "trusted" and "untrusted" devices, then having the
flush-queue enabled on a per-IOMMU or per-domain basis doesn't sound
unreasonable to me, but this change makes it a global property.

The intent here was just to streamline the existing behaviour of stuffing a
global property into a domain attribute then pulling it out again in the
illusion that it was in any way per-domain. We're still checking
dev_is_untrusted() before making an actual decision, and it's not like we
can't add more factors at that point if we want to.

Like I say, the cleanup is great. I'm just wondering whether there's a
better way to express the complicated logic to decide whether or not to use
the flush queue than what we end up with:

	if (!cookie->fq_domain && (!dev || !dev_is_untrusted(dev)) &&
	    domain->ops->flush_iotlb_all && !iommu_get_dma_strict())

which is mixing up globals, device properties and domain properties. The
result is that the driver code ends up just using the global to determine
whether or not to pass IO_PGTABLE_QUIRK_NON_STRICT to the page-table code,
which is a departure from the current way of doing things.

But previously, SMMU only ever saw the global policy piped through the
domain attribute by iommu_group_alloc_default_domain(), so there's no
functional change there.

For DMA domains sure, but I don't think that's the case for unmanaged
domains such as those used by VFIO.

Obviously some of the above checks could be factored out into some kind of
iommu_use_flush_queue() helper that IOMMU drivers can also call if they need
to keep in sync. Or maybe we just allow iommu-dma to set
IO_PGTABLE_QUIRK_NON_STRICT directly via iommu_set_pgtable_quirks() if we're
treating that as a generic thing now.

I think a helper that takes a domain would be a good starting point.

Will