[PATCH RFC v2 0/4] Vhost-vdpa Shadow Virtqueue _F_CTRL_RX commands support

[PATCH RFC v2 0/4] Vhost-vdpa Shadow Virtqueue _F_CTRL_RX commands support

[Hawkins Jiawei]

This series enables shadowed CVQ to intercept rx commands related to
VIRTIO_NET_F_CTRL_RX feature through shadowed CVQ, update the virtio
NIC device model so qemu send it in a migration, and the restore of
that rx state in the destination.

Note that this patch should be based on [1], which have not
been merged. I will submit a new version of patch after it is merged.

[1]. https://lists.nongnu.org/archive/html/qemu-devel/2023-06/msg05909.html

Changelog
=========
v2:
  - refactor vhost_vdpa_net_load_cmd() to accept iovec suggested by
Eugenio
  - avoid sending CVQ command in default state suggested by Eugenio

v1: https://lists.nongnu.org/archive/html/qemu-devel/2023-06/msg04423.html

Hawkins Jiawei (4):
  vdpa: Use iovec for vhost_vdpa_net_load_cmd()
  vdpa: Restore MAC address filtering state
  vdpa: Restore packet receive filtering state relative with _F_CTRL_RX
    feature
  vdpa: Allow VIRTIO_NET_F_CTRL_RX in SVQ

 net/vhost-vdpa.c | 198 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 188 insertions(+), 10 deletions(-)

-- 
2.25.1

[PATCH RFC v2 1/4] vdpa: Use iovec for vhost_vdpa_net_load_cmd()

[Hawkins Jiawei]

According to VirtIO standard, "The driver MUST follow
the VIRTIO_NET_CTRL_MAC_TABLE_SET command by a le32 number,
followed by that number of non-multicast MAC addresses,
followed by another le32 number, followed by that number
of multicast addresses."

Considering that these data is not stored in contiguous memory,
this patch refactors vhost_vdpa_net_load_cmd() to accept
scattered data, eliminating the need for an addtional data copy or
packing the data into s->cvq_cmd_out_buffer outside of
vhost_vdpa_net_load_cmd().

Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
---
v2:
  - refactor vhost_vdpa_load_cmd() to accept iovec suggested by
Eugenio

 net/vhost-vdpa.c | 42 ++++++++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 10 deletions(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 6f6a5c6df6..0bd1c7817c 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -620,29 +620,43 @@ static ssize_t vhost_vdpa_net_cvq_add(VhostVDPAState *s, size_t out_len,
 }
 
 static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, uint8_t class,
-                                       uint8_t cmd, const void *data,
-                                       size_t data_size)
+                                       uint8_t cmd, const struct iovec *data,
+                                       size_t data_len)
 {
     const struct virtio_net_ctrl_hdr ctrl = {
         .class = class,
         .cmd = cmd,
     };
+    void *cursor = s->cvq_cmd_out_buffer;
 
-    assert(data_size < vhost_vdpa_net_cvq_cmd_page_len() - sizeof(ctrl));
+    /* pack the CVQ command header */
+    assert(sizeof(ctrl) < vhost_vdpa_net_cvq_cmd_page_len() -
+                          (cursor - s->cvq_cmd_out_buffer));
+    memcpy(cursor, &ctrl, sizeof(ctrl));
+    cursor += sizeof(ctrl);
 
-    memcpy(s->cvq_cmd_out_buffer, &ctrl, sizeof(ctrl));
-    memcpy(s->cvq_cmd_out_buffer + sizeof(ctrl), data, data_size);
+    /* pack the CVQ command command-specific-data */
+    for (int i = 0; i < data_len; ++i) {
+        assert(data[i].iov_len < vhost_vdpa_net_cvq_cmd_page_len() -
+                                 (cursor - s->cvq_cmd_out_buffer));
+        memcpy(cursor, data[i].iov_base, data[i].iov_len);
+        cursor += data[i].iov_len;
+    }
 
-    return vhost_vdpa_net_cvq_add(s, sizeof(ctrl) + data_size,
+    return vhost_vdpa_net_cvq_add(s, cursor - s->cvq_cmd_out_buffer,
                                   sizeof(virtio_net_ctrl_ack));
 }
 
 static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
 {
     if (virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_MAC_ADDR)) {
+        const struct iovec data = {
+            .iov_base = (void *)n->mac,
+            .iov_len = sizeof(n->mac),
+        };
         ssize_t dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_MAC,
                                                   VIRTIO_NET_CTRL_MAC_ADDR_SET,
-                                                  n->mac, sizeof(n->mac));
+                                                  &data, 1);
         if (unlikely(dev_written < 0)) {
             return dev_written;
         }
@@ -665,9 +679,13 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
     }
 
     mq.virtqueue_pairs = cpu_to_le16(n->curr_queue_pairs);
+    const struct iovec data = {
+        .iov_base = &mq,
+        .iov_len = sizeof(mq),
+    };
     dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_MQ,
-                                          VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &mq,
-                                          sizeof(mq));
+                                          VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
+                                          &data, 1);
     if (unlikely(dev_written < 0)) {
         return dev_written;
     }
@@ -706,9 +724,13 @@ static int vhost_vdpa_net_load_offloads(VhostVDPAState *s,
     }
 
     offloads = cpu_to_le64(n->curr_guest_offloads);
+    const struct iovec data = {
+        .iov_base = &offloads,
+        .iov_len = sizeof(offloads),
+    };
     dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_GUEST_OFFLOADS,
                                           VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET,
-                                          &offloads, sizeof(offloads));
+                                          &data, 1);
     if (unlikely(dev_written < 0)) {
         return dev_written;
     }
-- 
2.25.1

Re: [PATCH RFC v2 1/4] vdpa: Use iovec for vhost_vdpa_net_load_cmd()

[Eugenio Perez Martin]

On Thu, Jun 29, 2023 at 5:25 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> According to VirtIO standard, "The driver MUST follow
> the VIRTIO_NET_CTRL_MAC_TABLE_SET command by a le32 number,
> followed by that number of non-multicast MAC addresses,
> followed by another le32 number, followed by that number
> of multicast addresses."
>
> Considering that these data is not stored in contiguous memory,
> this patch refactors vhost_vdpa_net_load_cmd() to accept
> scattered data, eliminating the need for an addtional data copy or
> packing the data into s->cvq_cmd_out_buffer outside of
> vhost_vdpa_net_load_cmd().
>
> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
> ---
> v2:
>   - refactor vhost_vdpa_load_cmd() to accept iovec suggested by
> Eugenio
>
>  net/vhost-vdpa.c | 42 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 6f6a5c6df6..0bd1c7817c 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -620,29 +620,43 @@ static ssize_t vhost_vdpa_net_cvq_add(VhostVDPAState *s, size_t out_len,
>  }
>
>  static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, uint8_t class,
> -                                       uint8_t cmd, const void *data,
> -                                       size_t data_size)
> +                                       uint8_t cmd, const struct iovec *data,
> +                                       size_t data_len)
>  {
>      const struct virtio_net_ctrl_hdr ctrl = {
>          .class = class,
>          .cmd = cmd,
>      };
> +    void *cursor = s->cvq_cmd_out_buffer;
>
> -    assert(data_size < vhost_vdpa_net_cvq_cmd_page_len() - sizeof(ctrl));
> +    /* pack the CVQ command header */
> +    assert(sizeof(ctrl) < vhost_vdpa_net_cvq_cmd_page_len() -
> +                          (cursor - s->cvq_cmd_out_buffer));
> +    memcpy(cursor, &ctrl, sizeof(ctrl));
> +    cursor += sizeof(ctrl);
>
> -    memcpy(s->cvq_cmd_out_buffer, &ctrl, sizeof(ctrl));
> -    memcpy(s->cvq_cmd_out_buffer + sizeof(ctrl), data, data_size);
> +    /* pack the CVQ command command-specific-data */
> +    for (int i = 0; i < data_len; ++i) {
> +        assert(data[i].iov_len < vhost_vdpa_net_cvq_cmd_page_len() -
> +                                 (cursor - s->cvq_cmd_out_buffer));
> +        memcpy(cursor, data[i].iov_base, data[i].iov_len);
> +        cursor += data[i].iov_len;
> +    }

Can we replace all of the above by iov_to_buf?

>
> -    return vhost_vdpa_net_cvq_add(s, sizeof(ctrl) + data_size,
> +    return vhost_vdpa_net_cvq_add(s, cursor - s->cvq_cmd_out_buffer,
>                                    sizeof(virtio_net_ctrl_ack));
>  }
>
>  static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
>  {
>      if (virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_MAC_ADDR)) {
> +        const struct iovec data = {
> +            .iov_base = (void *)n->mac,
> +            .iov_len = sizeof(n->mac),
> +        };
>          ssize_t dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_MAC,
>                                                    VIRTIO_NET_CTRL_MAC_ADDR_SET,
> -                                                  n->mac, sizeof(n->mac));
> +                                                  &data, 1);
>          if (unlikely(dev_written < 0)) {
>              return dev_written;
>          }
> @@ -665,9 +679,13 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
>      }
>
>      mq.virtqueue_pairs = cpu_to_le16(n->curr_queue_pairs);
> +    const struct iovec data = {
> +        .iov_base = &mq,
> +        .iov_len = sizeof(mq),
> +    };
>      dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_MQ,
> -                                          VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &mq,
> -                                          sizeof(mq));
> +                                          VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
> +                                          &data, 1);
>      if (unlikely(dev_written < 0)) {
>          return dev_written;
>      }
> @@ -706,9 +724,13 @@ static int vhost_vdpa_net_load_offloads(VhostVDPAState *s,
>      }
>
>      offloads = cpu_to_le64(n->curr_guest_offloads);
> +    const struct iovec data = {
> +        .iov_base = &offloads,
> +        .iov_len = sizeof(offloads),
> +    };
>      dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_GUEST_OFFLOADS,
>                                            VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET,
> -                                          &offloads, sizeof(offloads));
> +                                          &data, 1);
>      if (unlikely(dev_written < 0)) {
>          return dev_written;
>      }
> --
> 2.25.1
>

