Re: [ovs-dev] [patch v4 2/2] packets: ersapn_metadata header fixups.

2018-05-25 Thread Ben Pfaff 
On Fri, May 25, 2018 at 05:56:51AM -0700, William Tu wrote:
> >
> > OK, now I understand what's going on.  This is only used to define the
> > format of the OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS attribute.  Since it's not
> > a wire format at all, we don't need to use ovs_16aligned_be32 either,
> > and an "int" is fine too.
> >
> > Since erspan_metadata is part of the kernel ABI, which is exposed via
> > Netlink, it should normally be defined by including a kernel header
> > rather than in packets.h, which normally just defines wire formats for
> > things.  Can we arrange for that to happen?
> >
> 
> But the kernel UAPI erspan.h defines both 'struct erspan_md2' and
> 'struct erspan_metadata'.  If we choose to include from kernel, then the
> wire format 'strct erspan_md2' isn't defined using ovs_16aligned_be32.

In some cases we have an alternative data structure for use in
situations where we need it, e.g. struct ovs_16aligned_ip6_hdr.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [patch v4 2/2] packets: ersapn_metadata header fixups.

2018-05-25 Thread William Tu 
>
> OK, now I understand what's going on.  This is only used to define the
> format of the OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS attribute.  Since it's not
> a wire format at all, we don't need to use ovs_16aligned_be32 either,
> and an "int" is fine too.
>
> Since erspan_metadata is part of the kernel ABI, which is exposed via
> Netlink, it should normally be defined by including a kernel header
> rather than in packets.h, which normally just defines wire formats for
> things.  Can we arrange for that to happen?
>

But the kernel UAPI erspan.h defines both 'struct erspan_md2' and
'struct erspan_metadata'.  If we choose to include from kernel, then the
wire format 'strct erspan_md2' isn't defined using ovs_16aligned_be32.

or should we generate an erspan.h using kernel UAPI erspan.h?
something like:
sed -f ./build-aux/extract-odp-netlink-h <
datapath/linux/compat/include/linux/openvswitch.h >
include/odp-netlink.h
but generate include/erspan.h

Thanks
William
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [patch v4 2/2] packets: ersapn_metadata header fixups.

2018-05-25 Thread Darrell Ball 
On Thu, May 24, 2018 at 2:16 PM, Ben Pfaff  wrote:

