You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2022/01/17 21:26:51 UTC
[GitHub] [incubator-nuttx] pkarashchenko opened a new pull request #5251: ethernet: apply compiler packing to all ethernet packet based types that are memory mapped
pkarashchenko opened a new pull request #5251:
URL: https://github.com/apache/incubator-nuttx/pull/5251
## Summary
The network stack does not have any information about memory that stores the Ethernet (TCP/IP) packet, so in order to generate proper code that takes care all the alignment issues we need to use "packed" types.
Equip all Ethernet packet based types with `begin_packed_struct` / `end_packed_struct`
## Impact
Ethernet based networking sub-system
## Testing
Pass CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5251: ethernet: apply compiler packing to all ethernet packet based types that are memory mapped
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5251:
URL: https://github.com/apache/incubator-nuttx/pull/5251#issuecomment-1014914339
> > @a-lunev could you please also participate in the review since you are doing many changes to the networking sub-system?
>
> So far, I do not well understand if adding "FAR" is tightly related to adding "begin_packed_struct / end_packed_struct". If they are not related issues, I would split the patch into two different ones, because the change is massive.
Ok. I will split the changes. The `FAR` is added only for common code consystency
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] ptka commented on pull request #5251: ethernet: apply compiler packing to all ethernet packet based types that are memory mapped
Posted by GitBox <gi...@apache.org>.
ptka commented on pull request #5251:
URL: https://github.com/apache/incubator-nuttx/pull/5251#issuecomment-1014924189
I took incubator head revision, your change and my additonal changes (board configuration level to get cdcecm included and started).
I cannot judge on the changes, but (sorry to say) the issue addressed in PR https://github.com/apache/incubator-nuttx/pull/5249 is not fixed with this PR --> on the rp2040 the crash is still available.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] a-lunev commented on pull request #5251: ethernet: apply compiler packing to all ethernet packet based types that are memory mapped
Posted by GitBox <gi...@apache.org>.
a-lunev commented on pull request #5251:
URL: https://github.com/apache/incubator-nuttx/pull/5251#issuecomment-1014911072
> @a-lunev could you please also participate in the review since you are doing many changes to the networking sub-system?
So far, I do not well understand if adding "FAR" is tightly related to adding "begin_packed_struct / end_packed_struct". If they are not related issues, I would split the patch into two different ones, because the change is massive.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5251: ethernet: apply compiler packing to all ethernet packet based types that are memory mapped
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5251:
URL: https://github.com/apache/incubator-nuttx/pull/5251#issuecomment-1014902244
@a-lunev could you please also participate in the review since you are doing many changes to the networking sub-system?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5251: ethernet: apply compiler packing to all ethernet packet based types that are memory mapped
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5251:
URL: https://github.com/apache/incubator-nuttx/pull/5251#issuecomment-1014944528
But maybe this changes are not worth of moving forward with. There are many places that are taking addresses that now become unaligned. Maybe we just need to ensure that memory pointed by `d_buf` is 16-bit aligned and add a couple of `DEBUGASSERT`s.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5251: ethernet: apply compiler packing to all ethernet packet based types that are memory mapped
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5251:
URL: https://github.com/apache/incubator-nuttx/pull/5251#issuecomment-1014942062
> I think the bug is that some architectures do not support dereferencing a pointer to an unaligned address. Packing the structs will create more of those, won't it?
The case is that packing for Ethernet packet structure will not impact on unaligned addresses, but should fix the cases when 16 or 32 bit types are accessed with instructions that require addresses to be aligned.
Similar is available for Ethernet header in Linux https://github.com/spotify/linux/blob/master/include/linux/if_ether.h
Here is some example test1.c:
```
#include <stdint.h>
struct eth_hdr_s
{
uint8_t dest[6]; /* Ethernet destination address (6 bytes) */
uint8_t src[6]; /* Ethernet source address (6 bytes) */
uint16_t type; /* Type code (2 bytes) */
};
struct packed_eth_hdr_s
{
uint8_t dest[6]; /* Ethernet destination address (6 bytes) */
uint8_t src[6]; /* Ethernet source address (6 bytes) */
uint16_t type; /* Type code (2 bytes) */
} __attribute__ ((packed));
uint8_t pkt[128];
#define BUF ((struct eth_hdr_s *)pkt)
#define BUF_PACKED ((struct packed_eth_hdr_s *)pkt)
int f(void)
{
if (BUF->type == 0x0008)
{
return 0;
}
return 1;
}
int f_packed(void)
{
if (BUF_PACKED->type == 0x0008)
{
return 0;
}
return 1;
}
```
It is compiled with `arm-none-eabi-gcc -save-temps -Wall -Wextra -c -mcpu=cortex-m0 -mthumb -mfloat-abi=soft test1.c` and generates the next output:
```
.cpu cortex-m0
.arch armv6s-m
.fpu softvfp
.eabi_attribute 20, 1
.eabi_attribute 21, 1
.eabi_attribute 23, 3
.eabi_attribute 24, 1
.eabi_attribute 25, 1
.eabi_attribute 26, 1
.eabi_attribute 30, 6
.eabi_attribute 34, 0
.eabi_attribute 18, 4
.file "test1.c"
.text
.global pkt
.bss
.align 2
.type pkt, %object
.size pkt, 128
pkt:
.space 128
.text
.align 1
.global f
.syntax unified
.code 16
.thumb_func
.type f, %function
f:
@ args = 0, pretend = 0, frame = 0
@ frame_needed = 1, uses_anonymous_args = 0
push {r7, lr}
add r7, sp, #0
ldr r3, .L4
ldrh r3, [r3, #12]
cmp r3, #8
bne .L2
movs r3, #0
b .L3
.L2:
movs r3, #1
.L3:
movs r0, r3
mov sp, r7
@ sp needed
pop {r7, pc}
.L5:
.align 2
.L4:
.word pkt
.size f, .-f
.align 1
.global f_packed
.syntax unified
.code 16
.thumb_func
.type f_packed, %function
f_packed:
@ args = 0, pretend = 0, frame = 0
@ frame_needed = 1, uses_anonymous_args = 0
push {r7, lr}
add r7, sp, #0
ldr r3, .L9
ldrb r2, [r3, #12]
ldrb r3, [r3, #13]
lsls r3, r3, #8
orrs r3, r2
uxth r3, r3
cmp r3, #8
bne .L7
movs r3, #0
b .L8
.L7:
movs r3, #1
.L8:
movs r0, r3
mov sp, r7
@ sp needed
pop {r7, pc}
.L10:
.align 2
.L9:
.word pkt
.size f_packed, .-f_packed
.ident "GCC: (GNU Arm Embedded Toolchain 10.3-2021.10) 10.3.1 20210824 (release)"
```
So in case is struct is unpacked we are getting `ldrh` instruction that expects 16 bit alignment of `.L4`
```
ldr r3, .L4
ldrh r3, [r3, #12]
cmp r3, #8
```
In case if packed is used we are getting:
```
ldr r3, .L9
ldrb r2, [r3, #12]
ldrb r3, [r3, #13]
lsls r3, r3, #8
orrs r3, r2
uxth r3, r3
cmp r3, #8
```
So `.L9` can have any alignment and the value is reassembled using multiple `ldrb` instructions, byte-shift and "or".
But somehow @ptka reports that it does not work in his case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko commented on pull request #5251: ethernet: apply compiler packing to all ethernet packet based types that are memory mapped
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on pull request #5251:
URL: https://github.com/apache/incubator-nuttx/pull/5251#issuecomment-1014924077
> > @a-lunev could you please also participate in the review since you are doing many changes to the networking sub-system?
>
> So far, I do not well understand if adding "FAR" is tightly related to adding "begin_packed_struct / end_packed_struct". If they are not related issues, I would split the patch into two different ones, because the change is massive.
Done
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-nuttx] pkarashchenko closed pull request #5251: ethernet: apply compiler packing to all ethernet packet based types that are memory mapped
Posted by GitBox <gi...@apache.org>.
pkarashchenko closed pull request #5251:
URL: https://github.com/apache/incubator-nuttx/pull/5251
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org