[PATCH] iommu/dma-iommu: properly respect configured address space size

[PATCH] iommu/dma-iommu: properly respect configured address space size

[Marek Szyprowski]

When one called iommu_dma_init_domain() with size smaller than device's
DMA mask, the alloc_iova() will not respect it and always assume that all
IOVA addresses will be allocated from the the (base ... dev->dma_mask) range.

This patch fixes this issue by taking the configured address space size
parameter into account (if it is smaller than the device's dma_mask).

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/iommu/dma-iommu.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index c5ab8667e6f2..8b4b72654359 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -212,11 +212,13 @@ static struct iova *__alloc_iova(struct iommu_domain *domain, size_t size,
 
 	if (domain->geometry.force_aperture)
 		dma_limit = min(dma_limit, domain->geometry.aperture_end);
+
+	dma_limit = min(dma_limit >> shift, (dma_addr_t)iovad->dma_32bit_pfn);
 	/*
 	 * Enforce size-alignment to be safe - there could perhaps be an
 	 * attribute to control this per-device, or at least per-domain...
 	 */
-	return alloc_iova(iovad, length, dma_limit >> shift, true);
+	return alloc_iova(iovad, length, dma_limit, true);
 }
 
 /* The IOVA allocator knows what we mapped, so just unmap whatever that was */
-- 
1.9.1

Re: [PATCH] iommu/dma-iommu: properly respect configured address space size

[Robin Murphy]

Hi Marek,

On 07/11/16 13:06, Marek Szyprowski wrote:
> When one called iommu_dma_init_domain() with size smaller than device's
> DMA mask, the alloc_iova() will not respect it and always assume that all
> IOVA addresses will be allocated from the the (base ... dev->dma_mask) range.

Is that actually a problem for anything?

> This patch fixes this issue by taking the configured address space size
> parameter into account (if it is smaller than the device's dma_mask).

TBH I've been pondering ripping the size stuff out of dma-iommu, as it
all stems from me originally failing to understand what dma_32bit_pfn is
actually for. The truth is that iova_domains just don't have a size or
upper limit; however if devices with both large and small DMA masks
share a domain, then the top-down nature of the allocator means that
allocating for the less-capable devices would involve walking through
every out-of-range entry in the tree every time. Having cached32_node
based on dma_32bit_pfn just provides an optimised starting point for
searching within the smaller mask.

Would it hurt any of your use-cases to relax/rework the reinitialisation
checks in iommu_dma_init_domain()? Alternatively if we really do have a
case for wanting a hard upper limit, it might make more sense to add an
end_pfn to the iova_domain and handle it in the allocator itself.

Robin.

> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/iommu/dma-iommu.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index c5ab8667e6f2..8b4b72654359 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -212,11 +212,13 @@ static struct iova *__alloc_iova(struct iommu_domain *domain, size_t size,
>  
>  	if (domain->geometry.force_aperture)
>  		dma_limit = min(dma_limit, domain->geometry.aperture_end);
> +
> +	dma_limit = min(dma_limit >> shift, (dma_addr_t)iovad->dma_32bit_pfn);
>  	/*
>  	 * Enforce size-alignment to be safe - there could perhaps be an
>  	 * attribute to control this per-device, or at least per-domain...
>  	 */
> -	return alloc_iova(iovad, length, dma_limit >> shift, true);
> +	return alloc_iova(iovad, length, dma_limit, true);
>  }
>  
>  /* The IOVA allocator knows what we mapped, so just unmap whatever that was */
> 

Re: [PATCH] iommu/dma-iommu: properly respect configured address space size

[Marek Szyprowski]

Hi Robin,


On 2016-11-08 12:37, Robin Murphy wrote:
> On 07/11/16 13:06, Marek Szyprowski wrote:
>> When one called iommu_dma_init_domain() with size smaller than device's
>> DMA mask, the alloc_iova() will not respect it and always assume that all
>> IOVA addresses will be allocated from the the (base ... dev->dma_mask) range.
> Is that actually a problem for anything?

Yes, I found this issue while working on next version of ARM & ARM64
DMA-mapping/IOMMU integration patchset and adapting Exynos drivers for the
new IOMMU/DMA-mapping glue.

Some Exynos devices (codec and camera isp) operate only on the limited (and
really small: 256M for example) DMA window. They use non-standard way of
addressing memory: an offset from the firmware base. However they still have
32bit DMA mask, as the firmware can be located basically everywhere in the
real DMA address space, but then they can access only next 256M from that
firmware base.

>
>> This patch fixes this issue by taking the configured address space size
>> parameter into account (if it is smaller than the device's dma_mask).
> TBH I've been pondering ripping the size stuff out of dma-iommu, as it
> all stems from me originally failing to understand what dma_32bit_pfn is
> actually for. The truth is that iova_domains just don't have a size or
> upper limit; however if devices with both large and small DMA masks
> share a domain, then the top-down nature of the allocator means that
> allocating for the less-capable devices would involve walking through
> every out-of-range entry in the tree every time. Having cached32_node
> based on dma_32bit_pfn just provides an optimised starting point for
> searching within the smaller mask.

Right, this dma_32bit_pfn was really misleading at the first glance,
but then I found that this was something like end_pfn in case of dma-iommu
code.

> Would it hurt any of your use-cases to relax/rework the reinitialisation
> checks in iommu_dma_init_domain()? Alternatively if we really do have a
> case for wanting a hard upper limit, it might make more sense to add an
> end_pfn to the iova_domain and handle it in the allocator itself.

The proper support for end_pfn would be a better approach probably,
especially if we consider readability of the code.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] iommu/dma-iommu: properly respect configured address space size

[Robin Murphy]

On 08/11/16 13:41, Marek Szyprowski wrote:
> Hi Robin,
> 
> 
> On 2016-11-08 12:37, Robin Murphy wrote:
>> On 07/11/16 13:06, Marek Szyprowski wrote:
>>> When one called iommu_dma_init_domain() with size smaller than device's
>>> DMA mask, the alloc_iova() will not respect it and always assume that
>>> all
>>> IOVA addresses will be allocated from the the (base ...
>>> dev->dma_mask) range.
>> Is that actually a problem for anything?
> 
> Yes, I found this issue while working on next version of ARM & ARM64
> DMA-mapping/IOMMU integration patchset and adapting Exynos drivers for the
> new IOMMU/DMA-mapping glue.
> 
> Some Exynos devices (codec and camera isp) operate only on the limited (and
> really small: 256M for example) DMA window. They use non-standard way of
> addressing memory: an offset from the firmware base. However they still
> have
> 32bit DMA mask, as the firmware can be located basically everywhere in the
> real DMA address space, but then they can access only next 256M from that
> firmware base.

OK, that's good to know, thanks. However, I think in this case it sounds
like it's really the DMA mask that's the underlying problem - if those
blocks themselves can only drive 28 address bits, then the struct
devices representing them should have 28-bit DMA masks, and the
"firmware base" of whoever's driving the upper bits modelled with a
dma_pfn_offset. Even if they do have full 32-bit interfaces themselves,
but are constrained to segment-offset addressing internally, I still
think it would be tidier to represent things that way.

At some point in dma-iommu development I did have support for DMA
offsets upstream of the IOMMU, and am happy to reinstate it if there's a
real use case (assuming you can't simply always set the firmware base to
0 when using the IOMMU).

>>> This patch fixes this issue by taking the configured address space size
>>> parameter into account (if it is smaller than the device's dma_mask).
>> TBH I've been pondering ripping the size stuff out of dma-iommu, as it
>> all stems from me originally failing to understand what dma_32bit_pfn is
>> actually for. The truth is that iova_domains just don't have a size or
>> upper limit; however if devices with both large and small DMA masks
>> share a domain, then the top-down nature of the allocator means that
>> allocating for the less-capable devices would involve walking through
>> every out-of-range entry in the tree every time. Having cached32_node
>> based on dma_32bit_pfn just provides an optimised starting point for
>> searching within the smaller mask.
> 
> Right, this dma_32bit_pfn was really misleading at the first glance,
> but then I found that this was something like end_pfn in case of dma-iommu
> code.

Yes, that was my incorrect assumption - at the time I interpreted it as
a de-facto upper limit which was still possible to allocate above in
special circumstances, which turns out to be almost entirely backwards.
I'd rather not bake that into the dma-iommu code any further if we can
avoid it (I'll try throwing an RFC together to clear up what's there
already).

Robin.

>> Would it hurt any of your use-cases to relax/rework the reinitialisation
>> checks in iommu_dma_init_domain()? Alternatively if we really do have a
>> case for wanting a hard upper limit, it might make more sense to add an
>> end_pfn to the iova_domain and handle it in the allocator itself.
> 
> The proper support for end_pfn would be a better approach probably,
> especially if we consider readability of the code.
> 
> Best regards

Re: [PATCH] iommu/dma-iommu: properly respect configured address space size

[Marek Szyprowski]

Hi Robin,


On 2016-11-08 15:44, Robin Murphy wrote:
> On 08/11/16 13:41, Marek Szyprowski wrote:
>> On 2016-11-08 12:37, Robin Murphy wrote:
>>> On 07/11/16 13:06, Marek Szyprowski wrote:
>>>> When one called iommu_dma_init_domain() with size smaller than device's
>>>> DMA mask, the alloc_iova() will not respect it and always assume that
>>>> all
>>>> IOVA addresses will be allocated from the the (base ...
>>>> dev->dma_mask) range.
>>> Is that actually a problem for anything?
>> Yes, I found this issue while working on next version of ARM & ARM64
>> DMA-mapping/IOMMU integration patchset and adapting Exynos drivers for the
>> new IOMMU/DMA-mapping glue.
>>
>> Some Exynos devices (codec and camera isp) operate only on the limited (and
>> really small: 256M for example) DMA window. They use non-standard way of
>> addressing memory: an offset from the firmware base. However they still
>> have
>> 32bit DMA mask, as the firmware can be located basically everywhere in the
>> real DMA address space, but then they can access only next 256M from that
>> firmware base.
> OK, that's good to know, thanks. However, I think in this case it sounds
> like it's really the DMA mask that's the underlying problem - if those
> blocks themselves can only drive 28 address bits, then the struct
> devices representing them should have 28-bit DMA masks, and the
> "firmware base" of whoever's driving the upper bits modelled with a
> dma_pfn_offset. Even if they do have full 32-bit interfaces themselves,
> but are constrained to segment-offset addressing internally, I still
> think it would be tidier to represent things that way.
>
> At some point in dma-iommu development I did have support for DMA
> offsets upstream of the IOMMU, and am happy to reinstate it if there's a
> real use case (assuming you can't simply always set the firmware base to
> 0 when using the IOMMU).

That would indeed look a bit simpler, but I've already tried such approach
and the firmware crashes when its base in real DMA address space is set to
zero.

>>>> This patch fixes this issue by taking the configured address space size
>>>> parameter into account (if it is smaller than the device's dma_mask).
>>> TBH I've been pondering ripping the size stuff out of dma-iommu, as it
>>> all stems from me originally failing to understand what dma_32bit_pfn is
>>> actually for. The truth is that iova_domains just don't have a size or
>>> upper limit; however if devices with both large and small DMA masks
>>> share a domain, then the top-down nature of the allocator means that
>>> allocating for the less-capable devices would involve walking through
>>> every out-of-range entry in the tree every time. Having cached32_node
>>> based on dma_32bit_pfn just provides an optimised starting point for
>>> searching within the smaller mask.
>> Right, this dma_32bit_pfn was really misleading at the first glance,
>> but then I found that this was something like end_pfn in case of dma-iommu
>> code.
> Yes, that was my incorrect assumption - at the time I interpreted it as
> a de-facto upper limit which was still possible to allocate above in
> special circumstances, which turns out to be almost entirely backwards.
> I'd rather not bake that into the dma-iommu code any further if we can
> avoid it (I'll try throwing an RFC together to clear up what's there
> already).

Okay.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] iommu/dma-iommu: properly respect configured address space size

[Robin Murphy]

On 08/11/16 14:53, Marek Szyprowski wrote:
> Hi Robin,
> 
> 
> On 2016-11-08 15:44, Robin Murphy wrote:
>> On 08/11/16 13:41, Marek Szyprowski wrote:
>>> On 2016-11-08 12:37, Robin Murphy wrote:
>>>> On 07/11/16 13:06, Marek Szyprowski wrote:
>>>>> When one called iommu_dma_init_domain() with size smaller than
>>>>> device's
>>>>> DMA mask, the alloc_iova() will not respect it and always assume that
>>>>> all
>>>>> IOVA addresses will be allocated from the the (base ...
>>>>> dev->dma_mask) range.
>>>> Is that actually a problem for anything?
>>> Yes, I found this issue while working on next version of ARM & ARM64
>>> DMA-mapping/IOMMU integration patchset and adapting Exynos drivers
>>> for the
>>> new IOMMU/DMA-mapping glue.
>>>
>>> Some Exynos devices (codec and camera isp) operate only on the
>>> limited (and
>>> really small: 256M for example) DMA window. They use non-standard way of
>>> addressing memory: an offset from the firmware base. However they still
>>> have
>>> 32bit DMA mask, as the firmware can be located basically everywhere
>>> in the
>>> real DMA address space, but then they can access only next 256M from
>>> that
>>> firmware base.
>> OK, that's good to know, thanks. However, I think in this case it sounds
>> like it's really the DMA mask that's the underlying problem - if those
>> blocks themselves can only drive 28 address bits, then the struct
>> devices representing them should have 28-bit DMA masks, and the
>> "firmware base" of whoever's driving the upper bits modelled with a
>> dma_pfn_offset. Even if they do have full 32-bit interfaces themselves,
>> but are constrained to segment-offset addressing internally, I still
>> think it would be tidier to represent things that way.
>>
>> At some point in dma-iommu development I did have support for DMA
>> offsets upstream of the IOMMU, and am happy to reinstate it if there's a
>> real use case (assuming you can't simply always set the firmware base to
>> 0 when using the IOMMU).
> 
> That would indeed look a bit simpler, but I've already tried such approach
> and the firmware crashes when its base in real DMA address space is set to
> zero.

Ah, sadly I'm not too surprised... ;)

Having pondered it a little more, what if the firmware base is instead
set equal to the max offset, then you can have a power-of-two DMA mask
at twice that size (e.g. 512M) such that the top-down IOVA allocations
start in the right place. That would appear to achieve pretty much the
same result as this patch, but more robustly.

Incidentally, how do the init size and DMA mask end up different in the
first place? Is this another case of the "dma-ranges" info from
of_dma_configure() getting clobbered by a driver calling dma_set_mask()
later?

Robin.

>>>>> This patch fixes this issue by taking the configured address space
>>>>> size
>>>>> parameter into account (if it is smaller than the device's dma_mask).
>>>> TBH I've been pondering ripping the size stuff out of dma-iommu, as it
>>>> all stems from me originally failing to understand what
>>>> dma_32bit_pfn is
>>>> actually for. The truth is that iova_domains just don't have a size or
>>>> upper limit; however if devices with both large and small DMA masks
>>>> share a domain, then the top-down nature of the allocator means that
>>>> allocating for the less-capable devices would involve walking through
>>>> every out-of-range entry in the tree every time. Having cached32_node
>>>> based on dma_32bit_pfn just provides an optimised starting point for
>>>> searching within the smaller mask.
>>> Right, this dma_32bit_pfn was really misleading at the first glance,
>>> but then I found that this was something like end_pfn in case of
>>> dma-iommu
>>> code.
>> Yes, that was my incorrect assumption - at the time I interpreted it as
>> a de-facto upper limit which was still possible to allocate above in
>> special circumstances, which turns out to be almost entirely backwards.
>> I'd rather not bake that into the dma-iommu code any further if we can
>> avoid it (I'll try throwing an RFC together to clear up what's there
>> already).
> 
> Okay.
> 
> Best regards

Re: [PATCH] iommu/dma-iommu: properly respect configured address space size

[Joerg Roedel]

On Tue, Nov 08, 2016 at 11:37:23AM +0000, Robin Murphy wrote:
> TBH I've been pondering ripping the size stuff out of dma-iommu, as it
> all stems from me originally failing to understand what dma_32bit_pfn is
> actually for.

The point of dma_32bit_pfn is to allocate dma-address below 4G by
default. This is a performance optimization so that even devices capable
of 64bit DMA are using SAC by default instead of DAC.

Since it is the goal to share a dma-iommu implemenation between
architectures, I would rather prefer not to rip this stuff out.


	Joerg

Re: [PATCH] iommu/dma-iommu: properly respect configured address space size

[Robin Murphy]

On 10/11/16 15:59, Joerg Roedel wrote:
> On Tue, Nov 08, 2016 at 11:37:23AM +0000, Robin Murphy wrote:
>> TBH I've been pondering ripping the size stuff out of dma-iommu, as it
>> all stems from me originally failing to understand what dma_32bit_pfn is
>> actually for.
> 
> The point of dma_32bit_pfn is to allocate dma-address below 4G by
> default. This is a performance optimization so that even devices capable
> of 64bit DMA are using SAC by default instead of DAC.
> 
> Since it is the goal to share a dma-iommu implemenation between
> architectures, I would rather prefer not to rip this stuff out.

Oh, I didn't mean rip it out entirely, just get rid of the bogus
assumption that it's the "size" of the domain, especially when given a
>32-bit DMA mask, since that defeats the very optimisation I do now
understand (although it might still be OK for platform devices where
SAC/DAC doesn't apply, to avoid the rb_last() overhead every time).

>From the patch I've started, "rip it out" turns out to actually be
mostly "rewrite the comments" anyway - I'll post something soon.

Robin.