Re: [PATCH RFC v2 1/4] vdpa: Use iovec for vhost_vdpa_net_load_cmd()

[Hawkins Jiawei]

On 2023/7/4 22:17, Eugenio Perez Martin wrote:
> On Thu, Jun 29, 2023 at 5:25 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>>
>> According to VirtIO standard, "The driver MUST follow
>> the VIRTIO_NET_CTRL_MAC_TABLE_SET command by a le32 number,
>> followed by that number of non-multicast MAC addresses,
>> followed by another le32 number, followed by that number
>> of multicast addresses."
>>
>> Considering that these data is not stored in contiguous memory,
>> this patch refactors vhost_vdpa_net_load_cmd() to accept
>> scattered data, eliminating the need for an addtional data copy or
>> packing the data into s->cvq_cmd_out_buffer outside of
>> vhost_vdpa_net_load_cmd().
>>
>> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
>> ---
>> v2:
>>    - refactor vhost_vdpa_load_cmd() to accept iovec suggested by
>> Eugenio
>>
>>   net/vhost-vdpa.c | 42 ++++++++++++++++++++++++++++++++----------
>>   1 file changed, 32 insertions(+), 10 deletions(-)
>>
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index 6f6a5c6df6..0bd1c7817c 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -620,29 +620,43 @@ static ssize_t vhost_vdpa_net_cvq_add(VhostVDPAState *s, size_t out_len,
>>   }
>>
>>   static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, uint8_t class,
>> -                                       uint8_t cmd, const void *data,
>> -                                       size_t data_size)
>> +                                       uint8_t cmd, const struct iovec *data,
>> +                                       size_t data_len)
>>   {
>>       const struct virtio_net_ctrl_hdr ctrl = {
>>           .class = class,
>>           .cmd = cmd,
>>       };
>> +    void *cursor = s->cvq_cmd_out_buffer;
>>
>> -    assert(data_size < vhost_vdpa_net_cvq_cmd_page_len() - sizeof(ctrl));
>> +    /* pack the CVQ command header */
>> +    assert(sizeof(ctrl) < vhost_vdpa_net_cvq_cmd_page_len() -
>> +                          (cursor - s->cvq_cmd_out_buffer));
>> +    memcpy(cursor, &ctrl, sizeof(ctrl));
>> +    cursor += sizeof(ctrl);
>>
>> -    memcpy(s->cvq_cmd_out_buffer, &ctrl, sizeof(ctrl));
>> -    memcpy(s->cvq_cmd_out_buffer + sizeof(ctrl), data, data_size);
>> +    /* pack the CVQ command command-specific-data */
>> +    for (int i = 0; i < data_len; ++i) {
>> +        assert(data[i].iov_len < vhost_vdpa_net_cvq_cmd_page_len() -
>> +                                 (cursor - s->cvq_cmd_out_buffer));
>> +        memcpy(cursor, data[i].iov_base, data[i].iov_len);
>> +        cursor += data[i].iov_len;
>> +    }
>
> Can we replace all of the above by iov_to_buf?

Yes, you are right.

We should use iov_to_buf() here. I will refactor this patch according to
your suggestion.

Thanks!


>
>>
>> -    return vhost_vdpa_net_cvq_add(s, sizeof(ctrl) + data_size,
>> +    return vhost_vdpa_net_cvq_add(s, cursor - s->cvq_cmd_out_buffer,
>>                                     sizeof(virtio_net_ctrl_ack));
>>   }
>>
>>   static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
>>   {
>>       if (virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_MAC_ADDR)) {
>> +        const struct iovec data = {
>> +            .iov_base = (void *)n->mac,
>> +            .iov_len = sizeof(n->mac),
>> +        };
>>           ssize_t dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_MAC,
>>                                                     VIRTIO_NET_CTRL_MAC_ADDR_SET,
>> -                                                  n->mac, sizeof(n->mac));
>> +                                                  &data, 1);
>>           if (unlikely(dev_written < 0)) {
>>               return dev_written;
>>           }
>> @@ -665,9 +679,13 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
>>       }
>>
>>       mq.virtqueue_pairs = cpu_to_le16(n->curr_queue_pairs);
>> +    const struct iovec data = {
>> +        .iov_base = &mq,
>> +        .iov_len = sizeof(mq),
>> +    };
>>       dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_MQ,
>> -                                          VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &mq,
>> -                                          sizeof(mq));
>> +                                          VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
>> +                                          &data, 1);
>>       if (unlikely(dev_written < 0)) {
>>           return dev_written;
>>       }
>> @@ -706,9 +724,13 @@ static int vhost_vdpa_net_load_offloads(VhostVDPAState *s,
>>       }
>>
>>       offloads = cpu_to_le64(n->curr_guest_offloads);
>> +    const struct iovec data = {
>> +        .iov_base = &offloads,
>> +        .iov_len = sizeof(offloads),
>> +    };
>>       dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_GUEST_OFFLOADS,
>>                                             VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET,
>> -                                          &offloads, sizeof(offloads));
>> +                                          &data, 1);
>>       if (unlikely(dev_written < 0)) {
>>           return dev_written;
>>       }
>> --
>> 2.25.1
>>
>

[PATCH RFC v2 2/4] vdpa: Restore MAC address filtering state

[Hawkins Jiawei]

This patch refactors vhost_vdpa_net_load_mac() to
restore the MAC address filtering state at device's startup.

Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
---
v2:
  - use iovec suggested by Eugenio
  - avoid sending CVQ command in default state

v1: https://lore.kernel.org/all/00f72fe154a882fd6dc15bc39e3a1ac63f9dadce.1687402580.git.yin31149@gmail.com/

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

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 0bd1c7817c..cb45c84c88 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -665,6 +665,57 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
         }
     }
 
