On Fri, Sep 25, 2020 at 07:29:43PM +0200, Greg Kurz 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 <gr...@kaod.org>
I'm a little confused as to what's going on here. Obviously
allocating dirty bitmaps in IOVA space doesn't make much sense.
But.. in all cases isn't the ring ending up in guest memory, whether
translated or not. So why do specific addresses of the ring make a
difference in *any* case.
> ---
> 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)
> {
>
>
--
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
signature.asc
Description: PGP signature
