On Mon, Sep 05, 2016 at 11:56:12AM +0200, Paolo Bonzini wrote:
> Yeah, if you really want to have these semantics, you need to define an
> enum like this:
>
> IOMMU_NOTIFIER_NONE = -1,
> IOMMU_NOTIFIER_FLUSH = 0,
> IOMMU_NOTIFIER_CHANGED_ENTRY = 1,
>
> But I'm still not convinced of the exclusivity between "flush" and
> "entry changed" notifiers. If I saw the above, my first reaction would
> be that you need a bit mask:
>
> IOMMU_NOTIFIER_NONE = -1,
> IOMMU_NOTIFIER_FLUSH = 1,
> IOMMU_NOTIFIER_CHANGED_ENTRY = 2,
>
> But perhaps what you're looking for is to change the "notifier" to a
> "listener" like
>
> struct IOMMUListener {
> void (*flush)(IOMMUListener *);
> void (*entry_changed)(IOMMUListener *, IOMMUTLBEntry *);
> QLIST_ENTRY(IOMMUListener) node;
> };
>
> The patches can start with an IOMMUListener that only has the
> entry_changed callback and that replaces the current use of Notifier.
> Then notify_started and notify_stopped can be called on every notifier
> that is added/removed (see attached prototype), and the Intel IOMMU can
> simply reject registration of a listener that has a non-NULL
> iotlb_changed member.
Thanks for the quick prototyping. :-)
Maybe I haven't explained the idea very clearly, but device-IOTLB is
not a "flush" of whole device cache. It still needs a IOMMUTLBEntry,
and works just like how general IOMMU invalidations. E.g., we can do
device-IOTLB invalidation for a single 4K page.
However, I agree with you that the namings are confusing, maybe at
least we should introduce IOMMU_NOTIFIER_* macros, though instead of a
_FLUSH one, we can have:
IOMMU_NOTIFIER_NONE = -1,
IOMMU_NOTIFIER_DEVICE_INVALIDATE = 0,
IOMMU_NOTIFIER_IOTLB_CHANGED = 1,
To clarify that these are two non-overlapped cases.
Thanks,
-- peterx
