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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
