Re: [PATCH v3] vhost: make SET_VRING_ADDR, SET_FEATURES send replies

["Michael S. Tsirkin" <mst@redhat.com>]

Looks good. Some cosmetics:

On Mon, Aug 09, 2021 at 12:03:30PM +0300, Denis Plotnikov wrote:
> On vhost-user-blk migration, qemu normally sends a number of commands
> to enable logging if VHOST_USER_PROTOCOL_F_LOG_SHMFD is negotiated.
> Qemu sends VHOST_USER_SET_FEATURES to enable buffers logging and
> VHOST_USER_SET_VRING_ADDR per each started ring to enable "used ring"
> data logging.
> The issue is that qemu doesn't wait for reply from the vhost daemon
> for these commands which may result in races between qemu expectation
> of logging starting and actual login starting in vhost daemon.
> 
> The race can appear as follows: on migration setup, qemu enables dirty page
> logging by sending VHOST_USER_SET_FEATURES. The command doesn't arrive to a
> vhost-user-blk daemon immediately and the daemon needs some time to turn the
> logging on internally. If qemu doesn't wait for reply, after sending the
> command, qemu may start migrate memory pages to a destination. At this time,

start migrating

> the logging may not be actually turned on in the daemon but some guest pages,
> which the daemon is about to write to, may have already been transferred
> without logging to the destination. Since the logging wasn't turned on,
> those pages won't be transferred again as dirty. So we may end up with
> corrupted data on the destination.
> The same scenario is applicable for "used ring" data logging, which is
> turned on with VHOST_USER_SET_VRING_ADDR command.
> 
> To resolve this issue, this patch makes qemu wait for the commands result

command result

> explicilty if VHOST_USER_PROTOCOL_F_REPLY_ACK is negotiated and logging enabled.

typo

