Re: [PATCH] vhost: Ignore vrings in dirty log when using a vIOMMU

[Greg Kurz <groug@kaod.org>]

Cc'ing Jason since this was detected using vhost-net.

On Fri, 25 Sep 2020 19:29:43 +0200
Greg Kurz <groug@kaod.org> wrote:

> When a vIOMMU is present, any address comming from the guest is an IO
> virtual address, including those of the vrings. The backend's accesses
> to the vrings happen through vIOMMU translation : the backend hence
> only logs the final guest physical address, not the IO virtual one.
> It thus doesn't make sense to make room for the vring addresses in the
> dirty log in this case.
> 
> This fixes a crash of the source when migrating a guest using in-kernel
> vhost-net and iommu_platform=on on POWER, because DMA regions are put
> at very high addresses and the resulting log size is likely to cause
> g_malloc0() to abort.
> 
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1879349
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/virtio/vhost.c |   38 ++++++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 1a1384e7a642..0b83d6b8e65e 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -106,6 +106,20 @@ static void vhost_dev_sync_region(struct vhost_dev *dev,
>      }
>  }
>  
> +static int vhost_dev_has_iommu(struct vhost_dev *dev)
> +{
> +    VirtIODevice *vdev = dev->vdev;
> +
> +    /*
> +     * For vhost, VIRTIO_F_IOMMU_PLATFORM means the backend support
> +     * incremental memory mapping API via IOTLB API. For platform that
> +     * does not have IOMMU, there's no need to enable this feature
> +     * which may cause unnecessary IOTLB miss/update trnasactions.
> +     */
> +    return vdev->dma_as != &address_space_memory &&
> +           virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
> +}
> +
>  static int vhost_sync_dirty_bitmap(struct vhost_dev *dev,
>                                     MemoryRegionSection *section,
>                                     hwaddr first,
> @@ -130,6 +144,11 @@ static int vhost_sync_dirty_bitmap(struct vhost_dev *dev,
>                                range_get_last(reg->guest_phys_addr,
>                                               reg->memory_size));
>      }
> +
> +    if (vhost_dev_has_iommu(dev)) {
> +        return 0;
> +    }
> +
>      for (i = 0; i < dev->nvqs; ++i) {
>          struct vhost_virtqueue *vq = dev->vqs + i;
>  
> @@ -172,6 +191,11 @@ static uint64_t vhost_get_log_size(struct vhost_dev *dev)
>                                         reg->memory_size);
>          log_size = MAX(log_size, last / VHOST_LOG_CHUNK + 1);
>      }
> +
> +    if (vhost_dev_has_iommu(dev)) {
> +        return log_size;
> +    }
> +
>      for (i = 0; i < dev->nvqs; ++i) {
>          struct vhost_virtqueue *vq = dev->vqs + i;
>  
> @@ -287,20 +311,6 @@ static inline void vhost_dev_log_resize(struct vhost_dev *dev, uint64_t size)
>      dev->log_size = size;
>  }
>  
> -static int vhost_dev_has_iommu(struct vhost_dev *dev)
> -{
> -    VirtIODevice *vdev = dev->vdev;
> -
> -    /*
> -     * For vhost, VIRTIO_F_IOMMU_PLATFORM means the backend support
> -     * incremental memory mapping API via IOTLB API. For platform that
> -     * does not have IOMMU, there's no need to enable this feature
> -     * which may cause unnecessary IOTLB miss/update trnasactions.
> -     */
> -    return vdev->dma_as != &address_space_memory &&
> -           virtio_host_has_feature(vdev, VIRTIO_F_IOMMU_PLATFORM);
> -}
> -
>  static void *vhost_memory_map(struct vhost_dev *dev, hwaddr addr,
>                                hwaddr *plen, bool is_write)
>  {
> 
> 
>