[v3] Optimize UNMAP call and bug fix

[PATCH v3 4/5] intel_iommu: Fix address space unmap

Posted by Zhenzhong Duan 2 years, 8 months ago

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(-)

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);
 }
 
-- 
2.34.1

Re: [PATCH v3 4/5] intel_iommu: Fix address space unmap

Posted by Peter Xu 2 years, 8 months ago

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>

-- 
Peter Xu

RE: [PATCH v3 4/5] intel_iommu: Fix address space unmap

Posted by Duan, Zhenzhong 2 years, 8 months ago


>-----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

Re: [PATCH v3 4/5] intel_iommu: Fix address space unmap

Posted by Peter Xu 2 years, 8 months ago

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.

-- 
Peter Xu

RE: [PATCH v3 4/5] intel_iommu: Fix address space unmap

Posted by Duan, Zhenzhong 2 years, 8 months ago


>-----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