[PATCH v3 0/7] Vhost-vdpa Shadow Virtqueue _F_CTRL_RX commands support

[PATCH v3 0/7] 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] patch "vdpa: Return -EIO if device ack is VIRTIO_NET_ERR".

[1]. https://lore.kernel.org/all/cover.1688438055.git.yin31149@gmail.com/

TestStep
========
1. test the patch series using vp-vdpa device
  - For L0 guest, boot QEMU with virtio-net-pci net device with `ctrl_vq`
and `ctrl_rx` feature on, something like:
      -device virtio-net-pci,rx_queue_size=256,tx_queue_size=256,
iommu_platform=on,ctrl_vq=on,ctrl_rx=on,...

  - For L1 guest, apply the patch series and compile the code,
start QEMU with vdpa device with svq mode and enable the `ctrl_vq`
and `ctrl_rx` feature on, something like:
      -netdev type=vhost-vdpa,x-svq=true,...
      -device virtio-net-pci,ctrl_vq=on,ctrl_rx=on,...

With this series, QEMU should not trigger any error or warning.
Without this series, QEMU should fail with
"vdpa svq does not work with features 0x40000".

2. test the patch "vhost: Fix false positive out-of-bounds"
  - For L0 guest, boot QEMU with virtio-net-pci net device with `ctrl_vq`
and `ctrl_rx` feature on, something like:
      -device virtio-net-pci,rx_queue_size=256,tx_queue_size=256,
iommu_platform=on,ctrl_vq=on,ctrl_rx=on,...

  - For L1 guest, apply the patch series except
patch "vhost: Fix false positive out-of-bounds". start QEMU with vdpa device
with svq mode and enable the `ctrl_vq`
and `ctrl_rx` feature on, something like:
      -netdev type=vhost-vdpa,x-svq=true,...
      -device virtio-net-pci,ctrl_vq=on,ctrl_rx=on,...
  
  - For L2 guest, run the following bash command:
```bash
for idx1 in {0..9}
do
  for idx2 in {0..9}
  do
    for idx3 in {0..6}
    do
      ip link add macvlan$idx1$idx2$idx3 link eth0
address 4a:30:10:19:$idx1$idx2:1$idx3 type macvlan mode bridge
      ip link set macvlan$idx1$idx2$idx3 up
    done
  done
done
```

With the patch "vhost: Fix false positive out-of-bounds", QEMU should not
trigger any error or warning.
Without that patch, QEMU should fail with something like
"free(): double free detected in tcache 2". Note that this UAF will be
solved in another patch.

3. test the patch "vdpa: Avoid forwarding large CVQ command failures"
  - For L0 guest, boot QEMU with virtio-net-pci net device with `ctrl_vq`
and `ctrl_rx` feature on, something like:
      -device virtio-net-pci,rx_queue_size=256,tx_queue_size=256,
iommu_platform=on,ctrl_vq=on,ctrl_rx=on,...

  - For L1 guest, apply the patch series except
patch "vdpa: Avoid forwarding large CVQ command failures". Start QEMU
with vdpa device with svq mode and enable the `ctrl_vq`
and `ctrl_rx` feature on, something like:
      -netdev type=vhost-vdpa,x-svq=true,...
      -device virtio-net-pci,ctrl_vq=on,ctrl_rx=on,...
  
  - For L2 guest, run the following bash command:
```bash
for idx1 in {0..9}
do
  for idx2 in {0..9}
  do
    for idx3 in {0..6}
    do
      ip link add macvlan$idx1$idx2$idx3 link eth0
address 4a:30:10:19:$idx1$idx2:1$idx3 type macvlan mode bridge
      ip link set macvlan$idx1$idx2$idx3 up
    done
  done
done
```

With the patch "vdpa: Avoid forwarding large CVQ command failures",
L2 guest should not trigger any error or warning.
Without that patch, L2 guest should get warning like
"Failed to set Mac filter table.".

4. test the migration
  - For L0 guest, boot QEMU with two virtio-net-pci net device with `ctrl_vq`
and `ctrl_rx` feature on, something like:
      -device virtio-net-pci,rx_queue_size=256,tx_queue_size=256,
iommu_platform=on,ctrl_vq=on,ctrl_rx=on,...

  - For L1 guest, apply the patch series. Start QEMU
with two vdpa device with svq mode and enable the `ctrl_vq`
and `ctrl_rx` feature on, something like:
      -netdev type=vhost-vdpa,x-svq=true,...
      -device virtio-net-pci,ctrl_vq=on,ctrl_rx=on,...
gdb attach the destination VM and break at the
net/vhost-vdpa.c:904
  
  - For L2 source guest, run the following bash command:
```bash
for idx1 in {0..2}
do
  for idx2 in {0..9}
  do
    ip link add macvlan$idx1$idx2 link eth0
address 4a:30:10:19:$idx1$idx2:1a type macvlan mode bridge
    ip link set macvlan$idx1$idx2 up
    done
  done
done
```, then executing the live migration in monitor.

With the patch series, gdb can hit the breakpoint and see those
30 MAC addresses according to `x/180bx n->mac_table.macs`.
Without that patch, QEMU should fail with
"vdpa svq does not work with features 0x40000".


ChangeLog
=========
v3:
  - rename argument name and use iov_to_buf suggested by Eugenio in
patch 1 "vdpa: Use iovec for vhost_vdpa_net_load_cmd()"
  - return early if mismatch the condition suggested by Eugenio in
patch 2 "vdpa: Restore MAC address filtering state" and
patch 3 "vdpa: Restore packet receive filtering state relative with
_F_CTRL_RX feature"
  - remove the `on` variable suggested by Eugenio in patch 3 "vdpa:
Restore packet receive filtering state relative with _F_CTRL_RX feature"
  - fix possible false positive out-of-bounds in patch 4 "vhost:
Fix false positive out-of-bounds"
  - avoid forwarding large CVQ command failures suggested by Eugenio by
patch 5 "vdpa: Accessing CVQ header through its structure" and
patch 6 "vdpa: Avoid forwarding large CVQ command failures"

v2: https://lore.kernel.org/all/cover.1688051252.git.yin31149@gmail.com/
  - 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 (7):
  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
  vhost: Fix false positive out-of-bounds
  vdpa: Accessing CVQ header through its structure
  vdpa: Avoid forwarding large CVQ command failures
  vdpa: Allow VIRTIO_NET_F_CTRL_RX in SVQ

 hw/virtio/vhost-shadow-virtqueue.c |   2 +-
 net/vhost-vdpa.c                   | 338 ++++++++++++++++++++++++++++-
 2 files changed, 329 insertions(+), 11 deletions(-)

-- 
2.25.1

[PATCH v3 1/7] 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>
---
v3:
  - rename argument name to `data_sg` and `data_num`
  - use iov_to_buf() suggested by Eugenio

v2: https://lore.kernel.org/all/6d3dc0fc076564a03501e222ef1102a6a7a643af.1688051252.git.yin31149@gmail.com/
  - refactor vhost_vdpa_load_cmd() to accept iovec suggested by
Eugenio

 net/vhost-vdpa.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 373609216f..31ef6ad6ec 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -620,29 +620,38 @@ 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_sg,