+    if (virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) {
+        if (n->mac_table.in_use != 0) {
+            /*
+             * According to virtio_net_reset(), device uses an empty MAC filter
+             * table as its default state.
+             *
+             * Therefore, there is no need to send this CVQ command if the
+             * driver also sets an empty MAC filter table, which aligns with
+             * the device's defaults.
+             *
+             * Note that the device's defaults can mismatch the driver's
+             * configuration only at live migration.
+             */
+            uint32_t uni_entries = n->mac_table.first_multi,
+                     uni_macs_size = uni_entries * ETH_ALEN,
+                     mul_entries = n->mac_table.in_use - uni_entries,
+                     mul_macs_size = mul_entries * ETH_ALEN;
+            struct virtio_net_ctrl_mac uni = {
+                .entries = cpu_to_le32(uni_entries),
+            };
+            struct virtio_net_ctrl_mac mul = {
+                .entries = cpu_to_le32(mul_entries),
+            };
+            const struct iovec data[] = {
+                {
+                    .iov_base = &uni,
+                    .iov_len = sizeof(uni),
+                }, {
+                    .iov_base = n->mac_table.macs,
+                    .iov_len = uni_macs_size,
+                }, {
+                    .iov_base = &mul,
+                    .iov_len = sizeof(mul),
+                }, {
+                    .iov_base = &n->mac_table.macs[uni_macs_size],
+                    .iov_len = mul_macs_size,
+                },
+            };
+            ssize_t dev_written = vhost_vdpa_net_load_cmd(s,
+                                        VIRTIO_NET_CTRL_MAC,
+                                        VIRTIO_NET_CTRL_MAC_TABLE_SET,
+                                        data, ARRAY_SIZE(data));
+            if (unlikely(dev_written < 0)) {
+                return dev_written;
+            }
+            if (*s->status != VIRTIO_NET_OK) {
+                return -EINVAL;
+            }
+        }
+    }
+
     return 0;
 }
 
-- 
2.25.1

Re: [PATCH RFC v2 2/4] vdpa: Restore MAC address filtering state

[Eugenio Perez Martin]

On Thu, Jun 29, 2023 at 5:26 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> This patch refactors vhost_vdpa_net_load_mac() to
> restore the MAC address filtering state at device's startup.
>
> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
> ---
> v2:
>   - use iovec suggested by Eugenio
>   - avoid sending CVQ command in default state
>
> v1: https://lore.kernel.org/all/00f72fe154a882fd6dc15bc39e3a1ac63f9dadce.1687402580.git.yin31149@gmail.com/
>
>  net/vhost-vdpa.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 0bd1c7817c..cb45c84c88 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -665,6 +665,57 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
>          }
>      }
>
> +    if (virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) {
> +        if (n->mac_table.in_use != 0) {

This may be just style nitpicking, but I find it more clear to return
early if conditions are not met and then send the CVQ command.
Something like:
/*
 * According to ...
 */
if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) ||
(n->mac_table.in_use == 0)) {
  return 0
}

uni_entries = n->mac_table.first_multi,
...
---

Now I just realized vhost_vdpa_net_load_mac does not follow this for
checking VIRTIO_NET_F_CTRL_MAC_ADDR.

I'm ok if you leave it this way though.

Thanks!

> +            /*
> +             * According to virtio_net_reset(), device uses an empty MAC filter
> +             * table as its default state.
> +             *
> +             * Therefore, there is no need to send this CVQ command if the
> +             * driver also sets an empty MAC filter table, which aligns with
> +             * the device's defaults.
> +             *
> +             * Note that the device's defaults can mismatch the driver's
> +             * configuration only at live migration.
> +             */
> +            uint32_t uni_entries = n->mac_table.first_multi,
> +                     uni_macs_size = uni_entries * ETH_ALEN,
> +                     mul_entries = n->mac_table.in_use - uni_entries,
> +                     mul_macs_size = mul_entries * ETH_ALEN;
> +            struct virtio_net_ctrl_mac uni = {
> +                .entries = cpu_to_le32(uni_entries),
> +            };
> +            struct virtio_net_ctrl_mac mul = {
> +                .entries = cpu_to_le32(mul_entries),
> +            };
> +            const struct iovec data[] = {
> +                {
> +                    .iov_base = &uni,
> +                    .iov_len = sizeof(uni),
> +                }, {
> +                    .iov_base = n->mac_table.macs,
> +                    .iov_len = uni_macs_size,
> +                }, {
> +                    .iov_base = &mul,
> +                    .iov_len = sizeof(mul),
> +                }, {
> +                    .iov_base = &n->mac_table.macs[uni_macs_size],
> +                    .iov_len = mul_macs_size,
> +                },
> +            };
> +            ssize_t dev_written = vhost_vdpa_net_load_cmd(s,
> +                                        VIRTIO_NET_CTRL_MAC,
> +                                        VIRTIO_NET_CTRL_MAC_TABLE_SET,
> +                                        data, ARRAY_SIZE(data));
> +            if (unlikely(dev_written < 0)) {
> +                return dev_written;
> +            }
> +            if (*s->status != VIRTIO_NET_OK) {
> +                return -EINVAL;
> +            }
> +        }
> +    }
> +
>      return 0;
>  }
>
> --
> 2.25.1
>

Re: [PATCH RFC v2 2/4] vdpa: Restore MAC address filtering state

[Hawkins Jiawei]

On 2023/7/4 22:53, Eugenio Perez Martin wrote:
> On Thu, Jun 29, 2023 at 5:26 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>>
>> This patch refactors vhost_vdpa_net_load_mac() to
>> restore the MAC address filtering state at device's startup.
>>
>> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
>> ---
>> v2:
>>    - use iovec suggested by Eugenio
>>    - avoid sending CVQ command in default state
>>
>> v1: https://lore.kernel.org/all/00f72fe154a882fd6dc15bc39e3a1ac63f9dadce.1687402580.git.yin31149@gmail.com/
>>
>>   net/vhost-vdpa.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 51 insertions(+)
>>
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index 0bd1c7817c..cb45c84c88 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -665,6 +665,57 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
>>           }
>>       }
>>
>> +    if (virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) {
>> +        if (n->mac_table.in_use != 0) {
>
> This may be just style nitpicking, but I find it more clear to return
> early if conditions are not met and then send the CVQ command.

Yes, this makes code more clear to read.

But it appears that we may meet a problem if the function
vhost_vdpa_net_load_x() sends multiple CVQ commands. It is possible that
we might not meet the condition for one of the CVQ commands, but we
could still meet the conditions for other CVQ commands.

Therefore, in the case of vhost_vdpa_net_load_x() sending multiple CVQ
commands, if we still hope to use this style, should we split the
function into multiple functions, with each function responsible for
sending only one CVQ command? Or should we jump to the next CVQ command
instead of returning from the function if the conditions are not met?

Thanks!


> Something like:
> /*
>   * According to ...
>   */
> if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) ||
> (n->mac_table.in_use == 0)) {
>    return 0
> }
>
> uni_entries = n->mac_table.first_multi,
> ...
> ---
>
> Now I just realized vhost_vdpa_net_load_mac does not follow this for
> checking VIRTIO_NET_F_CTRL_MAC_ADDR.
>
> I'm ok if you leave it this way though.
>
> Thanks!
>
>> +            /*
>> +             * According to virtio_net_reset(), device uses an empty MAC filter
>> +             * table as its default state.
>> +             *
>> +             * Therefore, there is no need to send this CVQ command if the
>> +             * driver also sets an empty MAC filter table, which aligns with
>> +             * the device's defaults.
>> +             *
>> +             * Note that the device's defaults can mismatch the driver's
>> +             * configuration only at live migration.
>> +             */
>> +            uint32_t uni_entries = n->mac_table.first_multi,
>> +                     uni_macs_size = uni_entries * ETH_ALEN,
>> +                     mul_entries = n->mac_table.in_use - uni_entries,
>> +                     mul_macs_size = mul_entries * ETH_ALEN;
>> +            struct virtio_net_ctrl_mac uni = {
>> +                .entries = cpu_to_le32(uni_entries),
>> +            };
>> +            struct virtio_net_ctrl_mac mul = {
>> +                .entries = cpu_to_le32(mul_entries),
>> +            };
>> +            const struct iovec data[] = {
>> +                {
>> +                    .iov_base = &uni,
>> +                    .iov_len = sizeof(uni),
>> +                }, {
>> +                    .iov_base = n->mac_table.macs,
>> +                    .iov_len = uni_macs_size,
>> +                }, {
>> +                    .iov_base = &mul,
>> +                    .iov_len = sizeof(mul),
>> +                }, {
>> +                    .iov_base = &n->mac_table.macs[uni_macs_size],
>> +                    .iov_len = mul_macs_size,
>> +                },
>> +            };
>> +            ssize_t dev_written = vhost_vdpa_net_load_cmd(s,
>> +                                        VIRTIO_NET_CTRL_MAC,
>> +                                        VIRTIO_NET_CTRL_MAC_TABLE_SET,
>> +                                        data, ARRAY_SIZE(data));
>> +            if (unlikely(dev_written < 0)) {
>> +                return dev_written;
>> +            }
>> +            if (*s->status != VIRTIO_NET_OK) {
>> +                return -EINVAL;
>> +            }
>> +        }
>> +    }
>> +
>>       return 0;
>>   }
>>
>> --
>> 2.25.1
>>
>

