buglwIP - A Lightweight TCP/IP stack - Bugs: bug #26134, Insert a dummy u16_t field to...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #26134: Insert a dummy u16_t field to prevent alignment exceptions

Submitted by:  Zhenwei Chu <blackfin>
Submitted on:  Thu 09 Apr 2009 02:06:54 PM UTC  
 
Category: NoneSeverity: 3 - Normal
Item Group: Change RequestStatus: Invalid
Privacy: PublicAssigned to: Simon Goldschmidt <goldsimon>
Open/Closed: ClosedPlanned Release: None
lwIP version: 1.3.0

(Jump to the original submission Jump to the original submission)

Wed 15 Apr 2009 08:44:01 PM UTC, comment #6:

> The crux of the issue is certain architectures word accesses has
> to be aligned to 4 byte boundary. In the arp header source ip
> address will be starting at non-aligned (4byte) location.


That is the case with most architectures and the reason 'packed structs' have been introduced to C. (Forgive me if you already know this): with packed structs, the compiler basically accesses memory byte-by-byte and reconstructs the u32_t in a register. This is slow but there is no workaround for this as the structure layout is defined by the network protocol (we cannot just change it in the lwIP code).

lwIP ports can control structure packing either through defines (PACK_STRUCT_BEGIN / PACK_STRUCT_FIELD / PACK_STRUCT_STRUCT / PACK_STRUCT_END) or through include files (PACK_STRUCT_USE_INCLUDES). Both ways can be found as example either in the win32 or unix port. There are some compilers that don't support structure packing, but these are so rare that we cannot afford to support them (and these are really, really rare so I doubt that is the case for you).

I'll close this as invalid. Please don't hesitate to reply to the lwip_devel list on further questions.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Wed 15 Apr 2009 08:00:39 PM UTC, comment #5:

Yes. ETH_PAD_SIZE is much flexible.It just got overlooked in the migration. It's good enough for eth_hdr.

With etharp_hdr there will be an issue for us. The crux of the issue is certain architectures word accesses has to be aligned to 4 byte boundary. In the arp header source ip address will be starting at non-aligned (4byte) location.

Are there any other ports that has similar changes with regards to alignment? Neverthless if no one has similar requirements please close this out. we will work out a way with a seperate copy or use byte accesses instead of word accesses.

By the way is there any reason for ETHARP_HWADDR_LEN macro configurable? I'm curious as this may be already used for extra padding.

Srinivas Gollakota <gollakota>
Mon 13 Apr 2009 08:46:53 PM UTC, comment #4:

What exactly are you trying to solve with this? The struct etharp_hdr is the exact representation of the message as defined in RFC 826. By inserting "dummy" or "unused" fields, you only will break everyone else's port except yours. Which brings us back to the first question.

Besides, would you mind answering to Kieran's question in comment #1? If not, we'll have to close this as, to us, it's invalid without further comments.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Mon 13 Apr 2009 06:53:41 PM UTC, comment #3:

one more change required:

From:
Etharp.h:

Line98:
/** the ARP message */
struct etharp_hdr {
PACK_STRUCT_FIELD(struct eth_hdr ethhdr);
PACK_STRUCT_FIELD(u16_t hwtype);
PACK_STRUCT_FIELD(u16_t proto);
PACK_STRUCT_FIELD(u16_t _hwlen_protolen);
PACK_STRUCT_FIELD(u16_t opcode);
PACK_STRUCT_FIELD(struct eth_addr shwaddr);
PACK_STRUCT_FIELD(struct ip_addr2 sipaddr);
PACK_STRUCT_FIELD(struct eth_addr dhwaddr);
PACK_STRUCT_FIELD(struct ip_addr2 dipaddr);
} PACK_STRUCT_STRUCT;

To:

Line98:
/** the ARP message */
struct etharp_hdr {
PACK_STRUCT_FIELD(struct eth_hdr ethhdr);
PACK_STRUCT_FIELD(u16_t hwtype);
PACK_STRUCT_FIELD(u16_t proto);
PACK_STRUCT_FIELD(u16_t _hwlen_protolen);
PACK_STRUCT_FIELD(u16_t opcode);
PACK_STRUCT_FIELD(struct eth_addr shwaddr);

/* insert a dummy u16_t field to prevent alignment exceptions */
PACK_STRUCT_FIELD(u16_t unused2);
PACK_STRUCT_FIELD(struct ip_addr2 sipaddr);
PACK_STRUCT_FIELD(struct eth_addr dhwaddr);

/* insert a dummy u16_t field to prevent alignment exceptions */
PACK_STRUCT_FIELD(u16_t unused3);
PACK_STRUCT_FIELD(struct ip_addr2 dipaddr);
} PACK_STRUCT_STRUCT;

Zhenwei Chu <blackfin>
Fri 10 Apr 2009 05:58:12 AM UTC, comment #2:

> Why not just define ETH_PAD_SIZE?


Especially as this change would break every port that does ot use ETH_PAD_SIZE as now the header is always padded.

Plus the drivers wouldn't know where to start copying to the MAC...

Last but not least, who knows an u16_t would be enough? It is for 32-bit architectures, but ETH_PAD_SIZE is more flexible.

Simon Goldschmidt <goldsimon>
Project AdministratorIn charge of this item.
Thu 09 Apr 2009 02:44:46 PM UTC, comment #1:

Why not just define ETH_PAD_SIZE?

Kieran Mansley <kieranm>
Project Administrator
Thu 09 Apr 2009 02:06:54 PM UTC, original submission:

Insert a dummy u16_t field to prevent alignment exceptions

Files Changed:
Etharp.h: line75

From:
PACK_STRUCT_BEGIN
struct eth_hdr {

#if ETH_PAD_SIZE
PACK_STRUCT_FIELD(u8_t padding[ETH_PAD_SIZE]);
#endif
PACK_STRUCT_FIELD(struct eth_addr dest);
PACK_STRUCT_FIELD(struct eth_addr src);
PACK_STRUCT_FIELD(u16_t type);
} PACK_STRUCT_STRUCT;
PACK_STRUCT_END

To:
PACK_STRUCT_BEGIN
struct eth_hdr {

#ifdef ETH_PAD_SIZE
PACK_STRUCT_FIELD(u8_t padding[ETH_PAD_SIZE]);

#else
/* insert a dummy u16_t field to prevent alignment exceptions */
PACK_STRUCT_FIELD(u16_t unused1);
#endif
PACK_STRUCT_FIELD(struct eth_addr dest);
PACK_STRUCT_FIELD(struct eth_addr src);
PACK_STRUCT_FIELD(u16_t type);
} PACK_STRUCT_STRUCT;
PACK_STRUCT_END

Zhenwei Chu <blackfin>

 

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by gollakota (Posted a comment)
  • -unavailable- added by goldsimon (Posted a comment)
  • -unavailable- added by kieranm (Posted a comment)
  • -unavailable- added by blackfin (Submitted the item)
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 3 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Wed 15 Apr 2009 08:44:01 PM UTCgoldsimonStatusNone=>Invalid
      Assigned toNone=>goldsimon
      Open/ClosedOpen=>Closed

    Back to the top


    Powered by Savane 3.1-cleanup1