The last patch in the series states and fixes the problem; prior patches
only refactor the code.

Cc: "Michael S. Tsirkin" <mst@redhat.com> (supporter:vhost)
Cc: Eugenio Perez Martin <eperezma@redhat.com>
Cc: German Maglione <gmaglione@redhat.com>
Cc: Liu Jiang <gerry@linux.alibaba.com>
Cc: Sergio Lopez Pascual <slp@redhat.com>
Cc: Stefano Garzarella <sgarzare@redhat.com>

Thanks,
Laszlo

Laszlo Ersek (7):
  vhost-user: strip superfluous whitespace
  vhost-user: tighten "reply_supported" scope in "set_vring_addr"
  vhost-user: factor out "vhost_user_write_msg"
  vhost-user: flatten "enforce_reply" into "vhost_user_write_msg"
  vhost-user: hoist "write_msg", "get_features", "get_u64"
  vhost-user: allow "vhost_set_vring" to wait for a reply
  vhost-user: call VHOST_USER_SET_VRING_ENABLE synchronously

 hw/virtio/vhost-user.c | 202 +++++++++-----------
 1 file changed, 94 insertions(+), 108 deletions(-)


base-commit: 50e7a40af372ee5931c99ef7390f5d3d6fbf6ec4

On Sun, Aug 27, 2023 at 08:29:30PM +0200, Laszlo Ersek wrote:
>The last patch in the series states and fixes the problem; prior patches
>only refactor the code.

Thanks for the fix and great cleanup!

I fully reviewed the series and LGTM.

An additional step that we can take (not in this series) crossed my
mind, though. In some places we repeat the following pattern:

     vhost_user_write(dev, &msg, NULL, 0);
     ...

     if (reply_supported) {
         return process_message_reply(dev, &msg);
     }

So what about extending the vhost_user_write_msg() added in this series
to support also this cases and remove some code.
Or maybe integrate vhost_user_write_msg() in vhost_user_write().

I mean something like this (untested):

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 01e0ca90c5..9ee2a78afa 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -1130,13 +1130,19 @@ static int vhost_user_get_features(struct vhost_dev *dev, uint
64_t *features)
      return 0;
  }

+typedef enum {
+    NO_REPLY,
+    REPLY_IF_SUPPORTED,
+    REPLY_FORCED,
+} VhostUserReply;
+
  /* Note: "msg->hdr.flags" may be modified. */
  static int vhost_user_write_msg(struct vhost_dev *dev, VhostUserMsg *msg,
-                                bool wait_for_reply)
+                                VhostUserReply reply)
  {
      int ret;

-    if (wait_for_reply) {
+    if (reply != NO_REPLY) {
          bool reply_supported = virtio_has_feature(dev->protocol_features,
                                            VHOST_USER_PROTOCOL_F_REPLY_ACK);
          if (reply_supported) {
@@ -1149,7 +1155,7 @@ static int vhost_user_write_msg(struct vhost_dev *dev, VhostUserMsg *msg,
          return ret;
      }

-    if (wait_for_reply) {
+    if (reply != NO_REPLY) {
          uint64_t dummy;

          if (msg->hdr.flags & VHOST_USER_NEED_REPLY_MASK) {
@@ -1162,7 +1168,9 @@ static int vhost_user_write_msg(struct vhost_dev 
*dev, VhostUserMsg *msg,
          * Send VHOST_USER_GET_FEATURES which makes all backends
          * send a reply.
          */
-        return vhost_user_get_features(dev, &dummy);
+        if (reply == REPLY_FORCED) {
+            return vhost_user_get_features(dev, &dummy);
+        }
      }

      return 0;
@@ -2207,9 +2228,6 @@ static bool vhost_user_can_merge(struct vhost_dev *dev,
  static int vhost_user_net_set_mtu(struct vhost_dev *dev, uint16_t mtu)
  {
      VhostUserMsg msg;
-    bool reply_supported = virtio_has_feature(dev->protocol_features,
-                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
-    int ret;
  
      if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_NET_MTU))) {
          return 0;
@@ -2219,21 +2237,9 @@ static int vhost_user_net_set_mtu(struct vhost_dev *dev, uint16_t mtu)
      msg.payload.u64 = mtu;
      msg.hdr.size = sizeof(msg.payload.u64);
      msg.hdr.flags = VHOST_USER_VERSION;
-    if (reply_supported) {
-        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
-    }
-
-    ret = vhost_user_write(dev, &msg, NULL, 0);
-    if (ret < 0) {
-        return ret;
-    }
  
      /* If reply_ack supported, backend has to ack specified MTU is valid */
-    if (reply_supported) {
-        return process_message_reply(dev, &msg);
-    }
-
-    return 0;
+    return vhost_user_write_msg(dev, &msg, REPLY_IF_SUPPORTED);
  }
  
  static int vhost_user_send_device_iotlb_msg(struct vhost_dev *dev,
@@ -2313,10 +2319,7 @@ static int vhost_user_get_config(struct vhost_dev *dev, uint8_t *config,
  static int vhost_user_set_config(struct vhost_dev *dev, const uint8_t *data,
                                   uint32_t offset, uint32_t size, uint32_t flags)
  {
-    int ret;
      uint8_t *p;
-    bool reply_supported = virtio_has_feature(dev->protocol_features,
-                                              VHOST_USER_PROTOCOL_F_REPLY_ACK);
  
      VhostUserMsg msg = {
          .hdr.request = VHOST_USER_SET_CONFIG,
@@ -2329,10 +2332,6 @@ static int vhost_user_set_config(struct vhost_dev *dev, const uint8_t *data,
          return -ENOTSUP;
      }
  
-    if (reply_supported) {
-        msg.hdr.flags |= VHOST_USER_NEED_REPLY_MASK;
-    }
-
      if (size > VHOST_USER_MAX_CONFIG_SIZE) {
          return -EINVAL;
      }
@@ -2343,16 +2342,7 @@ static int vhost_user_set_config(struct vhost_dev *dev, const uint8_t *data,
      p = msg.payload.config.region;
      memcpy(p, data, size);
  
-    ret = vhost_user_write(dev, &msg, NULL, 0);
-    if (ret < 0) {
-        return ret;
-    }
-
-    if (reply_supported) {
-        return process_message_reply(dev, &msg);
-    }
-
-    return 0;
+    return vhost_user_write_msg(dev, &msg, REPLY_IF_SUPPORTED);
  }

Thanks,
Stefano

On 8/30/23 10:48, Stefano Garzarella wrote:
> On Sun, Aug 27, 2023 at 08:29:30PM +0200, Laszlo Ersek wrote:
>> The last patch in the series states and fixes the problem; prior patches
>> only refactor the code.
> 
> Thanks for the fix and great cleanup!
> 
> I fully reviewed the series and LGTM.
> 
> An additional step that we can take (not in this series) crossed my
> mind, though. In some places we repeat the following pattern:
> 
>     vhost_user_write(dev, &msg, NULL, 0);
>     ...
> 
>     if (reply_supported) {
>         return process_message_reply(dev, &msg);
>     }
> 
> So what about extending the vhost_user_write_msg() added in this series
> to support also this cases and remove some code.
> Or maybe integrate vhost_user_write_msg() in vhost_user_write().

Good idea, I'd just like someone else to do it -- and as you say, after
this series :)

This series is relatively packed with "thought" already (in the last
patch), plus a week ago I knew absolutely nothing about vhost /
vhost-user. (And, I read the whole blog series at
<https://www.redhat.com/en/virtio-networking-series> in 1-2 days, while
analyzing this issue, to understand the design of vhost.)

So I'd prefer keeping my first contribution in this area limited -- what
you are suggesting touches on some of the requests that require genuine
responses, and I didn't want to fiddle with those.

(I think your patch should be fine BTW!)

Laszlo