[RFC PATCH 0/4] Genericise the IOVA allocator

[RFC PATCH 0/4] Genericise the IOVA allocator

[Robin Murphy]

Hi all,

I've been implementing IOMMU DMA mapping for arm64, based on tidied-up
parts of the existing arch/arm/mm/dma-mapping.c with a clear divide
between the arch-specific parts and the general DMA-API to IOMMU-API layer
so that that can be shared; similar to what Ritesh started before and was
unable to complete[1], but working in the other direction.

The first part of that tidy-up involved ripping out the homebrewed IOVA
allocator and plumbing in iova.c, necessitating the changes presented here.
The rest is currently sat under arch/arm64 for the sake of getting it
working quickly with minimal impact - ideally I'd move it out and port
arch/arm before merging, but I don't know quite how impatient people are.
Regardless of that decision, this bit stands alone, so here it is.

Feel free to ignore patches 1 and 2, since I see Sakari has recently
posted a more thorough series for that[2], that frankly looks nicer ;)
I've merely left them in as context here.

[1]:http://thread.gmane.org/gmane.linux.ports.arm.kernel/331299
[2]:http://article.gmane.org/gmane.linux.kernel.iommu/7436

Robin Murphy (4):
  iommu: build iova.c for any IOMMU
  iommu: consolidate IOVA allocator code
  iommu: make IOVA domain low limit flexible
  iommu: make IOVA domain page size explicit

 drivers/iommu/Makefile      |  4 ++--
 drivers/iommu/intel-iommu.c | 45 ++++++++++-------------------------------
 drivers/iommu/iova.c        | 49 +++++++++++++++++++++++++++++++++++++++++----
 include/linux/iova.h        | 41 +++++++++++++++++++++++++++++++++----
 4 files changed, 95 insertions(+), 44 deletions(-)

-- 
1.9.1

[RFC PATCH 1/4] iommu: build iova.c for any IOMMU

[Robin Murphy]

In preparation for sharing the IOVA allocator, build it for all
IOMMU API users.

Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---
 drivers/iommu/Makefile | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 16edef7..9f7910b 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -1,4 +1,4 @@
-obj-$(CONFIG_IOMMU_API) += iommu.o
+obj-$(CONFIG_IOMMU_API) += iommu.o iova.o
 obj-$(CONFIG_IOMMU_API) += iommu-traces.o
 obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
 obj-$(CONFIG_OF_IOMMU)	+= of_iommu.o
@@ -7,7 +7,7 @@ obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o
 obj-$(CONFIG_AMD_IOMMU_V2) += amd_iommu_v2.o
 obj-$(CONFIG_ARM_SMMU) += arm-smmu.o
 obj-$(CONFIG_DMAR_TABLE) += dmar.o
-obj-$(CONFIG_INTEL_IOMMU) += iova.o intel-iommu.o
+obj-$(CONFIG_INTEL_IOMMU) += intel-iommu.o
 obj-$(CONFIG_IPMMU_VMSA) += ipmmu-vmsa.o
 obj-$(CONFIG_IRQ_REMAP) += intel_irq_remapping.o irq_remapping.o
 obj-$(CONFIG_OMAP_IOMMU) += omap-iommu.o
-- 
1.9.1

Re: [RFC PATCH 1/4] iommu: build iova.c for any IOMMU

[leizhen]

On 2014/11/26 1:27, Robin Murphy wrote:
> In preparation for sharing the IOVA allocator, build it for all
> IOMMU API users.
> 
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/iommu/Makefile | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 16edef7..9f7910b 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -1,4 +1,4 @@
> -obj-$(CONFIG_IOMMU_API) += iommu.o
> +obj-$(CONFIG_IOMMU_API) += iommu.o iova.o

I'm not a maintainer, so just code review comment.

I don't known whether or not you will apply iova to all iommu drivers. But if only x86 and arm/arm64,
it's no good to force other iommu drivers to include iova.o, because they have not used it yet.

Maybe it's good to define a new config, like CONFIG_IOMMU_IOVA.

config INTEL_IOMMU
	select IOMMU_IOVA

config ARM_SMMU
	select IOMMU_IOVA

obj-$(CONFIG_IOMMU_IOVA) += iova.o

>  obj-$(CONFIG_IOMMU_API) += iommu-traces.o
>  obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
>  obj-$(CONFIG_OF_IOMMU)	+= of_iommu.o
> @@ -7,7 +7,7 @@ obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o
>  obj-$(CONFIG_AMD_IOMMU_V2) += amd_iommu_v2.o
>  obj-$(CONFIG_ARM_SMMU) += arm-smmu.o
>  obj-$(CONFIG_DMAR_TABLE) += dmar.o
> -obj-$(CONFIG_INTEL_IOMMU) += iova.o intel-iommu.o
> +obj-$(CONFIG_INTEL_IOMMU) += intel-iommu.o
>  obj-$(CONFIG_IPMMU_VMSA) += ipmmu-vmsa.o
>  obj-$(CONFIG_IRQ_REMAP) += intel_irq_remapping.o irq_remapping.o
>  obj-$(CONFIG_OMAP_IOMMU) += omap-iommu.o
> 

Re: [RFC PATCH 1/4] iommu: build iova.c for any IOMMU

[Robin Murphy]

Hi,

On 26/11/14 06:58, leizhen wrote:
> On 2014/11/26 1:27, Robin Murphy wrote:
>> In preparation for sharing the IOVA allocator, build it for all
>> IOMMU API users.
>>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>>   drivers/iommu/Makefile | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
>> index 16edef7..9f7910b 100644
>> --- a/drivers/iommu/Makefile
>> +++ b/drivers/iommu/Makefile
>> @@ -1,4 +1,4 @@
>> -obj-$(CONFIG_IOMMU_API) += iommu.o
>> +obj-$(CONFIG_IOMMU_API) += iommu.o iova.o
>
> I'm not a maintainer, so just code review comment.
>
> I don't known whether or not you will apply iova to all iommu drivers. But if only x86 and arm/arm64,
> it's no good to force other iommu drivers to include iova.o, because they have not used it yet.

