Re: [PATCH v2 5/7] tap, tcp, udp: Prepare the to-guest path for vhost-net

[Laurent Vivier <lvivier@redhat.com>]

On 10/16/26 22:49, aerosouund wrote:
> From: Ammar Yasser <aerosound161@gmail.com>
> 
> Frames sent through vhost-net carry a virtio-net header where pasta's
> tap header would otherwise go, this necessitates the following two
> changes:
> 
> - udp_tap_prepare() has been modified to take a context parameter so it
>    can check if vhost is set up. Only if it isn't should tap_hdr_update
>    be called because this means that the active union variant is vnet_len
>    no the virtio net header
> - tap_l2_offset() a new helper, has been introduced which tells
>    pcap_multiple() how many bytes precede the L2 frame, which now varies
>    with vhost as well as with the mode
> 
> tap_send_single() calls tap_send_frames_passt() and
> tap_send_frames_pasta() directly rather than going through
> tap_send_frames(). Its callers - ARP, TCP resets, and DHCP, DHCPv6, NDP
> and ICMP via tap_udp*_send()/tap_icmp*_send() - build their frame in a
> buffer on the stack, not in the buffers registered with vhost-net. The
> kernel can only read from registered memory, so these protocols can
> never take the vhost path and shouldn't be tested for it on every frame.
> 
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> Signed-off-by: Ammar Yasser <aerosound161@gmail.com>
> ---
>   tap.c     | 112 ++++++++++++++++++++++++++++++++++--------------------
>   tcp_buf.c |  12 ++++--
>   udp.c     |  27 +++++++++----
>   3 files changed, 99 insertions(+), 52 deletions(-)
> 
> diff --git a/tap.c b/tap.c
> index a5fdee8..fd500f1 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -123,45 +123,6 @@ unsigned long tap_l2_max_len(const struct ctx *c)
>   	return 0; /* Unreachable, for cppcheck's sake */
>   }
> 
> -/**
> - * tap_send_single() - Send a single frame
> - * @c:		Execution context
> - * @data:	Packet buffer
> - * @l2len:	Total L2 packet length
> - */
> -void tap_send_single(const struct ctx *c, const void *data, size_t l2len)
> -{
> -	uint8_t padded[ETH_ZLEN] = { 0 };
> -	struct iovec iov[2];
> -	size_t iovcnt = 0;
> -	uint32_t vnet_len;
> -
> -	if (l2len < ETH_ZLEN) {
> -		memcpy(padded, data, l2len);
> -		data = padded;
> -		l2len = ETH_ZLEN;
> -	}
> -
> -	vnet_len = htonl(l2len);
> -
> -	switch (c->mode) {
> -	case MODE_PASST:
> -		iov[iovcnt] = IOV_OF_LVALUE(vnet_len);
> -		iovcnt++;
> -		/* fall through */
> -	case MODE_PASTA:
> -		iov[iovcnt].iov_base = (void *)data;
> -		iov[iovcnt].iov_len = l2len;
> -		iovcnt++;
> -
> -		tap_send_frames(c, iov, iovcnt, 1);
> -		break;
> -	case MODE_VU:
> -		vu_send_single(c, data, l2len);
> -		break;
> -	}
> -}
> -
>   /**
>    * tap_push_l2h() - Build an L2 header for an inbound packet
>    * @c:		Execution context
> @@ -497,6 +458,76 @@ static size_t tap_send_frames_passt(const struct ctx *c,
>   	return i / bufs_per_frame;
>   }
> 
> +/**
> + * tap_l2_offset() - Backend specific header size preceding each L2 frame
> + * @c:		Execution context
> + *
> + * Return: offset of the L2 frame within each frame's first buffer
> + */
> +static size_t tap_l2_offset(const struct ctx *c)
> +{
> +	if (c->mode == MODE_PASST)
> +		return sizeof(uint32_t);	/* vnet_len */
> +
> +	if (c->vhost.fd != -1)
> +		return VNET_HLEN;
> +
> +	return 0;
> +}
> +
> +/**
> + * tap_send_single() - Send a single frame
> + * @c:		Execution context
> + * @data:	Packet buffer
> + * @l2len:	Total L2 packet length
> + */
> +void tap_send_single(const struct ctx *c, const void *data, size_t l2len)
> +{
> +	uint8_t padded[ETH_ZLEN] = { 0 };
> +	struct iovec iov[2];
> +	uint32_t vnet_len;
> +	size_t iovcnt = 0;
> +	size_t m = 0;
> +
> +	if (l2len < ETH_ZLEN) {
> +		memcpy(padded, data, l2len);
> +		data = padded;
> +		l2len = ETH_ZLEN;
> +	}
> +
> +	vnet_len = htonl(l2len);
> +
> +	switch (c->mode) {
> +	case MODE_PASST:
> +		/* create an iov for the length */
> +		iov[iovcnt] = IOV_OF_LVALUE(vnet_len);
> +		iovcnt++;
> +		/* create the data iov */
> +		iov[iovcnt].iov_base = (void *)data;
> +		iov[iovcnt].iov_len = l2len;
> +		iovcnt++;
> +
> +		m = tap_send_frames_passt(c, iov, iovcnt, 1);
> +		break;
> +	case MODE_PASTA:
> +		/* don't create a length iov in the case of pasta */
> +		iov[iovcnt].iov_base = (void *)data;
> +		iov[iovcnt].iov_len = l2len;
> +		iovcnt++;
> +
> +		m = tap_send_frames_pasta(c, iov, iovcnt, 1);
> +		break;
> +	case MODE_VU:
> +		vu_send_single(c, data, l2len);
> +		return;
> +	}
> +
> +	if (!m)
> +		debug("tap: failed to send a single frame");
> +
> +	pcap_multiple(iov, iovcnt, m, tap_l2_offset(c));

for MODE_PASTA with vhost, tap_l2_offset(c) is VNET_HLEN, but in "case MODE_PASTA" 
iov_base points directly to data, so it should be 0.

> +}
> +
>   /**
>    * tap_send_frames() - Send out multiple prepared frames
>    * @c:			Execution context
> @@ -537,8 +568,7 @@ size_t tap_send_frames(const struct ctx *c, const struct iovec *iov,
>   		debug("tap: failed to send %zu frames of %zu",
>   		      nframes - m, nframes);
> 
> -	pcap_multiple(iov, bufs_per_frame, m,
> -		      c->mode == MODE_PASST ? sizeof(uint32_t) : 0);
> +	pcap_multiple(iov, bufs_per_frame, m, tap_l2_offset(c));
> 
>   	return m;
>   }
> diff --git a/tcp_buf.c b/tcp_buf.c
> index dd80384..493c5c9 100644
> --- a/tcp_buf.c
> +++ b/tcp_buf.c
> @@ -21,6 +21,7 @@
>   #include <netinet/ip.h>
> 
>   #include <netinet/tcp.h>
> +#include <linux/virtio_net.h>
> 
>   #include "util.h"
>   #include "ip.h"
> @@ -44,7 +45,7 @@
>   /* Ethernet header for IPv4 and IPv6 frames */
>   static struct ethhdr		tcp_eth_hdr[TCP_FRAMES_MEM];
> 
> -static struct tap_hdr		tcp_payload_tap_hdr[TCP_FRAMES_MEM];
> +static struct virtio_net_hdr_mrg_rxbuf tcp_payload_tap_hdr[TCP_FRAMES_MEM];

