Can't add more than one verify_checksum in Checksum Verification block

[sergio-gimenez]

I want to add checksum verification for two different headers. Here it is the code in the Checksum Verification block of my bmv2 simple switch:

/*************************************************************************
************   C H E C K S U M    V E R I F I C A T I O N   *************
*************************************************************************/

control MyVerifyChecksum(inout headers hdr,
                        inout metadata meta) {
    apply {

/*
* Verify checksum for EFCP packets
*/
            verify_checksum(hdr.efcp.isValid(),
            { hdr.efcp.vers,
	          hdr.efcp.dstAddr,
              hdr.efcp.srcAddr,
              hdr.efcp.qosID,
              hdr.efcp.dstCEPID,
              hdr.efcp.srcCEPID,
              hdr.efcp.pduType,
              hdr.efcp.flags,
              hdr.efcp.length,
              hdr.efcp.seqnum },
            hdr.efcp.hdrChecksum,
            HashAlgorithm.csum16);

/*
* Verify checksum for IPv4 packets
*/

            verify_checksum(hdr.ipv4.isValid(),
            { hdr.ipv4.version,
	          hdr.ipv4.ihl,
              hdr.ipv4.diffserv,
              hdr.ipv4.totalLen,
              hdr.ipv4.identification,
              hdr.ipv4.flags,
              hdr.ipv4.fragOffset,
              hdr.ipv4.ttl,
              hdr.ipv4.protocol,
              hdr.ipv4.hdrChecksum,
              hdr.ipv4.srcAddr,
              hdr.ipv4.dstAddr
              },
            hdr.ipv4.hdrChecksum,
            HashAlgorithm.csum16);

    }
}

This is the compiler output that I have:

[--Werror=legacy] error: AssignmentStatement: Only calls to verify_checksum or verify_checksum_with_payload allowed

I think it should be no problem for calling verify_checksum more than one time, but actually the compiler does not allow me to do it. I am missing something?

Thank you in advance

[jafingerhut]

Are you able to publicly share the complete P4 program for which you got this error? e.g. attach it as a file to a comment on this issue?

Also if you could share the version of p4c you are using (the output of the command 'p4c --version'), that might help diagnose it.

[sergio-gimenez]

The version of p4c I am currently using is p4c 1.1.0-rc1 (SHA: 61409c8)

Sure, here is the complete P4 program.

[jafingerhut]

I can confirm that with the latest version of p4c as of today, built from this version of the source code:

commit 474ea783d2adf41c1b424e04cb0dc1981ce4b124 (HEAD -> master, origin/master, origin/HEAD)
Author: Mihai Budiu <mbudiu@vmware.com>
Date:   Wed Oct 9 17:59:46 2019 -0700

the program attached to this comment gives the following error:

$ p4c --target bmv2 --arch v1model efcpRouter.p4
[--Werror=legacy] error: AssignmentStatement: Only calls to verify_checksum or verify_checksum_with_payload allowed

and it does not give an error when even only the second of the two verify_checksum calls is present (the one for the IPv4 header), so the root cause of the issue is not two verify_checksum calls present.

Instead, it is something about the remaining verify_checksum call in the attached program that causes the compiler front end pass named 'SideEffectOrdering' to turn the verify_checksum call into a sequence of two assignments, then verify_checksum, then another assignment afterwards, plus ...

