From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTP id 2E91C5A02C8 for ; Tue, 30 Apr 2024 20:47:09 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1714502828; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=w/i+vw48BQ0r6prZDhGaTSdiRIXsatE+DwRoMsTHW0c=; b=BwL0jGseALfueVvX0d+rZe+RFzflvDWOoOrOcpjgJxbg5+Ri1oUH9Yfa3hsAbbeCaYL9jo XfIEJDMKKapdeEx8UfIsIyQpcPUH/ae1WAruju1fruRsgZfzMNixBN0x9DNvAI+gY5+o/d +XyA/vUKYVkJZolBDbhgtcVrafe9VIM= Received: from mail-ej1-f69.google.com (mail-ej1-f69.google.com [209.85.218.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-122-uV1a5XstNWCTdhY1c63loA-1; Tue, 30 Apr 2024 14:47:06 -0400 X-MC-Unique: uV1a5XstNWCTdhY1c63loA-1 Received: by mail-ej1-f69.google.com with SMTP id a640c23a62f3a-a590fa4a117so110629666b.1 for ; Tue, 30 Apr 2024 11:47:06 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714502824; x=1715107624; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=w/i+vw48BQ0r6prZDhGaTSdiRIXsatE+DwRoMsTHW0c=; b=lqv9JgsPwWSRRxJi/EfEDhBBsRwfErxaFUaKvL9fttsrCe/d8udhFECTXJibxuKokA WJbj86HaMjzMAIDdlZHfWcIqho2GKdPcWcZUfFNn82kDUDtPvNTa74Re044QWdBoiUTz TuyAhDZN8b9mWXU+wrnSvCnKHDjYnhqXwSnVXuSuYJv8qF82z/JigXr4GCi7rkwDJbpt YWsU3CHsnFE8I7vybB1JraPJi4wZE/+dOMlzpKDF4+L/CVmiiYcpF9r8Yty3wQlYa0sL x0gA3JHykASZIy/EBOdTymTI59IbAcWGrVJRD9fkSqbvVsoOyvs5uhqhSEiQCkgn3Tar 6Dxw== X-Gm-Message-State: AOJu0Yzbbc5cTZL7NsmURbHaS3Xc9ImfcOcL+MM4Lz4HS7rHKP6oZWg4 OMVjtJaRGcxDRiQnZ8n3eH8dlTphptxVdOfTA57tZDUb2ejoLs1OAD10alRXyNaBuYKfzYcL2RN 3bGwdcqGapVhG9oCmx3SCoVohYygFb4x/gVwAMhExo4u88MIK2dbnNmB6Xd3k X-Received: by 2002:a17:906:3592:b0:a58:e5e4:24d7 with SMTP id o18-20020a170906359200b00a58e5e424d7mr362236ejb.12.1714502823955; Tue, 30 Apr 2024 11:47:03 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGaPCmIGV7rSDve6R+OeUAb9GmIjqDawv9s/yGstqoN41vzKSNK1A3e5KIJxzjavdWc5R1d5A== X-Received: by 2002:a17:906:3592:b0:a58:e5e4:24d7 with SMTP id o18-20020a170906359200b00a58e5e424d7mr362220ejb.12.1714502823192; Tue, 30 Apr 2024 11:47:03 -0700 (PDT) Received: from maya.cloud.tilaa.com (maya.cloud.tilaa.com. [164.138.29.33]) by smtp.gmail.com with ESMTPSA id h26-20020a17090619da00b00a588729de82sm8547440ejd.142.2024.04.30.11.47.02 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Tue, 30 Apr 2024 11:47:02 -0700 (PDT) Date: Tue, 30 Apr 2024 20:46:29 +0200 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 2/7] tap: Split tap specific and L2 (ethernet) headers Message-ID: <20240430204629.1f0cc9c6@elisabeth> In-Reply-To: <20240429070933.1366881-3-david@gibson.dropbear.id.au> References: <20240429070933.1366881-1-david@gibson.dropbear.id.au> <20240429070933.1366881-3-david@gibson.dropbear.id.au> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.36; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: ZYZ4M5FWFSXP2FDOT3QTK7HOLBCNPO4X X-Message-ID-Hash: ZYZ4M5FWFSXP2FDOT3QTK7HOLBCNPO4X X-MailFrom: sbrivio@redhat.com 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: On Mon, 29 Apr 2024 17:09:28 +1000 David Gibson wrote: > 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 changes > in the TCP code working towards vhost-user support suggest that treating > them separately is more useful, more often. > > Alter struct tap_hdr to represent only the TAP backend specific headers. > Updated related helpers and the UDP code to match. > > Signed-off-by: David Gibson > --- > tap.h | 21 +++++++++------------ > udp.c | 23 ++++++++++++++--------- > 2 files changed, 23 insertions(+), 21 deletions(-) > > 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 > > +#define ETH_HDR_INIT(proto) { .h_proto = 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)); No need to have it packed anymore. > -#define TAP_HDR_INIT(proto) { .eh.h_proto = htons_constant(proto) } > - > static inline size_t tap_hdr_len_(const struct ctx *c) > { > if (c->mode == MODE_PASST) > return sizeof(struct tap_hdr); > else > - return sizeof(struct ethhdr); > + return 0; > } > > /** > * 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 - suitable > + * 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 == MODE_PASST) > - taph->vnet_len = htonl(plen + sizeof(taph->eh)); > + taph->vnet_len = htonl(plen); > return plen + tap_hdr_len_(c); > } > > 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_MAX][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; > > 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 > > 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 = &udp4_l2_buf[i]; > struct udp6_l2_buf_t *b6 = &udp6_l2_buf[i]; > > - 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); > } > } > > @@ -307,7 +311,7 @@ static void udp_sock4_iov_init_one(const struct ctx *c, size_t i) > struct iovec *tiov = &udp4_l2_iov_tap[i]; > > *buf = (struct udp4_l2_buf_t) { > - .taph = TAP_HDR_INIT(ETH_P_IP), > + .eh = ETH_HDR_INIT(ETH_P_IP), > .iph = L2_BUF_IP4_INIT(IPPROTO_UDP) > }; > > @@ -335,7 +339,7 @@ static void udp_sock6_iov_init_one(const struct ctx *c, size_t i) > struct iovec *tiov = &udp6_l2_iov_tap[i]; > > *buf = (struct udp6_l2_buf_t) { > - .taph = TAP_HDR_INIT(ETH_P_IPV6), > + .eh = ETH_HDR_INIT(ETH_P_IPV6), > .ip6h = L2_BUF_IP6_INIT(IPPROTO_UDP) > }; > > @@ -608,7 +612,7 @@ static size_t udp_update_hdr4(const struct ctx *c, struct udp4_l2_buf_t *b, > b->uh.dest = htons(dstport); > b->uh.len = htons(datalen + sizeof(b->uh)); > > - return tap_frame_len(c, &b->taph, ip_len); > + return tap_frame_len(c, &b->taph, ip_len + sizeof(b->eh)); > } > > /** > @@ -676,7 +680,8 @@ static size_t udp_update_hdr6(const struct ctx *c, struct udp6_l2_buf_t *b, > b->uh.len = b->ip6h.payload_len; > csum_udp6(&b->uh, src, dst, b->data, datalen); > > - 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)); Nit: the + operator should be on the first line for consistency, and for readability I'd rather do: return tap_frame_len(c, &b->taph, payload_len + sizeof(b->ip6h) + sizeof(b->eh)); -- Stefano