Re: [PATCH RFC v2 2/4] vdpa: Restore MAC address filtering state

[Eugenio Perez Martin]

On Wed, Jul 5, 2023 at 3:43 AM Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> On 2023/7/4 22:53, Eugenio Perez Martin wrote:
> > On Thu, Jun 29, 2023 at 5:26 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
> >>
> >> This patch refactors vhost_vdpa_net_load_mac() to
> >> restore the MAC address filtering state at device's startup.
> >>
> >> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
> >> ---
> >> v2:
> >>    - use iovec suggested by Eugenio
> >>    - avoid sending CVQ command in default state
> >>
> >> v1: https://lore.kernel.org/all/00f72fe154a882fd6dc15bc39e3a1ac63f9dadce.1687402580.git.yin31149@gmail.com/
> >>
> >>   net/vhost-vdpa.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 51 insertions(+)
> >>
> >> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> >> index 0bd1c7817c..cb45c84c88 100644
> >> --- a/net/vhost-vdpa.c
> >> +++ b/net/vhost-vdpa.c
> >> @@ -665,6 +665,57 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
> >>           }
> >>       }
> >>
> >> +    if (virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) {
> >> +        if (n->mac_table.in_use != 0) {
> >
> > This may be just style nitpicking, but I find it more clear to return
> > early if conditions are not met and then send the CVQ command.
>
> Yes, this makes code more clear to read.
>
> But it appears that we may meet a problem if the function
> vhost_vdpa_net_load_x() sends multiple CVQ commands. It is possible that
> we might not meet the condition for one of the CVQ commands, but we
> could still meet the conditions for other CVQ commands.
>
> Therefore, in the case of vhost_vdpa_net_load_x() sending multiple CVQ
> commands, if we still hope to use this style, should we split the
> function into multiple functions, with each function responsible for
> sending only one CVQ command? Or should we jump to the next CVQ command
> instead of returning from the function if the conditions are not met?
>

In that case I'd suggest using multiples if() {}, as each ctrl command
processing code is very small.

But for VIRTIO_NET_F_CTRL_RX particular case your patch propose:
if (x) {
  if (y) {
    ...
  }
}

So in my opinion it makes sense to convert to:
if (!x || !y) {
  return;
}
...

We can always change later if needed.

Thanks!

> Thanks!
>
>
> > Something like:
> > /*
> >   * According to ...
> >   */
> > if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) ||
> > (n->mac_table.in_use == 0)) {
> >    return 0
> > }
> >
> > uni_entries = n->mac_table.first_multi,
> > ...
> > ---
> >
> > Now I just realized vhost_vdpa_net_load_mac does not follow this for
> > checking VIRTIO_NET_F_CTRL_MAC_ADDR.
> >
> > I'm ok if you leave it this way though.
> >
> > Thanks!
> >
> >> +            /*
> >> +             * According to virtio_net_reset(), device uses an empty MAC filter
> >> +             * table as its default state.
> >> +             *
> >> +             * Therefore, there is no need to send this CVQ command if the
> >> +             * driver also sets an empty MAC filter table, which aligns with
> >> +             * the device's defaults.
> >> +             *
> >> +             * Note that the device's defaults can mismatch the driver's
> >> +             * configuration only at live migration.
> >> +             */
> >> +            uint32_t uni_entries = n->mac_table.first_multi,
> >> +                     uni_macs_size = uni_entries * ETH_ALEN,
> >> +                     mul_entries = n->mac_table.in_use - uni_entries,
> >> +                     mul_macs_size = mul_entries * ETH_ALEN;
> >> +            struct virtio_net_ctrl_mac uni = {
> >> +                .entries = cpu_to_le32(uni_entries),
> >> +            };
> >> +            struct virtio_net_ctrl_mac mul = {
> >> +                .entries = cpu_to_le32(mul_entries),
> >> +            };
> >> +            const struct iovec data[] = {
> >> +                {
> >> +                    .iov_base = &uni,
> >> +                    .iov_len = sizeof(uni),
> >> +                }, {
> >> +                    .iov_base = n->mac_table.macs,
> >> +                    .iov_len = uni_macs_size,
> >> +                }, {
> >> +                    .iov_base = &mul,
> >> +                    .iov_len = sizeof(mul),
> >> +                }, {
> >> +                    .iov_base = &n->mac_table.macs[uni_macs_size],
> >> +                    .iov_len = mul_macs_size,
> >> +                },
> >> +            };
> >> +            ssize_t dev_written = vhost_vdpa_net_load_cmd(s,
> >> +                                        VIRTIO_NET_CTRL_MAC,
> >> +                                        VIRTIO_NET_CTRL_MAC_TABLE_SET,
> >> +                                        data, ARRAY_SIZE(data));
> >> +            if (unlikely(dev_written < 0)) {
> >> +                return dev_written;
> >> +            }
> >> +            if (*s->status != VIRTIO_NET_OK) {
> >> +                return -EINVAL;
> >> +            }
> >> +        }
> >> +    }
> >> +
> >>       return 0;
> >>   }
> >>
> >> --
> >> 2.25.1
> >>
> >
>

Re: [PATCH RFC v2 2/4] vdpa: Restore MAC address filtering state

[Hawkins Jiawei]