A change made to the p4c v1model back end, I believe in March 2019 (this PR: #1769), that rejects programs that do anything except verify_checksum calls in the verifyChecksum control of a v1model program.

That March 2019 change is desirable and necessary for simple_switch to correctly interpret the BMV2 JSON file output by the compiler, so the real question is: why does the compiler introduce the assignment statements during the SideEffectOrdering pass, and is there a way to prevent it from doing that, even if only for the verifyChecksum and perhaps also the updateChecksum controls in the v1model architecture?

efcpRouter.p4.txt

[jafingerhut]

Additional question: what is different about the two verify_checksum calls (see the commented-out one in the verifyChecksum control -- only one is commented out, and does not cause the problem by itself), such that the first one by itself does not cause the SideEffectORdering pass to introduce assignment statements, but the second one by itself does?

I am a little surprised that there are not other existing test programs that make the same, or nearly the same, verify_checksum call on an IPv4 header already. If so, what is different about this program that causes the error?

Regarding the second question, I just checked and found test program p4c/testdata/p4_16_samples/issue270-bmv2.p4 that has a similar verify_checksum call on an IPv4 header defined with similar sized header fields, and in fact two such verify_checksum calls, and it compiles without error. Weird.

[jafingerhut]

I think I have got the root cause. The call to verify_checksum that is causing the problem has the field hdr.ipv4.hdrChecksum in the list of fields over which to calculate the checksum, and is also the field where the correct checksum to compare against is found (and it is declared direction inout -- more on that below).

Most P4 programs leave the packet header checksum out of the list of fields over which to calculate the checksum. There is no reason for it to be there in a correct program. If you remove that field from the field list, the compilation error goes away.

For P4 developers, though, doesn't it seem a bit odd that the verify_checksum extern is defined this way?

extern void verify_checksum<T, O>(in bool condition, in T data, inout O checksum, HashAlgorithm algo);

In particular, why is the checksum argument direction inout? That makes sense for update_checksum, but if verify_checksum only reads packet header fields, there is no reason for it to write any of them.

[mihaibudiu]

Yes, perhaps the checksum should be only "in", and then the side-effects pass won't make that change.

[sergio-gimenez]

I think I have got the root cause. The call to verify_checksum that is causing the problem has the field hdr.ipv4.hdrChecksum in the list of fields over which to calculate the checksum, and is also the field where the correct checksum to compare against is found (and it is declared direction inout -- more on that below).

Most P4 programs leave the packet header checksum out of the list of fields over which to calculate the checksum. There is no reason for it to be there in a correct program. If you remove that field from the field list, the compilation error goes away.

Yes, taking off the hdr.ipv4.hdrChecksum from the list worked. Sorry for missing that.

Also very thankful to your fast and complete response. Really apreciate it.

[jafingerhut]

@mbudiu-vmw @antoninbas I have read through the implementation of verify_checksum in behavioral-model code for the v1model architecture, and as far as I can tell it never modifies anything except the architecture-internal state of an error status bit. (That architecture-internal state is used just before ingress processing begins to choose what value to assign to the checksum_error field in v1model standard metadata.)

If that is all correct, then it seems like it would be a correct change to replace inout with in in the parameter list for the verify_checksum function. I don't see it as any kind of high priority to make such a change, but it could be done.

[jafingerhut]

If such an inout -> in direction change were made to the checksum parameter of verify_checksum, it would be good for consistency to do the same for function verify_checksum_with_payload.

It would also seem reasonable to change the signature of the VerifyChecksum control from the current:

control VerifyChecksum<H, M>(inout H hdr,
                             inout M meta);

to replace both inout directions with in, given that v1model only allows calls to verify_checksum and verify_checksum_with_payload inside of it, so it cannot modify anything except the architecture-internal state that remembers whether a checksum error was discovered.

[mihaibudiu]

If we make this change this will break all user programs that are written with the old prototype.
Users will have to edit these programs manually.
Also, the same program will not compile with different compiler versions.

[mihaibudiu]

I.e., different v1model header versions.

[jafingerhut]

Making the change I mention above to the VerifyChecksum control signature would definitely cause all v1model programs written for the current v1model.p4 include file to no longer compile, agreed, and I am definitely not here pushing for such a change. I only mentioned it as an observation of what would become possible, if the changes to the verify_checksum and verify_checksum_with_payload signatures were changed.

Changing only the signatures of verify_checksum and verify_checksum_with_payload should not break anyone's v1model programs, unless I am missing something.