diff --git a/proto/frontend/src/common.cpp b/proto/frontend/src/common.cpp index ef2b713e..c40ed788 100644 --- a/proto/frontend/src/common.cpp +++ b/proto/frontend/src/common.cpp @@ -148,8 +148,9 @@ Code check_proto_bytestring(const std::string &str, size_t nbits) { return Code::OK; } -bool check_prefix_trailing_zeros(const std::string &str, int pLen) { - size_t bitwidth = str.size() * 8; +bool check_prefix_trailing_zeros(const std::string &str, int pLen, + size_t bitwidth) { + // Use field bitwidth, not storage size (str.size() * 8) // must be guaranteed by caller assert(pLen >= 0 && static_cast(pLen) <= bitwidth); size_t trailing_zeros = bitwidth - pLen; diff --git a/proto/frontend/src/common.h b/proto/frontend/src/common.h index 0ffe4e9d..2875b281 100644 --- a/proto/frontend/src/common.h +++ b/proto/frontend/src/common.h @@ -164,7 +164,8 @@ std::string pi_port_to_bytestring(pi_port_t port, size_t num_bytes); Code check_proto_bytestring(const std::string &str, size_t nbits); -bool check_prefix_trailing_zeros(const std::string &str, int pLen); +bool check_prefix_trailing_zeros(const std::string &str, int pLen, + size_t bitwidth); std::string range_default_lo(size_t nbits); std::string range_default_hi(size_t nbits); diff --git a/proto/frontend/src/device_mgr.cpp b/proto/frontend/src/device_mgr.cpp index bad2f9a2..f53c256a 100644 --- a/proto/frontend/src/device_mgr.cpp +++ b/proto/frontend/src/device_mgr.cpp @@ -2246,7 +2246,7 @@ class DeviceMgrImp { "omit match field instead of using a prefix length of 0"); } // makes sure that value ends with zeros - if (!common::check_prefix_trailing_zeros(value, pLen)) { + if (!common::check_prefix_trailing_zeros(value, pLen, bitwidth)) { RETURN_ERROR_STATUS( Code::INVALID_ARGUMENT, "Invalid LPM value, incorrect number of trailing zeros"); diff --git a/proto/tests/test_proto_fe.cpp b/proto/tests/test_proto_fe.cpp index 9792aa00..d8d8d618 100644 --- a/proto/tests/test_proto_fe.cpp +++ b/proto/tests/test_proto_fe.cpp @@ -3410,6 +3410,189 @@ TEST_F(LpmOneTest, DontCare) { #undef EXPECT_ONE_TABLE_ENTRY +#define EXPECT_ONE_TABLE_ENTRY(response, expected_entry) \ + do { \ + const auto &entities = response.entities(); \ + ASSERT_EQ(1, entities.size()); \ + EXPECT_PROTO_EQ(entities.Get(0).table_entry(), expected_entry); \ + } while (false) + +// Test class for LpmNonByte table with 20-bit field (non-byte-aligned) +class LpmNonByteTest : public DeviceMgrTest { + protected: + LpmNonByteTest() + : LpmNonByteTest("LpmNonByte", "header_test.field20") { } + + LpmNonByteTest(const std::string &t_name, const std::string &f_name) + : f_name(f_name) { + t_id = pi_p4info_table_id_from_name(p4info, t_name.c_str()); + a_id = pi_p4info_action_id_from_name(p4info, "actionA"); + } + + p4v1::TableEntry make_entry(const boost::optional &mf_v, + int pLen, + const std::string ¶m_v) { + p4v1::TableEntry table_entry; + table_entry.set_table_id(t_id); + if (mf_v.is_initialized()) { + auto mf = table_entry.add_match(); + mf->set_field_id(pi_p4info_table_match_field_id_from_name( + p4info, t_id, f_name.c_str())); + auto mf_lpm = mf->mutable_lpm(); + mf_lpm->set_value(*mf_v); + mf_lpm->set_prefix_len(pLen); + } + auto entry = table_entry.mutable_action(); + auto action = entry->mutable_action(); + + action->set_action_id(a_id); + auto param = action->add_params(); + param->set_param_id( + pi_p4info_action_param_id_from_name(p4info, a_id, "param")); + param->set_value(param_v); + return table_entry; + } + + const std::string f_name; + pi_p4_id_t t_id; + pi_p4_id_t a_id; +}; + +// Test that validates byte alignment for 20-bit LPM field (non-byte-aligned) +// +// According to P4Runtime spec, a 20-bit field is stored in 3 bytes with MSB +// padding: +// Byte 0 [MSB]: 0000_xxxx (4 leading zero bits for padding) +// Byte 1: xxxx_xxxx (8 bits of field) +// Byte 2 [LSB]: xxxx_xxxx (8 bits of field) +// +// The check_prefix_trailing_zeros() function has been FIXED to use field +// bitwidth instead of storage size when calculating required trailing zeros. +TEST_F(LpmNonByteTest, NonByteAlignedField) { + // First verify the table and field exist + ASSERT_NE(t_id, PI_INVALID_ID) << "Table LpmNonByte not found in p4info"; + ASSERT_NE(a_id, PI_INVALID_ID) << "Action actionA not found in p4info"; + + std::string adata(6, '\xcd'); + + // Test with prefix length 20 (full field width) + // With the fix, all 20 bits of the field can be used! + int pLen(20); + + { // Valid: all 20 bits of the field set (no trailing zeros needed) + // Binary: 0000_1111 1111_1111 1111_1111 + // Represents value 0xFFFFF (all 20 bits set) + std::string mf("\x0f\xff\xff", 3); + auto entry = make_entry(mf, pLen, adata); + EXPECT_CALL(*mock, table_entry_add(t_id, _, _, _)); + auto status = add_entry(&entry); + ASSERT_EQ(status.code(), Code::OK); + } + + { // Invalid: MSB has too many bits set (violates 20-bit field width) + // Binary: 1111_1111 1111_1111 1111_1111 + std::string mf("\xff\xff\xff", 3); + auto entry = make_entry(mf, pLen, adata); + EXPECT_CALL(*mock, table_entry_add(t_id, _, _, _)).Times(0); + auto status = add_entry(&entry); + EXPECT_EQ(status, OneExpectedError(Code::INVALID_ARGUMENT)); + } + + // Test with prefix length 16 + // Requires 20-16=4 trailing zeros (not 24-16=8) + pLen = 16; + + { // Valid: first 16 bits of field set, last 4 bits zero + // Binary: 0000_1111 1111_1111 1111_0000 + std::string mf("\x0f\xff\xf0", 3); + auto entry = make_entry(mf, pLen, adata); + EXPECT_CALL(*mock, table_entry_add(t_id, _, _, _)); + auto status = add_entry(&entry); + ASSERT_EQ(status.code(), Code::OK); + } + + { // Invalid: not enough trailing zeros (has 0, needs 4) + // Binary: 0000_1111 1111_1111 1111_1111 + std::string mf("\x0f\xff\xff", 3); + auto entry = make_entry(mf, pLen, adata); + EXPECT_CALL(*mock, table_entry_add(t_id, _, _, _)).Times(0); + auto status = add_entry(&entry); + EXPECT_EQ(status, OneExpectedError(Code::INVALID_ARGUMENT)); + } + + { // Valid: more trailing zeros than needed (has 8, needs 4) - fine for LPM + // Binary: 0000_1111 1111_1111 0000_0000 + std::string mf("\x0f\xff\x00", 3); + auto entry = make_entry(mf, pLen, adata); + EXPECT_CALL(*mock, table_entry_add(t_id, _, _, _)); + auto status = add_entry(&entry); + ASSERT_EQ(status.code(), Code::OK); + } + + // Test with prefix length 12 + // Requires 20-12=8 trailing zeros (not 24-12=12) + pLen = 12; + + { // Valid: first 12 bits of field set, last 8 bits zero + // Binary: 0000_1111 1111_0000 0000_0000 + std::string mf("\x0f\xf0\x00", 3); + auto entry = make_entry(mf, pLen, adata); + EXPECT_CALL(*mock, table_entry_add(t_id, _, _, _)); + auto status = add_entry(&entry); + ASSERT_EQ(status.code(), Code::OK); + } + + { // Valid: alternative pattern with exactly 8 trailing zeros + // Binary: 0000_1111 1100_0000 0000_0000 + std::string mf("\x0f\xc0\x00", 3); + auto entry = make_entry(mf, pLen, adata); + EXPECT_CALL(*mock, table_entry_add(t_id, _, _, _)); + auto status = add_entry(&entry); + ASSERT_EQ(status.code(), Code::OK); + } + + { // Invalid: not enough trailing zeros (has 4, needs 8) + // Binary: 0000_1111 1111_1111 0000_0000 + std::string mf("\x0f\xff\xf0", 3); + auto entry = make_entry(mf, pLen, adata); + EXPECT_CALL(*mock, table_entry_add(t_id, _, _, _)).Times(0); + auto status = add_entry(&entry); + EXPECT_EQ(status, OneExpectedError(Code::INVALID_ARGUMENT)); + } + + { // Valid: more trailing zeros than needed (has 12, needs 8) - fine for LPM + // Binary: 0000_1111 1000_0000 0000_0000 + std::string mf("\x0f\x80\x00", 3); + auto entry = make_entry(mf, pLen, adata); + EXPECT_CALL(*mock, table_entry_add(t_id, _, _, _)); + auto status = add_entry(&entry); + ASSERT_EQ(status.code(), Code::OK); + } + + // Test with prefix length 8 (exactly 1 byte) + pLen = 8; + + { // Valid: first 8 bits set, last 12 bits zero + // Binary: 0000_1111 1000_0000 0000_0000 + std::string mf("\x0f\x80\x00", 3); + auto entry = make_entry(mf, pLen, adata); + EXPECT_CALL(*mock, table_entry_add(t_id, _, _, _)); + auto status = add_entry(&entry); + ASSERT_EQ(status.code(), Code::OK); + } + + { // Valid: different pattern with 12 trailing zeros + // Binary: 0000_1010 0000_0000 0000_0000 + std::string mf("\x0a\x00\x00", 3); + auto entry = make_entry(mf, pLen, adata); + EXPECT_CALL(*mock, table_entry_add(t_id, _, _, _)); + auto status = add_entry(&entry); + ASSERT_EQ(status.code(), Code::OK); + } +} + +#undef EXPECT_ONE_TABLE_ENTRY + class TernaryTwoTest : public DeviceMgrTest { protected: TernaryTwoTest() { diff --git a/tests/testdata/unittest.p4 b/tests/testdata/unittest.p4 index 013e619f..c7d68f25 100644 --- a/tests/testdata/unittest.p4 +++ b/tests/testdata/unittest.p4 @@ -94,6 +94,15 @@ control ingress(inout headers_t hdr, inout metadata_t meta, inout standard_metad size = 512; } + @name(".LpmNonByte") + table LpmNonByte { + key = { + hdr.header_test.field20 : lpm; + } + actions = { actionA; } + size = 512; + } + @name(".TernaryOne") table TernaryOne { key = { diff --git a/tests/testdata/unittest.p4info.txt b/tests/testdata/unittest.p4info.txt index 2ab06cb7..3b082baa 100644 --- a/tests/testdata/unittest.p4info.txt +++ b/tests/testdata/unittest.p4info.txt @@ -50,6 +50,28 @@ tables { } size: 512 } +tables { + preamble { + id: 33567651 + name: "LpmNonByte" + alias: "LpmNonByte" + } + match_fields { + id: 1 + name: "header_test.field20" + bitwidth: 20 + match_type: LPM + } + action_refs { + id: 16783703 + } + action_refs { + id: 16800567 + annotations: "@defaultonly" + scope: DEFAULT_ONLY + } + size: 512 +} tables { preamble { id: 33584148