Robin Murphy
2021-Mar-31 13:09 UTC
[PATCH 16/18] iommu: remove DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE
On 2021-03-31 12:49, Will Deacon wrote:> On Tue, Mar 30, 2021 at 05:28:19PM +0100, Robin Murphy wrote: >> On 2021-03-30 14:58, Will Deacon wrote: >>> On Tue, Mar 30, 2021 at 02:19:38PM +0100, Robin Murphy wrote: >>>> On 2021-03-30 14:11, Will Deacon wrote: >>>>> On Tue, Mar 16, 2021 at 04:38:22PM +0100, Christoph Hellwig wrote: >>>>>> From: Robin Murphy <robin.murphy at 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 at arm.com>. >>>>>> [ported on top of the other iommu_attr changes and added a few small >>>>>> missing bits] >>>>>> Signed-off-by: Christoph Hellwig <hch at 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.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.>> 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.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.
Will Deacon
2021-Mar-31 15:32 UTC
[PATCH 16/18] iommu: remove DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE
On Wed, Mar 31, 2021 at 02:09:37PM +0100, Robin Murphy wrote:> On 2021-03-31 12:49, Will Deacon wrote: > > On Tue, Mar 30, 2021 at 05:28:19PM +0100, Robin Murphy wrote: > > > On 2021-03-30 14:58, Will Deacon wrote: > > > > On Tue, Mar 30, 2021 at 02:19:38PM +0100, Robin Murphy wrote: > > > > > On 2021-03-30 14:11, Will Deacon wrote: > > > > > > On Tue, Mar 16, 2021 at 04:38:22PM +0100, Christoph Hellwig wrote: > > > > > > > From: Robin Murphy <robin.murphy at 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 at arm.com>. > > > > > > > [ported on top of the other iommu_attr changes and added a few small > > > > > > > missing bits] > > > > > > > Signed-off-by: Christoph Hellwig <hch at 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. > > 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.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?> > > 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. > > 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).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