Re: [PATCH 1/2] net: TX SCTP checksum offload

[Akihiko Odaki]

On 2023/04/05 23:23, Tomasz Dzieciol wrote:
> Modern NICs are able to offload SCTP checksumming. SCTP calculates
> checksums differently than TCP / UDP: no pseudo-header and CRC32C
> algorithm are used.

Hi,

I actually have some patches to add SCTP checksumming for igb, which are 
currently under tests and I'm going to submit them soon. You can see the 
current version at:
https://github.com/daynix/qemu/commit/bee0bf5792ffbbd7331f6a22d3b498b5685724cd
https://github.com/daynix/qemu/commit/13e061667f40e015c6af2ebb131421042b86a35f

I see some room of improvement in your patches and commented below. As 
they are already addressed in my patches, I'd like to push them forward 
instead. Please review them when they are submitted to the mailing list.

Regards,
Akihiko Odaki

> Signed-off-by: Tomasz Dzieciol <t.dzieciol@partner.samsung.com>
> ---
>   hw/net/net_tx_pkt.c    | 22 ++++++++++++++++++++++
>   hw/net/net_tx_pkt.h    |  8 ++++++++
>   include/net/checksum.h | 12 ++++++++++++
>   include/net/eth.h      |  8 ++++++++
>   net/checksum.c         | 24 ++++++++++++++++++++++++
>   5 files changed, 74 insertions(+)
>
> diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
> index 986a3adfe9..1b97249000 100644
> --- a/hw/net/net_tx_pkt.c
> +++ b/hw/net/net_tx_pkt.c
> @@ -510,6 +510,27 @@ static void net_tx_pkt_do_sw_csum(struct NetTxPkt *pkt,
>       iov_from_buf(iov, iov_len, csum_offset, &csum, sizeof csum);
>   }
>   
> +void net_tx_pkt_update_sctp_crc(struct NetTxPkt *pkt)
> +{
> +    struct iovec *iov = &pkt->vec[NET_TX_PKT_L2HDR_FRAG];
> +    uint32_t csum_cntr = 0;
> +    /* num of iovec without vhdr */
> +    uint32_t iov_len = pkt->payload_frags + NET_TX_PKT_PL_START_FRAG - 1;
> +    size_t csum_offset = pkt->hdr_len + offsetof(struct sctp_hdr, sh_sum);
> +
> +    if (pkt->l4proto != IP_PROTO_SCTP) {
> +        return;
> +    }

It checks for the protocol indicated in the packet, but it should 
actually refer to the respective bit in igb's transmit descriptor. 
Unfortunately the current net_tx_pkt is not implemented in this way for 
other protocols but we don't need to follow that bad habit for a new 
protocol.

> +
> +    /* Put zero to checksum field */
> +    iov_from_buf(iov, iov_len, csum_offset, &csum_cntr, sizeof csum_cntr);
> +
> +    csum_cntr = net_sctp_checksum_add_iov(iov, iov_len,
> +                                          pkt->hdr_len,
> +                                          pkt->payload_len);

Instead of using pkt->hdr_len as the offset, we can simply specify 
pkt->vec[NET_TX_PKT_PL_START_FRAG] as the first iov. Removing the offset 
will simplify net_sctp_checksum_add_iov() a lot.

> +    iov_from_buf(iov, iov_len, csum_offset, &csum_cntr, sizeof csum_cntr);
> +}
> +
>   #define NET_MAX_FRAG_SG_LIST (64)
>   
>   static size_t net_tx_pkt_fetch_fragment(struct NetTxPkt *pkt,
> @@ -846,3 +867,4 @@ void net_tx_pkt_fix_ip6_payload_len(struct NetTxPkt *pkt)
>           }
>       }
>   }
> +
> diff --git a/hw/net/net_tx_pkt.h b/hw/net/net_tx_pkt.h
> index f57b4e034b..56475b462c 100644
> --- a/hw/net/net_tx_pkt.h
> +++ b/hw/net/net_tx_pkt.h
> @@ -204,4 +204,12 @@ bool net_tx_pkt_has_fragments(struct NetTxPkt *pkt);
>    */
>   void net_tx_pkt_fix_ip6_payload_len(struct NetTxPkt *pkt);
>   
> +/**
> +* Update SCTP CRC32C value.
> +*
> +* @pkt:            packet
> +*
> +*/
> +void net_tx_pkt_update_sctp_crc(struct NetTxPkt *pkt);
> +
>   #endif
> diff --git a/include/net/checksum.h b/include/net/checksum.h
> index 7dec37e56c..987d66546d 100644
> --- a/include/net/checksum.h
> +++ b/include/net/checksum.h
> @@ -64,6 +64,18 @@ uint32_t net_checksum_add_iov(const struct iovec *iov,
>                                 uint32_t iov_off, uint32_t size,
>                                 uint32_t csum_offset);
>   
> +/**
> + * net_sctp_checksum_add_iov: scatter-gather vector checksumming for SCTP
> + *
> + * @iov: input scatter-gather array
> + * @iov_cnt: number of array elements
> + * @iov_off: starting iov offset for checksumming
> + * @size: length of data to be checksummed
> + */
> +uint32_t net_sctp_checksum_add_iov(const struct iovec *iov,
> +                                   const unsigned int iov_cnt,
> +                                   uint32_t iov_off, uint32_t size);
> +
>   typedef struct toeplitz_key_st {
>       uint32_t leftmost_32_bits;
>       uint8_t *next_byte;
> diff --git a/include/net/eth.h b/include/net/eth.h
> index c5ae4493b4..a946de1250 100644
> --- a/include/net/eth.h
> +++ b/include/net/eth.h
> @@ -147,6 +147,13 @@ struct ip6_option_hdr {
>       uint8_t len;
>   };
>   
> +struct sctp_hdr {
> +  uint16_t sh_sport;           /* source port */
> +  uint16_t sh_dport;           /* destination port */
> +  uint32_t sh_vtag;            /* verification tag */
> +  uint32_t sh_sum;             /* sctp checksum */
> +};
> +
>   struct udp_hdr {
>     uint16_t uh_sport;           /* source port */
>     uint16_t uh_dport;           /* destination port */
> @@ -222,6 +229,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
> diff --git a/net/checksum.c b/net/checksum.c
> index 68245fd748..4869b1ef14 100644
> --- a/net/checksum.c
> +++ b/net/checksum.c
> @@ -18,6 +18,7 @@
>   #include "qemu/osdep.h"
>   #include "net/checksum.h"
>   #include "net/eth.h"
> +#include "qemu/crc32c.h"
>   
>   uint32_t net_checksum_add_cont(int len, uint8_t *buf, int seq)
>   {
> @@ -206,3 +207,26 @@ net_checksum_add_iov(const struct iovec *iov, const 
> unsigned int iov_cnt,
>       }
>       return res;
>   }
> +
> +uint32_t
> +net_sctp_checksum_add_iov(const struct iovec *iov, const unsigned int iov_cnt,
> +                          uint32_t iov_off, uint32_t size)
> +{
> +    size_t iovec_off;
> +    unsigned int i;
> +    uint32_t res = 0xffffffff;
> +
> +    iovec_off = 0;
> +    for (i = 0; i < iov_cnt && size; i++) {
> +        if (iov_off < (iovec_off + iov[i].iov_len)) {
> +            size_t len = MIN((iovec_off + iov[i].iov_len) - iov_off , size);
> +            void *chunk_buf = iov[i].iov_base + (iov_off - iovec_off);
> +
> +            res = crc32c(res, chunk_buf, len);
> +            iov_off += len;
> +            size -= len;
> +        }
> +        iovec_off += iov[i].iov_len;
> +    }
> +    return res;
> +}

This is a generic function which calculates CRC for iov so I think it 
should be in crc32c.c.