> On Thu, May 24, 2018 at 12:08:31PM -0700, William Tu wrote:
> > On Thu, May 24, 2018 at 10:20 AM, Ben Pfaff  wrote:
> > > On Thu, May 24, 2018 at 05:36:10AM -0700, William Tu wrote:
> > >> On Wed, May 23, 2018 at 7:13 PM, Darrell Ball 
> wrote:
> > >> > The struct erspan_metadata is updated to replace the 'version'
> > >> > placeholder with the erspan base hdr.  Also, the erspan
> > >> > index is defined explicitly as ovs_16aligned_be32 to mirror
> > >> > its encoding.
> > >> > Changes to odp_util result from updating the erspan index
> > >> > type.
> > >> >
> > >> > CC: William Tu 
> > >> > Fixes: 068794b43f0e ("erspan: Add flow-based erspan options")
> > >> > Signed-off-by: Darrell Ball 
> > >> > ---
> > >> >  lib/odp-util.c | 36 
> > >> >  lib/packets.h  |  6 +++---
> > >> >  2 files changed, 23 insertions(+), 19 deletions(-)
> > >> >
> > >> > diff --git a/lib/odp-util.c b/lib/odp-util.c
> > >> > index 105ac80..767281f 100644
> > >> > --- a/lib/odp-util.c
> > >> > +++ b/lib/odp-util.c
> > >> > @@ -2786,9 +2786,9 @@ odp_tun_key_from_attr__(const struct nlattr
> *attr, bool is_mask,
> > >> >
> > >> >  memcpy(, nl_attr_get(a), attr_len);
> > >> >
> > >> > -tun->erspan_ver = opts.version;
> > >> > +tun->erspan_ver = opts.bh.ver;
> > >> >  if (tun->erspan_ver == 1) {
> > >> > -tun->erspan_idx = ntohl(opts.u.index);
> > >> > +tun->erspan_idx = ntohl(get_16aligned_be32(&
> opts.u.index));
> > >> >  } else if (tun->erspan_ver == 2) {
> > >> >  tun->erspan_dir = opts.u.md2.dir;
> > >> >  tun->erspan_hwid = get_hwid();
> > >> > @@ -2890,10 +2890,11 @@ tun_key_to_attr(struct ofpbuf *a, const
> struct flow_tnl *tun_key,
> > >> >  !strcmp(tnl_type, "ip6erspan")) &&
> > >> >  (tun_key->erspan_ver == 1 || tun_key->erspan_ver == 2)) {
> > >> >  struct erspan_metadata opts;
> > >> > +memset(, 0, sizeof opts);
> > >> >
> > >> > -opts.version = tun_key->erspan_ver;
> > >> > -if (opts.version == 1) {
> > >> > -opts.u.index = htonl(tun_key->erspan_idx);
> > >> > +opts.bh.ver = tun_key->erspan_ver;
> > >> > +if (opts.bh.ver == 1) {
> > >> > +put_16aligned_be32(,
> htonl(tun_key->erspan_idx));
> > >> >  } else {
> > >> >  opts.u.md2.dir = tun_key->erspan_dir;
> > >> >  set_hwid(, tun_key->erspan_hwid);
> > >> > @@ -3368,22 +3369,23 @@ format_odp_tun_erspan_opt(const struct
> nlattr *attr,
> > >> >  opts = nl_attr_get(attr);
> > >> >  mask = mask_attr ? nl_attr_get(mask_attr) : NULL;
> > >> >
> > >> > -ver = (uint8_t)opts->version;
> > >> > +ver = opts->bh.ver;
> > >> >  if (mask) {
> > >> > -ver_ma = (uint8_t)mask->version;
> > >> > +ver_ma = mask->bh.ver;
> > >> >  }
> > >> >
> > >> >  format_u8u(ds, "ver", ver, mask ? _ma : NULL, verbose);
> > >> >
> > >> > -if (opts->version == 1) {
> > >> > +if (opts->bh.ver == 1) {
> > >> >  if (mask) {
> > >> >  ds_put_format(ds, "idx=%#"PRIx32"/%#"PRIx32",",
> > >> > -  ntohl(opts->u.index),
> > >> > -  ntohl(mask->u.index));
> > >> > +  ntohl(get_16aligned_be32(&
> opts->u.index)),
> > >> > +  ntohl(get_16aligned_be32(&
> mask->u.index)));
> > >> >  } else {
> > >> > -ds_put_format(ds, "idx=%#"PRIx32",",
> ntohl(opts->u.index));
> > >> > +ds_put_format(ds, "idx=%#"PRIx32",",
> > >> > +  ntohl(get_16aligned_be32(&
> opts->u.index)));
> > >> >  }
> > >> > -} else if (opts->version == 2) {
> > >> > +} else if (opts->bh.ver == 2) {
> > >> >  dir = opts->u.md2.dir;
> > >> >  hwid = opts->u.md2.hwid;
> > >> >  if (mask) {
> > >> > @@ -4859,10 +4861,11 @@ scan_erspan_metadata(const char *s,
> > >> >
> > >> >  if (!strncmp(s, ")", 1)) {
> > >> >  s += 1;
> > >> > -key->version = ver;
> > >> > -key->u.index = htonl(idx);
> > >> > +memset(>bh, 0, sizeof key->bh);
> > >> > +key->bh.ver = ver;
> > >> > +put_16aligned_be32(>u.index, htonl(idx));
> > >> >  if (mask) {
> > >> > -mask->u.index = htonl(idx_mask);
> > >> > +put_16aligned_be32(>u.index,
> htonl(idx_mask));
> > >> >  }
> > >> >  }
> > >> >  return s - s_base;
> > >> > @@ -4882,7 +4885,8 @@ scan_erspan_metadata(const char *s,
> > >> >
> > >> >  if (!strncmp(s, ")", 1)) {
> > >> >  s += 1;
> > >> > -key->version = ver;
> > >> > +memset(>bh, 0, sizeof key->bh);
> > >> > +

Re: [ovs-dev] [patch v4 2/2] packets: ersapn_metadata header fixups.

2018-05-24 Thread Ben Pfaff 
On Thu, May 24, 2018 at 05:22:37PM -0700, William Tu wrote:
> On Thu, May 24, 2018 at 2:16 PM, Ben Pfaff  wrote:
> > On Thu, May 24, 2018 at 12:08:31PM -0700, William Tu wrote:
> >> On Thu, May 24, 2018 at 10:20 AM, Ben Pfaff  wrote:
> >> > On Thu, May 24, 2018 at 05:36:10AM -0700, William Tu wrote:
> >> >> On Wed, May 23, 2018 at 7:13 PM, Darrell Ball  wrote:
> >> >> > The struct erspan_metadata is updated to replace the 'version'
> >> >> > placeholder with the erspan base hdr.  Also, the erspan
> >> >> > index is defined explicitly as ovs_16aligned_be32 to mirror
> >> >> > its encoding.
> >> >> > Changes to odp_util result from updating the erspan index
> >> >> > type.
> >> >> >
> >> >> > CC: William Tu 
> >> >> > Fixes: 068794b43f0e ("erspan: Add flow-based erspan options")
> >> >> > Signed-off-by: Darrell Ball 
> >> >> > ---
> >> >> >  lib/odp-util.c | 36 
> >> >> >  lib/packets.h  |  6 +++---
> >> >> >  2 files changed, 23 insertions(+), 19 deletions(-)
> >> >> >
> >> >> > diff --git a/lib/odp-util.c b/lib/odp-util.c
> >> >> > index 105ac80..767281f 100644
> >> >> > --- a/lib/odp-util.c
> >> >> > +++ b/lib/odp-util.c
> >> >> > @@ -2786,9 +2786,9 @@ odp_tun_key_from_attr__(const struct nlattr 
> >> >> > *attr, bool is_mask,
> >> >> >
> >> >> >  memcpy(, nl_attr_get(a), attr_len);
> >> >> >
> >> >> > -tun->erspan_ver = opts.version;
> >> >> > +tun->erspan_ver = opts.bh.ver;
> >> >> >  if (tun->erspan_ver == 1) {
> >> >> > -tun->erspan_idx = ntohl(opts.u.index);
> >> >> > +tun->erspan_idx = 
> >> >> > ntohl(get_16aligned_be32());
> >> >> >  } else if (tun->erspan_ver == 2) {
> >> >> >  tun->erspan_dir = opts.u.md2.dir;
> >> >> >  tun->erspan_hwid = get_hwid();
> >> >> > @@ -2890,10 +2890,11 @@ tun_key_to_attr(struct ofpbuf *a, const 
> >> >> > struct flow_tnl *tun_key,
> >> >> >  !strcmp(tnl_type, "ip6erspan")) &&
> >> >> >  (tun_key->erspan_ver == 1 || tun_key->erspan_ver == 2)) {
> >> >> >  struct erspan_metadata opts;
> >> >> > +memset(, 0, sizeof opts);
> >> >> >
> >> >> > -opts.version = tun_key->erspan_ver;
> >> >> > -if (opts.version == 1) {
> >> >> > -opts.u.index = htonl(tun_key->erspan_idx);
> >> >> > +opts.bh.ver = tun_key->erspan_ver;
> >> >> > +if (opts.bh.ver == 1) {
> >> >> > +put_16aligned_be32(, 
> >> >> > htonl(tun_key->erspan_idx));
> >> >> >  } else {
> >> >> >  opts.u.md2.dir = tun_key->erspan_dir;
> >> >> >  set_hwid(, tun_key->erspan_hwid);
> >> >> > @@ -3368,22 +3369,23 @@ format_odp_tun_erspan_opt(const struct nlattr 
> >> >> > *attr,
> >> >> >  opts = nl_attr_get(attr);
> >> >> >  mask = mask_attr ? nl_attr_get(mask_attr) : NULL;
> >> >> >
> >> >> > -ver = (uint8_t)opts->version;
> >> >> > +ver = opts->bh.ver;
> >> >> >  if (mask) {
> >> >> > -ver_ma = (uint8_t)mask->version;
> >> >> > +ver_ma = mask->bh.ver;
> >> >> >  }
> >> >> >
> >> >> >  format_u8u(ds, "ver", ver, mask ? _ma : NULL, verbose);
> >> >> >
> >> >> > -if (opts->version == 1) {
> >> >> > +if (opts->bh.ver == 1) {
> >> >> >  if (mask) {
> >> >> >  ds_put_format(ds, "idx=%#"PRIx32"/%#"PRIx32",",
> >> >> > -  ntohl(opts->u.index),
> >> >> > -  ntohl(mask->u.index));
> >> >> > +  ntohl(get_16aligned_be32(>u.index)),
> >> >> > +  ntohl(get_16aligned_be32(>u.index)));
> >> >> >  } else {
> >> >> > -ds_put_format(ds, "idx=%#"PRIx32",", 
> >> >> > ntohl(opts->u.index));
> >> >> > +ds_put_format(ds, "idx=%#"PRIx32",",
> >> >> > +  ntohl(get_16aligned_be32(>u.index)));
> >> >> >  }
> >> >> > -} else if (opts->version == 2) {
> >> >> > +} else if (opts->bh.ver == 2) {
> >> >> >  dir = opts->u.md2.dir;
> >> >> >  hwid = opts->u.md2.hwid;
> >> >> >  if (mask) {
> >> >> > @@ -4859,10 +4861,11 @@ scan_erspan_metadata(const char *s,
> >> >> >
> >> >> >  if (!strncmp(s, ")", 1)) {
> >> >> >  s += 1;
> >> >> > -key->version = ver;
> >> >> > -key->u.index = htonl(idx);
> >> >> > +memset(>bh, 0, sizeof key->bh);
> >> >> > +key->bh.ver = ver;
> >> >> > +put_16aligned_be32(>u.index, htonl(idx));
> >> >> >  if (mask) {
> >> >> > -mask->u.index = htonl(idx_mask);
> >> >> > +put_16aligned_be32(>u.index, htonl(idx_mask));
> >> >> >  }
> >> >> >  }
> >> >> >  return s - s_base;
> >> >> > @@ -4882,7 +4885,8 @@ scan_erspan_metadata(const char *s,
> >> >> >
> >> >> >

Re: [ovs-dev] [patch v4 2/2] packets: ersapn_metadata header fixups.

2018-05-24 Thread William Tu 
On Thu, May 24, 2018 at 2:16 PM, Ben Pfaff  wrote:
> On Thu, May 24, 2018 at 12:08:31PM -0700, William Tu wrote:
>> On Thu, May 24, 2018 at 10:20 AM, Ben Pfaff  wrote:
>> > On Thu, May 24, 2018 at 05:36:10AM -0700, William Tu wrote:
>> >> On Wed, May 23, 2018 at 7:13 PM, Darrell Ball  wrote:
>> >> > The struct erspan_metadata is updated to replace the 'version'
>> >> > placeholder with the erspan base hdr.  Also, the erspan
>> >> > index is defined explicitly as ovs_16aligned_be32 to mirror
>> >> > its encoding.
>> >> > Changes to odp_util result from updating the erspan index
>> >> > type.
>> >> >
>> >> > CC: William Tu 
>> >> > Fixes: 068794b43f0e ("erspan: Add flow-based erspan options")
>> >> > Signed-off-by: Darrell Ball 
>> >> > ---
>> >> >  lib/odp-util.c | 36 
>> >> >  lib/packets.h  |  6 +++---
>> >> >  2 files changed, 23 insertions(+), 19 deletions(-)
>> >> >
>> >> > diff --git a/lib/odp-util.c b/lib/odp-util.c
>> >> > index 105ac80..767281f 100644
>> >> > --- a/lib/odp-util.c
>> >> > +++ b/lib/odp-util.c
>> >> > @@ -2786,9 +2786,9 @@ odp_tun_key_from_attr__(const struct nlattr 
>> >> > *attr, bool is_mask,
>> >> >
>> >> >  memcpy(, nl_attr_get(a), attr_len);
>> >> >
>> >> > -tun->erspan_ver = opts.version;
>> >> > +tun->erspan_ver = opts.bh.ver;
>> >> >  if (tun->erspan_ver == 1) {
>> >> > -tun->erspan_idx = ntohl(opts.u.index);
>> >> > +tun->erspan_idx = 
>> >> > ntohl(get_16aligned_be32());
>> >> >  } else if (tun->erspan_ver == 2) {
>> >> >  tun->erspan_dir = opts.u.md2.dir;
>> >> >  tun->erspan_hwid = get_hwid();
>> >> > @@ -2890,10 +2890,11 @@ tun_key_to_attr(struct ofpbuf *a, const struct 
>> >> > flow_tnl *tun_key,
>> >> >  !strcmp(tnl_type, "ip6erspan")) &&
>> >> >  (tun_key->erspan_ver == 1 || tun_key->erspan_ver == 2)) {
>> >> >  struct erspan_metadata opts;
>> >> > +memset(, 0, sizeof opts);
>> >> >
>> >> > -opts.version = tun_key->erspan_ver;
>> >> > -if (opts.version == 1) {
>> >> > -opts.u.index = htonl(tun_key->erspan_idx);
>> >> > +opts.bh.ver = tun_key->erspan_ver;
>> >> > +if (opts.bh.ver == 1) {
>> >> > +put_16aligned_be32(, 
>> >> > htonl(tun_key->erspan_idx));
>> >> >  } else {
>> >> >  opts.u.md2.dir = tun_key->erspan_dir;
>> >> >  set_hwid(, tun_key->erspan_hwid);
>> >> > @@ -3368,22 +3369,23 @@ format_odp_tun_erspan_opt(const struct nlattr 
>> >> > *attr,
>> >> >  opts = nl_attr_get(attr);
>> >> >  mask = mask_attr ? nl_attr_get(mask_attr) : NULL;
>> >> >
>> >> > -ver = (uint8_t)opts->version;
>> >> > +ver = opts->bh.ver;
>> >> >  if (mask) {
>> >> > -ver_ma = (uint8_t)mask->version;
>> >> > +ver_ma = mask->bh.ver;
>> >> >  }
>> >> >
>> >> >  format_u8u(ds, "ver", ver, mask ? _ma : NULL, verbose);
>> >> >
>> >> > -if (opts->version == 1) {
>> >> > +if (opts->bh.ver == 1) {
>> >> >  if (mask) {
>> >> >  ds_put_format(ds, "idx=%#"PRIx32"/%#"PRIx32",",
>> >> > -  ntohl(opts->u.index),
>> >> > -  ntohl(mask->u.index));
>> >> > +  ntohl(get_16aligned_be32(>u.index)),
>> >> > +  ntohl(get_16aligned_be32(>u.index)));
>> >> >  } else {
>> >> > -ds_put_format(ds, "idx=%#"PRIx32",", ntohl(opts->u.index));
>> >> > +ds_put_format(ds, "idx=%#"PRIx32",",
>> >> > +  ntohl(get_16aligned_be32(>u.index)));
>> >> >  }
>> >> > -} else if (opts->version == 2) {
>> >> > +} else if (opts->bh.ver == 2) {
>> >> >  dir = opts->u.md2.dir;
>> >> >  hwid = opts->u.md2.hwid;
>> >> >  if (mask) {
>> >> > @@ -4859,10 +4861,11 @@ scan_erspan_metadata(const char *s,
>> >> >
>> >> >  if (!strncmp(s, ")", 1)) {
>> >> >  s += 1;
>> >> > -key->version = ver;
>> >> > -key->u.index = htonl(idx);
>> >> > +memset(>bh, 0, sizeof key->bh);
>> >> > +key->bh.ver = ver;
>> >> > +put_16aligned_be32(>u.index, htonl(idx));
>> >> >  if (mask) {
>> >> > -mask->u.index = htonl(idx_mask);
>> >> > +put_16aligned_be32(>u.index, htonl(idx_mask));
>> >> >  }
>> >> >  }
>> >> >  return s - s_base;
>> >> > @@ -4882,7 +4885,8 @@ scan_erspan_metadata(const char *s,
>> >> >
>> >> >  if (!strncmp(s, ")", 1)) {
>> >> >  s += 1;
>> >> > -key->version = ver;
>> >> > +memset(>bh, 0, sizeof key->bh);
>> >> > +key->bh.ver = ver;
>> >> >  key->u.md2.hwid = hwid;
>> >> >  key->u.md2.dir = dir;
>>

Re: [ovs-dev] [patch v4 2/2] packets: ersapn_metadata header fixups.

2018-05-24 Thread Ben Pfaff 
On Thu, May 24, 2018 at 12:08:31PM -0700, William Tu wrote:
> On Thu, May 24, 2018 at 10:20 AM, Ben Pfaff  wrote:
> > On Thu, May 24, 2018 at 05:36:10AM -0700, William Tu wrote:
> >> On Wed, May 23, 2018 at 7:13 PM, Darrell Ball  wrote:
> >> > The struct erspan_metadata is updated to replace the 'version'
> >> > placeholder with the erspan base hdr.  Also, the erspan
> >> > index is defined explicitly as ovs_16aligned_be32 to mirror
> >> > its encoding.
> >> > Changes to odp_util result from updating the erspan index
> >> > type.
> >> >
> >> > CC: William Tu 
> >> > Fixes: 068794b43f0e ("erspan: Add flow-based erspan options")
> >> > Signed-off-by: Darrell Ball 
> >> > ---
> >> >  lib/odp-util.c | 36 
> >> >  lib/packets.h  |  6 +++---
> >> >  2 files changed, 23 insertions(+), 19 deletions(-)
> >> >
> >> > diff --git a/lib/odp-util.c b/lib/odp-util.c
> >> > index 105ac80..767281f 100644
> >> > --- a/lib/odp-util.c
> >> > +++ b/lib/odp-util.c
> >> > @@ -2786,9 +2786,9 @@ odp_tun_key_from_attr__(const struct nlattr *attr, 
> >> > bool is_mask,
> >> >
> >> >  memcpy(, nl_attr_get(a), attr_len);
> >> >
> >> > -tun->erspan_ver = opts.version;
> >> > +tun->erspan_ver = opts.bh.ver;
> >> >  if (tun->erspan_ver == 1) {
> >> > -tun->erspan_idx = ntohl(opts.u.index);
> >> > +tun->erspan_idx = 
> >> > ntohl(get_16aligned_be32());
> >> >  } else if (tun->erspan_ver == 2) {
> >> >  tun->erspan_dir = opts.u.md2.dir;
> >> >  tun->erspan_hwid = get_hwid();
> >> > @@ -2890,10 +2890,11 @@ tun_key_to_attr(struct ofpbuf *a, const struct 
> >> > flow_tnl *tun_key,
> >> >  !strcmp(tnl_type, "ip6erspan")) &&
> >> >  (tun_key->erspan_ver == 1 || tun_key->erspan_ver == 2)) {
> >> >  struct erspan_metadata opts;
> >> > +memset(, 0, sizeof opts);
> >> >
> >> > -opts.version = tun_key->erspan_ver;
> >> > -if (opts.version == 1) {
> >> > -opts.u.index = htonl(tun_key->erspan_idx);
> >> > +opts.bh.ver = tun_key->erspan_ver;
> >> > +if (opts.bh.ver == 1) {
> >> > +put_16aligned_be32(, 
> >> > htonl(tun_key->erspan_idx));
> >> >  } else {
> >> >  opts.u.md2.dir = tun_key->erspan_dir;
> >> >  set_hwid(, tun_key->erspan_hwid);
> >> > @@ -3368,22 +3369,23 @@ format_odp_tun_erspan_opt(const struct nlattr 
> >> > *attr,
> >> >  opts = nl_attr_get(attr);
> >> >  mask = mask_attr ? nl_attr_get(mask_attr) : NULL;
> >> >
> >> > -ver = (uint8_t)opts->version;
> >> > +ver = opts->bh.ver;
> >> >  if (mask) {
> >> > -ver_ma = (uint8_t)mask->version;
> >> > +ver_ma = mask->bh.ver;
> >> >  }
> >> >
> >> >  format_u8u(ds, "ver", ver, mask ? _ma : NULL, verbose);
> >> >
> >> > -if (opts->version == 1) {
> >> > +if (opts->bh.ver == 1) {
> >> >  if (mask) {
> >> >  ds_put_format(ds, "idx=%#"PRIx32"/%#"PRIx32",",
> >> > -  ntohl(opts->u.index),
> >> > -  ntohl(mask->u.index));
> >> > +  ntohl(get_16aligned_be32(>u.index)),
> >> > +  ntohl(get_16aligned_be32(>u.index)));
> >> >  } else {
> >> > -ds_put_format(ds, "idx=%#"PRIx32",", ntohl(opts->u.index));
> >> > +ds_put_format(ds, "idx=%#"PRIx32",",
> >> > +  ntohl(get_16aligned_be32(>u.index)));
> >> >  }
> >> > -} else if (opts->version == 2) {
> >> > +} else if (opts->bh.ver == 2) {
> >> >  dir = opts->u.md2.dir;
> >> >  hwid = opts->u.md2.hwid;
> >> >  if (mask) {
> >> > @@ -4859,10 +4861,11 @@ scan_erspan_metadata(const char *s,
> >> >
> >> >  if (!strncmp(s, ")", 1)) {
> >> >  s += 1;
> >> > -key->version = ver;
> >> > -key->u.index = htonl(idx);
> >> > +memset(>bh, 0, sizeof key->bh);
> >> > +key->bh.ver = ver;
> >> > +put_16aligned_be32(>u.index, htonl(idx));
> >> >  if (mask) {
> >> > -mask->u.index = htonl(idx_mask);
> >> > +put_16aligned_be32(>u.index, htonl(idx_mask));
> >> >  }
> >> >  }
> >> >  return s - s_base;
> >> > @@ -4882,7 +4885,8 @@ scan_erspan_metadata(const char *s,
> >> >
> >> >  if (!strncmp(s, ")", 1)) {
> >> >  s += 1;
> >> > -key->version = ver;
> >> > +memset(>bh, 0, sizeof key->bh);
> >> > +key->bh.ver = ver;
> >> >  key->u.md2.hwid = hwid;
> >> >  key->u.md2.dir = dir;
> >> >  if (mask) {
> >> > diff --git a/lib/packets.h b/lib/packets.h
> >> > index 7645a9d..5c013a3 100644
> >> > --- a/lib/packets.h
> >> > +++ b/lib/packets.h
> >> >

Re: [ovs-dev] [patch v4 2/2] packets: ersapn_metadata header fixups.

2018-05-24 Thread William Tu 
On Thu, May 24, 2018 at 10:20 AM, Ben Pfaff  wrote:
> On Thu, May 24, 2018 at 05:36:10AM -0700, William Tu wrote:
>> On Wed, May 23, 2018 at 7:13 PM, Darrell Ball  wrote:
>> > The struct erspan_metadata is updated to replace the 'version'
>> > placeholder with the erspan base hdr.  Also, the erspan
>> > index is defined explicitly as ovs_16aligned_be32 to mirror
>> > its encoding.
>> > Changes to odp_util result from updating the erspan index
>> > type.
>> >
>> > CC: William Tu 
>> > Fixes: 068794b43f0e ("erspan: Add flow-based erspan options")
>> > Signed-off-by: Darrell Ball 
>> > ---
>> >  lib/odp-util.c | 36 
>> >  lib/packets.h  |  6 +++---
>> >  2 files changed, 23 insertions(+), 19 deletions(-)
>> >
>> > diff --git a/lib/odp-util.c b/lib/odp-util.c
>> > index 105ac80..767281f 100644
>> > --- a/lib/odp-util.c
>> > +++ b/lib/odp-util.c
>> > @@ -2786,9 +2786,9 @@ odp_tun_key_from_attr__(const struct nlattr *attr, 
>> > bool is_mask,
>> >
>> >  memcpy(, nl_attr_get(a), attr_len);
>> >
>> > -tun->erspan_ver = opts.version;
>> > +tun->erspan_ver = opts.bh.ver;
>> >  if (tun->erspan_ver == 1) {
>> > -tun->erspan_idx = ntohl(opts.u.index);
>> > +tun->erspan_idx = 
>> > ntohl(get_16aligned_be32());
>> >  } else if (tun->erspan_ver == 2) {
>> >  tun->erspan_dir = opts.u.md2.dir;
>> >  tun->erspan_hwid = get_hwid();
>> > @@ -2890,10 +2890,11 @@ tun_key_to_attr(struct ofpbuf *a, const struct 
>> > flow_tnl *tun_key,
>> >  !strcmp(tnl_type, "ip6erspan")) &&
>> >  (tun_key->erspan_ver == 1 || tun_key->erspan_ver == 2)) {
>> >  struct erspan_metadata opts;
>> > +memset(, 0, sizeof opts);
>> >
>> > -opts.version = tun_key->erspan_ver;
>> > -if (opts.version == 1) {
>> > -opts.u.index = htonl(tun_key->erspan_idx);
>> > +opts.bh.ver = tun_key->erspan_ver;
>> > +if (opts.bh.ver == 1) {
>> > +put_16aligned_be32(, htonl(tun_key->erspan_idx));
>> >  } else {
>> >  opts.u.md2.dir = tun_key->erspan_dir;
>> >  set_hwid(, tun_key->erspan_hwid);
>> > @@ -3368,22 +3369,23 @@ format_odp_tun_erspan_opt(const struct nlattr 
>> > *attr,
>> >  opts = nl_attr_get(attr);
>> >  mask = mask_attr ? nl_attr_get(mask_attr) : NULL;
>> >
>> > -ver = (uint8_t)opts->version;
>> > +ver = opts->bh.ver;
>> >  if (mask) {
>> > -ver_ma = (uint8_t)mask->version;
>> > +ver_ma = mask->bh.ver;
>> >  }
>> >
>> >  format_u8u(ds, "ver", ver, mask ? _ma : NULL, verbose);
>> >
>> > -if (opts->version == 1) {
>> > +if (opts->bh.ver == 1) {
>> >  if (mask) {
>> >  ds_put_format(ds, "idx=%#"PRIx32"/%#"PRIx32",",
>> > -  ntohl(opts->u.index),
>> > -  ntohl(mask->u.index));
>> > +  ntohl(get_16aligned_be32(>u.index)),
>> > +  ntohl(get_16aligned_be32(>u.index)));
>> >  } else {
>> > -ds_put_format(ds, "idx=%#"PRIx32",", ntohl(opts->u.index));
>> > +ds_put_format(ds, "idx=%#"PRIx32",",
>> > +  ntohl(get_16aligned_be32(>u.index)));
>> >  }
>> > -} else if (opts->version == 2) {
>> > +} else if (opts->bh.ver == 2) {
>> >  dir = opts->u.md2.dir;
>> >  hwid = opts->u.md2.hwid;
>> >  if (mask) {
>> > @@ -4859,10 +4861,11 @@ scan_erspan_metadata(const char *s,
>> >
>> >  if (!strncmp(s, ")", 1)) {
>> >  s += 1;
>> > -key->version = ver;
>> > -key->u.index = htonl(idx);
>> > +memset(>bh, 0, sizeof key->bh);
>> > +key->bh.ver = ver;
>> > +put_16aligned_be32(>u.index, htonl(idx));
>> >  if (mask) {
>> > -mask->u.index = htonl(idx_mask);
>> > +put_16aligned_be32(>u.index, htonl(idx_mask));
>> >  }
>> >  }
>> >  return s - s_base;
>> > @@ -4882,7 +4885,8 @@ scan_erspan_metadata(const char *s,
>> >
>> >  if (!strncmp(s, ")", 1)) {
>> >  s += 1;
>> > -key->version = ver;
>> > +memset(>bh, 0, sizeof key->bh);
>> > +key->bh.ver = ver;
>> >  key->u.md2.hwid = hwid;
>> >  key->u.md2.dir = dir;
>> >  if (mask) {
>> > diff --git a/lib/packets.h b/lib/packets.h
>> > index 7645a9d..5c013a3 100644
>> > --- a/lib/packets.h
>> > +++ b/lib/packets.h
>> > @@ -1312,10 +1312,10 @@ struct erspan_md2 {
>> >  };
>> >
>> >  struct erspan_metadata {
>> > -int version;
>> > +struct erspan_base_hdr bh;
>> >  union {
>> > -ovs_be32 index; /* Version 1 (type II)*/
>> > -struct erspan_md2 md2;  /* Version 2 (type III) */

Re: [ovs-dev] [patch v4 2/2] packets: ersapn_metadata header fixups.

2018-05-24 Thread Ben Pfaff 
On Thu, May 24, 2018 at 05:36:10AM -0700, William Tu wrote:
> On Wed, May 23, 2018 at 7:13 PM, Darrell Ball  wrote:
> > The struct erspan_metadata is updated to replace the 'version'
> > placeholder with the erspan base hdr.  Also, the erspan
> > index is defined explicitly as ovs_16aligned_be32 to mirror
> > its encoding.
> > Changes to odp_util result from updating the erspan index
> > type.
> >
> > CC: William Tu 
> > Fixes: 068794b43f0e ("erspan: Add flow-based erspan options")
> > Signed-off-by: Darrell Ball 
> > ---
> >  lib/odp-util.c | 36 
> >  lib/packets.h  |  6 +++---
> >  2 files changed, 23 insertions(+), 19 deletions(-)
> >
> > diff --git a/lib/odp-util.c b/lib/odp-util.c
> > index 105ac80..767281f 100644
> > --- a/lib/odp-util.c
> > +++ b/lib/odp-util.c
> > @@ -2786,9 +2786,9 @@ odp_tun_key_from_attr__(const struct nlattr *attr, 
> > bool is_mask,
> >
> >  memcpy(, nl_attr_get(a), attr_len);
> >
> > -tun->erspan_ver = opts.version;
> > +tun->erspan_ver = opts.bh.ver;
> >  if (tun->erspan_ver == 1) {
> > -tun->erspan_idx = ntohl(opts.u.index);
> > +tun->erspan_idx = ntohl(get_16aligned_be32());
> >  } else if (tun->erspan_ver == 2) {
> >  tun->erspan_dir = opts.u.md2.dir;
> >  tun->erspan_hwid = get_hwid();
> > @@ -2890,10 +2890,11 @@ tun_key_to_attr(struct ofpbuf *a, const struct 
> > flow_tnl *tun_key,
> >  !strcmp(tnl_type, "ip6erspan")) &&
> >  (tun_key->erspan_ver == 1 || tun_key->erspan_ver == 2)) {
> >  struct erspan_metadata opts;
> > +memset(, 0, sizeof opts);
> >
> > -opts.version = tun_key->erspan_ver;
> > -if (opts.version == 1) {
> > -opts.u.index = htonl(tun_key->erspan_idx);
> > +opts.bh.ver = tun_key->erspan_ver;
> > +if (opts.bh.ver == 1) {
> > +put_16aligned_be32(, htonl(tun_key->erspan_idx));
> >  } else {
> >  opts.u.md2.dir = tun_key->erspan_dir;
> >  set_hwid(, tun_key->erspan_hwid);
> > @@ -3368,22 +3369,23 @@ format_odp_tun_erspan_opt(const struct nlattr *attr,
> >  opts = nl_attr_get(attr);
> >  mask = mask_attr ? nl_attr_get(mask_attr) : NULL;
> >
> > -ver = (uint8_t)opts->version;
> > +ver = opts->bh.ver;
> >  if (mask) {
> > -ver_ma = (uint8_t)mask->version;
> > +ver_ma = mask->bh.ver;
> >  }
> >
> >  format_u8u(ds, "ver", ver, mask ? _ma : NULL, verbose);
> >
> > -if (opts->version == 1) {
> > +if (opts->bh.ver == 1) {
> >  if (mask) {
> >  ds_put_format(ds, "idx=%#"PRIx32"/%#"PRIx32",",
> > -  ntohl(opts->u.index),
> > -  ntohl(mask->u.index));
> > +  ntohl(get_16aligned_be32(>u.index)),
> > +  ntohl(get_16aligned_be32(>u.index)));
> >  } else {
> > -ds_put_format(ds, "idx=%#"PRIx32",", ntohl(opts->u.index));
> > +ds_put_format(ds, "idx=%#"PRIx32",",
> > +  ntohl(get_16aligned_be32(>u.index)));
> >  }
> > -} else if (opts->version == 2) {
> > +} else if (opts->bh.ver == 2) {
> >  dir = opts->u.md2.dir;
> >  hwid = opts->u.md2.hwid;
> >  if (mask) {
> > @@ -4859,10 +4861,11 @@ scan_erspan_metadata(const char *s,
> >
> >  if (!strncmp(s, ")", 1)) {
> >  s += 1;
> > -key->version = ver;
> > -key->u.index = htonl(idx);
> > +memset(>bh, 0, sizeof key->bh);
> > +key->bh.ver = ver;
> > +put_16aligned_be32(>u.index, htonl(idx));
> >  if (mask) {
> > -mask->u.index = htonl(idx_mask);
> > +put_16aligned_be32(>u.index, htonl(idx_mask));
> >  }
> >  }
> >  return s - s_base;
> > @@ -4882,7 +4885,8 @@ scan_erspan_metadata(const char *s,
> >
> >  if (!strncmp(s, ")", 1)) {
> >  s += 1;
> > -key->version = ver;
> > +memset(>bh, 0, sizeof key->bh);
> > +key->bh.ver = ver;
> >  key->u.md2.hwid = hwid;
> >  key->u.md2.dir = dir;
> >  if (mask) {
> > diff --git a/lib/packets.h b/lib/packets.h
> > index 7645a9d..5c013a3 100644
> > --- a/lib/packets.h
> > +++ b/lib/packets.h
> > @@ -1312,10 +1312,10 @@ struct erspan_md2 {
> >  };
> >
> >  struct erspan_metadata {
> > -int version;
> > +struct erspan_base_hdr bh;
> >  union {
> > -ovs_be32 index; /* Version 1 (type II)*/
> > -struct erspan_md2 md2;  /* Version 2 (type III) */
> > +ovs_16aligned_be32 index;  /* Version 1 (type II). */
> > +struct erspan_md2 md2; /* Version 2 (type III). */
> >  } u;
> >  };
> >
> Thanks for the patch.
> 
> We

Re: [ovs-dev] [patch v4 2/2] packets: ersapn_metadata header fixups.

2018-05-24 Thread William Tu 
On Wed, May 23, 2018 at 7:13 PM, Darrell Ball  wrote:
> The struct erspan_metadata is updated to replace the 'version'
> placeholder with the erspan base hdr.  Also, the erspan
> index is defined explicitly as ovs_16aligned_be32 to mirror
> its encoding.
> Changes to odp_util result from updating the erspan index
> type.
>
> CC: William Tu 
> Fixes: 068794b43f0e ("erspan: Add flow-based erspan options")
> Signed-off-by: Darrell Ball 
> ---
>  lib/odp-util.c | 36 
>  lib/packets.h  |  6 +++---
>  2 files changed, 23 insertions(+), 19 deletions(-)
>
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 105ac80..767281f 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -2786,9 +2786,9 @@ odp_tun_key_from_attr__(const struct nlattr *attr, bool 
> is_mask,
>
>  memcpy(, nl_attr_get(a), attr_len);
>
> -tun->erspan_ver = opts.version;
> +tun->erspan_ver = opts.bh.ver;
>  if (tun->erspan_ver == 1) {
> -tun->erspan_idx = ntohl(opts.u.index);
> +tun->erspan_idx = ntohl(get_16aligned_be32());
>  } else if (tun->erspan_ver == 2) {
>  tun->erspan_dir = opts.u.md2.dir;
>  tun->erspan_hwid = get_hwid();
> @@ -2890,10 +2890,11 @@ tun_key_to_attr(struct ofpbuf *a, const struct 
> flow_tnl *tun_key,
>  !strcmp(tnl_type, "ip6erspan")) &&
>  (tun_key->erspan_ver == 1 || tun_key->erspan_ver == 2)) {
>  struct erspan_metadata opts;
> +memset(, 0, sizeof opts);
>
> -opts.version = tun_key->erspan_ver;
> -if (opts.version == 1) {
> -opts.u.index = htonl(tun_key->erspan_idx);
> +opts.bh.ver = tun_key->erspan_ver;
> +if (opts.bh.ver == 1) {
> +put_16aligned_be32(, htonl(tun_key->erspan_idx));
>  } else {
>  opts.u.md2.dir = tun_key->erspan_dir;
>  set_hwid(, tun_key->erspan_hwid);
> @@ -3368,22 +3369,23 @@ format_odp_tun_erspan_opt(const struct nlattr *attr,
>  opts = nl_attr_get(attr);
>  mask = mask_attr ? nl_attr_get(mask_attr) : NULL;
>
> -ver = (uint8_t)opts->version;
> +ver = opts->bh.ver;
>  if (mask) {
> -ver_ma = (uint8_t)mask->version;
> +ver_ma = mask->bh.ver;
>  }
>
>  format_u8u(ds, "ver", ver, mask ? _ma : NULL, verbose);
>
> -if (opts->version == 1) {
> +if (opts->bh.ver == 1) {
>  if (mask) {
>  ds_put_format(ds, "idx=%#"PRIx32"/%#"PRIx32",",
> -  ntohl(opts->u.index),
> -  ntohl(mask->u.index));
> +  ntohl(get_16aligned_be32(>u.index)),
> +  ntohl(get_16aligned_be32(>u.index)));
>  } else {
> -ds_put_format(ds, "idx=%#"PRIx32",", ntohl(opts->u.index));
> +ds_put_format(ds, "idx=%#"PRIx32",",
> +  ntohl(get_16aligned_be32(>u.index)));
>  }
> -} else if (opts->version == 2) {
> +} else if (opts->bh.ver == 2) {
>  dir = opts->u.md2.dir;
>  hwid = opts->u.md2.hwid;
>  if (mask) {
> @@ -4859,10 +4861,11 @@ scan_erspan_metadata(const char *s,
>
>  if (!strncmp(s, ")", 1)) {
>  s += 1;
> -key->version = ver;
> -key->u.index = htonl(idx);
> +memset(>bh, 0, sizeof key->bh);
> +key->bh.ver = ver;
> +put_16aligned_be32(>u.index, htonl(idx));
>  if (mask) {
> -mask->u.index = htonl(idx_mask);
> +put_16aligned_be32(>u.index, htonl(idx_mask));
>  }
>  }
>  return s - s_base;
> @@ -4882,7 +4885,8 @@ scan_erspan_metadata(const char *s,
>
>  if (!strncmp(s, ")", 1)) {
>  s += 1;
> -key->version = ver;
> +memset(>bh, 0, sizeof key->bh);
> +key->bh.ver = ver;
>  key->u.md2.hwid = hwid;
>  key->u.md2.dir = dir;
>  if (mask) {
> diff --git a/lib/packets.h b/lib/packets.h
> index 7645a9d..5c013a3 100644
> --- a/lib/packets.h
> +++ b/lib/packets.h
> @@ -1312,10 +1312,10 @@ struct erspan_md2 {
>  };
>
>  struct erspan_metadata {
> -int version;
> +struct erspan_base_hdr bh;
>  union {
> -ovs_be32 index; /* Version 1 (type II)*/
> -struct erspan_md2 md2;  /* Version 2 (type III) */
> +ovs_16aligned_be32 index;  /* Version 1 (type II). */
> +struct erspan_md2 md2; /* Version 2 (type III). */
>  } u;
>  };
>
Thanks for the patch.

We shouldn't change this header since struct erspan_metadata is exposed
from kernel's UAPI header.  (see include/uapi/linux/erspan.h).
OVS kernel module expects this binary format.

Regards,
William
___
dev mailing list
d...@openvswitch.org

[ovs-dev] [patch v4 2/2] packets: ersapn_metadata header fixups.

2018-05-23 Thread Darrell Ball 
The struct erspan_metadata is updated to replace the 'version'
placeholder with the erspan base hdr.  Also, the erspan
index is defined explicitly as ovs_16aligned_be32 to mirror
its encoding.
Changes to odp_util result from updating the erspan index
type.

CC: William Tu 
Fixes: 068794b43f0e ("erspan: Add flow-based erspan options")
Signed-off-by: Darrell Ball 
---
 lib/odp-util.c | 36 
 lib/packets.h  |  6 +++---
 2 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 105ac80..767281f 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -2786,9 +2786,9 @@ odp_tun_key_from_attr__(const struct nlattr *attr, bool 
is_mask,
 
 memcpy(, nl_attr_get(a), attr_len);
 
-tun->erspan_ver = opts.version;
+tun->erspan_ver = opts.bh.ver;
 if (tun->erspan_ver == 1) {
-tun->erspan_idx = ntohl(opts.u.index);
+tun->erspan_idx = ntohl(get_16aligned_be32());
 } else if (tun->erspan_ver == 2) {
 tun->erspan_dir = opts.u.md2.dir;
 tun->erspan_hwid = get_hwid();
@@ -2890,10 +2890,11 @@ tun_key_to_attr(struct ofpbuf *a, const struct flow_tnl 
*tun_key,
 !strcmp(tnl_type, "ip6erspan")) &&
 (tun_key->erspan_ver == 1 || tun_key->erspan_ver == 2)) {
 struct erspan_metadata opts;
+memset(, 0, sizeof opts);
 
-opts.version = tun_key->erspan_ver;
-if (opts.version == 1) {
-opts.u.index = htonl(tun_key->erspan_idx);
+opts.bh.ver = tun_key->erspan_ver;
+if (opts.bh.ver == 1) {
+put_16aligned_be32(, htonl(tun_key->erspan_idx));
 } else {
 opts.u.md2.dir = tun_key->erspan_dir;
 set_hwid(, tun_key->erspan_hwid);
@@ -3368,22 +3369,23 @@ format_odp_tun_erspan_opt(const struct nlattr *attr,
 opts = nl_attr_get(attr);
 mask = mask_attr ? nl_attr_get(mask_attr) : NULL;
 
-ver = (uint8_t)opts->version;
+ver = opts->bh.ver;
 if (mask) {
-ver_ma = (uint8_t)mask->version;
+ver_ma = mask->bh.ver;
 }
 
 format_u8u(ds, "ver", ver, mask ? _ma : NULL, verbose);
 
-if (opts->version == 1) {
+if (opts->bh.ver == 1) {
 if (mask) {
 ds_put_format(ds, "idx=%#"PRIx32"/%#"PRIx32",",
-  ntohl(opts->u.index),
-  ntohl(mask->u.index));
+  ntohl(get_16aligned_be32(>u.index)),
+  ntohl(get_16aligned_be32(>u.index)));
 } else {
-ds_put_format(ds, "idx=%#"PRIx32",", ntohl(opts->u.index));
+ds_put_format(ds, "idx=%#"PRIx32",",
+  ntohl(get_16aligned_be32(>u.index)));
 }
-} else if (opts->version == 2) {
+} else if (opts->bh.ver == 2) {
 dir = opts->u.md2.dir;
 hwid = opts->u.md2.hwid;
 if (mask) {
@@ -4859,10 +4861,11 @@ scan_erspan_metadata(const char *s,
 
 if (!strncmp(s, ")", 1)) {
 s += 1;
-key->version = ver;
-key->u.index = htonl(idx);
+memset(>bh, 0, sizeof key->bh);
+key->bh.ver = ver;
+put_16aligned_be32(>u.index, htonl(idx));
 if (mask) {
-mask->u.index = htonl(idx_mask);
+put_16aligned_be32(>u.index, htonl(idx_mask));
 }
 }
 return s - s_base;
@@ -4882,7 +4885,8 @@ scan_erspan_metadata(const char *s,
 
 if (!strncmp(s, ")", 1)) {
 s += 1;
-key->version = ver;
+memset(>bh, 0, sizeof key->bh);
+key->bh.ver = ver;
 key->u.md2.hwid = hwid;
 key->u.md2.dir = dir;
 if (mask) {
diff --git a/lib/packets.h b/lib/packets.h
index 7645a9d..5c013a3 100644
--- a/lib/packets.h
+++ b/lib/packets.h
@@ -1312,10 +1312,10 @@ struct erspan_md2 {
 };
 
 struct erspan_metadata {
-int version;
+struct erspan_base_hdr bh;
 union {
-ovs_be32 index; /* Version 1 (type II)*/
-struct erspan_md2 md2;  /* Version 2 (type III) */
+ovs_16aligned_be32 index;  /* Version 1 (type II). */
+struct erspan_md2 md2; /* Version 2 (type III). */
 } u;
 };
 
-- 
1.9.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev