From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: vio(4): tso
To: jan@openbsd.org
Cc: Brian Conway <bconway@rcesoftware.com>, tech@openbsd.org
Date: Tue, 9 Apr 2024 16:31:10 +0200

On Tue, Apr 09, 2024 at 02:51:27PM +0200, jan@openbsd.org wrote:
> ok?

Some remarks.

> +void
> +vio_tx_offload(struct virtio_net_hdr *hdr, struct mbuf *m)
> +{
> +	struct ether_extracted ext;
> +
> +	/*
> +	 * Checksum Offload
> +	 */
> +
> +	if (!ISSET(m->m_pkthdr.csum_flags, M_TCP_CSUM_OUT) &&
> +	    !ISSET(m->m_pkthdr.csum_flags, M_UDP_CSUM_OUT))
> +		return;
> +
> +	ether_extract_headers(m, &ext);
> +
> +	/* Consistency Checks */
> +	if ((!ext.ip4 && !ext.ip6) || (!ext.tcp && !ext.udp))
> +		return;
> +
> +	if (ISSET(m->m_pkthdr.csum_flags, M_TCP_CSUM_OUT) &&
> +	    ISSET(m->m_pkthdr.csum_flags, M_UDP_CSUM_OUT))
> +		return;

I think this M_TCP_CSUM_OUT M_UDP_CSUM_OUT check is not necessary.

> +
> +	if ((ext.tcp && !ISSET(m->m_pkthdr.csum_flags, M_TCP_CSUM_OUT)) ||
> +	    (ext.udp && !ISSET(m->m_pkthdr.csum_flags, M_UDP_CSUM_OUT)))
> +		return;
> +
> +	if (ext.udp && ISSET(m->m_pkthdr.csum_flags, M_TCP_TSO)) {
> +		tcpstat_inc(tcps_outbadtso);
> +		return;
> +	}

This M_TCP_TSO block should check for !ext.tcp instead of ext.udp.
And it can be moved down.  Then checksum offloading works even if
TSO is inconsistent.

> +
> +	hdr->csum_start = sizeof(*ext.eh);
> +#if NVLAN > 0
> +	if (ext.evh)
> +		hdr->csum_start = sizeof(*ext.evh);
> +#endif
> +	hdr->csum_start += ext.iphlen;
> +
> +	if (ext.tcp)
> +		hdr->csum_offset = offsetof(struct tcphdr, th_sum);
> +	else if (ext.udp)
> +		hdr->csum_offset = offsetof(struct udphdr, uh_sum);
> +
> +	hdr->flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
> +
> +	/*
> +	 * TCP Segmentation Offload
> +	 */
> +
> +	if (!ISSET(m->m_pkthdr.csum_flags, M_TCP_TSO))
> +		return;
> +

If you add this block here, I think all cases are covered.

	if (!ext.tcp) {
		tcpstat_inc(tcps_outbadtso);
		return;
	}

> +	hdr->hdr_len = hdr->csum_start + ext.tcphlen;
> +	hdr->gso_size = m->m_pkthdr.ph_mss;
> +
> +	if (ext.ip4)
> +		hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
> +#ifdef INET6
> +	else if (ext.ip6)
> +		hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
> +#endif
> +
> +	/* VirtIO-Net need pseudo header cksum with IP-payload length for TSO */
> +	ext.tcp->th_sum = vio_cksum_update(ext.tcp->th_sum,
> +	    htons(ext.iplen - ext.iphlen));
> +
> +	tcpstat_add(tcps_outpkttso,
> +	    (ext.paylen + m->m_pkthdr.ph_mss - 1) / m->m_pkthdr.ph_mss);
> +}
> +
>  void
>  vio_start(struct ifnet *ifp)
>  {
> @@ -750,24 +843,7 @@ again:
>  
>  		hdr = &sc->sc_tx_hdrs[slot];
>  		memset(hdr, 0, sc->sc_hdr_size);
> -		if (m->m_pkthdr.csum_flags & (M_TCP_CSUM_OUT|M_UDP_CSUM_OUT)) {
> -			struct ether_extracted ext;
> -
> -			ether_extract_headers(m, &ext);
> -			hdr->csum_start = sizeof(*ext.eh);
> -#if NVLAN > 0
> -			if (ext.evh)
> -				hdr->csum_start = sizeof(*ext.evh);
> -#endif
> -			if (m->m_pkthdr.csum_flags & M_TCP_CSUM_OUT)
> -				hdr->csum_offset = offsetof(struct tcphdr, th_sum);
> -			else
> -				hdr->csum_offset = offsetof(struct udphdr, uh_sum);
> -
> -			if (ext.ip4 || ext.ip6)
> -				hdr->csum_start += ext.iphlen;
> -			hdr->flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
> -		}
> +		vio_tx_offload(hdr, m);
>  
>  		r = vio_encap(sc, slot, m);
>  		if (r != 0) {