Re: [PATCH RFC 1/3] vdpa: Restore MAC address filtering state

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

On Thu, Jun 22, 2023 at 5:02 AM 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>
> ---
>  net/vhost-vdpa.c | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index ecfa8852b5..10264d3e96 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -651,8 +651,45 @@ static int vhost_vdpa_net_load_mac(VhostVDPAState *s, const VirtIONet *n)
>          if (unlikely(dev_written < 0)) {
>              return dev_written;
>          }
> +        if (*s->status != VIRTIO_NET_OK) {
> +            return -EINVAL;
> +        }

I think this part should go in its individual patch, explaining why it
is needed and with corresponding Fixes tag.

> +    }
> +
> +    if (virtio_vdev_has_feature(&n->parent_obj, VIRTIO_NET_F_CTRL_RX)) {
> +        /* Load the MAC address filtering */
> +        uint32_t uni_entries = n->mac_table.first_multi,
> +                 uni_macs_size = uni_entries * ETH_ALEN,
> +                 uni_size = sizeof(struct virtio_net_ctrl_mac) + uni_macs_size,
> +                 mul_entries = n->mac_table.in_use - uni_entries,
> +                 mul_macs_size = mul_entries * ETH_ALEN,
> +                 mul_size = sizeof(struct virtio_net_ctrl_mac) + mul_macs_size,
> +                 data_size = uni_size + mul_size;
> +        void *data = g_malloc(data_size);

If we keep this part, please use g_autofree here [1].

But I think it is not worth copying all the data actually. Maybe it is
worth it to convert vhost_vdpa_net_load_cmd to const iovec?

Thanks!

[1] https://www.qemu.org/docs/master/devel/style#automatic-memory-deallocation

> +        struct virtio_net_ctrl_mac *ctrl_mac;
> +
> +        /* Pack the non-multicast(unicast) MAC addresses */
> +        ctrl_mac = data;
> +        ctrl_mac->entries = cpu_to_le32(uni_entries);
> +        memcpy(ctrl_mac->macs, n->mac_table.macs, uni_macs_size);
> +
> +        /* Pack the multicast MAC addresses */
> +        ctrl_mac = data + uni_size;
> +        ctrl_mac->entries = cpu_to_le32(mul_entries);
> +        memcpy(ctrl_mac->macs, &n->mac_table.macs[uni_macs_size],
> +               mul_macs_size);
> +
> +        ssize_t dev_written = vhost_vdpa_net_load_cmd(s, VIRTIO_NET_CTRL_MAC,
> +                                                  VIRTIO_NET_CTRL_MAC_TABLE_SET,
> +                                                  data, data_size);
> +        g_free(data);
>
> -        return *s->status != VIRTIO_NET_OK;
> +        if (unlikely(dev_written < 0)) {
> +            return dev_written;
> +        }
> +        if (*s->status != VIRTIO_NET_OK) {
> +            return -EINVAL;
> +        }
>      }
>
>      return 0;
> --
> 2.25.1
>