[PATCH v3 0/6] vfio/type1: Add support for valid iova list management

[PATCH v3 0/6] vfio/type1: Add support for valid iova list management

[Shameer Kolothum]

This series introduces an iova list associated with a vfio 
iommu. The list is kept updated taking care of iommu apertures,
and reserved regions. Also this series adds checks for any conflict
with existing dma mappings whenever a new device group is attached to
the domain.

User-space can retrieve valid iova ranges using VFIO_IOMMU_GET_INFO
ioctl capability chains. Any dma map request outside the valid iova
range will be rejected.

RFCv2 --> v3
 Removed RFC tag.
 Addressed comments from Alex and Eric:
 - Added comments to make iova list management logic more clear.
 - Use of iova list copy so that original is not altered in
   case of failure.

RFCv1 --> RFCv2
 Addressed comments from Alex:
-Introduced IOVA list management and added checks for conflicts with 
 existing dma map entries during attach/detach.

Shameer Kolothum (6):
  vfio/type1: Introduce iova list and add iommu aperture validity check
  vfio/type1: Check reserve region conflict and update iova list
  vfio/type1: Update iova list on detach
  vfio/type1: check dma map request is within a valid iova range
  vfio/type1: Add IOVA range capability support
  vfio/type1: remove duplicate retrieval of reserved regions.

 drivers/vfio/vfio_iommu_type1.c | 499 +++++++++++++++++++++++++++++++++++++++-
 include/uapi/linux/vfio.h       |  23 ++
 2 files changed, 511 insertions(+), 11 deletions(-)

-- 
2.7.4

[PATCH v3 1/6] vfio/type1: Introduce iova list and add iommu aperture validity check

[Shameer Kolothum]

This introduces an iova list that is valid for dma mappings. Make
sure the new iommu aperture window doesn't conflict with the current
one or with any existing dma mappings during attach.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/vfio/vfio_iommu_type1.c | 183 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 181 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index e30e29a..4726f55 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -60,6 +60,7 @@ MODULE_PARM_DESC(disable_hugepages,
 
 struct vfio_iommu {
 	struct list_head	domain_list;
+	struct list_head	iova_list;
 	struct vfio_domain	*external_domain; /* domain for external user */
 	struct mutex		lock;
 	struct rb_root		dma_list;
@@ -92,6 +93,12 @@ struct vfio_group {
 	struct list_head	next;
 };
 
+struct vfio_iova {
+	struct list_head	list;
+	dma_addr_t		start;
+	dma_addr_t		end;
+};
+
 /*
  * Guest RAM pinning working set or DMA target
  */
@@ -1192,6 +1199,142 @@ static bool vfio_iommu_has_sw_msi(struct iommu_group *group, phys_addr_t *base)
 	return ret;
 }
 
+/*
+ * This is a helper function to insert an address range to iova list.
+ * The list starts with a single entry corresponding to the IOMMU
+ * domain geometry to which the device group is attached. The list
+ * aperture gets modified when a new domain is added to the container
+ * if the new aperture doesn't conflict with the current one or with
+ * any existing dma mappings. The list is also modified to exclude
+ * any reserved regions associated with the device group.
+ */
+static int vfio_insert_iova(phys_addr_t start, phys_addr_t end,
+				struct list_head *head)
+{
+	struct vfio_iova *region;
+
+	region = kmalloc(sizeof(*region), GFP_KERNEL);
+	if (!region)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&region->list);
+	region->start = start;
+	region->end = end;
+
+	list_add_tail(&region->list, head);
+	return 0;
+}
+
+/*
+ * Check the new iommu aperture conflicts with existing aper or
+ * with any existing dma mappings.
+ */
+static bool vfio_iommu_aper_conflict(struct vfio_iommu *iommu,
+				     phys_addr_t start,
+				     phys_addr_t end)
+{
+	struct vfio_iova *first, *last;
+	struct list_head *iova = &iommu->iova_list;
+
+	if (list_empty(iova))
+		return false;
+
+	/* Disjoint sets, return conflict */
+	first = list_first_entry(iova, struct vfio_iova, list);
+	last = list_last_entry(iova, struct vfio_iova, list);
+	if ((start > last->end) || (end < first->start))
+		return true;
+
+	/* Check for any existing dma mappings outside the new start */
+	if (start > first->start) {
+		if (vfio_find_dma(iommu, first->start, start - first->start))
+			return true;
+	}
+
+	/* Check for any existing dma mappings outside the new end */
+	if (end < last->end) {
+		if (vfio_find_dma(iommu, end + 1, last->end - end))
+			return true;
+	}
+
+	return false;
+}
+
+/*
+ * Resize iommu iova aperture window. This is called only if the new
+ * aperture has no conflict with existing aperture and dma mappings.
+ */
+static int vfio_iommu_aper_resize(struct list_head *iova,
+				      dma_addr_t start,
+				      dma_addr_t end)
+{
+	struct vfio_iova *node, *next;
+
+	if (list_empty(iova))
+		return vfio_insert_iova(start, end, iova);
+
+	/* Adjust iova list start */
+	list_for_each_entry_safe(node, next, iova, list) {
+		if (start < node->start)
+			break;
+		if ((start >= node->start) && (start < node->end)) {
+			node->start = start;
+			break;
+		}
+		/* Delete nodes before new start */
+		list_del(&node->list);
+		kfree(node);
+	}
+
+	/* Adjust iova list end */
+	list_for_each_entry_safe(node, next, iova, list) {
+		if (end > node->end)
+			continue;
+
+		if ((end >= node->start) && (end < node->end)) {
+			node->end = end;
+			continue;
+		}
+		/* Delete nodes after new end */
+		list_del(&node->list);
+		kfree(node);
+	}
+
+	return 0;
+}
+
+static int vfio_iommu_get_iova_copy(struct vfio_iommu *iommu,
+				struct list_head *iova_copy)
+{
+
+	struct list_head *iova = &iommu->iova_list;
+	struct vfio_iova *n;
+
+	list_for_each_entry(n, iova, list) {
+		int ret;
+
+		ret = vfio_insert_iova(n->start, n->end, iova_copy);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static void vfio_iommu_insert_iova_copy(struct vfio_iommu *iommu,
+					struct list_head *iova_copy)
+{
+	struct list_head *iova = &iommu->iova_list;
+	struct vfio_iova *n, *next;
+
+	list_for_each_entry_safe(n, next, iova, list) {
+		list_del(&n->list);
+		kfree(n);
+	}
+
+	list_splice_tail(iova_copy, iova);
+}
+
 static int vfio_iommu_type1_attach_group(void *iommu_data,
 					 struct iommu_group *iommu_group)
 {
@@ -1202,6 +1345,9 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	int ret;
 	bool resv_msi, msi_remap;
 	phys_addr_t resv_msi_base;
+	struct iommu_domain_geometry geo;
+	struct list_head iova_copy;
+	struct vfio_iova *iova, *iova_next;
 
 	mutex_lock(&iommu->lock);
 
@@ -1271,6 +1417,26 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	if (ret)
 		goto out_domain;
 
+	/* Get aperture info */
+	iommu_domain_get_attr(domain->domain, DOMAIN_ATTR_GEOMETRY, &geo);
+
+	if (vfio_iommu_aper_conflict(iommu, geo.aperture_start,
+					    geo.aperture_end)) {
+		ret = -EINVAL;
+		goto out_detach;
+	}
+
+	/* Get a copy of the current iova list and work on it */
+	INIT_LIST_HEAD(&iova_copy);
+	ret = vfio_iommu_get_iova_copy(iommu, &iova_copy);
+	if (ret)
+		goto out_detach;
+
+	ret = vfio_iommu_aper_resize(&iova_copy, geo.aperture_start,
+							geo.aperture_end);
+	if (ret)
+		goto out_detach;
+
 	resv_msi = vfio_iommu_has_sw_msi(iommu_group, &resv_msi_base);
 
 	INIT_LIST_HEAD(&domain->group_list);
@@ -1304,8 +1470,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 				list_add(&group->next, &d->group_list);
 				iommu_domain_free(domain->domain);
 				kfree(domain);
-				mutex_unlock(&iommu->lock);
-				return 0;
+				goto done;
 			}
 
 			ret = iommu_attach_group(domain->domain, iommu_group);
@@ -1328,6 +1493,9 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	}
 
 	list_add(&domain->next, &iommu->domain_list);
+done:
+	/* Delete the old one and insert new iova list */
+	vfio_iommu_insert_iova_copy(iommu, &iova_copy);
 
 	mutex_unlock(&iommu->lock);
 
@@ -1337,6 +1505,8 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	iommu_detach_group(domain->domain, iommu_group);
 out_domain:
 	iommu_domain_free(domain->domain);
+	list_for_each_entry_safe(iova, iova_next, &iova_copy, list)
+		kfree(iova);
 out_free:
 	kfree(domain);
 	kfree(group);
@@ -1475,6 +1645,7 @@ static void *vfio_iommu_type1_open(unsigned long arg)
 	}
 
 	INIT_LIST_HEAD(&iommu->domain_list);
+	INIT_LIST_HEAD(&iommu->iova_list);
 	iommu->dma_list = RB_ROOT;
 	mutex_init(&iommu->lock);
 	BLOCKING_INIT_NOTIFIER_HEAD(&iommu->notifier);
@@ -1502,6 +1673,7 @@ static void vfio_iommu_type1_release(void *iommu_data)
 {
 	struct vfio_iommu *iommu = iommu_data;
 	struct vfio_domain *domain, *domain_tmp;
+	struct vfio_iova *iova, *iova_next;
 
 	if (iommu->external_domain) {
 		vfio_release_domain(iommu->external_domain, true);
@@ -1517,6 +1689,13 @@ static void vfio_iommu_type1_release(void *iommu_data)
 		list_del(&domain->next);
 		kfree(domain);
 	}
+
+	list_for_each_entry_safe(iova, iova_next,
+				 &iommu->iova_list, list) {
+		list_del(&iova->list);
+		kfree(iova);
+	}
+
 	kfree(iommu);
 }
 
-- 
2.7.4

Re: [PATCH v3 1/6] vfio/type1: Introduce iova list and add iommu aperture validity check

[Alex Williamson]

