Re: [PATCH 091/104] libvhost-user: Fix some memtable remap cases

[Marc-André Lureau]

Hi

On Thu, Dec 12, 2019 at 10:05 PM Dr. David Alan Gilbert (git)
<address@hidden> wrote:
>
> From: "Dr. David Alan Gilbert" <address@hidden>
>
> If a new setmemtable command comes in once the vhost threads are
> running, it will remap the guests address space and the threads
> will now be looking in the wrong place.
>
> Fortunately we're running this command under lock, so we can
> update the queue mappings so that threads will look in the new-right
> place.
>
> Note: This doesn't fix things that the threads might be doing
> without a lock (e.g. a readv/writev!)  That's for another time.
>
> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> ---
>  contrib/libvhost-user/libvhost-user.c | 33 ++++++++++++++++++++-------
>  contrib/libvhost-user/libvhost-user.h |  3 +++
>  2 files changed, 28 insertions(+), 8 deletions(-)
>
> diff --git a/contrib/libvhost-user/libvhost-user.c 
> b/contrib/libvhost-user/libvhost-user.c
> index 63e41062a4..b89bf18501 100644
> --- a/contrib/libvhost-user/libvhost-user.c
> +++ b/contrib/libvhost-user/libvhost-user.c
> @@ -564,6 +564,21 @@ vu_reset_device_exec(VuDev *dev, VhostUserMsg *vmsg)
>      return false;
>  }
>
> +static bool
> +map_ring(VuDev *dev, VuVirtq *vq)
> +{
> +    vq->vring.desc = qva_to_va(dev, vq->vra.desc_user_addr);
> +    vq->vring.used = qva_to_va(dev, vq->vra.used_user_addr);
> +    vq->vring.avail = qva_to_va(dev, vq->vra.avail_user_addr);
> +
> +    DPRINT("Setting virtq addresses:\n");
> +    DPRINT("    vring_desc  at %p\n", vq->vring.desc);
> +    DPRINT("    vring_used  at %p\n", vq->vring.used);
> +    DPRINT("    vring_avail at %p\n", vq->vring.avail);
> +
> +    return !(vq->vring.desc && vq->vring.used && vq->vring.avail);
> +}
> +
>  static bool
>  vu_set_mem_table_exec_postcopy(VuDev *dev, VhostUserMsg *vmsg)
>  {
> @@ -767,6 +782,14 @@ vu_set_mem_table_exec(VuDev *dev, VhostUserMsg *vmsg)
>          close(vmsg->fds[i]);
>      }
>
> +    for (i = 0; i < dev->max_queues; i++) {
> +        if (dev->vq[i].vring.desc) {

The code usually checks for all (vq->vring.desc && vq->vring.used &&
vq->vring.avail).

Perhaps we should introduce a VQ_RING_IS_SET() macro?

> +            if (map_ring(dev, &dev->vq[i])) {
> +                vu_panic(dev, "remaping queue %d during setmemtable", i);
> +            }
> +        }
> +    }
> +
>      return false;
>  }
>
> @@ -853,18 +876,12 @@ vu_set_vring_addr_exec(VuDev *dev, VhostUserMsg *vmsg)
>      DPRINT("    avail_user_addr:  0x%016" PRIx64 "\n", vra->avail_user_addr);
>      DPRINT("    log_guest_addr:   0x%016" PRIx64 "\n", vra->log_guest_addr);
>
> +    vq->vra = *vra;
>      vq->vring.flags = vra->flags;
> -    vq->vring.desc = qva_to_va(dev, vra->desc_user_addr);
> -    vq->vring.used = qva_to_va(dev, vra->used_user_addr);
> -    vq->vring.avail = qva_to_va(dev, vra->avail_user_addr);
>      vq->vring.log_guest_addr = vra->log_guest_addr;
>
> -    DPRINT("Setting virtq addresses:\n");
> -    DPRINT("    vring_desc  at %p\n", vq->vring.desc);
> -    DPRINT("    vring_used  at %p\n", vq->vring.used);
> -    DPRINT("    vring_avail at %p\n", vq->vring.avail);
>
> -    if (!(vq->vring.desc && vq->vring.used && vq->vring.avail)) {
> +    if (map_ring(dev, vq)) {
>          vu_panic(dev, "Invalid vring_addr message");
>          return false;
>      }
> diff --git a/contrib/libvhost-user/libvhost-user.h 
> b/contrib/libvhost-user/libvhost-user.h
> index 1844b6f8d4..5cb7708559 100644
> --- a/contrib/libvhost-user/libvhost-user.h
> +++ b/contrib/libvhost-user/libvhost-user.h
> @@ -327,6 +327,9 @@ typedef struct VuVirtq {
>      int err_fd;
>      unsigned int enable;
>      bool started;
> +
> +    /* Guest addresses of our ring */
> +    struct vhost_vring_addr vra;
>  } VuVirtq;
>
>  enum VuWatchCondtion {
> --
> 2.23.0
>
>

Looks reasonable otherwise (assuming all running threads were flushed
somehow by other means)

-- 
Marc-André Lureau