| Message ID | 20210122092635.19900-3-zhukeqian1@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | vfio/iommu_type1: some fixes | expand |
On Fri, 22 Jan 2021 17:26:35 +0800 Keqian Zhu <zhukeqian1@huawei.com> wrote: > vfio_sanity_check_pfn_list() is used to check whether pfn_list and > notifier are empty when remove the external domain, so it makes a > wrong assumption that only external domain will use the pinning > interface. > > Now we apply the pfn_list check when a vfio_dma is removed and apply > the notifier check when all domains are removed. > > Fixes: a54eb55045ae ("vfio iommu type1: Add support for mediated devices") > Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> > --- > drivers/vfio/vfio_iommu_type1.c | 33 ++++++++++----------------------- > 1 file changed, 10 insertions(+), 23 deletions(-) > > diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c > index 161725395f2f..d8c10f508321 100644 > --- a/drivers/vfio/vfio_iommu_type1.c > +++ b/drivers/vfio/vfio_iommu_type1.c > @@ -957,6 +957,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma, > > static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma) > { > + WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list)); > vfio_unmap_unpin(iommu, dma, true); > vfio_unlink_dma(iommu, dma); > put_task_struct(dma->task); > @@ -2250,23 +2251,6 @@ static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu) > } > } > > -static void vfio_sanity_check_pfn_list(struct vfio_iommu *iommu) > -{ > - struct rb_node *n; > - > - n = rb_first(&iommu->dma_list); > - for (; n; n = rb_next(n)) { > - struct vfio_dma *dma; > - > - dma = rb_entry(n, struct vfio_dma, node); > - > - if (WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list))) > - break; > - } > - /* mdev vendor driver must unregister notifier */ > - 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 > @@ -2366,10 +2350,10 @@ static void vfio_iommu_type1_detach_group(void *iommu_data, > kfree(group); > > if (list_empty(&iommu->external_domain->group_list)) { > - vfio_sanity_check_pfn_list(iommu); > - > - if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu)) > + if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu)) { > + WARN_ON(iommu->notifier.head); > vfio_iommu_unmap_unpin_all(iommu); > + } > > kfree(iommu->external_domain); > iommu->external_domain = NULL; > @@ -2403,10 +2387,12 @@ static void vfio_iommu_type1_detach_group(void *iommu_data, > */ > if (list_empty(&domain->group_list)) { > if (list_is_singular(&iommu->domain_list)) { > - if (!iommu->external_domain) > + if (!iommu->external_domain) { > + WARN_ON(iommu->notifier.head); > vfio_iommu_unmap_unpin_all(iommu); > - else > + } else { > vfio_iommu_unmap_unpin_reaccount(iommu); > + } > } > iommu_domain_free(domain->domain); > list_del(&domain->next); > @@ -2488,9 +2474,10 @@ static void vfio_iommu_type1_release(void *iommu_data) > struct vfio_iommu *iommu = iommu_data; > struct vfio_domain *domain, *domain_tmp; > > + WARN_ON(iommu->notifier.head); I don't see that this does any harm, but isn't it actually redundant? It seems vfio-core only calls the iommu backend release function after removing all the groups, so the tests in _detach_group should catch all cases. We're expecting the vfio bus/mdev driver to remove the notifier when a device is closed, which necessarily occurs before detaching the group. Thanks, Alex > + > if (iommu->external_domain) { > vfio_release_domain(iommu->external_domain, true); > - vfio_sanity_check_pfn_list(iommu); > kfree(iommu->external_domain); > } >
On 2021/1/28 7:46, Alex Williamson wrote: > On Fri, 22 Jan 2021 17:26:35 +0800 > Keqian Zhu <zhukeqian1@huawei.com> wrote: > >> vfio_sanity_check_pfn_list() is used to check whether pfn_list and >> notifier are empty when remove the external domain, so it makes a >> wrong assumption that only external domain will use the pinning >> interface. >> >> Now we apply the pfn_list check when a vfio_dma is removed and apply >> the notifier check when all domains are removed. >> >> Fixes: a54eb55045ae ("vfio iommu type1: Add support for mediated devices") >> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> >> --- >> drivers/vfio/vfio_iommu_type1.c | 33 ++++++++++----------------------- >> 1 file changed, 10 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c >> index 161725395f2f..d8c10f508321 100644 >> --- a/drivers/vfio/vfio_iommu_type1.c >> +++ b/drivers/vfio/vfio_iommu_type1.c >> @@ -957,6 +957,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma, >> >> static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma) >> { >> + WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list)); >> vfio_unmap_unpin(iommu, dma, true); >> vfio_unlink_dma(iommu, dma); >> put_task_struct(dma->task); >> @@ -2250,23 +2251,6 @@ static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu) >> } >> } >> >> -static void vfio_sanity_check_pfn_list(struct vfio_iommu *iommu) >> -{ >> - struct rb_node *n; >> - >> - n = rb_first(&iommu->dma_list); >> - for (; n; n = rb_next(n)) { >> - struct vfio_dma *dma; >> - >> - dma = rb_entry(n, struct vfio_dma, node); >> - >> - if (WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list))) >> - break; >> - } >> - /* mdev vendor driver must unregister notifier */ >> - 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 >> @@ -2366,10 +2350,10 @@ static void vfio_iommu_type1_detach_group(void *iommu_data, >> kfree(group); >> >> if (list_empty(&iommu->external_domain->group_list)) { >> - vfio_sanity_check_pfn_list(iommu); >> - >> - if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu)) >> + if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu)) { >> + WARN_ON(iommu->notifier.head); >> vfio_iommu_unmap_unpin_all(iommu); >> + } >> >> kfree(iommu->external_domain); >> iommu->external_domain = NULL; >> @@ -2403,10 +2387,12 @@ static void vfio_iommu_type1_detach_group(void *iommu_data, >> */ >> if (list_empty(&domain->group_list)) { >> if (list_is_singular(&iommu->domain_list)) { >> - if (!iommu->external_domain) >> + if (!iommu->external_domain) { >> + WARN_ON(iommu->notifier.head); >> vfio_iommu_unmap_unpin_all(iommu); >> - else >> + } else { >> vfio_iommu_unmap_unpin_reaccount(iommu); >> + } >> } >> iommu_domain_free(domain->domain); >> list_del(&domain->next); >> @@ -2488,9 +2474,10 @@ static void vfio_iommu_type1_release(void *iommu_data) >> struct vfio_iommu *iommu = iommu_data; >> struct vfio_domain *domain, *domain_tmp; >> >> + WARN_ON(iommu->notifier.head); > > I don't see that this does any harm, but isn't it actually redundant? > It seems vfio-core only calls the iommu backend release function after > removing all the groups, so the tests in _detach_group should catch all > cases. We're expecting the vfio bus/mdev driver to remove the notifier > when a device is closed, which necessarily occurs before detaching the > group. Thanks, > > Alex Hi Alex, Sorry that today I was busy at sending the smmu HTTU based dma dirty log tracking. I will reply you tomorrow. Thanks! Keqian. > >> + >> if (iommu->external_domain) { >> vfio_release_domain(iommu->external_domain, true); >> - vfio_sanity_check_pfn_list(iommu); >> kfree(iommu->external_domain); >> } >> > > . >
On 2021/1/28 7:46, Alex Williamson wrote: > On Fri, 22 Jan 2021 17:26:35 +0800 > Keqian Zhu <zhukeqian1@huawei.com> wrote: > >> vfio_sanity_check_pfn_list() is used to check whether pfn_list and >> notifier are empty when remove the external domain, so it makes a >> wrong assumption that only external domain will use the pinning >> interface. >> >> Now we apply the pfn_list check when a vfio_dma is removed and apply >> the notifier check when all domains are removed. >> >> Fixes: a54eb55045ae ("vfio iommu type1: Add support for mediated devices") >> Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> >> --- >> drivers/vfio/vfio_iommu_type1.c | 33 ++++++++++----------------------- >> 1 file changed, 10 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c >> index 161725395f2f..d8c10f508321 100644 >> --- a/drivers/vfio/vfio_iommu_type1.c >> +++ b/drivers/vfio/vfio_iommu_type1.c >> @@ -957,6 +957,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma, >> >> static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma) >> { >> + WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list)); >> vfio_unmap_unpin(iommu, dma, true); >> vfio_unlink_dma(iommu, dma); >> put_task_struct(dma->task); >> @@ -2250,23 +2251,6 @@ static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu) >> } >> } >> >> -static void vfio_sanity_check_pfn_list(struct vfio_iommu *iommu) >> -{ >> - struct rb_node *n; >> - >> - n = rb_first(&iommu->dma_list); >> - for (; n; n = rb_next(n)) { >> - struct vfio_dma *dma; >> - >> - dma = rb_entry(n, struct vfio_dma, node); >> - >> - if (WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list))) >> - break; >> - } >> - /* mdev vendor driver must unregister notifier */ >> - 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 >> @@ -2366,10 +2350,10 @@ static void vfio_iommu_type1_detach_group(void *iommu_data, >> kfree(group); >> >> if (list_empty(&iommu->external_domain->group_list)) { >> - vfio_sanity_check_pfn_list(iommu); >> - >> - if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu)) >> + if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu)) { >> + WARN_ON(iommu->notifier.head); >> vfio_iommu_unmap_unpin_all(iommu); >> + } >> >> kfree(iommu->external_domain); >> iommu->external_domain = NULL; >> @@ -2403,10 +2387,12 @@ static void vfio_iommu_type1_detach_group(void *iommu_data, >> */ >> if (list_empty(&domain->group_list)) { >> if (list_is_singular(&iommu->domain_list)) { >> - if (!iommu->external_domain) >> + if (!iommu->external_domain) { >> + WARN_ON(iommu->notifier.head); >> vfio_iommu_unmap_unpin_all(iommu); >> - else >> + } else { >> vfio_iommu_unmap_unpin_reaccount(iommu); >> + } >> } >> iommu_domain_free(domain->domain); >> list_del(&domain->next); >> @@ -2488,9 +2474,10 @@ static void vfio_iommu_type1_release(void *iommu_data) >> struct vfio_iommu *iommu = iommu_data; >> struct vfio_domain *domain, *domain_tmp; >> >> + WARN_ON(iommu->notifier.head); > > I don't see that this does any harm, but isn't it actually redundant? > It seems vfio-core only calls the iommu backend release function after > removing all the groups, so the tests in _detach_group should catch all > cases. We're expecting the vfio bus/mdev driver to remove the notifier > when a device is closed, which necessarily occurs before detaching the > group. Thanks, Right. Devices of a specific group must be closed before detach this group. Detach the last group have checked this, so vfio_iommu_type1_release doesn't need to do this check again. Could you please queue this patch and delete this check btw? Thanks. ;-) Keqian. > > Alex > >> + >> if (iommu->external_domain) { >> vfio_release_domain(iommu->external_domain, true); >> - vfio_sanity_check_pfn_list(iommu); >> kfree(iommu->external_domain); >> } >> > > . >
diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c index 161725395f2f..d8c10f508321 100644 --- a/drivers/vfio/vfio_iommu_type1.c +++ b/drivers/vfio/vfio_iommu_type1.c @@ -957,6 +957,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma, static void vfio_remove_dma(struct vfio_iommu *iommu, struct vfio_dma *dma) { + WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list)); vfio_unmap_unpin(iommu, dma, true); vfio_unlink_dma(iommu, dma); put_task_struct(dma->task); @@ -2250,23 +2251,6 @@ static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu) } } -static void vfio_sanity_check_pfn_list(struct vfio_iommu *iommu) -{ - struct rb_node *n; - - n = rb_first(&iommu->dma_list); - for (; n; n = rb_next(n)) { - struct vfio_dma *dma; - - dma = rb_entry(n, struct vfio_dma, node); - - if (WARN_ON(!RB_EMPTY_ROOT(&dma->pfn_list))) - break; - } - /* mdev vendor driver must unregister notifier */ - 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 @@ -2366,10 +2350,10 @@ static void vfio_iommu_type1_detach_group(void *iommu_data, kfree(group); if (list_empty(&iommu->external_domain->group_list)) { - vfio_sanity_check_pfn_list(iommu); - - if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu)) + if (!IS_IOMMU_CAP_DOMAIN_IN_CONTAINER(iommu)) { + WARN_ON(iommu->notifier.head); vfio_iommu_unmap_unpin_all(iommu); + } kfree(iommu->external_domain); iommu->external_domain = NULL; @@ -2403,10 +2387,12 @@ static void vfio_iommu_type1_detach_group(void *iommu_data, */ if (list_empty(&domain->group_list)) { if (list_is_singular(&iommu->domain_list)) { - if (!iommu->external_domain) + if (!iommu->external_domain) { + WARN_ON(iommu->notifier.head); vfio_iommu_unmap_unpin_all(iommu); - else + } else { vfio_iommu_unmap_unpin_reaccount(iommu); + } } iommu_domain_free(domain->domain); list_del(&domain->next); @@ -2488,9 +2474,10 @@ static void vfio_iommu_type1_release(void *iommu_data) struct vfio_iommu *iommu = iommu_data; struct vfio_domain *domain, *domain_tmp; + WARN_ON(iommu->notifier.head); + if (iommu->external_domain) { vfio_release_domain(iommu->external_domain, true); - vfio_sanity_check_pfn_list(iommu); kfree(iommu->external_domain); }
vfio_sanity_check_pfn_list() is used to check whether pfn_list and notifier are empty when remove the external domain, so it makes a wrong assumption that only external domain will use the pinning interface. Now we apply the pfn_list check when a vfio_dma is removed and apply the notifier check when all domains are removed. Fixes: a54eb55045ae ("vfio iommu type1: Add support for mediated devices") Signed-off-by: Keqian Zhu <zhukeqian1@huawei.com> --- drivers/vfio/vfio_iommu_type1.c | 33 ++++++++++----------------------- 1 file changed, 10 insertions(+), 23 deletions(-)