[ovs-dev] [generic tci mask 8/8] nx-match: Implement support for arbitrary VLAN TCI masks.

[Ben Pfaff]

On Mon, Nov 22, 2010 at 06:49:07PM -0800, Justin Pettit wrote:
> On Nov 10, 2010, at 3:38 PM, Ben Pfaff wrote:
> 
> > +/* Modifies 'rule' so that the VLAN VID is wildcarded.  If the PCP is already
> > + * wildcarded, then 'rule' will match a packet regardless of whether it has an
> > + * 802.1Q header or not. */
> > +void
> > +cls_rule_set_any_vid(struct cls_rule *rule)
> > +{
> > +    if (rule->wc.vlan_tci_mask & htons(VLAN_PCP_MASK)) {
> > +        rule->wc.vlan_tci_mask &= htons(VLAN_VID_MASK);
> > +        rule->flow.vlan_tci &= htons(VLAN_VID_MASK);
> 
> Shouldn't these masks be inverted?

Right, thanks.

> > +/* Modifies 'rule' so that the VLAN PCP is wildcarded.  If the VID is already
> > + * wildcarded, then 'rule' will match a packet regardless of whether it has an
> > + * 802.1Q header or not. */
> > +void
> > +cls_rule_set_any_pcp(struct cls_rule *rule)
> > +{
> > +    if (rule->wc.vlan_tci_mask & htons(VLAN_VID_MASK)) {
> > +        rule->wc.vlan_tci_mask &= htons(VLAN_PCP_MASK);
> > +        rule->flow.vlan_tci &= htons(VLAN_PCP_MASK);
> 
> Same here.

Ditto.

> > @@ -99,7 +98,6 @@ typedef unsigned int OVS_BITWISE flow_wildcards_t;
> > 
> > /* Same values and meanings as corresponding OFPFW_* bits. */
> > #define FWW_IN_PORT     ((OVS_FORCE flow_wildcards_t) (1 << 0))
> > -#define FWW_DL_VLAN     ((OVS_FORCE flow_wildcards_t) (1 << 1))
> > #define FWW_DL_SRC      ((OVS_FORCE flow_wildcards_t) (1 << 2))
> > #define FWW_DL_DST      ((OVS_FORCE flow_wildcards_t) (1 << 3))
> >                                               /* excluding the multicast bit */
> > @@ -108,14 +106,14 @@ typedef unsigned int OVS_BITWISE flow_wildcards_t;
> > #define FWW_TP_SRC      ((OVS_FORCE flow_wildcards_t) (1 << 6))
> > #define FWW_TP_DST      ((OVS_FORCE flow_wildcards_t) (1 << 7))
> > /* Same meanings as corresponding OFPFW_* bits, but differ in value. */
> > -#define FWW_DL_VLAN_PCP ((OVS_FORCE flow_wildcards_t) (1 << 8))
> > -#define FWW_NW_TOS      ((OVS_FORCE flow_wildcards_t) (1 << 9))
> > +#define FWW_NW_TOS      ((OVS_FORCE flow_wildcards_t) (1 << 1))
> > /* No OFPFW_* bits, but they do have corresponding OVSFW_* bits. */
> > -#define FWW_TUN_ID      ((OVS_FORCE flow_wildcards_t) (1 << 10))
> > +#define FWW_TUN_ID      ((OVS_FORCE flow_wildcards_t) (1 << 8))
> > /* No corresponding OFPFW_* or OVSFW_* bits. */
> > -#define FWW_ETH_MCAST   ((OVS_FORCE flow_wildcards_t) (1 << 11))
> > +#define FWW_VLAN_TCI    ((OVS_FORCE flow_wildcards_t) (1 << 9)
> > +#define FWW_ETH_MCAST   ((OVS_FORCE flow_wildcards_t) (1 << 10))
> >                                                        /* multicast bit only */
> > -#define FWW_ALL         ((OVS_FORCE flow_wildcards_t) (((1 << 12)) - 1))
> > +#define FWW_ALL         ((OVS_FORCE flow_wildcards_t) (((1 << 11)) - 1))
> 
> I think trying to keep the OpenFlow wildcard bits somewhat in line
> with the OVS ones is a bit confusing.  I understand it saves bit
> manipulation for the ones that are in common, but I think this
> divergence is only going to get worse.

I guess I see maintaining this correspondence as trivial.  And if it
gets worse, we just don't do it anymore.

> >     case NFI_NXM_OF_VLAN_TCI_W:
> > -        return parse_tci(rule, get_unaligned_u16(value),
> > -                         get_unaligned_u16(mask));
> > +        if (wc->vlan_tci_mask) {
> > +            return NXM_DUP_TYPE;
> > +        } else {
> > +            cls_rule_set_dl_tci_masked(rule, get_unaligned_u16(value),
> > +                                       get_unaligned_u16(mask));
> > +            return 0;
> > +        }
> 
> In the previous version, a NXM_INVALID would be sent if an invalid
> mask/value combination was specified (e.g., an exact match with a
> nonzero TCI value with CFI=0).  Is there a reason not to do those
> sanity checks still?

Well, the old code was a special case, and the new code is general.  The
other fields don't apply that kind of logic, so it seems cleaner not to
apply it here either.

> > +    /* Translate VLANs. */
> > +    vid = match->dl_vlan & htons(VLAN_VID_MASK);
> > +    pcp = htons((match->dl_vlan_pcp << VLAN_PCP_SHIFT) & VLAN_PCP_MASK);
> > +    switch (ofpfw & (OFPFW_DL_VLAN | OFPFW_DL_VLAN_PCP)) {
> > ...
> > +    case OFPFW_DL_VLAN_PCP:
> > +        if (match->dl_vlan == htons(OFP_VLAN_NONE)) {
> > +            /* Match only packets without 802.1Q header. */
> > +            rule->flow.vlan_tci = htons(0);
> > +            rule->wc.vlan_tci_mask = htons(0xffff);
> > ...
> > +    case 0:
> > +        /* Specific VID and PCP. */
> > +        rule->flow.vlan_tci = vid | pcp | htons(VLAN_CFI);
> > +        rule->wc.vlan_tci_mask = htons(0xffff);
> > +        break;
> > +    }
> 
> Should you special-case the OFP_VLAN_NONE for case "0", so that it
> doesn't set the VLAN_CFI bit?  If not, it seems inconsistent with the
> definition in nicira-ext.h and the behavior for the OFPFW_DL_VLAN_PCP
> case.

Good spotting, thanks.  I fixed this up.