[Qemu-devel] [PATCH v4 0/5] handle vhost reset/start/stop correctly

[Qemu-devel] [PATCH v4 0/5] handle vhost reset/start/stop correctly

[Yuanhan Liu]

Patch 1 rename RESET_DEVICE back to RESET_OWNER

Patch 2 introduced a new function: vhost_net_reset(), which is invoked
        when reset happens, say, driver is unloaded (unbinded).

        Michael suggested to do that only when MQ is not negotiated.
        However, reset happens no matter MQ is enabled or negotiated
        or not, and we should give a sign to backend to reset some
        device to a proper state after it happens.

        Note that the message sent is still RESET_OWNER. It might not
        be a good idea, but we could not simply rename it to RESET_DEVICE,
        and maybe adding another RESET_DEVICE might be better.

Patch 3 and 4 send SET_PROTOCOL_FEATURES at start, just like we send
        SET_FEATURES.

Patch 5 send SET_VRING_ENABLE at start/stop

        Michael, I intended to send it when MQ is negotiated as you suggested,
        however, I found that VHOST_USER_PROTOCOL_F_MQ is defined in vhost-user.c,
        which is not accessible to vhost.c.

        Exporting it to vhost.h will resolve that, however, it's not a good
        idea to move vhost user specific stuff to there. We could also introduce
        another vhost callback to test whether MQ is negotiated: I just don't
        think it's worthy.

        Hence, here I just used a simple test: invoke set_vring_enable() just
        when it is defined. Judging the callback self has another MQ check,
        I guess it's okay.

And sorry that it took so long to send this version.

---
Yuanhan Liu (5):
  vhost: rename RESET_DEVICE backto RESET_OWNER
  vhost: reset vhost net when virtio_net_reset happens
  vhost: introduce vhost_set/get_protocol_features callbacks
  vhost: send SET_PROTOCOL_FEATURES at start
  vhost: send SET_VRING_ENABLE at start/stop

 docs/specs/vhost-user.txt         |  4 ++--
 hw/net/vhost_net.c                | 20 ++++++++++++++------
 hw/net/virtio-net.c               | 14 ++++++++++++++
 hw/virtio/vhost-backend.c         |  2 +-
 hw/virtio/vhost-user.c            | 13 ++++++++++---
 hw/virtio/vhost.c                 | 17 +++++++++++++++++
 include/hw/virtio/vhost-backend.h |  6 ++++++
 include/net/vhost_net.h           |  1 +
 linux-headers/linux/vhost.h       |  2 +-
 tests/vhost-user-bridge.c         |  6 +++---
 tests/vhost-user-test.c           |  4 ++--
 11 files changed, 71 insertions(+), 18 deletions(-)

-- 
1.9.0

[Qemu-devel] [PATCH v4 1/5] vhost: rename RESET_DEVICE backto RESET_OWNER

[Yuanhan Liu]

This patch basically reverts commit d1f8b30e.

It turned out that it breaks stuff, so revert it:
    http://lists.nongnu.org/archive/html/qemu-devel/2015-10/msg00949.html

CC: "Michael S. Tsirkin" <mst@redhat.com>
Reported-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 docs/specs/vhost-user.txt   | 4 ++--
 hw/virtio/vhost-backend.c   | 2 +-
 hw/virtio/vhost-user.c      | 6 +++---
 linux-headers/linux/vhost.h | 2 +-
 tests/vhost-user-bridge.c   | 6 +++---
 tests/vhost-user-test.c     | 4 ++--
 6 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index e0d71e2..d319715 100644
--- a/docs/specs/vhost-user.txt
+++ b/docs/specs/vhost-user.txt
@@ -255,10 +255,10 @@ Message types
       as an owner of the session. This can be used on the Slave as a
       "session start" flag.
 
- * VHOST_USER_RESET_DEVICE
+ * VHOST_USER_RESET_OWNER
 
       Id: 4
-      Equivalent ioctl: VHOST_RESET_DEVICE
+      Equivalent ioctl: VHOST_RESET_OWNER
       Master payload: N/A
 
       Issued when a new connection is about to be closed. The Master will no
