[PATCH v2] virtio-net: Fix the interpretation of max_tx_vq

[PATCH v2] virtio-net: Fix the interpretation of max_tx_vq

[Akihiko Odaki]

virtio-net uses the max_tx_vq field of struct virtio_net_rss_config to
determine the number of queue pairs and emits an error message saying
"Can't get queue_pairs". However, the field tells only about tx.

Examine unclassified_queue and indirection_table to determine the number
of queues required for rx, and correct the name of field in the error
message, clarifying its correct semantics.

Fixes: 590790297c0d ("virtio-net: implement RSS configuration command")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
Changes in v2:
- Handled unclassified_queue too.
- Added a Fixes: tag.
- Link to v1: https://lore.kernel.org/qemu-devel/20250321-vq-v1-1-6d6d285e5cbc@daynix.com
---
 hw/net/virtio-net.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index de87cfadffe1..afc6b82f13c9 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -1450,23 +1450,29 @@ static uint16_t virtio_net_handle_rss(VirtIONet *n,
         err_value = (uint32_t)s;
         goto error;
     }
-    for (i = 0; i < n->rss_data.indirections_len; ++i) {
-        uint16_t val = n->rss_data.indirections_table[i];
-        n->rss_data.indirections_table[i] = virtio_lduw_p(vdev, &val);
-    }
     offset += size_get;
     size_get = sizeof(temp);
     s = iov_to_buf(iov, iov_cnt, offset, &temp, size_get);
     if (s != size_get) {
-        err_msg = "Can't get queue_pairs";
+        err_msg = "Can't get max_tx_vq";
         err_value = (uint32_t)s;
         goto error;
     }
-    queue_pairs = do_rss ? virtio_lduw_p(vdev, &temp.us) : n->curr_queue_pairs;
-    if (queue_pairs == 0 || queue_pairs > n->max_queue_pairs) {
-        err_msg = "Invalid number of queue_pairs";
-        err_value = queue_pairs;
-        goto error;
+    if (do_rss) {
+        queue_pairs = MAX(virtio_lduw_p(vdev, &temp.us),
+                          n->rss_data.default_queue);
+        for (i = 0; i < n->rss_data.indirections_len; ++i) {
+            uint16_t val = n->rss_data.indirections_table[i];
+            n->rss_data.indirections_table[i] = virtio_lduw_p(vdev, &val);
+            queue_pairs = MAX(queue_pairs, n->rss_data.indirections_table[i]);
+        }
+        if (queue_pairs == 0 || queue_pairs > n->max_queue_pairs) {
+            err_msg = "Invalid number of queue_pairs";
+            err_value = queue_pairs;
+            goto error;
+        }
+    } else {
+        queue_pairs = n->curr_queue_pairs;
     }
     if (temp.b > VIRTIO_NET_RSS_MAX_KEY_SIZE) {
         err_msg = "Invalid key size";

---
base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32
change-id: 20250321-vq-87aff4f531bf

Best regards,
-- 
Akihiko Odaki <akihiko.odaki@daynix.com>

Re: [PATCH v2] virtio-net: Fix the interpretation of max_tx_vq

[Lei Yang]

QE tested this patch with virtio-net regression tests, everything works fine.

Tested-by: Lei Yang <leiyang@redhat.com>

On Sat, Mar 22, 2025 at 2:48 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> virtio-net uses the max_tx_vq field of struct virtio_net_rss_config to
> determine the number of queue pairs and emits an error message saying
> "Can't get queue_pairs". However, the field tells only about tx.
>
> Examine unclassified_queue and indirection_table to determine the number
> of queues required for rx, and correct the name of field in the error
> message, clarifying its correct semantics.
>
> Fixes: 590790297c0d ("virtio-net: implement RSS configuration command")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> Changes in v2:
> - Handled unclassified_queue too.
> - Added a Fixes: tag.
> - Link to v1: https://lore.kernel.org/qemu-devel/20250321-vq-v1-1-6d6d285e5cbc@daynix.com
> ---
>  hw/net/virtio-net.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index de87cfadffe1..afc6b82f13c9 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1450,23 +1450,29 @@ static uint16_t virtio_net_handle_rss(VirtIONet *n,
>          err_value = (uint32_t)s;
>          goto error;
>      }
> -    for (i = 0; i < n->rss_data.indirections_len; ++i) {
> -        uint16_t val = n->rss_data.indirections_table[i];
> -        n->rss_data.indirections_table[i] = virtio_lduw_p(vdev, &val);
> -    }
>      offset += size_get;
>      size_get = sizeof(temp);
>      s = iov_to_buf(iov, iov_cnt, offset, &temp, size_get);
>      if (s != size_get) {
> -        err_msg = "Can't get queue_pairs";
> +        err_msg = "Can't get max_tx_vq";
>          err_value = (uint32_t)s;
>          goto error;
>      }
> -    queue_pairs = do_rss ? virtio_lduw_p(vdev, &temp.us) : n->curr_queue_pairs;
> -    if (queue_pairs == 0 || queue_pairs > n->max_queue_pairs) {
> -        err_msg = "Invalid number of queue_pairs";
> -        err_value = queue_pairs;
> -        goto error;
> +    if (do_rss) {
> +        queue_pairs = MAX(virtio_lduw_p(vdev, &temp.us),
> +                          n->rss_data.default_queue);
> +        for (i = 0; i < n->rss_data.indirections_len; ++i) {
> +            uint16_t val = n->rss_data.indirections_table[i];
> +            n->rss_data.indirections_table[i] = virtio_lduw_p(vdev, &val);
> +            queue_pairs = MAX(queue_pairs, n->rss_data.indirections_table[i]);
> +        }
> +        if (queue_pairs == 0 || queue_pairs > n->max_queue_pairs) {
> +            err_msg = "Invalid number of queue_pairs";
> +            err_value = queue_pairs;
> +            goto error;
> +        }
> +    } else {
> +        queue_pairs = n->curr_queue_pairs;
>      }
>      if (temp.b > VIRTIO_NET_RSS_MAX_KEY_SIZE) {
>          err_msg = "Invalid key size";
>
> ---
> base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32
> change-id: 20250321-vq-87aff4f531bf
>
> Best regards,
> --
> Akihiko Odaki <akihiko.odaki@daynix.com>
>
>

Re: [PATCH v2] virtio-net: Fix the interpretation of max_tx_vq

[Michael S. Tsirkin]

On Sat, Mar 22, 2025 at 03:47:17PM +0900, Akihiko Odaki wrote:
> virtio-net uses the max_tx_vq field of struct virtio_net_rss_config to
> determine the number of queue pairs and emits an error message saying
> "Can't get queue_pairs". However, the field tells only about tx.
> 
> Examine unclassified_queue and indirection_table to determine the number
> of queues required for rx, and correct the name of field in the error
> message, clarifying its correct semantics.
> 
> Fixes: 590790297c0d ("virtio-net: implement RSS configuration command")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---


Breaks CI:


https://gitlab.com/mstredhat/qemu/-/jobs/10199709161


../hw/net/virtio-net.c:1463:48: error: taking address of packed member 'us' of class or structure 'struct (unnamed at ../hw/net/virtio-net.c:1392:5)' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
 1463 |         queue_pairs = MAX(virtio_lduw_p(vdev, &temp.us),
      |                                                ^~~~~~~
../include/qemu/osdep.h:419:19: note: expanded from macro 'MAX'
  419 |     MAX_INTERNAL((a), (b), MAKE_IDENTIFIER(_a), MAKE_IDENTIFIER(_b))
      |                   ^
../include/qemu/osdep.h:414:21: note: expanded from macro 'MAX_INTERNAL'
  414 |         typeof(1 ? (a) : (b)) _a = (a), _b = (b);       \
      |                     ^
1 error generated.



> Changes in v2:
> - Handled unclassified_queue too.
> - Added a Fixes: tag.
> - Link to v1: https://lore.kernel.org/qemu-devel/20250321-vq-v1-1-6d6d285e5cbc@daynix.com
> ---
>  hw/net/virtio-net.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index de87cfadffe1..afc6b82f13c9 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1450,23 +1450,29 @@ static uint16_t virtio_net_handle_rss(VirtIONet *n,
>          err_value = (uint32_t)s;
>          goto error;
>      }
> -    for (i = 0; i < n->rss_data.indirections_len; ++i) {
> -        uint16_t val = n->rss_data.indirections_table[i];
> -        n->rss_data.indirections_table[i] = virtio_lduw_p(vdev, &val);
> -    }
>      offset += size_get;
>      size_get = sizeof(temp);
>      s = iov_to_buf(iov, iov_cnt, offset, &temp, size_get);
>      if (s != size_get) {
> -        err_msg = "Can't get queue_pairs";
> +        err_msg = "Can't get max_tx_vq";
>          err_value = (uint32_t)s;
>          goto error;
>      }
> -    queue_pairs = do_rss ? virtio_lduw_p(vdev, &temp.us) : n->curr_queue_pairs;
> -    if (queue_pairs == 0 || queue_pairs > n->max_queue_pairs) {
> -        err_msg = "Invalid number of queue_pairs";
> -        err_value = queue_pairs;
> -        goto error;
> +    if (do_rss) {
> +        queue_pairs = MAX(virtio_lduw_p(vdev, &temp.us),
> +                          n->rss_data.default_queue);
> +        for (i = 0; i < n->rss_data.indirections_len; ++i) {
> +            uint16_t val = n->rss_data.indirections_table[i];
> +            n->rss_data.indirections_table[i] = virtio_lduw_p(vdev, &val);
> +            queue_pairs = MAX(queue_pairs, n->rss_data.indirections_table[i]);
> +        }
> +        if (queue_pairs == 0 || queue_pairs > n->max_queue_pairs) {
> +            err_msg = "Invalid number of queue_pairs";
> +            err_value = queue_pairs;
> +            goto error;
> +        }
> +    } else {
> +        queue_pairs = n->curr_queue_pairs;
>      }
>      if (temp.b > VIRTIO_NET_RSS_MAX_KEY_SIZE) {
>          err_msg = "Invalid key size";
> 
> ---
> base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32
> change-id: 20250321-vq-87aff4f531bf
> 
> Best regards,
> -- 
> Akihiko Odaki <akihiko.odaki@daynix.com>