| Message ID | ed26ed3213bf07ab4977211702dc0898680524cd.1684154219.git.robin.murphy@arm.com |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | iommu: tegra-gart cleanups | expand |
On Mon, May 15, 2023 at 01:49:32PM +0100, Robin Murphy wrote: > It was never entirely clear whether the force_aperture flag was supposed > to be a hardware capability or a software policy. So far things seem to > have leant towards the latter, given that the only drivers not setting > the flag are ones where the aperture is seemingly a whole virtual > address space such that accesses outside it wouldn't be possible, and > the one driver which definitely can't enforce it in hardware *does* > still set the flag. > > On reflection, though, it makes little sense for drivers to dictate > usage policy to callers, and the interpretation that a driver setting > the flag might also translate addresses outside the given aperture but > has for some reason chosen not to is not actually a useful one. It seems > a lot more logical to treat the aperture as the absolute limit of what > can be translated, and the flag to indicate what would happen if a > device did try to access an address outside the aperture, i.e. whether > the access would fault or pass through untranslated. As such, reframe > the flag consistent with a hardware capability in the hope of clearing > up the misconception. > > Signed-off-by: Robin Murphy <robin.murphy@arm.com> > --- > drivers/iommu/dma-iommu.c | 19 +++++++------------ > drivers/iommu/mtk_iommu_v1.c | 4 ++++ > drivers/iommu/sprd-iommu.c | 1 + > drivers/iommu/tegra-gart.c | 2 +- > 4 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c > index 7a9f0b0bddbd..4693b021d54f 100644 > --- a/drivers/iommu/dma-iommu.c > +++ b/drivers/iommu/dma-iommu.c > @@ -548,24 +548,19 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base, > if (!cookie || cookie->type != IOMMU_DMA_IOVA_COOKIE) > return -EINVAL; > > + /* > + * If the IOMMU only offers a non-isolated GART-style translation > + * aperture, just let the device use dma-direct. > + */ So we add code to dma-iommu.c for a driver that never enables dma-iommu.c and uses def_default_domain to prevent it from ever being used anyhow? :( > + if (!domain->geometry.force_aperture) > + return -EINVAL; We need more checks than just this, the 'blocking domain is an empty unmanaged' thing needs it, there is a check in virtio-iommu that looks wonky and the name doesn't really convay what it does anymore.. "partial_translation" or something would be better. I still prefer deleting tegra-gart and deleting force_aperture completely. Jason
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c index 7a9f0b0bddbd..4693b021d54f 100644 --- a/drivers/iommu/dma-iommu.c +++ b/drivers/iommu/dma-iommu.c @@ -548,24 +548,19 @@ static int iommu_dma_init_domain(struct iommu_domain *domain, dma_addr_t base, if (!cookie || cookie->type != IOMMU_DMA_IOVA_COOKIE) return -EINVAL; + /* + * If the IOMMU only offers a non-isolated GART-style translation + * aperture, just let the device use dma-direct. + */ + if (!domain->geometry.force_aperture) + return -EINVAL; + iovad = &cookie->iovad; /* Use the smallest supported page size for IOVA granularity */ order = __ffs(domain->pgsize_bitmap); base_pfn = max_t(unsigned long, 1, base >> order); - /* Check the domain allows at least some access to the device... */ - if (domain->geometry.force_aperture) { - if (base > domain->geometry.aperture_end || - limit < domain->geometry.aperture_start) { - pr_warn("specified DMA range outside IOMMU capability\n"); - return -EFAULT; - } - /* ...then finally give it a kicking to make sure it fits */ - base_pfn = max_t(unsigned long, base_pfn, - domain->geometry.aperture_start >> order); - } - /* start_pfn is always nonzero for an already-initialised domain */ mutex_lock(&cookie->mutex); if (iovad->start_pfn) { diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c index 8a0a5e5d049f..3ca813919974 100644 --- a/drivers/iommu/mtk_iommu_v1.c +++ b/drivers/iommu/mtk_iommu_v1.c @@ -281,6 +281,10 @@ static struct iommu_domain *mtk_iommu_v1_domain_alloc(unsigned type) if (!dom) return NULL; + dom->domain.geometry.aperture_start = 0; + dom->domain.geometry.aperture_end = SZ_4G - 1; + dom->domain.geometry.force_aperture = true; + return &dom->domain; } diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c index 39e34fdeccda..eb684d8807ca 100644 --- a/drivers/iommu/sprd-iommu.c +++ b/drivers/iommu/sprd-iommu.c @@ -148,6 +148,7 @@ static struct iommu_domain *sprd_iommu_domain_alloc(unsigned int domain_type) dom->domain.geometry.aperture_start = 0; dom->domain.geometry.aperture_end = SZ_256M - 1; + dom->domain.geometry.force_aperture = true; return &dom->domain; } diff --git a/drivers/iommu/tegra-gart.c b/drivers/iommu/tegra-gart.c index 0a121cbc17b8..c221af290798 100644 --- a/drivers/iommu/tegra-gart.c +++ b/drivers/iommu/tegra-gart.c @@ -160,7 +160,7 @@ static struct iommu_domain *gart_iommu_domain_alloc(unsigned type) domain->domain.geometry.aperture_start = gart->iovmm_base; domain->domain.geometry.aperture_end = gart->iovmm_end - 1; - domain->domain.geometry.force_aperture = true; + domain->domain.geometry.force_aperture = false; num_pages = (gart->iovmm_end - gart->iovmm_base) / GART_PAGE_SIZE; domain->savedata = vcalloc(num_pages, sizeof(u32));
It was never entirely clear whether the force_aperture flag was supposed to be a hardware capability or a software policy. So far things seem to have leant towards the latter, given that the only drivers not setting the flag are ones where the aperture is seemingly a whole virtual address space such that accesses outside it wouldn't be possible, and the one driver which definitely can't enforce it in hardware *does* still set the flag. On reflection, though, it makes little sense for drivers to dictate usage policy to callers, and the interpretation that a driver setting the flag might also translate addresses outside the given aperture but has for some reason chosen not to is not actually a useful one. It seems a lot more logical to treat the aperture as the absolute limit of what can be translated, and the flag to indicate what would happen if a device did try to access an address outside the aperture, i.e. whether the access would fault or pass through untranslated. As such, reframe the flag consistent with a hardware capability in the hope of clearing up the misconception. Signed-off-by: Robin Murphy <robin.murphy@arm.com> --- drivers/iommu/dma-iommu.c | 19 +++++++------------ drivers/iommu/mtk_iommu_v1.c | 4 ++++ drivers/iommu/sprd-iommu.c | 1 + drivers/iommu/tegra-gart.c | 2 +- 4 files changed, 13 insertions(+), 13 deletions(-)