From 5ff35b28704e90fa6b1ecd931090c8b8ea76e7e0 Mon Sep 17 00:00:00 2001 From: Andy Fingerhut Date: Sat, 11 Jan 2025 21:25:08 +0000 Subject: [PATCH 1/9] Add checks on the contents of the "next_tables" value in tables Signed-off-by: Andy Fingerhut --- include/bm/bm_sim/P4Objects.h | 6 +++ src/bm_sim/P4Objects.cpp | 85 ++++++++++++++++++++++++++++++++--- 2 files changed, 84 insertions(+), 7 deletions(-) diff --git a/include/bm/bm_sim/P4Objects.h b/include/bm/bm_sim/P4Objects.h index f1e6cb337..555a35fef 100644 --- a/include/bm/bm_sim/P4Objects.h +++ b/include/bm/bm_sim/P4Objects.h @@ -59,6 +59,8 @@ class Value; namespace bm { +using std::string; + using ConfigOptionMap = std::unordered_map; class P4Objects { @@ -378,6 +380,10 @@ class P4Objects { void init_meter_arrays(const Json::Value &root, InitState *); void init_register_arrays(const Json::Value &root); void init_actions(const Json::Value &root); + void check_next_nodes(const Json::Value &cfg_next_nodes, + const Json::Value &cfg_actions, + const string table_name, + bool *next_is_hit_miss); void init_pipelines(const Json::Value &root, LookupStructureFactory *, InitState *); void init_checksums(const Json::Value &root); diff --git a/src/bm_sim/P4Objects.cpp b/src/bm_sim/P4Objects.cpp index c17bdcf7b..44851b9ee 100644 --- a/src/bm_sim/P4Objects.cpp +++ b/src/bm_sim/P4Objects.cpp @@ -1575,6 +1575,65 @@ std::vector parse_match_key( } // namespace +void +P4Objects::check_next_nodes(const Json::Value &cfg_next_nodes, + const Json::Value &cfg_actions, + const string table_name, + bool *next_is_hit_miss) { + // For each table, the value of its key "next_tables" must be an + // object with one of the following sets of keys: + // (a) The set of keys must include "__HIT__" and "__MISS__", but no + // others. This is how a P4 table is implemented if, where it + // is applied, it uses "t1.apply().hit" or "t1.apply().miss" + // conditions to control which code is executed next. + // (b) The set of keys must include each action name of the table + // exactly once, but no others. This is how a P4 table is + // implemented if where it is applied, it uses + // "t1.apply().action_run" and a switch statement to control + // which code is executed next. It is also used by the p4c BMv2 + // backend for tables that use none of .hit, .miss, and + // .action_run, and always execute the same code next regardless + // of hit, miss, or which action the table executed. In that + // case, every action will have the same next node to execute + // regardless of the action. + int num_next_nodes = 0; + for (const auto &node : cfg_next_nodes) { + (void) node; + num_next_nodes++; + } + bool next_has_hit = cfg_next_nodes.isMember("__HIT__"); + bool next_has_miss = cfg_next_nodes.isMember("__MISS__"); + if (next_has_hit || next_has_miss) { + if (next_has_hit && next_has_miss && (num_next_nodes == 2)) { + *next_is_hit_miss = true; + } else { + throw json_exception( + EFormat() << "Table '" << table_name << "' has one" + << " of keys '__HIT__' and '__MISS__' in 'next_tables'" + << " but either it does not have both of them," + << " or it has other keys that should not be there.", + cfg_next_nodes); + } + } else { + *next_is_hit_miss = false; + int num_actions = 0; + for (const auto &cfg_action : cfg_actions) { + (void) cfg_action; + num_actions++; + // The check that each action name is a key in cfg_next_nodes is + // done near where check_next_nodes is called, to avoid + // duplicating here the code that calculates action_name. + } + if (num_next_nodes != num_actions) { + throw json_exception( + EFormat() << "Table '" << table_name << "' should have exactly " + << num_actions << " keys, one for each table action, but found " + << num_next_nodes << "keys.", + cfg_next_nodes); + } + } +} + void P4Objects::init_pipelines(const Json::Value &cfg_root, LookupStructureFactory *lookup_factory, @@ -1818,6 +1877,9 @@ P4Objects::init_pipelines(const Json::Value &cfg_root, std::string actions_key = cfg_table.isMember("action_ids") ? "action_ids" : "actions"; const Json::Value &cfg_actions = cfg_table[actions_key]; + bool next_is_hit_miss = false; + check_next_nodes(cfg_next_nodes, cfg_actions, table_name, + &next_is_hit_miss); for (const auto &cfg_action : cfg_actions) { p4object_id_t action_id = 0; string action_name = ""; @@ -1831,7 +1893,13 @@ P4Objects::init_pipelines(const Json::Value &cfg_root, action = get_one_action_with_name(action_name); assert(action); action_id = action->get_id(); } - + if (!next_is_hit_miss && !cfg_next_nodes.isMember(action_name)) { + throw json_exception( + EFormat() << "Table '" << table_name << "' should have key" + << " for action '" << action_name + << "' in its 'next_tables' object.", + cfg_next_nodes); + } const Json::Value &cfg_next_node = cfg_next_nodes[action_name]; const ControlFlowNode *next_node = get_next_node(cfg_next_node); table->set_next_node(action_id, next_node); @@ -1839,11 +1907,10 @@ P4Objects::init_pipelines(const Json::Value &cfg_root, if (act_prof_name != "") add_action_to_act_prof(act_prof_name, action_name, action); } - - if (cfg_next_nodes.isMember("__HIT__")) - table->set_next_node_hit(get_next_node(cfg_next_nodes["__HIT__"])); - if (cfg_next_nodes.isMember("__MISS__")) - table->set_next_node_miss(get_next_node(cfg_next_nodes["__MISS__"])); + if (next_is_hit_miss) { + table->set_next_node_hit(get_next_node(cfg_next_nodes["__HIT__"])); + table->set_next_node_miss(get_next_node(cfg_next_nodes["__MISS__"])); + } if (cfg_table.isMember("base_default_next")) { table->set_next_node_miss_default( @@ -1951,9 +2018,13 @@ P4Objects::init_pipelines(const Json::Value &cfg_root, auto conditional_name = cfg_conditional["name"].asString(); auto conditional = get_conditional(conditional_name); + if (!cfg_conditional.isMember("true_next") && !cfg_conditional.isMember("false_next")) { + throw json_exception("conditional must have either or both of the" + " keys 'true_next' and 'false_next'.", + cfg_conditional); + } const auto &cfg_true_next = cfg_conditional["true_next"]; const auto &cfg_false_next = cfg_conditional["false_next"]; - if (!cfg_true_next.isNull()) { auto next_node = get_control_node_cfg(cfg_true_next.asString()); conditional->set_next_node_if_true(next_node); From e5c617e88e243e898190f2ba93c06c92088a963f Mon Sep 17 00:00:00 2001 From: Andy Fingerhut Date: Mon, 13 Jan 2025 04:32:48 +0000 Subject: [PATCH 2/9] More notes in JSON_format.md on how p4c creates JSON files Signed-off-by: Andy Fingerhut --- docs/JSON_format.md | 28 +++++++++++++++++++++++++--- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/docs/JSON_format.md b/docs/JSON_format.md index 7d4a8defb..6f76a6d9c 100644 --- a/docs/JSON_format.md +++ b/docs/JSON_format.md @@ -625,7 +625,7 @@ attributes for these objects are: - `actions`: the list of actions (order does not matter) supported by this table - `next_tables`: maps each action to a next table name. Alternatively, maps - special string `__HIT__` and `__MISS__` to a next table name. + special string `__HIT__` and `__MISS__` to a next table name. See Note 3 below. - `direct_meters`: the name of the associated direct meter array, or null if the match table has no associated meter array - `default_entry`: an optional JSON item which can force the default entry for @@ -634,14 +634,14 @@ attributes for these objects are: - `action_id`: the id of the default action - `action_const`: an optional boolean value which is `true` iff the control plane is not allowed to change the default action function. Default value is - `false`. It can only be set to `true` for `simple` tables. + `false`. It can only be set to `true` for `simple` tables. See Note 2 below. - `action_data`: an optional JSON array where each entry is the hexstring value for an action argument. The size of the array needs to match the number of parameters expected by the action function with id `action_id`. - `action_entry_const`: an optional boolean value which is `true` iff the control plane is not allowed to modify the action entry (action function + action data). Default value is `false`. This attribute is ignored if the - `action_data` attribute it missing. + `action_data` attribute it missing. See Note 2 below. - `entries`: enables you to optionally specify match-action entries for this table. Specifying entries in the JSON makes the table immutable, which means the added entries cannot be modified / deleted and that new entries cannot be @@ -685,6 +685,28 @@ a miss every time it is applied, and execute its default action. A dummy table has a const default action that is equal to the action `a` in the original source code that it is replacing. +Note 2: Since May 2017 when [PR +#653](https://github.com/p4lang/p4c/pull/653) was merged into p4c, p4c +has always created tables with the value of `action_entry_const` equal +to `action_const`. They are both true if the `default_action` in the +P4 source code for the table is declared `const`, and both false if +the `default_action` is not declared `const`. + +Note 3: p4c always creates the value of the `next_tables` key in one +of these ways: ++ If you use the P4 constructs `t1.apply().hit` or `t1.apply().miss`, + and use that Boolean value to choose between two execution paths, + e.g. in an `if` statement, then the table's `next_tables` value will + contain the keys `__HIT__` and/or `__MISS__` to specify these two + paths, and no other keys will be present. ++ If you do not use those P4 constructs, then the `next_tables` value + will contain keys equal to the action names of the table. If the P4 + program invokes the table using `switch (t1.apply().action_run)`, + then in general the different action names can specify different + next nodes to execute next, after the table is applied. If you do + not use that construct, then the next node to be executed will be + the same for all actions. + The `match_type` for the table needs to follow the following rules: - If one match field is `range`, the table `match_type` has to be `range` - If one match field is `ternary`, the table `match_type` has to be `ternary` From 545580de8a46768eb5f737ecfe2f01eff51d1fe0 Mon Sep 17 00:00:00 2001 From: Andy Fingerhut Date: Sun, 12 Jan 2025 22:04:19 -0800 Subject: [PATCH 3/9] Fix style check warning. Signed-off-by: Andy Fingerhut --- src/bm_sim/P4Objects.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/bm_sim/P4Objects.cpp b/src/bm_sim/P4Objects.cpp index 44851b9ee..318e14ea4 100644 --- a/src/bm_sim/P4Objects.cpp +++ b/src/bm_sim/P4Objects.cpp @@ -2018,7 +2018,8 @@ P4Objects::init_pipelines(const Json::Value &cfg_root, auto conditional_name = cfg_conditional["name"].asString(); auto conditional = get_conditional(conditional_name); - if (!cfg_conditional.isMember("true_next") && !cfg_conditional.isMember("false_next")) { + if (!cfg_conditional.isMember("true_next") && + !cfg_conditional.isMember("false_next")) { throw json_exception("conditional must have either or both of the" " keys 'true_next' and 'false_next'.", cfg_conditional); From 2834e72cfce8d07473e6425c2be00c6b974c9957 Mon Sep 17 00:00:00 2001 From: Andy Fingerhut Date: Mon, 13 Jan 2025 20:38:35 -0800 Subject: [PATCH 4/9] Address review comments Signed-off-by: Andy Fingerhut --- include/bm/bm_sim/P4Objects.h | 4 +--- src/bm_sim/P4Objects.cpp | 20 ++++++-------------- 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/include/bm/bm_sim/P4Objects.h b/include/bm/bm_sim/P4Objects.h index 555a35fef..b2720fe2f 100644 --- a/include/bm/bm_sim/P4Objects.h +++ b/include/bm/bm_sim/P4Objects.h @@ -59,8 +59,6 @@ class Value; namespace bm { -using std::string; - using ConfigOptionMap = std::unordered_map; class P4Objects { @@ -382,7 +380,7 @@ class P4Objects { void init_actions(const Json::Value &root); void check_next_nodes(const Json::Value &cfg_next_nodes, const Json::Value &cfg_actions, - const string table_name, + const std::string &table_name, bool *next_is_hit_miss); void init_pipelines(const Json::Value &root, LookupStructureFactory *, InitState *); diff --git a/src/bm_sim/P4Objects.cpp b/src/bm_sim/P4Objects.cpp index 318e14ea4..6c3d74410 100644 --- a/src/bm_sim/P4Objects.cpp +++ b/src/bm_sim/P4Objects.cpp @@ -1578,7 +1578,7 @@ std::vector parse_match_key( void P4Objects::check_next_nodes(const Json::Value &cfg_next_nodes, const Json::Value &cfg_actions, - const string table_name, + const std::string &table_name, bool *next_is_hit_miss) { // For each table, the value of its key "next_tables" must be an // object with one of the following sets of keys: @@ -1596,11 +1596,7 @@ P4Objects::check_next_nodes(const Json::Value &cfg_next_nodes, // of hit, miss, or which action the table executed. In that // case, every action will have the same next node to execute // regardless of the action. - int num_next_nodes = 0; - for (const auto &node : cfg_next_nodes) { - (void) node; - num_next_nodes++; - } + int num_next_nodes = cfg_next_nodes.size(); bool next_has_hit = cfg_next_nodes.isMember("__HIT__"); bool next_has_miss = cfg_next_nodes.isMember("__MISS__"); if (next_has_hit || next_has_miss) { @@ -1616,14 +1612,10 @@ P4Objects::check_next_nodes(const Json::Value &cfg_next_nodes, } } else { *next_is_hit_miss = false; - int num_actions = 0; - for (const auto &cfg_action : cfg_actions) { - (void) cfg_action; - num_actions++; - // The check that each action name is a key in cfg_next_nodes is - // done near where check_next_nodes is called, to avoid - // duplicating here the code that calculates action_name. - } + int num_actions = cfg_actions.size(); + // The check that each action name is a key in cfg_next_nodes is + // done near where check_next_nodes is called, to avoid + // duplicating here the code that calculates action_name. if (num_next_nodes != num_actions) { throw json_exception( EFormat() << "Table '" << table_name << "' should have exactly " From c66046309fa55b01b6995fe30017bfc8c97d3f1c Mon Sep 17 00:00:00 2001 From: Andy Fingerhut Date: Mon, 13 Jan 2025 20:48:39 -0800 Subject: [PATCH 5/9] Throw JSON error if table lacks a default_action key, or action_data Signed-off-by: Andy Fingerhut --- src/bm_sim/P4Objects.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/bm_sim/P4Objects.cpp b/src/bm_sim/P4Objects.cpp index 6c3d74410..d11a2c20a 100644 --- a/src/bm_sim/P4Objects.cpp +++ b/src/bm_sim/P4Objects.cpp @@ -1947,7 +1947,17 @@ P4Objects::init_pipelines(const Json::Value &cfg_root, table->set_default_default_entry(action, std::move(adata), is_action_entry_const); + } else { + throw json_exception( + EFormat() << "'default_action' of table '" << table_name + << "' should have key 'action_data'", + cfg_default_entry); } + } else { + throw json_exception( + EFormat() << "Table '" << table_name << "' should have key" + << " 'default_action'", + cfg_table); } // for 'simple' tables, it is possible to specify immutable entries From 8672f5e36c67c8bd6b194dce525c479fb95ee5af Mon Sep 17 00:00:00 2001 From: Andy Fingerhut Date: Wed, 15 Jan 2025 01:46:58 +0000 Subject: [PATCH 6/9] Undo making default_entry a required key for table definitions It requires changing many old unit tests if we require default_entry as a key in all table definitions. Signed-off-by: Andy Fingerhut --- src/bm_sim/P4Objects.cpp | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/bm_sim/P4Objects.cpp b/src/bm_sim/P4Objects.cpp index d11a2c20a..b5116687f 100644 --- a/src/bm_sim/P4Objects.cpp +++ b/src/bm_sim/P4Objects.cpp @@ -1949,15 +1949,10 @@ P4Objects::init_pipelines(const Json::Value &cfg_root, is_action_entry_const); } else { throw json_exception( - EFormat() << "'default_action' of table '" << table_name + EFormat() << "'default_entry' of table '" << table_name << "' should have key 'action_data'", cfg_default_entry); } - } else { - throw json_exception( - EFormat() << "Table '" << table_name << "' should have key" - << " 'default_action'", - cfg_table); } // for 'simple' tables, it is possible to specify immutable entries From 47ce102572e17eab55b181e64c6645a226c2b153 Mon Sep 17 00:00:00 2001 From: Andy Fingerhut Date: Wed, 15 Jan 2025 01:50:50 +0000 Subject: [PATCH 7/9] Added notes that action_id and action_data are required fields ... inside of default_entry value. Signed-off-by: Andy Fingerhut --- docs/JSON_format.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/JSON_format.md b/docs/JSON_format.md index 6f76a6d9c..aa2f72ba1 100644 --- a/docs/JSON_format.md +++ b/docs/JSON_format.md @@ -631,13 +631,15 @@ attributes for these objects are: - `default_entry`: an optional JSON item which can force the default entry for the table to be set when loading the JSON, without intervention from the control plane. It has the following attributes: - - `action_id`: the id of the default action + - `action_id`: the id of the default action. This is required if + `default_entry` is present. - `action_const`: an optional boolean value which is `true` iff the control plane is not allowed to change the default action function. Default value is `false`. It can only be set to `true` for `simple` tables. See Note 2 below. - `action_data`: an optional JSON array where each entry is the hexstring value for an action argument. The size of the array needs to match the number of parameters expected by the action function with id `action_id`. + This is required if `default_entry` is present. - `action_entry_const`: an optional boolean value which is `true` iff the control plane is not allowed to modify the action entry (action function + action data). Default value is `false`. This attribute is ignored if the From ecca451a954134a726d7d25d04156423079d53fc Mon Sep 17 00:00:00 2001 From: Andy Fingerhut Date: Wed, 15 Jan 2025 01:54:28 +0000 Subject: [PATCH 8/9] Add a note in JSON docs about when p4c include default_entry key for a table Signed-off-by: Andy Fingerhut --- docs/JSON_format.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/JSON_format.md b/docs/JSON_format.md index aa2f72ba1..efef1f01f 100644 --- a/docs/JSON_format.md +++ b/docs/JSON_format.md @@ -693,6 +693,11 @@ has always created tables with the value of `action_entry_const` equal to `action_const`. They are both true if the `default_action` in the P4 source code for the table is declared `const`, and both false if the `default_action` is not declared `const`. +Also since 2017 and perhaps earlier, p4c has always included the key +`default_entry` in all table definitions with `type` equal to `simple`. +Since then it has _not_ included the key `default_entry` in table +definitions with types that were not `simple` (i.e. tables with +action profiles or action selectors). Note 3: p4c always creates the value of the `next_tables` key in one of these ways: From 722bb68ab801ab2b4e2d3d8d196282b7157cd8ec Mon Sep 17 00:00:00 2001 From: Andy Fingerhut Date: Wed, 15 Jan 2025 19:18:35 +0000 Subject: [PATCH 9/9] Update JSON docs making it clearer that action_id and action_data are required Signed-off-by: Andy Fingerhut --- docs/JSON_format.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/docs/JSON_format.md b/docs/JSON_format.md index efef1f01f..6b1d99472 100644 --- a/docs/JSON_format.md +++ b/docs/JSON_format.md @@ -631,15 +631,13 @@ attributes for these objects are: - `default_entry`: an optional JSON item which can force the default entry for the table to be set when loading the JSON, without intervention from the control plane. It has the following attributes: - - `action_id`: the id of the default action. This is required if - `default_entry` is present. + - `action_id`: the id of the default action. Required. - `action_const`: an optional boolean value which is `true` iff the control plane is not allowed to change the default action function. Default value is `false`. It can only be set to `true` for `simple` tables. See Note 2 below. - - `action_data`: an optional JSON array where each entry is the hexstring + - `action_data`: a required JSON array where each entry is the hexstring value for an action argument. The size of the array needs to match the number of parameters expected by the action function with id `action_id`. - This is required if `default_entry` is present. - `action_entry_const`: an optional boolean value which is `true` iff the control plane is not allowed to modify the action entry (action function + action data). Default value is `false`. This attribute is ignored if the