diff mbox

[ovs-dev,ovn-ipv6,05/26] Introduce 128-bit xxregs.

Message ID 1468306616-125783-6-git-send-email-jpettit@ovn.org
State Accepted
Headers show

Commit Message

Justin Pettit July 12, 2016, 6:56 a.m. UTC
These are needed to handle IPv6 addresses.

Signed-off-by: Justin Pettit <jpettit@ovn.org>
---
 include/openvswitch/flow.h      | 10 +++++++++-
 include/openvswitch/match.h     |  3 +++
 include/openvswitch/meta-flow.h | 37 +++++++++++++++++++++++++++++++++++++
 lib/flow.c                      |  9 +++++++++
 lib/flow.h                      | 22 ++++++++++++++++++++++
 lib/match.c                     | 15 +++++++++++++++
 lib/meta-flow.c                 | 29 +++++++++++++++++++++++++++++
 lib/odp-util.c                  |  6 ++++++
 tests/ofproto-dpif.at           | 25 +++++++++++++++++++++++++
 tests/ofproto.at                |  6 +++++-
 tests/ovs-ofctl.at              |  8 ++++++++
 utilities/ovs-ofctl.8.in        | 11 +++++++++++
 12 files changed, 179 insertions(+), 2 deletions(-)

Comments

Ben Pfaff July 12, 2016, 11:15 p.m. UTC | #1
On Mon, Jul 11, 2016 at 11:56:35PM -0700, Justin Pettit wrote:
> These are needed to handle IPv6 addresses.
> 
> Signed-off-by: Justin Pettit <jpettit@ovn.org>

The comment in meta-flow.h talks about "Nicira extension" registers.  I
think this made sense a few years ago, but Nicira isn't very relevant
anymore, so I tend to try to say "Open vSwitch extension" these days;
Open vSwitch is still quite relevant!

I don't think that flow_get_xxreg() and flow_set_xxreg() are endian
independent.  That is, suppose that actions set reg0, reg1, reg2, and
reg3 independently, and then read xxreg0.  Will xxreg0 have the same
value on big-endian and little-endian architectures?  That is something
that we guarantee for the xregs, so it would be bad to drop it for
xxregs.

Instead of this:
+        ovs_u128 u_zero = {.u64.hi = 0, .u64.lo = 0};
I'd suggest adding an OVS_U128_ZERO to include/openvswitch/types.h to go
along with OVS_U128_MAX.

Acked-by: Ben Pfaff <blp@ovn.org>
diff mbox

Patch

diff --git a/include/openvswitch/flow.h b/include/openvswitch/flow.h
index 12e70ea..6b3005b 100644
--- a/include/openvswitch/flow.h
+++ b/include/openvswitch/flow.h
@@ -28,7 +28,7 @@ 
 /* Number of Open vSwitch extension 32-bit registers. */
 #define FLOW_N_REGS 16
 BUILD_ASSERT_DECL(FLOW_N_REGS <= NXM_NX_MAX_REGS);
-BUILD_ASSERT_DECL(FLOW_N_REGS % 2 == 0); /* Even. */
+BUILD_ASSERT_DECL(FLOW_N_REGS % 4 == 0); /* Handle xxregs. */
 
 /* Number of OpenFlow 1.5+ 64-bit registers.
  *
@@ -36,6 +36,12 @@  BUILD_ASSERT_DECL(FLOW_N_REGS % 2 == 0); /* Even. */
  * are half as many of them.*/
 #define FLOW_N_XREGS (FLOW_N_REGS / 2)
 
