[PATCH char-misc-next 10/19] lib: convert iova.c into a library

[PATCH char-misc-next 10/19] lib: convert iova.c into a library

[Ashutosh Dixit]

From: Harish Chegondi <harish.chegondi@intel.com>

This patch converts iova.c into a library, moving it from
drivers/iommu/ to lib/, and exports its virtual address allocation and
management functions so that other modules can reuse them.

Cc: Joerg Roedel <joro@8bytes.org>
Reviewed-by: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
Reviewed-by: Sudeep Dutt <sudeep.dutt@intel.com>
Signed-off-by: Harish Chegondi <harish.chegondi@intel.com>
---
 drivers/iommu/Kconfig         | 5 +----
 drivers/iommu/Makefile        | 1 -
 lib/Kconfig                   | 6 ++++++
 lib/Makefile                  | 2 ++
 {drivers/iommu => lib}/iova.c | 9 +++++++++
 5 files changed, 18 insertions(+), 5 deletions(-)
 rename {drivers/iommu => lib}/iova.c (97%)

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index f1fb1d3..c711889 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -41,9 +41,6 @@ config IOMMU_IO_PGTABLE_LPAE_SELFTEST
 
 endmenu
 
-config IOMMU_IOVA
-	bool
-
 config OF_IOMMU
        def_bool y
        depends on OF && IOMMU_API
@@ -125,8 +122,8 @@ config INTEL_IOMMU
 	bool "Support for Intel IOMMU using DMA Remapping Devices"
 	depends on PCI_MSI && ACPI && (X86 || IA64_GENERIC)
 	select IOMMU_API
-	select IOMMU_IOVA
 	select DMAR_TABLE
+	select IOVA
 	help
 	  DMA remapping (DMAR) devices support enables independent address
 	  translations for Direct Memory Access (DMA) from devices.
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index c6dcc51..fbb1372 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -3,7 +3,6 @@ obj-$(CONFIG_IOMMU_API) += iommu-traces.o
 obj-$(CONFIG_IOMMU_API) += iommu-sysfs.o
 obj-$(CONFIG_IOMMU_IO_PGTABLE) += io-pgtable.o
 obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o
-obj-$(CONFIG_IOMMU_IOVA) += iova.o
 obj-$(CONFIG_OF_IOMMU)	+= of_iommu.o
 obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o msm_iommu_dev.o
 obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o
diff --git a/lib/Kconfig b/lib/Kconfig
index 3a2ef67..62ba145 100644
--- a/lib/Kconfig
+++ b/lib/Kconfig
@@ -62,6 +62,12 @@ config ARCH_USE_CMPXCHG_LOCKREF
 config ARCH_HAS_FAST_MULTIPLIER
 	bool
 
+config IOVA
+	bool
+	help
+	  This option enables a 64 bit virtual address allocation and management
+	  library.
+
 config CRC_CCITT
 	tristate "CRC-CCITT functions"
 	help
diff --git a/lib/Makefile b/lib/Makefile
index 6897b52..058722d 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -205,3 +205,5 @@ quiet_cmd_build_OID_registry = GEN     $@
 clean-files	+= oid_registry_data.c
 
 obj-$(CONFIG_UCS2_STRING) += ucs2_string.o
+
+obj-$(CONFIG_IOVA) += iova.o
diff --git a/drivers/iommu/iova.c b/lib/iova.c
similarity index 97%
rename from drivers/iommu/iova.c
rename to lib/iova.c
index b7c3d92..7753006 100644
--- a/drivers/iommu/iova.c
+++ b/lib/iova.c
@@ -72,6 +72,7 @@ init_iova_domain(struct iova_domain *iovad, unsigned long granule,
 	iovad->start_pfn = start_pfn;
 	iovad->dma_32bit_pfn = pfn_32bit;
 }
+EXPORT_SYMBOL_GPL(init_iova_domain);
 
 static struct rb_node *
 __get_cached_rbnode(struct iova_domain *iovad, unsigned long *limit_pfn)
@@ -281,6 +282,7 @@ alloc_iova(struct iova_domain *iovad, unsigned long size,
 
 	return new_iova;
 }