On Thu, 15 Feb 2018 09:44:59 +0000
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> This introduces an iova list that is valid for dma mappings. Make
> sure the new iommu aperture window doesn't conflict with the current
> one or with any existing dma mappings during attach.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 183 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 181 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index e30e29a..4726f55 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -60,6 +60,7 @@ MODULE_PARM_DESC(disable_hugepages,
>  
>  struct vfio_iommu {
>  	struct list_head	domain_list;
> +	struct list_head	iova_list;
>  	struct vfio_domain	*external_domain; /* domain for external user */
>  	struct mutex		lock;
>  	struct rb_root		dma_list;
> @@ -92,6 +93,12 @@ struct vfio_group {
>  	struct list_head	next;
>  };
>  
> +struct vfio_iova {
> +	struct list_head	list;
> +	dma_addr_t		start;
> +	dma_addr_t		end;
> +};
> +
>  /*
>   * Guest RAM pinning working set or DMA target
>   */
> @@ -1192,6 +1199,142 @@ static bool vfio_iommu_has_sw_msi(struct iommu_group *group, phys_addr_t *base)
>  	return ret;
>  }
>  
> +/*
> + * This is a helper function to insert an address range to iova list.
> + * The list starts with a single entry corresponding to the IOMMU
> + * domain geometry to which the device group is attached. The list
> + * aperture gets modified when a new domain is added to the container
> + * if the new aperture doesn't conflict with the current one or with
> + * any existing dma mappings. The list is also modified to exclude
> + * any reserved regions associated with the device group.
> + */
> +static int vfio_insert_iova(phys_addr_t start, phys_addr_t end,
> +				struct list_head *head)

The args seem more natural to me and have better consistency with the
other functions re-ordered as (head, start, end).

Also, if the iova list is dma_addr_t, why are we using phys_addr_t for
args?

> +{
> +	struct vfio_iova *region;
> +
> +	region = kmalloc(sizeof(*region), GFP_KERNEL);
> +	if (!region)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&region->list);
> +	region->start = start;
> +	region->end = end;
> +
> +	list_add_tail(&region->list, head);
> +	return 0;
> +}
> +
> +/*
> + * Check the new iommu aperture conflicts with existing aper or
> + * with any existing dma mappings.
> + */
> +static bool vfio_iommu_aper_conflict(struct vfio_iommu *iommu,
> +				     phys_addr_t start,
> +				     phys_addr_t end)

Same here, why phys_addr_t when comparing to dma_addr_t?

> +{
> +	struct vfio_iova *first, *last;
> +	struct list_head *iova = &iommu->iova_list;
> +
> +	if (list_empty(iova))
> +		return false;
> +
> +	/* Disjoint sets, return conflict */
> +	first = list_first_entry(iova, struct vfio_iova, list);
> +	last = list_last_entry(iova, struct vfio_iova, list);
> +	if ((start > last->end) || (end < first->start))
> +		return true;
> +
> +	/* Check for any existing dma mappings outside the new start */
> +	if (start > first->start) {
> +		if (vfio_find_dma(iommu, first->start, start - first->start))
> +			return true;
> +	}
> +
> +	/* Check for any existing dma mappings outside the new end */
> +	if (end < last->end) {
> +		if (vfio_find_dma(iommu, end + 1, last->end - end))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +/*
> + * Resize iommu iova aperture window. This is called only if the new
> + * aperture has no conflict with existing aperture and dma mappings.
> + */
> +static int vfio_iommu_aper_resize(struct list_head *iova,
> +				      dma_addr_t start,
> +				      dma_addr_t end)

And here we're back to dma_addr_t, let's be consistent.

> +{
> +	struct vfio_iova *node, *next;
> +
> +	if (list_empty(iova))
> +		return vfio_insert_iova(start, end, iova);
> +
> +	/* Adjust iova list start */
> +	list_for_each_entry_safe(node, next, iova, list) {
> +		if (start < node->start)
> +			break;
> +		if ((start >= node->start) && (start < node->end)) {
> +			node->start = start;
> +			break;
> +		}
> +		/* Delete nodes before new start */
> +		list_del(&node->list);
> +		kfree(node);
> +	}
> +
> +	/* Adjust iova list end */
> +	list_for_each_entry_safe(node, next, iova, list) {
> +		if (end > node->end)
> +			continue;
> +

nit, extra blank line vs block above.

> +		if ((end >= node->start) && (end < node->end)) {

This test is still incorrect, if end == node->start, we get a zero
sized range, we should have let it pass over to get deleted.  Therefore
the first test should be (end > node->start).  The second test was
changed and is now incorrect, if end == node->end, then we want to keep
this range, not delete it.  This test should have remained (end <=
node->end) as it was in the previous version.  IOW, my previous comment
was applied to the wrong test.

> +			node->end = end;
> +			continue;
> +		}
> +		/* Delete nodes after new end */
> +		list_del(&node->list);
> +		kfree(node);
> +	}
> +
> +	return 0;
> +}
> +
> +static int vfio_iommu_get_iova_copy(struct vfio_iommu *iommu,
> +				struct list_head *iova_copy)
> +{
> +
> +	struct list_head *iova = &iommu->iova_list;
> +	struct vfio_iova *n;
> +
> +	list_for_each_entry(n, iova, list) {
> +		int ret;
> +
> +		ret = vfio_insert_iova(n->start, n->end, iova_copy);
> +		if (ret)
> +			return ret;

Let's delete and free any entries added to the copy here too.

> +	}
> +
> +	return 0;
> +}
> +
> +static void vfio_iommu_insert_iova_copy(struct vfio_iommu *iommu,
> +					struct list_head *iova_copy)
> +{
> +	struct list_head *iova = &iommu->iova_list;
> +	struct vfio_iova *n, *next;
> +
> +	list_for_each_entry_safe(n, next, iova, list) {
> +		list_del(&n->list);
> +		kfree(n);
> +	}
> +
> +	list_splice_tail(iova_copy, iova);
> +}
> +
>  static int vfio_iommu_type1_attach_group(void *iommu_data,
>  					 struct iommu_group *iommu_group)
>  {
> @@ -1202,6 +1345,9 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	int ret;
>  	bool resv_msi, msi_remap;
>  	phys_addr_t resv_msi_base;
> +	struct iommu_domain_geometry geo;
> +	struct list_head iova_copy;
> +	struct vfio_iova *iova, *iova_next;
>  
>  	mutex_lock(&iommu->lock);
>  
> @@ -1271,6 +1417,26 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	if (ret)
>  		goto out_domain;
>  
> +	/* Get aperture info */
> +	iommu_domain_get_attr(domain->domain, DOMAIN_ATTR_GEOMETRY, &geo);
> +
> +	if (vfio_iommu_aper_conflict(iommu, geo.aperture_start,
> +					    geo.aperture_end)) {
> +		ret = -EINVAL;
> +		goto out_detach;
> +	}
> +
> +	/* Get a copy of the current iova list and work on it */
> +	INIT_LIST_HEAD(&iova_copy);

We could have just declared this as:

LIST_HEAD(iova_copy);

to avoid needing to init it separately.

> +	ret = vfio_iommu_get_iova_copy(iommu, &iova_copy);
> +	if (ret)
> +		goto out_detach;
> +
> +	ret = vfio_iommu_aper_resize(&iova_copy, geo.aperture_start,
> +							geo.aperture_end);
> +	if (ret)
> +		goto out_detach;
> +
>  	resv_msi = vfio_iommu_has_sw_msi(iommu_group, &resv_msi_base);
>  
>  	INIT_LIST_HEAD(&domain->group_list);
> @@ -1304,8 +1470,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  				list_add(&group->next, &d->group_list);
>  				iommu_domain_free(domain->domain);
>  				kfree(domain);
> -				mutex_unlock(&iommu->lock);
> -				return 0;
> +				goto done;
>  			}
>  
>  			ret = iommu_attach_group(domain->domain, iommu_group);
> @@ -1328,6 +1493,9 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	}
>  
>  	list_add(&domain->next, &iommu->domain_list);
> +done:
> +	/* Delete the old one and insert new iova list */
> +	vfio_iommu_insert_iova_copy(iommu, &iova_copy);
>  
>  	mutex_unlock(&iommu->lock);
>  
> @@ -1337,6 +1505,8 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	iommu_detach_group(domain->domain, iommu_group);
>  out_domain:
>  	iommu_domain_free(domain->domain);
> +	list_for_each_entry_safe(iova, iova_next, &iova_copy, list)
> +		kfree(iova);

Let's do the list_del() too, it's making me squirm that it's not here
and this is not a performance path.

>  out_free:
>  	kfree(domain);
>  	kfree(group);
> @@ -1475,6 +1645,7 @@ static void *vfio_iommu_type1_open(unsigned long arg)
>  	}
>  
>  	INIT_LIST_HEAD(&iommu->domain_list);
> +	INIT_LIST_HEAD(&iommu->iova_list);
>  	iommu->dma_list = RB_ROOT;
>  	mutex_init(&iommu->lock);
>  	BLOCKING_INIT_NOTIFIER_HEAD(&iommu->notifier);
> @@ -1502,6 +1673,7 @@ static void vfio_iommu_type1_release(void *iommu_data)
>  {
>  	struct vfio_iommu *iommu = iommu_data;
>  	struct vfio_domain *domain, *domain_tmp;
> +	struct vfio_iova *iova, *iova_next;
>  
>  	if (iommu->external_domain) {
>  		vfio_release_domain(iommu->external_domain, true);
> @@ -1517,6 +1689,13 @@ static void vfio_iommu_type1_release(void *iommu_data)
>  		list_del(&domain->next);
>  		kfree(domain);
>  	}
> +
> +	list_for_each_entry_safe(iova, iova_next,
> +				 &iommu->iova_list, list) {
> +		list_del(&iova->list);
> +		kfree(iova);
> +	}
> +
>  	kfree(iommu);
>  }
>  

RE: [PATCH v3 1/6] vfio/type1: Introduce iova list and add iommu aperture validity check

[Shameerali Kolothum Thodi]

