From: Ethan Jackson Date: Tue, 19 Jul 2011 22:47:02 +0000 (-0700) Subject: nx-match: New helpers. X-Git-Tag: v1.2.0~56 X-Git-Url: http://git.onelab.eu/?a=commitdiff_plain;h=43edca5717cf389eb87850abbf30efc738049921;p=sliver-openvswitch.git nx-match: New helpers. This patch creates two new helper functions, nxm_reg_load() and nxm_dst_check(). The new nxm_dst_check() function may be used to check the validity of destination fields used by actions. The new nxm_reg_load() function may be used by actions which need to write to NXM fields. This patch also allows multipath and autopath to write their result to non-register NXM fields. --- diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h index 960b53fe3..09f4ee392 100644 --- a/include/openflow/nicira-ext.h +++ b/include/openflow/nicira-ext.h @@ -540,8 +540,7 @@ OFP_ASSERT(sizeof(struct nx_action_note) == 16); * * 3. Stores 'link' in dst[ofs:ofs+n_bits]. The format and semantics of * 'dst' and 'ofs_nbits' are similar to those for the NXAST_REG_LOAD - * action, except that 'dst' must be NXM_NX_REG(idx) for 'idx' in the - * switch's supported range. + * action. * * The switch will reject actions that have an unknown 'fields', or an unknown * 'algorithm', or in which ofs+n_bits is greater than the width of 'dst', or @@ -567,7 +566,7 @@ struct nx_action_multipath { /* Where to store the result. */ ovs_be16 ofs_nbits; /* (ofs << 6) | (n_bits - 1). */ - ovs_be32 dst; /* Destination register. */ + ovs_be32 dst; /* Destination. */ }; OFP_ASSERT(sizeof(struct nx_action_multipath) == 32); @@ -645,8 +644,7 @@ enum nx_mp_algorithm { * 3. Stores 'port' in dst[ofs:ofs+n_bits]. * * The format and semantics of 'dst' and 'ofs_nbits' are similar to those - * for the NXAST_REG_LOAD action, except that 'dst' must be - * NXM_NX_REG(idx) for 'idx' in the switch's supported range. + * for the NXAST_REG_LOAD action. * * The switch will reject actions in which ofs+n_bits is greater than the width * of 'dst', with error type OFPET_BAD_ACTION, code OFPBAC_BAD_ARGUMENT. @@ -659,7 +657,7 @@ struct nx_action_autopath { /* Where to store the result. */ ovs_be16 ofs_nbits; /* (ofs << 6) | (n_bits - 1). */ - ovs_be32 dst; /* Destination register. */ + ovs_be32 dst; /* Destination. */ ovs_be32 id; /* Autopath ID. */ ovs_be32 pad; diff --git a/lib/autopath.c b/lib/autopath.c index 889b037bf..22c7c1109 100644 --- a/lib/autopath.c +++ b/lib/autopath.c @@ -29,19 +29,13 @@ VLOG_DEFINE_THIS_MODULE(autopath); -static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); - /* Loads 'ofp_port' into the appropriate register in accordance with the * autopath action. */ void autopath_execute(const struct nx_action_autopath *ap, struct flow *flow, uint16_t ofp_port) { - uint32_t *reg = &flow->regs[NXM_NX_REG_IDX(ntohl(ap->dst))]; - int ofs = nxm_decode_ofs(ap->ofs_nbits); - int n_bits = nxm_decode_n_bits(ap->ofs_nbits); - uint32_t mask = n_bits == 32 ? UINT32_MAX : (UINT32_C(1) << n_bits) - 1; - *reg = (*reg & ~(mask << ofs)) | ((ofp_port & mask) << ofs); + nxm_reg_load(ap->dst, ap->ofs_nbits, ofp_port, flow); } void @@ -68,10 +62,6 @@ autopath_parse(struct nx_action_autopath *ap, const char *s_) } nxm_parse_field_bits(dst, ®, &ofs, &n_bits); - if (!NXM_IS_NX_REG(reg) || NXM_NX_REG_IDX(reg) >= FLOW_N_REGS) { - ovs_fatal(0, "%s: destination field must be a register", s_); - } - if (n_bits < 16) { ovs_fatal(0, "%s: %d-bit destination field has %u possible values, " "less than required 65536", s_, n_bits, 1u << n_bits); @@ -90,21 +80,7 @@ autopath_parse(struct nx_action_autopath *ap, const char *s_) } int -autopath_check(const struct nx_action_autopath *ap) +autopath_check(const struct nx_action_autopath *ap, const struct flow *flow) { - uint32_t dst = ntohl(ap->dst); - int ofs = nxm_decode_ofs(ap->ofs_nbits); - int n_bits = nxm_decode_n_bits(ap->ofs_nbits); - - if (!NXM_IS_NX_REG(dst) || NXM_NX_REG_IDX(dst) >= FLOW_N_REGS) { - VLOG_WARN_RL(&rl, "unsupported destination field %#"PRIx32, dst); - } else if (ofs + n_bits > nxm_field_bits(dst)) { - VLOG_WARN_RL(&rl, "destination overflows output field"); - } else if (n_bits < 16) { - VLOG_WARN_RL(&rl, "minimum of 16 bits required in output field"); - } else { - return 0; - } - - return ofp_mkerr(OFPET_BAD_ACTION, OFPBAC_BAD_ARGUMENT); + return nxm_dst_check(ap->dst, ap->ofs_nbits, 16, flow); } diff --git a/lib/autopath.h b/lib/autopath.h index ba55f90d3..98b02b41c 100644 --- a/lib/autopath.h +++ b/lib/autopath.h @@ -29,6 +29,6 @@ struct nx_action_autopath; void autopath_execute(const struct nx_action_autopath *, struct flow *, uint16_t ofp_port); void autopath_parse(struct nx_action_autopath *, const char *); -int autopath_check(const struct nx_action_autopath *); +int autopath_check(const struct nx_action_autopath *, const struct flow *); #endif /* autopath.h */ diff --git a/lib/multipath.c b/lib/multipath.c index 4ee6fd025..96845563b 100644 --- a/lib/multipath.c +++ b/lib/multipath.c @@ -34,11 +34,16 @@ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); /* multipath_check(). */ int -multipath_check(const struct nx_action_multipath *mp) +multipath_check(const struct nx_action_multipath *mp, const struct flow *flow) { - uint32_t dst = ntohl(mp->dst); - int ofs = nxm_decode_ofs(mp->ofs_nbits); - int n_bits = nxm_decode_n_bits(mp->ofs_nbits); + uint32_t n_links = ntohs(mp->max_link) + 1; + size_t min_n_bits = log_2_floor(n_links) + 1; + int error; + + error = nxm_dst_check(mp->dst, mp->ofs_nbits, min_n_bits, flow); + if (error) { + return error; + } if (!flow_hash_fields_valid(ntohs(mp->fields))) { VLOG_WARN_RL(&rl, "unsupported fields %"PRIu16, ntohs(mp->fields)); @@ -48,12 +53,6 @@ multipath_check(const struct nx_action_multipath *mp) && mp->algorithm != htons(NX_MP_ALG_ITER_HASH)) { VLOG_WARN_RL(&rl, "unsupported algorithm %"PRIu16, ntohs(mp->algorithm)); - } else if (!NXM_IS_NX_REG(dst) || NXM_NX_REG_IDX(dst) >= FLOW_N_REGS) { - VLOG_WARN_RL(&rl, "unsupported destination field %#"PRIx32, dst); - } else if (ofs + n_bits > nxm_field_bits(dst)) { - VLOG_WARN_RL(&rl, "destination overflows output field"); - } else if (n_bits < 16 && ntohs(mp->max_link) > (1u << n_bits)) { - VLOG_WARN_RL(&rl, "max_link overflows output field"); } else { return 0; } @@ -76,12 +75,7 @@ multipath_execute(const struct nx_action_multipath *mp, struct flow *flow) ntohs(mp->max_link) + 1, ntohl(mp->arg)); - /* Store it. */ - uint32_t *reg = &flow->regs[NXM_NX_REG_IDX(ntohl(mp->dst))]; - int ofs = nxm_decode_ofs(mp->ofs_nbits); - int n_bits = nxm_decode_n_bits(mp->ofs_nbits); - uint32_t mask = n_bits == 32 ? UINT32_MAX : (UINT32_C(1) << n_bits) - 1; - *reg = (*reg & ~(mask << ofs)) | (link << ofs); + nxm_reg_load(mp->dst, mp->ofs_nbits, link, flow); } static uint16_t @@ -214,9 +208,6 @@ multipath_parse(struct nx_action_multipath *mp, const char *s_) mp->arg = htonl(atoi(arg)); nxm_parse_field_bits(dst, &header, &ofs, &n_bits); - if (!NXM_IS_NX_REG(header) || NXM_NX_REG_IDX(header) >= FLOW_N_REGS) { - ovs_fatal(0, "%s: destination field must be register", s_); - } if (n_bits < 16 && n_links > (1u << n_bits)) { ovs_fatal(0, "%s: %d-bit destination field has %u possible values, " "less than specified n_links %d", diff --git a/lib/multipath.h b/lib/multipath.h index 962602b62..8ac4bfd35 100644 --- a/lib/multipath.h +++ b/lib/multipath.h @@ -29,7 +29,7 @@ struct nx_action_reg_move; * See include/openflow/nicira-ext.h for NXAST_MULTIPATH specification. */ -int multipath_check(const struct nx_action_multipath *); +int multipath_check(const struct nx_action_multipath *, const struct flow *); void multipath_execute(const struct nx_action_multipath *, struct flow *); void multipath_parse(struct nx_action_multipath *, const char *); diff --git a/lib/nx-match.c b/lib/nx-match.c index 3712781d7..6c48d0247 100644 --- a/lib/nx-match.c +++ b/lib/nx-match.c @@ -1211,30 +1211,51 @@ nxm_check_reg_move(const struct nx_action_reg_move *action, return 0; } +/* Given a flow, checks that the destination field represented by 'dst_header' + * and 'ofs_nbits' is valid and large enough for 'min_n_bits' bits of data. */ int -nxm_check_reg_load(const struct nx_action_reg_load *action, - const struct flow *flow) +nxm_dst_check(ovs_be32 dst_header, ovs_be16 ofs_nbits, size_t min_n_bits, + const struct flow *flow) { const struct nxm_field *dst; int ofs, n_bits; - ofs = nxm_decode_ofs(action->ofs_nbits); - n_bits = nxm_decode_n_bits(action->ofs_nbits); - dst = nxm_field_lookup(ntohl(action->dst)); + ofs = nxm_decode_ofs(ofs_nbits); + n_bits = nxm_decode_n_bits(ofs_nbits); + dst = nxm_field_lookup(ntohl(dst_header)); + if (!field_ok(dst, flow, ofs + n_bits)) { - return BAD_ARGUMENT; + VLOG_WARN_RL(&rl, "invalid destination field"); + } else if (!dst->writable) { + VLOG_WARN_RL(&rl, "destination field is not writable"); + } else if (n_bits < min_n_bits) { + VLOG_WARN_RL(&rl, "insufficient bits in destination"); + } else { + return 0; + } + + return BAD_ARGUMENT; +} + +int +nxm_check_reg_load(const struct nx_action_reg_load *action, + const struct flow *flow) +{ + int n_bits; + int error; + + error = nxm_dst_check(action->dst, action->ofs_nbits, 0, flow); + if (error) { + return error; } /* Reject 'action' if a bit numbered 'n_bits' or higher is set to 1 in * action->value. */ + n_bits = nxm_decode_n_bits(action->ofs_nbits); if (n_bits < 64 && ntohll(action->value) >> n_bits) { return BAD_ARGUMENT; } - if (!dst->writable) { - return BAD_ARGUMENT; - } - return 0; } @@ -1427,31 +1448,30 @@ nxm_execute_reg_move(const struct nx_action_reg_move *action, int src_ofs = ntohs(action->src_ofs); uint64_t src_data = nxm_read_field(src, flow) & (mask << src_ofs); - /* Get the remaining bits of the destination field. */ - const struct nxm_field *dst = nxm_field_lookup(ntohl(action->dst)); - int dst_ofs = ntohs(action->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 >> src_ofs) << dst_ofs); - - nxm_write_field(dst, flow, new_data); + nxm_reg_load(action->dst, + nxm_encode_ofs_nbits(ntohs(action->dst_ofs), n_bits), + src_data, flow); } void nxm_execute_reg_load(const struct nx_action_reg_load *action, struct flow *flow) { - /* Preparation. */ - int n_bits = nxm_decode_n_bits(action->ofs_nbits); - uint64_t mask = n_bits == 64 ? UINT64_MAX : (UINT64_C(1) << n_bits) - 1; + nxm_reg_load(action->dst, action->ofs_nbits, ntohll(action->value), flow); +} - /* Get source data. */ - uint64_t src_data = ntohll(action->value); +/* Calculates ofs and n_bits from the given 'ofs_nbits' parameter, and copies + * 'src_data'[0:n_bits] to 'dst_header'[ofs:ofs+n_bits] in the given 'flow'. */ +void +nxm_reg_load(ovs_be32 dst_header, ovs_be16 ofs_nbits, uint64_t src_data, + struct flow *flow) +{ + int n_bits = nxm_decode_n_bits(ofs_nbits); + int dst_ofs = nxm_decode_ofs(ofs_nbits); + uint64_t mask = n_bits == 64 ? UINT64_MAX : (UINT64_C(1) << n_bits) - 1; /* 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); + const struct nxm_field *dst = nxm_field_lookup(ntohl(dst_header)); uint64_t dst_data = nxm_read_field(dst, flow) & ~(mask << dst_ofs); /* Get the final value. */ diff --git a/lib/nx-match.h b/lib/nx-match.h index b2260b1f0..5365ccafa 100644 --- a/lib/nx-match.h +++ b/lib/nx-match.h @@ -49,9 +49,13 @@ void nxm_format_reg_load(const struct nx_action_reg_load *, struct ds *); int nxm_check_reg_move(const struct nx_action_reg_move *, const struct flow *); int nxm_check_reg_load(const struct nx_action_reg_load *, const struct flow *); +int nxm_dst_check(ovs_be32 dst, ovs_be16 ofs_nbits, size_t min_n_bits, + const struct flow *); void nxm_execute_reg_move(const struct nx_action_reg_move *, struct flow *); void nxm_execute_reg_load(const struct nx_action_reg_load *, struct flow *); +void nxm_reg_load(ovs_be32 dst, ovs_be16 ofs_nbits, uint64_t src_data, + struct flow *); int nxm_field_bytes(uint32_t header); int nxm_field_bits(uint32_t header); diff --git a/lib/ofp-util.c b/lib/ofp-util.c index 803b0333d..2e62b9188 100644 --- a/lib/ofp-util.c +++ b/lib/ofp-util.c @@ -2029,11 +2029,13 @@ validate_actions(const union ofp_action *actions, size_t n_actions, break; case OFPUTIL_NXAST_MULTIPATH: - error = multipath_check((const struct nx_action_multipath *) a); + error = multipath_check((const struct nx_action_multipath *) a, + flow); break; case OFPUTIL_NXAST_AUTOPATH: - error = autopath_check((const struct nx_action_autopath *) a); + error = autopath_check((const struct nx_action_autopath *) a, + flow); break; case OFPUTIL_NXAST_BUNDLE: diff --git a/tests/autopath.at b/tests/autopath.at index 79ecb02f8..6b837f84b 100644 --- a/tests/autopath.at +++ b/tests/autopath.at @@ -21,12 +21,6 @@ AT_CHECK([ovs-ofctl parse-flow 'actions=autopath(bad, NXM_NX_REG0[[]])'], [1], [ ]) AT_CLEANUP -AT_SETUP([autopath action bad destination]) -AT_CHECK([ovs-ofctl parse-flow 'actions=autopath(1, NXM_OF_VLAN_TCI[[]])'], [1], [], - [ovs-ofctl: 1, NXM_OF_VLAN_TCI[[]]: destination field must be a register -]) -AT_CLEANUP - AT_SETUP([autopath action destination too narrow]) AT_CHECK([ovs-ofctl parse-flow 'actions=autopath(1,NXM_NX_REG0[[0..7]])'], [1], [], [ovs-ofctl: 1,NXM_NX_REG0[[0..7]]: 8-bit destination field has 256 possible values, less than required 65536 diff --git a/tests/multipath.at b/tests/multipath.at index 6eafa9a3f..361c2c47a 100644 --- a/tests/multipath.at +++ b/tests/multipath.at @@ -303,12 +303,6 @@ AT_CHECK([ovs-ofctl parse-flow 'actions=multipath(eth_src,50,modulo_n,0,0,NXM_NX ]) AT_CLEANUP -AT_SETUP([multipath action bad destination]) -AT_CHECK([ovs-ofctl parse-flow 'actions=multipath(eth_src,50,modulo_n,1,0,NXM_OF_VLAN_TCI[[]])'], [1], [], - [ovs-ofctl: eth_src,50,modulo_n,1,0,NXM_OF_VLAN_TCI[[]]: destination field must be register -]) -AT_CLEANUP - AT_SETUP([multipath action destination too narrow]) AT_CHECK([ovs-ofctl parse-flow 'actions=multipath(eth_src,50,modulo_n,1024,0,NXM_NX_REG0[[0..7]])'], [1], [], [ovs-ofctl: eth_src,50,modulo_n,1024,0,NXM_NX_REG0[[0..7]]: 8-bit destination field has 256 possible values, less than specified n_links 1024 diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in index 857230291..b45f01c6d 100644 --- a/utilities/ovs-ofctl.8.in +++ b/utilities/ovs-ofctl.8.in @@ -705,7 +705,7 @@ Hashes \fIfields\fR using \fIbasis\fR as a universal hash parameter, then the applies multipath link selection \fIalgorithm\fR (with parameter \fIarg\fR) to choose one of \fIn_links\fR output links numbered 0 through \fIn_links\fR minus 1, and stores the link into -\fIdst\fB[\fIstart\fB..\fIend\fB]\fR, which must be an NXM register as +\fIdst\fB[\fIstart\fB..\fIend\fB]\fR, which must be an NXM field as described above. .IP Currently, \fIfields\fR must be either \fBeth_src\fR or @@ -717,7 +717,7 @@ Refer to \fBnicira\-ext.h\fR for more details. . .IP "\fBautopath(\fIid\fB, \fIdst\fB[\fIstart\fB..\fIend\fB])\fR" Given \fIid\fR, chooses an OpenFlow port and populates it in -\fIdst\fB[\fIstart\fB..\fIend\fB]\fR, which must be an NXM register as +\fIdst\fB[\fIstart\fB..\fIend\fB]\fR, which must be an NXM field as described above. .IP Currently, \fIid\fR should be the OpenFlow port number of an interface on the