Re: [U-Boot] IP_t should be a packed struct
Hi ML,
I noticed that also the others headers structs have the same potential
problem.
I attached the patch to fix the net.h header file.
best regards,
luigi
On Tuesday 27 January 2009 19:32:10 Luigi 'Comio' Mantellini wrote:
Hi ML,
I'm working on a mips target and I used qemu_mips target to simulate my
target (that I hope to have in the next week...)
Following my activities I noticed that IP_t structure is no defined with
attribute packed. I noticed this issue because using a self-made
toolchain (gcc4.2.4+binutils2.8+uclibc0.9.30) the compiler has aligned all
bytes to 32bit boundary. This is not ok, because the packets IP_t can be
non aligned (see the /net/net.c PingSend function, for an example).
The dirty solution is to define the structure with the
__attribute__((__packed__))... but, from my point of view, a better packet
forging mechanism should be implemented into the net.c stack.
I attached a trivial patch that solved the issue on my target.
Any comments is welcome.
best regards,
luigi
--
Luigi Mantellini
RD - Software
Industrie Dial Face S.p.A.
Via Canzo, 4
20068 Peschiera Borromeo (MI), Italy
Tel.: +39 02 5167 2813
Fax: +39 02 5167 2459
Email: luigi.mantell...@idf-hit.com
diff --git a/include/net.h b/include/net.h
index d2d394f..f14df6c 100644
--- a/include/net.h
+++ b/include/net.h
@@ -154,7 +154,7 @@ typedef struct {
uchar et_snap2;
uchar et_snap3;
ushort et_prot; /* 802 protocol */
-} Ethernet_t;
+} __attribute__((__packed__)) Ethernet_t;
#define ETHER_HDR_SIZE 14 /* Ethernet header size */
#define E802_HDR_SIZE 22 /* 802 ethernet header size */
@@ -168,7 +168,7 @@ typedef struct {
ushort vet_vlan_type; /* PROT_VLAN */
ushort vet_tag; /* TAG of VLAN */
ushort vet_type; /* protocol type */
-} VLAN_Ethernet_t;
+} __attribute__((__packed__)) VLAN_Ethernet_t;
#define VLAN_ETHER_HDR_SIZE 18 /* VLAN Ethernet header size */
@@ -198,7 +198,7 @@ typedef struct {
ushort udp_dst; /* UDP destination port */
ushort udp_len; /* Length of UDP packet */
ushort udp_xsum; /* Checksum */
-} IP_t;
+} __attribute__((__packed__)) IP_t;
#define IP_OFFS 0x1fff /* ip offset *= 8 */
#define IP_FLAGS 0xe000 /* first 3 bits */
@@ -239,7 +239,7 @@ typedef struct
uchar ar_tha[]; /* Target hardware address */
uchar ar_tpa[]; /* Target protocol address */
#endif /* 0 */
-} ARP_t;
+} __attribute__((__packed__)) ARP_t;
#define ARP_HDR_SIZE (8+20) /* Size assuming ethernet */
@@ -269,7 +269,7 @@ typedef struct icmphdr {
ushort mtu;
} frag;
} un;
-} ICMP_t;
+} __attribute__((__packed__)) ICMP_t;
/*
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] IP_t should be a packed struct
Luigi 'Comio' Mantellini wrote: Hi ML, I'm working on a mips target and I used qemu_mips target to simulate my target (that I hope to have in the next week...) Following my activities I noticed that IP_t structure is no defined with attribute packed. I noticed this issue because using a self-made toolchain (gcc4.2.4+binutils2.8+uclibc0.9.30) the compiler has aligned all bytes to 32bit boundary. This is not ok, because the packets IP_t can be non aligned (see the /net/net.c PingSend function, for an example). Why is your compiler aligning all bytes to 32-bit boundary? Seems like an awful waste of space. This struct should pack itself nicely, and does on the small sample of toolchains I've tried (gcc 4.3.2 x86_64 and gcc 4.0.0 ppc_4xx). The dirty solution is to define the structure with the __attribute__((__packed__))... but, from my point of view, a better packet forging mechanism should be implemented into the net.c stack. I attached a trivial patch that solved the issue on my target. Any comments is welcome. best regards, luigi I'd focus on fixing your toolchain. Your problem will not be confined to protocol headers. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot regards, Ben ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] IP_t should be a packed struct
Ben Warren wrote: Luigi 'Comio' Mantellini wrote: Hi ML, I'm working on a mips target and I used qemu_mips target to simulate my target (that I hope to have in the next week...) Following my activities I noticed that IP_t structure is no defined with attribute packed. I noticed this issue because using a self-made toolchain (gcc4.2.4+binutils2.8+uclibc0.9.30) the compiler has aligned all bytes to 32bit boundary. This is not ok, because the packets IP_t can be non aligned (see the /net/net.c PingSend function, for an example). Why is your compiler aligning all bytes to 32-bit boundary? Seems like an awful waste of space. This struct should pack itself nicely, and does on the small sample of toolchains I've tried (gcc 4.3.2 x86_64 and gcc 4.0.0 ppc_4xx). The compiler is optimizing for speed and/or execution size at the expense of larger data structures either by command (e.g. a -O option) or as part of the compiler writer's choice. CPUs almost always execute code significantly faster when the data is properly aligned. Many CPUs require software to deal with the misalignment which costs code space and execution time. Since the compiler wasn't instructed that the IP headers needed to be packed, it is within the compiler's right to not pack them. [snip] Best regards, gvb ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] IP_t should be a packed struct
Jerry Van Baren wrote: Ben Warren wrote: Luigi 'Comio' Mantellini wrote: Hi ML, I'm working on a mips target and I used qemu_mips target to simulate my target (that I hope to have in the next week...) Following my activities I noticed that IP_t structure is no defined with attribute packed. I noticed this issue because using a self-made toolchain (gcc4.2.4+binutils2.8+uclibc0.9.30) the compiler has aligned all bytes to 32bit boundary. This is not ok, because the packets IP_t can be non aligned (see the /net/net.c PingSend function, for an example). Why is your compiler aligning all bytes to 32-bit boundary? Seems like an awful waste of space. This struct should pack itself nicely, and does on the small sample of toolchains I've tried (gcc 4.3.2 x86_64 and gcc 4.0.0 ppc_4xx). The compiler is optimizing for speed and/or execution size at the expense of larger data structures either by command (e.g. a -O option) or as part of the compiler writer's choice. CPUs almost always execute code significantly faster when the data is properly aligned. Many CPUs require software to deal with the misalignment which costs code space and execution time. Since the compiler wasn't instructed that the IP headers needed to be packed, it is within the compiler's right to not pack them. Sure. In this case, though, the optimization's not necessarily going to gain anything since we never use a raw struct IP_t, only pointers that overlay other char arrays through casting. Of course there's no point discussing this much further here, but I suspect that this packing problem will exist in many more places than the protocol headers. [snip] Best regards, gvb regards, Ben ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] IP_t should be a packed struct
Dear All
From my point of view, when packing is formally required (ie packets
headers), the structs should be declared explicitly as __packed__. The
correctness of the object code should be independent from the compiler
optimizations and we should always remember that the offset of a
struct field is not necessarily the sum of the sizes of the fields
that precede it.
struct {
type1 a;
type2 b;
type3 c;
} mystruct
offset(mystruct.c) != sizeof(type1) + sizeof(type2).
Regarding the CFLAGS used by me... I haven't set any CFLAGS and I just
do a make qemu_mips_config CROSS_COMPILE=mips-linux- make
CROSS_COMPILE=mips-linux... and a boundary alignment is not an alien
choice for a good compiler (a standard gcc4.2.4 in my case).
Furthermore I expect a correct object always... with any -Ox flag (a
apart bugs... of course).
My idea should be to declare a define like this
#define PKT_HEADER __attribute__((__packed__))
my 2EuroCents.
best regards,
luigi
2009/1/28 Ben Warren biggerbadder...@gmail.com:
Jerry Van Baren wrote:
Ben Warren wrote:
Luigi 'Comio' Mantellini wrote:
Hi ML,
I'm working on a mips target and I used qemu_mips target to simulate my
target (that I hope to have in the next week...)
Following my activities I noticed that IP_t structure is no defined with
attribute packed. I noticed this issue because using a self-made
toolchain
(gcc4.2.4+binutils2.8+uclibc0.9.30) the compiler has aligned all bytes to
32bit boundary. This is not ok, because the packets IP_t can be non aligned
(see the /net/net.c PingSend function, for an example).
Why is your compiler aligning all bytes to 32-bit boundary? Seems like
an awful waste of space. This struct should pack itself nicely, and does on
the small sample of toolchains I've tried (gcc 4.3.2 x86_64 and gcc 4.0.0
ppc_4xx).
The compiler is optimizing for speed and/or execution size at the expense
of larger data structures either by command (e.g. a -O option) or as part of
the compiler writer's choice. CPUs almost always execute code significantly
faster when the data is properly aligned. Many CPUs require software to
deal with the misalignment which costs code space and execution time.
Since the compiler wasn't instructed that the IP headers needed to be
packed, it is within the compiler's right to not pack them.
Sure. In this case, though, the optimization's not necessarily going to
gain anything since we never use a raw struct IP_t, only pointers that
overlay other char arrays through casting. Of course there's no point
discussing this much further here, but I suspect that this packing problem
will exist in many more places than the protocol headers.
[snip]
Best regards,
gvb
regards,
Ben
--
Luigi 'Comio' Mantellini
RD - Software
Industrie Dial Face S.p.A.
Via Canzo, 4
20068 Peschiera Borromeo (MI), Italy
Tel.: +39 02 5167 2813
Fax: +39 02 5167 2459
web: www.idf-hit.com
mail: luigi.mantell...@idf-hit.com
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] IP_t should be a packed struct
2009/1/28 Ben Warren biggerbadder...@gmail.com: .. I'd focus on fixing your toolchain. Your problem will not be confined to protocol headers. my toolchain works fine ;) -- Luigi 'Comio' Mantellini RD - Software Industrie Dial Face S.p.A. Via Canzo, 4 20068 Peschiera Borromeo (MI), Italy Tel.: +39 02 5167 2813 Fax: +39 02 5167 2459 web: www.idf-hit.com mail: luigi.mantell...@idf-hit.com ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] IP_t should be a packed struct
Luigi Mantellini wrote:
Dear All
From my point of view, when packing is formally required (ie packets
headers), the structs should be declared explicitly as __packed__. The
correctness of the object code should be independent from the compiler
optimizations and we should always remember that the offset of a
struct field is not necessarily the sum of the sizes of the fields
that precede it.
struct {
type1 a;
type2 b;
type3 c;
} mystruct
offset(mystruct.c) != sizeof(type1) + sizeof(type2).
Regarding the CFLAGS used by me... I haven't set any CFLAGS and I just
do a make qemu_mips_config CROSS_COMPILE=mips-linux- make
CROSS_COMPILE=mips-linux... and a boundary alignment is not an alien
choice for a good compiler (a standard gcc4.2.4 in my case).
Furthermore I expect a correct object always... with any -Ox flag (a
apart bugs... of course).
My idea should be to declare a define like this
#define PKT_HEADER __attribute__((__packed__))
my 2EuroCents.
best regards,
luigi
OK, sounds good. Send a patch please.
regards,
Ben
snip
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] IP_t should be a packed struct
Dear Luigi Mantellini, In message b73e93990901281258s37f7e4d6hc6c2ff4c5d5fa...@mail.gmail.com you wrote: I'd focus on fixing your toolchain. Your problem will not be confined to protocol headers. my toolchain works fine ;) Except that it adds padding where it doesn't make sense. I think you want to fix it. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Don't tell me how hard you work. Tell me how much you get done. -- James J. Ling ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] IP_t should be a packed struct
Dear Ben Warren,
In message 4980cc59.1070...@gmail.com you wrote:
My idea should be to declare a define like this
#define PKT_HEADER __attribute__((__packed__))
my 2EuroCents.
best regards,
luigi
OK, sounds good. Send a patch please.
Hm... and what does this give us?
How long until sombebody detects that all the data structures
describing device register layout or so many processors and chips
don't use any __packed__ either? Except for some (IMHO broken, but I
know that I'm more attached to PowerPC anyway) ARM systems it is
sufficient to use a sane selection of data types.
I vote against changing this code. Just for a quick cross-check: do
the corresponding data structs in the Linux kernel use any
__packed__?
Here is for example a copy of /usr/include/netinet/ip.h :
...
struct iphdr
{
#if __BYTE_ORDER == __LITTLE_ENDIAN
unsigned int ihl:4;
unsigned int version:4;
#elif __BYTE_ORDER == __BIG_ENDIAN
unsigned int version:4;
unsigned int ihl:4;
#else
# error Please fix bits/endian.h
#endif
u_int8_t tos;
u_int16_t tot_len;
u_int16_t id;
u_int16_t frag_off;
u_int8_t ttl;
u_int8_t protocol;
u_int16_t check;
u_int32_t saddr;
u_int32_t daddr;
/*The options start here. */
};
...
Do you see any __packed__?
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
The C-shell doesn't parse. It adhoculates.
- casper@holland.sun.com in 3ol96k$...@engnews2.eng.sun.com
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] IP_t should be a packed struct
Wolfgang Denk wrote:
Dear Ben Warren,
In message 4980cc59.1070...@gmail.com you wrote:
My idea should be to declare a define like this
#define PKT_HEADER __attribute__((__packed__))
my 2EuroCents.
best regards,
luigi
OK, sounds good. Send a patch please.
Hm... and what does this give us?
How long until sombebody detects that all the data structures
describing device register layout or so many processors and chips
don't use any __packed__ either? Except for some (IMHO broken, but I
know that I'm more attached to PowerPC anyway) ARM systems it is
sufficient to use a sane selection of data types.
Well, yeah, I alluded to that in my other e-mail. I'm not crazy about
this either, but don't see any significant downside. Obviously if this
was a common problem, it would have surfaced a long time ago...
I vote against changing this code. Just for a quick cross-check: do
the corresponding data structs in the Linux kernel use any
__packed__?
Here is for example a copy of /usr/include/netinet/ip.h :
...
struct iphdr
{
#if __BYTE_ORDER == __LITTLE_ENDIAN
unsigned int ihl:4;
unsigned int version:4;
#elif __BYTE_ORDER == __BIG_ENDIAN
unsigned int version:4;
unsigned int ihl:4;
#else
# error Please fix bits/endian.h
#endif
u_int8_t tos;
u_int16_t tot_len;
u_int16_t id;
u_int16_t frag_off;
u_int8_t ttl;
u_int8_t protocol;
u_int16_t check;
u_int32_t saddr;
u_int32_t daddr;
/*The options start here. */
};
...
Do you see any __packed__?
Yeah, I made the same observation, but am not fluent enough in the black
magic of Linux header files to know if packing was being enforced
somewhere else or in some other way that my little brain can't comprehend.
Best regards,
Wolfgang Denk
regards,
Ben
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] IP_t should be a packed struct
Dear All,
2009/1/28 Wolfgang Denk w...@denx.de:
Dear Ben Warren,
In message 4980cc59.1070...@gmail.com you wrote:
My idea should be to declare a define like this
#define PKT_HEADER __attribute__((__packed__))
my 2EuroCents.
best regards,
luigi
OK, sounds good. Send a patch please.
I think that an audit of the code is important to understand if we
have a problem (or not) and how large is the problem.
Hm... and what does this give us?
How long until sombebody detects that all the data structures
describing device register layout or so many processors and chips
don't use any __packed__ either? Except for some (IMHO broken, but I
know that I'm more attached to PowerPC anyway) ARM systems it is
sufficient to use a sane selection of data types.
My compiler is not broken...
I vote against changing this code. Just for a quick cross-check: do
the corresponding data structs in the Linux kernel use any
__packed__?
cassini linux # head Makefile -n5
VERSION = 2
PATCHLEVEL = 6
SUBLEVEL = 28
EXTRAVERSION = -tuxonice-r1
NAME = Erotic Pickled Herring
cassini linux # find -name \*.c -o -name \*.h |xargs grep attribute |
grep packed | wc -l
3153
I see a lot of packed structs...
Here is for example a copy of /usr/include/netinet/ip.h :
...
struct iphdr
{
#if __BYTE_ORDER == __LITTLE_ENDIAN
unsigned int ihl:4;
unsigned int version:4;
#elif __BYTE_ORDER == __BIG_ENDIAN
unsigned int version:4;
unsigned int ihl:4;
#else
# error Please fix bits/endian.h
#endif
u_int8_t tos;
u_int16_t tot_len;
u_int16_t id;
u_int16_t frag_off;
u_int8_t ttl;
u_int8_t protocol;
u_int16_t check;
u_int32_t saddr;
u_int32_t daddr;
/*The options start here. */
};
...
Do you see any __packed__?
This doesn't say anything regarding how kernel guys have resolved the
problem (if they are solved...). Checking the kernel headers a lot
(all?) of structs that refers to drivers and internal structures are
defined using the attribute packed. For me this shows that somebody
asked himself if packed is needed or not.
I think that struct packing needs to be understood and a global
mechanism should be used (like -fpack-struct option always defined or
a style guideline that requires a tag for each structs). From my point
of view, we cannot define as wrong a compiler that follows different
choices that are anyway conform to the language standard.
My usual 2Eurocents.
best regards,
luigi
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
The C-shell doesn't parse. It adhoculates.
- casper@holland.sun.com in 3ol96k$...@engnews2.eng.sun.com
--
Luigi 'Comio' Mantellini
RD - Software
Industrie Dial Face S.p.A.
Via Canzo, 4
20068 Peschiera Borromeo (MI), Italy
Tel.: +39 02 5167 2813
Fax: +39 02 5167 2459
web: www.idf-hit.com
mail: luigi.mantell...@idf-hit.com
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] IP_t should be a packed struct
Dear Ben, In message 4980d38f.4020...@gmail.com you wrote: Here is for example a copy of /usr/include/netinet/ip.h : ... struct iphdr ... Yeah, I made the same observation, but am not fluent enough in the black magic of Linux header files to know if packing was being enforced somewhere else or in some other way that my little brain can't comprehend. Note that this was standard netinet/ip.h, i. e. no Linux kernel header file, but a standard user land header. Any application code using network functions would break if this was a real problem. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de The human race has one really effective weapon, and that is laughter. - Mark Twain ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] IP_t should be a packed struct
Dear Luigi Mantellini, In message b73e93990901281416k766b2bf3qc6429f77545f9...@mail.gmail.com you wrote: I think that an audit of the code is important to understand if we have a problem (or not) and how large is the problem. We (i. e. all of us except you) do not have a problem. My compiler is not broken... Well, YMMV... cassini linux # find -name \*.c -o -name \*.h |xargs grep attribute | grep packed | wc -l 3153 I see a lot of packed structs... Here is for example a copy of /usr/include/netinet/ip.h : ... This doesn't say anything regarding how kernel guys have resolved the problem (if they are solved...). Checking the kernel headers a lot This is not a kernel header. This is one of the standard user space network headers, and a pretty central one. Feel free to check any others. I think that struct packing needs to be understood and a global I agree on this. This definitely needs to be understood. Hm... I think I understand it, and it works for me :-) mechanism should be used (like -fpack-struct option always defined or a style guideline that requires a tag for each structs). From my point I think this is not needed. Please read the docuemntation about alignment and padding. It is pretty clear. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de And it should be the law: If you use the word `paradigm' without knowing what the dictionary says it means, you go to jail. No exceptions. - David Jones @ Megatest Corporation ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] IP_t should be a packed struct
Dear Wolfgang, 2009/1/28 Wolfgang Denk w...@denx.de: Dear Luigi Mantellini, In message b73e93990901281416k766b2bf3qc6429f77545f9...@mail.gmail.com you wrote: I think that an audit of the code is important to understand if we have a problem (or not) and how large is the problem. We (i. e. all of us except you) do not have a problem. my question is: how can you be sure on this? I haven't used strange compilers or strange CFLAGS.. I just do make on a supported target (qemu_mips). I haven't any problem to correct my local source tree. I ask myself why gcc offers a packed atribute, ms-vc offers pragma packed, ... My compiler is not broken... Well, YMMV... the behavior is clear: in my environment, the default choice is to align fields on 32bit for speed reasons... and I like this by default for my applications. I can ignore the problem using options like -Os (I will try tomorrow) or -fpack-struct or other global mechanisms or, pay attention on structures definitions to be sure that the structure size is compiler and cflags independent. cassini linux # find -name \*.c -o -name \*.h |xargs grep attribute | grep packed | wc -l 3153 I see a lot of packed structs... Here is for example a copy of /usr/include/netinet/ip.h : ... This doesn't say anything regarding how kernel guys have resolved the problem (if they are solved...). Checking the kernel headers a lot This is not a kernel header. This is one of the standard user space network headers, and a pretty central one. Feel free to check any others. I see. I would like to understand why this structure is optimization-prof. I will study tomorrow... I think that struct packing needs to be understood and a global I agree on this. This definitely needs to be understood. Hm... I think I understand it, and it works for me :-) iso C doensn't require packing by default of the structures. To assume that a structure is packed by default is not a good assumption. All compilers offer directives to control the behavior of packing... I believe that there is a reason... mechanism should be used (like -fpack-struct option always defined or a style guideline that requires a tag for each structs). From my point I think this is not needed. Please read the docuemntation about alignment and padding. It is pretty clear. which documentation should I read? iso c documents? gcc manuals? I haven't found anything that opposes my affirmations. I want underline that the structures (IP_t, ... ) don't have field across the word-machine boundary... but this doesn't exclude an memory-access optimization. Kindly, can you give me any good links? Anyway, I don't want to talk about philosophy. I just noticed an anomaly and I wanted to share with the ML. my 2LireItaliane best regards, luigi Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de And it should be the law: If you use the word `paradigm' without knowing what the dictionary says it means, you go to jail. No exceptions. - David Jones @ Megatest Corporation -- Luigi 'Comio' Mantellini RD - Software Industrie Dial Face S.p.A. Via Canzo, 4 20068 Peschiera Borromeo (MI), Italy Tel.: +39 02 5167 2813 Fax: +39 02 5167 2459 web: www.idf-hit.com mail: luigi.mantell...@idf-hit.com ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] IP_t should be a packed struct
Hi ML,
I'm working on a mips target and I used qemu_mips target to simulate my target
(that I hope to have in the next week...)
Following my activities I noticed that IP_t structure is no defined with
attribute packed. I noticed this issue because using a self-made toolchain
(gcc4.2.4+binutils2.8+uclibc0.9.30) the compiler has aligned all bytes to
32bit boundary. This is not ok, because the packets IP_t can be non aligned
(see the /net/net.c PingSend function, for an example).
The dirty solution is to define the structure with the
__attribute__((__packed__))... but, from my point of view, a better packet
forging mechanism should be implemented into the net.c stack.
I attached a trivial patch that solved the issue on my target.
Any comments is welcome.
best regards,
luigi
--
Luigi Mantellini
RD - Software
Industrie Dial Face S.p.A.
Via Canzo, 4
20068 Peschiera Borromeo (MI), Italy
Tel.: +39 02 5167 2813
Fax: +39 02 5167 2459
Email: luigi.mantell...@idf-hit.com
diff --git a/include/net.h b/include/net.h
index d2d394f..c04f84c 100644
--- a/include/net.h
+++ b/include/net.h
@@ -198,7 +198,7 @@ typedef struct {
ushort udp_dst; /* UDP destination port */
ushort udp_len; /* Length of UDP packet */
ushort udp_xsum; /* Checksum */
-} IP_t;
+} __attribute__((__packed__)) IP_t;
#define IP_OFFS 0x1fff /* ip offset *= 8 */
#define IP_FLAGS 0xe000 /* first 3 bits */
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