On 2023/7/5 14:29, Eugenio Perez Martin wrote:
> On Wed, Jul 5, 2023 at 3:43 AM Hawkins Jiawei <yin31149@gmail.com> wrote:
>>
>> On 2023/7/4 22:53, Eugenio Perez Martin wrote:
>>> On Thu, Jun 29, 2023 at 5:26 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>>>>
>>>> This patch refactors vhost_vdpa_net_load_mac() to
>>>> restore the MAC address filtering state at device's startup.
>>>>
>>>> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
>>>> ---
>>>> v2:
>>>>     - use iovec suggested by Eugenio
>>>>     - avoid sending CVQ command in default state
>>>>
>>>> v1: https://lore.kernel.org/all/00f72fe154a882fd6dc15bc39e3a1ac63f9dadce.1687402580.git.yin31149@gmail.com/
>>>>
>>>>    net/vhost-vdpa.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 51 insertions(+)
>>>>
>>>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>>>> index 0bd1c7817c..cb45c84c88 100644
>>>> --- a/net/vhost-vdpa.c
>>>> +++ b/net/vhost-vdpa.c
>>>> @@ -665,6 +665,57 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
>>>>            }
>>>>        }
>>>>
>>>> +    if (virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) {
>>>> +        if (n->mac_table.in_use != 0) {
>>>
>>> This may be just style nitpicking, but I find it more clear to return
>>> early if conditions are not met and then send the CVQ command.
>>
>> Yes, this makes code more clear to read.
>>
>> But it appears that we may meet a problem if the function
>> vhost_vdpa_net_load_x() sends multiple CVQ commands. It is possible that
>> we might not meet the condition for one of the CVQ commands, but we
>> could still meet the conditions for other CVQ commands.
>>
>> Therefore, in the case of vhost_vdpa_net_load_x() sending multiple CVQ
>> commands, if we still hope to use this style, should we split the
>> function into multiple functions, with each function responsible for
>> sending only one CVQ command? Or should we jump to the next CVQ command
>> instead of returning from the function if the conditions are not met?
>>
>
> In that case I'd suggest using multiples if() {}, as each ctrl command
> processing code is very small.
>
> But for VIRTIO_NET_F_CTRL_RX particular case your patch propose:
> if (x) {
>    if (y) {
>      ...
>    }
> }
>
> So in my opinion it makes sense to convert to:
> if (!x || !y) {
>    return;
> }
> ...

Thanks for your explanation.

I will refactor the patch according to your suggestion.

Thanks!


>
> We can always change later if needed.
>
> Thanks!
>
>> Thanks!
>>
>>
>>> Something like:
>>> /*
>>>    * According to ...
>>>    */
>>> if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) ||
>>> (n->mac_table.in_use == 0)) {
>>>     return 0
>>> }
>>>
>>> uni_entries = n->mac_table.first_multi,
>>> ...
>>> ---
>>>
>>> Now I just realized vhost_vdpa_net_load_mac does not follow this for
>>> checking VIRTIO_NET_F_CTRL_MAC_ADDR.
>>>
>>> I'm ok if you leave it this way though.
>>>
>>> Thanks!
>>>
>>>> +            /*
>>>> +             * According to virtio_net_reset(), device uses an empty MAC filter
>>>> +             * table as its default state.
>>>> +             *
>>>> +             * Therefore, there is no need to send this CVQ command if the
>>>> +             * driver also sets an empty MAC filter table, which aligns with
>>>> +             * the device's defaults.
>>>> +             *
>>>> +             * Note that the device's defaults can mismatch the driver's
>>>> +             * configuration only at live migration.
>>>> +             */
>>>> +            uint32_t uni_entries = n->mac_table.first_multi,
>>>> +                     uni_macs_size = uni_entries * ETH_ALEN,
>>>> +                     mul_entries = n->mac_table.in_use - uni_entries,
>>>> +                     mul_macs_size = mul_entries * ETH_ALEN;
>>>> +            struct virtio_net_ctrl_mac uni = {
>>>> +                .entries = cpu_to_le32(uni_entries),
>>>> +            };
>>>> +            struct virtio_net_ctrl_mac mul = {
>>>> +                .entries = cpu_to_le32(mul_entries),
>>>> +            };
>>>> +            const struct iovec data[] = {
>>>> +                {
>>>> +                    .iov_base = &uni,
>>>> +                    .iov_len = sizeof(uni),
>>>> +                }, {
>>>> +                    .iov_base = n->mac_table.macs,
>>>> +                    .iov_len = uni_macs_size,
>>>> +                }, {
>>>> +                    .iov_base = &mul,
>>>> +                    .iov_len = sizeof(mul),
>>>> +                }, {
>>>> +                    .iov_base = &n->mac_table.macs[uni_macs_size],
>>>> +                    .iov_len = mul_macs_size,
>>>> +                },
>>>> +            };
>>>> +            ssize_t dev_written = vhost_vdpa_net_load_cmd(s,
>>>> +                                        VIRTIO_NET_CTRL_MAC,
>>>> +                                        VIRTIO_NET_CTRL_MAC_TABLE_SET,
>>>> +                                        data, ARRAY_SIZE(data));
>>>> +            if (unlikely(dev_written < 0)) {
>>>> +                return dev_written;
>>>> +            }
>>>> +            if (*s->status != VIRTIO_NET_OK) {
>>>> +                return -EINVAL;
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +
>>>>        return 0;
>>>>    }
>>>>
>>>> --
>>>> 2.25.1
>>>>
>>>
>>
>

[PATCH RFC v2 3/4] vdpa: Restore packet receive filtering state relative with _F_CTRL_RX feature

[Hawkins Jiawei]

This patch introduces vhost_vdpa_net_load_rx_mode()
and vhost_vdpa_net_load_rx() to restore the packet
receive filtering state in relation to
VIRTIO_NET_F_CTRL_RX feature at device's startup.

Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
---
v2:
  - avoid sending CVQ command in default state suggested by Eugenio

v1: https://lore.kernel.org/all/86eeddcd6f6b04e5c1e44e901ddea3b1b8b6c183.1687402580.git.yin31149@gmail.com/

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

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index cb45c84c88..9d5d88756c 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -792,6 +792,106 @@ static int vhost_vdpa_net_load_offloads(VhostVDPAState *s,
     return 0;
 }
 
+static int vhost_vdpa_net_load_rx_mode(VhostVDPAState *s,
+                                       uint8_t cmd,
+                                       uint8_t on)
+{
+    ssize_t dev_written;
+    const struct iovec data = {
+        .iov_base = &on,
+        .iov_len = sizeof(on),
+    };
+    dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_RX,
+                                          cmd, &data, 1);
+    if (unlikely(dev_written < 0)) {
+        return dev_written;
+    }
+    if (*s->status != VIRTIO_NET_OK) {
+        return -EINVAL;
+    }
+
+    return 0;
+}
+
+static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
+                                  const VirtIONet *n)
+{
+    uint8_t on;
+    int r;
+
+    if (virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) {
+        /* Load the promiscous mode */
+        if (n->mac_table.uni_overflow) {
+            /*
+             * According to VirtIO standard, "Since there are no guarantees,
+             * it can use a hash filter or silently switch to
+             * allmulti or promiscuous mode if it is given too many addresses."
+             *
+             * QEMU ignores non-multicast(unicast) MAC addresses and
+             * marks `uni_overflow` for the device internal state
+             * if guest sets too many non-multicast(unicast) MAC addresses.
+             * Therefore, we should turn promiscous mode on in this case.
+             */
+            on = 1;
+        } else {
+            on = n->promisc;
+        }
+        if (on != 1) {
+            /*
+             * According to virtio_net_reset(), device turns promiscuous mode on
+             * by default.
+             *
+             * Therefore, there is no need to send this CVQ command if the
+             * driver also sets promiscuous mode on, which aligns with
+             * the device's defaults.
+             *
+             * Note that the device's defaults can mismatch the driver's
+             * configuration only at live migration.
+             */
+            r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_PROMISC, on);
+            if (r < 0) {
+                return r;
+            }
+        }
+
+        /* Load the all-multicast mode */
+        if (n->mac_table.multi_overflow) {
+            /*
+             * According to VirtIO standard, "Since there are no guarantees,
+             * it can use a hash filter or silently switch to
+             * allmulti or promiscuous mode if it is given too many addresses."
+             *
+             * QEMU ignores multicast MAC addresses and
+             * marks `multi_overflow` for the device internal state
+             * if guest sets too many multicast MAC addresses.
+             * Therefore, we should turn all-multicast mode on in this case.
+             */
+            on = 1;
+        } else {
+            on = n->allmulti;
+        }
+        if (on != 0) {
+            /*
+             * According to virtio_net_reset(), device turns all-multicast mode
+             * off by default.
+             *
+             * Therefore, there is no need to send this CVQ command if the
+             * driver also sets all-multicast mode off, which aligns with
+             * the device's defaults.
+             *
+             * Note that the device's defaults can mismatch the driver's
+             * configuration only at live migration.
+             */
+            r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_ALLMULTI, on);
+            if (r < 0) {
+                return r;
+            }
+        }
+    }
+
+    return 0;
+}
+
 static int vhost_vdpa_net_load(NetClientState *nc)
 {
     VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
@@ -818,6 +918,10 @@ static int vhost_vdpa_net_load(NetClientState *nc)
     if (unlikely(r)) {
         return r;
     }
+    r = vhost_vdpa_net_load_rx(s, n);
+    if (unlikely(r)) {
+        return r;
+    }
 
     return 0;
 }
-- 
2.25.1

Re: [PATCH RFC v2 3/4] vdpa: Restore packet receive filtering state relative with _F_CTRL_RX feature

[Eugenio Perez Martin]

