diff --git a/include/bm/bm_sim/packet.h b/include/bm/bm_sim/packet.h index 48f2d0e8f..53b450f44 100644 --- a/include/bm/bm_sim/packet.h +++ b/include/bm/bm_sim/packet.h @@ -150,6 +150,11 @@ class Packet final { //! length of the outgoing packet may be different. int get_ingress_length() const { return ingress_length; } + //! Sets the ingress_length. This may be needed for some architectures, + //! although usually it is better to create a new Packet object. See + //! get_ingress_length() for more information about ingress_length. + void set_ingress_length(int length) { ingress_length = length; } + void set_payload_size(size_t size) { payload_size = size; } size_t get_payload_size() const { return payload_size; } diff --git a/src/bm_sim/deparser.cpp b/src/bm_sim/deparser.cpp index 59a836dbd..f5dde6951 100644 --- a/src/bm_sim/deparser.cpp +++ b/src/bm_sim/deparser.cpp @@ -42,7 +42,7 @@ Deparser::get_headers_size(const PHV &phv) const { void Deparser::deparse(Packet *pkt) const { - PHV *phv = pkt->get_phv(); + const PHV *phv = pkt->get_phv(); BMELOG(deparser_start, *pkt, *this); // TODO(antonin) // this is temporary while we experiment with the debugger @@ -56,16 +56,14 @@ Deparser::deparse(Packet *pkt) const { // invalidating headers, and resetting header stacks is done in the Packet // destructor, when the PHV is released for (auto it = headers.begin(); it != headers.end(); ++it) { - Header &header = phv->get_header(*it); + const auto &header = phv->get_header(*it); if (header.is_valid()) { BMELOG(deparser_emit, *pkt, *it); BMLOG_DEBUG_PKT(*pkt, "Deparsing header '{}'", header.get_name()); header.deparse(data + bytes_parsed); bytes_parsed += header.get_nbytes_packet(); - // header.mark_invalid(); } } - // phv->reset_header_stacks(); BMELOG(deparser_done, *pkt, *this); DEBUGGER_NOTIFY_CTR( Debugger::PacketId::make(pkt->get_packet_id(), pkt->get_copy_id()), diff --git a/src/bm_sim/parser.cpp b/src/bm_sim/parser.cpp index e0825e739..857091439 100644 --- a/src/bm_sim/parser.cpp +++ b/src/bm_sim/parser.cpp @@ -92,7 +92,7 @@ ParserOpSet::operator()(Packet *pkt, const char *data, size_t *bytes_parsed) const { static thread_local ByteContainer bc; auto phv = pkt->get_phv(); - if (pkt->get_ingress_length() - *bytes_parsed < src.nbytes) + if (pkt->get_data_size() - *bytes_parsed < src.nbytes) throw parser_exception_core(ErrorCodeMap::Core::PacketTooShort); auto &f_dst = phv->get_field(dst.header, dst.offset); auto f_bits = f_dst.get_nbits(); @@ -235,7 +235,7 @@ namespace { void check_enough_data_for_extract(const Packet &pkt, size_t bytes_parsed, const Header &hdr) { - if (pkt.get_ingress_length() - bytes_parsed < + if (pkt.get_data_size() - bytes_parsed < static_cast(hdr.get_nbytes_packet())) throw parser_exception_core(ErrorCodeMap::Core::PacketTooShort); } diff --git a/targets/simple_switch/simple_switch.cpp b/targets/simple_switch/simple_switch.cpp index 5b3ffedb1..4827f04b5 100644 --- a/targets/simple_switch/simple_switch.cpp +++ b/targets/simple_switch/simple_switch.cpp @@ -541,6 +541,9 @@ SimpleSwitch::egress_thread(size_t worker_id) { size_t packet_size = packet_copy->get_data_size(); packet_copy->set_register(PACKET_LENGTH_REG_IDX, packet_size); phv_copy->get_field("standard_metadata.packet_length").set(packet_size); + // TODO(antonin): really it may be better to create a new packet here or + // to fold this functionality into the Packet class? + packet_copy->set_ingress_length(packet_size); input_buffer.push_front(std::move(packet_copy)); continue; } diff --git a/targets/simple_switch/tests/Makefile.am b/targets/simple_switch/tests/Makefile.am index 31e9e22da..c4ee627f6 100644 --- a/targets/simple_switch/tests/Makefile.am +++ b/targets/simple_switch/tests/Makefile.am @@ -19,7 +19,8 @@ common_source = main.cpp utils.cpp utils.h TESTS = test_packet_redirect \ test_truncate \ test_swap \ -test_queueing +test_queueing \ +test_recirc check_PROGRAMS = $(TESTS) test_all @@ -28,16 +29,19 @@ test_packet_redirect_SOURCES = $(common_source) test_packet_redirect.cpp test_truncate_SOURCES = $(common_source) test_truncate.cpp test_swap_SOURCES = $(common_source) test_swap.cpp test_queueing_SOURCES = $(common_source) test_queueing.cpp +test_recirc_SOURCES = $(common_source) test_recirc.cpp test_all_SOURCES = $(common_source) \ test_packet_redirect.cpp \ test_truncate.cpp \ test_swap.cpp \ -test_queueing.cpp +test_queueing.cpp \ +test_recirc.cpp EXTRA_DIST = \ testdata/packet_redirect.json \ testdata/truncate.json \ testdata/swap_1.json \ testdata/swap_2.json \ -testdata/queueing.json +testdata/queueing.json \ +testdata/recirc.json diff --git a/targets/simple_switch/tests/test_recirc.cpp b/targets/simple_switch/tests/test_recirc.cpp new file mode 100644 index 000000000..28ba2db95 --- /dev/null +++ b/targets/simple_switch/tests/test_recirc.cpp @@ -0,0 +1,137 @@ +/* Copyright 2013-present Barefoot Networks, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include + +#include + +#include + +#include +#include +#include +#include // for std::fill_n + +#include "simple_switch.h" + +#include "utils.h" + +namespace fs = boost::filesystem; + +using bm::MatchErrorCode; +using bm::ActionData; +using bm::MatchKeyParam; +using bm::entry_handle_t; + +namespace { + +void +packet_handler(int port_num, const char *buffer, int len, void *cookie) { + static_cast(cookie)->receive(port_num, buffer, len); +} + +} // namespace + +class SimpleSwitch_RecircP4 : public ::testing::Test { + protected: + static constexpr size_t kMaxBufSize = 512; + + static constexpr bm::device_id_t device_id{0}; + + SimpleSwitch_RecircP4() + : packet_inject(packet_in_addr) { } + + // Per-test-case set-up. + // We make the switch a shared resource for all tests. This is mainly because + // the simple_switch target detaches threads + static void SetUpTestCase() { + // bm::Logger::set_logger_console(); + + test_switch = new SimpleSwitch(8); // 8 ports + + // load JSON + fs::path json_path = fs::path(testdata_dir) / fs::path(test_json); + test_switch->init_objects(json_path.string()); + + // packet in - packet out + test_switch->set_dev_mgr_packet_in(device_id, packet_in_addr, nullptr); + test_switch->Switch::start(); // there is a start member in SimpleSwitch + test_switch->set_packet_handler(packet_handler, + static_cast(test_switch)); + test_switch->start_and_return(); + } + + // Per-test-case tear-down. + static void TearDownTestCase() { + delete test_switch; + } + + virtual void SetUp() { + packet_inject.start(); + auto cb = std::bind(&PacketInReceiver::receive, &receiver, + std::placeholders::_1, std::placeholders::_2, + std::placeholders::_3, std::placeholders::_4); + packet_inject.set_packet_receiver(cb, nullptr); + + // default actions for all tables + test_switch->mt_set_default_action(0, "t_ingress", "_nop", ActionData()); + } + + virtual void TearDown() { + // kind of experimental, so reserved for testing + test_switch->reset_state(); + } + + protected: + static const std::string packet_in_addr; + static SimpleSwitch *test_switch; + bm_apps::PacketInject packet_inject; + PacketInReceiver receiver{}; + + private: + static const std::string testdata_dir; + static const std::string test_json; +}; + +const std::string SimpleSwitch_RecircP4::packet_in_addr = + "inproc://packets"; + +SimpleSwitch *SimpleSwitch_RecircP4::test_switch = nullptr; + +const std::string SimpleSwitch_RecircP4::testdata_dir = TESTDATADIR; +const std::string SimpleSwitch_RecircP4::test_json = + "recirc.json"; + +// Test added for this issue: +// https://github.com/p4lang/behavioral-model/issues/626 +// The parser used the ingress_length field of Packet to determine whether the +// packet has enough available bytes for an extract, which may not be correct +// (e.g. an architecture like PSA may have an egress parser...). In this +// specific case, the ingress_length field wasn't updated properly for +// recirculated packet, which exposed the issue (by raising a PacketTooShort +// exception in the parser). +TEST_F(SimpleSwitch_RecircP4, Recirc) { + static constexpr int port = 1; + + const char pkt[] = {'\x00'}; + packet_inject.send(port, pkt, sizeof(pkt)); + char recv_buffer[] = {'\x00', '\x00'}; + int recv_port = -1; + size_t recv_size = receiver.read( + recv_buffer, sizeof(recv_buffer), &recv_port); + EXPECT_EQ(port, recv_port); + EXPECT_EQ(2, recv_size); + EXPECT_EQ('\xab', recv_buffer[1]); +} diff --git a/targets/simple_switch/tests/testdata/recirc.json b/targets/simple_switch/tests/testdata/recirc.json new file mode 100644 index 000000000..409c3cde5 --- /dev/null +++ b/targets/simple_switch/tests/testdata/recirc.json @@ -0,0 +1,517 @@ +{ + "__meta__": { + "version": [ + 2, + 5 + ], + "compiler": "https://github.com/p4lang/p4c-bm" + }, + "header_types": [ + { + "name": "standard_metadata_t", + "id": 0, + "fields": [ + [ + "ingress_port", + 9 + ], + [ + "packet_length", + 32 + ], + [ + "egress_spec", + 9 + ], + [ + "egress_port", + 9 + ], + [ + "egress_instance", + 32 + ], + [ + "instance_type", + 32 + ], + [ + "clone_spec", + 32 + ], + [ + "_padding", + 5 + ] + ], + "length_exp": null, + "max_length": null + }, + { + "name": "intrinsic_metadata_t", + "id": 1, + "fields": [ + [ + "mcast_grp", + 4 + ], + [ + "egress_rid", + 4 + ], + [ + "mcast_hash", + 16 + ], + [ + "lf_field_list", + 32 + ], + [ + "ingress_global_timestamp", + 64 + ], + [ + "resubmit_flag", + 16 + ], + [ + "recirculate_flag", + 16 + ] + ], + "length_exp": null, + "max_length": null + }, + { + "name": "hdrA_t", + "id": 2, + "fields": [ + [ + "f1", + 8 + ] + ], + "length_exp": null, + "max_length": null + } + ], + "headers": [ + { + "name": "standard_metadata", + "id": 0, + "header_type": "standard_metadata_t", + "metadata": true + }, + { + "name": "intrinsic_metadata", + "id": 1, + "header_type": "intrinsic_metadata_t", + "metadata": true + }, + { + "name": "hdrA1", + "id": 2, + "header_type": "hdrA_t", + "metadata": false + }, + { + "name": "hdrA2", + "id": 3, + "header_type": "hdrA_t", + "metadata": false + } + ], + "header_stacks": [], + "parsers": [ + { + "name": "parser", + "id": 0, + "init_state": "start", + "parse_states": [ + { + "name": "start", + "id": 0, + "parser_ops": [ + { + "op": "extract", + "parameters": [ + { + "type": "regular", + "value": "hdrA1" + } + ] + } + ], + "transition_key": [ + { + "type": "field", + "value": [ + "hdrA1", + "f1" + ] + } + ], + "transitions": [ + { + "type": "hexstr", + "value": "0x00", + "mask": null, + "next_state": null + }, + { + "type": "default", + "value": null, + "mask": null, + "next_state": "more" + } + ] + }, + { + "name": "more", + "id": 1, + "parser_ops": [ + { + "op": "extract", + "parameters": [ + { + "type": "regular", + "value": "hdrA2" + } + ] + }, + { + "op": "set", + "parameters": [ + { + "type": "field", + "value": [ + "hdrA2", + "f1" + ] + }, + { + "type": "hexstr", + "value": "0xab" + } + ] + } + ], + "transition_key": [], + "transitions": [ + { + "type": "default", + "value": null, + "mask": null, + "next_state": null + } + ] + } + ] + } + ], + "parse_vsets": [], + "deparsers": [ + { + "name": "deparser", + "id": 0, + "order": [ + "hdrA1", + "hdrA2" + ] + } + ], + "meter_arrays": [], + "actions": [ + { + "name": "loopback", + "id": 0, + "runtime_data": [], + "primitives": [ + { + "op": "modify_field", + "parameters": [ + { + "type": "field", + "value": [ + "standard_metadata", + "egress_spec" + ] + }, + { + "type": "field", + "value": [ + "standard_metadata", + "ingress_port" + ] + } + ] + } + ] + }, + { + "name": "recirc", + "id": 1, + "runtime_data": [], + "primitives": [ + { + "op": "modify_field", + "parameters": [ + { + "type": "field", + "value": [ + "hdrA1", + "f1" + ] + }, + { + "type": "hexstr", + "value": "0x1" + } + ] + }, + { + "op": "add_header", + "parameters": [ + { + "type": "header", + "value": "hdrA2" + } + ] + }, + { + "op": "recirculate", + "parameters": [ + { + "type": "hexstr", + "value": "0x1" + } + ] + } + ] + } + ], + "pipelines": [ + { + "name": "ingress", + "id": 0, + "init_table": "t_loopback", + "tables": [ + { + "name": "t_loopback", + "id": 0, + "match_type": "exact", + "type": "simple", + "max_size": 16384, + "with_counters": false, + "direct_meters": null, + "support_timeout": false, + "key": [], + "actions": [ + "loopback" + ], + "next_tables": { + "loopback": null + }, + "default_entry": { + "action_id": 0, + "action_const": true, + "action_data": [], + "action_entry_const": false + }, + "base_default_next": null + } + ], + "action_profiles": [], + "conditionals": [] + }, + { + "name": "egress", + "id": 1, + "init_table": "_condition_0", + "tables": [ + { + "name": "t_recirc", + "id": 1, + "match_type": "exact", + "type": "simple", + "max_size": 16384, + "with_counters": false, + "direct_meters": null, + "support_timeout": false, + "key": [], + "actions": [ + "recirc" + ], + "next_tables": { + "recirc": null + }, + "default_entry": { + "action_id": 1, + "action_const": true, + "action_data": [], + "action_entry_const": false + }, + "base_default_next": null + } + ], + "action_profiles": [], + "conditionals": [ + { + "name": "_condition_0", + "id": 0, + "expression": { + "type": "expression", + "value": { + "op": "==", + "left": { + "type": "field", + "value": [ + "standard_metadata", + "instance_type" + ] + }, + "right": { + "type": "hexstr", + "value": "0x0" + } + } + }, + "true_next": "t_recirc", + "false_next": null + } + ] + } + ], + "calculations": [], + "checksums": [], + "learn_lists": [], + "field_lists": [ + { + "id": 1, + "name": "recirc_FL", + "elements": [ + { + "type": "field", + "value": [ + "standard_metadata", + "ingress_port" + ] + }, + { + "type": "field", + "value": [ + "standard_metadata", + "packet_length" + ] + }, + { + "type": "field", + "value": [ + "standard_metadata", + "egress_spec" + ] + }, + { + "type": "field", + "value": [ + "standard_metadata", + "egress_port" + ] + }, + { + "type": "field", + "value": [ + "standard_metadata", + "egress_instance" + ] + }, + { + "type": "field", + "value": [ + "standard_metadata", + "instance_type" + ] + }, + { + "type": "field", + "value": [ + "standard_metadata", + "clone_spec" + ] + }, + { + "type": "field", + "value": [ + "standard_metadata", + "_padding" + ] + } + ] + } + ], + "counter_arrays": [], + "register_arrays": [], + "force_arith": [ + [ + "standard_metadata", + "ingress_port" + ], + [ + "standard_metadata", + "packet_length" + ], + [ + "standard_metadata", + "egress_spec" + ], + [ + "standard_metadata", + "egress_port" + ], + [ + "standard_metadata", + "egress_instance" + ], + [ + "standard_metadata", + "instance_type" + ], + [ + "standard_metadata", + "clone_spec" + ], + [ + "standard_metadata", + "_padding" + ], + [ + "intrinsic_metadata", + "mcast_grp" + ], + [ + "intrinsic_metadata", + "egress_rid" + ], + [ + "intrinsic_metadata", + "mcast_hash" + ], + [ + "intrinsic_metadata", + "lf_field_list" + ], + [ + "intrinsic_metadata", + "ingress_global_timestamp" + ], + [ + "intrinsic_metadata", + "resubmit_flag" + ], + [ + "intrinsic_metadata", + "recirculate_flag" + ] + ] +} \ No newline at end of file diff --git a/targets/simple_switch/tests/testdata/recirc.p4 b/targets/simple_switch/tests/testdata/recirc.p4 new file mode 100644 index 000000000..ff06f5091 --- /dev/null +++ b/targets/simple_switch/tests/testdata/recirc.p4 @@ -0,0 +1,85 @@ +/* Copyright 2013-present Barefoot Networks, Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +header_type intrinsic_metadata_t { + fields { + mcast_grp : 4; + egress_rid : 4; + mcast_hash : 16; + lf_field_list: 32; + ingress_global_timestamp : 64; + resubmit_flag : 16; + recirculate_flag : 16; + } +} + +metadata intrinsic_metadata_t intrinsic_metadata; + +header_type hdrA_t { + fields { + f1 : 8; + } +} + +header hdrA_t hdrA1; +header hdrA_t hdrA2; + +parser start { + extract(hdrA1); + return select(hdrA1.f1) { + 0x00: ingress; + default: more; + } +} + +parser more { + extract(hdrA2); + set_metadata(hdrA2.f1, 0xab); + return ingress; +} + +field_list recirc_FL { + standard_metadata; +} + +action loopback() { + modify_field(standard_metadata.egress_spec, standard_metadata.ingress_port); +} + +table t_loopback { + actions { loopback; } + default_action: loopback(); +} + +action recirc() { + modify_field(hdrA1.f1, 0x01); + add_header(hdrA2); + recirculate(recirc_FL); +} + +table t_recirc { + actions { recirc; } + default_action: recirc(); +} + +control ingress { + apply(t_loopback); +} + +control egress { + if (standard_metadata.instance_type == 0) { + apply(t_recirc); + } +} diff --git a/tests/test_parser.cpp b/tests/test_parser.cpp index 60785e46c..344ffbe48 100644 --- a/tests/test_parser.cpp +++ b/tests/test_parser.cpp @@ -398,6 +398,11 @@ class ParserOpSetTest : public ::testing::Test { return Packet::make_new(64, PacketBuffer(128), phv_source.get()); } + Packet get_pkt(const char *data, size_t size) { + return Packet::make_new( + size, PacketBuffer(size + 128, data, size), phv_source.get()); + } + virtual void SetUp() { phv_source->set_phv_factory(0, &phv_factory); } @@ -437,11 +442,11 @@ TEST_F(ParserOpSetTest, SetFromLookahead) { auto lookahead = ParserLookAhead::make(offset, bitwidth); const ParserOpSet op(testHeader1, 1, lookahead); // f32 const ParserOp &opRef = op; - auto pkt = get_pkt(); + auto pkt = get_pkt(data, sizeof(data_)); auto &f = pkt.get_phv()->get_field(testHeader1, 1); f.set(0); size_t bytes_parsed = 0; - opRef(&pkt, reinterpret_cast(data), &bytes_parsed); + opRef(&pkt, data, &bytes_parsed); ASSERT_EQ(v, f.get_uint()); };