From 1df35a4e37bf171f983e1c633f3dcf0c124a6e7f Mon Sep 17 00:00:00 2001 From: Andy Fingerhut Date: Mon, 22 Apr 2019 21:40:34 -0700 Subject: [PATCH 1/6] Change "private" v1model standard_metadata fields to BMv2 packet registers Remove the use of the following 4 fields from BMv2 simple_switch and simple_switch_grpc. They no longer need to be in the BMv2 JSON file for such programs, and clone, recirculate, resubmit, and digest operations are still able to work without them there, because they are now stored in a BMv2 "packet register", similar to how the packet length field currently is. + clone_spec + resubmit_flag + recirculate_flag + lf_field_list --- docs/simple_switch.md | 71 +++---------- include/bm/bm_sim/packet.h | 2 +- targets/simple_switch/primitives.cpp | 46 +++++---- targets/simple_switch/register_access.h | 105 +++++++++++++++++++ targets/simple_switch/simple_switch.cpp | 132 ++++++++++++------------ 5 files changed, 212 insertions(+), 144 deletions(-) create mode 100644 targets/simple_switch/register_access.h diff --git a/docs/simple_switch.md b/docs/simple_switch.md index 43cf1e214..263f89eb8 100644 --- a/docs/simple_switch.md +++ b/docs/simple_switch.md @@ -96,14 +96,6 @@ Here are the fields: 0. Calls to `verify_checksum` should be in the `VerifyChecksum` control in v1model, which is executed after the parser and before ingress. -- `clone_spec` (v1m): should not be accessed directly. It is set by - the `clone` and `clone3` primitive actions in P4_16 programs, or the - `clone_ingress_pkt_to_egress` and `clone_egress_pkt_to_egress` - primitive actions for P4_14 programs, and is required for the packet - clone (aka mirror) feature. The "ingress to egress" clone primitive - action must be called from the ingress pipeline, and the "egress to - egress" clone primitive action must be called from the egress - pipeline. ## Intrinsic metadata @@ -133,11 +125,8 @@ header_type intrinsic_metadata_t { fields { ingress_global_timestamp : 48; egress_global_timestamp : 48; - lf_field_list : 8; mcast_grp : 16; egress_rid : 16; - resubmit_flag : 8; - recirculate_flag : 8; } } metadata intrinsic_metadata_t intrinsic_metadata; @@ -150,8 +139,6 @@ not be written to. starts egress processing. The clock is the same as for `ingress_global_timestamp`. This field should only be read from the egress pipeline, but should not be written to. -- `lf_field_list`: used to store the learn id when calling `generate_digest`; do -not access directly. - `mcast_grp`: needed for the multicast feature. This field needs to be written in the ingress pipeline when you wish the packet to be multicast. A value of 0 means no multicast. This value must be one of a valid multicast group configured @@ -161,32 +148,6 @@ end of ingress. - `egress_rid`: needed for the multicast feature. This field is only valid in the egress pipeline and can only be read from. It is used to uniquely identify multicast copies of the same ingress packet. -- `resubmit_flag`: should not be accessed directly. It is set by the -`resubmit` action primitive and is required for the resubmit -feature. As a reminder, `resubmit` needs to be called in the ingress -pipeline. See the "after-ingress pseudocode" for relative priority of -this vs. other possible packet operations at end of ingress. -- `recirculate_flag`: should not be accessed directly. It is set by the -`recirculate` action primitive and is required for the recirculate feature. As a -reminder, `recirculate` needs to be called from the egress pipeline. -See the "after-egress pseudocode" for the relative priority of this -vs. other possible packet operations at the end of egress processing. - -Several of these fields should be considered internal implementation -details for how simple_switch implements some packet processing -features. They are: `lf_field_list`, `resubmit_flag`, -`recirculate_flag`, and `clone_spec`. They have the following -properties in common: - -- They are initialized to 0, and are assigned a compiler-chosen non-0 - value when the corresponding primitive action is called. -- Your P4 program should never assign them a value directly. -- Reading the values may be helpful for debugging. -- Reading them may also be useful for knowing whether the - corresponding primitive action was called earlier in the - execution of the P4 program, but if you want to know whether such a - use is portable to P4 implementations other than simple_switch, you - will have to check the documentation for that other implementation. ### `queueing_metadata` header @@ -240,13 +201,13 @@ file](../targets/simple_switch/primitives.cpp). After-ingress pseudocode - the short version: ``` -if (clone_spec != 0) { // because your code called a clone primitive action +if (a clone primitive action was called) { make a clone of the packet with details configured for the clone session } -if (lf_field_list != 0) { // because your code called generate_digest +if (digest to generate) { // because your code called generate_digest send a digest message to the control plane software } -if (resubmit_flag != 0) { // because your code called resubmit +if (resubmit was called) { start ingress processing over again for the original packet } else if (mcast_grp != 0) { // because your code assigned a value to mcast_grp multicast the packet to the output port(s) configured for group mcast_grp @@ -262,7 +223,7 @@ after ingress processing is complete. The longer more detailed version: ``` -if (clone_spec != 0) { +if (a clone primitive action was called) { // This condition will be true if your code called the `clone` or // `clone3` primitive action from a P4_16 program, or the // `clone_ingress_pkt_to_egress` primitive action in a P4_14 @@ -283,29 +244,27 @@ if (clone_spec != 0) { If it was a clone3 (P4_16) or clone_ingress_pkt_to_egress (P4_14) action, also preserve the final ingress values of the metadata fields specified in the field list argument, except assign - clone_spec a value of 0 always, and instance_type a value of - PKT_INSTANCE_TYPE_INGRESS_CLONE. + instance_type a value of PKT_INSTANCE_TYPE_INGRESS_CLONE. The cloned packet will continue processing at the beginning of your egress code. // fall through to code below } -if (lf_field_list != 0) { +if (digest to generate) { // This condition will be true if your code called the // generate_digest primitive action during ingress processing. Send a digest message to the control plane that contains the values of the fields in the specified field list. // fall through to code below } -if (resubmit_flag != 0) { +if (resubmit was called) { // This condition will be true if your code called the resubmit // primitive action during ingress processing. Start ingress over again for this packet, with its original unmodified packet contents and metadata values. Preserve the final ingress values of any fields specified in the field list given as an argument to the last resubmit() primitive operation - called, except assign resubmit_flag a value of 0 always, and - instance_type a value of PKT_INSTANCE_TYPE_RESUBMIT. + called, except assign instance_type a value of PKT_INSTANCE_TYPE_RESUBMIT. } else if (mcast_grp != 0) { // This condition will be true if your code made an assignment to // standard_metadata.mcast_grp during ingress processing. There @@ -331,12 +290,12 @@ if (resubmit_flag != 0) { After-egress pseudocode - the short version: ``` -if (clone_spec != 0) { // because your code called a clone primitive action +if (a clone primitive action was called) { make a clone of the packet with details configured for the clone session } if (egress_spec == DROP_PORT) { // e.g. because your code called drop/mark_to_drop Drop packet. -} else if (recirculate_flag != 0) { // because your code called recirculate +} else if (recirculate was called) { start ingress processing over again for deparsed packet } else { Send the packet to the port in egress_port. @@ -348,7 +307,7 @@ after egress processing is complete. The longer more detailed version: ``` -if (clone_spec != 0) { +if (a clone primitive action was called) { // This condition will be true if your code called the `clone` or // `clone3` primitive action from a P4_16 program, or the // `clone_egress_pkt_to_egress` primitive action in a P4_14 @@ -365,8 +324,7 @@ if (clone_spec != 0) { If it was a clone3 (P4_16) or clone_egress_pkt_to_egress (P4_14) action, also preserve the final egress values of the metadata fields specified in the field list argument, except assign - clone_spec a value of 0 always, and instance_type a value of - PKT_INSTANCE_TYPE_EGRESS_CLONE. + instance_type a value of PKT_INSTANCE_TYPE_EGRESS_CLONE. The cloned packet will continue processing at the beginning of your egress code. @@ -377,7 +335,7 @@ if (egress_spec == DROP_PORT) { // mark_to_drop (P4_16) or drop (P4_14) primitive action during // egress processing. Drop packet. -} else if (recirculate_flag != 0) { +} else if (recirculate was called) { // This condition will be true if your code called the recirculate // primitive action during egress processing. Start ingress over again, for the packet as constructed by the @@ -385,8 +343,7 @@ if (egress_spec == DROP_PORT) { ingress and egress processing. Preserve the final egress values of any fields specified in the field list given as an argument to the last recirculate primitive action called, except assign - recirculate_flag a value of 0 always, and instance_type a value of - PKT_INSTANCE_TYPE_RECIRC. + instance_type a value of PKT_INSTANCE_TYPE_RECIRC. } else { Send the packet to the port in egress_port. Since egress_port is read only during egress processing, note that its value must have diff --git a/include/bm/bm_sim/packet.h b/include/bm/bm_sim/packet.h index c974f6b90..8581525e7 100644 --- a/include/bm/bm_sim/packet.h +++ b/include/bm/bm_sim/packet.h @@ -105,7 +105,7 @@ class Packet final { using buffer_state_t = PacketBuffer::state_t; //! Number of general purpose registers per packet - static constexpr size_t nb_registers = 2u; + static constexpr size_t nb_registers = 3u; static constexpr size_t INVALID_ENTRY_INDEX = std::numeric_limits::max(); diff --git a/targets/simple_switch/primitives.cpp b/targets/simple_switch/primitives.cpp index 23e57bd6a..c1e9ad78e 100644 --- a/targets/simple_switch/primitives.cpp +++ b/targets/simple_switch/primitives.cpp @@ -31,6 +31,7 @@ #include #include "simple_switch.h" +#include "register_access.h" template using ActionPrimitive = bm::ActionPrimitive; @@ -224,7 +225,8 @@ class generate_digest : public ActionPrimitive { void operator ()(const Data &receiver, const Data &learn_id) { // discared receiver for now (void) receiver; - get_field("intrinsic_metadata.lf_field_list").set(learn_id); + auto &packet = get_packet(); + RegisterAccess::set_lf_field_list(packet, learn_id.get()); } }; @@ -238,7 +240,9 @@ class add_header : public ActionPrimitive
{ hdr.mark_valid(); // updated the length packet register (register 0) auto &packet = get_packet(); - packet.set_register(0, packet.get_register(0) + hdr.get_nbytes_packet()); + packet.set_register(PACKET_LENGTH_REG_IDX, + packet.get_register(PACKET_LENGTH_REG_IDX) + + hdr.get_nbytes_packet()); } } }; @@ -258,7 +262,9 @@ class remove_header : public ActionPrimitive
{ if (hdr.is_valid()) { // updated the length packet register (register 0) auto &packet = get_packet(); - packet.set_register(0, packet.get_register(0) - hdr.get_nbytes_packet()); + packet.set_register(PACKET_LENGTH_REG_IDX, + packet.get_register(PACKET_LENGTH_REG_IDX) - + hdr.get_nbytes_packet()); hdr.mark_invalid(); } } @@ -274,14 +280,14 @@ class copy_header : public ActionPrimitive
{ REGISTER_PRIMITIVE(copy_header); -/* standard_metadata.clone_spec will contain the mirror id (16 LSB) and the - field list id to copy (16 MSB) */ class clone_ingress_pkt_to_egress : public ActionPrimitive { - void operator ()(const Data &clone_spec, const Data &field_list_id) { - Field &f_clone_spec = get_field("standard_metadata.clone_spec"); - f_clone_spec.shift_left(field_list_id, 16); - f_clone_spec.add(f_clone_spec, clone_spec); + void operator ()(const Data &mirror_session_id, const Data &field_list_id) { + auto &packet = get_packet(); + RegisterAccess::set_clone_mirror_session_id(packet, + mirror_session_id.get()); + RegisterAccess::set_clone_field_list(packet, + field_list_id.get()); } }; @@ -289,10 +295,12 @@ REGISTER_PRIMITIVE(clone_ingress_pkt_to_egress); class clone_egress_pkt_to_egress : public ActionPrimitive { - void operator ()(const Data &clone_spec, const Data &field_list_id) { - Field &f_clone_spec = get_field("standard_metadata.clone_spec"); - f_clone_spec.shift_left(field_list_id, 16); - f_clone_spec.add(f_clone_spec, clone_spec); + void operator ()(const Data &mirror_session_id, const Data &field_list_id) { + auto &packet = get_packet(); + RegisterAccess::set_clone_mirror_session_id(packet, + mirror_session_id.get()); + RegisterAccess::set_clone_field_list(packet, + field_list_id.get()); } }; @@ -300,10 +308,8 @@ REGISTER_PRIMITIVE(clone_egress_pkt_to_egress); class resubmit : public ActionPrimitive { void operator ()(const Data &field_list_id) { - if (get_phv().has_field("intrinsic_metadata.resubmit_flag")) { - get_phv().get_field("intrinsic_metadata.resubmit_flag") - .set(field_list_id); - } + auto &packet = get_packet(); + RegisterAccess::set_resubmit_flag(packet, field_list_id.get()); } }; @@ -311,10 +317,8 @@ REGISTER_PRIMITIVE(resubmit); class recirculate : public ActionPrimitive { void operator ()(const Data &field_list_id) { - if (get_phv().has_field("intrinsic_metadata.recirculate_flag")) { - get_phv().get_field("intrinsic_metadata.recirculate_flag") - .set(field_list_id); - } + auto &packet = get_packet(); + RegisterAccess::set_recirculate_flag(packet, field_list_id.get()); } }; diff --git a/targets/simple_switch/register_access.h b/targets/simple_switch/register_access.h new file mode 100644 index 000000000..45c40d88f --- /dev/null +++ b/targets/simple_switch/register_access.h @@ -0,0 +1,105 @@ +/* Copyright 2019-present Cisco Systems, 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. + */ + +/* + * Andy Fingerhut (jafinger@cisco.com) + * + */ + +#define PACKET_LENGTH_REG_IDX 0 + +#define CLONE_MIRROR_SESSION_ID_REG_IDX 1 +#define CLONE_MIRROR_SESSION_ID_MASK 0x000000000000ffff +#define CLONE_MIRROR_SESSION_ID_SHIFT 0 +#define CLONE_FIELD_LIST_REG_IDX 1 +#define CLONE_FIELD_LIST_MASK 0x00000000ffff0000 +#define CLONE_FIELD_LIST_SHIFT 16 +#define LF_FIELD_LIST_REG_IDX 1 +#define LF_FIELD_LIST_MASK 0x0000ffff00000000 +#define LF_FIELD_LIST_SHIFT 32 +#define RESUBMIT_FLAG_REG_IDX 1 +#define RESUBMIT_FLAG_MASK 0xffff000000000000 +#define RESUBMIT_FLAG_SHIFT 48 + +#define RECIRCULATE_FLAG_REG_IDX 2 +#define RECIRCULATE_FLAG_MASK 0x000000000000ffff +#define RECIRCULATE_FLAG_SHIFT 0 + +//#define CLONE_SPEC_REG_IDX 1 + + +class RegisterAccess { + public: + static void clear_all(Packet &pkt) { + // except do not clear packet length + pkt.set_register(1, 0); + pkt.set_register(2, 0); + } + static uint16_t get_clone_mirror_session_id(Packet &pkt) { + uint64_t rv = pkt.get_register(CLONE_MIRROR_SESSION_ID_REG_IDX); + return (uint16_t) ((rv & CLONE_MIRROR_SESSION_ID_MASK) >> + CLONE_MIRROR_SESSION_ID_SHIFT); + } + static void set_clone_mirror_session_id(Packet &pkt, + uint16_t mirror_session_id) { + uint64_t rv = pkt.get_register(CLONE_MIRROR_SESSION_ID_REG_IDX); + rv = ((rv & ~CLONE_MIRROR_SESSION_ID_MASK) | + (((uint64_t) mirror_session_id) << + CLONE_MIRROR_SESSION_ID_SHIFT)); + pkt.set_register(CLONE_MIRROR_SESSION_ID_REG_IDX, rv); + } + static uint16_t get_clone_field_list(Packet &pkt) { + uint64_t rv = pkt.get_register(CLONE_FIELD_LIST_REG_IDX); + return (uint16_t) ((rv & CLONE_FIELD_LIST_MASK) >> + CLONE_FIELD_LIST_SHIFT); + } + static void set_clone_field_list(Packet &pkt, uint16_t field_list_id) { + uint64_t rv = pkt.get_register(CLONE_FIELD_LIST_REG_IDX); + rv = ((rv & ~CLONE_FIELD_LIST_MASK) | + (((uint64_t) field_list_id) << CLONE_FIELD_LIST_SHIFT)); + pkt.set_register(CLONE_FIELD_LIST_REG_IDX, rv); + } + static uint16_t get_lf_field_list(Packet &pkt) { + uint64_t rv = pkt.get_register(LF_FIELD_LIST_REG_IDX); + return (uint16_t) ((rv & LF_FIELD_LIST_MASK) >> LF_FIELD_LIST_SHIFT); + } + static void set_lf_field_list(Packet &pkt, uint16_t field_list_id) { + uint64_t rv = pkt.get_register(LF_FIELD_LIST_REG_IDX); + rv = ((rv & ~LF_FIELD_LIST_MASK) | + (((uint64_t) field_list_id) << LF_FIELD_LIST_SHIFT)); + pkt.set_register(LF_FIELD_LIST_REG_IDX, rv); + } + static uint16_t get_resubmit_flag(Packet &pkt) { + uint64_t rv = pkt.get_register(RESUBMIT_FLAG_REG_IDX); + return (uint16_t) ((rv & RESUBMIT_FLAG_MASK) >> RESUBMIT_FLAG_SHIFT); + } + static void set_resubmit_flag(Packet &pkt, uint16_t field_list_id) { + uint64_t rv = pkt.get_register(RESUBMIT_FLAG_REG_IDX); + rv = ((rv & ~RESUBMIT_FLAG_MASK) | + (((uint64_t) field_list_id) << RESUBMIT_FLAG_SHIFT)); + pkt.set_register(RESUBMIT_FLAG_REG_IDX, rv); + } + static uint16_t get_recirculate_flag(Packet &pkt) { + uint64_t rv = pkt.get_register(RECIRCULATE_FLAG_REG_IDX); + return (uint16_t) ((rv & RECIRCULATE_FLAG_MASK) >> + RECIRCULATE_FLAG_SHIFT); + } + static void set_recirculate_flag(Packet &pkt, uint16_t field_list_id) { + uint64_t rv = pkt.get_register(RECIRCULATE_FLAG_REG_IDX); + rv = ((rv & ~RECIRCULATE_FLAG_MASK) | + (((uint64_t) field_list_id) << RECIRCULATE_FLAG_SHIFT)); + pkt.set_register(RECIRCULATE_FLAG_REG_IDX, rv); + } +}; diff --git a/targets/simple_switch/simple_switch.cpp b/targets/simple_switch/simple_switch.cpp index 4b4ada99f..f56cf4bfc 100644 --- a/targets/simple_switch/simple_switch.cpp +++ b/targets/simple_switch/simple_switch.cpp @@ -33,6 +33,7 @@ #include #include "simple_switch.h" +#include "register_access.h" namespace { @@ -214,7 +215,6 @@ SimpleSwitch::SimpleSwitch(bool enable_swap, port_t drop_port) add_required_field("standard_metadata", "packet_length"); add_required_field("standard_metadata", "instance_type"); add_required_field("standard_metadata", "egress_spec"); - add_required_field("standard_metadata", "clone_spec"); add_required_field("standard_metadata", "egress_port"); force_arith_header("standard_metadata"); @@ -224,8 +224,6 @@ SimpleSwitch::SimpleSwitch(bool enable_swap, port_t drop_port) import_primitives(this); } -#define PACKET_LENGTH_REG_IDX 0 - int SimpleSwitch::receive_(port_t port_num, const char *buffer, int len) { // this is a good place to call this, because blocking this thread will not @@ -246,6 +244,7 @@ SimpleSwitch::receive_(port_t port_num, const char *buffer, int len) { // many current P4 programs assume this // it is also part of the original P4 spec phv->reset_metadata(); + RegisterAccess::clear_all(*packet); // setting standard metadata @@ -447,6 +446,7 @@ SimpleSwitch::multicast(Packet *packet, unsigned int mgid) { BMLOG_DEBUG_PKT(*packet, "Replicating packet on port {}", egress_port); f_rid.set(out.rid); std::unique_ptr packet_copy = packet->clone_with_phv_ptr(); + RegisterAccess::clear_all(*packet_copy); packet_copy->set_register(PACKET_LENGTH_REG_IDX, packet_size); enqueue(egress_port, std::move(packet_copy)); } @@ -501,17 +501,14 @@ SimpleSwitch::ingress_thread() { Field &f_egress_spec = phv->get_field("standard_metadata.egress_spec"); port_t egress_spec = f_egress_spec.get_uint(); - Field &f_clone_spec = phv->get_field("standard_metadata.clone_spec"); - unsigned int clone_spec = f_clone_spec.get_uint(); + unsigned int clone_mirror_session_id = + RegisterAccess::get_clone_mirror_session_id(*packet); + unsigned int clone_field_list = + RegisterAccess::get_clone_field_list(*packet); - int learn_id = 0; + int learn_id = RegisterAccess::get_lf_field_list(*packet); unsigned int mgid = 0u; - if (phv->has_field("intrinsic_metadata.lf_field_list")) { - Field &f_learn_id = phv->get_field("intrinsic_metadata.lf_field_list"); - learn_id = f_learn_id.get_int(); - } - // detect mcast support, if this is true we assume that other fields needed // for mcast are also defined if (phv->has_field("intrinsic_metadata.mcast_grp")) { @@ -520,18 +517,20 @@ SimpleSwitch::ingress_thread() { } // INGRESS CLONING - if (clone_spec) { + if (clone_mirror_session_id) { BMLOG_DEBUG_PKT(*packet, "Cloning packet at ingress"); - f_clone_spec.set(0); + RegisterAccess::set_clone_mirror_session_id(*packet, 0); + RegisterAccess::set_clone_field_list(*packet, 0); MirroringSessionConfig config; bool is_session_configured = mirroring_get_session( - static_cast(clone_spec & 0xFFFF), &config); + static_cast(clone_mirror_session_id), &config); if (is_session_configured) { const Packet::buffer_state_t packet_out_state = packet->save_buffer_state(); packet->restore_buffer_state(packet_in_state); - p4object_id_t field_list_id = clone_spec >> 16; + p4object_id_t field_list_id = clone_field_list; std::unique_ptr packet_copy = packet->clone_no_phv_ptr(); + RegisterAccess::clear_all(*packet_copy); packet_copy->set_register(PACKET_LENGTH_REG_IDX, ingress_packet_size); // we need to parse again // the alternative would be to pay the (huge) price of PHV copy for @@ -559,25 +558,24 @@ SimpleSwitch::ingress_thread() { } // RESUBMIT - if (phv->has_field("intrinsic_metadata.resubmit_flag")) { - Field &f_resubmit = phv->get_field("intrinsic_metadata.resubmit_flag"); - if (f_resubmit.get_int()) { - BMLOG_DEBUG_PKT(*packet, "Resubmitting packet"); - // get the packet ready for being parsed again at the beginning of - // ingress - packet->restore_buffer_state(packet_in_state); - p4object_id_t field_list_id = f_resubmit.get_int(); - f_resubmit.set(0); - // TODO(antonin): a copy is not needed here, but I don't yet have an - // optimized way of doing this - std::unique_ptr packet_copy = packet->clone_no_phv_ptr(); - copy_field_list_and_set_type(packet, packet_copy, - PKT_INSTANCE_TYPE_RESUBMIT, - field_list_id); - input_buffer->push_front( - InputBuffer::PacketType::RESUBMIT, std::move(packet_copy)); - continue; - } + unsigned int resubmit_flag = RegisterAccess::get_resubmit_flag(*packet); + if (resubmit_flag) { + BMLOG_DEBUG_PKT(*packet, "Resubmitting packet"); + // get the packet ready for being parsed again at the beginning of + // ingress + packet->restore_buffer_state(packet_in_state); + p4object_id_t field_list_id = resubmit_flag; + RegisterAccess::set_resubmit_flag(*packet, 0); + // TODO(antonin): a copy is not needed here, but I don't yet have an + // optimized way of doing this + std::unique_ptr packet_copy = packet->clone_no_phv_ptr(); + copy_field_list_and_set_type(packet, packet_copy, + PKT_INSTANCE_TYPE_RESUBMIT, + field_list_id); + RegisterAccess::clear_all(*packet_copy); + input_buffer->push_front( + InputBuffer::PacketType::RESUBMIT, std::move(packet_copy)); + continue; } // MULTICAST @@ -656,18 +654,21 @@ SimpleSwitch::egress_thread(size_t worker_id) { egress_mau->apply(packet.get()); - Field &f_clone_spec = phv->get_field("standard_metadata.clone_spec"); - unsigned int clone_spec = f_clone_spec.get_uint(); + unsigned int clone_mirror_session_id = + RegisterAccess::get_clone_mirror_session_id(*packet); + unsigned int clone_field_list = + RegisterAccess::get_clone_field_list(*packet); // EGRESS CLONING - if (clone_spec) { + if (clone_mirror_session_id) { BMLOG_DEBUG_PKT(*packet, "Cloning packet at egress"); - f_clone_spec.set(0); + RegisterAccess::set_clone_mirror_session_id(*packet, 0); + RegisterAccess::set_clone_field_list(*packet, 0); MirroringSessionConfig config; bool is_session_configured = mirroring_get_session( - static_cast(clone_spec & 0xFFFF), &config); + static_cast(clone_mirror_session_id), &config); if (is_session_configured) { - p4object_id_t field_list_id = clone_spec >> 16; + p4object_id_t field_list_id = clone_field_list; std::unique_ptr packet_copy = packet->clone_with_phv_reset_metadata_ptr(); PHV *phv_copy = packet_copy->get_phv(); @@ -682,6 +683,7 @@ SimpleSwitch::egress_thread(size_t worker_id) { if (config.egress_port_valid) { BMLOG_DEBUG_PKT(*packet, "Cloning packet to egress port {}", config.egress_port); + RegisterAccess::clear_all(*packet_copy); enqueue(config.egress_port, std::move(packet_copy)); } } @@ -697,31 +699,31 @@ SimpleSwitch::egress_thread(size_t worker_id) { deparser->deparse(packet.get()); // RECIRCULATE - if (phv->has_field("intrinsic_metadata.recirculate_flag")) { - Field &f_recirc = phv->get_field("intrinsic_metadata.recirculate_flag"); - if (f_recirc.get_int()) { - BMLOG_DEBUG_PKT(*packet, "Recirculating packet"); - p4object_id_t field_list_id = f_recirc.get_int(); - f_recirc.set(0); - FieldList *field_list = this->get_field_list(field_list_id); - // TODO(antonin): just like for resubmit, there is no need for a copy - // here, but it is more convenient for this first prototype - std::unique_ptr packet_copy = packet->clone_no_phv_ptr(); - PHV *phv_copy = packet_copy->get_phv(); - phv_copy->reset_metadata(); - field_list->copy_fields_between_phvs(phv_copy, phv); - phv_copy->get_field("standard_metadata.instance_type") - .set(PKT_INSTANCE_TYPE_RECIRC); - 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( - InputBuffer::PacketType::RECIRCULATE, std::move(packet_copy)); - continue; - } + unsigned int recirculate_flag = + RegisterAccess::get_recirculate_flag(*packet); + if (recirculate_flag) { + BMLOG_DEBUG_PKT(*packet, "Recirculating packet"); + p4object_id_t field_list_id = recirculate_flag; + RegisterAccess::set_recirculate_flag(*packet, 0); + FieldList *field_list = this->get_field_list(field_list_id); + // TODO(antonin): just like for resubmit, there is no need for a copy + // here, but it is more convenient for this first prototype + std::unique_ptr packet_copy = packet->clone_no_phv_ptr(); + PHV *phv_copy = packet_copy->get_phv(); + phv_copy->reset_metadata(); + field_list->copy_fields_between_phvs(phv_copy, phv); + phv_copy->get_field("standard_metadata.instance_type") + .set(PKT_INSTANCE_TYPE_RECIRC); + size_t packet_size = packet_copy->get_data_size(); + RegisterAccess::clear_all(*packet_copy); + 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( + InputBuffer::PacketType::RECIRCULATE, std::move(packet_copy)); + continue; } output_buffer.push_front(std::move(packet)); From 163fe53cb08f334c73554cf80c9f16d82b7257ca Mon Sep 17 00:00:00 2001 From: Andy Fingerhut Date: Mon, 22 Apr 2019 22:17:33 -0700 Subject: [PATCH 2/6] Fix check_style errors --- targets/simple_switch/register_access.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/targets/simple_switch/register_access.h b/targets/simple_switch/register_access.h index 45c40d88f..2affce86d 100644 --- a/targets/simple_switch/register_access.h +++ b/targets/simple_switch/register_access.h @@ -18,6 +18,9 @@ * */ +#ifndef SIMPLE_SWITCH_REGISTER_ACCESS_H_ +#define SIMPLE_SWITCH_REGISTER_ACCESS_H_ + #define PACKET_LENGTH_REG_IDX 0 #define CLONE_MIRROR_SESSION_ID_REG_IDX 1 @@ -37,8 +40,6 @@ #define RECIRCULATE_FLAG_MASK 0x000000000000ffff #define RECIRCULATE_FLAG_SHIFT 0 -//#define CLONE_SPEC_REG_IDX 1 - class RegisterAccess { public: @@ -103,3 +104,5 @@ class RegisterAccess { pkt.set_register(RECIRCULATE_FLAG_REG_IDX, rv); } }; + +#endif // SIMPLE_SWITCH_REGISTER_ACCESS_H_ From 197e51ab95f2b8e8f409169adae8763beba76eb1 Mon Sep 17 00:00:00 2001 From: Andy Fingerhut Date: Tue, 23 Apr 2019 12:36:18 -0700 Subject: [PATCH 3/6] Address review comments. --- include/bm/bm_sim/packet.h | 2 +- targets/simple_switch/primitives.cpp | 17 +++++--- targets/simple_switch/register_access.h | 54 +++++++++++++------------ targets/simple_switch/simple_switch.cpp | 40 ++++++++++-------- 4 files changed, 64 insertions(+), 49 deletions(-) diff --git a/include/bm/bm_sim/packet.h b/include/bm/bm_sim/packet.h index 8581525e7..0da8c2146 100644 --- a/include/bm/bm_sim/packet.h +++ b/include/bm/bm_sim/packet.h @@ -105,7 +105,7 @@ class Packet final { using buffer_state_t = PacketBuffer::state_t; //! Number of general purpose registers per packet - static constexpr size_t nb_registers = 3u; + static constexpr size_t nb_registers = 4u; static constexpr size_t INVALID_ENTRY_INDEX = std::numeric_limits::max(); diff --git a/targets/simple_switch/primitives.cpp b/targets/simple_switch/primitives.cpp index c1e9ad78e..60613c324 100644 --- a/targets/simple_switch/primitives.cpp +++ b/targets/simple_switch/primitives.cpp @@ -240,8 +240,8 @@ class add_header : public ActionPrimitive
{ hdr.mark_valid(); // updated the length packet register (register 0) auto &packet = get_packet(); - packet.set_register(PACKET_LENGTH_REG_IDX, - packet.get_register(PACKET_LENGTH_REG_IDX) + + packet.set_register(RegisterAccess::PACKET_LENGTH_REG_IDX, + packet.get_register(RegisterAccess::PACKET_LENGTH_REG_IDX) + hdr.get_nbytes_packet()); } } @@ -262,8 +262,8 @@ class remove_header : public ActionPrimitive
{ if (hdr.is_valid()) { // updated the length packet register (register 0) auto &packet = get_packet(); - packet.set_register(PACKET_LENGTH_REG_IDX, - packet.get_register(PACKET_LENGTH_REG_IDX) - + packet.set_register(RegisterAccess::PACKET_LENGTH_REG_IDX, + packet.get_register(RegisterAccess::PACKET_LENGTH_REG_IDX) - hdr.get_nbytes_packet()); hdr.mark_invalid(); } @@ -284,8 +284,12 @@ class clone_ingress_pkt_to_egress : public ActionPrimitive { void operator ()(const Data &mirror_session_id, const Data &field_list_id) { auto &packet = get_packet(); + // Assume mirror_session_id < (1u << 15) so that we can use most + // significant bit to always make the value non-0. This enables + // cleanly supporting mirror_session_id == 0, in case that is ever + // helpful. RegisterAccess::set_clone_mirror_session_id(packet, - mirror_session_id.get()); + mirror_session_id.get() | (1u << 15)); RegisterAccess::set_clone_field_list(packet, field_list_id.get()); } @@ -297,8 +301,9 @@ class clone_egress_pkt_to_egress : public ActionPrimitive { void operator ()(const Data &mirror_session_id, const Data &field_list_id) { auto &packet = get_packet(); + // See clone_ingress_pkt_to_egress for why the arithmetic. RegisterAccess::set_clone_mirror_session_id(packet, - mirror_session_id.get()); + mirror_session_id.get() | (1u << 15)); RegisterAccess::set_clone_field_list(packet, field_list_id.get()); } diff --git a/targets/simple_switch/register_access.h b/targets/simple_switch/register_access.h index 2affce86d..b377f0572 100644 --- a/targets/simple_switch/register_access.h +++ b/targets/simple_switch/register_access.h @@ -21,28 +21,28 @@ #ifndef SIMPLE_SWITCH_REGISTER_ACCESS_H_ #define SIMPLE_SWITCH_REGISTER_ACCESS_H_ -#define PACKET_LENGTH_REG_IDX 0 -#define CLONE_MIRROR_SESSION_ID_REG_IDX 1 -#define CLONE_MIRROR_SESSION_ID_MASK 0x000000000000ffff -#define CLONE_MIRROR_SESSION_ID_SHIFT 0 -#define CLONE_FIELD_LIST_REG_IDX 1 -#define CLONE_FIELD_LIST_MASK 0x00000000ffff0000 -#define CLONE_FIELD_LIST_SHIFT 16 -#define LF_FIELD_LIST_REG_IDX 1 -#define LF_FIELD_LIST_MASK 0x0000ffff00000000 -#define LF_FIELD_LIST_SHIFT 32 -#define RESUBMIT_FLAG_REG_IDX 1 -#define RESUBMIT_FLAG_MASK 0xffff000000000000 -#define RESUBMIT_FLAG_SHIFT 48 +class RegisterAccess { + public: + static constexpr int PACKET_LENGTH_REG_IDX = 0; -#define RECIRCULATE_FLAG_REG_IDX 2 -#define RECIRCULATE_FLAG_MASK 0x000000000000ffff -#define RECIRCULATE_FLAG_SHIFT 0 + static constexpr int CLONE_MIRROR_SESSION_ID_REG_IDX = 1; + static constexpr uint64_t CLONE_MIRROR_SESSION_ID_MASK = 0x000000000000ffff; + static constexpr uint64_t CLONE_MIRROR_SESSION_ID_SHIFT = 0; + static constexpr int CLONE_FIELD_LIST_REG_IDX = 1; + static constexpr uint64_t CLONE_FIELD_LIST_MASK = 0x00000000ffff0000; + static constexpr uint64_t CLONE_FIELD_LIST_SHIFT = 16; + static constexpr int LF_FIELD_LIST_REG_IDX = 1; + static constexpr uint64_t LF_FIELD_LIST_MASK = 0x0000ffff00000000; + static constexpr uint64_t LF_FIELD_LIST_SHIFT = 32; + static constexpr int RESUBMIT_FLAG_REG_IDX = 1; + static constexpr uint64_t RESUBMIT_FLAG_MASK = 0xffff000000000000; + static constexpr uint64_t RESUBMIT_FLAG_SHIFT = 48; + static constexpr int RECIRCULATE_FLAG_REG_IDX = 2; + static constexpr uint64_t RECIRCULATE_FLAG_MASK = 0x000000000000ffff; + static constexpr uint64_t RECIRCULATE_FLAG_SHIFT = 0; -class RegisterAccess { - public: static void clear_all(Packet &pkt) { // except do not clear packet length pkt.set_register(1, 0); @@ -50,8 +50,8 @@ class RegisterAccess { } static uint16_t get_clone_mirror_session_id(Packet &pkt) { uint64_t rv = pkt.get_register(CLONE_MIRROR_SESSION_ID_REG_IDX); - return (uint16_t) ((rv & CLONE_MIRROR_SESSION_ID_MASK) >> - CLONE_MIRROR_SESSION_ID_SHIFT); + return static_cast((rv & CLONE_MIRROR_SESSION_ID_MASK) >> + CLONE_MIRROR_SESSION_ID_SHIFT); } static void set_clone_mirror_session_id(Packet &pkt, uint16_t mirror_session_id) { @@ -63,8 +63,8 @@ class RegisterAccess { } static uint16_t get_clone_field_list(Packet &pkt) { uint64_t rv = pkt.get_register(CLONE_FIELD_LIST_REG_IDX); - return (uint16_t) ((rv & CLONE_FIELD_LIST_MASK) >> - CLONE_FIELD_LIST_SHIFT); + return static_cast((rv & CLONE_FIELD_LIST_MASK) >> + CLONE_FIELD_LIST_SHIFT); } static void set_clone_field_list(Packet &pkt, uint16_t field_list_id) { uint64_t rv = pkt.get_register(CLONE_FIELD_LIST_REG_IDX); @@ -74,7 +74,8 @@ class RegisterAccess { } static uint16_t get_lf_field_list(Packet &pkt) { uint64_t rv = pkt.get_register(LF_FIELD_LIST_REG_IDX); - return (uint16_t) ((rv & LF_FIELD_LIST_MASK) >> LF_FIELD_LIST_SHIFT); + return static_cast((rv & LF_FIELD_LIST_MASK) >> + LF_FIELD_LIST_SHIFT); } static void set_lf_field_list(Packet &pkt, uint16_t field_list_id) { uint64_t rv = pkt.get_register(LF_FIELD_LIST_REG_IDX); @@ -84,7 +85,8 @@ class RegisterAccess { } static uint16_t get_resubmit_flag(Packet &pkt) { uint64_t rv = pkt.get_register(RESUBMIT_FLAG_REG_IDX); - return (uint16_t) ((rv & RESUBMIT_FLAG_MASK) >> RESUBMIT_FLAG_SHIFT); + return static_cast((rv & RESUBMIT_FLAG_MASK) >> + RESUBMIT_FLAG_SHIFT); } static void set_resubmit_flag(Packet &pkt, uint16_t field_list_id) { uint64_t rv = pkt.get_register(RESUBMIT_FLAG_REG_IDX); @@ -94,8 +96,8 @@ class RegisterAccess { } static uint16_t get_recirculate_flag(Packet &pkt) { uint64_t rv = pkt.get_register(RECIRCULATE_FLAG_REG_IDX); - return (uint16_t) ((rv & RECIRCULATE_FLAG_MASK) >> - RECIRCULATE_FLAG_SHIFT); + return static_cast((rv & RECIRCULATE_FLAG_MASK) >> + RECIRCULATE_FLAG_SHIFT); } static void set_recirculate_flag(Packet &pkt, uint16_t field_list_id) { uint64_t rv = pkt.get_register(RECIRCULATE_FLAG_REG_IDX); diff --git a/targets/simple_switch/simple_switch.cpp b/targets/simple_switch/simple_switch.cpp index f56cf4bfc..0063ad886 100644 --- a/targets/simple_switch/simple_switch.cpp +++ b/targets/simple_switch/simple_switch.cpp @@ -251,7 +251,7 @@ SimpleSwitch::receive_(port_t port_num, const char *buffer, int len) { phv->get_field("standard_metadata.ingress_port").set(port_num); // using packet register 0 to store length, this register will be updated for // each add_header / remove_header primitive call - packet->set_register(PACKET_LENGTH_REG_IDX, len); + packet->set_register(RegisterAccess::PACKET_LENGTH_REG_IDX, len); phv->get_field("standard_metadata.packet_length").set(len); Field &f_instance_type = phv->get_field("standard_metadata.instance_type"); f_instance_type.set(PKT_INSTANCE_TYPE_NORMAL); @@ -440,14 +440,16 @@ SimpleSwitch::multicast(Packet *packet, unsigned int mgid) { auto *phv = packet->get_phv(); auto &f_rid = phv->get_field("intrinsic_metadata.egress_rid"); const auto pre_out = pre->replicate({mgid}); - auto packet_size = packet->get_register(PACKET_LENGTH_REG_IDX); + auto packet_size = + packet->get_register(RegisterAccess::PACKET_LENGTH_REG_IDX); for (const auto &out : pre_out) { auto egress_port = out.egress_port; BMLOG_DEBUG_PKT(*packet, "Replicating packet on port {}", egress_port); f_rid.set(out.rid); std::unique_ptr packet_copy = packet->clone_with_phv_ptr(); RegisterAccess::clear_all(*packet_copy); - packet_copy->set_register(PACKET_LENGTH_REG_IDX, packet_size); + packet_copy->set_register(RegisterAccess::PACKET_LENGTH_REG_IDX, + packet_size); enqueue(egress_port, std::move(packet_copy)); } } @@ -472,7 +474,8 @@ SimpleSwitch::ingress_thread() { BMLOG_DEBUG_PKT(*packet, "Processing packet received on port {}", ingress_port); - auto ingress_packet_size = packet->get_register(PACKET_LENGTH_REG_IDX); + auto ingress_packet_size = + packet->get_register(RegisterAccess::PACKET_LENGTH_REG_IDX); /* This looks like it comes out of the blue. However this is needed for ingress cloning. The parser updates the buffer state (pops the parsed @@ -501,10 +504,9 @@ SimpleSwitch::ingress_thread() { Field &f_egress_spec = phv->get_field("standard_metadata.egress_spec"); port_t egress_spec = f_egress_spec.get_uint(); - unsigned int clone_mirror_session_id = + auto clone_mirror_session_id = RegisterAccess::get_clone_mirror_session_id(*packet); - unsigned int clone_field_list = - RegisterAccess::get_clone_field_list(*packet); + auto clone_field_list = RegisterAccess::get_clone_field_list(*packet); int learn_id = RegisterAccess::get_lf_field_list(*packet); unsigned int mgid = 0u; @@ -522,6 +524,9 @@ SimpleSwitch::ingress_thread() { RegisterAccess::set_clone_mirror_session_id(*packet, 0); RegisterAccess::set_clone_field_list(*packet, 0); MirroringSessionConfig config; + // Clear upper bit of clone_mirror_session_id, which is always + // set if a clone operation was performed. + clone_mirror_session_id &= 0x7fffu; bool is_session_configured = mirroring_get_session( static_cast(clone_mirror_session_id), &config); if (is_session_configured) { @@ -531,7 +536,8 @@ SimpleSwitch::ingress_thread() { p4object_id_t field_list_id = clone_field_list; std::unique_ptr packet_copy = packet->clone_no_phv_ptr(); RegisterAccess::clear_all(*packet_copy); - packet_copy->set_register(PACKET_LENGTH_REG_IDX, ingress_packet_size); + packet_copy->set_register(RegisterAccess::PACKET_LENGTH_REG_IDX, + ingress_packet_size); // we need to parse again // the alternative would be to pay the (huge) price of PHV copy for // every ingress packet @@ -558,7 +564,7 @@ SimpleSwitch::ingress_thread() { } // RESUBMIT - unsigned int resubmit_flag = RegisterAccess::get_resubmit_flag(*packet); + auto resubmit_flag = RegisterAccess::get_resubmit_flag(*packet); if (resubmit_flag) { BMLOG_DEBUG_PKT(*packet, "Resubmitting packet"); // get the packet ready for being parsed again at the beginning of @@ -650,14 +656,13 @@ SimpleSwitch::egress_thread(size_t worker_id) { f_egress_spec.set(0); phv->get_field("standard_metadata.packet_length").set( - packet->get_register(PACKET_LENGTH_REG_IDX)); + packet->get_register(RegisterAccess::PACKET_LENGTH_REG_IDX)); egress_mau->apply(packet.get()); - unsigned int clone_mirror_session_id = + auto clone_mirror_session_id = RegisterAccess::get_clone_mirror_session_id(*packet); - unsigned int clone_field_list = - RegisterAccess::get_clone_field_list(*packet); + auto clone_field_list = RegisterAccess::get_clone_field_list(*packet); // EGRESS CLONING if (clone_mirror_session_id) { @@ -665,6 +670,9 @@ SimpleSwitch::egress_thread(size_t worker_id) { RegisterAccess::set_clone_mirror_session_id(*packet, 0); RegisterAccess::set_clone_field_list(*packet, 0); MirroringSessionConfig config; + // Clear upper bit of clone_mirror_session_id, which is always + // set if a clone operation was performed. + clone_mirror_session_id &= 0x7fffu; bool is_session_configured = mirroring_get_session( static_cast(clone_mirror_session_id), &config); if (is_session_configured) { @@ -699,8 +707,7 @@ SimpleSwitch::egress_thread(size_t worker_id) { deparser->deparse(packet.get()); // RECIRCULATE - unsigned int recirculate_flag = - RegisterAccess::get_recirculate_flag(*packet); + auto recirculate_flag = RegisterAccess::get_recirculate_flag(*packet); if (recirculate_flag) { BMLOG_DEBUG_PKT(*packet, "Recirculating packet"); p4object_id_t field_list_id = recirculate_flag; @@ -716,7 +723,8 @@ SimpleSwitch::egress_thread(size_t worker_id) { .set(PKT_INSTANCE_TYPE_RECIRC); size_t packet_size = packet_copy->get_data_size(); RegisterAccess::clear_all(*packet_copy); - packet_copy->set_register(PACKET_LENGTH_REG_IDX, packet_size); + packet_copy->set_register(RegisterAccess::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? From 6a2cdc26f3db6c1791be1c237e812454560fceec Mon Sep 17 00:00:00 2001 From: Andy Fingerhut Date: Tue, 23 Apr 2019 13:27:02 -0700 Subject: [PATCH 4/6] Fix cpplint warning --- targets/simple_switch/primitives.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/targets/simple_switch/primitives.cpp b/targets/simple_switch/primitives.cpp index 60613c324..855caf366 100644 --- a/targets/simple_switch/primitives.cpp +++ b/targets/simple_switch/primitives.cpp @@ -241,8 +241,8 @@ class add_header : public ActionPrimitive
{ // updated the length packet register (register 0) auto &packet = get_packet(); packet.set_register(RegisterAccess::PACKET_LENGTH_REG_IDX, - packet.get_register(RegisterAccess::PACKET_LENGTH_REG_IDX) + - hdr.get_nbytes_packet()); + packet.get_register(RegisterAccess::PACKET_LENGTH_REG_IDX) + + hdr.get_nbytes_packet()); } } }; @@ -263,8 +263,8 @@ class remove_header : public ActionPrimitive
{ // updated the length packet register (register 0) auto &packet = get_packet(); packet.set_register(RegisterAccess::PACKET_LENGTH_REG_IDX, - packet.get_register(RegisterAccess::PACKET_LENGTH_REG_IDX) - - hdr.get_nbytes_packet()); + packet.get_register(RegisterAccess::PACKET_LENGTH_REG_IDX) - + hdr.get_nbytes_packet()); hdr.mark_invalid(); } } From 6bc1c318602d2670fc479fd0e3c8f9f65fa3741b Mon Sep 17 00:00:00 2001 From: Andy Fingerhut Date: Wed, 24 Apr 2019 10:03:42 -0700 Subject: [PATCH 5/6] Update new methods to take Packet ptr rather than ref --- targets/simple_switch/primitives.cpp | 15 +++---- targets/simple_switch/register_access.h | 56 ++++++++++++------------- targets/simple_switch/simple_switch.cpp | 38 ++++++++--------- 3 files changed, 55 insertions(+), 54 deletions(-) diff --git a/targets/simple_switch/primitives.cpp b/targets/simple_switch/primitives.cpp index 855caf366..b6a7a35ab 100644 --- a/targets/simple_switch/primitives.cpp +++ b/targets/simple_switch/primitives.cpp @@ -226,7 +226,7 @@ class generate_digest : public ActionPrimitive { // discared receiver for now (void) receiver; auto &packet = get_packet(); - RegisterAccess::set_lf_field_list(packet, learn_id.get()); + RegisterAccess::set_lf_field_list(&packet, learn_id.get()); } }; @@ -288,9 +288,9 @@ class clone_ingress_pkt_to_egress // significant bit to always make the value non-0. This enables // cleanly supporting mirror_session_id == 0, in case that is ever // helpful. - RegisterAccess::set_clone_mirror_session_id(packet, + RegisterAccess::set_clone_mirror_session_id(&packet, mirror_session_id.get() | (1u << 15)); - RegisterAccess::set_clone_field_list(packet, + RegisterAccess::set_clone_field_list(&packet, field_list_id.get()); } }; @@ -302,9 +302,9 @@ class clone_egress_pkt_to_egress void operator ()(const Data &mirror_session_id, const Data &field_list_id) { auto &packet = get_packet(); // See clone_ingress_pkt_to_egress for why the arithmetic. - RegisterAccess::set_clone_mirror_session_id(packet, + RegisterAccess::set_clone_mirror_session_id(&packet, mirror_session_id.get() | (1u << 15)); - RegisterAccess::set_clone_field_list(packet, + RegisterAccess::set_clone_field_list(&packet, field_list_id.get()); } }; @@ -314,7 +314,7 @@ REGISTER_PRIMITIVE(clone_egress_pkt_to_egress); class resubmit : public ActionPrimitive { void operator ()(const Data &field_list_id) { auto &packet = get_packet(); - RegisterAccess::set_resubmit_flag(packet, field_list_id.get()); + RegisterAccess::set_resubmit_flag(&packet, field_list_id.get()); } }; @@ -323,7 +323,8 @@ REGISTER_PRIMITIVE(resubmit); class recirculate : public ActionPrimitive { void operator ()(const Data &field_list_id) { auto &packet = get_packet(); - RegisterAccess::set_recirculate_flag(packet, field_list_id.get()); + RegisterAccess::set_recirculate_flag(&packet, + field_list_id.get()); } }; diff --git a/targets/simple_switch/register_access.h b/targets/simple_switch/register_access.h index b377f0572..537576507 100644 --- a/targets/simple_switch/register_access.h +++ b/targets/simple_switch/register_access.h @@ -43,67 +43,67 @@ class RegisterAccess { static constexpr uint64_t RECIRCULATE_FLAG_MASK = 0x000000000000ffff; static constexpr uint64_t RECIRCULATE_FLAG_SHIFT = 0; - static void clear_all(Packet &pkt) { + static void clear_all(Packet *pkt) { // except do not clear packet length - pkt.set_register(1, 0); - pkt.set_register(2, 0); + pkt->set_register(1, 0); + pkt->set_register(2, 0); } - static uint16_t get_clone_mirror_session_id(Packet &pkt) { - uint64_t rv = pkt.get_register(CLONE_MIRROR_SESSION_ID_REG_IDX); + static uint16_t get_clone_mirror_session_id(Packet *pkt) { + uint64_t rv = pkt->get_register(CLONE_MIRROR_SESSION_ID_REG_IDX); return static_cast((rv & CLONE_MIRROR_SESSION_ID_MASK) >> CLONE_MIRROR_SESSION_ID_SHIFT); } - static void set_clone_mirror_session_id(Packet &pkt, + static void set_clone_mirror_session_id(Packet *pkt, uint16_t mirror_session_id) { - uint64_t rv = pkt.get_register(CLONE_MIRROR_SESSION_ID_REG_IDX); + uint64_t rv = pkt->get_register(CLONE_MIRROR_SESSION_ID_REG_IDX); rv = ((rv & ~CLONE_MIRROR_SESSION_ID_MASK) | (((uint64_t) mirror_session_id) << CLONE_MIRROR_SESSION_ID_SHIFT)); - pkt.set_register(CLONE_MIRROR_SESSION_ID_REG_IDX, rv); + pkt->set_register(CLONE_MIRROR_SESSION_ID_REG_IDX, rv); } - static uint16_t get_clone_field_list(Packet &pkt) { - uint64_t rv = pkt.get_register(CLONE_FIELD_LIST_REG_IDX); + static uint16_t get_clone_field_list(Packet *pkt) { + uint64_t rv = pkt->get_register(CLONE_FIELD_LIST_REG_IDX); return static_cast((rv & CLONE_FIELD_LIST_MASK) >> CLONE_FIELD_LIST_SHIFT); } - static void set_clone_field_list(Packet &pkt, uint16_t field_list_id) { - uint64_t rv = pkt.get_register(CLONE_FIELD_LIST_REG_IDX); + static void set_clone_field_list(Packet *pkt, uint16_t field_list_id) { + uint64_t rv = pkt->get_register(CLONE_FIELD_LIST_REG_IDX); rv = ((rv & ~CLONE_FIELD_LIST_MASK) | (((uint64_t) field_list_id) << CLONE_FIELD_LIST_SHIFT)); - pkt.set_register(CLONE_FIELD_LIST_REG_IDX, rv); + pkt->set_register(CLONE_FIELD_LIST_REG_IDX, rv); } - static uint16_t get_lf_field_list(Packet &pkt) { - uint64_t rv = pkt.get_register(LF_FIELD_LIST_REG_IDX); + static uint16_t get_lf_field_list(Packet *pkt) { + uint64_t rv = pkt->get_register(LF_FIELD_LIST_REG_IDX); return static_cast((rv & LF_FIELD_LIST_MASK) >> LF_FIELD_LIST_SHIFT); } - static void set_lf_field_list(Packet &pkt, uint16_t field_list_id) { - uint64_t rv = pkt.get_register(LF_FIELD_LIST_REG_IDX); + static void set_lf_field_list(Packet *pkt, uint16_t field_list_id) { + uint64_t rv = pkt->get_register(LF_FIELD_LIST_REG_IDX); rv = ((rv & ~LF_FIELD_LIST_MASK) | (((uint64_t) field_list_id) << LF_FIELD_LIST_SHIFT)); - pkt.set_register(LF_FIELD_LIST_REG_IDX, rv); + pkt->set_register(LF_FIELD_LIST_REG_IDX, rv); } - static uint16_t get_resubmit_flag(Packet &pkt) { - uint64_t rv = pkt.get_register(RESUBMIT_FLAG_REG_IDX); + static uint16_t get_resubmit_flag(Packet *pkt) { + uint64_t rv = pkt->get_register(RESUBMIT_FLAG_REG_IDX); return static_cast((rv & RESUBMIT_FLAG_MASK) >> RESUBMIT_FLAG_SHIFT); } - static void set_resubmit_flag(Packet &pkt, uint16_t field_list_id) { - uint64_t rv = pkt.get_register(RESUBMIT_FLAG_REG_IDX); + static void set_resubmit_flag(Packet *pkt, uint16_t field_list_id) { + uint64_t rv = pkt->get_register(RESUBMIT_FLAG_REG_IDX); rv = ((rv & ~RESUBMIT_FLAG_MASK) | (((uint64_t) field_list_id) << RESUBMIT_FLAG_SHIFT)); - pkt.set_register(RESUBMIT_FLAG_REG_IDX, rv); + pkt->set_register(RESUBMIT_FLAG_REG_IDX, rv); } - static uint16_t get_recirculate_flag(Packet &pkt) { - uint64_t rv = pkt.get_register(RECIRCULATE_FLAG_REG_IDX); + static uint16_t get_recirculate_flag(Packet *pkt) { + uint64_t rv = pkt->get_register(RECIRCULATE_FLAG_REG_IDX); return static_cast((rv & RECIRCULATE_FLAG_MASK) >> RECIRCULATE_FLAG_SHIFT); } - static void set_recirculate_flag(Packet &pkt, uint16_t field_list_id) { - uint64_t rv = pkt.get_register(RECIRCULATE_FLAG_REG_IDX); + static void set_recirculate_flag(Packet *pkt, uint16_t field_list_id) { + uint64_t rv = pkt->get_register(RECIRCULATE_FLAG_REG_IDX); rv = ((rv & ~RECIRCULATE_FLAG_MASK) | (((uint64_t) field_list_id) << RECIRCULATE_FLAG_SHIFT)); - pkt.set_register(RECIRCULATE_FLAG_REG_IDX, rv); + pkt->set_register(RECIRCULATE_FLAG_REG_IDX, rv); } }; diff --git a/targets/simple_switch/simple_switch.cpp b/targets/simple_switch/simple_switch.cpp index 0063ad886..d8afe2688 100644 --- a/targets/simple_switch/simple_switch.cpp +++ b/targets/simple_switch/simple_switch.cpp @@ -244,7 +244,7 @@ SimpleSwitch::receive_(port_t port_num, const char *buffer, int len) { // many current P4 programs assume this // it is also part of the original P4 spec phv->reset_metadata(); - RegisterAccess::clear_all(*packet); + RegisterAccess::clear_all(packet.get()); // setting standard metadata @@ -447,7 +447,7 @@ SimpleSwitch::multicast(Packet *packet, unsigned int mgid) { BMLOG_DEBUG_PKT(*packet, "Replicating packet on port {}", egress_port); f_rid.set(out.rid); std::unique_ptr packet_copy = packet->clone_with_phv_ptr(); - RegisterAccess::clear_all(*packet_copy); + RegisterAccess::clear_all(packet_copy.get()); packet_copy->set_register(RegisterAccess::PACKET_LENGTH_REG_IDX, packet_size); enqueue(egress_port, std::move(packet_copy)); @@ -505,10 +505,10 @@ SimpleSwitch::ingress_thread() { port_t egress_spec = f_egress_spec.get_uint(); auto clone_mirror_session_id = - RegisterAccess::get_clone_mirror_session_id(*packet); - auto clone_field_list = RegisterAccess::get_clone_field_list(*packet); + RegisterAccess::get_clone_mirror_session_id(packet.get()); + auto clone_field_list = RegisterAccess::get_clone_field_list(packet.get()); - int learn_id = RegisterAccess::get_lf_field_list(*packet); + int learn_id = RegisterAccess::get_lf_field_list(packet.get()); unsigned int mgid = 0u; // detect mcast support, if this is true we assume that other fields needed @@ -521,8 +521,8 @@ SimpleSwitch::ingress_thread() { // INGRESS CLONING if (clone_mirror_session_id) { BMLOG_DEBUG_PKT(*packet, "Cloning packet at ingress"); - RegisterAccess::set_clone_mirror_session_id(*packet, 0); - RegisterAccess::set_clone_field_list(*packet, 0); + RegisterAccess::set_clone_mirror_session_id(packet.get(), 0); + RegisterAccess::set_clone_field_list(packet.get(), 0); MirroringSessionConfig config; // Clear upper bit of clone_mirror_session_id, which is always // set if a clone operation was performed. @@ -535,7 +535,7 @@ SimpleSwitch::ingress_thread() { packet->restore_buffer_state(packet_in_state); p4object_id_t field_list_id = clone_field_list; std::unique_ptr packet_copy = packet->clone_no_phv_ptr(); - RegisterAccess::clear_all(*packet_copy); + RegisterAccess::clear_all(packet_copy.get()); packet_copy->set_register(RegisterAccess::PACKET_LENGTH_REG_IDX, ingress_packet_size); // we need to parse again @@ -564,21 +564,21 @@ SimpleSwitch::ingress_thread() { } // RESUBMIT - auto resubmit_flag = RegisterAccess::get_resubmit_flag(*packet); + auto resubmit_flag = RegisterAccess::get_resubmit_flag(packet.get()); if (resubmit_flag) { BMLOG_DEBUG_PKT(*packet, "Resubmitting packet"); // get the packet ready for being parsed again at the beginning of // ingress packet->restore_buffer_state(packet_in_state); p4object_id_t field_list_id = resubmit_flag; - RegisterAccess::set_resubmit_flag(*packet, 0); + RegisterAccess::set_resubmit_flag(packet.get(), 0); // TODO(antonin): a copy is not needed here, but I don't yet have an // optimized way of doing this std::unique_ptr packet_copy = packet->clone_no_phv_ptr(); copy_field_list_and_set_type(packet, packet_copy, PKT_INSTANCE_TYPE_RESUBMIT, field_list_id); - RegisterAccess::clear_all(*packet_copy); + RegisterAccess::clear_all(packet_copy.get()); input_buffer->push_front( InputBuffer::PacketType::RESUBMIT, std::move(packet_copy)); continue; @@ -661,14 +661,14 @@ SimpleSwitch::egress_thread(size_t worker_id) { egress_mau->apply(packet.get()); auto clone_mirror_session_id = - RegisterAccess::get_clone_mirror_session_id(*packet); - auto clone_field_list = RegisterAccess::get_clone_field_list(*packet); + RegisterAccess::get_clone_mirror_session_id(packet.get()); + auto clone_field_list = RegisterAccess::get_clone_field_list(packet.get()); // EGRESS CLONING if (clone_mirror_session_id) { BMLOG_DEBUG_PKT(*packet, "Cloning packet at egress"); - RegisterAccess::set_clone_mirror_session_id(*packet, 0); - RegisterAccess::set_clone_field_list(*packet, 0); + RegisterAccess::set_clone_mirror_session_id(packet.get(), 0); + RegisterAccess::set_clone_field_list(packet.get(), 0); MirroringSessionConfig config; // Clear upper bit of clone_mirror_session_id, which is always // set if a clone operation was performed. @@ -691,7 +691,7 @@ SimpleSwitch::egress_thread(size_t worker_id) { if (config.egress_port_valid) { BMLOG_DEBUG_PKT(*packet, "Cloning packet to egress port {}", config.egress_port); - RegisterAccess::clear_all(*packet_copy); + RegisterAccess::clear_all(packet_copy.get()); enqueue(config.egress_port, std::move(packet_copy)); } } @@ -707,11 +707,11 @@ SimpleSwitch::egress_thread(size_t worker_id) { deparser->deparse(packet.get()); // RECIRCULATE - auto recirculate_flag = RegisterAccess::get_recirculate_flag(*packet); + auto recirculate_flag = RegisterAccess::get_recirculate_flag(packet.get()); if (recirculate_flag) { BMLOG_DEBUG_PKT(*packet, "Recirculating packet"); p4object_id_t field_list_id = recirculate_flag; - RegisterAccess::set_recirculate_flag(*packet, 0); + RegisterAccess::set_recirculate_flag(packet.get(), 0); FieldList *field_list = this->get_field_list(field_list_id); // TODO(antonin): just like for resubmit, there is no need for a copy // here, but it is more convenient for this first prototype @@ -722,7 +722,7 @@ SimpleSwitch::egress_thread(size_t worker_id) { phv_copy->get_field("standard_metadata.instance_type") .set(PKT_INSTANCE_TYPE_RECIRC); size_t packet_size = packet_copy->get_data_size(); - RegisterAccess::clear_all(*packet_copy); + RegisterAccess::clear_all(packet_copy.get()); packet_copy->set_register(RegisterAccess::PACKET_LENGTH_REG_IDX, packet_size); phv_copy->get_field("standard_metadata.packet_length").set(packet_size); From 009302c44d7ee72661aa103a59d4f53d28fb665b Mon Sep 17 00:00:00 2001 From: Andy Fingerhut Date: Thu, 25 Apr 2019 11:18:28 -0700 Subject: [PATCH 6/6] Change remaining C-style casts to static_cast Also add range checking on mirror session id values when control plane tries to add or delete one. --- targets/simple_switch/primitives.cpp | 15 +++++++------ targets/simple_switch/register_access.h | 16 +++++++++----- targets/simple_switch/simple_switch.cpp | 28 +++++++++++++++++-------- 3 files changed, 39 insertions(+), 20 deletions(-) diff --git a/targets/simple_switch/primitives.cpp b/targets/simple_switch/primitives.cpp index b6a7a35ab..784f57575 100644 --- a/targets/simple_switch/primitives.cpp +++ b/targets/simple_switch/primitives.cpp @@ -284,12 +284,14 @@ class clone_ingress_pkt_to_egress : public ActionPrimitive { void operator ()(const Data &mirror_session_id, const Data &field_list_id) { auto &packet = get_packet(); - // Assume mirror_session_id < (1u << 15) so that we can use most - // significant bit to always make the value non-0. This enables - // cleanly supporting mirror_session_id == 0, in case that is ever - // helpful. + // We limit mirror_session_id values to small enough values that + // we can use one of the bit positions as a "clone was performed" + // indicator, making mirror_seesion_id stored here always non-0 if + // a clone was done. This enables cleanly supporting + // mirror_session_id == 0, in case that is ever helpful. RegisterAccess::set_clone_mirror_session_id(&packet, - mirror_session_id.get() | (1u << 15)); + mirror_session_id.get() | + RegisterAccess::MIRROR_SESSION_ID_VALID_MASK); RegisterAccess::set_clone_field_list(&packet, field_list_id.get()); } @@ -303,7 +305,8 @@ class clone_egress_pkt_to_egress auto &packet = get_packet(); // See clone_ingress_pkt_to_egress for why the arithmetic. RegisterAccess::set_clone_mirror_session_id(&packet, - mirror_session_id.get() | (1u << 15)); + mirror_session_id.get() | + RegisterAccess::MIRROR_SESSION_ID_VALID_MASK); RegisterAccess::set_clone_field_list(&packet, field_list_id.get()); } diff --git a/targets/simple_switch/register_access.h b/targets/simple_switch/register_access.h index 537576507..b5a618b38 100644 --- a/targets/simple_switch/register_access.h +++ b/targets/simple_switch/register_access.h @@ -43,6 +43,10 @@ class RegisterAccess { static constexpr uint64_t RECIRCULATE_FLAG_MASK = 0x000000000000ffff; static constexpr uint64_t RECIRCULATE_FLAG_SHIFT = 0; + static constexpr uint16_t MAX_MIRROR_SESSION_ID = (1u << 15) - 1; + static constexpr uint16_t MIRROR_SESSION_ID_VALID_MASK = (1u << 15); + static constexpr uint16_t MIRROR_SESSION_ID_MASK = 0x7FFFu; + static void clear_all(Packet *pkt) { // except do not clear packet length pkt->set_register(1, 0); @@ -57,7 +61,7 @@ class RegisterAccess { uint16_t mirror_session_id) { uint64_t rv = pkt->get_register(CLONE_MIRROR_SESSION_ID_REG_IDX); rv = ((rv & ~CLONE_MIRROR_SESSION_ID_MASK) | - (((uint64_t) mirror_session_id) << + ((static_cast(mirror_session_id)) << CLONE_MIRROR_SESSION_ID_SHIFT)); pkt->set_register(CLONE_MIRROR_SESSION_ID_REG_IDX, rv); } @@ -69,7 +73,8 @@ class RegisterAccess { static void set_clone_field_list(Packet *pkt, uint16_t field_list_id) { uint64_t rv = pkt->get_register(CLONE_FIELD_LIST_REG_IDX); rv = ((rv & ~CLONE_FIELD_LIST_MASK) | - (((uint64_t) field_list_id) << CLONE_FIELD_LIST_SHIFT)); + ((static_cast(field_list_id)) << + CLONE_FIELD_LIST_SHIFT)); pkt->set_register(CLONE_FIELD_LIST_REG_IDX, rv); } static uint16_t get_lf_field_list(Packet *pkt) { @@ -80,7 +85,7 @@ class RegisterAccess { static void set_lf_field_list(Packet *pkt, uint16_t field_list_id) { uint64_t rv = pkt->get_register(LF_FIELD_LIST_REG_IDX); rv = ((rv & ~LF_FIELD_LIST_MASK) | - (((uint64_t) field_list_id) << LF_FIELD_LIST_SHIFT)); + ((static_cast(field_list_id)) << LF_FIELD_LIST_SHIFT)); pkt->set_register(LF_FIELD_LIST_REG_IDX, rv); } static uint16_t get_resubmit_flag(Packet *pkt) { @@ -91,7 +96,7 @@ class RegisterAccess { static void set_resubmit_flag(Packet *pkt, uint16_t field_list_id) { uint64_t rv = pkt->get_register(RESUBMIT_FLAG_REG_IDX); rv = ((rv & ~RESUBMIT_FLAG_MASK) | - (((uint64_t) field_list_id) << RESUBMIT_FLAG_SHIFT)); + ((static_cast(field_list_id)) << RESUBMIT_FLAG_SHIFT)); pkt->set_register(RESUBMIT_FLAG_REG_IDX, rv); } static uint16_t get_recirculate_flag(Packet *pkt) { @@ -102,7 +107,8 @@ class RegisterAccess { static void set_recirculate_flag(Packet *pkt, uint16_t field_list_id) { uint64_t rv = pkt->get_register(RECIRCULATE_FLAG_REG_IDX); rv = ((rv & ~RECIRCULATE_FLAG_MASK) | - (((uint64_t) field_list_id) << RECIRCULATE_FLAG_SHIFT)); + ((static_cast(field_list_id)) << + RECIRCULATE_FLAG_SHIFT)); pkt->set_register(RECIRCULATE_FLAG_REG_IDX, rv); } }; diff --git a/targets/simple_switch/simple_switch.cpp b/targets/simple_switch/simple_switch.cpp index d8afe2688..469933d01 100644 --- a/targets/simple_switch/simple_switch.cpp +++ b/targets/simple_switch/simple_switch.cpp @@ -76,13 +76,23 @@ class SimpleSwitch::MirroringSessions { bool add_session(mirror_id_t mirror_id, const MirroringSessionConfig &config) { Lock lock(mutex); - sessions_map[mirror_id] = config; - return true; + if (0 <= mirror_id && mirror_id <= RegisterAccess::MAX_MIRROR_SESSION_ID) { + sessions_map[mirror_id] = config; + return true; + } else { + bm::Logger::get()->error("mirror_id out of range. No session added."); + return false; + } } bool delete_session(mirror_id_t mirror_id) { Lock lock(mutex); - return sessions_map.erase(mirror_id) == 1; + if (0 <= mirror_id && mirror_id <= RegisterAccess::MAX_MIRROR_SESSION_ID) { + return sessions_map.erase(mirror_id) == 1; + } else { + bm::Logger::get()->error("mirror_id out of range. No session deleted."); + return false; + } } bool get_session(mirror_id_t mirror_id, @@ -524,9 +534,9 @@ SimpleSwitch::ingress_thread() { RegisterAccess::set_clone_mirror_session_id(packet.get(), 0); RegisterAccess::set_clone_field_list(packet.get(), 0); MirroringSessionConfig config; - // Clear upper bit of clone_mirror_session_id, which is always - // set if a clone operation was performed. - clone_mirror_session_id &= 0x7fffu; + // Extract the part of clone_mirror_session_id that contains the + // actual session id. + clone_mirror_session_id &= RegisterAccess::MIRROR_SESSION_ID_MASK; bool is_session_configured = mirroring_get_session( static_cast(clone_mirror_session_id), &config); if (is_session_configured) { @@ -670,9 +680,9 @@ SimpleSwitch::egress_thread(size_t worker_id) { RegisterAccess::set_clone_mirror_session_id(packet.get(), 0); RegisterAccess::set_clone_field_list(packet.get(), 0); MirroringSessionConfig config; - // Clear upper bit of clone_mirror_session_id, which is always - // set if a clone operation was performed. - clone_mirror_session_id &= 0x7fffu; + // Extract the part of clone_mirror_session_id that contains the + // actual session id. + clone_mirror_session_id &= RegisterAccess::MIRROR_SESSION_ID_MASK; bool is_session_configured = mirroring_get_session( static_cast(clone_mirror_session_id), &config); if (is_session_configured) {