I agree, the "build it anyway and rely on the linker to throw it away" 
approach is very hacky. As I mentioned in the cover letter, the other 
series which appeared as I was cleaning this up for posting duplicates 
patch 1 and 2 here, but in a nicer way, so I'm fully expecting to simply 
rebase patches 3 and 4 on top of that series.

Thanks,
Robin.

>
> Maybe it's good to define a new config, like CONFIG_IOMMU_IOVA.
>
> config INTEL_IOMMU
> 	select IOMMU_IOVA
>
> config ARM_SMMU
> 	select IOMMU_IOVA
>
> obj-$(CONFIG_IOMMU_IOVA) += iova.o
>
>>   obj-$(CONFIG_IOMMU_API) += iommu-traces.o
>>   obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
>>   obj-$(CONFIG_OF_IOMMU)	+= of_iommu.o
>> @@ -7,7 +7,7 @@ obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o
>>   obj-$(CONFIG_AMD_IOMMU_V2) += amd_iommu_v2.o
>>   obj-$(CONFIG_ARM_SMMU) += arm-smmu.o
>>   obj-$(CONFIG_DMAR_TABLE) += dmar.o
>> -obj-$(CONFIG_INTEL_IOMMU) += iova.o intel-iommu.o
>> +obj-$(CONFIG_INTEL_IOMMU) += intel-iommu.o
>>   obj-$(CONFIG_IPMMU_VMSA) += ipmmu-vmsa.o
>>   obj-$(CONFIG_IRQ_REMAP) += intel_irq_remapping.o irq_remapping.o
>>   obj-$(CONFIG_OMAP_IOMMU) += omap-iommu.o
>>
>
>
>

[RFC PATCH 2/4] iommu: consolidate IOVA allocator code

[Robin Murphy]

In order to share the IOVA allocator with other architectures, break
the unnecssary dependency on the Intel IOMMU driver and move the
remaining IOVA internals to iova.c

Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---
 drivers/iommu/intel-iommu.c | 33 ++-------------------------------
 drivers/iommu/iova.c        | 35 +++++++++++++++++++++++++++++++++++
 include/linux/iova.h        |  3 +++
 3 files changed, 40 insertions(+), 31 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index a27d6cb..9258860 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -490,7 +490,6 @@ __setup("intel_iommu=", intel_iommu_setup);
 
 static struct kmem_cache *iommu_domain_cache;
 static struct kmem_cache *iommu_devinfo_cache;
-static struct kmem_cache *iommu_iova_cache;
 
 static inline void *alloc_pgtable_page(int node)
 {
@@ -528,16 +527,6 @@ static inline void free_devinfo_mem(void *vaddr)
 	kmem_cache_free(iommu_devinfo_cache, vaddr);
 }
 
-struct iova *alloc_iova_mem(void)
-{
-	return kmem_cache_alloc(iommu_iova_cache, GFP_ATOMIC);
-}
-
-void free_iova_mem(struct iova *iova)
-{
-	kmem_cache_free(iommu_iova_cache, iova);
-}
-
 static inline int domain_type_is_vm(struct dmar_domain *domain)
 {
 	return domain->flags & DOMAIN_FLAG_VIRTUAL_MACHINE;
@@ -3429,23 +3418,6 @@ static inline int iommu_devinfo_cache_init(void)
 	return ret;
 }
 
-static inline int iommu_iova_cache_init(void)
-{
-	int ret = 0;
-
-	iommu_iova_cache = kmem_cache_create("iommu_iova",
-					 sizeof(struct iova),
-					 0,
-					 SLAB_HWCACHE_ALIGN,
-					 NULL);
-	if (!iommu_iova_cache) {
-		printk(KERN_ERR "Couldn't create iova cache\n");
-		ret = -ENOMEM;
-	}
-
-	return ret;
-}
-
 static int __init iommu_init_mempool(void)
 {
 	int ret;
@@ -3463,7 +3435,7 @@ static int __init iommu_init_mempool(void)
 
 	kmem_cache_destroy(iommu_domain_cache);
 domain_error:
-	kmem_cache_destroy(iommu_iova_cache);
+	iommu_iova_cache_destroy();
 
 	return -ENOMEM;
 }
@@ -3472,8 +3444,7 @@ static void __init iommu_exit_mempool(void)
 {
 	kmem_cache_destroy(iommu_devinfo_cache);
 	kmem_cache_destroy(iommu_domain_cache);
-	kmem_cache_destroy(iommu_iova_cache);
-
+	iommu_iova_cache_destroy();
 }
 
 static void quirk_ioat_snb_local_iommu(struct pci_dev *pdev)
diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index f6b17e6..520b8c8 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -18,6 +18,41 @@
  */
 
 #include <linux/iova.h>
+#include <linux/slab.h>
+
+static struct kmem_cache *iommu_iova_cache;
+
+int iommu_iova_cache_init(void)
+{
+	int ret = 0;
+
+	iommu_iova_cache = kmem_cache_create("iommu_iova",
+					 sizeof(struct iova),
+					 0,
+					 SLAB_HWCACHE_ALIGN,
+					 NULL);
+	if (!iommu_iova_cache) {
+		pr_err("Couldn't create iova cache\n");
+		ret = -ENOMEM;
+	}
+
+	return ret;
+}
+
+void iommu_iova_cache_destroy(void)
+{
+	kmem_cache_destroy(iommu_iova_cache);
+}
+
+struct iova *alloc_iova_mem(void)
+{
+	return kmem_cache_alloc(iommu_iova_cache, GFP_ATOMIC);
+}
+
+void free_iova_mem(struct iova *iova)
+{
+	kmem_cache_free(iommu_iova_cache, iova);
+}
 
 void
 init_iova_domain(struct iova_domain *iovad, unsigned long pfn_32bit)
