RE: [RFC v2 2/5] vfio/type1: Check reserve region conflict and update iova list

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Auger Eric [mailto:eric.auger@xxxxxxxxxx]
> Sent: Tuesday, January 23, 2018 12:52 PM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>;
> Alex Williamson <alex.williamson@xxxxxxxxxx>
> Cc: pmorel@xxxxxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; Linuxarm <linuxarm@xxxxxxxxxx>; John Garry
> <john.garry@xxxxxxxxxx>; xuwei (O) <xuwei5@xxxxxxxxxx>
> Subject: Re: [RFC v2 2/5] vfio/type1: Check reserve region conflict and update
> iova list
> 
> Hi Shameer,
> 
> On 23/01/18 13:16, Shameerali Kolothum Thodi wrote:
> > Hi Eric,
> >
> >> -----Original Message-----
> >> From: Auger Eric [mailto:eric.auger@xxxxxxxxxx]
> >> Sent: Tuesday, January 23, 2018 8:32 AM
> >> To: Alex Williamson <alex.williamson@xxxxxxxxxx>; Shameerali Kolothum
> >> Thodi <shameerali.kolothum.thodi@xxxxxxxxxx>
> >> Cc: pmorel@xxxxxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx; linux-
> >> kernel@xxxxxxxxxxxxxxx; Linuxarm <linuxarm@xxxxxxxxxx>; John Garry
> >> <john.garry@xxxxxxxxxx>; xuwei (O) <xuwei5@xxxxxxxxxx>
> >> Subject: Re: [RFC v2 2/5] vfio/type1: Check reserve region conflict and
> update
> >> iova list
> >>
> >> Hi Shameer,
> >>
> >> On 18/01/18 01:04, Alex Williamson wrote:
> >>> On Fri, 12 Jan 2018 16:45:28 +0000
> >>> Shameer Kolothum <shameerali.kolothum.thodi@xxxxxxxxxx> 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@xxxxxxxxxx>
> >>>> ---
> >>>>  drivers/vfio/vfio_iommu_type1.c | 161
> >> +++++++++++++++++++++++++++++++++++++++-
> >>>>  1 file changed, 159 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/vfio/vfio_iommu_type1.c
> >> b/drivers/vfio/vfio_iommu_type1.c
> >>>> index 11cbd49..7609070 100644
> >>>> --- a/drivers/vfio/vfio_iommu_type1.c
> >>>> +++ b/drivers/vfio/vfio_iommu_type1.c
> >>>> @@ -28,6 +28,7 @@
> >>>>  #include <linux/device.h>
> >>>>  #include <linux/fs.h>
> >>>>  #include <linux/iommu.h>
> >>>> +#include <linux/list_sort.h>
> >>>>  #include <linux/module.h>
> >>>>  #include <linux/mm.h>
> >>>>  #include <linux/rbtree.h>
> >>>> @@ -1199,6 +1200,20 @@ static bool vfio_iommu_has_sw_msi(struct
> >> iommu_group *group, phys_addr_t *base)
> >>>>  	return ret;
> >>>>  }
> >>>>
> >>>
> >>> /* list_sort helper */
> >>>
> >>>> +static int vfio_resv_cmp(void *priv, struct list_head *a, struct list_head
> *b)
> >>>> +{
> >>>> +	struct iommu_resv_region *ra, *rb;
> >>>> +
> >>>> +	ra = container_of(a, struct iommu_resv_region, list);
> >>>> +	rb = container_of(b, struct iommu_resv_region, list);
> >>>> +
> >>>> +	if (ra->start < rb->start)
> >>>> +		return -1;
> >>>> +	if (ra->start > rb->start)
> >>>> +		return 1;
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>>  static int vfio_insert_iova(phys_addr_t start, phys_addr_t end,
> >>>>  				struct list_head *head)
> >>>>  {
> >>>> @@ -1274,6 +1289,24 @@ static int vfio_iommu_valid_aperture(struct
> >> vfio_iommu *iommu,
> >>>>  }
> >>>>
> >>>>  /*
> >>>> + * Check reserved region conflicts with existing dma mappings
> >>>> + */
> >>>> +static int vfio_iommu_resv_region_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_overlap(iommu, region->start,
> >>>> +				    region->start + region->length - 1))
> >>>> +			return -EINVAL;
> >>>> +	}
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>
> >>> This basically does the same test as vfio_iommu_valid_aperture but
> >>> properly names it a conflict test.  Please be consistent.  Should this
> >>> also return bool, "conflict" is a yes/no answer.
> >>>
> >>>> +
> >>>> +/*
> >>>>   * Adjust the iommu aperture window if new aperture is a valid one
> >>>>   */
> >>>>  static int vfio_iommu_iova_aper_adjust(struct vfio_iommu *iommu,
> >>>> @@ -1316,6 +1349,51 @@ static int vfio_iommu_iova_aper_adjust(struct
> >> vfio_iommu *iommu,
> >>>>  	return 0;
> >>>>  }
> >>>>
> >>>> +/*
> >>>> + * Check and update iova region list in case a reserved region
> >>>> + * overlaps the iommu iova range
> >>>> + */
> >>>> +static int vfio_iommu_iova_resv_adjust(struct vfio_iommu *iommu,
> >>>> +					struct list_head *resv_regions)
> >>>
> >>> "resv_region" in previous function, just "resv" here, use consistent
> >>> names.  Also, what are we adjusting.  Maybe "exclude" is a better term.
> >>>
> >>>> +{
> >>>> +	struct iommu_resv_region *resv;
> >>>> +	struct list_head *iova = &iommu->iova_list;
> >>>> +	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) {
> >>>> +			phys_addr_t a, b;
> >>>> +			int ret = 0;
> >>>> +
> >>>> +			a = n->start;
> >>>> +			b = n->end;
> >>>
> >>> 'a' and 'b' variables actually make this incredibly confusing.  Use
> >>> better variable names or just drop them entirely, it's much easier to
> >>> follow as n->start & n->end.
> >>>
> >>>> +			/* No overlap */
> >>>> +			if ((start > b) || (end < a))
> >>>> +				continue;
> >>>> +			/* Split the current node and create holes */
> >>>> +			if (start > a)
> >>>> +				ret = vfio_insert_iova(a, start - 1, &n->list);
> >>>> +			if (!ret && end < b)
> >>>> +				ret = vfio_insert_iova(end + 1, b, &n->list);
> >>>> +			if (ret)
> >>>> +				return ret;
> >>>> +
> >>>> +			list_del(&n->list);
> >>>
> >>> This is trickier than it appears and deserves some explanation.  AIUI,
> >>> we're actually inserting duplicate entries for the remainder at the
> >>> start of the range and then at the end of the range (and the order is
> >>> important here because we're inserting each before the current node),
> >>> and then we delete the current node.  So the iova_list is kept sorted
> >>> through this process, though temporarily includes some bogus, unordered
> >>> sub-sets.
> >>>
> >>>> +			kfree(n);
> >>>> +		}
> >>>> +	}
> >>>> +
> >>>> +	if (list_empty(iova))
> >>>> +		return -EINVAL;
> >>>> +
> >>>> +	return 0;
> >>>> +}
> >>>> +
> >>>>  static int vfio_iommu_type1_attach_group(void *iommu_data,
> >>>>  					 struct iommu_group *iommu_group)
> >>>>  {
> >>>> @@ -1327,6 +1405,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 group_resv_regions;
> >>>> +	struct iommu_resv_region *resv, *resv_next;
> >>>>
> >>>>  	mutex_lock(&iommu->lock);
> >>>>
> >>>> @@ -1404,6 +1484,14 @@ static int
> vfio_iommu_type1_attach_group(void
> >> *iommu_data,
> >>>>  	if (ret)
> >>>>  		goto out_detach;
> >>>>
> >>>> +	INIT_LIST_HEAD(&group_resv_regions);
> >>>> +	iommu_get_group_resv_regions(iommu_group, &group_resv_regions);
> >>>> +	list_sort(NULL, &group_resv_regions, vfio_resv_cmp);
> >> iommu_get_group_resv_regions returns a sorted list (see
> >> iommu_insert_resv_regions kerneldoc comment). You can have overlapping
> >> regions of different types though.
> >
> > Hmm..I am not sure. It looks like it is sorted only if the regions are of same
> type.
> >
> > "* The new element is sorted by address with respect to the other
> >  * regions of the same type."
> >
> > So hypothetically if there are two groups with regions like,
> >
> > Group 1.
> >  Start       size            type
> >   0x0000   0x1000        1
> >   0x2000   0x1000        1
> >   0x5000   0x1000        1
> >
> > Group 2
> >   Start       size              type
> >    0x2000  0x4000           2
> >    0x7000   0x1000          1
> >
> > Then the  iommu_get_group_resv_regions() will return,
> >
> > 0x0000   0x1000        1
> > 0x2000   0x1000        1
> > 0x5000   0x1000        1
> > 0x2000  0x4000         2
> > 0x7000   0x1000        1
> 
> Hum yes, I remember now, sorry. It was made on purpose to avoid to
> display interleaved resv region types in
> /sys/kernel/iommu_groups/reserved_regions. I think it gives a better
> user experience.

Ok. However, I have a feeling that sorting may not be required in this 
patch. I will double check the logic in vfio_iommu_iova_resv_adjust() and if
possible will remove the sorting.

Thanks,
Shameer

> Thanks
> 
> Eric
> >
> > But honestly I am not sure the above is a valid scenario or not. I am
> > happy to remove the sorting if such a case will never happen.
> >
> > Please let me know.
> >
> > Thanks,
> > Shameer
> >
> >> Eric
> >>>> +
> >>>> +	ret = vfio_iommu_resv_region_conflict(iommu, &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);
> >>>> @@ -1434,11 +1522,15 @@ static int
> vfio_iommu_type1_attach_group(void
> >> *iommu_data,
> >>>>  		    d->prot == domain->prot) {
> >>>>  			iommu_detach_group(domain->domain,
> >> iommu_group);
> >>>>  			if (!iommu_attach_group(d->domain, iommu_group)) {
> >>>> +				ret = vfio_iommu_iova_resv_adjust(iommu,
> >>>> +
> >> 	&group_resv_regions);
> >>>> +				if (!ret)
> >>>> +					goto out_domain;
> >>>
> >>> The above function is not without side effects if it fails, it's
> >>> altered the iova_list.  It needs to be valid for the remaining domains
> >>> if we're going to continue.
> >>>
> >>>> +
> >>>>  				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);
> >>>> @@ -1465,8 +1557,15 @@ static int
> vfio_iommu_type1_attach_group(void
> >> *iommu_data,
> >>>>  	if (ret)
> >>>>  		goto out_detach;
> >>>>
> >>>> +	ret = vfio_iommu_iova_resv_adjust(iommu, &group_resv_regions);
> >>>> +	if (ret)
> >>>> +		goto out_detach;
> >>>
> >>> Can't we process the reserved regions once before we get here rather
> >>> than have two separate call points that do the same thing?  In order to
> >>> roll back from errors above, it seems like we need to copy iova_list
> >>> and work on the copy, installing it and deleting the original only on
> >>> success.
> >>>
> >>>> +
> >>>>  	list_add(&domain->next, &iommu->domain_list);
> >>>>
> >>>> +done:
> >>>> +	list_for_each_entry_safe(resv, resv_next, &group_resv_regions, list)
> >>>> +		kfree(resv);
> >>>>  	mutex_unlock(&iommu->lock);
> >>>>
> >>>>  	return 0;
> >>>> @@ -1475,6 +1574,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(resv, resv_next, &group_resv_regions, list)
> >>>> +		kfree(resv);
> >>>>  out_free:
> >>>>  	kfree(domain);
> >>>>  	kfree(group);
> >>>> @@ -1559,6 +1660,60 @@ static void
> vfio_iommu_iova_aper_refresh(struct
> >> vfio_iommu *iommu)
> >>>>  	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 void vfio_iommu_iova_resv_refresh(struct vfio_iommu *iommu)
> >>>> +{
> >>>> +	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;
> >>>> +
> >>>> +	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;
> >>>> +
> >>>> +	list_sort(NULL, &resv_regions, vfio_resv_cmp);
> >>>> +
> >>>> +	node = list_first_entry(&iommu->iova_list, struct vfio_iova, list);
> >>>> +	start = node->start;
> >>>> +	node = list_last_entry(&iommu->iova_list, struct vfio_iova, list);
> >>>> +	end = node->end;
> >>>
> >>> list_sort() only sorts based on ->start, we added reserved regions for
> >>> all our groups to one list, we potentially have multiple entries with
> >>> the same ->start.  How can we be sure that the last one in the list
> >>> actually has the largest ->end value?
> >>>
> >>>> +
> >>>> +	/* purge the iova list and create new one */
> >>>> +	list_for_each_entry_safe(node, tmp, &iommu->iova_list, list) {
> >>>> +		list_del(&node->list);
> >>>> +		kfree(node);
> >>>> +	}
> >>>> +
> >>>> +	if (vfio_iommu_iova_aper_adjust(iommu, start, end)) {
> >>>> +		pr_warn("%s: Failed to update iova aperture. VFIO DMA map
> >> request may fail\n",
> >>>> +			__func__);
> >>>
> >>> Map requests "will" fail.  Is this the right error strategy?  Detaching
> >>> a group cannot fail.  Aren't we better off leaving the iova_list we had
> >>> in place?  If we cannot expand the iova aperture when a group is
> >>> removed, a user can continue unscathed.
> >>>
> >>>> +		goto done;
> >>>> +	}
> >>>> +
> >>>> +	/* adjust the iova with current reserved regions */
> >>>> +	if (vfio_iommu_iova_resv_adjust(iommu, &resv_regions))
> >>>> +		pr_warn("%s: Failed to update iova list with reserve regions.
> >> VFIO DMA map request may fail\n",
> >>>> +			__func__);
> >>>
> >>> Same.
> >>>
> >>>> +done:
> >>>> +	list_for_each_entry_safe(resv, resv_next, &resv_regions, list)
> >>>> +		kfree(resv);
> >>>> +}
> >>>> +
> >>>>  static void vfio_iommu_type1_detach_group(void *iommu_data,
> >>>>  					  struct iommu_group *iommu_group)
> >>>>  {
> >>>> @@ -1617,6 +1772,8 @@ static void
> vfio_iommu_type1_detach_group(void
> >> *iommu_data,
> >>>>  		break;
> >>>>  	}
> >>>>
> >>>> +	vfio_iommu_iova_resv_refresh(iommu);
> >>>> +
> >>>>  detach_group_done:
> >>>>  	mutex_unlock(&iommu->lock);
> >>>>  }
> >>>