+                                       size_t data_num)
 {
     const struct virtio_net_ctrl_hdr ctrl = {
         .class = class,
         .cmd = cmd,
     };
+    size_t data_size = iov_size(data_sg, data_num);
 
     assert(data_size < vhost_vdpa_net_cvq_cmd_page_len() - sizeof(ctrl));
 
+    /* pack the CVQ command header */
     memcpy(s->cvq_cmd_out_buffer, &ctrl, sizeof(ctrl));
-    memcpy(s->cvq_cmd_out_buffer + sizeof(ctrl), data, data_size);
 
-    return vhost_vdpa_net_cvq_add(s, sizeof(ctrl) + data_size,
+    /* pack the CVQ command command-specific-data */
+    iov_to_buf(data_sg, data_num, 0,
+               s->cvq_cmd_out_buffer + sizeof(ctrl), data_size);
+
+    return vhost_vdpa_net_cvq_add(s, data_size + sizeof(ctrl),
                                   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 +674,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 +719,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 v3 2/7] 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>
---
v3:
  - return early if mismatch the condition suggested by Eugenio

v2: https://lore.kernel.org/all/2f2560f749186c0eb1055f9926f464587e419eeb.1688051252.git.yin31149@gmail.com/
  - 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 | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 31ef6ad6ec..7189ccafaf 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -660,6 +660,58 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
         }
     }
 
+    /*
+     * According to VirtIO standard, "The device MUST have an
+     * empty MAC filtering table on reset.".
+     *
+     * 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.
+     */
+    if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX) ||
+        n->mac_table.in_use == 0) {
+        return 0;
+    }
+
+    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 -EIO;
+    }
+
     return 0;
 }
 
-- 
2.25.1

[PATCH v3 3/7] 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>
---
v3:
  - return early if mismatch the condition suggested by Eugenio
  - remove the `on` variable suggested by Eugenio

v2: https://lore.kernel.org/all/d9d7641ef25d7a4477f8fc4df8cba026380dab76.1688051252.git.yin31149@gmail.com/
  - 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 | 85 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index 7189ccafaf..e80d4b4ef3 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -788,6 +788,87 @@ 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)
+{
+    const struct iovec data = {
+        .iov_base = &on,
+        .iov_len = sizeof(on),
+    };
+    return vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_RX,
+                                   cmd, &data, 1);
+}
+
+static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
+                                  const VirtIONet *n)
+{
+    ssize_t dev_written;
+
+    if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) {
+        return 0;
+    }
+
+    /*
+     * According to virtio_net_reset(), device turns promiscuous mode
+     * on by default.
+     *
+     * Addtionally, 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 marks `n->mac_table.uni_overflow` if guest sets too many
+     * non-multicast MAC addresses, indicating that promiscuous mode
+     * should be enabled.
+     *
+     * Therefore, QEMU should only send this CVQ command if the
+     * `n->mac_table.uni_overflow` is not marked and `n->promisc` is off,
+     * which sets promiscuous mode on, different from the device's defaults.
+     *
+     * Note that the device's defaults can mismatch the driver's
+     * configuration only at live migration.
+     */
+    if (!n->mac_table.uni_overflow && !n->promisc) {
+        dev_written = vhost_vdpa_net_load_rx_mode(s,
+                                            VIRTIO_NET_CTRL_RX_PROMISC, 0);
+        if (unlikely(dev_written < 0)) {
+            return dev_written;
+        }
+        if (*s->status != VIRTIO_NET_OK) {
+            return -EIO;
+        }
+    }
+
+    /*
+     * According to virtio_net_reset(), device turns all-multicast mode
+     * off by default.
+     *
+     * 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 marks
+     * `n->mac_table.multi_overflow` if guest sets too many
+     * non-multicast MAC addresses.
+     *
+     * Therefore, QEMU should only send this CVQ command if the
+     * `n->mac_table.multi_overflow` is marked or `n->allmulti` is on,
+     * which sets all-multicast mode on, different from the device's defaults.
+     *
+     * Note that the device's defaults can mismatch the driver's
+     * configuration only at live migration.
+     */
+    if (n->mac_table.multi_overflow || n->allmulti) {
+        dev_written = vhost_vdpa_net_load_rx_mode(s,
+                                            VIRTIO_NET_CTRL_RX_ALLMULTI, 1);
+        if (unlikely(dev_written < 0)) {
+            return dev_written;
+        }
+        if (*s->status != VIRTIO_NET_OK) {
+            return -EIO;
+        }
+    }
+
+    return 0;
+}
+
 static int vhost_vdpa_net_load(NetClientState *nc)
 {
     VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
@@ -814,6 +895,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 v3 4/7] vhost: Fix false positive out-of-bounds

[Hawkins Jiawei]

QEMU uses vhost_svq_translate_addr() to translate addresses
between the QEMU's virtual address and the SVQ IOVA. In order
to validate this translation, QEMU checks whether the translated
range falls within the mapped range.

Yet the problem is that, the value of `needle_last`, which is calculated
by `needle.translated_addr + iovec[i].iov_len`, should represent the
exclusive boundary of the translated range, rather than the last
inclusive addresses of the range. Consequently, QEMU fails the check
when the translated range matches the size of the mapped range.

This patch solves this problem by fixing the `needle_last` value to
the last inclusive address of the translated range.

Note that this bug cannot be triggered at the moment, because QEMU
is unable to translate such a big range due to the truncation of
the CVQ command in vhost_vdpa_net_handle_ctrl_avail().

Fixes: 34e3c94eda ("vdpa: Add custom IOTLB translations to SVQ")
Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
---
 hw/virtio/vhost-shadow-virtqueue.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
