p4lang / p4-applications

P4 Applications WG repo
107 stars 46 forks source link

Unclear intention on DSCP value match #50

Closed ederollora closed 6 years ago

ederollora commented 6 years ago

We have been recently going through the INT spec and checking the "P4 program specification for INT Transit" (page 20) of the latest spec. Looks like the value 0x17 is being used to identify the presence of INT headers in the DSCP field of the ip header. In order to parse the INT shim header, the program states a this (page 27):

/* indicate INT at LSB of DSCP */
const bit<6> DSCP_INT = 0x17;

/*...*/

state parse_tcp {
    packet.extract(hdr.tcp);
    transition select(hdr.ipv4.dscp) {
        /* &&& is a mask operator in p4_16 */
        DSCP_INT &&& DSCP_INT: parse_intl4_shim;
        default: accept;
    }
}

After some tests we have seen that DSCP_INT &&& DSCP_INT might allow matching against 4 different values (0x17, 0x1F, 0x37 and 0x3F). We would like to know if matching against 4 different value codepoints was intended or not.

Thanks!

jklr commented 6 years ago

While the reference code at the end of the spec can serve as a canonical representation of INT header format, the implementation of protocol logic and the setting of domain-specific parameters do not. Which DSCP value(s) or bit to use to indicate INT in a production system is per-INT-domain decision. Effectively half or more of DSCP space should be allocated for INT. Please check section 4.6.1 of the spec.

As for the example, the code intends to mask hdr.ipv4.dscp by DSCP_INT (as bitmask) and match the masked value with DSCP_INT (as value), equivalent to C-style if ((hdr.ipv4.dscp & DSCP_INT) == DSCP_INT) It should match only with 0x17. Did you run it on a PSA-compatible backend?

jklr commented 6 years ago

The comment ("LSB of DSCP") is out-dated. I will update. Thanks for pointing out.

jklr commented 6 years ago

DSCP_INT used to be one bit, used as both match value and mask. Since the value was changed to match full value, the mask must be 0x3F. Will fix and update. Thanks.

ederollora commented 6 years ago

hi @jklr ! Thanks for comming back, we were just trying to understand the standard and the sample code. We are aware that the sample code is under development and serves an illustrative purpose so we really appreciate your answer. As you pointed out, DSCP_INT used to be one bit so we first thought that keeping DSCP_INT &&& DSCP_INT must have made reference to the previous LSB implementation. Finally, yes, 0x3F should work as the mask.