[PATCH v3 0/2] Vhost-vdpa Shadow Virtqueue Hash calculation Support

[PATCH v3 0/2] Vhost-vdpa Shadow Virtqueue Hash calculation Support

[Hawkins Jiawei]

This series enables shadowed CVQ to intercept
VIRTIO_NET_CTRL_MQ_HASH_CONFIG command through shadowed CVQ,
update the virtio NIC device model so qemu send it in a
migration, and the restore of that Hash calculation state
in the destination.

ChangeLog
=========
v3:
  - remove the `do_rss` argument in vhost_vdpa_net_load_rss()
  - zero reserved fields in "cfg" manually instead of using memset()
to prevent compiler "array-bounds" warning

v2: https://lore.kernel.org/all/cover.1693297766.git.yin31149@gmail.com/
  - resolve conflict with updated patch
"vdpa: Send all CVQ state load commands in parallel", move the
`table` declaration at the beginning of the vhost_vdpa_net_load_rss()
in patch
"vdpa: Restore hash calculation state"

RFC: https://lore.kernel.org/all/cover.1691762906.git.yin31149@gmail.com/#t

TestStep
========
1. test the migration using vp-vdpa device
  - For L0 guest, boot QEMU with two virtio-net-pci net device with
`ctrl_vq`, `mq`, `hash` features on, command line like:
	-netdev tap,...
	-device virtio-net-pci,disable-legacy=on,disable-modern=off,
iommu_platform=on,mq=on,ctrl_vq=on,hash=on,guest_announce=off,
indirect_desc=off,queue_reset=off,guest_uso4=off,guest_uso6=off,
host_uso=off,...

  - For L1 guest, apply the relative patch series and compile the
source code, start QEMU with two vdpa device with svq mode on,
enable the `ctrl_vq`, `mq`, `hash` features on, command line like:
      -netdev type=vhost-vdpa,x-svq=true,...
      -device virtio-net-pci,mq=on,guest_announce=off,ctrl_vq=on,
hash=on,...

  - For L2 source guest, run the following bash command:
```bash
#!/bin/sh

ethtool -K eth0 rxhash on
```
  - Gdb attach the destination VM and break at the
vhost_vdpa_net_load_rss()

  - Execute the live migration in L2 source monitor

  - Result
    * with this series, gdb can hit the breakpoint and continue
the executing without triggering any error or warning.

Hawkins Jiawei (2):
  vdpa: Restore hash calculation state
  vdpa: Allow VIRTIO_NET_F_HASH_REPORT in SVQ

 net/vhost-vdpa.c | 90 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 90 insertions(+)

-- 
2.25.1

[PATCH v3 1/2] vdpa: Restore hash calculation state

[Hawkins Jiawei]

This patch introduces vhost_vdpa_net_load_rss() to restore
the hash calculation state at device's startup.

Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
---
v3:
  - remove the `do_rss` argument in vhost_vdpa_net_load_rss()
  - zero reserved fields in "cfg" manually instead of using memset()
to prevent compiler "array-bounds" warning

v2: https://lore.kernel.org/all/f5ffad10699001107022851e0560cb394039d6b0.1693297766.git.yin31149@gmail.com/
  - resolve conflict with updated patch
"vdpa: Send all CVQ state load commands in parallel"
  - move the `table` declaration at the beginning of the
vhost_vdpa_net_load_rss()

RFC: https://lore.kernel.org/all/a54ca70b12ebe2f3c391864e41241697ab1aba30.1691762906.git.yin31149@gmail.com/

 net/vhost-vdpa.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 89 insertions(+)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 4b7c3b81b8..2e4bad65b4 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -817,6 +817,86 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n,
     return 0;
 }
 
+static int vhost_vdpa_net_load_rss(VhostVDPAState *s, const VirtIONet *n,
+                                   struct iovec *out_cursor,
+                                   struct iovec *in_cursor)
+{
+    struct virtio_net_rss_config cfg;
+    ssize_t r;
+    g_autofree uint16_t *table = NULL;
+
+    /*
+     * According to VirtIO standard, "Initially the device has all hash
+     * types disabled and reports only VIRTIO_NET_HASH_REPORT_NONE.".
+     *
+     * Therefore, there is no need to send this CVQ command if the
+     * driver disable the all hash types, which aligns with
+     * the device's defaults.
+     *
+     * Note that the device's defaults can mismatch the driver's
+     * configuration only at live migration.
+     */
+    if (!n->rss_data.enabled ||
+        n->rss_data.hash_types == VIRTIO_NET_HASH_REPORT_NONE) {
+        return 0;
+    }
+
+    table = g_malloc_n(n->rss_data.indirections_len,
+                       sizeof(n->rss_data.indirections_table[0]));
+    cfg.hash_types = cpu_to_le32(n->rss_data.hash_types);
+
+    /*
+     * According to VirtIO standard, "Field reserved MUST contain zeroes.
+     * It is defined to make the structure to match the layout of
+     * virtio_net_rss_config structure, defined in 5.1.6.5.7.".
+     *
+     * Therefore, we need to zero the fields in struct virtio_net_rss_config,
+     * which corresponds the `reserved` field in
+     * struct virtio_net_hash_config.
+     */
+    cfg.indirection_table_mask = 0;
+    cfg.unclassified_queue = 0;
+    table[0] = 0; /* the actual indirection table for cfg */
+    cfg.max_tx_vq = 0;
+
+    /*
+     * Consider that virtio_net_handle_rss() currently does not restore the
+     * hash key length parsed from the CVQ command sent from the guest into
+     * n->rss_data and uses the maximum key length in other code, so we also
+     * employthe the maxium key length here.
+     */
+    cfg.hash_key_length = sizeof(n->rss_data.key);
+
+    const struct iovec data[] = {
+        {
+            .iov_base = &cfg,
+            .iov_len = offsetof(struct virtio_net_rss_config,
+                                indirection_table),
+        }, {
+            .iov_base = table,
+            .iov_len = n->rss_data.indirections_len *
+                       sizeof(n->rss_data.indirections_table[0]),
+        }, {
+            .iov_base = &cfg.max_tx_vq,
+            .iov_len = offsetof(struct virtio_net_rss_config, hash_key_data) -
+                       offsetof(struct virtio_net_rss_config, max_tx_vq),
+        }, {
+            .iov_base = (void *)n->rss_data.key,
+            .iov_len = sizeof(n->rss_data.key),
+        }
+    };
+
+    r = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
+                                VIRTIO_NET_CTRL_MQ,
+                                VIRTIO_NET_CTRL_MQ_HASH_CONFIG,
+                                data, ARRAY_SIZE(data));
+    if (unlikely(r < 0)) {
+        return r;
+    }
+
+    return 0;
+}
+
 static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
                                   const VirtIONet *n,
                                   struct iovec *out_cursor,
@@ -842,6 +922,15 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
         return r;
     }
 
+    if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_HASH_REPORT)) {
+        return 0;
+    }
+
+    r = vhost_vdpa_net_load_rss(s, n, out_cursor, in_cursor);
+    if (unlikely(r < 0)) {
+        return r;
+    }
+
     return 0;
 }
 
-- 
2.25.1

[PATCH v3 2/2] vdpa: Allow VIRTIO_NET_F_HASH_REPORT in SVQ

[Hawkins Jiawei]

Enable SVQ with VIRTIO_NET_F_HASH_REPORT feature.

Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
---
v3: no code changes

v2: https://lore.kernel.org/all/a67d4abc2c8c5c7636addc729daa5432fa8193bd.1693297766.git.yin31149@gmail.com/

 net/vhost-vdpa.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 2e4bad65b4..4c65c53fd2 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -121,6 +121,7 @@ static const uint64_t vdpa_svq_device_features =
     BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR) |
     /* VHOST_F_LOG_ALL is exposed by SVQ */
     BIT_ULL(VHOST_F_LOG_ALL) |
+    BIT_ULL(VIRTIO_NET_F_HASH_REPORT) |
     BIT_ULL(VIRTIO_NET_F_RSC_EXT) |
     BIT_ULL(VIRTIO_NET_F_STANDBY) |
     BIT_ULL(VIRTIO_NET_F_SPEED_DUPLEX);
-- 
2.25.1

Re: [PATCH v3 1/2] vdpa: Restore hash calculation state

[Michael S. Tsirkin]

On Sun, Oct 22, 2023 at 10:00:48AM +0800, Hawkins Jiawei wrote:
> This patch introduces vhost_vdpa_net_load_rss() to restore
> the hash calculation state at device's startup.
> 
> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
> ---
> v3:
>   - remove the `do_rss` argument in vhost_vdpa_net_load_rss()
>   - zero reserved fields in "cfg" manually instead of using memset()
> to prevent compiler "array-bounds" warning
> 
> v2: https://lore.kernel.org/all/f5ffad10699001107022851e0560cb394039d6b0.1693297766.git.yin31149@gmail.com/
>   - resolve conflict with updated patch
> "vdpa: Send all CVQ state load commands in parallel"
>   - move the `table` declaration at the beginning of the
> vhost_vdpa_net_load_rss()
> 
> RFC: https://lore.kernel.org/all/a54ca70b12ebe2f3c391864e41241697ab1aba30.1691762906.git.yin31149@gmail.com/
> 
>  net/vhost-vdpa.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
> 
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 4b7c3b81b8..2e4bad65b4 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -817,6 +817,86 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n,
>      return 0;
>  }
>  
> +static int vhost_vdpa_net_load_rss(VhostVDPAState *s, const VirtIONet *n,
> +                                   struct iovec *out_cursor,
> +                                   struct iovec *in_cursor)
> +{
> +    struct virtio_net_rss_config cfg;
> +    ssize_t r;
> +    g_autofree uint16_t *table = NULL;
> +
> +    /*
> +     * According to VirtIO standard, "Initially the device has all hash
> +     * types disabled and reports only VIRTIO_NET_HASH_REPORT_NONE.".
> +     *
> +     * Therefore, there is no need to send this CVQ command if the
> +     * driver disable the all hash types, which aligns with

disables? or disabled

> +     * the device's defaults.
> +     *
> +     * Note that the device's defaults can mismatch the driver's
> +     * configuration only at live migration.
> +     */
> +    if (!n->rss_data.enabled ||
> +        n->rss_data.hash_types == VIRTIO_NET_HASH_REPORT_NONE) {
> +        return 0;
> +    }
> +
> +    table = g_malloc_n(n->rss_data.indirections_len,
> +                       sizeof(n->rss_data.indirections_table[0]));
> +    cfg.hash_types = cpu_to_le32(n->rss_data.hash_types);
> +
> +    /*
> +     * According to VirtIO standard, "Field reserved MUST contain zeroes.
> +     * It is defined to make the structure to match the layout of
> +     * virtio_net_rss_config structure, defined in 5.1.6.5.7.".
> +     *
> +     * Therefore, we need to zero the fields in struct virtio_net_rss_config,
> +     * which corresponds the `reserved` field in

corresponds to

> +     * struct virtio_net_hash_config.
> +     */
> +    cfg.indirection_table_mask = 0;
> +    cfg.unclassified_queue = 0;
> +    table[0] = 0; /* the actual indirection table for cfg */
> +    cfg.max_tx_vq = 0;

Wouldn't it be easier to just do cfg = {} where it is defined?

> +
> +    /*
> +     * Consider that virtio_net_handle_rss() currently does not restore the
> +     * hash key length parsed from the CVQ command sent from the guest into
> +     * n->rss_data and uses the maximum key length in other code, so we also
> +     * employthe the maxium key length here.

two typos

> +     */
> +    cfg.hash_key_length = sizeof(n->rss_data.key);
> +
> +    const struct iovec data[] = {
> +        {
> +            .iov_base = &cfg,
> +            .iov_len = offsetof(struct virtio_net_rss_config,
> +                                indirection_table),
> +        }, {
> +            .iov_base = table,
> +            .iov_len = n->rss_data.indirections_len *
> +                       sizeof(n->rss_data.indirections_table[0]),
> +        }, {
> +            .iov_base = &cfg.max_tx_vq,
> +            .iov_len = offsetof(struct virtio_net_rss_config, hash_key_data) -
> +                       offsetof(struct virtio_net_rss_config, max_tx_vq),
> +        }, {
> +            .iov_base = (void *)n->rss_data.key,

cast to void * should not be needed here.

> +            .iov_len = sizeof(n->rss_data.key),
> +        }
> +    };
> +
> +    r = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
> +                                VIRTIO_NET_CTRL_MQ,
> +                                VIRTIO_NET_CTRL_MQ_HASH_CONFIG,
> +                                data, ARRAY_SIZE(data));
> +    if (unlikely(r < 0)) {
> +        return r;
> +    }
> +
> +    return 0;
> +}
> +
>  static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
>                                    const VirtIONet *n,
>                                    struct iovec *out_cursor,
> @@ -842,6 +922,15 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
>          return r;
>      }
>  
> +    if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_HASH_REPORT)) {
> +        return 0;
> +    }
> +
> +    r = vhost_vdpa_net_load_rss(s, n, out_cursor, in_cursor);
> +    if (unlikely(r < 0)) {
> +        return r;
> +    }
> +
>      return 0;
>  }
>  
> -- 
> 2.25.1

