On 30/08/2018 14:33, Simon Horman wrote: > Hi Pieter, > > On Tue, Aug 21, 2018 at 02:49:45PM +0100, Pieter Jansen van Vuuren wrote: >> Add TC offload support for encapsulating geneve tunnels with options. >> >> Signed-off-by: Pieter Jansen van Vuuren >> <pieter.jansenvanvuu...@netronome.com> >> Reviewed-by: Simon Horman <simon.hor...@netronome.com> > > thanks for your patch. In general I am happy with this and would value any > feedback from others. > > Travis noticed an alignment problem which I have annotated inline. > >> --- >> include/linux/tc_act/tc_tunnel_key.h | 23 ++++++ >> lib/netdev-tc-offloads.c | 30 ++++++++ >> lib/tc.c | 106 ++++++++++++++++++++++++++- >> lib/tc.h | 2 + >> 4 files changed, 158 insertions(+), 3 deletions(-) >> >> diff --git a/include/linux/tc_act/tc_tunnel_key.h >> b/include/linux/tc_act/tc_tunnel_key.h >> index 26cbd2ff1..f13acf17d 100644 >> --- a/include/linux/tc_act/tc_tunnel_key.h >> +++ b/include/linux/tc_act/tc_tunnel_key.h >> @@ -47,6 +47,29 @@ enum { >> >> #define TCA_TUNNEL_KEY_MAX (__TCA_TUNNEL_KEY_MAX - 1) >> >> +enum { >> + TCA_TUNNEL_KEY_ENC_OPTS_UNSPEC, >> + TCA_TUNNEL_KEY_ENC_OPTS_GENEVE, /* Nested >> + * TCA_TUNNEL_KEY_ENC_OPTS_GENEVE >> + * attributes >> + */ >> + __TCA_TUNNEL_KEY_ENC_OPTS_MAX, >> +}; >> + >> +#define TCA_TUNNEL_KEY_ENC_OPTS_MAX (__TCA_TUNNEL_KEY_ENC_OPTS_MAX - 1) >> + >> +enum { >> + TCA_TUNNEL_KEY_ENC_OPT_GENEVE_UNSPEC, >> + TCA_TUNNEL_KEY_ENC_OPT_GENEVE_CLASS, /* be16 */ >> + TCA_TUNNEL_KEY_ENC_OPT_GENEVE_TYPE, /* u8 */ >> + TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA, /* 4 to 128 bytes */ >> + >> + __TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX, >> +}; >> + >> +#define TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX \ >> + (__TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX - 1) >> + >> #endif /* __KERNEL__ || HAVE_TCA_TUNNEL_KEY_ENC_TTL */ >> >> #endif /* __LINUX_TC_ACT_TC_TUNNEL_KEY_WRAPPER_H */ >> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c >> index 7bc745e95..0612b1348 100644 >> --- a/lib/netdev-tc-offloads.c >> +++ b/lib/netdev-tc-offloads.c >> @@ -409,6 +409,29 @@ parse_flower_rewrite_to_netlink_action(struct ofpbuf >> *buf, >> } >> } >> >> +static void parse_tc_flower_geneve_opts(struct tc_action *action, >> + struct ofpbuf *buf) >> +{ >> + int tun_opt_len = action->encap.data.present.len; >> + size_t geneve_off; >> + int idx = 0; >> + >> + if (!tun_opt_len) { >> + return; >> + } >> + >> + geneve_off = nl_msg_start_nested(buf, OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS); >> + while (tun_opt_len) { >> + struct geneve_opt *opt; >> + >> + opt = &action->encap.data.opts.gnv[idx]; >> + nl_msg_put(buf, opt, sizeof(struct geneve_opt) + opt->length * 4); >> + idx += sizeof(struct geneve_opt) + opt->length * 4; >> + tun_opt_len -= sizeof(struct geneve_opt) + opt->length * 4; >> + } >> + nl_msg_end_nested(buf, geneve_off); >> +} >> + >> static int >> parse_tc_flower_to_match(struct tc_flower *flower, >> struct match *match, >> @@ -580,6 +603,7 @@ parse_tc_flower_to_match(struct tc_flower *flower, >> nl_msg_put_be16(buf, OVS_TUNNEL_KEY_ATTR_TP_DST, >> action->encap.tp_dst); >> >> + parse_tc_flower_geneve_opts(action, buf); >> nl_msg_end_nested(buf, tunnel_offset); >> nl_msg_end_nested(buf, set_offset); >> } >> @@ -786,6 +810,12 @@ parse_put_flow_set_action(struct tc_flower *flower, >> struct tc_action *action, >> action->encap.tp_dst = nl_attr_get_be16(tun_attr); >> } >> break; >> + case OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS: { >> + memcpy(action->encap.data.opts.gnv, nl_attr_get(tun_attr), >> + nl_attr_get_size(tun_attr)); >> + action->encap.data.present.len = nl_attr_get_size(tun_attr); >> + } >> + break; >> } >> } >> >> diff --git a/lib/tc.c b/lib/tc.c >> index bbc382326..fcaf7832e 100644 >> --- a/lib/tc.c >> +++ b/lib/tc.c >> @@ -659,8 +659,72 @@ static const struct nl_policy tunnel_key_policy[] = { >> [TCA_TUNNEL_KEY_ENC_DST_PORT] = { .type = NL_A_U16, .optional = true, }, >> [TCA_TUNNEL_KEY_ENC_TOS] = { .type = NL_A_U8, .optional = true, }, >> [TCA_TUNNEL_KEY_ENC_TTL] = { .type = NL_A_U8, .optional = true, }, >> + [TCA_TUNNEL_KEY_ENC_OPTS] = { .type = NL_A_NESTED, .optional = >> true,}, >> }; >> >> +static void >> +nl_parse_act_geneve_opts(const struct nlattr *in_nlattr, >> + struct tc_action *action) >> +{ >> + struct geneve_opt *opt = NULL; >> + const struct ofpbuf *msg; >> + struct nlattr *nla; >> + struct ofpbuf buf; >> + size_t left; >> + int cnt; >> + >> + nl_attr_get_nested(in_nlattr, &buf); >> + msg = &buf; >> + >> + cnt = 0; >> + NL_ATTR_FOR_EACH (nla, left, ofpbuf_at(msg, 0, 0), msg->size) { >> + uint16_t type = nl_attr_type(nla); >> + >> + switch (type) { >> + case TCA_TUNNEL_KEY_ENC_OPT_GENEVE_CLASS: >> + opt = (struct geneve_opt *) &action->encap.data.opts.gnv[cnt]; > > Clang believe that casting from uint8_t * to unsigned char * > changes the alignment requirements from 1 to 2. > > https://travis-ci.org/horms2/ovs/jobs/422509606
Thanks Simon, I'll fix this up and repost. > >> + opt->opt_class = nl_attr_get_be16(nla); >> + action->encap.data.present.len += sizeof(struct geneve_opt); >> + cnt += sizeof(struct geneve_opt); >> + break; >> + case TCA_TUNNEL_KEY_ENC_OPT_GENEVE_TYPE: >> + opt->type = nl_attr_get_u8(nla); >> + break; >> + case TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA: >> + opt->length = nl_attr_get_size(nla) / 4; >> + memcpy(opt + 1, nl_attr_get_unspec(nla, 1), opt->length * 4); >> + action->encap.data.present.len += opt->length * 4; >> + cnt += opt->length * 4; >> + break; >> + } >> + } >> +} >> + >> +static void >> +nl_parse_act_tunnel_opts(struct nlattr *options, struct tc_action *action) >> +{ >> + const struct ofpbuf *msg; >> + struct nlattr *nla; >> + struct ofpbuf buf; >> + size_t left; >> + >> + if (!options) { >> + return; >> + } >> + >> + nl_attr_get_nested(options, &buf); >> + msg = &buf; >> + >> + NL_ATTR_FOR_EACH (nla, left, ofpbuf_at(msg, 0, 0), msg->size) { >> + uint16_t type = nl_attr_type(nla); >> + switch (type) { >> + case TCA_TUNNEL_KEY_ENC_OPTS_GENEVE: >> + nl_parse_act_geneve_opts(nla, action); >> + break; >> + } >> + } >> +} >> + >> static int >> nl_parse_act_tunnel_key(struct nlattr *options, struct tc_flower *flower) >> { >> @@ -686,6 +750,7 @@ nl_parse_act_tunnel_key(struct nlattr *options, struct >> tc_flower *flower) >> struct nlattr *ipv6_dst = tun_attrs[TCA_TUNNEL_KEY_ENC_IPV6_DST]; >> struct nlattr *tos = tun_attrs[TCA_TUNNEL_KEY_ENC_TOS]; >> struct nlattr *ttl = tun_attrs[TCA_TUNNEL_KEY_ENC_TTL]; >> + struct nlattr *tun_opt = tun_attrs[TCA_TUNNEL_KEY_ENC_OPTS]; >> >> action = &flower->actions[flower->action_count++]; >> action->type = TC_ACT_ENCAP; >> @@ -701,6 +766,7 @@ nl_parse_act_tunnel_key(struct nlattr *options, struct >> tc_flower *flower) >> action->encap.tp_dst = dst_port ? nl_attr_get_be16(dst_port) : 0; >> action->encap.tos = tos ? nl_attr_get_u8(tos) : 0; >> action->encap.ttl = ttl ? nl_attr_get_u8(ttl) : 0; >> + nl_parse_act_tunnel_opts(tun_opt, action); >> } else if (tun->t_action == TCA_TUNNEL_KEY_ACT_RELEASE) { >> flower->tunnel.tunnel = true; >> } else { >> @@ -1268,13 +1334,44 @@ nl_msg_put_act_tunnel_key_release(struct ofpbuf >> *request) >> nl_msg_end_nested(request, offset); >> } >> >> +static void >> +nl_msg_put_act_tunnel_geneve_option(struct ofpbuf *request, >> + uint8_t *tun_options, int tun_opt_len) >> +{ >> + const struct geneve_opt *opt; >> + size_t outer, inner; >> + int idx = 0; >> + >> + if (!tun_opt_len) { >> + return; >> + } >> + >> + outer = nl_msg_start_nested(request, TCA_TUNNEL_KEY_ENC_OPTS); >> + >> + while (tun_opt_len) { >> + opt = (struct geneve_opt *) &tun_options[idx]; >> + inner = nl_msg_start_nested(request, >> TCA_TUNNEL_KEY_ENC_OPTS_GENEVE); >> + nl_msg_put_be16(request, TCA_TUNNEL_KEY_ENC_OPT_GENEVE_CLASS, >> + opt->opt_class); >> + nl_msg_put_u8(request, TCA_TUNNEL_KEY_ENC_OPT_GENEVE_TYPE, >> opt->type); >> + nl_msg_put_unspec(request, TCA_TUNNEL_KEY_ENC_OPT_GENEVE_DATA, opt >> + 1, >> + opt->length * 4); >> + nl_msg_end_nested(request, inner); >> + >> + idx += sizeof(struct geneve_opt) + opt->length * 4; >> + tun_opt_len -= sizeof(struct geneve_opt) + opt->length * 4; >> + } >> + >> + nl_msg_end_nested(request, outer); >> +} >> + >> static void >> nl_msg_put_act_tunnel_key_set(struct ofpbuf *request, ovs_be64 id, >> ovs_be32 ipv4_src, ovs_be32 ipv4_dst, >> struct in6_addr *ipv6_src, >> struct in6_addr *ipv6_dst, >> - ovs_be16 tp_dst, >> - uint8_t tos, uint8_t ttl) >> + ovs_be16 tp_dst, uint8_t tos, uint8_t ttl, >> + void *tun_options, int tun_opt_len) >> { >> size_t offset; >> >> @@ -1304,6 +1401,7 @@ nl_msg_put_act_tunnel_key_set(struct ofpbuf *request, >> ovs_be64 id, >> nl_msg_put_u8(request, TCA_TUNNEL_KEY_ENC_TTL, ttl); >> } >> nl_msg_put_be16(request, TCA_TUNNEL_KEY_ENC_DST_PORT, tp_dst); >> + nl_msg_put_act_tunnel_geneve_option(request, tun_options, >> tun_opt_len); >> } >> nl_msg_end_nested(request, offset); >> } >> @@ -1546,7 +1644,9 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct >> tc_flower *flower) >> &action->encap.ipv6.ipv6_dst, >> action->encap.tp_dst, >> action->encap.tos, >> - action->encap.ttl); >> + action->encap.ttl, >> + action->encap.data.opts.gnv, >> + >> action->encap.data.present.len); >> nl_msg_end_nested(request, act_offset); >> } >> break; >> diff --git a/lib/tc.h b/lib/tc.h >> index aa8805df2..4ae06eb27 100644 >> --- a/lib/tc.h >> +++ b/lib/tc.h >> @@ -27,6 +27,7 @@ >> #include "odp-netlink.h" >> #include "openvswitch/ofpbuf.h" >> #include "openvswitch/flow.h" >> +#include "openvswitch/tun-metadata.h" >> >> /* For backwards compatability with older kernels */ >> #ifndef TC_H_CLSACT >> @@ -140,6 +141,7 @@ struct tc_action { >> struct in6_addr ipv6_src; >> struct in6_addr ipv6_dst; >> } ipv6; >> + struct tun_metadata data; >> } encap; >> }; >> >> -- >> 2.17.0 >> >> _______________________________________________ >> dev mailing list >> d...@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev