[ovs-dev] [PATCHv2 2/2] flow: Adds support for arbitrary ethernet masking
Ben Pfaff
blp at nicira.com
Tue May 29 19:22:08 UTC 2012
Thanks Joe! I looked this over and fixed a couple of issues I
noticed. I'm appending the changes I made. I'll push this soon.
On Tue, May 29, 2012 at 12:38:21AM +1200, Joe Stringer wrote:
> There is currently two tests that this patch fails:-
>
> [Test 137] "ovs-ofctl dump-flows honors -F option" (ovs-ofctl.at:657)
>
> I don't entirely understand this -- what is "reg0=0x12345" meant to match on?
> It seems that the flow which matches this is not added, so we get the
> following diff on the test:
Hmm, it passed once I made my changes. Probably some seemingly this
simply found a bug in a surprising way.
To answer your question, "reg0=0x12345" matches a "register" that OVS
implements as an OpenFlow extension to the match. NXM supports
registers, OpenFlow 1.0 doesn't, so this test verifies that OpenFlow
1.0 is really in use by demonstrating that the register match doesn't
show up in OpenFlow 1.0.
> [Test 419] "ofproto - basic flow_mod commands (NXM)" (ofproto.at:109)
>
> Here, the command "del-flows" is sent, but when we then dump-flows, one of
> them is still present:
>
> NXST_FLOW reply:
> + table=1, in_port=3 actions=output:2
Again it disappeared with my changes.
> Any tips about how to fix these problems would be welcome. All other tests
> pass, and I've included a couple of extra tests for arbitrary ethernet source
> masks.
Thanks.
> I have one other query -- Now that FWW_ETH_* have been removed, should I
> shuffle the rest of the FWW_* down? (while retaining OFPFW compatibility)
Yes, we should. I made that change. Perhaps this was the problem
you saw somehow, though I didn't check carefully.
Other changes I made:
* Rephrased the nicira-ext.h comment a bit.
* Moved the zero padding to the end of struct flow_wildcards.
It seemed a little odd to me to have it in the middle,
though I can't say off-hand that it is wrong.
* Factor out some code from nx_put_match() into
nxm_put_eth_masked().
* Remove some always-true conditions from
ofputil_usable_protocols().
* Remove redundant memcpy() from ofputil_normalize_rule().
(The existing "wc = rule->wc;" assignment already copied
these fields.)
* Update tests slightly.
--8<--------------------------cut here-------------------------->8--
diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
index 99c2cae..708eee0 100644
--- a/include/openflow/nicira-ext.h
+++ b/include/openflow/nicira-ext.h
@@ -1369,9 +1369,8 @@ OFP_ASSERT(sizeof(struct nx_action_output_reg) == 24);
* Format: 48-bit Ethernet MAC address.
*
* Masking: Fully maskable, in versions 1.8 and later. Earlier versions only
- * require support for NXM_OF_ETH_DST_W, with the following masks:
- * 00:00:00:00:00:00, fe:ff:ff:ff:ff:ff,
- * 01:00:00:00:00:00, ff:ff:ff:ff:ff:ff. */
+ * supported the following masks for NXM_OF_ETH_DST_W: 00:00:00:00:00:00,
+ * fe:ff:ff:ff:ff:ff, 01:00:00:00:00:00, ff:ff:ff:ff:ff:ff. */
#define NXM_OF_ETH_DST NXM_HEADER (0x0000, 1, 6)
#define NXM_OF_ETH_DST_W NXM_HEADER_W(0x0000, 1, 6)
#define NXM_OF_ETH_SRC NXM_HEADER (0x0000, 2, 6)
diff --git a/lib/flow.h b/lib/flow.h
index 0595aff..1964115 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -150,16 +150,16 @@ typedef unsigned int OVS_BITWISE flow_wildcards_t;
#define FWW_DL_TYPE ((OVS_FORCE flow_wildcards_t) (1 << 4))
#define FWW_NW_PROTO ((OVS_FORCE flow_wildcards_t) (1 << 5))
/* No corresponding OFPFW_* bits. */
-#define FWW_NW_DSCP ((OVS_FORCE flow_wildcards_t) (1 << 6))
-#define FWW_NW_ECN ((OVS_FORCE flow_wildcards_t) (1 << 7))
-#define FWW_ARP_SHA ((OVS_FORCE flow_wildcards_t) (1 << 8))
-#define FWW_ARP_THA ((OVS_FORCE flow_wildcards_t) (1 << 9))
-#define FWW_IPV6_LABEL ((OVS_FORCE flow_wildcards_t) (1 << 10))
-#define FWW_NW_TTL ((OVS_FORCE flow_wildcards_t) (1 << 11))
-#define FWW_ALL ((OVS_FORCE flow_wildcards_t) (((1 << 12)) - 1))
+#define FWW_NW_DSCP ((OVS_FORCE flow_wildcards_t) (1 << 1))
+#define FWW_NW_ECN ((OVS_FORCE flow_wildcards_t) (1 << 2))
+#define FWW_ARP_SHA ((OVS_FORCE flow_wildcards_t) (1 << 3))
+#define FWW_ARP_THA ((OVS_FORCE flow_wildcards_t) (1 << 6))
+#define FWW_IPV6_LABEL ((OVS_FORCE flow_wildcards_t) (1 << 7))
+#define FWW_NW_TTL ((OVS_FORCE flow_wildcards_t) (1 << 8))
+#define FWW_ALL ((OVS_FORCE flow_wildcards_t) (((1 << 9)) - 1))
/* Remember to update FLOW_WC_SEQ when adding or removing FWW_*. */
-BUILD_ASSERT_DECL(FWW_ALL == ((1 << 12) - 1) && FLOW_WC_SEQ == 11);
+BUILD_ASSERT_DECL(FWW_ALL == ((1 << 9) - 1) && FLOW_WC_SEQ == 11);
/* Information on wildcards for a flow, as a supplement to "struct flow".
*
@@ -179,9 +179,9 @@ struct flow_wildcards {
ovs_be16 tp_src_mask; /* 1-bit in each significant tp_src bit. */
ovs_be16 tp_dst_mask; /* 1-bit in each significant tp_dst bit. */
uint8_t nw_frag_mask; /* 1-bit in each significant nw_frag bit. */
- uint8_t zeros[1]; /* Padding field set to zero. */
uint8_t dl_src_mask[6]; /* 1-bit in each significant dl_src bit. */
uint8_t dl_dst_mask[6]; /* 1-bit in each significant dl_dst bit. */
+ uint8_t zeros[1]; /* Padding field set to zero. */
};
/* Remember to update FLOW_WC_SEQ when updating struct flow_wildcards. */
diff --git a/lib/nx-match.c b/lib/nx-match.c
index 75d4358..5e3c3dc 100644
--- a/lib/nx-match.c
+++ b/lib/nx-match.c
@@ -354,6 +354,22 @@ nxm_put_eth(struct ofpbuf *b, uint32_t header,
}
static void
+nxm_put_eth_masked(struct ofpbuf *b, uint32_t header,
+ const uint8_t value[ETH_ADDR_LEN],
+ const uint8_t mask[ETH_ADDR_LEN])
+{
+ if (!eth_addr_is_zero(mask)) {
+ if (eth_mask_is_exact(mask)) {
+ nxm_put_eth(b, header, value);
+ } else {
+ nxm_put_header(b, NXM_MAKE_WILD_HEADER(header));
+ ofpbuf_put(b, value, ETH_ADDR_LEN);
+ ofpbuf_put(b, mask, ETH_ADDR_LEN);
+ }
+ }
+}
+
+static void
nxm_put_ipv6(struct ofpbuf *b, uint32_t header,
const struct in6_addr *value, const struct in6_addr *mask)
{
@@ -462,24 +478,8 @@ nx_put_match(struct ofpbuf *b, const struct cls_rule *cr,
}
/* Ethernet. */
- if (!eth_addr_is_zero(cr->wc.dl_src_mask)) {
- if (eth_mask_is_exact(cr->wc.dl_src_mask)) {
- nxm_put_eth(b, NXM_OF_ETH_SRC, flow->dl_src);
- } else {
- nxm_put_eth(b, NXM_OF_ETH_SRC_W, flow->dl_src);
- ofpbuf_put(b, cr->wc.dl_src_mask, ETH_ADDR_LEN);
- }
- }
-
- if (!eth_addr_is_zero(cr->wc.dl_dst_mask)) {
- if (eth_mask_is_exact(cr->wc.dl_dst_mask)) {
- nxm_put_eth(b, NXM_OF_ETH_DST, flow->dl_dst);
- } else {
- nxm_put_eth(b, NXM_OF_ETH_DST_W, flow->dl_dst);
- ofpbuf_put(b, cr->wc.dl_dst_mask, ETH_ADDR_LEN);
- }
- }
-
+ nxm_put_eth_masked(b, NXM_OF_ETH_SRC, flow->dl_src, cr->wc.dl_src_mask);
+ nxm_put_eth_masked(b, NXM_OF_ETH_DST, flow->dl_dst, cr->wc.dl_dst_mask);
if (!(wc & FWW_DL_TYPE)) {
nxm_put_16(b, NXM_OF_ETH_TYPE,
ofputil_dl_type_to_openflow(flow->dl_type));
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 3f93d9e..e423797 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -1181,12 +1181,12 @@ ofputil_usable_protocols(const struct cls_rule *rule)
BUILD_ASSERT_DECL(FLOW_WC_SEQ == 11);
/* NXM and OF1.1+ supports bitwise matching on ethernet addresses. */
- if (wc->dl_src_mask && !eth_mask_is_exact(wc->dl_src_mask)
- && !eth_addr_is_zero(wc->dl_src_mask)) {
+ if (!eth_mask_is_exact(wc->dl_src_mask)
+ && !eth_addr_is_zero(wc->dl_src_mask)) {
return OFPUTIL_P_NXM_ANY;
}
- if (wc->dl_dst_mask && !eth_mask_is_exact(wc->dl_dst_mask)
- && !eth_addr_is_zero(wc->dl_dst_mask)) {
+ if (!eth_mask_is_exact(wc->dl_dst_mask)
+ && !eth_addr_is_zero(wc->dl_dst_mask)) {
return OFPUTIL_P_NXM_ANY;
}
@@ -3867,10 +3867,6 @@ ofputil_normalize_rule(struct cls_rule *rule)
wc.nd_target_mask = in6addr_any;
}
- /* Ethernet can always be matched. */
- memcpy(wc.dl_src_mask, rule->wc.dl_src_mask, ETH_ADDR_LEN);
- memcpy(wc.dl_dst_mask, rule->wc.dl_dst_mask, ETH_ADDR_LEN);
-
/* Log any changes. */
if (!flow_wildcards_equal(&wc, &rule->wc)) {
bool log = !VLOG_DROP_INFO(&bad_ofmsg_rl);
diff --git a/tests/ovs-ofctl.at b/tests/ovs-ofctl.at
index 8fea809..9cface8 100644
--- a/tests/ovs-ofctl.at
+++ b/tests/ovs-ofctl.at
@@ -230,6 +230,7 @@ NXM_OF_ETH_DST_W(000000000000/010000000000)
NXM_OF_ETH_DST_W(ffffffffffff/010000000000)
NXM_OF_ETH_DST_W(0002e30f80a4/ffffffffffff)
NXM_OF_ETH_DST_W(0002e30f80a4/feffffffffff)
+NXM_OF_ETH_DST_W(60175619848f/5a5a5a5a5a5a)
# eth src
NXM_OF_ETH_SRC(020898456ddb)
@@ -429,9 +430,12 @@ NXM_OF_ETH_DST_W(000000000000/010000000000)
NXM_OF_ETH_DST_W(010000000000/010000000000)
NXM_OF_ETH_DST(0002e30f80a4)
NXM_OF_ETH_DST_W(0002e30f80a4/feffffffffff)
+NXM_OF_ETH_DST_W(40125218000a/5a5a5a5a5a5a)
# eth src
NXM_OF_ETH_SRC(020898456ddb)
+NXM_OF_ETH_SRC_W(012345014545/ffffff555555)
+NXM_OF_ETH_SRC(020898456ddb)
# eth type
NXM_OF_ETH_TYPE(0800)
More information about the dev
mailing list