From 7ed61a606513d053465154786b6eedd06a521c0e Mon Sep 17 00:00:00 2001 From: Ramesh Raghupathy Date: Mon, 31 Jul 2023 20:49:02 -0700 Subject: [PATCH 1/2] Review comments addressed --- platform/saivpp/vpplib/SwitchStateBase.h | 13 +- platform/saivpp/vpplib/SwitchStateBaseRif.cpp | 288 +++++++++++++----- platform/saivpp/vpplib/vppxlate/SaiVppXlate.c | 2 +- 3 files changed, 216 insertions(+), 87 deletions(-) diff --git a/platform/saivpp/vpplib/SwitchStateBase.h b/platform/saivpp/vpplib/SwitchStateBase.h index 3d0d260..9bf67f7 100644 --- a/platform/saivpp/vpplib/SwitchStateBase.h +++ b/platform/saivpp/vpplib/SwitchStateBase.h @@ -711,6 +711,7 @@ namespace saivpp std::unordered_map lpbInstMap; std::unordered_map lpbIpToHostIfMap; std::unordered_map lpbIpToIfMap; + std::unordered_map lpbHostIfToVppIfMap; std::map> m_nxthop_grp_mbr_map; @@ -768,14 +769,18 @@ namespace saivpp _In_ const std::string& ip_prefix_key, _In_ sai_route_entry_t& route_entry, _In_ bool is_add); - sai_status_t process_interface_loopback ( + sai_status_t vpp_interface_ip_address_update ( + _In_ const char *hw_ifname, _In_ const std::string &serializedObjectId, - _In_ bool &isLoopback, _In_ bool is_add); - sai_status_t vpp_add_del_lpb_intf_ip_addr ( + sai_status_t process_interface_loopback ( _In_ const std::string &serializedObjectId, + _In_ bool &isLoopback, _In_ bool is_add); - + sai_status_t vpp_add_lpb_intf_ip_addr ( + _In_ const std::string &serializedObjectId); + sai_status_t vpp_del_lpb_intf_ip_addr ( + _In_ const std::string &serializedObjectId); sai_status_t vpp_get_router_intf_name ( _In_ sai_ip_prefix_t& ip_prefix, _In_ sai_object_id_t rif_id, diff --git a/platform/saivpp/vpplib/SwitchStateBaseRif.cpp b/platform/saivpp/vpplib/SwitchStateBaseRif.cpp index 2efb9ec..c265f66 100644 --- a/platform/saivpp/vpplib/SwitchStateBaseRif.cpp +++ b/platform/saivpp/vpplib/SwitchStateBaseRif.cpp @@ -886,6 +886,39 @@ sai_status_t SwitchStateBase::vpp_add_del_intf_ip_addr_norif ( } } +enum class LpbOpType { + NOP_LPB_IF = 0, + ADD_IP_LPB_IF = 1, + DEL_IP_LPB_IF = 2, + ADD_LPB_IF = 3, + DEL_LPB_IF = 4, +}; + +LpbOpType getLoopbackOperationType ( + _In_ bool is_add, + _In_ const std::string vppIfName, + _In_ sai_route_entry_t route_entry) +{ + if (is_add && (!vppIfName.empty()) && + (vppIfName.find("loop") != std::string::npos)) + { + return LpbOpType::ADD_IP_LPB_IF; + } else if (!is_add && + !get_intf_name_for_prefix(route_entry).empty()) + { + return LpbOpType::DEL_IP_LPB_IF; + } else if (!is_add && + get_intf_name_for_prefix(route_entry).empty()) + { + return LpbOpType::DEL_LPB_IF; + } else if (is_add && vppIfName.empty()) + { + return LpbOpType::ADD_LPB_IF; + } else { + return LpbOpType::NOP_LPB_IF; + } +} + sai_status_t SwitchStateBase::process_interface_loopback ( _In_ const std::string &serializedObjectId, _In_ bool &isLoopback, @@ -896,27 +929,77 @@ sai_status_t SwitchStateBase::process_interface_loopback ( sai_route_entry_t route_entry; sai_deserialize_route_entry(serializedObjectId, route_entry); std::string destinationIP = extractDestinationIP(serializedObjectId); - std::string interfaceName; + std::string hostIfName = ""; if (is_add) { - interfaceName = get_intf_name_for_prefix(route_entry); + hostIfName = get_intf_name_for_prefix(route_entry); } else { - interfaceName = lpbIpToHostIfMap[destinationIP]; + hostIfName = lpbIpToHostIfMap[destinationIP]; } - isLoopback = (interfaceName.find("Loopback") != std::string::npos); - SWSS_LOG_NOTICE("interfaceName:%s isLoopback:%u", interfaceName.c_str(), isLoopback); + isLoopback = (hostIfName.find("Loopback") != std::string::npos); + SWSS_LOG_NOTICE("hostIfName:%s isLoopback:%u", hostIfName.c_str(), isLoopback); if (isLoopback) { - vpp_add_del_lpb_intf_ip_addr(serializedObjectId, is_add); + std::string vppIfName = lpbHostIfToVppIfMap[hostIfName]; + LpbOpType lpbOp = getLoopbackOperationType(is_add, vppIfName, route_entry); + + switch (lpbOp) { + case LpbOpType::ADD_IP_LPB_IF: + + // interface exists - add ip to interface + SWSS_LOG_NOTICE("hostIfName:%s exists new-ip:%s", + hostIfName.c_str(), destinationIP.c_str()); + + // update interafce with ip add + vpp_interface_ip_address_update(vppIfName.c_str(), + serializedObjectId, true); + + lpbIpToIfMap[destinationIP] = vppIfName; + lpbIpToHostIfMap[destinationIP] = hostIfName; + break; + + case LpbOpType::DEL_IP_LPB_IF: + + // interface exists - remove ip from interface + SWSS_LOG_NOTICE("hostIfName:%s exists new-ip:%s", + hostIfName.c_str(), destinationIP.c_str()); + + // update interafce with ip remove + vpp_interface_ip_address_update(vppIfName.c_str(), + serializedObjectId, false); + + lpbIpToIfMap.erase(destinationIP); + lpbIpToHostIfMap.erase(destinationIP); + break; + + case LpbOpType::DEL_LPB_IF: + + // interface exists - delete interface + vpp_del_lpb_intf_ip_addr(serializedObjectId); + break; + + case LpbOpType::ADD_LPB_IF: + + // interface does not exist - create interface + vpp_add_lpb_intf_ip_addr(serializedObjectId); + break; + + default: + + SWSS_LOG_INFO("No matching loopback hostIfName:%s new-ip:%s", + hostIfName.c_str(), destinationIP.c_str()); + break; + } } return SAI_STATUS_SUCCESS; } -sai_status_t SwitchStateBase::vpp_add_del_lpb_intf_ip_addr ( +sai_status_t SwitchStateBase::vpp_interface_ip_address_update ( + _In_ const char *vppIfname, _In_ const std::string &serializedObjectId, _In_ bool is_add) { @@ -924,107 +1007,144 @@ sai_status_t SwitchStateBase::vpp_add_del_lpb_intf_ip_addr ( sai_route_entry_t route_entry; sai_deserialize_route_entry(serializedObjectId, route_entry); - std::string ip_prefix_str; std::string destinationIP = extractDestinationIP(serializedObjectId); - if (is_add) + vpp_ip_route_t ip_route; + create_route_prefix(&route_entry, &ip_route); + + if (route_entry.destination.addr_family == SAI_IP_ADDR_FAMILY_IPV6) { - // Retrieve the current instance for the interface - uint32_t instance = getNextLoopbackInstance(); + char prefixIp6Str[INET6_ADDRSTRLEN]; + inet_ntop(AF_INET6, &(ip_route.prefix_addr.addr.ip6.sin6_addr), + prefixIp6Str, INET6_ADDRSTRLEN); - // Generate the loopback interface name - std::string interfaceName = "loop" + std::to_string(instance); + } else if (route_entry.destination.addr_family == SAI_IP_ADDR_FAMILY_IPV4) + { + char prefixIp4Str[INET_ADDRSTRLEN]; + inet_ntop(AF_INET, &(ip_route.prefix_addr.addr.ip4.sin_addr), + prefixIp4Str, INET_ADDRSTRLEN); + } else { + SWSS_LOG_ERROR("Could not determine IP address family! destinationIP:%s", + destinationIP.c_str()); + } - // Store the current instance interface pair - lpbInstMap[interfaceName] = instance; + int ret = interface_ip_address_add_del(vppIfname, &ip_route, is_add); + if (ret != 0) { + SWSS_LOG_ERROR("interface_ip_address_add returned error"); + } - // Store the ip/interfaceName pair - lpbIpToIfMap[destinationIP] = interfaceName; - const char *hw_ifname = interfaceName.c_str(); + return SAI_STATUS_SUCCESS; +} - SWSS_LOG_NOTICE("create_loopback_instance interfaceName:%s hwif_name:%s instance:%u ", - interfaceName.c_str(), hw_ifname, instance); +sai_status_t SwitchStateBase::vpp_add_lpb_intf_ip_addr ( + _In_ const std::string &serializedObjectId) +{ + SWSS_LOG_ENTER(); - // Create the loopback instance - int ret = create_loopback_instance(hw_ifname, instance); - if (ret != 0) { - SWSS_LOG_ERROR("create_loopback_instance returned error"); - } + sai_route_entry_t route_entry; + sai_deserialize_route_entry(serializedObjectId, route_entry); + std::string ip_prefix_str; + std::string destinationIP = extractDestinationIP(serializedObjectId); - /* Get new list of physical interfaces from VPP */ - refresh_interfaces_list(); + // Retrieve the current instance for the interface + uint32_t instance = getNextLoopbackInstance(); - vpp_ip_route_t ip_route; - create_route_prefix(&route_entry, &ip_route); + // Generate the loopback interface name + std::string vppIfName = "loop" + std::to_string(instance); - char prefixIp4Str[INET_ADDRSTRLEN]; - inet_ntop(AF_INET, &(ip_route.prefix_addr.addr.ip4.sin_addr), prefixIp4Str, INET_ADDRSTRLEN); + // Store the current instance interface pair + lpbInstMap[vppIfName] = instance; - SWSS_LOG_DEBUG("hw_ifname:%s is_add:%u", hw_ifname, is_add); - ret = interface_ip_address_add_del(hw_ifname, &ip_route, is_add); - if (ret != 0) { - SWSS_LOG_ERROR("interface_ip_address_add_del returned error"); - } + // Store the ip/vppIfName pair + lpbIpToIfMap[destinationIP] = vppIfName; - //Set state up - bool is_up = true; - interface_set_state(hw_ifname, is_up); + SWSS_LOG_NOTICE("vpp_add_lpb vppIfName:%s instance:%u", + vppIfName.c_str(), instance); - const std::string hostIfname = get_intf_name_for_prefix(route_entry); - SWSS_LOG_NOTICE("get_intf_name_for_prefix:%s", hostIfname.c_str()); - lpbIpToHostIfMap[destinationIP] = hostIfname; + // Get new list of physical interfaces from VPP + refresh_interfaces_list(); - // remove host looback interface before creating lcp tap - bool lpb_add = false; - const std::string destinationIp = destinationIP; - int result = configureLoopbackInterface(lpb_add, hostIfname, destinationIp, ip_route.prefix_len); - if (result != 0) - { - SWSS_LOG_ERROR("Failed to configure loopback interface remove"); - } + // Create the loopback instance + int ret = create_loopback_instance(vppIfName.c_str(), instance); + if (ret != 0) { + SWSS_LOG_ERROR("create_loopback_instance returned error"); + } - // create lcp tap between vpp and host - { - const char *sonic_name = hostIfname.c_str(); - const char *vpp_name = interfaceName.c_str(); + // Get new list of physical interfaces from VPP + refresh_interfaces_list(); - SWSS_LOG_DEBUG("configure_lcp_interface vpp_name:%s sonic_name:%s", vpp_name, sonic_name); - configure_lcp_interface(vpp_name, sonic_name, is_add); - } + // Update vpp interface ip address + vpp_interface_ip_address_update(vppIfName.c_str(), serializedObjectId, true); - // add back host looback interface after creating lcp tap - lpb_add = true; - result = configureLoopbackInterface(lpb_add, hostIfname, destinationIp, ip_route.prefix_len); - if (result != 0) - { - SWSS_LOG_ERROR("Failed to configure loopback interface add"); - } + //Set state up + bool is_up = true; + interface_set_state(vppIfName.c_str(), is_up); - return SAI_STATUS_SUCCESS; + const std::string hostIfname = get_intf_name_for_prefix(route_entry); + SWSS_LOG_NOTICE("get_intf_name_for_prefix:%s", hostIfname.c_str()); + lpbIpToHostIfMap[destinationIP] = hostIfname; + lpbHostIfToVppIfMap[hostIfname] = vppIfName; - } else + vpp_ip_route_t ip_route; + create_route_prefix(&route_entry, &ip_route); + // remove host looback interface before creating lcp tap + bool lpb_add = false; + const std::string destinationIp = destinationIP; + int result = configureLoopbackInterface(lpb_add, hostIfname, destinationIp, ip_route.prefix_len); + if (result != 0) { - std::string interfaceName = lpbIpToIfMap[destinationIP]; - const std::string hostIfname = lpbIpToHostIfMap[destinationIP]; - uint32_t instance = lpbInstMap[interfaceName]; - const char *hw_ifname = interfaceName.c_str(); + SWSS_LOG_ERROR("Failed to configure loopback interface remove"); + } - // Delete the loopback instance - delete_loopback(hw_ifname, instance); + // create lcp tap between vpp and host + { + init_vpp_client(); + SWSS_LOG_DEBUG("configure_lcp_interface vpp_name:%s sonic_name:%s", + vppIfName.c_str(), hostIfname.c_str()); + configure_lcp_interface(vppIfName.c_str(), hostIfname.c_str(), true); + } - // refresh interfaces list as we have deleted the loopback interface - refresh_interfaces_list(); + // add back host looback interface after creating lcp tap + lpb_add = true; + result = configureLoopbackInterface(lpb_add, hostIfname, destinationIp, ip_route.prefix_len); + if (result != 0) + { + SWSS_LOG_ERROR("Failed to configure loopback interface add"); + } - // Remove the IP/interface mappings from the maps - lpbInstMap.erase(interfaceName); - lpbIpToIfMap.erase(destinationIP); - lpbIpToHostIfMap.erase(destinationIP); + return SAI_STATUS_SUCCESS; +} - // Mark the loopback instance available - markLoopbackInstanceDeleted(instance); +sai_status_t SwitchStateBase::vpp_del_lpb_intf_ip_addr ( + _In_ const std::string &serializedObjectId) +{ + SWSS_LOG_ENTER(); - return SAI_STATUS_SUCCESS; - } + sai_route_entry_t route_entry; + sai_deserialize_route_entry(serializedObjectId, route_entry); + std::string destinationIP = extractDestinationIP(serializedObjectId); + + std::string vppIfName = lpbIpToIfMap[destinationIP]; + const std::string hostIfname = lpbIpToHostIfMap[destinationIP]; + uint32_t instance = lpbInstMap[vppIfName]; + + // Update vpp interface ip address + vpp_interface_ip_address_update(vppIfName.c_str(), serializedObjectId, false); + + // Delete the loopback instance + delete_loopback(vppIfName.c_str(), instance); + + // refresh interfaces list as we have deleted the loopback interface + refresh_interfaces_list(); + + // Remove the IP/interface mappings from the maps + lpbInstMap.erase(vppIfName); + lpbIpToIfMap.erase(destinationIP); + lpbIpToHostIfMap.erase(destinationIP); + lpbHostIfToVppIfMap.erase(hostIfname); + + // Mark the loopback instance available + markLoopbackInstanceDeleted(instance); return SAI_STATUS_SUCCESS; } @@ -1296,6 +1416,8 @@ sai_status_t SwitchStateBase::vpp_create_router_interface( { snprintf(host_subifname, sizeof(host_subifname), "%s.%u", dev, vlan_id); + init_vpp_client(); + /* The host(tap) subinterface is also created as part of the vpp subinterface creation */ create_sub_interface(tap_to_hwif_name(dev), vlan_id, vlan_id); @@ -1560,6 +1682,7 @@ sai_status_t SwitchStateBase::vpp_remove_router_interface(sai_object_id_t rif_id const char *dev = if_name.c_str(); + init_vpp_client(); delete_sub_interface(tap_to_hwif_name(dev), vlan_id); /* Get new list of physical interfaces from VPP */ refresh_interfaces_list(); @@ -1635,3 +1758,4 @@ sai_status_t SwitchStateBase::removeVrf( return SAI_STATUS_SUCCESS; } + diff --git a/platform/saivpp/vpplib/vppxlate/SaiVppXlate.c b/platform/saivpp/vpplib/vppxlate/SaiVppXlate.c index e4b27aa..1ed2102 100644 --- a/platform/saivpp/vpplib/vppxlate/SaiVppXlate.c +++ b/platform/saivpp/vpplib/vppxlate/SaiVppXlate.c @@ -964,7 +964,7 @@ static int __create_loopback_instance (vat_main_t *vam, u32 instance) __plugin_msg_base = interface_msg_id_base; - M (CREATE_LOOPBACK_INSTANCE, mp); + M (CREATE_LOOPBACK, mp); mp->is_specified = true; mp->user_instance = instance; /* Set MAC address */ From c0652b750dade3c0c168f8111f9a8afb7994a65d Mon Sep 17 00:00:00 2001 From: Ramesh Raghupathy Date: Mon, 31 Jul 2023 21:27:51 -0700 Subject: [PATCH 2/2] Removed accidental changes - somehow "vpp_client_init" got added in couple of places --- platform/saivpp/vpplib/SwitchStateBaseRif.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/platform/saivpp/vpplib/SwitchStateBaseRif.cpp b/platform/saivpp/vpplib/SwitchStateBaseRif.cpp index c265f66..f22c49b 100644 --- a/platform/saivpp/vpplib/SwitchStateBaseRif.cpp +++ b/platform/saivpp/vpplib/SwitchStateBaseRif.cpp @@ -1416,8 +1416,6 @@ sai_status_t SwitchStateBase::vpp_create_router_interface( { snprintf(host_subifname, sizeof(host_subifname), "%s.%u", dev, vlan_id); - init_vpp_client(); - /* The host(tap) subinterface is also created as part of the vpp subinterface creation */ create_sub_interface(tap_to_hwif_name(dev), vlan_id, vlan_id); @@ -1682,7 +1680,6 @@ sai_status_t SwitchStateBase::vpp_remove_router_interface(sai_object_id_t rif_id const char *dev = if_name.c_str(); - init_vpp_client(); delete_sub_interface(tap_to_hwif_name(dev), vlan_id); /* Get new list of physical interfaces from VPP */ refresh_interfaces_list(); @@ -1758,4 +1755,3 @@ sai_status_t SwitchStateBase::removeVrf( return SAI_STATUS_SUCCESS; } -