Re: [PATCH 0/7] vhost-user: call VHOST_USER_SET_VRING_ENABLE synchronously

[Stefano Garzarella]

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