New checksum APIs in v1model; better alias analysis in side-effects; … #858
Conversation
…removed some hacks from bmv2 backend
|
This PR simplifies the way checksums are computed and verified in v1model by using an extern function for each API; no extern objects exist anymore. This is much closer to the way BMv2 (and P4-14) handle checksums, so translation is much simpler in both directions. This new API also makes it much easier to give good error messages when users misuse the verify/update checksum controls. The better alias analysis was needed so that the compiler does not mess up these nice extern function calls. On the positive side, a bunch of relatively hacky code to handle checksums in the BMv2 back-end has been completely removed. |
| /// Given a struct S { bit a; bit b; } and a variable S x; | ||
| /// a path can be x.a, or just x. An array index is represented as a | ||
| /// number (encoded as a string) or as "*", denoting an unknown index. | ||
| struct LocationPath : public IHasDbPrint { |
There was a problem hiding this comment.
Would have been nice to make the changes to this pass in a separate PR
There was a problem hiding this comment.
yes, but this PR would depend on that other one for being merged first.
p4include/v1model.p4
Outdated
| @param data Data whose checksum is verified. | ||
| @param checksum Expected checksum of the data. | ||
| */ | ||
| extern void verify_checksum<T>(in bool condition, in T data, in bit<16> checksum); |
There was a problem hiding this comment.
is there a reason to limit these new extern methods to the "csum16" algorithm?
why not support verify_checksum<T, U>(in bool condition, in T data, in U checksum, in HashAlgorithm algo)?
There was a problem hiding this comment.
No, but currently this is the only algorithm supported - at least in the back-end, so I didn't want to make the API more complicated than necessary.
There was a problem hiding this comment.
I think that was my question. Is there a reason not to support the other algorithms in the backend? It looks like you would just need to map the enum value to the algo name and call createCalculation with that name as the first argument. It may be worth it to add support now rather than modify v1model.p4 in a non-backward compatible fashion in the future.
There was a problem hiding this comment.
I will amend the PR with support for an algorithm parameter.
Only csum16 will be supported initially.
p4include/v1model.p4
Outdated
| T must be a list expression where all the fields are bit-fields or varbits | ||
| and where the total dynamic length of the fields is a multiple of 16 bits. | ||
| If this method detects that a checksum of the data is not correct it | ||
| sets an internal error flag. |
There was a problem hiding this comment.
isn't the plan to expose this information as standard_metadata which can be consumed in ingress?
There was a problem hiding this comment.
Yes, but that will be a separate PR.
|
@antoninbas : this additional commit adds the Algorithm argument to the two checksum methods. |
|
@mbudiu-vmw I believe that the destination field's type need to be a generic type (e.g. needs to be |
|
I hope I have addressed all of @antoninbas comments. |
…ffects; … (p4lang#858)" This reverts commit f09a95d. This commit breaks all existing programs using checksums. This is a change that needs to be staged more carefully, announced on the mailing list and deprecate the old way in a more graceful manner, giving people the chance of transitioning their programs.
…ffects; … (#858)" This reverts commit f09a95d. This commit breaks all existing programs using checksums. This is a change that needs to be staged more carefully, announced on the mailing list and deprecate the old way in a more graceful manner, giving people the chance of transitioning their programs.
p4lang#858) - keep old Checksum16 available, marked as @deprecated
…removed some hacks from bmv2 backend which were related to checksums
This patch still does not generate JSON code for verifying checksum, but it should be very easy to add that capability now. There is a whole set of issues that should be addressed by this PR; I will list them separately.