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

[Laurent Vivier <lvivier@redhat.com>]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1



On 24/09/2015 01:50, David Gibson wrote:
> On Wed, Sep 23, 2015 at 07:04:55PM +0200, Laurent Vivier wrote:
>> 
>> 
>> On 17/09/2015 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 <david@gibson.dropbear.id.au> --- 
>>> include/exec/memory.h | 16 ++++++++++++++++ memory.c
>>> | 18 ++++++++++++++++++ 2 files changed, 34 insertions(+)
>>> 
>>> diff --git a/include/exec/memory.h b/include/exec/memory.h 
>>> index 5baaf48..3cf145b 100644 --- a/include/exec/memory.h +++
>>> b/include/exec/memory.h @@ -583,6 +583,22 @@ void
>>> memory_region_notify_iommu(MemoryRegion *mr, void
>>> memory_region_register_iommu_notifier(MemoryRegion *mr,
>>> Notifier *n);
>>> 
>>> /** + * memory_region_register_iommu_notifier_replay: register
>>> a notifier + * for changes to IOMMU translation entries, and
>>> replay existing IOMMU + * translations to the new notifier. +
>>> * + * @mr: the memory region to observe + * @n: the notifier to
>>> be added; the notifier receives a pointer to an + *
>>> #IOMMUTLBEntry as the opaque value; the pointer ceases to be +
>>> *     valid on exit from the notifier. + * @granularity:
>>> Minimum page granularity to replay notifications for + *
>>> @is_write: Whether to treat the replay as a translate "write" +
>>> *     through the iommu + */ +void
>>> memory_region_register_iommu_notifier_replay(MemoryRegion *mr,
>>> Notifier *n, +
>>> hwaddr granularity, bool is_write); + +/** *
>>> memory_region_unregister_iommu_notifier: unregister a notifier
>>> for * changes to IOMMU translation entries. * 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) +{ +    hwaddr addr; +
>>> IOMMUTLBEntry iotlb; + +
>>> memory_region_register_iommu_notifier(mr, n); + +    for (addr
>>> = 0; +         int128_lt(int128_make64(addr), mr->size);
>> 
>> "addr < memory_region_size(mr)" should be enough.
> 
> Ah, yes, much neater, thanks.

but rethinking about that, you can have an infinite loop (with int128
or with memory_region_size()) if mr->size >= UINT64_MAX:

as hwaddr is a 64bit and a multiple of granularity which is a power of
two. the last value of addr is UINT64 + 1 - granularity, so the next
is (uint64_t)(UINT64 + 1), which is 0, so addr is never >= mr->size.

> 
>>> +         addr += granularity) { + +        iotlb =
>>> mr->iommu_ops->translate(mr, addr, is_write); +        if
>>> (iotlb.perm != IOMMU_NONE) +            n->notify(n, &iotlb); +
>>> } +} + void memory_region_unregister_iommu_notifier(Notifier
>>> *n) { notifier_remove(n);
>>> 
>> 
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iEYEARECAAYFAlYDob0ACgkQNKT2yavzbFOnFACcDk+2PHhX/WfCCkTdXKH4XhWi
UYcAoOpe+C+8tzX02VlGTsCAV9ZxiEwQ
=Cnfl
-----END PGP SIGNATURE-----