Re: [PATCH v4 7/8] vdpa: Introduce cursors to vhost_vdpa_net_loadx()

[Eugenio Perez Martin <eperezma@redhat.com>]

On Tue, Aug 29, 2023 at 7:55 AM Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> This patch introduces two new arugments, `out_cursor`
> and `in_cursor`, to vhost_vdpa_net_loadx(). Addtionally,
> it includes a helper function
> vhost_vdpa_net_load_cursor_reset() for resetting these
> cursors.
>
> Furthermore, this patch refactors vhost_vdpa_net_load_cmd()
> so that vhost_vdpa_net_load_cmd() prepares buffers
> for the device using the cursors arguments, instead
> of directly accesses `s->cvq_cmd_out_buffer` and
> `s->status` fields.
>
> By making these change, next patches in this series
> can refactor vhost_vdpa_net_load_cmd() directly to
> iterate through the control commands shadow buffers,
> allowing QEMU to send CVQ state load commands in parallel
> at device startup.
>
> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com>
> ---
> v4:
>   - use `struct iovec` instead of `void **` as cursor
> suggested by Eugenio
>   - add vhost_vdpa_net_load_cursor_reset() helper function
> to reset the cursors
>   - refactor vhost_vdpa_net_load_cmd() to prepare buffers
> by iov_copy() instead of accessing `in` and `out` directly
> suggested by Eugenio
>
> v3: https://lore.kernel.org/all/bf390934673f2b613359ea9d7ac6c89199c31384.1689748694.git.yin31149@gmail.com/
>
>  net/vhost-vdpa.c | 114 ++++++++++++++++++++++++++++++++---------------
>  1 file changed, 77 insertions(+), 37 deletions(-)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index d9b8b3cf6c..a71e8c9090 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -633,7 +633,22 @@ static uint16_t vhost_vdpa_net_svq_available_slots(VhostVDPAState *s)
>      return vhost_svq_available_slots(svq);
>  }
>
> -static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, uint8_t class,
> +static void vhost_vdpa_net_load_cursor_reset(VhostVDPAState *s,
> +                                             struct iovec *out_cursor,
> +                                             struct iovec *in_cursor)
> +{
> +    /* reset the cursor of the output buffer for the device */
> +    out_cursor->iov_base = s->cvq_cmd_out_buffer;
> +    out_cursor->iov_len = vhost_vdpa_net_cvq_cmd_page_len();
> +
> +    /* reset the cursor of the in buffer for the device */
> +    in_cursor->iov_base = s->status;
> +    in_cursor->iov_len = vhost_vdpa_net_cvq_cmd_page_len();
> +}
> +
> +static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s,
> +                                       struct iovec *out_cursor,
> +                                       struct iovec *in_cursor, uint8_t class,
>                                         uint8_t cmd, const struct iovec *data_sg,
>                                         size_t data_num)
>  {
> @@ -641,28 +656,25 @@ static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, uint8_t class,
>          .class = class,
>          .cmd = cmd,
>      };
> -    size_t data_size = iov_size(data_sg, data_num);
> -    /* Buffers for the device */
> -    const struct iovec out = {
> -        .iov_base = s->cvq_cmd_out_buffer,
> -        .iov_len = sizeof(ctrl) + data_size,
> -    };
> -    const struct iovec in = {
> -        .iov_base = s->status,
> -        .iov_len = sizeof(*s->status),
> -    };
> +    size_t data_size = iov_size(data_sg, data_num),
> +           cmd_size = sizeof(ctrl) + data_size;
> +    struct iovec out, in;
>      ssize_t r;
>
>      assert(data_size < vhost_vdpa_net_cvq_cmd_page_len() - sizeof(ctrl));
>      /* Each CVQ command has one out descriptor and one in descriptor */
>      assert(vhost_vdpa_net_svq_available_slots(s) >= 2);
>
> -    /* pack the CVQ command header */
> -    memcpy(s->cvq_cmd_out_buffer, &ctrl, sizeof(ctrl));
> +    /* Prepare the buffer for out descriptor for the device */
> +    iov_copy(&out, 1, out_cursor, 1, 0, cmd_size);

I may be missing something here, but cmd_size should be the bytes
available in "out", so we don't overrun it.

> +    /* Prepare the buffer for in descriptor for the device. */
> +    iov_copy(&in, 1, in_cursor, 1, 0, sizeof(*s->status));
>

Same here, although it is impossible for the moment to overrun it as
all cmds only return one byte.

> +    /* pack the CVQ command header */
> +    iov_from_buf(&out, 1, 0, &ctrl, sizeof(ctrl));
>      /* pack the CVQ command command-specific-data */
>      iov_to_buf(data_sg, data_num, 0,
> -               s->cvq_cmd_out_buffer + sizeof(ctrl), data_size);
> +               out.iov_base + sizeof(ctrl), data_size);

Is it possible to replace this by:
iov_to_buf(data_sg, data_num, sizeof(ctrl), out.iov_base, data_size)

In other words, let iov_to_but handle the offset in the buffer instead
of adding it manually.

The rest looks good to me.

