Re: [PATCH] vhost/net: Fill virtio_net_hdr GSO/csum metadata on RX

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jul 13, 2026 at 09:04:42AM +0800, weimin xiong wrote:
> From: xiongweimin <xiongweimin@kylinos.cn>
> 
> When VHOST_NET_F_VIRTIO_NET_HDR is set, vhost supplies virtio_net_hdr to
> the guest but previously always wrote a zeroed header (GSO_NONE). Guests
> that rely on GUEST_TSO*/GUEST_CSUM therefore never saw offload metadata.

Right. Question is why are you using VHOST_NET_F_VIRTIO_NET_HDR?

> 
> Peek the socket skb before recvmsg and populate the header with
> virtio_net_hdr_from_skb(). Also advertise the corresponding guest offload
> feature bits from VHOST_GET_FEATURES.
> 
> TX TSO toward backends without IFF_VNET_HDR is intentionally left for a
> follow-up series.
> 
> Signed-off-by: xiongweimin <xiongweimin@kylinos.cn>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: virtualization@vger.kernel.org
> Cc: netdev@vger.kernel.org
> ---
> 
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -73,6 +73,10 @@
>  	VHOST_FEATURES,
>  	VHOST_NET_F_VIRTIO_NET_HDR,
>  	VIRTIO_NET_F_MRG_RXBUF,
> +	VIRTIO_NET_F_GUEST_CSUM,
> +	VIRTIO_NET_F_GUEST_TSO4,
> +	VIRTIO_NET_F_GUEST_TSO6,
> +	VIRTIO_NET_F_GUEST_ECN,
>  	VIRTIO_F_ACCESS_PLATFORM,
>  	VIRTIO_F_RING_RESET,
>  	VIRTIO_F_IN_ORDER,
> @@ -644,7 +648,7 @@
>  static size_t init_iov_iter(struct vhost_virtqueue *vq, struct iov_iter *iter,
>  			    size_t hdr_size, int out)
>  {
> -	/* Skip header. TODO: support TSO. */
> +	/* Skip guest virtio_net_hdr; TX TSO handled in a follow-up. */
>  	size_t len = iov_length(vq->iov, out);
>  
>  	iov_iter_init(iter, ITER_SOURCE, vq->iov, out, len);
> @@ -1025,6 +1029,35 @@
>  	return len;
>  }
>  
> +/*
> + * When VHOST_NET_F_VIRTIO_NET_HDR is set, vhost supplies virtio_net_hdr.
> + * Populate GSO/checksum metadata from the socket skb so guests that
> + * negotiated GUEST_TSO*/GUEST_CSUM receive correct offload information.
> + */

this is a wrong type of multiline comment. this file follows net
convention:

/* AAA
 * BBB
 */

> +static int vhost_net_hdr_from_sock(struct vhost_virtqueue *vq, struct sock *sk,
> +				   struct virtio_net_hdr *hdr)
> +{
> +	struct sk_buff *skb;
> +	unsigned long flags;
> +	int vlan_hlen = 0;
> +	int ret;
> +
> +	spin_lock_irqsave(&sk->sk_receive_queue.lock, flags);
> +	skb = skb_peek(&sk->sk_receive_queue);
> +	if (!skb) {
> +		spin_unlock_irqrestore(&sk->sk_receive_queue.lock, flags);
> +		memset(hdr, 0, sizeof(*hdr));
> +		hdr->gso_type = VIRTIO_NET_HDR_GSO_NONE;
> +		return 0;
> +	}
> +	if (skb_vlan_tag_present(skb))
> +		vlan_hlen = VLAN_HLEN;
> +	ret = virtio_net_hdr_from_skb(skb, hdr, vhost_is_little_endian(vq),
> +				      true, vlan_hlen);
> +	spin_unlock_irqrestore(&sk->sk_receive_queue.lock, flags);
> +	return ret;
> +}


This means the header will be wrong if something consumes
the skb after we drop the lock, no?

That's why in the end we put the header filling logic
in tun, it can avoid races there.


> +
>  static int vhost_net_rx_peek_head_len(struct vhost_net *net, struct sock *sk,
>  				      bool *busyloop_intr, unsigned int *count)
>  {
> @@ -1239,10 +1272,18 @@
>  		/* We don't need to be notified again. */
>  		iov_iter_init(&msg.msg_iter, ITER_DEST, vq->iov, in, vhost_len);
>  		fixup = msg.msg_iter;
> -		if (unlikely((vhost_hlen))) {
> -			/* We will supply the header ourselves
> -			 * TODO: support TSO.
> +		if (unlikely(vhost_hlen)) {
> +			/*
> +			 * Build virtio_net_hdr from the socket skb before
> +			 * recvmsg consumes it. Skip for ptr_ring backends
> +			 * where the skb is not on sk_receive_queue.
>  			 */
> +			if (!nvq->rx_ring &&
> +			    vhost_net_hdr_from_sock(vq, sock->sk, &hdr)) {
> +				vq_err(vq, "Failed to build vnet_hdr from skb\n");
> +				vhost_discard_vq_desc(vq, headcount, ndesc);
> +				continue;
> +			}
>  			iov_iter_advance(&msg.msg_iter, vhost_hlen);
>  		}
>  		err = sock->ops->recvmsg(sock, &msg,
> @@ -1270,7 +1311,6 @@
>  			 */
>  			iov_iter_advance(&fixup, sizeof(hdr));
>  		}
> -		/* TODO: Should check and handle checksum. */
>  
>  		num_buffers = cpu_to_vhost16(vq, headcount);
>  		if (likely(set_num_buffers) &&