+/* Number of 128-bit registers.
+ *
+ * Each of these overlays four Open vSwitch 32-bit registers, so there
+ * are a quarter as many of them.*/
+#define FLOW_N_XXREGS (FLOW_N_REGS / 4)
+
 /* 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
@@ -180,6 +186,8 @@  void flow_wildcards_set_reg_mask(struct flow_wildcards *,
                                  int idx, uint32_t mask);
 void flow_wildcards_set_xreg_mask(struct flow_wildcards *,
                                   int idx, uint64_t mask);
+void flow_wildcards_set_xxreg_mask(struct flow_wildcards *,
+                                   int idx, ovs_u128 mask);
 
 void flow_wildcards_and(struct flow_wildcards *dst,
                         const struct flow_wildcards *src1,
diff --git a/include/openvswitch/match.h b/include/openvswitch/match.h
index c955753..3b7f32f 100644
--- a/include/openvswitch/match.h
+++ b/include/openvswitch/match.h
@@ -60,6 +60,9 @@  void match_set_reg_masked(struct match *, unsigned int reg_idx,
 void match_set_xreg(struct match *, unsigned int xreg_idx, uint64_t value);
 void match_set_xreg_masked(struct match *, unsigned int xreg_idx,
                            uint64_t value, uint64_t mask);
+void match_set_xxreg(struct match *, unsigned int xxreg_idx, ovs_u128 value);
+void match_set_xxreg_masked(struct match *, unsigned int xxreg_idx,
+                           ovs_u128 value, ovs_u128 mask);
 void match_set_actset_output(struct match *, ofp_port_t actset_output);
 void match_set_metadata(struct match *, ovs_be64 metadata);
 void match_set_metadata_masked(struct match *,
diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
index 828c40c..0a54afb 100644
--- a/include/openvswitch/meta-flow.h
+++ b/include/openvswitch/meta-flow.h
@@ -923,6 +923,34 @@  enum OVS_PACKED_ENUM mf_field_id {
 #error "Need to update MFF_REG* to match FLOW_N_XREGS"
 #endif
 
+#if FLOW_N_XXREGS == 4
+    /* "xxreg<N>".
+     *
+     * ``extended-extended register".  Each of these extended registers
+     * overlays four of the Nicira extension 32-bit registers: xxreg0
+     * overlays reg0 through reg3, with reg0 supplying the
+     * most-significant bits of xxreg0 and reg3 the least-significant.
+     * xxreg1 similarly overlays reg4 and reg7.
+     *
+     * Type: be128.
+     * Maskable: bitwise.
+     * Formatting: hexadecimal.
+     * Prerequisites: none.
+     * Access: read/write.
+     * NXM: NXM_NX_XXREG0(111) since v2.6.              <0>
+     * NXM: NXM_NX_XXREG1(112) since v2.6.              <1>
+     * NXM: NXM_NX_XXREG0(113) since v2.6.              <2>
+     * NXM: NXM_NX_XXREG1(114) since v2.6.              <3>
+     * OXM: none.
+     */
+    MFF_XXREG0,
+    MFF_XXREG1,
+    MFF_XXREG2,
+    MFF_XXREG3,
+#else
+#error "Need to update MFF_REG* to match FLOW_N_XXREGS"
+#endif
+
 /* ## -------- ## */
 /* ## Ethernet ## */
 /* ## -------- ## */
@@ -1771,6 +1799,15 @@  struct mf_bitmap {
 #error "Need to update CASE_MFF_XREGS to match FLOW_N_XREGS"
 #endif
 
+/* Use this macro as CASE_MFF_XXREGS: in a switch statement to choose
+ * all of the MFF_REGn cases. */
+#if FLOW_N_XXREGS == 4
+#define CASE_MFF_XXREGS                                              \
+    case MFF_XXREG0: case MFF_XXREG1: case MFF_XXREG2: case MFF_XXREG3
+#else
+#error "Need to update CASE_MFF_XXREGS to match FLOW_N_XXREGS"
+#endif
+
 /* Use this macro as CASE_MFF_TUN_METADATA: in a switch statement to choose
  * all of the MFF_TUN_METADATAn cases. */
 #define CASE_MFF_TUN_METADATA                         \
diff --git a/lib/flow.c b/lib/flow.c
index b97b853..8842e8d 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -1578,6 +1578,15 @@  flow_wildcards_set_xreg_mask(struct flow_wildcards *wc, int idx, uint64_t mask)
     flow_set_xreg(&wc->masks, idx, mask);
 }
 
+/* Sets the wildcard mask for register 'idx' in 'wc' to 'mask'.
+ * (A 0-bit indicates a wildcard bit.) */
+void
+flow_wildcards_set_xxreg_mask(struct flow_wildcards *wc, int idx,
+                              ovs_u128 mask)
+{
+    flow_set_xxreg(&wc->masks, idx, mask);
+}
+
 /* Calculates the 5-tuple hash from the given miniflow.
  * This returns the same value as flow_hash_5tuple for the corresponding
  * flow. */
diff --git a/lib/flow.h b/lib/flow.h
index 5479677..7f373fe 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -119,6 +119,28 @@  flow_set_xreg(struct flow *flow, int idx, uint64_t value)
     flow->regs[idx * 2 + 1] = value;
 }
 
+static inline ovs_u128
+flow_get_xxreg(const struct flow *flow, int idx)
+{
+    ovs_u128 value;
+
+    value.u32[3] = flow->regs[idx * 4];
+    value.u32[2] = flow->regs[idx * 4 + 1];
+    value.u32[1] = flow->regs[idx * 4 + 2];
+    value.u32[0] = flow->regs[idx * 4 + 3];
+
+    return value;
+}
+
+static inline void
+flow_set_xxreg(struct flow *flow, int idx, ovs_u128 value)
+{
+    flow->regs[idx * 4] = value.u32[3];
+    flow->regs[idx * 4 + 1] = value.u32[2];
+    flow->regs[idx * 4 + 2] = value.u32[1];
+    flow->regs[idx * 4 + 3] = value.u32[0];
+}
+
 static inline int
 flow_compare_3way(const struct flow *a, const struct flow *b)
 {
diff --git a/lib/match.c b/lib/match.c
index 906308b..d5deb7d 100644
--- a/lib/match.c
+++ b/lib/match.c
@@ -133,6 +133,21 @@  match_set_xreg_masked(struct match *match, unsigned int xreg_idx,
 }
 
 void
+match_set_xxreg(struct match *match, unsigned int xxreg_idx, ovs_u128 value)
+{
+    match_set_xxreg_masked(match, xxreg_idx, value, OVS_U128_MAX);
+}
+
+void
+match_set_xxreg_masked(struct match *match, unsigned int xxreg_idx,
+                      ovs_u128 value, ovs_u128 mask)
+{
+    ovs_assert(xxreg_idx < FLOW_N_XXREGS);
+    flow_wildcards_set_xxreg_mask(&match->wc, xxreg_idx, mask);
+    flow_set_xxreg(&match->flow, xxreg_idx, ovs_u128_and(value, mask));
+}
+
+void
 match_set_actset_output(struct match *match, ofp_port_t actset_output)
 {
     match->wc.masks.actset_output = u16_to_ofp(UINT16_MAX);
diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index e160de1..b85ed97 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -237,6 +237,10 @@  mf_is_all_wild(const struct mf_field *mf, const struct flow_wildcards *wc)
         return !wc->masks.regs[mf->id - MFF_REG0];
     CASE_MFF_XREGS:
         return !flow_get_xreg(&wc->masks, mf->id - MFF_XREG0);
+    CASE_MFF_XXREGS: {
+        ovs_u128 value = flow_get_xxreg(&wc->masks, mf->id - MFF_XXREG0);
+        return ovs_u128_is_zero(value);
+    }
     case MFF_ACTSET_OUTPUT:
         return !wc->masks.actset_output;
 
@@ -526,6 +530,7 @@  mf_is_value_valid(const struct mf_field *mf, const union mf_value *value)
     case MFF_CT_LABEL:
     CASE_MFF_REGS:
     CASE_MFF_XREGS:
+    CASE_MFF_XXREGS:
     case MFF_ETH_SRC:
     case MFF_ETH_DST:
     case MFF_ETH_TYPE:
@@ -705,6 +710,10 @@  mf_get_value(const struct mf_field *mf, const struct flow *flow,
         value->be64 = htonll(flow_get_xreg(flow, mf->id - MFF_XREG0));
         break;
 
+    CASE_MFF_XXREGS:
+        value->be128 = hton128(flow_get_xxreg(flow, mf->id - MFF_XXREG0));
+        break;
+
     case MFF_ETH_SRC:
         value->mac = flow->dl_src;
         break;
@@ -963,6 +972,10 @@  mf_set_value(const struct mf_field *mf,
         match_set_xreg(match, mf->id - MFF_XREG0, ntohll(value->be64));
         break;
 
+    CASE_MFF_XXREGS:
+        match_set_xxreg(match, mf->id - MFF_XXREG0, ntoh128(value->be128));
+        break;
+
     case MFF_ETH_SRC:
         match_set_dl_src(match, value->mac);
         break;
@@ -1273,6 +1286,10 @@  mf_set_flow_value(const struct mf_field *mf,
         flow_set_xreg(flow, mf->id - MFF_XREG0, ntohll(value->be64));
         break;
 
+    CASE_MFF_XXREGS:
+        flow_set_xxreg(flow, mf->id - MFF_XXREG0, ntoh128(value->be128));
+        break;
+
     case MFF_ETH_SRC:
         flow->dl_src = value->mac;
         break;
@@ -1598,6 +1615,12 @@  mf_set_wild(const struct mf_field *mf, struct match *match, char **err_str)
         match_set_xreg_masked(match, mf->id - MFF_XREG0, 0, 0);
         break;
 
+    CASE_MFF_XXREGS: {
+        ovs_u128 u_zero = {.u64.hi = 0, .u64.lo = 0};
+        match_set_xxreg_masked(match, mf->id - MFF_XXREG0, u_zero, u_zero);
+        break;
+    }
+
     case MFF_ETH_SRC:
         match->flow.dl_src = eth_addr_zero;
         match->wc.masks.dl_src = eth_addr_zero;
@@ -1863,6 +1886,12 @@  mf_set(const struct mf_field *mf,
                               ntohll(value->be64), ntohll(mask->be64));
         break;
 
+    CASE_MFF_XXREGS: {
+        match_set_xxreg_masked(match, mf->id - MFF_XXREG0,
+                ntoh128(value->be128), ntoh128(mask->be128));
+        break;
+    }
+
     case MFF_PKT_MARK:
         match_set_pkt_mark_masked(match, ntohl(value->be32),
                                   ntohl(mask->be32));
diff --git a/lib/odp-util.c b/lib/odp-util.c
index fd1ca9b..b0977de 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -2991,6 +2991,12 @@  format_u128(struct ds *ds, const ovs_u128 *key, const ovs_u128 *mask,
     }
 }
 
+/* Read the string from 's_' as a 128-bit value.  If the string contains
+ * a "/", the rest of the string will be treated as a 128-bit mask.
+ *
+ * If either the value or mask is larger than 64 bits, the string must
+ * be in hexadecimal.
+ */
 static int
 scan_u128(const char *s_, ovs_u128 *value, ovs_u128 *mask)
 {
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 8287d90..87f9efd 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -558,6 +558,27 @@  AT_CHECK([tail -1 stdout], [0], [Datapath actions: 2
 OVS_VSWITCHD_STOP
 AT_CLEANUP
 
+dnl Tests that the standardized xxregs are mapped onto the legacy OVS
+dnl registers in the manner documented in ovs-ofctl(8).
+AT_SETUP([ofproto-dpif - extended-extended registers])
+OVS_VSWITCHD_START
+add_of_ports br0 1 2 3
+AT_DATA([flows.txt], [dnl
+table=0 actions=load:0x0123456789abcdeffedcba9876543210->NXM_NX_XXREG1[[]],resubmit(,1)
+table=1,reg4=0x01234567,reg5=0x89abcdef,reg6=0xfedcba98,reg7=0x76543210   actions=2
+
+# These low-priority rules shouldn't match.  They're here only to make really
+# sure that the test fails if either of the above rules fails to match.
+table=0,priority=0                        actions=3
+table=1,priority=0                        actions=3
+])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=128,frag=no),icmp(type=8,code=0)'], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0], [Datapath actions: 2
+])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
 AT_SETUP([ofproto-dpif - load and move order])
 OVS_VSWITCHD_START
 add_of_ports br0 1 10 11
@@ -629,6 +650,10 @@  table=2,priority=10                  actions=5,goto_table(3)
 table=3,priority=20,xreg0=2  actions=6,goto_table(4)
 table=3,priority=10          actions=7,goto_table(4)
 
+# Verify that xxreg0 got copied properly from actset_output.
+table=3,priority=20,xxreg0=2  actions=6,goto_table(4)
+table=3,priority=10           actions=7,goto_table(4)
+
 # Verify that adding a group action unsets actset_output,
 # even if output follows group.
 table=4 actions=write_actions(group(5),output(10)),goto_table(5)
diff --git a/tests/ofproto.at b/tests/ofproto.at
index 78ad09e..4abd335 100644
--- a/tests/ofproto.at
+++ b/tests/ofproto.at
@@ -1807,7 +1807,7 @@  head_table () {
         actions: output group set_field strip_vlan push_vlan mod_nw_ttl dec_ttl set_mpls_ttl dec_mpls_ttl push_mpls pop_mpls set_queue
         supported on Set-Field: tun_id tun_src tun_dst tun_ipv6_src tun_ipv6_dst tun_flags tun_gbp_id tun_gbp_flags tun_metadata0 dnl
 tun_metadata1 tun_metadata2 tun_metadata3 tun_metadata4 tun_metadata5 tun_metadata6 tun_metadata7 tun_metadata8 tun_metadata9 tun_metadata10 tun_metadata11 tun_metadata12 tun_metadata13 tun_metadata14 tun_metadata15 tun_metadata16 tun_metadata17 tun_metadata18 tun_metadata19 tun_metadata20 tun_metadata21 tun_metadata22 tun_metadata23 tun_metadata24 tun_metadata25 tun_metadata26 tun_metadata27 tun_metadata28 tun_metadata29 tun_metadata30 tun_metadata31 tun_metadata32 tun_metadata33 tun_metadata34 tun_metadata35 tun_metadata36 tun_metadata37 tun_metadata38 tun_metadata39 tun_metadata40 tun_metadata41 tun_metadata42 tun_metadata43 tun_metadata44 tun_metadata45 tun_metadata46 tun_metadata47 tun_metadata48 tun_metadata49 tun_metadata50 tun_metadata51 tun_metadata52 tun_metadata53 tun_metadata54 tun_metadata55 tun_metadata56 tun_metadata57 tun_metadata58 tun_metadata59 tun_metadata60 tun_metadata61 tun_metadata62 tun_metadata63 dnl
-metadata in_port in_port_oxm pkt_mark ct_mark ct_label reg0 reg1 reg2 reg3 reg4 reg5 reg6 reg7 reg8 reg9 reg10 reg11 reg12 reg13 reg14 reg15 xreg0 xreg1 xreg2 xreg3 xreg4 xreg5 xreg6 xreg7 eth_src eth_dst vlan_tci vlan_vid vlan_pcp mpls_label mpls_tc mpls_ttl ip_src ip_dst ipv6_src ipv6_dst ipv6_label nw_tos ip_dscp nw_ecn nw_ttl arp_op arp_spa arp_tpa arp_sha arp_tha tcp_src tcp_dst udp_src udp_dst sctp_src sctp_dst icmp_type icmp_code icmpv6_type icmpv6_code nd_target nd_sll nd_tll
+metadata in_port in_port_oxm pkt_mark ct_mark ct_label reg0 reg1 reg2 reg3 reg4 reg5 reg6 reg7 reg8 reg9 reg10 reg11 reg12 reg13 reg14 reg15 xreg0 xreg1 xreg2 xreg3 xreg4 xreg5 xreg6 xreg7 xxreg0 xxreg1 xxreg2 xxreg3 eth_src eth_dst vlan_tci vlan_vid vlan_pcp mpls_label mpls_tc mpls_ttl ip_src ip_dst ipv6_src ipv6_dst ipv6_label nw_tos ip_dscp nw_ecn nw_ttl arp_op arp_spa arp_tpa arp_sha arp_tha tcp_src tcp_dst udp_src udp_dst sctp_src sctp_dst icmp_type icmp_code icmpv6_type icmpv6_code nd_target nd_sll nd_tll
     matching:
       dp_hash: arbitrary mask
       recirc_id: exact match or wildcard
@@ -1917,6 +1917,10 @@  metadata in_port in_port_oxm pkt_mark ct_mark ct_label reg0 reg1 reg2 reg3 reg4
       xreg5: arbitrary mask
       xreg6: arbitrary mask
       xreg7: arbitrary mask
+      xxreg0: arbitrary mask
+      xxreg1: arbitrary mask
+      xxreg2: arbitrary mask
+      xxreg3: arbitrary mask
       eth_src: arbitrary mask
       eth_dst: arbitrary mask
       eth_type: exact match or wildcard
diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
index 65ce51b..e3d3e7a 100644
--- a/tests/ovs-ofctl.at
+++ b/tests/ovs-ofctl.at
@@ -71,6 +71,14 @@  for test_case in \
     'xreg6=6/1                                   NXM,OXM' \
     'xreg7=7                                     NXM,OXM' \
     'xreg7=7/1                                   NXM,OXM' \
+    'xxreg0=0                                    NXM,OXM' \
+    'xxreg0=0/1                                  NXM,OXM' \
+    'xxreg1=1                                    NXM,OXM' \
+    'xxreg1=1/1                                  NXM,OXM' \
+    'xxreg2=2                                    NXM,OXM' \
+    'xxreg2=2/1                                  NXM,OXM' \
+    'xxreg3=3                                    NXM,OXM' \
+    'xxreg3=3/1                                  NXM,OXM' \
     'dl_src=00:11:22:33:44:55                    any' \
     'dl_src=00:11:22:33:44:55/00:ff:ff:ff:ff:ff  NXM,OXM,OpenFlow11' \
     'dl_dst=00:11:22:33:44:55                    any' \
diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in
index 28a5437..0d91e5f 100644
--- a/utilities/ovs-ofctl.8.in
+++ b/utilities/ovs-ofctl.8.in
@@ -1336,6 +1336,17 @@  just the ``packet registers,'' but Open vSwitch already had 32-bit
 registers by that name, which is why Open vSwitch refers to the
 standard registers as ``extended registers''.
 .
+.IP "\fBxxreg\fIidx\fB=\fIvalue\fR[\fB/\fImask\fR]"
+Matches \fIvalue\fR either exactly or with optional \fImask\fR in
+128-bit ``extended-extended register'' number \fIidx\fR.  Each of the
+128-bit extended registers overlays four of the 32-bit registers:
+\fBxxreg0\fR overlays \fBreg0\fR through \fBreg3\fR, with \fBreg0\fR
+supplying the most-significant bits of \fBxxreg0\fR and \fBreg3\fR the
+least-significant.  \fBxxreg1\fR similarly overlays \fBreg4\fR through
+\fBreg7\fR, and so on.
+.IP
+These fields were added in Open vSwitch 2.6.
+.
 .IP \fBpkt_mark=\fIvalue\fR[\fB/\fImask\fR]
 Matches packet metadata mark \fIvalue\fR either exactly or with optional
 \fImask\fR. The mark is associated data that may be passed into other