Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make some v1model standard_metadata fields inaccessible from P4 program #1845

Closed
jafingerhut opened this issue Apr 7, 2019 · 5 comments
Closed
Labels
enhancement This topic discusses an improvement to existing compiler code.

Comments

@jafingerhut
Copy link
Contributor

This issue is a separate follow-up issue related to #1828 which was primarily concerned with changing mark_to_drop(). That one can be closed when mark_to_drop() is fixed. This one goes slightly further in cleaning up v1model implementation details.

The following fields are modified by one of the v1model operations:

  • clone_spec - modified by clone and clone3 operations
  • lf_field_list - modified by digest operations
  • resubmit_flag - modified by resubmit operations
  • recirculate_flag - modified by recirculate operations

As documented here: https://github.com/p4lang/behavioral-model/blob/master/docs/simple_switch.md they should never be assigned a value by the user's P4 program, and the only possible reason to read them is for debug purposes.

Given the comments raised in #1828 it would fit the P4_16 language semantics better if a change was made so that the v1model operations listed above no longer access these fields that are visible to the user's P4 program, without being explicit inout parameters to those extern functions.

Here are some reasonable choices:

(a) Remove these fields from the standard_metadata struct, so they are no longer accessible to a user's P4 program. They will still be used by the BMv2 implementation in order to implement the extern functions that modify them today.

(b) Add the standard_metadata struct as an inout direction parameter to the 5 extern functions clone, clone3, digest, resubmit, recirculate.

I believe @antoninbas has suggested option (a), and likely considers that preferable over option (b). Option (a) has the advantage of not requiring any changes to the signatures of those 5 extern functions, or stated another way, option (b) has the disadvantage of changing the signatures of those 5 extern functions, thus requiring changes to any P4_16+v1model program that uses those extern functions.

@mihaibudiu
Copy link
Contributor

The more you can hide, the better.
Unfortunately just adding an argument to resubmit won't solve the problem that resubmit does not work correctly, so it is not a complete solution.

@jafingerhut
Copy link
Contributor Author

Agreed that the changes proposed here do nothing to address issue #1669

There is already an issue for that.

This issue is just another part of getting to recirculate/resubmit/clone implementations that are completely aligned with the P4_16 language specification.

@mihaibudiu mihaibudiu added the enhancement This topic discusses an improvement to existing compiler code. label Apr 18, 2019
@jafingerhut
Copy link
Contributor Author

Changes have recently been committed to BMv2 simple_switch so that it has no need whatsoever for the following fields to be defined in v1model.p4's standard_metadata struct (at least within a few hours, after the automated systems have updated the version of simple_switch used by p4c's automated tests): p4lang/behavioral-model@7cd8d68

Thus option (a) in the original issue is completely feasible. We can simply remove this 4 fields from v1model standard_metadata, and as long as your simple_switch version has that commit, it should work as well as it does with those fields present in the standard_metadata struct.

Note that this change to simple_switch does not require an immediate change to p4c or v1model.p4. If those 4 fields are present in the standard_metadata struct, they will simply be ignored by simple_switch.

@jafingerhut
Copy link
Contributor Author

Those fields can also be removed from any and all P4_14 test programs that define them in intrinsic_metadata, and there are a handful of P4_16 programs that mention them for "debug table" purposes where they could be removed.

@jafingerhut
Copy link
Contributor Author

Closing this issue, as the v1model standard_metadata fields mentioned in the original issue, and 2 more that were never used by simple_switch, were removed from p4c with the merge of this PR: #1911

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This topic discusses an improvement to existing compiler code.
Projects
None yet
Development

No branches or pull requests

2 participants