+EXPORT_SYMBOL_GPL(alloc_iova);
 
 /**
  * find_iova - find's an iova for a given pfn
@@ -321,6 +323,7 @@ struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn)
 	spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
 	return NULL;
 }
+EXPORT_SYMBOL_GPL(find_iova);
 
 /**
  * __free_iova - frees the given iova
@@ -339,6 +342,7 @@ __free_iova(struct iova_domain *iovad, struct iova *iova)
 	spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
 	free_iova_mem(iova);
 }
+EXPORT_SYMBOL_GPL(__free_iova);
 
 /**
  * free_iova - finds and frees the iova for a given pfn
@@ -356,6 +360,7 @@ free_iova(struct iova_domain *iovad, unsigned long pfn)
 		__free_iova(iovad, iova);
 
 }
+EXPORT_SYMBOL_GPL(free_iova);
 
 /**
  * put_iova_domain - destroys the iova doamin
@@ -378,6 +383,7 @@ void put_iova_domain(struct iova_domain *iovad)
 	}
 	spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
 }
+EXPORT_SYMBOL_GPL(put_iova_domain);
 
 static int
 __is_range_overlap(struct rb_node *node,
@@ -467,6 +473,7 @@ finish:
 	spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
 	return iova;
 }
+EXPORT_SYMBOL_GPL(reserve_iova);
 
 /**
  * copy_reserved_iova - copies the reserved between domains
@@ -493,6 +500,7 @@ copy_reserved_iova(struct iova_domain *from, struct iova_domain *to)
 	}
 	spin_unlock_irqrestore(&from->iova_rbtree_lock, flags);
 }
+EXPORT_SYMBOL_GPL(copy_reserved_iova);
 
 struct iova *
 split_and_remove_iova(struct iova_domain *iovad, struct iova *iova,
@@ -534,3 +542,4 @@ error:
 		free_iova_mem(prev);
 	return NULL;
 }
+EXPORT_SYMBOL_GPL(split_and_remove_iova);
-- 
2.0.0.rc3.2.g998f840

Re: [PATCH char-misc-next 10/19] lib: convert iova.c into a library

[Joerg Roedel]

On Mon, Jul 27, 2015 at 04:57:32PM -0700, Ashutosh Dixit wrote:
> From: Harish Chegondi <harish.chegondi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> This patch converts iova.c into a library, moving it from
> drivers/iommu/ to lib/, and exports its virtual address allocation and
> management functions so that other modules can reuse them.
> 
> Cc: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
> Reviewed-by: Anil S Keshavamurthy <anil.s.keshavamurthy-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Sudeep Dutt <sudeep.dutt-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Harish Chegondi <harish.chegondi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

Where is this going to be used outside of the IOMMU world?



	Joerg

Re: [PATCH char-misc-next 10/19] lib: convert iova.c into a library

[Robin Murphy]

On 28/07/15 11:03, Joerg Roedel wrote:
> On Mon, Jul 27, 2015 at 04:57:32PM -0700, Ashutosh Dixit wrote:
>> From: Harish Chegondi <harish.chegondi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>
>> This patch converts iova.c into a library, moving it from
>> drivers/iommu/ to lib/, and exports its virtual address allocation and
>> management functions so that other modules can reuse them.
>>
>> Cc: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
>> Reviewed-by: Anil S Keshavamurthy <anil.s.keshavamurthy-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> Reviewed-by: Sudeep Dutt <sudeep.dutt-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>> Signed-off-by: Harish Chegondi <harish.chegondi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>
> Where is this going to be used outside of the IOMMU world?
>

...and how does it relate to the patches from Sakari (+CC) doing much 
the same thing[1]?

Having gone and fished out the main LKML thread ([2], to help anyone 
else missing it), I don't see any obvious dependency on the Intel IOMMU 
driver - what happens here if that is compiled out and hasn't called 
iommu_iova_cache_init() first?

Robin.

[1]:http://thread.gmane.org/gmane.linux.kernel.iommu/10142
[2]:http://thread.gmane.org/gmane.linux.kernel/2005895

Re: [PATCH char-misc-next 10/19] lib: convert iova.c into a library

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 1389 bytes --]

On Tue, 2015-07-28 at 11:41 +0100, Robin Murphy wrote:
> On 28/07/15 11:03, Joerg Roedel wrote:
> > On Mon, Jul 27, 2015 at 04:57:32PM -0700, Ashutosh Dixit wrote:
> > > From: Harish Chegondi <harish.chegondi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > 
> > > This patch converts iova.c into a library, moving it from
> > > drivers/iommu/ to lib/, and exports its virtual address 
> > > allocation and
> > > management functions so that other modules can reuse them.
> > > 
> > > Cc: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
> > > Reviewed-by: Anil S Keshavamurthy <anil.s.keshavamurthy-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org
> > > >
> > > Reviewed-by: Sudeep Dutt <sudeep.dutt-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > Signed-off-by: Harish Chegondi <harish.chegondi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > 
> > Where is this going to be used outside of the IOMMU world?
> > 
> 
> ...and how does it relate to the patches from Sakari (+CC) doing much 
> the same thing[1]?

I merged Sakari's patches into the intel-iommu git tree today, FWIW.

If there's really a need to move it from drivers/iommu/ to lib/ then we
could feasibly do that too.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org                              Intel Corporation

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5691 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH char-misc-next 10/19] lib: convert iova.c into a library

[Sudeep Dutt]

On Tue, 2015-07-28 at 15:38 +0100, David Woodhouse wrote:
> On Tue, 2015-07-28 at 11:41 +0100, Robin Murphy wrote:
> > On 28/07/15 11:03, Joerg Roedel wrote:
> > > On Mon, Jul 27, 2015 at 04:57:32PM -0700, Ashutosh Dixit wrote:
> > > > From: Harish Chegondi <harish.chegondi@intel.com>
> > > > 
> > > > This patch converts iova.c into a library, moving it from
> > > > drivers/iommu/ to lib/, and exports its virtual address 
> > > > allocation and
> > > > management functions so that other modules can reuse them.
> > > > 
> > > > Cc: Joerg Roedel <joro@8bytes.org>
> > > > Reviewed-by: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com
> > > > >
> > > > Reviewed-by: Sudeep Dutt <sudeep.dutt@intel.com>
> > > > Signed-off-by: Harish Chegondi <harish.chegondi@intel.com>
> > > 
> > > Where is this going to be used outside of the IOMMU world?
> > > 

We are using the IOVA generator in the SCIF driver posted @
http://thread.gmane.org/gmane.linux.kernel/2005895 under
drivers/misc/mic/scif

> > 
> > ...and how does it relate to the patches from Sakari (+CC) doing much 
> > the same thing[1]?
> 

The patch series from Sakari does the right thing by moving the IOVA
cache management to the IOVA library. We will simply drop this current
patch as it is incorrect.

> I merged Sakari's patches into the intel-iommu git tree today, FWIW.
> 
> If there's really a need to move it from drivers/iommu/ to lib/ then we
> could feasibly do that too.
> 

The patch series from Sakari should work perfectly for us. We will post
a v2 of the current SCIF patch series without this IOVA patch and modify
the SCIF driver to use the newly added iova_cache_get(..) and
iova_cache_put(..) APIs once it is available in Linus's tree. It would
make it easier for us to integrate if Sakari's patches reach mainline
soon.

It might be cleaner to move IOVA to lib/ in the longer term since we
will have multiple driver subsystems using it, but it should work just
fine for now.

Thanks for the review!

Sudeep Dutt

Re: [PATCH char-misc-next 10/19] lib: convert iova.c into a library

[Andrew Morton]

On Mon, 27 Jul 2015 16:57:32 -0700 Ashutosh Dixit <ashutosh.dixit-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:

> From: Harish Chegondi <harish.chegondi-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> This patch converts iova.c into a library, moving it from
> drivers/iommu/ to lib/, and exports its virtual address allocation and
> management functions so that other modules can reuse them.

>From the following emails it is unclear to me whether we'll actually be
going ahead with this, but whatever.  It's a chance to do some code
reading.

>  {drivers/iommu => lib}/iova.c | 9 +++++++++

Except the code isn't here.  Stupid git.  Here 'tis:

> /*
>  * Copyright __ 2006-2009, Intel Corporation.

Non-ascii thing which I can't get to display correctly in anything. 
Give it up, it'll never work, use "(c)".

>  * This program is free software; you can redistribute it and/or modify it
>  * under the terms and conditions of the GNU General Public License,
>  * version 2, as published by the Free Software Foundation.
>  *
>  * This program is distributed in the hope it will be useful, but WITHOUT
>  * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>  * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>  * more details.
>  *
>  * You should have received a copy of the GNU General Public License along with
>  * this program; if not, write to the Free Software Foundation, Inc., 59 Temple
>  * Place - Suite 330, Boston, MA 02111-1307 USA.
>  *
>  * Author: Anil S Keshavamurthy <anil.s.keshavamurthy-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>  */
> 
> #include <linux/iova.h>
> #include <linux/slab.h>