On Thu, Jun 29, 2023 at 5:26 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> This patch introduces vhost_vdpa_net_load_rx_mode()
> and vhost_vdpa_net_load_rx() to restore the packet
> receive filtering state in relation to
> VIRTIO_NET_F_CTRL_RX feature at device's startup.
>
> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
> ---
> v2:
>   - avoid sending CVQ command in default state suggested by Eugenio
>
> v1: https://lore.kernel.org/all/86eeddcd6f6b04e5c1e44e901ddea3b1b8b6c183.1687402580.git.yin31149@gmail.com/
>
>  net/vhost-vdpa.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 104 insertions(+)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index cb45c84c88..9d5d88756c 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -792,6 +792,106 @@ static int vhost_vdpa_net_load_offloads(VhostVDPAState *s,
>      return 0;
>  }
>
> +static int vhost_vdpa_net_load_rx_mode(VhostVDPAState *s,
> +                                       uint8_t cmd,
> +                                       uint8_t on)
> +{
> +    ssize_t dev_written;
> +    const struct iovec data = {
> +        .iov_base = &on,
> +        .iov_len = sizeof(on),
> +    };
> +    dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_RX,
> +                                          cmd, &data, 1);
> +    if (unlikely(dev_written < 0)) {
> +        return dev_written;
> +    }
> +    if (*s->status != VIRTIO_NET_OK) {
> +        return -EINVAL;
> +    }
> +
> +    return 0;
> +}
> +
> +static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
> +                                  const VirtIONet *n)
> +{
> +    uint8_t on;
> +    int r;
> +
> +    if (virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) {

Also suggesting early returns here.

> +        /* Load the promiscous mode */
> +        if (n->mac_table.uni_overflow) {
> +            /*
> +             * According to VirtIO standard, "Since there are no guarantees,
> +             * it can use a hash filter or silently switch to
> +             * allmulti or promiscuous mode if it is given too many addresses."
> +             *
> +             * QEMU ignores non-multicast(unicast) MAC addresses and
> +             * marks `uni_overflow` for the device internal state
> +             * if guest sets too many non-multicast(unicast) MAC addresses.
> +             * Therefore, we should turn promiscous mode on in this case.
> +             */
> +            on = 1;
> +        } else {
> +            on = n->promisc;
> +        }

I think we can remove the "on" variable and just do:

/*
 * According to ...
 */
if (n->mac_table.uni_overflow || n->promisc) {
  r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_PROMISC, on);
  if (r < 0) {
    return r;
  }
---

And the equivalent for multicast.

Would that make sense?

Thanks!

> +        if (on != 1) {
> +            /*
> +             * According to virtio_net_reset(), device turns promiscuous mode on
> +             * by default.
> +             *
> +             * Therefore, there is no need to send this CVQ command if the
> +             * driver also sets promiscuous mode on, which aligns with
> +             * the device's defaults.
> +             *
> +             * Note that the device's defaults can mismatch the driver's
> +             * configuration only at live migration.
> +             */
> +            r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_PROMISC, on);
> +            if (r < 0) {
> +                return r;
> +            }
> +        }
> +
> +        /* Load the all-multicast mode */
> +        if (n->mac_table.multi_overflow) {
> +            /*
> +             * According to VirtIO standard, "Since there are no guarantees,
> +             * it can use a hash filter or silently switch to
> +             * allmulti or promiscuous mode if it is given too many addresses."
> +             *
> +             * QEMU ignores multicast MAC addresses and
> +             * marks `multi_overflow` for the device internal state
> +             * if guest sets too many multicast MAC addresses.
> +             * Therefore, we should turn all-multicast mode on in this case.
> +             */
> +            on = 1;
> +        } else {
> +            on = n->allmulti;
> +        }
> +        if (on != 0) {
> +            /*
> +             * According to virtio_net_reset(), device turns all-multicast mode
> +             * off by default.
> +             *
> +             * Therefore, there is no need to send this CVQ command if the
> +             * driver also sets all-multicast mode off, which aligns with
> +             * the device's defaults.
> +             *
> +             * Note that the device's defaults can mismatch the driver's
> +             * configuration only at live migration.
> +             */
> +            r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_ALLMULTI, on);
> +            if (r < 0) {
> +                return r;
> +            }
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static int vhost_vdpa_net_load(NetClientState *nc)
>  {
>      VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> @@ -818,6 +918,10 @@ static int vhost_vdpa_net_load(NetClientState *nc)
>      if (unlikely(r)) {
>          return r;
>      }
> +    r = vhost_vdpa_net_load_rx(s, n);
> +    if (unlikely(r)) {
> +        return r;
> +    }
>
>      return 0;
>  }
> --
> 2.25.1
>

Re: [PATCH RFC v2 3/4] vdpa: Restore packet receive filtering state relative with _F_CTRL_RX feature

[Hawkins Jiawei]

On 2023/7/4 23:39, Eugenio Perez Martin wrote:
> On Thu, Jun 29, 2023 at 5:26 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>>
>> This patch introduces vhost_vdpa_net_load_rx_mode()
>> and vhost_vdpa_net_load_rx() to restore the packet
>> receive filtering state in relation to
>> VIRTIO_NET_F_CTRL_RX feature at device's startup.
>>
>> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
>> ---
>> v2:
>>    - avoid sending CVQ command in default state suggested by Eugenio
>>
>> v1: https://lore.kernel.org/all/86eeddcd6f6b04e5c1e44e901ddea3b1b8b6c183.1687402580.git.yin31149@gmail.com/
>>
>>   net/vhost-vdpa.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 104 insertions(+)
>>
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index cb45c84c88..9d5d88756c 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -792,6 +792,106 @@ static int vhost_vdpa_net_load_offloads(VhostVDPAState *s,
>>       return 0;
>>   }
>>
>> +static int vhost_vdpa_net_load_rx_mode(VhostVDPAState *s,
>> +                                       uint8_t cmd,
>> +                                       uint8_t on)
>> +{
>> +    ssize_t dev_written;
>> +    const struct iovec data = {
>> +        .iov_base = &on,
>> +        .iov_len = sizeof(on),
>> +    };
>> +    dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_RX,
>> +                                          cmd, &data, 1);
>> +    if (unlikely(dev_written < 0)) {
>> +        return dev_written;
>> +    }
>> +    if (*s->status != VIRTIO_NET_OK) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
>> +                                  const VirtIONet *n)
>> +{
>> +    uint8_t on;
>> +    int r;
>> +
>> +    if (virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) {
>
> Also suggesting early returns here.

So, for CVQ commands related to VIRTIO_NET_F_CTRL_EXTRA_RX, would it be
more appropriate to create a new function, maybe
vhost_vdpa_net_load_rx_extra, to handle them instead of sending those
CVQ commands within this function, if we choose to return early?

>
>> +        /* Load the promiscous mode */
>> +        if (n->mac_table.uni_overflow) {
>> +            /*
>> +             * According to VirtIO standard, "Since there are no guarantees,
>> +             * it can use a hash filter or silently switch to
>> +             * allmulti or promiscuous mode if it is given too many addresses."
>> +             *
>> +             * QEMU ignores non-multicast(unicast) MAC addresses and
>> +             * marks `uni_overflow` for the device internal state
>> +             * if guest sets too many non-multicast(unicast) MAC addresses.
>> +             * Therefore, we should turn promiscous mode on in this case.
>> +             */
>> +            on = 1;
>> +        } else {
>> +            on = n->promisc;
>> +        }
>
> I think we can remove the "on" variable and just do:
>
> /*
>   * According to ...
>   */
> if (n->mac_table.uni_overflow || n->promisc) {
>    r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_PROMISC, on);
>    if (r < 0) {
>      return r;
>    }
> ---
>
> And the equivalent for multicast.
>
> Would that make sense?

Yes, I will refactor these according to your suggestion.

Thanks!


>
> Thanks!
>
>> +        if (on != 1) {
>> +            /*
>> +             * According to virtio_net_reset(), device turns promiscuous mode on
>> +             * by default.
>> +             *
>> +             * Therefore, there is no need to send this CVQ command if the
>> +             * driver also sets promiscuous mode on, which aligns with
>> +             * the device's defaults.
>> +             *
>> +             * Note that the device's defaults can mismatch the driver's
>> +             * configuration only at live migration.
>> +             */
>> +            r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_PROMISC, on);
>> +            if (r < 0) {
>> +                return r;
>> +            }
>> +        }
>> +
>> +        /* Load the all-multicast mode */
>> +        if (n->mac_table.multi_overflow) {
>> +            /*
>> +             * According to VirtIO standard, "Since there are no guarantees,
>> +             * it can use a hash filter or silently switch to
>> +             * allmulti or promiscuous mode if it is given too many addresses."
>> +             *
>> +             * QEMU ignores multicast MAC addresses and
>> +             * marks `multi_overflow` for the device internal state
>> +             * if guest sets too many multicast MAC addresses.
>> +             * Therefore, we should turn all-multicast mode on in this case.
>> +             */
>> +            on = 1;
>> +        } else {
>> +            on = n->allmulti;
>> +        }
>> +        if (on != 0) {
>> +            /*
>> +             * According to virtio_net_reset(), device turns all-multicast mode
>> +             * off by default.
>> +             *
>> +             * Therefore, there is no need to send this CVQ command if the
>> +             * driver also sets all-multicast mode off, which aligns with
>> +             * the device's defaults.
>> +             *
>> +             * Note that the device's defaults can mismatch the driver's
>> +             * configuration only at live migration.
>> +             */
>> +            r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_ALLMULTI, on);
>> +            if (r < 0) {
>> +                return r;
>> +            }
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>   static int vhost_vdpa_net_load(NetClientState *nc)
>>   {
>>       VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
>> @@ -818,6 +918,10 @@ static int vhost_vdpa_net_load(NetClientState *nc)
>>       if (unlikely(r)) {
>>           return r;
>>       }
>> +    r = vhost_vdpa_net_load_rx(s, n);
>> +    if (unlikely(r)) {
>> +        return r;
>> +    }
>>
>>       return 0;
>>   }
>> --
>> 2.25.1
>>
>

Re: [PATCH RFC v2 3/4] vdpa: Restore packet receive filtering state relative with _F_CTRL_RX feature

[Eugenio Perez Martin]

On Wed, Jul 5, 2023 at 4:09 AM Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> On 2023/7/4 23:39, Eugenio Perez Martin wrote:
> > On Thu, Jun 29, 2023 at 5:26 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
> >>
> >> This patch introduces vhost_vdpa_net_load_rx_mode()
> >> and vhost_vdpa_net_load_rx() to restore the packet
> >> receive filtering state in relation to
> >> VIRTIO_NET_F_CTRL_RX feature at device's startup.
> >>
> >> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
> >> ---
> >> v2:
> >>    - avoid sending CVQ command in default state suggested by Eugenio
> >>
> >> v1: https://lore.kernel.org/all/86eeddcd6f6b04e5c1e44e901ddea3b1b8b6c183.1687402580.git.yin31149@gmail.com/
> >>
> >>   net/vhost-vdpa.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 104 insertions(+)
> >>
> >> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> >> index cb45c84c88..9d5d88756c 100644
> >> --- a/net/vhost-vdpa.c
> >> +++ b/net/vhost-vdpa.c
> >> @@ -792,6 +792,106 @@ static int vhost_vdpa_net_load_offloads(VhostVDPAState *s,
> >>       return 0;
> >>   }
> >>
> >> +static int vhost_vdpa_net_load_rx_mode(VhostVDPAState *s,
> >> +                                       uint8_t cmd,
> >> +                                       uint8_t on)
> >> +{
> >> +    ssize_t dev_written;
> >> +    const struct iovec data = {
> >> +        .iov_base = &on,
> >> +        .iov_len = sizeof(on),
> >> +    };
> >> +    dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_RX,
> >> +                                          cmd, &data, 1);
> >> +    if (unlikely(dev_written < 0)) {
> >> +        return dev_written;
> >> +    }
> >> +    if (*s->status != VIRTIO_NET_OK) {
> >> +        return -EINVAL;
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >> +
> >> +static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
> >> +                                  const VirtIONet *n)
> >> +{
> >> +    uint8_t on;
> >> +    int r;
> >> +
> >> +    if (virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) {
> >
> > Also suggesting early returns here.
>
> So, for CVQ commands related to VIRTIO_NET_F_CTRL_EXTRA_RX, would it be
> more appropriate to create a new function, maybe
> vhost_vdpa_net_load_rx_extra, to handle them instead of sending those
> CVQ commands within this function, if we choose to return early?
>

My understanding is that VIRTIO_NET_F_CTRL_RX_EXTRA depends on
VIRTIO_NET_F_CTRL_RX, so we can do:

if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) {
  return;
}

// Process CTRL_RX commands

if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) {
  return;
}

// process CTRL_RX_EXTRA commands

> >
> >> +        /* Load the promiscous mode */
> >> +        if (n->mac_table.uni_overflow) {
> >> +            /*
> >> +             * According to VirtIO standard, "Since there are no guarantees,
> >> +             * it can use a hash filter or silently switch to
> >> +             * allmulti or promiscuous mode if it is given too many addresses."
> >> +             *
> >> +             * QEMU ignores non-multicast(unicast) MAC addresses and
> >> +             * marks `uni_overflow` for the device internal state
> >> +             * if guest sets too many non-multicast(unicast) MAC addresses.
> >> +             * Therefore, we should turn promiscous mode on in this case.
> >> +             */
> >> +            on = 1;
> >> +        } else {
> >> +            on = n->promisc;
> >> +        }
> >
> > I think we can remove the "on" variable and just do:
> >
> > /*
> >   * According to ...
> >   */
> > if (n->mac_table.uni_overflow || n->promisc) {
> >    r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_PROMISC, on);
> >    if (r < 0) {
> >      return r;
> >    }
> > ---
> >
> > And the equivalent for multicast.
> >
> > Would that make sense?
>
> Yes, I will refactor these according to your suggestion.
>
> Thanks!
>
>
> >
> > Thanks!
> >
> >> +        if (on != 1) {
> >> +            /*
> >> +             * According to virtio_net_reset(), device turns promiscuous mode on
> >> +             * by default.
> >> +             *
> >> +             * Therefore, there is no need to send this CVQ command if the
> >> +             * driver also sets promiscuous mode on, which aligns with
> >> +             * the device's defaults.
> >> +             *
> >> +             * Note that the device's defaults can mismatch the driver's
> >> +             * configuration only at live migration.
> >> +             */
> >> +            r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_PROMISC, on);
> >> +            if (r < 0) {
> >> +                return r;
> >> +            }
> >> +        }
> >> +
> >> +        /* Load the all-multicast mode */
> >> +        if (n->mac_table.multi_overflow) {
> >> +            /*
> >> +             * According to VirtIO standard, "Since there are no guarantees,
> >> +             * it can use a hash filter or silently switch to
> >> +             * allmulti or promiscuous mode if it is given too many addresses."
> >> +             *
> >> +             * QEMU ignores multicast MAC addresses and
> >> +             * marks `multi_overflow` for the device internal state
> >> +             * if guest sets too many multicast MAC addresses.
> >> +             * Therefore, we should turn all-multicast mode on in this case.
> >> +             */
> >> +            on = 1;
> >> +        } else {
> >> +            on = n->allmulti;
> >> +        }
> >> +        if (on != 0) {
> >> +            /*
> >> +             * According to virtio_net_reset(), device turns all-multicast mode
> >> +             * off by default.
> >> +             *
> >> +             * Therefore, there is no need to send this CVQ command if the
> >> +             * driver also sets all-multicast mode off, which aligns with
> >> +             * the device's defaults.
> >> +             *
> >> +             * Note that the device's defaults can mismatch the driver's
> >> +             * configuration only at live migration.
> >> +             */
> >> +            r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_ALLMULTI, on);
> >> +            if (r < 0) {
> >> +                return r;
> >> +            }
> >> +        }
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >> +
> >>   static int vhost_vdpa_net_load(NetClientState *nc)
> >>   {
> >>       VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> >> @@ -818,6 +918,10 @@ static int vhost_vdpa_net_load(NetClientState *nc)
> >>       if (unlikely(r)) {
> >>           return r;
> >>       }
> >> +    r = vhost_vdpa_net_load_rx(s, n);
> >> +    if (unlikely(r)) {
> >> +        return r;
> >> +    }
> >>
> >>       return 0;
> >>   }
> >> --
> >> 2.25.1
> >>
> >
>

