From f5711d49188013794ab03f263b04fef49c08db15 Mon Sep 17 00:00:00 2001 From: Aaron Bernardino Date: Sun, 28 Jun 2026 01:59:28 +0000 Subject: [PATCH 1/5] [vpp] Handle RIF loopback packet action attribute in saivpp Translate SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION on router interface create and set into a call to the iface_loopback VPP plugin's binary API. SwitchVppRif: add vpp_set_interface_loopback_action(), wired into both vpp_create_router_interface and vpp_update_router_interface. The RIF is resolved to its hwif name via the existing vpp_get_hwif_name() helper (handles port, LAG/BondEthernet, and sub-ports); SAI_PACKET_ACTION_DROP maps to 1, everything else to 0 (forward). SaiVppXlate: add the iface_loopback plugin API include block, msg_id_base registration, the set_action reply handler, and a vpp_iface_loopback_set_action() send helper that resolves the hwif name to a sw_if_index via the existing get_swif_idx() helper. Depends on the iface_loopback plugin in sonic-platform-vpp. Issue: sonic-net/sonic-buildimage#25788 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Aaron Bernardino --- vslib/vpp/SwitchVpp.h | 5 +++ vslib/vpp/SwitchVppRif.cpp | 41 +++++++++++++++++ vslib/vpp/vppxlate/SaiVppXlate.c | 75 +++++++++++++++++++++++++++++++- vslib/vpp/vppxlate/SaiVppXlate.h | 1 + 4 files changed, 120 insertions(+), 2 deletions(-) diff --git a/vslib/vpp/SwitchVpp.h b/vslib/vpp/SwitchVpp.h index 26eab9f10e..fd2e45be68 100644 --- a/vslib/vpp/SwitchVpp.h +++ b/vslib/vpp/SwitchVpp.h @@ -709,6 +709,11 @@ namespace saivs _In_ uint32_t vlan_id, _In_ uint32_t mtu); + sai_status_t vpp_set_interface_loopback_action ( + _In_ sai_object_id_t object_id, + _In_ uint32_t vlan_id, + _In_ int32_t packet_action); + sai_status_t UpdatePort( _In_ sai_object_id_t object_id, _In_ uint32_t attr_count, diff --git a/vslib/vpp/SwitchVppRif.cpp b/vslib/vpp/SwitchVppRif.cpp index 2a07d209b9..9f55156e99 100644 --- a/vslib/vpp/SwitchVppRif.cpp +++ b/vslib/vpp/SwitchVppRif.cpp @@ -572,6 +572,33 @@ sai_status_t SwitchVpp::vpp_set_interface_mtu ( return SAI_STATUS_SUCCESS; } +sai_status_t SwitchVpp::vpp_set_interface_loopback_action ( + _In_ sai_object_id_t object_id, + _In_ uint32_t vlan_id, + _In_ int32_t packet_action) +{ + SWSS_LOG_ENTER(); + + if (is_ip_nbr_active() == false) { + return SAI_STATUS_SUCCESS; + } + + std::string ifname; + + if (vpp_get_hwif_name(object_id, vlan_id, ifname) == true) { + const char *hwif_name = ifname.c_str(); + int action = (packet_action == SAI_PACKET_ACTION_DROP) ? 1 : 0; + + int ret = vpp_iface_loopback_set_action(hwif_name, action); + SWSS_LOG_NOTICE("Setting router interface loopback action %s to %s (ret %d)", + hwif_name, action ? "drop" : "forward", ret); + if (ret != 0) { + return SAI_STATUS_FAILURE; + } + } + return SAI_STATUS_SUCCESS; +} + sai_status_t SwitchVpp::UpdatePort( _In_ sai_object_id_t object_id, _In_ uint32_t attr_count, @@ -1659,6 +1686,13 @@ sai_status_t SwitchVpp::vpp_create_router_interface( vpp_set_interface_mtu(obj_id, vlan_id, attr_type_mtu->value.u32); } + auto attr_loopback = sai_metadata_get_attr_by_id(SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION, attr_count, attr_list); + + if (attr_loopback != NULL) + { + vpp_set_interface_loopback_action(obj_id, vlan_id, attr_loopback->value.s32); + } + bool v4_is_up = false, v6_is_up = false; auto attr_type_v4 = sai_metadata_get_attr_by_id(SAI_ROUTER_INTERFACE_ATTR_ADMIN_V4_STATE, attr_count, attr_list); @@ -1756,6 +1790,13 @@ sai_status_t SwitchVpp::vpp_update_router_interface( vpp_set_interface_mtu(obj_id, vlan_id, attr_type_mtu->value.u32); } + auto attr_loopback = sai_metadata_get_attr_by_id(SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION, attr_count, attr_list); + + if (attr_loopback != NULL) + { + vpp_set_interface_loopback_action(obj_id, vlan_id, attr_loopback->value.s32); + } + bool v4_is_up = false, v6_is_up = false; auto attr_type_v4 = sai_metadata_get_attr_by_id(SAI_ROUTER_INTERFACE_ATTR_ADMIN_V4_STATE, attr_count, attr_list); diff --git a/vslib/vpp/vppxlate/SaiVppXlate.c b/vslib/vpp/vppxlate/SaiVppXlate.c index 3cd77edf28..e6cf5ad659 100644 --- a/vslib/vpp/vppxlate/SaiVppXlate.c +++ b/vslib/vpp/vppxlate/SaiVppXlate.c @@ -56,6 +56,9 @@ #include #include +#include +#include + #include #include @@ -118,6 +121,24 @@ #include #undef vl_api_version +/* iface_loopback API inclusion */ + +#define vl_typedefs +#include +#undef vl_typedefs + +#define vl_endianfun +#include +#undef vl_endianfun + +#define vl_calcsizefun +#include +#undef vl_calcsizefun + +#define vl_api_version(n, v) static u32 iface_loopback_api_version = v; +#include +#undef vl_api_version + /* interface API inclusion */ #define vl_typedefs @@ -1288,6 +1309,13 @@ vl_api_tunterm_acl_interface_add_del_reply_t_handler(vl_api_tunterm_acl_interfac set_reply_status(retval); } +static void +vl_api_iface_loopback_set_action_reply_t_handler(vl_api_iface_loopback_set_action_reply_t *msg) +{ + int retval = (int)ntohl((uint32_t)msg->retval); + set_reply_status(retval); +} + static void vl_api_bond_create_reply_t_handler (vl_api_bond_create_reply_t *msg) { @@ -1374,6 +1402,7 @@ vl_api_sr_set_encap_source_reply_t_handler(vl_api_sr_set_encap_source_reply_t *m static u16 interface_msg_id_base, memclnt_msg_id_base, __plugin_msg_base; static u16 l2_msg_id_base, vxlan_msg_id_base, ipip_msg_id_base; static u16 tunterm_msg_id_base; +static u16 iface_loopback_msg_id_base; static u16 bfd_msg_id_base; static u16 sr_msg_id_base; static u16 bond_msg_id_base; @@ -1541,6 +1570,9 @@ vl_api_acl_interface_add_del_reply_t_handler(vl_api_acl_interface_add_del_reply_ #define TUNTERM_MSG_ID(id) \ (VL_API_##id + tunterm_msg_id_base) +#define IFACE_LOOPBACK_MSG_ID(id) \ + (VL_API_##id + iface_loopback_msg_id_base) + #define VXLAN_MSG_ID(id) \ (VL_API_##id + vxlan_msg_id_base) @@ -1569,7 +1601,8 @@ vl_api_acl_interface_add_del_reply_t_handler(vl_api_acl_interface_add_del_reply_ _(SFLOW_MSG_ID(SFLOW_ENABLE_DISABLE_REPLY), sflow_enable_disable_reply) \ _(SFLOW_MSG_ID(SFLOW_SAMPLING_RATE_SET_REPLY), sflow_sampling_rate_set_reply) \ _(IPIP_MSG_ID(IPIP_ADD_TUNNEL_REPLY), ipip_add_tunnel_reply) \ - _(IPIP_MSG_ID(IPIP_DEL_TUNNEL_REPLY), ipip_del_tunnel_reply) + _(IPIP_MSG_ID(IPIP_DEL_TUNNEL_REPLY), ipip_del_tunnel_reply) \ + _(IFACE_LOOPBACK_MSG_ID(IFACE_LOOPBACK_SET_ACTION_REPLY), iface_loopback_set_action_reply) static void vpp_plugin_vpe_init(void) { @@ -1645,6 +1678,10 @@ static void get_base_msg_id() msg_base_lookup_name = format (0, "sflow_%08x%c", sflow_api_version, 0); sflow_msg_id_base = vl_client_get_first_plugin_msg_id ((char *) msg_base_lookup_name); assert(sflow_msg_id_base != (u16) ~0); + + msg_base_lookup_name = format (0, "iface_loopback_%08x%c", iface_loopback_api_version, 0); + iface_loopback_msg_id_base = vl_client_get_first_plugin_msg_id ((char *) msg_base_lookup_name); + assert(iface_loopback_msg_id_base != (u16) ~0); } #define API_SOCKET_FILE "/run/vpp/api.sock" @@ -1900,6 +1937,40 @@ static int __delete_loopback (vat_main_t *vam, const char *hwif_name, u32 instan return ret; } +int vpp_iface_loopback_set_action (const char *hwif_name, int action) +{ + vat_main_t *vam = &vat_main; + vl_api_iface_loopback_set_action_t *mp; + u32 idx; + int ret; + + VPP_LOCK(); + + __plugin_msg_base = iface_loopback_msg_id_base; + + idx = get_swif_idx(vam, hwif_name); + if (idx == (u32) -1) { + SAIVPP_ERROR("Unable to get sw_index for %s", hwif_name); + VPP_UNLOCK(); + return -EINVAL; + } + + M (IFACE_LOOPBACK_SET_ACTION, mp); + mp->sw_if_index = htonl(idx); + mp->action = (u8) action; + + S (mp); + + WR (ret); + + if (ret) { SAIVPP_ERROR("%s failed(%d) %s action %d", __func__, ret, hwif_name, action); } + else { SAIVPP_INFO("%s %s action %d", __func__, hwif_name, action); } + + VPP_UNLOCK(); + + return ret; +} + static int __create_sub_interface (vat_main_t *vam, vl_api_interface_index_t if_idx, u32 sub_id, u16 vlan_id) { vl_api_create_subif_t *mp; @@ -4723,7 +4794,7 @@ static int __sw_interface_get_table(uint32_t sw_if_index, bool is_ipv6, uint32_t return ret; } -int vpp_sw_interface_find_by_ip(vpp_ip_addr_t *search_ip, uint32_t vrf_id, +int vpp_sw_interface_find_by_ip(vpp_ip_addr_t *search_ip, uint32_t vrf_id, uint32_t *out_sw_if_index) { int ret; diff --git a/vslib/vpp/vppxlate/SaiVppXlate.h b/vslib/vpp/vppxlate/SaiVppXlate.h index 352bfc1534..4755d22a6b 100644 --- a/vslib/vpp/vppxlate/SaiVppXlate.h +++ b/vslib/vpp/vppxlate/SaiVppXlate.h @@ -398,6 +398,7 @@ typedef enum { extern int vpp_sw_interface_find_by_ip(vpp_ip_addr_t *search_ip, uint32_t vrf_id, uint32_t *out_sw_if_index); + extern int vpp_iface_loopback_set_action(const char *hwif_name, int action); #ifdef __cplusplus } #endif From 1ae8b4d80f2c2923004c7f661979419e0f4002fa Mon Sep 17 00:00:00 2001 From: Aaron Bernardino Date: Mon, 29 Jun 2026 18:04:59 +0000 Subject: [PATCH 2/5] [vpp] Populate router interface stats from VPP counters RIF counters (show interfaces counters rif) read 0 for every router interface on VPP because only PORT stats were wired to the VPP stats segment (setPortStats); ROUTER_INTERFACE fell through to the base virtual-switch handler which returns 0. Add setRifStats(), mirroring setPortStats(): resolve the RIF to its VPP hardware interface (port/LAG, including sub-ports via the outer VLAN id) and map the VPP interface counters to SAI_ROUTER_INTERFACE_STAT_*, including tx-error -> SAI_ROUTER_INTERFACE_STAT_OUT_ERROR_PACKETS. Wire it into getStatsExt for SAI_OBJECT_TYPE_ROUTER_INTERFACE. This makes RIF counters reflect dataplane activity and, in particular, surfaces the loopback (hairpin) drop tx-error increment in the RIF tx_err counter. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Aaron Bernardino --- vslib/vpp/SwitchVpp.cpp | 67 +++++++++++++++++++++++++++++++++++++++++ vslib/vpp/SwitchVpp.h | 3 ++ 2 files changed, 70 insertions(+) diff --git a/vslib/vpp/SwitchVpp.cpp b/vslib/vpp/SwitchVpp.cpp index 890c6e8b80..bc039e43ec 100644 --- a/vslib/vpp/SwitchVpp.cpp +++ b/vslib/vpp/SwitchVpp.cpp @@ -803,6 +803,69 @@ void SwitchVpp::setPortStats( debugSetStats(oid, stats); } +void SwitchVpp::setRifStats( + _In_ sai_object_id_t oid) +{ + SWSS_LOG_ENTER(); + + sai_attribute_t attr; + + attr.id = SAI_ROUTER_INTERFACE_ATTR_TYPE; + if (get(SAI_OBJECT_TYPE_ROUTER_INTERFACE, oid, 1, &attr) != SAI_STATUS_SUCCESS) + { + return; + } + int32_t rif_type = attr.value.s32; + + attr.id = SAI_ROUTER_INTERFACE_ATTR_PORT_ID; + if (get(SAI_OBJECT_TYPE_ROUTER_INTERFACE, oid, 1, &attr) != SAI_STATUS_SUCCESS) + { + // Only port/LAG based RIFs (incl. sub-ports) are backed by a VPP + // hardware interface whose counters we can read here. + return; + } + sai_object_id_t port_oid = attr.value.oid; + + sai_object_type_t ot = objectTypeQuery(port_oid); + if (ot != SAI_OBJECT_TYPE_PORT && ot != SAI_OBJECT_TYPE_LAG) + { + return; + } + + uint16_t vlan_id = 0; + if (rif_type == SAI_ROUTER_INTERFACE_TYPE_SUB_PORT) + { + attr.id = SAI_ROUTER_INTERFACE_ATTR_OUTER_VLAN_ID; + if (get(SAI_OBJECT_TYPE_ROUTER_INTERFACE, oid, 1, &attr) != SAI_STATUS_SUCCESS) + { + return; + } + vlan_id = attr.value.u16; + } + + std::string if_name; + if (!vpp_get_hwif_name(port_oid, vlan_id, if_name)) + { + return; + } + + vpp_interface_stats_t rif_stats; + + if (vpp_intf_stats_query(if_name.c_str(), &rif_stats) == 0) + { + std::map stats; + + stats[SAI_ROUTER_INTERFACE_STAT_IN_PACKETS] = rif_stats.rx; + stats[SAI_ROUTER_INTERFACE_STAT_IN_OCTETS] = rif_stats.rx_bytes; + stats[SAI_ROUTER_INTERFACE_STAT_OUT_PACKETS] = rif_stats.tx; + stats[SAI_ROUTER_INTERFACE_STAT_OUT_OCTETS] = rif_stats.tx_bytes; + stats[SAI_ROUTER_INTERFACE_STAT_IN_ERROR_PACKETS] = rif_stats.rx_error; + stats[SAI_ROUTER_INTERFACE_STAT_OUT_ERROR_PACKETS] = rif_stats.tx_error; + + debugSetStats(oid, stats); + } +} + sai_status_t SwitchVpp::getRouteCounterStats( _In_ sai_object_id_t oid, _Out_ std::map& stats, @@ -1163,6 +1226,10 @@ sai_status_t SwitchVpp::getStatsExt( { setPortStats(object_id); } + else if (object_type == SAI_OBJECT_TYPE_ROUTER_INTERFACE) + { + setRifStats(object_id); + } else if (object_type == SAI_OBJECT_TYPE_COUNTER) { std::string route; diff --git a/vslib/vpp/SwitchVpp.h b/vslib/vpp/SwitchVpp.h index fd2e45be68..383397aa2b 100644 --- a/vslib/vpp/SwitchVpp.h +++ b/vslib/vpp/SwitchVpp.h @@ -95,6 +95,9 @@ namespace saivs void setPortStats( _In_ sai_object_id_t oid); + void setRifStats( + _In_ sai_object_id_t oid); + sai_status_t getRouteStatsExt( _In_ sai_object_id_t oid, _In_ uint32_t number_of_counters, From 0e785b7ce4db3f35884a8b959233c2f6f03dc827 Mon Sep 17 00:00:00 2001 From: Aaron Bernardino Date: Mon, 29 Jun 2026 19:17:34 +0000 Subject: [PATCH 3/5] [vpp] Support loopback packet action and stats for VLAN RIFs VLAN router interfaces are backed by the bridge BVI (bvi) rather than a port/LAG, so vpp_update_router_interface returned early for the VLAN type and never applied SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION, and setRifStats could not resolve their counters. Add a unified vpp_get_rif_hwif_name() resolver that maps any RIF (port, LAG, sub-port, or VLAN) to its backing VPP interface, handle the loopback action for VLAN RIFs on the BVI, and use the resolver in setRifStats so VLAN RIF counters are populated too. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Aaron Bernardino --- vslib/vpp/SwitchVpp.cpp | 41 ++-------------- vslib/vpp/SwitchVpp.h | 8 ++++ vslib/vpp/SwitchVppRif.cpp | 96 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 36 deletions(-) diff --git a/vslib/vpp/SwitchVpp.cpp b/vslib/vpp/SwitchVpp.cpp index bc039e43ec..3c6a443fb3 100644 --- a/vslib/vpp/SwitchVpp.cpp +++ b/vslib/vpp/SwitchVpp.cpp @@ -808,43 +808,12 @@ void SwitchVpp::setRifStats( { SWSS_LOG_ENTER(); - sai_attribute_t attr; - - attr.id = SAI_ROUTER_INTERFACE_ATTR_TYPE; - if (get(SAI_OBJECT_TYPE_ROUTER_INTERFACE, oid, 1, &attr) != SAI_STATUS_SUCCESS) - { - return; - } - int32_t rif_type = attr.value.s32; - - attr.id = SAI_ROUTER_INTERFACE_ATTR_PORT_ID; - if (get(SAI_OBJECT_TYPE_ROUTER_INTERFACE, oid, 1, &attr) != SAI_STATUS_SUCCESS) - { - // Only port/LAG based RIFs (incl. sub-ports) are backed by a VPP - // hardware interface whose counters we can read here. - return; - } - sai_object_id_t port_oid = attr.value.oid; - - sai_object_type_t ot = objectTypeQuery(port_oid); - if (ot != SAI_OBJECT_TYPE_PORT && ot != SAI_OBJECT_TYPE_LAG) - { - return; - } - - uint16_t vlan_id = 0; - if (rif_type == SAI_ROUTER_INTERFACE_TYPE_SUB_PORT) - { - attr.id = SAI_ROUTER_INTERFACE_ATTR_OUTER_VLAN_ID; - if (get(SAI_OBJECT_TYPE_ROUTER_INTERFACE, oid, 1, &attr) != SAI_STATUS_SUCCESS) - { - return; - } - vlan_id = attr.value.u16; - } - std::string if_name; - if (!vpp_get_hwif_name(port_oid, vlan_id, if_name)) + + // Resolve the RIF to its backing VPP interface: the port/LAG/sub-port + // hwif, or the bridge BVI (bvi) for a VLAN RIF. Only these are + // backed by a VPP hardware interface whose counters we can read here. + if (!vpp_get_rif_hwif_name(oid, if_name)) { return; } diff --git a/vslib/vpp/SwitchVpp.h b/vslib/vpp/SwitchVpp.h index 383397aa2b..81ac0584f9 100644 --- a/vslib/vpp/SwitchVpp.h +++ b/vslib/vpp/SwitchVpp.h @@ -717,6 +717,14 @@ namespace saivs _In_ uint32_t vlan_id, _In_ int32_t packet_action); + sai_status_t vpp_set_vlan_rif_loopback_action ( + _In_ sai_object_id_t rif_oid, + _In_ int32_t packet_action); + + bool vpp_get_rif_hwif_name ( + _In_ sai_object_id_t rif_oid, + _Out_ std::string& ifname); + sai_status_t UpdatePort( _In_ sai_object_id_t object_id, _In_ uint32_t attr_count, diff --git a/vslib/vpp/SwitchVppRif.cpp b/vslib/vpp/SwitchVppRif.cpp index 9f55156e99..66841ca87c 100644 --- a/vslib/vpp/SwitchVppRif.cpp +++ b/vslib/vpp/SwitchVppRif.cpp @@ -572,6 +572,62 @@ sai_status_t SwitchVpp::vpp_set_interface_mtu ( return SAI_STATUS_SUCCESS; } +bool SwitchVpp::vpp_get_rif_hwif_name ( + _In_ sai_object_id_t rif_oid, + _Out_ std::string& ifname) +{ + SWSS_LOG_ENTER(); + + sai_attribute_t attr; + + attr.id = SAI_ROUTER_INTERFACE_ATTR_TYPE; + if (get(SAI_OBJECT_TYPE_ROUTER_INTERFACE, rif_oid, 1, &attr) != SAI_STATUS_SUCCESS) { + return false; + } + int32_t rif_type = attr.value.s32; + + if (rif_type == SAI_ROUTER_INTERFACE_TYPE_VLAN) { + // VLAN RIF is backed by the bridge's BVI interface (bvi). + attr.id = SAI_ROUTER_INTERFACE_ATTR_VLAN_ID; + if (get(SAI_OBJECT_TYPE_ROUTER_INTERFACE, rif_oid, 1, &attr) != SAI_STATUS_SUCCESS) { + return false; + } + sai_object_id_t vlan_oid = attr.value.oid; + if (objectTypeQuery(vlan_oid) != SAI_OBJECT_TYPE_VLAN) { + return false; + } + sai_attribute_t vattr; + vattr.id = SAI_VLAN_ATTR_VLAN_ID; + if (get(SAI_OBJECT_TYPE_VLAN, vlan_oid, 1, &vattr) != SAI_STATUS_SUCCESS) { + return false; + } + ifname = std::string("bvi") + std::to_string(vattr.value.u16); + return true; + } + + attr.id = SAI_ROUTER_INTERFACE_ATTR_PORT_ID; + if (get(SAI_OBJECT_TYPE_ROUTER_INTERFACE, rif_oid, 1, &attr) != SAI_STATUS_SUCCESS) { + return false; + } + sai_object_id_t port_oid = attr.value.oid; + + sai_object_type_t ot = objectTypeQuery(port_oid); + if (ot != SAI_OBJECT_TYPE_PORT && ot != SAI_OBJECT_TYPE_LAG) { + return false; + } + + uint16_t vlan_id = 0; + if (rif_type == SAI_ROUTER_INTERFACE_TYPE_SUB_PORT) { + attr.id = SAI_ROUTER_INTERFACE_ATTR_OUTER_VLAN_ID; + if (get(SAI_OBJECT_TYPE_ROUTER_INTERFACE, rif_oid, 1, &attr) != SAI_STATUS_SUCCESS) { + return false; + } + vlan_id = attr.value.u16; + } + + return vpp_get_hwif_name(port_oid, vlan_id, ifname); +} + sai_status_t SwitchVpp::vpp_set_interface_loopback_action ( _In_ sai_object_id_t object_id, _In_ uint32_t vlan_id, @@ -599,6 +655,32 @@ sai_status_t SwitchVpp::vpp_set_interface_loopback_action ( return SAI_STATUS_SUCCESS; } +sai_status_t SwitchVpp::vpp_set_vlan_rif_loopback_action ( + _In_ sai_object_id_t rif_oid, + _In_ int32_t packet_action) +{ + SWSS_LOG_ENTER(); + + if (is_ip_nbr_active() == false) { + return SAI_STATUS_SUCCESS; + } + + std::string ifname; + + if (vpp_get_rif_hwif_name(rif_oid, ifname) == true) { + const char *hwif_name = ifname.c_str(); + int action = (packet_action == SAI_PACKET_ACTION_DROP) ? 1 : 0; + + int ret = vpp_iface_loopback_set_action(hwif_name, action); + SWSS_LOG_NOTICE("Setting router interface loopback action %s to %s (ret %d)", + hwif_name, action ? "drop" : "forward", ret); + if (ret != 0) { + return SAI_STATUS_FAILURE; + } + } + return SAI_STATUS_SUCCESS; +} + sai_status_t SwitchVpp::UpdatePort( _In_ sai_object_id_t object_id, _In_ uint32_t attr_count, @@ -1737,6 +1819,20 @@ sai_status_t SwitchVpp::vpp_update_router_interface( } rif_type = attr.value.s32; + if (rif_type == SAI_ROUTER_INTERFACE_TYPE_VLAN) + { + // VLAN RIFs carry no PORT_ID; they are backed by the bridge BVI + // (bvi). Apply the attributes we support on the BVI. + auto attr_loopback = sai_metadata_get_attr_by_id(SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION, attr_count, attr_list); + + if (attr_loopback != NULL) + { + vpp_set_vlan_rif_loopback_action(object_id, attr_loopback->value.s32); + } + + return SAI_STATUS_SUCCESS; + } + attr.id = SAI_ROUTER_INTERFACE_ATTR_PORT_ID; status = get(SAI_OBJECT_TYPE_ROUTER_INTERFACE, object_id, 1, &attr); From d774ada69caf0aa4976ac6a4bccb04f22a71f133 Mon Sep 17 00:00:00 2001 From: Aaron Bernardino Date: Sat, 4 Jul 2026 21:12:11 +0000 Subject: [PATCH 4/5] [vslib][vpp] Fix pre-existing RIF/FDB defects and harden loopback action Fixes several pre-existing defects surfaced while enabling iface_loopback_action on the VPP KVM testbed, plus review hardening: - Guard every std::stoi on SONiC interface names with a shared vpp_safe_stoi() (SwitchVppUtils): an uncaught std::invalid_argument dropped syncd into shutdown-wait mode and stalled the whole pipeline. - Program VLAN RIF neighbors: resolve the backing VPP interface via vpp_get_rif_hwif_name() (bvi for a VLAN RIF) in addRemoveIpNbr. - Add the PortChannel sub-port RIF IP on BondEthernet., including SONiC's short Po. form, so the connected route resolves. - Skip PortChannel sub-interface entries in find_new_bond_id() so a bogus bond id 0 is not assigned to a real LAG. - Apply the loopback packet action symmetrically: on VLAN RIF create (vpp_create_bvi_interface) and clear it to FORWARD on RIF remove so a recycled interface does not inherit a stale DROP. - De-duplicate the loopback setters into vpp_apply_loopback_action and fix log/format hygiene (%u for uint32_t, descriptive parse tags, explicit / includes). Signed-off-by: Aaron Bernardino Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- vslib/vpp/SwitchVpp.h | 6 ++- vslib/vpp/SwitchVppFdb.cpp | 24 +++++++++- vslib/vpp/SwitchVppNbr.cpp | 44 ++++-------------- vslib/vpp/SwitchVppRif.cpp | 76 +++++++++++++++++++++----------- vslib/vpp/SwitchVppUtils.cpp | 14 ++++++ vslib/vpp/SwitchVppUtils.h | 10 +++++ vslib/vpp/vppxlate/SaiVppXlate.c | 2 +- 7 files changed, 112 insertions(+), 64 deletions(-) diff --git a/vslib/vpp/SwitchVpp.h b/vslib/vpp/SwitchVpp.h index 81ac0584f9..1b6e74b9d7 100644 --- a/vslib/vpp/SwitchVpp.h +++ b/vslib/vpp/SwitchVpp.h @@ -712,12 +712,16 @@ namespace saivs _In_ uint32_t vlan_id, _In_ uint32_t mtu); + sai_status_t vpp_apply_loopback_action ( + _In_ const std::string& ifname, + _In_ int32_t packet_action); + sai_status_t vpp_set_interface_loopback_action ( _In_ sai_object_id_t object_id, _In_ uint32_t vlan_id, _In_ int32_t packet_action); - sai_status_t vpp_set_vlan_rif_loopback_action ( + sai_status_t vpp_set_rif_loopback_action ( _In_ sai_object_id_t rif_oid, _In_ int32_t packet_action); diff --git a/vslib/vpp/SwitchVppFdb.cpp b/vslib/vpp/SwitchVppFdb.cpp index d4d79d8bba..6523974a5a 100644 --- a/vslib/vpp/SwitchVppFdb.cpp +++ b/vslib/vpp/SwitchVppFdb.cpp @@ -483,6 +483,15 @@ sai_status_t SwitchVpp::vpp_create_bvi_interface( uint32_t bd_id = (uint32_t) vlan_id; set_bridge_domain_flags(bd_id, VPP_BD_FLAG_ARP_TERM,true); + // Create-path parity with vpp_update_router_interface: if the VLAN RIF is created + // with SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION, apply it on the BVI now. + // Otherwise the action would only take effect on a later set. + auto attr_loopback = sai_metadata_get_attr_by_id(SAI_ROUTER_INTERFACE_ATTR_LOOPBACK_PACKET_ACTION, attr_count, attr_list); + if (attr_loopback != NULL && is_ip_nbr_active()) + { + vpp_apply_loopback_action(hw_ifname, attr_loopback->value.s32); + } + return SAI_STATUS_SUCCESS; } @@ -653,10 +662,21 @@ uint32_t SwitchVpp::find_new_bond_id() bool found_new_bond_id = false; while (std::getline(iss, line)) { std::string portchannel_name = line.substr(0, line.find('\n')); - bond_id = std::stoi(portchannel_name.substr(strlen(PORTCHANNEL_PREFIX))); + // A base PortChannel is named "PortChannel". Sub-interfaces ("PortChannelX.Y" + // or the short "PoX.Y" form) and any malformed entry must not be mistaken for a new + // bond: a '.' or non-digit right after the prefix parses to a bogus id (often 0 via + // the safe-stoi fallback), which previously got assigned to a real LAG and left its + // RIF/neighbor on the wrong BondEthernet. + std::string suffix = portchannel_name.substr(strlen(PORTCHANNEL_PREFIX)); + if (suffix.empty() || suffix[0] < '0' || suffix[0] > '9' || + suffix.find('.') != std::string::npos || suffix.find('@') != std::string::npos) { + SWSS_LOG_DEBUG("Skipping non-base PortChannel entry for bond id: %s", portchannel_name.c_str()); + continue; + } + bond_id = (uint32_t) vpp_safe_stoi(suffix, "find_new_bond_id"); if (existing_bond_ids.find(bond_id) == existing_bond_ids.end()) { - SWSS_LOG_NOTICE("Found new bond id from PortChannel name: %d", bond_id); + SWSS_LOG_NOTICE("Found new bond id from PortChannel name: %u", bond_id); found_new_bond_id = true; break; } diff --git a/vslib/vpp/SwitchVppNbr.cpp b/vslib/vpp/SwitchVppNbr.cpp index 8bf84cb911..a106f58ce8 100644 --- a/vslib/vpp/SwitchVppNbr.cpp +++ b/vslib/vpp/SwitchVppNbr.cpp @@ -31,37 +31,19 @@ sai_status_t SwitchVpp::addRemoveIpNbr( sai_deserialize_neighbor_entry(serializedObjectId, nbr_entry); - attr.id = SAI_ROUTER_INTERFACE_ATTR_PORT_ID; - - CHECK_STATUS(get(SAI_OBJECT_TYPE_ROUTER_INTERFACE, nbr_entry.rif_id, 1, &attr)); - - auto port_obj_type = objectTypeQuery(attr.value.oid); - if (port_obj_type != SAI_OBJECT_TYPE_PORT && port_obj_type != SAI_OBJECT_TYPE_LAG) - { - return SAI_STATUS_SUCCESS; - } - auto port_oid = attr.value.oid; - - attr.id = SAI_ROUTER_INTERFACE_ATTR_TYPE; - - CHECK_STATUS(get(SAI_OBJECT_TYPE_ROUTER_INTERFACE, nbr_entry.rif_id, 1, &attr)); - if (attr.value.s32 != SAI_ROUTER_INTERFACE_TYPE_SUB_PORT && - attr.value.s32 != SAI_ROUTER_INTERFACE_TYPE_PORT) + // Resolve the VPP interface backing this RIF. vpp_get_rif_hwif_name handles + // PORT, LAG, SUB_PORT (hwif.vlan) and VLAN (bvi) RIFs. The previous + // logic only accepted a PORT/LAG PORT_ID with PORT/SUB_PORT type, so VLAN RIF + // neighbors were silently skipped and never programmed into VPP -- the DUT then + // ARPed instead of forwarding looped traffic on a VLAN RIF. + std::string hwif_name; + if (vpp_get_rif_hwif_name(nbr_entry.rif_id, hwif_name) == false) { - SWSS_LOG_NOTICE("Skipping neighbor add for attr type %d", attr.value.s32); - + SWSS_LOG_NOTICE("Skipping neighbor add: no VPP hwif for rif %s", + sai_serialize_object_id(nbr_entry.rif_id).c_str()); return SAI_STATUS_SUCCESS; } - uint16_t vlan_id = 0; - if (attr.value.s32 == SAI_ROUTER_INTERFACE_TYPE_SUB_PORT) - { - attr.id = SAI_ROUTER_INTERFACE_ATTR_OUTER_VLAN_ID; - - CHECK_STATUS(get(SAI_OBJECT_TYPE_ROUTER_INTERFACE, nbr_entry.rif_id, 1, &attr)); - vlan_id = attr.value.u16; - } - sai_mac_t nbr_mac; bool no_mac = true; @@ -95,14 +77,6 @@ sai_status_t SwitchVpp::addRemoveIpNbr( return SAI_STATUS_FAILURE; } - std::string hwif_name; - bool found = vpp_get_hwif_name(port_oid, vlan_id, hwif_name); - if (found == false) - { - SWSS_LOG_ERROR("hw interface for port/lag id %s not found", serializedObjectId.c_str()); - return SAI_STATUS_FAILURE; - } - const char *vpp_ifname = hwif_name.c_str(); init_vpp_client(); diff --git a/vslib/vpp/SwitchVppRif.cpp b/vslib/vpp/SwitchVppRif.cpp index 66841ca87c..36f27b2df0 100644 --- a/vslib/vpp/SwitchVppRif.cpp +++ b/vslib/vpp/SwitchVppRif.cpp @@ -628,6 +628,22 @@ bool SwitchVpp::vpp_get_rif_hwif_name ( return vpp_get_hwif_name(port_oid, vlan_id, ifname); } +sai_status_t SwitchVpp::vpp_apply_loopback_action ( + _In_ const std::string& ifname, + _In_ int32_t packet_action) +{ + SWSS_LOG_ENTER(); + + const char *hwif_name = ifname.c_str(); + int action = (packet_action == SAI_PACKET_ACTION_DROP) ? 1 : 0; + + int ret = vpp_iface_loopback_set_action(hwif_name, action); + SWSS_LOG_NOTICE("Setting router interface loopback action %s to %s (ret %d)", + hwif_name, action ? "drop" : "forward", ret); + + return (ret != 0) ? SAI_STATUS_FAILURE : SAI_STATUS_SUCCESS; +} + sai_status_t SwitchVpp::vpp_set_interface_loopback_action ( _In_ sai_object_id_t object_id, _In_ uint32_t vlan_id, @@ -642,20 +658,12 @@ sai_status_t SwitchVpp::vpp_set_interface_loopback_action ( std::string ifname; if (vpp_get_hwif_name(object_id, vlan_id, ifname) == true) { - const char *hwif_name = ifname.c_str(); - int action = (packet_action == SAI_PACKET_ACTION_DROP) ? 1 : 0; - - int ret = vpp_iface_loopback_set_action(hwif_name, action); - SWSS_LOG_NOTICE("Setting router interface loopback action %s to %s (ret %d)", - hwif_name, action ? "drop" : "forward", ret); - if (ret != 0) { - return SAI_STATUS_FAILURE; - } + return vpp_apply_loopback_action(ifname, packet_action); } return SAI_STATUS_SUCCESS; } -sai_status_t SwitchVpp::vpp_set_vlan_rif_loopback_action ( +sai_status_t SwitchVpp::vpp_set_rif_loopback_action ( _In_ sai_object_id_t rif_oid, _In_ int32_t packet_action) { @@ -668,15 +676,7 @@ sai_status_t SwitchVpp::vpp_set_vlan_rif_loopback_action ( std::string ifname; if (vpp_get_rif_hwif_name(rif_oid, ifname) == true) { - const char *hwif_name = ifname.c_str(); - int action = (packet_action == SAI_PACKET_ACTION_DROP) ? 1 : 0; - - int ret = vpp_iface_loopback_set_action(hwif_name, action); - SWSS_LOG_NOTICE("Setting router interface loopback action %s to %s (ret %d)", - hwif_name, action ? "drop" : "forward", ret); - if (ret != 0) { - return SAI_STATUS_FAILURE; - } + return vpp_apply_loopback_action(ifname, packet_action); } return SAI_STATUS_SUCCESS; } @@ -942,7 +942,7 @@ static void get_intf_vlanid (std::string& sub_ifname, int *vlan_id, std::string& } else { if_name = sub_ifname.substr(0, pos); std::string vlan = sub_ifname.substr(pos+1); - *vlan_id = std::stoi(vlan); + *vlan_id = vpp_safe_stoi(vlan, "sub_port_vlan_id"); } } static void get_vlan_intf_vlanid(std::string& if_name, std::string& vlan_prefix, int* vlan_id) @@ -970,7 +970,7 @@ static void get_vlan_intf_vlanid(std::string& if_name, std::string& vlan_prefix, std::string numeric_part = if_name.substr(pos); // Convert the numeric part to an integer using stoi - *vlan_id = std::stoi(numeric_part); + *vlan_id = vpp_safe_stoi(numeric_part, "vlan_intf_id"); } static void vpp_serialize_intf_data (std::string& k1, std::string& k2, std::string &serializedData) { @@ -1133,8 +1133,29 @@ sai_status_t SwitchVpp::vpp_add_del_intf_ip_addr_norif ( snprintf(hw_bviifname, sizeof(hw_bviifname), "%s%d","bvi",vlan_id); hw_ifname = hw_bviifname; } else if (full_if_name.compare(0, strlen(PORTCHANNEL_PREFIX), PORTCHANNEL_PREFIX) == 0) { - uint32_t bond_id = std::stoi(full_if_name.substr(strlen(PORTCHANNEL_PREFIX))); - snprintf(hw_bondifname, sizeof(hw_bondifname), "%s%d", BONDETHERNET_PREFIX, bond_id); + uint32_t bond_id = (uint32_t)vpp_safe_stoi(full_if_name.substr(strlen(PORTCHANNEL_PREFIX)), "portchannel_bond_id"); + if (vlan_id) { + // PortChannel sub-port RIF (e.g. PortChannel54.54): the IP must be added on the + // bond sub-interface (BondEthernet.), not the base bond. Without the + // . suffix the connected route/adjacency is never programmed, leaving the + // FIB entry UNRESOLVED and traffic dropped. + snprintf(hw_bondifname, sizeof(hw_bondifname), "%s%u.%d", BONDETHERNET_PREFIX, bond_id, vlan_id); + } else { + snprintf(hw_bondifname, sizeof(hw_bondifname), "%s%u", BONDETHERNET_PREFIX, bond_id); + } + hw_ifname = hw_bondifname; + } else if (full_if_name.compare(0, 2, "Po") == 0 && full_if_name.length() > 2 && + full_if_name[2] >= '0' && full_if_name[2] <= '9') { + // SONiC uses the short form "Po." (e.g. Po54.54) for PortChannel + // sub-interfaces. Map it to the VPP bond sub-interface BondEthernet. + // so the RIF IP / connected route is programmed (the base bond name here would + // leave the sub-interface without an IP and the FIB entry UNRESOLVED). + uint32_t bond_id = (uint32_t)vpp_safe_stoi(if_name.substr(2), "portchannel_short_bond_id"); + if (vlan_id) { + snprintf(hw_bondifname, sizeof(hw_bondifname), "%s%u.%d", BONDETHERNET_PREFIX, bond_id, vlan_id); + } else { + snprintf(hw_bondifname, sizeof(hw_bondifname), "%s%u", BONDETHERNET_PREFIX, bond_id); + } hw_ifname = hw_bondifname; } else { hwifname = tap_to_hwif_name(if_name.c_str()); @@ -1612,7 +1633,7 @@ int SwitchVpp::vpp_get_vrf_id (const char *linux_ifname, uint32_t *vrf_id) if (res.length() != 0) { - *vrf_id = std::stoi(res); + *vrf_id = vpp_safe_stoi(res, "vrf_id"); } else { *vrf_id = 0; } @@ -1827,7 +1848,7 @@ sai_status_t SwitchVpp::vpp_update_router_interface( if (attr_loopback != NULL) { - vpp_set_vlan_rif_loopback_action(object_id, attr_loopback->value.s32); + vpp_set_rif_loopback_action(object_id, attr_loopback->value.s32); } return SAI_STATUS_SUCCESS; @@ -1980,6 +2001,11 @@ sai_status_t SwitchVpp::vpp_remove_router_interface(sai_object_id_t rif_id) return SAI_STATUS_FAILURE; } + // Clear any loopback (hairpin) drop action first so the underlying port/LAG/BVI + // does not retain the DROP output-arc feature after this RIF is removed; a later + // RIF reusing the same VPP interface would otherwise inherit a stale DROP. + vpp_set_rif_loopback_action(rif_id, SAI_PACKET_ACTION_FORWARD); + if (attr.value.s32 == SAI_ROUTER_INTERFACE_TYPE_VLAN) { SWSS_LOG_NOTICE("Invoking BVI interface create for attr type %d", attr.value.s32); diff --git a/vslib/vpp/SwitchVppUtils.cpp b/vslib/vpp/SwitchVppUtils.cpp index 1648931128..7553e6404c 100644 --- a/vslib/vpp/SwitchVppUtils.cpp +++ b/vslib/vpp/SwitchVppUtils.cpp @@ -4,8 +4,22 @@ #include "vppxlate/SaiVppXlate.h" +#include +#include + using namespace saivs; +int saivs::vpp_safe_stoi(const std::string& s, const char* tag) +{ + try { + return std::stoi(s); + } catch (const std::exception& e) { + SWSS_LOG_ERROR("vpp_safe_stoi: failed to parse '%s' as int (%s): %s", + s.c_str(), tag, e.what()); + return 0; + } +} + sai_status_t saivs::find_attrib_in_list( _In_ uint32_t attr_count, _In_ const sai_attribute_t *attr_list, diff --git a/vslib/vpp/SwitchVppUtils.h b/vslib/vpp/SwitchVppUtils.h index aca8a342b2..41b025cddf 100644 --- a/vslib/vpp/SwitchVppUtils.h +++ b/vslib/vpp/SwitchVppUtils.h @@ -38,6 +38,16 @@ extern "C" { namespace saivs { + /* + * Parse a decimal integer, guarding against std::stoi throwing on malformed + * input. An uncaught std::invalid_argument/out_of_range here propagates to + * Syncd::run's handler, which drops syncd into shutdown-wait mode and stops + * answering any request. On failure this logs and returns 0; callers that + * cannot tolerate a 0 fallback must validate the input before calling. + * 'tag' is a short descriptive context string for the error log. + */ + int vpp_safe_stoi(const std::string& s, const char* tag); + sai_status_t find_attrib_in_list( _In_ uint32_t attr_count, _In_ const sai_attribute_t *attr_list, diff --git a/vslib/vpp/vppxlate/SaiVppXlate.c b/vslib/vpp/vppxlate/SaiVppXlate.c index e6cf5ad659..1c92d8f39a 100644 --- a/vslib/vpp/vppxlate/SaiVppXlate.c +++ b/vslib/vpp/vppxlate/SaiVppXlate.c @@ -4794,7 +4794,7 @@ static int __sw_interface_get_table(uint32_t sw_if_index, bool is_ipv6, uint32_t return ret; } -int vpp_sw_interface_find_by_ip(vpp_ip_addr_t *search_ip, uint32_t vrf_id, +int vpp_sw_interface_find_by_ip(vpp_ip_addr_t *search_ip, uint32_t vrf_id, uint32_t *out_sw_if_index) { int ret; From 656883722e4d314dbb42a20836fbf66220ad1666 Mon Sep 17 00:00:00 2001 From: Aaron Bernardino Date: Sun, 5 Jul 2026 04:59:54 +0000 Subject: [PATCH 5/5] [vslib][vpp] Bind RIF loopback action to the sonic_ext plugin API The RIF loopback packet action node moved from the standalone iface_loopback VPP plugin into the consolidated sonic_ext plugin (sonic-platform-vpp). Point the saivpp binding at sonic_ext's binary API: include the sonic_ext .api headers, look up the sonic_ext plugin msg-id base, and rename the api-version symbol. The API message name (iface_loopback_set_action) is unchanged, so the generated VL_API_* / vl_api_* symbols and the vpp_iface_loopback_set_action() send helper are unchanged. Signed-off-by: Aaron Bernardino Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- vslib/vpp/vppxlate/SaiVppXlate.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/vslib/vpp/vppxlate/SaiVppXlate.c b/vslib/vpp/vppxlate/SaiVppXlate.c index 1c92d8f39a..26e2507b77 100644 --- a/vslib/vpp/vppxlate/SaiVppXlate.c +++ b/vslib/vpp/vppxlate/SaiVppXlate.c @@ -56,8 +56,8 @@ #include #include -#include -#include +#include +#include #include #include @@ -121,22 +121,22 @@ #include #undef vl_api_version -/* iface_loopback API inclusion */ +/* sonic_ext (RIF loopback) API inclusion */ #define vl_typedefs -#include +#include #undef vl_typedefs #define vl_endianfun -#include +#include #undef vl_endianfun #define vl_calcsizefun -#include +#include #undef vl_calcsizefun -#define vl_api_version(n, v) static u32 iface_loopback_api_version = v; -#include +#define vl_api_version(n, v) static u32 sonic_ext_api_version = v; +#include #undef vl_api_version /* interface API inclusion */ @@ -1679,7 +1679,7 @@ static void get_base_msg_id() sflow_msg_id_base = vl_client_get_first_plugin_msg_id ((char *) msg_base_lookup_name); assert(sflow_msg_id_base != (u16) ~0); - msg_base_lookup_name = format (0, "iface_loopback_%08x%c", iface_loopback_api_version, 0); + msg_base_lookup_name = format (0, "sonic_ext_%08x%c", sonic_ext_api_version, 0); iface_loopback_msg_id_base = vl_client_get_first_plugin_msg_id ((char *) msg_base_lookup_name); assert(iface_loopback_msg_id_base != (u16) ~0); }