Wow.  This file only compiles by sheer dumb luck.  kernel.h? 
spinlock.h?  bitops.h?  Some of these things come in thanks to iova.h,
others don't.

> static struct kmem_cache *iommu_iova_cache;
> 
> int iommu_iova_cache_init(void)

Should actually be called iommu_iova_cache_create().  Because it's a
wrapper around kmem_cache_create(), and "create" is what it does.

> {
> 	int ret = 0;
> 
> 	iommu_iova_cache = kmem_cache_create("iommu_iova",
> 					 sizeof(struct iova),
> 					 0,
> 					 SLAB_HWCACHE_ALIGN,
> 					 NULL);

Could use KMEM_CACHE().

> 	if (!iommu_iova_cache) {
> 		pr_err("Couldn't create iova cache\n");
> 		ret = -ENOMEM;
> 	}
> 
> 	return ret;
> }
> 
> void iommu_iova_cache_destroy(void)

The naming throughout this driver is a mess.  Sometimes it's
iommu_iova_<operation>.  Other times it's <operation>_iova.  Other
times it's <operation>_iova_<operation>.  There's no thought and no
consistency.

The usual standard will work well here.  Stick with iova_<operation>
and use it consistently.

So

iova_cache_create
iova_cache_destroy
iova_mem_free
iova_init_domain
iova_alloc
iova_find
iova_etcetera

