[RFC][PATCH 1/2] Introduce the IOMMU_DEVICE flag.

[RFC][PATCH 1/2] Introduce the IOMMU_DEVICE flag.

[Varun Sethi]

This is used for indicating device memory type for a DMA transaction. IOMMU
driver would set up attributes indicationg access to device memory.

Signed-off-by: Varun Sethi <Varun.Sethi-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 include/linux/iommu.h |    1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 20f9a52..0599fe1 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -28,6 +28,7 @@
 #define IOMMU_WRITE	(1 << 1)
 #define IOMMU_CACHE	(1 << 2) /* DMA cache coherency */
 #define IOMMU_EXEC	(1 << 3)
+#define IOMMU_DEVICE	(1 << 4) /* Indicates access to device memory */
 
 struct iommu_ops;
 struct iommu_group;
-- 
1.7.9.5

[RFC][PATCH 2/2] Add support of the IOMMU_DEVICE flag.

[Varun Sethi]

This flag is used for specifying access to device memory. SMMU would apply
device memory attributes for a DMA transaction. This is required for setting
access to GIC registers, for generating message interrupts. This would ensure that 
transactions targetting device memory are not gathered or reordered.

Signed-off-by: Varun Sethi <Varun.Sethi-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 drivers/iommu/arm-smmu.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index ca18d6d..f8338d6 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1263,6 +1263,10 @@ static int arm_smmu_alloc_init_pte(struct arm_smmu_device *smmu, pmd_t *pmd,
 		if (prot & IOMMU_CACHE)
 			pteval |= (MAIR_ATTR_IDX_CACHE <<
 				   ARM_SMMU_PTE_ATTRINDX_SHIFT);
+
+		if (prot & IOMMU_DEVICE)
+			pteval |= (MAIR_ATTR_IDX_DEV <<
+				   ARM_SMMU_PTE_ATTRINDX_SHIFT);
 	} else {
 		pteval |= ARM_SMMU_PTE_HAP_FAULT;
 		if (prot & IOMMU_READ)
@@ -1273,6 +1277,9 @@ static int arm_smmu_alloc_init_pte(struct arm_smmu_device *smmu, pmd_t *pmd,
 			pteval |= ARM_SMMU_PTE_MEMATTR_OIWB;
 		else
 			pteval |= ARM_SMMU_PTE_MEMATTR_NC;
+
+		if (prot & IOMMU_DEVICE)
+			pteval |= ARM_SMMU_PTE_MEMATTR_DEV;
 	}
 
 	/* If no access, create a faulting entry to avoid TLB fills */
-- 
1.7.9.5

Re: [RFC][PATCH 2/2] Add support of the IOMMU_DEVICE flag.

[Will Deacon]

On Mon, Oct 06, 2014 at 11:28:16AM +0100, Varun Sethi wrote:
> This flag is used for specifying access to device memory. SMMU would apply
> device memory attributes for a DMA transaction. This is required for setting
> access to GIC registers, for generating message interrupts. This would ensure that 
> transactions targetting device memory are not gathered or reordered.
> 
> Signed-off-by: Varun Sethi <Varun.Sethi-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index ca18d6d..f8338d6 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1263,6 +1263,10 @@ static int arm_smmu_alloc_init_pte(struct arm_smmu_device *smmu, pmd_t *pmd,
>  		if (prot & IOMMU_CACHE)
>  			pteval |= (MAIR_ATTR_IDX_CACHE <<
>  				   ARM_SMMU_PTE_ATTRINDX_SHIFT);
> +
> +		if (prot & IOMMU_DEVICE)
> +			pteval |= (MAIR_ATTR_IDX_DEV <<
> +				   ARM_SMMU_PTE_ATTRINDX_SHIFT);

This is slightly odd, as I could in theory pass IOMMU_CACHE | IOMMU_DEVICE
to iommu_map.

Will

RE: [RFC][PATCH 2/2] Add support of the IOMMU_DEVICE flag.

[Varun Sethi]

> -----Original Message-----
> From: Will Deacon [mailto:will.deacon-5wv7dgnIgG8@public.gmane.org]
> Sent: Monday, October 06, 2014 4:47 PM
> To: Sethi Varun-B16395
> Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> Subject: Re: [RFC][PATCH 2/2] Add support of the IOMMU_DEVICE flag.
> 
> On Mon, Oct 06, 2014 at 11:28:16AM +0100, Varun Sethi wrote:
> > This flag is used for specifying access to device memory. SMMU would
> > apply device memory attributes for a DMA transaction. This is required
> > for setting access to GIC registers, for generating message
> > interrupts. This would ensure that transactions targetting device memory are
> not gathered or reordered.
> >
> > Signed-off-by: Varun Sethi <Varun.Sethi-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > ---
> >  drivers/iommu/arm-smmu.c |    7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index
> > ca18d6d..f8338d6 100644
> > --- a/drivers/iommu/arm-smmu.c
> > +++ b/drivers/iommu/arm-smmu.c
> > @@ -1263,6 +1263,10 @@ static int arm_smmu_alloc_init_pte(struct
> arm_smmu_device *smmu, pmd_t *pmd,
> >  		if (prot & IOMMU_CACHE)
> >  			pteval |= (MAIR_ATTR_IDX_CACHE <<
> >  				   ARM_SMMU_PTE_ATTRINDX_SHIFT);
> > +
> > +		if (prot & IOMMU_DEVICE)
> > +			pteval |= (MAIR_ATTR_IDX_DEV <<
> > +				   ARM_SMMU_PTE_ATTRINDX_SHIFT);
> 
> This is slightly odd, as I could in theory pass IOMMU_CACHE | IOMMU_DEVICE
> to iommu_map.
Agreed, this should only be relevant for the non-cacheable case.

-Varun

Re: [RFC][PATCH 2/2] Add support of the IOMMU_DEVICE flag.

[Mitchel Humpherys]

On Mon, Oct 06 2014 at 03:28:16 AM, Varun Sethi <Varun.Sethi-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> This flag is used for specifying access to device memory. SMMU would apply
> device memory attributes for a DMA transaction. This is required for setting
> access to GIC registers, for generating message interrupts. This would ensure that 

Nit: long line and trailing whitespace.


-Mitch

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [RFC][PATCH 1/2] Introduce the IOMMU_DEVICE flag.

[Will Deacon]

Hi Varun,

[adding the Qualcomm guys, as I have an open question below]

On Mon, Oct 06, 2014 at 11:28:15AM +0100, Varun Sethi wrote:
> This is used for indicating device memory type for a DMA transaction. IOMMU
> driver would set up attributes indicationg access to device memory.
> 
> Signed-off-by: Varun Sethi <Varun.Sethi-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  include/linux/iommu.h |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 20f9a52..0599fe1 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -28,6 +28,7 @@
>  #define IOMMU_WRITE	(1 << 1)
>  #define IOMMU_CACHE	(1 << 2) /* DMA cache coherency */
>  #define IOMMU_EXEC	(1 << 3)
> +#define IOMMU_DEVICE	(1 << 4) /* Indicates access to device memory */

An alternative to this would be to make device-memory the default type for
!IOMMU_CACHE mappings (i.e. MAIR index 0).

I'd value feedback either way; the argument comes down to whether we should
use normal non-cacheable or device-nGnRE as the default (!IOMMU_CACHE) memory
type. The latter is likely to be significantly slower, but provides the
ordering guarantees that you need for MSI delivery.

If we do go down the route of adding a new IOMMU_* option, it probably needs
a better name (IOMMU_MMIO, for example).

Will

RE: [RFC][PATCH 1/2] Introduce the IOMMU_DEVICE flag.

[Varun Sethi]

Hi Will,

> -----Original Message-----
> From: Will Deacon [mailto:will.deacon-5wv7dgnIgG8@public.gmane.org]
> Sent: Monday, October 06, 2014 4:34 PM
> To: Sethi Varun-B16395
> Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org; mitchelh-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org;
> ohaugan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org
> Subject: Re: [RFC][PATCH 1/2] Introduce the IOMMU_DEVICE flag.
> 
> Hi Varun,
> 
> [adding the Qualcomm guys, as I have an open question below]
> 
> On Mon, Oct 06, 2014 at 11:28:15AM +0100, Varun Sethi wrote:
> > This is used for indicating device memory type for a DMA transaction.
> > IOMMU driver would set up attributes indicationg access to device memory.
> >
> > Signed-off-by: Varun Sethi <Varun.Sethi-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > ---
> >  include/linux/iommu.h |    1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/include/linux/iommu.h b/include/linux/iommu.h index
> > 20f9a52..0599fe1 100644
> > --- a/include/linux/iommu.h
> > +++ b/include/linux/iommu.h
> > @@ -28,6 +28,7 @@
> >  #define IOMMU_WRITE	(1 << 1)
> >  #define IOMMU_CACHE	(1 << 2) /* DMA cache coherency */
> >  #define IOMMU_EXEC	(1 << 3)
> > +#define IOMMU_DEVICE	(1 << 4) /* Indicates access to device memory
> */
> 
> An alternative to this would be to make device-memory the default type for
> !IOMMU_CACHE mappings (i.e. MAIR index 0).
> 
> I'd value feedback either way; the argument comes down to whether we
> should use normal non-cacheable or device-nGnRE as the default
> (!IOMMU_CACHE) memory type. The latter is likely to be significantly slower,
> but provides the ordering guarantees that you need for MSI delivery.
> 
> If we do go down the route of adding a new IOMMU_* option, it probably
> needs a better name (IOMMU_MMIO, for example).
I think it would be better to have a  separate flag for handling DMA access to device memory. There might be cases where we might want DMA accesses to be non cacheable.  This may be required for preventing cache evictions (depending on the cache size).

-Varun

Re: [RFC][PATCH 1/2] Introduce the IOMMU_DEVICE flag.

[Olav Haugan]

On 10/6/2014 4:04 AM, Will Deacon wrote:
> Hi Varun,
>
> [adding the Qualcomm guys, as I have an open question below]
>
> On Mon, Oct 06, 2014 at 11:28:15AM +0100, Varun Sethi wrote:
>> This is used for indicating device memory type for a DMA transaction. IOMMU
>> driver would set up attributes indicationg access to device memory.
>>
>> Signed-off-by: Varun Sethi <Varun.Sethi-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>> ---
>>   include/linux/iommu.h |    1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
>> index 20f9a52..0599fe1 100644
>> --- a/include/linux/iommu.h
>> +++ b/include/linux/iommu.h
>> @@ -28,6 +28,7 @@
>>   #define IOMMU_WRITE	(1 << 1)
>>   #define IOMMU_CACHE	(1 << 2) /* DMA cache coherency */
>>   #define IOMMU_EXEC	(1 << 3)
>> +#define IOMMU_DEVICE	(1 << 4) /* Indicates access to device memory */
>
> An alternative to this would be to make device-memory the default type for
> !IOMMU_CACHE mappings (i.e. MAIR index 0).
>
> I'd value feedback either way; the argument comes down to whether we should
> use normal non-cacheable or device-nGnRE as the default (!IOMMU_CACHE) memory
> type. The latter is likely to be significantly slower, but provides the
> ordering guarantees that you need for MSI delivery.

We would prefer to stay with normal non-cacheable as the default since 
this is our most common use case and this is what is the default now.

.Olav

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation