datapath: Convert odp_flow_key to use Netlink attributes instead.

One of the goals for Open vSwitch is to decouple kernel and userspace
software, so that either one can be upgraded or rolled back independent of
the other.  To do this in full generality, it must be possible to change
the kernel's idea of the flow key separately from the userspace version.
In turn, that means that flow keys must become variable-length.  This
commit makes that change using Netlink attribute sequences.

This commit does not actually make userspace flexible enough to handle
changes in the kernel flow key structure, because userspace doesn't yet
have enough information to do that intelligently.  Upcoming commits will
fix that.

Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Jesse Gross <jesse@nicira.com>

10 files changed, 553 insertions, 90 deletions

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 5d7fa206..d945e3b0 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -36,6 +36,7 @@
 #include "dpif.h"
 #include "dpif-provider.h"
 #include "dummy.h"
+#include "dynamic-string.h"
 #include "flow.h"
 #include "hmap.h"
 #include "list.h"
@@ -603,6 +604,32 @@ answer_flow_query(struct dp_netdev_flow *flow, uint32_t query_flags,
 }
 
 static int
+dpif_netdev_flow_from_nlattrs(const struct nlattr *key, uint32_t key_len,
+                              struct flow *flow)
+{
+    if (odp_flow_key_to_flow(key, key_len, flow)) {
+        /* This should not happen: it indicates that odp_flow_key_from_flow()
+         * and odp_flow_key_to_flow() disagree on the acceptable form of a
+         * flow.  Log the problem as an error, with enough details to enable
+         * debugging. */
+        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+
+        if (!VLOG_DROP_ERR(&rl)) {
+            struct ds s;
+
+            ds_init(&s);
+            odp_flow_key_format(key, key_len, &s);
+            VLOG_ERR("internal error parsing flow key %s", ds_cstr(&s));
+            ds_destroy(&s);
+        }
+
+        return EINVAL;
+    }
+
+    return 0;
+}
+
+static int
 dpif_netdev_flow_get(const struct dpif *dpif, struct odp_flow flows[], int n)
 {
     struct dp_netdev *dp = get_dp_netdev(dpif);
@@ -611,8 +638,14 @@ dpif_netdev_flow_get(const struct dpif *dpif, struct odp_flow flows[], int n)
     for (i = 0; i < n; i++) {
         struct odp_flow *odp_flow = &flows[i];
         struct flow key;
+        int error;
+
+        error = dpif_netdev_flow_from_nlattrs(odp_flow->key, odp_flow->key_len,
+                                              &key);
+        if (error) {
+            return error;
+        }
 
-        odp_flow_key_to_flow(&odp_flow->key, &key);
         answer_flow_query(dp_netdev_lookup_flow(dp, &key),
                           odp_flow->flags, odp_flow);
     }
@@ -699,14 +732,14 @@ set_flow_actions(struct dp_netdev_flow *flow, struct odp_flow *odp_flow)
 }
 
 static int
