[U-Boot] [PATCH] Add simple IP/UDP fragmentation support

[Ben Warren <biggerbadderben@gmail.com>]

Hi Josh & Frank,

A few nits.  Overall a nice patch.

One question: Will multicast TFTP still work when this is applied?

Josh Boyer wrote:
> From: Frank Haverkamp <haver@vnet.ibm.com>
>
> http://tools.ietf.org/html/rfc2348 describes the TFTP block size option
> which allows larger packtes than the 512 byte default. This reduces the
>   
s/packtes/packets/
> number of TFTP ACKs significantly and improves performance.
>
> To get the most benefit out of the tftp block size option the support
> of defragementation of IP/UDP packet is helpful. The current implemenation
>   
s/defragementation/defragmentation/
s/implemenation/implementation/

> should work even with packets received out of order. To enable the large
> packet size the user should set "tftp_block_size" so a value like 16352.
>
>   
s/so/to/
> We experimented with different packet sizes and found that more than those
> 16KiB do not contribute much to the performance anymore. Therefor I limited
>   
s/Therefor/Therefore/
> the defragmentation buffer to 16KiB no too waste memory.
>
>   
so as to not waste memory
> Signed-off-by: Frank Haverkamp <haver@vnet.ibm.com>
> Signed-off-by: Josh Boyer <jwboyer@linux.vnet.ibm.com>
>
> ---
>  include/net.h |   17 ++++++
>  net/net.c     |  156 ++++++++++++++++++++++++++++++++++++++++++++++++++--------
>  net/tftp.c    |   22 ++++++++
>  net/tftp.h    |   10 +++
>  4 files changed, 185 insertions(+), 20 deletions(-)
>
> --- u-boot.git.orig/include/net.h
> +++ u-boot.git/include/net.h
> @@ -200,6 +200,13 @@ typedef struct {
>  	ushort		udp_xsum;	/* Checksum			*/
>  } IP_t;
>  
> +#define IP_OFFS			 0x1FFF	/* ip offset *= 8 */
> +#define	  IP_OFFS_SHIFT	  3	/* in 8 byte steps */
> +#define IP_FLAGS	 0xE000	/* first 3 bits */
> +#define		 IP_FLAGS_RES	 0x8000	/* reserved */
> +#define		 IP_FLAGS_DFRAG	 0x4000	/* don't fragments */
> +#define		 IP_FLAGS_MFRAG	 0x2000	/* more fragments */
> +
>   
Please align these better.  After applying, it's:

#define IP_OFFS			 0x1FFF	/* ip offset *= 8 */
#define	  IP_OFFS_SHIFT	  3	/* in 8 byte steps */
#define IP_FLAGS	 0xE000	/* first 3 bits */
#define		 IP_FLAGS_RES	 0x8000	/* reserved */
#define		 IP_FLAGS_DFRAG	 0x4000	/* don't fragments */
#define		 IP_FLAGS_MFRAG	 0x2000	/* more fragments */