> {
> 	kmem_cache_destroy(iommu_iova_cache);
> }
> 
> struct iova *alloc_iova_mem(void)
> {
> 	return kmem_cache_alloc(iommu_iova_cache, GFP_ATOMIC);
> }

GFP_ATOMIC is unreliable and undesirably depletes page allocator
reserves.  If it is really really the case that no caller can ever
allocate an iova in any other context then OK, but that's an
extraordinary thing and I suggest it should be well documented in code
comments.

However I suspect the thing to do here is to convert this to take a
gfp_t argument to permit callers to use the stronger memory allocation
strategies.


> void free_iova_mem(struct iova *iova)
> {
> 	kmem_cache_free(iommu_iova_cache, iova);
> }
> 
> void
> init_iova_domain(struct iova_domain *iovad, unsigned long granule,
> 	unsigned long start_pfn, unsigned long pfn_32bit)
> {
> 	/*
> 	 * IOVA granularity will normally be equal to the smallest
> 	 * supported IOMMU page size; both *must* be capable of
> 	 * representing individual CPU pages exactly.
> 	 */
> 	BUG_ON((granule > PAGE_SIZE) || !is_power_of_2(granule));
> 
> 	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;
> }
> 
> static struct rb_node *
> __get_cached_rbnode(struct iova_domain *iovad, unsigned long *limit_pfn)
> {
> 	if ((*limit_pfn != iovad->dma_32bit_pfn) ||
> 		(iovad->cached32_node == NULL))
> 		return rb_last(&iovad->rbroot);
> 	else {
> 		struct rb_node *prev_node = rb_prev(iovad->cached32_node);
> 		struct iova *curr_iova =
> 			container_of(iovad->cached32_node, struct iova, node);
> 		*limit_pfn = curr_iova->pfn_lo - 1;
> 		return prev_node;
> 	}
> }
> 
> static void
> __cached_rbnode_insert_update(struct iova_domain *iovad,
> 	unsigned long limit_pfn, struct iova *new)
> {
> 	if (limit_pfn != iovad->dma_32bit_pfn)
> 		return;
> 	iovad->cached32_node = &new->node;
> }
> 
> static void
> __cached_rbnode_delete_update(struct iova_domain *iovad, struct iova *free)
> {
> 	struct iova *cached_iova;
> 	struct rb_node *curr;
> 
> 	if (!iovad->cached32_node)
> 		return;
> 	curr = iovad->cached32_node;
> 	cached_iova = container_of(curr, struct iova, node);
> 
> 	if (free->pfn_lo >= cached_iova->pfn_lo) {
> 		struct rb_node *node = rb_next(&free->node);
> 		struct iova *iova = container_of(node, struct iova, node);
> 
> 		/* only cache if it's below 32bit pfn */
> 		if (node && iova->pfn_lo < iovad->dma_32bit_pfn)
> 			iovad->cached32_node = node;
> 		else
> 			iovad->cached32_node = NULL;
> 	}
> }
> 
> /* Computes the padding size required, to make the
>  * the start address naturally aligned on its size
>  */

