On 16/01/2019 18:38, Ben Pfaff wrote:
> On Wed, Jan 16, 2019 at 09:04:16AM +0000, Pieter Jansen van Vuuren wrote:
>> +/* These functions specifically help shifting values that are in
>> + * network byte order but stored in uint32_t variables. */
>> +static uint32_t shift_ovs_be32_left(uint32_t word, int shift)
>> +{
>> + uint32_t word_shifted = ntohl((OVS_FORCE ovs_be32)word) << shift;
>> +
>> + return (OVS_FORCE uint32_t)htonl(word_shifted);
>> +}
>> +
>> +static uint32_t shift_ovs_be32_right(uint32_t word, int shift)
>> +{
>> + uint32_t word_shifted = ntohl((OVS_FORCE ovs_be32)word) >> shift;
>> +
>> + return (OVS_FORCE uint32_t)htonl(word_shifted);
>> +}
>
> Let me try this again.
>
> Why are we storing network-byte-order data in uint32_t? It should be in
> ovs_be32.
>
Sorry for not explaining this more clearly earlier. I think mask and val in
struct tc_pedit_key in tc_pedit.h holds network-byte-order data. I think we
inherit this from the kernel UAPI:
struct tc_pedit_key {
__u32 mask; /* AND */
__u32 val; /*XOR */
__u32 off; /*offset */
__u32 at;
__u32 offmask;
__u32 shift;
};
(I think at least mask and val here are actually network-byte-order)
Some drivers do this in the kernel for pedit:
/* We are expecting tcf_pedit to return a big endian value */
mask = (__force __be32)~tcf_pedit_mask(action, idx);
exact = (__force __be32)tcf_pedit_val(action, idx);
Others:
mask_be32 = *(__be32 *)&mask;
mask = (__force unsigned long)cpu_to_le32(be32_to_cpu(mask_be32));
Also in userspace iproute2, we see this in tc/m_pedit.c:
tkey->val = htonl(tkey->val & retain);
tkey->mask = htonl(tkey->mask | ~retain);
Iproute2 indicates that there is an issue somewhere:
"
* TODO:
* 1) Big endian broken in some spots
"
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