diff --git a/include/linux/iova.h b/include/linux/iova.h
index 19e81d5..ad0507c 100644
--- a/include/linux/iova.h
+++ b/include/linux/iova.h
@@ -39,6 +39,9 @@ static inline unsigned long iova_size(struct iova *iova)
 	return iova->pfn_hi - iova->pfn_lo + 1;
 }
 
+int iommu_iova_cache_init(void);
+void iommu_iova_cache_destroy(void);
+
 struct iova *alloc_iova_mem(void);
 void free_iova_mem(struct iova *iova);
 void free_iova(struct iova_domain *iovad, unsigned long pfn);
-- 
1.9.1

[RFC PATCH 3/4] iommu: make IOVA domain low limit flexible

[Robin Murphy]

To share the IOVA allocator with other architectures, it needs to
accommodate more general aperture restrictions; move the lower limit
from a compile-time constant to a runtime domain property to allow
IOVA domains with different requirements to co-exist.

Also reword the slightly unclear description of alloc_iova since we're
touching it anyway.

Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---
 drivers/iommu/intel-iommu.c |  9 ++++++---
 drivers/iommu/iova.c        | 10 ++++++----
 include/linux/iova.h        |  7 +++----
 3 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 9258860..daba0c2 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -71,6 +71,9 @@
 				__DOMAIN_MAX_PFN(gaw), (unsigned long)-1))
 #define DOMAIN_MAX_ADDR(gaw)	(((uint64_t)__DOMAIN_MAX_PFN(gaw)) << VTD_PAGE_SHIFT)
 
+/* IO virtual address start page frame number */
+#define IOVA_START_PFN		(1)
+
 #define IOVA_PFN(addr)		((addr) >> PAGE_SHIFT)
 #define DMA_32BIT_PFN		IOVA_PFN(DMA_BIT_MASK(32))
 #define DMA_64BIT_PFN		IOVA_PFN(DMA_BIT_MASK(64))
@@ -1629,7 +1632,7 @@ static int dmar_init_reserved_ranges(void)
 	struct iova *iova;
 	int i;
 
-	init_iova_domain(&reserved_iova_list, DMA_32BIT_PFN);
+	init_iova_domain(&reserved_iova_list, IOVA_START_PFN, DMA_32BIT_PFN);
 
 	lockdep_set_class(&reserved_iova_list.iova_rbtree_lock,
 		&reserved_rbtree_key);
@@ -1687,7 +1690,7 @@ static int domain_init(struct dmar_domain *domain, int guest_width)
 	int adjust_width, agaw;
 	unsigned long sagaw;
 
-	init_iova_domain(&domain->iovad, DMA_32BIT_PFN);
+	init_iova_domain(&domain->iovad, IOVA_START_PFN, DMA_32BIT_PFN);
 	domain_reserve_special_ranges(domain);
 
 	/* calculate AGAW */
@@ -4160,7 +4163,7 @@ static int md_domain_init(struct dmar_domain *domain, int guest_width)
 {
 	int adjust_width;
 
-	init_iova_domain(&domain->iovad, DMA_32BIT_PFN);
+	init_iova_domain(&domain->iovad, IOVA_START_PFN, DMA_32BIT_PFN);
 	domain_reserve_special_ranges(domain);
 
 	/* calculate AGAW */
diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index 520b8c8..a3dbba8 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -55,11 +55,13 @@ void free_iova_mem(struct iova *iova)
 }
 
 void
-init_iova_domain(struct iova_domain *iovad, unsigned long pfn_32bit)
+init_iova_domain(struct iova_domain *iovad, unsigned long start_pfn,
+	unsigned long pfn_32bit)
 {
 	spin_lock_init(&iovad->iova_rbtree_lock);
 	iovad->rbroot = RB_ROOT;
 	iovad->cached32_node = NULL;
+	iovad->start_pfn = start_pfn;
 	iovad->dma_32bit_pfn = pfn_32bit;
 }
 