>
>      r = vhost_vdpa_net_cvq_add(s, &out, 1, &in, 1);
>      if (unlikely(r < 0)) {
> @@ -676,14 +688,17 @@ static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s, uint8_t class,
>      return vhost_vdpa_net_svq_poll(s, 1);
>  }
>
> -static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
> +static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n,
> +                                   struct iovec *out_cursor,
> +                                   struct iovec *in_cursor)
>  {
>      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,
> +        ssize_t dev_written = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
> +                                                  VIRTIO_NET_CTRL_MAC,
>                                                    VIRTIO_NET_CTRL_MAC_ADDR_SET,
>                                                    &data, 1);
>          if (unlikely(dev_written < 0)) {
> @@ -735,7 +750,7 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
>              .iov_len = mul_macs_size,
>          },
>      };
> -    ssize_t dev_written = vhost_vdpa_net_load_cmd(s,
> +    ssize_t dev_written = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
>                                  VIRTIO_NET_CTRL_MAC,
>                                  VIRTIO_NET_CTRL_MAC_TABLE_SET,
>                                  data, ARRAY_SIZE(data));
> @@ -750,7 +765,9 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
>  }
>
>  static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
> -                                  const VirtIONet *n)
> +                                  const VirtIONet *n,
> +                                  struct iovec *out_cursor,
> +                                  struct iovec *in_cursor)
>  {
>      struct virtio_net_ctrl_mq mq;
>      ssize_t dev_written;
> @@ -764,7 +781,8 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
>          .iov_base = &mq,
>          .iov_len = sizeof(mq),
>      };
> -    dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_MQ,
> +    dev_written = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
> +                                          VIRTIO_NET_CTRL_MQ,
>                                            VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET,
>                                            &data, 1);
>      if (unlikely(dev_written < 0)) {
> @@ -778,7 +796,9 @@ static int vhost_vdpa_net_load_mq(VhostVDPAState *s,
>  }
>
>  static int vhost_vdpa_net_load_offloads(VhostVDPAState *s,
> -                                        const VirtIONet *n)
> +                                        const VirtIONet *n,
> +                                        struct iovec *out_cursor,
> +                                        struct iovec *in_cursor)
>  {
>      uint64_t offloads;
>      ssize_t dev_written;
> @@ -809,7 +829,8 @@ static int vhost_vdpa_net_load_offloads(VhostVDPAState *s,
>          .iov_base = &offloads,
>          .iov_len = sizeof(offloads),
>      };
> -    dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_GUEST_OFFLOADS,
> +    dev_written = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
> +                                          VIRTIO_NET_CTRL_GUEST_OFFLOADS,
>                                            VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET,
>                                            &data, 1);
>      if (unlikely(dev_written < 0)) {
> @@ -823,6 +844,8 @@ static int vhost_vdpa_net_load_offloads(VhostVDPAState *s,
>  }
>
>  static int vhost_vdpa_net_load_rx_mode(VhostVDPAState *s,
> +                                       struct iovec *out_cursor,
> +                                       struct iovec *in_cursor,
>                                         uint8_t cmd,
>                                         uint8_t on)
>  {
> @@ -832,7 +855,8 @@ static int vhost_vdpa_net_load_rx_mode(VhostVDPAState *s,
>      };
>      ssize_t dev_written;
>
> -    dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_RX,
> +    dev_written = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
> +                                          VIRTIO_NET_CTRL_RX,
>                                            cmd, &data, 1);
>      if (unlikely(dev_written < 0)) {
>          return dev_written;
> @@ -845,7 +869,9 @@ static int vhost_vdpa_net_load_rx_mode(VhostVDPAState *s,
>  }
>
>  static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
> -                                  const VirtIONet *n)
> +                                  const VirtIONet *n,
> +                                  struct iovec *out_cursor,
> +                                  struct iovec *in_cursor)
>  {
>      ssize_t r;
>
> @@ -872,7 +898,8 @@ static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
>       * configuration only at live migration.
>       */
>      if (!n->mac_table.uni_overflow && !n->promisc) {
> -        r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_PROMISC, 0);
> +        r = vhost_vdpa_net_load_rx_mode(s, out_cursor, in_cursor,
> +                                        VIRTIO_NET_CTRL_RX_PROMISC, 0);
>          if (unlikely(r < 0)) {
>              return r;
>          }
> @@ -896,7 +923,8 @@ static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
>       * configuration only at live migration.
>       */
>      if (n->mac_table.multi_overflow || n->allmulti) {
> -        r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_ALLMULTI, 1);
> +        r = vhost_vdpa_net_load_rx_mode(s, out_cursor, in_cursor,
> +                                        VIRTIO_NET_CTRL_RX_ALLMULTI, 1);
>          if (unlikely(r < 0)) {
>              return r;
>          }
> @@ -917,7 +945,8 @@ static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
>       * configuration only at live migration.
>       */
>      if (n->alluni) {
> -        r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_ALLUNI, 1);
> +        r = vhost_vdpa_net_load_rx_mode(s, out_cursor, in_cursor,
> +                                        VIRTIO_NET_CTRL_RX_ALLUNI, 1);
>          if (r < 0) {
>              return r;
>          }
> @@ -934,7 +963,8 @@ static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
>       * configuration only at live migration.
>       */
>      if (n->nomulti) {
> -        r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_NOMULTI, 1);
> +        r = vhost_vdpa_net_load_rx_mode(s, out_cursor, in_cursor,
> +                                        VIRTIO_NET_CTRL_RX_NOMULTI, 1);
>          if (r < 0) {
>              return r;
>          }
> @@ -951,7 +981,8 @@ static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
>       * configuration only at live migration.
>       */
>      if (n->nouni) {
> -        r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_NOUNI, 1);
> +        r = vhost_vdpa_net_load_rx_mode(s, out_cursor, in_cursor,
> +                                        VIRTIO_NET_CTRL_RX_NOUNI, 1);
>          if (r < 0) {
>              return r;
>          }
> @@ -968,7 +999,8 @@ static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
>       * configuration only at live migration.
>       */
>      if (n->nobcast) {
> -        r = vhost_vdpa_net_load_rx_mode(s, VIRTIO_NET_CTRL_RX_NOBCAST, 1);
> +        r = vhost_vdpa_net_load_rx_mode(s, out_cursor, in_cursor,
> +                                        VIRTIO_NET_CTRL_RX_NOBCAST, 1);
>          if (r < 0) {
>              return r;
>          }
> @@ -979,13 +1011,16 @@ static int vhost_vdpa_net_load_rx(VhostVDPAState *s,
>
>  static int vhost_vdpa_net_load_single_vlan(VhostVDPAState *s,
>                                             const VirtIONet *n,
> +                                           struct iovec *out_cursor,
> +                                           struct iovec *in_cursor,
>                                             uint16_t vid)
>  {
>      const struct iovec data = {
>          .iov_base = &vid,
>          .iov_len = sizeof(vid),
>      };
> -    ssize_t dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_VLAN,
> +    ssize_t dev_written = vhost_vdpa_net_load_cmd(s, out_cursor, in_cursor,
> +                                                  VIRTIO_NET_CTRL_VLAN,
>                                                    VIRTIO_NET_CTRL_VLAN_ADD,
>                                                    &data, 1);
>      if (unlikely(dev_written < 0)) {
> @@ -999,7 +1034,9 @@ static int vhost_vdpa_net_load_single_vlan(VhostVDPAState *s,
>  }
>
>  static int vhost_vdpa_net_load_vlan(VhostVDPAState *s,
> -                                    const VirtIONet *n)
> +                                    const VirtIONet *n,
> +                                    struct iovec *out_cursor,
> +                                    struct iovec *in_cursor)
>  {
>      int r;
>
> @@ -1010,7 +1047,8 @@ static int vhost_vdpa_net_load_vlan(VhostVDPAState *s,
>      for (int i = 0; i < MAX_VLAN >> 5; i++) {
>          for (int j = 0; n->vlans[i] && j <= 0x1f; j++) {
>              if (n->vlans[i] & (1U << j)) {
> -                r = vhost_vdpa_net_load_single_vlan(s, n, (i << 5) + j);
> +                r = vhost_vdpa_net_load_single_vlan(s, n, out_cursor,
> +                                                    in_cursor, (i << 5) + j);
>                  if (unlikely(r != 0)) {
>                      return r;
>                  }
> @@ -1027,6 +1065,7 @@ static int vhost_vdpa_net_load(NetClientState *nc)
>      struct vhost_vdpa *v = &s->vhost_vdpa;
>      const VirtIONet *n;
>      int r;
> +    struct iovec out_cursor, in_cursor;
>
>      assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
>
> @@ -1035,23 +1074,24 @@ static int vhost_vdpa_net_load(NetClientState *nc)
>      }
>
>      n = VIRTIO_NET(v->dev->vdev);
> -    r = vhost_vdpa_net_load_mac(s, n);
> +    vhost_vdpa_net_load_cursor_reset(s, &out_cursor, &in_cursor);
> +    r = vhost_vdpa_net_load_mac(s, n, &out_cursor, &in_cursor);
>      if (unlikely(r < 0)) {
>          return r;
>      }
> -    r = vhost_vdpa_net_load_mq(s, n);
> +    r = vhost_vdpa_net_load_mq(s, n, &out_cursor, &in_cursor);
>      if (unlikely(r)) {
>          return r;
>      }
> -    r = vhost_vdpa_net_load_offloads(s, n);
> +    r = vhost_vdpa_net_load_offloads(s, n, &out_cursor, &in_cursor);
>      if (unlikely(r)) {
>          return r;
>      }
> -    r = vhost_vdpa_net_load_rx(s, n);
> +    r = vhost_vdpa_net_load_rx(s, n, &out_cursor, &in_cursor);
>      if (unlikely(r)) {
>          return r;
>      }
> -    r = vhost_vdpa_net_load_vlan(s, n);
> +    r = vhost_vdpa_net_load_vlan(s, n, &out_cursor, &in_cursor);
>      if (unlikely(r)) {
>          return r;
>      }
> --
> 2.25.1
>