| Message ID | 20180315163509.17740-3-shameerali.kolothum.thodi@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> From: Shameer Kolothum > Sent: Friday, March 16, 2018 12:35 AM > > 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 | 90 > +++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 90 insertions(+) > > diff --git a/drivers/vfio/vfio_iommu_type1.c > b/drivers/vfio/vfio_iommu_type1.c > index 1123c74..cfe2bb2 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -1313,6 +1313,82 @@ 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_iommu_iova_insert(&n->list, > + n->start, start - 1); > + if (!ret && end < n->end) > + ret = vfio_iommu_iova_insert(&n->list, > + end + 1, n->end); > + if (ret) > + return ret; Is it safer to delete the 1st node here in case of failure of the 2nd node? There is no problem with current logic since upon error iova_copy will be released anyway. However this function alone doesn't assume the fact of a temporary list, thus it's better to keep the list clean w/o garbage left from any error handling. > + > + list_del(&n->list); > + kfree(n); > + } > + } > + > + if (list_empty(iova)) > + return -EINVAL; if list_empty should BUG_ON here? or do we really need this check? > + > + return 0; > +} > + > +static void vfio_iommu_resv_free(struct list_head *resv_regions) > +{ > + struct iommu_resv_region *n, *next; > + > + list_for_each_entry_safe(n, next, resv_regions, list) { > + list_del(&n->list); > + kfree(n); > + } > +} > + > static void vfio_iommu_iova_free(struct list_head *iova) > { > struct vfio_iova *n, *next; > @@ -1366,6 +1442,7 @@ static int vfio_iommu_type1_attach_group(void > *iommu_data, > phys_addr_t resv_msi_base; > struct iommu_domain_geometry geo; > LIST_HEAD(iova_copy); > + LIST_HEAD(group_resv_regions); > > mutex_lock(&iommu->lock); > > @@ -1444,6 +1521,13 @@ static int vfio_iommu_type1_attach_group(void > *iommu_data, > goto out_detach; > } > > + 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 */ > ret = vfio_iommu_iova_get_copy(iommu, &iova_copy); > if (ret) > @@ -1454,6 +1538,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); > @@ -1514,6 +1602,7 @@ static int vfio_iommu_type1_attach_group(void > *iommu_data, > /* Delete the old one and insert new iova list */ > vfio_iommu_iova_insert_copy(iommu, &iova_copy); > mutex_unlock(&iommu->lock); > + vfio_iommu_resv_free(&group_resv_regions); > > return 0; > > @@ -1522,6 +1611,7 @@ static int vfio_iommu_type1_attach_group(void > *iommu_data, > out_domain: > iommu_domain_free(domain->domain); > vfio_iommu_iova_free(&iova_copy); > + vfio_iommu_resv_free(&group_resv_regions); > out_free: > kfree(domain); > kfree(group); > -- > 2.7.4 > > > _______________________________________________ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu
> -----Original Message----- > From: Tian, Kevin [mailto:kevin.tian@intel.com] > Sent: Monday, March 19, 2018 7:52 AM > To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; > alex.williamson@redhat.com; eric.auger@redhat.com; > pmorel@linux.vnet.ibm.com > Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; xuwei (O) > <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>; > iommu@lists.linux-foundation.org > Subject: RE: [PATCH v5 2/7] vfio/type1: Check reserve region conflict and > update iova list > > > From: Shameer Kolothum > > Sent: Friday, March 16, 2018 12:35 AM > > > > 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 | 90 > > +++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 90 insertions(+) > > > > diff --git a/drivers/vfio/vfio_iommu_type1.c > > b/drivers/vfio/vfio_iommu_type1.c > > index 1123c74..cfe2bb2 100644 > > --- a/drivers/vfio/vfio_iommu_type1.c > > +++ b/drivers/vfio/vfio_iommu_type1.c > > @@ -1313,6 +1313,82 @@ 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_iommu_iova_insert(&n->list, > > + n->start, start - 1); > > + if (!ret && end < n->end) > > + ret = vfio_iommu_iova_insert(&n->list, > > + end + 1, n->end); > > + if (ret) > > + return ret; > > Is it safer to delete the 1st node here in case of failure of the 2nd node? > There is no problem with current logic since upon error iova_copy will > be released anyway. However this function alone doesn't assume the > fact of a temporary list, thus it's better to keep the list clean w/o garbage > left from any error handling. Agree. I will consider this. > > + > > + list_del(&n->list); > > + kfree(n); > > + } > > + } > > + > > + if (list_empty(iova)) > > + return -EINVAL; > > if list_empty should BUG_ON here? or do we really need this check? I think we need the check here. This basically means there is no valid iova region as the reserved regions overlaps it completely(very unlikely, a bad configuration maybe). The __attach will fail if that happens and may be WARN_ON is a good idea to notify the user. Thanks, Shameer > > + > > + return 0; > > +} > > + > > +static void vfio_iommu_resv_free(struct list_head *resv_regions) > > +{ > > + struct iommu_resv_region *n, *next; > > + > > + list_for_each_entry_safe(n, next, resv_regions, list) { > > + list_del(&n->list); > > + kfree(n); > > + } > > +} > > + > > static void vfio_iommu_iova_free(struct list_head *iova) > > { > > struct vfio_iova *n, *next; > > @@ -1366,6 +1442,7 @@ static int vfio_iommu_type1_attach_group(void > > *iommu_data, > > phys_addr_t resv_msi_base; > > struct iommu_domain_geometry geo; > > LIST_HEAD(iova_copy); > > + LIST_HEAD(group_resv_regions); > > > > mutex_lock(&iommu->lock); > > > > @@ -1444,6 +1521,13 @@ static int vfio_iommu_type1_attach_group(void > > *iommu_data, > > goto out_detach; > > } > > > > + 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 */ > > ret = vfio_iommu_iova_get_copy(iommu, &iova_copy); > > if (ret) > > @@ -1454,6 +1538,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); > > @@ -1514,6 +1602,7 @@ static int vfio_iommu_type1_attach_group(void > > *iommu_data, > > /* Delete the old one and insert new iova list */ > > vfio_iommu_iova_insert_copy(iommu, &iova_copy); > > mutex_unlock(&iommu->lock); > > + vfio_iommu_resv_free(&group_resv_regions); > > > > return 0; > > > > @@ -1522,6 +1611,7 @@ static int vfio_iommu_type1_attach_group(void > > *iommu_data, > > out_domain: > > iommu_domain_free(domain->domain); > > vfio_iommu_iova_free(&iova_copy); > > + vfio_iommu_resv_free(&group_resv_regions); > > out_free: > > kfree(domain); > > kfree(group); > > -- > > 2.7.4 > > > > > > _______________________________________________ > > iommu mailing list > > iommu@lists.linux-foundation.org > > https://lists.linuxfoundation.org/mailman/listinfo/iommu
> From: Shameerali Kolothum Thodi > [mailto:shameerali.kolothum.thodi@huawei.com] > > > -----Original Message----- > > From: Tian, Kevin [mailto:kevin.tian@intel.com] > > Sent: Monday, March 19, 2018 7:52 AM > > To: Shameerali Kolothum Thodi > <shameerali.kolothum.thodi@huawei.com>; > > alex.williamson@redhat.com; eric.auger@redhat.com; > > pmorel@linux.vnet.ibm.com > > Cc: kvm@vger.kernel.org; linux-kernel@vger.kernel.org; xuwei (O) > > <xuwei5@huawei.com>; Linuxarm <linuxarm@huawei.com>; > > iommu@lists.linux-foundation.org > > Subject: RE: [PATCH v5 2/7] vfio/type1: Check reserve region conflict and > > update iova list > > > > > From: Shameer Kolothum > > > Sent: Friday, March 16, 2018 12:35 AM > > > > > > 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 | 90 > > > +++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 90 insertions(+) > > > > > > diff --git a/drivers/vfio/vfio_iommu_type1.c > > > b/drivers/vfio/vfio_iommu_type1.c > > > index 1123c74..cfe2bb2 100644 > > > --- a/drivers/vfio/vfio_iommu_type1.c > > > +++ b/drivers/vfio/vfio_iommu_type1.c > > > @@ -1313,6 +1313,82 @@ 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_iommu_iova_insert(&n->list, > > > + n->start, start - 1); > > > + if (!ret && end < n->end) > > > + ret = vfio_iommu_iova_insert(&n->list, > > > + end + 1, n->end); > > > + if (ret) > > > + return ret; > > > > Is it safer to delete the 1st node here in case of failure of the 2nd node? > > There is no problem with current logic since upon error iova_copy will > > be released anyway. However this function alone doesn't assume the > > fact of a temporary list, thus it's better to keep the list clean w/o garbage > > left from any error handling. > > Agree. I will consider this. > > > > + > > > + list_del(&n->list); > > > + kfree(n); > > > + } > > > + } > > > + > > > + if (list_empty(iova)) > > > + return -EINVAL; > > > > if list_empty should BUG_ON here? or do we really need this check? > > I think we need the check here. This basically means there is no valid iova > region as the reserved regions overlaps it completely(very unlikely, a bad > configuration maybe). The __attach will fail if that happens and may be > WARN_ON is a good idea to notify the user. > you are right. I misread the code that deletion happens only after insertion... Thanks Kevin
On Mon, 19 Mar 2018 07:51:58 +0000 "Tian, Kevin" <kevin.tian@intel.com> wrote: > > From: Shameer Kolothum > > Sent: Friday, March 16, 2018 12:35 AM > > > > 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 | 90 > > +++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 90 insertions(+) > > > > diff --git a/drivers/vfio/vfio_iommu_type1.c > > b/drivers/vfio/vfio_iommu_type1.c > > index 1123c74..cfe2bb2 100644 > > --- a/drivers/vfio/vfio_iommu_type1.c > > +++ b/drivers/vfio/vfio_iommu_type1.c > > @@ -1313,6 +1313,82 @@ 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_iommu_iova_insert(&n->list, > > + n->start, start - 1); > > + if (!ret && end < n->end) > > + ret = vfio_iommu_iova_insert(&n->list, > > + end + 1, n->end); > > + if (ret) > > + return ret; > > Is it safer to delete the 1st node here in case of failure of the 2nd node? > There is no problem with current logic since upon error iova_copy will > be released anyway. However this function alone doesn't assume the > fact of a temporary list, thus it's better to keep the list clean w/o garbage > left from any error handling. I don't think the proposal makes the list notably more sane on failure than we have here. If the function returns an error and the list is modified in any way, how can the caller recover? We're operating on a principle of modify a copy and throw it away on error, the only function level solution to the problem you're noting is to make each function generate a working copy, which is clearly inefficient. This is a static function, not intended for general use, so I think a sufficient approach to address your concern is to simply note the error behavior in the comment above the function, the list is in an unknown/inconsistent state on error. Thanks, Alex
> From: Alex Williamson [mailto:alex.williamson@redhat.com] > Sent: Wednesday, March 21, 2018 6:38 AM > > On Mon, 19 Mar 2018 07:51:58 +0000 > "Tian, Kevin" <kevin.tian@intel.com> wrote: > > > > From: Shameer Kolothum > > > Sent: Friday, March 16, 2018 12:35 AM > > > > > > 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 | 90 > > > +++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 90 insertions(+) > > > > > > diff --git a/drivers/vfio/vfio_iommu_type1.c > > > b/drivers/vfio/vfio_iommu_type1.c > > > index 1123c74..cfe2bb2 100644 > > > --- a/drivers/vfio/vfio_iommu_type1.c > > > +++ b/drivers/vfio/vfio_iommu_type1.c > > > @@ -1313,6 +1313,82 @@ 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_iommu_iova_insert(&n->list, > > > + n->start, start - 1); > > > + if (!ret && end < n->end) > > > + ret = vfio_iommu_iova_insert(&n->list, > > > + end + 1, n->end); > > > + if (ret) > > > + return ret; > > > > Is it safer to delete the 1st node here in case of failure of the 2nd node? > > There is no problem with current logic since upon error iova_copy will > > be released anyway. However this function alone doesn't assume the > > fact of a temporary list, thus it's better to keep the list clean w/o garbage > > left from any error handling. > > I don't think the proposal makes the list notably more sane on failure > than we have here. If the function returns an error and the list is > modified in any way, how can the caller recover? We're operating on a > principle of modify a copy and throw it away on error, the only > function level solution to the problem you're noting is to make each > function generate a working copy, which is clearly inefficient. This > is a static function, not intended for general use, so I think a > sufficient approach to address your concern is to simply note the error > behavior in the comment above the function, the list is in an > unknown/inconsistent state on error. Thanks, > 'static' doesn't mean it cannot be used for general purpose in the same file. Clarifying the expected behavior in comment is OK to me. Thanks Kevin
On Wed, 21 Mar 2018 03:30:29 +0000 "Tian, Kevin" <kevin.tian@intel.com> wrote: > > From: Alex Williamson [mailto:alex.williamson@redhat.com] > > Sent: Wednesday, March 21, 2018 6:38 AM > > > > On Mon, 19 Mar 2018 07:51:58 +0000 > > "Tian, Kevin" <kevin.tian@intel.com> wrote: > > > > > > From: Shameer Kolothum > > > > Sent: Friday, March 16, 2018 12:35 AM > > > > > > > > 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 | 90 > > > > +++++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 90 insertions(+) > > > > > > > > diff --git a/drivers/vfio/vfio_iommu_type1.c > > > > b/drivers/vfio/vfio_iommu_type1.c > > > > index 1123c74..cfe2bb2 100644 > > > > --- a/drivers/vfio/vfio_iommu_type1.c > > > > +++ b/drivers/vfio/vfio_iommu_type1.c > > > > @@ -1313,6 +1313,82 @@ 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_iommu_iova_insert(&n->list, > > > > + n->start, start - 1); > > > > + if (!ret && end < n->end) > > > > + ret = vfio_iommu_iova_insert(&n->list, > > > > + end + 1, n->end); > > > > + if (ret) > > > > + return ret; > > > > > > Is it safer to delete the 1st node here in case of failure of the 2nd node? > > > There is no problem with current logic since upon error iova_copy will > > > be released anyway. However this function alone doesn't assume the > > > fact of a temporary list, thus it's better to keep the list clean w/o garbage > > > left from any error handling. > > > > I don't think the proposal makes the list notably more sane on failure > > than we have here. If the function returns an error and the list is > > modified in any way, how can the caller recover? We're operating on a > > principle of modify a copy and throw it away on error, the only > > function level solution to the problem you're noting is to make each > > function generate a working copy, which is clearly inefficient. This > > is a static function, not intended for general use, so I think a > > sufficient approach to address your concern is to simply note the error > > behavior in the comment above the function, the list is in an > > unknown/inconsistent state on error. Thanks, > > > > 'static' doesn't mean it cannot be used for general purpose in the same > file. Obviously this is true, but expecting robust error handling, as might be found in an exported general purpose function, from a static specific purpose helper, is a bit absurd. The strategy is therefore, a) can we make it more general purpose without compromising the intent of the function; probably not without adding overhead of using a local copy of the list, b) can we modify the API, function name, arg names, etc to make the behavior more intuitive; maybe, c) Can we at least add a comment to make the potentially non-intuitive behavior obvious; of course. Thanks, Alex
> -----Original Message----- > From: Alex Williamson [mailto:alex.williamson@redhat.com] > Sent: Wednesday, March 21, 2018 4:31 PM > To: Tian, Kevin <kevin.tian@intel.com> > Cc: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>; > eric.auger@redhat.com; pmorel@linux.vnet.ibm.com; kvm@vger.kernel.org; > linux-kernel@vger.kernel.org; xuwei (O) <xuwei5@huawei.com>; Linuxarm > <linuxarm@huawei.com>; iommu@lists.linux-foundation.org > Subject: Re: [PATCH v5 2/7] vfio/type1: Check reserve region conflict and > update iova list > > On Wed, 21 Mar 2018 03:30:29 +0000 > "Tian, Kevin" <kevin.tian@intel.com> wrote: > > > > From: Alex Williamson [mailto:alex.williamson@redhat.com] > > > Sent: Wednesday, March 21, 2018 6:38 AM > > > > > > On Mon, 19 Mar 2018 07:51:58 +0000 > > > "Tian, Kevin" <kevin.tian@intel.com> wrote: > > > > > > > > From: Shameer Kolothum > > > > > Sent: Friday, March 16, 2018 12:35 AM > > > > > > > > > > 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 | 90 > > > > > +++++++++++++++++++++++++++++++++++++++++ > > > > > 1 file changed, 90 insertions(+) > > > > > > > > > > diff --git a/drivers/vfio/vfio_iommu_type1.c > > > > > b/drivers/vfio/vfio_iommu_type1.c > > > > > index 1123c74..cfe2bb2 100644 > > > > > --- a/drivers/vfio/vfio_iommu_type1.c > > > > > +++ b/drivers/vfio/vfio_iommu_type1.c > > > > > @@ -1313,6 +1313,82 @@ 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_iommu_iova_insert(&n->list, > > > > > + n->start, start > - 1); > > > > > + if (!ret && end < n->end) > > > > > + ret = vfio_iommu_iova_insert(&n->list, > > > > > + end + 1, n- > >end); > > > > > + if (ret) > > > > > + return ret; > > > > > > > > Is it safer to delete the 1st node here in case of failure of the 2nd node? > > > > There is no problem with current logic since upon error iova_copy will > > > > be released anyway. However this function alone doesn't assume the > > > > fact of a temporary list, thus it's better to keep the list clean w/o garbage > > > > left from any error handling. > > > > > > I don't think the proposal makes the list notably more sane on failure > > > than we have here. If the function returns an error and the list is > > > modified in any way, how can the caller recover? We're operating on a > > > principle of modify a copy and throw it away on error, the only > > > function level solution to the problem you're noting is to make each > > > function generate a working copy, which is clearly inefficient. This > > > is a static function, not intended for general use, so I think a > > > sufficient approach to address your concern is to simply note the error > > > behavior in the comment above the function, the list is in an > > > unknown/inconsistent state on error. Thanks, > > > > > > > 'static' doesn't mean it cannot be used for general purpose in the same > > file. > > Obviously this is true, but expecting robust error handling, as might > be found in an exported general purpose function, from a static > specific purpose helper, is a bit absurd. The strategy is therefore, > a) can we make it more general purpose without compromising the intent > of the function; probably not without adding overhead of using a local > copy of the list, b) can we modify the API, function name, arg names, > etc to make the behavior more intuitive; maybe, c) Can we at least add > a comment to make the potentially non-intuitive behavior obvious; of > course. Thanks, I had a look at this function again and agree with the observation that the only way to make it more sane would be to create another local copy of the list and free up that in case of error. That sounds very inefficient. I will go with the suggestion of adding a comment here to make the behavior obvious. Hi Alex, If there are other comments to this series, then I will sent a revised one along with this change added. Otherwise hope, its ok for you to add the comment. Please let me know. Thanks, Shameer
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index 1123c74..cfe2bb2 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -1313,6 +1313,82 @@ 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_iommu_iova_insert(&n->list, + n->start, start - 1); + if (!ret && end < n->end) + ret = vfio_iommu_iova_insert(&n->list, + end + 1, n->end); + if (ret) + return ret; + + list_del(&n->list); + kfree(n); + } + } + + if (list_empty(iova)) + return -EINVAL; + + return 0; +} + +static void vfio_iommu_resv_free(struct list_head *resv_regions) +{ + struct iommu_resv_region *n, *next; + + list_for_each_entry_safe(n, next, resv_regions, list) { + list_del(&n->list); + kfree(n); + } +} + static void vfio_iommu_iova_free(struct list_head *iova) { struct vfio_iova *n, *next; @@ -1366,6 +1442,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data, phys_addr_t resv_msi_base; struct iommu_domain_geometry geo; LIST_HEAD(iova_copy); + LIST_HEAD(group_resv_regions); mutex_lock(&iommu->lock); @@ -1444,6 +1521,13 @@ static int vfio_iommu_type1_attach_group(void *iommu_data, goto out_detach; } + 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 */ ret = vfio_iommu_iova_get_copy(iommu, &iova_copy); if (ret) @@ -1454,6 +1538,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); @@ -1514,6 +1602,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data, /* Delete the old one and insert new iova list */ vfio_iommu_iova_insert_copy(iommu, &iova_copy); mutex_unlock(&iommu->lock); + vfio_iommu_resv_free(&group_resv_regions); return 0; @@ -1522,6 +1611,7 @@ static int vfio_iommu_type1_attach_group(void *iommu_data, out_domain: iommu_domain_free(domain->domain); vfio_iommu_iova_free(&iova_copy); + vfio_iommu_resv_free(&group_resv_regions); out_free: kfree(domain); kfree(group);
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 | 90 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+)