[PATCH/RFC] iommu/dma: Per-domain flag to control size-alignment
diff mbox

Message ID 148549826507.23190.1460689571069015835.sendpatchset@little-apple
State Under Review
Delegated to: Geert Uytterhoeven
Headers show

Commit Message

Magnus Damm Jan. 27, 2017, 6:24 a.m. UTC
From: Magnus Damm <damm+renesas@opensource.se>

Introduce the flag "no_size_align" to allow disabling size-alignment
on a per-domain basis. This follows the suggestion by the comment
in the code, however a per-device control may be preferred?

Needed to make virtual space contiguous for certain devices.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 drivers/iommu/dma-iommu.c |    6 +++++-
 include/linux/iommu.h     |    1 +
 2 files changed, 6 insertions(+), 1 deletion(-)

Comments

Robin Murphy Jan. 27, 2017, 5:38 p.m. UTC | #1
Hi Magnus,

On 27/01/17 06:24, Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
> 
> Introduce the flag "no_size_align" to allow disabling size-alignment
> on a per-domain basis. This follows the suggestion by the comment
> in the code, however a per-device control may be preferred?
> 
> Needed to make virtual space contiguous for certain devices.

That sounds very suspicious - a single allocation is contiguous with
itself by definition, and anyone relying on multiple allocations being
contiguous with one another is doing it wrong, because there's no way we
could ever guarantee that (with this allocator, at any rate). I'd be
very reticent to touch this without a specific example of what problem
it solves.

Since I understand all this stuff a lot more deeply now that back when I
first wrote that comment, I'd say that if there really is some real need
to implement this feature, it should be a dma_attr flag, i.e. not even
per-device, but per-allocation. We'd be breaking a behaviour currently
guaranteed by the DMA API, so we need to be really sure the caller is OK
with that - having it be their responsibility to explicitly ask is
definitely safest.

Robin.

> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
> 
>  drivers/iommu/dma-iommu.c |    6 +++++-
>  include/linux/iommu.h     |    1 +
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> --- 0001/drivers/iommu/dma-iommu.c
> +++ work/drivers/iommu/dma-iommu.c	2017-01-27 15:17:50.280607110 +0900
> @@ -209,14 +209,18 @@ static struct iova *__alloc_iova(struct
>  	struct iova_domain *iovad = cookie_iovad(domain);
>  	unsigned long shift = iova_shift(iovad);
>  	unsigned long length = iova_align(iovad, size) >> shift;
> +	bool size_aligned = true;
>  
>  	if (domain->geometry.force_aperture)
>  		dma_limit = min(dma_limit, domain->geometry.aperture_end);
> +
> +	if (domain->no_size_align)
> +		size_aligned = false;
>  	/*
>  	 * 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 >> shift, size_aligned);
>  }
>  
>  /* The IOVA allocator knows what we mapped, so just unmap whatever that was */
> --- 0001/include/linux/iommu.h
> +++ work/include/linux/iommu.h	2017-01-27 15:16:37.630607110 +0900
> @@ -83,6 +83,7 @@ struct iommu_domain {
>  	iommu_fault_handler_t handler;
>  	void *handler_token;
>  	struct iommu_domain_geometry geometry;
> +	bool no_size_align;
>  	void *iova_cookie;
>  };
>  
>
Yoshihiro Shimoda Jan. 30, 2017, 7:20 a.m. UTC | #2
Hi Robin, Magnus,

> -----Original Message-----
> From: Robin Murphy
> Sent: Saturday, January 28, 2017 2:38 AM
> 
> Hi Magnus,
> 
> On 27/01/17 06:24, Magnus Damm wrote:
> > From: Magnus Damm <damm+renesas@opensource.se>
> >
> > Introduce the flag "no_size_align" to allow disabling size-alignment
> > on a per-domain basis. This follows the suggestion by the comment
> > in the code, however a per-device control may be preferred?
> >
> > Needed to make virtual space contiguous for certain devices.
> 
> That sounds very suspicious - a single allocation is contiguous with
> itself by definition, and anyone relying on multiple allocations being
> contiguous with one another is doing it wrong, because there's no way we
> could ever guarantee that (with this allocator, at any rate). I'd be
> very reticent to touch this without a specific example of what problem
> it solves.

Thank you for the comment! This patch was from my request.
But, I completely misunderstood this "size-alignment" behavior.
And, my concern was already resolved by the following patch at last April:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/iommu?id=809eac54cdd62c67afea1e17080e681dfa33dc09

So, no one needs this patch anymore.

Best regards,
Yoshihiro Shimoda
Robin Murphy Jan. 30, 2017, 12:07 p.m. UTC | #3
On 30/01/17 07:20, Yoshihiro Shimoda wrote:
> Hi Robin, Magnus,
> 
>> -----Original Message-----
>> From: Robin Murphy
>> Sent: Saturday, January 28, 2017 2:38 AM
>>
>> Hi Magnus,
>>
>> On 27/01/17 06:24, Magnus Damm wrote:
>>> From: Magnus Damm <damm+renesas@opensource.se>
>>>
>>> Introduce the flag "no_size_align" to allow disabling size-alignment
>>> on a per-domain basis. This follows the suggestion by the comment
>>> in the code, however a per-device control may be preferred?
>>>
>>> Needed to make virtual space contiguous for certain devices.
>>
>> That sounds very suspicious - a single allocation is contiguous with
>> itself by definition, and anyone relying on multiple allocations being
>> contiguous with one another is doing it wrong, because there's no way we
>> could ever guarantee that (with this allocator, at any rate). I'd be
>> very reticent to touch this without a specific example of what problem
>> it solves.
> 
> Thank you for the comment! This patch was from my request.
> But, I completely misunderstood this "size-alignment" behavior.
> And, my concern was already resolved by the following patch at last April:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/iommu?id=809eac54cdd62c67afea1e17080e681dfa33dc09
> 
> So, no one needs this patch anymore.

Cool, thanks for the clarification :)

Robin.

> 
> Best regards,
> Yoshihiro Shimoda
>

Patch
diff mbox

--- 0001/drivers/iommu/dma-iommu.c
+++ work/drivers/iommu/dma-iommu.c	2017-01-27 15:17:50.280607110 +0900
@@ -209,14 +209,18 @@  static struct iova *__alloc_iova(struct
 	struct iova_domain *iovad = cookie_iovad(domain);
 	unsigned long shift = iova_shift(iovad);
 	unsigned long length = iova_align(iovad, size) >> shift;
+	bool size_aligned = true;
 
 	if (domain->geometry.force_aperture)
 		dma_limit = min(dma_limit, domain->geometry.aperture_end);
+
+	if (domain->no_size_align)
+		size_aligned = false;
 	/*
 	 * 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 >> shift, size_aligned);
 }
 
 /* The IOVA allocator knows what we mapped, so just unmap whatever that was */
--- 0001/include/linux/iommu.h
+++ work/include/linux/iommu.h	2017-01-27 15:16:37.630607110 +0900
@@ -83,6 +83,7 @@  struct iommu_domain {
 	iommu_fault_handler_t handler;
 	void *handler_token;
 	struct iommu_domain_geometry geometry;
+	bool no_size_align;
 	void *iova_cookie;
 };