| Message ID | 1473226344-28520-3-git-send-email-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Sep 07, 2016 at 01:32:23PM +0800, Peter Xu wrote: > Considering that we may have multiple IOMMU notifier consumers in the > future, converting iommu_ops.notify_{started|stopped} into some more > general form. Now we can trap all notifier registerations and > deregistrations, rather than only the first ones. > > Power was leveraging the notifier_{started|stopped}, adding iommu_user > field for counting on Power guests to achieve the same goal. Requiring each vIOMMU frontend to reference count or whatever seems like a pain. The semantics of notify_started() were designed to avoid that. Instead I'd suggest a callback which gets tripped any time the logical OR of the requested notifications for all current notifiers changes. > Suggested-by: Paolo Bonzini <pbonzini@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > hw/i386/intel_iommu.c | 4 ++-- > hw/ppc/spapr_iommu.c | 18 ++++++++++++------ > include/exec/memory.h | 8 ++++---- > include/hw/ppc/spapr.h | 1 + > memory.c | 10 ++++------ > 5 files changed, 23 insertions(+), 18 deletions(-) > > diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c > index 28c31a2..c6bd8f6 100644 > --- a/hw/i386/intel_iommu.c > +++ b/hw/i386/intel_iommu.c > @@ -1974,7 +1974,7 @@ static IOMMUTLBEntry vtd_iommu_translate(MemoryRegion *iommu, hwaddr addr, > return ret; > } > > -static void vtd_iommu_notify_started(MemoryRegion *iommu) > +static void vtd_iommu_notifier_add(MemoryRegion *iommu, IOMMUNotifier *n) > { > VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu); > > @@ -2348,7 +2348,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.notifier_add = vtd_iommu_notifier_add; > 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..99c83a4 100644 > --- a/hw/ppc/spapr_iommu.c > +++ b/hw/ppc/spapr_iommu.c > @@ -156,14 +156,20 @@ static uint64_t spapr_tce_get_min_page_size(MemoryRegion *iommu) > return 1ULL << tcet->page_shift; > } > > -static void spapr_tce_notify_started(MemoryRegion *iommu) > +static void spapr_tce_notifier_add(MemoryRegion *iommu, IOMMUNotifier *n) > { > - spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), true); > + sPAPRTCETable *tcet = container_of(iommu, sPAPRTCETable, iommu); > + if (tcet->iommu_users++ == 0) { > + spapr_tce_set_need_vfio(tcet, true); > + } > } > > -static void spapr_tce_notify_stopped(MemoryRegion *iommu) > +static void spapr_tce_notifier_del(MemoryRegion *iommu, IOMMUNotifier *n) > { > - spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), false); > + sPAPRTCETable *tcet = container_of(iommu, sPAPRTCETable, iommu); > + if (--tcet->iommu_users == 0) { > + spapr_tce_set_need_vfio(tcet, false); > + } > } > > static int spapr_tce_table_post_load(void *opaque, int version_id) > @@ -246,8 +252,8 @@ 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, > + .notifier_add = spapr_tce_notifier_add, > + .notifier_del = spapr_tce_notifier_del, > }; > > static int spapr_tce_table_realize(DeviceState *dev) > diff --git a/include/exec/memory.h b/include/exec/memory.h > index 92f14db..9efcf1b 100644 > --- a/include/exec/memory.h > +++ b/include/exec/memory.h > @@ -175,10 +175,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 someone registers to the notify list */ > + void (*notifier_add)(MemoryRegion *iommu, IOMMUNotifier *n); > + /* Called when someone unregisters from the notify list */ > + void (*notifier_del)(MemoryRegion *iommu, IOMMUNotifier *n); > }; > > typedef struct CoalescedMemoryRange CoalescedMemoryRange; > diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h > index caf7be9..08627ec 100644 > --- a/include/hw/ppc/spapr.h > +++ b/include/hw/ppc/spapr.h > @@ -564,6 +564,7 @@ struct sPAPRTCETable { > MemoryRegion root, iommu; > struct VIOsPAPRDevice *vdev; /* for @bypass migration compatibility only */ > QLIST_ENTRY(sPAPRTCETable) list; > + int iommu_users; > }; > > sPAPRTCETable *spapr_tce_find_by_liobn(target_ulong liobn); > diff --git a/memory.c b/memory.c > index 45a3902..d913043 100644 > --- a/memory.c > +++ b/memory.c > @@ -1518,9 +1518,8 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr, > { > /* We need to register for at least one bitfield */ > assert(n->notifier_caps != IOMMU_NOTIFIER_NONE); > - if (mr->iommu_ops->notify_started && > - QLIST_EMPTY(&mr->iommu_notify)) { > - mr->iommu_ops->notify_started(mr); > + if (mr->iommu_ops->notifier_add) { > + mr->iommu_ops->notifier_add(mr, n); > } > QLIST_INSERT_HEAD(&mr->iommu_notify, n, node); > } > @@ -1560,9 +1559,8 @@ 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); > + if (mr->iommu_ops->notifier_del) { > + mr->iommu_ops->notifier_del(mr, n); > } > } >
On Wed, Sep 07, 2016 at 04:05:50PM +1000, David Gibson wrote: > On Wed, Sep 07, 2016 at 01:32:23PM +0800, Peter Xu wrote: > > Considering that we may have multiple IOMMU notifier consumers in the > > future, converting iommu_ops.notify_{started|stopped} into some more > > general form. Now we can trap all notifier registerations and > > deregistrations, rather than only the first ones. > > > > Power was leveraging the notifier_{started|stopped}, adding iommu_user > > field for counting on Power guests to achieve the same goal. > > Requiring each vIOMMU frontend to reference count or whatever seems > like a pain. The semantics of notify_started() were designed to avoid > that. The problem is that, I think we need something like "notifier_add", just like you have mentioned before, e.g., what if we have more than one registers for the notifier list? And if with that, it'll be awkward to still keep the notify_started since it's actually a subset of "notifier_add". Considering the above, I think simply adding a count for IOMMUs who want it is a reasonable trade-off. > > Instead I'd suggest a callback which gets tripped any time the logical > OR of the requested notifications for all current notifiers changes. If so, we will need two callbacks (notify_started, notifier_xxx_changed) instead of one. In that case I'd prefer a single notifier_add. Besides that, I'd say the notifier_xxx_changed interface is really hard to understand from the first glance. Another reason to have notifier_add() is that, this is more easily to be extended. E.g., we can add more fields to IOMMUNotifier in the future (a channel between the consumer and provider), and it'll be passed to each IOMMU's notifier_add() naturally. Thanks, -- peterx
On Wed, Sep 07, 2016 at 03:23:16PM +0800, Peter Xu wrote: > On Wed, Sep 07, 2016 at 04:05:50PM +1000, David Gibson wrote: > > On Wed, Sep 07, 2016 at 01:32:23PM +0800, Peter Xu wrote: > > > Considering that we may have multiple IOMMU notifier consumers in the > > > future, converting iommu_ops.notify_{started|stopped} into some more > > > general form. Now we can trap all notifier registerations and > > > deregistrations, rather than only the first ones. > > > > > > Power was leveraging the notifier_{started|stopped}, adding iommu_user > > > field for counting on Power guests to achieve the same goal. > > > > Requiring each vIOMMU frontend to reference count or whatever seems > > like a pain. The semantics of notify_started() were designed to avoid > > that. > > The problem is that, I think we need something like "notifier_add", > just like you have mentioned before, e.g., what if we have more than > one registers for the notifier list? The approach I had in mind is that whenever the notifier list changed, the infrastructure would rescan the list and recompute the bitmask of "events any notifier cares about". If that changed from the previous value, it would call the notify_change() cb on the vIOMMU. > And if with that, it'll be > awkward to still keep the notify_started since it's actually a subset > of "notifier_add". > > Considering the above, I think simply adding a count for IOMMUs who > want it is a reasonable trade-off. > > > > > Instead I'd suggest a callback which gets tripped any time the logical > > OR of the requested notifications for all current notifiers changes. > > If so, we will need two callbacks (notify_started, > notifier_xxx_changed) instead of one. No, just one. notify_started is just notify_change() with a non-zero bitmask, when the mask was previously zero. > In that case I'd prefer a single > notifier_add. Besides that, I'd say the notifier_xxx_changed interface > is really hard to understand from the first glance. > > Another reason to have notifier_add() is that, this is more easily to > be extended. E.g., we can add more fields to IOMMUNotifier in the > future (a channel between the consumer and provider), and it'll be > passed to each IOMMU's notifier_add() naturally. Hm, I suppose.
On 07/09/2016 08:05, David Gibson wrote: > On Wed, Sep 07, 2016 at 01:32:23PM +0800, Peter Xu wrote: >> Considering that we may have multiple IOMMU notifier consumers in the >> future, converting iommu_ops.notify_{started|stopped} into some more >> general form. Now we can trap all notifier registerations and >> deregistrations, rather than only the first ones. >> >> Power was leveraging the notifier_{started|stopped}, adding iommu_user >> field for counting on Power guests to achieve the same goal. > > Requiring each vIOMMU frontend to reference count or whatever seems > like a pain. The semantics of notify_started() were designed to avoid > that. > > Instead I'd suggest a callback which gets tripped any time the logical > OR of the requested notifications for all current notifiers changes. I like the suggestion. Alternatively you could call notify_stopped if old & ~new is nonzero (and you pass old & ~new), and notify_started if new & ~old is nonzero (and you pass new & ~old). Paolo
On Wed, Sep 07, 2016 at 12:54:23PM +0200, Paolo Bonzini wrote: > > > On 07/09/2016 08:05, David Gibson wrote: > > On Wed, Sep 07, 2016 at 01:32:23PM +0800, Peter Xu wrote: > >> Considering that we may have multiple IOMMU notifier consumers in the > >> future, converting iommu_ops.notify_{started|stopped} into some more > >> general form. Now we can trap all notifier registerations and > >> deregistrations, rather than only the first ones. > >> > >> Power was leveraging the notifier_{started|stopped}, adding iommu_user > >> field for counting on Power guests to achieve the same goal. > > > > Requiring each vIOMMU frontend to reference count or whatever seems > > like a pain. The semantics of notify_started() were designed to avoid > > that. > > > > Instead I'd suggest a callback which gets tripped any time the logical > > OR of the requested notifications for all current notifiers changes. > > I like the suggestion. Alternatively you could call notify_stopped if > old & ~new is nonzero (and you pass old & ~new), and notify_started if > new & ~old is nonzero (and you pass new & ~old). I think now I understand the point... Then I'd prefer to use David's suggestion. A single notify_changed() looks cleaner. To be more explicit, I would prefer to rename it to notifier_flag_changed(), since notify_changed() looks like to be called every time notifier list changed, but actually it is for monitoring the flags. Thanks, -- peterx
On Thu, Sep 08, 2016 at 06:22:40PM +0800, Peter Xu wrote: > On Wed, Sep 07, 2016 at 12:54:23PM +0200, Paolo Bonzini wrote: > > > > > > On 07/09/2016 08:05, David Gibson wrote: > > > On Wed, Sep 07, 2016 at 01:32:23PM +0800, Peter Xu wrote: > > >> Considering that we may have multiple IOMMU notifier consumers in the > > >> future, converting iommu_ops.notify_{started|stopped} into some more > > >> general form. Now we can trap all notifier registerations and > > >> deregistrations, rather than only the first ones. > > >> > > >> Power was leveraging the notifier_{started|stopped}, adding iommu_user > > >> field for counting on Power guests to achieve the same goal. > > > > > > Requiring each vIOMMU frontend to reference count or whatever seems > > > like a pain. The semantics of notify_started() were designed to avoid > > > that. > > > > > > Instead I'd suggest a callback which gets tripped any time the logical > > > OR of the requested notifications for all current notifiers changes. > > > > I like the suggestion. Alternatively you could call notify_stopped if > > old & ~new is nonzero (and you pass old & ~new), and notify_started if > > new & ~old is nonzero (and you pass new & ~old). > > I think now I understand the point... Then I'd prefer to use David's > suggestion. A single notify_changed() looks cleaner. To be more > explicit, I would prefer to rename it to notifier_flag_changed(), > since notify_changed() looks like to be called every time notifier > list changed, but actually it is for monitoring the flags. That sounds reasonable. I think notifier_flag_changed() should be passed both the old and new flags, to save the backend having to keep track of the old ones - which flags have changed might affect what the callback needs to do.
On Mon, Sep 12, 2016 at 11:17:37AM +1000, David Gibson wrote: [...] > > I think now I understand the point... Then I'd prefer to use David's > > suggestion. A single notify_changed() looks cleaner. To be more > > explicit, I would prefer to rename it to notifier_flag_changed(), > > since notify_changed() looks like to be called every time notifier > > list changed, but actually it is for monitoring the flags. > > That sounds reasonable. I think notifier_flag_changed() should be > passed both the old and new flags, to save the backend having to keep > track of the old ones - which flags have changed might affect what the > callback needs to do. Agree. That's exactly what v4 was doing. Thanks, -- peterx
diff --git a/hw/i386/intel_iommu.c b/hw/i386/intel_iommu.c index 28c31a2..c6bd8f6 100644 --- a/hw/i386/intel_iommu.c +++ b/hw/i386/intel_iommu.c @@ -1974,7 +1974,7 @@ static IOMMUTLBEntry vtd_iommu_translate(MemoryRegion *iommu, hwaddr addr, return ret; } -static void vtd_iommu_notify_started(MemoryRegion *iommu) +static void vtd_iommu_notifier_add(MemoryRegion *iommu, IOMMUNotifier *n) { VTDAddressSpace *vtd_as = container_of(iommu, VTDAddressSpace, iommu); @@ -2348,7 +2348,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.notifier_add = vtd_iommu_notifier_add; 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..99c83a4 100644 --- a/hw/ppc/spapr_iommu.c +++ b/hw/ppc/spapr_iommu.c @@ -156,14 +156,20 @@ static uint64_t spapr_tce_get_min_page_size(MemoryRegion *iommu) return 1ULL << tcet->page_shift; } -static void spapr_tce_notify_started(MemoryRegion *iommu) +static void spapr_tce_notifier_add(MemoryRegion *iommu, IOMMUNotifier *n) { - spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), true); + sPAPRTCETable *tcet = container_of(iommu, sPAPRTCETable, iommu); + if (tcet->iommu_users++ == 0) { + spapr_tce_set_need_vfio(tcet, true); + } } -static void spapr_tce_notify_stopped(MemoryRegion *iommu) +static void spapr_tce_notifier_del(MemoryRegion *iommu, IOMMUNotifier *n) { - spapr_tce_set_need_vfio(container_of(iommu, sPAPRTCETable, iommu), false); + sPAPRTCETable *tcet = container_of(iommu, sPAPRTCETable, iommu); + if (--tcet->iommu_users == 0) { + spapr_tce_set_need_vfio(tcet, false); + } } static int spapr_tce_table_post_load(void *opaque, int version_id) @@ -246,8 +252,8 @@ 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, + .notifier_add = spapr_tce_notifier_add, + .notifier_del = spapr_tce_notifier_del, }; static int spapr_tce_table_realize(DeviceState *dev) diff --git a/include/exec/memory.h b/include/exec/memory.h index 92f14db..9efcf1b 100644 --- a/include/exec/memory.h +++ b/include/exec/memory.h @@ -175,10 +175,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 someone registers to the notify list */ + void (*notifier_add)(MemoryRegion *iommu, IOMMUNotifier *n); + /* Called when someone unregisters from the notify list */ + void (*notifier_del)(MemoryRegion *iommu, IOMMUNotifier *n); }; typedef struct CoalescedMemoryRange CoalescedMemoryRange; diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h index caf7be9..08627ec 100644 --- a/include/hw/ppc/spapr.h +++ b/include/hw/ppc/spapr.h @@ -564,6 +564,7 @@ struct sPAPRTCETable { MemoryRegion root, iommu; struct VIOsPAPRDevice *vdev; /* for @bypass migration compatibility only */ QLIST_ENTRY(sPAPRTCETable) list; + int iommu_users; }; sPAPRTCETable *spapr_tce_find_by_liobn(target_ulong liobn); diff --git a/memory.c b/memory.c index 45a3902..d913043 100644 --- a/memory.c +++ b/memory.c @@ -1518,9 +1518,8 @@ void memory_region_register_iommu_notifier(MemoryRegion *mr, { /* We need to register for at least one bitfield */ assert(n->notifier_caps != IOMMU_NOTIFIER_NONE); - if (mr->iommu_ops->notify_started && - QLIST_EMPTY(&mr->iommu_notify)) { - mr->iommu_ops->notify_started(mr); + if (mr->iommu_ops->notifier_add) { + mr->iommu_ops->notifier_add(mr, n); } QLIST_INSERT_HEAD(&mr->iommu_notify, n, node); } @@ -1560,9 +1559,8 @@ 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); + if (mr->iommu_ops->notifier_del) { + mr->iommu_ops->notifier_del(mr, n); } }
Considering that we may have multiple IOMMU notifier consumers in the future, converting iommu_ops.notify_{started|stopped} into some more general form. Now we can trap all notifier registerations and deregistrations, rather than only the first ones. Power was leveraging the notifier_{started|stopped}, adding iommu_user field for counting on Power guests to achieve the same goal. Suggested-by: Paolo Bonzini <pbonzini@redhat.com> Signed-off-by: Peter Xu <peterx@redhat.com> --- hw/i386/intel_iommu.c | 4 ++-- hw/ppc/spapr_iommu.c | 18 ++++++++++++------ include/exec/memory.h | 8 ++++---- include/hw/ppc/spapr.h | 1 + memory.c | 10 ++++------ 5 files changed, 23 insertions(+), 18 deletions(-)