[U-Boot] [PATCH] Use packed structures for networking
PXE boot is broken with GCC 7.1 due option '-fstore-merging' enabled by default for '-O2':
BOOTP broadcast 1 data abort pc : [<8ff8bb30>] lr : [<00004f1f>] reloc pc : [<17832b30>] lr : [<878abf1f>] sp : 8f558bc0 ip : 00000000 fp : 8ffef5a4 r10: 8ffed248 r9 : 8f558ee0 r8 : 8ffef594 r7 : 0000000e r6 : 8ffed700 r5 : 00000000 r4 : 8ffed74e r3 : 00060101 r2 : 8ffed230 r1 : 8ffed706 r0 : 00000ddd Flags: nzcv IRQs off FIQs off Mode SVC_32 Resetting CPU ...
Core reason is usage of structures for network headers without packed attribute.
Reviewed-by: Yauheni Kaliuta yauheni.kaliuta@redhat.com Signed-off-by: Denis Pynkin denis.pynkin@collabora.com --- include/net.h | 14 +++++++------- net/bootp.h | 2 +- net/dns.h | 2 +- net/nfs.h | 2 +- net/sntp.h | 2 +- 5 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/include/net.h b/include/net.h index 2eaa882..e126948 100644 --- a/include/net.h +++ b/include/net.h @@ -308,7 +308,7 @@ struct ethernet_hdr { u8 et_dest[ARP_HLEN]; /* Destination node */ u8 et_src[ARP_HLEN]; /* Source node */ u16 et_protlen; /* Protocol or length */ -}; +} __attribute__((packed));
/* Ethernet header size */ #define ETHER_HDR_SIZE (sizeof(struct ethernet_hdr)) @@ -326,7 +326,7 @@ struct e802_hdr { u8 et_snap2; u8 et_snap3; u16 et_prot; /* 802 protocol */ -}; +} __attribute__((packed));
/* 802 + SNAP + ethernet header size */ #define E802_HDR_SIZE (sizeof(struct e802_hdr)) @@ -340,7 +340,7 @@ struct vlan_ethernet_hdr { u16 vet_vlan_type; /* PROT_VLAN */ u16 vet_tag; /* TAG of VLAN */ u16 vet_type; /* protocol type */ -}; +} __attribute__((packed));
/* VLAN Ethernet header size */ #define VLAN_ETHER_HDR_SIZE (sizeof(struct vlan_ethernet_hdr)) @@ -369,7 +369,7 @@ struct ip_hdr { u16 ip_sum; /* checksum */ struct in_addr ip_src; /* Source IP address */ struct in_addr ip_dst; /* Destination IP address */ -}; +} __attribute__((packed));
#define IP_OFFS 0x1fff /* ip offset *= 8 */ #define IP_FLAGS 0xe000 /* first 3 bits */ @@ -397,7 +397,7 @@ struct ip_udp_hdr { u16 udp_dst; /* UDP destination port */ u16 udp_len; /* Length of UDP packet */ u16 udp_xsum; /* Checksum */ -}; +} __attribute__((packed));
#define IP_UDP_HDR_SIZE (sizeof(struct ip_udp_hdr)) #define UDP_HDR_SIZE (IP_UDP_HDR_SIZE - IP_HDR_SIZE) @@ -435,7 +435,7 @@ struct arp_hdr { u8 ar_tha[]; /* Target hardware address */ u8 ar_tpa[]; /* Target protocol address */ #endif /* 0 */ -}; +} __attribute__((packed));
#define ARP_HDR_SIZE (8+20) /* Size assuming ethernet */
@@ -470,7 +470,7 @@ struct icmp_hdr { } frag; u8 data[0]; } un; -}; +} __attribute__((packed));
#define ICMP_HDR_SIZE (sizeof(struct icmp_hdr)) #define IP_ICMP_HDR_SIZE (IP_HDR_SIZE + ICMP_HDR_SIZE) diff --git a/net/bootp.h b/net/bootp.h index fcb0a64..a1291be 100644 --- a/net/bootp.h +++ b/net/bootp.h @@ -49,7 +49,7 @@ struct bootp_hdr { char bp_sname[64]; /* Server host name */ char bp_file[128]; /* Boot file name */ char bp_vend[OPT_FIELD_SIZE]; /* Vendor information */ -}; +}__attribute__((packed));
#define BOOTP_HDR_SIZE sizeof(struct bootp_hdr)
diff --git a/net/dns.h b/net/dns.h index c4e96af..c55a5c1 100644 --- a/net/dns.h +++ b/net/dns.h @@ -29,7 +29,7 @@ struct header { uint16_t nauth; /* Authority PRs */ uint16_t nother; /* Other PRs */ unsigned char data[1]; /* Data, variable length */ -}; +} __attribute__((packed));
void dns_start(void); /* Begin DNS */
diff --git a/net/nfs.h b/net/nfs.h index 45da246..70a1a6d 100644 --- a/net/nfs.h +++ b/net/nfs.h @@ -79,7 +79,7 @@ struct rpc_t { uint32_t data[NFS_READ_SIZE]; } reply; } u; -}; +} __attribute__((packed)); void nfs_start(void); /* Begin NFS */
diff --git a/net/sntp.h b/net/sntp.h index 6a9c6bb..c38bcee 100644 --- a/net/sntp.h +++ b/net/sntp.h @@ -51,7 +51,7 @@ struct sntp_pkt_t { unsigned long long originate_timestamp; unsigned long long receive_timestamp; unsigned long long transmit_timestamp; -}; +} __attribute__((packed));
void sntp_start(void); /* Begin SNTP */
Adding Tom and Joe.
On Fri, Jul 21, 2017 at 1:28 PM, Denis Pynkin denis.pynkin@collabora.com wrote:
PXE boot is broken with GCC 7.1 due option '-fstore-merging' enabled by default for '-O2':
BOOTP broadcast 1 data abort pc : [<8ff8bb30>] lr : [<00004f1f>] reloc pc : [<17832b30>] lr : [<878abf1f>] sp : 8f558bc0 ip : 00000000 fp : 8ffef5a4 r10: 8ffed248 r9 : 8f558ee0 r8 : 8ffef594 r7 : 0000000e r6 : 8ffed700 r5 : 00000000 r4 : 8ffed74e r3 : 00060101 r2 : 8ffed230 r1 : 8ffed706 r0 : 00000ddd Flags: nzcv IRQs off FIQs off Mode SVC_32 Resetting CPU ...
Core reason is usage of structures for network headers without packed attribute.
Reviewed-by: Yauheni Kaliuta yauheni.kaliuta@redhat.com Signed-off-by: Denis Pynkin denis.pynkin@collabora.com
include/net.h | 14 +++++++------- net/bootp.h | 2 +- net/dns.h | 2 +- net/nfs.h | 2 +- net/sntp.h | 2 +- 5 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/include/net.h b/include/net.h index 2eaa882..e126948 100644 --- a/include/net.h +++ b/include/net.h @@ -308,7 +308,7 @@ struct ethernet_hdr { u8 et_dest[ARP_HLEN]; /* Destination node */ u8 et_src[ARP_HLEN]; /* Source node */ u16 et_protlen; /* Protocol or length */ -}; +} __attribute__((packed));
/* Ethernet header size */ #define ETHER_HDR_SIZE (sizeof(struct ethernet_hdr)) @@ -326,7 +326,7 @@ struct e802_hdr { u8 et_snap2; u8 et_snap3; u16 et_prot; /* 802 protocol */ -}; +} __attribute__((packed));
/* 802 + SNAP + ethernet header size */ #define E802_HDR_SIZE (sizeof(struct e802_hdr)) @@ -340,7 +340,7 @@ struct vlan_ethernet_hdr { u16 vet_vlan_type; /* PROT_VLAN */ u16 vet_tag; /* TAG of VLAN */ u16 vet_type; /* protocol type */ -}; +} __attribute__((packed));
/* VLAN Ethernet header size */ #define VLAN_ETHER_HDR_SIZE (sizeof(struct vlan_ethernet_hdr)) @@ -369,7 +369,7 @@ struct ip_hdr { u16 ip_sum; /* checksum */ struct in_addr ip_src; /* Source IP address */ struct in_addr ip_dst; /* Destination IP address */ -}; +} __attribute__((packed));
#define IP_OFFS 0x1fff /* ip offset *= 8 */ #define IP_FLAGS 0xe000 /* first 3 bits */ @@ -397,7 +397,7 @@ struct ip_udp_hdr { u16 udp_dst; /* UDP destination port */ u16 udp_len; /* Length of UDP packet */ u16 udp_xsum; /* Checksum */ -}; +} __attribute__((packed));
#define IP_UDP_HDR_SIZE (sizeof(struct ip_udp_hdr)) #define UDP_HDR_SIZE (IP_UDP_HDR_SIZE - IP_HDR_SIZE) @@ -435,7 +435,7 @@ struct arp_hdr { u8 ar_tha[]; /* Target hardware address */ u8 ar_tpa[]; /* Target protocol address */ #endif /* 0 */ -}; +} __attribute__((packed));
#define ARP_HDR_SIZE (8+20) /* Size assuming ethernet */
@@ -470,7 +470,7 @@ struct icmp_hdr { } frag; u8 data[0]; } un; -}; +} __attribute__((packed));
#define ICMP_HDR_SIZE (sizeof(struct icmp_hdr)) #define IP_ICMP_HDR_SIZE (IP_HDR_SIZE + ICMP_HDR_SIZE) diff --git a/net/bootp.h b/net/bootp.h index fcb0a64..a1291be 100644 --- a/net/bootp.h +++ b/net/bootp.h @@ -49,7 +49,7 @@ struct bootp_hdr { char bp_sname[64]; /* Server host name */ char bp_file[128]; /* Boot file name */ char bp_vend[OPT_FIELD_SIZE]; /* Vendor information */ -}; +}__attribute__((packed));
#define BOOTP_HDR_SIZE sizeof(struct bootp_hdr)
diff --git a/net/dns.h b/net/dns.h index c4e96af..c55a5c1 100644 --- a/net/dns.h +++ b/net/dns.h @@ -29,7 +29,7 @@ struct header { uint16_t nauth; /* Authority PRs */ uint16_t nother; /* Other PRs */ unsigned char data[1]; /* Data, variable length */ -}; +} __attribute__((packed));
void dns_start(void); /* Begin DNS */
diff --git a/net/nfs.h b/net/nfs.h index 45da246..70a1a6d 100644 --- a/net/nfs.h +++ b/net/nfs.h @@ -79,7 +79,7 @@ struct rpc_t { uint32_t data[NFS_READ_SIZE]; } reply; } u; -}; +} __attribute__((packed)); void nfs_start(void); /* Begin NFS */
diff --git a/net/sntp.h b/net/sntp.h index 6a9c6bb..c38bcee 100644 --- a/net/sntp.h +++ b/net/sntp.h @@ -51,7 +51,7 @@ struct sntp_pkt_t { unsigned long long originate_timestamp; unsigned long long receive_timestamp; unsigned long long transmit_timestamp; -}; +} __attribute__((packed));
void sntp_start(void); /* Begin SNTP */
-- 2.10.3
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
On Fri, Jul 21, 2017 at 07:28:42PM +0300, Denis Pynkin wrote:
PXE boot is broken with GCC 7.1 due option '-fstore-merging' enabled by default for '-O2':
BOOTP broadcast 1 data abort pc : [<8ff8bb30>] lr : [<00004f1f>] reloc pc : [<17832b30>] lr : [<878abf1f>] sp : 8f558bc0 ip : 00000000 fp : 8ffef5a4 r10: 8ffed248 r9 : 8f558ee0 r8 : 8ffef594 r7 : 0000000e r6 : 8ffed700 r5 : 00000000 r4 : 8ffed74e r3 : 00060101 r2 : 8ffed230 r1 : 8ffed706 r0 : 00000ddd Flags: nzcv IRQs off FIQs off Mode SVC_32 Resetting CPU ...
Core reason is usage of structures for network headers without packed attribute.
Reviewed-by: Yauheni Kaliuta yauheni.kaliuta@redhat.com Signed-off-by: Denis Pynkin denis.pynkin@collabora.com
include/net.h | 14 +++++++------- net/bootp.h | 2 +- net/dns.h | 2 +- net/nfs.h | 2 +- net/sntp.h | 2 +- 5 files changed, 11 insertions(+), 11 deletions(-)
So, what I've been wondering, and others have poked me about (who can chime in if they like), how is the kernel not also tripping over this?
On Fri, Jul 21, 2017 at 2:00 PM, Tom Rini trini@konsulko.com wrote:
On Fri, Jul 21, 2017 at 07:28:42PM +0300, Denis Pynkin wrote:
PXE boot is broken with GCC 7.1 due option '-fstore-merging' enabled by default for '-O2':
BOOTP broadcast 1 data abort pc : [<8ff8bb30>] lr : [<00004f1f>] reloc pc : [<17832b30>] lr : [<878abf1f>] sp : 8f558bc0 ip : 00000000 fp : 8ffef5a4 r10: 8ffed248 r9 : 8f558ee0 r8 : 8ffef594 r7 : 0000000e r6 : 8ffed700 r5 : 00000000 r4 : 8ffed74e r3 : 00060101 r2 : 8ffed230 r1 : 8ffed706 r0 : 00000ddd Flags: nzcv IRQs off FIQs off Mode SVC_32 Resetting CPU ...
Core reason is usage of structures for network headers without packed attribute.
Reviewed-by: Yauheni Kaliuta yauheni.kaliuta@redhat.com Signed-off-by: Denis Pynkin denis.pynkin@collabora.com
include/net.h | 14 +++++++------- net/bootp.h | 2 +- net/dns.h | 2 +- net/nfs.h | 2 +- net/sntp.h | 2 +- 5 files changed, 11 insertions(+), 11 deletions(-)
So, what I've been wondering, and others have poked me about (who can chime in if they like), how is the kernel not also tripping over this?
Great question.
28.07.2017 00:26, Joe Hershberger wrote:
PXE boot is broken with GCC 7.1 due option '-fstore-merging' enabled by default for '-O2':
So, what I've been wondering, and others have poked me about (who can chime in if they like), how is the kernel not also tripping over this?
Great question.
Believe kernel do not work in single static buffer for the whole packet as u-boot do.
Problem is in ethernet header structure with size ETHER_HDR_SIZE=14, so IP and BOOTP headers are not aligned to 4 bytes.
Let's take bootp.c code as example:
755 bp = (struct bootp_hdr *)pkt; 756 bp->bp_op = OP_BOOTREQUEST; 757 bp->bp_htype = HWT_ETHER; 758 bp->bp_hlen = HWL_ETHER; 759 bp->bp_hops = 0;
Without '-fstore-merging' or with enabled 'packed' structures assembler code looks like: 6a: 77a3 strb r3, [r4, #30] 6c: f884 b01c strb.w fp, [r4, #28] 70: f884 b01d strb.w fp, [r4, #29] 74: 77e5 strb r5, [r4, #31]
but with option '-fstore-merging' all these instructions merged into single instruction: 62: 61e3 str r3, [r4, #28]
and store address is not aligned to 32b boundary here, so it results to 'data abort'.
I see some possible solutions here: - add option '-fno-store-merging' -- works, I test it already - use packed structures -- since there are some unsafe code in sources - rewrite networking part to work with any protocol separately and merge headers and data only on send.
Even simple reordering of header flags initialization helps here, but it looks like a bit of black magic in code.
PS forgot to mention in patch description -- '-Os' includes '-fstore-merging' option as well for gcc 7.1.
On Sat, Jul 29, 2017 at 12:53:36PM +0300, Denis Pynkin wrote:
28.07.2017 00:26, Joe Hershberger wrote:
PXE boot is broken with GCC 7.1 due option '-fstore-merging' enabled by default for '-O2':
So, what I've been wondering, and others have poked me about (who can chime in if they like), how is the kernel not also tripping over this?
Great question.
Believe kernel do not work in single static buffer for the whole packet as u-boot do.
Problem is in ethernet header structure with size ETHER_HDR_SIZE=14, so IP and BOOTP headers are not aligned to 4 bytes.
Let's take bootp.c code as example:
755 bp = (struct bootp_hdr *)pkt; 756 bp->bp_op = OP_BOOTREQUEST; 757 bp->bp_htype = HWT_ETHER; 758 bp->bp_hlen = HWL_ETHER; 759 bp->bp_hops = 0;
Without '-fstore-merging' or with enabled 'packed' structures assembler code looks like: 6a: 77a3 strb r3, [r4, #30] 6c: f884 b01c strb.w fp, [r4, #28] 70: f884 b01d strb.w fp, [r4, #29] 74: 77e5 strb r5, [r4, #31]
but with option '-fstore-merging' all these instructions merged into single instruction: 62: 61e3 str r3, [r4, #28]
and store address is not aligned to 32b boundary here, so it results to 'data abort'.
I see some possible solutions here:
- add option '-fno-store-merging' -- works, I test it already
- use packed structures -- since there are some unsafe code in sources
- rewrite networking part to work with any protocol separately and
merge headers and data only on send.
Even simple reordering of header flags initialization helps here, but it looks like a bit of black magic in code.
PS forgot to mention in patch description -- '-Os' includes '-fstore-merging' option as well for gcc 7.1.
Thanks for explaining. My inclination is that we should have a packed change in for -rc1. Philipp, have you had a chance to edit doc/README.unaligned-memory-access.txt to your liking? Thanks!
28.07.2017 00:26, Joe Hershberger wrote:
PXE boot is broken with GCC 7.1 due option '-fstore-merging' enabled by default for '-O2':
So, what I've been wondering, and others have poked me about (who can chime in if they like), how is the kernel not also tripping over this?
Great question.
Believe kernel do not work in single static buffer for the whole packet as u-boot do.
Problem is in ethernet header structure with size ETHER_HDR_SIZE=14, so IP and BOOTP headers are not aligned to 4 bytes.
Let's take bootp.c code as example:
755 bp = (struct bootp_hdr *)pkt; 756 bp->bp_op = OP_BOOTREQUEST; 757 bp->bp_htype = HWT_ETHER; 758 bp->bp_hlen = HWL_ETHER; 759 bp->bp_hops = 0;
Without '-fstore-merging' or with enabled 'packed' structures assembler code looks like: 6a: 77a3 strb r3, [r4, #30] 6c: f884 b01c strb.w fp, [r4, #28] 70: f884 b01d strb.w fp, [r4, #29] 74: 77e5 strb r5, [r4, #31]
but with option '-fstore-merging' all these instructions merged into single instruction: 62: 61e3 str r3, [r4, #28]
and store address is not aligned to 32b boundary here, so it results to 'data abort'.
I see some possible solutions here: - add option '-fno-store-merging' -- works, I test it already - use packed structures -- since there are some unsafe code in sources - rewrite networking part to work with any protocol separately and merge headers and data only on send.
Even simple reordering of header flags initialization helps here, but it looks like a bit of black magic in code.
PS forgot to mention in patch description -- '-Os' includes '-fstore-merging' option as well for gcc 7.1.
On Fri, Jul 21, 2017 at 11:28 AM, Denis Pynkin denis.pynkin@collabora.com wrote:
PXE boot is broken with GCC 7.1 due option '-fstore-merging' enabled by default for '-O2':
BOOTP broadcast 1 data abort pc : [<8ff8bb30>] lr : [<00004f1f>] reloc pc : [<17832b30>] lr : [<878abf1f>] sp : 8f558bc0 ip : 00000000 fp : 8ffef5a4 r10: 8ffed248 r9 : 8f558ee0 r8 : 8ffef594 r7 : 0000000e r6 : 8ffed700 r5 : 00000000 r4 : 8ffed74e r3 : 00060101 r2 : 8ffed230 r1 : 8ffed706 r0 : 00000ddd Flags: nzcv IRQs off FIQs off Mode SVC_32 Resetting CPU ...
Core reason is usage of structures for network headers without packed attribute.
Reviewed-by: Yauheni Kaliuta yauheni.kaliuta@redhat.com Signed-off-by: Denis Pynkin denis.pynkin@collabora.com
Acked-by: Joe Hershberger joe.hershberger@ni.com
participants (6)
-
Denis Pynkin -
Denis Pynkin -
Fabio Estevam -
Joe Hershberger -
Joe Hershberger -
Tom Rini