* [Qemu-devel] [PATCH v2 1/5] Revert "vhost: rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE"
@ 2015-10-21 9:07 Yuanhan Liu
2015-10-21 9:07 ` [Qemu-devel] [PATCH v2 2/5] doc: vhost-user: request naming fix Yuanhan Liu
` (4 more replies)
0 siblings, 5 replies; 17+ messages in thread
From: Yuanhan Liu @ 2015-10-21 9:07 UTC (permalink / raw)
To: qemu-devel; +Cc: Yuanhan Liu, Michael S. Tsirkin
This reverts commit d1f8b30ec8dde0318fd1b98d24a64926feae9625.
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/net/vhost_net.c | 2 +-
hw/virtio/vhost-user.c | 8 ++++----
| 2 +-
tests/vhost-user-test.c | 2 +-
5 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index 4eadad1..4bcd17d 100644
--- a/docs/specs/vhost-user.txt
+++ b/docs/specs/vhost-user.txt
@@ -211,10 +211,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/net/vhost_net.c b/hw/net/vhost_net.c
index 2bce891..804f5c9 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -296,7 +296,7 @@ static void vhost_net_stop_one(struct vhost_net *net,
} 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_call(&net->dev, VHOST_RESET_DEVICE,
+ int r = vhost_ops->vhost_call(&net->dev, VHOST_RESET_OWNER,
NULL);
assert(r >= 0);
}
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index b11c0d2..12a9104 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -34,7 +34,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,
@@ -102,7 +102,7 @@ static unsigned long int ioctl_to_vhost_user_request[VHOST_USER_MAX] = {
VHOST_GET_FEATURES, /* VHOST_USER_GET_FEATURES */
VHOST_SET_FEATURES, /* VHOST_USER_SET_FEATURES */
VHOST_SET_OWNER, /* VHOST_USER_SET_OWNER */
- VHOST_RESET_DEVICE, /* VHOST_USER_RESET_DEVICE */
+ VHOST_RESET_OWNER, /* VHOST_USER_RESET_OWNER */
VHOST_SET_MEM_TABLE, /* VHOST_USER_SET_MEM_TABLE */
VHOST_SET_LOG_BASE, /* VHOST_USER_SET_LOG_BASE */
VHOST_SET_LOG_FD, /* VHOST_USER_SET_LOG_FD */
@@ -192,7 +192,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;
@@ -249,7 +249,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
break;
case VHOST_USER_SET_OWNER:
- case VHOST_USER_RESET_DEVICE:
+ case VHOST_USER_RESET_OWNER:
break;
case VHOST_USER_SET_MEM_TABLE:
--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-test.c b/tests/vhost-user-test.c
index 56df5cc..f181391 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -53,7 +53,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,
--
1.9.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v2 2/5] doc: vhost-user: request naming fix
2015-10-21 9:07 [Qemu-devel] [PATCH v2 1/5] Revert "vhost: rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE" Yuanhan Liu
@ 2015-10-21 9:07 ` Yuanhan Liu
2015-10-21 9:07 ` [Qemu-devel] [PATCH v2 3/5] vhost-user-test: add multiple queue test Yuanhan Liu
` (3 subsequent siblings)
4 siblings, 0 replies; 17+ messages in thread
From: Yuanhan Liu @ 2015-10-21 9:07 UTC (permalink / raw)
To: qemu-devel; +Cc: Yuanhan Liu, Michael S. Tsirkin
They are VHOST_USER_XXX instead of VHOST_XXX messages.
Also, add VHOST_USER_GET_QUEUE_NUM to the section that
requries replies.
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
docs/specs/vhost-user.txt | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
index 4bcd17d..b23d88f 100644
--- a/docs/specs/vhost-user.txt
+++ b/docs/specs/vhost-user.txt
@@ -112,18 +112,19 @@ The communication consists of master sending message requests and slave sending
message replies. Most of the requests don't require replies. Here is a list of
the ones that do:
- * VHOST_GET_FEATURES
- * VHOST_GET_PROTOCOL_FEATURES
- * VHOST_GET_VRING_BASE
+ * VHOST_USER_GET_FEATURES
+ * VHOST_USER_GET_PROTOCOL_FEATURES
+ * VHOST_USER_GET_VRING_BASE
+ * VHOST_USER_GET_QUEUE_NUM
There are several messages that the master sends with file descriptors passed
in the ancillary data:
- * VHOST_SET_MEM_TABLE
- * VHOST_SET_LOG_FD
- * VHOST_SET_VRING_KICK
- * VHOST_SET_VRING_CALL
- * VHOST_SET_VRING_ERR
+ * VHOST_USER_SET_MEM_TABLE
+ * VHOST_USER_SET_LOG_FD
+ * VHOST_USER_SET_VRING_KICK
+ * VHOST_USER_SET_VRING_CALL
+ * VHOST_USER_SET_VRING_ERR
If Master is unable to send the full message or receives a wrong reply it will
close the connection. An optional reconnection mechanism can be implemented.
--
1.9.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v2 3/5] vhost-user-test: add multiple queue test
2015-10-21 9:07 [Qemu-devel] [PATCH v2 1/5] Revert "vhost: rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE" Yuanhan Liu
2015-10-21 9:07 ` [Qemu-devel] [PATCH v2 2/5] doc: vhost-user: request naming fix Yuanhan Liu
@ 2015-10-21 9:07 ` Yuanhan Liu
2015-10-21 10:34 ` Michael S. Tsirkin
2015-10-21 9:07 ` [Qemu-devel] [PATCH v2 4/5] Revert "vhost-user: Send VHOST_RESET_OWNER on vhost stop" Yuanhan Liu
` (2 subsequent siblings)
4 siblings, 1 reply; 17+ messages in thread
From: Yuanhan Liu @ 2015-10-21 9:07 UTC (permalink / raw)
To: qemu-devel; +Cc: Yuanhan Liu, Jason Wang, Michael S. Tsirkin
Setting VHOST_USER_PROTOCOL_F_MQ protocol feature bit to claim that we
support MQ feature, and simply assume we support 0xff queue pairs at most.
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Jason Wang <jasowang@redhat.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
v2: use macro to define the max queues we support
---
tests/vhost-user-test.c | 21 +++++++++++++++++++--
1 file changed, 19 insertions(+), 2 deletions(-)
diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
index f181391..8162e64 100644
--- a/tests/vhost-user-test.c
+++ b/tests/vhost-user-test.c
@@ -29,11 +29,14 @@
#define HAVE_MONOTONIC_TIME
#endif
+#define MAX_QUEUS 0xff
+
#define QEMU_CMD_ACCEL " -machine accel=tcg"
#define QEMU_CMD_MEM " -m 512 -object memory-backend-file,id=mem,size=512M,"\
"mem-path=%s,share=on -numa node,memdev=mem"
#define QEMU_CMD_CHR " -chardev socket,id=chr0,path=%s"
-#define QEMU_CMD_NETDEV " -netdev vhost-user,id=net0,chardev=chr0,vhostforce"
+#define QEMU_CMD_NETDEV " -netdev vhost-user,id=net0,chardev=chr0,vhostforce," \
+ "queues=2"
#define QEMU_CMD_NET " -device virtio-net-pci,netdev=net0 "
#define QEMU_CMD_ROM " -option-rom ../pc-bios/pxe-virtio.rom"
@@ -48,6 +51,8 @@
#define VHOST_USER_F_PROTOCOL_FEATURES 30
+#define VHOST_USER_PROTOCOL_F_MQ 0
+
typedef enum VhostUserRequest {
VHOST_USER_NONE = 0,
VHOST_USER_GET_FEATURES = 1,
@@ -66,6 +71,8 @@ typedef enum VhostUserRequest {
VHOST_USER_SET_VRING_ERR = 14,
VHOST_USER_GET_PROTOCOL_FEATURES = 15,
VHOST_USER_SET_PROTOCOL_FEATURES = 16,
+ VHOST_USER_GET_QUEUE_NUM = 17,
+ VHOST_USER_SET_VRING_ENABLE = 18,
VHOST_USER_MAX
} VhostUserRequest;
@@ -232,7 +239,7 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
/* send back features to qemu */
msg.flags |= VHOST_USER_REPLY_MASK;
msg.size = sizeof(m.u64);
- msg.u64 = 0;
+ msg.u64 = (1ULL << VHOST_USER_PROTOCOL_F_MQ);
p = (uint8_t *) &msg;
qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
break;
@@ -266,6 +273,16 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
*/
qemu_set_nonblock(fd);
break;
+
+ case VHOST_USER_GET_QUEUE_NUM:
+ /* send back the number of queues we support (let it be 2) to qemu */
+ msg.flags |= VHOST_USER_REPLY_MASK;
+ msg.size = sizeof(m.u64);
+ msg.u64 = MAX_QUEUS;
+ p = (uint8_t *) &msg;
+ qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
+ break;
+
default:
break;
}
--
1.9.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v2 4/5] Revert "vhost-user: Send VHOST_RESET_OWNER on vhost stop"
2015-10-21 9:07 [Qemu-devel] [PATCH v2 1/5] Revert "vhost: rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE" Yuanhan Liu
2015-10-21 9:07 ` [Qemu-devel] [PATCH v2 2/5] doc: vhost-user: request naming fix Yuanhan Liu
2015-10-21 9:07 ` [Qemu-devel] [PATCH v2 3/5] vhost-user-test: add multiple queue test Yuanhan Liu
@ 2015-10-21 9:07 ` Yuanhan Liu
2015-10-21 10:35 ` Michael S. Tsirkin
2015-10-21 10:39 ` Michael S. Tsirkin
2015-10-21 9:07 ` [Qemu-devel] [PATCH v2 5/5] vhost: send VHOST_USER_SET_VRING_ENABLE at start/stop Yuanhan Liu
2015-10-21 10:40 ` [Qemu-devel] [PATCH v2 1/5] Revert "vhost: rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE" Michael S. Tsirkin
4 siblings, 2 replies; 17+ messages in thread
From: Yuanhan Liu @ 2015-10-21 9:07 UTC (permalink / raw)
To: qemu-devel; +Cc: Yuanhan Liu, Luke Gorrie, Michael S. Tsirkin
Don't send VHOST_RESET_OWNER, for as Michael stated:
Because we need to get the state from remote after stop.
RESET_OWNER discards that, so we can't resume the VM.
This reverts commit 294ce717e0f212ed0763307f3eab72b4a1bdf4d0.
Cc: Luke Gorrie <luke@snabb.co>
Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
hw/net/vhost_net.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index 804f5c9..95da5f8 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -293,13 +293,6 @@ static void vhost_net_stop_one(struct vhost_net *net,
&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_call(&net->dev, VHOST_RESET_OWNER,
- NULL);
- assert(r >= 0);
- }
}
if (net->nc->info->poll) {
net->nc->info->poll(net->nc, true);
--
1.9.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Qemu-devel] [PATCH v2 5/5] vhost: send VHOST_USER_SET_VRING_ENABLE at start/stop
2015-10-21 9:07 [Qemu-devel] [PATCH v2 1/5] Revert "vhost: rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE" Yuanhan Liu
` (2 preceding siblings ...)
2015-10-21 9:07 ` [Qemu-devel] [PATCH v2 4/5] Revert "vhost-user: Send VHOST_RESET_OWNER on vhost stop" Yuanhan Liu
@ 2015-10-21 9:07 ` Yuanhan Liu
2015-10-21 10:39 ` Michael S. Tsirkin
2015-10-21 10:40 ` [Qemu-devel] [PATCH v2 1/5] Revert "vhost: rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE" Michael S. Tsirkin
4 siblings, 1 reply; 17+ messages in thread
From: Yuanhan Liu @ 2015-10-21 9:07 UTC (permalink / raw)
To: qemu-devel; +Cc: Yuanhan Liu
Send VHOST_USER_SET_VRING_ENABLE at start/stop when multiple queue
is negotiated, to inform the backend that we are ready or not.
And exclude VHOST_USER_GET_QUEUE_NUM as one time request, as we need
to get max_queues for each vhost_dev.
Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
hw/virtio/vhost-user.c | 1 -
hw/virtio/vhost.c | 18 ++++++++++++++++++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
index 12a9104..6532a73 100644
--- a/hw/virtio/vhost-user.c
+++ b/hw/virtio/vhost-user.c
@@ -194,7 +194,6 @@ static bool vhost_user_one_time_request(VhostUserRequest request)
case VHOST_USER_SET_OWNER:
case VHOST_USER_RESET_OWNER:
case VHOST_USER_SET_MEM_TABLE:
- case VHOST_USER_GET_QUEUE_NUM:
return true;
default:
return false;
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index c0ed5b2..54a4633 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1146,6 +1146,19 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
}
}
+ /*
+ * Send VHOST_USER_SET_VRING_ENABLE message when multiple queue
+ * is negotiated to inform the back end that we are ready.
+ *
+ * Only set enable to 1 for first queue pair, as we enable one
+ * queue pair by default.
+ */
+ if (hdev->max_queues > 1 &&
+ hdev->vhost_ops->vhost_backend_set_vring_enable) {
+ hdev->vhost_ops->vhost_backend_set_vring_enable(hdev,
+ hdev->vq_index == 0);
+ }
+
return 0;
fail_log:
vhost_log_put(hdev, false);
@@ -1180,5 +1193,10 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
hdev->started = false;
hdev->log = NULL;
hdev->log_size = 0;
+
+ if (hdev->max_queues > 1 &&
+ hdev->vhost_ops->vhost_backend_set_vring_enable) {
+ hdev->vhost_ops->vhost_backend_set_vring_enable(hdev, 0);
+ }
}
--
1.9.0
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v2 3/5] vhost-user-test: add multiple queue test
2015-10-21 9:07 ` [Qemu-devel] [PATCH v2 3/5] vhost-user-test: add multiple queue test Yuanhan Liu
@ 2015-10-21 10:34 ` Michael S. Tsirkin
0 siblings, 0 replies; 17+ messages in thread
From: Michael S. Tsirkin @ 2015-10-21 10:34 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: Jason Wang, qemu-devel
On Wed, Oct 21, 2015 at 05:07:16PM +0800, Yuanhan Liu wrote:
> Setting VHOST_USER_PROTOCOL_F_MQ protocol feature bit to claim that we
> support MQ feature, and simply assume we support 0xff queue pairs at most.
>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>
> ---
> v2: use macro to define the max queues we support
Well this doesn't buy us anything - it's only used in
one place. I really meant the "2" which is the actual
# of queues.
> ---
> tests/vhost-user-test.c | 21 +++++++++++++++++++--
> 1 file changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/tests/vhost-user-test.c b/tests/vhost-user-test.c
> index f181391..8162e64 100644
> --- a/tests/vhost-user-test.c
> +++ b/tests/vhost-user-test.c
> @@ -29,11 +29,14 @@
> #define HAVE_MONOTONIC_TIME
> #endif
>
> +#define MAX_QUEUS 0xff
> +
> #define QEMU_CMD_ACCEL " -machine accel=tcg"
> #define QEMU_CMD_MEM " -m 512 -object memory-backend-file,id=mem,size=512M,"\
> "mem-path=%s,share=on -numa node,memdev=mem"
> #define QEMU_CMD_CHR " -chardev socket,id=chr0,path=%s"
> -#define QEMU_CMD_NETDEV " -netdev vhost-user,id=net0,chardev=chr0,vhostforce"
> +#define QEMU_CMD_NETDEV " -netdev vhost-user,id=net0,chardev=chr0,vhostforce," \
> + "queues=2"
> #define QEMU_CMD_NET " -device virtio-net-pci,netdev=net0 "
> #define QEMU_CMD_ROM " -option-rom ../pc-bios/pxe-virtio.rom"
>
> @@ -48,6 +51,8 @@
>
> #define VHOST_USER_F_PROTOCOL_FEATURES 30
>
> +#define VHOST_USER_PROTOCOL_F_MQ 0
> +
> typedef enum VhostUserRequest {
> VHOST_USER_NONE = 0,
> VHOST_USER_GET_FEATURES = 1,
> @@ -66,6 +71,8 @@ typedef enum VhostUserRequest {
> VHOST_USER_SET_VRING_ERR = 14,
> VHOST_USER_GET_PROTOCOL_FEATURES = 15,
> VHOST_USER_SET_PROTOCOL_FEATURES = 16,
> + VHOST_USER_GET_QUEUE_NUM = 17,
> + VHOST_USER_SET_VRING_ENABLE = 18,
> VHOST_USER_MAX
> } VhostUserRequest;
>
> @@ -232,7 +239,7 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
> /* send back features to qemu */
> msg.flags |= VHOST_USER_REPLY_MASK;
> msg.size = sizeof(m.u64);
> - msg.u64 = 0;
> + msg.u64 = (1ULL << VHOST_USER_PROTOCOL_F_MQ);
> p = (uint8_t *) &msg;
> qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
> break;
> @@ -266,6 +273,16 @@ static void chr_read(void *opaque, const uint8_t *buf, int size)
> */
> qemu_set_nonblock(fd);
> break;
> +
> + case VHOST_USER_GET_QUEUE_NUM:
> + /* send back the number of queues we support (let it be 2) to qemu */
> + msg.flags |= VHOST_USER_REPLY_MASK;
> + msg.size = sizeof(m.u64);
> + msg.u64 = MAX_QUEUS;
> + p = (uint8_t *) &msg;
> + qemu_chr_fe_write_all(chr, p, VHOST_USER_HDR_SIZE + msg.size);
> + break;
> +
> default:
> break;
> }
> --
> 1.9.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/5] Revert "vhost-user: Send VHOST_RESET_OWNER on vhost stop"
2015-10-21 9:07 ` [Qemu-devel] [PATCH v2 4/5] Revert "vhost-user: Send VHOST_RESET_OWNER on vhost stop" Yuanhan Liu
@ 2015-10-21 10:35 ` Michael S. Tsirkin
2015-10-21 10:39 ` Michael S. Tsirkin
1 sibling, 0 replies; 17+ messages in thread
From: Michael S. Tsirkin @ 2015-10-21 10:35 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: Luke Gorrie, qemu-devel
On Wed, Oct 21, 2015 at 05:07:17PM +0800, Yuanhan Liu wrote:
> Don't send VHOST_RESET_OWNER, for as Michael stated:
>
> Because we need to get the state from remote after stop.
> RESET_OWNER discards that, so we can't resume the VM.
>
> This reverts commit 294ce717e0f212ed0763307f3eab72b4a1bdf4d0.
>
> Cc: Luke Gorrie <luke@snabb.co>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
OK but we still need to call it on device reset.
Otherwise device will corrupt memory.
> ---
> hw/net/vhost_net.c | 7 -------
> 1 file changed, 7 deletions(-)
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 804f5c9..95da5f8 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -293,13 +293,6 @@ static void vhost_net_stop_one(struct vhost_net *net,
> &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_call(&net->dev, VHOST_RESET_OWNER,
> - NULL);
> - assert(r >= 0);
> - }
> }
> if (net->nc->info->poll) {
> net->nc->info->poll(net->nc, true);
> --
> 1.9.0
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v2 5/5] vhost: send VHOST_USER_SET_VRING_ENABLE at start/stop
2015-10-21 9:07 ` [Qemu-devel] [PATCH v2 5/5] vhost: send VHOST_USER_SET_VRING_ENABLE at start/stop Yuanhan Liu
@ 2015-10-21 10:39 ` Michael S. Tsirkin
2015-10-21 13:43 ` Yuanhan Liu
0 siblings, 1 reply; 17+ messages in thread
From: Michael S. Tsirkin @ 2015-10-21 10:39 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: qemu-devel
On Wed, Oct 21, 2015 at 05:07:18PM +0800, Yuanhan Liu wrote:
> Send VHOST_USER_SET_VRING_ENABLE at start/stop when multiple queue
> is negotiated, to inform the backend that we are ready or not.
OK but that's only if MQ is set. If now, we need to do
RESET_OWNER followed by SET_OWNER.
> And exclude VHOST_USER_GET_QUEUE_NUM as one time request, as we need
> to get max_queues for each vhost_dev.
Pls split this part out.
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
> hw/virtio/vhost-user.c | 1 -
> hw/virtio/vhost.c | 18 ++++++++++++++++++
> 2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index 12a9104..6532a73 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -194,7 +194,6 @@ static bool vhost_user_one_time_request(VhostUserRequest request)
> case VHOST_USER_SET_OWNER:
> case VHOST_USER_RESET_OWNER:
> case VHOST_USER_SET_MEM_TABLE:
> - case VHOST_USER_GET_QUEUE_NUM:
> return true;
> default:
> return false;
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index c0ed5b2..54a4633 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1146,6 +1146,19 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> }
> }
>
> + /*
> + * Send VHOST_USER_SET_VRING_ENABLE message when multiple queue
> + * is negotiated to inform the back end that we are ready.
> + *
> + * Only set enable to 1 for first queue pair, as we enable one
> + * queue pair by default.
> + */
> + if (hdev->max_queues > 1 &&
this makes no sense in the generic code.
This is a work around for a protocol bug - belongs in
vhost user internally.
And that's not the way to test this. MQ could be negotiated
even if there is a single pair of queues.
> + hdev->vhost_ops->vhost_backend_set_vring_enable) {
> + hdev->vhost_ops->vhost_backend_set_vring_enable(hdev,
> + hdev->vq_index == 0);
> + }
> +
> return 0;
> fail_log:
> vhost_log_put(hdev, false);
> @@ -1180,5 +1193,10 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
> hdev->started = false;
> hdev->log = NULL;
> hdev->log_size = 0;
> +
> + if (hdev->max_queues > 1 &&
> + hdev->vhost_ops->vhost_backend_set_vring_enable) {
> + hdev->vhost_ops->vhost_backend_set_vring_enable(hdev, 0);
> + }
> }
>
> --
> 1.9.0
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v2 4/5] Revert "vhost-user: Send VHOST_RESET_OWNER on vhost stop"
2015-10-21 9:07 ` [Qemu-devel] [PATCH v2 4/5] Revert "vhost-user: Send VHOST_RESET_OWNER on vhost stop" Yuanhan Liu
2015-10-21 10:35 ` Michael S. Tsirkin
@ 2015-10-21 10:39 ` Michael S. Tsirkin
1 sibling, 0 replies; 17+ messages in thread
From: Michael S. Tsirkin @ 2015-10-21 10:39 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: Luke Gorrie, qemu-devel
On Wed, Oct 21, 2015 at 05:07:17PM +0800, Yuanhan Liu wrote:
> Don't send VHOST_RESET_OWNER, for as Michael stated:
>
> Because we need to get the state from remote after stop.
> RESET_OWNER discards that, so we can't resume the VM.
>
> This reverts commit 294ce717e0f212ed0763307f3eab72b4a1bdf4d0.
>
> Cc: Luke Gorrie <luke@snabb.co>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Yes but poking at guest memory after driver is unloaded
is even worse.
> ---
> hw/net/vhost_net.c | 7 -------
> 1 file changed, 7 deletions(-)
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 804f5c9..95da5f8 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -293,13 +293,6 @@ static void vhost_net_stop_one(struct vhost_net *net,
> &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_call(&net->dev, VHOST_RESET_OWNER,
> - NULL);
> - assert(r >= 0);
> - }
> }
> if (net->nc->info->poll) {
> net->nc->info->poll(net->nc, true);
> --
> 1.9.0
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/5] Revert "vhost: rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE"
2015-10-21 9:07 [Qemu-devel] [PATCH v2 1/5] Revert "vhost: rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE" Yuanhan Liu
` (3 preceding siblings ...)
2015-10-21 9:07 ` [Qemu-devel] [PATCH v2 5/5] vhost: send VHOST_USER_SET_VRING_ENABLE at start/stop Yuanhan Liu
@ 2015-10-21 10:40 ` Michael S. Tsirkin
2015-10-21 13:04 ` Yuanhan Liu
4 siblings, 1 reply; 17+ messages in thread
From: Michael S. Tsirkin @ 2015-10-21 10:40 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: qemu-devel
On Wed, Oct 21, 2015 at 05:07:14PM +0800, Yuanhan Liu wrote:
> This reverts commit d1f8b30ec8dde0318fd1b98d24a64926feae9625.
>
> 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>
Are these patches dependent on each other? If yes pls send a cover letter.
If not pls don't send a series, post them separately.
And, pls Cc me on all vhost user patches.
> ---
> docs/specs/vhost-user.txt | 4 ++--
> hw/net/vhost_net.c | 2 +-
> hw/virtio/vhost-user.c | 8 ++++----
> linux-headers/linux/vhost.h | 2 +-
> tests/vhost-user-test.c | 2 +-
> 5 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> index 4eadad1..4bcd17d 100644
> --- a/docs/specs/vhost-user.txt
> +++ b/docs/specs/vhost-user.txt
> @@ -211,10 +211,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/net/vhost_net.c b/hw/net/vhost_net.c
> index 2bce891..804f5c9 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -296,7 +296,7 @@ static void vhost_net_stop_one(struct vhost_net *net,
> } 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_call(&net->dev, VHOST_RESET_DEVICE,
> + int r = vhost_ops->vhost_call(&net->dev, VHOST_RESET_OWNER,
> NULL);
> assert(r >= 0);
> }
> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> index b11c0d2..12a9104 100644
> --- a/hw/virtio/vhost-user.c
> +++ b/hw/virtio/vhost-user.c
> @@ -34,7 +34,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,
> @@ -102,7 +102,7 @@ static unsigned long int ioctl_to_vhost_user_request[VHOST_USER_MAX] = {
> VHOST_GET_FEATURES, /* VHOST_USER_GET_FEATURES */
> VHOST_SET_FEATURES, /* VHOST_USER_SET_FEATURES */
> VHOST_SET_OWNER, /* VHOST_USER_SET_OWNER */
> - VHOST_RESET_DEVICE, /* VHOST_USER_RESET_DEVICE */
> + VHOST_RESET_OWNER, /* VHOST_USER_RESET_OWNER */
> VHOST_SET_MEM_TABLE, /* VHOST_USER_SET_MEM_TABLE */
> VHOST_SET_LOG_BASE, /* VHOST_USER_SET_LOG_BASE */
> VHOST_SET_LOG_FD, /* VHOST_USER_SET_LOG_FD */
> @@ -192,7 +192,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;
> @@ -249,7 +249,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> break;
>
> case VHOST_USER_SET_OWNER:
> - case VHOST_USER_RESET_DEVICE:
> + case VHOST_USER_RESET_OWNER:
> break;
>
> case VHOST_USER_SET_MEM_TABLE:
> 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-test.c b/tests/vhost-user-test.c
> index 56df5cc..f181391 100644
> --- a/tests/vhost-user-test.c
> +++ b/tests/vhost-user-test.c
> @@ -53,7 +53,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,
> --
> 1.9.0
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/5] Revert "vhost: rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE"
2015-10-21 10:40 ` [Qemu-devel] [PATCH v2 1/5] Revert "vhost: rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE" Michael S. Tsirkin
@ 2015-10-21 13:04 ` Yuanhan Liu
2015-10-21 14:13 ` Michael S. Tsirkin
0 siblings, 1 reply; 17+ messages in thread
From: Yuanhan Liu @ 2015-10-21 13:04 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
On Wed, Oct 21, 2015 at 01:40:59PM +0300, Michael S. Tsirkin wrote:
> On Wed, Oct 21, 2015 at 05:07:14PM +0800, Yuanhan Liu wrote:
> > This reverts commit d1f8b30ec8dde0318fd1b98d24a64926feae9625.
> >
> > 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>
>
>
> Are these patches dependent on each other? If yes pls send a cover letter.
> If not pls don't send a series, post them separately.
Got it.
> And, pls Cc me on all vhost user patches.
Isn't I always doing that? :)
--yliu
>
> > ---
> > docs/specs/vhost-user.txt | 4 ++--
> > hw/net/vhost_net.c | 2 +-
> > hw/virtio/vhost-user.c | 8 ++++----
> > linux-headers/linux/vhost.h | 2 +-
> > tests/vhost-user-test.c | 2 +-
> > 5 files changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> > index 4eadad1..4bcd17d 100644
> > --- a/docs/specs/vhost-user.txt
> > +++ b/docs/specs/vhost-user.txt
> > @@ -211,10 +211,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/net/vhost_net.c b/hw/net/vhost_net.c
> > index 2bce891..804f5c9 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -296,7 +296,7 @@ static void vhost_net_stop_one(struct vhost_net *net,
> > } 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_call(&net->dev, VHOST_RESET_DEVICE,
> > + int r = vhost_ops->vhost_call(&net->dev, VHOST_RESET_OWNER,
> > NULL);
> > assert(r >= 0);
> > }
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index b11c0d2..12a9104 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -34,7 +34,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,
> > @@ -102,7 +102,7 @@ static unsigned long int ioctl_to_vhost_user_request[VHOST_USER_MAX] = {
> > VHOST_GET_FEATURES, /* VHOST_USER_GET_FEATURES */
> > VHOST_SET_FEATURES, /* VHOST_USER_SET_FEATURES */
> > VHOST_SET_OWNER, /* VHOST_USER_SET_OWNER */
> > - VHOST_RESET_DEVICE, /* VHOST_USER_RESET_DEVICE */
> > + VHOST_RESET_OWNER, /* VHOST_USER_RESET_OWNER */
> > VHOST_SET_MEM_TABLE, /* VHOST_USER_SET_MEM_TABLE */
> > VHOST_SET_LOG_BASE, /* VHOST_USER_SET_LOG_BASE */
> > VHOST_SET_LOG_FD, /* VHOST_USER_SET_LOG_FD */
> > @@ -192,7 +192,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;
> > @@ -249,7 +249,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> > break;
> >
> > case VHOST_USER_SET_OWNER:
> > - case VHOST_USER_RESET_DEVICE:
> > + case VHOST_USER_RESET_OWNER:
> > break;
> >
> > case VHOST_USER_SET_MEM_TABLE:
> > 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-test.c b/tests/vhost-user-test.c
> > index 56df5cc..f181391 100644
> > --- a/tests/vhost-user-test.c
> > +++ b/tests/vhost-user-test.c
> > @@ -53,7 +53,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,
> > --
> > 1.9.0
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v2 5/5] vhost: send VHOST_USER_SET_VRING_ENABLE at start/stop
2015-10-21 10:39 ` Michael S. Tsirkin
@ 2015-10-21 13:43 ` Yuanhan Liu
2015-10-21 14:11 ` Michael S. Tsirkin
0 siblings, 1 reply; 17+ messages in thread
From: Yuanhan Liu @ 2015-10-21 13:43 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
On Wed, Oct 21, 2015 at 01:39:11PM +0300, Michael S. Tsirkin wrote:
> On Wed, Oct 21, 2015 at 05:07:18PM +0800, Yuanhan Liu wrote:
> > Send VHOST_USER_SET_VRING_ENABLE at start/stop when multiple queue
> > is negotiated, to inform the backend that we are ready or not.
>
> OK but that's only if MQ is set.
Maybe we could just call vhost_backend_set_vring_enable() unconditionally?
It's nil operation when MQ is not set.
> If now, we need to do
> RESET_OWNER followed by SET_OWNER.
Could you be more specific? Why sending RESET_OWNER followed by
SET_OWNER?
TBH, I'm a bit confused with RESET_OWNER now: what it does, and when is
supposed to send it :(
And, sending RESET_OWNER inside virtio_net_reset() also looks weird.
I made a quick try before sending this patchset, and the vhost-user
request dump doesn't look right to me: the message is sent after
vhost dev init (GET_FEATURES, GET_PROTOCOL_FEATURE, SET_OWNER, ...,
SET_VRING_CALL), and before peer attach (SET_VRING_ENABLE) and
vhost_dev_start (SET_MEM_TABLE, ... SET_VRING_KICK ...):
# start of a VM
VHOST_CONFIG: new virtio connection is 28
VHOST_CONFIG: new device, handle is 0
VHOST_CONFIG: read message VHOST_USER_GET_FEATURES
VHOST_CONFIG: read message VHOST_USER_GET_PROTOCOL_FEATURES
VHOST_CONFIG: read message VHOST_USER_SET_PROTOCOL_FEATURES
VHOST_CONFIG: read message VHOST_USER_GET_QUEUE_NUM
VHOST_CONFIG: read message VHOST_USER_SET_OWNER
VHOST_CONFIG: read message VHOST_USER_GET_FEATURES
VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
VHOST_CONFIG: vring call idx:0 file:29
VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
VHOST_CONFIG: vring call idx:1 file:30
VHOST_CONFIG: read message VHOST_USER_GET_FEATURES
VHOST_CONFIG: read message VHOST_USER_GET_PROTOCOL_FEATURES
VHOST_CONFIG: read message VHOST_USER_SET_PROTOCOL_FEATURES
VHOST_CONFIG: read message VHOST_USER_GET_QUEUE_NUM
VHOST_CONFIG: read message VHOST_USER_GET_FEATURES
VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
...
...
VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
VHOST_CONFIG: vring call idx:6 file:35
VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
VHOST_CONFIG: vring call idx:7 file:36
==> VHOST_CONFIG: read message VHOST_USER_RESET_OWNER
VHOST_CONFIG: read message VHOST_USER_RESET_OWNER
VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
VHOST_CONFIG: set queue enable: 1 to qp idx: 0
VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
VHOST_CONFIG: set queue enable: 0 to qp idx: 2
VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
VHOST_CONFIG: set queue enable: 0 to qp idx: 4
VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
VHOST_CONFIG: set queue enable: 0 to qp idx: 6
VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
VHOST_CONFIG: vring call idx:0 file:29
VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
VHOST_CONFIG: vring call idx:1 file:30
VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
VHOST_CONFIG: vring call idx:2 file:31
VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
VHOST_CONFIG: vring call idx:3 file:32
VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
VHOST_CONFIG: vring call idx:4 file:33
VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
VHOST_CONFIG: vring call idx:5 file:34
VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
VHOST_CONFIG: vring call idx:6 file:35
VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
VHOST_CONFIG: vring call idx:7 file:36
VHOST_CONFIG: read message VHOST_USER_SET_FEATURES
VHOST_CONFIG: read message VHOST_USER_SET_MEM_TABLE
VHOST_CONFIG: mapped region 0 fd:37 to 0x2aaac0000000 sz:0xa0000 off:0x0
VHOST_CONFIG: mapped region 1 fd:38 to 0x2aab00000000 sz:0x80000000 off:0xc0000
VHOST_CONFIG: read message VHOST_USER_SET_VRING_NUM
VHOST_CONFIG: read message VHOST_USER_SET_VRING_BASE
VHOST_CONFIG: read message VHOST_USER_SET_VRING_ADDR
VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
VHOST_CONFIG: vring kick idx:0 file:39
VHOST_CONFIG: virtio is not ready for processing.
VHOST_CONFIG: read message VHOST_USER_SET_VRING_NUM
VHOST_CONFIG: read message VHOST_USER_SET_VRING_BASE
VHOST_CONFIG: read message VHOST_USER_SET_VRING_ADDR
VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
VHOST_CONFIG: vring kick idx:1 file:40
VHOST_CONFIG: virtio is not ready for processing.
VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
VHOST_CONFIG: set queue enable: 1 to qp idx: 0
VHOST_CONFIG: read message VHOST_USER_SET_FEATURES
VHOST_CONFIG: read message VHOST_USER_SET_VRING_NUM
VHOST_CONFIG: read message VHOST_USER_SET_VRING_BASE
VHOST_CONFIG: read message VHOST_USER_SET_VRING_ADDR
VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
VHOST_CONFIG: vring kick idx:2 file:41
VHOST_CONFIG: virtio is not ready for processing.
VHOST_CONFIG: read message VHOST_USER_SET_VRING_NUM
VHOST_CONFIG: read message VHOST_USER_SET_VRING_BASE
VHOST_CONFIG: read message VHOST_USER_SET_VRING_ADDR
VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
...
And I didn't see RESET_OWNER is sent while I shutdown a VM.
>
> > And exclude VHOST_USER_GET_QUEUE_NUM as one time request, as we need
> > to get max_queues for each vhost_dev.
>
> Pls split this part out.
>
> > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > ---
> > hw/virtio/vhost-user.c | 1 -
> > hw/virtio/vhost.c | 18 ++++++++++++++++++
> > 2 files changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > index 12a9104..6532a73 100644
> > --- a/hw/virtio/vhost-user.c
> > +++ b/hw/virtio/vhost-user.c
> > @@ -194,7 +194,6 @@ static bool vhost_user_one_time_request(VhostUserRequest request)
> > case VHOST_USER_SET_OWNER:
> > case VHOST_USER_RESET_OWNER:
> > case VHOST_USER_SET_MEM_TABLE:
> > - case VHOST_USER_GET_QUEUE_NUM:
> > return true;
> > default:
> > return false;
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index c0ed5b2..54a4633 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -1146,6 +1146,19 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> > }
> > }
> >
> > + /*
> > + * Send VHOST_USER_SET_VRING_ENABLE message when multiple queue
> > + * is negotiated to inform the back end that we are ready.
> > + *
> > + * Only set enable to 1 for first queue pair, as we enable one
> > + * queue pair by default.
> > + */
> > + if (hdev->max_queues > 1 &&
>
> this makes no sense in the generic code.
> This is a work around for a protocol bug - belongs in
> vhost user internally.
Maybe we could add a dummy vhost_backend_set_vring_enable() for
vhost-kernel?
> And that's not the way to test this. MQ could be negotiated
> even if there is a single pair of queues.
Yeah, right. Just as stated, how about calling it unconditionally here?
--yliu
>
> > + hdev->vhost_ops->vhost_backend_set_vring_enable) {
> > + hdev->vhost_ops->vhost_backend_set_vring_enable(hdev,
> > + hdev->vq_index == 0);
> > + }
> > +
> > return 0;
> > fail_log:
> > vhost_log_put(hdev, false);
> > @@ -1180,5 +1193,10 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
> > hdev->started = false;
> > hdev->log = NULL;
> > hdev->log_size = 0;
> > +
> > + if (hdev->max_queues > 1 &&
> > + hdev->vhost_ops->vhost_backend_set_vring_enable) {
> > + hdev->vhost_ops->vhost_backend_set_vring_enable(hdev, 0);
> > + }
> > }
> >
> > --
> > 1.9.0
> >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v2 5/5] vhost: send VHOST_USER_SET_VRING_ENABLE at start/stop
2015-10-21 13:43 ` Yuanhan Liu
@ 2015-10-21 14:11 ` Michael S. Tsirkin
2015-10-21 14:55 ` Yuanhan Liu
0 siblings, 1 reply; 17+ messages in thread
From: Michael S. Tsirkin @ 2015-10-21 14:11 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: qemu-devel
On Wed, Oct 21, 2015 at 09:43:16PM +0800, Yuanhan Liu wrote:
> On Wed, Oct 21, 2015 at 01:39:11PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Oct 21, 2015 at 05:07:18PM +0800, Yuanhan Liu wrote:
> > > Send VHOST_USER_SET_VRING_ENABLE at start/stop when multiple queue
> > > is negotiated, to inform the backend that we are ready or not.
> >
> > OK but that's only if MQ is set.
>
> Maybe we could just call vhost_backend_set_vring_enable() unconditionally?
> It's nil operation when MQ is not set.
>
> > If now, we need to do
> > RESET_OWNER followed by SET_OWNER.
>
> Could you be more specific? Why sending RESET_OWNER followed by
> SET_OWNER?
>
> TBH, I'm a bit confused with RESET_OWNER now: what it does, and when is
> supposed to send it :(
It's not well specified, but it does say it's analogous to RESET_OWNER
in kernel. That one is well documented:
/* Set current process as the (exclusive) owner of this file descriptor.
* This must be called before any other vhost command. Further calls to
* VHOST_OWNER_SET fail until VHOST_OWNER_RESET is called. */
#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_OWNER _IO(VHOST_VIRTIO, 0x02)
So if we want just the reset part, we need to do VHOST_RESET_OWNER
then redo everything that we did previously: VHOST_SET_OWNER
SET_VRING_CALL etc etc.
> And, sending RESET_OWNER inside virtio_net_reset() also looks weird.
> I made a quick try before sending this patchset, and the vhost-user
> request dump doesn't look right to me: the message is sent after
> vhost dev init (GET_FEATURES, GET_PROTOCOL_FEATURE, SET_OWNER, ...,
> SET_VRING_CALL), and before peer attach (SET_VRING_ENABLE) and
> vhost_dev_start (SET_MEM_TABLE, ... SET_VRING_KICK ...):
Food for thought.
>
> # start of a VM
>
> VHOST_CONFIG: new virtio connection is 28
> VHOST_CONFIG: new device, handle is 0
> VHOST_CONFIG: read message VHOST_USER_GET_FEATURES
> VHOST_CONFIG: read message VHOST_USER_GET_PROTOCOL_FEATURES
> VHOST_CONFIG: read message VHOST_USER_SET_PROTOCOL_FEATURES
> VHOST_CONFIG: read message VHOST_USER_GET_QUEUE_NUM
> VHOST_CONFIG: read message VHOST_USER_SET_OWNER
> VHOST_CONFIG: read message VHOST_USER_GET_FEATURES
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> VHOST_CONFIG: vring call idx:0 file:29
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> VHOST_CONFIG: vring call idx:1 file:30
> VHOST_CONFIG: read message VHOST_USER_GET_FEATURES
> VHOST_CONFIG: read message VHOST_USER_GET_PROTOCOL_FEATURES
> VHOST_CONFIG: read message VHOST_USER_SET_PROTOCOL_FEATURES
> VHOST_CONFIG: read message VHOST_USER_GET_QUEUE_NUM
> VHOST_CONFIG: read message VHOST_USER_GET_FEATURES
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> ...
> ...
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> VHOST_CONFIG: vring call idx:6 file:35
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> VHOST_CONFIG: vring call idx:7 file:36
>
> ==> VHOST_CONFIG: read message VHOST_USER_RESET_OWNER
> VHOST_CONFIG: read message VHOST_USER_RESET_OWNER
>
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
> VHOST_CONFIG: set queue enable: 1 to qp idx: 0
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
> VHOST_CONFIG: set queue enable: 0 to qp idx: 2
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
> VHOST_CONFIG: set queue enable: 0 to qp idx: 4
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
> VHOST_CONFIG: set queue enable: 0 to qp idx: 6
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> VHOST_CONFIG: vring call idx:0 file:29
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> VHOST_CONFIG: vring call idx:1 file:30
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> VHOST_CONFIG: vring call idx:2 file:31
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> VHOST_CONFIG: vring call idx:3 file:32
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> VHOST_CONFIG: vring call idx:4 file:33
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> VHOST_CONFIG: vring call idx:5 file:34
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> VHOST_CONFIG: vring call idx:6 file:35
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> VHOST_CONFIG: vring call idx:7 file:36
> VHOST_CONFIG: read message VHOST_USER_SET_FEATURES
> VHOST_CONFIG: read message VHOST_USER_SET_MEM_TABLE
> VHOST_CONFIG: mapped region 0 fd:37 to 0x2aaac0000000 sz:0xa0000 off:0x0
> VHOST_CONFIG: mapped region 1 fd:38 to 0x2aab00000000 sz:0x80000000 off:0xc0000
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_NUM
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_BASE
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_ADDR
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
> VHOST_CONFIG: vring kick idx:0 file:39
> VHOST_CONFIG: virtio is not ready for processing.
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_NUM
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_BASE
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_ADDR
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
> VHOST_CONFIG: vring kick idx:1 file:40
> VHOST_CONFIG: virtio is not ready for processing.
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
> VHOST_CONFIG: set queue enable: 1 to qp idx: 0
> VHOST_CONFIG: read message VHOST_USER_SET_FEATURES
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_NUM
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_BASE
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_ADDR
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
> VHOST_CONFIG: vring kick idx:2 file:41
> VHOST_CONFIG: virtio is not ready for processing.
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_NUM
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_BASE
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_ADDR
> VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
> ...
>
>
> And I didn't see RESET_OWNER is sent while I shutdown a VM.
>
reboot, not shutdown.
> >
> > > And exclude VHOST_USER_GET_QUEUE_NUM as one time request, as we need
> > > to get max_queues for each vhost_dev.
> >
> > Pls split this part out.
> >
> > > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > > ---
> > > hw/virtio/vhost-user.c | 1 -
> > > hw/virtio/vhost.c | 18 ++++++++++++++++++
> > > 2 files changed, 18 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > index 12a9104..6532a73 100644
> > > --- a/hw/virtio/vhost-user.c
> > > +++ b/hw/virtio/vhost-user.c
> > > @@ -194,7 +194,6 @@ static bool vhost_user_one_time_request(VhostUserRequest request)
> > > case VHOST_USER_SET_OWNER:
> > > case VHOST_USER_RESET_OWNER:
> > > case VHOST_USER_SET_MEM_TABLE:
> > > - case VHOST_USER_GET_QUEUE_NUM:
> > > return true;
> > > default:
> > > return false;
> > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > index c0ed5b2..54a4633 100644
> > > --- a/hw/virtio/vhost.c
> > > +++ b/hw/virtio/vhost.c
> > > @@ -1146,6 +1146,19 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> > > }
> > > }
> > >
> > > + /*
> > > + * Send VHOST_USER_SET_VRING_ENABLE message when multiple queue
> > > + * is negotiated to inform the back end that we are ready.
> > > + *
> > > + * Only set enable to 1 for first queue pair, as we enable one
> > > + * queue pair by default.
> > > + */
> > > + if (hdev->max_queues > 1 &&
> >
> > this makes no sense in the generic code.
> > This is a work around for a protocol bug - belongs in
> > vhost user internally.
>
> Maybe we could add a dummy vhost_backend_set_vring_enable() for
> vhost-kernel?
>
> > And that's not the way to test this. MQ could be negotiated
> > even if there is a single pair of queues.
>
> Yeah, right. Just as stated, how about calling it unconditionally here?
>
> --yliu
I prefer an if condition. Seems easier to follow.
> >
> > > + hdev->vhost_ops->vhost_backend_set_vring_enable) {
> > > + hdev->vhost_ops->vhost_backend_set_vring_enable(hdev,
> > > + hdev->vq_index == 0);
> > > + }
> > > +
> > > return 0;
> > > fail_log:
> > > vhost_log_put(hdev, false);
> > > @@ -1180,5 +1193,10 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
> > > hdev->started = false;
> > > hdev->log = NULL;
> > > hdev->log_size = 0;
> > > +
> > > + if (hdev->max_queues > 1 &&
> > > + hdev->vhost_ops->vhost_backend_set_vring_enable) {
> > > + hdev->vhost_ops->vhost_backend_set_vring_enable(hdev, 0);
> > > + }
> > > }
> > >
> > > --
> > > 1.9.0
> > >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/5] Revert "vhost: rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE"
2015-10-21 13:04 ` Yuanhan Liu
@ 2015-10-21 14:13 ` Michael S. Tsirkin
2015-10-21 14:18 ` Yuanhan Liu
0 siblings, 1 reply; 17+ messages in thread
From: Michael S. Tsirkin @ 2015-10-21 14:13 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: qemu-devel
On Wed, Oct 21, 2015 at 09:04:17PM +0800, Yuanhan Liu wrote:
> On Wed, Oct 21, 2015 at 01:40:59PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Oct 21, 2015 at 05:07:14PM +0800, Yuanhan Liu wrote:
> > > This reverts commit d1f8b30ec8dde0318fd1b98d24a64926feae9625.
> > >
> > > 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>
> >
> >
> > Are these patches dependent on each other? If yes pls send a cover letter.
> > If not pls don't send a series, post them separately.
>
> Got it.
>
> > And, pls Cc me on all vhost user patches.
>
> Isn't I always doing that? :)
>
> --yliu
You didn't for 5/5.
> >
> > > ---
> > > docs/specs/vhost-user.txt | 4 ++--
> > > hw/net/vhost_net.c | 2 +-
> > > hw/virtio/vhost-user.c | 8 ++++----
> > > linux-headers/linux/vhost.h | 2 +-
> > > tests/vhost-user-test.c | 2 +-
> > > 5 files changed, 9 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> > > index 4eadad1..4bcd17d 100644
> > > --- a/docs/specs/vhost-user.txt
> > > +++ b/docs/specs/vhost-user.txt
> > > @@ -211,10 +211,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/net/vhost_net.c b/hw/net/vhost_net.c
> > > index 2bce891..804f5c9 100644
> > > --- a/hw/net/vhost_net.c
> > > +++ b/hw/net/vhost_net.c
> > > @@ -296,7 +296,7 @@ static void vhost_net_stop_one(struct vhost_net *net,
> > > } 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_call(&net->dev, VHOST_RESET_DEVICE,
> > > + int r = vhost_ops->vhost_call(&net->dev, VHOST_RESET_OWNER,
> > > NULL);
> > > assert(r >= 0);
> > > }
> > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > index b11c0d2..12a9104 100644
> > > --- a/hw/virtio/vhost-user.c
> > > +++ b/hw/virtio/vhost-user.c
> > > @@ -34,7 +34,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,
> > > @@ -102,7 +102,7 @@ static unsigned long int ioctl_to_vhost_user_request[VHOST_USER_MAX] = {
> > > VHOST_GET_FEATURES, /* VHOST_USER_GET_FEATURES */
> > > VHOST_SET_FEATURES, /* VHOST_USER_SET_FEATURES */
> > > VHOST_SET_OWNER, /* VHOST_USER_SET_OWNER */
> > > - VHOST_RESET_DEVICE, /* VHOST_USER_RESET_DEVICE */
> > > + VHOST_RESET_OWNER, /* VHOST_USER_RESET_OWNER */
> > > VHOST_SET_MEM_TABLE, /* VHOST_USER_SET_MEM_TABLE */
> > > VHOST_SET_LOG_BASE, /* VHOST_USER_SET_LOG_BASE */
> > > VHOST_SET_LOG_FD, /* VHOST_USER_SET_LOG_FD */
> > > @@ -192,7 +192,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;
> > > @@ -249,7 +249,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> > > break;
> > >
> > > case VHOST_USER_SET_OWNER:
> > > - case VHOST_USER_RESET_DEVICE:
> > > + case VHOST_USER_RESET_OWNER:
> > > break;
> > >
> > > case VHOST_USER_SET_MEM_TABLE:
> > > 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-test.c b/tests/vhost-user-test.c
> > > index 56df5cc..f181391 100644
> > > --- a/tests/vhost-user-test.c
> > > +++ b/tests/vhost-user-test.c
> > > @@ -53,7 +53,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,
> > > --
> > > 1.9.0
> > >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v2 1/5] Revert "vhost: rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE"
2015-10-21 14:13 ` Michael S. Tsirkin
@ 2015-10-21 14:18 ` Yuanhan Liu
0 siblings, 0 replies; 17+ messages in thread
From: Yuanhan Liu @ 2015-10-21 14:18 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
On Wed, Oct 21, 2015 at 05:13:49PM +0300, Michael S. Tsirkin wrote:
> On Wed, Oct 21, 2015 at 09:04:17PM +0800, Yuanhan Liu wrote:
> > On Wed, Oct 21, 2015 at 01:40:59PM +0300, Michael S. Tsirkin wrote:
> > > On Wed, Oct 21, 2015 at 05:07:14PM +0800, Yuanhan Liu wrote:
> > > > This reverts commit d1f8b30ec8dde0318fd1b98d24a64926feae9625.
> > > >
> > > > 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>
> > >
> > >
> > > Are these patches dependent on each other? If yes pls send a cover letter.
> > > If not pls don't send a series, post them separately.
> >
> > Got it.
> >
> > > And, pls Cc me on all vhost user patches.
> >
> > Isn't I always doing that? :)
> >
> > --yliu
>
> You didn't for 5/5.
Oops, that obviously is a mistake. Sorry for that.
--yliu
>
> > >
> > > > ---
> > > > docs/specs/vhost-user.txt | 4 ++--
> > > > hw/net/vhost_net.c | 2 +-
> > > > hw/virtio/vhost-user.c | 8 ++++----
> > > > linux-headers/linux/vhost.h | 2 +-
> > > > tests/vhost-user-test.c | 2 +-
> > > > 5 files changed, 9 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/docs/specs/vhost-user.txt b/docs/specs/vhost-user.txt
> > > > index 4eadad1..4bcd17d 100644
> > > > --- a/docs/specs/vhost-user.txt
> > > > +++ b/docs/specs/vhost-user.txt
> > > > @@ -211,10 +211,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/net/vhost_net.c b/hw/net/vhost_net.c
> > > > index 2bce891..804f5c9 100644
> > > > --- a/hw/net/vhost_net.c
> > > > +++ b/hw/net/vhost_net.c
> > > > @@ -296,7 +296,7 @@ static void vhost_net_stop_one(struct vhost_net *net,
> > > > } 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_call(&net->dev, VHOST_RESET_DEVICE,
> > > > + int r = vhost_ops->vhost_call(&net->dev, VHOST_RESET_OWNER,
> > > > NULL);
> > > > assert(r >= 0);
> > > > }
> > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > > index b11c0d2..12a9104 100644
> > > > --- a/hw/virtio/vhost-user.c
> > > > +++ b/hw/virtio/vhost-user.c
> > > > @@ -34,7 +34,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,
> > > > @@ -102,7 +102,7 @@ static unsigned long int ioctl_to_vhost_user_request[VHOST_USER_MAX] = {
> > > > VHOST_GET_FEATURES, /* VHOST_USER_GET_FEATURES */
> > > > VHOST_SET_FEATURES, /* VHOST_USER_SET_FEATURES */
> > > > VHOST_SET_OWNER, /* VHOST_USER_SET_OWNER */
> > > > - VHOST_RESET_DEVICE, /* VHOST_USER_RESET_DEVICE */
> > > > + VHOST_RESET_OWNER, /* VHOST_USER_RESET_OWNER */
> > > > VHOST_SET_MEM_TABLE, /* VHOST_USER_SET_MEM_TABLE */
> > > > VHOST_SET_LOG_BASE, /* VHOST_USER_SET_LOG_BASE */
> > > > VHOST_SET_LOG_FD, /* VHOST_USER_SET_LOG_FD */
> > > > @@ -192,7 +192,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;
> > > > @@ -249,7 +249,7 @@ static int vhost_user_call(struct vhost_dev *dev, unsigned long int request,
> > > > break;
> > > >
> > > > case VHOST_USER_SET_OWNER:
> > > > - case VHOST_USER_RESET_DEVICE:
> > > > + case VHOST_USER_RESET_OWNER:
> > > > break;
> > > >
> > > > case VHOST_USER_SET_MEM_TABLE:
> > > > 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-test.c b/tests/vhost-user-test.c
> > > > index 56df5cc..f181391 100644
> > > > --- a/tests/vhost-user-test.c
> > > > +++ b/tests/vhost-user-test.c
> > > > @@ -53,7 +53,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,
> > > > --
> > > > 1.9.0
> > > >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v2 5/5] vhost: send VHOST_USER_SET_VRING_ENABLE at start/stop
2015-10-21 14:11 ` Michael S. Tsirkin
@ 2015-10-21 14:55 ` Yuanhan Liu
2015-10-21 14:59 ` Michael S. Tsirkin
0 siblings, 1 reply; 17+ messages in thread
From: Yuanhan Liu @ 2015-10-21 14:55 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
On Wed, Oct 21, 2015 at 05:11:44PM +0300, Michael S. Tsirkin wrote:
> On Wed, Oct 21, 2015 at 09:43:16PM +0800, Yuanhan Liu wrote:
> > On Wed, Oct 21, 2015 at 01:39:11PM +0300, Michael S. Tsirkin wrote:
> > > On Wed, Oct 21, 2015 at 05:07:18PM +0800, Yuanhan Liu wrote:
> > > > Send VHOST_USER_SET_VRING_ENABLE at start/stop when multiple queue
> > > > is negotiated, to inform the backend that we are ready or not.
> > >
> > > OK but that's only if MQ is set.
> >
> > Maybe we could just call vhost_backend_set_vring_enable() unconditionally?
> > It's nil operation when MQ is not set.
> >
> > > If now, we need to do
> > > RESET_OWNER followed by SET_OWNER.
> >
> > Could you be more specific? Why sending RESET_OWNER followed by
> > SET_OWNER?
> >
> > TBH, I'm a bit confused with RESET_OWNER now: what it does, and when is
> > supposed to send it :(
>
> It's not well specified, but it does say it's analogous to RESET_OWNER
> in kernel. That one is well documented:
>
> /* Set current process as the (exclusive) owner of this file descriptor.
> * This must be called before any other vhost command. Further calls to
> * VHOST_OWNER_SET fail until VHOST_OWNER_RESET is called. */
> #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_OWNER _IO(VHOST_VIRTIO, 0x02)
Thanks, that helps (I think).
I recalled my old question, and rechecked your answer again:
Because we need to get the state from remote after stop.
RESET_OWNER discards that, so we can't resume the VM.
So, if I understand it correctly this time, you want to keep the
VM state at the backend side even after the VM is shut down, and
then we can resume it with those saved state
And why don't do that when MQ is enabled? I don't see it has anyting
to do with MQ.
>
> So if we want just the reset part, we need to do VHOST_RESET_OWNER
> then redo everything that we did previously: VHOST_SET_OWNER
> SET_VRING_CALL etc etc.
>
> > And, sending RESET_OWNER inside virtio_net_reset() also looks weird.
> > I made a quick try before sending this patchset, and the vhost-user
> > request dump doesn't look right to me: the message is sent after
> > vhost dev init (GET_FEATURES, GET_PROTOCOL_FEATURE, SET_OWNER, ...,
> > SET_VRING_CALL), and before peer attach (SET_VRING_ENABLE) and
> > vhost_dev_start (SET_MEM_TABLE, ... SET_VRING_KICK ...):
>
> Food for thought.
Aha...
>
> >
> > # start of a VM
> >
> > VHOST_CONFIG: new virtio connection is 28
> > VHOST_CONFIG: new device, handle is 0
> > VHOST_CONFIG: read message VHOST_USER_GET_FEATURES
> > VHOST_CONFIG: read message VHOST_USER_GET_PROTOCOL_FEATURES
> > VHOST_CONFIG: read message VHOST_USER_SET_PROTOCOL_FEATURES
> > VHOST_CONFIG: read message VHOST_USER_GET_QUEUE_NUM
> > VHOST_CONFIG: read message VHOST_USER_SET_OWNER
> > VHOST_CONFIG: read message VHOST_USER_GET_FEATURES
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > VHOST_CONFIG: vring call idx:0 file:29
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > VHOST_CONFIG: vring call idx:1 file:30
> > VHOST_CONFIG: read message VHOST_USER_GET_FEATURES
> > VHOST_CONFIG: read message VHOST_USER_GET_PROTOCOL_FEATURES
> > VHOST_CONFIG: read message VHOST_USER_SET_PROTOCOL_FEATURES
> > VHOST_CONFIG: read message VHOST_USER_GET_QUEUE_NUM
> > VHOST_CONFIG: read message VHOST_USER_GET_FEATURES
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > ...
> > ...
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > VHOST_CONFIG: vring call idx:6 file:35
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > VHOST_CONFIG: vring call idx:7 file:36
> >
> > ==> VHOST_CONFIG: read message VHOST_USER_RESET_OWNER
> > VHOST_CONFIG: read message VHOST_USER_RESET_OWNER
> >
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
> > VHOST_CONFIG: set queue enable: 1 to qp idx: 0
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
> > VHOST_CONFIG: set queue enable: 0 to qp idx: 2
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
> > VHOST_CONFIG: set queue enable: 0 to qp idx: 4
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
> > VHOST_CONFIG: set queue enable: 0 to qp idx: 6
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > VHOST_CONFIG: vring call idx:0 file:29
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > VHOST_CONFIG: vring call idx:1 file:30
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > VHOST_CONFIG: vring call idx:2 file:31
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > VHOST_CONFIG: vring call idx:3 file:32
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > VHOST_CONFIG: vring call idx:4 file:33
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > VHOST_CONFIG: vring call idx:5 file:34
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > VHOST_CONFIG: vring call idx:6 file:35
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > VHOST_CONFIG: vring call idx:7 file:36
> > VHOST_CONFIG: read message VHOST_USER_SET_FEATURES
> > VHOST_CONFIG: read message VHOST_USER_SET_MEM_TABLE
> > VHOST_CONFIG: mapped region 0 fd:37 to 0x2aaac0000000 sz:0xa0000 off:0x0
> > VHOST_CONFIG: mapped region 1 fd:38 to 0x2aab00000000 sz:0x80000000 off:0xc0000
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_NUM
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_BASE
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_ADDR
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
> > VHOST_CONFIG: vring kick idx:0 file:39
> > VHOST_CONFIG: virtio is not ready for processing.
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_NUM
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_BASE
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_ADDR
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
> > VHOST_CONFIG: vring kick idx:1 file:40
> > VHOST_CONFIG: virtio is not ready for processing.
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
> > VHOST_CONFIG: set queue enable: 1 to qp idx: 0
> > VHOST_CONFIG: read message VHOST_USER_SET_FEATURES
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_NUM
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_BASE
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_ADDR
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
> > VHOST_CONFIG: vring kick idx:2 file:41
> > VHOST_CONFIG: virtio is not ready for processing.
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_NUM
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_BASE
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_ADDR
> > VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
> > ...
> >
> >
> > And I didn't see RESET_OWNER is sent while I shutdown a VM.
> >
>
> reboot, not shutdown.
I see.
According to the log, virtio_net_reset() doesn't seem to the right
place, as you can see, SET_OWNER is invoked before RESET_OWNER.
>
>
> > >
> > > > And exclude VHOST_USER_GET_QUEUE_NUM as one time request, as we need
> > > > to get max_queues for each vhost_dev.
> > >
> > > Pls split this part out.
> > >
> > > > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> > > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > > > ---
> > > > hw/virtio/vhost-user.c | 1 -
> > > > hw/virtio/vhost.c | 18 ++++++++++++++++++
> > > > 2 files changed, 18 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > > index 12a9104..6532a73 100644
> > > > --- a/hw/virtio/vhost-user.c
> > > > +++ b/hw/virtio/vhost-user.c
> > > > @@ -194,7 +194,6 @@ static bool vhost_user_one_time_request(VhostUserRequest request)
> > > > case VHOST_USER_SET_OWNER:
> > > > case VHOST_USER_RESET_OWNER:
> > > > case VHOST_USER_SET_MEM_TABLE:
> > > > - case VHOST_USER_GET_QUEUE_NUM:
> > > > return true;
> > > > default:
> > > > return false;
> > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > > index c0ed5b2..54a4633 100644
> > > > --- a/hw/virtio/vhost.c
> > > > +++ b/hw/virtio/vhost.c
> > > > @@ -1146,6 +1146,19 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> > > > }
> > > > }
> > > >
> > > > + /*
> > > > + * Send VHOST_USER_SET_VRING_ENABLE message when multiple queue
> > > > + * is negotiated to inform the back end that we are ready.
> > > > + *
> > > > + * Only set enable to 1 for first queue pair, as we enable one
> > > > + * queue pair by default.
> > > > + */
> > > > + if (hdev->max_queues > 1 &&
> > >
> > > this makes no sense in the generic code.
> > > This is a work around for a protocol bug - belongs in
> > > vhost user internally.
> >
> > Maybe we could add a dummy vhost_backend_set_vring_enable() for
> > vhost-kernel?
> >
> > > And that's not the way to test this. MQ could be negotiated
> > > even if there is a single pair of queues.
> >
> > Yeah, right. Just as stated, how about calling it unconditionally here?
> >
> > --yliu
>
> I prefer an if condition. Seems easier to follow.
Ok.
--yliu
>
> > >
> > > > + hdev->vhost_ops->vhost_backend_set_vring_enable) {
> > > > + hdev->vhost_ops->vhost_backend_set_vring_enable(hdev,
> > > > + hdev->vq_index == 0);
> > > > + }
> > > > +
> > > > return 0;
> > > > fail_log:
> > > > vhost_log_put(hdev, false);
> > > > @@ -1180,5 +1193,10 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
> > > > hdev->started = false;
> > > > hdev->log = NULL;
> > > > hdev->log_size = 0;
> > > > +
> > > > + if (hdev->max_queues > 1 &&
> > > > + hdev->vhost_ops->vhost_backend_set_vring_enable) {
> > > > + hdev->vhost_ops->vhost_backend_set_vring_enable(hdev, 0);
> > > > + }
> > > > }
> > > >
> > > > --
> > > > 1.9.0
> > > >
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH v2 5/5] vhost: send VHOST_USER_SET_VRING_ENABLE at start/stop
2015-10-21 14:55 ` Yuanhan Liu
@ 2015-10-21 14:59 ` Michael S. Tsirkin
0 siblings, 0 replies; 17+ messages in thread
From: Michael S. Tsirkin @ 2015-10-21 14:59 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: qemu-devel
On Wed, Oct 21, 2015 at 10:55:39PM +0800, Yuanhan Liu wrote:
> On Wed, Oct 21, 2015 at 05:11:44PM +0300, Michael S. Tsirkin wrote:
> > On Wed, Oct 21, 2015 at 09:43:16PM +0800, Yuanhan Liu wrote:
> > > On Wed, Oct 21, 2015 at 01:39:11PM +0300, Michael S. Tsirkin wrote:
> > > > On Wed, Oct 21, 2015 at 05:07:18PM +0800, Yuanhan Liu wrote:
> > > > > Send VHOST_USER_SET_VRING_ENABLE at start/stop when multiple queue
> > > > > is negotiated, to inform the backend that we are ready or not.
> > > >
> > > > OK but that's only if MQ is set.
> > >
> > > Maybe we could just call vhost_backend_set_vring_enable() unconditionally?
> > > It's nil operation when MQ is not set.
> > >
> > > > If now, we need to do
> > > > RESET_OWNER followed by SET_OWNER.
> > >
> > > Could you be more specific? Why sending RESET_OWNER followed by
> > > SET_OWNER?
> > >
> > > TBH, I'm a bit confused with RESET_OWNER now: what it does, and when is
> > > supposed to send it :(
> >
> > It's not well specified, but it does say it's analogous to RESET_OWNER
> > in kernel. That one is well documented:
> >
> > /* Set current process as the (exclusive) owner of this file descriptor.
> > * This must be called before any other vhost command. Further calls to
> > * VHOST_OWNER_SET fail until VHOST_OWNER_RESET is called. */
> > #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_OWNER _IO(VHOST_VIRTIO, 0x02)
>
> Thanks, that helps (I think).
>
> I recalled my old question, and rechecked your answer again:
>
> Because we need to get the state from remote after stop.
> RESET_OWNER discards that, so we can't resume the VM.
>
> So, if I understand it correctly this time, you want to keep the
> VM state at the backend side even after the VM is shut down, and
> then we can resume it with those saved state
Not shut down. That makes no sense. When VM is stopped.
> And why don't do that when MQ is enabled? I don't see it has anyting
> to do with MQ.
With MQ we have enable/disable vq so we can just stop them
cleanly.
> >
> > So if we want just the reset part, we need to do VHOST_RESET_OWNER
> > then redo everything that we did previously: VHOST_SET_OWNER
> > SET_VRING_CALL etc etc.
> >
> > > And, sending RESET_OWNER inside virtio_net_reset() also looks weird.
> > > I made a quick try before sending this patchset, and the vhost-user
> > > request dump doesn't look right to me: the message is sent after
> > > vhost dev init (GET_FEATURES, GET_PROTOCOL_FEATURE, SET_OWNER, ...,
> > > SET_VRING_CALL), and before peer attach (SET_VRING_ENABLE) and
> > > vhost_dev_start (SET_MEM_TABLE, ... SET_VRING_KICK ...):
> >
> > Food for thought.
>
> Aha...
>
> >
> > >
> > > # start of a VM
> > >
> > > VHOST_CONFIG: new virtio connection is 28
> > > VHOST_CONFIG: new device, handle is 0
> > > VHOST_CONFIG: read message VHOST_USER_GET_FEATURES
> > > VHOST_CONFIG: read message VHOST_USER_GET_PROTOCOL_FEATURES
> > > VHOST_CONFIG: read message VHOST_USER_SET_PROTOCOL_FEATURES
> > > VHOST_CONFIG: read message VHOST_USER_GET_QUEUE_NUM
> > > VHOST_CONFIG: read message VHOST_USER_SET_OWNER
> > > VHOST_CONFIG: read message VHOST_USER_GET_FEATURES
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > > VHOST_CONFIG: vring call idx:0 file:29
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > > VHOST_CONFIG: vring call idx:1 file:30
> > > VHOST_CONFIG: read message VHOST_USER_GET_FEATURES
> > > VHOST_CONFIG: read message VHOST_USER_GET_PROTOCOL_FEATURES
> > > VHOST_CONFIG: read message VHOST_USER_SET_PROTOCOL_FEATURES
> > > VHOST_CONFIG: read message VHOST_USER_GET_QUEUE_NUM
> > > VHOST_CONFIG: read message VHOST_USER_GET_FEATURES
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > > ...
> > > ...
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > > VHOST_CONFIG: vring call idx:6 file:35
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > > VHOST_CONFIG: vring call idx:7 file:36
> > >
> > > ==> VHOST_CONFIG: read message VHOST_USER_RESET_OWNER
> > > VHOST_CONFIG: read message VHOST_USER_RESET_OWNER
> > >
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
> > > VHOST_CONFIG: set queue enable: 1 to qp idx: 0
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
> > > VHOST_CONFIG: set queue enable: 0 to qp idx: 2
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
> > > VHOST_CONFIG: set queue enable: 0 to qp idx: 4
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
> > > VHOST_CONFIG: set queue enable: 0 to qp idx: 6
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > > VHOST_CONFIG: vring call idx:0 file:29
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > > VHOST_CONFIG: vring call idx:1 file:30
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > > VHOST_CONFIG: vring call idx:2 file:31
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > > VHOST_CONFIG: vring call idx:3 file:32
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > > VHOST_CONFIG: vring call idx:4 file:33
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > > VHOST_CONFIG: vring call idx:5 file:34
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > > VHOST_CONFIG: vring call idx:6 file:35
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_CALL
> > > VHOST_CONFIG: vring call idx:7 file:36
> > > VHOST_CONFIG: read message VHOST_USER_SET_FEATURES
> > > VHOST_CONFIG: read message VHOST_USER_SET_MEM_TABLE
> > > VHOST_CONFIG: mapped region 0 fd:37 to 0x2aaac0000000 sz:0xa0000 off:0x0
> > > VHOST_CONFIG: mapped region 1 fd:38 to 0x2aab00000000 sz:0x80000000 off:0xc0000
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_NUM
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_BASE
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_ADDR
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
> > > VHOST_CONFIG: vring kick idx:0 file:39
> > > VHOST_CONFIG: virtio is not ready for processing.
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_NUM
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_BASE
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_ADDR
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
> > > VHOST_CONFIG: vring kick idx:1 file:40
> > > VHOST_CONFIG: virtio is not ready for processing.
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_ENABLE
> > > VHOST_CONFIG: set queue enable: 1 to qp idx: 0
> > > VHOST_CONFIG: read message VHOST_USER_SET_FEATURES
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_NUM
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_BASE
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_ADDR
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
> > > VHOST_CONFIG: vring kick idx:2 file:41
> > > VHOST_CONFIG: virtio is not ready for processing.
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_NUM
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_BASE
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_ADDR
> > > VHOST_CONFIG: read message VHOST_USER_SET_VRING_KICK
> > > ...
> > >
> > >
> > > And I didn't see RESET_OWNER is sent while I shutdown a VM.
> > >
> >
> > reboot, not shutdown.
>
> I see.
>
>
> According to the log, virtio_net_reset() doesn't seem to the right
> place, as you can see, SET_OWNER is invoked before RESET_OWNER.
>
> >
> >
> > > >
> > > > > And exclude VHOST_USER_GET_QUEUE_NUM as one time request, as we need
> > > > > to get max_queues for each vhost_dev.
> > > >
> > > > Pls split this part out.
> > > >
> > > > > Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > > > > ---
> > > > > hw/virtio/vhost-user.c | 1 -
> > > > > hw/virtio/vhost.c | 18 ++++++++++++++++++
> > > > > 2 files changed, 18 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c
> > > > > index 12a9104..6532a73 100644
> > > > > --- a/hw/virtio/vhost-user.c
> > > > > +++ b/hw/virtio/vhost-user.c
> > > > > @@ -194,7 +194,6 @@ static bool vhost_user_one_time_request(VhostUserRequest request)
> > > > > case VHOST_USER_SET_OWNER:
> > > > > case VHOST_USER_RESET_OWNER:
> > > > > case VHOST_USER_SET_MEM_TABLE:
> > > > > - case VHOST_USER_GET_QUEUE_NUM:
> > > > > return true;
> > > > > default:
> > > > > return false;
> > > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > > > > index c0ed5b2..54a4633 100644
> > > > > --- a/hw/virtio/vhost.c
> > > > > +++ b/hw/virtio/vhost.c
> > > > > @@ -1146,6 +1146,19 @@ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev)
> > > > > }
> > > > > }
> > > > >
> > > > > + /*
> > > > > + * Send VHOST_USER_SET_VRING_ENABLE message when multiple queue
> > > > > + * is negotiated to inform the back end that we are ready.
> > > > > + *
> > > > > + * Only set enable to 1 for first queue pair, as we enable one
> > > > > + * queue pair by default.
> > > > > + */
> > > > > + if (hdev->max_queues > 1 &&
> > > >
> > > > this makes no sense in the generic code.
> > > > This is a work around for a protocol bug - belongs in
> > > > vhost user internally.
> > >
> > > Maybe we could add a dummy vhost_backend_set_vring_enable() for
> > > vhost-kernel?
> > >
> > > > And that's not the way to test this. MQ could be negotiated
> > > > even if there is a single pair of queues.
> > >
> > > Yeah, right. Just as stated, how about calling it unconditionally here?
> > >
> > > --yliu
> >
> > I prefer an if condition. Seems easier to follow.
>
> Ok.
>
> --yliu
> >
> > > >
> > > > > + hdev->vhost_ops->vhost_backend_set_vring_enable) {
> > > > > + hdev->vhost_ops->vhost_backend_set_vring_enable(hdev,
> > > > > + hdev->vq_index == 0);
> > > > > + }
> > > > > +
> > > > > return 0;
> > > > > fail_log:
> > > > > vhost_log_put(hdev, false);
> > > > > @@ -1180,5 +1193,10 @@ void vhost_dev_stop(struct vhost_dev *hdev, VirtIODevice *vdev)
> > > > > hdev->started = false;
> > > > > hdev->log = NULL;
> > > > > hdev->log_size = 0;
> > > > > +
> > > > > + if (hdev->max_queues > 1 &&
> > > > > + hdev->vhost_ops->vhost_backend_set_vring_enable) {
> > > > > + hdev->vhost_ops->vhost_backend_set_vring_enable(hdev, 0);
> > > > > + }
> > > > > }
> > > > >
> > > > > --
> > > > > 1.9.0
> > > > >
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2015-10-21 14:59 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-21 9:07 [Qemu-devel] [PATCH v2 1/5] Revert "vhost: rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE" Yuanhan Liu
2015-10-21 9:07 ` [Qemu-devel] [PATCH v2 2/5] doc: vhost-user: request naming fix Yuanhan Liu
2015-10-21 9:07 ` [Qemu-devel] [PATCH v2 3/5] vhost-user-test: add multiple queue test Yuanhan Liu
2015-10-21 10:34 ` Michael S. Tsirkin
2015-10-21 9:07 ` [Qemu-devel] [PATCH v2 4/5] Revert "vhost-user: Send VHOST_RESET_OWNER on vhost stop" Yuanhan Liu
2015-10-21 10:35 ` Michael S. Tsirkin
2015-10-21 10:39 ` Michael S. Tsirkin
2015-10-21 9:07 ` [Qemu-devel] [PATCH v2 5/5] vhost: send VHOST_USER_SET_VRING_ENABLE at start/stop Yuanhan Liu
2015-10-21 10:39 ` Michael S. Tsirkin
2015-10-21 13:43 ` Yuanhan Liu
2015-10-21 14:11 ` Michael S. Tsirkin
2015-10-21 14:55 ` Yuanhan Liu
2015-10-21 14:59 ` Michael S. Tsirkin
2015-10-21 10:40 ` [Qemu-devel] [PATCH v2 1/5] Revert "vhost: rename VHOST_RESET_OWNER to VHOST_RESET_DEVICE" Michael S. Tsirkin
2015-10-21 13:04 ` Yuanhan Liu
2015-10-21 14:13 ` Michael S. Tsirkin
2015-10-21 14:18 ` Yuanhan Liu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.