Re: [PATCH RFC v2 3/4] vdpa: Restore packet receive filtering state relative with _F_CTRL_RX feature

[Hawkins Jiawei]

On 2023/7/5 14:40, Eugenio Perez Martin wrote:
> On Wed, Jul 5, 2023 at 4:09 AM Hawkins Jiawei <yin31149@gmail.com> wrote:
>>
>> On 2023/7/4 23:39, Eugenio Perez Martin wrote:
>>> On Thu, Jun 29, 2023 at 5:26 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>>>>
>>>> This patch introduces vhost_vdpa_net_load_rx_mode()
>>>> and vhost_vdpa_net_load_rx() to restore the packet
>>>> receive filtering state in relation to
>>>> VIRTIO_NET_F_CTRL_RX feature at device's startup.
>>>>
>>>> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
>>>> ---
>>>> v2:
>>>>     - avoid sending CVQ command in default state suggested by Eugenio
>>>>
>>>> v1: https://lore.kernel.org/all/86eeddcd6f6b04e5c1e44e901ddea3b1b8b6c183.1687402580.git.yin31149@gmail.com/
>>>>
>>>>    net/vhost-vdpa.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 104 insertions(+)
>>>>
>>>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>>>> index cb45c84c88..9d5d88756c 100644
>>>> --- a/net/vhost-vdpa.c
>>>> +++ b/net/vhost-vdpa.c
>>>> @@ -792,6 +792,106 @@ static int vhost_vdpa_net_load_offloads(VhostVDPAState *s,
>>>>        return 0;
>>>>    }
>>>>
>>>> +static int vhost_vdpa_net_load_rx_mode(VhostVDPAState *s,
>>>> +                                       uint8_t cmd,
>>>> +                                       uint8_t on)
>>>> +{
>>>> +    ssize_t dev_written;
>>>> +    const struct iovec data = {
>>>> +        .iov_base = &on,
>>>> +        .iov_len = sizeof(on),
>>>> +    };
>>>> +    dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_RX,
>>>> +                                          cmd, &data, 1);
>>>> +    if (unlikely(dev_written < 0)) {
>>>> +        return dev_written;
>>>> +    }
>>>> +    if (*s->status != VIRTIO_NET_OK) {
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
>>>> +                                  const VirtIONet *n)
>>>> +{
>>>> +    uint8_t on;
>>>> +    int r;
>>>> +
>>>> +    if (virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) {
>>>
>>> Also suggesting early returns here.
>>
>> So, for CVQ commands related to VIRTIO_NET_F_CTRL_EXTRA_RX, would it be
>> more appropriate to create a new function, maybe
>> vhost_vdpa_net_load_rx_extra, to handle them instead of sending those
>> CVQ commands within this function, if we choose to return early?
>>
>
> My understanding is that VIRTIO_NET_F_CTRL_RX_EXTRA depends on
> VIRTIO_NET_F_CTRL_RX, so we can do:
>
> if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) {
>    return;
> }
>
> // Process CTRL_RX commands
>
> if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) {
>    return;
> }
>
> // process CTRL_RX_EXTRA commands