We can keep "struct tap_hdr" as it is an union of struct virtio_net_hdr_mrg_rxbuf.>
>   /* IP headers for IPv4 and IPv6 */
>   static struct iphdr		tcp4_payload_ip[TCP_FRAMES_MEM];
> @@ -113,7 +114,7 @@ void tcp_sock_iov_init(const struct ctx *c)
>   	for (i = 0; i < TCP_FRAMES_MEM; i++) {
>   		struct iovec *iov = tcp_l2_iov[i];
> 
> -		iov[TCP_IOV_TAP] = tap_hdr_iov(c, &tcp_payload_tap_hdr[i]);
> +		iov[TCP_IOV_TAP] = tap_hdr_iov(c, (struct tap_hdr *)&tcp_payload_tap_hdr[i]);

and then no need of the cast.

>   		iov[TCP_IOV_ETH].iov_len = sizeof(struct ethhdr);
>   		iov[TCP_IOV_PAYLOAD].iov_base = &tcp_payload[i];
>   		iov[TCP_IOV_ETH_PAD].iov_base = eth_pad;
> @@ -216,7 +217,12 @@ static void tcp_l2_buf_fill_headers(const struct ctx *c,
> 
>   	l2len = tcp_fill_headers(c, conn, eh, ip4h, ip6h, th, &tail,
>   				 iov_tail_size(&tail), csum_flags, seq);
> -	tap_hdr_update(taph, l2len);
> +
> +	/* With vhost-net this buffer holds a virtio-net header, which a tap
> +	 * one must not be written over
> +	 */
> +	if (c->vhost.fd == -1)
> +		tap_hdr_update(taph, l2len);
>   }
> 
>   /**
> diff --git a/udp.c b/udp.c
> index a4355a1..afe1c7f 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -103,6 +103,7 @@
>   #include <time.h>
>   #include <arpa/inet.h>
>   #include <linux/errqueue.h>
> +#include <linux/virtio_net.h>
> 
>   #include "checksum.h"
>   #include "util.h"
> @@ -149,12 +150,16 @@ static struct ethhdr udp_eth_hdr[UDP_MAX_FRAMES];
>    * struct udp_meta_t - Pre-cooked headers for UDP packets
>    * @ip6h:	Pre-filled IPv6 header (except for payload_len and addresses)
>    * @ip4h:	Pre-filled IPv4 header (except for tot_len and saddr)
> - * @taph:	Tap backend specific header
> + * @vnet_hdr:	virtio-net header, used when sending through vhost-net
> + * @taph:	Tap backend specific header, used otherwise
>    */
>   static struct udp_meta_t {
>   	struct ipv6hdr ip6h;
>   	struct iphdr ip4h;
> -	struct tap_hdr taph;
> +	union {
> +		struct virtio_net_hdr_mrg_rxbuf vnet_hdr;
> +		struct tap_hdr taph;
> +	};

Same here, "struct tap_hdr" is an union of "struct virtio_net_hdr_mrg_rxbuf"

>   }
>   #ifdef __AVX2__
>   __attribute__ ((aligned(32)))
> @@ -356,13 +361,15 @@ static void udp_tap_pad(struct iovec *iov)
> 
>   /**
>    * udp_tap_prepare() - Convert one datagram into a tap frame
> + * @c:      Execution context
>    * @mmh:	Receiving mmsghdr array
>    * @idx:	Index of the datagram to prepare
>    * @tap_omac:	MAC address of remote endpoint as seen from the guest
>    * @toside:	Flowside for destination side
>    * @no_udp_csum: Do not set UDP checksum
>    */
> -static void udp_tap_prepare(const struct mmsghdr *mmh,
> +static void udp_tap_prepare(const struct ctx *c,
> +				const struct mmsghdr *mmh,
>   			    unsigned int idx,
>   			    const uint8_t *tap_omac,
>   			    const struct flowside *toside,
> @@ -384,8 +391,10 @@ static void udp_tap_prepare(const struct mmsghdr *mmh,
>   		udp_update_hdr6(&bm->ip6h, uh, &payload, toside,
>   			        mmh[idx].msg_len, no_udp_csum);
> 
> -		l2len = MAX(l4len + sizeof(bm->ip6h) + ETH_HLEN, ETH_ZLEN);
> -		tap_hdr_update(&bm->taph, l2len);
> +		if (c->mode == MODE_PASST) {
> +			l2len = MAX(l4len + sizeof(bm->ip6h) + ETH_HLEN, ETH_ZLEN);
> +			tap_hdr_update(&bm->taph, l2len);
> +		}
> 
>   		eh->h_proto = htons_constant(ETH_P_IPV6);
>   		(*tap_iov)[UDP_IOV_IP] = IOV_OF_LVALUE(bm->ip6h);
> @@ -393,8 +402,10 @@ static void udp_tap_prepare(const struct mmsghdr *mmh,
>   		udp_update_hdr4(&bm->ip4h, uh, &payload, toside,
>   			        mmh[idx].msg_len, no_udp_csum);
> 
> -		l2len = MAX(l4len + sizeof(bm->ip4h) + ETH_HLEN, ETH_ZLEN);
> -		tap_hdr_update(&bm->taph, l2len);
> +		if (c->mode == MODE_PASST) {
> +			l2len = MAX(l4len + sizeof(bm->ip4h) + ETH_HLEN, ETH_ZLEN);
> +			tap_hdr_update(&bm->taph, l2len);
> +		}
> 
>   		eh->h_proto = htons_constant(ETH_P_IP);
>   		(*tap_iov)[UDP_IOV_IP] = IOV_OF_LVALUE(bm->ip4h);
> @@ -867,7 +878,7 @@ static void udp_buf_sock_to_tap(const struct ctx *c, int s, int n,
>   		fwd_neigh_mac_get(c, &toside->oaddr, omac);
> 
>   	for (i = 0; i < n; i++)
> -		udp_tap_prepare(udp_mh_recv, i, omac, toside, false);
> +		udp_tap_prepare(c, udp_mh_recv, i, omac, toside, false);
> 
>   	tap_send_frames(c, &udp_l2_iov[0][0], UDP_NUM_IOVS, n);
>   }
> --
> 2.39.5 (Apple Git-154)