Re: [Qemu-devel] [PATCH v3 2/3] memory: generalize iommu_ops.notify_started to notifier_add

[Peter Xu]

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