> 
> Signed-off-by: Denis Plotnikov <den-plotnikov@yandex-team.ru>
> 
> ---
> v2 -> v3:
>   * send VHOST_USER_GET_FEATURES to flush out outstanding messages [mst]
> 
> v1 -> v2:
>   * send reply only when logging is enabled [mst]
> 
> v0 -> v1:
>   * send reply for SET_VRING_ADDR, SET_FEATURES only [mst]
> ---
>  hw/virtio/vhost-user.c | 130 ++++++++++++++++++++++++++++-------------
>  1 file changed, 89 insertions(+), 41 deletions(-)
> 
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index ee57abe04526..18f685df549f 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -1095,23 +1095,6 @@ static int vhost_user_set_mem_table(struct vhost_dev *dev,
>      return 0;
>  }
>  
> -static int vhost_user_set_vring_addr(struct vhost_dev *dev,
> -                                     struct vhost_vring_addr *addr)
> -{
> -    VhostUserMsg msg = {
> -        .hdr.request = VHOST_USER_SET_VRING_ADDR,
> -        .hdr.flags = VHOST_USER_VERSION,
> -        .payload.addr = *addr,
> -        .hdr.size = sizeof(msg.payload.addr),
> -    };
> -
> -    if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
> -        return -1;
> -    }
> -
> -    return 0;
> -}
> -
>  static int vhost_user_set_vring_endian(struct vhost_dev *dev,
>                                         struct vhost_vring_state *ring)
>  {
> @@ -1288,72 +1271,137 @@ static int vhost_user_set_vring_call(struct vhost_dev *dev,
>      return vhost_set_vring_file(dev, VHOST_USER_SET_VRING_CALL, file);
>  }
>  
> -static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t u64)
> +
> +static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t *u64)
>  {
>      VhostUserMsg msg = {
>          .hdr.request = request,
>          .hdr.flags = VHOST_USER_VERSION,
> -        .payload.u64 = u64,
> -        .hdr.size = sizeof(msg.payload.u64),
>      };
>  
> +    if (vhost_user_one_time_request(request) && dev->vq_index != 0) {
> +        return 0;
> +    }
> +
>      if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
>          return -1;
>      }
>  
> +    if (vhost_user_read(dev, &msg) < 0) {
> +        return -1;
> +    }
> +
> +    if (msg.hdr.request != request) {
> +        error_report("Received unexpected msg type. Expected %d received %d",
> +                     request, msg.hdr.request);
> +        return -1;
> +    }
> +
> +    if (msg.hdr.size != sizeof(msg.payload.u64)) {
> +        error_report("Received bad msg size.");
> +        return -1;
> +    }
> +
> +    *u64 = msg.payload.u64;
> +
>      return 0;
>  }
>  
> -static int vhost_user_set_features(struct vhost_dev *dev,
> -                                   uint64_t features)
> +static int vhost_user_get_features(struct vhost_dev *dev, uint64_t *features)
>  {
> -    return vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES, features);
> +    return vhost_user_get_u64(dev, VHOST_USER_GET_FEATURES, features);
>  }
>  
> -static int vhost_user_set_protocol_features(struct vhost_dev *dev,
> -                                            uint64_t features)
> +static int enforce_reply(struct vhost_dev *dev)
>  {
> -    return vhost_user_set_u64(dev, VHOST_USER_SET_PROTOCOL_FEATURES, features);
> +   /*
> +    * we need a reply but can't get it from some command directly,

> +    * so send the command which must send a reply
> to make sure
> +    * the command we sent before is actually completed.


better:

We need to wait for a reply but the backend does not
support replies for the command we just sent.
Send VHOST_USER_GET_FEATURES which makes all backends
send a reply.


> +    */
> +    uint64_t dummy;

add an empty line here pls.

> +    return vhost_user_get_features(dev, &dummy);
>  }
>  
> -static int vhost_user_get_u64(struct vhost_dev *dev, int request, uint64_t *u64)
> +static int vhost_user_set_vring_addr(struct vhost_dev *dev,
> +                                     struct vhost_vring_addr *addr)
>  {
>      VhostUserMsg msg = {
> -        .hdr.request = request,
> +        .hdr.request = VHOST_USER_SET_VRING_ADDR,
>          .hdr.flags = VHOST_USER_VERSION,
> +        .payload.addr = *addr,
> +        .hdr.size = sizeof(msg.payload.addr),
>      };
>  
> -    if (vhost_user_one_time_request(request) && dev->vq_index != 0) {
> -        return 0;
> +    bool reply_supported = virtio_has_feature(dev->protocol_features,
> +                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
> +
> +    /* we need a reply anyway if logging is enabled */

better:

wait for a reply if logging is enabled to make sure backend is actually logging changes.

> +    bool need_reply = !!(addr->flags & (1 << VHOST_VRING_F_LOG));


Do we really need !! here? We are converting to bool here.

> +
> +    if (reply_supported && need_reply) {
> +        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
>      }
>  
>      if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
>          return -1;
>      }
>  
> -    if (vhost_user_read(dev, &msg) < 0) {
> -        return -1;
> +    if (msg.hdr.flags & VHOST_USER_NEED_REPLY_MASK) {
> +        return process_message_reply(dev, &msg);
> +    } else if (need_reply) {
> +        return enforce_reply(dev);
>      }

This logic is repeated in two places. How about moving the call
to process_message_reply into enforce_reply?


>  
> -    if (msg.hdr.request != request) {
> -        error_report("Received unexpected msg type. Expected %d received %d",
> -                     request, msg.hdr.request);
> -        return -1;
> +    return 0;
> +}
> +
> +static int vhost_user_set_u64(struct vhost_dev *dev, int request, uint64_t u64,
> +                              bool need_reply)

I think a better name would be "wait_for_reply": it's less about
needing the reply it's more about the wait.

> +{
> +    VhostUserMsg msg = {
> +        .hdr.request = request,
> +        .hdr.flags = VHOST_USER_VERSION,
> +        .payload.u64 = u64,
> +        .hdr.size = sizeof(msg.payload.u64),
> +    };
> +
> +    if (need_reply) {
> +        bool reply_supported = virtio_has_feature(dev->protocol_features,
> +                                          VHOST_USER_PROTOCOL_F_REPLY_ACK);
> +        if (reply_supported) {
> +            msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
> +        }
>      }
>  
> -    if (msg.hdr.size != sizeof(msg.payload.u64)) {
> -        error_report("Received bad msg size.");
> +    if (vhost_user_write(dev, &msg, NULL, 0) < 0) {
>          return -1;
>      }
>  
> -    *u64 = msg.payload.u64;
> +    if (msg.hdr.flags & VHOST_USER_NEED_REPLY_MASK) {
> +        return process_message_reply(dev, &msg);
> +    } else if (need_reply) {
> +        return enforce_reply(dev);
> +    }
>  
>      return 0;
>  }
>  
> -static int vhost_user_get_features(struct vhost_dev *dev, uint64_t *features)
> +static int vhost_user_set_features(struct vhost_dev *dev,
> +                                   uint64_t features)
>  {
> -    return vhost_user_get_u64(dev, VHOST_USER_GET_FEATURES, features);
> +    /* we need a reply anyway if logging is enabled */

better:

wait for a reply if logging is enabled to make sure backend is actually logging changes.


> +    bool log_enabled = !!(features & (0x1ULL << VHOST_F_LOG_ALL));


Do we need !! here?

> +
> +    return vhost_user_set_u64(dev, VHOST_USER_SET_FEATURES, features,
> +                              log_enabled);
> +}
> +
> +static int vhost_user_set_protocol_features(struct vhost_dev *dev,
> +                                            uint64_t features)
> +{
> +    return vhost_user_set_u64(dev, VHOST_USER_SET_PROTOCOL_FEATURES, features,
> +                              false);
>  }
>  
>  static int vhost_user_set_owner(struct vhost_dev *dev)
> -- 
> 2.25.1