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

[Peter Xu]

On Tue, Sep 06, 2016 at 09:51:28AM +0200, Paolo Bonzini wrote:
> 
> 
> On 06/09/2016 07:27, Peter Xu wrote:
> > 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.
> 
> Yes, it can be FLUSHED_ENTRY and CHANGED_ENTRY or
> INVALIDATE_ENTRY/CHANGE_ENTRY.
> 
> > 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,
> 
> I suggest making the names more similar:
> 
> - two participles (invalidated/changed) or two imperatives
> (invalidate!/change!);
> 
> - choose whether to keep the verb first ("invalidate device") or keep
> the noun first ("IOTLB changed"), and stick with one convention.

Sensible. Will follow.

> 
> > To clarify that these are two non-overlapped cases.
> 
> If they are not overlapping, they really should be using a bitmask or
> multiple callbacks in a struct...

After knowing the possibility that the two consumers might be
mixturely used in the future (as David has mentioned), I'd vote for a
bitmask for notification type:

    IOMMU_NOTIFIER_NONE = 0,
    IOMMU_NOTIFIER_INVALIDATION = 1,
    IOMMU_NOTIFIER_ADDITION = 2,

When registering the IOMMU notifier, user should specify the type. For
VFIO, it should be (INVALIDATION|ADDITION). For vhost, (INVALIDATION)
would suffice.

Will cook a v2 soon. Thanks!

-- peterx