Comment has unusual layout, doesn't use all the display and has "the
the".  Like this:

/*
 * Computes the padding size required, to make the start address naturally
 * aligned on its size.
 */

> static int
> iova_get_pad_size(int size, unsigned int limit_pfn)

iova_<operation>.  Yay.

> {
> 	unsigned int pad_size = 0;
> 	unsigned int order = ilog2(size);
> 
> 	if (order)
> 		pad_size = (limit_pfn + 1) % (1 << order);
> 
> 	return pad_size;
> }
> 
> static int __alloc_and_insert_iova_range(struct iova_domain *iovad,
> 		unsigned long size, unsigned long limit_pfn,
> 			struct iova *new, bool size_aligned)
> {
> 	struct rb_node *prev, *curr = NULL;
> 	unsigned long flags;
> 	unsigned long saved_pfn;
> 	unsigned int pad_size = 0;
> 
> 	/* Walk the tree backwards */
> 	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
> 	saved_pfn = limit_pfn;
> 	curr = __get_cached_rbnode(iovad, &limit_pfn);
> 	prev = curr;
> 	while (curr) {
> 		struct iova *curr_iova = container_of(curr, struct iova, node);
> 
> 		if (limit_pfn < curr_iova->pfn_lo)
> 			goto move_left;
> 		else if (limit_pfn < curr_iova->pfn_hi)
> 			goto adjust_limit_pfn;
> 		else {
> 			if (size_aligned)
> 				pad_size = iova_get_pad_size(size, limit_pfn);
> 			if ((curr_iova->pfn_hi + size + pad_size) <= limit_pfn)
> 				break;	/* found a free slot */
> 		}
> adjust_limit_pfn:
> 		limit_pfn = curr_iova->pfn_lo - 1;
> move_left:
> 		prev = curr;
> 		curr = rb_prev(curr);
> 	}
> 
> 	if (!curr) {
> 		if (size_aligned)
> 			pad_size = iova_get_pad_size(size, limit_pfn);
> 		if ((iovad->start_pfn + size + pad_size) > limit_pfn) {
> 			spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
> 			return -ENOMEM;
> 		}
> 	}
> 
> 	/* pfn_lo will point to size aligned address if size_aligned is set */
> 	new->pfn_lo = limit_pfn - (size + pad_size) + 1;
> 	new->pfn_hi = new->pfn_lo + size - 1;
> 
> 	/* Insert the new_iova into domain rbtree by holding writer lock */
> 	/* Add new node and rebalance tree. */
> 	{
> 		struct rb_node **entry, *parent = NULL;
> 
> 		/* If we have 'prev', it's a valid place to start the
> 		   insertion. Otherwise, start from the root. */
> 		if (prev)
> 			entry = &prev;
> 		else
> 			entry = &iovad->rbroot.rb_node;
> 
> 		/* Figure out where to put new node */
> 		while (*entry) {
> 			struct iova *this = container_of(*entry,
> 							struct iova, node);

Could do

			struct iova *this;

			this = container_of(*entry, struct iova, node);

and avoid 80-col weirdnesses.  This happens a lot.


> 			parent = *entry;
> 
> 			if (new->pfn_lo < this->pfn_lo)
> 				entry = &((*entry)->rb_left);
> 			else if (new->pfn_lo > this->pfn_lo)
> 				entry = &((*entry)->rb_right);
> 			else
> 				BUG(); /* this should not happen */
> 		}
> 
> 		/* Add new node and rebalance tree. */
> 		rb_link_node(&new->node, parent, entry);
> 		rb_insert_color(&new->node, &iovad->rbroot);
> 	}
> 	__cached_rbnode_insert_update(iovad, saved_pfn, new);
> 
> 	spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
> 
> 
> 	return 0;
> }
> 
> static void
> iova_insert_rbtree(struct rb_root *root, struct iova *iova)
> {
> 	struct rb_node **new = &(root->rb_node), *parent = NULL;
> 	/* Figure out where to put new node */
> 	while (*new) {
> 		struct iova *this = container_of(*new, struct iova, node);
> 
> 		parent = *new;
> 
> 		if (iova->pfn_lo < this->pfn_lo)
> 			new = &((*new)->rb_left);
> 		else if (iova->pfn_lo > this->pfn_lo)
> 			new = &((*new)->rb_right);
> 		else
> 			BUG(); /* this should not happen */
> 	}
> 	/* Add new node and rebalance tree. */
> 	rb_link_node(&iova->node, parent, new);
> 	rb_insert_color(&iova->node, root);
> }
> 
> /**
>  * alloc_iova - allocates an iova
>  * @iovad: - iova domain in question
>  * @size: - size of page frames to allocate
>  * @limit_pfn: - max limit address
>  * @size_aligned: - set if size_aligned address range is required

The "-" on the function arguments isn't correct kerneldoc, I think.

>  * 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).
>  */
> struct iova *
> alloc_iova(struct iova_domain *iovad, unsigned long size,
> 	unsigned long limit_pfn,
> 	bool size_aligned)

Layout of the arguments is strange and straggly.

> {
> 	struct iova *new_iova;
> 	int ret;
> 
> 	new_iova = alloc_iova_mem();
> 	if (!new_iova)
> 		return NULL;
> 
> 	/* If size aligned is set then round the size to
> 	 * to next power of two.
> 	 */

That's a pretty useless comment!

> 	if (size_aligned)
> 		size = __roundup_pow_of_two(size);
> 
> 	ret = __alloc_and_insert_iova_range(iovad, size, limit_pfn,
> 			new_iova, size_aligned);
> 
> 	if (ret) {
> 		free_iova_mem(new_iova);
> 		return NULL;
> 	}
> 
> 	return new_iova;
> }
> 
> /**
>  * find_iova - find's an iova for a given pfn
>  * @iovad: - iova domain in question.
>  * @pfn: - page frame number
>  * This function finds and returns an iova belonging to the
>  * given doamin which matches the given pfn.
>  */
> struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn)
> {
> 	unsigned long flags;
> 	struct rb_node *node;
> 
> 	/* Take the lock so that no other thread is manipulating the rbtree */
> 	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
> 	node = iovad->rbroot.rb_node;
> 	while (node) {
> 		struct iova *iova = container_of(node, struct iova, node);
> 
> 		/* If pfn falls within iova's range, return iova */
> 		if ((pfn >= iova->pfn_lo) && (pfn <= iova->pfn_hi)) {
> 			spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
> 			/* We are not holding the lock while this iova
> 			 * is referenced by the caller as the same thread
> 			 * which called this function also calls __free_iova()
> 			 * and it is by design that only one thread can possibly
> 			 * reference a particular iova and hence no conflict.
> 			 */

Well that sounds fishy.

I'm generally assuming that this code isn't going to be worked on by
the lay masses - much of the design resides within a few people's heads
and that is where it will stay.  Otherwise a whole lotta documetation
will need to be added.  eg, what is this "design" of which you speak.

> 			return iova;
> 		}
> 
> 		if (pfn < iova->pfn_lo)
> 			node = node->rb_left;
> 		else if (pfn > iova->pfn_lo)
> 			node = node->rb_right;
> 	}
> 
> 	spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
> 	return NULL;
> }
> 
> /**
>  * __free_iova - frees the given iova
>  * @iovad: iova domain in question.
>  * @iova: iova in question.
>  * Frees the given iova belonging to the giving domain
>  */
> void
> __free_iova(struct iova_domain *iovad, struct iova *iova)
> {
> 	unsigned long flags;
> 
> 	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
> 	__cached_rbnode_delete_update(iovad, iova);
> 	rb_erase(&iova->node, &iovad->rbroot);
> 	spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
> 	free_iova_mem(iova);
> }
> 
> /**
>  * free_iova - finds and frees the iova for a given pfn
>  * @iovad: - iova domain in question.
>  * @pfn: - pfn that is allocated previously
>  * This functions finds an iova for a given pfn and then
>  * frees the iova from that domain.
>  */
> void
> free_iova(struct iova_domain *iovad, unsigned long pfn)
> {
> 	struct iova *iova = find_iova(iovad, pfn);
> 
> 	if (iova)
> 		__free_iova(iovad, iova);
> 
> }
> 
> /**
>  * put_iova_domain - destroys the iova doamin
>  * @iovad: - iova domain in question.
>  * All the iova's in that domain are destroyed.
>  */
> void put_iova_domain(struct iova_domain *iovad)

This is not a "put" function.  In Linux, "put" means "decrement the
refcount and free if it fell to zero".  A better name would be
iova_domain_free().

> {
> 	struct rb_node *node;
> 	unsigned long flags;
> 
> 	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
> 	node = rb_first(&iovad->rbroot);
> 	while (node) {
> 		struct iova *iova = container_of(node, struct iova, node);
> 
> 		rb_erase(node, &iovad->rbroot);
> 		free_iova_mem(iova);
> 		node = rb_first(&iovad->rbroot);
> 	}
> 	spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
> }
> 
> static int
> __is_range_overlap(struct rb_node *node,
> 	unsigned long pfn_lo, unsigned long pfn_hi)
> {
> 	struct iova *iova = container_of(node, struct iova, node);
> 
> 	if ((pfn_lo <= iova->pfn_hi) && (pfn_hi >= iova->pfn_lo))
> 		return 1;
> 	return 0;

	return (pfn_lo <= iova->pfn_hi) && (pfn_hi >= iova->pfn_lo);

> }
> 
> static inline struct iova *
> alloc_and_init_iova(unsigned long pfn_lo, unsigned long pfn_hi)
> {
> 	struct iova *iova;
> 
> 	iova = alloc_iova_mem();
> 	if (iova) {
> 		iova->pfn_lo = pfn_lo;
> 		iova->pfn_hi = pfn_hi;
> 	}
> 
> 	return iova;
> }
> 
> static struct iova *
> __insert_new_range(struct iova_domain *iovad,
> 	unsigned long pfn_lo, unsigned long pfn_hi)
> {
> 	struct iova *iova;
> 
> 	iova = alloc_and_init_iova(pfn_lo, pfn_hi);
> 	if (iova)
> 		iova_insert_rbtree(&iovad->rbroot, iova);
> 
> 	return iova;
> }
> 
> static void
> __adjust_overlap_range(struct iova *iova,
> 	unsigned long *pfn_lo, unsigned long *pfn_hi)
> {
> 	if (*pfn_lo < iova->pfn_lo)
> 		iova->pfn_lo = *pfn_lo;
> 	if (*pfn_hi > iova->pfn_hi)
> 		*pfn_lo = iova->pfn_hi + 1;
> }
> 
> /**
>  * reserve_iova - reserves an iova in the given range
>  * @iovad: - iova domain pointer
>  * @pfn_lo: - lower page frame address
>  * @pfn_hi:- higher pfn adderss

More kerneldoc funnies.

s/adderss/address/

>  * This function allocates reserves the address range from pfn_lo to pfn_hi so

Sentence needs an edit
>  * that this address is not dished out as part of alloc_iova.
>  */
> struct iova *
> reserve_iova(struct iova_domain *iovad,
> 	unsigned long pfn_lo, unsigned long pfn_hi)
> {
> 	struct rb_node *node;
> 	unsigned long flags;
> 	struct iova *iova;
> 	unsigned int overlap = 0;
> 
> 	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
> 	for (node = rb_first(&iovad->rbroot); node; node = rb_next(node)) {
> 		if (__is_range_overlap(node, pfn_lo, pfn_hi)) {
> 			iova = container_of(node, struct iova, node);
> 			__adjust_overlap_range(iova, &pfn_lo, &pfn_hi);
> 			if ((pfn_lo >= iova->pfn_lo) &&
> 				(pfn_hi <= iova->pfn_hi))
> 				goto finish;
> 			overlap = 1;
> 
> 		} else if (overlap)
> 				break;

indenting

> 	}
> 
> 	/* We are here either because this is the first reserver node
> 	 * or need to insert remaining non overlap addr range
> 	 */
> 	iova = __insert_new_range(iovad, pfn_lo, pfn_hi);
> finish:
> 
> 	spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
> 	return iova;
> }
> 
> /**
>  * copy_reserved_iova - copies the reserved between domains
>  * @from: - source doamin from where to copy
>  * @to: - destination domin where to copy
>  * This function copies reserved iova's from one doamin to
>  * other.
>  */
> void
> copy_reserved_iova(struct iova_domain *from, struct iova_domain *to)
> {
> 	unsigned long flags;
> 	struct rb_node *node;
> 
> 	spin_lock_irqsave(&from->iova_rbtree_lock, flags);
> 	for (node = rb_first(&from->rbroot); node; node = rb_next(node)) {
> 		struct iova *iova = container_of(node, struct iova, node);
> 		struct iova *new_iova;
> 
> 		new_iova = reserve_iova(to, iova->pfn_lo, iova->pfn_hi);
> 		if (!new_iova)
> 			printk(KERN_ERR "Reserve iova range %lx@%lx failed\n",
> 				iova->pfn_lo, iova->pfn_lo);
> 	}
> 	spin_unlock_irqrestore(&from->iova_rbtree_lock, flags);
> }
> 
> struct iova *
> split_and_remove_iova(struct iova_domain *iovad, struct iova *iova,
> 		      unsigned long pfn_lo, unsigned long pfn_hi)

