[PATCH rtems-libbsd 2/2] microblaze: Finish AXI Ethernet support

[Sebastian Huber]

On 24/01/2022 17:04, Alex White wrote:
> On Thu, Jan 20, 2022 at 12:12 AM Sebastian Huber<sebastian.huber at embedded-brains.de>  wrote:
>> On 20/01/2022 04:54, Alex White wrote:
>>> diff --git a/freebsd/sys/netinet/tcp_input.c b/freebsd/sys/netinet/tcp_input.c
>>> index fc111d9c..60f9123a 100644
>>> --- a/freebsd/sys/netinet/tcp_input.c
>>> +++ b/freebsd/sys/netinet/tcp_input.c
>>> @@ -721,6 +721,15 @@ tcp_input(struct mbuf **mp, int *offp, int proto)
>>>        }
>>>    #endif /* INET */
>>>   
>>> +#ifdef __rtems__
>>> +#ifdef __MICROBLAZE__
>>> +     /* Ensure that the TCP header is properly aligned in memory. */
>>> +     struct tcphdr aligned_hdr;
>>> +     memcpy(&aligned_hdr, th, sizeof(struct tcphdr));
>>> +     th = &aligned_hdr;
>>> +#endif
>>> +#endif
>>> +
>>>        /*
>>>         * Check that TCP offset makes sense,
>>>         * pull out TCP options and adjust length.              XXX
>> Changes like this should not be hidden in a driver-specific patch. The
>> alignment changes are hacks. Why don't you fix the Ethernet frame
>> alignment in the network interface driver?
>>
> I have made the realization while testing fixes for this that if I
> change the definition of struct tcphdr in netinet/tcp.h to include
> __attribute__((packed)), GCC generates code that avoids the unaligned
> memory access. This removes the need for any code to handle memory
> alignment of the network headers.
> 
> I then noticed that you had some notes on what I think is this same
> phenomenon at freebsd/contrib/libpcap/extract.h line 40.
> 
> Since, for example, the ip struct in netinet/ip.h is defined as packed,
> shouldn't the tcphdr struct be packed, too?
> 
> In addition, struct tcphdr is being used to represent raw bytes on the
> wire so it makes sense to declare it as packed so no padding is
> included.
> 
> The initial solution I came up with was to change
> newlib/libc/sys/rtems/include/netinet/tcp.h in newlib to add
> __attribute__((packed)) to struct tcphdr . This could be added inside
> an #ifdef __microblaze__ to minimize impact elsewhere. This would, of
> course, diverge from FreeBSD's netinet/tcp.h.
> 
> Does that sound like a reasonable solution?

No, I would try to fix this in the network interface driver. See for 
example (freebsd/sys/dev/dwc/if_dwc.c):

static int
dwc_setup_rxbuf(struct dwc_softc *sc, int idx, struct mbuf *m)
{
	bus_dma_segment_t seg;
#ifndef __rtems__
	int error, nsegs;
#endif /* __rtems__ */

	m_adj(m, ETHER_ALIGN);

Several other drivers use the same approach.

-- 
embedded brains GmbH
Herr Sebastian HUBER
Dornierstr. 4
82178 Puchheim
Germany
email: sebastian.huber at embedded-brains.de
phone: +49-89-18 94 741 - 16
fax:   +49-89-18 94 741 - 08

Registergericht: Amtsgericht München
Registernummer: HRB 157899
Vertretungsberechtigte Geschäftsführer: Peter Rasmussen, Thomas Dörfler
Unsere Datenschutzerklärung finden Sie hier:
https://embedded-brains.de/datenschutzerklaerung/