On 12-10-05 12:01 PM, Rob Herring wrote:
On 10/05/2012 08:51 AM, Mikael Pettersson wrote:
Rob Herring writes:
> On 10/05/2012 03:24 AM, Russell King - ARM Linux wrote:
> > On Fri, Oct 05, 2012 at 09:20:56AM +0100, Mans Rullgard wrote:
> >> On 5 October 2012 08:12, Russell King - ARM Linux
> >> <li...@arm.linux.org.uk> wrote:
> >>> On Fri, Oct 05, 2012 at 03:25:16AM +0100, Mans Rullgard wrote:
> >>>> On 5 October 2012 02:56, Rob Herring <robherri...@gmail.com> wrote:
> >>>>> This struct is the IP header, so a struct ptr is just set to the
> >>>>> beginning of the received data. Since ethernet headers are 14 bytes,
> >>>>> often the IP header is not aligned unless the NIC can place the frame
at
> >>>>> a 2 byte offset (which is something I need to investigate). So this
> >>>>> function cannot make any assumptions about the alignment. Does the ABI
> >>>>> define structs have some minimum alignment? Does the struct need to be
> >>>>> declared as packed or something?
> >>>>
> >>>> The ABI defines the alignment of structs as the maximum alignment of
its
> >>>> members. Since this struct contains 32-bit members, the alignment for
the
> >>>> whole struct becomes 32 bits as well. Declaring it as packed tells
gcc it
> >>>> might be unaligned (in addition to removing any holes within).
> >>>
> >>> This has come up before in the past.
> >>>
> >>> The Linux network folk will _not_ allow - in any shape or form - for
> >>> this struct to be marked packed (it's the struct which needs to be
> >>> marked packed) because by doing so, it causes GCC to issue byte loads/
> >>> stores on architectures where there isn't a problem, and that decreases
> >>> the performance of the Linux IP stack unnecessarily.
> >>
> >> Which architectures? I have never seen anything like that.
> >
> > Does it matter? I'm just relaying the argument against adding __packed
> > which was used before we were forced (by the networking folk) to implement
> > the alignment fault handler.
>
> It doesn't really matter what will be accepted or not as adding __packed
> to struct iphdr doesn't fix the problem anyway. gcc still emits a ldm.
> The only way I've found to eliminate the alignment fault is adding a
> barrier between the 2 loads. That seems like a compiler issue to me if
> there is not a better fix.
If you suspect a GCC bug, please prepare a standalone user-space test case
and submit it to GCC's bugzilla (I can do the latter if you absolutely do not
want to). It wouldn't be the first alignment-related GCC bug...
Here's a testcase. Compiled on ubuntu precise with
"arm-linux-gnueabihf-gcc -O2 -marm -march=armv7-a test.c".
typedef unsigned short u16;
typedef unsigned short __sum16;
typedef unsigned int __u32;
typedef unsigned char __u8;
typedef __u32 __be32;
typedef u16 __be16;
struct iphdr {
__u8 ihl:4,
version:4;
__u8 tos;
__be16 tot_len;
__be16 id;
__be16 frag_off;
__u8 ttl;
__u8 protocol;
__sum16 check;
__be32 saddr;
__be32 daddr;
/*The options start here. */
};
I was reading this thread with some interest. AFAIK, with the default
alignment rules the above struct is packed; there will be no holes in it.
#define ntohl(x) __swab32((__u32)(__be32)(x))
#define IP_DF 0x4000 /* Flag: "Don't Fragment" */
static inline __attribute__((const)) __u32 __swab32(__u32 x)
{
__asm__ ("rev %0, %1" : "=r" (x) : "r" (x));
return x;
}
int main(void * buffer, unsigned int *p_id)
{
unsigned int id;
int flush = 1;
const struct iphdr *iph = buffer;
__u32 len = *p_id;
id = ntohl(*(__be32 *)&iph->id);
The above statement is the problem. I think it is poorly written
networking code. It takes the address of a 16 bit quantity (aligned on
a halfword address), attempts to do a type conversion using pointers,
then dereference it. I would have thought:
id = ntohs(iph->id);
would have been enough.
Scott
--
Scott Bambrough
Technical Director, Member Services
Linaro Ltd.
email: scott.bambro...@linaro.org
irc: scottb (freenode, irc.linaro.org)
web: http://www.linaro.org
Linaro: The future of Linux on ARM.
_______________________________________________
linaro-toolchain mailing list
linaro-toolchain@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-toolchain
