On Wed, Nov 23, 2016 at 04:06:30PM +1100, Alexey Kardashevskiy wrote:

On 23/11/16 12:35, David Gibson wrote:

quoted

On Tue, Nov 22, 2016 at 06:29:39PM +1100, Alexey Kardashevskiy wrote:

quoted

On 22/11/16 13:50, David Gibson wrote:

quoted

On Fri, Nov 11, 2016 at 11:32:15PM +1100, Alexey Kardashevskiy wrote:

quoted

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 <redacted>
---
 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.

quoted

quoted

-
-	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.

quoted

quoted

+		}
+
 		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