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

[Peter Xu <peterx@redhat.com>]

On Mon, Sep 05, 2016 at 10:04:42AM +0200, Paolo Bonzini wrote:
> 
> 
> On 05/09/2016 09:21, Peter Xu wrote:
> >  void memory_region_notify_iommu(MemoryRegion *mr,
> > -                                IOMMUTLBEntry entry)
> > +                                IOMMUTLBEntry entry, IOMMUAccessFlags flag)
> >  {
> >      assert(memory_region_is_iommu(mr));
> > +    assert(flag == mr->iommu_notify_flag);
> >      notifier_list_notify(&mr->iommu_notify, &entry);
> >  }
> 
> Shouldn't it be possible to have IOMMU_RW and IOMMU_NONE on the same
> IOMMU, if the IOMMU supports IOMMU_RW at all?

Yeah, this is a good point...

If we see IOMMU_NONE as a subset of IOMMU_RW, we should allow notify
IOMMU_NONE even if the cached flag is IOMMU_RW.

However in this patch I was not meant to do that. I made it an
exclusive flag to identify two different use cases. I don't know
whether this is good, but at least for Intel IOMMU's current use case,
these two types should be totally isolated from each other:

- IOMMU_NONE notification is used by future DMAR-enabled vhost, it
  should only be notified with device-IOTLB invalidations, this will
  only require "Device IOTLB" capability for Intel IOMMUs, and be
  notified in Device IOTLB invalidation handlers.

- IOMMU_RW notifications should only be used for vfio-pci, notified
  with IOTLB invalidations. This will only require Cache Mode (CM=1)
  capability, and will be notified in common IOTLB invalidations (no
  matter whether it's an cache invalidation or not, we will all use
  IOMMU_RW flag for this kind of notifies).

Maybe here naming the flags as IOMMU_{RW_NONE} is a little bit
confusing (just to leverage existing access flags), but what I was
trying to do is to make the two things not overlapped at all, since I
didn't find a mixture use case.

Thanks,

-- peterx