diff --git a/hw/virtio/vhost-backend.c b/hw/virtio/vhost-backend.c
index 1d5f684..b734a60 100644
--- a/hw/virtio/vhost-backend.c
+++ b/hw/virtio/vhost-backend.c
@@ -156,7 +156,7 @@ static int vhost_kernel_set_owner(struct vhost_dev *dev)
 
 static int vhost_kernel_reset_device(struct vhost_dev *dev)
 {
-    return vhost_kernel_call(dev, VHOST_RESET_DEVICE, NULL);
+    return vhost_kernel_call(dev, VHOST_RESET_OWNER, NULL);
 }
 
 static int vhost_kernel_get_vq_index(struct vhost_dev *dev, int idx)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 83c84f1..4766f98 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -43,7 +43,7 @@ typedef enum VhostUserRequest {
     VHOST_USER_GET_FEATURES = 1,
     VHOST_USER_SET_FEATURES = 2,
     VHOST_USER_SET_OWNER = 3,
-    VHOST_USER_RESET_DEVICE = 4,
+    VHOST_USER_RESET_OWNER = 4,
     VHOST_USER_SET_MEM_TABLE = 5,
     VHOST_USER_SET_LOG_BASE = 6,
     VHOST_USER_SET_LOG_FD = 7,
@@ -157,7 +157,7 @@ static bool vhost_user_one_time_request(VhostUserRequest request)
 {
     switch (request) {
     case VHOST_USER_SET_OWNER:
-    case VHOST_USER_RESET_DEVICE:
+    case VHOST_USER_RESET_OWNER:
     case VHOST_USER_SET_MEM_TABLE:
     case VHOST_USER_GET_QUEUE_NUM:
         return true;
@@ -486,7 +486,7 @@ static int vhost_user_set_owner(struct vhost_dev *dev)
 static int vhost_user_reset_device(struct vhost_dev *dev)
 {
     VhostUserMsg msg = {
-        .request = VHOST_USER_RESET_DEVICE,
+        .request = VHOST_USER_RESET_OWNER,
         .flags = VHOST_USER_VERSION,
     };
 
diff --git a/linux-headers/linux/vhost.h b/linux-headers/linux/vhost.h
index 14a0160..ead86db 100644
--- a/linux-headers/linux/vhost.h
+++ b/linux-headers/linux/vhost.h
@@ -78,7 +78,7 @@ struct vhost_memory {
 #define VHOST_SET_OWNER _IO(VHOST_VIRTIO, 0x01)
 /* Give up ownership, and reset the device to default values.
  * Allows subsequent call to VHOST_OWNER_SET to succeed. */
-#define VHOST_RESET_DEVICE _IO(VHOST_VIRTIO, 0x02)
+#define VHOST_RESET_OWNER _IO(VHOST_VIRTIO, 0x02)
 
 /* Set up/modify memory layout */
 #define VHOST_SET_MEM_TABLE	_IOW(VHOST_VIRTIO, 0x03, struct vhost_memory)
diff --git a/tests/vhost-user-bridge.c b/tests/vhost-user-bridge.c
index fa18ad5..864f69e 100644
--- a/tests/vhost-user-bridge.c
+++ b/tests/vhost-user-bridge.c
@@ -188,7 +188,7 @@ typedef enum VhostUserRequest {
     VHOST_USER_GET_FEATURES = 1,
     VHOST_USER_SET_FEATURES = 2,
     VHOST_USER_SET_OWNER = 3,
-    VHOST_USER_RESET_DEVICE = 4,
+    VHOST_USER_RESET_OWNER = 4,
     VHOST_USER_SET_MEM_TABLE = 5,
     VHOST_USER_SET_LOG_BASE = 6,
     VHOST_USER_SET_LOG_FD = 7,
@@ -274,7 +274,7 @@ static const char *vubr_request_str[] = {
     [VHOST_USER_GET_FEATURES]           =  "VHOST_USER_GET_FEATURES",
     [VHOST_USER_SET_FEATURES]           =  "VHOST_USER_SET_FEATURES",
     [VHOST_USER_SET_OWNER]              =  "VHOST_USER_SET_OWNER",
-    [VHOST_USER_RESET_DEVICE]           =  "VHOST_USER_RESET_DEVICE",
+    [VHOST_USER_RESET_OWNER]           =  "VHOST_USER_RESET_OWNER",
     [VHOST_USER_SET_MEM_TABLE]          =  "VHOST_USER_SET_MEM_TABLE",
     [VHOST_USER_SET_LOG_BASE]           =  "VHOST_USER_SET_LOG_BASE",
     [VHOST_USER_SET_LOG_FD]             =  "VHOST_USER_SET_LOG_FD",
@@ -921,7 +921,7 @@ vubr_execute_request(VubrDev *dev, VhostUserMsg *vmsg)
         return vubr_set_features_exec(dev, vmsg);
     case VHOST_USER_SET_OWNER:
         return vubr_set_owner_exec(dev, vmsg);
-    case VHOST_USER_RESET_DEVICE:
+    case VHOST_USER_RESET_OWNER:
         return vubr_reset_device_exec(dev, vmsg);
     case VHOST_USER_SET_MEM_TABLE:
         return vubr_set_mem_table_exec(dev, vmsg);
diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index b6dde75..66778c4 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -57,7 +57,7 @@ typedef enum VhostUserRequest {
     VHOST_USER_GET_FEATURES = 1,
     VHOST_USER_SET_FEATURES = 2,
     VHOST_USER_SET_OWNER = 3,
-    VHOST_USER_RESET_DEVICE = 4,
+    VHOST_USER_RESET_OWNER = 4,
     VHOST_USER_SET_MEM_TABLE = 5,
     VHOST_USER_SET_LOG_BASE = 6,
     VHOST_USER_SET_LOG_FD = 7,
@@ -307,7 +307,7 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
         g_cond_signal(&s->data_cond);
         break;
 
-    case VHOST_USER_RESET_DEVICE:
+    case VHOST_USER_RESET_OWNER:
         s->fds_num = 0;
         break;
 
-- 
1.9.0

[Qemu-devel] [PATCH v4 2/5] vhost: reset vhost net when virtio_net_reset happens

[Yuanhan Liu]

When a virtio net driver is unloaded (unbind), virtio_net_reset happens.
For vhost net, we should also send a message (RESET_OWNER) to the backend
to do some proper reset settings.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 hw/net/vhost_net.c      | 20 ++++++++++++++------
 hw/net/virtio-net.c     | 14 ++++++++++++++
 include/net/vhost_net.h |  1 +
 3 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index d91b7b1..387f851 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -292,12 +292,6 @@ static void vhost_net_stop_one(struct vhost_net *net,
             int r = vhost_ops->vhost_net_set_backend(&net->dev, &file);
             assert(r >= 0);
         }
-    } else if (net->nc->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER) {
-        for (file.index = 0; file.index < net->dev.nvqs; ++file.index) {
-            const VhostOps *vhost_ops = net->dev.vhost_ops;
-            int r = vhost_ops->vhost_reset_device(&net->dev);
-            assert(r >= 0);
-        }
     }
     if (net->nc->info->poll) {
         net->nc->info->poll(net->nc, true);
@@ -382,6 +376,16 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
     assert(vhost_net_set_vnet_endian(dev, ncs[0].peer, false) >= 0);
 }
 
+void vhost_net_reset(VirtIODevice *vdev)
+{
+    VirtIONet *n = VIRTIO_NET(vdev);
+    struct vhost_net *net = get_vhost_net(n->nic->ncs[0].peer);
+
+    if (net->nc->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER) {
+        net->dev.vhost_ops->vhost_reset_device(&net->dev);
+    }
+}
+
 void vhost_net_cleanup(struct vhost_net *net)
 {
     vhost_dev_cleanup(&net->dev);
@@ -469,6 +473,10 @@ void vhost_net_stop(VirtIODevice *dev,
 {
 }
 
+void vhost_net_reset(VirtIODevice *vdev)
+{
+}
+
 void vhost_net_cleanup(struct vhost_net *net)
 {
 }
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index a877614..75472ba 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -318,10 +318,24 @@ static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc)
     return info;
 }
 
+static void virtio_net_vhost_reset(VirtIONet *n)
+{
+    VirtIODevice *vdev = VIRTIO_DEVICE(n);
+    NetClientState *nc = qemu_get_queue(n->nic);
+
+    if (!get_vhost_net(nc->peer)) {
+        return;
+    }
+
+    vhost_net_reset(vdev);
+}
+
 static void virtio_net_reset(VirtIODevice *vdev)
 {
     VirtIONet *n = VIRTIO_NET(vdev);
 
+    virtio_net_vhost_reset(n);
+
     /* Reset back to compatibility mode */
     n->promisc = 1;
     n->allmulti = 0;
diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
index 3389b41..d1c1331 100644
--- a/include/net/vhost_net.h
+++ b/include/net/vhost_net.h
@@ -18,6 +18,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options);
 
 int vhost_net_start(VirtIODevice *dev, NetClientState *ncs, int total_queues);
 void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs, int total_queues);
+void vhost_net_reset(VirtIODevice *dev);
 
 void vhost_net_cleanup(VHostNetState *net);
 
-- 
1.9.0

Re: [Qemu-devel] [PATCH v4 2/5] vhost: reset vhost net when virtio_net_reset happens

[Michael S. Tsirkin]

On Wed, Nov 11, 2015 at 09:24:38PM +0800, Yuanhan Liu wrote:
> When a virtio net driver is unloaded (unbind), virtio_net_reset happens.
> For vhost net, we should also send a message (RESET_OWNER) to the backend
> to do some proper reset settings.
> 
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

How about we just skip RESET_OWNER completely?
After all, modern clients have VRING_ENABLE.

> ---
>  hw/net/vhost_net.c      | 20 ++++++++++++++------
>  hw/net/virtio-net.c     | 14 ++++++++++++++
>  include/net/vhost_net.h |  1 +
>  3 files changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index d91b7b1..387f851 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -292,12 +292,6 @@ static void vhost_net_stop_one(struct vhost_net *net,
>              int r = vhost_ops->vhost_net_set_backend(&net->dev, &file);
>              assert(r >= 0);
>          }
> -    } else if (net->nc->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER) {
> -        for (file.index = 0; file.index < net->dev.nvqs; ++file.index) {
> -            const VhostOps *vhost_ops = net->dev.vhost_ops;
> -            int r = vhost_ops->vhost_reset_device(&net->dev);
> -            assert(r >= 0);
> -        }
>      }
>      if (net->nc->info->poll) {
>          net->nc->info->poll(net->nc, true);
> @@ -382,6 +376,16 @@ void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs,
>      assert(vhost_net_set_vnet_endian(dev, ncs[0].peer, false) >= 0);
>  }
>  
> +void vhost_net_reset(VirtIODevice *vdev)
> +{
> +    VirtIONet *n = VIRTIO_NET(vdev);
> +    struct vhost_net *net = get_vhost_net(n->nic->ncs[0].peer);
> +
> +    if (net->nc->info->type == NET_CLIENT_OPTIONS_KIND_VHOST_USER) {
> +        net->dev.vhost_ops->vhost_reset_device(&net->dev);
> +    }
> +}
> +
>  void vhost_net_cleanup(struct vhost_net *net)
>  {
>      vhost_dev_cleanup(&net->dev);
> @@ -469,6 +473,10 @@ void vhost_net_stop(VirtIODevice *dev,
>  {
>  }
>  
> +void vhost_net_reset(VirtIODevice *vdev)
> +{
> +}
> +
>  void vhost_net_cleanup(struct vhost_net *net)
>  {
>  }
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index a877614..75472ba 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -318,10 +318,24 @@ static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc)
>      return info;
>  }
>  
> +static void virtio_net_vhost_reset(VirtIONet *n)
> +{
> +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
> +    NetClientState *nc = qemu_get_queue(n->nic);
> +
> +    if (!get_vhost_net(nc->peer)) {
> +        return;
> +    }
> +
> +    vhost_net_reset(vdev);
> +}
> +
>  static void virtio_net_reset(VirtIODevice *vdev)
>  {
>      VirtIONet *n = VIRTIO_NET(vdev);
>  
> +    virtio_net_vhost_reset(n);
> +
>      /* Reset back to compatibility mode */
>      n->promisc = 1;
>      n->allmulti = 0;
> diff --git a/include/net/vhost_net.h b/include/net/vhost_net.h
> index 3389b41..d1c1331 100644
> --- a/include/net/vhost_net.h
> +++ b/include/net/vhost_net.h
> @@ -18,6 +18,7 @@ struct vhost_net *vhost_net_init(VhostNetOptions *options);
>  
>  int vhost_net_start(VirtIODevice *dev, NetClientState *ncs, int total_queues);
>  void vhost_net_stop(VirtIODevice *dev, NetClientState *ncs, int total_queues);
> +void vhost_net_reset(VirtIODevice *dev);
>  
>  void vhost_net_cleanup(VHostNetState *net);
>  
> -- 
> 1.9.0

[Qemu-devel] [PATCH v4 3/5] vhost: introduce vhost_set/get_protocol_features callbacks

[Yuanhan Liu]

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 hw/virtio/vhost-user.c            | 7 +++++++
 include/hw/virtio/vhost-backend.h | 6 ++++++
 2 files changed, 13 insertions(+)

diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 4766f98..2d8bdbd 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -471,6 +471,11 @@ static int vhost_user_get_features(struct vhost_dev *dev, uint64_t *features)
     return vhost_user_get_u64(dev, VHOST_USER_GET_FEATURES, features);
 }
 
+static int vhost_user_get_protocol_features(struct vhost_dev *dev, uint64_t *features)
+{
+    return vhost_user_get_u64(dev, VHOST_USER_GET_PROTOCOL_FEATURES, features);
+}
+
 static int vhost_user_set_owner(struct vhost_dev *dev)
 {
     VhostUserMsg msg = {
@@ -616,6 +621,8 @@ const VhostOps user_ops = {
         .vhost_set_vring_call = vhost_user_set_vring_call,
         .vhost_set_features = vhost_user_set_features,
         .vhost_get_features = vhost_user_get_features,
+        .vhost_set_protocol_features = vhost_user_set_protocol_features,
+        .vhost_get_protocol_features = vhost_user_get_protocol_features,
         .vhost_set_owner = vhost_user_set_owner,
         .vhost_reset_device = vhost_user_reset_device,
         .vhost_get_vq_index = vhost_user_get_vq_index,
diff --git a/include/hw/virtio/vhost-backend.h b/include/hw/virtio/vhost-backend.h
index c59cc81..7e705ce 100644
--- a/include/hw/virtio/vhost-backend.h
+++ b/include/hw/virtio/vhost-backend.h
@@ -62,6 +62,10 @@ typedef int (*vhost_set_features_op)(struct vhost_dev *dev,
                                      uint64_t features);
 typedef int (*vhost_get_features_op)(struct vhost_dev *dev,
                                      uint64_t *features);
+typedef int (*vhost_set_protocol_features_op)(struct vhost_dev *dev,
+                                              uint64_t features);
+typedef int (*vhost_get_protocol_features_op)(struct vhost_dev *dev,
+                                              uint64_t *features);
 typedef int (*vhost_set_owner_op)(struct vhost_dev *dev);
 typedef int (*vhost_reset_device_op)(struct vhost_dev *dev);
 typedef int (*vhost_get_vq_index_op)(struct vhost_dev *dev, int idx);
@@ -91,6 +95,8 @@ typedef struct VhostOps {
     vhost_set_vring_call_op vhost_set_vring_call;
     vhost_set_features_op vhost_set_features;
     vhost_get_features_op vhost_get_features;
+    vhost_set_protocol_features_op vhost_set_protocol_features;
+    vhost_get_protocol_features_op vhost_get_protocol_features;
     vhost_set_owner_op vhost_set_owner;
     vhost_reset_device_op vhost_reset_device;
     vhost_get_vq_index_op vhost_get_vq_index;
-- 
1.9.0

[Qemu-devel] [PATCH v4 4/5] vhost: send SET_PROTOCOL_FEATURES at start

[Yuanhan Liu]

So that the backend can restore the protocol features after a reset.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 hw/virtio/vhost.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index de29968..be48511 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1195,6 +1195,14 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
     if (r < 0) {
         goto fail_features;
     }
+    if (hdev->vhost_ops->vhost_set_protocol_features) {
+        r = hdev->vhost_ops->vhost_set_protocol_features(hdev,
+                hdev->protocol_features);
+        if (r < 0) {
+            goto fail_features;
+        }
+    }
+
     r = hdev->vhost_ops->vhost_set_mem_table(hdev, hdev->mem);
     if (r < 0) {
         r = -errno;
-- 
1.9.0

[Qemu-devel] [PATCH v4 5/5] vhost: send SET_VRING_ENABLE at start/stop

[Yuanhan Liu]

Send SET_VRING_ENABLE at start/stop, to give the backend
an explicit sign of our state.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 hw/virtio/vhost.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index be48511..0e956b5 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1234,6 +1234,11 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
         }
     }
 
+    if (hdev->vhost_ops->vhost_set_vring_enable) {
+        /* only enable first vq pair by default */
+        hdev->vhost_ops->vhost_set_vring_enable(hdev, hdev->vq_index == 0);
+    }
+
     return 0;
 fail_log:
     vhost_log_put(hdev, false);
@@ -1264,6 +1269,10 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
                              hdev->vq_index + i);
     }
 
+    if (hdev->vhost_ops->vhost_set_vring_enable) {
+        hdev->vhost_ops->vhost_set_vring_enable(hdev, 0);
+    }
+
     vhost_log_put(hdev, true);
     hdev->started = false;
     hdev->log = NULL;
-- 
1.9.0

Re: [Qemu-devel] [PATCH v4 0/5] handle vhost reset/start/stop correctly

[Michael S. Tsirkin]

On Wed, Nov 11, 2015 at 09:24:36PM +0800, Yuanhan Liu wrote:
> 
> Patch 1 rename RESET_DEVICE back to RESET_OWNER
> 
> Patch 2 introduced a new function: vhost_net_reset(), which is invoked
>         when reset happens, say, driver is unloaded (unbinded).
> 
>         Michael suggested to do that only when MQ is not negotiated.
>         However, reset happens no matter MQ is enabled or negotiated
>         or not, and we should give a sign to backend to reset some
>         device to a proper state after it happens.

I don't think it's needed: we set all state at start anyway.

>         Note that the message sent is still RESET_OWNER. It might not
>         be a good idea, but we could not simply rename it to RESET_DEVICE,
>         and maybe adding another RESET_DEVICE might be better.

So modern clients don't need this at all.  Old clients need something to
stop device, but singling out reset is not a good idea: even if driver
is unloaded, you need to do that.  snabbswitch (ab)uses RESET_OWNER for
this, so maybe RESET_OWNER should be renamed DISABLE_ALL, to just make
it stop all queues.

Does any dpdk version that was released respond to RESET_OWNER in some
way? How exactly?


> Patch 3 and 4 send SET_PROTOCOL_FEATURES at start, just like we send
>         SET_FEATURES.
> 
> Patch 5 send SET_VRING_ENABLE at start/stop
> 
>         Michael, I intended to send it when MQ is negotiated as you suggested,
>         however, I found that VHOST_USER_PROTOCOL_F_MQ is defined in vhost-user.c,
>         which is not accessible to vhost.c.
> 
>         Exporting it to vhost.h will resolve that, however, it's not a good
>         idea to move vhost user specific stuff to there. We could also introduce
>         another vhost callback to test whether MQ is negotiated: I just don't
>         think it's worthy.
> 
>         Hence, here I just used a simple test: invoke set_vring_enable() just
>         when it is defined. Judging the callback self has another MQ check,
>         I guess it's okay.
> 
> And sorry that it took so long to send this version.

Hmm, you are saying everyone needs SET_VRING_ENABLE?
Maybe we should make SET_VRING_ENABLE depend on protocol features then,
and not MQ?

I applied patches 1 and 5 for now.

Thanks!

> ---
> Yuanhan Liu (5):
>   vhost: rename RESET_DEVICE backto RESET_OWNER
>   vhost: reset vhost net when virtio_net_reset happens
>   vhost: introduce vhost_set/get_protocol_features callbacks
>   vhost: send SET_PROTOCOL_FEATURES at start
>   vhost: send SET_VRING_ENABLE at start/stop
> 
>  docs/specs/vhost-user.txt         |  4 ++--
>  hw/net/vhost_net.c                | 20 ++++++++++++++------
>  hw/net/virtio-net.c               | 14 ++++++++++++++
>  hw/virtio/vhost-backend.c         |  2 +-
>  hw/virtio/vhost-user.c            | 13 ++++++++++---
>  hw/virtio/vhost.c                 | 17 +++++++++++++++++
>  include/hw/virtio/vhost-backend.h |  6 ++++++
>  include/net/vhost_net.h           |  1 +
>  linux-headers/linux/vhost.h       |  2 +-
>  tests/vhost-user-bridge.c         |  6 +++---
>  tests/vhost-user-test.c           |  4 ++--
>  11 files changed, 71 insertions(+), 18 deletions(-)
> 
> -- 
> 1.9.0

Re: [Qemu-devel] [PATCH v4 0/5] handle vhost reset/start/stop correctly

[Yuanhan Liu]

On Thu, Nov 12, 2015 at 03:33:41PM +0200, Michael S. Tsirkin wrote:
> On Wed, Nov 11, 2015 at 09:24:36PM +0800, Yuanhan Liu wrote:
> > 
> > Patch 1 rename RESET_DEVICE back to RESET_OWNER
> > 
> > Patch 2 introduced a new function: vhost_net_reset(), which is invoked
> >         when reset happens, say, driver is unloaded (unbinded).
> > 
> >         Michael suggested to do that only when MQ is not negotiated.
> >         However, reset happens no matter MQ is enabled or negotiated
> >         or not, and we should give a sign to backend to reset some
> >         device to a proper state after it happens.
> 
> I don't think it's needed: we set all state at start anyway.

Agree with that.

> 
> >         Note that the message sent is still RESET_OWNER. It might not
> >         be a good idea, but we could not simply rename it to RESET_DEVICE,
> >         and maybe adding another RESET_DEVICE might be better.
> 
> So modern clients don't need this at all.  Old clients need something to
> stop device, but singling out reset is not a good idea: even if driver
> is unloaded, you need to do that.  snabbswitch (ab)uses RESET_OWNER for
> this, so maybe RESET_OWNER should be renamed DISABLE_ALL, to just make
> it stop all queues.
> 
> Does any dpdk version that was released respond to RESET_OWNER in some
> way? How exactly?

It just resets some states, such as closing call fd and kick fd,
unmapping buf from hugetlbfs from set_mem_table.

And, apparently we could do that on stop, too. So, from this pov, we
don't need RESET_OWNER.

> 
> > Patch 3 and 4 send SET_PROTOCOL_FEATURES at start, just like we send
> >         SET_FEATURES.
> > 
> > Patch 5 send SET_VRING_ENABLE at start/stop
> > 
> >         Michael, I intended to send it when MQ is negotiated as you suggested,
> >         however, I found that VHOST_USER_PROTOCOL_F_MQ is defined in vhost-user.c,
> >         which is not accessible to vhost.c.
> > 
> >         Exporting it to vhost.h will resolve that, however, it's not a good
> >         idea to move vhost user specific stuff to there. We could also introduce
> >         another vhost callback to test whether MQ is negotiated: I just don't
> >         think it's worthy.
> > 
> >         Hence, here I just used a simple test: invoke set_vring_enable() just
> >         when it is defined. Judging the callback self has another MQ check,
> >         I guess it's okay.
> > 
> > And sorry that it took so long to send this version.
> 
> Hmm, you are saying everyone needs SET_VRING_ENABLE?
> Maybe we should make SET_VRING_ENABLE depend on protocol features then,
> and not MQ?

I'm thinking something same. Otherwise, there is still no way to inform
the backend (or client) when a vhost dev is stopped when MQ is disabled
(which is the default state).

So, let's assume all clients have protocol features enabled, and send
SET_VRING_ENABLE at start/stop? And if it does not, it's just like we
are back to QEMU v2.3, where no RESET_OWNER nor SET_VRING_ENABLE
messages are sent on stop: it worked before, and it should also work
now.

> 
> I applied patches 1 and 5 for now.

Do you have comment about patch 3 and 4? Should we set protocol features
just like we set features at start?

Thanks.

	--yliu

Re: [Qemu-devel] [PATCH v4 0/5] handle vhost reset/start/stop correctly

[Michael S. Tsirkin]

On Thu, Nov 12, 2015 at 10:08:15PM +0800, Yuanhan Liu wrote:
> On Thu, Nov 12, 2015 at 03:33:41PM +0200, Michael S. Tsirkin wrote:
> > On Wed, Nov 11, 2015 at 09:24:36PM +0800, Yuanhan Liu wrote:
> > > 
> > > Patch 1 rename RESET_DEVICE back to RESET_OWNER
> > > 
> > > Patch 2 introduced a new function: vhost_net_reset(), which is invoked
> > >         when reset happens, say, driver is unloaded (unbinded).
> > > 
> > >         Michael suggested to do that only when MQ is not negotiated.
> > >         However, reset happens no matter MQ is enabled or negotiated
> > >         or not, and we should give a sign to backend to reset some
> > >         device to a proper state after it happens.
> > 
> > I don't think it's needed: we set all state at start anyway.
> 
> Agree with that.
> 
> > 
> > >         Note that the message sent is still RESET_OWNER. It might not
> > >         be a good idea, but we could not simply rename it to RESET_DEVICE,
> > >         and maybe adding another RESET_DEVICE might be better.
> > 
> > So modern clients don't need this at all.  Old clients need something to
> > stop device, but singling out reset is not a good idea: even if driver
> > is unloaded, you need to do that.  snabbswitch (ab)uses RESET_OWNER for
> > this, so maybe RESET_OWNER should be renamed DISABLE_ALL, to just make
> > it stop all queues.
> > 
> > Does any dpdk version that was released respond to RESET_OWNER in some
> > way? How exactly?
> 
> It just resets some states, such as closing call fd and kick fd,
> unmapping buf from hugetlbfs from set_mem_table.

And is this in some dpdk version that has been released?

> And, apparently we could do that on stop, too. So, from this pov, we
> don't need RESET_OWNER.
> 
> > 
> > > Patch 3 and 4 send SET_PROTOCOL_FEATURES at start, just like we send
> > >         SET_FEATURES.
> > > 
> > > Patch 5 send SET_VRING_ENABLE at start/stop
> > > 
> > >         Michael, I intended to send it when MQ is negotiated as you suggested,
> > >         however, I found that VHOST_USER_PROTOCOL_F_MQ is defined in vhost-user.c,
> > >         which is not accessible to vhost.c.
> > > 
> > >         Exporting it to vhost.h will resolve that, however, it's not a good
> > >         idea to move vhost user specific stuff to there. We could also introduce
> > >         another vhost callback to test whether MQ is negotiated: I just don't
> > >         think it's worthy.
> > > 
> > >         Hence, here I just used a simple test: invoke set_vring_enable() just
> > >         when it is defined. Judging the callback self has another MQ check,
> > >         I guess it's okay.
> > > 
> > > And sorry that it took so long to send this version.
> > 
> > Hmm, you are saying everyone needs SET_VRING_ENABLE?
> > Maybe we should make SET_VRING_ENABLE depend on protocol features then,
> > and not MQ?
> 
> I'm thinking something same. Otherwise, there is still no way to inform
> the backend (or client) when a vhost dev is stopped when MQ is disabled
> (which is the default state).
> 
> So, let's assume all clients have protocol features enabled, and send
> SET_VRING_ENABLE at start/stop? And if it does not, it's just like we
> are back to QEMU v2.3, where no RESET_OWNER nor SET_VRING_ENABLE
> messages are sent on stop: it worked before, and it should also work
> now.

So maybe we should drop RESET_OWNER from stop then?

> > 
> > I applied patches 1 and 5 for now.
> 
> Do you have comment about patch 3 and 4? Should we set protocol features
> just like we set features at start?
> 
> Thanks.
> 
> 	--yliu

We don't set the at start - we set them on connect.

Re: [Qemu-devel] [PATCH v4 0/5] handle vhost reset/start/stop correctly

[Yuanhan Liu]

On Thu, Nov 12, 2015 at 04:44:19PM +0200, Michael S. Tsirkin wrote:
> On Thu, Nov 12, 2015 at 10:08:15PM +0800, Yuanhan Liu wrote:
> > On Thu, Nov 12, 2015 at 03:33:41PM +0200, Michael S. Tsirkin wrote:
> > > On Wed, Nov 11, 2015 at 09:24:36PM +0800, Yuanhan Liu wrote:
> > > > 
> > > > Patch 1 rename RESET_DEVICE back to RESET_OWNER
> > > > 
> > > > Patch 2 introduced a new function: vhost_net_reset(), which is invoked
> > > >         when reset happens, say, driver is unloaded (unbinded).
> > > > 
> > > >         Michael suggested to do that only when MQ is not negotiated.
> > > >         However, reset happens no matter MQ is enabled or negotiated
> > > >         or not, and we should give a sign to backend to reset some
> > > >         device to a proper state after it happens.
> > > 
> > > I don't think it's needed: we set all state at start anyway.
> > 
> > Agree with that.
> > 
> > > 
> > > >         Note that the message sent is still RESET_OWNER. It might not
> > > >         be a good idea, but we could not simply rename it to RESET_DEVICE,
> > > >         and maybe adding another RESET_DEVICE might be better.
> > > 
> > > So modern clients don't need this at all.  Old clients need something to
> > > stop device, but singling out reset is not a good idea: even if driver
> > > is unloaded, you need to do that.  snabbswitch (ab)uses RESET_OWNER for
> > > this, so maybe RESET_OWNER should be renamed DISABLE_ALL, to just make
> > > it stop all queues.
> > > 
> > > Does any dpdk version that was released respond to RESET_OWNER in some
> > > way? How exactly?
> > 
> > It just resets some states, such as closing call fd and kick fd,
> > unmapping buf from hugetlbfs from set_mem_table.
> 
> And is this in some dpdk version that has been released?

Yes, it's been there long time ago, and you can find them in recent
releases (say, v2.1).

However, QEMU just added RESET_OWNER since v2.4.0, and it's likely that
we will remove it since this version, v2.5.0.

> 
> > And, apparently we could do that on stop, too. So, from this pov, we
> > don't need RESET_OWNER.
> > 
> > > 
> > > > Patch 3 and 4 send SET_PROTOCOL_FEATURES at start, just like we send
> > > >         SET_FEATURES.
> > > > 
> > > > Patch 5 send SET_VRING_ENABLE at start/stop
> > > > 
> > > >         Michael, I intended to send it when MQ is negotiated as you suggested,
> > > >         however, I found that VHOST_USER_PROTOCOL_F_MQ is defined in vhost-user.c,
> > > >         which is not accessible to vhost.c.
> > > > 
> > > >         Exporting it to vhost.h will resolve that, however, it's not a good
> > > >         idea to move vhost user specific stuff to there. We could also introduce
> > > >         another vhost callback to test whether MQ is negotiated: I just don't
> > > >         think it's worthy.
> > > > 
> > > >         Hence, here I just used a simple test: invoke set_vring_enable() just
> > > >         when it is defined. Judging the callback self has another MQ check,
> > > >         I guess it's okay.
> > > > 
> > > > And sorry that it took so long to send this version.
> > > 
> > > Hmm, you are saying everyone needs SET_VRING_ENABLE?
> > > Maybe we should make SET_VRING_ENABLE depend on protocol features then,
> > > and not MQ?
> > 
> > I'm thinking something same. Otherwise, there is still no way to inform
> > the backend (or client) when a vhost dev is stopped when MQ is disabled
> > (which is the default state).
> > 
> > So, let's assume all clients have protocol features enabled, and send
> > SET_VRING_ENABLE at start/stop? And if it does not, it's just like we
> > are back to QEMU v2.3, where no RESET_OWNER nor SET_VRING_ENABLE
> > messages are sent on stop: it worked before, and it should also work
> > now.
> 
> So maybe we should drop RESET_OWNER from stop then?

Yeah, agree with you. Sending reset meessage at stop doesn't make too
much sense.

> 
> > > 
> > > I applied patches 1 and 5 for now.
> > 
> > Do you have comment about patch 3 and 4? Should we set protocol features
> > just like we set features at start?
> > 
> > Thanks.
> > 
> > 	--yliu
> 
> We don't set the at start - we set them on connect.

Okay to me. I will drop them then.

	--yliu

Re: [Qemu-devel] [PATCH v4 0/5] handle vhost reset/start/stop correctly

[Michael S. Tsirkin]

On Fri, Nov 13, 2015 at 10:03:29AM +0800, Yuanhan Liu wrote:
> On Thu, Nov 12, 2015 at 04:44:19PM +0200, Michael S. Tsirkin wrote:
> > On Thu, Nov 12, 2015 at 10:08:15PM +0800, Yuanhan Liu wrote:
> > > On Thu, Nov 12, 2015 at 03:33:41PM +0200, Michael S. Tsirkin wrote:
> > > > On Wed, Nov 11, 2015 at 09:24:36PM +0800, Yuanhan Liu wrote:
> > > > > 
> > > > > Patch 1 rename RESET_DEVICE back to RESET_OWNER
> > > > > 
> > > > > Patch 2 introduced a new function: vhost_net_reset(), which is invoked
> > > > >         when reset happens, say, driver is unloaded (unbinded).
> > > > > 
> > > > >         Michael suggested to do that only when MQ is not negotiated.
> > > > >         However, reset happens no matter MQ is enabled or negotiated
> > > > >         or not, and we should give a sign to backend to reset some
> > > > >         device to a proper state after it happens.
> > > > 
> > > > I don't think it's needed: we set all state at start anyway.
> > > 
> > > Agree with that.
> > > 
> > > > 
> > > > >         Note that the message sent is still RESET_OWNER. It might not
> > > > >         be a good idea, but we could not simply rename it to RESET_DEVICE,
> > > > >         and maybe adding another RESET_DEVICE might be better.
> > > > 
> > > > So modern clients don't need this at all.  Old clients need something to
> > > > stop device, but singling out reset is not a good idea: even if driver
> > > > is unloaded, you need to do that.  snabbswitch (ab)uses RESET_OWNER for
> > > > this, so maybe RESET_OWNER should be renamed DISABLE_ALL, to just make
> > > > it stop all queues.
> > > > 
> > > > Does any dpdk version that was released respond to RESET_OWNER in some
> > > > way? How exactly?
> > > 
> > > It just resets some states, such as closing call fd and kick fd,
> > > unmapping buf from hugetlbfs from set_mem_table.
> > 
> > And is this in some dpdk version that has been released?
> 
> Yes, it's been there long time ago, and you can find them in recent
> releases (say, v2.1).
> 
> However, QEMU just added RESET_OWNER since v2.4.0, and it's likely that
> we will remove it since this version, v2.5.0.


I see. So I think that if MQ is negotiated, we should not send it.
VRING_ENABLE(0) is enough.  But what if MQ is not negotiated?
In any case, it must be done after all rings are stopped in vhost_dev_stop.
Otherwise we can not retrieve the ring index so we can't resume.

I'm not sure it's worth the hassle.
To work correctly, when MQ is not negotiated then clients
need to stop ring on GET_VRING_BASE, and that seems enough
to make everything work correctly across reboot.
If MQ is negotiated then there's VRING_ENABLE so hacks
are not needed anymore.

Cc Luke for an opinion on whether it's too late for
snabbswitch to adopt this approach.


> > 
> > > And, apparently we could do that on stop, too. So, from this pov, we
> > > don't need RESET_OWNER.
> > > 
> > > > 
> > > > > Patch 3 and 4 send SET_PROTOCOL_FEATURES at start, just like we send
> > > > >         SET_FEATURES.
> > > > > 
> > > > > Patch 5 send SET_VRING_ENABLE at start/stop
> > > > > 
> > > > >         Michael, I intended to send it when MQ is negotiated as you suggested,
> > > > >         however, I found that VHOST_USER_PROTOCOL_F_MQ is defined in vhost-user.c,
> > > > >         which is not accessible to vhost.c.
> > > > > 
> > > > >         Exporting it to vhost.h will resolve that, however, it's not a good
> > > > >         idea to move vhost user specific stuff to there. We could also introduce
> > > > >         another vhost callback to test whether MQ is negotiated: I just don't
> > > > >         think it's worthy.
> > > > > 
> > > > >         Hence, here I just used a simple test: invoke set_vring_enable() just
> > > > >         when it is defined. Judging the callback self has another MQ check,
> > > > >         I guess it's okay.
> > > > > 
> > > > > And sorry that it took so long to send this version.
> > > > 
> > > > Hmm, you are saying everyone needs SET_VRING_ENABLE?
> > > > Maybe we should make SET_VRING_ENABLE depend on protocol features then,
> > > > and not MQ?
> > > 
> > > I'm thinking something same. Otherwise, there is still no way to inform
> > > the backend (or client) when a vhost dev is stopped when MQ is disabled
> > > (which is the default state).
> > > 
> > > So, let's assume all clients have protocol features enabled, and send
> > > SET_VRING_ENABLE at start/stop? And if it does not, it's just like we
> > > are back to QEMU v2.3, where no RESET_OWNER nor SET_VRING_ENABLE
> > > messages are sent on stop: it worked before, and it should also work
> > > now.
> > 
> > So maybe we should drop RESET_OWNER from stop then?
> 
> Yeah, agree with you. Sending reset meessage at stop doesn't make too
> much sense.
> 
> > 
> > > > 
> > > > I applied patches 1 and 5 for now.
> > > 
> > > Do you have comment about patch 3 and 4? Should we set protocol features
> > > just like we set features at start?
> > > 
> > > Thanks.
> > > 
> > > 	--yliu
> > 
> > We don't set the at start - we set them on connect.
> 
> Okay to me. I will drop them then.
> 
> 	--yliu

Re: [Qemu-devel] [PATCH v4 0/5] handle vhost reset/start/stop correctly

[Yuanhan Liu]

On Fri, Nov 13, 2015 at 12:24:52PM +0200, Michael S. Tsirkin wrote:
> On Fri, Nov 13, 2015 at 10:03:29AM +0800, Yuanhan Liu wrote:
> > On Thu, Nov 12, 2015 at 04:44:19PM +0200, Michael S. Tsirkin wrote:
> > > On Thu, Nov 12, 2015 at 10:08:15PM +0800, Yuanhan Liu wrote:
> > > > On Thu, Nov 12, 2015 at 03:33:41PM +0200, Michael S. Tsirkin wrote:
> > > > > On Wed, Nov 11, 2015 at 09:24:36PM +0800, Yuanhan Liu wrote:
> > > > > > 
> > > > > > Patch 1 rename RESET_DEVICE back to RESET_OWNER
> > > > > > 
> > > > > > Patch 2 introduced a new function: vhost_net_reset(), which is invoked
> > > > > >         when reset happens, say, driver is unloaded (unbinded).
> > > > > > 
> > > > > >         Michael suggested to do that only when MQ is not negotiated.
> > > > > >         However, reset happens no matter MQ is enabled or negotiated
> > > > > >         or not, and we should give a sign to backend to reset some
> > > > > >         device to a proper state after it happens.
> > > > > 
> > > > > I don't think it's needed: we set all state at start anyway.
> > > > 
> > > > Agree with that.
> > > > 
> > > > > 
> > > > > >         Note that the message sent is still RESET_OWNER. It might not
> > > > > >         be a good idea, but we could not simply rename it to RESET_DEVICE,
> > > > > >         and maybe adding another RESET_DEVICE might be better.
> > > > > 
> > > > > So modern clients don't need this at all.  Old clients need something to
> > > > > stop device, but singling out reset is not a good idea: even if driver
> > > > > is unloaded, you need to do that.  snabbswitch (ab)uses RESET_OWNER for
> > > > > this, so maybe RESET_OWNER should be renamed DISABLE_ALL, to just make
> > > > > it stop all queues.
> > > > > 
> > > > > Does any dpdk version that was released respond to RESET_OWNER in some
> > > > > way? How exactly?
> > > > 
> > > > It just resets some states, such as closing call fd and kick fd,
> > > > unmapping buf from hugetlbfs from set_mem_table.
> > > 
> > > And is this in some dpdk version that has been released?
> > 
> > Yes, it's been there long time ago, and you can find them in recent
> > releases (say, v2.1).
> > 
> > However, QEMU just added RESET_OWNER since v2.4.0, and it's likely that
> > we will remove it since this version, v2.5.0.
> 
> 
> I see. So I think that if MQ is negotiated, we should not send it.
> VRING_ENABLE(0) is enough.  But what if MQ is not negotiated?
> In any case, it must be done after all rings are stopped in vhost_dev_stop.
> Otherwise we can not retrieve the ring index so we can't resume.
> 
> I'm not sure it's worth the hassle.
> To work correctly, when MQ is not negotiated then clients
> need to stop ring on GET_VRING_BASE, and that seems enough
> to make everything work correctly across reboot.
> If MQ is negotiated then there's VRING_ENABLE so hacks
> are not needed anymore.

I'm okay with that. And if so, we may need put those kind of information
to vhost-user spec, to make it a standard.

Or, I'm wondering could we make it be consistent: sending GET_VRING_BASE
as the only sign of vhost dev stop, no matter MQ is negotiated or not?

And I'm re-think what's the necessary of sending VRING_ENABLE at start/stop?
Just because it's a better name than GET_VRING_BASE? With current code,
VRING_ENABLE will be sent in following several places:

- virtio_net_set_queues, which will be invoked in the virtio net
  initiation stage. And it also will be invoked after user executing
  "ethtool -L" command.

- vhost_dev_start

- vhost_dev_stop

We might need take different actions for each above cases, say we need
release some resources (such as closing the kick fd) on vhost_dev_stop.
But we definitely could not do that for VRING_ENABLE messages from
ethtool as user can re-run the ethtool command to bring those vrings
back to functional: doing those kind of resource release would not be
able to bring it back.

So, it might be difficult (or more accurate, messy) for the client to
figure out which case it is: at which time we should release some
resources, and at which time we should not.

	--yliu
> 
> Cc Luke for an opinion on whether it's too late for
> snabbswitch to adopt this approach.
> 
> 
> > > 
> > > > And, apparently we could do that on stop, too. So, from this pov, we
> > > > don't need RESET_OWNER.
> > > > 
> > > > > 
> > > > > > Patch 3 and 4 send SET_PROTOCOL_FEATURES at start, just like we send
> > > > > >         SET_FEATURES.
> > > > > > 
> > > > > > Patch 5 send SET_VRING_ENABLE at start/stop
> > > > > > 
> > > > > >         Michael, I intended to send it when MQ is negotiated as you suggested,
> > > > > >         however, I found that VHOST_USER_PROTOCOL_F_MQ is defined in vhost-user.c,
> > > > > >         which is not accessible to vhost.c.
> > > > > > 
> > > > > >         Exporting it to vhost.h will resolve that, however, it's not a good
> > > > > >         idea to move vhost user specific stuff to there. We could also introduce
> > > > > >         another vhost callback to test whether MQ is negotiated: I just don't
> > > > > >         think it's worthy.
> > > > > > 
> > > > > >         Hence, here I just used a simple test: invoke set_vring_enable() just
> > > > > >         when it is defined. Judging the callback self has another MQ check,
> > > > > >         I guess it's okay.
> > > > > > 
> > > > > > And sorry that it took so long to send this version.
> > > > > 
> > > > > Hmm, you are saying everyone needs SET_VRING_ENABLE?
> > > > > Maybe we should make SET_VRING_ENABLE depend on protocol features then,
> > > > > and not MQ?
> > > > 
> > > > I'm thinking something same. Otherwise, there is still no way to inform
> > > > the backend (or client) when a vhost dev is stopped when MQ is disabled
> > > > (which is the default state).
> > > > 
> > > > So, let's assume all clients have protocol features enabled, and send
> > > > SET_VRING_ENABLE at start/stop? And if it does not, it's just like we
> > > > are back to QEMU v2.3, where no RESET_OWNER nor SET_VRING_ENABLE
> > > > messages are sent on stop: it worked before, and it should also work
> > > > now.
> > > 
> > > So maybe we should drop RESET_OWNER from stop then?
> > 
> > Yeah, agree with you. Sending reset meessage at stop doesn't make too
> > much sense.
> > 
> > > 
> > > > > 
> > > > > I applied patches 1 and 5 for now.
> > > > 
> > > > Do you have comment about patch 3 and 4? Should we set protocol features
> > > > just like we set features at start?
> > > > 
> > > > Thanks.
> > > > 
> > > > 	--yliu
> > > 
> > > We don't set the at start - we set them on connect.
> > 
> > Okay to me. I will drop them then.
> > 
> > 	--yliu