RE: [PATCH 06/40] net/net_rx_pkt: Use iovec for net_rx_pkt_set_protocols()

[Sriram Yagnaraman]

> -----Original Message-----
> From: Akihiko Odaki <akihiko.odaki@daynix.com>
> Sent: Friday, 14 April 2023 13:37
> Cc: Sriram Yagnaraman <sriram.yagnaraman@est.tech>; Jason Wang
> <jasowang@redhat.com>; Dmitry Fleytman <dmitry.fleytman@gmail.com>;
> Michael S. Tsirkin <mst@redhat.com>; Alex Bennée <alex.bennee@linaro.org>;
> Philippe Mathieu-Daudé <philmd@linaro.org>; Thomas Huth
> <thuth@redhat.com>; Wainer dos Santos Moschetta
> <wainersm@redhat.com>; Beraldo Leal <bleal@redhat.com>; Cleber Rosa
> <crosa@redhat.com>; Laurent Vivier <lvivier@redhat.com>; Paolo Bonzini
> <pbonzini@redhat.com>; qemu-devel@nongnu.org; Akihiko Odaki
> <akihiko.odaki@daynix.com>
> Subject: [PATCH 06/40] net/net_rx_pkt: Use iovec for
> net_rx_pkt_set_protocols()
> 
> igb does not properly ensure the buffer passed to
> net_rx_pkt_set_protocols() is contiguous for the entire L2/L3/L4 header.
> Allow it to pass scattered data to net_rx_pkt_set_protocols().
> 
> Fixes: 3a977deebe ("Intrdocue igb device emulation")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  hw/net/igb_core.c   |  2 +-
>  hw/net/net_rx_pkt.c | 14 +++++---------  hw/net/net_rx_pkt.h | 10 ++++++----
> hw/net/virtio-net.c |  7 +++++--
>  hw/net/vmxnet3.c    |  7 ++++++-
>  include/net/eth.h   |  6 +++---
>  net/eth.c           | 18 ++++++++----------
>  7 files changed, 34 insertions(+), 30 deletions(-)
> 

Very nice. 
Reviewed-by: Sriram Yagnaraman <sriram.yagnaraman@est.tech>

> diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c index
> 5d4884b834..53f60fc3d3 100644
> --- a/hw/net/igb_core.c
> +++ b/hw/net/igb_core.c
> @@ -1650,7 +1650,7 @@ igb_receive_internal(IGBCore *core, const struct
> iovec *iov, int iovcnt,
> 
>      ehdr = PKT_GET_ETH_HDR(filter_buf);
>      net_rx_pkt_set_packet_type(core->rx_pkt, get_eth_packet_type(ehdr));
> -    net_rx_pkt_set_protocols(core->rx_pkt, filter_buf, size);
> +    net_rx_pkt_set_protocols(core->rx_pkt, iov, iovcnt, iov_ofs);
> 
>      queues = igb_receive_assign(core, ehdr, size, &rss_info, external_tx);
>      if (!queues) {
> diff --git a/hw/net/net_rx_pkt.c b/hw/net/net_rx_pkt.c index
> 39cdea06de..63be6e05ad 100644
> --- a/hw/net/net_rx_pkt.c
> +++ b/hw/net/net_rx_pkt.c
> @@ -103,7 +103,7 @@ net_rx_pkt_pull_data(struct NetRxPkt *pkt,
>                                  iov, iovcnt, ploff, pkt->tot_len);
>      }
> 
> -    eth_get_protocols(pkt->vec, pkt->vec_len, &pkt->hasip4, &pkt->hasip6,
> +    eth_get_protocols(pkt->vec, pkt->vec_len, 0, &pkt->hasip4,
> + &pkt->hasip6,
>                        &pkt->l3hdr_off, &pkt->l4hdr_off, &pkt->l5hdr_off,
>                        &pkt->ip6hdr_info, &pkt->ip4hdr_info, 
> &pkt->l4hdr_info);
> 
> @@ -186,17 +186,13 @@ size_t net_rx_pkt_get_total_len(struct NetRxPkt
> *pkt)
>      return pkt->tot_len;
>  }
> 
> -void net_rx_pkt_set_protocols(struct NetRxPkt *pkt, const void *data,
> -                              size_t len)
> +void net_rx_pkt_set_protocols(struct NetRxPkt *pkt,
> +                              const struct iovec *iov, size_t iovcnt,
> +                              size_t iovoff)
>  {
> -    const struct iovec iov = {
> -        .iov_base = (void *)data,
> -        .iov_len = len
> -    };
> -
>      assert(pkt);
> 
> -    eth_get_protocols(&iov, 1, &pkt->hasip4, &pkt->hasip6,
> +    eth_get_protocols(iov, iovcnt, iovoff, &pkt->hasip4, &pkt->hasip6,
>                        &pkt->l3hdr_off, &pkt->l4hdr_off, &pkt->l5hdr_off,
>                        &pkt->ip6hdr_info, &pkt->ip4hdr_info, 
> &pkt->l4hdr_info);  } diff --
> git a/hw/net/net_rx_pkt.h b/hw/net/net_rx_pkt.h index
> d00b484900..a06f5c2675 100644
> --- a/hw/net/net_rx_pkt.h
> +++ b/hw/net/net_rx_pkt.h
> @@ -55,12 +55,14 @@ size_t net_rx_pkt_get_total_len(struct NetRxPkt *pkt);
>   * parse and set packet analysis results
>   *
>   * @pkt:            packet
> - * @data:           pointer to the data buffer to be parsed
> - * @len:            data length
> + * @iov:            received data scatter-gather list
> + * @iovcnt:         number of elements in iov
> + * @iovoff:         data start offset in the iov
>   *
>   */
> -void net_rx_pkt_set_protocols(struct NetRxPkt *pkt, const void *data,
> -                              size_t len);
> +void net_rx_pkt_set_protocols(struct NetRxPkt *pkt,
> +                              const struct iovec *iov, size_t iovcnt,
> +                              size_t iovoff);
> 
>  /**
>   * fetches packet analysis results
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index
> 53e1c32643..37551fd854 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -1835,9 +1835,12 @@ static int virtio_net_process_rss(NetClientState
> *nc, const uint8_t *buf,
>          VIRTIO_NET_HASH_REPORT_UDPv6,
>          VIRTIO_NET_HASH_REPORT_UDPv6_EX
>      };
> +    struct iovec iov = {
> +        .iov_base = (void *)buf,
> +        .iov_len = size
> +    };
> 
> -    net_rx_pkt_set_protocols(pkt, buf + n->host_hdr_len,
> -                             size - n->host_hdr_len);
> +    net_rx_pkt_set_protocols(pkt, &iov, 1, n->host_hdr_len);
>      net_rx_pkt_get_protocols(pkt, &hasip4, &hasip6, &l4hdr_proto);
>      net_hash_type = virtio_net_get_hash_type(hasip4, hasip6, l4hdr_proto,
>                                               n->rss_data.hash_types); diff 
> --git
> a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c index 9acff310e7..05f41b6dfa
> 100644
> --- a/hw/net/vmxnet3.c
> +++ b/hw/net/vmxnet3.c
> @@ -2001,7 +2001,12 @@ vmxnet3_receive(NetClientState *nc, const uint8_t
> *buf, size_t size)
>          get_eth_packet_type(PKT_GET_ETH_HDR(buf)));
> 
>      if (vmxnet3_rx_filter_may_indicate(s, buf, size)) {
> -        net_rx_pkt_set_protocols(s->rx_pkt, buf, size);
> +        struct iovec iov = {
> +            .iov_base = (void *)buf,
> +            .iov_len = size
> +        };
> +
> +        net_rx_pkt_set_protocols(s->rx_pkt, &iov, 1, 0);
>          vmxnet3_rx_need_csum_calculate(s->rx_pkt, buf, size);
>          net_rx_pkt_attach_data(s->rx_pkt, buf, size, s->rx_vlan_stripping);
>          bytes_indicated = vmxnet3_indicate_packet(s) ? size : -1; diff --git
> a/include/net/eth.h b/include/net/eth.h index c5ae4493b4..9f19c3a695
> 100644
> --- a/include/net/eth.h
> +++ b/include/net/eth.h
> @@ -312,10 +312,10 @@ eth_get_l2_hdr_length(const void *p)  }
> 
>  static inline uint32_t
> -eth_get_l2_hdr_length_iov(const struct iovec *iov, int iovcnt)
> +eth_get_l2_hdr_length_iov(const struct iovec *iov, size_t iovcnt,
> +size_t iovoff)
>  {
>      uint8_t p[sizeof(struct eth_header) + sizeof(struct vlan_header)];
> -    size_t copied = iov_to_buf(iov, iovcnt, 0, p, ARRAY_SIZE(p));
> +    size_t copied = iov_to_buf(iov, iovcnt, iovoff, p, ARRAY_SIZE(p));
> 
>      if (copied < ARRAY_SIZE(p)) {
>          return copied;
> @@ -397,7 +397,7 @@ typedef struct eth_l4_hdr_info_st {
>      bool has_tcp_data;
>  } eth_l4_hdr_info;
> 
> -void eth_get_protocols(const struct iovec *iov, int iovcnt,
> +void eth_get_protocols(const struct iovec *iov, size_t iovcnt, size_t
> +iovoff,
>                         bool *hasip4, bool *hasip6,
>                         size_t *l3hdr_off,
>                         size_t *l4hdr_off, diff --git a/net/eth.c b/net/eth.c 
> index
> 70bcd8e355..d7b30df79f 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -136,7 +136,7 @@ _eth_tcp_has_data(bool is_ip4,
>      return l4len > TCP_HEADER_DATA_OFFSET(tcp);  }
> 
> -void eth_get_protocols(const struct iovec *iov, int iovcnt,
> +void eth_get_protocols(const struct iovec *iov, size_t iovcnt, size_t
> +iovoff,
>                         bool *hasip4, bool *hasip6,
>                         size_t *l3hdr_off,
>                         size_t *l4hdr_off, @@ -147,26 +147,24 @@ void
> eth_get_protocols(const struct iovec *iov, int iovcnt,  {
>      int proto;
>      bool fragment = false;
> -    size_t l2hdr_len = eth_get_l2_hdr_length_iov(iov, iovcnt);
>      size_t input_size = iov_size(iov, iovcnt);
>      size_t copied;
>      uint8_t ip_p;
> 
>      *hasip4 = *hasip6 = false;
> +    *l3hdr_off = iovoff + eth_get_l2_hdr_length_iov(iov, iovcnt,
> + iovoff);
>      l4hdr_info->proto = ETH_L4_HDR_PROTO_INVALID;
> 
> -    proto = eth_get_l3_proto(iov, iovcnt, l2hdr_len);
> -
> -    *l3hdr_off = l2hdr_len;
> +    proto = eth_get_l3_proto(iov, iovcnt, *l3hdr_off);
> 
>      if (proto == ETH_P_IP) {
>          struct ip_header *iphdr = &ip4hdr_info->ip4_hdr;
> 
> -        if (input_size < l2hdr_len) {
> +        if (input_size < *l3hdr_off) {
>              return;
>          }
> 
> -        copied = iov_to_buf(iov, iovcnt, l2hdr_len, iphdr, sizeof(*iphdr));
> +        copied = iov_to_buf(iov, iovcnt, *l3hdr_off, iphdr,
> + sizeof(*iphdr));
>          if (copied < sizeof(*iphdr) ||
>              IP_HEADER_VERSION(iphdr) != IP_HEADER_VERSION_4) {
>              return;
> @@ -175,17 +173,17 @@ void eth_get_protocols(const struct iovec *iov, int
> iovcnt,
>          *hasip4 = true;
>          ip_p = iphdr->ip_p;
>          ip4hdr_info->fragment = IP4_IS_FRAGMENT(iphdr);
> -        *l4hdr_off = l2hdr_len + IP_HDR_GET_LEN(iphdr);
> +        *l4hdr_off = *l3hdr_off + IP_HDR_GET_LEN(iphdr);
> 
>          fragment = ip4hdr_info->fragment;
>      } else if (proto == ETH_P_IPV6) {
> -        if (!eth_parse_ipv6_hdr(iov, iovcnt, l2hdr_len, ip6hdr_info)) {
> +        if (!eth_parse_ipv6_hdr(iov, iovcnt, *l3hdr_off, ip6hdr_info))
> + {
>              return;
>          }
> 
>          *hasip6 = true;
>          ip_p = ip6hdr_info->l4proto;
> -        *l4hdr_off = l2hdr_len + ip6hdr_info->full_hdr_len;
> +        *l4hdr_off = *l3hdr_off + ip6hdr_info->full_hdr_len;
>          fragment = ip6hdr_info->fragment;
>      } else {
>          return;
> --
> 2.40.0