From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 906455A02E4 for ; Wed, 1 May 2024 02:10:32 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1714522228; bh=8vcU/CuKCoHaZhUaCuypuYb7Qy+Wr523iss6jCHo5vA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=nt2IkikmXJnFw6bWxgAE2AH1ys2RW7vEDBX6DOn8ttr6ZIDgfh/SLfNA98PYbtgxW rjqjpQ6RfkrBMyBtyvlGn8O4gabBcJoKfYCnGtq0ewUVnP44gVx8NyVUomiS4sVO1F Vm9CGkT1T0mpkWQuKq1ujdM1nAlX+471yazkNiFebuHfFQOlZV4PTldjOpbnCPqFNg VRT0ilT/gDLBwSfQRUIY9NPRd0CVevDyVd18ryQw8PTjOQAZIF24CAyfev/Fl9BqEv 7KiQ/TYeEFPTUDrECQ3t2PFNZ58wY8Uf8tzWDinlcFCFYZzWN93fV2GtnwQjtw+xDh 2UUr3FsGr4YGg== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4VTcsJ2nMvz4x10; Wed, 1 May 2024 10:10:28 +1000 (AEST) Date: Wed, 1 May 2024 09:53:56 +1000 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 2/7] tap: Split tap specific and L2 (ethernet) headers Message-ID: References: <20240429070933.1366881-1-david@gibson.dropbear.id.au> <20240429070933.1366881-3-david@gibson.dropbear.id.au> <20240430204629.1f0cc9c6@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="xoH8z9eLZ7ttZCBb" Content-Disposition: inline In-Reply-To: <20240430204629.1f0cc9c6@elisabeth> Message-ID-Hash: IZLHMGPHCFBW4ZGFCI3LRSJIQ2EEO244 X-Message-ID-Hash: IZLHMGPHCFBW4ZGFCI3LRSJIQ2EEO244 X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, Laurent Vivier X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: --xoH8z9eLZ7ttZCBb Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Apr 30, 2024 at 08:46:29PM +0200, Stefano Brivio wrote: > On Mon, 29 Apr 2024 17:09:28 +1000 > David Gibson wrote: >=20 > > In some places (well, actually only UDP now) we use struct tap_hdr to > > represent both tap backend specific and L2 ethernet headers. Handling > > these together seemed like a good idea at the time, but Laurent's chang= es > > in the TCP code working towards vhost-user support suggest that treating > > them separately is more useful, more often. > >=20 > > Alter struct tap_hdr to represent only the TAP backend specific headers. > > Updated related helpers and the UDP code to match. > >=20 > > Signed-off-by: David Gibson > > --- > > tap.h | 21 +++++++++------------ > > udp.c | 23 ++++++++++++++--------- > > 2 files changed, 23 insertions(+), 21 deletions(-) > >=20 > > diff --git a/tap.h b/tap.h > > index 2adc4e2b..dbc23b31 100644 > > --- a/tap.h > > +++ b/tap.h > > @@ -6,30 +6,28 @@ > > #ifndef TAP_H > > #define TAP_H > > =20 > > +#define ETH_HDR_INIT(proto) { .h_proto =3D htons_constant(proto) } > > + > > /** > > - * struct tap_hdr - L2 and tap specific headers > > + * struct tap_hdr - tap backend specific headers > > * @vnet_len: Frame length (for qemu socket transport) > > - * @eh: Ethernet header > > */ > > struct tap_hdr { > > uint32_t vnet_len; > > - struct ethhdr eh; > > } __attribute__((packed)); >=20 > No need to have it packed anymore. I'd arguably that conceptually it should still be packed. This is still expected to be a contiguous block of bytes, laid out in a specific way. Of course, since it's just a single u32, ((packed)) doesn't have any effect, but that could change if we need any different tap-specific headers in future. > > -#define TAP_HDR_INIT(proto) { .eh.h_proto =3D htons_constant(proto) } > > - > > static inline size_t tap_hdr_len_(const struct ctx *c) > > { > > if (c->mode =3D=3D MODE_PASST) > > return sizeof(struct tap_hdr); > > else > > - return sizeof(struct ethhdr); > > + return 0; > > } > > =20 > > /** > > * tap_frame_base() - Find start of tap frame > > * @c: Execution context > > - * @taph: Pointer to L2 and tap specific header buffer > > + * @taph: Pointer to tap specific header buffer > > * > > * Returns: pointer to the start of tap frame - suitable for an > > * iov_base to be passed to tap_send_frames()) > > @@ -43,17 +41,16 @@ static inline void *tap_frame_base(const struct ctx= *c, struct tap_hdr *taph) > > * tap_frame_len() - Finalize tap frame and return total length > > * @c: Execution context > > * @taph: Tap header to finalize > > - * @plen: L3 packet length (excludes L2 and tap specific headers) > > + * @plen: L2 packet length (includes L2, excludes tap specific headers) > > * > > - * Returns: length of the tap frame including L2 and tap specific > > - * headers - suitable for an iov_len to be passed to > > - * tap_send_frames() > > + * Returns: length of the tap frame including tap specific headers - s= uitable > > + * for an iov_len to be passed to tap_send_frames() > > */ > > static inline size_t tap_frame_len(const struct ctx *c, struct tap_hdr= *taph, > > size_t plen) > > { > > if (c->mode =3D=3D MODE_PASST) > > - taph->vnet_len =3D htonl(plen + sizeof(taph->eh)); > > + taph->vnet_len =3D htonl(plen); > > return plen + tap_hdr_len_(c); > > } > > =20 > > diff --git a/udp.c b/udp.c > > index 594ea191..c3e4f6b6 100644 > > --- a/udp.c > > +++ b/udp.c > > @@ -173,7 +173,8 @@ static uint8_t udp_act[IP_VERSIONS][UDP_ACT_TYPE_MA= X][DIV_ROUND_UP(NUM_PORTS, 8) > > /** > > * udp4_l2_buf_t - Pre-cooked IPv4 packet buffers for tap connections > > * @s_in: Source socket address, filled in by recvmmsg() > > - * @taph: Tap-level headers (partially pre-filled) > > + * @taph: Tap backend specific header > > + * @eh: Prefilled ethernet header > > * @iph: Pre-filled IP header (except for tot_len and saddr) > > * @uh: Headroom for UDP header > > * @data: Storage for UDP payload > > @@ -182,6 +183,7 @@ static struct udp4_l2_buf_t { > > struct sockaddr_in s_in; > > =20 > > struct tap_hdr taph; > > + struct ethhdr eh; > > struct iphdr iph; > > struct udphdr uh; > > uint8_t data[USHRT_MAX - > > @@ -192,7 +194,8 @@ udp4_l2_buf[UDP_MAX_FRAMES]; > > /** > > * udp6_l2_buf_t - Pre-cooked IPv6 packet buffers for tap connections > > * @s_in6: Source socket address, filled in by recvmmsg() > > - * @taph: Tap-level headers (partially pre-filled) > > + * @taph: Tap backend specific header > > + * @eh: Pre-filled ethernet header > > * @ip6h: Pre-filled IP header (except for payload_len and addresses) > > * @uh: Headroom for UDP header > > * @data: Storage for UDP payload > > @@ -202,10 +205,11 @@ struct udp6_l2_buf_t { > > #ifdef __AVX2__ > > /* Align ip6h to 32-byte boundary. */ > > uint8_t pad[64 - (sizeof(struct sockaddr_in6) + sizeof(struct ethhdr)= + > > - sizeof(uint32_t))]; > > + sizeof(struct tap_hdr))]; > > #endif > > =20 > > struct tap_hdr taph; > > + struct ethhdr eh; > > struct ipv6hdr ip6h; > > struct udphdr uh; > > uint8_t data[USHRT_MAX - > > @@ -289,8 +293,8 @@ void udp_update_l2_buf(const unsigned char *eth_d, = const unsigned char *eth_s) > > struct udp4_l2_buf_t *b4 =3D &udp4_l2_buf[i]; > > struct udp6_l2_buf_t *b6 =3D &udp6_l2_buf[i]; > > =20 > > - eth_update_mac(&b4->taph.eh, eth_d, eth_s); > > - eth_update_mac(&b6->taph.eh, eth_d, eth_s); > > + eth_update_mac(&b4->eh, eth_d, eth_s); > > + eth_update_mac(&b6->eh, eth_d, eth_s); > > } > > } > > =20 > > @@ -307,7 +311,7 @@ static void udp_sock4_iov_init_one(const struct ctx= *c, size_t i) > > struct iovec *tiov =3D &udp4_l2_iov_tap[i]; > > =20 > > *buf =3D (struct udp4_l2_buf_t) { > > - .taph =3D TAP_HDR_INIT(ETH_P_IP), > > + .eh =3D ETH_HDR_INIT(ETH_P_IP), > > .iph =3D L2_BUF_IP4_INIT(IPPROTO_UDP) > > }; > > =20 > > @@ -335,7 +339,7 @@ static void udp_sock6_iov_init_one(const struct ctx= *c, size_t i) > > struct iovec *tiov =3D &udp6_l2_iov_tap[i]; > > =20 > > *buf =3D (struct udp6_l2_buf_t) { > > - .taph =3D TAP_HDR_INIT(ETH_P_IPV6), > > + .eh =3D ETH_HDR_INIT(ETH_P_IPV6), > > .ip6h =3D L2_BUF_IP6_INIT(IPPROTO_UDP) > > }; > > =20 > > @@ -608,7 +612,7 @@ static size_t udp_update_hdr4(const struct ctx *c, = struct udp4_l2_buf_t *b, > > b->uh.dest =3D htons(dstport); > > b->uh.len =3D htons(datalen + sizeof(b->uh)); > > =20 > > - return tap_frame_len(c, &b->taph, ip_len); > > + return tap_frame_len(c, &b->taph, ip_len + sizeof(b->eh)); > > } > > =20 > > /** > > @@ -676,7 +680,8 @@ static size_t udp_update_hdr6(const struct ctx *c, = struct udp6_l2_buf_t *b, > > b->uh.len =3D b->ip6h.payload_len; > > csum_udp6(&b->uh, src, dst, b->data, datalen); > > =20 > > - return tap_frame_len(c, &b->taph, payload_len + sizeof(b->ip6h)); > > + return tap_frame_len(c, &b->taph, payload_len + sizeof(b->ip6h) > > + + sizeof(b->eh)); >=20 > Nit: the + operator should be on the first line for consistency, and > for readability I'd rather do: >=20 > return tap_frame_len(c, &b->taph, payload_len + sizeof(b->ip6h) + > sizeof(b->eh)); Tweaked, thanks. --=20 David Gibson | 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 --xoH8z9eLZ7ttZCBb Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmYxhIQACgkQzQJF27ox 2GcxCQ/+Jt3/ctBQO1N+A3LBXlAXyIsIW51tz6OuhH2ih7bp6CO1UZRswOZlKqGF rQvp3cPjb4XNPzKY3jY5cSJuCVuOZXMG2/KgRwfSPuC9nmvrtCd2mUAXR6BLcFYV gQoa+4pGiSzbQwc5NOKrW2/st8yj0/XtgFqaxC9+FAL0W7Nz9HOCUSggYSPOSgDZ YA3V+cCuboOxzYtKNVEx6EoAKiYsbw/AxBlXePwaCRGyNL5gnS/RqCcQo4g9PMtN aUyJezJ93tGx59eMM4k/T2STifZ+g8lKv+HUH2NoKEhHzT+gJ/HCQ43KskPSn8/O rm16V6xE+opUaWOMzl9LrbgqqGljhRMgxBnj1/c7dhcCcMFitwIlHqobRuWGbEhl 9SC97nV+qDdftRz8zotHGbX7AogbozQ6/JSSl7/+DMLbTyK5dF+2T545QgUsHRwU hcSBp/iixa/s/6Jpa+exSDAeOu/lZFfTX6sCViyc5gTD8j7O1nDstBdKbtprPpEp yYan6YsDlGEJnV1NDrnvvxjo+dqSoesVfIOUE8KJkLz5zzoYPQLfx3s4OenMb86f xkyL1Dj+AeSMhM3uqoMZq97pnoob/8sZQYJzLA7LmAbY0FJ8QDGvP/sTUuWPRSLh Xp+VT3vN7BsDZPikgFdVX0WUE6xIoxdIQQOsm3i4r+KhzqHzruI= =FpaK -----END PGP SIGNATURE----- --xoH8z9eLZ7ttZCBb--