Re: [PATCH v2 2/6] vdpa: extract vhost_vdpa_net_load_mac from vhost_vdpa_net_load

[Jason Wang]

On Thu, Aug 25, 2022 at 2:36 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> Since there may be many commands we need to issue to load the NIC
> state, let's split them in individual functions
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> --
> v2: Add vhost_vdpa_net_load_cmd helper
> ---
>  net/vhost-vdpa.c | 54 ++++++++++++++++++++++++++++++++----------------
>  1 file changed, 36 insertions(+), 18 deletions(-)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 468e460ac2..c89e2262d9 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -365,35 +365,31 @@ static ssize_t vhost_vdpa_net_cvq_add(VhostVDPAState 
> *s, size_t out_len,
>      return vhost_svq_poll(svq);
>  }
>
> -static int vhost_vdpa_net_load(NetClientState *nc)
> +static ssize_t vhost_vdpa_net_load_cmd(VhostVDPAState *s,
> +                                       const struct virtio_net_ctrl_hdr 
> *ctrl,
> +                                       const void *data, size_t data_size)
>  {
> -    VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> -    const struct vhost_vdpa *v = &s->vhost_vdpa;
> -    const VirtIONet *n;
> -    uint64_t features;
> +    assert(data_size < vhost_vdpa_net_cvq_cmd_page_len() - sizeof(*ctrl));
>
> -    assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> +    memcpy(s->cvq_cmd_out_buffer, ctrl, sizeof(*ctrl));
> +    memcpy(s->cvq_cmd_out_buffer + sizeof(ctrl), data, data_size);
>
> -    if (!v->shadow_vqs_enabled) {
> -        return 0;
> -    }
> +    return vhost_vdpa_net_cvq_add(s, sizeof(ctrl) + data_size,
> +                                  sizeof(virtio_net_ctrl_ack));
> +}
>
> -    n = VIRTIO_NET(v->dev->vdev);
> -    features = n->parent_obj.guest_features;
> +static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
> +{
> +    uint64_t features = n->parent_obj.guest_features;
>      if (features & BIT_ULL(VIRTIO_NET_F_CTRL_MAC_ADDR)) {
>          const struct virtio_net_ctrl_hdr ctrl = {
>              .class = VIRTIO_NET_CTRL_MAC,
>              .cmd = VIRTIO_NET_CTRL_MAC_ADDR_SET,
>          };
> -        char *cursor = s->cvq_cmd_out_buffer;
>          ssize_t dev_written;
>
> -        memcpy(cursor, &ctrl, sizeof(ctrl));
> -        cursor += sizeof(ctrl);
> -        memcpy(cursor, n->mac, sizeof(n->mac));
> -
> -        dev_written = vhost_vdpa_net_cvq_add(s, sizeof(ctrl) + 
> sizeof(n->mac),
> -                                             sizeof(virtio_net_ctrl_ack));
> +        dev_written = vhost_vdpa_net_load_cmd(s, &ctrl, n->mac,
> +                                              sizeof(n->mac));

Patch looks good, if there's another respin, I'd accept class/cmd as
parameter then there's no need for the caller to prepare
virtio_net_ctrl_hdr.

Thanks

>          if (unlikely(dev_written < 0)) {
>              return dev_written;
>          }
> @@ -404,6 +400,28 @@ static int vhost_vdpa_net_load(NetClientState *nc)
>      return 0;
>  }
>
> +static int vhost_vdpa_net_load(NetClientState *nc)
> +{
> +    VhostVDPAState *s = DO_UPCAST(VhostVDPAState, nc, nc);
> +    struct vhost_vdpa *v = &s->vhost_vdpa;
> +    const VirtIONet *n;
> +    int r;
> +
> +    assert(nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA);
> +
> +    if (!v->shadow_vqs_enabled) {
> +        return 0;
> +    }
> +
> +    n = VIRTIO_NET(v->dev->vdev);
> +    r = vhost_vdpa_net_load_mac(s, n);
> +    if (unlikely(r < 0)) {
> +        return r;
> +    }
> +
> +    return 0;
> +}
> +
>  static NetClientInfo net_vhost_vdpa_cvq_info = {
>      .type = NET_CLIENT_DRIVER_VHOST_VDPA,
>      .size = sizeof(VhostVDPAState),
> --
> 2.31.1
>