Thanks for your explanation, it makes sense.

I will send the v3 patch based on your suggestion.

Thanks!


>
>>>
>>>> +        /* Load the promiscous mode */
>>>> +        if (n->mac_table.uni_overflow) {
>>>> +            /*
>>>> +             * According to VirtIO standard, "Since there are no guarantees,
>>>> +             * it can use a hash filter or silently switch to
>>>> +             * allmulti or promiscuous mode if it is given too many addresses."
>>>> +             *
>>>> +             * QEMU ignores non-multicast(unicast) MAC addresses and
>>>> +             * marks `uni_overflow` for the device internal state
>>>> +             * if guest sets too many non-multicast(unicast) MAC addresses.
>>>> +             * Therefore, we should turn promiscous mode on in this case.
>>>> +             */
>>>> +            on = 1;
>>>> +        } else {
>>>> +            on = n->promisc;
>>>> +        }
>>>
>>> I think we can remove the "on" variable and just do:
>>>
>>> /*
>>>    * According to ...
>>>    */
>>> if (n->mac_table.uni_overflow || n->promisc) {
>>>     r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_PROMISC, on);
>>>     if (r < 0) {
>>>       return r;
>>>     }
>>> ---
>>>
>>> And the equivalent for multicast.
>>>
>>> Would that make sense?
>>
>> Yes, I will refactor these according to your suggestion.
>>
>> Thanks!
>>
>>
>>>
>>> Thanks!
>>>
>>>> +        if (on != 1) {
>>>> +            /*
>>>> +             * According to virtio_net_reset(), device turns promiscuous mode on
>>>> +             * by default.
>>>> +             *
>>>> +             * Therefore, there is no need to send this CVQ command if the
>>>> +             * driver also sets promiscuous mode on, which aligns with
>>>> +             * the device's defaults.
>>>> +             *
>>>> +             * Note that the device's defaults can mismatch the driver's
>>>> +             * configuration only at live migration.
>>>> +             */
>>>> +            r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_PROMISC, on);
>>>> +            if (r < 0) {
>>>> +                return r;
>>>> +            }
>>>> +        }
>>>> +
>>>> +        /* Load the all-multicast mode */
>>>> +        if (n->mac_table.multi_overflow) {
>>>> +            /*
>>>> +             * According to VirtIO standard, "Since there are no guarantees,
>>>> +             * it can use a hash filter or silently switch to
>>>> +             * allmulti or promiscuous mode if it is given too many addresses."
>>>> +             *
>>>> +             * QEMU ignores multicast MAC addresses and
>>>> +             * marks `multi_overflow` for the device internal state
>>>> +             * if guest sets too many multicast MAC addresses.
>>>> +             * Therefore, we should turn all-multicast mode on in this case.
>>>> +             */
>>>> +            on = 1;
>>>> +        } else {
>>>> +            on = n->allmulti;
>>>> +        }
>>>> +        if (on != 0) {
>>>> +            /*
>>>> +             * According to virtio_net_reset(), device turns all-multicast mode
>>>> +             * off by default.
>>>> +             *
>>>> +             * Therefore, there is no need to send this CVQ command if the
>>>> +             * driver also sets all-multicast mode off, which aligns with
>>>> +             * the device's defaults.
>>>> +             *
>>>> +             * Note that the device's defaults can mismatch the driver's
>>>> +             * configuration only at live migration.
>>>> +             */
>>>> +            r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_ALLMULTI, on);
>>>> +            if (r < 0) {
>>>> +                return r;
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>>    static int vhost_vdpa_net_load(NetClientState *nc)
>>>>    {
>>>>        VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
>>>> @@ -818,6 +918,10 @@ static int vhost_vdpa_net_load(NetClientState *nc)
>>>>        if (unlikely(r)) {
>>>>            return r;
>>>>        }
>>>> +    r = vhost_vdpa_net_load_rx(s, n);
>>>> +    if (unlikely(r)) {
>>>> +        return r;
>>>> +    }
>>>>
>>>>        return 0;
>>>>    }
>>>> --
>>>> 2.25.1
>>>>
>>>
>>
>

[PATCH RFC v2 4/4] vdpa: Allow VIRTIO_NET_F_CTRL_RX in SVQ

[Hawkins Jiawei]

Enable SVQ with VIRTIO_NET_F_CTRL_RX feature.

Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
Acked-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 9d5d88756c..0410a52043 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -104,6 +104,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_CTRL_RX) |
     BIT_ULL(VIRTIO_NET_F_MQ) |
     BIT_ULL(VIRTIO_F_ANY_LAYOUT) |
     BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR) |
-- 
2.25.1