On 17/01/2019 17:26, Ben Pfaff wrote:
> On Thu, Jan 17, 2019 at 12:55:54PM +0000, Pieter Jansen van Vuuren wrote:
>> 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
>> "
>
> OK. I understand now.
>
> You might be able to work around this by adding a file
> include/sparse/linux/tc_act/tc_pedit.h in the OVS tree with contents
> something like this:
>
> #ifndef FIX_LINUX_TC_PEDIT_H
> #define FIX_LINUX_TC_PEDIT_H
>
> #include_next <linux/tc_act/tc_pedit.h>
>
> /* Fixes endianness of 'mask' and 'val' members. */
> #define tc_pedit_key rpl_tc_pedit_key
> struct rpl_tc_pedit_key {
> __be32 mask; /* AND */
> __be32 val; /*XOR */
> __u32 off; /*offset */
> __u32 at;
> __u32 offmask;
> __u32 shift;
> };
>
> #endif
>
I will do. Thank you.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
