[PATCH kernel v5 0/6] powerpc/spapr/vfio: Put pages on VFIO container shutdown

[PATCH kernel v5 0/6] powerpc/spapr/vfio: Put pages on VFIO container shutdown

[Alexey Kardashevskiy]

These patches are to fix a bug when pages stay pinned hours
after QEMU which requested pinning exited.

The only change to v4 is it is now 6 patches and mm is
referenced rather later than sooner.

Please comment. Thanks.

Alexey Kardashevskiy (6):
  powerpc/iommu: Pass mm_struct to init/cleanup helpers
  powerpc/iommu: Stop using @current in mm_iommu_xxx
  vfio/spapr: Postpone allocation of userspace version of TCE table
  vfio/spapr: Postpone default window creation
  vfio/spapr: Reference mm in tce_container
  powerpc/mm/iommu, vfio/spapr: Put pages on VFIO container shutdown

 arch/powerpc/include/asm/mmu_context.h |  20 ++-
 arch/powerpc/kernel/setup-common.c     |   2 +-
 arch/powerpc/mm/mmu_context_book3s64.c |   6 +-
 arch/powerpc/mm/mmu_context_iommu.c    |  60 ++-----
 drivers/vfio/vfio_iommu_spapr_tce.c    | 316 ++++++++++++++++++++++-----------
 5 files changed, 240 insertions(+), 164 deletions(-)

-- 
2.5.0.rc3

[PATCH kernel v5 1/6] powerpc/iommu: Pass mm_struct to init/cleanup helpers

[Alexey Kardashevskiy]

We are going to get rid of @current references in mmu_context_boos3s64.c
and cache mm_struct in the VFIO container. Since mm_context_t does not
have reference counting, we will be using mm_struct which does have
the reference counter.

This changes mm_iommu_init/mm_iommu_cleanup to receive mm_struct rather
than mm_context_t (which is embedded into mm).

This should not cause any behavioral change.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 arch/powerpc/include/asm/mmu_context.h | 4 ++--
 arch/powerpc/kernel/setup-common.c     | 2 +-
 arch/powerpc/mm/mmu_context_book3s64.c | 4 ++--
 arch/powerpc/mm/mmu_context_iommu.c    | 9 +++++----
 4 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h
index 5c45114..424844b 100644
--- a/arch/powerpc/include/asm/mmu_context.h
+++ b/arch/powerpc/include/asm/mmu_context.h
@@ -23,8 +23,8 @@ extern bool mm_iommu_preregistered(void);
 extern long mm_iommu_get(unsigned long ua, unsigned long entries,
 		struct mm_iommu_table_group_mem_t **pmem);
 extern long mm_iommu_put(struct mm_iommu_table_group_mem_t *mem);
-extern void mm_iommu_init(mm_context_t *ctx);
-extern void mm_iommu_cleanup(mm_context_t *ctx);
+extern void mm_iommu_init(struct mm_struct *mm);
+extern void mm_iommu_cleanup(struct mm_struct *mm);
 extern struct mm_iommu_table_group_mem_t *mm_iommu_lookup(unsigned long ua,
 		unsigned long size);
 extern struct mm_iommu_table_group_mem_t *mm_iommu_find(unsigned long ua,
diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c
index 270ee30..f516ac5 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -915,7 +915,7 @@ void __init setup_arch(char **cmdline_p)
 	init_mm.context.pte_frag = NULL;
 #endif
 #ifdef CONFIG_SPAPR_TCE_IOMMU
-	mm_iommu_init(&init_mm.context);
+	mm_iommu_init(&init_mm);
 #endif
 	irqstack_early_init();
 	exc_lvl_early_init();
diff --git a/arch/powerpc/mm/mmu_context_book3s64.c b/arch/powerpc/mm/mmu_context_book3s64.c
index b114f8b..ad82735 100644
--- a/arch/powerpc/mm/mmu_context_book3s64.c
+++ b/arch/powerpc/mm/mmu_context_book3s64.c
@@ -115,7 +115,7 @@ int init_new_context(struct task_struct *tsk, struct mm_struct *mm)
 	mm->context.pte_frag = NULL;
 #endif
 #ifdef CONFIG_SPAPR_TCE_IOMMU
-	mm_iommu_init(&mm->context);
+	mm_iommu_init(mm);
 #endif
 	return 0;
 }
@@ -160,7 +160,7 @@ static inline void destroy_pagetable_page(struct mm_struct *mm)
 void destroy_context(struct mm_struct *mm)
 {
 #ifdef CONFIG_SPAPR_TCE_IOMMU
-	mm_iommu_cleanup(&mm->context);
+	mm_iommu_cleanup(mm);
 #endif
 
 #ifdef CONFIG_PPC_ICSWX
diff --git a/arch/powerpc/mm/mmu_context_iommu.c b/arch/powerpc/mm/mmu_context_iommu.c
index e0f1c33..ad2e575 100644
--- a/arch/powerpc/mm/mmu_context_iommu.c
+++ b/arch/powerpc/mm/mmu_context_iommu.c
@@ -373,16 +373,17 @@ void mm_iommu_mapped_dec(struct mm_iommu_table_group_mem_t *mem)
 }
 EXPORT_SYMBOL_GPL(mm_iommu_mapped_dec);
 
-void mm_iommu_init(mm_context_t *ctx)
+void mm_iommu_init(struct mm_struct *mm)
 {
-	INIT_LIST_HEAD_RCU(&ctx->iommu_group_mem_list);
+	INIT_LIST_HEAD_RCU(&mm->context.iommu_group_mem_list);
 }
 