index 1b1d85306c..49e5aed931 100644
--- a/hw/virtio/vhost-shadow-virtqueue.c
+++ b/hw/virtio/vhost-shadow-virtqueue.c
@@ -111,7 +111,7 @@ static bool vhost_svq_translate_addr(const VhostShadowVirtqueue *svq,
         addrs[i] = map->iova + off;
 
         needle_last = int128_add(int128_make64(needle.translated_addr),
-                                 int128_make64(iovec[i].iov_len));
+                                 int128_makes64(iovec[i].iov_len - 1));
         map_last = int128_make64(map->translated_addr + map->size);
         if (unlikely(int128_gt(needle_last, map_last))) {
             qemu_log_mask(LOG_GUEST_ERROR,
-- 
2.25.1

[PATCH v3 5/7] vdpa: Accessing CVQ header through its structure

[Hawkins Jiawei]

We can access the CVQ header through `struct virtio_net_ctrl_hdr`,
instead of accessing it through a `uint8_t` pointer,
which improves the code's readability and maintainability.

Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
---
 net/vhost-vdpa.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index e80d4b4ef3..a84eb088a0 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -928,6 +928,7 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
 {
     VhostVDPAState *s = opaque;
     size_t in_len;
+    const struct virtio_net_ctrl_hdr *ctrl;
     virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
     /* Out buffer sent to both the vdpa device and the device model */
     struct iovec out = {
@@ -943,7 +944,9 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
     out.iov_len = iov_to_buf(elem->out_sg, elem->out_num, 0,
                              s->cvq_cmd_out_buffer,
                              vhost_vdpa_net_cvq_cmd_len());
-    if (*(uint8_t *)s->cvq_cmd_out_buffer == VIRTIO_NET_CTRL_ANNOUNCE) {
+
+    ctrl = s->cvq_cmd_out_buffer;
+    if (ctrl->class == VIRTIO_NET_CTRL_ANNOUNCE) {
         /*
          * Guest announce capability is emulated by qemu, so don't forward to
          * the device.
-- 
2.25.1

[PATCH v3 6/7] vdpa: Avoid forwarding large CVQ command failures

[Hawkins Jiawei]

Due to the size limitation of the out buffer sent to the vdpa device,
which is determined by vhost_vdpa_net_cvq_cmd_len(), excessive CVQ
command is truncated in QEMU. As a result, the vdpa device rejects
this flawd CVQ command.

However, the problem is that, the VIRTIO_NET_CTRL_MAC_TABLE_SET
CVQ command has a variable length, which may exceed
vhost_vdpa_net_cvq_cmd_len() if the guest sets more than
`MAC_TABLE_ENTRIES` MAC addresses for the filter table.

This patch solves this problem by following steps:

  * Increase the out buffer size to vhost_vdpa_net_cvq_cmd_page_len(),
which represents the size of the buffer that is allocated and mmaped.
This ensures that everything works correctly as long as the guest
sets fewer than `(vhost_vdpa_net_cvq_cmd_page_len() -
sizeof(struct virtio_net_ctrl_hdr)
- 2 * sizeof(struct virtio_net_ctrl_mac)) / ETH_ALEN` MAC addresses.
    Considering the highly unlikely scenario for the guest setting
more than that number of MAC addresses for the filter table, this
should work fine for the majority of cases.

  * If the CVQ command exceeds vhost_vdpa_net_cvq_cmd_page_len(),
instead of directly sending this CVQ command, QEMU should send
a VIRTIO_NET_CTRL_RX_PROMISC CVQ command to vdpa device. Addtionally,
a fake VIRTIO_NET_CTRL_MAC_TABLE_SET command including
(`MAC_TABLE_ENTRIES` + 1) non-multicast MAC addresses and
(`MAC_TABLE_ENTRIES` + 1) multicast MAC addresses should be provided
to the device model.
    By doing so, the vdpa device turns promiscuous mode on, aligning
with the VirtIO standard. The device model marks
`n->mac_table.uni_overflow` and `n->mac_table.multi_overflow`,
which aligns with the state of the vdpa device.

Note that the bug cannot be triggered at the moment, since
VIRTIO_NET_F_CTRL_RX feature is not enabled for SVQ.

Fixes: 7a7f87e94c ("vdpa: Move command buffers map to start of net device")
Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
---
 net/vhost-vdpa.c | 162 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 161 insertions(+), 1 deletion(-)

diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
index a84eb088a0..a4ff6c52b7 100644
--- a/net/vhost-vdpa.c
+++ b/net/vhost-vdpa.c
@@ -916,6 +916,148 @@ static NetClientInfo net_vhost_vdpa_cvq_info = {
     .check_peer_type = vhost_vdpa_check_peer_type,
 };
 
+/*
+ * Forward the excessive VIRTIO_NET_CTRL_MAC_TABLE_SET CVQ command to
+ * vdpa device.
+ *
+ * Considering that QEMU cannot send the entire filter table to the
+ * vdpa device, it should send the VIRTIO_NET_CTRL_RX_PROMISC CVQ
+ * command to enable promiscuous mode to receive all packets,
+ * 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.".
+ *
+ * Since QEMU ignores MAC addresses beyond `MAC_TABLE_ENTRIES` and
+ * marks `n->mac_table.x_overflow` accordingly, it should have
+ * the same effect on the device model to receive
+ * (`MAC_TABLE_ENTRIES` + 1) or more non-multicast MAC addresses.
+ * The same applies to multicast MAC addresses.
+ *
+ * Therefore, QEMU can provide the device model with a fake
+ * VIRTIO_NET_CTRL_MAC_TABLE_SET command with (`MAC_TABLE_ENTRIES` + 1)
+ * non-multicast MAC addresses and (`MAC_TABLE_ENTRIES` + 1) multicast
+ * MAC addresses. This ensures that the device model marks
+ * `n->mac_table.uni_overflow` and `n->mac_table.multi_overflow`,
+ * allowing all packets to be received, which aligns with the
+ * state of the vdpa device.
+ */
+static int vhost_vdpa_net_excessive_mac_filter_cvq_add(VhostVDPAState *s,
+                                                       VirtQueueElement *elem,
+                                                       struct iovec *out)
+{
+    struct virtio_net_ctrl_mac mac_data, *mac_ptr;
+    struct virtio_net_ctrl_hdr *hdr_ptr;
+    uint32_t cursor;
+    ssize_t r;
+
+    /* parse the non-multicast MAC address entries from CVQ command */
+    cursor = sizeof(*hdr_ptr);
+    r = iov_to_buf(elem->out_sg, elem->out_num, cursor,
+                   &mac_data, sizeof(mac_data));
+    if (unlikely(r != sizeof(mac_data))) {
+        /*
+         * If the CVQ command is invalid, we should simulate the vdpa device
+         * to reject the VIRTIO_NET_CTRL_MAC_TABLE_SET CVQ command
+         */
+        *s->status = VIRTIO_NET_ERR;
+        return sizeof(*s->status);
+    }
+    cursor += sizeof(mac_data) + le32_to_cpu(mac_data.entries) * ETH_ALEN;
+
+    /* parse the multicast MAC address entries from CVQ command */
+    r = iov_to_buf(elem->out_sg, elem->out_num, cursor,
+                   &mac_data, sizeof(mac_data));
+    if (r != sizeof(mac_data)) {
+        /*
+         * If the CVQ command is invalid, we should simulate the vdpa device
+         * to reject the VIRTIO_NET_CTRL_MAC_TABLE_SET CVQ command
+         */
+        *s->status = VIRTIO_NET_ERR;
+        return sizeof(*s->status);
+    }
+    cursor += sizeof(mac_data) + le32_to_cpu(mac_data.entries) * ETH_ALEN;
+
+    /* validate the CVQ command */
+    if (iov_size(elem->out_sg, elem->out_num) != cursor) {
+        /*
+         * If the CVQ command is invalid, we should simulate the vdpa device
+         * to reject the VIRTIO_NET_CTRL_MAC_TABLE_SET CVQ command
+         */
+        *s->status = VIRTIO_NET_ERR;
+        return sizeof(*s->status);
+    }
+
+    /*
+     * 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.".
+     *
+     * Therefore, considering that QEMU is unable to send the entire
+     * filter table to the vdpa device, it should send the
+     * VIRTIO_NET_CTRL_RX_PROMISC CVQ command to enable promiscuous mode
+     */
+    r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_PROMISC, 1);
+    if (unlikely(r < 0)) {
+        return r;
+    }
+    if (*s->status != VIRTIO_NET_OK) {
+        return sizeof(*s->status);
+    }
+
+    /*
+     * QEMU should also send a fake VIRTIO_NET_CTRL_MAC_TABLE_SET CVQ
+     * command to the device model, including (`MAC_TABLE_ENTRIES` + 1)
+     * non-multicast MAC addresses and (`MAC_TABLE_ENTRIES` + 1)
+     * multicast MAC addresses.
+     *
+     * By doing so, the device model can mark `n->mac_table.uni_overflow`
+     * and `n->mac_table.multi_overflow`, enabling all packets to be
+     * received, which aligns with the state of the vdpa device.
+     */
+    cursor = 0;
+    uint32_t fake_uni_entries = MAC_TABLE_ENTRIES + 1,
+             fake_mul_entries = MAC_TABLE_ENTRIES + 1,
+             fake_cvq_size = sizeof(struct virtio_net_ctrl_hdr) +
+                             sizeof(mac_data) + fake_uni_entries * ETH_ALEN +
+                             sizeof(mac_data) + fake_mul_entries * ETH_ALEN;
+
+    assert(fake_cvq_size < vhost_vdpa_net_cvq_cmd_page_len());
+    out->iov_len = fake_cvq_size;
+
+    /* pack the header for fake CVQ command */
+    hdr_ptr = out->iov_base + cursor;
+    hdr_ptr->class = VIRTIO_NET_CTRL_MAC;
+    hdr_ptr->cmd = VIRTIO_NET_CTRL_MAC_TABLE_SET;
+    cursor += sizeof(*hdr_ptr);
+
+    /*
+     * Pack the non-multicast MAC addresses part for fake CVQ command.
+     *
+     * According to virtio_net_handle_mac(), QEMU doesn't verify the MAC
+     * addresses provieded in CVQ command. Therefore, only the entries
+     * field need to be prepared in the CVQ command.
+     */
+    mac_ptr = out->iov_base + cursor;
+    mac_ptr->entries = cpu_to_le32(fake_uni_entries);
+    cursor += sizeof(*mac_ptr) + fake_uni_entries * ETH_ALEN;
+
+    /*
+     * Pack the multicast MAC addresses part for fake CVQ command.
+     *
+     * According to virtio_net_handle_mac(), QEMU doesn't verify the MAC
+     * addresses provieded in CVQ command. Therefore, only the entries
+     * field need to be prepared in the CVQ command.
+     */
+    mac_ptr = out->iov_base + cursor;
+    mac_ptr->entries = cpu_to_le32(fake_mul_entries);
+
+    /*
+     * Simulating QEMU poll a vdpa device used buffer
+     * for VIRTIO_NET_CTRL_MAC_TABLE_SET CVQ command
+     */
+    return sizeof(*s->status);
+}
+
 /**
  * Validate and copy control virtqueue commands.
  *
@@ -943,7 +1085,7 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
 
     out.iov_len = iov_to_buf(elem->out_sg, elem->out_num, 0,
                              s->cvq_cmd_out_buffer,
-                             vhost_vdpa_net_cvq_cmd_len());
+                             vhost_vdpa_net_cvq_cmd_page_len());
 
     ctrl = s->cvq_cmd_out_buffer;
     if (ctrl->class == VIRTIO_NET_CTRL_ANNOUNCE) {
@@ -953,6 +1095,24 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
          */
         dev_written = sizeof(status);
         *s->status = VIRTIO_NET_OK;
+    } else if (unlikely(ctrl->class == VIRTIO_NET_CTRL_MAC &&
+                        ctrl->cmd == VIRTIO_NET_CTRL_MAC_TABLE_SET &&
+                        iov_size(elem->out_sg, elem->out_num) > out.iov_len)) {
+        /*
+         * Due to the size limitation of the out buffer sent to the vdpa device,
+         * which is determined by vhost_vdpa_net_cvq_cmd_page_len(), excessive
+         * MAC addresses set by the driver for the filter table can cause
+         * truncation of the CVQ command in QEMU. As a result, the vdpa device
+         * rejects the flawed CVQ command.
+         *
+         * Therefore, QEMU must handle this situation instead of sending
+         * the CVQ command direclty.
+         */
+        dev_written = vhost_vdpa_net_excessive_mac_filter_cvq_add(s, elem,
+                                                                  &out);
+        if (unlikely(dev_written < 0)) {
+            goto out;
+        }
     } else {
         dev_written = vhost_vdpa_net_cvq_add(s, out.iov_len, sizeof(status));
         if (unlikely(dev_written < 0)) {
-- 
2.25.1

[PATCH v3 7/7] 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 a4ff6c52b7..0994836f8c 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

Re: [PATCH v3 1/7] vdpa: Use iovec for vhost_vdpa_net_load_cmd()

[Eugenio Perez Martin]

On Fri, Jul 7, 2023 at 5:27 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>
> ---
> v3:
>   - rename argument name to `data_sg` and `data_num`
>   - use iov_to_buf() suggested by Eugenio
>
> v2: https://lore.kernel.org/all/6d3dc0fc076564a03501e222ef1102a6a7a643af.1688051252.git.yin31149@gmail.com/
>   - refactor vhost_vdpa_load_cmd() to accept iovec suggested by
> Eugenio
>
>  net/vhost-vdpa.c | 33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 373609216f..31ef6ad6ec 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -620,29 +620,38 @@ 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_sg,
> +                                       size_t data_num)
>  {
>      const struct virtio_net_ctrl_hdr ctrl = {
>          .class = class,
>          .cmd = cmd,
>      };
> +    size_t data_size = iov_size(data_sg, data_num);
>
>      assert(data_size < vhost_vdpa_net_cvq_cmd_page_len() - sizeof(ctrl));
>
> +    /* pack the CVQ command header */
>      memcpy(s->cvq_cmd_out_buffer, &ctrl, sizeof(ctrl));
> -    memcpy(s->cvq_cmd_out_buffer + sizeof(ctrl), data, data_size);
>
> -    return vhost_vdpa_net_cvq_add(s, sizeof(ctrl) + data_size,
> +    /* pack the CVQ command command-specific-data */
> +    iov_to_buf(data_sg, data_num, 0,
> +               s->cvq_cmd_out_buffer + sizeof(ctrl), data_size);
> +
> +    return vhost_vdpa_net_cvq_add(s, data_size + sizeof(ctrl),

Nit, any reason for changing the order of the addends? sizeof(ctrl) +
data_size ?

>                                    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,

Assign to void should always be valid, no need for casting here.

> +            .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 +674,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 +719,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;
>      }

Apart from that:

Acked-by: Eugenio Pérez <eperezma@redhat.com>

Thanks!

Re: [PATCH v3 2/7] vdpa: Restore MAC address filtering state

[Eugenio Perez Martin]

On Fri, Jul 7, 2023 at 5:27 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>
> ---
> v3:
>   - return early if mismatch the condition suggested by Eugenio
>
> v2: https://lore.kernel.org/all/2f2560f749186c0eb1055f9926f464587e419eeb.1688051252.git.yin31149@gmail.com/
>   - 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 | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 31ef6ad6ec..7189ccafaf 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -660,6 +660,58 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
>          }
>      }
>
> +    /*
> +     * According to VirtIO standard, "The device MUST have an
> +     * empty MAC filtering table on reset.".
> +     *
> +     * 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.
> +     */
> +    if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX) ||
> +        n->mac_table.in_use == 0) {
> +        return 0;
> +    }
> +
> +    uint32_t uni_entries = n->mac_table.first_multi,

QEMU coding style prefers declarations at the beginning of the code
block. Previous uses of these variable names would need to be
refactored to met this rule.

Apart from that,

Acked-by: Eugenio Pérez <eperezma@redhat.com>

> +             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 -EIO;
> +    }
> +
>      return 0;
>  }
>
> --
> 2.25.1
>

Re: [PATCH v3 3/7] vdpa: Restore packet receive filtering state relative with _F_CTRL_RX feature

[Eugenio Perez Martin]

On Fri, Jul 7, 2023 at 5:27 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>

Acked-by: Eugenio Pérez <eperezma@redhat.com>

> ---
> v3:
>   - return early if mismatch the condition suggested by Eugenio
>   - remove the `on` variable suggested by Eugenio
>
> v2: https://lore.kernel.org/all/d9d7641ef25d7a4477f8fc4df8cba026380dab76.1688051252.git.yin31149@gmail.com/
>   - 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 | 85 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 7189ccafaf..e80d4b4ef3 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -788,6 +788,87 @@ 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)
> +{
> +    const struct iovec data = {
> +        .iov_base = &on,
> +        .iov_len = sizeof(on),
> +    };
> +    return vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_RX,
> +                                   cmd, &data, 1);
> +}
> +
> +static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
> +                                  const VirtIONet *n)
> +{
> +    ssize_t dev_written;
> +
> +    if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) {
> +        return 0;
> +    }
> +
> +    /*
> +     * According to virtio_net_reset(), device turns promiscuous mode
> +     * on by default.
> +     *
> +     * Addtionally, 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 marks `n->mac_table.uni_overflow` if guest sets too many
> +     * non-multicast MAC addresses, indicating that promiscuous mode
> +     * should be enabled.
> +     *
> +     * Therefore, QEMU should only send this CVQ command if the
> +     * `n->mac_table.uni_overflow` is not marked and `n->promisc` is off,
> +     * which sets promiscuous mode on, different from the device's defaults.
> +     *
> +     * Note that the device's defaults can mismatch the driver's
> +     * configuration only at live migration.
> +     */
> +    if (!n->mac_table.uni_overflow && !n->promisc) {
> +        dev_written = vhost_vdpa_net_load_rx_mode(s,
> +                                            VIRTIO_NET_CTRL_RX_PROMISC, 0);
> +        if (unlikely(dev_written < 0)) {
> +            return dev_written;
> +        }
> +        if (*s->status != VIRTIO_NET_OK) {
> +            return -EIO;
> +        }
> +    }
> +
> +    /*
> +     * According to virtio_net_reset(), device turns all-multicast mode
> +     * off by default.
> +     *
> +     * 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 marks
> +     * `n->mac_table.multi_overflow` if guest sets too many
> +     * non-multicast MAC addresses.
> +     *
> +     * Therefore, QEMU should only send this CVQ command if the
> +     * `n->mac_table.multi_overflow` is marked or `n->allmulti` is on,
> +     * which sets all-multicast mode on, different from the device's defaults.
> +     *
> +     * Note that the device's defaults can mismatch the driver's
> +     * configuration only at live migration.
> +     */
> +    if (n->mac_table.multi_overflow || n->allmulti) {
> +        dev_written = vhost_vdpa_net_load_rx_mode(s,
> +                                            VIRTIO_NET_CTRL_RX_ALLMULTI, 1);
> +        if (unlikely(dev_written < 0)) {
> +            return dev_written;
> +        }
> +        if (*s->status != VIRTIO_NET_OK) {
> +            return -EIO;
> +        }
> +    }
> +
> +    return 0;
> +}
> +
>  static int vhost_vdpa_net_load(NetClientState *nc)
>  {
>      VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> @@ -814,6 +895,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 v3 4/7] vhost: Fix false positive out-of-bounds

