diff mbox

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

Message ID 367FDB28-EC42-4DF9-B4ED-8A25457C0AF7@ovn.org
State Not Applicable
Headers show

Commit Message

Justin Pettit July 13, 2016, 12:16 a.m. UTC 
> On Jul 12, 2016, at 4:15 PM, Ben Pfaff <blp@ovn.org> wrote:
> 
> 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!

True enough.  I went ahead and changed the xreg definitions, too.

> 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.

Ugh, good catch.  I've appended a diff that should address this.  Let me know if you see any problems.

> 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.

Done.

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

Thanks!

--Justin


-=-=-=-=-=-=-=-=-

Comments

Ben Pfaff July 13, 2016, 3:47 a.m. UTC | #1 
On Tue, Jul 12, 2016 at 05:16:32PM -0700, Justin Pettit wrote:
> 
> > On Jul 12, 2016, at 4:15 PM, Ben Pfaff <blp@ovn.org> wrote:
> > 
> > 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>
> > 
> > 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.
> 
> Ugh, good catch.  I've appended a diff that should address this.  Let
> me know if you see any problems.

There's a test, so anything we missed should get caught on the first
big-endian build, which makes me unworried.

Thanks,

Ben.
diff mbox

Patch

diff --git a/include/openvswitch/meta-flow.h b/include/openvswitch/meta-flow.h
index 0a54afb..e2e9220 100644
--- a/include/openvswitch/meta-flow.h
+++ b/include/openvswitch/meta-flow.h
@@ -894,10 +894,10 @@  enum OVS_PACKED_ENUM mf_field_id {
     /* "xreg<N>".
      *
      * OpenFlow 1.5 ``extended register".  Each extended register
-     * overlays two of the Nicira extension 32-bit registers: xreg0 overlays
-     * reg0 and reg1, with reg0 supplying the most-significant bits of xreg0
-     * and reg1 the least-significant.  xreg1 similarly overlays reg2 and reg3,
-     * and so on.
+     * overlays two of the Open vSwitch extension 32-bit registers:
+     * xreg0 overlays reg0 and reg1, with reg0 supplying the
+     * most-significant bits of xreg0 and reg1 the least-significant.
+     * xreg1 similarly overlays reg2 and reg3, and so on.
      *
      * These registers were introduced in OpenFlow 1.5, but EXT-244 in the ONF
      * JIRA also publishes them as a (draft) OpenFlow extension to OpenFlow
@@ -927,8 +927,8 @@  enum OVS_PACKED_ENUM mf_field_id {
     /* "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
+     * overlays four of the Open vSwitch 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.
      *
diff --git a/include/openvswitch/types.h b/include/openvswitch/types.h
index bc94145..da56d4b 100644
--- a/include/openvswitch/types.h
+++ b/include/openvswitch/types.h
@@ -103,6 +103,7 @@  typedef union {
 /* MSVC2015 doesn't support designated initializers when compiling C++,
  * and doesn't support ternary operators with non-designated initializers.
  * So we use these static definitions rather than using initializer macros. */
+static const ovs_u128 OVS_U128_ZERO = { { 0, 0, 0, 0 } };
 static const ovs_u128 OVS_U128_MAX = { { UINT32_MAX, UINT32_MAX,
                                          UINT32_MAX, UINT32_MAX } };
 static const ovs_be128 OVS_BE128_MAX OVS_UNUSED = { { OVS_BE32_MAX, OVS_BE32_MA
diff --git a/lib/flow.h b/lib/flow.h
index 7f373fe..4cff48c 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -124,10 +124,10 @@  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];
+    value.u64.hi = (uint64_t) flow->regs[idx * 4] << 32;
+    value.u64.hi |= flow->regs[idx * 4 + 1];
+    value.u64.lo = (uint64_t) flow->regs[idx * 4 + 2] << 32;
+    value.u64.lo |= flow->regs[idx * 4 + 3];
 
     return value;
 }
@@ -135,10 +135,10 @@  flow_get_xxreg(const struct flow *flow, int idx)
 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];
+    flow->regs[idx * 4] = value.u64.hi >> 32;
+    flow->regs[idx * 4 + 1] = value.u64.hi;
+    flow->regs[idx * 4 + 2] = value.u64.lo >> 32;
+    flow->regs[idx * 4 + 3] = value.u64.lo;
 }
 
 static inline int
diff --git a/lib/meta-flow.c b/lib/meta-flow.c
index b85ed97..af579a2 100644
--- a/lib/meta-flow.c
+++ b/lib/meta-flow.c
@@ -1616,8 +1616,8 @@  mf_set_wild(const struct mf_field *mf, struct match *match
         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);
+        match_set_xxreg_masked(match, mf->id - MFF_XXREG0, OVS_U128_ZERO,
+                               OVS_U128_ZERO);
         break;
     }