>  #define IP_HDR_SIZE_NO_UDP	(sizeof (IP_t) - 8)
>  #define IP_HDR_SIZE		(sizeof (IP_t))
>  
> @@ -282,6 +289,16 @@ typedef struct icmphdr {
>  #define PKTSIZE_ALIGN		1536
>  /*#define PKTSIZE		608*/
>  
> + /*
> +  * IP/UDP Fragmentation support
> +  * See: http://en.wikipedia.org/wiki/IPv4#Fragmentation_and_reassembly
> +  * MAX possible UDP packet size is 64 KiB, if there is memory available.
> +  */
> +#define NET_ETH_MTU		1500
> +#define NET_FRAG_BUF_SIZE	(16 * 1024) /* MAX is 64 KiB */
> +#define NET_UDP_FRAG_SIZE	(NET_ETH_MTU - IP_HDR_SIZE_NO_UDP) /* 1480 */
> +#define NET_FRAG_BUF_USED	(NET_FRAG_BUF_SIZE / NET_UDP_FRAG_SIZE + 1)
> +
>  /*
>   * Maximum receive ring size; that is, the number of packets
>   * we can buffer before overflow happens. Basically, this just
> --- u-boot.git.orig/net/net.c
> +++ u-boot.git/net/net.c
> @@ -192,6 +192,15 @@ volatile uchar	PktBuf[(PKTBUFSRX+1) * PK
>  
>  volatile uchar *NetRxPackets[PKTBUFSRX]; /* Receive packets			*/
>  
> +/* Packet fragmentation support */
> +static uint16_t ip_id = 0; /* sequence number */
> +static uint16_t udp_len = 0;
> +static uint16_t udp_src = 0;
> +static uint16_t udp_dst = 0;
> +static int max_idx = 0;
> +static uchar NetFragBuf[NET_FRAG_BUF_SIZE];
> +static char NetFragBufUsed[NET_FRAG_BUF_USED] = { 0, };
> +
>  static rxhand_f *packetHandler;		/* Current RX packet handler		*/
>  static thand_f *timeHandler;		/* Current timeout handler		*/
>  static ulong	timeStart;		/* Time base value			*/
> @@ -288,6 +297,13 @@ NetLoop(proto_t protocol)
>  {
>  	bd_t *bd = gd->bd;
>  
> +	/* Packet fragmentation support */
> +	ip_id = udp_len = udp_src = udp_dst = max_idx = 0;
> +	memset(NetFragBuf, 0xFF, sizeof(NetFragBuf));
> +	memset(NetFragBufUsed, 0, sizeof(NetFragBufUsed));
> +	printf("NetFragBuf @ %08x max tftp_block_size=%d udp_frag_size=%d\n",
> +	       NetFragBuf, TFTP_BLOCK_SIZE_MAX, NET_UDP_FRAG_SIZE);
> +
>  #ifdef CONFIG_NET_MULTI
>  	NetRestarted = 0;
>  	NetDevExists = 0;
> @@ -1150,6 +1166,39 @@ static void CDPStart(void)
>  }
>  #endif
>  
> +#ifdef CONFIG_UDP_CHECKSUM
> +/*
> + * @sumptr: Points to UDP data
> + * @sumlen: Size of UDP data
> + * @xsum: UDP checksum across IP source, destination address, protocol and size
> + *
> + * Returns 0 when checksum is correct and 1 if it is not
>   
Can you return -1 on failure?  Not a big deal, just more conventional.
> + */
> +static int udp_checksum(ushort *sumptr, ushort sumlen, ulong xsum)
> +{
> +       while (sumlen > 1) {
> +	       ushort sumdata;
> +
> +	       sumdata = *sumptr++;
> +	       xsum += ntohs(sumdata);
> +	       sumlen -= 2;
> +       }
> +       if (sumlen > 0) {
> +	       ushort sumdata;
> +
> +	       sumdata = *(unsigned char *) sumptr;
> +	       sumdata = (sumdata << 8) & 0xff00;
> +	       xsum += sumdata;
> +       }
> +       while ((xsum >> 16) != 0) {
> +	       xsum = (xsum & 0x0000ffff) + ((xsum >> 16) & 0x0000ffff);
> +       }
> +       if ((xsum != 0x00000000) && (xsum != 0x0000ffff))
> +	       return 1;
> +
> +       return 0;
> +}
> +#endif /* CONFIG_UDP_CHECKSUM */
>  
>  void
>  NetReceive(volatile uchar * inpkt, int len)
> @@ -1164,6 +1213,7 @@ NetReceive(volatile uchar * inpkt, int l
>  	int iscdp;
>  #endif
>  	ushort cti = 0, vlanid = VLAN_NONE, myvlanid, mynvlanid;
> +	uint32_t off;		/* ip_off for fragmentation */
>   
Can you pick a better variable name than 'off'?  'offset' maybe?
>  
>  #ifdef ET_DEBUG
>  	printf("packet received\n");
> @@ -1404,9 +1454,11 @@ NetReceive(volatile uchar * inpkt, int l
>  		if ((ip->ip_hl_v & 0xf0) != 0x40) {
>  			return;
>  		}
> +#if 0		/* Obsolete after adding the fragmentation support */
>  		if (ip->ip_off & htons(0x1fff)) { /* Can't deal w/ fragments */
>  			return;
>  		}
> +#endif
>   
Please delete this block if it's now dead code.
>  		/* can't deal with headers > 20 bytes */
>  		if ((ip->ip_hl_v & 0x0f) > 0x05) {
>  			return;
> @@ -1422,6 +1474,88 @@ NetReceive(volatile uchar * inpkt, int l
>  #endif
>  			return;
>  		}
> +
> +		/*
> +		 * Fragmentation support. We need to check the ip_id
> +		 * and if all fragments were received correctly.
> +		 */
> +		off = (ntohs(ip->ip_off) & IP_OFFS) << IP_OFFS_SHIFT;
> +		if ((off != 0) || (ip->ip_off & htons(IP_FLAGS_MFRAG))) {
> +			int size, idx, complete;
> +			char *start;
> +
> +			/* New fragmented packet arrived, clear data. */
> +			if (ntohs(ip->ip_id) != ip_id) {
> +				ip_id = ntohs(ip->ip_id);
> +				memset(NetFragBufUsed, 0, sizeof(NetFragBufUsed));
> +				udp_len = udp_src = udp_dst = max_idx = 0;
> +			}
> +
> +			idx = off / NET_UDP_FRAG_SIZE;
> +
> +			/* Packet does not fit into IP/UDP fragmentation buf */
> +			if (idx >= NET_FRAG_BUF_USED) {
> +				return;
> +			}
> +
> +			NetFragBufUsed[idx] = 1;
> +
> +			/* Copy the UDP hdr with the data for 1st
> +			   fragment, else copy just payload */
> +			if (off == 0) {
> +				udp_len = ntohs(ip->udp_len);
> +				udp_src = ntohs(ip->udp_src);
> +				udp_dst = ntohs(ip->udp_dst);
> +			}
> +			size = ntohs(ip->ip_len) - IP_HDR_SIZE_NO_UDP;
> +			start = (char *)ip + IP_HDR_SIZE_NO_UDP;
> +			memcpy(NetFragBuf + off, start, size);
> +
> +			/*
> +			 * When last fragement has been received we
>   
s/fragement/fragment/
> +			 * know the number of fragments we expect. If
> +			 * all have arrived we process the packet.
> +			 */
> +			if (((off != 0) && !(ip->ip_off & htons(IP_FLAGS_MFRAG))))
> +				max_idx = idx;
> +
> +			if (max_idx == 0)
> +				return;
> +
> +			complete = 1;
> +			for (idx = 0; idx < max_idx; idx++) {
> +				if (NetFragBufUsed[idx] == 0) {
> +					complete = 0;
> +					break;
> +				}
> +			}
> +			if (!complete)
> +				return;
> +#ifdef CONFIG_UDP_CHECKSUM
> +			if (ip->udp_xsum != 0) {
> +				ulong xsum = ip->ip_p;
> +				uint16_t *sumptr;
> +
> +				xsum += udp_len;
> +				xsum += (ntohl(ip->ip_src) >> 16) & 0xffff;
> +				xsum += (ntohl(ip->ip_src) >>  0) & 0xffff;
> +				xsum += (ntohl(ip->ip_dst) >> 16) & 0xffff;
> +				xsum += (ntohl(ip->ip_dst) >>  0) & 0xffff;
> +				sumptr = (ushort *)NetFragBuf;
> +
> +				if (udp_checksum(sumptr, udp_len, xsum)) {
> +					putc('U');
> +					return;
> +				}
> +			}
> +#endif /* CONFIG_UDP_CHECKSUM */
> +			(*packetHandler)(NetFragBuf + 8,
> +					 udp_dst,
> +					 udp_src,
> +					 udp_len - 8);
>   
Can some of these arguments share lines?
> +			return;
> +		}
> +
>  		/*
>  		 * watch for ICMP host redirects
>  		 *
> @@ -1502,26 +1636,8 @@ NetReceive(volatile uchar * inpkt, int l
>  			sumlen = ntohs(ip->udp_len);
>  			sumptr = (ushort *) &(ip->udp_src);
>  
> -			while (sumlen > 1) {
> -				ushort sumdata;
> -
> -				sumdata = *sumptr++;
> -				xsum += ntohs(sumdata);
> -				sumlen -= 2;
> -			}
> -			if (sumlen > 0) {
> -				ushort sumdata;
> -
> -				sumdata = *(unsigned char *) sumptr;
> -				sumdata = (sumdata << 8) & 0xff00;
> -				xsum += sumdata;
> -			}
> -			while ((xsum >> 16) != 0) {
> -				xsum = (xsum & 0x0000ffff) + ((xsum >> 16) & 0x0000ffff);
> -			}
> -			if ((xsum != 0x00000000) && (xsum != 0x0000ffff)) {
> -				printf(" UDP wrong checksum %08lx %08x\n",
> -					xsum, ntohs(ip->udp_xsum));
> +			if (udp_checksum(sumptr, sumlen, xsum)) {
> +				putc('U');
>  				return;
>  			}
>  		}
> --- u-boot.git.orig/net/tftp.c
> +++ u-boot.git/net/tftp.c
> @@ -456,6 +456,7 @@ TftpTimeout (void)
>  void
>  TftpStart (void)
>  {
> +	char *s, *err;
>  #ifdef CONFIG_TFTP_PORT
>  	char *ep;             /* Environment pointer */
>  #endif
> @@ -518,6 +519,27 @@ TftpStart (void)
>  
>  	puts ("Loading: *\b");
>  
> +	/* Get alternate tftp_block_size */
> +	if ((s = getenv("tftp_block_size")) != NULL) {
>   
Kind of a long environment variable name.  Maybe "tftp_bs"?
> +		err = NULL;
> +
> +		TftpBlkSizeOption = simple_strtoul(s, &err, 10);
> +		if (*err) {
> +			printf("ERR: \"tftp_block_size\" is not a number\n");
> +			TftpBlkSizeOption = TFTP_BLOCK_SIZE;
> +		}
> +		/*
> +		 * Reject values which require extensive handling.
> +		 * block size of 1428 octets (Ethernet MTU, less
> +		 * the TFTP, UDP and IP header lengths).
> +		 */
> +		if (TftpBlkSizeOption > TFTP_BLOCK_SIZE_MAX) {
> +			printf("ERR: tftp_block_sizes larger than %d not "
> +			       "supported\n", TFTP_BLOCK_SIZE_MAX);
> +			TftpBlkSizeOption = TFTP_BLOCK_SIZE;
> +		}
> +	}
> +
>  	NetSetTimeout (TIMEOUT * CFG_HZ, TftpTimeout);
>  	NetSetHandler (TftpHandler);
>  
> --- u-boot.git.orig/net/tftp.h
> +++ u-boot.git/net/tftp.h
> @@ -8,11 +8,21 @@
>  #ifndef __TFTP_H__
>  #define __TFTP_H__
>  
> +#include <net.h>
> +
>  /**********************************************************************/
>  /*
>   *	Global functions and variables.
>   */
>  
> +/*
> + * Maximum TFTP block size bound to max size of fragmented IP/UDP
> + * packets minus TFTP and UDP/IP overhead. TFTP overhead is 2 byte
> + * opcode and 2 byte block-number.
> + */
> +#define TFTP_BLOCK_SIZE_MAX     (NET_FRAG_BUF_SIZE - sizeof(IP_t) - 4)
> +
> +
>  /* tftp.c */
>  extern void	TftpStart (void);	/* Begin TFTP get */
>  
>
>   
regards,
Ben