PSA - define what P4_16 lists mean when passed as arguments to certain externs

[jafingerhut]

... and what types of values are permitted inside of those lists.

For example, there are code examples that demonstrate calling the InternetChecksum add() method with a P4_16 list of values, each of which is type bit<W>. The assumption when I wrote that example, and I think the way that most people interpret it, is that all of those individual bit<W> values are concatenated together, with no alignment or padding done by the extern. It would be good to explicitly state that, if that is the intended behavior. For the InternetChecksum in particular, most of its intended use cases are made worthless if an implementation can add extra bits to those given in the list.

What types of values should be allowed in such lists?

• bit<W> definitely
• int<W> and serializable enum enum bit<W> ... types probably, and should be treated as if cast to the corresponding same-width bit<W> type.
• headers? If so, what should the behavior be? One answer that seems close to what P4_14 allows is to treat a header as expanded into its list of fields, ignoring the validity bit. If the header is invalid, then the header field values are undefined, and the P4 programmer should avoid doing this if they want the result to be predictable and portable.
• header stacks?
• header_union? Allowing this and getting predictable portable results out as a result of a checksum calculation needs precise definition, e.g. if no members are valid, then it is like the invalid header case above. If one member is valid, then only the fields of that valid member are included in the checksum calculation, which no alignment or padding. This could be tricky for implementations to do in that way, if different header_union members have different sizes. I am perfectly fine saying that such list elements need not be supported by an implementation.
• header_union stacks? The combination of fun of the previous two cases.

The same or similar answers could apply for the Hash extern, although for most uses of the Hash extern I am aware of, portability of results are not often required or expected, e.g. for LAG or ECMP load balancing. Thus the restriction of no alignment and no padding could perhaps be relaxed here.

[jafingerhut]

The attached program is P4_16 + v1model, not PSA, but is being added at the request of Hemant Singh, who was curious to see it. It demonstrates that with the latest p4c as of 2019-Mar-24, it fails to compile calls to update_checksum() in v1model's updateChecksum control.

I do not intend this program to greatly affect the decision on the issue above, unless this example demonstrates some very subtle or expensive-to-change aspect of p4c's behavior that should affect the decision. The example is intended to point out an aspect of today's p4c implementation that would need to be changed if we do wish to support values of type header in a P4_16 list argument to a v1model verify_checksum or update_checksum call in the limited-functionality controls where v1model supports those.

I believe that the corresponding extern calls in PSA are expected to be made inside of ingress and egress controls. PSA does not have separate controls limited to making only calls to checksum verify/update functions. Thus this example should perhaps have little or no bearing on a PSA implementation.

Version of p4c source code used:

$ git log -n 1 | head -n 3
commit fe90bb971002bd41a6aa694eaef41195e185d5c1
Author: Alex Wong <[email protected]>
Date:   Sat Mar 23 14:34:15 2019 -0400

Commands I ran to compile and get extra debug output to show what is going on in one compiler intermediate step:

$ mkdir -p tmp
$ p4c --target bmv2 --arch v1model --dump tmp --top4 FrontEndLast checksum-header.p4
[--Werror=legacy] error: AssignmentStatement: Only calls to update_checksum or update_checksum_with_payload allowed

Here is the original contents of the updateChecksum control in the input P4 program:

control updateChecksum(inout headers_t hdr, inout metadata_t meta)
{
    apply {
        update_checksum(hdr.tcp.isValid(),
            {
                hdr.ipv4.srcAddr,
                hdr.ipv4.dstAddr,
                hdr.tcp
            },
            hdr.tcp.checksum, HashAlgorithm.csum16);
    }
}

and here is how it looks after the last step of p4c's front end:

control updateChecksum(inout headers_t hdr, inout metadata_t meta) {
    tuple<bit<32>, bit<32>, tcp_t> tmp;
    bit<16> tmp_0;
    apply {
        tmp = { hdr.ipv4.srcAddr, hdr.ipv4.dstAddr, hdr.tcp };
        tmp_0 = hdr.tcp.checksum;
        update_checksum<tuple<bit<32>, bit<32>, tcp_t>, bit<16>>(hdr.tcp.isValid(), tmp, tmp_0, HashAlgorithm.csum16);
        hdr.tcp.checksum = tmp_0;
    }
}

The resulting code fails in the p4c bmv2 back end for the v1model architecture, I believe because it does not support any statements except calls to update_checksum() and update_checksum_with_payload().

checksum-header.p4.txt

[hesingh]

The transformation by p4c frontend for the update_checksum extern is correct. If one looks at the API for update_checksum in p4c/p4include/v1model.p4, the T for data is defined as

@param T          Must be a tuple type where all the fields are bit-fields or varbits.
                  The total dynamic length of the fields is a multiple of the output size.

The transformation by the frontend has changed the data input to a tuple.

In summary, I think we will have to discuss in the LDWG to see if externs should be extended to support a list.

[jafingerhut]

Externs today already support lists, e.g. update_checksum and verify_checksum support lists of values of type bit<W> already. I do not think anyone in the LDWG will argue against that.

The issue with that particular example P4_16 program for the v1model architecture is some transformations made by passes in p4c, that end up violating the limitations imposed by the v1model back end code on what it supports inside of an updateChecksum control.

If people look at that program and the current v1model.p4 docs and say "Yep, that is a limitation we want to keep", then no changes are needed, although a clearer error message would be nice. Even a clearer error message would likely require surgery on the passes of the compiler, though, at least for the ones that are permitted to be used on the updateChecksum control.

[hesingh]

Ok, got it. I agree the error message could be better.