| Message ID | 20230608095231.225450-5-zhenzhong.duan@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Optimize UNMAP call and bug fix | expand |
On Thu, Jun 08, 2023 at 05:52:30PM +0800, Zhenzhong Duan wrote: > During address space unmap, corresponding IOVA tree entries are > also removed. But DMAMap is set beyond notifier's scope by 1, so > in theory there is possibility to remove a continuous entry above > the notifier's scope but falling in adjacent notifier's scope. This function is only called in "loop over all notifiers" case (or replay() that just got removed, but even so there'll be only 1 notifier normally iiuc at least for vt-d), hopefully it means no bug exist (no Fixes needed, no backport needed either), but still worth fixing it up. > > There is no issue currently as no use cases allocate notifiers > continuously, but let's be robust. > > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> Reviewed-by: Peter Xu <peterx@redhat.com>
>-----Original Message----- >From: Peter Xu <peterx@redhat.com> >Sent: Thursday, June 8, 2023 9:48 PM >To: Duan, Zhenzhong <zhenzhong.duan@intel.com> >Cc: qemu-devel@nongnu.org; mst@redhat.com; jasowang@redhat.com; >pbonzini@redhat.com; richard.henderson@linaro.org; eduardo@habkost.net; >marcel.apfelbaum@gmail.com; alex.williamson@redhat.com; >clg@redhat.com; david@redhat.com; philmd@linaro.org; >kwankhede@nvidia.com; cjia@nvidia.com; Liu, Yi L <yi.l.liu@intel.com>; Peng, >Chao P <chao.p.peng@intel.com> >Subject: Re: [PATCH v3 4/5] intel_iommu: Fix address space unmap > >On Thu, Jun 08, 2023 at 05:52:30PM +0800, Zhenzhong Duan wrote: >> During address space unmap, corresponding IOVA tree entries are also >> removed. But DMAMap is set beyond notifier's scope by 1, so in theory >> there is possibility to remove a continuous entry above the notifier's >> scope but falling in adjacent notifier's scope. > >This function is only called in "loop over all notifiers" case (or replay() that just >got removed, but even so there'll be only 1 notifier normally iiuc at least for >vt-d), hopefully it means no bug exist (no Fixes needed, no backport needed >either), but still worth fixing it up. Not two notifiers as vtd-ir splits for vt-d? Thanks Zhenzhong > >> >> There is no issue currently as no use cases allocate notifiers >> continuously, but let's be robust. >> >> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> > >Reviewed-by: Peter Xu <peterx@redhat.com> > >-- >Peter Xu
On Fri, Jun 09, 2023 at 03:31:46AM +0000, Duan, Zhenzhong wrote: > > > >-----Original Message----- > >From: Peter Xu <peterx@redhat.com> > >Sent: Thursday, June 8, 2023 9:48 PM > >To: Duan, Zhenzhong <zhenzhong.duan@intel.com> > >Cc: qemu-devel@nongnu.org; mst@redhat.com; jasowang@redhat.com; > >pbonzini@redhat.com; richard.henderson@linaro.org; eduardo@habkost.net; > >marcel.apfelbaum@gmail.com; alex.williamson@redhat.com; > >clg@redhat.com; david@redhat.com; philmd@linaro.org; > >kwankhede@nvidia.com; cjia@nvidia.com; Liu, Yi L <yi.l.liu@intel.com>; Peng, > >Chao P <chao.p.peng@intel.com> > >Subject: Re: [PATCH v3 4/5] intel_iommu: Fix address space unmap > > > >On Thu, Jun 08, 2023 at 05:52:30PM +0800, Zhenzhong Duan wrote: > >> During address space unmap, corresponding IOVA tree entries are also > >> removed. But DMAMap is set beyond notifier's scope by 1, so in theory > >> there is possibility to remove a continuous entry above the notifier's > >> scope but falling in adjacent notifier's scope. > > > >This function is only called in "loop over all notifiers" case (or replay() that just > >got removed, but even so there'll be only 1 notifier normally iiuc at least for > >vt-d), hopefully it means no bug exist (no Fixes needed, no backport needed > >either), but still worth fixing it up. > > Not two notifiers as vtd-ir splits for vt-d? The two notifiers will all be attached to the same IOMMU mr, so IOMMU_NOTIFIER_FOREACH() will loop over them all always? And this actually shouldn't matter, IMHO, as the IR split has the 0xfeeXXXXX hole only, so when notifying with end=0xfee00000 (comparing to end=0xfedfffff) it shouldn't make a difference iiuc because there should have no iova entry at 0xfee00000 anyway in the tree.
>-----Original Message----- >From: Peter Xu <peterx@redhat.com> >Sent: Friday, June 9, 2023 9:37 PM >To: Duan, Zhenzhong <zhenzhong.duan@intel.com> >Cc: qemu-devel@nongnu.org; mst@redhat.com; jasowang@redhat.com; >pbonzini@redhat.com; richard.henderson@linaro.org; eduardo@habkost.net; >marcel.apfelbaum@gmail.com; alex.williamson@redhat.com; >clg@redhat.com; david@redhat.com; philmd@linaro.org; >kwankhede@nvidia.com; cjia@nvidia.com; Liu, Yi L <yi.l.liu@intel.com>; Peng, >Chao P <chao.p.peng@intel.com> >Subject: Re: [PATCH v3 4/5] intel_iommu: Fix address space unmap > >On Fri, Jun 09, 2023 at 03:31:46AM +0000, Duan, Zhenzhong wrote: >> >> >> >-----Original Message----- >> >From: Peter Xu <peterx@redhat.com> >> >Sent: Thursday, June 8, 2023 9:48 PM >> >To: Duan, Zhenzhong <zhenzhong.duan@intel.com> >> >Cc: qemu-devel@nongnu.org; mst@redhat.com; jasowang@redhat.com; >> >pbonzini@redhat.com; richard.henderson@linaro.org; >> >eduardo@habkost.net; marcel.apfelbaum@gmail.com; >> >alex.williamson@redhat.com; clg@redhat.com; david@redhat.com; >> >philmd@linaro.org; kwankhede@nvidia.com; cjia@nvidia.com; Liu, Yi L >> ><yi.l.liu@intel.com>; Peng, Chao P <chao.p.peng@intel.com> >> >Subject: Re: [PATCH v3 4/5] intel_iommu: Fix address space unmap >> > >> >On Thu, Jun 08, 2023 at 05:52:30PM +0800, Zhenzhong Duan wrote: >> >> During address space unmap, corresponding IOVA tree entries are >> >> also removed. But DMAMap is set beyond notifier's scope by 1, so in >> >> theory there is possibility to remove a continuous entry above the >> >> notifier's scope but falling in adjacent notifier's scope. >> > >> >This function is only called in "loop over all notifiers" case (or >> >replay() that just got removed, but even so there'll be only 1 >> >notifier normally iiuc at least for vt-d), hopefully it means no bug >> >exist (no Fixes needed, no backport needed either), but still worth fixing it >up. >> >> Not two notifiers as vtd-ir splits for vt-d? > >The two notifiers will all be attached to the same IOMMU mr, so >IOMMU_NOTIFIER_FOREACH() will loop over them all always? Yes. > >And this actually shouldn't matter, IMHO, as the IR split has the 0xfeeXXXXX >hole only, so when notifying with end=0xfee00000 (comparing to >end=0xfedfffff) it shouldn't make a difference iiuc because there should have >no iova entry at 0xfee00000 anyway in the tree. Clear. Thanks Zhenzhong
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index f046f8591335..dcc334060cd6 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -3791,7 +3791,7 @@ static void vtd_address_space_unmap(VTDAddressSpace *as, IOMMUNotifier *n) n->start, size); map.iova = n->start; - map.size = size; + map.size = size - 1; /* Inclusive */ iova_tree_remove(as->iova_tree, map); }
During address space unmap, corresponding IOVA tree entries are also removed. But DMAMap is set beyond notifier's scope by 1, so in theory there is possibility to remove a continuous entry above the notifier's scope but falling in adjacent notifier's scope. There is no issue currently as no use cases allocate notifiers continuously, but let's be robust. Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com> --- hw/i386/intel_iommu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)