Re: [Qemu-devel] [RFC PATCH 05/10] memory: Allow replay of IOMMU mapping notifications

[David Gibson]

On Wed, Sep 23, 2015 at 12:40:18PM +0200, Thomas Huth wrote:
> On 17/09/15 15:09, David Gibson wrote:
> > When we have guest visible IOMMUs, we allow notifiers to be registered
> > which will be informed of all changes to IOMMU mappings.  This is used by
> > vfio to keep the host IOMMU mappings in sync with guest IOMMU mappings.
> > 
> > However, unlike with a memory region listener, an iommu notifier won't be
> > told about any mappings which already exist in the (guest) IOMMU at the
> > time it is registered.  This can cause problems if hotplugging a VFIO
> > device onto a guest bus which had existing guest IOMMU mappings, but didn't
> > previously have an VFIO devices (and hence no host IOMMU mappings).
> > 
> > This adds a memory_region_register_iommu_notifier_replay() function to
> > handle this case.  As well as registering the new notifier it replays
> > existing mappings.  Because the IOMMU memory region doesn't internally
> > remember the granularity of the guest IOMMU it has a small hack where the
> > caller must specify a granularity at which to replay mappings.
> > 
> > If there are finer mappings in the guest IOMMU these will be reported in
> > the iotlb structures passed to the notifier which it must handle (probably
> > causing it to flag an error).  This isn't new - the VFIO iommu notifier
> > must already handle notifications about guest IOMMU mappings too short
> > for it to represent in the host IOMMU.
> > 
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> >  include/exec/memory.h | 16 ++++++++++++++++
> >  memory.c              | 18 ++++++++++++++++++
> >  2 files changed, 34 insertions(+)
> 
> > diff --git a/memory.c b/memory.c
> > index 0d8b2d9..6b5a2f1 100644
> > --- a/memory.c
> > +++ b/memory.c
> > @@ -1403,6 +1403,24 @@ void 
> > memory_region_register_iommu_notifier(MemoryRegion *mr, Notifier *n)
> >      notifier_list_add(&mr->iommu_notify, n);
> >  }
> >  
> > +void memory_region_register_iommu_notifier_replay(MemoryRegion *mr, 
> > Notifier *n,
> > +                                                  hwaddr granularity, bool 
> > is_write)
> 
> granularity itself is not an address, but a size, isn't it? So using
> "hwaddr" sounds wrong here.

As Laurent says, hwaddr is used for sizes through memory.c - I think
the rationale is that a size is a difference between two hwaddrs, so
should have the same type.

> > +{
> > +    hwaddr addr;
> 
> dma_addr_t ?

As mentioned in previus reply, I don't think so on consideration.

> > +    IOMMUTLBEntry iotlb;
> > +
> > +    memory_region_register_iommu_notifier(mr, n);
> > +
> > +    for (addr = 0;
> > +         int128_lt(int128_make64(addr), mr->size);
> > +         addr += granularity) {
> > +
> > +        iotlb = mr->iommu_ops->translate(mr, addr, is_write);
> > +        if (iotlb.perm != IOMMU_NONE)
> > +            n->notify(n, &iotlb);
> 
> Missing curly braces.

Thanks, fixed.

> 
> > +    }
> > +}
> > +
> 
>  Thomas
> 

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

pgpjFqCRUYF_7.pgp
Description: PGP signature