RE: [PATCH v2 32/41] igb: Implement Rx SCTP CSO

[Sriram Yagnaraman]

> -----Original Message-----
> From: Akihiko Odaki <akihiko.odaki@daynix.com>
> Sent: Thursday, 20 April 2023 07:47
> 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; Tomasz Dzieciol
> <t.dzieciol@partner.samsung.com>; Akihiko Odaki
> <akihiko.odaki@daynix.com>
> Subject: [PATCH v2 32/41] igb: Implement Rx SCTP CSO
> 
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  hw/net/igb_regs.h     |  1 +
>  include/net/eth.h     |  4 ++-
>  include/qemu/crc32c.h |  1 +
>  hw/net/e1000e_core.c  |  5 ++++
>  hw/net/igb_core.c     | 15 +++++++++-
>  hw/net/net_rx_pkt.c   | 64 +++++++++++++++++++++++++++++++++++--------
>  net/eth.c             |  4 +++
>  util/crc32c.c         |  8 ++++++
>  8 files changed, 89 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/net/igb_regs.h b/hw/net/igb_regs.h index
> e6ac26dc0e..4b4ebd3369 100644
> --- a/hw/net/igb_regs.h
> +++ b/hw/net/igb_regs.h
> @@ -670,6 +670,7 @@ union e1000_adv_rx_desc {  #define
> E1000_ADVRXD_PKT_IP6 BIT(6)  #define E1000_ADVRXD_PKT_TCP BIT(8)
> #define E1000_ADVRXD_PKT_UDP BIT(9)
> +#define E1000_ADVRXD_PKT_SCTP BIT(10)
> 
>  static inline uint8_t igb_ivar_entry_rx(uint8_t i)  { diff --git 
> a/include/net/eth.h
> b/include/net/eth.h index 048e434685..75e7f1551c 100644
> --- a/include/net/eth.h
> +++ b/include/net/eth.h
> @@ -224,6 +224,7 @@ struct tcp_hdr {
>  #define IP_HEADER_VERSION_6       (6)
>  #define IP_PROTO_TCP              (6)
>  #define IP_PROTO_UDP              (17)
> +#define IP_PROTO_SCTP             (132)
>  #define IPTOS_ECN_MASK            0x03
>  #define IPTOS_ECN(x)              ((x) & IPTOS_ECN_MASK)
>  #define IPTOS_ECN_CE              0x03
> @@ -379,7 +380,8 @@ typedef struct eth_ip4_hdr_info_st {  typedef enum
> EthL4HdrProto {
>      ETH_L4_HDR_PROTO_INVALID,
>      ETH_L4_HDR_PROTO_TCP,
> -    ETH_L4_HDR_PROTO_UDP
> +    ETH_L4_HDR_PROTO_UDP,
> +    ETH_L4_HDR_PROTO_SCTP
>  } EthL4HdrProto;
> 
>  typedef struct eth_l4_hdr_info_st {
> diff --git a/include/qemu/crc32c.h b/include/qemu/crc32c.h index
> 5b78884c38..88b4d2b3b3 100644
> --- a/include/qemu/crc32c.h
> +++ b/include/qemu/crc32c.h
> @@ -30,5 +30,6 @@
> 
> 
>  uint32_t crc32c(uint32_t crc, const uint8_t *data, unsigned int length);
> +uint32_t iov_crc32c(uint32_t crc, const struct iovec *iov, size_t
> +iov_cnt);
> 
>  #endif
> diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c index
> 27124bba07..8b35735799 100644
> --- a/hw/net/e1000e_core.c
> +++ b/hw/net/e1000e_core.c
> @@ -1114,6 +1114,11 @@ e1000e_verify_csum_in_sw(E1000ECore *core,
>          return;
>      }
> 
> +    if (l4hdr_proto != ETH_L4_HDR_PROTO_TCP &&
> +        l4hdr_proto != ETH_L4_HDR_PROTO_UDP) {
> +        return;
> +    }
> +
>      if (!net_rx_pkt_validate_l4_csum(pkt, &csum_valid)) {
>          trace_e1000e_rx_metadata_l4_csum_validation_failed();
>          return;
> diff --git a/hw/net/igb_core.c b/hw/net/igb_core.c index
> 4dc8e3ae7b..b7f7e765a5 100644
> --- a/hw/net/igb_core.c
> +++ b/hw/net/igb_core.c
> @@ -1212,7 +1212,7 @@ igb_build_rx_metadata(IGBCore *core,
>                        uint16_t *vlan_tag)  {
>      struct virtio_net_hdr *vhdr;
> -    bool hasip4, hasip6;
> +    bool hasip4, hasip6, csum_valid;
>      EthL4HdrProto l4hdr_proto;
> 
>      *status_flags = E1000_RXD_STAT_DD;
> @@ -1272,6 +1272,10 @@ igb_build_rx_metadata(IGBCore *core,
>              *pkt_info |= E1000_ADVRXD_PKT_UDP;
>              break;
> 
> +        case ETH_L4_HDR_PROTO_SCTP:
> +            *pkt_info |= E1000_ADVRXD_PKT_SCTP;
> +            break;
> +
>          default:
>              break;
>          }
> @@ -1304,6 +1308,15 @@ igb_build_rx_metadata(IGBCore *core,
> 
>      if (igb_rx_l4_cso_enabled(core)) {
>          switch (l4hdr_proto) {
> +        case ETH_L4_HDR_PROTO_SCTP:
> +            if (!net_rx_pkt_validate_l4_csum(pkt, &csum_valid)) {

Forgive my naive question, doesn't tap device validate SCTP checksum? Is it 
something we can improve? I can help with adding in linux tap device if you 
think that would make this code simpler, just like UDP/TCP.

> +                trace_e1000e_rx_metadata_l4_csum_validation_failed();
> +                goto func_exit;
> +            }
> +            if (!csum_valid) {
> +                *status_flags |= E1000_RXDEXT_STATERR_TCPE;
> +            }
> +            /* fall through */
>          case ETH_L4_HDR_PROTO_TCP:
>              *status_flags |= E1000_RXD_STAT_TCPCS;
>              break;
> diff --git a/hw/net/net_rx_pkt.c b/hw/net/net_rx_pkt.c index
> 1de42b4f51..3575c8b9f9 100644
> --- a/hw/net/net_rx_pkt.c
> +++ b/hw/net/net_rx_pkt.c
> @@ -16,6 +16,7 @@
>   */
> 
>  #include "qemu/osdep.h"
> +#include "qemu/crc32c.h"
>  #include "trace.h"
>  #include "net_rx_pkt.h"
>  #include "net/checksum.h"
> @@ -554,32 +555,73 @@ _net_rx_pkt_calc_l4_csum(struct NetRxPkt *pkt)
>      return csum;
>  }
> 
> -bool net_rx_pkt_validate_l4_csum(struct NetRxPkt *pkt, bool *csum_valid)
> +static bool
> +_net_rx_pkt_validate_sctp_sum(struct NetRxPkt *pkt)
>  {
> -    uint16_t csum;
> +    size_t csum_off;
> +    size_t off = pkt->l4hdr_off;
> +    size_t vec_len = pkt->vec_len;
> +    struct iovec *vec;
> +    uint32_t calculated = 0;
> +    uint32_t original;
> +    bool valid;
> 
> -    trace_net_rx_pkt_l4_csum_validate_entry();
> +    for (vec = pkt->vec; vec->iov_len < off; vec++) {
> +        off -= vec->iov_len;
> +        vec_len--;
> +    }
> 
> -    if (pkt->l4hdr_info.proto != ETH_L4_HDR_PROTO_TCP &&
> -        pkt->l4hdr_info.proto != ETH_L4_HDR_PROTO_UDP) {
> -        trace_net_rx_pkt_l4_csum_validate_not_xxp();
> +    csum_off = off + 8;
> +
> +    if (!iov_to_buf(vec, vec_len, csum_off, &original,
> + sizeof(original))) {
>          return false;
>      }
> 
> -    if (pkt->l4hdr_info.proto == ETH_L4_HDR_PROTO_UDP &&
> -        pkt->l4hdr_info.hdr.udp.uh_sum == 0) {
> -        trace_net_rx_pkt_l4_csum_validate_udp_with_no_checksum();
> +    if (!iov_from_buf(vec, vec_len, csum_off,
> +                      &calculated, sizeof(calculated))) {
>          return false;
>      }
> 
> +    calculated = crc32c(0xffffffff,
> +                        (uint8_t *)vec->iov_base + off, vec->iov_len - off);
> +    calculated = iov_crc32c(calculated ^ 0xffffffff, vec + 1, vec_len - 1);
> +    valid = calculated == le32_to_cpu(original);
> +    iov_from_buf(vec, vec_len, csum_off, &original, sizeof(original));
> +
> +    return valid;
> +}
> +
> +bool net_rx_pkt_validate_l4_csum(struct NetRxPkt *pkt, bool
> +*csum_valid) {
> +    uint32_t csum;
> +
> +    trace_net_rx_pkt_l4_csum_validate_entry();
> +
>      if (pkt->hasip4 && pkt->ip4hdr_info.fragment) {
>          trace_net_rx_pkt_l4_csum_validate_ip4_fragment();
>          return false;
>      }
> 
> -    csum = _net_rx_pkt_calc_l4_csum(pkt);
> +    switch (pkt->l4hdr_info.proto) {
> +    case ETH_L4_HDR_PROTO_UDP:
> +        if (pkt->l4hdr_info.hdr.udp.uh_sum == 0) {
> +            trace_net_rx_pkt_l4_csum_validate_udp_with_no_checksum();
> +            return false;
> +        }
> +        /* fall through */
> +    case ETH_L4_HDR_PROTO_TCP:
> +        csum = _net_rx_pkt_calc_l4_csum(pkt);
> +        *csum_valid = ((csum == 0) || (csum == 0xFFFF));
> +        break;
> +
> +    case ETH_L4_HDR_PROTO_SCTP:
> +        *csum_valid = _net_rx_pkt_validate_sctp_sum(pkt);
> +        break;
> 
> -    *csum_valid = ((csum == 0) || (csum == 0xFFFF));
> +    default:
> +        trace_net_rx_pkt_l4_csum_validate_not_xxp();
> +        return false;
> +    }
> 
>      trace_net_rx_pkt_l4_csum_validate_csum(*csum_valid);
> 
> diff --git a/net/eth.c b/net/eth.c
> index 5307978486..7f02aea010 100644
> --- a/net/eth.c
> +++ b/net/eth.c
> @@ -211,6 +211,10 @@ void eth_get_protocols(const struct iovec *iov, size_t
> iovcnt, size_t iovoff,
>              *l5hdr_off = *l4hdr_off + sizeof(l4hdr_info->hdr.udp);
>          }
>          break;
> +
> +    case IP_PROTO_SCTP:
> +        l4hdr_info->proto = ETH_L4_HDR_PROTO_SCTP;
> +        break;
>      }
>  }
> 
> diff --git a/util/crc32c.c b/util/crc32c.c index 762657d853..ea7f345de8 100644
> --- a/util/crc32c.c
> +++ b/util/crc32c.c
> @@ -113,3 +113,11 @@ uint32_t crc32c(uint32_t crc, const uint8_t *data,
> unsigned int length)
>      return crc^0xffffffff;
>  }
> 
> +uint32_t iov_crc32c(uint32_t crc, const struct iovec *iov, size_t
> +iov_cnt) {
> +    while (iov_cnt--) {
> +        crc = crc32c(crc, iov->iov_base, iov->iov_len) ^ 0xffffffff;
> +        iov++;
> +    }
> +    return crc ^ 0xffffffff;
> +}
> --
> 2.40.0