[ovs-dev] [load tun_id 2/2] nx-match: Allow NXM_NX_TUN_ID and NXM_OF_VLAN_TCI on NXAST_REG_LOAD.
Justin Pettit
jpettit at nicira.com
Thu Jan 20 00:13:46 UTC 2011
Looks good. Thanks for getting this turned around so quickly!
--Justin
On Jan 19, 2011, at 2:56 PM, Ben Pfaff wrote:
> NXM_NX_TUN_ID and NXM_OF_VLAN_TCI were already allowed on NXAST_REG_MOVE,
> but not on NXAST_REG_LOAD. This makes them valid on both.
>
> Requested-by: Pankaj Thakkar <thakkar at nicira.com>
> ---
> include/openflow/nicira-ext.h | 6 +-
> lib/nx-match.c | 102 +++++++++++++++++++++++++++++++---------
> lib/nx-match.def | 58 ++++++++++++------------
> ofproto/ofproto.c | 33 ++++++++++----
> 4 files changed, 135 insertions(+), 64 deletions(-)
>
> diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
> index 0b01aaa..5013a5a 100644
> --- a/include/openflow/nicira-ext.h
> +++ b/include/openflow/nicira-ext.h
> @@ -430,9 +430,9 @@ OFP_ASSERT(sizeof(struct nx_action_reg_move) == 24);
> * starts at 0 for the least-significant bit, 1 for the next most significant
> * bit, and so on.
> *
> - * 'dst' is an nxm_header with nxm_hasmask=0. It must be one of the following:
> - *
> - * - NXM_NX_REG(idx) for idx in the switch's accepted range.
> + * 'dst' is an nxm_header with nxm_hasmask=0. See the documentation for
> + * NXAST_REG_MOVE, above, for the permitted fields and for the side effects of
> + * loading them.
> *
> * The 'ofs' and 'n_bits' fields are combined into a single 'ofs_nbits' field
> * to avoid enlarging the structure by another 8 bytes. To allow 'n_bits' to
> diff --git a/lib/nx-match.c b/lib/nx-match.c
> index f887cdb..fc3af68 100644
> --- a/lib/nx-match.c
> +++ b/lib/nx-match.c
> @@ -46,7 +46,8 @@ enum {
> /* For each NXM_* field, define NFI_NXM_* as consecutive integers starting from
> * zero. */
> enum nxm_field_index {
> -#define DEFINE_FIELD(HEADER, WILDCARD, DL_TYPE, NW_PROTO) NFI_NXM_##HEADER,
> +#define DEFINE_FIELD(HEADER, WILDCARD, DL_TYPE, NW_PROTO, WRITABLE) \
> + NFI_NXM_##HEADER,
> #include "nx-match.def"
> N_NXM_FIELDS
> };
> @@ -59,13 +60,14 @@ struct nxm_field {
> ovs_be16 dl_type; /* dl_type prerequisite, if nonzero. */
> uint8_t nw_proto; /* nw_proto prerequisite, if nonzero. */
> const char *name; /* "NXM_*" string. */
> + bool writable; /* Writable with NXAST_REG_{MOVE,LOAD}? */
> };
>
> /* All the known fields. */
> static struct nxm_field nxm_fields[N_NXM_FIELDS] = {
> -#define DEFINE_FIELD(HEADER, WILDCARD, DL_TYPE, NW_PROTO) \
> +#define DEFINE_FIELD(HEADER, WILDCARD, DL_TYPE, NW_PROTO, WRITABLE) \
> { HMAP_NODE_NULL_INITIALIZER, NFI_NXM_##HEADER, NXM_##HEADER, WILDCARD, \
> - CONSTANT_HTONS(DL_TYPE), NW_PROTO, "NXM_" #HEADER },
> + CONSTANT_HTONS(DL_TYPE), NW_PROTO, "NXM_" #HEADER, WRITABLE },
> #include "nx-match.def"
> };
>
> @@ -97,7 +99,7 @@ nxm_init(void)
> /* Verify that the header values are unique (duplicate "case" values
> * cause a compile error). */
> switch (0) {
> -#define DEFINE_FIELD(HEADER, WILDCARD, DL_TYPE, NW_PROTO) \
> +#define DEFINE_FIELD(HEADER, WILDCARD, DL_TYPE, NW_PROTO, WRITABLE) \
> case NXM_##HEADER: break;
> #include "nx-match.def"
> }
> @@ -1016,9 +1018,7 @@ nxm_check_reg_move(const struct nx_action_reg_move *action,
> return BAD_ARGUMENT;
> }
>
> - if (!NXM_IS_NX_REG(dst->header)
> - && dst->header != NXM_OF_VLAN_TCI
> - && dst->header != NXM_NX_TUN_ID) {
> + if (!dst->writable) {
> return BAD_ARGUMENT;
> }
>
> @@ -1045,7 +1045,7 @@ nxm_check_reg_load(const struct nx_action_reg_load *action,
> return BAD_ARGUMENT;
> }
>
> - if (!NXM_IS_NX_REG(dst->header)) {
> + if (!dst->writable) {
> return BAD_ARGUMENT;
> }
>
> @@ -1138,6 +1138,68 @@ nxm_read_field(const struct nxm_field *src, const struct flow *flow)
> NOT_REACHED();
> }
>
> +static void
> +nxm_write_field(const struct nxm_field *dst, struct flow *flow,
> + uint64_t new_value)
> +{
> + switch (dst->index) {
> + case NFI_NXM_OF_VLAN_TCI:
> + flow->vlan_tci = htons(new_value);
> + break;
> +
> + case NFI_NXM_NX_TUN_ID:
> + flow->tun_id = htonll(new_value);
> + break;
> +
> +#define NXM_WRITE_REGISTER(IDX) \
> + case NFI_NXM_NX_REG##IDX: \
> + flow->regs[IDX] = new_value; \
> + break; \
> + case NFI_NXM_NX_REG##IDX##_W: \
> + NOT_REACHED();
> +
> + NXM_WRITE_REGISTER(0);
> +#if FLOW_N_REGS >= 2
> + NXM_WRITE_REGISTER(1);
> +#endif
> +#if FLOW_N_REGS >= 3
> + NXM_WRITE_REGISTER(2);
> +#endif
> +#if FLOW_N_REGS >= 4
> + NXM_WRITE_REGISTER(3);
> +#endif
> +#if FLOW_N_REGS > 4
> +#error
> +#endif
> +
> + case NFI_NXM_OF_IN_PORT:
> + case NFI_NXM_OF_ETH_DST:
> + case NFI_NXM_OF_ETH_SRC:
> + case NFI_NXM_OF_ETH_TYPE:
> + case NFI_NXM_OF_IP_TOS:
> + case NFI_NXM_OF_IP_PROTO:
> + case NFI_NXM_OF_ARP_OP:
> + case NFI_NXM_OF_IP_SRC:
> + case NFI_NXM_OF_ARP_SPA:
> + case NFI_NXM_OF_IP_DST:
> + case NFI_NXM_OF_ARP_TPA:
> + case NFI_NXM_OF_TCP_SRC:
> + case NFI_NXM_OF_UDP_SRC:
> + case NFI_NXM_OF_TCP_DST:
> + case NFI_NXM_OF_UDP_DST:
> + case NFI_NXM_OF_ICMP_TYPE:
> + case NFI_NXM_OF_ICMP_CODE:
> + case NFI_NXM_OF_ETH_DST_W:
> + case NFI_NXM_OF_VLAN_TCI_W:
> + case NFI_NXM_OF_IP_SRC_W:
> + case NFI_NXM_OF_IP_DST_W:
> + case NFI_NXM_OF_ARP_SPA_W:
> + case NFI_NXM_OF_ARP_TPA_W:
> + case N_NXM_FIELDS:
> + NOT_REACHED();
> + }
> +}
> +
> void
> nxm_execute_reg_move(const struct nx_action_reg_move *action,
> struct flow *flow)
> @@ -1159,16 +1221,7 @@ nxm_execute_reg_move(const struct nx_action_reg_move *action,
> /* Get the final value. */
> uint64_t new_data = dst_data | ((src_data >> src_ofs) << dst_ofs);
>
> - /* Store the result. */
> - if (NXM_IS_NX_REG(dst->header)) {
> - flow->regs[NXM_NX_REG_IDX(dst->header)] = new_data;
> - } else if (dst->header == NXM_OF_VLAN_TCI) {
> - flow->vlan_tci = htons(new_data);
> - } else if (dst->header == NXM_NX_TUN_ID) {
> - flow->tun_id = htonll(new_data);
> - } else {
> - NOT_REACHED();
> - }
> + nxm_write_field(dst, flow, new_data);
> }
>
> void
> @@ -1177,15 +1230,18 @@ nxm_execute_reg_load(const struct nx_action_reg_load *action,
> {
> /* Preparation. */
> int n_bits = nxm_decode_n_bits(action->ofs_nbits);
> - uint32_t mask = n_bits == 32 ? UINT32_MAX : (UINT32_C(1) << n_bits) - 1;
> - uint32_t *reg = &flow->regs[NXM_NX_REG_IDX(ntohl(action->dst))];
> + uint64_t mask = n_bits == 64 ? UINT64_MAX : (UINT64_C(1) << n_bits) - 1;
>
> /* Get source data. */
> - uint32_t src_data = ntohll(action->value);
> + uint64_t src_data = ntohll(action->value);
>
> /* Get remaining bits of the destination field. */
> + const struct nxm_field *dst = nxm_field_lookup(ntohl(action->dst));
> int dst_ofs = nxm_decode_ofs(action->ofs_nbits);
> - uint32_t dst_data = *reg & ~(mask << dst_ofs);
> + uint64_t dst_data = nxm_read_field(dst, flow) & ~(mask << dst_ofs);
> +
> + /* Get the final value. */
> + uint64_t new_data = dst_data | (src_data << dst_ofs);
>
> - *reg = dst_data | (src_data << dst_ofs);
> + nxm_write_field(dst, flow, new_data);
> }
> diff --git a/lib/nx-match.def b/lib/nx-match.def
> index e045d0f..9c113eb 100644
> --- a/lib/nx-match.def
> +++ b/lib/nx-match.def
> @@ -1,5 +1,5 @@
> /* -*- c -*-
> - * Copyright (c) 2008, 2009, 2010 Nicira Networks.
> + * Copyright (c) 2008, 2009, 2010, 2011 Nicira Networks.
> *
> * Licensed under the Apache License, Version 2.0 (the "License");
> * you may not use this file except in compliance with the License.
> @@ -14,41 +14,41 @@
> * limitations under the License.
> */
>
> -#define DEFINE_FIELD_M(HEADER, WILDCARD, DL_TYPE, NW_PROTO) \
> - DEFINE_FIELD(HEADER, WILDCARD, DL_TYPE, NW_PROTO) \
> - DEFINE_FIELD(HEADER##_W, WILDCARD, DL_TYPE, NW_PROTO)
> +#define DEFINE_FIELD_M(HEADER, WILDCARD, DL_TYPE, NW_PROTO, WRITABLE) \
> + DEFINE_FIELD(HEADER, WILDCARD, DL_TYPE, NW_PROTO, WRITABLE) \
> + DEFINE_FIELD(HEADER##_W, WILDCARD, DL_TYPE, NW_PROTO, false)
>
> -/* NXM_ suffix FWW_* bit dl_type nw_proto */
> -/* ------------ ------------ ----------- ------------- */
> -DEFINE_FIELD (OF_IN_PORT, FWW_IN_PORT, 0, 0)
> -DEFINE_FIELD_M(OF_ETH_DST, 0, 0, 0)
> -DEFINE_FIELD (OF_ETH_SRC, FWW_DL_SRC, 0, 0)
> -DEFINE_FIELD (OF_ETH_TYPE, FWW_DL_TYPE, 0, 0)
> -DEFINE_FIELD_M(OF_VLAN_TCI, 0, 0, 0)
> -DEFINE_FIELD (OF_IP_TOS, FWW_NW_TOS, ETH_TYPE_IP, 0)
> -DEFINE_FIELD (OF_IP_PROTO, FWW_NW_PROTO, ETH_TYPE_IP, 0)
> -DEFINE_FIELD_M(OF_IP_SRC, 0, ETH_TYPE_IP, 0)
> -DEFINE_FIELD_M(OF_IP_DST, 0, ETH_TYPE_IP, 0)
> -DEFINE_FIELD (OF_TCP_SRC, FWW_TP_SRC, ETH_TYPE_IP, IP_TYPE_TCP)
> -DEFINE_FIELD (OF_TCP_DST, FWW_TP_DST, ETH_TYPE_IP, IP_TYPE_TCP)
> -DEFINE_FIELD (OF_UDP_SRC, FWW_TP_SRC, ETH_TYPE_IP, IP_TYPE_UDP)
> -DEFINE_FIELD (OF_UDP_DST, FWW_TP_DST, ETH_TYPE_IP, IP_TYPE_UDP)
> -DEFINE_FIELD (OF_ICMP_TYPE, FWW_TP_SRC, ETH_TYPE_IP, IP_TYPE_ICMP)
> -DEFINE_FIELD (OF_ICMP_CODE, FWW_TP_DST, ETH_TYPE_IP, IP_TYPE_ICMP)
> -DEFINE_FIELD (OF_ARP_OP, FWW_NW_PROTO, ETH_TYPE_ARP, 0)
> -DEFINE_FIELD_M(OF_ARP_SPA, 0, ETH_TYPE_ARP, 0)
> -DEFINE_FIELD_M(OF_ARP_TPA, 0, ETH_TYPE_ARP, 0)
> -DEFINE_FIELD (NX_TUN_ID, FWW_TUN_ID, 0, 0)
> +/* NXM_ suffix FWW_* bit dl_type nw_proto rw? */
> +/* ------------ ------------ ----------- ------------- --- */
> +DEFINE_FIELD (OF_IN_PORT, FWW_IN_PORT, 0, 0, false)
> +DEFINE_FIELD_M(OF_ETH_DST, 0, 0, 0, false)
> +DEFINE_FIELD (OF_ETH_SRC, FWW_DL_SRC, 0, 0, false)
> +DEFINE_FIELD (OF_ETH_TYPE, FWW_DL_TYPE, 0, 0, false)
> +DEFINE_FIELD_M(OF_VLAN_TCI, 0, 0, 0, true)
> +DEFINE_FIELD (OF_IP_TOS, FWW_NW_TOS, ETH_TYPE_IP, 0, false)
> +DEFINE_FIELD (OF_IP_PROTO, FWW_NW_PROTO, ETH_TYPE_IP, 0, false)
> +DEFINE_FIELD_M(OF_IP_SRC, 0, ETH_TYPE_IP, 0, false)
> +DEFINE_FIELD_M(OF_IP_DST, 0, ETH_TYPE_IP, 0, false)
> +DEFINE_FIELD (OF_TCP_SRC, FWW_TP_SRC, ETH_TYPE_IP, IP_TYPE_TCP, false)
> +DEFINE_FIELD (OF_TCP_DST, FWW_TP_DST, ETH_TYPE_IP, IP_TYPE_TCP, false)
> +DEFINE_FIELD (OF_UDP_SRC, FWW_TP_SRC, ETH_TYPE_IP, IP_TYPE_UDP, false)
> +DEFINE_FIELD (OF_UDP_DST, FWW_TP_DST, ETH_TYPE_IP, IP_TYPE_UDP, false)
> +DEFINE_FIELD (OF_ICMP_TYPE, FWW_TP_SRC, ETH_TYPE_IP, IP_TYPE_ICMP, false)
> +DEFINE_FIELD (OF_ICMP_CODE, FWW_TP_DST, ETH_TYPE_IP, IP_TYPE_ICMP, false)
> +DEFINE_FIELD (OF_ARP_OP, FWW_NW_PROTO, ETH_TYPE_ARP, 0, false)
> +DEFINE_FIELD_M(OF_ARP_SPA, 0, ETH_TYPE_ARP, 0, false)
> +DEFINE_FIELD_M(OF_ARP_TPA, 0, ETH_TYPE_ARP, 0, false)
> +DEFINE_FIELD (NX_TUN_ID, FWW_TUN_ID, 0, 0, true)
>
> -DEFINE_FIELD_M(NX_REG0, 0, 0, 0)
> +DEFINE_FIELD_M(NX_REG0, 0, 0, 0, true)
> #if FLOW_N_REGS >= 2
> -DEFINE_FIELD_M(NX_REG1, 0, 0, 0)
> +DEFINE_FIELD_M(NX_REG1, 0, 0, 0, true)
> #endif
> #if FLOW_N_REGS >= 3
> -DEFINE_FIELD_M(NX_REG2, 0, 0, 0)
> +DEFINE_FIELD_M(NX_REG2, 0, 0, 0, true)
> #endif
> #if FLOW_N_REGS >= 4
> -DEFINE_FIELD_M(NX_REG3, 0, 0, 0)
> +DEFINE_FIELD_M(NX_REG3, 0, 0, 0, true)
> #endif
> #if FLOW_N_REGS > 4
> #error
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 3084c09..2eae86d 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -2858,19 +2858,27 @@ xlate_set_dl_tci(struct action_xlate_ctx *ctx)
> }
> }
>
> +struct xlate_reg_state {
> + ovs_be16 vlan_tci;
> + ovs_be64 tun_id;
> +};
> +
> static void
> -xlate_reg_move_action(struct action_xlate_ctx *ctx,
> - const struct nx_action_reg_move *narm)
> +save_reg_state(const struct action_xlate_ctx *ctx,
> + struct xlate_reg_state *state)
> {
> - ovs_be16 old_tci = ctx->flow.vlan_tci;
> - ovs_be64 old_tun = ctx->flow.tun_id;
> -
> - nxm_execute_reg_move(narm, &ctx->flow);
> + state->vlan_tci = ctx->flow.vlan_tci;
> + state->tun_id = ctx->flow.tun_id;
> +}
>
> - if (ctx->flow.vlan_tci != old_tci) {
> +static void
> +update_reg_state(struct action_xlate_ctx *ctx,
> + const struct xlate_reg_state *state)
> +{
> + if (ctx->flow.vlan_tci != state->vlan_tci) {
> xlate_set_dl_tci(ctx);
> }
> - if (ctx->flow.tun_id != old_tun) {
> + if (ctx->flow.tun_id != state->tun_id) {
> nl_msg_put_be64(ctx->odp_actions, ODPAT_SET_TUNNEL, ctx->flow.tun_id);
> }
> }
> @@ -2884,6 +2892,7 @@ xlate_nicira_action(struct action_xlate_ctx *ctx,
> const struct nx_action_set_queue *nasq;
> const struct nx_action_multipath *nam;
> enum nx_action_subtype subtype = ntohs(nah->subtype);
> + struct xlate_reg_state state;
> ovs_be64 tun_id;
>
> assert(nah->vendor == htonl(NX_VENDOR_ID));
> @@ -2916,12 +2925,18 @@ xlate_nicira_action(struct action_xlate_ctx *ctx,
> break;
>
> case NXAST_REG_MOVE:
> - xlate_reg_move_action(ctx, (const struct nx_action_reg_move *) nah);
> + save_reg_state(ctx, &state);
> + nxm_execute_reg_move((const struct nx_action_reg_move *) nah,
> + &ctx->flow);
> + update_reg_state(ctx, &state);
> break;
>
> case NXAST_REG_LOAD:
> + save_reg_state(ctx, &state);
> nxm_execute_reg_load((const struct nx_action_reg_load *) nah,
> &ctx->flow);
> + update_reg_state(ctx, &state);
> + break;
>
> case NXAST_NOTE:
> /* Nothing to do. */
> --
> 1.7.1
>
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
More information about the dev
mailing list