On Thu, Sep 24, 2015 at 06:08:59PM +0200, Laurent Vivier wrote:
>
>
> On 24/09/2015 06:33, 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 <da...@gibson.dropbear.id.au>
> > ---
> > include/exec/memory.h | 17 +++++++++++++++++
> > memory.c | 18 ++++++++++++++++++
> > 2 files changed, 35 insertions(+)
> >
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index 5baaf48..304f985 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -583,6 +583,23 @@ void memory_region_notify_iommu(MemoryRegion
> > *mr,
[snip]
> > +void memory_region_register_iommu_notifier_replay(MemoryRegion *mr,
> > Notifier *n,
> > + hwaddr granularity,
> > + bool is_write)
> > +{
> > + hwaddr addr;
> > + IOMMUTLBEntry iotlb;
> > +
> > + memory_region_register_iommu_notifier(mr, n);
> > +
> > + for (addr = 0; addr < memory_region_size(mr); addr += granularity) {
> > +
> > + iotlb = mr->iommu_ops->translate(mr, addr, is_write);
> > + if (iotlb.perm != IOMMU_NONE) {
> > + n->notify(n, &iotlb);
> > + }
> > + }
> > +}
>
> If mr->size > (UINT64_MAX + 1 - granularity), you run into an infinite
> loop because hwaddr is a 64bit value and the stop condition is beyond
> its max value. You can avoid this by using the power of 2 of the
Ugh, yes, and I think my old version with more int128s was still
wrong, too.
> granularity, instead of the granularity:
>
> int shift = ctz64(granularity);
> hwaddr size = memory_region_size(mr) >> shift;
> for (addr = 0; addr < size; addr++)
> {
> iotlb = mr->iommu_ops->translate(mr, addr << shift, is_write);
> ...
>
> so in patch 6, you should pass the power of 2 instead of the value of
> the granularity.
>
> Of course, it works if granularity is at least 2....
Hrm, rather clunky.
I've instead gone for putting this at the end of the loop body:
/* if (2^64 - MR size) < granularity, it's possible to get an
* infinite loop here. This should catch such a wraparound */
if ((addr + granularity) < addr) {
break;
}
Of course, unless granularity is huge, stepping through a whole 2^64
address space might be indistinguishable from an infinite loop in
practice..
--
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
pgpTwx4Vi_ouL.pgp
Description: PGP signature