-add_flow(struct dpif *dpif, struct odp_flow *odp_flow)
+add_flow(struct dpif *dpif, const struct flow *key, struct odp_flow *odp_flow)
 {
     struct dp_netdev *dp = get_dp_netdev(dpif);
     struct dp_netdev_flow *flow;
     int error;
 
     flow = xzalloc(sizeof *flow);
-    odp_flow_key_to_flow(&odp_flow->key, &flow->key);
+    flow->key = *key;
 
     error = set_flow_actions(flow, odp_flow);
     if (error) {
@@ -734,13 +767,19 @@ dpif_netdev_flow_put(struct dpif *dpif, struct odp_flow_put *put)
     struct dp_netdev *dp = get_dp_netdev(dpif);
     struct dp_netdev_flow *flow;
     struct flow key;
+    int error;
+
+    error = dpif_netdev_flow_from_nlattrs(put->flow.key, put->flow.key_len,
+                                          &key);
+    if (error) {
+        return error;
+    }
 
-    odp_flow_key_to_flow(&put->flow.key, &key);
     flow = dp_netdev_lookup_flow(dp, &key);
     if (!flow) {
         if (put->flags & ODPPF_CREATE) {
             if (hmap_count(&dp->flow_table) < MAX_FLOWS) {
-                return add_flow(dpif, &put->flow);
+                return add_flow(dpif, &key, &put->flow);
             } else {
                 return EFBIG;
             }
@@ -767,8 +806,14 @@ dpif_netdev_flow_del(struct dpif *dpif, struct odp_flow *odp_flow)
     struct dp_netdev *dp = get_dp_netdev(dpif);
     struct dp_netdev_flow *flow;
     struct flow key;
+    int error;
+
+    error = dpif_netdev_flow_from_nlattrs(odp_flow->key, odp_flow->key_len,
+                                          &key);
+    if (error) {
+        return error;
+    }
 
-    odp_flow_key_to_flow(&odp_flow->key, &key);
     flow = dp_netdev_lookup_flow(dp, &key);
     if (flow) {
         answer_flow_query(flow, 0, odp_flow);
@@ -799,6 +844,7 @@ dpif_netdev_flow_dump_next(const struct dpif *dpif, void *state_,
     struct dp_netdev *dp = get_dp_netdev(dpif);
     struct dp_netdev_flow *flow;
     struct hmap_node *node;
+    struct ofpbuf key;
 
     node = hmap_at_position(&dp->flow_table, &state->bucket, &state->offset);
     if (!node) {
@@ -806,7 +852,12 @@ dpif_netdev_flow_dump_next(const struct dpif *dpif, void *state_,
     }
 
     flow = CONTAINER_OF(node, struct dp_netdev_flow, node);
-    odp_flow_key_from_flow(&odp_flow->key, &flow->key);
+
+    ofpbuf_use_stack(&key, odp_flow->key, odp_flow->key_len);
+    odp_flow_key_from_flow(&key, &flow->key);
+    odp_flow->key_len = key.size;
+    ofpbuf_uninit(&key);
+
     answer_flow_query(flow, 0, odp_flow);
 
     return 0;
diff --git a/lib/dpif.c b/lib/dpif.c
index 1bf6e410..73c92cc0 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -83,7 +83,8 @@ static void log_flow_operation(const struct dpif *, const char *operation,
 static void log_flow_put(struct dpif *, int error,
                          const struct odp_flow_put *);
 static bool should_log_flow_message(int error);
-static void check_rw_odp_flow(struct odp_flow *);
+static void check_rw_flow_actions(struct odp_flow *);
+static void check_rw_flow_key(struct odp_flow *);
 
 static void
 dp_initialize(void)
@@ -679,7 +680,7 @@ dpif_flow_get(const struct dpif *dpif, struct odp_flow *flow)
 
     COVERAGE_INC(dpif_flow_get);
 
-    check_rw_odp_flow(flow);
+    check_rw_flow_actions(flow);
     error = dpif->dpif_class->flow_get(dpif, flow, 1);
     if (!error) {
         error = flow->stats.error;
@@ -732,7 +733,7 @@ dpif_flow_get_multiple(const struct dpif *dpif,
     COVERAGE_ADD(dpif_flow_get, n);
 
     for (i = 0; i < n; i++) {
-        check_rw_odp_flow(&flows[i]);
+        check_rw_flow_actions(&flows[i]);
     }
 
     error = dpif->dpif_class->flow_get(dpif, flows, n);
@@ -782,7 +783,7 @@ dpif_flow_del(struct dpif *dpif, struct odp_flow *flow)
 
     COVERAGE_INC(dpif_flow_del);
 
-    check_rw_odp_flow(flow);
+    check_rw_flow_actions(flow);
     memset(&flow->stats, 0, sizeof flow->stats);
 
     error = dpif->dpif_class->flow_del(dpif, flow);
@@ -824,7 +825,8 @@ dpif_flow_dump_next(struct dpif_flow_dump *dump, struct odp_flow *flow)
 {
     const struct dpif *dpif = dump->dpif;
 
-    check_rw_odp_flow(flow);
+    check_rw_flow_actions(flow);
+    check_rw_flow_key(flow);
 
     if (dump->error) {
         return false;
@@ -834,6 +836,9 @@ dpif_flow_dump_next(struct dpif_flow_dump *dump, struct odp_flow *flow)
     if (dump->error == EOF) {
         VLOG_DBG_RL(&dpmsg_rl, "%s: dumped all flows", dpif_name(dpif));
     } else {
+        if (dump->error) {
+            flow->key_len = 0;
+        }
         if (should_log_flow_message(dump->error)) {
             log_flow_operation(dpif, "flow_dump_next", dump->error, flow);
         }
@@ -1117,7 +1122,7 @@ should_log_flow_message(int error)
 
 static void
 log_flow_message(const struct dpif *dpif, int error, const char *operation,
-                 const struct odp_flow_key *flow,
+                 const struct nlattr *key, size_t key_len,
                  const struct odp_flow_stats *stats,
                  const struct nlattr *actions, size_t actions_len)
 {
@@ -1130,7 +1135,7 @@ log_flow_message(const struct dpif *dpif, int error, const char *operation,
     if (error) {
         ds_put_format(&ds, "(%s) ", strerror(error));
     }
-    format_odp_flow_key(&ds, flow);
+    odp_flow_key_format(key, key_len, &ds);
     if (stats) {
         ds_put_cstr(&ds, ", ");
         format_odp_flow_stats(&ds, stats);
@@ -1148,10 +1153,11 @@ log_flow_operation(const struct dpif *dpif, const char *operation, int error,
                    struct odp_flow *flow)
 {
     if (error) {
+        flow->key_len = 0;
         flow->actions_len = 0;
     }
-    log_flow_message(dpif, error, operation, &flow->key,
-                     !error ? &flow->stats : NULL,
+    log_flow_message(dpif, error, operation,
+                     flow->key, flow->key_len, !error ? &flow->stats : NULL,
                      flow->actions, flow->actions_len);
 }
 
@@ -1175,7 +1181,8 @@ log_flow_put(struct dpif *dpif, int error, const struct odp_flow_put *put)
     if (put->flags & ~ODPPF_ALL) {
         ds_put_format(&s, "[%x]", put->flags & ~ODPPF_ALL);
     }
-    log_flow_message(dpif, error, ds_cstr(&s), &put->flow.key,
+    log_flow_message(dpif, error, ds_cstr(&s),
+                     put->flow.key, put->flow.key_len,
                      !error ? &put->flow.stats : NULL,
                      put->flow.actions, put->flow.actions_len);
     ds_destroy(&s);
@@ -1198,9 +1205,18 @@ log_flow_put(struct dpif *dpif, int error, const struct odp_flow_put *put)
  *        "actions" or "actions_len" was not initialized.
  */
 static void
-check_rw_odp_flow(struct odp_flow *flow)
+check_rw_flow_actions(struct odp_flow *flow)
 {
     if (flow->actions_len) {
         memset(&flow->actions[0], 0xcc, sizeof flow->actions[0]);
     }
 }
+
+/* Same as check_rw_flow_actions() but for flow->key. */
+static void
+check_rw_flow_key(struct odp_flow *flow)
+{
+    if (flow->key_len) {
+        memset(&flow->key[0], 0xcc, sizeof flow->key[0]);
+    }
+}
diff --git a/lib/flow.c b/lib/flow.c
index 42f850fb..38dee7b0 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -101,12 +101,12 @@ parse_ethertype(struct ofpbuf *b)
     ovs_be16 proto;
 
     proto = *(ovs_be16 *) ofpbuf_pull(b, sizeof proto);
-    if (ntohs(proto) >= ODP_DL_TYPE_ETH2_CUTOFF) {
+    if (ntohs(proto) >= ETH_TYPE_MIN) {
         return proto;
     }
 
     if (b->size < sizeof *llc) {
-        return htons(ODP_DL_TYPE_NOT_ETH_TYPE);
+        return htons(FLOW_DL_TYPE_NONE);
     }
 
     llc = b->data;
@@ -115,7 +115,7 @@ parse_ethertype(struct ofpbuf *b)
         || llc->llc.llc_cntl != LLC_CNTL_SNAP
         || memcmp(llc->snap.snap_org, SNAP_ORG_ETHERNET,
                   sizeof llc->snap.snap_org)) {
-        return htons(ODP_DL_TYPE_NOT_ETH_TYPE);
+        return htons(FLOW_DL_TYPE_NONE);
     }
 
     ofpbuf_pull(b, sizeof *llc);
diff --git a/lib/flow.h b/lib/flow.h
index 966f1d68..06da995a 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -35,6 +35,10 @@ struct ofpbuf;
 #define FLOW_N_REGS 4
 BUILD_ASSERT_DECL(FLOW_N_REGS <= NXM_NX_MAX_REGS);
 
+/* Used for struct flow's dl_type member for frames that have no Ethernet
+ * type, that is, pure 802.2 frames. */
+#define FLOW_DL_TYPE_NONE 0x5ff
+
 struct flow {
     ovs_be64 tun_id;            /* Encapsulating tunnel ID. */
     uint32_t regs[FLOW_N_REGS]; /* Registers. */
diff --git a/lib/nx-match.c b/lib/nx-match.c
index 392ea869..fcc1081e 100644
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -211,7 +211,7 @@ parse_nxm_entry(struct cls_rule *rule, const struct nxm_field *f,
         memcpy(flow->dl_src, value, ETH_ADDR_LEN);
         return 0;
     case NFI_NXM_OF_ETH_TYPE:
-        flow->dl_type = get_unaligned_be16(value);
+        flow->dl_type = ofputil_dl_type_from_openflow(get_unaligned_be16(value));
         return 0;
 
         /* 802.1Q header. */
@@ -636,7 +636,8 @@ nx_put_match(struct ofpbuf *b, const struct cls_rule *cr)
         nxm_put_eth(b, NXM_OF_ETH_SRC, flow->dl_src);
     }
     if (!(wc & FWW_DL_TYPE)) {
-        nxm_put_16(b, NXM_OF_ETH_TYPE, flow->dl_type);
+        nxm_put_16(b, NXM_OF_ETH_TYPE,
+                   ofputil_dl_type_to_openflow(flow->dl_type));
     }
 
     /* 802.1Q. */
@@ -1104,7 +1105,7 @@ nxm_read_field(const struct nxm_field *src, const struct flow *flow)
         return eth_addr_to_uint64(flow->dl_src);
 
     case NFI_NXM_OF_ETH_TYPE:
-        return ntohs(flow->dl_type);
+        return ntohs(ofputil_dl_type_to_openflow(flow->dl_type));
 
     case NFI_NXM_OF_VLAN_TCI:
         return ntohs(flow->vlan_tci);
diff --git a/lib/odp-util.c b/lib/odp-util.c
index dc6843fb..193f97be 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -16,6 +16,7 @@
 
 #include <config.h>
 #include "odp-util.h"
+#include <errno.h>
 #include <inttypes.h>
 #include <stdlib.h>
 #include <string.h>
@@ -29,26 +30,6 @@
 #include "timeval.h"
 #include "util.h"
 
-void
-format_odp_flow_key(struct ds *ds, const struct odp_flow_key *key)
-{
-    ds_put_format(ds, "tun_id%#"PRIx64" in_port%d tci(",
-                  ntohll(key->tun_id), key->in_port);
-    if (key->dl_tci) {
-        ds_put_format(ds, "vlan%"PRIu16",pcp%d",
-                      vlan_tci_to_vid(key->dl_tci),
-                      vlan_tci_to_pcp(key->dl_tci));
-    } else {
-        ds_put_char(ds, '0');
-    }
-    ds_put_format(ds, ") mac"ETH_ADDR_FMT"->"ETH_ADDR_FMT" type%04x "
-                  "proto%"PRId8" tos%"PRIu8" ip"IP_FMT"->"IP_FMT" port%d->%d",
-                  ETH_ADDR_ARGS(key->dl_src), ETH_ADDR_ARGS(key->dl_dst),
-                  ntohs(key->dl_type), key->nw_proto, key->nw_tos,
-                  IP_ARGS(&key->nw_src), IP_ARGS(&key->nw_dst),
-                  ntohs(key->tp_src), ntohs(key->tp_dst));
-}
-
 int
 odp_action_len(uint16_t type)
 {
@@ -190,7 +171,7 @@ format_odp_actions(struct ds *ds, const struct nlattr *actions,
             if (left == actions_len) {
                 ds_put_cstr(ds, "<empty>");
             }
-            ds_put_format(ds, " ***%u leftover bytes***", left);
+            ds_put_format(ds, ",***%u leftover bytes***", left);
         }
     } else {
         ds_put_cstr(ds, "drop");
@@ -214,7 +195,7 @@ format_odp_flow_stats(struct ds *ds, const struct odp_flow_stats *s)
 void
 format_odp_flow(struct ds *ds, const struct odp_flow *f)
 {
-    format_odp_flow_key(ds, &f->key);
+    odp_flow_key_format(f->key, f->key_len, ds);
     ds_put_cstr(ds, ", ");
     format_odp_flow_stats(ds, &f->stats);
     ds_put_cstr(ds, ", actions:");
@@ -250,37 +231,420 @@ format_odp_port_type(struct ds *ds, const struct odp_port *p)
     }
 }
 
+/* Returns the correct length of the payload for a flow key attribute of the
+ * specified 'type', or -1 if 'type' is unknown. */
+static int
+odp_flow_key_attr_len(uint16_t type)
+{
+    if (type > ODP_KEY_ATTR_MAX) {
+        return -1;
+    }
+
+    switch ((enum odp_key_type) type) {
+    case ODP_KEY_ATTR_TUN_ID: return 8;
+    case ODP_KEY_ATTR_IN_PORT: return 4;
+    case ODP_KEY_ATTR_ETHERNET: return sizeof(struct odp_key_ethernet);
+    case ODP_KEY_ATTR_8021Q: return sizeof(struct odp_key_8021q);
+    case ODP_KEY_ATTR_ETHERTYPE: return 2;
+    case ODP_KEY_ATTR_IPV4: return sizeof(struct odp_key_ipv4);
+    case ODP_KEY_ATTR_TCP: return sizeof(struct odp_key_tcp);
+    case ODP_KEY_ATTR_UDP: return sizeof(struct odp_key_udp);
+    case ODP_KEY_ATTR_ICMP: return sizeof(struct odp_key_icmp);
+    case ODP_KEY_ATTR_ARP: return sizeof(struct odp_key_arp);
+
+    case ODP_KEY_ATTR_UNSPEC:
+    case __ODP_KEY_ATTR_MAX:
+        return -1;
+    }
+
+    return -1;
+}
+
+
+static void
+format_generic_odp_key(const struct nlattr *a, struct ds *ds)
+{
+    size_t len = nl_attr_get_size(a);
+
+    ds_put_format(ds, "key%"PRId16, nl_attr_type(a));
+    if (len) {
+        const uint8_t *unspec;
+        unsigned int i;
+
+        unspec = nl_attr_get(a);
+        for (i = 0; i < len; i++) {
+            ds_put_char(ds, i ? ' ': '(');
+            ds_put_format(ds, "%02x", unspec[i]);
+        }
+        ds_put_char(ds, ')');
+    }
+}
+
+static void
+format_odp_key_attr(const struct nlattr *a, struct ds *ds)
+{
+    const struct odp_key_ethernet *eth_key;
+    const struct odp_key_8021q *q_key;
+    const struct odp_key_ipv4 *ipv4_key;
+    const struct odp_key_tcp *tcp_key;
+    const struct odp_key_udp *udp_key;
+    const struct odp_key_icmp *icmp_key;
+    const struct odp_key_arp *arp_key;
+
+    if (nl_attr_get_size(a) != odp_flow_key_attr_len(nl_attr_type(a))) {
+        ds_put_format(ds, "bad length %zu, expected %d for: ",
+                      nl_attr_get_size(a),
+                      odp_flow_key_attr_len(nl_attr_type(a)));
+        format_generic_odp_key(a, ds);
+        return;
+    }
+
+    switch (nl_attr_type(a)) {
+    case ODP_KEY_ATTR_TUN_ID:
+        ds_put_format(ds, "tun_id(%#"PRIx64")", nl_attr_get_be64(a));
+        break;
+
+    case ODP_KEY_ATTR_IN_PORT:
+        ds_put_format(ds, "in_port(%"PRIu32")", nl_attr_get_u32(a));
+        break;
+
+    case ODP_KEY_ATTR_ETHERNET:
+        eth_key = nl_attr_get(a);
+        ds_put_format(ds, "eth(src="ETH_ADDR_FMT",dst="ETH_ADDR_FMT")",
+                      ETH_ADDR_ARGS(eth_key->eth_src),
+                      ETH_ADDR_ARGS(eth_key->eth_dst));
+        break;
+
+    case ODP_KEY_ATTR_8021Q:
+        q_key = nl_attr_get(a);
+        ds_put_cstr(ds, "vlan(");
+        if (q_key->q_tpid != htons(ETH_TYPE_VLAN)) {
+            ds_put_format(ds, "tpid=%#"PRIx16",", ntohs(q_key->q_tpid));
+        }
+        ds_put_format(ds, "vid%"PRIu16",pcp%d)",
+                      vlan_tci_to_vid(q_key->q_tci),
+                      vlan_tci_to_pcp(q_key->q_tci));
+        break;
+
+    case ODP_KEY_ATTR_ETHERTYPE:
+        ds_put_format(ds, "eth_type(%#04"PRIx16")",
+                      ntohs(nl_attr_get_be16(a)));
+        break;
+
+    case ODP_KEY_ATTR_IPV4:
+        ipv4_key = nl_attr_get(a);
+        ds_put_format(ds, "ipv4(src="IP_FMT",dst="IP_FMT","
+                      "proto=%"PRId8",tos=%"PRIu8")",
+                      IP_ARGS(&ipv4_key->ipv4_src),
+                      IP_ARGS(&ipv4_key->ipv4_dst),
+                      ipv4_key->ipv4_proto, ipv4_key->ipv4_tos);
+        break;
+
+    case ODP_KEY_ATTR_TCP:
+        tcp_key = nl_attr_get(a);
+        ds_put_format(ds, "tcp(src=%"PRIu16",dst=%"PRIu16")",
+                      ntohs(tcp_key->tcp_src), ntohs(tcp_key->tcp_dst));
+        break;
+
+    case ODP_KEY_ATTR_UDP:
+        udp_key = nl_attr_get(a);
+        ds_put_format(ds, "udp(src=%"PRIu16",dst=%"PRIu16")",
+                      ntohs(udp_key->udp_src), ntohs(udp_key->udp_dst));
+        break;
+
+    case ODP_KEY_ATTR_ICMP:
+        icmp_key = nl_attr_get(a);
+        ds_put_format(ds, "icmp(type=%"PRIu8",code=%"PRIu8")",
+                      icmp_key->icmp_type, icmp_key->icmp_code);
+        break;
+
+    case ODP_KEY_ATTR_ARP:
+        arp_key = nl_attr_get(a);
+        ds_put_format(ds, "arp(sip="IP_FMT",tip="IP_FMT",op=%"PRIu16")",
+                      IP_ARGS(&arp_key->arp_sip), IP_ARGS(&arp_key->arp_tip),
+                      ntohs(arp_key->arp_op));
+        break;
+
+    default:
+        format_generic_odp_key(a, ds);
+        break;
+    }
+}
+
+/* Appends to 'ds' a string representation of the 'key_len' bytes of
+ * ODP_KEY_ATTR_* attributes in 'key'. */
 void
-odp_flow_key_from_flow(struct odp_flow_key *key, const struct flow *flow)
+odp_flow_key_format(const struct nlattr *key, size_t key_len, struct ds *ds)
 {
-    key->tun_id = flow->tun_id;
-    key->nw_src = flow->nw_src;
-    key->nw_dst = flow->nw_dst;
-    key->in_port = flow->in_port;
-    key->dl_tci = flow->vlan_tci;
-    key->dl_type = flow->dl_type;
-    key->tp_src = flow->tp_src;
-    key->tp_dst = flow->tp_dst;
-    memcpy(key->dl_src, flow->dl_src, ETH_ADDR_LEN);
-    memcpy(key->dl_dst, flow->dl_dst, ETH_ADDR_LEN);
-    key->nw_proto = flow->nw_proto;
-    key->nw_tos = flow->nw_tos;
+    if (key_len) {
+        const struct nlattr *a;
+        unsigned int left;
+
+        NL_ATTR_FOR_EACH (a, left, key, key_len) {
+            if (a != key) {
+                ds_put_char(ds, ',');
+            }
+            format_odp_key_attr(a, ds);
+        }
+        if (left) {
+            if (left == key_len) {
+                ds_put_cstr(ds, "<empty>");
+            }
+            ds_put_format(ds, ",***%u leftover bytes***", left);
+        }
+    } else {
+        ds_put_cstr(ds, "<empty>");
+    }
 }
 
+/* Appends a representation of 'flow' as ODP_KEY_ATTR_* attributes to 'buf'. */
 void
-odp_flow_key_to_flow(const struct odp_flow_key *key, struct flow *flow)
+odp_flow_key_from_flow(struct ofpbuf *buf, const struct flow *flow)
 {
-    memset(flow->regs, 0, sizeof flow->regs);
-    flow->tun_id = key->tun_id;
-    flow->nw_src = key->nw_src;
-    flow->nw_dst = key->nw_dst;
-    flow->in_port = key->in_port;
-    flow->vlan_tci = key->dl_tci;
-    flow->dl_type = key->dl_type;
-    flow->tp_src = key->tp_src;
-    flow->tp_dst = key->tp_dst;
-    memcpy(flow->dl_src, key->dl_src, ETH_ADDR_LEN);
-    memcpy(flow->dl_dst, key->dl_dst, ETH_ADDR_LEN);
-    flow->nw_proto = key->nw_proto;
-    flow->nw_tos = key->nw_tos;
+    struct odp_key_ethernet *eth_key;
+
+    if (flow->tun_id != htonll(0)) {
+        nl_msg_put_be64(buf, ODP_KEY_ATTR_TUN_ID, flow->tun_id);
+    }
+
+    nl_msg_put_u32(buf, ODP_KEY_ATTR_IN_PORT, flow->in_port);
+
+    eth_key = nl_msg_put_unspec_uninit(buf, ODP_KEY_ATTR_ETHERNET,
+                                       sizeof *eth_key);
+    memcpy(eth_key->eth_src, flow->dl_src, ETH_ADDR_LEN);
+    memcpy(eth_key->eth_dst, flow->dl_dst, ETH_ADDR_LEN);
+
+    if (flow->vlan_tci != htons(0)) {
+        struct odp_key_8021q *q_key;
+
+        q_key = nl_msg_put_unspec_uninit(buf, ODP_KEY_ATTR_8021Q,
+                                         sizeof *q_key);
+        q_key->q_tpid = htons(ETH_TYPE_VLAN);
+        q_key->q_tci = flow->vlan_tci & ~htons(VLAN_CFI);
+    }
+
+    if (ntohs(flow->dl_type) < ETH_TYPE_MIN) {
+        return;
+    }
+
+    nl_msg_put_be16(buf, ODP_KEY_ATTR_ETHERTYPE, flow->dl_type);
+
+    if (flow->dl_type == htons(ETH_TYPE_IP)) {
+        struct odp_key_ipv4 *ipv4_key;
+
+        ipv4_key = nl_msg_put_unspec_uninit(buf, ODP_KEY_ATTR_IPV4,
+                                            sizeof *ipv4_key);
+        ipv4_key->ipv4_src = flow->nw_src;
+        ipv4_key->ipv4_dst = flow->nw_dst;
+        ipv4_key->ipv4_proto = flow->nw_proto;
+        ipv4_key->ipv4_tos = flow->nw_tos;
+
+        if (flow->nw_proto == IP_TYPE_TCP) {
+            struct odp_key_tcp *tcp_key;
+
+            tcp_key = nl_msg_put_unspec_uninit(buf, ODP_KEY_ATTR_TCP,
+                                               sizeof *tcp_key);
+            tcp_key->tcp_src = flow->tp_src;
+            tcp_key->tcp_dst = flow->tp_dst;
+        } else if (flow->nw_proto == IP_TYPE_UDP) {
+            struct odp_key_udp *udp_key;
+
+            udp_key = nl_msg_put_unspec_uninit(buf, ODP_KEY_ATTR_UDP,
+                                               sizeof *udp_key);
+            udp_key->udp_src = flow->tp_src;
+            udp_key->udp_dst = flow->tp_dst;
+        } else if (flow->nw_proto == IP_TYPE_ICMP) {
+            struct odp_key_icmp *icmp_key;
+
+            icmp_key = nl_msg_put_unspec_uninit(buf, ODP_KEY_ATTR_ICMP,
+                                                sizeof *icmp_key);
+            icmp_key->icmp_type = ntohs(flow->tp_src);
+            icmp_key->icmp_code = ntohs(flow->tp_dst);
+        }
+    } else if (flow->dl_type == htons(ETH_TYPE_ARP)) {
+        struct odp_key_arp *arp_key;
+
+        arp_key = nl_msg_put_unspec_uninit(buf, ODP_KEY_ATTR_ARP,
+                                           sizeof *arp_key);
+        arp_key->arp_sip = flow->nw_src;
+        arp_key->arp_tip = flow->nw_dst;
+        arp_key->arp_op = htons(flow->nw_proto);
+    }
+}
+
+/* Converts the 'key_len' bytes of ODP_KEY_ATTR_* attributes in 'key' to a flow
+ * structure in 'flow'.  Returns 0 if successful, otherwise EINVAL. */
+int
+odp_flow_key_to_flow(const struct nlattr *key, size_t key_len,
+                     struct flow *flow)
+{
+    const struct nlattr *nla;
+    enum odp_key_type prev_type;
+    size_t left;
+
+    memset(flow, 0, sizeof *flow);
+    flow->dl_type = htons(FLOW_DL_TYPE_NONE);
+
+    prev_type = ODP_KEY_ATTR_UNSPEC;
+    NL_ATTR_FOR_EACH (nla, left, key, key_len) {
+        const struct odp_key_ethernet *eth_key;
+        const struct odp_key_8021q *q_key;
+        const struct odp_key_ipv4 *ipv4_key;
+        const struct odp_key_tcp *tcp_key;
+        const struct odp_key_udp *udp_key;
+        const struct odp_key_icmp *icmp_key;
+        const struct odp_key_arp *arp_key;
+
+        uint16_t type = nl_attr_type(nla);
+        int len = odp_flow_key_attr_len(type);
+
+        if (nl_attr_get_size(nla) != len && len != -1) {
+            return EINVAL;
+        }
+
+#define TRANSITION(PREV_TYPE, TYPE) (((PREV_TYPE) << 16) | (TYPE))
+        switch (TRANSITION(prev_type, type)) {
+        case TRANSITION(ODP_KEY_ATTR_UNSPEC, ODP_KEY_ATTR_TUN_ID):
+            flow->tun_id = nl_attr_get_be64(nla);
+            break;
+
+        case TRANSITION(ODP_KEY_ATTR_UNSPEC, ODP_KEY_ATTR_IN_PORT):
+        case TRANSITION(ODP_KEY_ATTR_TUN_ID, ODP_KEY_ATTR_IN_PORT):
+            if (nl_attr_get_u32(nla) >= UINT16_MAX) {
+                return EINVAL;
+            }
+            flow->in_port = nl_attr_get_u32(nla);
+            break;
+
+        case TRANSITION(ODP_KEY_ATTR_IN_PORT, ODP_KEY_ATTR_ETHERNET):
+            eth_key = nl_attr_get(nla);
+            memcpy(flow->dl_src, eth_key->eth_src, ETH_ADDR_LEN);
+            memcpy(flow->dl_dst, eth_key->eth_dst, ETH_ADDR_LEN);
+            break;
+
+        case TRANSITION(ODP_KEY_ATTR_ETHERNET, ODP_KEY_ATTR_8021Q):
+            q_key = nl_attr_get(nla);
+            if (q_key->q_tpid != htons(ETH_TYPE_VLAN)) {
+                /* Only standard 0x8100 VLANs currently supported. */
+                return EINVAL;
+            }
+            if (q_key->q_tci & htons(VLAN_CFI)) {
+                return EINVAL;
+            }
+            flow->vlan_tci = q_key->q_tci | htons(VLAN_CFI);
+            break;
+
+        case TRANSITION(ODP_KEY_ATTR_8021Q, ODP_KEY_ATTR_ETHERTYPE):
+        case TRANSITION(ODP_KEY_ATTR_ETHERNET, ODP_KEY_ATTR_ETHERTYPE):
+            flow->dl_type = nl_attr_get_be16(nla);
+            if (ntohs(flow->dl_type) < 1536) {
+                return EINVAL;
+            }
+            break;
+
+        case TRANSITION(ODP_KEY_ATTR_ETHERTYPE, ODP_KEY_ATTR_IPV4):
+            if (flow->dl_type != htons(ETH_TYPE_IP)) {
+                return EINVAL;
+            }
+            ipv4_key = nl_attr_get(nla);
+            flow->nw_src = ipv4_key->ipv4_src;
+            flow->nw_dst = ipv4_key->ipv4_dst;
+            flow->nw_proto = ipv4_key->ipv4_proto;
+            flow->nw_tos = ipv4_key->ipv4_tos;
+            if (flow->nw_tos & IP_ECN_MASK) {
+                return EINVAL;
+            }
+            break;
+
+        case TRANSITION(ODP_KEY_ATTR_IPV4, ODP_KEY_ATTR_TCP):
+            if (flow->nw_proto != IP_TYPE_TCP) {
+                return EINVAL;
+            }
+            tcp_key = nl_attr_get(nla);
+            flow->tp_src = tcp_key->tcp_src;
+            flow->tp_dst = tcp_key->tcp_dst;
+            break;
+
+        case TRANSITION(ODP_KEY_ATTR_IPV4, ODP_KEY_ATTR_UDP):
+            if (flow->nw_proto != IP_TYPE_UDP) {
+                return EINVAL;
+            }
+            udp_key = nl_attr_get(nla);
+            flow->tp_src = udp_key->udp_src;
+            flow->tp_dst = udp_key->udp_dst;
+            break;
+
+        case TRANSITION(ODP_KEY_ATTR_IPV4, ODP_KEY_ATTR_ICMP):
+            if (flow->nw_proto != IP_TYPE_ICMP) {
+                return EINVAL;
+            }
+            icmp_key = nl_attr_get(nla);
+            flow->tp_src = htons(icmp_key->icmp_type);
+            flow->tp_dst = htons(icmp_key->icmp_code);
+            break;
+
+        case TRANSITION(ODP_KEY_ATTR_ETHERTYPE, ODP_KEY_ATTR_ARP):
+            if (flow->dl_type != htons(ETH_TYPE_ARP)) {
+                return EINVAL;
+            }
+            arp_key = nl_attr_get(nla);
+            flow->nw_src = arp_key->arp_sip;
+            flow->nw_dst = arp_key->arp_tip;
+            if (arp_key->arp_op & htons(0xff00)) {
+                return EINVAL;
+            }
+            flow->nw_proto = ntohs(arp_key->arp_op);
+            break;
+
+        default:
+            if (type == ODP_KEY_ATTR_UNSPEC
+                || prev_type == ODP_KEY_ATTR_UNSPEC) {
+                return EINVAL;
+            }
+            return EINVAL;
+        }
+
+        prev_type = type;
+    }
+    if (left) {
+        return EINVAL;
+    }
+
+    switch (prev_type) {
+    case ODP_KEY_ATTR_UNSPEC:
+        return EINVAL;
+
+    case ODP_KEY_ATTR_TUN_ID:
+    case ODP_KEY_ATTR_IN_PORT:
+        return EINVAL;
+
+    case ODP_KEY_ATTR_ETHERNET:
+    case ODP_KEY_ATTR_8021Q:
+        return 0;
+
+    case ODP_KEY_ATTR_ETHERTYPE:
+        if (flow->dl_type == htons(ETH_TYPE_IP)
+            || flow->dl_type == htons(ETH_TYPE_ARP)) {
+            return EINVAL;
+        }
+        return 0;
+
+    case ODP_KEY_ATTR_IPV4:
+        if (flow->nw_proto == IP_TYPE_TCP
+            || flow->nw_proto == IP_TYPE_UDP
+            || flow->nw_proto == IP_TYPE_ICMP) {
+            return EINVAL;
+        }
+        return 0;
+
+    case ODP_KEY_ATTR_TCP:
+    case ODP_KEY_ATTR_UDP:
+    case ODP_KEY_ATTR_ICMP:
+    case ODP_KEY_ATTR_ARP:
+        return 0;
+
+    case __ODP_KEY_ATTR_MAX:
+    default:
+        NOT_REACHED();
+    }
 }
diff --git a/lib/odp-util.h b/lib/odp-util.h
index c8333be0..b5ead840 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -28,6 +28,7 @@
 
 struct ds;
 struct flow;
+struct ofpbuf;
 
 static inline uint16_t
 ofp_port_to_odp_port(uint16_t ofp_port)
@@ -55,7 +56,6 @@ odp_port_to_ofp_port(uint16_t odp_port)
     }
 }
 
-void format_odp_flow_key(struct ds *, const struct odp_flow_key *);
 int odp_action_len(uint16_t type);
 void format_odp_action(struct ds *, const struct nlattr *);
 void format_odp_actions(struct ds *, const struct nlattr *odp_actions,
@@ -64,21 +64,19 @@ void format_odp_flow_stats(struct ds *, const struct odp_flow_stats *);
 void format_odp_flow(struct ds *, const struct odp_flow *);
 void format_odp_port_type(struct ds *, const struct odp_port *);
 
-void odp_flow_key_from_flow(struct odp_flow_key *, const struct flow *);
-void odp_flow_key_to_flow(const struct odp_flow_key *, struct flow *);
+/* By my calculations currently the longest valid nlattr-formatted flow key is
+ * 80 bytes long, so this leaves some safety margin.
+ *
+ * We allocate temporary on-stack buffers for flow keys as arrays of uint32_t
+ * to ensure proper 32-bit alignment for Netlink attributes.  (An array of
+ * "struct nlattr" might not, in theory, be sufficiently aligned because it
+ * only contains 16-bit types.) */
+#define ODPUTIL_FLOW_KEY_BYTES 96
+#define ODPUTIL_FLOW_KEY_U32S DIV_ROUND_UP(ODPUTIL_FLOW_KEY_BYTES, 4)
 
-static inline bool
-odp_flow_key_equal(const struct odp_flow_key *a, const struct odp_flow_key *b)
-{
-    return !memcmp(a, b, sizeof *a);
-}
+void odp_flow_key_format(const struct nlattr *, size_t, struct ds *);
 
-static inline size_t
-odp_flow_key_hash(const struct odp_flow_key *flow, uint32_t basis)
-{
-    BUILD_ASSERT_DECL(!(sizeof *flow % sizeof(uint32_t)));
-    return hash_words((const uint32_t *) flow,
-                      sizeof *flow / sizeof(uint32_t), basis);
-}
+void odp_flow_key_from_flow(struct ofpbuf *, const struct flow *);
+int odp_flow_key_to_flow(const struct nlattr *, size_t, struct flow *);
 
 #endif /* odp-util.h */
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index ed2325c3..925f45fd 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -146,7 +146,7 @@ ofputil_cls_rule_from_match(const struct ofp_match *match,
     rule->flow.nw_dst = match->nw_dst;
     rule->flow.in_port = (match->in_port == htons(OFPP_LOCAL) ? ODPP_LOCAL
                      : ntohs(match->in_port));
-    rule->flow.dl_type = match->dl_type;
+    rule->flow.dl_type = ofputil_dl_type_from_openflow(match->dl_type);
     rule->flow.tp_src = match->tp_src;
     rule->flow.tp_dst = match->tp_dst;
     memcpy(rule->flow.dl_src, match->dl_src, ETH_ADDR_LEN);
@@ -270,7 +270,7 @@ ofputil_cls_rule_to_match(const struct cls_rule *rule,
                            : rule->flow.in_port);
     memcpy(match->dl_src, rule->flow.dl_src, ETH_ADDR_LEN);
     memcpy(match->dl_dst, rule->flow.dl_dst, ETH_ADDR_LEN);
-    match->dl_type = rule->flow.dl_type;
+    match->dl_type = ofputil_dl_type_to_openflow(rule->flow.dl_type);
     match->nw_src = rule->flow.nw_src;
     match->nw_dst = rule->flow.nw_dst;
     match->nw_tos = rule->flow.nw_tos;
@@ -281,6 +281,27 @@ ofputil_cls_rule_to_match(const struct cls_rule *rule,
     memset(match->pad2, '\0', sizeof match->pad2);
 }
 
+/* Given a 'dl_type' value in the format used in struct flow, returns the
+ * corresponding 'dl_type' value for use in an OpenFlow ofp_match structure. */
+ovs_be16
+ofputil_dl_type_to_openflow(ovs_be16 flow_dl_type)
+{
+    return (flow_dl_type == htons(FLOW_DL_TYPE_NONE)
+            ? htons(OFP_DL_TYPE_NOT_ETH_TYPE)
+            : flow_dl_type);
+}
+
+/* Given a 'dl_type' value in the format used in an OpenFlow ofp_match
+ * structure, returns the corresponding 'dl_type' value for use in struct
+ * flow. */
+ovs_be16
+ofputil_dl_type_from_openflow(ovs_be16 ofp_dl_type)
+{
+    return (ofp_dl_type == htons(OFP_DL_TYPE_NOT_ETH_TYPE)
+            ? htons(FLOW_DL_TYPE_NONE)
+            : ofp_dl_type);
+}
+
 /* Returns a transaction ID to use for an outgoing OpenFlow message. */
 static ovs_be32
 alloc_xid(void)
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index 5d9e8c4d..6eff9804 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -110,6 +110,10 @@ void ofputil_cls_rule_to_match(const struct cls_rule *, enum nx_flow_format,
 void normalize_match(struct ofp_match *);
 char *ofp_match_to_literal_string(const struct ofp_match *match);
 
+/* dl_type translation between OpenFlow and 'struct flow' format. */
+ovs_be16 ofputil_dl_type_to_openflow(ovs_be16 flow_dl_type);
+ovs_be16 ofputil_dl_type_from_openflow(ovs_be16 ofp_dl_type);
+
 /* Flow formats. */
 bool ofputil_flow_format_is_valid(enum nx_flow_format);
 const char *ofputil_flow_format_to_string(enum nx_flow_format);
diff --git a/lib/packets.h b/lib/packets.h
index 39e88f1f..96e23e18 100644
--- a/lib/packets.h
+++ b/lib/packets.h
@@ -155,6 +155,10 @@ void compose_benign_packet(struct ofpbuf *, const char *tag,
 #define ETH_TYPE_VLAN          0x8100
 #define ETH_TYPE_CFM           0x8902
 
+/* Minimum value for an Ethernet type.  Values below this are IEEE 802.2 frame
+ * lengths. */
+#define ETH_TYPE_MIN           0x600
+
 #define ETH_HEADER_LEN 14
 #define ETH_PAYLOAD_MIN 46
 #define ETH_PAYLOAD_MAX 1500