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 960b53fe37d6204f5a8535f74bc598839c882f6f..09f4ee392b46b6898e1dc58a97662d5c4eea4107 100644 (file)
--- 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 889b037bf5984f20424d37c9fd592b6e59fc67f6..22c7c110927dadf3a6696b157ea10779def53e0d 100644 (file)
--- 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, &reg, &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 ba55f90d3ef02860b521c7e3041cd311670f0364..98b02b41c9550b460438368853d24d11be7323a7 100644 (file)
--- 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 4ee6fd025e4062ad664d00212537349a8086916d..96845563b908ea55c03571dd34a9be85c703dfad 100644 (file)
--- a/lib/multipath.c
+++ b/lib/multipath.c
@@ -34,11 +34,16 @@ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
 \f
 /* 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 962602b629c9dbf2cd7b2b50845445b67933cbd0..8ac4bfd350687ba85c24d795cb3ef43f669c2d61 100644 (file)
--- 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 3712781d7649a69ab6d2eea7c35f51301ea8444f..6c48d024706909c73fb232b23b79eac9b792a08c 100644 (file)
--- 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;
 }
 \f
@@ -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 b2260b1f00399955947aa626424fc295184ce4b2..5365ccafa2332ceac9228b4ead0d0882bc521568 100644 (file)
--- 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 803b0333d5dcc30f43497335f4446839736cd692..2e62b9188e50f128d5f3b3a26dec649344fbfa3c 100644 (file)
--- 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 79ecb02f8b9fbf0e44e605b6e0dfc7284b855d9b..6b837f84bfc63ed2be15ffa1595ba7195344dfca 100644 (file)
--- 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 6eafa9a3f750aa7eacb65265e9091922c4c8c400..361c2c47ade0bf240f7b8b213f66a85fb262c701 100644 (file)
--- 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 857230291830ad6292f9ce2db2e01d596e30ff2e..b45f01c6db2d4a362d2b04e99fde9c0f20b27204 100644 (file)
--- 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