It would be nice to complete the kerneldoc, at least on the exported
interface.


> {
> 	unsigned long flags;
> 	struct iova *prev = NULL, *next = NULL;
> 
> 	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
> 	if (iova->pfn_lo < pfn_lo) {
> 		prev = alloc_and_init_iova(iova->pfn_lo, pfn_lo - 1);
> 		if (prev == NULL)
> 			goto error;
> 	}
> 	if (iova->pfn_hi > pfn_hi) {
> 		next = alloc_and_init_iova(pfn_hi + 1, iova->pfn_hi);
> 		if (next == NULL)
> 			goto error;
> 	}
> 
> 	__cached_rbnode_delete_update(iovad, iova);
> 	rb_erase(&iova->node, &iovad->rbroot);
> 
> 	if (prev) {
> 		iova_insert_rbtree(&iovad->rbroot, prev);
> 		iova->pfn_lo = pfn_lo;
> 	}
> 	if (next) {
> 		iova_insert_rbtree(&iovad->rbroot, next);
> 		iova->pfn_hi = pfn_hi;
> 	}
> 	spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
> 
> 	return iova;
> 
> error:
> 	spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
> 	if (prev)
> 		free_iova_mem(prev);
> 	return NULL;
> }
> 
> 

Re: [PATCH char-misc-next 10/19] lib: convert iova.c into a library

[Ashutosh Dixit]

On Tue, Jul 28 2015 at 01:40:19 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Mon, 27 Jul 2015 16:57:32 -0700 Ashutosh Dixit <ashutosh.dixit@intel.com> wrote:
>
>> From: Harish Chegondi <harish.chegondi@intel.com>
>>
>> This patch converts iova.c into a library, moving it from
>> drivers/iommu/ to lib/, and exports its virtual address allocation
>> and management functions so that other modules can reuse them.
>
> From the following emails it is unclear to me whether we'll actually
> be going ahead with this, but whatever.  It's a chance to do some code
> reading.

Thanks for the review. Either us or the IOMMU team will submit a patch
incorporating your suggestions.

Ashutosh