-void mm_iommu_cleanup(mm_context_t *ctx)
+void mm_iommu_cleanup(struct mm_struct *mm)
 {
 	struct mm_iommu_table_group_mem_t *mem, *tmp;
 
-	list_for_each_entry_safe(mem, tmp, &ctx->iommu_group_mem_list, next) {
+	list_for_each_entry_safe(mem, tmp, &mm->context.iommu_group_mem_list,
+			next) {
 		list_del_rcu(&mem->next);
 		mm_iommu_do_free(mem);
 	}
-- 
2.5.0.rc3

[PATCH kernel v5 2/6] powerpc/iommu: Stop using @current in mm_iommu_xxx

[Alexey Kardashevskiy]

This changes mm_iommu_xxx helpers to take mm_struct as a parameter
instead of getting it from @current which in some situations may
not have a valid reference to mm.

This changes helpers to receive @mm and moves all references to @current
to the caller, including checks for !current and !current->mm;
checks in mm_iommu_preregistered() are removed as there is no caller
yet.

This moves the mm_iommu_adjust_locked_vm() call to the caller as
it receives mm_iommu_table_group_mem_t but it needs mm.

This should cause no behavioral change.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
---
 arch/powerpc/include/asm/mmu_context.h | 16 ++++++------
 arch/powerpc/mm/mmu_context_iommu.c    | 46 +++++++++++++---------------------
 drivers/vfio/vfio_iommu_spapr_tce.c    | 14 ++++++++---
 3 files changed, 36 insertions(+), 40 deletions(-)

diff --git a/arch/powerpc/include/asm/mmu_context.h b/arch/powerpc/include/asm/mmu_context.h
index 424844b..b9e3f0a 100644
--- a/arch/powerpc/include/asm/mmu_context.h
+++ b/arch/powerpc/include/asm/mmu_context.h
@@ -19,16 +19,18 @@ extern void destroy_context(struct mm_struct *mm);
 struct mm_iommu_table_group_mem_t;
 
 extern int isolate_lru_page(struct page *page);	/* from internal.h */
-extern bool mm_iommu_preregistered(void);
-extern long mm_iommu_get(unsigned long ua, unsigned long entries,
+extern bool mm_iommu_preregistered(struct mm_struct *mm);
+extern long mm_iommu_get(struct mm_struct *mm,
+		unsigned long ua, unsigned long entries,
 		struct mm_iommu_table_group_mem_t **pmem);
-extern long mm_iommu_put(struct mm_iommu_table_group_mem_t *mem);
+extern long mm_iommu_put(struct mm_struct *mm,
+		struct mm_iommu_table_group_mem_t *mem);
 extern void mm_iommu_init(struct mm_struct *mm);
 extern void mm_iommu_cleanup(struct mm_struct *mm);
-extern struct mm_iommu_table_group_mem_t *mm_iommu_lookup(unsigned long ua,
-		unsigned long size);
-extern struct mm_iommu_table_group_mem_t *mm_iommu_find(unsigned long ua,
-		unsigned long entries);
+extern struct mm_iommu_table_group_mem_t *mm_iommu_lookup(struct mm_struct *mm,
+		unsigned long ua, unsigned long size);
+extern struct mm_iommu_table_group_mem_t *mm_iommu_find(struct mm_struct *mm,
+		unsigned long ua, unsigned long entries);
 extern long mm_iommu_ua_to_hpa(struct mm_iommu_table_group_mem_t *mem,
 		unsigned long ua, unsigned long *hpa);
 extern long mm_iommu_mapped_inc(struct mm_iommu_table_group_mem_t *mem);
diff --git a/arch/powerpc/mm/mmu_context_iommu.c b/arch/powerpc/mm/mmu_context_iommu.c
index ad2e575..4c6db09 100644
--- a/arch/powerpc/mm/mmu_context_iommu.c
+++ b/arch/powerpc/mm/mmu_context_iommu.c
@@ -56,7 +56,7 @@ static long mm_iommu_adjust_locked_vm(struct mm_struct *mm,
 	}
 
 	pr_debug("[%d] RLIMIT_MEMLOCK HASH64 %c%ld %ld/%ld\n",
-			current->pid,
+			current ? current->pid : 0,
 			incr ? '+' : '-',
 			npages << PAGE_SHIFT,
 			mm->locked_vm << PAGE_SHIFT,
@@ -66,12 +66,9 @@ static long mm_iommu_adjust_locked_vm(struct mm_struct *mm,
 	return ret;
 }
 
-bool mm_iommu_preregistered(void)
+bool mm_iommu_preregistered(struct mm_struct *mm)
 {
-	if (!current || !current->mm)
-		return false;
-
-	return !list_empty(&current->mm->context.iommu_group_mem_list);
+	return !list_empty(&mm->context.iommu_group_mem_list);
 }
 EXPORT_SYMBOL_GPL(mm_iommu_preregistered);
 
@@ -124,19 +121,16 @@ static int mm_iommu_move_page_from_cma(struct page *page)
 	return 0;
 }
 
-long mm_iommu_get(unsigned long ua, unsigned long entries,
+long mm_iommu_get(struct mm_struct *mm, unsigned long ua, unsigned long entries,
 		struct mm_iommu_table_group_mem_t **pmem)
 {
 	struct mm_iommu_table_group_mem_t *mem;
 	long i, j, ret = 0, locked_entries = 0;
 	struct page *page = NULL;
 
-	if (!current || !current->mm)
-		return -ESRCH; /* process exited */
-
 	mutex_lock(&mem_list_mutex);
 
-	list_for_each_entry_rcu(mem, &current->mm->context.iommu_group_mem_list,
+	list_for_each_entry_rcu(mem, &mm->context.iommu_group_mem_list,
 			next) {
 		if ((mem->ua == ua) && (mem->entries == entries)) {
 			++mem->used;
@@ -154,7 +148,7 @@ long mm_iommu_get(unsigned long ua, unsigned long entries,
 
 	}
 
-	ret = mm_iommu_adjust_locked_vm(current->mm, entries, true);
+	ret = mm_iommu_adjust_locked_vm(mm, entries, true);
 	if (ret)
 		goto unlock_exit;
 
@@ -215,11 +209,11 @@ long mm_iommu_get(unsigned long ua, unsigned long entries,
 	mem->entries = entries;
 	*pmem = mem;
 
-	list_add_rcu(&mem->next, &current->mm->context.iommu_group_mem_list);
+	list_add_rcu(&mem->next, &mm->context.iommu_group_mem_list);
 
 unlock_exit:
 	if (locked_entries && ret)
-		mm_iommu_adjust_locked_vm(current->mm, locked_entries, false);
+		mm_iommu_adjust_locked_vm(mm, locked_entries, false);
 
 	mutex_unlock(&mem_list_mutex);
 
@@ -264,17 +258,13 @@ static void mm_iommu_free(struct rcu_head *head)
 static void mm_iommu_release(struct mm_iommu_table_group_mem_t *mem)
 {
 	list_del_rcu(&mem->next);
-	mm_iommu_adjust_locked_vm(current->mm, mem->entries, false);
 	call_rcu(&mem->rcu, mm_iommu_free);
 }
 
-long mm_iommu_put(struct mm_iommu_table_group_mem_t *mem)
+long mm_iommu_put(struct mm_struct *mm, struct mm_iommu_table_group_mem_t *mem)
 {
 	long ret = 0;
 
-	if (!current || !current->mm)
-		return -ESRCH; /* process exited */
-
 	mutex_lock(&mem_list_mutex);
 
 	if (mem->used == 0) {
@@ -297,6 +287,8 @@ long mm_iommu_put(struct mm_iommu_table_group_mem_t *mem)
 	/* @mapped became 0 so now mappings are disabled, release the region */
 	mm_iommu_release(mem);
 
+	mm_iommu_adjust_locked_vm(mm, mem->entries, false);
+
 unlock_exit:
 	mutex_unlock(&mem_list_mutex);
 
@@ -304,14 +296,12 @@ long mm_iommu_put(struct mm_iommu_table_group_mem_t *mem)
 }
 EXPORT_SYMBOL_GPL(mm_iommu_put);
 
-struct mm_iommu_table_group_mem_t *mm_iommu_lookup(unsigned long ua,
-		unsigned long size)
+struct mm_iommu_table_group_mem_t *mm_iommu_lookup(struct mm_struct *mm,
+		unsigned long ua, unsigned long size)
 {
 	struct mm_iommu_table_group_mem_t *mem, *ret = NULL;
 
-	list_for_each_entry_rcu(mem,
-			&current->mm->context.iommu_group_mem_list,
-			next) {
+	list_for_each_entry_rcu(mem, &mm->context.iommu_group_mem_list, next) {
 		if ((mem->ua <= ua) &&
 				(ua + size <= mem->ua +
 				 (mem->entries << PAGE_SHIFT))) {
@@ -324,14 +314,12 @@ struct mm_iommu_table_group_mem_t *mm_iommu_lookup(unsigned long ua,
 }
 EXPORT_SYMBOL_GPL(mm_iommu_lookup);
 
-struct mm_iommu_table_group_mem_t *mm_iommu_find(unsigned long ua,
-		unsigned long entries)
+struct mm_iommu_table_group_mem_t *mm_iommu_find(struct mm_struct *mm,
+		unsigned long ua, unsigned long entries)
 {
 	struct mm_iommu_table_group_mem_t *mem, *ret = NULL;
 
-	list_for_each_entry_rcu(mem,
-			&current->mm->context.iommu_group_mem_list,
-			next) {
+	list_for_each_entry_rcu(mem, &mm->context.iommu_group_mem_list, next) {
 		if ((mem->ua == ua) && (mem->entries == entries)) {
 			ret = mem;
 			break;
diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
index 80378dd..d0c38b2 100644
--- a/drivers/vfio/vfio_iommu_spapr_tce.c
+++ b/drivers/vfio/vfio_iommu_spapr_tce.c
@@ -107,14 +107,17 @@ static long tce_iommu_unregister_pages(struct tce_container *container,
 {
 	struct mm_iommu_table_group_mem_t *mem;
 
+	if (!current || !current->mm)
+		return -ESRCH; /* process exited */
+
 	if ((vaddr & ~PAGE_MASK) || (size & ~PAGE_MASK))
 		return -EINVAL;
 
-	mem = mm_iommu_find(vaddr, size >> PAGE_SHIFT);
+	mem = mm_iommu_find(current->mm, vaddr, size >> PAGE_SHIFT);
 	if (!mem)
 		return -ENOENT;
 
-	return mm_iommu_put(mem);
+	return mm_iommu_put(current->mm, mem);
 }
 
 static long tce_iommu_register_pages(struct tce_container *container,
@@ -124,11 +127,14 @@ static long tce_iommu_register_pages(struct tce_container *container,
 	struct mm_iommu_table_group_mem_t *mem = NULL;
 	unsigned long entries = size >> PAGE_SHIFT;
 
+	if (!current || !current->mm)
+		return -ESRCH; /* process exited */
+
 	if ((vaddr & ~PAGE_MASK) || (size & ~PAGE_MASK) ||
 			((vaddr + size) < vaddr))
 		return -EINVAL;
 
-	ret = mm_iommu_get(vaddr, entries, &mem);
+	ret = mm_iommu_get(current->mm, vaddr, entries, &mem);
 	if (ret)
 		return ret;
 
@@ -375,7 +381,7 @@ static int tce_iommu_prereg_ua_to_hpa(unsigned long tce, unsigned long size,
 	long ret = 0;
 	struct mm_iommu_table_group_mem_t *mem;
 
-	mem = mm_iommu_lookup(tce, size);
+	mem = mm_iommu_lookup(current->mm, tce, size);
 	if (!mem)
 		return -EINVAL;
 
-- 
2.5.0.rc3

[PATCH kernel v5 3/6] vfio/spapr: Postpone allocation of userspace version of TCE table

[Alexey Kardashevskiy]

The iommu_table struct manages a hardware TCE table and a vmalloc'd
table with corresponding userspace addresses. Both are allocated when
the default DMA window is created and this happens when the very first
group is attached to a container.

As we are going to allow the userspace to configure container in one
memory context and pas container fd to another, we have to postpones
such allocations till a container fd is passed to the destination
user process so we would account locked memory limit against the actual
container user constrainsts.

This postpones the it_userspace array allocation till it is used first
time for mapping. The unmapping patch already checks if the array is
allocated.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 drivers/vfio/vfio_iommu_spapr_tce.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
index d0c38b2..442baac 100644
--- a/drivers/vfio/vfio_iommu_spapr_tce.c
+++ b/drivers/vfio/vfio_iommu_spapr_tce.c
@@ -515,6 +515,12 @@ static long tce_iommu_build_v2(struct tce_container *container,
 	unsigned long hpa;
 	enum dma_data_direction dirtmp;
 
+	if (!tbl->it_userspace) {
+		ret = tce_iommu_userspace_view_alloc(tbl);
+		if (ret)
+			return ret;
+	}
+
 	for (i = 0; i < pages; ++i) {
 		struct mm_iommu_table_group_mem_t *mem = NULL;
 		unsigned long *pua = IOMMU_TABLE_USERSPACE_ENTRY(tbl,
@@ -1068,10 +1074,7 @@ static int tce_iommu_take_ownership(struct tce_container *container,
 		if (!tbl || !tbl->it_map)
 			continue;
 
-		rc = tce_iommu_userspace_view_alloc(tbl);
-		if (!rc)
-			rc = iommu_take_ownership(tbl);
-
+		rc = iommu_take_ownership(tbl);
 		if (rc) {
 			for (j = 0; j < i; ++j)
 				iommu_release_ownership(
-- 
2.5.0.rc3

[PATCH kernel v5 4/6] vfio/spapr: Postpone default window creation

[Alexey Kardashevskiy]

As mentioned in the previous patch, we are going to allow the userspace
to configure container in one memory context and pass container fd to
another so we are postponing memory allocations accounted against
the locked memory limit. The previous patch took care of it_userspace.

At the moment we create the default DMA window when the first group is
attached to a container; this is done for the userspace which is not
DDW-aware but familiar with the SPAPR TCE IOMMU v2 in the part of memory
pre-registration - such client expects the default DMA window to exist.

This postpones the default DMA window allocation till first map/unmap
request happens.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 drivers/vfio/vfio_iommu_spapr_tce.c | 98 ++++++++++++++++++-------------------
 1 file changed, 47 insertions(+), 51 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
index 442baac..1c02498 100644
--- a/drivers/vfio/vfio_iommu_spapr_tce.c
+++ b/drivers/vfio/vfio_iommu_spapr_tce.c
@@ -97,6 +97,7 @@ struct tce_container {
 	struct mutex lock;
 	bool enabled;
 	bool v2;
+	bool def_window_pending;
 	unsigned long locked_pages;
 	struct iommu_table *tables[IOMMU_TABLE_GROUP_MAX_TABLES];
 	struct list_head group_list;
@@ -594,15 +595,6 @@ static long tce_iommu_create_table(struct tce_container *container,
 	WARN_ON(!ret && !(*ptbl)->it_ops->free);
 	WARN_ON(!ret && ((*ptbl)->it_allocated_size != table_size));
 
-	if (!ret && container->v2) {
-		ret = tce_iommu_userspace_view_alloc(*ptbl);
-		if (ret)
-			(*ptbl)->it_ops->free(*ptbl);
-	}
-
-	if (ret)
-		decrement_locked_vm(table_size >> PAGE_SHIFT);
-
 	return ret;
 }
 
@@ -719,6 +711,29 @@ static long tce_iommu_remove_window(struct tce_container *container,
 	return 0;
 }
 
+static long tce_iommu_create_default_window(struct tce_container *container)
+{
+	long ret;
+	__u64 start_addr = 0;
+	struct tce_iommu_group *tcegrp;
+	struct iommu_table_group *table_group;
+
+	if (!tce_groups_attached(container))
+		return -ENODEV;
+
+	tcegrp = list_first_entry(&container->group_list,
+			struct tce_iommu_group, next);
+	table_group = iommu_group_get_iommudata(tcegrp->grp);
+	if (!table_group)
+		return -ENODEV;
+
+	ret = tce_iommu_create_window(container, IOMMU_PAGE_SHIFT_4K,
+			table_group->tce32_size, 1, &start_addr);
+	WARN_ON_ONCE(!ret && start_addr);
+
+	return ret;
+}
+
 static long tce_iommu_ioctl(void *iommu_data,
 				 unsigned int cmd, unsigned long arg)
 {
@@ -809,6 +824,13 @@ static long tce_iommu_ioctl(void *iommu_data,
 				VFIO_DMA_MAP_FLAG_WRITE))
 			return -EINVAL;
 
+		if (container->def_window_pending) {
+			ret = tce_iommu_create_default_window(container);
+			if (ret)
+				return ret;
+			container->def_window_pending = false;
+		}
+
 		num = tce_iommu_find_table(container, param.iova, &tbl);
 		if (num < 0)
 			return -ENXIO;
@@ -872,6 +894,13 @@ static long tce_iommu_ioctl(void *iommu_data,
 		if (param.flags)
 			return -EINVAL;
 
+		if (container->def_window_pending) {
+			ret = tce_iommu_create_default_window(container);
+			if (ret)
+				return ret;
+			container->def_window_pending = false;
+		}
+
 		num = tce_iommu_find_table(container, param.iova, &tbl);
 		if (num < 0)
 			return -ENXIO;
@@ -998,6 +1027,8 @@ static long tce_iommu_ioctl(void *iommu_data,
 
 		mutex_lock(&container->lock);
 
+		container->def_window_pending = false;
+
 		ret = tce_iommu_create_window(container, create.page_shift,
 				create.window_size, create.levels,
 				&create.start_addr);
@@ -1030,6 +1061,12 @@ static long tce_iommu_ioctl(void *iommu_data,
 		if (remove.flags)
 			return -EINVAL;
 
+		if (container->def_window_pending && !remove.start_addr) {
+			container->def_window_pending = false;
+			return 0;
+		}
+		container->def_window_pending = false;
+
 		mutex_lock(&container->lock);
 
 		ret = tce_iommu_remove_window(container, remove.start_addr);
@@ -1109,9 +1146,6 @@ static void tce_iommu_release_ownership_ddw(struct tce_container *container,
 static long tce_iommu_take_ownership_ddw(struct tce_container *container,
 		struct iommu_table_group *table_group)
 {
-	long i, ret = 0;
-	struct iommu_table *tbl = NULL;
-
 	if (!table_group->ops->create_table || !table_group->ops->set_window ||
 			!table_group->ops->release_ownership) {
 		WARN_ON_ONCE(1);
@@ -1120,47 +1154,9 @@ static long tce_iommu_take_ownership_ddw(struct tce_container *container,
 
 	table_group->ops->take_ownership(table_group);
 
-	/*
-	 * If it the first group attached, check if there is
-	 * a default DMA window and create one if none as
-	 * the userspace expects it to exist.
-	 */
-	if (!tce_groups_attached(container) && !container->tables[0]) {
-		ret = tce_iommu_create_table(container,
-				table_group,
-				0, /* window number */
-				IOMMU_PAGE_SHIFT_4K,
-				table_group->tce32_size,
-				1, /* default levels */
-				&tbl);
-		if (ret)
-			goto release_exit;
-		else
-			container->tables[0] = tbl;
-	}
-
-	/* Set all windows to the new group */
-	for (i = 0; i < IOMMU_TABLE_GROUP_MAX_TABLES; ++i) {
-		tbl = container->tables[i];
-
-		if (!tbl)
-			continue;
-
-		/* Set the default window to a new group */
-		ret = table_group->ops->set_window(table_group, i, tbl);
-		if (ret)
-			goto release_exit;
-	}
+	container->def_window_pending = true;
 
 	return 0;
-
-release_exit:
-	for (i = 0; i < IOMMU_TABLE_GROUP_MAX_TABLES; ++i)
-		table_group->ops->unset_window(table_group, i);
-
-	table_group->ops->release_ownership(table_group);
-
-	return ret;
 }
 
 static int tce_iommu_attach_group(void *iommu_data,
-- 
2.5.0.rc3

[PATCH kernel v5 5/6] vfio/spapr: Reference mm in tce_container

[Alexey Kardashevskiy]

In some situations the userspace memory context may live longer than
the userspace process itself so if we need to do proper memory context
cleanup, we better have tce_container take a reference to mm_struct and
use it later when the process is gone (@current or @current->mm is NULL).

This references mm and stores the pointer in the container; this is done
in a new helper - tce_iommu_mm_set() - when one of the following happens:
- a container is enabled (IOMMU v1);
- a first attempt to pre-register memory is made (IOMMU v2);
- a DMA window is created (IOMMU v2).
The @mm stays referenced till the container is destroyed.

This replaces current->mm with container->mm everywhere except debug
prints.

This adds a check that current->mm is the same as the one stored in
the container to prevent userspace from making changes to a memory
context of other processes.

DMA map/unmap ioctls() do not check for @mm as they already check
for @enabled which is set after tce_iommu_mm_set() is called.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v5:
* postpone referencing of mm

v4:
* added check for container->mm!=current->mm in tce_iommu_ioctl()
for all ioctls and removed other redundand checks
---
 drivers/vfio/vfio_iommu_spapr_tce.c | 159 ++++++++++++++++++++++--------------
 1 file changed, 99 insertions(+), 60 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
index 1c02498..9a81a7e 100644
--- a/drivers/vfio/vfio_iommu_spapr_tce.c
+++ b/drivers/vfio/vfio_iommu_spapr_tce.c
@@ -31,49 +31,49 @@
 static void tce_iommu_detach_group(void *iommu_data,
 		struct iommu_group *iommu_group);
 
-static long try_increment_locked_vm(long npages)
+static long try_increment_locked_vm(struct mm_struct *mm, long npages)
 {
 	long ret = 0, locked, lock_limit;
 
-	if (!current || !current->mm)
-		return -ESRCH; /* process exited */
+	if (!mm)
+		return -EPERM;
 
 	if (!npages)
 		return 0;
 
-	down_write(&current->mm->mmap_sem);
-	locked = current->mm->locked_vm + npages;
+	down_write(&mm->mmap_sem);
+	locked = mm->locked_vm + npages;
 	lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
 	if (locked > lock_limit && !capable(CAP_IPC_LOCK))
 		ret = -ENOMEM;
 	else
-		current->mm->locked_vm += npages;
+		mm->locked_vm += npages;
 
 	pr_debug("[%d] RLIMIT_MEMLOCK +%ld %ld/%ld%s\n", current->pid,
 			npages << PAGE_SHIFT,
-			current->mm->locked_vm << PAGE_SHIFT,
+			mm->locked_vm << PAGE_SHIFT,
 			rlimit(RLIMIT_MEMLOCK),
 			ret ? " - exceeded" : "");
 
-	up_write(&current->mm->mmap_sem);
+	up_write(&mm->mmap_sem);
 
 	return ret;
 }
 
-static void decrement_locked_vm(long npages)
+static void decrement_locked_vm(struct mm_struct *mm, long npages)
 {
-	if (!current || !current->mm || !npages)
-		return; /* process exited */
+	if (!mm && !npages)
+		return;
 
-	down_write(&current->mm->mmap_sem);
-	if (WARN_ON_ONCE(npages > current->mm->locked_vm))
-		npages = current->mm->locked_vm;
-	current->mm->locked_vm -= npages;
+	down_write(&mm->mmap_sem);
+	if (WARN_ON_ONCE(npages > mm->locked_vm))
+		npages = mm->locked_vm;
+	mm->locked_vm -= npages;
 	pr_debug("[%d] RLIMIT_MEMLOCK -%ld %ld/%ld\n", current->pid,
 			npages << PAGE_SHIFT,
-			current->mm->locked_vm << PAGE_SHIFT,
+			mm->locked_vm << PAGE_SHIFT,
 			rlimit(RLIMIT_MEMLOCK));
-	up_write(&current->mm->mmap_sem);
+	up_write(&mm->mmap_sem);
 }
 
 /*
@@ -99,26 +99,38 @@ struct tce_container {
 	bool v2;
 	bool def_window_pending;
 	unsigned long locked_pages;
+	struct mm_struct *mm;
 	struct iommu_table *tables[IOMMU_TABLE_GROUP_MAX_TABLES];
 	struct list_head group_list;
 };
 
+static long tce_iommu_mm_set(struct tce_container *container)
+{
+	if (container->mm) {
+		if (container->mm == current->mm)
+			return 0;
+		return -EPERM;
+	}
+	BUG_ON(!current->mm);
+	container->mm = current->mm;
+	atomic_inc(&container->mm->mm_count);
+
+	return 0;
+}
+
 static long tce_iommu_unregister_pages(struct tce_container *container,
 		__u64 vaddr, __u64 size)
 {
 	struct mm_iommu_table_group_mem_t *mem;
 
-	if (!current || !current->mm)
-		return -ESRCH; /* process exited */
-
 	if ((vaddr & ~PAGE_MASK) || (size & ~PAGE_MASK))
 		return -EINVAL;
 
-	mem = mm_iommu_find(current->mm, vaddr, size >> PAGE_SHIFT);
+	mem = mm_iommu_find(container->mm, vaddr, size >> PAGE_SHIFT);
 	if (!mem)
 		return -ENOENT;
 
-	return mm_iommu_put(current->mm, mem);
+	return mm_iommu_put(container->mm, mem);
 }
 
 static long tce_iommu_register_pages(struct tce_container *container,
@@ -128,14 +140,11 @@ static long tce_iommu_register_pages(struct tce_container *container,
 	struct mm_iommu_table_group_mem_t *mem = NULL;
 	unsigned long entries = size >> PAGE_SHIFT;
 
-	if (!current || !current->mm)
-		return -ESRCH; /* process exited */
-
 	if ((vaddr & ~PAGE_MASK) || (size & ~PAGE_MASK) ||
 			((vaddr + size) < vaddr))
 		return -EINVAL;
 
-	ret = mm_iommu_get(current->mm, vaddr, entries, &mem);
+	ret = mm_iommu_get(container->mm, vaddr, entries, &mem);
 	if (ret)
 		return ret;
 
@@ -144,7 +153,8 @@ static long tce_iommu_register_pages(struct tce_container *container,
 	return 0;
 }
 
-static long tce_iommu_userspace_view_alloc(struct iommu_table *tbl)
+static long tce_iommu_userspace_view_alloc(struct iommu_table *tbl,
+		struct mm_struct *mm)
 {
 	unsigned long cb = _ALIGN_UP(sizeof(tbl->it_userspace[0]) *
 			tbl->it_size, PAGE_SIZE);
@@ -153,13 +163,13 @@ static long tce_iommu_userspace_view_alloc(struct iommu_table *tbl)
 
 	BUG_ON(tbl->it_userspace);
 
-	ret = try_increment_locked_vm(cb >> PAGE_SHIFT);
+	ret = try_increment_locked_vm(mm, cb >> PAGE_SHIFT);
 	if (ret)
 		return ret;
 
 	uas = vzalloc(cb);
 	if (!uas) {
-		decrement_locked_vm(cb >> PAGE_SHIFT);
+		decrement_locked_vm(mm, cb >> PAGE_SHIFT);
 		return -ENOMEM;
 	}
 	tbl->it_userspace = uas;
@@ -167,7 +177,8 @@ static long tce_iommu_userspace_view_alloc(struct iommu_table *tbl)
 	return 0;
 }
 
-static void tce_iommu_userspace_view_free(struct iommu_table *tbl)
+static void tce_iommu_userspace_view_free(struct iommu_table *tbl,
+		struct mm_struct *mm)
 {
 	unsigned long cb = _ALIGN_UP(sizeof(tbl->it_userspace[0]) *
 			tbl->it_size, PAGE_SIZE);
@@ -177,7 +188,7 @@ static void tce_iommu_userspace_view_free(struct iommu_table *tbl)
 
 	vfree(tbl->it_userspace);
 	tbl->it_userspace = NULL;
-	decrement_locked_vm(cb >> PAGE_SHIFT);
+	decrement_locked_vm(mm, cb >> PAGE_SHIFT);
 }
 
 static bool tce_page_is_contained(struct page *page, unsigned page_shift)
@@ -237,9 +248,6 @@ static int tce_iommu_enable(struct tce_container *container)
 	struct iommu_table_group *table_group;
 	struct tce_iommu_group *tcegrp;
 
-	if (!current->mm)
-		return -ESRCH; /* process exited */
-
 	if (container->enabled)
 		return -EBUSY;
 
@@ -284,8 +292,12 @@ static int tce_iommu_enable(struct tce_container *container)
 	if (!table_group->tce32_size)
 		return -EPERM;
 
+	ret = tce_iommu_mm_set(container);
+	if (ret)
+		return ret;
+
 	locked = table_group->tce32_size >> PAGE_SHIFT;
-	ret = try_increment_locked_vm(locked);
+	ret = try_increment_locked_vm(container->mm, locked);
 	if (ret)
 		return ret;
 
@@ -303,10 +315,8 @@ static void tce_iommu_disable(struct tce_container *container)
 
 	container->enabled = false;
 
-	if (!current->mm)
-		return;
-
-	decrement_locked_vm(container->locked_pages);
+	BUG_ON(!container->mm);
+	decrement_locked_vm(container->mm, container->locked_pages);
 }
 
 static void *tce_iommu_open(unsigned long arg)
@@ -333,7 +343,8 @@ static void *tce_iommu_open(unsigned long arg)
 static int tce_iommu_clear(struct tce_container *container,
 		struct iommu_table *tbl,
 		unsigned long entry, unsigned long pages);
-static void tce_iommu_free_table(struct iommu_table *tbl);
+static void tce_iommu_free_table(struct tce_container *container,
+		struct iommu_table *tbl);
 
 static void tce_iommu_release(void *iommu_data)
 {
@@ -358,10 +369,12 @@ static void tce_iommu_release(void *iommu_data)
 			continue;
 
 		tce_iommu_clear(container, tbl, tbl->it_offset, tbl->it_size);
-		tce_iommu_free_table(tbl);
+		tce_iommu_free_table(container, tbl);
 	}
 
 	tce_iommu_disable(container);
+	if (container->mm)
+		mmdrop(container->mm);
 	mutex_destroy(&container->lock);
 
 	kfree(container);
@@ -376,13 +389,14 @@ static void tce_iommu_unuse_page(struct tce_container *container,
 	put_page(page);
 }
 
-static int tce_iommu_prereg_ua_to_hpa(unsigned long tce, unsigned long size,
+static int tce_iommu_prereg_ua_to_hpa(struct tce_container *container,
+		unsigned long tce, unsigned long size,
 		unsigned long *phpa, struct mm_iommu_table_group_mem_t **pmem)
 {
 	long ret = 0;
 	struct mm_iommu_table_group_mem_t *mem;
 
-	mem = mm_iommu_lookup(current->mm, tce, size);
+	mem = mm_iommu_lookup(container->mm, tce, size);
 	if (!mem)
 		return -EINVAL;
 
@@ -395,18 +409,18 @@ static int tce_iommu_prereg_ua_to_hpa(unsigned long tce, unsigned long size,
 	return 0;
 }
 
-static void tce_iommu_unuse_page_v2(struct iommu_table *tbl,
-		unsigned long entry)
+static void tce_iommu_unuse_page_v2(struct tce_container *container,
+		struct iommu_table *tbl, unsigned long entry)
 {
 	struct mm_iommu_table_group_mem_t *mem = NULL;
 	int ret;
 	unsigned long hpa = 0;
 	unsigned long *pua = IOMMU_TABLE_USERSPACE_ENTRY(tbl, entry);
 
-	if (!pua || !current || !current->mm)
+	if (!pua)
 		return;
 
-	ret = tce_iommu_prereg_ua_to_hpa(*pua, IOMMU_PAGE_SIZE(tbl),
+	ret = tce_iommu_prereg_ua_to_hpa(container, *pua, IOMMU_PAGE_SIZE(tbl),
 			&hpa, &mem);
 	if (ret)
 		pr_debug("%s: tce %lx at #%lx was not cached, ret=%d\n",
@@ -436,7 +450,7 @@ static int tce_iommu_clear(struct tce_container *container,
 			continue;
 
 		if (container->v2) {
-			tce_iommu_unuse_page_v2(tbl, entry);
+			tce_iommu_unuse_page_v2(container, tbl, entry);
 			continue;
 		}
 
@@ -517,7 +531,7 @@ static long tce_iommu_build_v2(struct tce_container *container,
 	enum dma_data_direction dirtmp;
 
 	if (!tbl->it_userspace) {
-		ret = tce_iommu_userspace_view_alloc(tbl);
+		ret = tce_iommu_userspace_view_alloc(tbl, container->mm);
 		if (ret)
 			return ret;
 	}
@@ -527,8 +541,8 @@ static long tce_iommu_build_v2(struct tce_container *container,
 		unsigned long *pua = IOMMU_TABLE_USERSPACE_ENTRY(tbl,
 				entry + i);
 
-		ret = tce_iommu_prereg_ua_to_hpa(tce, IOMMU_PAGE_SIZE(tbl),
-				&hpa, &mem);
+		ret = tce_iommu_prereg_ua_to_hpa(container,
+				tce, IOMMU_PAGE_SIZE(tbl), &hpa, &mem);
 		if (ret)
 			break;
 
@@ -549,7 +563,7 @@ static long tce_iommu_build_v2(struct tce_container *container,
 		ret = iommu_tce_xchg(tbl, entry + i, &hpa, &dirtmp);
 		if (ret) {
 			/* dirtmp cannot be DMA_NONE here */
-			tce_iommu_unuse_page_v2(tbl, entry + i);
+			tce_iommu_unuse_page_v2(container, tbl, entry + i);
 			pr_err("iommu_tce: %s failed ioba=%lx, tce=%lx, ret=%ld\n",
 					__func__, entry << tbl->it_page_shift,
 					tce, ret);
@@ -557,7 +571,7 @@ static long tce_iommu_build_v2(struct tce_container *container,
 		}
 
 		if (dirtmp != DMA_NONE)
-			tce_iommu_unuse_page_v2(tbl, entry + i);
+			tce_iommu_unuse_page_v2(container, tbl, entry + i);
 
 		*pua = tce;
 
@@ -585,7 +599,7 @@ static long tce_iommu_create_table(struct tce_container *container,
 	if (!table_size)
 		return -EINVAL;
 
-	ret = try_increment_locked_vm(table_size >> PAGE_SHIFT);
+	ret = try_increment_locked_vm(container->mm, table_size >> PAGE_SHIFT);
 	if (ret)
 		return ret;
 
@@ -598,13 +612,14 @@ static long tce_iommu_create_table(struct tce_container *container,
 	return ret;
 }
 
-static void tce_iommu_free_table(struct iommu_table *tbl)
+static void tce_iommu_free_table(struct tce_container *container,
+		struct iommu_table *tbl)
 {
 	unsigned long pages = tbl->it_allocated_size >> PAGE_SHIFT;
 
-	tce_iommu_userspace_view_free(tbl);
+	tce_iommu_userspace_view_free(tbl, container->mm);
 	tbl->it_ops->free(tbl);
-	decrement_locked_vm(pages);
+	decrement_locked_vm(container->mm, pages);
 }
 
 static long tce_iommu_create_window(struct tce_container *container,
@@ -667,7 +682,7 @@ static long tce_iommu_create_window(struct tce_container *container,
 		table_group = iommu_group_get_iommudata(tcegrp->grp);
 		table_group->ops->unset_window(table_group, num);
 	}
-	tce_iommu_free_table(tbl);
+	tce_iommu_free_table(container, tbl);
 
 	return ret;
 }
@@ -705,7 +720,7 @@ static long tce_iommu_remove_window(struct tce_container *container,
 
 	/* Free table */
 	tce_iommu_clear(container, tbl, tbl->it_offset, tbl->it_size);
-	tce_iommu_free_table(tbl);
+	tce_iommu_free_table(container, tbl);
 	container->tables[num] = NULL;
 
 	return 0;
@@ -754,7 +769,17 @@ static long tce_iommu_ioctl(void *iommu_data,
 		}
 
 		return (ret < 0) ? 0 : ret;
+	}
 
+	/*
+	 * Sanity check to prevent one userspace from manipulating
+	 * another userspace mm.
+	 */
+	BUG_ON(!container);
+	if (container->mm && container->mm != current->mm)
+		return -EPERM;
+
+	switch (cmd) {
 	case VFIO_IOMMU_SPAPR_TCE_GET_INFO: {
 		struct vfio_iommu_spapr_tce_info info;
 		struct tce_iommu_group *tcegrp;
@@ -929,6 +954,10 @@ static long tce_iommu_ioctl(void *iommu_data,
 		minsz = offsetofend(struct vfio_iommu_spapr_register_memory,
 				size);
 
+		ret = tce_iommu_mm_set(container);
+		if (ret)
+			return ret;
+
 		if (copy_from_user(&param, (void __user *)arg, minsz))
 			return -EFAULT;
 
@@ -952,6 +981,9 @@ static long tce_iommu_ioctl(void *iommu_data,
 		if (!container->v2)
 			break;
 
+		if (!container->mm)
+			return -EPERM;
+
 		minsz = offsetofend(struct vfio_iommu_spapr_register_memory,
 				size);
 
@@ -1010,6 +1042,10 @@ static long tce_iommu_ioctl(void *iommu_data,
 		if (!container->v2)
 			break;
 
+		ret = tce_iommu_mm_set(container);
+		if (ret)
+			return ret;
+
 		if (!tce_groups_attached(container))
 			return -ENXIO;
 
@@ -1046,6 +1082,9 @@ static long tce_iommu_ioctl(void *iommu_data,
 		if (!container->v2)
 			break;
 
+		if (!container->mm)
+			return -EPERM;
+
 		if (!tce_groups_attached(container))
 			return -ENXIO;
 
@@ -1092,7 +1131,7 @@ static void tce_iommu_release_ownership(struct tce_container *container,
 			continue;
 
 		tce_iommu_clear(container, tbl, tbl->it_offset, tbl->it_size);
-		tce_iommu_userspace_view_free(tbl);
+		tce_iommu_userspace_view_free(tbl, container->mm);
 		if (tbl->it_map)
 			iommu_release_ownership(tbl);
 
-- 
2.5.0.rc3

[PATCH kernel v5 6/6] powerpc/mm/iommu, vfio/spapr: Put pages on VFIO container shutdown

[Alexey Kardashevskiy]

At the moment the userspace tool is expected to request pinning of
the entire guest RAM when VFIO IOMMU SPAPR v2 driver is present.
When the userspace process finishes, all the pinned pages need to
be put; this is done as a part of the userspace memory context (MM)
destruction which happens on the very last mmdrop().

This approach has a problem that a MM of the userspace process
may live longer than the userspace process itself as kernel threads
use userspace process MMs which was runnning on a CPU where
the kernel thread was scheduled to. If this happened, the MM remains
referenced until this exact kernel thread wakes up again
and releases the very last reference to the MM, on an idle system this
can take even hours.

This moves preregistered regions tracking from MM to VFIO; insteads of
using mm_iommu_table_group_mem_t::used, tce_container::prereg_list is
added so each container releases regions which it has pre-registered.

This changes the userspace interface to return EBUSY if a memory
region is already registered in a container. However it should not
have any practical effect as the only userspace tool available now
does register memory region once per container anyway.

As tce_iommu_register_pages/tce_iommu_unregister_pages are called
under container->lock, this does not need additional locking.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
---
Changes:
v4:
* changed tce_iommu_register_pages() to call mm_iommu_find() first and
avoid calling mm_iommu_put() if memory is preregistered already

v3:
* moved tce_iommu_prereg_free() call out of list_for_each_entry()

v2:
* updated commit log
---
 arch/powerpc/mm/mmu_context_book3s64.c |  4 ---
 arch/powerpc/mm/mmu_context_iommu.c    | 11 -------
 drivers/vfio/vfio_iommu_spapr_tce.c    | 58 +++++++++++++++++++++++++++++++++-
 3 files changed, 57 insertions(+), 16 deletions(-)

diff --git a/arch/powerpc/mm/mmu_context_book3s64.c b/arch/powerpc/mm/mmu_context_book3s64.c
index ad82735..1a07969 100644
--- a/arch/powerpc/mm/mmu_context_book3s64.c
+++ b/arch/powerpc/mm/mmu_context_book3s64.c
@@ -159,10 +159,6 @@ static inline void destroy_pagetable_page(struct mm_struct *mm)
 
 void destroy_context(struct mm_struct *mm)
 {
-#ifdef CONFIG_SPAPR_TCE_IOMMU
-	mm_iommu_cleanup(mm);
-#endif
-
 #ifdef CONFIG_PPC_ICSWX
 	drop_cop(mm->context.acop, mm);
 	kfree(mm->context.cop_lockp);
diff --git a/arch/powerpc/mm/mmu_context_iommu.c b/arch/powerpc/mm/mmu_context_iommu.c
index 4c6db09..104bad0 100644
--- a/arch/powerpc/mm/mmu_context_iommu.c
+++ b/arch/powerpc/mm/mmu_context_iommu.c
@@ -365,14 +365,3 @@ void mm_iommu_init(struct mm_struct *mm)
 {
 	INIT_LIST_HEAD_RCU(&mm->context.iommu_group_mem_list);
 }
-
-void mm_iommu_cleanup(struct mm_struct *mm)
-{
-	struct mm_iommu_table_group_mem_t *mem, *tmp;
-
-	list_for_each_entry_safe(mem, tmp, &mm->context.iommu_group_mem_list,
-			next) {
-		list_del_rcu(&mem->next);
-		mm_iommu_do_free(mem);
-	}
-}
diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
index 9a81a7e..6682431 100644
--- a/drivers/vfio/vfio_iommu_spapr_tce.c
+++ b/drivers/vfio/vfio_iommu_spapr_tce.c
@@ -89,6 +89,15 @@ struct tce_iommu_group {
 };
 
 /*
+ * A container needs to remember which preregistered region  it has
+ * referenced to do proper cleanup at the userspace process exit.
+ */
+struct tce_iommu_prereg {
+	struct list_head next;
+	struct mm_iommu_table_group_mem_t *mem;
+};
+
+/*
  * The container descriptor supports only a single group per container.
  * Required by the API as the container is not supplied with the IOMMU group
  * at the moment of initialization.
@@ -102,6 +111,7 @@ struct tce_container {
 	struct mm_struct *mm;
 	struct iommu_table *tables[IOMMU_TABLE_GROUP_MAX_TABLES];
 	struct list_head group_list;
+	struct list_head prereg_list;
 };
 
 static long tce_iommu_mm_set(struct tce_container *container)
@@ -118,10 +128,24 @@ static long tce_iommu_mm_set(struct tce_container *container)
 	return 0;
 }
 
+static long tce_iommu_prereg_free(struct tce_container *container,
+		struct tce_iommu_prereg *tcemem)
+{
+	long ret;
+
+	list_del(&tcemem->next);
+	ret = mm_iommu_put(container->mm, tcemem->mem);
+	kfree(tcemem);
+
+	return ret;
+}
+
 static long tce_iommu_unregister_pages(struct tce_container *container,
 		__u64 vaddr, __u64 size)
 {
 	struct mm_iommu_table_group_mem_t *mem;
+	struct tce_iommu_prereg *tcemem;
+	bool found = false;
 
 	if ((vaddr & ~PAGE_MASK) || (size & ~PAGE_MASK))
 		return -EINVAL;
@@ -130,7 +154,17 @@ static long tce_iommu_unregister_pages(struct tce_container *container,
 	if (!mem)
 		return -ENOENT;
 
-	return mm_iommu_put(container->mm, mem);
+	list_for_each_entry(tcemem, &container->prereg_list, next) {
+		if (tcemem->mem == mem) {
+			found = true;
+			break;
+		}
+	}
+
+	if (!found)
+		return -ENOENT;
+
+	return tce_iommu_prereg_free(container, tcemem);
 }
 
 static long tce_iommu_register_pages(struct tce_container *container,
@@ -138,16 +172,29 @@ static long tce_iommu_register_pages(struct tce_container *container,
 {
 	long ret = 0;
 	struct mm_iommu_table_group_mem_t *mem = NULL;
+	struct tce_iommu_prereg *tcemem;
 	unsigned long entries = size >> PAGE_SHIFT;
 
 	if ((vaddr & ~PAGE_MASK) || (size & ~PAGE_MASK) ||
 			((vaddr + size) < vaddr))
 		return -EINVAL;
 
+	mem = mm_iommu_find(container->mm, vaddr, entries);
+	if (mem) {
+		list_for_each_entry(tcemem, &container->prereg_list, next) {
+			if (tcemem->mem == mem)
+				return -EBUSY;
+		}
+	}
+
 	ret = mm_iommu_get(container->mm, vaddr, entries, &mem);
 	if (ret)
 		return ret;
 
+	tcemem = kzalloc(sizeof(*tcemem), GFP_KERNEL);
+	tcemem->mem = mem;
+	list_add(&tcemem->next, &container->prereg_list);
+
 	container->enabled = true;
 
 	return 0;
@@ -334,6 +381,7 @@ static void *tce_iommu_open(unsigned long arg)
 
 	mutex_init(&container->lock);
 	INIT_LIST_HEAD_RCU(&container->group_list);
+	INIT_LIST_HEAD_RCU(&container->prereg_list);
 
 	container->v2 = arg == VFIO_SPAPR_TCE_v2_IOMMU;
 
@@ -372,6 +420,14 @@ static void tce_iommu_release(void *iommu_data)
 		tce_iommu_free_table(container, tbl);
 	}
 
+	while (!list_empty(&container->prereg_list)) {
+		struct tce_iommu_prereg *tcemem;
+
+		tcemem = list_first_entry(&container->prereg_list,
+				struct tce_iommu_prereg, next);
+		tce_iommu_prereg_free(container, tcemem);
+	}
+
 	tce_iommu_disable(container);
 	if (container->mm)
 		mmdrop(container->mm);
-- 
2.5.0.rc3

Re: [PATCH kernel v5 5/6] vfio/spapr: Reference mm in tce_container

[Alexey Kardashevskiy]

On 11/11/16 23:32, Alexey Kardashevskiy wrote:
> In some situations the userspace memory context may live longer than
> the userspace process itself so if we need to do proper memory context
> cleanup, we better have tce_container take a reference to mm_struct and
> use it later when the process is gone (@current or @current->mm is NULL).
> 
> This references mm and stores the pointer in the container; this is done
> in a new helper - tce_iommu_mm_set() - when one of the following happens:
> - a container is enabled (IOMMU v1);
> - a first attempt to pre-register memory is made (IOMMU v2);
> - a DMA window is created (IOMMU v2).
> The @mm stays referenced till the container is destroyed.
> 
> This replaces current->mm with container->mm everywhere except debug
> prints.
> 
> This adds a check that current->mm is the same as the one stored in
> the container to prevent userspace from making changes to a memory
> context of other processes.
> 
> DMA map/unmap ioctls() do not check for @mm as they already check
> for @enabled which is set after tce_iommu_mm_set() is called.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> Changes:
> v5:
> * postpone referencing of mm
> 
> v4:
> * added check for container->mm!=current->mm in tce_iommu_ioctl()
> for all ioctls and removed other redundand checks
> ---
>  drivers/vfio/vfio_iommu_spapr_tce.c | 159 ++++++++++++++++++++++--------------
>  1 file changed, 99 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> index 1c02498..9a81a7e 100644
> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> @@ -31,49 +31,49 @@
>  static void tce_iommu_detach_group(void *iommu_data,
>  		struct iommu_group *iommu_group);
>  
> -static long try_increment_locked_vm(long npages)
> +static long try_increment_locked_vm(struct mm_struct *mm, long npages)
>  {
>  	long ret = 0, locked, lock_limit;
>  
> -	if (!current || !current->mm)
> -		return -ESRCH; /* process exited */
> +	if (!mm)
> +		return -EPERM;
>  
>  	if (!npages)
>  		return 0;
>  
> -	down_write(&current->mm->mmap_sem);
> -	locked = current->mm->locked_vm + npages;
> +	down_write(&mm->mmap_sem);
> +	locked = mm->locked_vm + npages;
>  	lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
>  	if (locked > lock_limit && !capable(CAP_IPC_LOCK))



Oh boy. Now it seems I have to reference a task, not just mm (which I may
not have to reference at all after all as the task reference should keep mm
alive) as I missed the fact capable() and rlimit() are working with @current.


Alex,

Is there anything else I should fix before posting v6? Thanks.



-- 
Alexey

Re: [PATCH kernel v5 5/6] vfio/spapr: Reference mm in tce_container

[Alex Williamson]

On Thu, 17 Nov 2016 18:39:41 +1100
Alexey Kardashevskiy <aik@ozlabs.ru> wrote:

> On 11/11/16 23:32, Alexey Kardashevskiy wrote:
> > In some situations the userspace memory context may live longer than
> > the userspace process itself so if we need to do proper memory context
> > cleanup, we better have tce_container take a reference to mm_struct and
> > use it later when the process is gone (@current or @current->mm is NULL).
> > 
> > This references mm and stores the pointer in the container; this is done
> > in a new helper - tce_iommu_mm_set() - when one of the following happens:
> > - a container is enabled (IOMMU v1);
> > - a first attempt to pre-register memory is made (IOMMU v2);
> > - a DMA window is created (IOMMU v2).
> > The @mm stays referenced till the container is destroyed.
> > 
> > This replaces current->mm with container->mm everywhere except debug
> > prints.
> > 
> > This adds a check that current->mm is the same as the one stored in
> > the container to prevent userspace from making changes to a memory
> > context of other processes.
> > 
> > DMA map/unmap ioctls() do not check for @mm as they already check
> > for @enabled which is set after tce_iommu_mm_set() is called.
> > 
> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > ---
> > Changes:
> > v5:
> > * postpone referencing of mm
> > 
> > v4:
> > * added check for container->mm!=current->mm in tce_iommu_ioctl()
> > for all ioctls and removed other redundand checks
> > ---
> >  drivers/vfio/vfio_iommu_spapr_tce.c | 159 ++++++++++++++++++++++--------------
> >  1 file changed, 99 insertions(+), 60 deletions(-)
> > 
> > diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> > index 1c02498..9a81a7e 100644
> > --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> > +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> > @@ -31,49 +31,49 @@
> >  static void tce_iommu_detach_group(void *iommu_data,
> >  		struct iommu_group *iommu_group);
> >  
> > -static long try_increment_locked_vm(long npages)
> > +static long try_increment_locked_vm(struct mm_struct *mm, long npages)
> >  {
> >  	long ret = 0, locked, lock_limit;
> >  
> > -	if (!current || !current->mm)
> > -		return -ESRCH; /* process exited */
> > +	if (!mm)
> > +		return -EPERM;
> >  
> >  	if (!npages)
> >  		return 0;
> >  
> > -	down_write(&current->mm->mmap_sem);
> > -	locked = current->mm->locked_vm + npages;
> > +	down_write(&mm->mmap_sem);
> > +	locked = mm->locked_vm + npages;
> >  	lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> >  	if (locked > lock_limit && !capable(CAP_IPC_LOCK))  
> 
> 
> 
> Oh boy. Now it seems I have to reference a task, not just mm (which I may
> not have to reference at all after all as the task reference should keep mm
> alive) as I missed the fact capable() and rlimit() are working with @current.
> 
> 
> Alex,
> 
> Is there anything else I should fix before posting v6? Thanks.

Nope, I was just hoping to see a R-b from David, you guys know the
spapr-tce iommu code far better than me.  Thanks,

Alex

Re: [PATCH kernel v5 3/6] vfio/spapr: Postpone allocation of userspace version of TCE table

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2265 bytes --]

On Fri, Nov 11, 2016 at 11:32:14PM +1100, Alexey Kardashevskiy wrote:
> The iommu_table struct manages a hardware TCE table and a vmalloc'd
> table with corresponding userspace addresses. Both are allocated when
> the default DMA window is created and this happens when the very first
> group is attached to a container.
> 
> As we are going to allow the userspace to configure container in one
> memory context and pas container fd to another, we have to postpones
> such allocations till a container fd is passed to the destination
> user process so we would account locked memory limit against the actual
> container user constrainsts.
> 
> This postpones the it_userspace array allocation till it is used first
> time for mapping. The unmapping patch already checks if the array is
> allocated.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  drivers/vfio/vfio_iommu_spapr_tce.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> index d0c38b2..442baac 100644
> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> @@ -515,6 +515,12 @@ static long tce_iommu_build_v2(struct tce_container *container,
>  	unsigned long hpa;
>  	enum dma_data_direction dirtmp;
>  
> +	if (!tbl->it_userspace) {
> +		ret = tce_iommu_userspace_view_alloc(tbl);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	for (i = 0; i < pages; ++i) {
>  		struct mm_iommu_table_group_mem_t *mem = NULL;
>  		unsigned long *pua = IOMMU_TABLE_USERSPACE_ENTRY(tbl,
> @@ -1068,10 +1074,7 @@ static int tce_iommu_take_ownership(struct tce_container *container,
>  		if (!tbl || !tbl->it_map)
>  			continue;
>  
> -		rc = tce_iommu_userspace_view_alloc(tbl);
> -		if (!rc)
> -			rc = iommu_take_ownership(tbl);
> -
> +		rc = iommu_take_ownership(tbl);
>  		if (rc) {
>  			for (j = 0; j < i; ++j)
>  				iommu_release_ownership(

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH kernel v5 5/6] vfio/spapr: Reference mm in tce_container

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3266 bytes --]

On Thu, Nov 17, 2016 at 06:39:41PM +1100, Alexey Kardashevskiy wrote:
> On 11/11/16 23:32, Alexey Kardashevskiy wrote:
> > In some situations the userspace memory context may live longer than
> > the userspace process itself so if we need to do proper memory context
> > cleanup, we better have tce_container take a reference to mm_struct and
> > use it later when the process is gone (@current or @current->mm is NULL).
> > 
> > This references mm and stores the pointer in the container; this is done
> > in a new helper - tce_iommu_mm_set() - when one of the following happens:
> > - a container is enabled (IOMMU v1);
> > - a first attempt to pre-register memory is made (IOMMU v2);
> > - a DMA window is created (IOMMU v2).
> > The @mm stays referenced till the container is destroyed.
> > 
> > This replaces current->mm with container->mm everywhere except debug
> > prints.
> > 
> > This adds a check that current->mm is the same as the one stored in
> > the container to prevent userspace from making changes to a memory
> > context of other processes.
> > 
> > DMA map/unmap ioctls() do not check for @mm as they already check
> > for @enabled which is set after tce_iommu_mm_set() is called.
> > 
> > Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> > ---
> > Changes:
> > v5:
> > * postpone referencing of mm
> > 
> > v4:
> > * added check for container->mm!=current->mm in tce_iommu_ioctl()
> > for all ioctls and removed other redundand checks
> > ---
> >  drivers/vfio/vfio_iommu_spapr_tce.c | 159 ++++++++++++++++++++++--------------
> >  1 file changed, 99 insertions(+), 60 deletions(-)
> > 
> > diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> > index 1c02498..9a81a7e 100644
> > --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> > +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> > @@ -31,49 +31,49 @@
> >  static void tce_iommu_detach_group(void *iommu_data,
> >  		struct iommu_group *iommu_group);
> >  
> > -static long try_increment_locked_vm(long npages)
> > +static long try_increment_locked_vm(struct mm_struct *mm, long npages)
> >  {
> >  	long ret = 0, locked, lock_limit;
> >  
> > -	if (!current || !current->mm)
> > -		return -ESRCH; /* process exited */
> > +	if (!mm)
> > +		return -EPERM;
> >  
> >  	if (!npages)
> >  		return 0;
> >  
> > -	down_write(&current->mm->mmap_sem);
> > -	locked = current->mm->locked_vm + npages;
> > +	down_write(&mm->mmap_sem);
> > +	locked = mm->locked_vm + npages;
> >  	lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> >  	if (locked > lock_limit && !capable(CAP_IPC_LOCK))
> 
> 
> 
> Oh boy. Now it seems I have to reference a task, not just mm (which I may
> not have to reference at all after all as the task reference should keep mm
> alive) as I missed the fact capable() and rlimit() are working with
> @current.

Sorry, what?  I'm not seeing how a task reference comes into this.

> 
> 
> Alex,
> 
> Is there anything else I should fix before posting v6? Thanks.
> 
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH kernel v5 4/6] vfio/spapr: Postpone default window creation

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 6778 bytes --]

On Fri, Nov 11, 2016 at 11:32:15PM +1100, Alexey Kardashevskiy wrote:
> As mentioned in the previous patch, we are going to allow the userspace
> to configure container in one memory context and pass container fd to
> another so we are postponing memory allocations accounted against
> the locked memory limit. The previous patch took care of it_userspace.
> 
> At the moment we create the default DMA window when the first group is
> attached to a container; this is done for the userspace which is not
> DDW-aware but familiar with the SPAPR TCE IOMMU v2 in the part of memory
> pre-registration - such client expects the default DMA window to exist.
> 
> This postpones the default DMA window allocation till first map/unmap
> request happens.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  drivers/vfio/vfio_iommu_spapr_tce.c | 98 ++++++++++++++++++-------------------
>  1 file changed, 47 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> index 442baac..1c02498 100644
> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> @@ -97,6 +97,7 @@ struct tce_container {
>  	struct mutex lock;
>  	bool enabled;
>  	bool v2;
> +	bool def_window_pending;
>  	unsigned long locked_pages;
>  	struct iommu_table *tables[IOMMU_TABLE_GROUP_MAX_TABLES];
>  	struct list_head group_list;
> @@ -594,15 +595,6 @@ static long tce_iommu_create_table(struct tce_container *container,
>  	WARN_ON(!ret && !(*ptbl)->it_ops->free);
>  	WARN_ON(!ret && ((*ptbl)->it_allocated_size != table_size));
>  
> -	if (!ret && container->v2) {
> -		ret = tce_iommu_userspace_view_alloc(*ptbl);
> -		if (ret)
> -			(*ptbl)->it_ops->free(*ptbl);
> -	}

Does this stuff for the user view belong in the previous patch?

> -
> -	if (ret)
> -		decrement_locked_vm(table_size >> PAGE_SHIFT);
> -
>  	return ret;
>  }
>  
> @@ -719,6 +711,29 @@ static long tce_iommu_remove_window(struct tce_container *container,
>  	return 0;
>  }
>  
> +static long tce_iommu_create_default_window(struct tce_container *container)
> +{
> +	long ret;
> +	__u64 start_addr = 0;
> +	struct tce_iommu_group *tcegrp;
> +	struct iommu_table_group *table_group;
> +
> +	if (!tce_groups_attached(container))
> +		return -ENODEV;
> +
> +	tcegrp = list_first_entry(&container->group_list,
> +			struct tce_iommu_group, next);
> +	table_group = iommu_group_get_iommudata(tcegrp->grp);
> +	if (!table_group)
> +		return -ENODEV;
> +
> +	ret = tce_iommu_create_window(container, IOMMU_PAGE_SHIFT_4K,
> +			table_group->tce32_size, 1, &start_addr);
> +	WARN_ON_ONCE(!ret && start_addr);
> +
> +	return ret;
> +}
> +
>  static long tce_iommu_ioctl(void *iommu_data,
>  				 unsigned int cmd, unsigned long arg)
>  {
> @@ -809,6 +824,13 @@ static long tce_iommu_ioctl(void *iommu_data,
>  				VFIO_DMA_MAP_FLAG_WRITE))
>  			return -EINVAL;
>  
> +		if (container->def_window_pending) {
> +			ret = tce_iommu_create_default_window(container);
> +			if (ret)
> +				return ret;
> +			container->def_window_pending = false;

Would it make sense to clear (and maybe test) def_window_pending
within create_default_window()?

> +		}
> +
>  		num = tce_iommu_find_table(container, param.iova, &tbl);
>  		if (num < 0)
>  			return -ENXIO;
> @@ -872,6 +894,13 @@ static long tce_iommu_ioctl(void *iommu_data,
>  		if (param.flags)
>  			return -EINVAL;
>  
> +		if (container->def_window_pending) {
> +			ret = tce_iommu_create_default_window(container);
> +			if (ret)
> +				return ret;
> +			container->def_window_pending = false;
> +		}
> +
>  		num = tce_iommu_find_table(container, param.iova, &tbl);
>  		if (num < 0)
>  			return -ENXIO;
> @@ -998,6 +1027,8 @@ static long tce_iommu_ioctl(void *iommu_data,
>  
>  		mutex_lock(&container->lock);
>  
> +		container->def_window_pending = false;

Uh.. why is it cleared here, without calling
tce_iommu_create_default_window() AFAICT?

>  		ret = tce_iommu_create_window(container, create.page_shift,
>  				create.window_size, create.levels,
>  				&create.start_addr);
> @@ -1030,6 +1061,12 @@ static long tce_iommu_ioctl(void *iommu_data,
>  		if (remove.flags)
>  			return -EINVAL;
>  
> +		if (container->def_window_pending && !remove.start_addr) {
> +			container->def_window_pending = false;
> +			return 0;
> +		}
> +		container->def_window_pending = false;
> +
>  		mutex_lock(&container->lock);
>  
>  		ret = tce_iommu_remove_window(container, remove.start_addr);
> @@ -1109,9 +1146,6 @@ static void tce_iommu_release_ownership_ddw(struct tce_container *container,
>  static long tce_iommu_take_ownership_ddw(struct tce_container *container,
>  		struct iommu_table_group *table_group)
>  {
> -	long i, ret = 0;
> -	struct iommu_table *tbl = NULL;
> -
>  	if (!table_group->ops->create_table || !table_group->ops->set_window ||
>  			!table_group->ops->release_ownership) {
>  		WARN_ON_ONCE(1);
> @@ -1120,47 +1154,9 @@ static long tce_iommu_take_ownership_ddw(struct tce_container *container,
>  
>  	table_group->ops->take_ownership(table_group);
>  
> -	/*
> -	 * If it the first group attached, check if there is
> -	 * a default DMA window and create one if none as
> -	 * the userspace expects it to exist.
> -	 */
> -	if (!tce_groups_attached(container) && !container->tables[0]) {
> -		ret = tce_iommu_create_table(container,
> -				table_group,
> -				0, /* window number */
> -				IOMMU_PAGE_SHIFT_4K,
> -				table_group->tce32_size,
> -				1, /* default levels */
> -				&tbl);
> -		if (ret)
> -			goto release_exit;
> -		else
> -			container->tables[0] = tbl;
> -	}
> -
> -	/* Set all windows to the new group */
> -	for (i = 0; i < IOMMU_TABLE_GROUP_MAX_TABLES; ++i) {
> -		tbl = container->tables[i];
> -
> -		if (!tbl)
> -			continue;
> -
> -		/* Set the default window to a new group */
> -		ret = table_group->ops->set_window(table_group, i, tbl);

Uh... nothing in the new code seems to replace these set_window (and
unset_window) calls.  What's up with that?

> -		if (ret)
> -			goto release_exit;
> -	}
> +	container->def_window_pending = true;
>  
>  	return 0;
> -
> -release_exit:
> -	for (i = 0; i < IOMMU_TABLE_GROUP_MAX_TABLES; ++i)
> -		table_group->ops->unset_window(table_group, i);
> -
> -	table_group->ops->release_ownership(table_group);
> -
> -	return ret;
>  }
>  
>  static int tce_iommu_attach_group(void *iommu_data,

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH kernel v5 5/6] vfio/spapr: Reference mm in tce_container

[Alexey Kardashevskiy]

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

On 22/11/16 13:38, David Gibson wrote:
> On Thu, Nov 17, 2016 at 06:39:41PM +1100, Alexey Kardashevskiy wrote:
>> On 11/11/16 23:32, Alexey Kardashevskiy wrote:
>>> In some situations the userspace memory context may live longer than
>>> the userspace process itself so if we need to do proper memory context
>>> cleanup, we better have tce_container take a reference to mm_struct and
>>> use it later when the process is gone (@current or @current->mm is NULL).
>>>
>>> This references mm and stores the pointer in the container; this is done
>>> in a new helper - tce_iommu_mm_set() - when one of the following happens:
>>> - a container is enabled (IOMMU v1);
>>> - a first attempt to pre-register memory is made (IOMMU v2);
>>> - a DMA window is created (IOMMU v2).
>>> The @mm stays referenced till the container is destroyed.
>>>
>>> This replaces current->mm with container->mm everywhere except debug
>>> prints.
>>>
>>> This adds a check that current->mm is the same as the one stored in
>>> the container to prevent userspace from making changes to a memory
>>> context of other processes.
>>>
>>> DMA map/unmap ioctls() do not check for @mm as they already check
>>> for @enabled which is set after tce_iommu_mm_set() is called.
>>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>> Changes:
>>> v5:
>>> * postpone referencing of mm
>>>
>>> v4:
>>> * added check for container->mm!=current->mm in tce_iommu_ioctl()
>>> for all ioctls and removed other redundand checks
>>> ---
>>>  drivers/vfio/vfio_iommu_spapr_tce.c | 159 ++++++++++++++++++++++--------------
>>>  1 file changed, 99 insertions(+), 60 deletions(-)
>>>
>>> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
>>> index 1c02498..9a81a7e 100644
>>> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
>>> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
>>> @@ -31,49 +31,49 @@
>>>  static void tce_iommu_detach_group(void *iommu_data,
>>>  		struct iommu_group *iommu_group);
>>>  
>>> -static long try_increment_locked_vm(long npages)
>>> +static long try_increment_locked_vm(struct mm_struct *mm, long npages)
>>>  {
>>>  	long ret = 0, locked, lock_limit;
>>>  
>>> -	if (!current || !current->mm)
>>> -		return -ESRCH; /* process exited */
>>> +	if (!mm)
>>> +		return -EPERM;
>>>  
>>>  	if (!npages)
>>>  		return 0;
>>>  
>>> -	down_write(&current->mm->mmap_sem);
>>> -	locked = current->mm->locked_vm + npages;
>>> +	down_write(&mm->mmap_sem);
>>> +	locked = mm->locked_vm + npages;
>>>  	lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
>>>  	if (locked > lock_limit && !capable(CAP_IPC_LOCK))
>>
>>
>>
>> Oh boy. Now it seems I have to reference a task, not just mm (which I may
>> not have to reference at all after all as the task reference should keep mm
>> alive) as I missed the fact capable() and rlimit() are working with
>> @current.
> 
> Sorry, what?  I'm not seeing how a task reference comes into this.

I reference @mm to make sure that just one mm uses a container. If mm
changes, I return an error, sanity check.

The code also increments locked_vm in mm. But it looks at the current task
if there is room for increments and for CAP_IPC_LOCK.

So, the options are:
1. I do not reference the current task, and if mm changes, then the mm
sanity check won't let me proceed to the code which tries using current OR
2. reference a task when I reference mm and do that sanity check not just
for mm but also for current task.

Makes sense?


> 
>>
>>
>> Alex,
>>
>> Is there anything else I should fix before posting v6? Thanks.
>>
>>
>>
> 


-- 
Alexey


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 839 bytes --]

Re: [PATCH kernel v5 4/6] vfio/spapr: Postpone default window creation

[Alexey Kardashevskiy]

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

On 22/11/16 13:50, David Gibson wrote:
> On Fri, Nov 11, 2016 at 11:32:15PM +1100, Alexey Kardashevskiy wrote:
>> As mentioned in the previous patch, we are going to allow the userspace
>> to configure container in one memory context and pass container fd to
>> another so we are postponing memory allocations accounted against
>> the locked memory limit. The previous patch took care of it_userspace.
>>
>> At the moment we create the default DMA window when the first group is
>> attached to a container; this is done for the userspace which is not
>> DDW-aware but familiar with the SPAPR TCE IOMMU v2 in the part of memory
>> pre-registration - such client expects the default DMA window to exist.
>>
>> This postpones the default DMA window allocation till first map/unmap
>> request happens.
>>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>>  drivers/vfio/vfio_iommu_spapr_tce.c | 98 ++++++++++++++++++-------------------
>>  1 file changed, 47 insertions(+), 51 deletions(-)
>>
>> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
>> index 442baac..1c02498 100644
>> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
>> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
>> @@ -97,6 +97,7 @@ struct tce_container {
>>  	struct mutex lock;
>>  	bool enabled;
>>  	bool v2;
>> +	bool def_window_pending;
>>  	unsigned long locked_pages;
>>  	struct iommu_table *tables[IOMMU_TABLE_GROUP_MAX_TABLES];
>>  	struct list_head group_list;
>> @@ -594,15 +595,6 @@ static long tce_iommu_create_table(struct tce_container *container,
>>  	WARN_ON(!ret && !(*ptbl)->it_ops->free);
>>  	WARN_ON(!ret && ((*ptbl)->it_allocated_size != table_size));
>>  
>> -	if (!ret && container->v2) {
>> -		ret = tce_iommu_userspace_view_alloc(*ptbl);
>> -		if (ret)
>> -			(*ptbl)->it_ops->free(*ptbl);
>> -	}
> 
> Does this stuff for the user view belong in the previous patch?

Yes it does, my mistake, will fix.


> 
>> -
>> -	if (ret)
>> -		decrement_locked_vm(table_size >> PAGE_SHIFT);
>> -
>>  	return ret;
>>  }
>>  
>> @@ -719,6 +711,29 @@ static long tce_iommu_remove_window(struct tce_container *container,
>>  	return 0;
>>  }
>>  
>> +static long tce_iommu_create_default_window(struct tce_container *container)
>> +{
>> +	long ret;
>> +	__u64 start_addr = 0;
>> +	struct tce_iommu_group *tcegrp;
>> +	struct iommu_table_group *table_group;
>> +
>> +	if (!tce_groups_attached(container))
>> +		return -ENODEV;
>> +
>> +	tcegrp = list_first_entry(&container->group_list,
>> +			struct tce_iommu_group, next);
>> +	table_group = iommu_group_get_iommudata(tcegrp->grp);
>> +	if (!table_group)
>> +		return -ENODEV;
>> +
>> +	ret = tce_iommu_create_window(container, IOMMU_PAGE_SHIFT_4K,
>> +			table_group->tce32_size, 1, &start_addr);
>> +	WARN_ON_ONCE(!ret && start_addr);
>> +
>> +	return ret;
>> +}
>> +
>>  static long tce_iommu_ioctl(void *iommu_data,
>>  				 unsigned int cmd, unsigned long arg)
>>  {
>> @@ -809,6 +824,13 @@ static long tce_iommu_ioctl(void *iommu_data,
>>  				VFIO_DMA_MAP_FLAG_WRITE))
>>  			return -EINVAL;
>>  
>> +		if (container->def_window_pending) {
>> +			ret = tce_iommu_create_default_window(container);
>> +			if (ret)
>> +				return ret;
>> +			container->def_window_pending = false;
> 
> Would it make sense to clear (and maybe test) def_window_pending
> within create_default_window()?

Dunno, matter of taste I suppose. I'll move it there.


> 
>> +		}
>> +
>>  		num = tce_iommu_find_table(container, param.iova, &tbl);
>>  		if (num < 0)
>>  			return -ENXIO;
>> @@ -872,6 +894,13 @@ static long tce_iommu_ioctl(void *iommu_data,
>>  		if (param.flags)
>>  			return -EINVAL;
>>  
>> +		if (container->def_window_pending) {
>> +			ret = tce_iommu_create_default_window(container);
>> +			if (ret)
>> +				return ret;
>> +			container->def_window_pending = false;
>> +		}
>> +
>>  		num = tce_iommu_find_table(container, param.iova, &tbl);
>>  		if (num < 0)
>>  			return -ENXIO;
>> @@ -998,6 +1027,8 @@ static long tce_iommu_ioctl(void *iommu_data,
>>  
>>  		mutex_lock(&container->lock);
>>  
>> +		container->def_window_pending = false;
> 
> Uh.. why is it cleared here, without calling
> tce_iommu_create_default_window() AFAICT?


It is a branch which creates new window, if we do not have a default
window, then it will be created as the result of this ioctl(), if there is
a default window, then the flag should be false already.




> 
>>  		ret = tce_iommu_create_window(container, create.page_shift,
>>  				create.window_size, create.levels,
>>  				&create.start_addr);
>> @@ -1030,6 +1061,12 @@ static long tce_iommu_ioctl(void *iommu_data,
>>  		if (remove.flags)
>>  			return -EINVAL;
>>  
>> +		if (container->def_window_pending && !remove.start_addr) {
>> +			container->def_window_pending = false;
>> +			return 0;
>> +		}
>> +		container->def_window_pending = false;
>> +
>>  		mutex_lock(&container->lock);
>>  
>>  		ret = tce_iommu_remove_window(container, remove.start_addr);
>> @@ -1109,9 +1146,6 @@ static void tce_iommu_release_ownership_ddw(struct tce_container *container,
>>  static long tce_iommu_take_ownership_ddw(struct tce_container *container,
>>  		struct iommu_table_group *table_group)
>>  {
>> -	long i, ret = 0;
>> -	struct iommu_table *tbl = NULL;
>> -
>>  	if (!table_group->ops->create_table || !table_group->ops->set_window ||
>>  			!table_group->ops->release_ownership) {
>>  		WARN_ON_ONCE(1);
>> @@ -1120,47 +1154,9 @@ static long tce_iommu_take_ownership_ddw(struct tce_container *container,
>>  
>>  	table_group->ops->take_ownership(table_group);
>>  
>> -	/*
>> -	 * If it the first group attached, check if there is
>> -	 * a default DMA window and create one if none as
>> -	 * the userspace expects it to exist.
>> -	 */
>> -	if (!tce_groups_attached(container) && !container->tables[0]) {
>> -		ret = tce_iommu_create_table(container,
>> -				table_group,
>> -				0, /* window number */
>> -				IOMMU_PAGE_SHIFT_4K,
>> -				table_group->tce32_size,
>> -				1, /* default levels */
>> -				&tbl);
>> -		if (ret)
>> -			goto release_exit;
>> -		else
>> -			container->tables[0] = tbl;
>> -	}
>> -
>> -	/* Set all windows to the new group */
>> -	for (i = 0; i < IOMMU_TABLE_GROUP_MAX_TABLES; ++i) {
>> -		tbl = container->tables[i];
>> -
>> -		if (!tbl)
>> -			continue;
>> -
>> -		/* Set the default window to a new group */
>> -		ret = table_group->ops->set_window(table_group, i, tbl);
> 
> Uh... nothing in the new code seems to replace these set_window (and
> unset_window) calls.  What's up with that?


tce_iommu_create_table() + set_window() is replaced with
tce_iommu_create_default_window() which calls tce_iommu_create_window()
which calls tce_iommu_create_table() + set_window().

I'll split this patch into 2 patches, one with the change I just explained
and one to postpone the default window creation.


> 
>> -		if (ret)
>> -			goto release_exit;
>> -	}
>> +	container->def_window_pending = true;
>>  
>>  	return 0;
>> -
>> -release_exit:
>> -	for (i = 0; i < IOMMU_TABLE_GROUP_MAX_TABLES; ++i)
>> -		table_group->ops->unset_window(table_group, i);
>> -
>> -	table_group->ops->release_ownership(table_group);
>> -
>> -	return ret;
>>  }
>>  
>>  static int tce_iommu_attach_group(void *iommu_data,
> 


-- 
Alexey


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 839 bytes --]

Re: [PATCH kernel v5 5/6] vfio/spapr: Reference mm in tce_container

[Alexey Kardashevskiy]

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

On 22/11/16 14:49, Alexey Kardashevskiy wrote:
> On 22/11/16 13:38, David Gibson wrote:
>> On Thu, Nov 17, 2016 at 06:39:41PM +1100, Alexey Kardashevskiy wrote:
>>> On 11/11/16 23:32, Alexey Kardashevskiy wrote:
>>>> In some situations the userspace memory context may live longer than
>>>> the userspace process itself so if we need to do proper memory context
>>>> cleanup, we better have tce_container take a reference to mm_struct and
>>>> use it later when the process is gone (@current or @current->mm is NULL).
>>>>
>>>> This references mm and stores the pointer in the container; this is done
>>>> in a new helper - tce_iommu_mm_set() - when one of the following happens:
>>>> - a container is enabled (IOMMU v1);
>>>> - a first attempt to pre-register memory is made (IOMMU v2);
>>>> - a DMA window is created (IOMMU v2).
>>>> The @mm stays referenced till the container is destroyed.
>>>>
>>>> This replaces current->mm with container->mm everywhere except debug
>>>> prints.
>>>>
>>>> This adds a check that current->mm is the same as the one stored in
>>>> the container to prevent userspace from making changes to a memory
>>>> context of other processes.
>>>>
>>>> DMA map/unmap ioctls() do not check for @mm as they already check
>>>> for @enabled which is set after tce_iommu_mm_set() is called.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>> Changes:
>>>> v5:
>>>> * postpone referencing of mm
>>>>
>>>> v4:
>>>> * added check for container->mm!=current->mm in tce_iommu_ioctl()
>>>> for all ioctls and removed other redundand checks
>>>> ---
>>>>  drivers/vfio/vfio_iommu_spapr_tce.c | 159 ++++++++++++++++++++++--------------
>>>>  1 file changed, 99 insertions(+), 60 deletions(-)
>>>>
>>>> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
>>>> index 1c02498..9a81a7e 100644
>>>> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
>>>> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
>>>> @@ -31,49 +31,49 @@
>>>>  static void tce_iommu_detach_group(void *iommu_data,
>>>>  		struct iommu_group *iommu_group);
>>>>  
>>>> -static long try_increment_locked_vm(long npages)
>>>> +static long try_increment_locked_vm(struct mm_struct *mm, long npages)
>>>>  {
>>>>  	long ret = 0, locked, lock_limit;
>>>>  
>>>> -	if (!current || !current->mm)
>>>> -		return -ESRCH; /* process exited */
>>>> +	if (!mm)
>>>> +		return -EPERM;
>>>>  
>>>>  	if (!npages)
>>>>  		return 0;
>>>>  
>>>> -	down_write(&current->mm->mmap_sem);
>>>> -	locked = current->mm->locked_vm + npages;
>>>> +	down_write(&mm->mmap_sem);
>>>> +	locked = mm->locked_vm + npages;
>>>>  	lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
>>>>  	if (locked > lock_limit && !capable(CAP_IPC_LOCK))
>>>
>>>
>>>
>>> Oh boy. Now it seems I have to reference a task, not just mm (which I may
>>> not have to reference at all after all as the task reference should keep mm
>>> alive) as I missed the fact capable() and rlimit() are working with
>>> @current.
>>
>> Sorry, what?  I'm not seeing how a task reference comes into this.
> 
> I reference @mm to make sure that just one mm uses a container. If mm
> changes, I return an error, sanity check.
> 
> The code also increments locked_vm in mm. But it looks at the current task
> if there is room for increments and for CAP_IPC_LOCK.
> 
> So, the options are:
> 1. I do not reference the current task, and if mm changes, then the mm
> sanity check won't let me proceed to the code which tries using current OR
> 2. reference a task when I reference mm and do that sanity check not just
> for mm but also for current task.
> 
> Makes sense?

I had a chat with Nick and now I think that having mm referenced and
checked should be enough and I do not need to reference the task as
multiple threads within the same mm are allowed to ioctl() to vfio and
supposedly they will have same limits and capabilities and if they do not,
we'll just fail and that's it, I cannot see any harm from this approach,
can you?


> 
> 
>>
>>>
>>>
>>> Alex,
>>>
>>> Is there anything else I should fix before posting v6? Thanks.
>>>
>>>
>>>
>>
> 
> 


-- 
Alexey


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 839 bytes --]

Re: [PATCH kernel v5 4/6] vfio/spapr: Postpone default window creation

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 8647 bytes --]

On Tue, Nov 22, 2016 at 06:29:39PM +1100, Alexey Kardashevskiy wrote:
> On 22/11/16 13:50, David Gibson wrote:
> > On Fri, Nov 11, 2016 at 11:32:15PM +1100, Alexey Kardashevskiy wrote:
> >> As mentioned in the previous patch, we are going to allow the userspace
> >> to configure container in one memory context and pass container fd to
> >> another so we are postponing memory allocations accounted against
> >> the locked memory limit. The previous patch took care of it_userspace.
> >>
> >> At the moment we create the default DMA window when the first group is
> >> attached to a container; this is done for the userspace which is not
> >> DDW-aware but familiar with the SPAPR TCE IOMMU v2 in the part of memory
> >> pre-registration - such client expects the default DMA window to exist.
> >>
> >> This postpones the default DMA window allocation till first map/unmap
> >> request happens.
> >>
> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >> ---
> >>  drivers/vfio/vfio_iommu_spapr_tce.c | 98 ++++++++++++++++++-------------------
> >>  1 file changed, 47 insertions(+), 51 deletions(-)
> >>
> >> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> >> index 442baac..1c02498 100644
> >> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> >> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> >> @@ -97,6 +97,7 @@ struct tce_container {
> >>  	struct mutex lock;
> >>  	bool enabled;
> >>  	bool v2;
> >> +	bool def_window_pending;
> >>  	unsigned long locked_pages;
> >>  	struct iommu_table *tables[IOMMU_TABLE_GROUP_MAX_TABLES];
> >>  	struct list_head group_list;
> >> @@ -594,15 +595,6 @@ static long tce_iommu_create_table(struct tce_container *container,
> >>  	WARN_ON(!ret && !(*ptbl)->it_ops->free);
> >>  	WARN_ON(!ret && ((*ptbl)->it_allocated_size != table_size));
> >>  
> >> -	if (!ret && container->v2) {
> >> -		ret = tce_iommu_userspace_view_alloc(*ptbl);
> >> -		if (ret)
> >> -			(*ptbl)->it_ops->free(*ptbl);
> >> -	}
> > 
> > Does this stuff for the user view belong in the previous patch?
> 
> Yes it does, my mistake, will fix.
> 
> 
> > 
> >> -
> >> -	if (ret)
> >> -		decrement_locked_vm(table_size >> PAGE_SHIFT);
> >> -
> >>  	return ret;
> >>  }
> >>  
> >> @@ -719,6 +711,29 @@ static long tce_iommu_remove_window(struct tce_container *container,
> >>  	return 0;
> >>  }
> >>  
> >> +static long tce_iommu_create_default_window(struct tce_container *container)
> >> +{
> >> +	long ret;
> >> +	__u64 start_addr = 0;
> >> +	struct tce_iommu_group *tcegrp;
> >> +	struct iommu_table_group *table_group;
> >> +
> >> +	if (!tce_groups_attached(container))
> >> +		return -ENODEV;
> >> +
> >> +	tcegrp = list_first_entry(&container->group_list,
> >> +			struct tce_iommu_group, next);
> >> +	table_group = iommu_group_get_iommudata(tcegrp->grp);
> >> +	if (!table_group)
> >> +		return -ENODEV;
> >> +
> >> +	ret = tce_iommu_create_window(container, IOMMU_PAGE_SHIFT_4K,
> >> +			table_group->tce32_size, 1, &start_addr);
> >> +	WARN_ON_ONCE(!ret && start_addr);
> >> +
> >> +	return ret;
> >> +}
> >> +
> >>  static long tce_iommu_ioctl(void *iommu_data,
> >>  				 unsigned int cmd, unsigned long arg)
> >>  {
> >> @@ -809,6 +824,13 @@ static long tce_iommu_ioctl(void *iommu_data,
> >>  				VFIO_DMA_MAP_FLAG_WRITE))
> >>  			return -EINVAL;
> >>  
> >> +		if (container->def_window_pending) {
> >> +			ret = tce_iommu_create_default_window(container);
> >> +			if (ret)
> >> +				return ret;
> >> +			container->def_window_pending = false;
> > 
> > Would it make sense to clear (and maybe test) def_window_pending
> > within create_default_window()?
> 
> Dunno, matter of taste I suppose. I'll move it there.
> 
> 
> > 
> >> +		}
> >> +
> >>  		num = tce_iommu_find_table(container, param.iova, &tbl);
> >>  		if (num < 0)
> >>  			return -ENXIO;
> >> @@ -872,6 +894,13 @@ static long tce_iommu_ioctl(void *iommu_data,
> >>  		if (param.flags)
> >>  			return -EINVAL;
> >>  
> >> +		if (container->def_window_pending) {
> >> +			ret = tce_iommu_create_default_window(container);
> >> +			if (ret)
> >> +				return ret;
> >> +			container->def_window_pending = false;
> >> +		}
> >> +
> >>  		num = tce_iommu_find_table(container, param.iova, &tbl);
> >>  		if (num < 0)
> >>  			return -ENXIO;
> >> @@ -998,6 +1027,8 @@ static long tce_iommu_ioctl(void *iommu_data,
> >>  
> >>  		mutex_lock(&container->lock);
> >>  
> >> +		container->def_window_pending = false;
> > 
> > Uh.. why is it cleared here, without calling
> > tce_iommu_create_default_window() AFAICT?
> 
> 
> It is a branch which creates new window, if we do not have a default
> window, then it will be created as the result of this ioctl(), if there is
> a default window, then the flag should be false already.

Um.. it will create *a* window, but not necessarily the default one.
Consider this scenario:

1. Container is opened
2. A group is attached
3. Userspace, expecting the default window to be in place, creates a
   second window
4. Mapping starts

Won't the above code mean that we create what userspace expected to be
the second window as the first window replacing the default one
instead?

> 
> 
> 
> 
> > 
> >>  		ret = tce_iommu_create_window(container, create.page_shift,
> >>  				create.window_size, create.levels,
> >>  				&create.start_addr);
> >> @@ -1030,6 +1061,12 @@ static long tce_iommu_ioctl(void *iommu_data,
> >>  		if (remove.flags)
> >>  			return -EINVAL;
> >>  
> >> +		if (container->def_window_pending && !remove.start_addr) {
> >> +			container->def_window_pending = false;
> >> +			return 0;
> >> +		}
> >> +		container->def_window_pending = false;
> >> +
> >>  		mutex_lock(&container->lock);
> >>  
> >>  		ret = tce_iommu_remove_window(container, remove.start_addr);
> >> @@ -1109,9 +1146,6 @@ static void tce_iommu_release_ownership_ddw(struct tce_container *container,
> >>  static long tce_iommu_take_ownership_ddw(struct tce_container *container,
> >>  		struct iommu_table_group *table_group)
> >>  {
> >> -	long i, ret = 0;
> >> -	struct iommu_table *tbl = NULL;
> >> -
> >>  	if (!table_group->ops->create_table || !table_group->ops->set_window ||
> >>  			!table_group->ops->release_ownership) {
> >>  		WARN_ON_ONCE(1);
> >> @@ -1120,47 +1154,9 @@ static long tce_iommu_take_ownership_ddw(struct tce_container *container,
> >>  
> >>  	table_group->ops->take_ownership(table_group);
> >>  
> >> -	/*
> >> -	 * If it the first group attached, check if there is
> >> -	 * a default DMA window and create one if none as
> >> -	 * the userspace expects it to exist.
> >> -	 */
> >> -	if (!tce_groups_attached(container) && !container->tables[0]) {
> >> -		ret = tce_iommu_create_table(container,
> >> -				table_group,
> >> -				0, /* window number */
> >> -				IOMMU_PAGE_SHIFT_4K,
> >> -				table_group->tce32_size,
> >> -				1, /* default levels */
> >> -				&tbl);
> >> -		if (ret)
> >> -			goto release_exit;
> >> -		else
> >> -			container->tables[0] = tbl;
> >> -	}
> >> -
> >> -	/* Set all windows to the new group */
> >> -	for (i = 0; i < IOMMU_TABLE_GROUP_MAX_TABLES; ++i) {
> >> -		tbl = container->tables[i];
> >> -
> >> -		if (!tbl)
> >> -			continue;
> >> -
> >> -		/* Set the default window to a new group */
> >> -		ret = table_group->ops->set_window(table_group, i, tbl);
> > 
> > Uh... nothing in the new code seems to replace these set_window (and
> > unset_window) calls.  What's up with that?
> 
> 
> tce_iommu_create_table() + set_window() is replaced with
> tce_iommu_create_default_window() which calls tce_iommu_create_window()
> which calls tce_iommu_create_table() + set_window().
> 
> I'll split this patch into 2 patches, one with the change I just explained
> and one to postpone the default window creation.

Ah, right, I see.


> 
> 
> > 
> >> -		if (ret)
> >> -			goto release_exit;
> >> -	}
> >> +	container->def_window_pending = true;
> >>  
> >>  	return 0;
> >> -
> >> -release_exit:
> >> -	for (i = 0; i < IOMMU_TABLE_GROUP_MAX_TABLES; ++i)
> >> -		table_group->ops->unset_window(table_group, i);
> >> -
> >> -	table_group->ops->release_ownership(table_group);
> >> -
> >> -	return ret;
> >>  }
> >>  
> >>  static int tce_iommu_attach_group(void *iommu_data,
> > 
> 
> 




-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH kernel v5 5/6] vfio/spapr: Reference mm in tce_container

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4777 bytes --]

On Tue, Nov 22, 2016 at 06:34:25PM +1100, Alexey Kardashevskiy wrote:
> On 22/11/16 14:49, Alexey Kardashevskiy wrote:
> > On 22/11/16 13:38, David Gibson wrote:
> >> On Thu, Nov 17, 2016 at 06:39:41PM +1100, Alexey Kardashevskiy wrote:
> >>> On 11/11/16 23:32, Alexey Kardashevskiy wrote:
> >>>> In some situations the userspace memory context may live longer than
> >>>> the userspace process itself so if we need to do proper memory context
> >>>> cleanup, we better have tce_container take a reference to mm_struct and
> >>>> use it later when the process is gone (@current or @current->mm is NULL).
> >>>>
> >>>> This references mm and stores the pointer in the container; this is done
> >>>> in a new helper - tce_iommu_mm_set() - when one of the following happens:
> >>>> - a container is enabled (IOMMU v1);
> >>>> - a first attempt to pre-register memory is made (IOMMU v2);
> >>>> - a DMA window is created (IOMMU v2).
> >>>> The @mm stays referenced till the container is destroyed.
> >>>>
> >>>> This replaces current->mm with container->mm everywhere except debug
> >>>> prints.
> >>>>
> >>>> This adds a check that current->mm is the same as the one stored in
> >>>> the container to prevent userspace from making changes to a memory
> >>>> context of other processes.
> >>>>
> >>>> DMA map/unmap ioctls() do not check for @mm as they already check
> >>>> for @enabled which is set after tce_iommu_mm_set() is called.
> >>>>
> >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>> ---
> >>>> Changes:
> >>>> v5:
> >>>> * postpone referencing of mm
> >>>>
> >>>> v4:
> >>>> * added check for container->mm!=current->mm in tce_iommu_ioctl()
> >>>> for all ioctls and removed other redundand checks
> >>>> ---
> >>>>  drivers/vfio/vfio_iommu_spapr_tce.c | 159 ++++++++++++++++++++++--------------
> >>>>  1 file changed, 99 insertions(+), 60 deletions(-)
> >>>>
> >>>> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> >>>> index 1c02498..9a81a7e 100644
> >>>> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> >>>> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> >>>> @@ -31,49 +31,49 @@
> >>>>  static void tce_iommu_detach_group(void *iommu_data,
> >>>>  		struct iommu_group *iommu_group);
> >>>>  
> >>>> -static long try_increment_locked_vm(long npages)
> >>>> +static long try_increment_locked_vm(struct mm_struct *mm, long npages)
> >>>>  {
> >>>>  	long ret = 0, locked, lock_limit;
> >>>>  
> >>>> -	if (!current || !current->mm)
> >>>> -		return -ESRCH; /* process exited */
> >>>> +	if (!mm)
> >>>> +		return -EPERM;
> >>>>  
> >>>>  	if (!npages)
> >>>>  		return 0;
> >>>>  
> >>>> -	down_write(&current->mm->mmap_sem);
> >>>> -	locked = current->mm->locked_vm + npages;
> >>>> +	down_write(&mm->mmap_sem);
> >>>> +	locked = mm->locked_vm + npages;
> >>>>  	lock_limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> >>>>  	if (locked > lock_limit && !capable(CAP_IPC_LOCK))
> >>>
> >>>
> >>>
> >>> Oh boy. Now it seems I have to reference a task, not just mm (which I may
> >>> not have to reference at all after all as the task reference should keep mm
> >>> alive) as I missed the fact capable() and rlimit() are working with
> >>> @current.
> >>
> >> Sorry, what?  I'm not seeing how a task reference comes into this.
> > 
> > I reference @mm to make sure that just one mm uses a container. If mm
> > changes, I return an error, sanity check.
> > 
> > The code also increments locked_vm in mm. But it looks at the current task
> > if there is room for increments and for CAP_IPC_LOCK.
> > 
> > So, the options are:
> > 1. I do not reference the current task, and if mm changes, then the mm
> > sanity check won't let me proceed to the code which tries using current OR
> > 2. reference a task when I reference mm and do that sanity check not just
> > for mm but also for current task.
> > 
> > Makes sense?
> 
> I had a chat with Nick and now I think that having mm referenced and
> checked should be enough and I do not need to reference the task as
> multiple threads within the same mm are allowed to ioctl() to vfio and
> supposedly they will have same limits and capabilities and if they do not,
> we'll just fail and that's it, I cannot see any harm from this approach,
> can you?

Yeah, that sounds sane to me.  If the different threads in the mm
somehow have different caps /  limits, we could get some weird results
depending on which thread attempts to do the mapping, but it shouldn't
actually be harmful.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH kernel v5 4/6] vfio/spapr: Postpone default window creation

[Alexey Kardashevskiy]

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

On 23/11/16 12:35, David Gibson wrote:
> On Tue, Nov 22, 2016 at 06:29:39PM +1100, Alexey Kardashevskiy wrote:
>> On 22/11/16 13:50, David Gibson wrote:
>>> On Fri, Nov 11, 2016 at 11:32:15PM +1100, Alexey Kardashevskiy wrote:
>>>> As mentioned in the previous patch, we are going to allow the userspace
>>>> to configure container in one memory context and pass container fd to
>>>> another so we are postponing memory allocations accounted against
>>>> the locked memory limit. The previous patch took care of it_userspace.
>>>>
>>>> At the moment we create the default DMA window when the first group is
>>>> attached to a container; this is done for the userspace which is not
>>>> DDW-aware but familiar with the SPAPR TCE IOMMU v2 in the part of memory
>>>> pre-registration - such client expects the default DMA window to exist.
>>>>
>>>> This postpones the default DMA window allocation till first map/unmap
>>>> request happens.
>>>>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>>  drivers/vfio/vfio_iommu_spapr_tce.c | 98 ++++++++++++++++++-------------------
>>>>  1 file changed, 47 insertions(+), 51 deletions(-)
>>>>
>>>> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
>>>> index 442baac..1c02498 100644
>>>> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
>>>> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
>>>> @@ -97,6 +97,7 @@ struct tce_container {
>>>>  	struct mutex lock;
>>>>  	bool enabled;
>>>>  	bool v2;
>>>> +	bool def_window_pending;
>>>>  	unsigned long locked_pages;
>>>>  	struct iommu_table *tables[IOMMU_TABLE_GROUP_MAX_TABLES];
>>>>  	struct list_head group_list;
>>>> @@ -594,15 +595,6 @@ static long tce_iommu_create_table(struct tce_container *container,
>>>>  	WARN_ON(!ret && !(*ptbl)->it_ops->free);
>>>>  	WARN_ON(!ret && ((*ptbl)->it_allocated_size != table_size));
>>>>  
>>>> -	if (!ret && container->v2) {
>>>> -		ret = tce_iommu_userspace_view_alloc(*ptbl);
>>>> -		if (ret)
>>>> -			(*ptbl)->it_ops->free(*ptbl);
>>>> -	}
>>>
>>> Does this stuff for the user view belong in the previous patch?
>>
>> Yes it does, my mistake, will fix.
>>
>>
>>>
>>>> -
>>>> -	if (ret)
>>>> -		decrement_locked_vm(table_size >> PAGE_SHIFT);
>>>> -
>>>>  	return ret;
>>>>  }
>>>>  
>>>> @@ -719,6 +711,29 @@ static long tce_iommu_remove_window(struct tce_container *container,
>>>>  	return 0;
>>>>  }
>>>>  
>>>> +static long tce_iommu_create_default_window(struct tce_container *container)
>>>> +{
>>>> +	long ret;
>>>> +	__u64 start_addr = 0;
>>>> +	struct tce_iommu_group *tcegrp;
>>>> +	struct iommu_table_group *table_group;
>>>> +
>>>> +	if (!tce_groups_attached(container))
>>>> +		return -ENODEV;
>>>> +
>>>> +	tcegrp = list_first_entry(&container->group_list,
>>>> +			struct tce_iommu_group, next);
>>>> +	table_group = iommu_group_get_iommudata(tcegrp->grp);
>>>> +	if (!table_group)
>>>> +		return -ENODEV;
>>>> +
>>>> +	ret = tce_iommu_create_window(container, IOMMU_PAGE_SHIFT_4K,
>>>> +			table_group->tce32_size, 1, &start_addr);
>>>> +	WARN_ON_ONCE(!ret && start_addr);
>>>> +
>>>> +	return ret;
>>>> +}
>>>> +
>>>>  static long tce_iommu_ioctl(void *iommu_data,
>>>>  				 unsigned int cmd, unsigned long arg)
>>>>  {
>>>> @@ -809,6 +824,13 @@ static long tce_iommu_ioctl(void *iommu_data,
>>>>  				VFIO_DMA_MAP_FLAG_WRITE))
>>>>  			return -EINVAL;
>>>>  
>>>> +		if (container->def_window_pending) {
>>>> +			ret = tce_iommu_create_default_window(container);
>>>> +			if (ret)
>>>> +				return ret;
>>>> +			container->def_window_pending = false;
>>>
>>> Would it make sense to clear (and maybe test) def_window_pending
>>> within create_default_window()?
>>
>> Dunno, matter of taste I suppose. I'll move it there.
>>
>>
>>>
>>>> +		}
>>>> +
>>>>  		num = tce_iommu_find_table(container, param.iova, &tbl);
>>>>  		if (num < 0)
>>>>  			return -ENXIO;
>>>> @@ -872,6 +894,13 @@ static long tce_iommu_ioctl(void *iommu_data,
>>>>  		if (param.flags)
>>>>  			return -EINVAL;
>>>>  
>>>> +		if (container->def_window_pending) {
>>>> +			ret = tce_iommu_create_default_window(container);
>>>> +			if (ret)
>>>> +				return ret;
>>>> +			container->def_window_pending = false;
>>>> +		}
>>>> +
>>>>  		num = tce_iommu_find_table(container, param.iova, &tbl);
>>>>  		if (num < 0)
>>>>  			return -ENXIO;
>>>> @@ -998,6 +1027,8 @@ static long tce_iommu_ioctl(void *iommu_data,
>>>>  
>>>>  		mutex_lock(&container->lock);
>>>>  
>>>> +		container->def_window_pending = false;
>>>
>>> Uh.. why is it cleared here, without calling
>>> tce_iommu_create_default_window() AFAICT?
>>
>>
>> It is a branch which creates new window, if we do not have a default
>> window, then it will be created as the result of this ioctl(), if there is
>> a default window, then the flag should be false already.
> 
> Um.. it will create *a* window, but not necessarily the default one.
> Consider this scenario:
> 
> 1. Container is opened
> 2. A group is attached
> 3. Userspace, expecting the default window to be in place, creates a
>    second window
> 4. Mapping starts
> 
> Won't the above code mean that we create what userspace expected to be
> the second window as the first window replacing the default one
> instead?

Uff. Yes, this is correct.

This is a bug in my initial design - I should not have created a "default"
window in the first place - it does not make sense at this level; and the
userspace (i.e. QEMU) removes all windows on reset anyway.

The only way QEMU will use this default window is when it is at 318f67ce1
"vfio: spapr: Add DMA memory preregistering (SPAPR IOMMU v2)" - memory
preregistration is there, but ddw is not yet but it is 2 patches later
where vfio_connect_container() unconditionally removes the default window.
Which is unlikely scenario.

Taking into account small number of DDW users (basically - QEMU v2.7.0),
does it still make sense to keep this default window code? The only
immediate problem I see here is QEMU will receive error from
vfio_spapr_remove_window() in vfio_connect_container() as there will be no
default window but I could workaround this.



> 
>>
>>
>>
>>
>>>
>>>>  		ret = tce_iommu_create_window(container, create.page_shift,
>>>>  				create.window_size, create.levels,
>>>>  				&create.start_addr);
>>>> @@ -1030,6 +1061,12 @@ static long tce_iommu_ioctl(void *iommu_data,
>>>>  		if (remove.flags)
>>>>  			return -EINVAL;
>>>>  
>>>> +		if (container->def_window_pending && !remove.start_addr) {
>>>> +			container->def_window_pending = false;
>>>> +			return 0;
>>>> +		}
>>>> +		container->def_window_pending = false;
>>>> +
>>>>  		mutex_lock(&container->lock);
>>>>  
>>>>  		ret = tce_iommu_remove_window(container, remove.start_addr);
>>>> @@ -1109,9 +1146,6 @@ static void tce_iommu_release_ownership_ddw(struct tce_container *container,
>>>>  static long tce_iommu_take_ownership_ddw(struct tce_container *container,
>>>>  		struct iommu_table_group *table_group)
>>>>  {
>>>> -	long i, ret = 0;
>>>> -	struct iommu_table *tbl = NULL;
>>>> -
>>>>  	if (!table_group->ops->create_table || !table_group->ops->set_window ||
>>>>  			!table_group->ops->release_ownership) {
>>>>  		WARN_ON_ONCE(1);
>>>> @@ -1120,47 +1154,9 @@ static long tce_iommu_take_ownership_ddw(struct tce_container *container,
>>>>  
>>>>  	table_group->ops->take_ownership(table_group);
>>>>  
>>>> -	/*
>>>> -	 * If it the first group attached, check if there is
>>>> -	 * a default DMA window and create one if none as
>>>> -	 * the userspace expects it to exist.
>>>> -	 */
>>>> -	if (!tce_groups_attached(container) && !container->tables[0]) {
>>>> -		ret = tce_iommu_create_table(container,
>>>> -				table_group,
>>>> -				0, /* window number */
>>>> -				IOMMU_PAGE_SHIFT_4K,
>>>> -				table_group->tce32_size,
>>>> -				1, /* default levels */
>>>> -				&tbl);
>>>> -		if (ret)
>>>> -			goto release_exit;
>>>> -		else
>>>> -			container->tables[0] = tbl;
>>>> -	}
>>>> -
>>>> -	/* Set all windows to the new group */
>>>> -	for (i = 0; i < IOMMU_TABLE_GROUP_MAX_TABLES; ++i) {
>>>> -		tbl = container->tables[i];
>>>> -
>>>> -		if (!tbl)
>>>> -			continue;
>>>> -
>>>> -		/* Set the default window to a new group */
>>>> -		ret = table_group->ops->set_window(table_group, i, tbl);
>>>
>>> Uh... nothing in the new code seems to replace these set_window (and
>>> unset_window) calls.  What's up with that?
>>
>>
>> tce_iommu_create_table() + set_window() is replaced with
>> tce_iommu_create_default_window() which calls tce_iommu_create_window()
>> which calls tce_iommu_create_table() + set_window().
>>
>> I'll split this patch into 2 patches, one with the change I just explained
>> and one to postpone the default window creation.
> 
> Ah, right, I see.
> 
> 
>>
>>
>>>
>>>> -		if (ret)
>>>> -			goto release_exit;
>>>> -	}
>>>> +	container->def_window_pending = true;
>>>>  
>>>>  	return 0;
>>>> -
>>>> -release_exit:
>>>> -	for (i = 0; i < IOMMU_TABLE_GROUP_MAX_TABLES; ++i)
>>>> -		table_group->ops->unset_window(table_group, i);
>>>> -
>>>> -	table_group->ops->release_ownership(table_group);
>>>> -
>>>> -	return ret;
>>>>  }
>>>>  
>>>>  static int tce_iommu_attach_group(void *iommu_data,
>>>
>>
>>
> 
> 
> 
> 


-- 
Alexey


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 839 bytes --]

Re: [PATCH kernel v5 4/6] vfio/spapr: Postpone default window creation

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 7051 bytes --]

On Wed, Nov 23, 2016 at 04:06:30PM +1100, Alexey Kardashevskiy wrote:
> On 23/11/16 12:35, David Gibson wrote:
> > On Tue, Nov 22, 2016 at 06:29:39PM +1100, Alexey Kardashevskiy wrote:
> >> On 22/11/16 13:50, David Gibson wrote:
> >>> On Fri, Nov 11, 2016 at 11:32:15PM +1100, Alexey Kardashevskiy wrote:
> >>>> As mentioned in the previous patch, we are going to allow the userspace
> >>>> to configure container in one memory context and pass container fd to
> >>>> another so we are postponing memory allocations accounted against
> >>>> the locked memory limit. The previous patch took care of it_userspace.
> >>>>
> >>>> At the moment we create the default DMA window when the first group is
> >>>> attached to a container; this is done for the userspace which is not
> >>>> DDW-aware but familiar with the SPAPR TCE IOMMU v2 in the part of memory
> >>>> pre-registration - such client expects the default DMA window to exist.
> >>>>
> >>>> This postpones the default DMA window allocation till first map/unmap
> >>>> request happens.
> >>>>
> >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>> ---
> >>>>  drivers/vfio/vfio_iommu_spapr_tce.c | 98 ++++++++++++++++++-------------------
> >>>>  1 file changed, 47 insertions(+), 51 deletions(-)
> >>>>
> >>>> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> >>>> index 442baac..1c02498 100644
> >>>> --- a/drivers/vfio/vfio_iommu_spapr_tce.c
> >>>> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> >>>> @@ -97,6 +97,7 @@ struct tce_container {
> >>>>  	struct mutex lock;
> >>>>  	bool enabled;
> >>>>  	bool v2;
> >>>> +	bool def_window_pending;
> >>>>  	unsigned long locked_pages;
> >>>>  	struct iommu_table *tables[IOMMU_TABLE_GROUP_MAX_TABLES];
> >>>>  	struct list_head group_list;
> >>>> @@ -594,15 +595,6 @@ static long tce_iommu_create_table(struct tce_container *container,
> >>>>  	WARN_ON(!ret && !(*ptbl)->it_ops->free);
> >>>>  	WARN_ON(!ret && ((*ptbl)->it_allocated_size != table_size));
> >>>>  
> >>>> -	if (!ret && container->v2) {
> >>>> -		ret = tce_iommu_userspace_view_alloc(*ptbl);
> >>>> -		if (ret)
> >>>> -			(*ptbl)->it_ops->free(*ptbl);
> >>>> -	}
> >>>
> >>> Does this stuff for the user view belong in the previous patch?
> >>
> >> Yes it does, my mistake, will fix.
> >>
> >>
> >>>
> >>>> -
> >>>> -	if (ret)
> >>>> -		decrement_locked_vm(table_size >> PAGE_SHIFT);
> >>>> -
> >>>>  	return ret;
> >>>>  }
> >>>>  
> >>>> @@ -719,6 +711,29 @@ static long tce_iommu_remove_window(struct tce_container *container,
> >>>>  	return 0;
> >>>>  }
> >>>>  
> >>>> +static long tce_iommu_create_default_window(struct tce_container *container)
> >>>> +{
> >>>> +	long ret;
> >>>> +	__u64 start_addr = 0;
> >>>> +	struct tce_iommu_group *tcegrp;
> >>>> +	struct iommu_table_group *table_group;
> >>>> +
> >>>> +	if (!tce_groups_attached(container))
> >>>> +		return -ENODEV;
> >>>> +
> >>>> +	tcegrp = list_first_entry(&container->group_list,
> >>>> +			struct tce_iommu_group, next);
> >>>> +	table_group = iommu_group_get_iommudata(tcegrp->grp);
> >>>> +	if (!table_group)
> >>>> +		return -ENODEV;
> >>>> +
> >>>> +	ret = tce_iommu_create_window(container, IOMMU_PAGE_SHIFT_4K,
> >>>> +			table_group->tce32_size, 1, &start_addr);
> >>>> +	WARN_ON_ONCE(!ret && start_addr);
> >>>> +
> >>>> +	return ret;
> >>>> +}
> >>>> +
> >>>>  static long tce_iommu_ioctl(void *iommu_data,
> >>>>  				 unsigned int cmd, unsigned long arg)
> >>>>  {
> >>>> @@ -809,6 +824,13 @@ static long tce_iommu_ioctl(void *iommu_data,
> >>>>  				VFIO_DMA_MAP_FLAG_WRITE))
> >>>>  			return -EINVAL;
> >>>>  
> >>>> +		if (container->def_window_pending) {
> >>>> +			ret = tce_iommu_create_default_window(container);
> >>>> +			if (ret)
> >>>> +				return ret;
> >>>> +			container->def_window_pending = false;
> >>>
> >>> Would it make sense to clear (and maybe test) def_window_pending
> >>> within create_default_window()?
> >>
> >> Dunno, matter of taste I suppose. I'll move it there.
> >>
> >>
> >>>
> >>>> +		}
> >>>> +
> >>>>  		num = tce_iommu_find_table(container, param.iova, &tbl);
> >>>>  		if (num < 0)
> >>>>  			return -ENXIO;
> >>>> @@ -872,6 +894,13 @@ static long tce_iommu_ioctl(void *iommu_data,
> >>>>  		if (param.flags)
> >>>>  			return -EINVAL;
> >>>>  
> >>>> +		if (container->def_window_pending) {
> >>>> +			ret = tce_iommu_create_default_window(container);
> >>>> +			if (ret)
> >>>> +				return ret;
> >>>> +			container->def_window_pending = false;
> >>>> +		}
> >>>> +
> >>>>  		num = tce_iommu_find_table(container, param.iova, &tbl);
> >>>>  		if (num < 0)
> >>>>  			return -ENXIO;
> >>>> @@ -998,6 +1027,8 @@ static long tce_iommu_ioctl(void *iommu_data,
> >>>>  
> >>>>  		mutex_lock(&container->lock);
> >>>>  
> >>>> +		container->def_window_pending = false;
> >>>
> >>> Uh.. why is it cleared here, without calling
> >>> tce_iommu_create_default_window() AFAICT?
> >>
> >>
> >> It is a branch which creates new window, if we do not have a default
> >> window, then it will be created as the result of this ioctl(), if there is
> >> a default window, then the flag should be false already.
> > 
> > Um.. it will create *a* window, but not necessarily the default one.
> > Consider this scenario:
> > 
> > 1. Container is opened
> > 2. A group is attached
> > 3. Userspace, expecting the default window to be in place, creates a
> >    second window
> > 4. Mapping starts
> > 
> > Won't the above code mean that we create what userspace expected to be
> > the second window as the first window replacing the default one
> > instead?
> 
> Uff. Yes, this is correct.
> 
> This is a bug in my initial design - I should not have created a "default"
> window in the first place - it does not make sense at this level; and the
> userspace (i.e. QEMU) removes all windows on reset anyway.
> 
> The only way QEMU will use this default window is when it is at 318f67ce1
> "vfio: spapr: Add DMA memory preregistering (SPAPR IOMMU v2)" - memory
> preregistration is there, but ddw is not yet but it is 2 patches later
> where vfio_connect_container() unconditionally removes the default window.
> Which is unlikely scenario.
> 
> Taking into account small number of DDW users (basically - QEMU v2.7.0),
> does it still make sense to keep this default window code? The only
> immediate problem I see here is QEMU will receive error from
> vfio_spapr_remove_window() in vfio_connect_container() as there will be no
> default window but I could workaround this.

Hmm.. I dislike changing the semantics now.

Can't you just
	if (def_window_pending)
		create_default_window()

Before completing the CREATE_WINDOW ioctl().  That should handle it,
no?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]