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

[David Gibson]

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.

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature