[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 08/10] intel-iommu: maintain per-device iova ran
From: |
Peter Xu |
Subject: |
Re: [Qemu-devel] [PATCH 08/10] intel-iommu: maintain per-device iova ranges |
Date: |
Fri, 27 Apr 2018 17:55:27 +0800 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
On Fri, Apr 27, 2018 at 07:44:07AM +0000, Tian, Kevin wrote:
> > From: Peter Xu [mailto:address@hidden
> > Sent: Friday, April 27, 2018 3:28 PM
> >
> > On Fri, Apr 27, 2018 at 07:02:14AM +0000, Tian, Kevin wrote:
> > > > From: Jason Wang [mailto:address@hidden
> > > > Sent: Friday, April 27, 2018 2:08 PM
> > > >
> > > > On 2018年04月25日 12:51, Peter Xu wrote:
> > > > > For each VTDAddressSpace, now we maintain what IOVA ranges we
> > have
> > > > > mapped and what we have not. With that information, now we only
> > > > send
> > > > > MAP or UNMAP when necessary. Say, we don't send MAP notifies if
> > we
> > > > know
> > > > > we have already mapped the range, meanwhile we don't send
> > UNMAP
> > > > notifies
> > > > > if we know we never mapped the range at all.
> > > > >
> > > > > Signed-off-by: Peter Xu <address@hidden>
> > > > > ---
> > > > > include/hw/i386/intel_iommu.h | 2 ++
> > > > > hw/i386/intel_iommu.c | 28 ++++++++++++++++++++++++++++
> > > > > hw/i386/trace-events | 2 ++
> > > > > 3 files changed, 32 insertions(+)
> > > > >
> > > > > diff --git a/include/hw/i386/intel_iommu.h
> > > > b/include/hw/i386/intel_iommu.h
> > > > > index 486e205e79..09a2e94404 100644
> > > > > --- a/include/hw/i386/intel_iommu.h
> > > > > +++ b/include/hw/i386/intel_iommu.h
> > > > > @@ -27,6 +27,7 @@
> > > > > #include "hw/i386/ioapic.h"
> > > > > #include "hw/pci/msi.h"
> > > > > #include "hw/sysbus.h"
> > > > > +#include "qemu/interval-tree.h"
> > > > >
> > > > > #define TYPE_INTEL_IOMMU_DEVICE "intel-iommu"
> > > > > #define INTEL_IOMMU_DEVICE(obj) \
> > > > > @@ -95,6 +96,7 @@ struct VTDAddressSpace {
> > > > > QLIST_ENTRY(VTDAddressSpace) next;
> > > > > /* Superset of notifier flags that this address space has */
> > > > > IOMMUNotifierFlag notifier_flags;
> > > > > + ITTree *iova_tree; /* Traces mapped IOVA ranges */
> > > > > };
> > > > >
> > > > > struct VTDBus {
> > > > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > > > > index a19c18b8d4..8f396a5d13 100644
> > > > > --- a/hw/i386/intel_iommu.c
> > > > > +++ b/hw/i386/intel_iommu.c
> > > > > @@ -768,12 +768,37 @@ typedef struct {
> > > > > static int vtd_page_walk_one(IOMMUTLBEntry *entry, int level,
> > > > > vtd_page_walk_info *info)
> > > > > {
> > > > > + VTDAddressSpace *as = info->as;
> > > > > vtd_page_walk_hook hook_fn = info->hook_fn;
> > > > > void *private = info->private;
> > > > > + ITRange *mapped = it_tree_find(as->iova_tree, entry->iova,
> > > > > + entry->iova + entry->addr_mask);
> > > > >
> > > > > assert(hook_fn);
> > > > > +
> > > > > + /* Update local IOVA mapped ranges */
> > > > > + if (entry->perm) {
> > > > > + if (mapped) {
> > > > > + /* Skip since we have already mapped this range */
> > > > > + trace_vtd_page_walk_one_skip_map(entry->iova, entry-
> > > > >addr_mask,
> > > > > + mapped->start,
> > > > > mapped->end);
> > > > > + return 0;
> > > > > + }
> > > > > + it_tree_insert(as->iova_tree, entry->iova,
> > > > > + entry->iova + entry->addr_mask);
> > > >
> > > > I was consider a case e.g:
> > > >
> > > > 1) map A (iova) to B (pa)
> > > > 2) invalidate A
> > > > 3) map A (iova) to C (pa)
> > > > 4) invalidate A
> > > >
> > > > In this case, we will probably miss a walk here. But I'm not sure it was
> > > > allowed by the spec (though I think so).
> > > >
> >
> > Hi, Kevin,
> >
> > Thanks for joining the discussion.
> >
> > >
> > > I thought it was wrong in a glimpse, but then changed my mind after
> > > another thinking. As long as device driver can quiescent the device
> > > to not access A (iova) within above window, then above sequence
> > > has no problem since any stale mappings (A->B) added before step 4)
> > > won't be used and then will get flushed after step 4). Regarding to
> > > that actually the 1st invalidation can be skipped:
> > >
> > > 1) map A (iova) to B (pa)
> > > 2) driver programs device to use A
> > > 3) driver programs device to not use A
> > > 4) map A (iova) to C (pa)
> > > A->B may be still valid in IOTLB
> > > 5) invalidate A
> > > 6) driver programs device to use A
> >
> > Note that IMHO this is a bit different from Jason's example, and it'll
> > be fine. Current code should work well with this scenario since the
> > emulation code will not aware of the map A until step (5). Then we'll
> > have the correct mapping.
>
> you are right. we still need the extra PSI otherwise the 1st mapping
> is problematic for use. So back to Jason's example.
>
> >
> > While for Jason's example it's exactly the extra PSI that might cause
> > stale mappings (though again I think it's still problematic...).
>
> problematic in software side (e.g. that way IOMMU core relies on
> device drivers which conflict with the value of using IOMMU) but
> it is OK from hardware p.o.v. btw the extra PSI itself doesn't cause
> stale mapping. Instead it is device activity after that PSI may cause it.
>
> >
> > Actually I think I can just fix up the code even if Jason's case
> > happens by unmapping that first then remap:
> >
> > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c
> > index 31e9b52452..2a9584f9d8 100644
> > --- a/hw/i386/intel_iommu.c
> > +++ b/hw/i386/intel_iommu.c
> > @@ -778,13 +778,21 @@ static int vtd_page_walk_one(IOMMUTLBEntry
> > *entry, int level,
> > /* Update local IOVA mapped ranges */
> > if (entry->perm) {
> > if (mapped) {
> > - /* Skip since we have already mapped this range */
> > - trace_vtd_page_walk_one_skip_map(entry->iova, entry-
> > >addr_mask,
> > - mapped->start, mapped->end);
> > - return 0;
> > + int ret;
> > + /* Cache the write permission */
> > + IOMMUAccessFlags flags = entry->perm;
> > +
> > + /* UNMAP the old first then remap. No need to touch IOVA tree
> > */
> > + entry->perm = IOMMU_NONE;
> > + ret = hook_fn(entry, private);
> > + if (ret) {
> > + return ret;
> > + }
> > + entry->perm = flags;
> > + } else {
> > + it_tree_insert(as->iova_tree, entry->iova,
> > + entry->iova + entry->addr_mask);
> > }
> > - it_tree_insert(as->iova_tree, entry->iova,
> > - entry->iova + entry->addr_mask);
> > } else {
> > if (!mapped) {
> > /* Skip since we didn't map this range at all */
> >
> > If we really think it necessary, I can squash this in, though this is
> > a bit ugly. But I just want to confirm whether this would be anything
> > we want...
> >
>
> I didn’t look into your actual code yet. If others think above
> change is OK then it's definitely good as we conform to hardware
> behavior here. Otherwise if there is a way to detect such unusual
> usage and then adopt some action (say kill the VM), it's also fine
> since user knows he runs a bad OS which is not supported by
> Qemu. It's just not good if such situation is not handled, which
> leads to some undefined behavior which nobody knows the reason
> w/o hard debug into. :-)
Yeah, then let me do this. :)
Jason, would you be good with above change squashed?
Thanks,
--
Peter Xu
- [Qemu-devel] [PATCH 04/10] intel-iommu: only do page walk for MAP notifiers, (continued)
- [Qemu-devel] [PATCH 04/10] intel-iommu: only do page walk for MAP notifiers, Peter Xu, 2018/04/25
- [Qemu-devel] [PATCH 07/10] util: implement simple interval tree logic, Peter Xu, 2018/04/25
- [Qemu-devel] [PATCH 08/10] intel-iommu: maintain per-device iova ranges, Peter Xu, 2018/04/25
- Re: [Qemu-devel] [PATCH 08/10] intel-iommu: maintain per-device iova ranges, Jason Wang, 2018/04/27
- Re: [Qemu-devel] [PATCH 08/10] intel-iommu: maintain per-device iova ranges, Peter Xu, 2018/04/27
- Re: [Qemu-devel] [PATCH 08/10] intel-iommu: maintain per-device iova ranges, Tian, Kevin, 2018/04/27
- Re: [Qemu-devel] [PATCH 08/10] intel-iommu: maintain per-device iova ranges, Peter Xu, 2018/04/27
- Re: [Qemu-devel] [PATCH 08/10] intel-iommu: maintain per-device iova ranges, Tian, Kevin, 2018/04/27
- Re: [Qemu-devel] [PATCH 08/10] intel-iommu: maintain per-device iova ranges,
Peter Xu <=
- Re: [Qemu-devel] [PATCH 08/10] intel-iommu: maintain per-device iova ranges, Peter Xu, 2018/04/27
- Re: [Qemu-devel] [PATCH 08/10] intel-iommu: maintain per-device iova ranges, Tian, Kevin, 2018/04/27
- Re: [Qemu-devel] [PATCH 08/10] intel-iommu: maintain per-device iova ranges, Jason Wang, 2018/04/27
[Qemu-devel] [PATCH 09/10] intel-iommu: don't unmap all for shadow page table, Peter Xu, 2018/04/25
[Qemu-devel] [PATCH 10/10] intel-iommu: remove notify_unmap for page walk, Peter Xu, 2018/04/25
Re: [Qemu-devel] [PATCH 00/10] intel-iommu: nested vIOMMU, cleanups, bug fixes, no-reply, 2018/04/25