Re: [Qemu-devel] [PATCH 2/3] memory: add iommu_notify_flag

[Peter Xu <peterx@redhat.com>]

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