Re: [PATCH 2/5] virtio-net: prepare for variable RSS key and indir table lengths

[Michael S. Tsirkin]

On Fri, Apr 15, 2022 at 01:39:04PM +0800, Jason Wang wrote:
> 
> 在 2022/4/8 20:28, Maxime Coquelin 写道:
> > This patch is a preliminary rework to support RSS with
> > Vhost-user backends. It enables supporting different types
> > of hashes, key lengths and indirection table lengths.
> > 
> > This patch does not introduces behavioral changes.
> > 
> > Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > ---
> >   ebpf/ebpf_rss.c                |  8 ++++----
> >   hw/net/virtio-net.c            | 35 +++++++++++++++++++++++++---------
> >   include/hw/virtio/virtio-net.h | 16 +++++++++++++---
> >   include/migration/vmstate.h    | 10 ++++++++++
> >   4 files changed, 53 insertions(+), 16 deletions(-)
> > 
> > diff --git a/ebpf/ebpf_rss.c b/ebpf/ebpf_rss.c
> > index 4a63854175..f03be5f919 100644
> > --- a/ebpf/ebpf_rss.c
> > +++ b/ebpf/ebpf_rss.c
> > @@ -96,7 +96,7 @@ static bool ebpf_rss_set_indirections_table(struct 
> > EBPFRSSContext *ctx,
> >       uint32_t i = 0;
> >       if (!ebpf_rss_is_loaded(ctx) || indirections_table == NULL ||
> > -       len > VIRTIO_NET_RSS_MAX_TABLE_LEN) {
> > +       len > VIRTIO_NET_RSS_DEFAULT_TABLE_LEN) {
> >           return false;
> >       }
> > @@ -116,13 +116,13 @@ static bool ebpf_rss_set_toepliz_key(struct 
> > EBPFRSSContext *ctx,
> >       uint32_t map_key = 0;
> >       /* prepare toeplitz key */
> > -    uint8_t toe[VIRTIO_NET_RSS_MAX_KEY_SIZE] = {};
> > +    uint8_t toe[VIRTIO_NET_RSS_DEFAULT_KEY_SIZE] = {};
> >       if (!ebpf_rss_is_loaded(ctx) || toeplitz_key == NULL ||
> > -            len != VIRTIO_NET_RSS_MAX_KEY_SIZE) {
> > +            len != VIRTIO_NET_RSS_DEFAULT_KEY_SIZE) {
> >           return false;
> >       }
> > -    memcpy(toe, toeplitz_key, VIRTIO_NET_RSS_MAX_KEY_SIZE);
> > +    memcpy(toe, toeplitz_key, VIRTIO_NET_RSS_DEFAULT_KEY_SIZE);
> >       *(uint32_t *)toe = ntohl(*(uint32_t *)toe);
> >       if (bpf_map_update_elem(ctx->map_toeplitz_key, &map_key, toe,
> > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> > index 73145d6390..38436e472b 100644
> > --- a/hw/net/virtio-net.c
> > +++ b/hw/net/virtio-net.c
> > @@ -137,12 +137,11 @@ static void virtio_net_get_config(VirtIODevice *vdev, 
> > uint8_t *config)
> >       memcpy(netcfg.mac, n->mac, ETH_ALEN);
> >       virtio_stl_p(vdev, &netcfg.speed, n->net_conf.speed);
> >       netcfg.duplex = n->net_conf.duplex;
> > -    netcfg.rss_max_key_size = VIRTIO_NET_RSS_MAX_KEY_SIZE;
> > +    netcfg.rss_max_key_size = n->rss_capa.max_key_size;
> >       virtio_stw_p(vdev, &netcfg.rss_max_indirection_table_length,
> > -                 virtio_host_has_feature(vdev, VIRTIO_NET_F_RSS) ?
> > -                 VIRTIO_NET_RSS_MAX_TABLE_LEN : 1);
> > +                 n->rss_capa.max_indirection_len);
> >       virtio_stl_p(vdev, &netcfg.supported_hash_types,
> > -                 VIRTIO_NET_RSS_SUPPORTED_HASHES);
> > +                 n->rss_capa.supported_hashes);
> >       memcpy(config, &netcfg, n->config_size);
> >       /*
> > @@ -1202,7 +1201,7 @@ static bool virtio_net_attach_epbf_rss(VirtIONet *n)
> >       if (!ebpf_rss_set_all(&n->ebpf_rss, &config,
> >                             n->rss_data.indirections_table, n->rss_data.key,
> > -                          VIRTIO_NET_RSS_MAX_KEY_SIZE)) {
> > +                          n->rss_data.key_len)) {
> >           return false;
> >       }
> > @@ -1277,7 +1276,7 @@ static uint16_t virtio_net_handle_rss(VirtIONet *n,
> >           err_value = n->rss_data.indirections_len;
> >           goto error;
> >       }
> > -    if (n->rss_data.indirections_len > VIRTIO_NET_RSS_MAX_TABLE_LEN) {
> > +    if (n->rss_data.indirections_len > n->rss_capa.max_indirection_len) {
> >           err_msg = "Too large indirection table";
> >           err_value = n->rss_data.indirections_len;
> >           goto error;
> > @@ -1323,7 +1322,7 @@ static uint16_t virtio_net_handle_rss(VirtIONet *n,
> >           err_value = queue_pairs;
> >           goto error;
> >       }
> > -    if (temp.b > VIRTIO_NET_RSS_MAX_KEY_SIZE) {
> > +    if (temp.b > n->rss_capa.max_key_size) {
> >           err_msg = "Invalid key size";
> >           err_value = temp.b;
> >           goto error;
> > @@ -1339,6 +1338,14 @@ static uint16_t virtio_net_handle_rss(VirtIONet *n,
> >       }
> >       offset += size_get;
> >       size_get = temp.b;
> > +    n->rss_data.key_len = temp.b;
> > +    g_free(n->rss_data.key);
> > +    n->rss_data.key = g_malloc(size_get);
> > +    if (!n->rss_data.key) {
> > +        err_msg = "Can't allocate key";
> > +        err_value = n->rss_data.key_len;
> > +        goto error;
> > +    }
> >       s = iov_to_buf(iov, iov_cnt, offset, n->rss_data.key, size_get);
> >       if (s != size_get) {
> >           err_msg = "Can get key buffer";
> > @@ -3093,8 +3100,9 @@ static const VMStateDescription 
> > vmstate_virtio_net_rss = {
> >           VMSTATE_UINT32(rss_data.hash_types, VirtIONet),
> >           VMSTATE_UINT16(rss_data.indirections_len, VirtIONet),
> >           VMSTATE_UINT16(rss_data.default_queue, VirtIONet),
> > -        VMSTATE_UINT8_ARRAY(rss_data.key, VirtIONet,
> > -                            VIRTIO_NET_RSS_MAX_KEY_SIZE),
> > +        VMSTATE_VARRAY_UINT8_ALLOC(rss_data.key, VirtIONet,
> > +                                   rss_data.key_len, 0,
> > +                                   vmstate_info_uint8, uint8_t),
> 
> 
> I wonder if we may break the migration compatibility here.
> 
> Thanks


To be more specific, we used to always send VIRTIO_NET_RSS_MAX_KEY_SIZE
elements, now we are sending rss_data.key_len.
Looks like we need to ensure old configs have rss_data.key_len equal
to VIRTIO_NET_RSS_MAX_KEY_SIZE, no?


> 
> >           VMSTATE_VARRAY_UINT16_ALLOC(rss_data.indirections_table, 
> > VirtIONet,
> >                                       rss_data.indirections_len, 0,
> >                                       vmstate_info_uint16, uint16_t),
> > @@ -3523,8 +3531,16 @@ static void virtio_net_device_realize(DeviceState 
> > *dev, Error **errp)
> >       net_rx_pkt_init(&n->rx_pkt, false);
> >       if (virtio_has_feature(n->host_features, VIRTIO_NET_F_RSS)) {
> > +        n->rss_capa.max_key_size = VIRTIO_NET_RSS_DEFAULT_KEY_SIZE;
> > +        n->rss_capa.max_indirection_len = VIRTIO_NET_RSS_DEFAULT_TABLE_LEN;
> > +        n->rss_capa.supported_hashes = VIRTIO_NET_RSS_SUPPORTED_HASHES;
> > +
> >           virtio_net_load_ebpf(n);
> > +    } else {
> > +        n->rss_capa.max_indirection_len = 1;
> >       }
> > +
> > +
> >   }
> >   static void virtio_net_device_unrealize(DeviceState *dev)
> > @@ -3567,6 +3583,7 @@ static void virtio_net_device_unrealize(DeviceState 
> > *dev)
> >       qemu_del_nic(n->nic);
> >       virtio_net_rsc_cleanup(n);
> >       g_free(n->rss_data.indirections_table);
> > +    g_free(n->rss_data.key);
> >       net_rx_pkt_uninit(n->rx_pkt);
> >       virtio_cleanup(vdev);
> >   }
> > diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> > index eb87032627..6794b354ad 100644
> > --- a/include/hw/virtio/virtio-net.h
> > +++ b/include/hw/virtio/virtio-net.h
> > @@ -127,8 +127,16 @@ typedef struct VirtioNetRscChain {
> >   /* Maximum packet size we can receive from tap device: header + 64k */
> >   #define VIRTIO_NET_MAX_BUFSIZE (sizeof(struct virtio_net_hdr) + (64 * 
> > KiB))
> > -#define VIRTIO_NET_RSS_MAX_KEY_SIZE     40
> > -#define VIRTIO_NET_RSS_MAX_TABLE_LEN    128
> > +typedef struct VirtioNetRssCapa {
> > +    uint8_t max_key_size;
> > +    uint16_t max_indirection_len;
> > +    uint32_t supported_hashes;
> > +} VirtioNetRssCapa;
> > +
> > +#define VIRTIO_NET_RSS_MIN_KEY_SIZE      40
> > +#define VIRTIO_NET_RSS_DEFAULT_KEY_SIZE  40
> > +#define VIRTIO_NET_RSS_MIN_TABLE_LEN     128
> > +#define VIRTIO_NET_RSS_DEFAULT_TABLE_LEN 128
> >   typedef struct VirtioNetRssData {
> >       bool    enabled;
> > @@ -136,7 +144,8 @@ typedef struct VirtioNetRssData {
> >       bool    redirect;
> >       bool    populate_hash;
> >       uint32_t hash_types;
> > -    uint8_t key[VIRTIO_NET_RSS_MAX_KEY_SIZE];
> > +    uint8_t key_len;
> > +    uint8_t *key;
> >       uint16_t indirections_len;
> >       uint16_t *indirections_table;
> >       uint16_t default_queue;
> > @@ -213,6 +222,7 @@ struct VirtIONet {
> >       QDict *primary_opts;
> >       bool primary_opts_from_json;
> >       Notifier migration_state;
> > +    VirtioNetRssCapa rss_capa;
> >       VirtioNetRssData rss_data;
> >       struct NetRxPkt *rx_pkt;
> >       struct EBPFRSSContext ebpf_rss;
> > diff --git a/include/migration/vmstate.h b/include/migration/vmstate.h
> > index ad24aa1934..9398cdf803 100644
> > --- a/include/migration/vmstate.h
> > +++ b/include/migration/vmstate.h
> > @@ -448,6 +448,16 @@ extern const VMStateInfo vmstate_info_qlist;
> >       .offset     = vmstate_offset_varray(_state, _field, _type),      \
> >   }
> > +#define VMSTATE_VARRAY_UINT8_ALLOC(_field, _state, _field_num, _version, 
> > _info, _type) {\
> > +    .name       = (stringify(_field)),                               \
> > +    .version_id = (_version),                                        \
> > +    .num_offset = vmstate_offset_value(_state, _field_num, uint8_t),\
> > +    .info       = &(_info),                                          \
> > +    .size       = sizeof(_type),                                     \
> > +    .flags      = VMS_VARRAY_UINT8 | VMS_POINTER | VMS_ALLOC,       \
> > +    .offset     = vmstate_offset_pointer(_state, _field, _type),     \
> > +}
> > +
> >   #define VMSTATE_VSTRUCT_TEST(_field, _state, _test, _version, _vmsd, 
> > _type, _struct_version) { \
> >       .name         = (stringify(_field)),                             \
> >       .version_id   = (_version),                                      \