Hi Alex,

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Friday, February 16, 2018 8:49 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: eric.auger@redhat.com; pmorel@linux.vnet.ibm.com;
> kvm@vger.kernel.org; linux-kernel@vger.kernel.org; Linuxarm
> <linuxarm@huawei.com>; John Garry <john.garry@huawei.com>; xuwei (O)
> <xuwei5@huawei.com>
> Subject: Re: [PATCH v3 1/6] vfio/type1: Introduce iova list and add iommu
> aperture validity check
> 
> On Thu, 15 Feb 2018 09:44:59 +0000
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> 
> > This introduces an iova list that is valid for dma mappings. Make
> > sure the new iommu aperture window doesn't conflict with the current
> > one or with any existing dma mappings during attach.
> >
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  drivers/vfio/vfio_iommu_type1.c | 183
> +++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 181 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vfio/vfio_iommu_type1.c
> b/drivers/vfio/vfio_iommu_type1.c
> > index e30e29a..4726f55 100644
> > --- a/drivers/vfio/vfio_iommu_type1.c
> > +++ b/drivers/vfio/vfio_iommu_type1.c
> > @@ -60,6 +60,7 @@ MODULE_PARM_DESC(disable_hugepages,
> >
> >  struct vfio_iommu {
> >  	struct list_head	domain_list;
> > +	struct list_head	iova_list;
> >  	struct vfio_domain	*external_domain; /* domain for external user
> */
> >  	struct mutex		lock;
> >  	struct rb_root		dma_list;
> > @@ -92,6 +93,12 @@ struct vfio_group {
> >  	struct list_head	next;
> >  };
> >
> > +struct vfio_iova {
> > +	struct list_head	list;
> > +	dma_addr_t		start;
> > +	dma_addr_t		end;
> > +};
> > +
> >  /*
> >   * Guest RAM pinning working set or DMA target
> >   */
> > @@ -1192,6 +1199,142 @@ static bool vfio_iommu_has_sw_msi(struct
> iommu_group *group, phys_addr_t *base)
> >  	return ret;
> >  }
> >
> > +/*
> > + * This is a helper function to insert an address range to iova list.
> > + * The list starts with a single entry corresponding to the IOMMU
> > + * domain geometry to which the device group is attached. The list
> > + * aperture gets modified when a new domain is added to the container
> > + * if the new aperture doesn't conflict with the current one or with
> > + * any existing dma mappings. The list is also modified to exclude
> > + * any reserved regions associated with the device group.
> > + */
> > +static int vfio_insert_iova(phys_addr_t start, phys_addr_t end,
> > +				struct list_head *head)
> 
> The args seem more natural to me and have better consistency with the
> other functions re-ordered as (head, start, end).
> 
> Also, if the iova list is dma_addr_t, why are we using phys_addr_t for
> args?
> 
> > +{
> > +	struct vfio_iova *region;
> > +
> > +	region = kmalloc(sizeof(*region), GFP_KERNEL);
> > +	if (!region)
> > +		return -ENOMEM;
> > +
> > +	INIT_LIST_HEAD(&region->list);
> > +	region->start = start;
> > +	region->end = end;
> > +
> > +	list_add_tail(&region->list, head);
> > +	return 0;
> > +}
> > +
> > +/*
> > + * Check the new iommu aperture conflicts with existing aper or
> > + * with any existing dma mappings.
> > + */
> > +static bool vfio_iommu_aper_conflict(struct vfio_iommu *iommu,
> > +				     phys_addr_t start,
> > +				     phys_addr_t end)
> 
> Same here, why phys_addr_t when comparing to dma_addr_t?
> 
> > +{
> > +	struct vfio_iova *first, *last;
> > +	struct list_head *iova = &iommu->iova_list;
> > +
> > +	if (list_empty(iova))
> > +		return false;
> > +
> > +	/* Disjoint sets, return conflict */
> > +	first = list_first_entry(iova, struct vfio_iova, list);
> > +	last = list_last_entry(iova, struct vfio_iova, list);
> > +	if ((start > last->end) || (end < first->start))
> > +		return true;
> > +
> > +	/* Check for any existing dma mappings outside the new start */
> > +	if (start > first->start) {
> > +		if (vfio_find_dma(iommu, first->start, start - first->start))
> > +			return true;
> > +	}
> > +
> > +	/* Check for any existing dma mappings outside the new end */
> > +	if (end < last->end) {
> > +		if (vfio_find_dma(iommu, end + 1, last->end - end))
> > +			return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> > +/*
> > + * Resize iommu iova aperture window. This is called only if the new
> > + * aperture has no conflict with existing aperture and dma mappings.
> > + */
> > +static int vfio_iommu_aper_resize(struct list_head *iova,
> > +				      dma_addr_t start,
> > +				      dma_addr_t end)
> 
> And here we're back to dma_addr_t, let's be consistent.

Ok. I will take care of all the above inconsistencies.

> > +{
> > +	struct vfio_iova *node, *next;
> > +
> > +	if (list_empty(iova))
> > +		return vfio_insert_iova(start, end, iova);
> > +
> > +	/* Adjust iova list start */
> > +	list_for_each_entry_safe(node, next, iova, list) {
> > +		if (start < node->start)
> > +			break;
> > +		if ((start >= node->start) && (start < node->end)) {
> > +			node->start = start;
> > +			break;
> > +		}
> > +		/* Delete nodes before new start */
> > +		list_del(&node->list);
> > +		kfree(node);
> > +	}
> > +
> > +	/* Adjust iova list end */
> > +	list_for_each_entry_safe(node, next, iova, list) {
> > +		if (end > node->end)
> > +			continue;
> > +
> 
> nit, extra blank line vs block above.
> 
> > +		if ((end >= node->start) && (end < node->end)) {
> 
> This test is still incorrect, if end == node->start, we get a zero
> sized range, we should have let it pass over to get deleted.  Therefore
> the first test should be (end > node->start).  The second test was
> changed and is now incorrect, if end == node->end, then we want to keep
> this range, not delete it.  This test should have remained (end <=
> node->end) as it was in the previous version.  IOW, my previous comment
> was applied to the wrong test.

Thanks. I got the test wrong for this case.

> > +			node->end = end;
> > +			continue;
> > +		}
> > +		/* Delete nodes after new end */
> > +		list_del(&node->list);
> > +		kfree(node);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int vfio_iommu_get_iova_copy(struct vfio_iommu *iommu,
> > +				struct list_head *iova_copy)
> > +{
> > +
> > +	struct list_head *iova = &iommu->iova_list;
> > +	struct vfio_iova *n;
> > +
> > +	list_for_each_entry(n, iova, list) {
> > +		int ret;
> > +
> > +		ret = vfio_insert_iova(n->start, n->end, iova_copy);
> > +		if (ret)
> > +			return ret;
> 
> Let's delete and free any entries added to the copy here too.

Ok. My original thought was caller will free up in case of error.

> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void vfio_iommu_insert_iova_copy(struct vfio_iommu *iommu,
> > +					struct list_head *iova_copy)
> > +{
> > +	struct list_head *iova = &iommu->iova_list;
> > +	struct vfio_iova *n, *next;
> > +
> > +	list_for_each_entry_safe(n, next, iova, list) {
> > +		list_del(&n->list);
> > +		kfree(n);
> > +	}
> > +
> > +	list_splice_tail(iova_copy, iova);
> > +}
> > +
> >  static int vfio_iommu_type1_attach_group(void *iommu_data,
> >  					 struct iommu_group *iommu_group)
> >  {
> > @@ -1202,6 +1345,9 @@ static int vfio_iommu_type1_attach_group(void
> *iommu_data,
> >  	int ret;
> >  	bool resv_msi, msi_remap;
> >  	phys_addr_t resv_msi_base;
> > +	struct iommu_domain_geometry geo;
> > +	struct list_head iova_copy;
> > +	struct vfio_iova *iova, *iova_next;
> >
> >  	mutex_lock(&iommu->lock);
> >
> > @@ -1271,6 +1417,26 @@ static int vfio_iommu_type1_attach_group(void
> *iommu_data,
> >  	if (ret)
> >  		goto out_domain;
> >
> > +	/* Get aperture info */
> > +	iommu_domain_get_attr(domain->domain,
> DOMAIN_ATTR_GEOMETRY, &geo);
> > +
> > +	if (vfio_iommu_aper_conflict(iommu, geo.aperture_start,
> > +					    geo.aperture_end)) {
> > +		ret = -EINVAL;
> > +		goto out_detach;
> > +	}
> > +
> > +	/* Get a copy of the current iova list and work on it */
> > +	INIT_LIST_HEAD(&iova_copy);
> 
> We could have just declared this as:
> 
> LIST_HEAD(iova_copy);
> 
> to avoid needing to init it separately.

Ok.

Thanks,
Shameer

> > +	ret = vfio_iommu_get_iova_copy(iommu, &iova_copy);
> > +	if (ret)
> > +		goto out_detach;
> > +
> > +	ret = vfio_iommu_aper_resize(&iova_copy, geo.aperture_start,
> > +							geo.aperture_end);
> > +	if (ret)
> > +		goto out_detach;
> > +
> >  	resv_msi = vfio_iommu_has_sw_msi(iommu_group, &resv_msi_base);
> >
> >  	INIT_LIST_HEAD(&domain->group_list);
> > @@ -1304,8 +1470,7 @@ static int vfio_iommu_type1_attach_group(void
> *iommu_data,
> >  				list_add(&group->next, &d->group_list);
> >  				iommu_domain_free(domain->domain);
> >  				kfree(domain);
> > -				mutex_unlock(&iommu->lock);
> > -				return 0;
> > +				goto done;
> >  			}
> >
> >  			ret = iommu_attach_group(domain->domain,
> iommu_group);
> > @@ -1328,6 +1493,9 @@ static int vfio_iommu_type1_attach_group(void
> *iommu_data,
> >  	}
> >
> >  	list_add(&domain->next, &iommu->domain_list);
> > +done:
> > +	/* Delete the old one and insert new iova list */
> > +	vfio_iommu_insert_iova_copy(iommu, &iova_copy);
> >
> >  	mutex_unlock(&iommu->lock);
> >
> > @@ -1337,6 +1505,8 @@ static int vfio_iommu_type1_attach_group(void
> *iommu_data,
> >  	iommu_detach_group(domain->domain, iommu_group);
> >  out_domain:
> >  	iommu_domain_free(domain->domain);
> > +	list_for_each_entry_safe(iova, iova_next, &iova_copy, list)
> > +		kfree(iova);
> 
> Let's do the list_del() too, it's making me squirm that it's not here
> and this is not a performance path.
> 
> >  out_free:
> >  	kfree(domain);
> >  	kfree(group);
> > @@ -1475,6 +1645,7 @@ static void *vfio_iommu_type1_open(unsigned
> long arg)
> >  	}
> >
> >  	INIT_LIST_HEAD(&iommu->domain_list);
> > +	INIT_LIST_HEAD(&iommu->iova_list);
> >  	iommu->dma_list = RB_ROOT;
> >  	mutex_init(&iommu->lock);
> >  	BLOCKING_INIT_NOTIFIER_HEAD(&iommu->notifier);
> > @@ -1502,6 +1673,7 @@ static void vfio_iommu_type1_release(void
> *iommu_data)
> >  {
> >  	struct vfio_iommu *iommu = iommu_data;
> >  	struct vfio_domain *domain, *domain_tmp;
> > +	struct vfio_iova *iova, *iova_next;
> >
> >  	if (iommu->external_domain) {
> >  		vfio_release_domain(iommu->external_domain, true);
> > @@ -1517,6 +1689,13 @@ static void vfio_iommu_type1_release(void
> *iommu_data)
> >  		list_del(&domain->next);
> >  		kfree(domain);
> >  	}
> > +
> > +	list_for_each_entry_safe(iova, iova_next,
> > +				 &iommu->iova_list, list) {
> > +		list_del(&iova->list);
> > +		kfree(iova);
> > +	}
> > +
> >  	kfree(iommu);
> >  }
> >

Re: [PATCH v3 1/6] vfio/type1: Introduce iova list and add iommu aperture validity check

[Alex Williamson]

On Mon, 19 Feb 2018 09:50:24 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Friday, February 16, 2018 8:49 PM 
> > On Thu, 15 Feb 2018 09:44:59 +0000
> > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> > > +			node->end = end;
> > > +			continue;
> > > +		}
> > > +		/* Delete nodes after new end */
> > > +		list_del(&node->list);
> > > +		kfree(node);
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int vfio_iommu_get_iova_copy(struct vfio_iommu *iommu,
> > > +				struct list_head *iova_copy)
> > > +{
> > > +
> > > +	struct list_head *iova = &iommu->iova_list;
> > > +	struct vfio_iova *n;
> > > +
> > > +	list_for_each_entry(n, iova, list) {
> > > +		int ret;
> > > +
> > > +		ret = vfio_insert_iova(n->start, n->end, iova_copy);
> > > +		if (ret)
> > > +			return ret;  
> > 
> > Let's delete and free any entries added to the copy here too.  
> 
> Ok. My original thought was caller will free up in case of error.

This comes down to Rusty's suggestions of how to make an API hard to
misuse rather than simply easy to use to me.  Placing the onus on the
caller to cleanup a list sounds simple, but the caller passed an empty
list and the function failed, why should the caller bother to check if
the function left any cruft on the list in the course of failing?  This
is not a hard to misuse interface, in fact it's very easy to forget
that cleanup.  Thanks,

Alex

RE: [PATCH v3 1/6] vfio/type1: Introduce iova list and add iommu aperture validity check

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Monday, February 19, 2018 7:51 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: eric.auger@redhat.com; pmorel@linux.vnet.ibm.com;
> kvm@vger.kernel.org; linux-kernel@vger.kernel.org; Linuxarm
> <linuxarm@huawei.com>; John Garry <john.garry@huawei.com>; xuwei (O)
> <xuwei5@huawei.com>
> Subject: Re: [PATCH v3 1/6] vfio/type1: Introduce iova list and add iommu
> aperture validity check
> 
> On Mon, 19 Feb 2018 09:50:24 +0000
> Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:
> > > -----Original Message-----
> > > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > > Sent: Friday, February 16, 2018 8:49 PM
> > > On Thu, 15 Feb 2018 09:44:59 +0000
> > > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> > > > +			node->end = end;
> > > > +			continue;
> > > > +		}
> > > > +		/* Delete nodes after new end */
> > > > +		list_del(&node->list);
> > > > +		kfree(node);
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > > +static int vfio_iommu_get_iova_copy(struct vfio_iommu *iommu,
> > > > +				struct list_head *iova_copy)
> > > > +{
> > > > +
> > > > +	struct list_head *iova = &iommu->iova_list;
> > > > +	struct vfio_iova *n;
> > > > +
> > > > +	list_for_each_entry(n, iova, list) {
> > > > +		int ret;
> > > > +
> > > > +		ret = vfio_insert_iova(n->start, n->end, iova_copy);
> > > > +		if (ret)
> > > > +			return ret;
> > >
> > > Let's delete and free any entries added to the copy here too.
> >
> > Ok. My original thought was caller will free up in case of error.
> 
> This comes down to Rusty's suggestions of how to make an API hard to
> misuse rather than simply easy to use to me.  Placing the onus on the
> caller to cleanup a list sounds simple, but the caller passed an empty
> list and the function failed, why should the caller bother to check if
> the function left any cruft on the list in the course of failing?  This
> is not a hard to misuse interface, in fact it's very easy to forget
> that cleanup.  Thanks,

Ok. I understand the concerns. I will sent out a revised one soon.

Thanks,
Shameer

[PATCH v3 2/6] vfio/type1: Check reserve region conflict and update iova list

[Shameer Kolothum]

This retrieves the reserved regions associated with dev group and
checks for conflicts with any existing dma mappings. Also update
the iova list excluding the reserved regions.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/vfio/vfio_iommu_type1.c | 86 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 85 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 4726f55..4db87a9 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -1303,6 +1303,72 @@ static int vfio_iommu_aper_resize(struct list_head *iova,
 	return 0;
 }
 
+/*
+ * Check reserved region conflicts with existing dma mappings
+ */
+static bool vfio_iommu_resv_conflict(struct vfio_iommu *iommu,
+				struct list_head *resv_regions)
+{
+	struct iommu_resv_region *region;
+
+	/* Check for conflict with existing dma mappings */
+	list_for_each_entry(region, resv_regions, list) {
+		if (vfio_find_dma(iommu, region->start, region->length))
+			return true;
+	}
+
+	return false;
+}
+
+/*
+ * Check iova region overlap with  reserved regions and
+ * exclude them from the iommu iova range
+ */
+static int vfio_iommu_resv_exclude(struct list_head *iova,
+					struct list_head *resv_regions)
+{
+	struct iommu_resv_region *resv;
+	struct vfio_iova *n, *next;
+
+	list_for_each_entry(resv, resv_regions, list) {
+		phys_addr_t start, end;
+
+		start = resv->start;
+		end = resv->start + resv->length - 1;
+
+		list_for_each_entry_safe(n, next, iova, list) {
+			int ret = 0;
+
+			/* No overlap */
+			if ((start > n->end) || (end < n->start))
+				continue;
+			/*
+			 * Insert a new node if current node overlaps with the
+			 * reserve region to exlude that from valid iova range.
+			 * Note that, new node is inserted before the current
+			 * node and finally the current node is deleted keeping
+			 * the list updated and sorted.
+			 */
+			if (start > n->start)
+				ret = vfio_insert_iova(n->start, start - 1,
+								 &n->list);
+			if (!ret && end < n->end)
+				ret = vfio_insert_iova(end + 1, n->end,
+							     &n->list);
+			if (ret)
+				return ret;
+
+			list_del(&n->list);
+			kfree(n);
+		}
+	}
+
+	if (list_empty(iova))
+		return -EINVAL;
+
+	return 0;
+}
+
 static int vfio_iommu_get_iova_copy(struct vfio_iommu *iommu,
 				struct list_head *iova_copy)
 {
@@ -1346,7 +1412,8 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	bool resv_msi, msi_remap;
 	phys_addr_t resv_msi_base;
 	struct iommu_domain_geometry geo;
-	struct list_head iova_copy;
+	struct list_head iova_copy, group_resv_regions;
+	struct iommu_resv_region *resv, *resv_next;
 	struct vfio_iova *iova, *iova_next;
 
 	mutex_lock(&iommu->lock);
@@ -1426,6 +1493,14 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 		goto out_detach;
 	}
 
+	INIT_LIST_HEAD(&group_resv_regions);
+	iommu_get_group_resv_regions(iommu_group, &group_resv_regions);
+
+	if (vfio_iommu_resv_conflict(iommu, &group_resv_regions)) {
+		ret = -EINVAL;
+		goto out_detach;
+	}
+
 	/* Get a copy of the current iova list and work on it */
 	INIT_LIST_HEAD(&iova_copy);
 	ret = vfio_iommu_get_iova_copy(iommu, &iova_copy);
@@ -1437,6 +1512,10 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	if (ret)
 		goto out_detach;
 
+	ret = vfio_iommu_resv_exclude(&iova_copy, &group_resv_regions);
+	if (ret)
+		goto out_detach;
+
 	resv_msi = vfio_iommu_has_sw_msi(iommu_group, &resv_msi_base);
 
 	INIT_LIST_HEAD(&domain->group_list);
@@ -1497,6 +1576,9 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	/* Delete the old one and insert new iova list */
 	vfio_iommu_insert_iova_copy(iommu, &iova_copy);
 
+	list_for_each_entry_safe(resv, resv_next, &group_resv_regions, list)
+		kfree(resv);
+
 	mutex_unlock(&iommu->lock);
 
 	return 0;
@@ -1507,6 +1589,8 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	iommu_domain_free(domain->domain);
 	list_for_each_entry_safe(iova, iova_next, &iova_copy, list)
 		kfree(iova);
+	list_for_each_entry_safe(resv, resv_next, &group_resv_regions, list)
+		kfree(resv);
 out_free:
 	kfree(domain);
 	kfree(group);
-- 
2.7.4

Re: [PATCH v3 2/6] vfio/type1: Check reserve region conflict and update iova list

[Alex Williamson]

On Thu, 15 Feb 2018 09:45:00 +0000
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> This retrieves the reserved regions associated with dev group and
> checks for conflicts with any existing dma mappings. Also update
> the iova list excluding the reserved regions.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 86 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 85 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 4726f55..4db87a9 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -1303,6 +1303,72 @@ static int vfio_iommu_aper_resize(struct list_head *iova,
>  	return 0;
>  }
>  
> +/*
> + * Check reserved region conflicts with existing dma mappings
> + */
> +static bool vfio_iommu_resv_conflict(struct vfio_iommu *iommu,
> +				struct list_head *resv_regions)
> +{
> +	struct iommu_resv_region *region;
> +
> +	/* Check for conflict with existing dma mappings */
> +	list_for_each_entry(region, resv_regions, list) {
> +		if (vfio_find_dma(iommu, region->start, region->length))
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
> +/*
> + * Check iova region overlap with  reserved regions and
> + * exclude them from the iommu iova range
> + */
> +static int vfio_iommu_resv_exclude(struct list_head *iova,
> +					struct list_head *resv_regions)
> +{
> +	struct iommu_resv_region *resv;
> +	struct vfio_iova *n, *next;
> +
> +	list_for_each_entry(resv, resv_regions, list) {
> +		phys_addr_t start, end;
> +
> +		start = resv->start;
> +		end = resv->start + resv->length - 1;
> +
> +		list_for_each_entry_safe(n, next, iova, list) {
> +			int ret = 0;
> +
> +			/* No overlap */
> +			if ((start > n->end) || (end < n->start))
> +				continue;
> +			/*
> +			 * Insert a new node if current node overlaps with the
> +			 * reserve region to exlude that from valid iova range.
> +			 * Note that, new node is inserted before the current
> +			 * node and finally the current node is deleted keeping
> +			 * the list updated and sorted.
> +			 */
> +			if (start > n->start)
> +				ret = vfio_insert_iova(n->start, start - 1,
> +								 &n->list);
> +			if (!ret && end < n->end)
> +				ret = vfio_insert_iova(end + 1, n->end,
> +							     &n->list);
> +			if (ret)
> +				return ret;
> +
> +			list_del(&n->list);
> +			kfree(n);
> +		}
> +	}
> +
> +	if (list_empty(iova))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static int vfio_iommu_get_iova_copy(struct vfio_iommu *iommu,
>  				struct list_head *iova_copy)
>  {
> @@ -1346,7 +1412,8 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	bool resv_msi, msi_remap;
>  	phys_addr_t resv_msi_base;
>  	struct iommu_domain_geometry geo;
> -	struct list_head iova_copy;
> +	struct list_head iova_copy, group_resv_regions;
> +	struct iommu_resv_region *resv, *resv_next;
>  	struct vfio_iova *iova, *iova_next;
>  
>  	mutex_lock(&iommu->lock);
> @@ -1426,6 +1493,14 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  		goto out_detach;
>  	}
>  
> +	INIT_LIST_HEAD(&group_resv_regions);

LIST_HEAD()

> +	iommu_get_group_resv_regions(iommu_group, &group_resv_regions);
> +
> +	if (vfio_iommu_resv_conflict(iommu, &group_resv_regions)) {
> +		ret = -EINVAL;
> +		goto out_detach;
> +	}
> +
>  	/* Get a copy of the current iova list and work on it */
>  	INIT_LIST_HEAD(&iova_copy);
>  	ret = vfio_iommu_get_iova_copy(iommu, &iova_copy);
> @@ -1437,6 +1512,10 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	if (ret)
>  		goto out_detach;
>  
> +	ret = vfio_iommu_resv_exclude(&iova_copy, &group_resv_regions);
> +	if (ret)
> +		goto out_detach;
> +
>  	resv_msi = vfio_iommu_has_sw_msi(iommu_group, &resv_msi_base);
>  
>  	INIT_LIST_HEAD(&domain->group_list);
> @@ -1497,6 +1576,9 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	/* Delete the old one and insert new iova list */
>  	vfio_iommu_insert_iova_copy(iommu, &iova_copy);
>  
> +	list_for_each_entry_safe(resv, resv_next, &group_resv_regions, list)
> +		kfree(resv);

list_del() here and below, also this can be done after the mutex unlock.

> +
>  	mutex_unlock(&iommu->lock);
>  
>  	return 0;
> @@ -1507,6 +1589,8 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
>  	iommu_domain_free(domain->domain);
>  	list_for_each_entry_safe(iova, iova_next, &iova_copy, list)
>  		kfree(iova);
> +	list_for_each_entry_safe(resv, resv_next, &group_resv_regions, list)
> +		kfree(resv);
>  out_free:
>  	kfree(domain);
>  	kfree(group);

RE: [PATCH v3 2/6] vfio/type1: Check reserve region conflict and update iova list

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Friday, February 16, 2018 9:18 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: eric.auger@redhat.com; pmorel@linux.vnet.ibm.com;
> kvm@vger.kernel.org; linux-kernel@vger.kernel.org; Linuxarm
> <linuxarm@huawei.com>; John Garry <john.garry@huawei.com>; xuwei (O)
> <xuwei5@huawei.com>
> Subject: Re: [PATCH v3 2/6] vfio/type1: Check reserve region conflict and
> update iova list
> 
> On Thu, 15 Feb 2018 09:45:00 +0000
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> 
> > This retrieves the reserved regions associated with dev group and
> > checks for conflicts with any existing dma mappings. Also update
> > the iova list excluding the reserved regions.
> >
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  drivers/vfio/vfio_iommu_type1.c | 86
> ++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 85 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vfio/vfio_iommu_type1.c
> b/drivers/vfio/vfio_iommu_type1.c
> > index 4726f55..4db87a9 100644
> > --- a/drivers/vfio/vfio_iommu_type1.c
> > +++ b/drivers/vfio/vfio_iommu_type1.c
> > @@ -1303,6 +1303,72 @@ static int vfio_iommu_aper_resize(struct
> list_head *iova,
> >  	return 0;
> >  }
> >
> > +/*
> > + * Check reserved region conflicts with existing dma mappings
> > + */
> > +static bool vfio_iommu_resv_conflict(struct vfio_iommu *iommu,
> > +				struct list_head *resv_regions)
> > +{
> > +	struct iommu_resv_region *region;
> > +
> > +	/* Check for conflict with existing dma mappings */
> > +	list_for_each_entry(region, resv_regions, list) {
> > +		if (vfio_find_dma(iommu, region->start, region->length))
> > +			return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> > +/*
> > + * Check iova region overlap with  reserved regions and
> > + * exclude them from the iommu iova range
> > + */
> > +static int vfio_iommu_resv_exclude(struct list_head *iova,
> > +					struct list_head *resv_regions)
> > +{
> > +	struct iommu_resv_region *resv;
> > +	struct vfio_iova *n, *next;
> > +
> > +	list_for_each_entry(resv, resv_regions, list) {
> > +		phys_addr_t start, end;
> > +
> > +		start = resv->start;
> > +		end = resv->start + resv->length - 1;
> > +
> > +		list_for_each_entry_safe(n, next, iova, list) {
> > +			int ret = 0;
> > +
> > +			/* No overlap */
> > +			if ((start > n->end) || (end < n->start))
> > +				continue;
> > +			/*
> > +			 * Insert a new node if current node overlaps with the
> > +			 * reserve region to exlude that from valid iova range.
> > +			 * Note that, new node is inserted before the current
> > +			 * node and finally the current node is deleted keeping
> > +			 * the list updated and sorted.
> > +			 */
> > +			if (start > n->start)
> > +				ret = vfio_insert_iova(n->start, start - 1,
> > +								 &n->list);
> > +			if (!ret && end < n->end)
> > +				ret = vfio_insert_iova(end + 1, n->end,
> > +							     &n->list);
> > +			if (ret)
> > +				return ret;
> > +
> > +			list_del(&n->list);
> > +			kfree(n);
> > +		}
> > +	}
> > +
> > +	if (list_empty(iova))
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> >  static int vfio_iommu_get_iova_copy(struct vfio_iommu *iommu,
> >  				struct list_head *iova_copy)
> >  {
> > @@ -1346,7 +1412,8 @@ static int vfio_iommu_type1_attach_group(void
> *iommu_data,
> >  	bool resv_msi, msi_remap;
> >  	phys_addr_t resv_msi_base;
> >  	struct iommu_domain_geometry geo;
> > -	struct list_head iova_copy;
> > +	struct list_head iova_copy, group_resv_regions;
> > +	struct iommu_resv_region *resv, *resv_next;
> >  	struct vfio_iova *iova, *iova_next;
> >
> >  	mutex_lock(&iommu->lock);
> > @@ -1426,6 +1493,14 @@ static int vfio_iommu_type1_attach_group(void
> *iommu_data,
> >  		goto out_detach;
> >  	}
> >
> > +	INIT_LIST_HEAD(&group_resv_regions);
> 
> LIST_HEAD()

Ok.

> > +	iommu_get_group_resv_regions(iommu_group, &group_resv_regions);
> > +
> > +	if (vfio_iommu_resv_conflict(iommu, &group_resv_regions)) {
> > +		ret = -EINVAL;
> > +		goto out_detach;
> > +	}
> > +
> >  	/* Get a copy of the current iova list and work on it */
> >  	INIT_LIST_HEAD(&iova_copy);
> >  	ret = vfio_iommu_get_iova_copy(iommu, &iova_copy);
> > @@ -1437,6 +1512,10 @@ static int vfio_iommu_type1_attach_group(void
> *iommu_data,
> >  	if (ret)
> >  		goto out_detach;
> >
> > +	ret = vfio_iommu_resv_exclude(&iova_copy, &group_resv_regions);
> > +	if (ret)
> > +		goto out_detach;
> > +
> >  	resv_msi = vfio_iommu_has_sw_msi(iommu_group, &resv_msi_base);
> >
> >  	INIT_LIST_HEAD(&domain->group_list);
> > @@ -1497,6 +1576,9 @@ static int vfio_iommu_type1_attach_group(void
> *iommu_data,
> >  	/* Delete the old one and insert new iova list */
> >  	vfio_iommu_insert_iova_copy(iommu, &iova_copy);
> >
> > +	list_for_each_entry_safe(resv, resv_next, &group_resv_regions, list)
> > +		kfree(resv);
> 
> list_del() here and below, also this can be done after the mutex unlock.

Ok. I thought that as the reserved regions are local to this function, list_del() is
not required.  Same for the iova_copy in the first patch as well(which I missed to
comment there).

Thanks,
Shameer 

> > +
> >  	mutex_unlock(&iommu->lock);
> >
> >  	return 0;
> > @@ -1507,6 +1589,8 @@ static int vfio_iommu_type1_attach_group(void
> *iommu_data,
> >  	iommu_domain_free(domain->domain);
> >  	list_for_each_entry_safe(iova, iova_next, &iova_copy, list)
> >  		kfree(iova);
> > +	list_for_each_entry_safe(resv, resv_next, &group_resv_regions, list)
> > +		kfree(resv);
> >  out_free:
> >  	kfree(domain);
> >  	kfree(group);

Re: [PATCH v3 2/6] vfio/type1: Check reserve region conflict and update iova list

[Alex Williamson]

On Mon, 19 Feb 2018 10:00:53 +0000
Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com> wrote:

> > -----Original Message-----
> > From: Alex Williamson [mailto:alex.williamson@redhat.com]
> > Sent: Friday, February 16, 2018 9:18 PM
> > 
> > On Thu, 15 Feb 2018 09:45:00 +0000
> > Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> > > +	iommu_get_group_resv_regions(iommu_group, &group_resv_regions);
> > > +
> > > +	if (vfio_iommu_resv_conflict(iommu, &group_resv_regions)) {
> > > +		ret = -EINVAL;
> > > +		goto out_detach;
> > > +	}
> > > +
> > >  	/* Get a copy of the current iova list and work on it */
> > >  	INIT_LIST_HEAD(&iova_copy);
> > >  	ret = vfio_iommu_get_iova_copy(iommu, &iova_copy);
> > > @@ -1437,6 +1512,10 @@ static int vfio_iommu_type1_attach_group(void  
> > *iommu_data,  
> > >  	if (ret)
> > >  		goto out_detach;
> > >
> > > +	ret = vfio_iommu_resv_exclude(&iova_copy, &group_resv_regions);
> > > +	if (ret)
> > > +		goto out_detach;
> > > +
> > >  	resv_msi = vfio_iommu_has_sw_msi(iommu_group, &resv_msi_base);
> > >
> > >  	INIT_LIST_HEAD(&domain->group_list);
> > > @@ -1497,6 +1576,9 @@ static int vfio_iommu_type1_attach_group(void  
> > *iommu_data,  
> > >  	/* Delete the old one and insert new iova list */
> > >  	vfio_iommu_insert_iova_copy(iommu, &iova_copy);
> > >
> > > +	list_for_each_entry_safe(resv, resv_next, &group_resv_regions, list)
> > > +		kfree(resv);  
> > 
> > list_del() here and below, also this can be done after the mutex unlock.  
> 
> Ok. I thought that as the reserved regions are local to this function, list_del() is
> not required.  Same for the iova_copy in the first patch as well(which I missed to
> comment there).

What you have works (afaik), it just seems sloppy to me to have free'd
entries in the list, even as the destructor, as this is not a
performance critical path.  Is this more common elsewhere in the kernel
than I suspect?  Thanks,

Alex

[PATCH v3 3/6] vfio/type1: Update iova list on detach

[Shameer Kolothum]

Get a copy of iova list on _group_detach and try to update the list.
On success replace the current one with the copy. Leave the list as
it is if update fails.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/vfio/vfio_iommu_type1.c | 103 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 103 insertions(+)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 4db87a9..8d8ddd7 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -1646,12 +1646,96 @@ static void vfio_sanity_check_pfn_list(struct vfio_iommu *iommu)
 	WARN_ON(iommu->notifier.head);
 }
 
+/*
+ * Called when a domain is removed in detach. It is possible that
+ * the removed domain decided the iova aperture window. Modify the
+ * iova aperture with the smallest window among existing domains.
+ */
+static void vfio_iommu_aper_expand(struct vfio_iommu *iommu,
+				   struct list_head *iova_copy)
+{
+	struct vfio_domain *domain;
+	struct iommu_domain_geometry geo;
+	struct vfio_iova *node;
+	phys_addr_t start = 0;
+	phys_addr_t end = (phys_addr_t)~0;
+
+	list_for_each_entry(domain, &iommu->domain_list, next) {
+		iommu_domain_get_attr(domain->domain, DOMAIN_ATTR_GEOMETRY,
+				      &geo);
+		if (geo.aperture_start > start)
+			start = geo.aperture_start;
+		if (geo.aperture_end < end)
+			end = geo.aperture_end;
+	}
+
+	/* Modify aperture limits. The new aper is either same or bigger */
+	node = list_first_entry(iova_copy, struct vfio_iova, list);
+	node->start = start;
+	node = list_last_entry(iova_copy, struct vfio_iova, list);
+	node->end = end;
+}
+
+/*
+ * Called when a group is detached. The reserved regions for that
+ * group can be part of valid iova now. But since reserved regions
+ * may be duplicated among groups, populate the iova valid regions
+ * list again.
+ */
+static int vfio_iommu_resv_refresh(struct vfio_iommu *iommu,
+				   struct list_head *iova_copy)
+{
+	struct vfio_domain *d;
+	struct vfio_group *g;
+	struct vfio_iova *node, *tmp;
+	struct iommu_resv_region *resv, *resv_next;
+	struct list_head resv_regions;
+	phys_addr_t start, end;
+	int ret;
+
+	INIT_LIST_HEAD(&resv_regions);
+
+	list_for_each_entry(d, &iommu->domain_list, next) {
+		list_for_each_entry(g, &d->group_list, next)
+			iommu_get_group_resv_regions(g->iommu_group,
+							 &resv_regions);
+	}
+
+	if (list_empty(&resv_regions))
+		return 0;
+
+	node = list_first_entry(iova_copy, struct vfio_iova, list);
+	start = node->start;
+	node = list_last_entry(iova_copy, struct vfio_iova, list);
+	end = node->end;
+
+	/* purge the iova list and create new one */
+	list_for_each_entry_safe(node, tmp, iova_copy, list) {
+		list_del(&node->list);
+		kfree(node);
+	}
+
+	ret = vfio_iommu_aper_resize(iova_copy, start, end);
+	if (ret)
+		goto done;
+
+	/* Exclude current reserved regions from iova ranges */
+	ret = vfio_iommu_resv_exclude(iova_copy, &resv_regions);
+done:
+	list_for_each_entry_safe(resv, resv_next, &resv_regions, list)
+		kfree(resv);
+	return ret;
+}
+
 static void vfio_iommu_type1_detach_group(void *iommu_data,
 					  struct iommu_group *iommu_group)
 {
 	struct vfio_iommu *iommu = iommu_data;
 	struct vfio_domain *domain;
 	struct vfio_group *group;
+	struct list_head iova_copy;
+	struct vfio_iova *iova, *iova_next;
+	bool iova_copy_fail;
 
 	mutex_lock(&iommu->lock);
 
@@ -1674,6 +1758,13 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 		}
 	}
 
+	/*
+	 * Get a copy of iova list. If success, use copy to update the
+	 * list and to replace the current one.
+	 */
+	INIT_LIST_HEAD(&iova_copy);
+	iova_copy_fail = !!vfio_iommu_get_iova_copy(iommu, &iova_copy);
+
 	list_for_each_entry(domain, &iommu->domain_list, next) {
 		group = find_iommu_group(domain, iommu_group);
 		if (!group)
@@ -1699,10 +1790,22 @@ static void vfio_iommu_type1_detach_group(void *iommu_data,
 			iommu_domain_free(domain->domain);
 			list_del(&domain->next);
 			kfree(domain);
+			if (!iova_copy_fail)
+				vfio_iommu_aper_expand(iommu, &iova_copy);
 		}
 		break;
 	}
 
+	if (!iova_copy_fail) {
+		if (!vfio_iommu_resv_refresh(iommu, &iova_copy)) {
+			/* Delete the current one and insert new iova list */
+			vfio_iommu_insert_iova_copy(iommu, &iova_copy);
+			goto detach_group_done;
+		}
+	}
+
+	list_for_each_entry_safe(iova, iova_next, &iova_copy, list)
+		kfree(iova);
 detach_group_done:
 	mutex_unlock(&iommu->lock);
 }
-- 
2.7.4

[PATCH v3 4/6] vfio/type1: check dma map request is within a valid iova range

[Shameer Kolothum]

This checks and rejects any dma map request outside valid iova
range.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/vfio/vfio_iommu_type1.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 8d8ddd7..dae01c5 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -970,6 +970,23 @@ static int vfio_pin_map_dma(struct vfio_iommu *iommu, struct vfio_dma *dma,
 	return ret;
 }
 
+/*
+ * Check dma map request is within a valid iova range
+ */
+static bool vfio_iommu_iova_dma_valid(struct vfio_iommu *iommu,
+				dma_addr_t start, dma_addr_t end)
+{
+	struct list_head *iova = &iommu->iova_list;
+	struct vfio_iova *node;
+
+	list_for_each_entry(node, iova, list) {
+		if ((start >= node->start) && (end <= node->end))
+			return true;
+	}
+
+	return false;
+}
+
 static int vfio_dma_do_map(struct vfio_iommu *iommu,
 			   struct vfio_iommu_type1_dma_map *map)
 {
@@ -1008,6 +1025,11 @@ static int vfio_dma_do_map(struct vfio_iommu *iommu,
 		goto out_unlock;
 	}
 
+	if (!vfio_iommu_iova_dma_valid(iommu, iova, iova + size - 1)) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
 	dma = kzalloc(sizeof(*dma), GFP_KERNEL);
 	if (!dma) {
 		ret = -ENOMEM;
-- 
2.7.4

[PATCH v3 5/6] vfio/type1: Add IOVA range capability support

[Shameer Kolothum]

This  allows the user-space to retrieve the supported IOVA
range(s), excluding any reserved regions. The implementation
is based on capability chains, added to VFIO_IOMMU_GET_INFO ioctl.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/vfio/vfio_iommu_type1.c | 92 +++++++++++++++++++++++++++++++++++++++++
 include/uapi/linux/vfio.h       | 23 +++++++++++
 2 files changed, 115 insertions(+)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index dae01c5..21e575c 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -1925,6 +1925,68 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
 	return ret;
 }
 
+static int vfio_add_iova_cap(struct vfio_info_cap *caps,
+		 struct vfio_iommu_type1_info_cap_iova_range *cap_iovas,
+		 size_t size)
+{
+	struct vfio_info_cap_header *header;
+	struct vfio_iommu_type1_info_cap_iova_range *iova_cap;
+
+	header = vfio_info_cap_add(caps, size,
+				VFIO_IOMMU_TYPE1_INFO_CAP_IOVA_RANGE, 1);
+	if (IS_ERR(header))
+		return PTR_ERR(header);
+
+	iova_cap = container_of(header,
+			struct vfio_iommu_type1_info_cap_iova_range, header);
+	iova_cap->nr_iovas = cap_iovas->nr_iovas;
+	memcpy(iova_cap->iova_ranges, cap_iovas->iova_ranges,
+		cap_iovas->nr_iovas * sizeof(*cap_iovas->iova_ranges));
+	return 0;
+}
+
+static int vfio_build_iommu_iova_caps(struct vfio_iommu *iommu,
+				struct vfio_info_cap *caps)
+{
+	struct vfio_iommu_type1_info_cap_iova_range *cap_iovas;
+	struct vfio_iova *iova;
+	size_t size;
+	int iovas = 0, i = 0, ret;
+
+	mutex_lock(&iommu->lock);
+
+	list_for_each_entry(iova, &iommu->iova_list, list)
+		iovas++;
+
+	if (!iovas) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
+	size = sizeof(*cap_iovas) + (iovas * sizeof(*cap_iovas->iova_ranges));
+
+	cap_iovas = kzalloc(size, GFP_KERNEL);
+	if (!cap_iovas) {
+		ret = -ENOMEM;
+		goto out_unlock;
+	}
+
+	cap_iovas->nr_iovas = iovas;
+
+	list_for_each_entry(iova, &iommu->iova_list, list) {
+		cap_iovas->iova_ranges[i].start = iova->start;
+		cap_iovas->iova_ranges[i].end = iova->end;
+		i++;
+	}
+
+	ret = vfio_add_iova_cap(caps, cap_iovas, size);
+
+	kfree(cap_iovas);
+out_unlock:
+	mutex_unlock(&iommu->lock);
+	return ret;
+}
+
 static long vfio_iommu_type1_ioctl(void *iommu_data,
 				   unsigned int cmd, unsigned long arg)
 {
@@ -1946,6 +2008,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
 		}
 	} else if (cmd == VFIO_IOMMU_GET_INFO) {
 		struct vfio_iommu_type1_info info;
+		struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
+		int ret;
 
 		minsz = offsetofend(struct vfio_iommu_type1_info, iova_pgsizes);
 
@@ -1959,6 +2023,34 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
 
 		info.iova_pgsizes = vfio_pgsize_bitmap(iommu);
 
+		if (info.argsz == minsz)
+			goto done;
+
+		ret = vfio_build_iommu_iova_caps(iommu, &caps);
+		if (ret)
+			return ret;
+
+		if (caps.size) {
+			info.flags |= VFIO_IOMMU_INFO_CAPS;
+			minsz = offsetofend(struct vfio_iommu_type1_info,
+							 cap_offset);
+			if (info.argsz < sizeof(info) + caps.size) {
+				info.argsz = sizeof(info) + caps.size;
+				info.cap_offset = 0;
+			} else {
+				vfio_info_cap_shift(&caps, sizeof(info));
+				if (copy_to_user((void __user *)arg +
+						sizeof(info), caps.buf,
+						caps.size)) {
+					kfree(caps.buf);
+					return -EFAULT;
+				}
+				info.cap_offset = sizeof(info);
+			}
+
+			kfree(caps.buf);
+		}
+done:
 		return copy_to_user((void __user *)arg, &info, minsz) ?
 			-EFAULT : 0;
 
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index c743721..46b49e9 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -589,7 +589,30 @@ struct vfio_iommu_type1_info {
 	__u32	argsz;
 	__u32	flags;
 #define VFIO_IOMMU_INFO_PGSIZES (1 << 0)	/* supported page sizes info */
+#define VFIO_IOMMU_INFO_CAPS	(1 << 1)	/* Info supports caps */
 	__u64	iova_pgsizes;		/* Bitmap of supported page sizes */
+	__u32   cap_offset;	/* Offset within info struct of first cap */
+};
+
+/*
+ * The IOVA capability allows to report the valid IOVA range(s)
+ * excluding any reserved regions associated with dev group. Any dma
+ * map attempt outside the valid iova range will return error.
+ *
+ * The structures below define version 1 of this capability.
+ */
+#define VFIO_IOMMU_TYPE1_INFO_CAP_IOVA_RANGE  1
+
+struct vfio_iova_range {
+	__u64	start;
+	__u64	end;
+};
+
+struct vfio_iommu_type1_info_cap_iova_range {
+	struct vfio_info_cap_header header;
+	__u32	nr_iovas;
+	__u32	reserved;
+	struct vfio_iova_range iova_ranges[];
 };
 
 #define VFIO_IOMMU_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
-- 
2.7.4

Re: [PATCH v3 5/6] vfio/type1: Add IOVA range capability support

[Alex Williamson]

On Thu, 15 Feb 2018 09:45:03 +0000
Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:

> This  allows the user-space to retrieve the supported IOVA
> range(s), excluding any reserved regions. The implementation
> is based on capability chains, added to VFIO_IOMMU_GET_INFO ioctl.
> 
> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>  drivers/vfio/vfio_iommu_type1.c | 92 +++++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/vfio.h       | 23 +++++++++++
>  2 files changed, 115 insertions(+)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index dae01c5..21e575c 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -1925,6 +1925,68 @@ static int vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
>  	return ret;
>  }
>  
> +static int vfio_add_iova_cap(struct vfio_info_cap *caps,
> +		 struct vfio_iommu_type1_info_cap_iova_range *cap_iovas,
> +		 size_t size)
> +{
> +	struct vfio_info_cap_header *header;
> +	struct vfio_iommu_type1_info_cap_iova_range *iova_cap;
> +
> +	header = vfio_info_cap_add(caps, size,
> +				VFIO_IOMMU_TYPE1_INFO_CAP_IOVA_RANGE, 1);
> +	if (IS_ERR(header))
> +		return PTR_ERR(header);
> +
> +	iova_cap = container_of(header,
> +			struct vfio_iommu_type1_info_cap_iova_range, header);
> +	iova_cap->nr_iovas = cap_iovas->nr_iovas;
> +	memcpy(iova_cap->iova_ranges, cap_iovas->iova_ranges,
> +		cap_iovas->nr_iovas * sizeof(*cap_iovas->iova_ranges));
> +	return 0;
> +}
> +
> +static int vfio_build_iommu_iova_caps(struct vfio_iommu *iommu,
> +				struct vfio_info_cap *caps)
> +{
> +	struct vfio_iommu_type1_info_cap_iova_range *cap_iovas;
> +	struct vfio_iova *iova;
> +	size_t size;
> +	int iovas = 0, i = 0, ret;
> +
> +	mutex_lock(&iommu->lock);
> +
> +	list_for_each_entry(iova, &iommu->iova_list, list)
> +		iovas++;
> +
> +	if (!iovas) {
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +
> +	size = sizeof(*cap_iovas) + (iovas * sizeof(*cap_iovas->iova_ranges));
> +
> +	cap_iovas = kzalloc(size, GFP_KERNEL);
> +	if (!cap_iovas) {
> +		ret = -ENOMEM;
> +		goto out_unlock;
> +	}
> +
> +	cap_iovas->nr_iovas = iovas;
> +
> +	list_for_each_entry(iova, &iommu->iova_list, list) {
> +		cap_iovas->iova_ranges[i].start = iova->start;
> +		cap_iovas->iova_ranges[i].end = iova->end;
> +		i++;
> +	}
> +
> +	ret = vfio_add_iova_cap(caps, cap_iovas, size);
> +
> +	kfree(cap_iovas);
> +out_unlock:
> +	mutex_unlock(&iommu->lock);
> +	return ret;
> +}
> +
>  static long vfio_iommu_type1_ioctl(void *iommu_data,
>  				   unsigned int cmd, unsigned long arg)
>  {
> @@ -1946,6 +2008,8 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>  		}
>  	} else if (cmd == VFIO_IOMMU_GET_INFO) {
>  		struct vfio_iommu_type1_info info;
> +		struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
> +		int ret;
>  
>  		minsz = offsetofend(struct vfio_iommu_type1_info, iova_pgsizes);
>  
> @@ -1959,6 +2023,34 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
>  
>  		info.iova_pgsizes = vfio_pgsize_bitmap(iommu);
>  
> +		if (info.argsz == minsz)
> +			goto done;

I don't think the above branch should exist, we want to tell the user
via argsz and flags that capabilities exist even if they only passed
the previous structure size through.

> +
> +		ret = vfio_build_iommu_iova_caps(iommu, &caps);
> +		if (ret)
> +			return ret;
> +
> +		if (caps.size) {
> +			info.flags |= VFIO_IOMMU_INFO_CAPS;
> +			minsz = offsetofend(struct vfio_iommu_type1_info,
> +							 cap_offset);

Only update minsz if this is within the provided argsz.

> +			if (info.argsz < sizeof(info) + caps.size) {
> +				info.argsz = sizeof(info) + caps.size;
> +				info.cap_offset = 0;

IOW, if cap_offset doesn't get copied to the user, that's ok, we've
provided them the flag and argsz they need to recognize it's there and
call with a sufficient buffer next time.

> +			} else {
> +				vfio_info_cap_shift(&caps, sizeof(info));
> +				if (copy_to_user((void __user *)arg +
> +						sizeof(info), caps.buf,
> +						caps.size)) {
> +					kfree(caps.buf);
> +					return -EFAULT;
> +				}
> +				info.cap_offset = sizeof(info);
> +			}
> +
> +			kfree(caps.buf);
> +		}
> +done:
>  		return copy_to_user((void __user *)arg, &info, minsz) ?
>  			-EFAULT : 0;
>  
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index c743721..46b49e9 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -589,7 +589,30 @@ struct vfio_iommu_type1_info {
>  	__u32	argsz;
>  	__u32	flags;
>  #define VFIO_IOMMU_INFO_PGSIZES (1 << 0)	/* supported page sizes info */
> +#define VFIO_IOMMU_INFO_CAPS	(1 << 1)	/* Info supports caps */
>  	__u64	iova_pgsizes;		/* Bitmap of supported page sizes */
> +	__u32   cap_offset;	/* Offset within info struct of first cap */
> +};
> +
> +/*
> + * The IOVA capability allows to report the valid IOVA range(s)
> + * excluding any reserved regions associated with dev group. Any dma
> + * map attempt outside the valid iova range will return error.
> + *
> + * The structures below define version 1 of this capability.
> + */
> +#define VFIO_IOMMU_TYPE1_INFO_CAP_IOVA_RANGE  1
> +
> +struct vfio_iova_range {
> +	__u64	start;
> +	__u64	end;
> +};
> +
> +struct vfio_iommu_type1_info_cap_iova_range {
> +	struct vfio_info_cap_header header;
> +	__u32	nr_iovas;
> +	__u32	reserved;
> +	struct vfio_iova_range iova_ranges[];
>  };
>  
>  #define VFIO_IOMMU_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)

RE: [PATCH v3 5/6] vfio/type1: Add IOVA range capability support

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Alex Williamson [mailto:alex.williamson@redhat.com]
> Sent: Friday, February 16, 2018 10:12 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: eric.auger@redhat.com; pmorel@linux.vnet.ibm.com;
> kvm@vger.kernel.org; linux-kernel@vger.kernel.org; Linuxarm
> <linuxarm@huawei.com>; John Garry <john.garry@huawei.com>; xuwei (O)
> <xuwei5@huawei.com>
> Subject: Re: [PATCH v3 5/6] vfio/type1: Add IOVA range capability support
> 
> On Thu, 15 Feb 2018 09:45:03 +0000
> Shameer Kolothum <shameerali.kolothum.thodi@huawei.com> wrote:
> 
> > This  allows the user-space to retrieve the supported IOVA
> > range(s), excluding any reserved regions. The implementation
> > is based on capability chains, added to VFIO_IOMMU_GET_INFO ioctl.
> >
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > ---
> >  drivers/vfio/vfio_iommu_type1.c | 92
> +++++++++++++++++++++++++++++++++++++++++
> >  include/uapi/linux/vfio.h       | 23 +++++++++++
> >  2 files changed, 115 insertions(+)
> >
> > diff --git a/drivers/vfio/vfio_iommu_type1.c
> b/drivers/vfio/vfio_iommu_type1.c
> > index dae01c5..21e575c 100644
> > --- a/drivers/vfio/vfio_iommu_type1.c
> > +++ b/drivers/vfio/vfio_iommu_type1.c
> > @@ -1925,6 +1925,68 @@ static int
> vfio_domains_have_iommu_cache(struct vfio_iommu *iommu)
> >  	return ret;
> >  }
> >
> > +static int vfio_add_iova_cap(struct vfio_info_cap *caps,
> > +		 struct vfio_iommu_type1_info_cap_iova_range *cap_iovas,
> > +		 size_t size)
> > +{
> > +	struct vfio_info_cap_header *header;
> > +	struct vfio_iommu_type1_info_cap_iova_range *iova_cap;
> > +
> > +	header = vfio_info_cap_add(caps, size,
> > +
> 	VFIO_IOMMU_TYPE1_INFO_CAP_IOVA_RANGE, 1);
> > +	if (IS_ERR(header))
> > +		return PTR_ERR(header);
> > +
> > +	iova_cap = container_of(header,
> > +			struct vfio_iommu_type1_info_cap_iova_range,
> header);
> > +	iova_cap->nr_iovas = cap_iovas->nr_iovas;
> > +	memcpy(iova_cap->iova_ranges, cap_iovas->iova_ranges,
> > +		cap_iovas->nr_iovas * sizeof(*cap_iovas->iova_ranges));
> > +	return 0;
> > +}
> > +
> > +static int vfio_build_iommu_iova_caps(struct vfio_iommu *iommu,
> > +				struct vfio_info_cap *caps)
> > +{
> > +	struct vfio_iommu_type1_info_cap_iova_range *cap_iovas;
> > +	struct vfio_iova *iova;
> > +	size_t size;
> > +	int iovas = 0, i = 0, ret;
> > +
> > +	mutex_lock(&iommu->lock);
> > +
> > +	list_for_each_entry(iova, &iommu->iova_list, list)
> > +		iovas++;
> > +
> > +	if (!iovas) {
> > +		ret = -EINVAL;
> > +		goto out_unlock;
> > +	}
> > +
> > +	size = sizeof(*cap_iovas) + (iovas * sizeof(*cap_iovas->iova_ranges));
> > +
> > +	cap_iovas = kzalloc(size, GFP_KERNEL);
> > +	if (!cap_iovas) {
> > +		ret = -ENOMEM;
> > +		goto out_unlock;
> > +	}
> > +
> > +	cap_iovas->nr_iovas = iovas;
> > +
> > +	list_for_each_entry(iova, &iommu->iova_list, list) {
> > +		cap_iovas->iova_ranges[i].start = iova->start;
> > +		cap_iovas->iova_ranges[i].end = iova->end;
> > +		i++;
> > +	}
> > +
> > +	ret = vfio_add_iova_cap(caps, cap_iovas, size);
> > +
> > +	kfree(cap_iovas);
> > +out_unlock:
> > +	mutex_unlock(&iommu->lock);
> > +	return ret;
> > +}
> > +
> >  static long vfio_iommu_type1_ioctl(void *iommu_data,
> >  				   unsigned int cmd, unsigned long arg)
> >  {
> > @@ -1946,6 +2008,8 @@ static long vfio_iommu_type1_ioctl(void
> *iommu_data,
> >  		}
> >  	} else if (cmd == VFIO_IOMMU_GET_INFO) {
> >  		struct vfio_iommu_type1_info info;
> > +		struct vfio_info_cap caps = { .buf = NULL, .size = 0 };
> > +		int ret;
> >
> >  		minsz = offsetofend(struct vfio_iommu_type1_info,
> iova_pgsizes);
> >
> > @@ -1959,6 +2023,34 @@ static long vfio_iommu_type1_ioctl(void
> *iommu_data,
> >
> >  		info.iova_pgsizes = vfio_pgsize_bitmap(iommu);
> >
> > +		if (info.argsz == minsz)
> > +			goto done;
> 
> I don't think the above branch should exist, we want to tell the user
> via argsz and flags that capabilities exist even if they only passed
> the previous structure size through.

Ok. I will remove this.
 
> > +
> > +		ret = vfio_build_iommu_iova_caps(iommu, &caps);
> > +		if (ret)
> > +			return ret;
> > +
> > +		if (caps.size) {
> > +			info.flags |= VFIO_IOMMU_INFO_CAPS;
> > +			minsz = offsetofend(struct vfio_iommu_type1_info,
> > +							 cap_offset);
> 
> Only update minsz if this is within the provided argsz.

Ok.

> > +			if (info.argsz < sizeof(info) + caps.size) {
> > +				info.argsz = sizeof(info) + caps.size;
> > +				info.cap_offset = 0;
> 
> IOW, if cap_offset doesn't get copied to the user, that's ok, we've
> provided them the flag and argsz they need to recognize it's there and
> call with a sufficient buffer next time.

Ok. I will change the logic here.

Thanks,
Shameer

> > +			} else {
> > +				vfio_info_cap_shift(&caps, sizeof(info));
> > +				if (copy_to_user((void __user *)arg +
> > +						sizeof(info), caps.buf,
> > +						caps.size)) {
> > +					kfree(caps.buf);
> > +					return -EFAULT;
> > +				}
> > +				info.cap_offset = sizeof(info);
> > +			}
> > +
> > +			kfree(caps.buf);
> > +		}
> > +done:
> >  		return copy_to_user((void __user *)arg, &info, minsz) ?
> >  			-EFAULT : 0;
> >
> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> > index c743721..46b49e9 100644
> > --- a/include/uapi/linux/vfio.h
> > +++ b/include/uapi/linux/vfio.h
> > @@ -589,7 +589,30 @@ struct vfio_iommu_type1_info {
> >  	__u32	argsz;
> >  	__u32	flags;
> >  #define VFIO_IOMMU_INFO_PGSIZES (1 << 0)	/* supported page sizes info */
> > +#define VFIO_IOMMU_INFO_CAPS	(1 << 1)	/* Info supports caps */
> >  	__u64	iova_pgsizes;		/* Bitmap of supported page sizes */
> > +	__u32   cap_offset;	/* Offset within info struct of first cap */
> > +};
> > +
> > +/*
> > + * The IOVA capability allows to report the valid IOVA range(s)
> > + * excluding any reserved regions associated with dev group. Any dma
> > + * map attempt outside the valid iova range will return error.
> > + *
> > + * The structures below define version 1 of this capability.
> > + */
> > +#define VFIO_IOMMU_TYPE1_INFO_CAP_IOVA_RANGE  1
> > +
> > +struct vfio_iova_range {
> > +	__u64	start;
> > +	__u64	end;
> > +};
> > +
> > +struct vfio_iommu_type1_info_cap_iova_range {
> > +	struct vfio_info_cap_header header;
> > +	__u32	nr_iovas;
> > +	__u32	reserved;
> > +	struct vfio_iova_range iova_ranges[];
> >  };
> >
> >  #define VFIO_IOMMU_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)

[PATCH v3 6/6] vfio/type1: remove duplicate retrieval of reserved regions.

[Shameer Kolothum]

As we now already have the reserved regions list, just pass that into
vfio_iommu_has_sw_msi() fn.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 drivers/vfio/vfio_iommu_type1.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
index 21e575c..ff9818b 100644
--- a/drivers/vfio/vfio_iommu_type1.c
+++ b/drivers/vfio/vfio_iommu_type1.c
@@ -1192,15 +1192,13 @@ static struct vfio_group *find_iommu_group(struct vfio_domain *domain,
 	return NULL;
 }
 
-static bool vfio_iommu_has_sw_msi(struct iommu_group *group, phys_addr_t *base)
+static bool vfio_iommu_has_sw_msi(struct list_head *group_resv_regions,
+						phys_addr_t *base)
 {
-	struct list_head group_resv_regions;
-	struct iommu_resv_region *region, *next;
+	struct iommu_resv_region *region;
 	bool ret = false;
 
-	INIT_LIST_HEAD(&group_resv_regions);
-	iommu_get_group_resv_regions(group, &group_resv_regions);
-	list_for_each_entry(region, &group_resv_regions, list) {
+	list_for_each_entry(region, group_resv_regions, list) {
 		/*
 		 * The presence of any 'real' MSI regions should take
 		 * precedence over the software-managed one if the
@@ -1216,8 +1214,7 @@ static bool vfio_iommu_has_sw_msi(struct iommu_group *group, phys_addr_t *base)
 			ret = true;
 		}
 	}
-	list_for_each_entry_safe(region, next, &group_resv_regions, list)
-		kfree(region);
+
 	return ret;
 }
 
@@ -1538,7 +1535,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data,
 	if (ret)
 		goto out_detach;
 
-	resv_msi = vfio_iommu_has_sw_msi(iommu_group, &resv_msi_base);
+	resv_msi = vfio_iommu_has_sw_msi(&group_resv_regions, &resv_msi_base);
 
 	INIT_LIST_HEAD(&domain->group_list);
 	list_add(&group->next, &domain->group_list);
-- 
2.7.4