| Message ID | 1473389864-19694-3-git-send-email-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Sep 09, 2016 at 10:57:43AM +0800, Peter Xu wrote: > The new interface can be used to replace the old notify_started() and > notify_stopped(). Meanwhile it provides explicit flags so that IOMMUs > can know what kind of notifications it is requested for. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > hw/i386/intel_iommu.c | 6 ++++-- > hw/ppc/spapr_iommu.c | 14 ++++++++++++-- > include/exec/memory.h | 9 +++++---- > memory.c | 29 +++++++++++++++++++++-------- > 4 files changed, 42 insertions(+), 16 deletions(-) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 28c31a2..9d49be7 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -1974,7 +1974,9 @@ static IOMMUTLBEntry vtd_iommu_translate(MemoryRegion *iommu, hwaddr addr, > return ret; > } > > -static void vtd_iommu_notify_started(MemoryRegion *iommu) > +static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu, > + IOMMUNotifierFlag old, > + IOMMUNotifierFlag new) > { > VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu); Shouldn't this have a sanity check that the new flags doesn't include MAP actions? > @@ -2348,7 +2350,7 @@ static void vtd_init(IntelIOMMUState *s) > memset(s->womask, 0, DMAR_REG_SIZE); > > s->iommu_ops.translate = vtd_iommu_translate; > - s->iommu_ops.notify_started = vtd_iommu_notify_started; > + s->iommu_ops.notify_flag_changed = vtd_iommu_notify_flag_changed; > s->root = 0; > s->root_extended = false; > s->dmar_enabled = false; > diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c > index 6bc4d4d..313f53f 100644 > --- a/hw/ppc/spapr_iommu.c > +++ b/hw/ppc/spapr_iommu.c > @@ -166,6 +166,17 @@ static void spapr_tce_notify_stopped(MemoryRegion *iommu) > spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), false); > } > > +static void spapr_tce_notify_flag_changed(MemoryRegion *iommu, > + IOMMUNotifierFlag old, > + IOMMUNotifierFlag new) > +{ > + if (old == IOMMU_NOTIFIER_NONE && new == IOMMU_NOTIFIER_ALL) { > + spapr_tce_notify_started(iommu); > + } else if (old == IOMMU_NOTIFIER_ALL && new == IOMMU_NOTIFIER_NONE) { > + spapr_tce_notify_stopped(iommu); > + } This is wrong. We need to do the notify_start and stop actions if *any* bits are set in the new/old flags, not just if all of them are set. I'd also prefer to see notify_started and notify_stopped folded into this function. > +} > + > static int spapr_tce_table_post_load(void *opaque, int version_id) > { > sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque); > @@ -246,8 +257,7 @@ static const VMStateDescription vmstate_spapr_tce_table = { > static MemoryRegionIOMMUOps spapr_iommu_ops = { > .translate = spapr_tce_translate_iommu, > .get_min_page_size = spapr_tce_get_min_page_size, > - .notify_started = spapr_tce_notify_started, > - .notify_stopped = spapr_tce_notify_stopped, > + .notify_flag_changed = spapr_tce_notify_flag_changed, > }; > > static int spapr_tce_table_realize(DeviceState *dev) > diff --git a/include/exec/memory.h b/include/exec/memory.h > index e69e984..e04b752 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -174,10 +174,10 @@ struct MemoryRegionIOMMUOps { > IOMMUTLBEntry (*translate)(MemoryRegion *iommu, hwaddr addr, bool is_write); > /* Returns minimum supported page size */ > uint64_t (*get_min_page_size)(MemoryRegion *iommu); > - /* Called when the first notifier is set */ > - void (*notify_started)(MemoryRegion *iommu); > - /* Called when the last notifier is removed */ > - void (*notify_stopped)(MemoryRegion *iommu); > + /* Called when IOMMU Notifier flag changed */ > + void (*notify_flag_changed)(MemoryRegion *iommu, > + IOMMUNotifierFlag old_flags, > + IOMMUNotifierFlag new_flags); > }; > > typedef struct CoalescedMemoryRange CoalescedMemoryRange; > @@ -223,6 +223,7 @@ struct MemoryRegion { > unsigned ioeventfd_nb; > MemoryRegionIoeventfd *ioeventfds; > QLIST_HEAD(, IOMMUNotifier) iommu_notify; > + IOMMUNotifierFlag iommu_notify_flags; > }; > > /** > diff --git a/memory.c b/memory.c > index f65c600..4faf1d9 100644 > --- a/memory.c > +++ b/memory.c > @@ -1419,6 +1419,7 @@ void memory_region_init_iommu(MemoryRegion *mr, > mr->iommu_ops = ops, > mr->terminates = true; /* then re-forwards */ > QLIST_INIT(&mr->iommu_notify); > + mr->iommu_notify_flags = IOMMU_NOTIFIER_NONE; > } > > static void memory_region_finalize(Object *obj) > @@ -1513,16 +1514,31 @@ bool memory_region_is_logging(MemoryRegion *mr, uint8_t client) > return memory_region_get_dirty_log_mask(mr) & (1 << client); > } > > +static void memory_region_update_iommu_notify_flags(MemoryRegion *mr) > +{ > + IOMMUNotifierFlag flags = IOMMU_NOTIFIER_NONE; > + IOMMUNotifier *iommu_notifier; > + > + QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { > + flags |= iommu_notifier->notifier_flags; > + } > + > + if (flags != mr->iommu_notify_flags && > + mr->iommu_ops->notify_flag_changed) { > + mr->iommu_ops->notify_flag_changed(mr, mr->iommu_notify_flags, > + flags); > + } > + > + mr->iommu_notify_flags = flags; > +} > + > void memory_region_register_iommu_notifier(MemoryRegion *mr, > IOMMUNotifier *n) > { > /* We need to register for at least one bitfield */ > assert(n->notifier_flags != IOMMU_NOTIFIER_NONE); > - if (mr->iommu_ops->notify_started && > - QLIST_EMPTY(&mr->iommu_notify)) { > - mr->iommu_ops->notify_started(mr); > - } > QLIST_INSERT_HEAD(&mr->iommu_notify, n, node); > + memory_region_update_iommu_notify_flags(mr); > } > > uint64_t memory_region_iommu_get_min_page_size(MemoryRegion *mr) > @@ -1560,10 +1576,7 @@ void memory_region_unregister_iommu_notifier(MemoryRegion *mr, > IOMMUNotifier *n) > { > QLIST_REMOVE(n, node); > - if (mr->iommu_ops->notify_stopped && > - QLIST_EMPTY(&mr->iommu_notify)) { > - mr->iommu_ops->notify_stopped(mr); > - } > + memory_region_update_iommu_notify_flags(mr); > } > > void memory_region_notify_iommu(MemoryRegion *mr,
On Wed, Sep 14, 2016 at 03:55:28PM +1000, David Gibson wrote: [...] > > -static void vtd_iommu_notify_started(MemoryRegion *iommu) > > +static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu, > > + IOMMUNotifierFlag old, > > + IOMMUNotifierFlag new) > > { > > VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu); > > Shouldn't this have a sanity check that the new flags doesn't include > MAP actions? See your r-b for patch 3, thanks! So skipping this one. [...] > > +static void spapr_tce_notify_flag_changed(MemoryRegion *iommu, > > + IOMMUNotifierFlag old, > > + IOMMUNotifierFlag new) > > +{ > > + if (old == IOMMU_NOTIFIER_NONE && new == IOMMU_NOTIFIER_ALL) { > > + spapr_tce_notify_started(iommu); > > + } else if (old == IOMMU_NOTIFIER_ALL && new == IOMMU_NOTIFIER_NONE) { > > + spapr_tce_notify_stopped(iommu); > > + } > > This is wrong. We need to do the notify_start and stop actions if > *any* bits are set in the new/old flags, not just if all of them are > set. Power should need both, right? I can switch all "== IOMMU_NOTIFIER_ALL" into: "!= IOMMU_NOTIFIER_NONE" in the next version if you like, but AFAICT they are totally the same. > > I'd also prefer to see notify_started and notify_stopped folded into > this function. I can do that for v5. Thanks, -- peterx
On Wed, Sep 14, 2016 at 03:12:43PM +0800, Peter Xu wrote: > On Wed, Sep 14, 2016 at 03:55:28PM +1000, David Gibson wrote: > > [...] > > > > -static void vtd_iommu_notify_started(MemoryRegion *iommu) > > > +static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu, > > > + IOMMUNotifierFlag old, > > > + IOMMUNotifierFlag new) > > > { > > > VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu); > > > > Shouldn't this have a sanity check that the new flags doesn't include > > MAP actions? > > See your r-b for patch 3, thanks! So skipping this one. > > [...] > > > > +static void spapr_tce_notify_flag_changed(MemoryRegion *iommu, > > > + IOMMUNotifierFlag old, > > > + IOMMUNotifierFlag new) > > > +{ > > > + if (old == IOMMU_NOTIFIER_NONE && new == IOMMU_NOTIFIER_ALL) { > > > + spapr_tce_notify_started(iommu); > > > + } else if (old == IOMMU_NOTIFIER_ALL && new == IOMMU_NOTIFIER_NONE) { > > > + spapr_tce_notify_stopped(iommu); > > > + } > > > > This is wrong. We need to do the notify_start and stop actions if > > *any* bits are set in the new/old flags, not just if all of them are > > set. > > Power should need both, right? I can switch all > > "== IOMMU_NOTIFIER_ALL" > > into: > > "!= IOMMU_NOTIFIER_NONE" Yes, that should be right. > in the next version if you like, but AFAICT they are totally the > same. Again, only if you assume things about how the notifiers will be used, which is not a good look when designing an interface. > > > > > I'd also prefer to see notify_started and notify_stopped folded into > > this function. > > I can do that for v5. > > Thanks, > > -- peterx >
On Wed, Sep 14, 2016 at 05:22:40PM +1000, David Gibson wrote: > On Wed, Sep 14, 2016 at 03:12:43PM +0800, Peter Xu wrote: > > On Wed, Sep 14, 2016 at 03:55:28PM +1000, David Gibson wrote: > > > > [...] > > > > > > -static void vtd_iommu_notify_started(MemoryRegion *iommu) > > > > +static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu, > > > > + IOMMUNotifierFlag old, > > > > + IOMMUNotifierFlag new) > > > > { > > > > VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu); > > > > > > Shouldn't this have a sanity check that the new flags doesn't include > > > MAP actions? > > > > See your r-b for patch 3, thanks! So skipping this one. > > > > [...] > > > > > > +static void spapr_tce_notify_flag_changed(MemoryRegion *iommu, > > > > + IOMMUNotifierFlag old, > > > > + IOMMUNotifierFlag new) > > > > +{ > > > > + if (old == IOMMU_NOTIFIER_NONE && new == IOMMU_NOTIFIER_ALL) { > > > > + spapr_tce_notify_started(iommu); > > > > + } else if (old == IOMMU_NOTIFIER_ALL && new == IOMMU_NOTIFIER_NONE) { > > > > + spapr_tce_notify_stopped(iommu); > > > > + } > > > > > > This is wrong. We need to do the notify_start and stop actions if > > > *any* bits are set in the new/old flags, not just if all of them are > > > set. > > > > Power should need both, right? I can switch all > > > > "== IOMMU_NOTIFIER_ALL" > > > > into: > > > > "!= IOMMU_NOTIFIER_NONE" > > Yes, that should be right. > > > in the next version if you like, but AFAICT they are totally the > > same. > > Again, only if you assume things about how the notifiers will be used, > which is not a good look when designing an interface. This should not be related to the interface at all? I was based on the assumption that "Power cannot support either one of MAP/UNMAP, but only if both exist". To be more specific, we possibly can have this at the beginning of flags_changed(): assert(old == IOMMU_NOTIFIER_NONE || old == IOMMU_NOTIFIER_ALL); assert(new == IOMMU_NOTIFIER_NONE || new == IOMMU_NOTIFIER_ALL); To make sure nothing will go wrong. Thanks, -- peterx
On Wed, Sep 14, 2016 at 03:49:41PM +0800, Peter Xu wrote: > On Wed, Sep 14, 2016 at 05:22:40PM +1000, David Gibson wrote: > > On Wed, Sep 14, 2016 at 03:12:43PM +0800, Peter Xu wrote: > > > On Wed, Sep 14, 2016 at 03:55:28PM +1000, David Gibson wrote: > > > > > > [...] > > > > > > > > -static void vtd_iommu_notify_started(MemoryRegion *iommu) > > > > > +static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu, > > > > > + IOMMUNotifierFlag old, > > > > > + IOMMUNotifierFlag new) > > > > > { > > > > > VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu); > > > > > > > > Shouldn't this have a sanity check that the new flags doesn't include > > > > MAP actions? > > > > > > See your r-b for patch 3, thanks! So skipping this one. > > > > > > [...] > > > > > > > > +static void spapr_tce_notify_flag_changed(MemoryRegion *iommu, > > > > > + IOMMUNotifierFlag old, > > > > > + IOMMUNotifierFlag new) > > > > > +{ > > > > > + if (old == IOMMU_NOTIFIER_NONE && new == IOMMU_NOTIFIER_ALL) { > > > > > + spapr_tce_notify_started(iommu); > > > > > + } else if (old == IOMMU_NOTIFIER_ALL && new == IOMMU_NOTIFIER_NONE) { > > > > > + spapr_tce_notify_stopped(iommu); > > > > > + } > > > > > > > > This is wrong. We need to do the notify_start and stop actions if > > > > *any* bits are set in the new/old flags, not just if all of them are > > > > set. > > > > > > Power should need both, right? I can switch all > > > > > > "== IOMMU_NOTIFIER_ALL" > > > > > > into: > > > > > > "!= IOMMU_NOTIFIER_NONE" > > > > Yes, that should be right. > > > > > in the next version if you like, but AFAICT they are totally the > > > same. > > > > Again, only if you assume things about how the notifiers will be used, > > which is not a good look when designing an interface. > > This should not be related to the interface at all? > > I was based on the assumption that "Power cannot support either one of > MAP/UNMAP, but only if both exist". Huh? I have no idea what you mean by that. Power can support notifying both map and unmap events just fine - but in order to provide *any* notifications, it has to disable KVM acceleration of the guest-side IOMMU (otherwise qemu won't know about any changes to the IOMMU state). So the change you you suggested before to != IOMMU_NOTIFIER_NONE is exactly correct, anything else is not. > To be more specific, we possibly > can have this at the beginning of flags_changed(): > > assert(old == IOMMU_NOTIFIER_NONE || old == IOMMU_NOTIFIER_ALL); > assert(new == IOMMU_NOTIFIER_NONE || new == IOMMU_NOTIFIER_ALL); > > To make sure nothing will go wrong. Hmm.. I really get the feeling you're confusing constraints of the guest side with constraints of the host side.
On Wed, Sep 14, 2016 at 08:37:34PM +1000, David Gibson wrote: [...] > > This should not be related to the interface at all? > > > > I was based on the assumption that "Power cannot support either one of > > MAP/UNMAP, but only if both exist". > > Huh? I have no idea what you mean by that. > > Power can support notifying both map and unmap events just fine - but > in order to provide *any* notifications, it has to disable KVM > acceleration of the guest-side IOMMU (otherwise qemu won't know about > any changes to the IOMMU state). > > So the change you you suggested before to != IOMMU_NOTIFIER_NONE is > exactly correct, anything else is not. Sorry I was wrong. Yes user can register with only one (MAP or UNMAP) notifier type for any platform. Thanks. -- peterx
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 28c31a2..9d49be7 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -1974,7 +1974,9 @@ static IOMMUTLBEntry vtd_iommu_translate(MemoryRegion *iommu, hwaddr addr, return ret; } -static void vtd_iommu_notify_started(MemoryRegion *iommu) +static void vtd_iommu_notify_flag_changed(MemoryRegion *iommu, + IOMMUNotifierFlag old, + IOMMUNotifierFlag new) { VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu); @@ -2348,7 +2350,7 @@ static void vtd_init(IntelIOMMUState *s) memset(s->womask, 0, DMAR_REG_SIZE); s->iommu_ops.translate = vtd_iommu_translate; - s->iommu_ops.notify_started = vtd_iommu_notify_started; + s->iommu_ops.notify_flag_changed = vtd_iommu_notify_flag_changed; s->root = 0; s->root_extended = false; s->dmar_enabled = false; diff --git a/hw/ppc/spapr_iommu.c b/hw/ppc/spapr_iommu.c index 6bc4d4d..313f53f 100644 --- a/hw/ppc/spapr_iommu.c +++ b/hw/ppc/spapr_iommu.c @@ -166,6 +166,17 @@ static void spapr_tce_notify_stopped(MemoryRegion *iommu) spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), false); } +static void spapr_tce_notify_flag_changed(MemoryRegion *iommu, + IOMMUNotifierFlag old, + IOMMUNotifierFlag new) +{ + if (old == IOMMU_NOTIFIER_NONE && new == IOMMU_NOTIFIER_ALL) { + spapr_tce_notify_started(iommu); + } else if (old == IOMMU_NOTIFIER_ALL && new == IOMMU_NOTIFIER_NONE) { + spapr_tce_notify_stopped(iommu); + } +} + static int spapr_tce_table_post_load(void *opaque, int version_id) { sPAPRTCETable *tcet = SPAPR_TCE_TABLE(opaque); @@ -246,8 +257,7 @@ static const VMStateDescription vmstate_spapr_tce_table = { static MemoryRegionIOMMUOps spapr_iommu_ops = { .translate = spapr_tce_translate_iommu, .get_min_page_size = spapr_tce_get_min_page_size, - .notify_started = spapr_tce_notify_started, - .notify_stopped = spapr_tce_notify_stopped, + .notify_flag_changed = spapr_tce_notify_flag_changed, }; static int spapr_tce_table_realize(DeviceState *dev) diff --git a/include/exec/memory.h b/include/exec/memory.h index e69e984..e04b752 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -174,10 +174,10 @@ struct MemoryRegionIOMMUOps { IOMMUTLBEntry (*translate)(MemoryRegion *iommu, hwaddr addr, bool is_write); /* Returns minimum supported page size */ uint64_t (*get_min_page_size)(MemoryRegion *iommu); - /* Called when the first notifier is set */ - void (*notify_started)(MemoryRegion *iommu); - /* Called when the last notifier is removed */ - void (*notify_stopped)(MemoryRegion *iommu); + /* Called when IOMMU Notifier flag changed */ + void (*notify_flag_changed)(MemoryRegion *iommu, + IOMMUNotifierFlag old_flags, + IOMMUNotifierFlag new_flags); }; typedef struct CoalescedMemoryRange CoalescedMemoryRange; @@ -223,6 +223,7 @@ struct MemoryRegion { unsigned ioeventfd_nb; MemoryRegionIoeventfd *ioeventfds; QLIST_HEAD(, IOMMUNotifier) iommu_notify; + IOMMUNotifierFlag iommu_notify_flags; }; /** diff --git a/memory.c b/memory.c index f65c600..4faf1d9 100644 --- a/memory.c +++ b/memory.c @@ -1419,6 +1419,7 @@ void memory_region_init_iommu(MemoryRegion *mr, mr->iommu_ops = ops, mr->terminates = true; /* then re-forwards */ QLIST_INIT(&mr->iommu_notify); + mr->iommu_notify_flags = IOMMU_NOTIFIER_NONE; } static void memory_region_finalize(Object *obj) @@ -1513,16 +1514,31 @@ bool memory_region_is_logging(MemoryRegion *mr, uint8_t client) return memory_region_get_dirty_log_mask(mr) & (1 << client); } +static void memory_region_update_iommu_notify_flags(MemoryRegion *mr) +{ + IOMMUNotifierFlag flags = IOMMU_NOTIFIER_NONE; + IOMMUNotifier *iommu_notifier; + + QLIST_FOREACH(iommu_notifier, &mr->iommu_notify, node) { + flags |= iommu_notifier->notifier_flags; + } + + if (flags != mr->iommu_notify_flags && + mr->iommu_ops->notify_flag_changed) { + mr->iommu_ops->notify_flag_changed(mr, mr->iommu_notify_flags, + flags); + } + + mr->iommu_notify_flags = flags; +} + void memory_region_register_iommu_notifier(MemoryRegion *mr, IOMMUNotifier *n) { /* We need to register for at least one bitfield */ assert(n->notifier_flags != IOMMU_NOTIFIER_NONE); - if (mr->iommu_ops->notify_started && - QLIST_EMPTY(&mr->iommu_notify)) { - mr->iommu_ops->notify_started(mr); - } QLIST_INSERT_HEAD(&mr->iommu_notify, n, node); + memory_region_update_iommu_notify_flags(mr); } uint64_t memory_region_iommu_get_min_page_size(MemoryRegion *mr) @@ -1560,10 +1576,7 @@ void memory_region_unregister_iommu_notifier(MemoryRegion *mr, IOMMUNotifier *n) { QLIST_REMOVE(n, node); - if (mr->iommu_ops->notify_stopped && - QLIST_EMPTY(&mr->iommu_notify)) { - mr->iommu_ops->notify_stopped(mr); - } + memory_region_update_iommu_notify_flags(mr); } void memory_region_notify_iommu(MemoryRegion *mr,
The new interface can be used to replace the old notify_started() and notify_stopped(). Meanwhile it provides explicit flags so that IOMMUs can know what kind of notifications it is requested for. Signed-off-by: Peter Xu <peterx@redhat.com> --- hw/i386/intel_iommu.c | 6 ++++-- hw/ppc/spapr_iommu.c | 14 ++++++++++++-- include/exec/memory.h | 9 +++++---- memory.c | 29 +++++++++++++++++++++-------- 4 files changed, 42 insertions(+), 16 deletions(-)