On Tue, Mar 16, 2021 at 04:38:22PM +0100, Christoph Hellwig wrote:
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.
For example, see the recent patch from Lu Baolu:
https://lore.kernel.org/r/20210225061454.2864009-1-baolu.lu@linux.intel.com
Will
On 2021-03-30 14:11, Will Deacon wrote:
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.
For example, see the recent patch from Lu Baolu:
https://lore.kernel.org/r/20210225061454.2864009-1-baolu.lu@linux.intel.com
Erm, this patch is based on that one, it's right there in the context :/
Thanks, Robin.
On Tue, Mar 30, 2021 at 02:19:38PM +0100, Robin Murphy wrote:
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.
Ah, sorry, I didn't spot that! I was just trying to illustrate that this is per-device.
Will
On 2021-03-30 14:58, Will Deacon wrote:
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.
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.
Sure, I understand - and I'm just trying to bang home that despite appearances it's never actually been treated as such for SMMU, so anything that's wrong after this change was already wrong before.
Robin.
On Tue, Mar 30, 2021 at 05:28:19PM +0100, Robin Murphy wrote:
For DMA domains sure, but I don't think that's the case for unmanaged domains such as those used by VFIO.
I think a helper that takes a domain would be a good starting point.
Will
On 2021-03-31 12:49, Will Deacon wrote:
Eh? This is only relevant to DMA domains anyway. Flush queues are part of the IOVA allocator that VFIO doesn't even use. It's always been the case that unmanaged domains only use strict invalidation.
You mean device, right? The one condition we currently have is at the device level, and there's really nothing inherent to the domain itself that matters (since the type is implicitly IOMMU_DOMAIN_DMA to even care about this).
Another idea that's just come to mind is now that IOMMU_DOMAIN_DMA has a standard meaning, maybe we could split out a separate IOMMU_DOMAIN_DMA_STRICT type such that it can all propagate from iommu_get_def_domain_type()? That feels like it might be quite promising, but I'd still do it as an improvement on top of this patch, since it's beyond just cleaning up the abuse of domain attributes to pass a command-line option around.
Robin.
On Wed, Mar 31, 2021 at 02:09:37PM +0100, Robin Murphy wrote:
Maybe I'm going mad. With this patch, the SMMU driver unconditionally sets IO_PGTABLE_QUIRK_NON_STRICT for page-tables if iommu_get_dma_strict() is true, no? In which case, that will get set for page-tables corresponding to unmanaged domains as well as DMA domains when it is enabled. That didn't happen before because you couldn't set the attribute for unmanaged domains.
What am I missing?
Device would probably work too; you'd pass the first device to attach to the domain when querying this from the SMMU driver, I suppose.
Will
On 2021-03-31 16:32, Will Deacon wrote:
Oh cock... sorry, all this time I've been saying what I *expect* it to do, while overlooking the fact that the IO_PGTABLE_QUIRK_NON_STRICT hunks were the bits I forgot to write and Christoph had to fix up. Indeed, those should be checking the domain type too to preserve the existing behaviour. Apologies for the confusion.
Robin.
dri-devel@lists.freedesktop.org
-
Robin Murphy -
Will Deacon