[Eugenio Perez Martin]

On Fri, Jul 7, 2023 at 5:27 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> QEMU uses vhost_svq_translate_addr() to translate addresses
> between the QEMU's virtual address and the SVQ IOVA. In order
> to validate this translation, QEMU checks whether the translated
> range falls within the mapped range.
>
> Yet the problem is that, the value of `needle_last`, which is calculated
> by `needle.translated_addr + iovec[i].iov_len`, should represent the
> exclusive boundary of the translated range, rather than the last
> inclusive addresses of the range. Consequently, QEMU fails the check
> when the translated range matches the size of the mapped range.
>
> This patch solves this problem by fixing the `needle_last` value to
> the last inclusive address of the translated range.
>
> Note that this bug cannot be triggered at the moment, because QEMU
> is unable to translate such a big range due to the truncation of
> the CVQ command in vhost_vdpa_net_handle_ctrl_avail().
>
> Fixes: 34e3c94eda ("vdpa: Add custom IOTLB translations to SVQ")
> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>

Acked-by: Eugenio Pérez <eperezma@redhat.com>

> ---
>  hw/virtio/vhost-shadow-virtqueue.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/virtio/vhost-shadow-virtqueue.c b/hw/virtio/vhost-shadow-virtqueue.c
> index 1b1d85306c..49e5aed931 100644
> --- a/hw/virtio/vhost-shadow-virtqueue.c
> +++ b/hw/virtio/vhost-shadow-virtqueue.c
> @@ -111,7 +111,7 @@ static bool vhost_svq_translate_addr(const VhostShadowVirtqueue *svq,
>          addrs[i] = map->iova + off;
>
>          needle_last = int128_add(int128_make64(needle.translated_addr),
> -                                 int128_make64(iovec[i].iov_len));
> +                                 int128_makes64(iovec[i].iov_len - 1));
>          map_last = int128_make64(map->translated_addr + map->size);
>          if (unlikely(int128_gt(needle_last, map_last))) {
>              qemu_log_mask(LOG_GUEST_ERROR,
> --
> 2.25.1
>

Re: [PATCH v3 5/7] vdpa: Accessing CVQ header through its structure

[Eugenio Perez Martin]

On Fri, Jul 7, 2023 at 5:27 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> We can access the CVQ header through `struct virtio_net_ctrl_hdr`,
> instead of accessing it through a `uint8_t` pointer,
> which improves the code's readability and maintainability.
>
> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>

Acked-by: Eugenio Pérez <eperezma@redhat.com>

> ---
>  net/vhost-vdpa.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index e80d4b4ef3..a84eb088a0 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -928,6 +928,7 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
>  {
>      VhostVDPAState *s = opaque;
>      size_t in_len;
> +    const struct virtio_net_ctrl_hdr *ctrl;
>      virtio_net_ctrl_ack status = VIRTIO_NET_ERR;
>      /* Out buffer sent to both the vdpa device and the device model */
>      struct iovec out = {
> @@ -943,7 +944,9 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
>      out.iov_len = iov_to_buf(elem->out_sg, elem->out_num, 0,
>                               s->cvq_cmd_out_buffer,
>                               vhost_vdpa_net_cvq_cmd_len());
> -    if (*(uint8_t *)s->cvq_cmd_out_buffer == VIRTIO_NET_CTRL_ANNOUNCE) {
> +
> +    ctrl = s->cvq_cmd_out_buffer;
> +    if (ctrl->class == VIRTIO_NET_CTRL_ANNOUNCE) {
>          /*
>           * Guest announce capability is emulated by qemu, so don't forward to
>           * the device.
> --
> 2.25.1
>

Re: [PATCH v3 6/7] vdpa: Avoid forwarding large CVQ command failures

[Eugenio Perez Martin]

On Fri, Jul 7, 2023 at 5:28 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> Due to the size limitation of the out buffer sent to the vdpa device,
> which is determined by vhost_vdpa_net_cvq_cmd_len(), excessive CVQ
> command is truncated in QEMU. As a result, the vdpa device rejects
> this flawd CVQ command.
>
> However, the problem is that, the VIRTIO_NET_CTRL_MAC_TABLE_SET
> CVQ command has a variable length, which may exceed
> vhost_vdpa_net_cvq_cmd_len() if the guest sets more than
> `MAC_TABLE_ENTRIES` MAC addresses for the filter table.
>
> This patch solves this problem by following steps:
>
>   * Increase the out buffer size to vhost_vdpa_net_cvq_cmd_page_len(),
> which represents the size of the buffer that is allocated and mmaped.
> This ensures that everything works correctly as long as the guest
> sets fewer than `(vhost_vdpa_net_cvq_cmd_page_len() -
> sizeof(struct virtio_net_ctrl_hdr)
> - 2 * sizeof(struct virtio_net_ctrl_mac)) / ETH_ALEN` MAC addresses.
>     Considering the highly unlikely scenario for the guest setting
> more than that number of MAC addresses for the filter table, this
> should work fine for the majority of cases.
>
>   * If the CVQ command exceeds vhost_vdpa_net_cvq_cmd_page_len(),
> instead of directly sending this CVQ command, QEMU should send
> a VIRTIO_NET_CTRL_RX_PROMISC CVQ command to vdpa device. Addtionally,
> a fake VIRTIO_NET_CTRL_MAC_TABLE_SET command including
> (`MAC_TABLE_ENTRIES` + 1) non-multicast MAC addresses and
> (`MAC_TABLE_ENTRIES` + 1) multicast MAC addresses should be provided
> to the device model.
>     By doing so, the vdpa device turns promiscuous mode on, aligning
> with the VirtIO standard. The device model marks
> `n->mac_table.uni_overflow` and `n->mac_table.multi_overflow`,
> which aligns with the state of the vdpa device.
>
> Note that the bug cannot be triggered at the moment, since
> VIRTIO_NET_F_CTRL_RX feature is not enabled for SVQ.
>
> Fixes: 7a7f87e94c ("vdpa: Move command buffers map to start of net device")
> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>

Acked-by: Eugenio Pérez <eperezma@redhat.com>

> ---
>  net/vhost-vdpa.c | 162 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 161 insertions(+), 1 deletion(-)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index a84eb088a0..a4ff6c52b7 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -916,6 +916,148 @@ static NetClientInfo net_vhost_vdpa_cvq_info = {
>      .check_peer_type = vhost_vdpa_check_peer_type,
>  };
>
> +/*
> + * Forward the excessive VIRTIO_NET_CTRL_MAC_TABLE_SET CVQ command to
> + * vdpa device.
> + *
> + * Considering that QEMU cannot send the entire filter table to the
> + * vdpa device, it should send the VIRTIO_NET_CTRL_RX_PROMISC CVQ
> + * command to enable promiscuous mode to receive all packets,
> + * 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.".
> + *
> + * Since QEMU ignores MAC addresses beyond `MAC_TABLE_ENTRIES` and
> + * marks `n->mac_table.x_overflow` accordingly, it should have
> + * the same effect on the device model to receive
> + * (`MAC_TABLE_ENTRIES` + 1) or more non-multicast MAC addresses.
> + * The same applies to multicast MAC addresses.
> + *
> + * Therefore, QEMU can provide the device model with a fake
> + * VIRTIO_NET_CTRL_MAC_TABLE_SET command with (`MAC_TABLE_ENTRIES` + 1)
> + * non-multicast MAC addresses and (`MAC_TABLE_ENTRIES` + 1) multicast
> + * MAC addresses. This ensures that the device model marks
> + * `n->mac_table.uni_overflow` and `n->mac_table.multi_overflow`,
> + * allowing all packets to be received, which aligns with the
> + * state of the vdpa device.
> + */
> +static int vhost_vdpa_net_excessive_mac_filter_cvq_add(VhostVDPAState *s,
> +                                                       VirtQueueElement *elem,
> +                                                       struct iovec *out)
> +{
> +    struct virtio_net_ctrl_mac mac_data, *mac_ptr;
> +    struct virtio_net_ctrl_hdr *hdr_ptr;
> +    uint32_t cursor;
> +    ssize_t r;
> +
> +    /* parse the non-multicast MAC address entries from CVQ command */
> +    cursor = sizeof(*hdr_ptr);
> +    r = iov_to_buf(elem->out_sg, elem->out_num, cursor,
> +                   &mac_data, sizeof(mac_data));
> +    if (unlikely(r != sizeof(mac_data))) {
> +        /*
> +         * If the CVQ command is invalid, we should simulate the vdpa device
> +         * to reject the VIRTIO_NET_CTRL_MAC_TABLE_SET CVQ command
> +         */
> +        *s->status = VIRTIO_NET_ERR;
> +        return sizeof(*s->status);
> +    }
> +    cursor += sizeof(mac_data) + le32_to_cpu(mac_data.entries) * ETH_ALEN;
> +
> +    /* parse the multicast MAC address entries from CVQ command */
> +    r = iov_to_buf(elem->out_sg, elem->out_num, cursor,
> +                   &mac_data, sizeof(mac_data));
> +    if (r != sizeof(mac_data)) {
> +        /*
> +         * If the CVQ command is invalid, we should simulate the vdpa device
> +         * to reject the VIRTIO_NET_CTRL_MAC_TABLE_SET CVQ command
> +         */
> +        *s->status = VIRTIO_NET_ERR;
> +        return sizeof(*s->status);
> +    }
> +    cursor += sizeof(mac_data) + le32_to_cpu(mac_data.entries) * ETH_ALEN;
> +
> +    /* validate the CVQ command */
> +    if (iov_size(elem->out_sg, elem->out_num) != cursor) {
> +        /*
> +         * If the CVQ command is invalid, we should simulate the vdpa device
> +         * to reject the VIRTIO_NET_CTRL_MAC_TABLE_SET CVQ command
> +         */
> +        *s->status = VIRTIO_NET_ERR;
> +        return sizeof(*s->status);
> +    }
> +
> +    /*
> +     * 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.".
> +     *
> +     * Therefore, considering that QEMU is unable to send the entire
> +     * filter table to the vdpa device, it should send the
> +     * VIRTIO_NET_CTRL_RX_PROMISC CVQ command to enable promiscuous mode
> +     */
> +    r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_PROMISC, 1);
> +    if (unlikely(r < 0)) {
> +        return r;
> +    }
> +    if (*s->status != VIRTIO_NET_OK) {
> +        return sizeof(*s->status);
> +    }
> +
> +    /*
> +     * QEMU should also send a fake VIRTIO_NET_CTRL_MAC_TABLE_SET CVQ
> +     * command to the device model, including (`MAC_TABLE_ENTRIES` + 1)
> +     * non-multicast MAC addresses and (`MAC_TABLE_ENTRIES` + 1)
> +     * multicast MAC addresses.
> +     *
> +     * By doing so, the device model can mark `n->mac_table.uni_overflow`
> +     * and `n->mac_table.multi_overflow`, enabling all packets to be
> +     * received, which aligns with the state of the vdpa device.
> +     */
> +    cursor = 0;
> +    uint32_t fake_uni_entries = MAC_TABLE_ENTRIES + 1,
> +             fake_mul_entries = MAC_TABLE_ENTRIES + 1,
> +             fake_cvq_size = sizeof(struct virtio_net_ctrl_hdr) +
> +                             sizeof(mac_data) + fake_uni_entries * ETH_ALEN +
> +                             sizeof(mac_data) + fake_mul_entries * ETH_ALEN;
> +
> +    assert(fake_cvq_size < vhost_vdpa_net_cvq_cmd_page_len());
> +    out->iov_len = fake_cvq_size;
> +
> +    /* pack the header for fake CVQ command */
> +    hdr_ptr = out->iov_base + cursor;
> +    hdr_ptr->class = VIRTIO_NET_CTRL_MAC;
> +    hdr_ptr->cmd = VIRTIO_NET_CTRL_MAC_TABLE_SET;
> +    cursor += sizeof(*hdr_ptr);
> +
> +    /*
> +     * Pack the non-multicast MAC addresses part for fake CVQ command.
> +     *
> +     * According to virtio_net_handle_mac(), QEMU doesn't verify the MAC
> +     * addresses provieded in CVQ command. Therefore, only the entries
> +     * field need to be prepared in the CVQ command.
> +     */
> +    mac_ptr = out->iov_base + cursor;
> +    mac_ptr->entries = cpu_to_le32(fake_uni_entries);
> +    cursor += sizeof(*mac_ptr) + fake_uni_entries * ETH_ALEN;
> +
> +    /*
> +     * Pack the multicast MAC addresses part for fake CVQ command.
> +     *
> +     * According to virtio_net_handle_mac(), QEMU doesn't verify the MAC
> +     * addresses provieded in CVQ command. Therefore, only the entries
> +     * field need to be prepared in the CVQ command.
> +     */
> +    mac_ptr = out->iov_base + cursor;
> +    mac_ptr->entries = cpu_to_le32(fake_mul_entries);
> +
> +    /*
> +     * Simulating QEMU poll a vdpa device used buffer
> +     * for VIRTIO_NET_CTRL_MAC_TABLE_SET CVQ command
> +     */
> +    return sizeof(*s->status);
> +}
> +
>  /**
>   * Validate and copy control virtqueue commands.
>   *
> @@ -943,7 +1085,7 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
>
>      out.iov_len = iov_to_buf(elem->out_sg, elem->out_num, 0,
>                               s->cvq_cmd_out_buffer,
> -                             vhost_vdpa_net_cvq_cmd_len());
> +                             vhost_vdpa_net_cvq_cmd_page_len());
>
>      ctrl = s->cvq_cmd_out_buffer;
>      if (ctrl->class == VIRTIO_NET_CTRL_ANNOUNCE) {
> @@ -953,6 +1095,24 @@ static int vhost_vdpa_net_handle_ctrl_avail(VhostShadowVirtqueue *svq,
>           */
>          dev_written = sizeof(status);
>          *s->status = VIRTIO_NET_OK;
> +    } else if (unlikely(ctrl->class == VIRTIO_NET_CTRL_MAC &&
> +                        ctrl->cmd == VIRTIO_NET_CTRL_MAC_TABLE_SET &&
> +                        iov_size(elem->out_sg, elem->out_num) > out.iov_len)) {
> +        /*
> +         * Due to the size limitation of the out buffer sent to the vdpa device,
> +         * which is determined by vhost_vdpa_net_cvq_cmd_page_len(), excessive
> +         * MAC addresses set by the driver for the filter table can cause
> +         * truncation of the CVQ command in QEMU. As a result, the vdpa device
> +         * rejects the flawed CVQ command.
> +         *
> +         * Therefore, QEMU must handle this situation instead of sending
> +         * the CVQ command direclty.
> +         */
> +        dev_written = vhost_vdpa_net_excessive_mac_filter_cvq_add(s, elem,
> +                                                                  &out);
> +        if (unlikely(dev_written < 0)) {
> +            goto out;
> +        }
>      } else {
>          dev_written = vhost_vdpa_net_cvq_add(s, out.iov_len, sizeof(status));
>          if (unlikely(dev_written < 0)) {
> --
> 2.25.1
>

Re: [PATCH v3 1/7] vdpa: Use iovec for vhost_vdpa_net_load_cmd()

[Hawkins Jiawei]

On 2023/8/17 17:23, Eugenio Perez Martin wrote:
> On Fri, Jul 7, 2023 at 5:27 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>
>> ---
>> v3:
>>    - rename argument name to `data_sg` and `data_num`
>>    - use iov_to_buf() suggested by Eugenio
>>
>> v2: https://lore.kernel.org/all/6d3dc0fc076564a03501e222ef1102a6a7a643af.1688051252.git.yin31149@gmail.com/
>>    - refactor vhost_vdpa_load_cmd() to accept iovec suggested by
>> Eugenio
>>
>>   net/vhost-vdpa.c | 33 +++++++++++++++++++++++++--------
>>   1 file changed, 25 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index 373609216f..31ef6ad6ec 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -620,29 +620,38 @@ 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_sg,
>> +                                       size_t data_num)
>>   {
>>       const struct virtio_net_ctrl_hdr ctrl = {
>>           .class = class,
>>           .cmd = cmd,
>>       };
>> +    size_t data_size = iov_size(data_sg, data_num);
>>
>>       assert(data_size < vhost_vdpa_net_cvq_cmd_page_len() - sizeof(ctrl));
>>
>> +    /* pack the CVQ command header */
>>       memcpy(s->cvq_cmd_out_buffer, &ctrl, sizeof(ctrl));
>> -    memcpy(s->cvq_cmd_out_buffer + sizeof(ctrl), data, data_size);
>>
>> -    return vhost_vdpa_net_cvq_add(s, sizeof(ctrl) + data_size,
>> +    /* pack the CVQ command command-specific-data */
>> +    iov_to_buf(data_sg, data_num, 0,
>> +               s->cvq_cmd_out_buffer + sizeof(ctrl), data_size);
>> +
>> +    return vhost_vdpa_net_cvq_add(s, data_size + sizeof(ctrl),
>
> Nit, any reason for changing the order of the addends? sizeof(ctrl) +
> data_size ?

Hi Eugenio,

Here the code should be changed to `sizeof(ctrl) + data_size` as you
point out.

Since this patch series has already been merged into master, I will
submit a separate patch to correct this problem.

>
>>                                     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,
>
> Assign to void should always be valid, no need for casting here.

Yes, assign to void should be valid for normal pointers.

However, `n->mac` is an array and is treated as a const pointer. It will
trigger the warning "error: initialization discards ‘const’ qualifier
from pointer target type" if we don't add this cast.

Thanks!


>
>> +            .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 +674,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 +719,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;
>>       }
>
> Apart from that:
>
> Acked-by: Eugenio Pérez <eperezma@redhat.com>
>
> Thanks!
>

Re: [PATCH v3 2/7] vdpa: Restore MAC address filtering state

[Hawkins Jiawei]

On 2023/8/17 18:18, Eugenio Perez Martin wrote:
> On Fri, Jul 7, 2023 at 5:27 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>
>> ---
>> v3:
>>    - return early if mismatch the condition suggested by Eugenio
>>
>> v2: https://lore.kernel.org/all/2f2560f749186c0eb1055f9926f464587e419eeb.1688051252.git.yin31149@gmail.com/
>>    - 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 | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 52 insertions(+)
>>
>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>> index 31ef6ad6ec..7189ccafaf 100644
>> --- a/net/vhost-vdpa.c
>> +++ b/net/vhost-vdpa.c
>> @@ -660,6 +660,58 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
>>           }
>>       }
>>
>> +    /*
>> +     * According to VirtIO standard, "The device MUST have an
>> +     * empty MAC filtering table on reset.".
>> +     *
>> +     * 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.
>> +     */
>> +    if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX) ||
>> +        n->mac_table.in_use == 0) {
>> +        return 0;
>> +    }
>> +
>> +    uint32_t uni_entries = n->mac_table.first_multi,
>
> QEMU coding style prefers declarations at the beginning of the code
> block. Previous uses of these variable names would need to be
> refactored to met this rule.

Hi Eugenio,

Thanks for the detailed explanation.

Since this patch series has already been merged into master, I will
submit a separate patch to correct this problem.

I will take care of this problem in the future.

Thanks!


>
> Apart from that,
>
> Acked-by: Eugenio Pérez <eperezma@redhat.com>
>
>> +             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 -EIO;
>> +    }
>> +
>>       return 0;
>>   }
>>
>> --
>> 2.25.1
>>
>

Re: [PATCH v3 1/7] vdpa: Use iovec for vhost_vdpa_net_load_cmd()

[Eugenio Perez Martin]

On Thu, Aug 17, 2023 at 2:42 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> On 2023/8/17 17:23, Eugenio Perez Martin wrote:
> > On Fri, Jul 7, 2023 at 5:27 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>
> >> ---
> >> v3:
> >>    - rename argument name to `data_sg` and `data_num`
> >>    - use iov_to_buf() suggested by Eugenio
> >>
> >> v2: https://lore.kernel.org/all/6d3dc0fc076564a03501e222ef1102a6a7a643af.1688051252.git.yin31149@gmail.com/
> >>    - refactor vhost_vdpa_load_cmd() to accept iovec suggested by
> >> Eugenio
> >>
> >>   net/vhost-vdpa.c | 33 +++++++++++++++++++++++++--------
> >>   1 file changed, 25 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> >> index 373609216f..31ef6ad6ec 100644
> >> --- a/net/vhost-vdpa.c
> >> +++ b/net/vhost-vdpa.c
> >> @@ -620,29 +620,38 @@ 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_sg,
> >> +                                       size_t data_num)
> >>   {
> >>       const struct virtio_net_ctrl_hdr ctrl = {
> >>           .class = class,
> >>           .cmd = cmd,
> >>       };
> >> +    size_t data_size = iov_size(data_sg, data_num);
> >>
> >>       assert(data_size < vhost_vdpa_net_cvq_cmd_page_len() - sizeof(ctrl));
> >>
> >> +    /* pack the CVQ command header */
> >>       memcpy(s->cvq_cmd_out_buffer, &ctrl, sizeof(ctrl));
> >> -    memcpy(s->cvq_cmd_out_buffer + sizeof(ctrl), data, data_size);
> >>
> >> -    return vhost_vdpa_net_cvq_add(s, sizeof(ctrl) + data_size,
> >> +    /* pack the CVQ command command-specific-data */
> >> +    iov_to_buf(data_sg, data_num, 0,
> >> +               s->cvq_cmd_out_buffer + sizeof(ctrl), data_size);
> >> +
> >> +    return vhost_vdpa_net_cvq_add(s, data_size + sizeof(ctrl),
> >
> > Nit, any reason for changing the order of the addends? sizeof(ctrl) +
> > data_size ?
>
> Hi Eugenio,
>
> Here the code should be changed to `sizeof(ctrl) + data_size` as you
> point out.
>
> Since this patch series has already been merged into master, I will
> submit a separate patch to correct this problem.
>

Ouch, I didn't realize that. No need to make it back again, I was just
trying to reduce lines changed.

> >
> >>                                     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,
> >
> > Assign to void should always be valid, no need for casting here.
>
> Yes, assign to void should be valid for normal pointers.
>
> However, `n->mac` is an array and is treated as a const pointer. It will
> trigger the warning "error: initialization discards ‘const’ qualifier
> from pointer target type" if we don't add this cast.
>

Got it, I didn't realize it. Everything is ok then.

Thanks!

> Thanks!
>
>
> >
> >> +            .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 +674,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 +719,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;
> >>       }
> >
> > Apart from that:
> >
> > Acked-by: Eugenio Pérez <eperezma@redhat.com>
> >
> > Thanks!
> >
>

Re: [PATCH v3 2/7] vdpa: Restore MAC address filtering state

[Eugenio Perez Martin]

On Thu, Aug 17, 2023 at 2:47 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> On 2023/8/17 18:18, Eugenio Perez Martin wrote:
> > On Fri, Jul 7, 2023 at 5:27 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>
> >> ---
> >> v3:
> >>    - return early if mismatch the condition suggested by Eugenio
> >>
> >> v2: https://lore.kernel.org/all/2f2560f749186c0eb1055f9926f464587e419eeb.1688051252.git.yin31149@gmail.com/
> >>    - 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 | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
> >>   1 file changed, 52 insertions(+)
> >>
> >> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> >> index 31ef6ad6ec..7189ccafaf 100644
> >> --- a/net/vhost-vdpa.c
> >> +++ b/net/vhost-vdpa.c
> >> @@ -660,6 +660,58 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
> >>           }
> >>       }
> >>
> >> +    /*
> >> +     * According to VirtIO standard, "The device MUST have an
> >> +     * empty MAC filtering table on reset.".
> >> +     *
> >> +     * 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.
> >> +     */
> >> +    if (!virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX) ||
> >> +        n->mac_table.in_use == 0) {
> >> +        return 0;
> >> +    }
> >> +
> >> +    uint32_t uni_entries = n->mac_table.first_multi,
> >
> > QEMU coding style prefers declarations at the beginning of the code
> > block. Previous uses of these variable names would need to be
> > refactored to met this rule.
>
> Hi Eugenio,
>
> Thanks for the detailed explanation.
>
> Since this patch series has already been merged into master, I will
> submit a separate patch to correct this problem.
>
> I will take care of this problem in the future.
>

