vhost: Don't special case vq->used_phys in vhost_get_log_size()

[PATCH] vhost: Don't special case vq->used_phys in vhost_get_log_size()

Posted by Greg Kurz 3 years, 7 months ago

The first loop in vhost_get_log_size() computes the size of the dirty log
bitmap so that it allows to track changes in the entire guest memory, in
terms of GPA.

When not using a vIOMMU, the address of the vring's used structure,
vq->used_phys, is a GPA. It is thus already covered by the first loop.

When using a vIOMMU, vq->used_phys is a GIOVA that will be translated
to an HVA when the vhost backend needs to update the used structure. It
will log the corresponding GPAs into the bitmap but it certainly won't
log the GIOVA.

So in any case, vq->used_phys shouldn't be explicitly used to size the
bitmap. Drop the second loop.

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
over 0x800000000000000ULL. The resulting insanely huge log size causes
g_malloc0() to abort.

BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1879349
Signed-off-by: Greg Kurz <groug@kaod.org>
---

This supersedes "vhost: Ignore vrings in dirty log when using a vIOMMU"

http://patchwork.ozlabs.org/project/qemu-devel/patch/160105498386.68108.2145229309875282336.stgit@bahia.lan/
---
 hw/virtio/vhost.c |   10 ----------
 1 file changed, 10 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 011951625442..c02b658b597f 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -172,16 +172,6 @@ static uint64_t vhost_get_log_size(struct vhost_dev *dev)
                                        reg->memory_size);
         log_size = MAX(log_size, last / VHOST_LOG_CHUNK + 1);
     }
-    for (i = 0; i < dev->nvqs; ++i) {
-        struct vhost_virtqueue *vq = dev->vqs + i;
-
-        if (!vq->used_phys && !vq->used_size) {
-            continue;
-        }
-
-        uint64_t last = vq->used_phys + vq->used_size - 1;
-        log_size = MAX(log_size, last / VHOST_LOG_CHUNK + 1);
-    }
     return log_size;
 }

Re: [PATCH] vhost: Don't special case vq->used_phys in vhost_get_log_size()

Posted by Jason Wang 3 years, 6 months ago

On 2020/10/8 上午12:30, Greg Kurz wrote:
> The first loop in vhost_get_log_size() computes the size of the dirty log
> bitmap so that it allows to track changes in the entire guest memory, in
> terms of GPA.
>
> When not using a vIOMMU, the address of the vring's used structure,
> vq->used_phys, is a GPA. It is thus already covered by the first loop.
>
> When using a vIOMMU, vq->used_phys is a GIOVA that will be translated
> to an HVA when the vhost backend needs to update the used structure. It
> will log the corresponding GPAs into the bitmap but it certainly won't
> log the GIOVA.
>
> So in any case, vq->used_phys shouldn't be explicitly used to size the
> bitmap. Drop the second loop.
>
> 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
> over 0x800000000000000ULL. The resulting insanely huge log size causes
> g_malloc0() to abort.
>
> BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1879349
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>
> This supersedes "vhost: Ignore vrings in dirty log when using a vIOMMU"
>
> http://patchwork.ozlabs.org/project/qemu-devel/patch/160105498386.68108.2145229309875282336.stgit@bahia.lan/
> ---
>   hw/virtio/vhost.c |   10 ----------
>   1 file changed, 10 deletions(-)
>
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 011951625442..c02b658b597f 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -172,16 +172,6 @@ static uint64_t vhost_get_log_size(struct vhost_dev *dev)
>                                          reg->memory_size);
>           log_size = MAX(log_size, last / VHOST_LOG_CHUNK + 1);
>       }
> -    for (i = 0; i < dev->nvqs; ++i) {
> -        struct vhost_virtqueue *vq = dev->vqs + i;
> -
> -        if (!vq->used_phys && !vq->used_size) {
> -            continue;
> -        }
> -
> -        uint64_t last = vq->used_phys + vq->used_size - 1;
> -        log_size = MAX(log_size, last / VHOST_LOG_CHUNK + 1);
> -    }
>       return log_size;
>   }
>   


Acked-by: Jason Wang <jasowang@redhat.com>



>

Re: [PATCH] vhost: Don't special case vq->used_phys in vhost_get_log_size()

Posted by Greg Kurz 3 years, 6 months ago

On Sat, 10 Oct 2020 11:30:28 +0800
Jason Wang <jasowang@redhat.com> wrote:

