Re: [RFC v2 03/11] tap: replace tx tap hdr with virtio_nethdr_mrg_rxbuf

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3834 bytes --]

On Wed, Jul 09, 2025 at 07:47:40PM +0200, Eugenio Pérez wrote:
> vhost kernel expects this as the first data of the frame.
> 
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
>  tap.c     |  2 +-
>  tcp_buf.c | 15 +++++++++++++--
>  2 files changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/tap.c b/tap.c
> index 0c49e6d..0656294 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -593,7 +593,7 @@ size_t tap_send_frames(const struct ctx *c, const struct iovec *iov,
>  		      nframes - m, nframes);
> 
>  	pcap_multiple(iov, bufs_per_frame, m,
> -		      c->mode == MODE_PASST ? sizeof(uint32_t) : 0);
> +		      c->mode == MODE_PASST ? sizeof(uint32_t) : sizeof(struct virtio_net_hdr_mrg_rxbuf));

If I understand correctly, you're always considering
virtio_net_hdr_mrg_rxbuf part of the extended frame in PASTA mode,
it's just unused when you're not using the vhost path.  Is that correct?

We probably want a helper that returns the correct tap header length,
a companion to tap_hdr_iov().

> 
>  	return m;
>  }
> diff --git a/tcp_buf.c b/tcp_buf.c
> index d1fca67..2fbd056 100644
> --- a/tcp_buf.c
> +++ b/tcp_buf.c
> @@ -22,6 +22,8 @@
> 
>  #include <netinet/tcp.h>
> 
> +#include <linux/virtio_net.h>
> +
>  #include "util.h"
>  #include "ip.h"
>  #include "iov.h"
> @@ -43,7 +45,7 @@
>  static struct ethhdr		tcp4_eth_src;
>  static struct ethhdr		tcp6_eth_src;
> 
> -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];

The intention is that struct tap_hdr can store whatever sort of "below
L2" header we need - it's just that so far we only had the trivial
qemu socket header or nothing.  Now we're adding another option, so it
should be a union branch of tap_hdr, rather than a separate structure.

>  /* IP headers for IPv4 and IPv6 */
>  struct iphdr		tcp4_payload_ip[TCP_FRAMES_MEM];
> @@ -75,6 +77,14 @@ void tcp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s)
>  	eth_update_mac(&tcp6_eth_src, eth_d, eth_s);
>  }
> 
> +static inline struct iovec virtio_net_hdr_iov(struct virtio_net_hdr_mrg_rxbuf *hdr)
> +{
> +        return (struct iovec){
> +                .iov_base = hdr,
> +                .iov_len = sizeof(*hdr),
> +        };
> +}

With that unification of tap_hdr_iov(), this should be a branch in
tap_hdr_iov(), rather than a separate function.  tap_hdr_update() will
also need to be updated to only update vnet_len in PASST (qemu socket)
mode.

It is now occurring to me that adding vhost is highlighting an
existing ambiguity in our terminology: "tap" can mean either "the
guest facing interface, of whatever mechanics", or specifically the
kernel tuntap device.  Maybe we need to look at a great renaming.

> +
>  /**
>   * tcp_sock_iov_init() - Initialise scatter-gather L2 buffers for IPv4 sockets
>   * @c:		Execution context
> @@ -85,6 +95,7 @@ void tcp_sock_iov_init(const struct ctx *c)
>  	struct iphdr iph = L2_BUF_IP4_INIT(IPPROTO_TCP);
>  	int i;
> 
> +	(void)c;
>  	tcp6_eth_src.h_proto = htons_constant(ETH_P_IPV6);
>  	tcp4_eth_src.h_proto = htons_constant(ETH_P_IP);
> 
> @@ -96,7 +107,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] = virtio_net_hdr_iov(&tcp_payload_tap_hdr[i]);
>  		iov[TCP_IOV_ETH].iov_len = sizeof(struct ethhdr);
>  		iov[TCP_IOV_PAYLOAD].iov_base = &tcp_payload[i];
>  	}

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]