Re: [Qemu-devel] [PATCH 5/7] memory: Allow replay of IOMMU mapping notifications

[Laurent Vivier <lvivier@redhat.com>]

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 <david@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,
>  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 ef87363..b4b6861 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; 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
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....

>  void memory_region_unregister_iommu_notifier(Notifier *n)
>  {
>      notifier_remove(n);
>