Re: Unaligned access fault in fxp on alpha

On Fri, 9 May 2003, Andrew Gallatin wrote:

> Can you try this patch please?
>
> It causes gcc to emit slightly different code, which deals with
> storing to aligned 16-bit values.
>
> What's happening is that because the u_int32_t link_addr (and rbd_addr)
> fields preceded the "size" field, gcc was assuming that the rfa struct
> would be aligned and was cheating.  It was using operations which only
> work on aligned-32 bit values on 16-bit values.  Removing the
> u_int32_t's disabuses gcc of this assumption, therby causing safe
> code to be emitted.

This is a valid assumption.  The struct apparently _never_ 32-bit aligned,
so it can't have any u_int32_t's in it.  The bug is hidden using bogus
casts.  From if_fxp.c:

%%%
/*
 * NOTE!  On the Alpha, we have an alignment constraint.  The
 * card DMAs the packet immediately following the RFA.  However,
 * the first thing in the packet is a 14-byte Ethernet header.
 * This means that the packet is misaligned.  To compensate,
 * we actually offset the RFA 2 bytes into the cluster.  This
 * alignes the packet after the Ethernet header at a 32-bit
 * boundary.  HOWEVER!  This means that the RFA is misaligned!
 */
#define	RFA_ALIGNMENT_FUDGE	2
...
		m = rxp->rx_mbuf;
		rfa = (struct fxp_rfa *)(m->m_ext.ext_buf +
		    RFA_ALIGNMENT_FUDGE);
	...
	rfa = mtod(m, struct fxp_rfa *);
	m->m_data += sc->rfa_size;
	rfa->size = htole16(MCLBYTES - sc->rfa_size - RFA_ALIGNMENT_FUDGE);
	...
		p_rfa = (struct fxp_rfa *)
		    (p_rx->rx_mbuf->m_ext.ext_buf + RFA_ALIGNMENT_FUDGE);
%%%

> I don't understand why mux changed these fields in rev 1.31, with, so
> I'm not sure that I want to commit this until mux reviews it.  For all
> I know, it breaks sparc64 or something..

Rev.1.32 actually.  Seems to be just wrong.

> Index: dev/fxp/if_fxpreg.h
> ===================================================================
> RCS file: /home/ncvs/src/sys/dev/fxp/if_fxpreg.h,v
> retrieving revision 1.33
> diff -u -r1.33 if_fxpreg.h
> --- dev/fxp/if_fxpreg.h	6 Apr 2003 21:35:45 -0000	1.33
> +++ dev/fxp/if_fxpreg.h	9 May 2003 18:55:10 -0000
> _at__at_ -346,8 +346,8 _at__at_
>  struct fxp_rfa {
>  	u_int16_t rfa_status;
>  	u_int16_t rfa_control;
> -	u_int32_t link_addr;
> -	u_int32_t rbd_addr;
> +	u_int8_t link_addr[4];
> +	u_int8_t rbd_addr[4];
>  	u_int16_t actual_size;
>  	u_int16_t size;

This may work, since the non-u_int32_t fields seem to be all set using
encoding functions that have weak enough type checking to work on both
pointers to u_int_32_t and pointers to u_int8_t.

The change seems to have been inspired by endianness fixes.  In rev.1.144
of if_fxp.c, the non-u_int32_t fields were set by fxp_lwcopy() which did
differently bogus casts and then set the fields using direct assignment
on i386's and using code that only worked in the little-endian case on
other machines.

Bruce