This will still change the user's intent (and change the flows that the
user sets up), breaking some of the forms described in meta-flow.h.
This is closer to what I had in mind:
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 71b8bef..41a03ef 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5033,6 +5033,12 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
*xout)
wc->masks.tp_src &= htons(UINT8_MAX);
wc->masks.tp_dst &= htons(UINT8_MAX);
}
+
+ /* When there is a VLAN, the VLAN match must always include match on
+ * "present" bit. */
+ if (flow->vlan_tci && wc->masks.vlan_tci) {
+ wc->masks.vlan_tci |= htons(VLAN_CFI);
+ }
}
}
However, this still doesn't fix every case, because of the cases like
the vlan_tci=0x000a/0xfff you mention, where "unwildcarding" the CFI bit
still results in an exact-match on a 0-bit, which the kernel will
reject. The most obvious place to fix that up is in the translation to
the kernel flow key. I think that we could do that in
odp_flow_key_from_flow__(). Do you think that would work out?
On Fri, Jun 05, 2015 at 11:03:53AM -0700, Alex Wang wrote:
> How about this?
>
> diff --git a/lib/match.c b/lib/match.c
> index 7d0b409..e34572d 100644
> --- a/lib/match.c
> +++ b/lib/match.c
> @@ -360,6 +360,10 @@ match_set_dl_tci(struct match *match, ovs_be16 tci)
> void
> match_set_dl_tci_masked(struct match *match, ovs_be16 tci, ovs_be16 mask)
> {
> + if (mask && mask != htons(0xffff)) {
> + tci |= htons(VLAN_CFI);
> + mask |= htons(VLAN_CFI);
> + }
> match->flow.vlan_tci = tci & mask;
> match->wc.masks.vlan_tci = mask;
> }
>
>
> On Fri, Jun 5, 2015 at 9:24 AM, Alex Wang <al...@nicira.com> wrote:
>
> > Thx for the reference, exactly what i want,
> >
> > Will change the solution~
> >
> > Thanks,
> > Alex Wang,
> >
> > On Fri, Jun 5, 2015 at 8:24 AM, Ben Pfaff <b...@nicira.com> wrote:
> >
> >> On Wed, Jun 03, 2015 at 11:21:50PM -0700, Alex Wang wrote:
> >> > OVS datapath has check which prevents the installation of flow
> >> > that matches VLAN TCI but does not have exact match for VLAN_CFI
> >> > bit. However, the ovs userspace does not enforce it, so OpenFlow
> >> > flow like "vlan_tci=0x000a/0x0fff,action=output:local" can be added
> >> > to ovs. Subsequently, the generated megaflow will have match
> >> > field for vlan like "vlan(vid=5/0xfff,pcp=0/0x0,cfi=1/0)".
> >> >
> >> > With the OVS datapath check, the installation of such megaflow
> >> > will be rejected with:
> >> > "|WARN|system@ovs-system: failed to put[create][modify] (Invalid
> >> argument)"
> >> >
> >> > This commit adds a check in userspace that mark the vlan mask
> >> > invalid if it does not exact match for VLAN_CFI. So users will
> >> > be asked to provide correct mask.
> >>
> >> This is not the right fix, because it is legitimate and useful not to
> >> match on the "CFI" (actually "vlan present") bit in OpenFlow. See the
> >> comment in meta-flow.h:
> >>
> >> /* "vlan_tci".
> >> *
> >> * 802.1Q TCI.
> >> *
> >> * For a packet with an 802.1Q header, this is the Tag Control
> >> Information
> >> * (TCI) field, with the CFI bit forced to 1. For a packet with no
> >> 802.1Q
> >> * header, this has value 0.
> >> *
> >> * This field can be used in various ways:
> >> *
> >> * - If it is not constrained at all, the nx_match matches packets
> >> * without an 802.1Q header or with an 802.1Q header that has any
> >> TCI
> >> * value.
> >> *
> >> * - Testing for an exact match with 0 matches only packets without
> >> an
> >> * 802.1Q header.
> >> *
> >> * - Testing for an exact match with a TCI value with CFI=1 matches
> >> * packets that have an 802.1Q header with a specified VID and
> >> PCP.
> >> *
> >> * - Testing for an exact match with a nonzero TCI value with CFI=0
> >> does
> >> * not make sense. The switch may reject this combination.
> >> *
> >> * - Testing with a specific VID and CFI=1, with nxm_mask=0x1fff,
> >> matches
> >> * packets that have an 802.1Q header with that VID (and any PCP).
> >> *
> >> * - Testing with a specific PCP and CFI=1, with nxm_mask=0xf000,
> >> matches
> >> * packets that have an 802.1Q header with that PCP (and any VID).
> >> *
> >> * - Testing with nxm_value=0, nxm_mask=0x0fff matches packets with
> >> no
> >> * 802.1Q header or with an 802.1Q header with a VID of 0.
> >> *
> >> * - Testing with nxm_value=0, nxm_mask=0xe000 matches packets with
> >> no
> >> * 802.1Q header or with an 802.1Q header with a PCP of 0.
> >> *
> >> * - Testing with nxm_value=0, nxm_mask=0xefff matches packets with
> >> no
> >> * 802.1Q header or with an 802.1Q header with both VID and PCP
> >> of 0.
> >> *
> >> * Type: be16.
> >> * Maskable: bitwise.
> >> * Formatting: hexadecimal.
> >> * Prerequisites: none.
> >> * Access: read/write.
> >> * NXM: NXM_OF_VLAN_TCI(4) since v1.1.
> >> * OXM: none.
> >> * OF1.0: exact match.
> >> * OF1.1: exact match.
> >> */
> >> MFF_VLAN_TCI,
> >>
> >> I think that we should fix this in flow translation, by "unwildcarding"
> >> the CFI bit if any other bits in vlan_tci are unwildcarded.
> >>
> >
> >
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