@@ -162,7 +164,7 @@ move_left:
 	if (!curr) {
 		if (size_aligned)
 			pad_size = iova_get_pad_size(size, limit_pfn);
-		if ((IOVA_START_PFN + size + pad_size) > limit_pfn) {
+		if ((iovad->start_pfn + size + pad_size) > limit_pfn) {
 			spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
 			return -ENOMEM;
 		}
@@ -237,8 +239,8 @@ iova_insert_rbtree(struct rb_root *root, struct iova *iova)
  * @size: - size of page frames to allocate
  * @limit_pfn: - max limit address
  * @size_aligned: - set if size_aligned address range is required
- * This function allocates an iova in the range limit_pfn to IOVA_START_PFN
- * looking from limit_pfn instead from IOVA_START_PFN. If the size_aligned
+ * This function allocates an iova in the range iovad->start_pfn to limit_pfn,
+ * searching top-down from limit_pfn to iovad->start_pfn. If the size_aligned
  * flag is set then the allocated address iova->pfn_lo will be naturally
  * aligned on roundup_power_of_two(size).
  */
diff --git a/include/linux/iova.h b/include/linux/iova.h
index ad0507c..591b196 100644
--- a/include/linux/iova.h
+++ b/include/linux/iova.h
@@ -16,9 +16,6 @@
 #include <linux/rbtree.h>
 #include <linux/dma-mapping.h>
 
-/* IO virtual address start page frame number */
-#define IOVA_START_PFN		(1)
-
 /* iova structure */
 struct iova {
 	struct rb_node	node;
@@ -31,6 +28,7 @@ struct iova_domain {
 	spinlock_t	iova_rbtree_lock; /* Lock to protect update of rbtree */
 	struct rb_root	rbroot;		/* iova domain rbtree root */
 	struct rb_node	*cached32_node; /* Save last alloced node */
+	unsigned long	start_pfn;	/* Lower limit for this domain */
 	unsigned long	dma_32bit_pfn;
 };
 
@@ -52,7 +50,8 @@ struct iova *alloc_iova(struct iova_domain *iovad, unsigned long size,
 struct iova *reserve_iova(struct iova_domain *iovad, unsigned long pfn_lo,
 	unsigned long pfn_hi);
 void copy_reserved_iova(struct iova_domain *from, struct iova_domain *to);
-void init_iova_domain(struct iova_domain *iovad, unsigned long pfn_32bit);
+void init_iova_domain(struct iova_domain *iovad, unsigned long start_pfn,
+	unsigned long pfn_32bit);
 struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn);
 void put_iova_domain(struct iova_domain *iovad);
 struct iova *split_and_remove_iova(struct iova_domain *iovad,
-- 
1.9.1

[RFC PATCH 4/4] iommu: make IOVA domain page size explicit

[Robin Murphy]

Systems may contain heterogeneous IOMMUs supporting differing minimum
page sizes, which may also not be common with the CPU page size.
Thus it is practical to have an explicit notion of IOVA granularity
to simplify handling of mapping and allocation constraints.

As an initial step, move the IOVA page granularity from an implicit
compile-time constant to a per-domain property so we can make use
of it in IOVA domain context at runtime. To keep the abstraction tidy,
extend the little API of inline iova_* helpers to parallel some of the
equivalent PAGE_* macros.

Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---
 drivers/iommu/intel-iommu.c |  9 ++++++---
 drivers/iommu/iova.c        |  8 ++++++--
 include/linux/iova.h        | 35 +++++++++++++++++++++++++++++++++--
 3 files changed, 45 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index daba0c2..d4cf6f0 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -1632,7 +1632,8 @@ static int dmar_init_reserved_ranges(void)
 	struct iova *iova;
 	int i;
 
-	init_iova_domain(&reserved_iova_list, IOVA_START_PFN, DMA_32BIT_PFN);
+	init_iova_domain(&reserved_iova_list, VTD_PAGE_SIZE, IOVA_START_PFN,
+			DMA_32BIT_PFN);
 
 	lockdep_set_class(&reserved_iova_list.iova_rbtree_lock,
 		&reserved_rbtree_key);
@@ -1690,7 +1691,8 @@ static int domain_init(struct dmar_domain *domain, int guest_width)
 	int adjust_width, agaw;
 	unsigned long sagaw;
 
-	init_iova_domain(&domain->iovad, IOVA_START_PFN, DMA_32BIT_PFN);
+	init_iova_domain(&domain->iovad, VTD_PAGE_SIZE, IOVA_START_PFN,
+			DMA_32BIT_PFN);
 	domain_reserve_special_ranges(domain);
 
 	/* calculate AGAW */
@@ -4163,7 +4165,8 @@ static int md_domain_init(struct dmar_domain *domain, int guest_width)
 {
 	int adjust_width;
 
-	init_iova_domain(&domain->iovad, IOVA_START_PFN, DMA_32BIT_PFN);
+	init_iova_domain(&domain->iovad, VTD_PAGE_SIZE, IOVA_START_PFN,
+			DMA_32BIT_PFN);
 	domain_reserve_special_ranges(domain);
 
 	/* calculate AGAW */
diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
index a3dbba8..9e50625 100644
--- a/drivers/iommu/iova.c
+++ b/drivers/iommu/iova.c
@@ -55,12 +55,16 @@ void free_iova_mem(struct iova *iova)
 }
 
 void
-init_iova_domain(struct iova_domain *iovad, unsigned long start_pfn,
-	unsigned long pfn_32bit)
+init_iova_domain(struct iova_domain *iovad, unsigned long granule,
+	unsigned long start_pfn, unsigned long pfn_32bit)
 {
+	/* IOMMUs must support at least the CPU page size */
+	BUG_ON(granule > PAGE_SIZE);
+
 	spin_lock_init(&iovad->iova_rbtree_lock);
 	iovad->rbroot = RB_ROOT;
 	iovad->cached32_node = NULL;
+	iovad->granule = granule;
 	iovad->start_pfn = start_pfn;
 	iovad->dma_32bit_pfn = pfn_32bit;
 }
diff --git a/include/linux/iova.h b/include/linux/iova.h
index 591b196..3920a19 100644
--- a/include/linux/iova.h
+++ b/include/linux/iova.h
@@ -28,6 +28,7 @@ struct iova_domain {
 	spinlock_t	iova_rbtree_lock; /* Lock to protect update of rbtree */
 	struct rb_root	rbroot;		/* iova domain rbtree root */
 	struct rb_node	*cached32_node; /* Save last alloced node */
+	unsigned long	granule;	/* pfn granularity for this domain */
 	unsigned long	start_pfn;	/* Lower limit for this domain */
 	unsigned long	dma_32bit_pfn;
 };
@@ -37,6 +38,36 @@ static inline unsigned long iova_size(struct iova *iova)
 	return iova->pfn_hi - iova->pfn_lo + 1;
 }
 
+static inline unsigned long iova_shift(struct iova_domain *iovad)
+{
+	return __ffs(iovad->granule);
+}
+
+static inline unsigned long iova_mask(struct iova_domain *iovad)
+{
+	return iovad->granule - 1;
+}
+
+static inline size_t iova_offset(struct iova_domain *iovad, dma_addr_t iova)
+{
+	return iova & iova_mask(iovad);
+}
+
+static inline size_t iova_align(struct iova_domain *iovad, size_t size)
+{
+	return ALIGN(size, iovad->granule);
+}
+
+static inline dma_addr_t iova_dma_addr(struct iova_domain *iovad, struct iova *iova)
+{
+	return (dma_addr_t)iova->pfn_lo << iova_shift(iovad);
+}
+
+static inline unsigned long iova_pfn(struct iova_domain *iovad, dma_addr_t iova)
+{
+	return iova >> iova_shift(iovad);
+}
+
 int iommu_iova_cache_init(void);
 void iommu_iova_cache_destroy(void);
 
@@ -50,8 +81,8 @@ struct iova *alloc_iova(struct iova_domain *iovad, unsigned long size,
 struct iova *reserve_iova(struct iova_domain *iovad, unsigned long pfn_lo,
 	unsigned long pfn_hi);
 void copy_reserved_iova(struct iova_domain *from, struct iova_domain *to);
-void init_iova_domain(struct iova_domain *iovad, unsigned long start_pfn,
-	unsigned long pfn_32bit);
+void init_iova_domain(struct iova_domain *iovad, unsigned long granule,
+	unsigned long start_pfn, unsigned long pfn_32bit);
 struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn);
 void put_iova_domain(struct iova_domain *iovad);
 struct iova *split_and_remove_iova(struct iova_domain *iovad,
-- 
1.9.1

Re: [RFC PATCH 4/4] iommu: make IOVA domain page size explicit

[leizhen]

On 2014/11/26 1:27, Robin Murphy wrote:
> Systems may contain heterogeneous IOMMUs supporting differing minimum
> page sizes, which may also not be common with the CPU page size.
> Thus it is practical to have an explicit notion of IOVA granularity
> to simplify handling of mapping and allocation constraints.
> 
> As an initial step, move the IOVA page granularity from an implicit
> compile-time constant to a per-domain property so we can make use
> of it in IOVA domain context at runtime. To keep the abstraction tidy,
> extend the little API of inline iova_* helpers to parallel some of the
> equivalent PAGE_* macros.
> 
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/iommu/intel-iommu.c |  9 ++++++---
>  drivers/iommu/iova.c        |  8 ++++++--
>  include/linux/iova.h        | 35 +++++++++++++++++++++++++++++++++--
>  3 files changed, 45 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index daba0c2..d4cf6f0 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -1632,7 +1632,8 @@ static int dmar_init_reserved_ranges(void)
>  	struct iova *iova;
>  	int i;
>  
> -	init_iova_domain(&reserved_iova_list, IOVA_START_PFN, DMA_32BIT_PFN);
> +	init_iova_domain(&reserved_iova_list, VTD_PAGE_SIZE, IOVA_START_PFN,
> +			DMA_32BIT_PFN);
>  
>  	lockdep_set_class(&reserved_iova_list.iova_rbtree_lock,
>  		&reserved_rbtree_key);
> @@ -1690,7 +1691,8 @@ static int domain_init(struct dmar_domain *domain, int guest_width)
>  	int adjust_width, agaw;
>  	unsigned long sagaw;
>  
> -	init_iova_domain(&domain->iovad, IOVA_START_PFN, DMA_32BIT_PFN);
> +	init_iova_domain(&domain->iovad, VTD_PAGE_SIZE, IOVA_START_PFN,
> +			DMA_32BIT_PFN);
>  	domain_reserve_special_ranges(domain);
>  
>  	/* calculate AGAW */
> @@ -4163,7 +4165,8 @@ static int md_domain_init(struct dmar_domain *domain, int guest_width)
>  {
>  	int adjust_width;
>  
> -	init_iova_domain(&domain->iovad, IOVA_START_PFN, DMA_32BIT_PFN);
> +	init_iova_domain(&domain->iovad, VTD_PAGE_SIZE, IOVA_START_PFN,
> +			DMA_32BIT_PFN);
>  	domain_reserve_special_ranges(domain);
>  
>  	/* calculate AGAW */
> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> index a3dbba8..9e50625 100644
> --- a/drivers/iommu/iova.c
> +++ b/drivers/iommu/iova.c
> @@ -55,12 +55,16 @@ void free_iova_mem(struct iova *iova)
>  }
>  
>  void
> -init_iova_domain(struct iova_domain *iovad, unsigned long start_pfn,
> -	unsigned long pfn_32bit)
> +init_iova_domain(struct iova_domain *iovad, unsigned long granule,
> +	unsigned long start_pfn, unsigned long pfn_32bit)
>  {
> +	/* IOMMUs must support at least the CPU page size */
> +	BUG_ON(granule > PAGE_SIZE);

??? BUG_ON(granule < PAGE_SIZE);

> +
>  	spin_lock_init(&iovad->iova_rbtree_lock);
>  	iovad->rbroot = RB_ROOT;
>  	iovad->cached32_node = NULL;
> +	iovad->granule = granule;

Do you need to check granule is 2^n ?

>  	iovad->start_pfn = start_pfn;
>  	iovad->dma_32bit_pfn = pfn_32bit;
>  }
> diff --git a/include/linux/iova.h b/include/linux/iova.h
> index 591b196..3920a19 100644
> --- a/include/linux/iova.h
> +++ b/include/linux/iova.h
> @@ -28,6 +28,7 @@ struct iova_domain {
>  	spinlock_t	iova_rbtree_lock; /* Lock to protect update of rbtree */
>  	struct rb_root	rbroot;		/* iova domain rbtree root */
>  	struct rb_node	*cached32_node; /* Save last alloced node */
> +	unsigned long	granule;	/* pfn granularity for this domain */
>  	unsigned long	start_pfn;	/* Lower limit for this domain */
>  	unsigned long	dma_32bit_pfn;
>  };
> @@ -37,6 +38,36 @@ static inline unsigned long iova_size(struct iova *iova)
>  	return iova->pfn_hi - iova->pfn_lo + 1;
>  }
>  
> +static inline unsigned long iova_shift(struct iova_domain *iovad)
> +{
> +	return __ffs(iovad->granule);

I think it's good to save the result to a struct member. If we caculate it ervertime, it's too slow.

> +}
> +
> +static inline unsigned long iova_mask(struct iova_domain *iovad)
> +{
> +	return iovad->granule - 1;
> +}
> +
> +static inline size_t iova_offset(struct iova_domain *iovad, dma_addr_t iova)
> +{
> +	return iova & iova_mask(iovad);
> +}
> +
> +static inline size_t iova_align(struct iova_domain *iovad, size_t size)
> +{
> +	return ALIGN(size, iovad->granule);
> +}
> +
> +static inline dma_addr_t iova_dma_addr(struct iova_domain *iovad, struct iova *iova)
> +{
> +	return (dma_addr_t)iova->pfn_lo << iova_shift(iovad);
> +}
> +
> +static inline unsigned long iova_pfn(struct iova_domain *iovad, dma_addr_t iova)
> +{
> +	return iova >> iova_shift(iovad);
> +}
> +
>  int iommu_iova_cache_init(void);
>  void iommu_iova_cache_destroy(void);
>  
> @@ -50,8 +81,8 @@ struct iova *alloc_iova(struct iova_domain *iovad, unsigned long size,
>  struct iova *reserve_iova(struct iova_domain *iovad, unsigned long pfn_lo,
>  	unsigned long pfn_hi);
>  void copy_reserved_iova(struct iova_domain *from, struct iova_domain *to);
> -void init_iova_domain(struct iova_domain *iovad, unsigned long start_pfn,
> -	unsigned long pfn_32bit);
> +void init_iova_domain(struct iova_domain *iovad, unsigned long granule,
> +	unsigned long start_pfn, unsigned long pfn_32bit);
>  struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn);
>  void put_iova_domain(struct iova_domain *iovad);
>  struct iova *split_and_remove_iova(struct iova_domain *iovad,
> 

Re: [RFC PATCH 4/4] iommu: make IOVA domain page size explicit

[Robin Murphy]

Hi,

On 26/11/14 07:17, leizhen wrote:
[...]
>> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
>> index a3dbba8..9e50625 100644
>> --- a/drivers/iommu/iova.c
>> +++ b/drivers/iommu/iova.c
>> @@ -55,12 +55,16 @@ void free_iova_mem(struct iova *iova)
>>   }
>>
>>   void
>> -init_iova_domain(struct iova_domain *iovad, unsigned long start_pfn,
>> -	unsigned long pfn_32bit)
>> +init_iova_domain(struct iova_domain *iovad, unsigned long granule,
>> +	unsigned long start_pfn, unsigned long pfn_32bit)
>>   {
>> +	/* IOMMUs must support at least the CPU page size */
>> +	BUG_ON(granule > PAGE_SIZE);
>
> ??? BUG_ON(granule < PAGE_SIZE);
>

Granule represents the smallest page size that the IOMMU can map - we'd 
never allocate less IOVA space than a single page, so we don't need a 
unit smaller than that, and a bigger unit would make it problematic to 
allocate IOVA space for individual pages.

If, for instance, the CPU has 64k pages, and the IOMMU supports 1k tiny 
pages, granule would be less than PAGE_SIZE, but the IOMMU could happily 
that 64k as 64 contiguous tiny pages, even if it's a waste of TLB space.

However, say the IOMMU only supports 16k pages and we try to map two 
noncontiguous 4k CPU pages as a contiguous 8k buffer in IOVA space - 
this can't work, hence why the _minimum_ IOMMU page size, and thus 
granule, may never be _bigger_ than a CPU page.

>> +
>>   	spin_lock_init(&iovad->iova_rbtree_lock);
>>   	iovad->rbroot = RB_ROOT;
>>   	iovad->cached32_node = NULL;
>> +	iovad->granule = granule;
>
> Do you need to check granule is 2^n ?
>

Yes, that would make a lot of sense.

>>   	iovad->start_pfn = start_pfn;
>>   	iovad->dma_32bit_pfn = pfn_32bit;
>>   }
>> diff --git a/include/linux/iova.h b/include/linux/iova.h
>> index 591b196..3920a19 100644
>> --- a/include/linux/iova.h
>> +++ b/include/linux/iova.h
>> @@ -28,6 +28,7 @@ struct iova_domain {
>>   	spinlock_t	iova_rbtree_lock; /* Lock to protect update of rbtree */
>>   	struct rb_root	rbroot;		/* iova domain rbtree root */
>>   	struct rb_node	*cached32_node; /* Save last alloced node */
>> +	unsigned long	granule;	/* pfn granularity for this domain */
>>   	unsigned long	start_pfn;	/* Lower limit for this domain */
>>   	unsigned long	dma_32bit_pfn;
>>   };
>> @@ -37,6 +38,36 @@ static inline unsigned long iova_size(struct iova *iova)
>>   	return iova->pfn_hi - iova->pfn_lo + 1;
>>   }
>>
>> +static inline unsigned long iova_shift(struct iova_domain *iovad)
>> +{
>> +	return __ffs(iovad->granule);
>
> I think it's good to save the result to a struct member. If we caculate it ervertime, it's too slow.
>

I did give some consideration at the time to which of size, shift and 
mask to store - given the redundancy between them storing more than one 
seems excessively wasteful - and this made for the nicest-looking code 
IMO. These IOVA operations are going to be mostly used in map/unmap 
situations leading to an IOMMU TLB flush anyway, so I'm not sure how 
performance-critical they really are.

Are we likely to see this code used on architectures where __ffs takes 
more than a few cycles and has more impact than the general memory bloat 
of making structures bigger?


Thanks for the review,
Robin.

>> +}
>> +
>> +static inline unsigned long iova_mask(struct iova_domain *iovad)
>> +{
>> +	return iovad->granule - 1;
>> +}
>> +
>> +static inline size_t iova_offset(struct iova_domain *iovad, dma_addr_t iova)
>> +{
>> +	return iova & iova_mask(iovad);
>> +}
>> +
>> +static inline size_t iova_align(struct iova_domain *iovad, size_t size)
>> +{
>> +	return ALIGN(size, iovad->granule);
>> +}
>> +
>> +static inline dma_addr_t iova_dma_addr(struct iova_domain *iovad, struct iova *iova)
>> +{
>> +	return (dma_addr_t)iova->pfn_lo << iova_shift(iovad);
>> +}
>> +
>> +static inline unsigned long iova_pfn(struct iova_domain *iovad, dma_addr_t iova)
>> +{
>> +	return iova >> iova_shift(iovad);
>> +}
>> +
>>   int iommu_iova_cache_init(void);
>>   void iommu_iova_cache_destroy(void);
>>
>> @@ -50,8 +81,8 @@ struct iova *alloc_iova(struct iova_domain *iovad, unsigned long size,
>>   struct iova *reserve_iova(struct iova_domain *iovad, unsigned long pfn_lo,
>>   	unsigned long pfn_hi);
>>   void copy_reserved_iova(struct iova_domain *from, struct iova_domain *to);
>> -void init_iova_domain(struct iova_domain *iovad, unsigned long start_pfn,
>> -	unsigned long pfn_32bit);
>> +void init_iova_domain(struct iova_domain *iovad, unsigned long granule,
>> +	unsigned long start_pfn, unsigned long pfn_32bit);
>>   struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn);
>>   void put_iova_domain(struct iova_domain *iovad);
>>   struct iova *split_and_remove_iova(struct iova_domain *iovad,
>>
>
>
>

Re: [RFC PATCH 4/4] iommu: make IOVA domain page size explicit

[leizhen]

On 2014/11/26 21:31, Robin Murphy wrote:
> Hi,
> 
> On 26/11/14 07:17, leizhen wrote:
> [...]
>>> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
>>> index a3dbba8..9e50625 100644
>>> --- a/drivers/iommu/iova.c
>>> +++ b/drivers/iommu/iova.c
>>> @@ -55,12 +55,16 @@ void free_iova_mem(struct iova *iova)
>>>   }
>>>
>>>   void
>>> -init_iova_domain(struct iova_domain *iovad, unsigned long start_pfn,
>>> -    unsigned long pfn_32bit)
>>> +init_iova_domain(struct iova_domain *iovad, unsigned long granule,
>>> +    unsigned long start_pfn, unsigned long pfn_32bit)
>>>   {
>>> +    /* IOMMUs must support at least the CPU page size */
>>> +    BUG_ON(granule > PAGE_SIZE);
>>
>> ??? BUG_ON(granule < PAGE_SIZE);
>>
> 
> Granule represents the smallest page size that the IOMMU can map - we'd never allocate less IOVA space than a single page, so we don't need a unit smaller than that, and a bigger unit would make it
> problematic to allocate IOVA space for individual pages.
> 
> If, for instance, the CPU has 64k pages, and the IOMMU supports 1k tiny pages, granule would be less than PAGE_SIZE, but the IOMMU could happily that 64k as 64 contiguous tiny pages, even if it's a
> waste of TLB space.
> 
> However, say the IOMMU only supports 16k pages and we try to map two noncontiguous 4k CPU pages as a contiguous 8k buffer in IOVA space - this can't work, hence why the _minimum_ IOMMU page size, and
> thus granule, may never be _bigger_ than a CPU page.
> 

Oh, I just confused with the comment: /* IOMMUs must support at least the CPU page size */
But the code means at most the CPU page size. Now, I got it.

>>> +
>>>       spin_lock_init(&iovad->iova_rbtree_lock);
>>>       iovad->rbroot = RB_ROOT;
>>>       iovad->cached32_node = NULL;
>>> +    iovad->granule = granule;
>>
>> Do you need to check granule is 2^n ?
>>
> 
> Yes, that would make a lot of sense.
> 
>>>       iovad->start_pfn = start_pfn;
>>>       iovad->dma_32bit_pfn = pfn_32bit;
>>>   }
>>> diff --git a/include/linux/iova.h b/include/linux/iova.h
>>> index 591b196..3920a19 100644
>>> --- a/include/linux/iova.h
>>> +++ b/include/linux/iova.h
>>> @@ -28,6 +28,7 @@ struct iova_domain {
>>>       spinlock_t    iova_rbtree_lock; /* Lock to protect update of rbtree */
>>>       struct rb_root    rbroot;        /* iova domain rbtree root */
>>>       struct rb_node    *cached32_node; /* Save last alloced node */
>>> +    unsigned long    granule;    /* pfn granularity for this domain */
>>>       unsigned long    start_pfn;    /* Lower limit for this domain */
>>>       unsigned long    dma_32bit_pfn;
>>>   };
>>> @@ -37,6 +38,36 @@ static inline unsigned long iova_size(struct iova *iova)
>>>       return iova->pfn_hi - iova->pfn_lo + 1;
>>>   }
>>>
>>> +static inline unsigned long iova_shift(struct iova_domain *iovad)
>>> +{
>>> +    return __ffs(iovad->granule);
>>
>> I think it's good to save the result to a struct member. If we caculate it ervertime, it's too slow.
>>
> 
> I did give some consideration at the time to which of size, shift and mask to store - given the redundancy between them storing more than one seems excessively wasteful - and this made for the
> nicest-looking code IMO. These IOVA operations are going to be mostly used in map/unmap situations leading to an IOMMU TLB flush anyway, so I'm not sure how performance-critical they really are.
> 
> Are we likely to see this code used on architectures where __ffs takes more than a few cycles and has more impact than the general memory bloat of making structures bigger?
> 
> 
> Thanks for the review,
> Robin.
> 
>>> +}
>>> +
>>> +static inline unsigned long iova_mask(struct iova_domain *iovad)
>>> +{
>>> +    return iovad->granule - 1;
>>> +}
>>> +
>>> +static inline size_t iova_offset(struct iova_domain *iovad, dma_addr_t iova)
>>> +{
>>> +    return iova & iova_mask(iovad);
>>> +}
>>> +
>>> +static inline size_t iova_align(struct iova_domain *iovad, size_t size)
>>> +{
>>> +    return ALIGN(size, iovad->granule);
>>> +}
>>> +
>>> +static inline dma_addr_t iova_dma_addr(struct iova_domain *iovad, struct iova *iova)
>>> +{
>>> +    return (dma_addr_t)iova->pfn_lo << iova_shift(iovad);
>>> +}
>>> +
>>> +static inline unsigned long iova_pfn(struct iova_domain *iovad, dma_addr_t iova)
>>> +{
>>> +    return iova >> iova_shift(iovad);
>>> +}
>>> +
>>>   int iommu_iova_cache_init(void);
>>>   void iommu_iova_cache_destroy(void);
>>>
>>> @@ -50,8 +81,8 @@ struct iova *alloc_iova(struct iova_domain *iovad, unsigned long size,
>>>   struct iova *reserve_iova(struct iova_domain *iovad, unsigned long pfn_lo,
>>>       unsigned long pfn_hi);
>>>   void copy_reserved_iova(struct iova_domain *from, struct iova_domain *to);
>>> -void init_iova_domain(struct iova_domain *iovad, unsigned long start_pfn,
>>> -    unsigned long pfn_32bit);
>>> +void init_iova_domain(struct iova_domain *iovad, unsigned long granule,
>>> +    unsigned long start_pfn, unsigned long pfn_32bit);
>>>   struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn);
>>>   void put_iova_domain(struct iova_domain *iovad);
>>>   struct iova *split_and_remove_iova(struct iova_domain *iovad,
>>>
>>
>>
>>
> 
> 
> 
> .
> 

Re: [RFC PATCH 0/4] Genericise the IOVA allocator

[Sakari Ailus]

Hi Robin,

Robin Murphy wrote:
> Hi all,
>
> I've been implementing IOMMU DMA mapping for arm64, based on tidied-up
> parts of the existing arch/arm/mm/dma-mapping.c with a clear divide
> between the arch-specific parts and the general DMA-API to IOMMU-API layer
> so that that can be shared; similar to what Ritesh started before and was
> unable to complete[1], but working in the other direction.
>
> The first part of that tidy-up involved ripping out the homebrewed IOVA
> allocator and plumbing in iova.c, necessitating the changes presented here.
> The rest is currently sat under arch/arm64 for the sake of getting it
> working quickly with minimal impact - ideally I'd move it out and port
> arch/arm before merging, but I don't know quite how impatient people are.
> Regardless of that decision, this bit stands alone, so here it is.
>
> Feel free to ignore patches 1 and 2, since I see Sakari has recently
> posted a more thorough series for that[2], that frankly looks nicer ;)
> I've merely left them in as context here.

Thank you. Could you review the set, please? I've gotten no comments on 
it so far.

You probably wouldn't need the 4th patch, and probably this isn't the 
right list to get that forward anyway.

> [1]:http://thread.gmane.org/gmane.linux.ports.arm.kernel/331299
> [2]:http://article.gmane.org/gmane.linux.kernel.iommu/7436

-- 
Kind regards,

Sakari Ailus
sakari.ailus-VuQAYsv1563Yd54FQh9/CA@public.gmane.org

Re: [RFC PATCH 0/4] Genericise the IOVA allocator

[Joerg Roedel]

Hi Robin,

On Tue, Nov 25, 2014 at 05:27:24PM +0000, Robin Murphy wrote:
> Hi all,
> 
> I've been implementing IOMMU DMA mapping for arm64, based on tidied-up
> parts of the existing arch/arm/mm/dma-mapping.c with a clear divide
> between the arch-specific parts and the general DMA-API to IOMMU-API layer
> so that that can be shared; similar to what Ritesh started before and was
> unable to complete[1], but working in the other direction.
> 
> The first part of that tidy-up involved ripping out the homebrewed IOVA
> allocator and plumbing in iova.c, necessitating the changes presented here.
> The rest is currently sat under arch/arm64 for the sake of getting it
> working quickly with minimal impact - ideally I'd move it out and port
> arch/arm before merging, but I don't know quite how impatient people are.
> Regardless of that decision, this bit stands alone, so here it is.
> 
> Feel free to ignore patches 1 and 2, since I see Sakari has recently
> posted a more thorough series for that[2], that frankly looks nicer ;)
> I've merely left them in as context here.
> 
> [1]:http://thread.gmane.org/gmane.linux.ports.arm.kernel/331299
> [2]:http://article.gmane.org/gmane.linux.kernel.iommu/7436
> 
> Robin Murphy (4):
>   iommu: build iova.c for any IOMMU
>   iommu: consolidate IOVA allocator code
>   iommu: make IOVA domain low limit flexible
>   iommu: make IOVA domain page size explicit

Thanks for doing this, I like this patch-set.

I would also appreciate if someone from Intel could have a look at it,
David?

Besides, can you please re-post this patch-set rebased to latest
upstream with the better versions of patch 1 and 2, please?

I consider to apply these changes then.


	Joerg

Re: [RFC PATCH 0/4] Genericise the IOVA allocator

[Robin Murphy]

Hi Joerg,

On 12/01/15 15:52, Joerg Roedel wrote:
[...]
>
> Thanks for doing this, I like this patch-set.
>
> I would also appreciate if someone from Intel could have a look at it,
> David?
>
> Besides, can you please re-post this patch-set rebased to latest
> upstream with the better versions of patch 1 and 2, please?
>
> I consider to apply these changes then.
>

Thanks! Funnily enough, that's on my "things I didn't quite get round to 
yesterday" list. I have this series rebased onto -rc3 with the comments 
addressed, and I'm in the middle of a final cleanup and check of the 
arm64 dma-mapping stuff on top of it. Expect to see both later today.

Robin.