Hi Dmitrii,
It seems to me that this series of patches breaks the unittest of TCP stack
in sandbox.
I'll see if I have some time to fix the unittest too.
The unittest can be built by the following commands.
1. "make sandbox64_defconfig"
2. turn on the options that need to be tested. For example, CMD_WGET
3. "make"
4. "sudo ./u-boot -D"
5. "ut lib net_test_wget" in the U-boot prompt.
Yours,
Paul
On Tue, 9 May 2023 at 01:15, Dmitrii Merkurev <dimori...@google.com> wrote:
> This allows us to reuse TCP logic between IP and IP6 stack.
>
> Signed-off-by: Dmitrii Merkurev <dimori...@google.com>
> Cc: Ying-Chun Liu (PaulLiu) <paul....@linaro.org>
> Cc: Simon Glass <s...@chromium.org>
> Сс: Joe Hershberger <joe.hershber...@ni.com>
> Сс: Ramon Fried <rfried....@gmail.com>
> ---
> include/net/tcp.h | 50 ++++++++++++++-------------------
> net/tcp.c | 70 +++++++++++++++++++++++------------------------
> test/cmd/wget.c | 16 +++++------
> 3 files changed, 64 insertions(+), 72 deletions(-)
>
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index c29d4ce24a..5ab1127d8b 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -5,20 +5,16 @@
> * Copyright 2017 Duncan Hare, All rights reserved.
> */
>
> +#ifndef __TCP_H__
> +#define __TCP_H__
> +
> #define TCP_ACTIVITY 127 /* Number of packets received */
> /* before console progress mark */
> +
> +#define GET_TCP_HDR_LEN_IN_BYTES(x) ((x) >> 2)
> +
> /**
> - * struct ip_tcp_hdr - IP and TCP header
> - * @ip_hl_v: header length and version
> - * @ip_tos: type of service
> - * @ip_len: total length
> - * @ip_id: identification
> - * @ip_off: fragment offset field
> - * @ip_ttl: time to live
> - * @ip_p: protocol
> - * @ip_sum: checksum
> - * @ip_src: Source IP address
> - * @ip_dst: Destination IP address
> + * struct tcp_hdr - TCP header
> * @tcp_src: TCP source port
> * @tcp_dst: TCP destination port
> * @tcp_seq: TCP sequence number
> @@ -28,18 +24,8 @@
> * @tcp_win: TCP windows size
> * @tcp_xsum: Checksum
> * @tcp_ugr: Pointer to urgent data
> - */
> -struct ip_tcp_hdr {
> - u8 ip_hl_v;
> - u8 ip_tos;
> - u16 ip_len;
> - u16 ip_id;
> - u16 ip_off;
> - u8 ip_ttl;
> - u8 ip_p;
> - u16 ip_sum;
> - struct in_addr ip_src;
> - struct in_addr ip_dst;
> +*/
> +struct tcp_hdr {
> u16 tcp_src;
> u16 tcp_dst;
> u32 tcp_seq;
> @@ -51,8 +37,8 @@ struct ip_tcp_hdr {
> u16 tcp_ugr;
> } __packed;
>
> -#define IP_TCP_HDR_SIZE (sizeof(struct ip_tcp_hdr))
> -#define TCP_HDR_SIZE (IP_TCP_HDR_SIZE - IP_HDR_SIZE)
> +#define TCP_HDR_SIZE (sizeof(struct tcp_hdr))
> +#define IP_TCP_HDR_SIZE (IP_HDR_SIZE + TCP_HDR_SIZE)
>
> #define TCP_DATA 0x00 /* Data Packet - internal use only */
> #define TCP_FIN 0x01 /* Finish flag
> */
> @@ -178,7 +164,8 @@ struct tcp_t_opt {
>
> /**
> * struct ip_tcp_hdr_o - IP + TCP header + TCP options
> - * @hdr: IP + TCP header
> + * @ip_hdr: IP + TCP header
> + * @tcp_hdr: TCP header
> * @mss: TCP MSS Option
> * @scale: TCP Windows Scale Option
> * @sack_p: TCP Sack-Permitted Option
> @@ -186,7 +173,8 @@ struct tcp_t_opt {
> * @end: end of options
> */
> struct ip_tcp_hdr_o {
> - struct ip_tcp_hdr hdr;
> + struct ip_hdr ip_hdr;
> + struct tcp_hdr tcp_hdr;
> struct tcp_mss mss;
> struct tcp_scale scale;
> struct tcp_sack_p sack_p;
> @@ -198,13 +186,15 @@ struct ip_tcp_hdr_o {
>
> /**
> * struct ip_tcp_hdr_s - IP + TCP header + TCP options
> - * @hdr: IP + TCP header
> + * @ip_hdr: IP + TCP header
> + * @tcp_hdr: TCP header
> * @t_opt: TCP Timestamp Option
> * @sack_v: TCP SACK Option
> * @end: end of options
> */
> struct ip_tcp_hdr_s {
> - struct ip_tcp_hdr hdr;
> + struct ip_hdr ip_hdr;
> + struct tcp_hdr tcp_hdr;
> struct tcp_t_opt t_opt;
> struct tcp_sack_v sack_v;
> u8 end;
> @@ -303,3 +293,5 @@ void rxhand_tcp_f(union tcp_build_pkt *b, unsigned int
> len);
>
> u16 tcp_set_pseudo_header(uchar *pkt, struct in_addr src, struct in_addr
> dest,
> int tcp_len, int pkt_len);
> +
> +#endif // __TCP_H__
> diff --git a/net/tcp.c b/net/tcp.c
> index a713e1dd60..10ce799814 100644
> --- a/net/tcp.c
> +++ b/net/tcp.c
> @@ -155,7 +155,7 @@ u16 tcp_set_pseudo_header(uchar *pkt, struct in_addr
> src, struct in_addr dest,
> */
> int net_set_ack_options(union tcp_build_pkt *b)
> {
> - b->sack.hdr.tcp_hlen =
> SHIFT_TO_TCPHDRLEN_FIELD(LEN_B_TO_DW(TCP_HDR_SIZE));
> + b->sack.tcp_hdr.tcp_hlen =
> SHIFT_TO_TCPHDRLEN_FIELD(LEN_B_TO_DW(TCP_HDR_SIZE));
>
> b->sack.t_opt.kind = TCP_O_TS;
> b->sack.t_opt.len = TCP_OPT_LEN_A;
> @@ -187,12 +187,12 @@ int net_set_ack_options(union tcp_build_pkt *b)
> b->sack.sack_v.hill[3].r = TCP_O_NOP;
> }
>
> - b->sack.hdr.tcp_hlen =
> SHIFT_TO_TCPHDRLEN_FIELD(ROUND_TCPHDR_LEN(TCP_HDR_SIZE +
> + b->sack.tcp_hdr.tcp_hlen =
> SHIFT_TO_TCPHDRLEN_FIELD(ROUND_TCPHDR_LEN(TCP_HDR_SIZE +
>
> TCP_TSOPT_SIZE +
>
> tcp_lost.len));
> } else {
> b->sack.sack_v.kind = 0;
> - b->sack.hdr.tcp_hlen =
> SHIFT_TO_TCPHDRLEN_FIELD(ROUND_TCPHDR_LEN(TCP_HDR_SIZE +
> + b->sack.tcp_hdr.tcp_hlen =
> SHIFT_TO_TCPHDRLEN_FIELD(ROUND_TCPHDR_LEN(TCP_HDR_SIZE +
>
> TCP_TSOPT_SIZE));
> }
>
> @@ -201,7 +201,7 @@ int net_set_ack_options(union tcp_build_pkt *b)
> * TCP header to add to the total packet length
> */
>
> - return GET_TCP_HDR_LEN_IN_BYTES(b->sack.hdr.tcp_hlen);
> + return GET_TCP_HDR_LEN_IN_BYTES(b->sack.tcp_hdr.tcp_hlen);
> }
>
> /**
> @@ -213,7 +213,7 @@ void net_set_syn_options(union tcp_build_pkt *b)
> if (IS_ENABLED(CONFIG_PROT_TCP_SACK))
> tcp_lost.len = 0;
>
> - b->ip.hdr.tcp_hlen = 0xa0;
> + b->ip.tcp_hdr.tcp_hlen = 0xa0;
>
> b->ip.mss.kind = TCP_O_MSS;
> b->ip.mss.len = TCP_OPT_LEN_4;
> @@ -249,9 +249,9 @@ int tcp_set_tcp_header(uchar *pkt, int dport, int
> sport, int payload_len,
> * Header: 5 32 bit words. 4 bits TCP header Length,
> * 4 bits reserved options
> */
> - b->ip.hdr.tcp_flags = action;
> + b->ip.tcp_hdr.tcp_flags = action;
> pkt_hdr_len = IP_TCP_HDR_SIZE;
> - b->ip.hdr.tcp_hlen =
> SHIFT_TO_TCPHDRLEN_FIELD(LEN_B_TO_DW(TCP_HDR_SIZE));
> + b->ip.tcp_hdr.tcp_hlen =
> SHIFT_TO_TCPHDRLEN_FIELD(LEN_B_TO_DW(TCP_HDR_SIZE));
>
> switch (action) {
> case TCP_SYN:
> @@ -274,7 +274,7 @@ int tcp_set_tcp_header(uchar *pkt, int dport, int
> sport, int payload_len,
> case TCP_SYN | TCP_ACK:
> case TCP_ACK:
> pkt_hdr_len = IP_HDR_SIZE + net_set_ack_options(b);
> - b->ip.hdr.tcp_flags = action;
> + b->ip.tcp_hdr.tcp_flags = action;
> debug_cond(DEBUG_DEV_PKT,
> "TCP Hdr:ACK (%pI4, %pI4, s=%u, a=%u, A=%x)\n",
> &net_server_ip, &net_ip, tcp_seq_num,
> tcp_ack_num,
> @@ -308,7 +308,7 @@ int tcp_set_tcp_header(uchar *pkt, int dport, int
> sport, int payload_len,
> fallthrough;
> default:
> pkt_hdr_len = IP_HDR_SIZE + net_set_ack_options(b);
> - b->ip.hdr.tcp_flags = action | TCP_PUSH | TCP_ACK;
> + b->ip.tcp_hdr.tcp_flags = action | TCP_PUSH | TCP_ACK;
> debug_cond(DEBUG_DEV_PKT,
> "TCP Hdr:dft (%pI4, %pI4, s=%u, a=%u, A=%x)\n",
> &net_server_ip, &net_ip,
> @@ -320,10 +320,10 @@ int tcp_set_tcp_header(uchar *pkt, int dport, int
> sport, int payload_len,
>
> tcp_ack_edge = tcp_ack_num;
> /* TCP Header */
> - b->ip.hdr.tcp_ack = htonl(tcp_ack_edge);
> - b->ip.hdr.tcp_src = htons(sport);
> - b->ip.hdr.tcp_dst = htons(dport);
> - b->ip.hdr.tcp_seq = htonl(tcp_seq_num);
> + b->ip.tcp_hdr.tcp_ack = htonl(tcp_ack_edge);
> + b->ip.tcp_hdr.tcp_src = htons(sport);
> + b->ip.tcp_hdr.tcp_dst = htons(dport);
> + b->ip.tcp_hdr.tcp_seq = htonl(tcp_seq_num);
>
> /*
> * TCP window size - TCP header variable tcp_win.
> @@ -340,13 +340,13 @@ int tcp_set_tcp_header(uchar *pkt, int dport, int
> sport, int payload_len,
> * it is, then the u-boot tftp or nfs kernel netboot should be
> * considered.
> */
> - b->ip.hdr.tcp_win = htons(PKTBUFSRX * TCP_MSS >> TCP_SCALE);
> + b->ip.tcp_hdr.tcp_win = htons(PKTBUFSRX * TCP_MSS >> TCP_SCALE);
>
> - b->ip.hdr.tcp_xsum = 0;
> - b->ip.hdr.tcp_ugr = 0;
> + b->ip.tcp_hdr.tcp_xsum = 0;
> + b->ip.tcp_hdr.tcp_ugr = 0;
>
> - b->ip.hdr.tcp_xsum = tcp_set_pseudo_header(pkt, net_ip,
> net_server_ip,
> - tcp_len, pkt_len);
> + b->ip.tcp_hdr.tcp_xsum = tcp_set_pseudo_header(pkt, net_ip,
> net_server_ip,
> + tcp_len, pkt_len);
>
> net_set_ip_header((uchar *)&b->ip, net_server_ip, net_ip,
> pkt_len, IPPROTO_TCP);
> @@ -638,7 +638,7 @@ static u8 tcp_state_machine(u8 tcp_flags, u32
> tcp_seq_num, int payload_len)
> void rxhand_tcp_f(union tcp_build_pkt *b, unsigned int pkt_len)
> {
> int tcp_len = pkt_len - IP_HDR_SIZE;
> - u16 tcp_rx_xsum = b->ip.hdr.ip_sum;
> + u16 tcp_rx_xsum = b->ip.ip_hdr.ip_sum;
> u8 tcp_action = TCP_DATA;
> u32 tcp_seq_num, tcp_ack_num;
> int tcp_hdr_len, payload_len;
> @@ -646,11 +646,11 @@ void rxhand_tcp_f(union tcp_build_pkt *b, unsigned
> int pkt_len)
> /* Verify IP header */
> debug_cond(DEBUG_DEV_PKT,
> "TCP RX in RX Sum (to=%pI4, from=%pI4, len=%d)\n",
> - &b->ip.hdr.ip_src, &b->ip.hdr.ip_dst, pkt_len);
> + &b->ip.ip_hdr.ip_src, &b->ip.ip_hdr.ip_dst, pkt_len);
>
> - b->ip.hdr.ip_src = net_server_ip;
> - b->ip.hdr.ip_dst = net_ip;
> - b->ip.hdr.ip_sum = 0;
> + b->ip.ip_hdr.ip_src = net_server_ip;
> + b->ip.ip_hdr.ip_dst = net_ip;
> + b->ip.ip_hdr.ip_sum = 0;
> if (tcp_rx_xsum != compute_ip_checksum(b, IP_HDR_SIZE)) {
> debug_cond(DEBUG_DEV_PKT,
> "TCP RX IP xSum Error (%pI4, =%pI4, len=%d)\n",
> @@ -659,10 +659,10 @@ void rxhand_tcp_f(union tcp_build_pkt *b, unsigned
> int pkt_len)
> }
>
> /* Build pseudo header and verify TCP header */
> - tcp_rx_xsum = b->ip.hdr.tcp_xsum;
> - b->ip.hdr.tcp_xsum = 0;
> - if (tcp_rx_xsum != tcp_set_pseudo_header((uchar *)b,
> b->ip.hdr.ip_src,
> - b->ip.hdr.ip_dst, tcp_len,
> + tcp_rx_xsum = b->ip.tcp_hdr.tcp_xsum;
> + b->ip.tcp_hdr.tcp_xsum = 0;
> + if (tcp_rx_xsum != tcp_set_pseudo_header((uchar *)b,
> b->ip.ip_hdr.ip_src,
> + b->ip.ip_hdr.ip_dst,
> tcp_len,
> pkt_len)) {
> debug_cond(DEBUG_DEV_PKT,
> "TCP RX TCP xSum Error (%pI4, %pI4, len=%d)\n",
> @@ -670,7 +670,7 @@ void rxhand_tcp_f(union tcp_build_pkt *b, unsigned int
> pkt_len)
> return;
> }
>
> - tcp_hdr_len = GET_TCP_HDR_LEN_IN_BYTES(b->ip.hdr.tcp_hlen);
> + tcp_hdr_len = GET_TCP_HDR_LEN_IN_BYTES(b->ip.tcp_hdr.tcp_hlen);
> payload_len = tcp_len - tcp_hdr_len;
>
> if (tcp_hdr_len > TCP_HDR_SIZE)
> @@ -680,11 +680,11 @@ void rxhand_tcp_f(union tcp_build_pkt *b, unsigned
> int pkt_len)
> * Incoming sequence and ack numbers are server's view of the
> numbers.
> * The app must swap the numbers when responding.
> */
> - tcp_seq_num = ntohl(b->ip.hdr.tcp_seq);
> - tcp_ack_num = ntohl(b->ip.hdr.tcp_ack);
> + tcp_seq_num = ntohl(b->ip.tcp_hdr.tcp_seq);
> + tcp_ack_num = ntohl(b->ip.tcp_hdr.tcp_ack);
>
> /* Packets are not ordered. Send to app as received. */
> - tcp_action = tcp_state_machine(b->ip.hdr.tcp_flags,
> + tcp_action = tcp_state_machine(b->ip.tcp_hdr.tcp_flags,
> tcp_seq_num, payload_len);
>
> tcp_activity_count++;
> @@ -698,8 +698,8 @@ void rxhand_tcp_f(union tcp_build_pkt *b, unsigned int
> pkt_len)
> "TCP Notify (action=%x, Seq=%u,Ack=%u,Pay%d)\n",
> tcp_action, tcp_seq_num, tcp_ack_num,
> payload_len);
>
> - (*tcp_packet_handler) ((uchar *)b + pkt_len - payload_len,
> b->ip.hdr.tcp_dst,
> - b->ip.hdr.ip_src,
> b->ip.hdr.tcp_src, tcp_seq_num,
> + (*tcp_packet_handler) ((uchar *)b + pkt_len - payload_len,
> b->ip.tcp_hdr.tcp_dst,
> + b->ip.ip_hdr.ip_src,
> b->ip.tcp_hdr.tcp_src, tcp_seq_num,
> tcp_ack_num, tcp_action,
> payload_len);
>
> } else if (tcp_action != TCP_DATA) {
> @@ -711,8 +711,8 @@ void rxhand_tcp_f(union tcp_build_pkt *b, unsigned int
> pkt_len)
> * Warning: Incoming Ack & Seq sequence numbers are
> transposed
> * here to outgoing Seq & Ack sequence numbers
> */
> - net_send_tcp_packet(0, ntohs(b->ip.hdr.tcp_src),
> - ntohs(b->ip.hdr.tcp_dst),
> + net_send_tcp_packet(0, ntohs(b->ip.tcp_hdr.tcp_src),
> + ntohs(b->ip.tcp_hdr.tcp_dst),
> (tcp_action & (~TCP_PUSH)),
> tcp_ack_num, tcp_ack_edge);
> }
> diff --git a/test/cmd/wget.c b/test/cmd/wget.c
> index ed83fc94a5..fd6ed26e8a 100644
> --- a/test/cmd/wget.c
> +++ b/test/cmd/wget.c
> @@ -52,9 +52,9 @@ static int sb_syn_handler(struct udevice *dev, void
> *packet,
> {
> struct eth_sandbox_priv *priv = dev_get_priv(dev);
> struct ethernet_hdr *eth = packet;
> - struct ip_tcp_hdr *tcp = packet + ETHER_HDR_SIZE;
> + struct tcp_hdr *tcp = packet + ETHER_HDR_SIZE + IP_HDR_SIZE;
> struct ethernet_hdr *eth_send;
> - struct ip_tcp_hdr *tcp_send;
> + struct tcp_hdr *tcp_send;
>
> /* Don't allow the buffer to overrun */
> if (priv->recv_packets >= PKTBUFSRX)
> @@ -64,7 +64,7 @@ static int sb_syn_handler(struct udevice *dev, void
> *packet,
> memcpy(eth_send->et_dest, eth->et_src, ARP_HLEN);
> memcpy(eth_send->et_src, priv->fake_host_hwaddr, ARP_HLEN);
> eth_send->et_protlen = htons(PROT_IP);
> - tcp_send = (void *)eth_send + ETHER_HDR_SIZE;
> + tcp_send = (void *)eth_send + ETHER_HDR_SIZE + IP_HDR_SIZE;
> tcp_send->tcp_src = tcp->tcp_dst;
> tcp_send->tcp_dst = tcp->tcp_src;
> tcp_send->tcp_seq = htonl(0);
> @@ -97,9 +97,9 @@ static int sb_ack_handler(struct udevice *dev, void
> *packet,
> {
> struct eth_sandbox_priv *priv = dev_get_priv(dev);
> struct ethernet_hdr *eth = packet;
> - struct ip_tcp_hdr *tcp = packet + ETHER_HDR_SIZE;
> + struct tcp_hdr *tcp = packet + ETHER_HDR_SIZE + IP_HDR_SIZE;
> struct ethernet_hdr *eth_send;
> - struct ip_tcp_hdr *tcp_send;
> + struct tcp_hdr *tcp_send;
> void *data;
> int pkt_len;
> int payload_len = 0;
> @@ -115,7 +115,7 @@ static int sb_ack_handler(struct udevice *dev, void
> *packet,
> memcpy(eth_send->et_dest, eth->et_src, ARP_HLEN);
> memcpy(eth_send->et_src, priv->fake_host_hwaddr, ARP_HLEN);
> eth_send->et_protlen = htons(PROT_IP);
> - tcp_send = (void *)eth_send + ETHER_HDR_SIZE;
> + tcp_send = (void *)eth_send + ETHER_HDR_SIZE + IP_HDR_SIZE;
> tcp_send->tcp_src = tcp->tcp_dst;
> tcp_send->tcp_dst = tcp->tcp_src;
> data = (void *)tcp_send + IP_TCP_HDR_SIZE;
> @@ -163,14 +163,14 @@ static int sb_http_handler(struct udevice *dev, void
> *packet,
> {
> struct ethernet_hdr *eth = packet;
> struct ip_hdr *ip;
> - struct ip_tcp_hdr *tcp;
> + struct tcp_hdr *tcp;
>
> if (ntohs(eth->et_protlen) == PROT_ARP) {
> return sb_arp_handler(dev, packet, len);
> } else if (ntohs(eth->et_protlen) == PROT_IP) {
> ip = packet + ETHER_HDR_SIZE;
> if (ip->ip_p == IPPROTO_TCP) {
> - tcp = packet + ETHER_HDR_SIZE;
> + tcp = ip + IP_HDR_SIZE;
> if (tcp->tcp_flags == TCP_SYN)
> return sb_syn_handler(dev, packet, len);
> else if (tcp->tcp_flags & TCP_ACK &&
> !(tcp->tcp_flags & TCP_SYN))
> --
> 2.40.1.521.gf1e218fcd8-goog
>
>
