| Message ID | 50bee17e9248ccfccb33a10238210d4ff4f4cf4d.1627468309.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu: Refactor DMA domain strictness | expand |
Hi Robin, On 7/28/21 11:58 PM, Robin Murphy wrote: > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 982545234cf3..eecb5657de69 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -136,6 +136,9 @@ static int __init iommu_subsys_init(void) > } > } > > + if (!iommu_default_passthrough() && !iommu_dma_strict) > + iommu_def_domain_type = IOMMU_DOMAIN_DMA_FQ; iommu_dma_strict could be changed later by the vendor iommu driver via iommu_set_dma_strict(). This seems not to be the right place to set iommu_def_domain_type. > + > pr_info("Default domain type: %s %s\n", > iommu_domain_type_str(iommu_def_domain_type), > (iommu_cmd_line & IOMMU_CMD_LINE_DMA_API) ? Best regards, baolu
On 2021-07-29 08:13, Lu Baolu wrote: > Hi Robin, > > On 7/28/21 11:58 PM, Robin Murphy wrote: >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >> index 982545234cf3..eecb5657de69 100644 >> --- a/drivers/iommu/iommu.c >> +++ b/drivers/iommu/iommu.c >> @@ -136,6 +136,9 @@ static int __init iommu_subsys_init(void) >> } >> } >> + if (!iommu_default_passthrough() && !iommu_dma_strict) >> + iommu_def_domain_type = IOMMU_DOMAIN_DMA_FQ; > > iommu_dma_strict could be changed later by the vendor iommu driver via > iommu_set_dma_strict(). This seems not to be the right place to set > iommu_def_domain_type. Ah yes, good catch once again, thanks! I think this *is* the right place to initially set it to honour the command-line option, since that matches what we do for passthrough. However also like passthrough we'll need to keep things in sync if it's updated later, like this: diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 87d7b299436e..593d4555bc57 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -359,6 +359,8 @@ early_param("iommu.strict", iommu_dma_setup); void iommu_set_dma_strict(void) { iommu_dma_strict = true; + if (iommu_def_domain_type == IOMMU_DOMAIN_DMA_FQ) + iommu_def_domain_type = IOMMU_DOMAIN_DMA; } static ssize_t iommu_group_attr_show(struct kobject *kobj, Does that seem reasonable? I'm not sure there's any cleaner way to do it since we don't want to inadvertently clobber the default type if the user has given us something funky like "intel_iommu=strict iommu.passthrough=1". Cheers, Robin. > >> + >> pr_info("Default domain type: %s %s\n", >> iommu_domain_type_str(iommu_def_domain_type), >> (iommu_cmd_line & IOMMU_CMD_LINE_DMA_API) ? > > Best regards, > baolu
On 2021/7/29 17:36, Robin Murphy wrote: > On 2021-07-29 08:13, Lu Baolu wrote: >> Hi Robin, >> >> On 7/28/21 11:58 PM, Robin Murphy wrote: >>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >>> index 982545234cf3..eecb5657de69 100644 >>> --- a/drivers/iommu/iommu.c >>> +++ b/drivers/iommu/iommu.c >>> @@ -136,6 +136,9 @@ static int __init iommu_subsys_init(void) >>> } >>> } >>> + if (!iommu_default_passthrough() && !iommu_dma_strict) >>> + iommu_def_domain_type = IOMMU_DOMAIN_DMA_FQ; >> >> iommu_dma_strict could be changed later by the vendor iommu driver via >> iommu_set_dma_strict(). This seems not to be the right place to set >> iommu_def_domain_type. > > Ah yes, good catch once again, thanks! > > I think this *is* the right place to initially set it to honour the > command-line option, since that matches what we do for passthrough. > However also like passthrough we'll need to keep things in sync if it's > updated later, like this: > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 87d7b299436e..593d4555bc57 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -359,6 +359,8 @@ early_param("iommu.strict", iommu_dma_setup); > void iommu_set_dma_strict(void) > { > iommu_dma_strict = true; > + if (iommu_def_domain_type == IOMMU_DOMAIN_DMA_FQ) > + iommu_def_domain_type = IOMMU_DOMAIN_DMA; > } > > static ssize_t iommu_group_attr_show(struct kobject *kobj, > > > Does that seem reasonable? I'm not sure there's any cleaner way to do it > since we don't want to inadvertently clobber the default type if the > user has given us something funky like "intel_iommu=strict > iommu.passthrough=1". Yeah! It's reasonable as far as I can see. Best regards, baolu > > Cheers, > Robin. > >> >>> + >>> pr_info("Default domain type: %s %s\n", >>> iommu_domain_type_str(iommu_def_domain_type), >>> (iommu_cmd_line & IOMMU_CMD_LINE_DMA_API) ? >> >> Best regards, >> baolu
On 7/29/21 5:36 PM, Robin Murphy wrote: > On 2021-07-29 08:13, Lu Baolu wrote: >> Hi Robin, >> >> On 7/28/21 11:58 PM, Robin Murphy wrote: >>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c >>> index 982545234cf3..eecb5657de69 100644 >>> --- a/drivers/iommu/iommu.c >>> +++ b/drivers/iommu/iommu.c >>> @@ -136,6 +136,9 @@ static int __init iommu_subsys_init(void) >>> } >>> } >>> + if (!iommu_default_passthrough() && !iommu_dma_strict) >>> + iommu_def_domain_type = IOMMU_DOMAIN_DMA_FQ; >> >> iommu_dma_strict could be changed later by the vendor iommu driver via >> iommu_set_dma_strict(). This seems not to be the right place to set >> iommu_def_domain_type. > > Ah yes, good catch once again, thanks! > > I think this *is* the right place to initially set it to honour the > command-line option, since that matches what we do for passthrough. > However also like passthrough we'll need to keep things in sync if it's > updated later, like this: > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 87d7b299436e..593d4555bc57 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -359,6 +359,8 @@ early_param("iommu.strict", iommu_dma_setup); > void iommu_set_dma_strict(void) > { > iommu_dma_strict = true; > + if (iommu_def_domain_type == IOMMU_DOMAIN_DMA_FQ) > + iommu_def_domain_type = IOMMU_DOMAIN_DMA; > } > > static ssize_t iommu_group_attr_show(struct kobject *kobj, > > > Does that seem reasonable? I'm not sure there's any cleaner way to do it > since we don't want to inadvertently clobber the default type if the > user has given us something funky like "intel_iommu=strict > iommu.passthrough=1". > > Cheers, > Robin. > >> >>> + >>> pr_info("Default domain type: %s %s\n", >>> iommu_domain_type_str(iommu_def_domain_type), >>> (iommu_cmd_line & IOMMU_CMD_LINE_DMA_API) ? >> >> Best regards, >> baolu With above fixed, Reviewed-by: Lu Baolu <baolu.lu@linux.intel.com> Best regards, baolu
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index a1f0d83d1eb5..19400826eba7 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2175,7 +2175,7 @@ static int arm_smmu_domain_finalise(struct iommu_domain *domain, .iommu_dev = smmu->dev, }; - if (!iommu_get_dma_strict(domain)) + if (domain->type == IOMMU_DOMAIN_DMA_FQ) pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT; pgtbl_ops = alloc_io_pgtable_ops(fmt, &pgtbl_cfg, smmu_domain); diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c index 936c5e9d5e82..109e4723f9f5 100644 --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c @@ -765,7 +765,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain, .iommu_dev = smmu->dev, }; - if (!iommu_get_dma_strict(domain)) + if (domain->type == IOMMU_DOMAIN_DMA_FQ) pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_NON_STRICT; if (smmu->impl && smmu->impl->init_context) { diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c index 8b3545c01077..7f3968865387 100644 --- a/drivers/iommu/dma-iommu.c +++ b/drivers/iommu/dma-iommu.c @@ -363,13 +363,14 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base, init_iova_domain(iovad, 1UL << order, base_pfn); - if (!cookie->fq_domain && !dev_is_untrusted(dev) && - domain->ops->flush_iotlb_all && !iommu_get_dma_strict(domain)) { + if (domain->type == IOMMU_DOMAIN_DMA_FQ && !cookie->fq_domain) { if (init_iova_flush_queue(iovad, iommu_dma_flush_iotlb_all, - iommu_dma_entry_dtor)) + iommu_dma_entry_dtor)) { pr_warn("iova flush queue initialization failed\n"); - else + domain->type = IOMMU_DOMAIN_DMA; + } else { cookie->fq_domain = domain; + } } return iova_reserve_iommu_regions(dev, domain); diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 982545234cf3..eecb5657de69 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -136,6 +136,9 @@ static int __init iommu_subsys_init(void) } } + if (!iommu_default_passthrough() && !iommu_dma_strict) + iommu_def_domain_type = IOMMU_DOMAIN_DMA_FQ; + pr_info("Default domain type: %s %s\n", iommu_domain_type_str(iommu_def_domain_type), (iommu_cmd_line & IOMMU_CMD_LINE_DMA_API) ? @@ -357,15 +360,6 @@ void iommu_set_dma_strict(void) iommu_dma_strict = true; } -bool iommu_get_dma_strict(struct iommu_domain *domain) -{ - /* only allow lazy flushing for DMA domains */ - if (domain->type == IOMMU_DOMAIN_DMA) - return iommu_dma_strict; - return true; -} -EXPORT_SYMBOL_GPL(iommu_get_dma_strict); - static ssize_t iommu_group_attr_show(struct kobject *kobj, struct attribute *__attr, char *buf) { diff --git a/include/linux/iommu.h b/include/linux/iommu.h index 046ba4d54cd2..edfe2fdb8368 100644 --- a/include/linux/iommu.h +++ b/include/linux/iommu.h @@ -498,7 +498,6 @@ int iommu_set_pgtable_quirks(struct iommu_domain *domain, unsigned long quirks); void iommu_set_dma_strict(void); -bool iommu_get_dma_strict(struct iommu_domain *domain); extern int report_iommu_fault(struct iommu_domain *domain, struct device *dev, unsigned long iova, int flags);
Eliminate the iommu_get_dma_strict() indirection and pipe the information through the domain type from the beginning. Besides the flow simplification this also has several nice side-effects: - Automatically implies strict mode for untrusted devices by virtue of their IOMMU_DOMAIN_DMA override. - Ensures that we only end up using flush queues for drivers which are aware of them and can actually benefit. - Allows us to handle flush queue init failure by falling back to strict mode instead of leaving it to possibly blow up later. Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 2 +- drivers/iommu/arm/arm-smmu/arm-smmu.c | 2 +- drivers/iommu/dma-iommu.c | 9 +++++---- drivers/iommu/iommu.c | 12 +++--------- include/linux/iommu.h | 1 - 5 files changed, 10 insertions(+), 16 deletions(-)