Implement TableAnnex classes (2/2) (#307)

- Integrated TdiTableAnnex with TdiTableManager.

- Updated TdiTableManager to create the TableAnnex object.
  Replaced ES2K-specific code with calls to Es2kTableAnnex methods.

- Removed ES2K-specific methods from TdiTableAnnex. This class is
  no longer dependent on the IDPF P4Info header file.

- Removed the TestUnsupportedMethod test case from
  es2k_extern_manager_test. It is no longer valid.

Signed-off-by: Derek Foster <derek.foster@intel.com>

stratum/hal/lib/tdi/BUILD

@@ -293,6 +293,7 @@ stratum_cc_library(
         ":tdi_get_meter_units",
         ":tdi_pkt_mod_meter_config",
         ":tdi_sde_interface",
+        ":tdi_table_annex",
         ":tdi_target_factory",
         ":utils",
         "//stratum/glue:integral_types",
@@ -686,13 +687,10 @@ stratum_cc_test(
 
 stratum_cc_library(
     name = "tdi_extern_manager",
-    srcs = ["tdi_extern_manager.cc"],
     hdrs = ["tdi_extern_manager.h"],
     deps = [
         "//stratum/hal/lib/p4:p4_extern_manager",
-        "//stratum/hal/lib/p4:p4_info_manager",
         "//stratum/hal/lib/p4:p4_resource_map",
-        "@com_github_p4lang_p4runtime//:idpf_p4info_cc_proto",
         "@com_github_p4lang_p4runtime//:p4info_cc_proto",
         "@com_google_absl//absl/memory",
     ],

stratum/hal/lib/tdi/CMakeLists.txt

@@ -13,7 +13,6 @@ add_library(stratum_tdi_common_o OBJECT
     tdi_action_profile_manager.h
     tdi_counter_manager.cc
     tdi_counter_manager.h
-    tdi_extern_manager.cc
     tdi_extern_manager.h
     tdi_get_meter_units.h
     tdi_global_vars.cc

stratum/hal/lib/tdi/es2k/es2k_extern_manager.h

@@ -21,7 +21,11 @@ namespace tdi {
 
 class Es2kExternManager : public TdiExternManager {
  public:
-  struct Statistics;
+  struct Statistics {
+    Statistics() : unknown_extern_id(0) {}
+    // Number of externs with unrecognized type IDs.
+    uint32 unknown_extern_id;
+  };
 
   Es2kExternManager();
   virtual ~Es2kExternManager() = default;
@@ -30,19 +34,21 @@ class Es2kExternManager : public TdiExternManager {
   void RegisterExterns(const ::p4::config::v1::P4Info& p4info,
                        const PreambleCallback& preamble_cb) override;
 
+  //--------------------------------------------------------------------
+
   // Retrieve a PacketModMeter configuration.
   ::util::StatusOr<const ::idpf::PacketModMeter> FindPktModMeterByID(
-      uint32 meter_id) const override;
+      uint32 meter_id) const;
 
   ::util::StatusOr<const ::idpf::PacketModMeter> FindPktModMeterByName(
-      const std::string& meter_name) const override;
+      const std::string& meter_name) const;
 
   // Retrieve a DirectPacketModMeter configuration.
   ::util::StatusOr<const ::idpf::DirectPacketModMeter>
-  FindDirectPktModMeterByID(uint32 meter_id) const override;
+  FindDirectPktModMeterByID(uint32 meter_id) const;
 
   ::util::StatusOr<const ::idpf::DirectPacketModMeter>
-  FindDirectPktModMeterByName(const std::string& meter_name) const override;
+  FindDirectPktModMeterByName(const std::string& meter_name) const;
 
   // Returns the number of entries in the DirectPacketModMeter map.
   uint32 direct_pkt_mod_meter_size() const {
@@ -55,12 +61,7 @@ class Es2kExternManager : public TdiExternManager {
   // Returns a reference to the Es2kExternManager statistics.
   const struct Statistics& statistics() { return stats_; }
 
-  // Es2kExternManager statistics.
-  struct Statistics {
-    Statistics() : unknown_extern_id(0) {}
-    // Number of externs with unrecognized type IDs.
-    uint32 unknown_extern_id;
-  };
+  //--------------------------------------------------------------------
 
  private:
   void RegisterPacketModMeters(const p4::config::v1::Extern& p4extern,

stratum/hal/lib/tdi/es2k/es2k_extern_manager_test.cc

@@ -379,15 +379,6 @@ TEST_F(Es2kExternManagerTest, TestUnknownExternType) {
   EXPECT_EQ(stats.unknown_extern_id, 1);
 }
 
-// An ES2K-specific method should return error status when invoked on a
-// TdiExternManager object.
-TEST_F(Es2kExternManagerTest, TestUnsupportedMethod) {
-  auto tdi_extern_manager = TdiExternManager::CreateInstance();
-  auto meter = tdi_extern_manager->FindPktModMeterByID(kPacketModMeterID1);
-  EXPECT_FALSE(meter.ok());
-  EXPECT_EQ(meter.status().error_code(), ERR_UNIMPLEMENTED);
-}
-
 //----------------------------------------------------------------------
 // P4InfoManager integration test
 //----------------------------------------------------------------------

stratum/hal/lib/tdi/es2k/es2k_table_annex.cc

@@ -107,7 +107,7 @@ ::util::Status Es2kTableAnnex::ReadPktModMeterEntry(
 ::util::Status Es2kTableAnnex::WritePktModMeterEntry(
     std::shared_ptr<TdiSdeInterface::SessionInterface> session,
     const ::p4::v1::Update::Type type, const ::p4::v1::MeterEntry& meter_entry,
-    uint32 meter_id) {
+    uint32 meter_rt_id) {
   bool units_in_packets;
   {
     absl::ReaderMutexLock l(lock_);
@@ -130,12 +130,12 @@ ::util::Status Es2kTableAnnex::WritePktModMeterEntry(
     config.isPktModMeter = units_in_packets;
 
     RETURN_IF_ERROR(tdi_sde_interface_->WritePktModMeter(
-        device_, session, meter_id, meter_index, config));
+        device_, session, meter_rt_id, meter_index, config));
   }
 
   if (type == ::p4::v1::Update::DELETE) {
     RETURN_IF_ERROR(tdi_sde_interface_->DeletePktModMeterConfig(
-        device_, session, meter_id, meter_index));
+        device_, session, meter_rt_id, meter_index));
   }
   return ::util::OkStatus();
 }

stratum/hal/lib/tdi/es2k/es2k_table_annex.h

@@ -60,7 +60,7 @@ class Es2kTableAnnex : public TdiTableAnnex {
   ::util::Status WritePktModMeterEntry(
       std::shared_ptr<TdiSdeInterface::SessionInterface> session,
       const ::p4::v1::Update::Type type,
-      const ::p4::v1::MeterEntry& meter_entry, uint32 meter_id) override;
+      const ::p4::v1::MeterEntry& meter_entry, uint32 meter_rt_id) override;
 
  protected:
   Es2kExternManager* es2k_extern_manager_;

stratum/hal/lib/tdi/tdi_extern_manager.h

@@ -5,12 +5,8 @@
 #define STRATUM_HAL_LIB_TDI_TDI_EXTERN_MANAGER_H_
 
 #include "absl/memory/memory.h"
-#include "idpf/p4info.pb.h"
 #include "p4/config/v1/p4info.pb.h"
-#include "stratum/glue/integral_types.h"
-#include "stratum/glue/status/statusor.h"
 #include "stratum/hal/lib/p4/p4_extern_manager.h"
-#include "stratum/hal/lib/p4/p4_info_manager.h"
 #include "stratum/hal/lib/p4/p4_resource_map.h"
 
 namespace stratum {
@@ -30,20 +26,6 @@ class TdiExternManager : public P4ExternManager {
   // Registers P4Extern resources. Called by P4InfoManager.
   void RegisterExterns(const ::p4::config::v1::P4Info& p4info,
                        const PreambleCallback& preamble_cb) override {}
-
-  // Retrieve a PacketModMeter configuration.
-  virtual ::util::StatusOr<const ::idpf::PacketModMeter> FindPktModMeterByID(
-      uint32 meter_id) const;
-
-  virtual ::util::StatusOr<const ::idpf::PacketModMeter> FindPktModMeterByName(
-      const std::string& meter_name) const;
-
-  // Retrieve a DirectPacketModMeter configuration.
-  virtual ::util::StatusOr<const ::idpf::DirectPacketModMeter>
-  FindDirectPktModMeterByID(uint32 meter_id) const;
-
-  virtual ::util::StatusOr<const ::idpf::DirectPacketModMeter>
-  FindDirectPktModMeterByName(const std::string& meter_name) const;
 };
 
 }  // namespace tdi

stratum/hal/lib/tdi/tdi_table_annex.h

@@ -65,7 +65,7 @@ class TdiTableAnnex {
   virtual ::util::Status WritePktModMeterEntry(
       std::shared_ptr<TdiSdeInterface::SessionInterface> session,
       const ::p4::v1::Update::Type type,
-      const ::p4::v1::MeterEntry& meter_entry, uint32 meter_id) {
+      const ::p4::v1::MeterEntry& meter_entry, uint32 meter_rt_id) {
     return ::util::OkStatus();
   }
 };

stratum/hal/lib/tdi/tdi_table_manager.cc

@@ -80,8 +80,13 @@ ::util::Status TdiTableManager::PushForwardingPipelineConfig(
   auto extern_manager = tdi_target_factory_.CreateTdiExternManager();
   RETURN_IF_ERROR(p4_info_manager->InitializeAndVerify(extern_manager.get()));
 
+  auto table_annex = tdi_target_factory_.CreateTdiTableAnnex();
+  RETURN_IF_ERROR(table_annex->Initialize(extern_manager.get(),
+                                          tdi_sde_interface_, &lock_, device_));
+
   p4_info_manager_ = std::move(p4_info_manager);
   tdi_extern_manager_ = std::move(extern_manager);
+  tdi_table_annex_ = std::move(table_annex);
 
   return ::util::OkStatus();
 }
@@ -258,20 +263,10 @@ ::util::Status TdiTableManager::BuildTableData(
           table_entry.counter_data().byte_count(),
           table_entry.counter_data().packet_count()));
     }
-    if (resource_type == "DirectPacketModMeter" &&
-        table_entry.has_meter_config()) {
-      // BuildDirPktModTableData
-      bool units_in_packets;  // or bytes
-      ASSIGN_OR_RETURN(
-          auto meter,
-          tdi_extern_manager_->FindDirectPktModMeterByID(resource_id));
-      RETURN_IF_ERROR(GetMeterUnitsInPackets(meter, units_in_packets));
 
-      TdiPktModMeterConfig config;
-      config.SetTableEntry(table_entry);
-      config.isPktModMeter = units_in_packets;
-
-      RETURN_IF_ERROR(table_data->SetPktModMeterConfig(config));
+    else if (resource_type == "DirectPacketModMeter") {
+      RETURN_IF_ERROR(tdi_table_annex_->BuildDirPktModTableData(
+          table_entry, table_data, resource_id));
     }
   }
 
@@ -876,11 +871,8 @@ TdiTableManager::ReadDirectMeterEntry(
       result.mutable_config()->set_pburst(static_cast<int64>(pburst));
     }
     if (resource_type == "DirectPacketModMeter") {
-      // ReadDirPktModMeterEntry
-      // build response entry from returned data
-      TdiPktModMeterConfig cfg;
-      RETURN_IF_ERROR(table_data->GetPktModMeterConfig(cfg));
-      cfg.GetDirectMeterEntry(&result);
+      RETURN_IF_ERROR(
+          tdi_table_annex_->ReadDirPktModMeterEntry(table_data.get(), result));
     }
   }
 
@@ -1025,41 +1017,8 @@ ::util::Status TdiTableManager::ReadMeterEntry(
   }
 
   else if (resource_type == "PacketModMeter") {
-    // ReadPktModMeterEntry
-    bool units_in_packets;
-    {
-      absl::ReaderMutexLock l(&lock_);
-      ::idpf::PacketModMeter meter;
-      ASSIGN_OR_RETURN(meter, tdi_extern_manager_->FindPktModMeterByID(
-                                  meter_entry.meter_id()));
-      RETURN_IF_ERROR(GetMeterUnitsInPackets(meter, units_in_packets));
-    }
-
-    // Index 0 is a valid value and not a wildcard.
-    absl::optional<uint32> optional_meter_index;
-    if (meter_entry.has_index()) {
-      optional_meter_index = meter_entry.index().index();
-    }
-
-    std::vector<uint32> meter_indices;
-    std::vector<TdiPktModMeterConfig> cfg;
-
-    RETURN_IF_ERROR(tdi_sde_interface_->ReadPktModMeters(
-        device_, session, table_id, optional_meter_index, &meter_indices, cfg));
-
-    ::p4::v1::ReadResponse resp;
-    for (size_t i = 0; i < meter_indices.size(); ++i) {
-      ::p4::v1::MeterEntry result;
-      result.set_meter_id(meter_entry.meter_id());
-      result.mutable_index()->set_index(meter_indices[i]);
-      cfg[i].GetMeterEntry(&result);
-      *resp.add_entities()->mutable_meter_entry() = result;
-    }
-
-    VLOG(1) << "ReadMeterEntry resp " << resp.DebugString();
-    if (!writer->Write(resp)) {
-      return MAKE_ERROR(ERR_INTERNAL) << "Write to stream for failed.";
-    }
+    RETURN_IF_ERROR(tdi_table_annex_->ReadPktModMeterEntry(session, meter_entry,
+                                                           writer, table_id));
   }
 
   return ::util::OkStatus();
@@ -1106,36 +1065,8 @@ ::util::Status TdiTableManager::WriteMeterEntry(
   }
 
   if (resource_type == "PacketModMeter") {
-    // WritePktModMeterEntry
-    bool units_in_packets;
-    {
-      absl::ReaderMutexLock l(&lock_);
-      ::idpf::PacketModMeter meter;
-      ASSIGN_OR_RETURN(meter, tdi_extern_manager_->FindPktModMeterByID(
-                                  meter_entry.meter_id()));
-      RETURN_IF_ERROR(GetMeterUnitsInPackets(meter, units_in_packets));
-    }
-
-    absl::optional<uint32> meter_index;
-    if (meter_entry.has_index()) {
-      meter_index = meter_entry.index().index();
-    } else {
-      return MAKE_ERROR(ERR_INVALID_PARAM) << "Invalid meter entry index";
-    }
-
-    if (meter_entry.has_config()) {
-      TdiPktModMeterConfig config;
-      config.SetMeterEntry(meter_entry);
-      config.isPktModMeter = units_in_packets;
-
-      RETURN_IF_ERROR(tdi_sde_interface_->WritePktModMeter(
-          device_, session, meter_rt_id, meter_index, config));
-    }
-
-    if (type == ::p4::v1::Update::DELETE) {
-      RETURN_IF_ERROR(tdi_sde_interface_->DeletePktModMeterConfig(
-          device_, session, meter_rt_id, meter_index));
-    }
+    RETURN_IF_ERROR(tdi_table_annex_->WritePktModMeterEntry(
+        session, type, meter_entry, meter_rt_id));
   }
 
   return ::util::OkStatus();

stratum/hal/lib/tdi/tdi_table_manager.h

@@ -20,6 +20,7 @@
 #include "stratum/hal/lib/p4/p4_info_manager.h"
 #include "stratum/hal/lib/tdi/tdi.pb.h"
 #include "stratum/hal/lib/tdi/tdi_sde_interface.h"
+#include "stratum/hal/lib/tdi/tdi_table_annex.h"
 #include "stratum/hal/lib/tdi/tdi_target_factory.h"
 
 namespace stratum {
@@ -185,6 +186,10 @@ class TdiTableManager {
   // Helper class to manage P4 Externs.
   std::unique_ptr<TdiExternManager> tdi_extern_manager_ GUARDED_BY(lock_);
 
+  // Pointer to an object that provides support for target-specific P4
+  // resources.
+  std::unique_ptr<TdiTableAnnex> tdi_table_annex_ GUARDED_BY(lock_);
+
   // Fixed zero-based Tofino device number corresponding to the node/ASIC
   // managed by this class instance. Assigned in the class constructor.
   const int device_;