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

Submitter:  Zhenwei Chu <blackfin>
Submitted:  Thu 09 Apr 2009 02:06:54 PM UTC
   
 
Category:  None Severity:  3 - Normal
Item Group:  Change Request Status:  Invalid
Privacy:  Public Assigned to:  goldsimon
Open/Closed:  Closed Planned Release:  None
lwIP version:  1.3.0

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>
Group administrator
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>
Group administrator
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>
Group administrator
Thu 09 Apr 2009 02:44:46 PM UTC, comment #1: 

Why not just define ETH_PAD_SIZE?


Kieran Mansley <kieranm>
Group Member
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>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 3 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2009-04-15 goldsimon StatusNone Invalid
        Assigned toNone goldsimon
        Open/ClosedOpen Closed

    Back to the top

    Powered by Savane 3.13-d3ae.
    Corresponding source code