* [PATCH 0/5] Vhost-vdpa Shadow Virtqueue multiqueue support.
@ 2022-08-19 17:13 Eugenio Pérez
2022-08-19 17:13 ` [PATCH 1/5] vdpa: extract vhost_vdpa_net_load_mac from vhost_vdpa_net_load Eugenio Pérez
` (4 more replies)
0 siblings, 5 replies; 22+ messages in thread
From: Eugenio Pérez @ 2022-08-19 17:13 UTC (permalink / raw
To: qemu-devel
Cc: Cindy Lu, Eli Cohen, Cornelia Huck, Laurent Vivier, Liuxiangdong,
Paolo Bonzini, Zhu Lingshan, Jason Wang, Harpreet Singh Anand,
Stefan Hajnoczi, Parav Pandit, Gautam Dawar, Stefano Garzarella,
Michael S. Tsirkin, Gonglei (Arei)
This series enables shadowed CVQ to intercept multiqueue commands through
shadowed CVQ, update the virtio NIC device model so qemu send it in a
migration, and the restore of that MQ state in the destination.
It needs to be applied on top of [1].
[1] https://lists.gnu.org/archive/html/qemu-devel/2022-08/msg02965.html
Eugenio Pérez (5):
vdpa: extract vhost_vdpa_net_load_mac from vhost_vdpa_net_load
vdpa: Add vhost_vdpa_net_load_mq
vdpa: validate MQ CVQ commands
virtio-net: Update virtio-net curr_queue_pairs in vdpa backends
vdpa: Allow MQ feture in SVQ
hw/net/virtio-net.c | 17 ++++------
net/vhost-vdpa.c | 82 +++++++++++++++++++++++++++++++++++++--------
2 files changed, 74 insertions(+), 25 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/5] vdpa: extract vhost_vdpa_net_load_mac from vhost_vdpa_net_load
2022-08-19 17:13 [PATCH 0/5] Vhost-vdpa Shadow Virtqueue multiqueue support Eugenio Pérez
@ 2022-08-19 17:13 ` Eugenio Pérez
2022-08-24 4:18 ` Jason Wang
2022-08-19 17:13 ` [PATCH 2/5] vdpa: Add vhost_vdpa_net_load_mq Eugenio Pérez
` (3 subsequent siblings)
4 siblings, 1 reply; 22+ messages in thread
From: Eugenio Pérez @ 2022-08-19 17:13 UTC (permalink / raw
To: qemu-devel
Cc: Cindy Lu, Eli Cohen, Cornelia Huck, Laurent Vivier, Liuxiangdong,
Paolo Bonzini, Zhu Lingshan, Jason Wang, Harpreet Singh Anand,
Stefan Hajnoczi, Parav Pandit, Gautam Dawar, Stefano Garzarella,
Michael S. Tsirkin, Gonglei (Arei)
Since there may be many commands we need to issue to load the NIC
state, let's split them in individual functions
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
net/vhost-vdpa.c | 39 +++++++++++++++++++++++++--------------
1 file changed, 25 insertions(+), 14 deletions(-)
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 97b658f412..1e0dbfcced 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -363,21 +363,10 @@ static ssize_t vhost_vdpa_net_cvq_add(VhostVDPAState *s, size_t out_len,
return vhost_svq_poll(svq);
}
-static int vhost_vdpa_net_load(NetClientState *nc)
+static int vhost_vdpa_net_load_mac(VhostVDPAState *s,
+ const VirtIONet *n)
{
- VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
- const struct vhost_vdpa *v = &s->vhost_vdpa;
- const VirtIONet *n;
- uint64_t features;
-
- assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
-
- if (!v->shadow_vqs_enabled) {
- return 0;
- }
-
- n = VIRTIO_NET(v->dev->vdev);
- features = n->parent_obj.guest_features;
+ uint64_t features = n->parent_obj.guest_features;
if (features & BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR)) {
const struct virtio_net_ctrl_hdr ctrl = {
.class = VIRTIO_NET_CTRL_MAC,
@@ -402,6 +391,28 @@ static int vhost_vdpa_net_load(NetClientState *nc)
return 0;
}
+static int vhost_vdpa_net_load(NetClientState *nc)
+{
+ VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
+ struct vhost_vdpa *v = &s->vhost_vdpa;
+ const VirtIONet *n;
+ int r;
+
+ assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
+
+ if (!v->shadow_vqs_enabled) {
+ return 0;
+ }
+
+ n = VIRTIO_NET(v->dev->vdev);
+ r = vhost_vdpa_net_load_mac(s, n);
+ if (unlikely(r < 0)) {
+ return r;
+ }
+
+ return 0;
+}
+
static NetClientInfo net_vhost_vdpa_cvq_info = {
.type = NET_CLIENT_DRIVER_VHOST_VDPA,
.size = sizeof(VhostVDPAState),
--
2.31.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/5] vdpa: Add vhost_vdpa_net_load_mq
2022-08-19 17:13 [PATCH 0/5] Vhost-vdpa Shadow Virtqueue multiqueue support Eugenio Pérez
2022-08-19 17:13 ` [PATCH 1/5] vdpa: extract vhost_vdpa_net_load_mac from vhost_vdpa_net_load Eugenio Pérez
@ 2022-08-19 17:13 ` Eugenio Pérez
2022-08-24 4:23 ` Jason Wang
2022-08-19 17:13 ` [PATCH 3/5] vdpa: validate MQ CVQ commands Eugenio Pérez
` (2 subsequent siblings)
4 siblings, 1 reply; 22+ messages in thread
From: Eugenio Pérez @ 2022-08-19 17:13 UTC (permalink / raw
To: qemu-devel
Cc: Cindy Lu, Eli Cohen, Cornelia Huck, Laurent Vivier, Liuxiangdong,
Paolo Bonzini, Zhu Lingshan, Jason Wang, Harpreet Singh Anand,
Stefan Hajnoczi, Parav Pandit, Gautam Dawar, Stefano Garzarella,
Michael S. Tsirkin, Gonglei (Arei)
Same way as with the MAC, restore the expected number of queues at
device's start.
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
net/vhost-vdpa.c | 33 +++++++++++++++++++++++++++++++++
1 file changed, 33 insertions(+)
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 1e0dbfcced..96fd3bc835 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -391,6 +391,35 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s,
return 0;
}
+static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
+ const VirtIONet *n)
+{
+ uint64_t features = n->parent_obj.guest_features;
+ ssize_t dev_written;
+ void *cursor = s->cvq_cmd_out_buffer;
+ if (!(features & BIT_ULL(VIRTIO_NET_F_MQ))) {
+ return 0;
+ }
+
+ *(struct virtio_net_ctrl_hdr *)cursor = (struct virtio_net_ctrl_hdr) {
+ .class = VIRTIO_NET_CTRL_MQ,
+ .cmd = VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
+ };
+ cursor += sizeof(struct virtio_net_ctrl_hdr);
+ *(struct virtio_net_ctrl_mq *)cursor = (struct virtio_net_ctrl_mq) {
+ .virtqueue_pairs = cpu_to_le16(n->curr_queue_pairs),
+ };
+ cursor += sizeof(struct virtio_net_ctrl_mq);
+
+ dev_written = vhost_vdpa_net_cvq_add(s, cursor - s->cvq_cmd_out_buffer,
+ sizeof(virtio_net_ctrl_ack));
+ if (unlikely(dev_written < 0)) {
+ return dev_written;
+ }
+
+ return *((virtio_net_ctrl_ack *)s->cvq_cmd_in_buffer) != VIRTIO_NET_OK;
+}
+
static int vhost_vdpa_net_load(NetClientState *nc)
{
VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
@@ -409,6 +438,10 @@ static int vhost_vdpa_net_load(NetClientState *nc)
if (unlikely(r < 0)) {
return r;
}
+ r = vhost_vdpa_net_load_mq(s, n);
+ if (unlikely(r)) {
+ return r;
+ }
return 0;
}
--
2.31.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 3/5] vdpa: validate MQ CVQ commands
2022-08-19 17:13 [PATCH 0/5] Vhost-vdpa Shadow Virtqueue multiqueue support Eugenio Pérez
2022-08-19 17:13 ` [PATCH 1/5] vdpa: extract vhost_vdpa_net_load_mac from vhost_vdpa_net_load Eugenio Pérez
2022-08-19 17:13 ` [PATCH 2/5] vdpa: Add vhost_vdpa_net_load_mq Eugenio Pérez
@ 2022-08-19 17:13 ` Eugenio Pérez
2022-08-19 17:13 ` [PATCH 4/5] virtio-net: Update virtio-net curr_queue_pairs in vdpa backends Eugenio Pérez
2022-08-19 17:13 ` [PATCH 5/5] vdpa: Allow MQ feture in SVQ Eugenio Pérez
4 siblings, 0 replies; 22+ messages in thread
From: Eugenio Pérez @ 2022-08-19 17:13 UTC (permalink / raw
To: qemu-devel
Cc: Cindy Lu, Eli Cohen, Cornelia Huck, Laurent Vivier, Liuxiangdong,
Paolo Bonzini, Zhu Lingshan, Jason Wang, Harpreet Singh Anand,
Stefan Hajnoczi, Parav Pandit, Gautam Dawar, Stefano Garzarella,
Michael S. Tsirkin, Gonglei (Arei)
So we are sure we can update the device model properly before sending to
the device.
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
net/vhost-vdpa.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 96fd3bc835..d5bbda37a1 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -484,6 +484,15 @@ static bool vhost_vdpa_net_cvq_validate_cmd(const void *out_buf, size_t len)
__func__, ctrl.cmd);
};
break;
+ case VIRTIO_NET_CTRL_MQ:
+ switch (ctrl.cmd) {
+ case VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET:
+ return true;
+ default:
+ qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid mq cmd %u\n",
+ __func__, ctrl.cmd);
+ };
+ break;
default:
qemu_log_mask(LOG_GUEST_ERROR, "%s: invalid control class %u\n",
__func__, ctrl.class);
--
2.31.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 4/5] virtio-net: Update virtio-net curr_queue_pairs in vdpa backends
2022-08-19 17:13 [PATCH 0/5] Vhost-vdpa Shadow Virtqueue multiqueue support Eugenio Pérez
` (2 preceding siblings ...)
2022-08-19 17:13 ` [PATCH 3/5] vdpa: validate MQ CVQ commands Eugenio Pérez
@ 2022-08-19 17:13 ` Eugenio Pérez
2022-08-24 4:27 ` Jason Wang
2022-08-19 17:13 ` [PATCH 5/5] vdpa: Allow MQ feture in SVQ Eugenio Pérez
4 siblings, 1 reply; 22+ messages in thread
From: Eugenio Pérez @ 2022-08-19 17:13 UTC (permalink / raw
To: qemu-devel
Cc: Cindy Lu, Eli Cohen, Cornelia Huck, Laurent Vivier, Liuxiangdong,
Paolo Bonzini, Zhu Lingshan, Jason Wang, Harpreet Singh Anand,
Stefan Hajnoczi, Parav Pandit, Gautam Dawar, Stefano Garzarella,
Michael S. Tsirkin, Gonglei (Arei)
It was returned as error before. Instead of it, simply update the
corresponding field so qemu can send it in the migration data.
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
hw/net/virtio-net.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index dd0d056fde..63a8332cd0 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1412,19 +1412,14 @@ static int virtio_net_handle_mq(VirtIONet *n, uint8_t cmd,
return VIRTIO_NET_ERR;
}
- /* Avoid changing the number of queue_pairs for vdpa device in
- * userspace handler. A future fix is needed to handle the mq
- * change in userspace handler with vhost-vdpa. Let's disable
- * the mq handling from userspace for now and only allow get
- * done through the kernel. Ripples may be seen when falling
- * back to userspace, but without doing it qemu process would
- * crash on a recursive entry to virtio_net_set_status().
- */
+ n->curr_queue_pairs = queue_pairs;
if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
- return VIRTIO_NET_ERR;
+ /*
+ * Avoid updating the backend for a vdpa device: We're only interested
+ * in updating the device model queues.
+ */
+ return VIRTIO_NET_OK;
}
-
- n->curr_queue_pairs = queue_pairs;
/* stop the backend before changing the number of queue_pairs to avoid handling a
* disabled queue */
virtio_net_set_status(vdev, vdev->status);
--
2.31.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 5/5] vdpa: Allow MQ feture in SVQ
2022-08-19 17:13 [PATCH 0/5] Vhost-vdpa Shadow Virtqueue multiqueue support Eugenio Pérez
` (3 preceding siblings ...)
2022-08-19 17:13 ` [PATCH 4/5] virtio-net: Update virtio-net curr_queue_pairs in vdpa backends Eugenio Pérez
@ 2022-08-19 17:13 ` Eugenio Pérez
4 siblings, 0 replies; 22+ messages in thread
From: Eugenio Pérez @ 2022-08-19 17:13 UTC (permalink / raw
To: qemu-devel
Cc: Cindy Lu, Eli Cohen, Cornelia Huck, Laurent Vivier, Liuxiangdong,
Paolo Bonzini, Zhu Lingshan, Jason Wang, Harpreet Singh Anand,
Stefan Hajnoczi, Parav Pandit, Gautam Dawar, Stefano Garzarella,
Michael S. Tsirkin, Gonglei (Arei)
Finally enable SVQ with MQ feature.
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
net/vhost-vdpa.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index d5bbda37a1..ccf933de2f 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -92,6 +92,7 @@ static const uint64_t vdpa_svq_device_features =
BIT_ULL(VIRTIO_NET_F_MRG_RXBUF) |
BIT_ULL(VIRTIO_NET_F_STATUS) |
BIT_ULL(VIRTIO_NET_F_CTRL_VQ) |
+ BIT_ULL(VIRTIO_NET_F_MQ) |
BIT_ULL(VIRTIO_F_ANY_LAYOUT) |
BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR) |
BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
--
2.31.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/5] vdpa: extract vhost_vdpa_net_load_mac from vhost_vdpa_net_load
2022-08-19 17:13 ` [PATCH 1/5] vdpa: extract vhost_vdpa_net_load_mac from vhost_vdpa_net_load Eugenio Pérez
@ 2022-08-24 4:18 ` Jason Wang
0 siblings, 0 replies; 22+ messages in thread
From: Jason Wang @ 2022-08-24 4:18 UTC (permalink / raw
To: Eugenio Pérez, qemu-devel
Cc: Cindy Lu, Eli Cohen, Cornelia Huck, Laurent Vivier, Liuxiangdong,
Paolo Bonzini, Zhu Lingshan, Harpreet Singh Anand,
Stefan Hajnoczi, Parav Pandit, Gautam Dawar, Stefano Garzarella,
Michael S. Tsirkin, Gonglei (Arei)
在 2022/8/20 01:13, Eugenio Pérez 写道:
> Since there may be many commands we need to issue to load the NIC
> state, let's split them in individual functions
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
Acked-by: Jason Wang <jasowang@redhat.com>
> ---
> net/vhost-vdpa.c | 39 +++++++++++++++++++++++++--------------
> 1 file changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 97b658f412..1e0dbfcced 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -363,21 +363,10 @@ static ssize_t vhost_vdpa_net_cvq_add(VhostVDPAState *s, size_t out_len,
> return vhost_svq_poll(svq);
> }
>
> -static int vhost_vdpa_net_load(NetClientState *nc)
> +static int vhost_vdpa_net_load_mac(VhostVDPAState *s,
> + const VirtIONet *n)
> {
> - VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> - const struct vhost_vdpa *v = &s->vhost_vdpa;
> - const VirtIONet *n;
> - uint64_t features;
> -
> - assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> -
> - if (!v->shadow_vqs_enabled) {
> - return 0;
> - }
> -
> - n = VIRTIO_NET(v->dev->vdev);
> - features = n->parent_obj.guest_features;
> + uint64_t features = n->parent_obj.guest_features;
> if (features & BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR)) {
> const struct virtio_net_ctrl_hdr ctrl = {
> .class = VIRTIO_NET_CTRL_MAC,
> @@ -402,6 +391,28 @@ static int vhost_vdpa_net_load(NetClientState *nc)
> return 0;
> }
>
> +static int vhost_vdpa_net_load(NetClientState *nc)
> +{
> + VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> + struct vhost_vdpa *v = &s->vhost_vdpa;
> + const VirtIONet *n;
> + int r;
> +
> + assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> +
> + if (!v->shadow_vqs_enabled) {
> + return 0;
> + }
> +
> + n = VIRTIO_NET(v->dev->vdev);
> + r = vhost_vdpa_net_load_mac(s, n);
> + if (unlikely(r < 0)) {
> + return r;
> + }
> +
> + return 0;
> +}
> +
> static NetClientInfo net_vhost_vdpa_cvq_info = {
> .type = NET_CLIENT_DRIVER_VHOST_VDPA,
> .size = sizeof(VhostVDPAState),
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/5] vdpa: Add vhost_vdpa_net_load_mq
2022-08-19 17:13 ` [PATCH 2/5] vdpa: Add vhost_vdpa_net_load_mq Eugenio Pérez
@ 2022-08-24 4:23 ` Jason Wang
2022-08-24 7:46 ` Eugenio Perez Martin
0 siblings, 1 reply; 22+ messages in thread
From: Jason Wang @ 2022-08-24 4:23 UTC (permalink / raw
To: Eugenio Pérez, qemu-devel
Cc: Cindy Lu, Eli Cohen, Cornelia Huck, Laurent Vivier, Liuxiangdong,
Paolo Bonzini, Zhu Lingshan, Harpreet Singh Anand,
Stefan Hajnoczi, Parav Pandit, Gautam Dawar, Stefano Garzarella,
Michael S. Tsirkin, Gonglei (Arei)
在 2022/8/20 01:13, Eugenio Pérez 写道:
> Same way as with the MAC, restore the expected number of queues at
> device's start.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
> net/vhost-vdpa.c | 33 +++++++++++++++++++++++++++++++++
> 1 file changed, 33 insertions(+)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 1e0dbfcced..96fd3bc835 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -391,6 +391,35 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s,
> return 0;
> }
>
> +static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
> + const VirtIONet *n)
> +{
> + uint64_t features = n->parent_obj.guest_features;
> + ssize_t dev_written;
> + void *cursor = s->cvq_cmd_out_buffer;
> + if (!(features & BIT_ULL(VIRTIO_NET_F_MQ))) {
> + return 0;
> + }
> +
> + *(struct virtio_net_ctrl_hdr *)cursor = (struct virtio_net_ctrl_hdr) {
> + .class = VIRTIO_NET_CTRL_MQ,
> + .cmd = VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
> + };
> + cursor += sizeof(struct virtio_net_ctrl_hdr);
> + *(struct virtio_net_ctrl_mq *)cursor = (struct virtio_net_ctrl_mq) {
> + .virtqueue_pairs = cpu_to_le16(n->curr_queue_pairs),
> + };
Such casting is not elegant, let's just prepare buffer and then do the
copy inside vhost_vdpa_net_cvq_add()?
> + cursor += sizeof(struct virtio_net_ctrl_mq);
> +
> + dev_written = vhost_vdpa_net_cvq_add(s, cursor - s->cvq_cmd_out_buffer,
> + sizeof(virtio_net_ctrl_ack));
> + if (unlikely(dev_written < 0)) {
> + return dev_written;
> + }
> +
> + return *((virtio_net_ctrl_ack *)s->cvq_cmd_in_buffer) != VIRTIO_NET_OK;
So I think we should have a dedicated buffer just for ack, then there's
no need for such casting.
Thanks
> +}
> +
> static int vhost_vdpa_net_load(NetClientState *nc)
> {
> VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> @@ -409,6 +438,10 @@ static int vhost_vdpa_net_load(NetClientState *nc)
> if (unlikely(r < 0)) {
> return r;
> }
> + r = vhost_vdpa_net_load_mq(s, n);
> + if (unlikely(r)) {
> + return r;
> + }
>
> return 0;
> }
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/5] virtio-net: Update virtio-net curr_queue_pairs in vdpa backends
2022-08-19 17:13 ` [PATCH 4/5] virtio-net: Update virtio-net curr_queue_pairs in vdpa backends Eugenio Pérez
@ 2022-08-24 4:27 ` Jason Wang
2022-08-25 0:38 ` Si-Wei Liu
0 siblings, 1 reply; 22+ messages in thread
From: Jason Wang @ 2022-08-24 4:27 UTC (permalink / raw
To: Eugenio Pérez, qemu-devel
Cc: Cindy Lu, Eli Cohen, Cornelia Huck, Laurent Vivier, Liuxiangdong,
Paolo Bonzini, Zhu Lingshan, Harpreet Singh Anand,
Stefan Hajnoczi, Parav Pandit, Gautam Dawar, Stefano Garzarella,
Michael S. Tsirkin, Gonglei (Arei), Si-Wei Liu
在 2022/8/20 01:13, Eugenio Pérez 写道:
> It was returned as error before. Instead of it, simply update the
> corresponding field so qemu can send it in the migration data.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
Looks correct.
Adding Si Wei for double check.
Thanks
> hw/net/virtio-net.c | 17 ++++++-----------
> 1 file changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index dd0d056fde..63a8332cd0 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1412,19 +1412,14 @@ static int virtio_net_handle_mq(VirtIONet *n, uint8_t cmd,
> return VIRTIO_NET_ERR;
> }
>
> - /* Avoid changing the number of queue_pairs for vdpa device in
> - * userspace handler. A future fix is needed to handle the mq
> - * change in userspace handler with vhost-vdpa. Let's disable
> - * the mq handling from userspace for now and only allow get
> - * done through the kernel. Ripples may be seen when falling
> - * back to userspace, but without doing it qemu process would
> - * crash on a recursive entry to virtio_net_set_status().
> - */
> + n->curr_queue_pairs = queue_pairs;
> if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
> - return VIRTIO_NET_ERR;
> + /*
> + * Avoid updating the backend for a vdpa device: We're only interested
> + * in updating the device model queues.
> + */
> + return VIRTIO_NET_OK;
> }
> -
> - n->curr_queue_pairs = queue_pairs;
> /* stop the backend before changing the number of queue_pairs to avoid handling a
> * disabled queue */
> virtio_net_set_status(vdev, vdev->status);
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/5] vdpa: Add vhost_vdpa_net_load_mq
2022-08-24 4:23 ` Jason Wang
@ 2022-08-24 7:46 ` Eugenio Perez Martin
2022-08-24 8:52 ` Jason Wang
0 siblings, 1 reply; 22+ messages in thread
From: Eugenio Perez Martin @ 2022-08-24 7:46 UTC (permalink / raw
To: Jason Wang
Cc: qemu-level, Cindy Lu, Eli Cohen, Cornelia Huck, Laurent Vivier,
Liuxiangdong, Paolo Bonzini, Zhu Lingshan, Harpreet Singh Anand,
Stefan Hajnoczi, Parav Pandit, Gautam Dawar, Stefano Garzarella,
Michael S. Tsirkin, Gonglei (Arei)
On Wed, Aug 24, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/8/20 01:13, Eugenio Pérez 写道:
> > Same way as with the MAC, restore the expected number of queues at
> > device's start.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> > net/vhost-vdpa.c | 33 +++++++++++++++++++++++++++++++++
> > 1 file changed, 33 insertions(+)
> >
> > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > index 1e0dbfcced..96fd3bc835 100644
> > --- a/net/vhost-vdpa.c
> > +++ b/net/vhost-vdpa.c
> > @@ -391,6 +391,35 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s,
> > return 0;
> > }
> >
> > +static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
> > + const VirtIONet *n)
> > +{
> > + uint64_t features = n->parent_obj.guest_features;
> > + ssize_t dev_written;
> > + void *cursor = s->cvq_cmd_out_buffer;
> > + if (!(features & BIT_ULL(VIRTIO_NET_F_MQ))) {
> > + return 0;
> > + }
> > +
> > + *(struct virtio_net_ctrl_hdr *)cursor = (struct virtio_net_ctrl_hdr) {
> > + .class = VIRTIO_NET_CTRL_MQ,
> > + .cmd = VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
> > + };
> > + cursor += sizeof(struct virtio_net_ctrl_hdr);
> > + *(struct virtio_net_ctrl_mq *)cursor = (struct virtio_net_ctrl_mq) {
> > + .virtqueue_pairs = cpu_to_le16(n->curr_queue_pairs),
> > + };
>
>
> Such casting is not elegant, let's just prepare buffer and then do the
> copy inside vhost_vdpa_net_cvq_add()?
>
I'm not sure what you propose here. I can pre-fill a buffer in the
stack and then do an extra copy in vhost_vdpa_net_cvq_add. The
compiler should be able to optimize it, but I'm not sure if it
simplifies the code.
We can have a dedicated buffer for mac, another for mq, and one for
each different command, and map all of them at the device's start. But
this seems too much overhead to me.
Some alternatives that come to my mind:
* Declare a struct with both virtio_net_ctrl_hdr and each of the
control commands (using unions?), and cast s->cvq_cmd_out_buffer
accordingly.
* Declare a struct with all of the supported commands one after
another, and let qemu fill and send these accordingly.
>
> > + cursor += sizeof(struct virtio_net_ctrl_mq);
> > +
> > + dev_written = vhost_vdpa_net_cvq_add(s, cursor - s->cvq_cmd_out_buffer,
> > + sizeof(virtio_net_ctrl_ack));
> > + if (unlikely(dev_written < 0)) {
> > + return dev_written;
> > + }
> > +
> > + return *((virtio_net_ctrl_ack *)s->cvq_cmd_in_buffer) != VIRTIO_NET_OK;
>
>
> So I think we should have a dedicated buffer just for ack, then there's
> no need for such casting.
>
You mean to declare cvq_cmd_in_buffer as virtio_net_ctrl_ack type
directly and map it to the device?
Thanks!
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/5] vdpa: Add vhost_vdpa_net_load_mq
2022-08-24 7:46 ` Eugenio Perez Martin
@ 2022-08-24 8:52 ` Jason Wang
2022-08-24 9:06 ` Eugenio Perez Martin
0 siblings, 1 reply; 22+ messages in thread
From: Jason Wang @ 2022-08-24 8:52 UTC (permalink / raw
To: Eugenio Perez Martin
Cc: qemu-level, Cindy Lu, Eli Cohen, Cornelia Huck, Laurent Vivier,
Liuxiangdong, Paolo Bonzini, Zhu Lingshan, Harpreet Singh Anand,
Stefan Hajnoczi, Parav Pandit, Gautam Dawar, Stefano Garzarella,
Michael S. Tsirkin, Gonglei (Arei)
On Wed, Aug 24, 2022 at 3:47 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Wed, Aug 24, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> >
> > 在 2022/8/20 01:13, Eugenio Pérez 写道:
> > > Same way as with the MAC, restore the expected number of queues at
> > > device's start.
> > >
> > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > ---
> > > net/vhost-vdpa.c | 33 +++++++++++++++++++++++++++++++++
> > > 1 file changed, 33 insertions(+)
> > >
> > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > index 1e0dbfcced..96fd3bc835 100644
> > > --- a/net/vhost-vdpa.c
> > > +++ b/net/vhost-vdpa.c
> > > @@ -391,6 +391,35 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s,
> > > return 0;
> > > }
> > >
> > > +static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
> > > + const VirtIONet *n)
> > > +{
> > > + uint64_t features = n->parent_obj.guest_features;
> > > + ssize_t dev_written;
> > > + void *cursor = s->cvq_cmd_out_buffer;
> > > + if (!(features & BIT_ULL(VIRTIO_NET_F_MQ))) {
> > > + return 0;
> > > + }
> > > +
> > > + *(struct virtio_net_ctrl_hdr *)cursor = (struct virtio_net_ctrl_hdr) {
> > > + .class = VIRTIO_NET_CTRL_MQ,
> > > + .cmd = VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
> > > + };
> > > + cursor += sizeof(struct virtio_net_ctrl_hdr);
> > > + *(struct virtio_net_ctrl_mq *)cursor = (struct virtio_net_ctrl_mq) {
> > > + .virtqueue_pairs = cpu_to_le16(n->curr_queue_pairs),
> > > + };
> >
> >
> > Such casting is not elegant, let's just prepare buffer and then do the
> > copy inside vhost_vdpa_net_cvq_add()?
> >
>
> I'm not sure what you propose here. I can pre-fill a buffer in the
> stack and then do an extra copy in vhost_vdpa_net_cvq_add. The
> compiler should be able to optimize it, but I'm not sure if it
> simplifies the code.
>
> We can have a dedicated buffer for mac, another for mq, and one for
> each different command, and map all of them at the device's start. But
> this seems too much overhead to me.
Considering we may need to support and restore a lot of other fields,
this looks a little complicated.
I meant the caller can simply do:
struct virtio_net_ctrl_mq mq = { ...};
Then we do
vhost_vdpa_net_cvq_add(&mq, sizeof(mq), ...);
Then we can do memcpy inside vhost_vdpa_net_cvq_add() and hide the
cmd_out_buffer etc from the caller.
>
> Some alternatives that come to my mind:
>
> * Declare a struct with both virtio_net_ctrl_hdr and each of the
> control commands (using unions?), and cast s->cvq_cmd_out_buffer
> accordingly.
> * Declare a struct with all of the supported commands one after
> another, and let qemu fill and send these accordingly.
>
> >
> > > + cursor += sizeof(struct virtio_net_ctrl_mq);
> > > +
> > > + dev_written = vhost_vdpa_net_cvq_add(s, cursor - s->cvq_cmd_out_buffer,
> > > + sizeof(virtio_net_ctrl_ack));
> > > + if (unlikely(dev_written < 0)) {
> > > + return dev_written;
> > > + }
> > > +
> > > + return *((virtio_net_ctrl_ack *)s->cvq_cmd_in_buffer) != VIRTIO_NET_OK;
> >
> >
> > So I think we should have a dedicated buffer just for ack, then there's
> > no need for such casting.
> >
>
> You mean to declare cvq_cmd_in_buffer as virtio_net_ctrl_ack type
> directly and map it to the device?
Kind of, considering the ack is the only kind of structure in the near
future, can we simply use the structure virtio_net_ctl_ack?
Thanks
>
> Thanks!
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/5] vdpa: Add vhost_vdpa_net_load_mq
2022-08-24 8:52 ` Jason Wang
@ 2022-08-24 9:06 ` Eugenio Perez Martin
2022-08-24 9:07 ` Jason Wang
0 siblings, 1 reply; 22+ messages in thread
From: Eugenio Perez Martin @ 2022-08-24 9:06 UTC (permalink / raw
To: Jason Wang
Cc: qemu-level, Cindy Lu, Eli Cohen, Cornelia Huck, Laurent Vivier,
Liuxiangdong, Paolo Bonzini, Zhu Lingshan, Harpreet Singh Anand,
Stefan Hajnoczi, Parav Pandit, Gautam Dawar, Stefano Garzarella,
Michael S. Tsirkin, Gonglei (Arei)
On Wed, Aug 24, 2022 at 10:52 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Aug 24, 2022 at 3:47 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Wed, Aug 24, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > >
> > > 在 2022/8/20 01:13, Eugenio Pérez 写道:
> > > > Same way as with the MAC, restore the expected number of queues at
> > > > device's start.
> > > >
> > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > ---
> > > > net/vhost-vdpa.c | 33 +++++++++++++++++++++++++++++++++
> > > > 1 file changed, 33 insertions(+)
> > > >
> > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > index 1e0dbfcced..96fd3bc835 100644
> > > > --- a/net/vhost-vdpa.c
> > > > +++ b/net/vhost-vdpa.c
> > > > @@ -391,6 +391,35 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s,
> > > > return 0;
> > > > }
> > > >
> > > > +static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
> > > > + const VirtIONet *n)
> > > > +{
> > > > + uint64_t features = n->parent_obj.guest_features;
> > > > + ssize_t dev_written;
> > > > + void *cursor = s->cvq_cmd_out_buffer;
> > > > + if (!(features & BIT_ULL(VIRTIO_NET_F_MQ))) {
> > > > + return 0;
> > > > + }
> > > > +
> > > > + *(struct virtio_net_ctrl_hdr *)cursor = (struct virtio_net_ctrl_hdr) {
> > > > + .class = VIRTIO_NET_CTRL_MQ,
> > > > + .cmd = VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
> > > > + };
> > > > + cursor += sizeof(struct virtio_net_ctrl_hdr);
> > > > + *(struct virtio_net_ctrl_mq *)cursor = (struct virtio_net_ctrl_mq) {
> > > > + .virtqueue_pairs = cpu_to_le16(n->curr_queue_pairs),
> > > > + };
> > >
> > >
> > > Such casting is not elegant, let's just prepare buffer and then do the
> > > copy inside vhost_vdpa_net_cvq_add()?
> > >
> >
> > I'm not sure what you propose here. I can pre-fill a buffer in the
> > stack and then do an extra copy in vhost_vdpa_net_cvq_add. The
> > compiler should be able to optimize it, but I'm not sure if it
> > simplifies the code.
> >
> > We can have a dedicated buffer for mac, another for mq, and one for
> > each different command, and map all of them at the device's start. But
> > this seems too much overhead to me.
>
> Considering we may need to support and restore a lot of other fields,
> this looks a little complicated.
>
> I meant the caller can simply do:
>
> struct virtio_net_ctrl_mq mq = { ...};
>
> Then we do
>
> vhost_vdpa_net_cvq_add(&mq, sizeof(mq), ...);
>
> Then we can do memcpy inside vhost_vdpa_net_cvq_add() and hide the
> cmd_out_buffer etc from the caller.
>
We need to add the ctrl header too. But yes, that is feasible, something like:
vhost_vdpa_net_cvq_add(&ctrl, &mq, sizeof(mq), ...);
> >
> > Some alternatives that come to my mind:
> >
> > * Declare a struct with both virtio_net_ctrl_hdr and each of the
> > control commands (using unions?), and cast s->cvq_cmd_out_buffer
> > accordingly.
> > * Declare a struct with all of the supported commands one after
> > another, and let qemu fill and send these accordingly.
> >
> > >
> > > > + cursor += sizeof(struct virtio_net_ctrl_mq);
> > > > +
> > > > + dev_written = vhost_vdpa_net_cvq_add(s, cursor - s->cvq_cmd_out_buffer,
> > > > + sizeof(virtio_net_ctrl_ack));
> > > > + if (unlikely(dev_written < 0)) {
> > > > + return dev_written;
> > > > + }
> > > > +
> > > > + return *((virtio_net_ctrl_ack *)s->cvq_cmd_in_buffer) != VIRTIO_NET_OK;
> > >
> > >
> > > So I think we should have a dedicated buffer just for ack, then there's
> > > no need for such casting.
> > >
> >
> > You mean to declare cvq_cmd_in_buffer as virtio_net_ctrl_ack type
> > directly and map it to the device?
>
> Kind of, considering the ack is the only kind of structure in the near
> future, can we simply use the structure virtio_net_ctl_ack?
>
Almost, but we need to map to the device in a page size. And I think
it's better to allocate a whole page for that, so it does not share
memory with qemu.
Other than that, yes, I think it can be declared as virtio_net_ctl_ack directly.
Thanks!
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/5] vdpa: Add vhost_vdpa_net_load_mq
2022-08-24 9:06 ` Eugenio Perez Martin
@ 2022-08-24 9:07 ` Jason Wang
2022-08-24 9:18 ` Eugenio Perez Martin
0 siblings, 1 reply; 22+ messages in thread
From: Jason Wang @ 2022-08-24 9:07 UTC (permalink / raw
To: Eugenio Perez Martin
Cc: qemu-level, Cindy Lu, Eli Cohen, Cornelia Huck, Laurent Vivier,
Liuxiangdong, Paolo Bonzini, Zhu Lingshan, Harpreet Singh Anand,
Stefan Hajnoczi, Parav Pandit, Gautam Dawar, Stefano Garzarella,
Michael S. Tsirkin, Gonglei (Arei)
On Wed, Aug 24, 2022 at 5:06 PM Eugenio Perez Martin
<eperezma@redhat.com> wrote:
>
> On Wed, Aug 24, 2022 at 10:52 AM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Wed, Aug 24, 2022 at 3:47 PM Eugenio Perez Martin
> > <eperezma@redhat.com> wrote:
> > >
> > > On Wed, Aug 24, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > >
> > > > 在 2022/8/20 01:13, Eugenio Pérez 写道:
> > > > > Same way as with the MAC, restore the expected number of queues at
> > > > > device's start.
> > > > >
> > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > ---
> > > > > net/vhost-vdpa.c | 33 +++++++++++++++++++++++++++++++++
> > > > > 1 file changed, 33 insertions(+)
> > > > >
> > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > > index 1e0dbfcced..96fd3bc835 100644
> > > > > --- a/net/vhost-vdpa.c
> > > > > +++ b/net/vhost-vdpa.c
> > > > > @@ -391,6 +391,35 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s,
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
> > > > > + const VirtIONet *n)
> > > > > +{
> > > > > + uint64_t features = n->parent_obj.guest_features;
> > > > > + ssize_t dev_written;
> > > > > + void *cursor = s->cvq_cmd_out_buffer;
> > > > > + if (!(features & BIT_ULL(VIRTIO_NET_F_MQ))) {
> > > > > + return 0;
> > > > > + }
> > > > > +
> > > > > + *(struct virtio_net_ctrl_hdr *)cursor = (struct virtio_net_ctrl_hdr) {
> > > > > + .class = VIRTIO_NET_CTRL_MQ,
> > > > > + .cmd = VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
> > > > > + };
> > > > > + cursor += sizeof(struct virtio_net_ctrl_hdr);
> > > > > + *(struct virtio_net_ctrl_mq *)cursor = (struct virtio_net_ctrl_mq) {
> > > > > + .virtqueue_pairs = cpu_to_le16(n->curr_queue_pairs),
> > > > > + };
> > > >
> > > >
> > > > Such casting is not elegant, let's just prepare buffer and then do the
> > > > copy inside vhost_vdpa_net_cvq_add()?
> > > >
> > >
> > > I'm not sure what you propose here. I can pre-fill a buffer in the
> > > stack and then do an extra copy in vhost_vdpa_net_cvq_add. The
> > > compiler should be able to optimize it, but I'm not sure if it
> > > simplifies the code.
> > >
> > > We can have a dedicated buffer for mac, another for mq, and one for
> > > each different command, and map all of them at the device's start. But
> > > this seems too much overhead to me.
> >
> > Considering we may need to support and restore a lot of other fields,
> > this looks a little complicated.
> >
> > I meant the caller can simply do:
> >
> > struct virtio_net_ctrl_mq mq = { ...};
> >
> > Then we do
> >
> > vhost_vdpa_net_cvq_add(&mq, sizeof(mq), ...);
> >
> > Then we can do memcpy inside vhost_vdpa_net_cvq_add() and hide the
> > cmd_out_buffer etc from the caller.
> >
>
> We need to add the ctrl header too. But yes, that is feasible, something like:
>
> vhost_vdpa_net_cvq_add(&ctrl, &mq, sizeof(mq), ...);
>
> > >
> > > Some alternatives that come to my mind:
> > >
> > > * Declare a struct with both virtio_net_ctrl_hdr and each of the
> > > control commands (using unions?), and cast s->cvq_cmd_out_buffer
> > > accordingly.
> > > * Declare a struct with all of the supported commands one after
> > > another, and let qemu fill and send these accordingly.
> > >
> > > >
> > > > > + cursor += sizeof(struct virtio_net_ctrl_mq);
> > > > > +
> > > > > + dev_written = vhost_vdpa_net_cvq_add(s, cursor - s->cvq_cmd_out_buffer,
> > > > > + sizeof(virtio_net_ctrl_ack));
> > > > > + if (unlikely(dev_written < 0)) {
> > > > > + return dev_written;
> > > > > + }
> > > > > +
> > > > > + return *((virtio_net_ctrl_ack *)s->cvq_cmd_in_buffer) != VIRTIO_NET_OK;
> > > >
> > > >
> > > > So I think we should have a dedicated buffer just for ack, then there's
> > > > no need for such casting.
> > > >
> > >
> > > You mean to declare cvq_cmd_in_buffer as virtio_net_ctrl_ack type
> > > directly and map it to the device?
> >
> > Kind of, considering the ack is the only kind of structure in the near
> > future, can we simply use the structure virtio_net_ctl_ack?
> >
>
> Almost, but we need to map to the device in a page size. And I think
> it's better to allocate a whole page for that, so it does not share
> memory with qemu.
I guess using a union will solve the problem?
Thanks
>
> Other than that, yes, I think it can be declared as virtio_net_ctl_ack directly.
>
> Thanks!
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/5] vdpa: Add vhost_vdpa_net_load_mq
2022-08-24 9:07 ` Jason Wang
@ 2022-08-24 9:18 ` Eugenio Perez Martin
0 siblings, 0 replies; 22+ messages in thread
From: Eugenio Perez Martin @ 2022-08-24 9:18 UTC (permalink / raw
To: Jason Wang
Cc: qemu-level, Cindy Lu, Eli Cohen, Cornelia Huck, Laurent Vivier,
Liuxiangdong, Paolo Bonzini, Zhu Lingshan, Harpreet Singh Anand,
Stefan Hajnoczi, Parav Pandit, Gautam Dawar, Stefano Garzarella,
Michael S. Tsirkin, Gonglei (Arei)
On Wed, Aug 24, 2022 at 11:08 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Wed, Aug 24, 2022 at 5:06 PM Eugenio Perez Martin
> <eperezma@redhat.com> wrote:
> >
> > On Wed, Aug 24, 2022 at 10:52 AM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Wed, Aug 24, 2022 at 3:47 PM Eugenio Perez Martin
> > > <eperezma@redhat.com> wrote:
> > > >
> > > > On Wed, Aug 24, 2022 at 6:23 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > >
> > > > > 在 2022/8/20 01:13, Eugenio Pérez 写道:
> > > > > > Same way as with the MAC, restore the expected number of queues at
> > > > > > device's start.
> > > > > >
> > > > > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > > > > > ---
> > > > > > net/vhost-vdpa.c | 33 +++++++++++++++++++++++++++++++++
> > > > > > 1 file changed, 33 insertions(+)
> > > > > >
> > > > > > diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> > > > > > index 1e0dbfcced..96fd3bc835 100644
> > > > > > --- a/net/vhost-vdpa.c
> > > > > > +++ b/net/vhost-vdpa.c
> > > > > > @@ -391,6 +391,35 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s,
> > > > > > return 0;
> > > > > > }
> > > > > >
> > > > > > +static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
> > > > > > + const VirtIONet *n)
> > > > > > +{
> > > > > > + uint64_t features = n->parent_obj.guest_features;
> > > > > > + ssize_t dev_written;
> > > > > > + void *cursor = s->cvq_cmd_out_buffer;
> > > > > > + if (!(features & BIT_ULL(VIRTIO_NET_F_MQ))) {
> > > > > > + return 0;
> > > > > > + }
> > > > > > +
> > > > > > + *(struct virtio_net_ctrl_hdr *)cursor = (struct virtio_net_ctrl_hdr) {
> > > > > > + .class = VIRTIO_NET_CTRL_MQ,
> > > > > > + .cmd = VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
> > > > > > + };
> > > > > > + cursor += sizeof(struct virtio_net_ctrl_hdr);
> > > > > > + *(struct virtio_net_ctrl_mq *)cursor = (struct virtio_net_ctrl_mq) {
> > > > > > + .virtqueue_pairs = cpu_to_le16(n->curr_queue_pairs),
> > > > > > + };
> > > > >
> > > > >
> > > > > Such casting is not elegant, let's just prepare buffer and then do the
> > > > > copy inside vhost_vdpa_net_cvq_add()?
> > > > >
> > > >
> > > > I'm not sure what you propose here. I can pre-fill a buffer in the
> > > > stack and then do an extra copy in vhost_vdpa_net_cvq_add. The
> > > > compiler should be able to optimize it, but I'm not sure if it
> > > > simplifies the code.
> > > >
> > > > We can have a dedicated buffer for mac, another for mq, and one for
> > > > each different command, and map all of them at the device's start. But
> > > > this seems too much overhead to me.
> > >
> > > Considering we may need to support and restore a lot of other fields,
> > > this looks a little complicated.
> > >
> > > I meant the caller can simply do:
> > >
> > > struct virtio_net_ctrl_mq mq = { ...};
> > >
> > > Then we do
> > >
> > > vhost_vdpa_net_cvq_add(&mq, sizeof(mq), ...);
> > >
> > > Then we can do memcpy inside vhost_vdpa_net_cvq_add() and hide the
> > > cmd_out_buffer etc from the caller.
> > >
> >
> > We need to add the ctrl header too. But yes, that is feasible, something like:
> >
> > vhost_vdpa_net_cvq_add(&ctrl, &mq, sizeof(mq), ...);
> >
> > > >
> > > > Some alternatives that come to my mind:
> > > >
> > > > * Declare a struct with both virtio_net_ctrl_hdr and each of the
> > > > control commands (using unions?), and cast s->cvq_cmd_out_buffer
> > > > accordingly.
> > > > * Declare a struct with all of the supported commands one after
> > > > another, and let qemu fill and send these accordingly.
> > > >
> > > > >
> > > > > > + cursor += sizeof(struct virtio_net_ctrl_mq);
> > > > > > +
> > > > > > + dev_written = vhost_vdpa_net_cvq_add(s, cursor - s->cvq_cmd_out_buffer,
> > > > > > + sizeof(virtio_net_ctrl_ack));
> > > > > > + if (unlikely(dev_written < 0)) {
> > > > > > + return dev_written;
> > > > > > + }
> > > > > > +
> > > > > > + return *((virtio_net_ctrl_ack *)s->cvq_cmd_in_buffer) != VIRTIO_NET_OK;
> > > > >
> > > > >
> > > > > So I think we should have a dedicated buffer just for ack, then there's
> > > > > no need for such casting.
> > > > >
> > > >
> > > > You mean to declare cvq_cmd_in_buffer as virtio_net_ctrl_ack type
> > > > directly and map it to the device?
> > >
> > > Kind of, considering the ack is the only kind of structure in the near
> > > future, can we simply use the structure virtio_net_ctl_ack?
> > >
> >
> > Almost, but we need to map to the device in a page size. And I think
> > it's better to allocate a whole page for that, so it does not share
> > memory with qemu.
>
> I guess using a union will solve the problem?
>
It was more a nitpick than a problem, pointing out the need to
allocate a whole page casting it or not to virtio_net_ctrl_ack. In
other words, we must init status as "status =
g_malloc0(real_host_page_size())", not "g_malloc0(sizeof(*status))".
But I think the union is a good idea. The problem is that
qemu_real_host_page_size is not a constant so we cannot declare a type
with that.
> Thanks
>
> >
> > Other than that, yes, I think it can be declared as virtio_net_ctl_ack directly.
> >
> > Thanks!
> >
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/5] virtio-net: Update virtio-net curr_queue_pairs in vdpa backends
2022-08-24 4:27 ` Jason Wang
@ 2022-08-25 0:38 ` Si-Wei Liu
2022-08-25 2:53 ` Jason Wang
2022-08-25 6:19 ` Eugenio Perez Martin
0 siblings, 2 replies; 22+ messages in thread
From: Si-Wei Liu @ 2022-08-25 0:38 UTC (permalink / raw
To: Jason Wang, Eugenio Pérez, qemu-devel
Cc: Cindy Lu, Eli Cohen, Cornelia Huck, Laurent Vivier, Liuxiangdong,
Paolo Bonzini, Zhu Lingshan, Harpreet Singh Anand,
Stefan Hajnoczi, Parav Pandit, Gautam Dawar, Stefano Garzarella,
Michael S. Tsirkin, Gonglei (Arei)
On 8/23/2022 9:27 PM, Jason Wang wrote:
>
> 在 2022/8/20 01:13, Eugenio Pérez 写道:
>> It was returned as error before. Instead of it, simply update the
>> corresponding field so qemu can send it in the migration data.
>>
>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>> ---
>
>
> Looks correct.
>
> Adding Si Wei for double check.
Hmmm, I understand why this change is needed for live migration, but
this would easily cause userspace out of sync with the kernel for other
use cases, such as link down or userspace fallback due to vdpa ioctl
error. Yes, these are edge cases. Not completely against it, but I
wonder if there's a way we can limit the change scope to live migration
case only?
-Siwei
>
> Thanks
>
>
>> hw/net/virtio-net.c | 17 ++++++-----------
>> 1 file changed, 6 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index dd0d056fde..63a8332cd0 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -1412,19 +1412,14 @@ static int virtio_net_handle_mq(VirtIONet *n,
>> uint8_t cmd,
>> return VIRTIO_NET_ERR;
>> }
>> - /* Avoid changing the number of queue_pairs for vdpa device in
>> - * userspace handler. A future fix is needed to handle the mq
>> - * change in userspace handler with vhost-vdpa. Let's disable
>> - * the mq handling from userspace for now and only allow get
>> - * done through the kernel. Ripples may be seen when falling
>> - * back to userspace, but without doing it qemu process would
>> - * crash on a recursive entry to virtio_net_set_status().
>> - */
>> + n->curr_queue_pairs = queue_pairs;
>> if (nc->peer && nc->peer->info->type ==
>> NET_CLIENT_DRIVER_VHOST_VDPA) {
>> - return VIRTIO_NET_ERR;
>> + /*
>> + * Avoid updating the backend for a vdpa device: We're only
>> interested
>> + * in updating the device model queues.
>> + */
>> + return VIRTIO_NET_OK;
>> }
>> -
>> - n->curr_queue_pairs = queue_pairs;
>> /* stop the backend before changing the number of queue_pairs
>> to avoid handling a
>> * disabled queue */
>> virtio_net_set_status(vdev, vdev->status);
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/5] virtio-net: Update virtio-net curr_queue_pairs in vdpa backends
2022-08-25 0:38 ` Si-Wei Liu
@ 2022-08-25 2:53 ` Jason Wang
2022-08-25 3:05 ` Jason Wang
2022-08-26 3:49 ` Si-Wei Liu
2022-08-25 6:19 ` Eugenio Perez Martin
1 sibling, 2 replies; 22+ messages in thread
From: Jason Wang @ 2022-08-25 2:53 UTC (permalink / raw
To: Si-Wei Liu
Cc: Eugenio Pérez, qemu-devel, Cindy Lu, Eli Cohen,
Cornelia Huck, Laurent Vivier, Liuxiangdong, Paolo Bonzini,
Zhu Lingshan, Harpreet Singh Anand, Stefan Hajnoczi, Parav Pandit,
Gautam Dawar, Stefano Garzarella, Michael S. Tsirkin,
Gonglei (Arei)
On Thu, Aug 25, 2022 at 8:38 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 8/23/2022 9:27 PM, Jason Wang wrote:
> >
> > 在 2022/8/20 01:13, Eugenio Pérez 写道:
> >> It was returned as error before. Instead of it, simply update the
> >> corresponding field so qemu can send it in the migration data.
> >>
> >> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >> ---
> >
> >
> > Looks correct.
> >
> > Adding Si Wei for double check.
> Hmmm, I understand why this change is needed for live migration, but
> this would easily cause userspace out of sync with the kernel for other
> use cases, such as link down or userspace fallback due to vdpa ioctl
> error. Yes, these are edge cases.
Considering 7.2 will start, maybe it's time to fix the root cause
instead of having a workaround like this?
THanks
> Not completely against it, but I
> wonder if there's a way we can limit the change scope to live migration
> case only?
>
> -Siwei
>
> >
> > Thanks
> >
> >
> >> hw/net/virtio-net.c | 17 ++++++-----------
> >> 1 file changed, 6 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >> index dd0d056fde..63a8332cd0 100644
> >> --- a/hw/net/virtio-net.c
> >> +++ b/hw/net/virtio-net.c
> >> @@ -1412,19 +1412,14 @@ static int virtio_net_handle_mq(VirtIONet *n,
> >> uint8_t cmd,
> >> return VIRTIO_NET_ERR;
> >> }
> >> - /* Avoid changing the number of queue_pairs for vdpa device in
> >> - * userspace handler. A future fix is needed to handle the mq
> >> - * change in userspace handler with vhost-vdpa. Let's disable
> >> - * the mq handling from userspace for now and only allow get
> >> - * done through the kernel. Ripples may be seen when falling
> >> - * back to userspace, but without doing it qemu process would
> >> - * crash on a recursive entry to virtio_net_set_status().
> >> - */
> >> + n->curr_queue_pairs = queue_pairs;
> >> if (nc->peer && nc->peer->info->type ==
> >> NET_CLIENT_DRIVER_VHOST_VDPA) {
> >> - return VIRTIO_NET_ERR;
> >> + /*
> >> + * Avoid updating the backend for a vdpa device: We're only
> >> interested
> >> + * in updating the device model queues.
> >> + */
> >> + return VIRTIO_NET_OK;
> >> }
> >> -
> >> - n->curr_queue_pairs = queue_pairs;
> >> /* stop the backend before changing the number of queue_pairs
> >> to avoid handling a
> >> * disabled queue */
> >> virtio_net_set_status(vdev, vdev->status);
> >
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/5] virtio-net: Update virtio-net curr_queue_pairs in vdpa backends
2022-08-25 2:53 ` Jason Wang
@ 2022-08-25 3:05 ` Jason Wang
2022-08-26 3:58 ` Si-Wei Liu
2022-08-26 3:49 ` Si-Wei Liu
1 sibling, 1 reply; 22+ messages in thread
From: Jason Wang @ 2022-08-25 3:05 UTC (permalink / raw
To: Si-Wei Liu
Cc: Eugenio Pérez, qemu-devel, Cindy Lu, Eli Cohen,
Cornelia Huck, Laurent Vivier, Liuxiangdong, Paolo Bonzini,
Zhu Lingshan, Harpreet Singh Anand, Stefan Hajnoczi, Parav Pandit,
Gautam Dawar, Stefano Garzarella, Michael S. Tsirkin,
Gonglei (Arei)
On Thu, Aug 25, 2022 at 10:53 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Aug 25, 2022 at 8:38 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
> >
> >
> >
> > On 8/23/2022 9:27 PM, Jason Wang wrote:
> > >
> > > 在 2022/8/20 01:13, Eugenio Pérez 写道:
> > >> It was returned as error before. Instead of it, simply update the
> > >> corresponding field so qemu can send it in the migration data.
> > >>
> > >> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > >> ---
> > >
> > >
> > > Looks correct.
> > >
> > > Adding Si Wei for double check.
> > Hmmm, I understand why this change is needed for live migration, but
> > this would easily cause userspace out of sync with the kernel for other
> > use cases, such as link down or userspace fallback due to vdpa ioctl
> > error. Yes, these are edge cases.
>
> Considering 7.2 will start, maybe it's time to fix the root cause
> instead of having a workaround like this?
Btw, the patch actually tries its best to limit the behaviour, e.g it
doesn't do the following set_status() stuff. So I think it won't
trigger the issue you mentioned here?
Thanks
>
> THanks
>
> > Not completely against it, but I
> > wonder if there's a way we can limit the change scope to live migration
> > case only?
> >
> > -Siwei
> >
> > >
> > > Thanks
> > >
> > >
> > >> hw/net/virtio-net.c | 17 ++++++-----------
> > >> 1 file changed, 6 insertions(+), 11 deletions(-)
> > >>
> > >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > >> index dd0d056fde..63a8332cd0 100644
> > >> --- a/hw/net/virtio-net.c
> > >> +++ b/hw/net/virtio-net.c
> > >> @@ -1412,19 +1412,14 @@ static int virtio_net_handle_mq(VirtIONet *n,
> > >> uint8_t cmd,
> > >> return VIRTIO_NET_ERR;
> > >> }
> > >> - /* Avoid changing the number of queue_pairs for vdpa device in
> > >> - * userspace handler. A future fix is needed to handle the mq
> > >> - * change in userspace handler with vhost-vdpa. Let's disable
> > >> - * the mq handling from userspace for now and only allow get
> > >> - * done through the kernel. Ripples may be seen when falling
> > >> - * back to userspace, but without doing it qemu process would
> > >> - * crash on a recursive entry to virtio_net_set_status().
> > >> - */
> > >> + n->curr_queue_pairs = queue_pairs;
> > >> if (nc->peer && nc->peer->info->type ==
> > >> NET_CLIENT_DRIVER_VHOST_VDPA) {
> > >> - return VIRTIO_NET_ERR;
> > >> + /*
> > >> + * Avoid updating the backend for a vdpa device: We're only
> > >> interested
> > >> + * in updating the device model queues.
> > >> + */
> > >> + return VIRTIO_NET_OK;
> > >> }
> > >> -
> > >> - n->curr_queue_pairs = queue_pairs;
> > >> /* stop the backend before changing the number of queue_pairs
> > >> to avoid handling a
> > >> * disabled queue */
> > >> virtio_net_set_status(vdev, vdev->status);
> > >
> >
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/5] virtio-net: Update virtio-net curr_queue_pairs in vdpa backends
2022-08-25 0:38 ` Si-Wei Liu
2022-08-25 2:53 ` Jason Wang
@ 2022-08-25 6:19 ` Eugenio Perez Martin
2022-08-26 4:28 ` Si-Wei Liu
1 sibling, 1 reply; 22+ messages in thread
From: Eugenio Perez Martin @ 2022-08-25 6:19 UTC (permalink / raw
To: Si-Wei Liu
Cc: Jason Wang, qemu-level, Cindy Lu, Eli Cohen, Cornelia Huck,
Laurent Vivier, Liuxiangdong, Paolo Bonzini, Zhu Lingshan,
Harpreet Singh Anand, Stefan Hajnoczi, Parav Pandit, Gautam Dawar,
Stefano Garzarella, Michael S. Tsirkin, Gonglei (Arei)
On Thu, Aug 25, 2022 at 2:38 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 8/23/2022 9:27 PM, Jason Wang wrote:
> >
> > 在 2022/8/20 01:13, Eugenio Pérez 写道:
> >> It was returned as error before. Instead of it, simply update the
> >> corresponding field so qemu can send it in the migration data.
> >>
> >> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >> ---
> >
> >
> > Looks correct.
> >
> > Adding Si Wei for double check.
> Hmmm, I understand why this change is needed for live migration, but
> this would easily cause userspace out of sync with the kernel for other
> use cases, such as link down or userspace fallback due to vdpa ioctl
> error. Yes, these are edge cases.
The link down case is not possible at this moment because that cvq
command does not call virtio_net_handle_ctrl_iov. A similar treatment
than mq would be needed when supported, and the call to
virtio_net_set_status will be avoided.
I'll double check device initialization ioctl failure with
n->curr_queue_pairs > 1 in the destination, but I think we should be
safe.
> Not completely against it, but I
> wonder if there's a way we can limit the change scope to live migration
> case only?
>
The reason to update the device model is to send the curr_queue_pairs
to the destination in a backend agnostic way. To send it otherwise
would limit the live migration possibilities, but sure we can explore
another way.
Thanks!
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/5] virtio-net: Update virtio-net curr_queue_pairs in vdpa backends
2022-08-25 2:53 ` Jason Wang
2022-08-25 3:05 ` Jason Wang
@ 2022-08-26 3:49 ` Si-Wei Liu
1 sibling, 0 replies; 22+ messages in thread
From: Si-Wei Liu @ 2022-08-26 3:49 UTC (permalink / raw
To: Jason Wang
Cc: Eugenio Pérez, qemu-devel, Cindy Lu, Eli Cohen,
Cornelia Huck, Laurent Vivier, Liuxiangdong, Paolo Bonzini,
Zhu Lingshan, Harpreet Singh Anand, Stefan Hajnoczi, Parav Pandit,
Gautam Dawar, Stefano Garzarella, Michael S. Tsirkin,
Gonglei (Arei)
Hi Jason,
On 8/24/2022 7:53 PM, Jason Wang wrote:
> On Thu, Aug 25, 2022 at 8:38 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>
>>
>> On 8/23/2022 9:27 PM, Jason Wang wrote:
>>> 在 2022/8/20 01:13, Eugenio Pérez 写道:
>>>> It was returned as error before. Instead of it, simply update the
>>>> corresponding field so qemu can send it in the migration data.
>>>>
>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>> ---
>>>
>>> Looks correct.
>>>
>>> Adding Si Wei for double check.
>> Hmmm, I understand why this change is needed for live migration, but
>> this would easily cause userspace out of sync with the kernel for other
>> use cases, such as link down or userspace fallback due to vdpa ioctl
>> error. Yes, these are edge cases.
> Considering 7.2 will start, maybe it's time to fix the root cause
> instead of having a workaround like this?
The fix for the immediate cause is not hard, though what is missing from
my WIP series for a full blown fix is something similar to Shadow CVQ
for all general cases than just live migration: QEMU will have to apply
the curr_queue_pairs to the kernel once switched back from the userspace
virtqueues. I think Shadow CVQ won't work if ASID support is missing
from kernel. Do you think if it bother to build another similar
facility, or we reuse Shadow CVQ code to make it work without ASID support?
I have been a bit busy with internal project for the moment, but I hope
I can post my series next week. Here's what I have for the relevant
patches from the WIP series.
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index dd0d056..16edfa3 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -361,16 +361,13 @@ static void
virtio_net_drop_tx_queue_data(VirtIODevice *vdev, VirtQueue *vq)
}
}
-static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t
status)
+static void virtio_net_queue_status(struct VirtIONet *n, uint8_t status)
{
- VirtIONet *n = VIRTIO_NET(vdev);
+ VirtIODevice *vdev = VIRTIO_DEVICE(n);
VirtIONetQueue *q;
int i;
uint8_t queue_status;
- virtio_net_vnet_endian_status(n, status);
- virtio_net_vhost_status(n, status);
-
for (i = 0; i < n->max_queue_pairs; i++) {
NetClientState *ncs = qemu_get_subqueue(n->nic, i);
bool queue_started;
@@ -418,6 +415,15 @@ static void virtio_net_set_status(struct
VirtIODevice *vdev, uint8_t status)
}
}
+static void virtio_net_set_status(struct VirtIODevice *vdev, uint8_t
status)
+{
+ VirtIONet *n = VIRTIO_NET(vdev);
+
+ virtio_net_vnet_endian_status(n, status);
+ virtio_net_vhost_status(n, status);
+ virtio_net_queue_status(n, status);
+}
+
static void virtio_net_set_link_status(NetClientState *nc)
{
VirtIONet *n = qemu_get_nic_opaque(nc);
@@ -1380,7 +1386,6 @@ static int virtio_net_handle_mq(VirtIONet *n,
uint8_t cmd,
{
VirtIODevice *vdev = VIRTIO_DEVICE(n);
uint16_t queue_pairs;
- NetClientState *nc = qemu_get_queue(n->nic);
virtio_net_disable_rss(n);
if (cmd == VIRTIO_NET_CTRL_MQ_HASH_CONFIG) {
@@ -1412,22 +1417,10 @@ static int virtio_net_handle_mq(VirtIONet *n,
uint8_t cmd,
return VIRTIO_NET_ERR;
}
- /* Avoid changing the number of queue_pairs for vdpa device in
- * userspace handler. A future fix is needed to handle the mq
- * change in userspace handler with vhost-vdpa. Let's disable
- * the mq handling from userspace for now and only allow get
- * done through the kernel. Ripples may be seen when falling
- * back to userspace, but without doing it qemu process would
- * crash on a recursive entry to virtio_net_set_status().
- */
- if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {
- return VIRTIO_NET_ERR;
- }
-
n->curr_queue_pairs = queue_pairs;
/* stop the backend before changing the number of queue_pairs to
avoid handling a
* disabled queue */
- virtio_net_set_status(vdev, vdev->status);
+ virtio_net_queue_status(n, vdev->status);
virtio_net_set_queue_pairs(n);
return VIRTIO_NET_OK;
----
Regards,
-Siwei
>
> THanks
>
>> Not completely against it, but I
>> wonder if there's a way we can limit the change scope to live migration
>> case only?
>>
>> -Siwei
>>
>>> Thanks
>>>
>>>
>>>> hw/net/virtio-net.c | 17 ++++++-----------
>>>> 1 file changed, 6 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>> index dd0d056fde..63a8332cd0 100644
>>>> --- a/hw/net/virtio-net.c
>>>> +++ b/hw/net/virtio-net.c
>>>> @@ -1412,19 +1412,14 @@ static int virtio_net_handle_mq(VirtIONet *n,
>>>> uint8_t cmd,
>>>> return VIRTIO_NET_ERR;
>>>> }
>>>> - /* Avoid changing the number of queue_pairs for vdpa device in
>>>> - * userspace handler. A future fix is needed to handle the mq
>>>> - * change in userspace handler with vhost-vdpa. Let's disable
>>>> - * the mq handling from userspace for now and only allow get
>>>> - * done through the kernel. Ripples may be seen when falling
>>>> - * back to userspace, but without doing it qemu process would
>>>> - * crash on a recursive entry to virtio_net_set_status().
>>>> - */
>>>> + n->curr_queue_pairs = queue_pairs;
>>>> if (nc->peer && nc->peer->info->type ==
>>>> NET_CLIENT_DRIVER_VHOST_VDPA) {
>>>> - return VIRTIO_NET_ERR;
>>>> + /*
>>>> + * Avoid updating the backend for a vdpa device: We're only
>>>> interested
>>>> + * in updating the device model queues.
>>>> + */
>>>> + return VIRTIO_NET_OK;
>>>> }
>>>> -
>>>> - n->curr_queue_pairs = queue_pairs;
>>>> /* stop the backend before changing the number of queue_pairs
>>>> to avoid handling a
>>>> * disabled queue */
>>>> virtio_net_set_status(vdev, vdev->status);
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 4/5] virtio-net: Update virtio-net curr_queue_pairs in vdpa backends
2022-08-25 3:05 ` Jason Wang
@ 2022-08-26 3:58 ` Si-Wei Liu
0 siblings, 0 replies; 22+ messages in thread
From: Si-Wei Liu @ 2022-08-26 3:58 UTC (permalink / raw
To: Jason Wang
Cc: Eugenio Pérez, qemu-devel, Cindy Lu, Eli Cohen,
Cornelia Huck, Laurent Vivier, Liuxiangdong, Paolo Bonzini,
Zhu Lingshan, Harpreet Singh Anand, Stefan Hajnoczi, Parav Pandit,
Gautam Dawar, Stefano Garzarella, Michael S. Tsirkin,
Gonglei (Arei)
On 8/24/2022 8:05 PM, Jason Wang wrote:
> On Thu, Aug 25, 2022 at 10:53 AM Jason Wang <jasowang@redhat.com> wrote:
>> On Thu, Aug 25, 2022 at 8:38 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>>
>>>
>>> On 8/23/2022 9:27 PM, Jason Wang wrote:
>>>> 在 2022/8/20 01:13, Eugenio Pérez 写道:
>>>>> It was returned as error before. Instead of it, simply update the
>>>>> corresponding field so qemu can send it in the migration data.
>>>>>
>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>>> ---
>>>>
>>>> Looks correct.
>>>>
>>>> Adding Si Wei for double check.
>>> Hmmm, I understand why this change is needed for live migration, but
>>> this would easily cause userspace out of sync with the kernel for other
>>> use cases, such as link down or userspace fallback due to vdpa ioctl
>>> error. Yes, these are edge cases.
>> Considering 7.2 will start, maybe it's time to fix the root cause
>> instead of having a workaround like this?
> Btw, the patch actually tries its best to limit the behaviour, e.g it
> doesn't do the following set_status() stuff. So I think it won't
> trigger the issue you mentioned here?
Well, we can claim we don't support the link down+up case while changing
queue numbers in between. On the other hand, the error recovery from
fallback userspace is another story, which would need more attention and
care on the error path. Yes, if see it from that perspective the change
is fine. For completeness, please refer to the patch in the other email.
-Siwei
>
> Thanks
>
>> THanks
>>
>>> Not completely against it, but I
>>> wonder if there's a way we can limit the change scope to live migration
>>> case only?
>>>
>>> -Siwei
>>>
>>>> Thanks
>>>>
>>>>
>>>>> hw/net/virtio-net.c | 17 ++++++-----------
>>>>> 1 file changed, 6 insertions(+), 11 deletions(-)
>>>>>
>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>>> index dd0d056fde..63a8332cd0 100644
>>>>> --- a/hw/net/virtio-net.c
>>>>> +++ b/hw/net/virtio-net.c
>>>>> @@ -1412,19 +1412,14 @@ static int virtio_net_handle_mq(VirtIONet *n,
>>>>> uint8_t cmd,
>>>>> return VIRTIO_NET_ERR;
>>>>> }
>>>>> - /* Avoid changing the number of queue_pairs for vdpa device in
>>>>> - * userspace handler. A future fix is needed to handle the mq
>>>>> - * change in userspace handler with vhost-vdpa. Let's disable
>>>>> - * the mq handling from userspace for now and only allow get
>>>>> - * done through the kernel. Ripples may be seen when falling
>>>>> - * back to userspace, but without doing it qemu process would
>>>>> - * crash on a recursive entry to virtio_net_set_status().
>>>>> - */
>>>>> + n->curr_queue_pairs = queue_pairs;
>>>>> if (nc->peer && nc->peer->info->type ==
>>>>> NET_CLIENT_DRIVER_VHOST_VDPA) {
>>>>> - return VIRTIO_NET_ERR;
>>>>> + /*
>>>>> + * Avoid updating the backend for a vdpa device: We're only
>>>>> interested
>>>>> + * in updating the device model queues.
>>>>> + */
>>>>> + return VIRTIO_NET_OK;
>>>>> }
>>>>> -
>>>>> - n->curr_queue_pairs = queue_pairs;
>>>>> /* stop the backend before changing the number of queue_pairs
>>>>> to avoid handling a
>>>>> * disabled queue */
>>>>> virtio_net_set_status(vdev, vdev->status);
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/5] virtio-net: Update virtio-net curr_queue_pairs in vdpa backends
2022-08-25 6:19 ` Eugenio Perez Martin
@ 2022-08-26 4:28 ` Si-Wei Liu
2022-08-26 8:22 ` Eugenio Perez Martin
0 siblings, 1 reply; 22+ messages in thread
From: Si-Wei Liu @ 2022-08-26 4:28 UTC (permalink / raw
To: Eugenio Perez Martin
Cc: Jason Wang, qemu-level, Cindy Lu, Eli Cohen, Cornelia Huck,
Laurent Vivier, Liuxiangdong, Paolo Bonzini, Zhu Lingshan,
Harpreet Singh Anand, Stefan Hajnoczi, Parav Pandit, Gautam Dawar,
Stefano Garzarella, Michael S. Tsirkin, Gonglei (Arei)
On 8/24/2022 11:19 PM, Eugenio Perez Martin wrote:
> On Thu, Aug 25, 2022 at 2:38 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>
>>
>> On 8/23/2022 9:27 PM, Jason Wang wrote:
>>> 在 2022/8/20 01:13, Eugenio Pérez 写道:
>>>> It was returned as error before. Instead of it, simply update the
>>>> corresponding field so qemu can send it in the migration data.
>>>>
>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>> ---
>>>
>>> Looks correct.
>>>
>>> Adding Si Wei for double check.
>> Hmmm, I understand why this change is needed for live migration, but
>> this would easily cause userspace out of sync with the kernel for other
>> use cases, such as link down or userspace fallback due to vdpa ioctl
>> error. Yes, these are edge cases.
> The link down case is not possible at this moment because that cvq
> command does not call virtio_net_handle_ctrl_iov.
Right. Though shadow cvq would need to rely on extra ASID support from
kernel. For the case without shadow cvq we still need to look for an
alternative mechanism.
> A similar treatment
> than mq would be needed when supported, and the call to
> virtio_net_set_status will be avoided.
So, maybe the seemingly "right" fix for the moment is to prohibit manual
set_link at all (for vDPA only)? In longer term we'd need to come up
with appropriate support for applying mq config regardless of asid or
shadow cvq support.
>
> I'll double check device initialization ioctl failure with
> n->curr_queue_pairs > 1 in the destination, but I think we should be
> safe.
>
>> Not completely against it, but I
>> wonder if there's a way we can limit the change scope to live migration
>> case only?
>>
> The reason to update the device model is to send the curr_queue_pairs
> to the destination in a backend agnostic way. To send it otherwise
> would limit the live migration possibilities, but sure we can explore
> another way.
A hacky workaround that came off the top of my head was to allow sending
curr_queue_pairs for the !vm_running case for vdpa. It doesn't look it
would affect other backend I think. But I agree with Jason, this doesn't
look decent so I give up on this idea. Hence for this patch,
Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>
>
> Thanks!
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 4/5] virtio-net: Update virtio-net curr_queue_pairs in vdpa backends
2022-08-26 4:28 ` Si-Wei Liu
@ 2022-08-26 8:22 ` Eugenio Perez Martin
0 siblings, 0 replies; 22+ messages in thread
From: Eugenio Perez Martin @ 2022-08-26 8:22 UTC (permalink / raw
To: Si-Wei Liu
Cc: Jason Wang, qemu-level, Cindy Lu, Eli Cohen, Cornelia Huck,
Laurent Vivier, Liuxiangdong, Paolo Bonzini, Zhu Lingshan,
Harpreet Singh Anand, Stefan Hajnoczi, Parav Pandit, Gautam Dawar,
Stefano Garzarella, Michael S. Tsirkin, Gonglei (Arei)
On Fri, Aug 26, 2022 at 6:29 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 8/24/2022 11:19 PM, Eugenio Perez Martin wrote:
> > On Thu, Aug 25, 2022 at 2:38 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
> >>
> >>
> >> On 8/23/2022 9:27 PM, Jason Wang wrote:
> >>> 在 2022/8/20 01:13, Eugenio Pérez 写道:
> >>>> It was returned as error before. Instead of it, simply update the
> >>>> corresponding field so qemu can send it in the migration data.
> >>>>
> >>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >>>> ---
> >>>
> >>> Looks correct.
> >>>
> >>> Adding Si Wei for double check.
> >> Hmmm, I understand why this change is needed for live migration, but
> >> this would easily cause userspace out of sync with the kernel for other
> >> use cases, such as link down or userspace fallback due to vdpa ioctl
> >> error. Yes, these are edge cases.
> > The link down case is not possible at this moment because that cvq
> > command does not call virtio_net_handle_ctrl_iov.
> Right. Though shadow cvq would need to rely on extra ASID support from
> kernel. For the case without shadow cvq we still need to look for an
> alternative mechanism.
>
> > A similar treatment
> > than mq would be needed when supported, and the call to
> > virtio_net_set_status will be avoided.
> So, maybe the seemingly "right" fix for the moment is to prohibit manual
> set_link at all (for vDPA only)?
We can apply a similar solution and just save the link status, without
stopping any vqp backend. The code can be more elegant than checking
if the backend is vhost-vdpa of course, but what is the problem with
doing it that way?
> In longer term we'd need to come up
> with appropriate support for applying mq config regardless of asid or
> shadow cvq support.
>
What do you mean by applying "mq config"? To the virtio-net device
model in qemu? Is there any use case to apply it to the model outside
of live migration?
On the other hand, the current approach is not using ASID at all, it
will be added on top. Do you mean that it is needed for data
passthrough & CVQ shadow, isn't it?
> >
> > I'll double check device initialization ioctl failure with
> > n->curr_queue_pairs > 1 in the destination, but I think we should be
> > safe.
> >
> >> Not completely against it, but I
> >> wonder if there's a way we can limit the change scope to live migration
> >> case only?
> >>
> > The reason to update the device model is to send the curr_queue_pairs
> > to the destination in a backend agnostic way. To send it otherwise
> > would limit the live migration possibilities, but sure we can explore
> > another way.
> A hacky workaround that came off the top of my head was to allow sending
> curr_queue_pairs for the !vm_running case for vdpa. It doesn't look it
> would affect other backend I think. But I agree with Jason, this doesn't
> look decent so I give up on this idea. Hence for this patch,
>
I still don't get the problem. Also, the guest would need to reset the
device anyway, so that information will be lost, isn't it?
Thanks!
> Acked-by: Si-Wei Liu <si-wei.liu@oracle.com>
>
> >
> > Thanks!
> >
>
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2022-08-26 8:26 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-08-19 17:13 [PATCH 0/5] Vhost-vdpa Shadow Virtqueue multiqueue support Eugenio Pérez
2022-08-19 17:13 ` [PATCH 1/5] vdpa: extract vhost_vdpa_net_load_mac from vhost_vdpa_net_load Eugenio Pérez
2022-08-24 4:18 ` Jason Wang
2022-08-19 17:13 ` [PATCH 2/5] vdpa: Add vhost_vdpa_net_load_mq Eugenio Pérez
2022-08-24 4:23 ` Jason Wang
2022-08-24 7:46 ` Eugenio Perez Martin
2022-08-24 8:52 ` Jason Wang
2022-08-24 9:06 ` Eugenio Perez Martin
2022-08-24 9:07 ` Jason Wang
2022-08-24 9:18 ` Eugenio Perez Martin
2022-08-19 17:13 ` [PATCH 3/5] vdpa: validate MQ CVQ commands Eugenio Pérez
2022-08-19 17:13 ` [PATCH 4/5] virtio-net: Update virtio-net curr_queue_pairs in vdpa backends Eugenio Pérez
2022-08-24 4:27 ` Jason Wang
2022-08-25 0:38 ` Si-Wei Liu
2022-08-25 2:53 ` Jason Wang
2022-08-25 3:05 ` Jason Wang
2022-08-26 3:58 ` Si-Wei Liu
2022-08-26 3:49 ` Si-Wei Liu
2022-08-25 6:19 ` Eugenio Perez Martin
2022-08-26 4:28 ` Si-Wei Liu
2022-08-26 8:22 ` Eugenio Perez Martin
2022-08-19 17:13 ` [PATCH 5/5] vdpa: Allow MQ feture in SVQ Eugenio Pérez
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.