If the maintainer is ok with this, I'm totally ok with leaving the
code as it is right now.

Thanks!

> Thanks!
>
>
> >
> > Apart from that,
> >
> > Acked-by: Eugenio Pérez <eperezma@redhat.com>
> >
> >> +             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 -EIO;
> >> +    }
> >> +
> >>       return 0;
> >>   }
> >>
> >> --
> >> 2.25.1
> >>
> >
>

Re: [PATCH v3 1/7] vdpa: Use iovec for vhost_vdpa_net_load_cmd()

[Hawkins Jiawei]

在 2023/8/17 22:05, Eugenio Perez Martin 写道:
> On Thu, Aug 17, 2023 at 2:42 PM Hawkins Jiawei <yin31149@gmail.com> wrote:
>>
>> On 2023/8/17 17:23, Eugenio Perez Martin wrote:
>>> On Fri, Jul 7, 2023 at 5:27 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>
>>>> ---
>>>> v3:
>>>>     - rename argument name to `data_sg` and `data_num`
>>>>     - use iov_to_buf() suggested by Eugenio
>>>>
>>>> v2: https://lore.kernel.org/all/6d3dc0fc076564a03501e222ef1102a6a7a643af.1688051252.git.yin31149@gmail.com/
>>>>     - refactor vhost_vdpa_load_cmd() to accept iovec suggested by
>>>> Eugenio
>>>>
>>>>    net/vhost-vdpa.c | 33 +++++++++++++++++++++++++--------
>>>>    1 file changed, 25 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
>>>> index 373609216f..31ef6ad6ec 100644
>>>> --- a/net/vhost-vdpa.c
>>>> +++ b/net/vhost-vdpa.c
>>>> @@ -620,29 +620,38 @@ 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_sg,
>>>> +                                       size_t data_num)
>>>>    {
>>>>        const struct virtio_net_ctrl_hdr ctrl = {
>>>>            .class = class,
>>>>            .cmd = cmd,
>>>>        };
>>>> +    size_t data_size = iov_size(data_sg, data_num);
>>>>
>>>>        assert(data_size < vhost_vdpa_net_cvq_cmd_page_len() - sizeof(ctrl));
>>>>
>>>> +    /* pack the CVQ command header */
>>>>        memcpy(s->cvq_cmd_out_buffer, &ctrl, sizeof(ctrl));
>>>> -    memcpy(s->cvq_cmd_out_buffer + sizeof(ctrl), data, data_size);
>>>>
>>>> -    return vhost_vdpa_net_cvq_add(s, sizeof(ctrl) + data_size,
>>>> +    /* pack the CVQ command command-specific-data */
>>>> +    iov_to_buf(data_sg, data_num, 0,
>>>> +               s->cvq_cmd_out_buffer + sizeof(ctrl), data_size);
>>>> +
>>>> +    return vhost_vdpa_net_cvq_add(s, data_size + sizeof(ctrl),
>>>
>>> Nit, any reason for changing the order of the addends? sizeof(ctrl) +
>>> data_size ?
>>
>> Hi Eugenio,
>>
>> Here the code should be changed to `sizeof(ctrl) + data_size` as you
>> point out.
>>
>> Since this patch series has already been merged into master, I will
>> submit a separate patch to correct this problem.
>>
>
> Ouch, I didn't realize that. No need to make it back again, I was just
> trying to reduce lines changed.

Ok, I got it. Regardless, thank you for your review!


>
>>>
>>>>                                      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,
>>>
>>> Assign to void should always be valid, no need for casting here.
>>
>> Yes, assign to void should be valid for normal pointers.
>>
>> However, `n->mac` is an array and is treated as a const pointer. It will
>> trigger the warning "error: initialization discards ‘const’ qualifier
>> from pointer target type" if we don't add this cast.
>>
>
> Got it, I didn't realize it. Everything is ok then.
>
> Thanks!
>
>> Thanks!
>>
>>
>>>
>>>> +            .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 +674,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 +719,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;
>>>>        }
>>>
>>> Apart from that:
>>>
>>> Acked-by: Eugenio Pérez <eperezma@redhat.com>
>>>
>>> Thanks!
>>>
>>
>