> 
> On 2020/10/8 上午12:30, Greg Kurz wrote:
> > The first loop in vhost_get_log_size() computes the size of the dirty log
> > bitmap so that it allows to track changes in the entire guest memory, in
> > terms of GPA.
> >
> > When not using a vIOMMU, the address of the vring's used structure,
> > vq->used_phys, is a GPA. It is thus already covered by the first loop.
> >
> > When using a vIOMMU, vq->used_phys is a GIOVA that will be translated
> > to an HVA when the vhost backend needs to update the used structure. It
> > will log the corresponding GPAs into the bitmap but it certainly won't
> > log the GIOVA.
> >
> > So in any case, vq->used_phys shouldn't be explicitly used to size the
> > bitmap. Drop the second loop.
> >
> > 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
> > over 0x800000000000000ULL. The resulting insanely huge log size causes
> > g_malloc0() to abort.
> >
> > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1879349
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >
> > This supersedes "vhost: Ignore vrings in dirty log when using a vIOMMU"
> >
> > http://patchwork.ozlabs.org/project/qemu-devel/patch/160105498386.68108.2145229309875282336.stgit@bahia.lan/
> > ---
> >   hw/virtio/vhost.c |   10 ----------
> >   1 file changed, 10 deletions(-)
> >
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 011951625442..c02b658b597f 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -172,16 +172,6 @@ static uint64_t vhost_get_log_size(struct vhost_dev *dev)
> >                                          reg->memory_size);
> >           log_size = MAX(log_size, last / VHOST_LOG_CHUNK + 1);
> >       }
> > -    for (i = 0; i < dev->nvqs; ++i) {
> > -        struct vhost_virtqueue *vq = dev->vqs + i;
> > -
> > -        if (!vq->used_phys && !vq->used_size) {
> > -            continue;
> > -        }
> > -
> > -        uint64_t last = vq->used_phys + vq->used_size - 1;
> > -        log_size = MAX(log_size, last / VHOST_LOG_CHUNK + 1);
> > -    }
> >       return log_size;
> >   }
> >   
> 
> 
> Acked-by: Jason Wang <jasowang@redhat.com>
> 

Ping ?

Re: [PATCH] vhost: Don't special case vq->used_phys in vhost_get_log_size()

Posted by Michael S. Tsirkin 3 years, 6 months ago

On Fri, Oct 23, 2020 at 01:10:06PM +0200, Greg Kurz wrote:
> On Sat, 10 Oct 2020 11:30:28 +0800
> Jason Wang <jasowang@redhat.com> wrote:
> 
> > 
> > On 2020/10/8 上午12:30, Greg Kurz wrote:
> > > The first loop in vhost_get_log_size() computes the size of the dirty log
> > > bitmap so that it allows to track changes in the entire guest memory, in
> > > terms of GPA.
> > >
> > > When not using a vIOMMU, the address of the vring's used structure,
> > > vq->used_phys, is a GPA. It is thus already covered by the first loop.
> > >
> > > When using a vIOMMU, vq->used_phys is a GIOVA that will be translated
> > > to an HVA when the vhost backend needs to update the used structure. It
> > > will log the corresponding GPAs into the bitmap but it certainly won't
> > > log the GIOVA.
> > >
> > > So in any case, vq->used_phys shouldn't be explicitly used to size the
> > > bitmap. Drop the second loop.
> > >
> > > 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
> > > over 0x800000000000000ULL. The resulting insanely huge log size causes
> > > g_malloc0() to abort.
> > >
> > > BugLink: https://bugzilla.redhat.com/show_bug.cgi?id=1879349
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > >
> > > This supersedes "vhost: Ignore vrings in dirty log when using a vIOMMU"
> > >
> > > http://patchwork.ozlabs.org/project/qemu-devel/patch/160105498386.68108.2145229309875282336.stgit@bahia.lan/
> > > ---
> > >   hw/virtio/vhost.c |   10 ----------
> > >   1 file changed, 10 deletions(-)
> > >
> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > index 011951625442..c02b658b597f 100644
> > > --- a/hw/virtio/vhost.c
> > > +++ b/hw/virtio/vhost.c
> > > @@ -172,16 +172,6 @@ static uint64_t vhost_get_log_size(struct vhost_dev *dev)
> > >                                          reg->memory_size);
> > >           log_size = MAX(log_size, last / VHOST_LOG_CHUNK + 1);
> > >       }
> > > -    for (i = 0; i < dev->nvqs; ++i) {
> > > -        struct vhost_virtqueue *vq = dev->vqs + i;
> > > -
> > > -        if (!vq->used_phys && !vq->used_size) {
> > > -            continue;
> > > -        }
> > > -
> > > -        uint64_t last = vq->used_phys + vq->used_size - 1;
> > > -        log_size = MAX(log_size, last / VHOST_LOG_CHUNK + 1);
> > > -    }
> > >       return log_size;
> > >   }
> > >   
> > 
> > 
> > Acked-by: Jason Wang <jasowang@redhat.com>
> > 
> 
> Ping ?

tagged, thanks!

-- 
MST