Re: [PATCH v3 1/2] vdpa: Restore hash calculation state

[Hawkins Jiawei]

在 2023/10/22 18:00, Michael S. Tsirkin 写道:
> On Sun, Oct 22, 2023 at 10:00:48AM +0800, Hawkins Jiawei wrote:
>> This patch introduces vhost_vdpa_net_load_rss() to restore
>> the hash calculation state at device's startup.
>>
>> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
>> ---
>> v3:
>>    - remove the `do_rss` argument in vhost_vdpa_net_load_rss()
>>    - zero reserved fields in "cfg" manually instead of using memset()
>> to prevent compiler "array-bounds" warning
>>
>> v2: https://lore.kernel.org/all/f5ffad10699001107022851e0560cb394039d6b0.1693297766.git.yin31149@gmail.com/
>>    - resolve conflict with updated patch
>> "vdpa: Send all CVQ state load commands in parallel"
>>    - move the `table` declaration at the beginning of the
>> vhost_vdpa_net_load_rss()
>>
>> RFC: https://lore.kernel.org/all/a54ca70b12ebe2f3c391864e41241697ab1aba30.1691762906.git.yin31149@gmail.com/
>>
>>   net/vhost-vdpa.c | 89 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 89 insertions(+)
>>
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index 4b7c3b81b8..2e4bad65b4 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -817,6 +817,86 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n,
>>       return 0;
>>   }
>>
>> +static int vhost_vdpa_net_load_rss(VhostVDPAState *s, const VirtIONet *n,
>> +                                   struct iovec *out_cursor,
>> +                                   struct iovec *in_cursor)
>> +{
>> +    struct virtio_net_rss_config cfg;
>> +    ssize_t r;
>> +    g_autofree uint16_t *table = NULL;
>> +
>> +    /*
>> +     * According to VirtIO standard, "Initially the device has all hash
>> +     * types disabled and reports only VIRTIO_NET_HASH_REPORT_NONE.".
>> +     *
>> +     * Therefore, there is no need to send this CVQ command if the
>> +     * driver disable the all hash types, which aligns with
>
> disables? or disabled

It should be "disables".
I will correct this in the v4 patch.

>
>> +     * the device's defaults.
>> +     *
>> +     * Note that the device's defaults can mismatch the driver's
>> +     * configuration only at live migration.
>> +     */
>> +    if (!n->rss_data.enabled ||
>> +        n->rss_data.hash_types == VIRTIO_NET_HASH_REPORT_NONE) {
>> +        return 0;
>> +    }
>> +
>> +    table = g_malloc_n(n->rss_data.indirections_len,
>> +                       sizeof(n->rss_data.indirections_table[0]));
>> +    cfg.hash_types = cpu_to_le32(n->rss_data.hash_types);
>> +
>> +    /*
>> +     * According to VirtIO standard, "Field reserved MUST contain zeroes.
>> +     * It is defined to make the structure to match the layout of
>> +     * virtio_net_rss_config structure, defined in 5.1.6.5.7.".
>> +     *
>> +     * Therefore, we need to zero the fields in struct virtio_net_rss_config,
>> +     * which corresponds the `reserved` field in
>
> corresponds to

I will correct this in the v4 patch.

>
>> +     * struct virtio_net_hash_config.
>> +     */
>> +    cfg.indirection_table_mask = 0;
>> +    cfg.unclassified_queue = 0;
>> +    table[0] = 0; /* the actual indirection table for cfg */
>> +    cfg.max_tx_vq = 0;
>
> Wouldn't it be easier to just do cfg = {} where it is defined?

Normally, it should follow your pattern, but there are two reasons why
I'm doing it in a different way here.

Firstly, in the subsequent patchset, both hash calculation and rss will
reuse vhost_vdpa_net_load_rss() to restore their state. Given the
similarity of their CVQ commands, if we only explicitly handle the
fields assignment for rss case, while placing the hash calculation field
assignment at the definition site, it would disperse the logic within
the function, making it look odd.

Secondly, to ensure compatibility for rss case, we cannot use the
'indirection_table' field in the cfg. Instead, we need to allocate a
separate 'table' variable here. Even if we initialize the other fields
of the hash calculation case at the definition site, we still need to
manually set 'table' to 0 here. Hence, it makes more sense to set
everything together at this point.

But I am okay if you think it is better to place the field assignment
for the hash calculation case at the definition site.

>
>> +
>> +    /*
>> +     * Consider that virtio_net_handle_rss() currently does not restore the
>> +     * hash key length parsed from the CVQ command sent from the guest into
>> +     * n->rss_data and uses the maximum key length in other code, so we also
>> +     * employthe the maxium key length here.
>
> two typos

I will correct these typos in the v4 patch.

>
>> +     */
>> +    cfg.hash_key_length = sizeof(n->rss_data.key);
>> +
>> +    const struct iovec data[] = {
>> +        {
>> +            .iov_base = &cfg,
>> +            .iov_len = offsetof(struct virtio_net_rss_config,
>> +                                indirection_table),
>> +        }, {
>> +            .iov_base = table,
>> +            .iov_len = n->rss_data.indirections_len *
>> +                       sizeof(n->rss_data.indirections_table[0]),
>> +        }, {
>> +            .iov_base = &cfg.max_tx_vq,
>> +            .iov_len = offsetof(struct virtio_net_rss_config, hash_key_data) -
>> +                       offsetof(struct virtio_net_rss_config, max_tx_vq),
>> +        }, {
>> +            .iov_base = (void *)n->rss_data.key,
>
> cast to void * should not be needed here.

Without this cast, the compiler raises the following warning:

../net/vhost-vdpa.c: In function ‘vhost_vdpa_net_load_rss’:
../net/vhost-vdpa.c:907:25: error: initialization discards ‘const’
qualifier from pointer target type [-Werror=discarded-qualifiers]
   907 |             .iov_base = n->rss_data.key,


The issue arises because `n` is a pointer to const, and
`n->rss_data.key` is an array. So `n->rss_data.key` is treated as a
pointer to const.

Thanks!


>
>> +            .iov_len = sizeof(n->rss_data.key),
>> +        }
>> +    };
>> +
>> +    r = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
>> +                                VIRTIO_NET_CTRL_MQ,
>> +                                VIRTIO_NET_CTRL_MQ_HASH_CONFIG,
>> +                                data, ARRAY_SIZE(data));
>> +    if (unlikely(r < 0)) {
>> +        return r;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
>>                                     const VirtIONet *n,
>>                                     struct iovec *out_cursor,
>> @@ -842,6 +922,15 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
>>           return r;
>>       }
>>
>> +    if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_HASH_REPORT)) {
>> +        return 0;
>> +    }
>> +
>> +    r = vhost_vdpa_net_load_rss(s, n, out_cursor, in_cursor);
>> +    if (unlikely(r < 0)) {
>> +        return r;
>> +    }
>> +
>>       return 0;
>>   }
>>
>> --
>> 2.25.1
>