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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2626 bytes --]

On Mon, Sep 05, 2016 at 04:38:04PM +0800, Peter Xu wrote:
> 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:

It's not good, it's horrible.  This makes a hideous interface that's
just a collection of separate use cases without any coherent
underlying semantics.

> - 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.

Maybe not now, but a common use case is absolutely possible.  If you
had a single (guest) bus with a single IO address space, with vIOMMU
and both VFIO and vhost devices on it, the same vIOMMU would need to
send all notifications to VFIO and (at least) unmap notifications to
vhost.

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]