* [stmmac][bug?] endianness of Flexible RX Parser code
@ 2018-08-04 1:19 Al Viro
2018-08-06 8:56 ` Jose Abreu
0 siblings, 1 reply; 2+ messages in thread
From: Al Viro @ 2018-08-04 1:19 UTC (permalink / raw)
To: Jose Abreu; +Cc: David S. Miller, netdev
The values passed in struct tc_u32_sel ->mask and ->val are
32bit net-endian. Your tc_fill_entry() does this:
data = sel->keys[0].val;
mask = sel->keys[0].mask;
...
entry->frag_ptr = frag;
entry->val.match_en = (mask << (rem * 8)) &
GENMASK(31, rem * 8);
entry->val.match_data = (data << (rem * 8)) &
GENMASK(31, rem * 8);
entry->val.frame_offset = real_off;
entry->prio = prio;
frag->val.match_en = (mask >> (rem * 8)) &
GENMASK(rem * 8 - 1, 0);
frag->val.match_data = (data >> (rem * 8)) &
GENMASK(rem * 8 - 1, 0);
frag->val.frame_offset = real_off + 1;
frag->prio = prio;
frag->is_frag = true;
and that looks very odd. rem here is offset modulo 4. Suppose offset is
equal to 5, val contains {V0, V1, V2, V3} and mask - {M0, M1, M2, M3}.
Then on little-endian host we get
entry->val.match_en: {0, M0, M1, M2}
entry->val.match_data: {0, V0, V1, V2}
entry->val.frame_offset = 1;
frag->val.match_en: {M3, 0, 0, 0}
frag->val.match_data: {V3, 0, 0, 0}
frag->val.frame_offset = 2;
and on big-endian
entry->val.match_en: {M1, M2, M3, 0}
entry->val.match_data: {V1, V2, V3, 0}
entry->val.frame_offset = 1;
frag->val.match_en: {0, 0, 0, M0}
frag->val.match_data: {0, 0, 0, V0}
frag->val.frame_offset = 2;
Little-endian variant looks like we mask octets 5, 6, 7 and 8 with
M0..M3 resp. and want V0..V3 in those. On big-endian, though, we
look at the octets 11, 4, 5 and 6 instead.
I don't know the hardware (and it might be pulling any kind of weird
endianness-dependent stunts), but that really smells like a bug.
It looks like that code is trying to do something like
data = ntohl(sel->keys[0].val);
mask = ntohl(sel->keys[0].mask);
shift = rem * 8;
entry->val.match_en = htonl(mask >> shift);
entry->val.match_data = htonl(data >> shift);
entry->val.frame_offset = real_off;
...
frag->val.match_en = htonl(mask << (32 - shift));
frag->val.match_data = htonl(data << (32 - shift));
entry->val.frame_offset = real_off + 1;
Comments?
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [stmmac][bug?] endianness of Flexible RX Parser code
2018-08-04 1:19 [stmmac][bug?] endianness of Flexible RX Parser code Al Viro
@ 2018-08-06 8:56 ` Jose Abreu
0 siblings, 0 replies; 2+ messages in thread
From: Jose Abreu @ 2018-08-06 8:56 UTC (permalink / raw)
To: Al Viro, Jose Abreu; +Cc: David S. Miller, netdev
Hi Al,
On 04-08-2018 02:19, Al Viro wrote:
> The values passed in struct tc_u32_sel ->mask and ->val are
> 32bit net-endian. Your tc_fill_entry() does this:
>
> data = sel->keys[0].val;
> mask = sel->keys[0].mask;
>
> ...
> entry->frag_ptr = frag;
> entry->val.match_en = (mask << (rem * 8)) &
> GENMASK(31, rem * 8);
> entry->val.match_data = (data << (rem * 8)) &
> GENMASK(31, rem * 8);
> entry->val.frame_offset = real_off;
> entry->prio = prio;
>
> frag->val.match_en = (mask >> (rem * 8)) &
> GENMASK(rem * 8 - 1, 0);
> frag->val.match_data = (data >> (rem * 8)) &
> GENMASK(rem * 8 - 1, 0);
> frag->val.frame_offset = real_off + 1;
> frag->prio = prio;
> frag->is_frag = true;
>
> and that looks very odd. rem here is offset modulo 4. Suppose offset is
> equal to 5, val contains {V0, V1, V2, V3} and mask - {M0, M1, M2, M3}.
> Then on little-endian host we get
> entry->val.match_en: {0, M0, M1, M2}
> entry->val.match_data: {0, V0, V1, V2}
> entry->val.frame_offset = 1;
> frag->val.match_en: {M3, 0, 0, 0}
> frag->val.match_data: {V3, 0, 0, 0}
> frag->val.frame_offset = 2;
> and on big-endian
> entry->val.match_en: {M1, M2, M3, 0}
> entry->val.match_data: {V1, V2, V3, 0}
> entry->val.frame_offset = 1;
> frag->val.match_en: {0, 0, 0, M0}
> frag->val.match_data: {0, 0, 0, V0}
> frag->val.frame_offset = 2;
>
> Little-endian variant looks like we mask octets 5, 6, 7 and 8 with
> M0..M3 resp. and want V0..V3 in those. On big-endian, though, we
> look at the octets 11, 4, 5 and 6 instead.
>
> I don't know the hardware (and it might be pulling any kind of weird
> endianness-dependent stunts), but that really smells like a bug.
There is a feature in HW that supports Byte-Invariant Big-Endian
data transfer. It's not activated by default though ...
> It looks like that code is trying to do something like
>
> data = ntohl(sel->keys[0].val);
> mask = ntohl(sel->keys[0].mask);
> shift = rem * 8;
>
> entry->val.match_en = htonl(mask >> shift);
> entry->val.match_data = htonl(data >> shift);
> entry->val.frame_offset = real_off;
> ...
> frag->val.match_en = htonl(mask << (32 - shift));
> frag->val.match_data = htonl(data << (32 - shift));
> entry->val.frame_offset = real_off + 1;
>
> Comments?
Looks good. Can you send a formal patch and a simple command that
I can use to validate this situation?
I used at the time at least two commands: one for validating
simple match by using 1 entry (i.e. plain match) and another to
validate 2 entries (i.e. two matches) and did not encounter any
problem ...
Thanks and Best Regards,
Jose Miguel Abreu
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-08-06 11:04 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-04 1:19 [stmmac][bug?] endianness of Flexible RX Parser code Al Viro
2018-08-06 8:56 ` Jose Abreu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.