| Message ID | 20200602051828.5734-4-xiaoliang.yang_1@nxp.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: ocelot: VCAP IS1 and ES0 support | expand |
Hi Xiaoliang, Happy to see that you are moving in the directions of multi chain - this seems ilke a much better fit to me. On 02.06.2020 13:18, Xiaoliang Yang wrote: >There are three hardware TCAMs for ocelot chips: IS1, IS2 and ES0. Each >one supports different actions. The hardware flow order is: IS1->IS2->ES0. > >This patch add three blocks to store rules according to chain index. >chain 0 is offloaded to IS1, chain 1 is offloaded to IS2, and egress chain >0 is offloaded to ES0. Using "static" allocation to to say chain-X goes to TCAM Y, also seems like the right approach to me. Given the capabilities of the HW, this will most likely be the easiest scheme to implement and to explain to the end-user. But I think we should make some adjustments to this mapping schema. Here are some important "things" I would like to consider when defining this schema: - As you explain, we have 3 TCAMs (IS1, IS2 and ES0), but we have 3 parallel lookups in IS1 and 2 parallel lookups in IS2 - and also these TCAMs has a wide verity of keys. - We can utilize these multiple parallel lookups such that it seems like they are done in serial (that is if they do not touch the same actions), but as they are done in parallel they can not influence each other. - We can let IS1 influence the IS2 lookup (like the GOTO actions was intended to be used). - The chip also has other QoS classification facilities which sits before the TCAM (take a look at 3.7.3 QoS, DP, and DSCP Classification in vsc7514 datasheet). It we at some point in time want to enable this, then I think we need to do that in the same tc-flower framework. Here is my initial suggestion for an alternative chain-schema: Chain 0: The default chain - today this is in IS2. If we proceed with this as is - then this will change. Chain 1-9999: These are offloaded by "basic" classification. Chain 10000-19999: These are offloaded in IS1 Chain 10000: Lookup-0 in IS1, and here we could limit the action to do QoS related stuff (priority update) Chain 11000: Lookup-1 in IS1, here we could do VLAN stuff Chain 12000: Lookup-2 in IS1, here we could apply the "PAG" which is essentially a GOTO. Chain 20000-29999: These are offloaded in IS2 Chain 20000-20255: Lookup-0 in IS2, where CHAIN-ID - 20000 is the PAG value. Chain 21000-21000: Lookup-1 in IS2. All these chains should be optional - users should only need to configure the chains they need. To make this work, we need to configure both the desired actions (could be priority update) and the goto action. Remember in HW, all packets goes through this process, while in SW they only follow the "goto" path. An example could be (I have not tested this yet - sorry): tc qdisc add dev eth0 ingress # Activate lookup 11000. We can not do any other rules in chain 0, also # this implicitly means that we do not want any chains <11000. tc filter add dev eth0 parent ffff: chain 0 action matchall goto 11000 tc filter add dev eth0 parent ffff: chain 11000 \ flower src_mac 00:01:00:00:00:00/00:ff:00:00:00:00 \ action \ vlan modify id 1234 \ pipe \ goto 20001 tc filter add dev eth0 parent ffff: chain 20001 ... Maybe it would be an idea to create some use-cases, implement them in a test which can pass with today's SW, and then once we have a common understanding of what we want, we can implement it? /Allan >Using action goto chain to express flow order as follows: > tc filter add dev swp0 chain 0 parent ffff: flower skip_sw \ > action goto chain 1 > >Signed-off-by: Xiaoliang Yang <xiaoliang.yang_1@nxp.com> >--- > drivers/net/ethernet/mscc/ocelot_ace.c | 51 +++++++++++++++-------- > drivers/net/ethernet/mscc/ocelot_ace.h | 7 ++-- > drivers/net/ethernet/mscc/ocelot_flower.c | 46 +++++++++++++++++--- > include/soc/mscc/ocelot.h | 2 +- > include/soc/mscc/ocelot_vcap.h | 4 +- > 5 files changed, 81 insertions(+), 29 deletions(-) > >diff --git a/drivers/net/ethernet/mscc/ocelot_ace.c b/drivers/net/ethernet/mscc/ocelot_ace.c >index 748c618db7d8..b76593b40097 100644 >--- a/drivers/net/ethernet/mscc/ocelot_ace.c >+++ b/drivers/net/ethernet/mscc/ocelot_ace.c >@@ -341,6 +341,8 @@ static void is2_action_set(struct ocelot *ocelot, struct vcap_data *data, > vcap_action_set(vcap, data, VCAP_IS2_ACT_CPU_QU_NUM, 0); > vcap_action_set(vcap, data, VCAP_IS2_ACT_CPU_COPY_ENA, 0); > break; >+ default: >+ break; > } > } > >@@ -644,9 +646,9 @@ static void is2_entry_set(struct ocelot *ocelot, int ix, > } > > static void vcap_entry_get(struct ocelot *ocelot, struct ocelot_ace_rule *rule, >- int ix) >+ int ix, int block_id) > { >- const struct vcap_props *vcap = &ocelot->vcap[VCAP_IS2]; >+ const struct vcap_props *vcap = &ocelot->vcap[block_id]; > struct vcap_data data; > int row, count; > u32 cnt; >@@ -663,6 +665,19 @@ static void vcap_entry_get(struct ocelot *ocelot, struct ocelot_ace_rule *rule, > rule->stats.pkts = cnt; > } > >+static void vcap_entry_set(struct ocelot *ocelot, int ix, >+ struct ocelot_ace_rule *ace, >+ int block_id) >+{ >+ switch (block_id) { >+ case VCAP_IS2: >+ is2_entry_set(ocelot, ix, ace); >+ break; >+ default: >+ break; >+ } >+} >+ > static void ocelot_ace_rule_add(struct ocelot *ocelot, > struct ocelot_acl_block *block, > struct ocelot_ace_rule *rule) >@@ -790,7 +805,7 @@ static bool ocelot_ace_is_problematic_non_mac_etype(struct ocelot_ace_rule *ace) > static bool ocelot_exclusive_mac_etype_ace_rules(struct ocelot *ocelot, > struct ocelot_ace_rule *ace) > { >- struct ocelot_acl_block *block = &ocelot->acl_block; >+ struct ocelot_acl_block *block = &ocelot->acl_block[VCAP_IS2]; > struct ocelot_ace_rule *tmp; > unsigned long port; > int i; >@@ -824,15 +839,16 @@ static bool ocelot_exclusive_mac_etype_ace_rules(struct ocelot *ocelot, > return true; > } > >-int ocelot_ace_rule_offload_add(struct ocelot *ocelot, >+int ocelot_ace_rule_offload_add(struct ocelot *ocelot, int block_id, > struct ocelot_ace_rule *rule, > struct netlink_ext_ack *extack) > { >- struct ocelot_acl_block *block = &ocelot->acl_block; >+ struct ocelot_acl_block *block = &ocelot->acl_block[block_id]; > struct ocelot_ace_rule *ace; > int i, index; > >- if (!ocelot_exclusive_mac_etype_ace_rules(ocelot, rule)) { >+ if (block_id == VCAP_IS2 && >+ !ocelot_exclusive_mac_etype_ace_rules(ocelot, rule)) { > NL_SET_ERR_MSG_MOD(extack, > "Cannot mix MAC_ETYPE with non-MAC_ETYPE rules"); > return -EBUSY; >@@ -847,11 +863,11 @@ int ocelot_ace_rule_offload_add(struct ocelot *ocelot, > /* Move down the rules to make place for the new rule */ > for (i = block->count - 1; i > index; i--) { > ace = ocelot_ace_rule_get_rule_index(block, i); >- is2_entry_set(ocelot, i, ace); >+ vcap_entry_set(ocelot, i, ace, block_id); > } > > /* Now insert the new rule */ >- is2_entry_set(ocelot, index, rule); >+ vcap_entry_set(ocelot, index, rule, block_id); > return 0; > } > >@@ -902,10 +918,10 @@ static void ocelot_ace_rule_del(struct ocelot *ocelot, > block->count--; > } > >-int ocelot_ace_rule_offload_del(struct ocelot *ocelot, >+int ocelot_ace_rule_offload_del(struct ocelot *ocelot, int block_id, > struct ocelot_ace_rule *rule) > { >- struct ocelot_acl_block *block = &ocelot->acl_block; >+ struct ocelot_acl_block *block = &ocelot->acl_block[block_id]; > struct ocelot_ace_rule del_ace; > struct ocelot_ace_rule *ace; > int i, index; >@@ -921,29 +937,29 @@ int ocelot_ace_rule_offload_del(struct ocelot *ocelot, > /* Move up all the blocks over the deleted rule */ > for (i = index; i < block->count; i++) { > ace = ocelot_ace_rule_get_rule_index(block, i); >- is2_entry_set(ocelot, i, ace); >+ vcap_entry_set(ocelot, i, ace, block_id); > } > > /* Now delete the last rule, because it is duplicated */ >- is2_entry_set(ocelot, block->count, &del_ace); >+ vcap_entry_set(ocelot, block->count, &del_ace, block_id); > > return 0; > } > >-int ocelot_ace_rule_stats_update(struct ocelot *ocelot, >+int ocelot_ace_rule_stats_update(struct ocelot *ocelot, int block_id, > struct ocelot_ace_rule *rule) > { >- struct ocelot_acl_block *block = &ocelot->acl_block; >+ struct ocelot_acl_block *block = &ocelot->acl_block[block_id]; > struct ocelot_ace_rule *tmp; > int index; > > index = ocelot_ace_rule_get_index_id(block, rule); >- vcap_entry_get(ocelot, rule, index); >+ vcap_entry_get(ocelot, rule, index, block_id); > > /* After we get the result we need to clear the counters */ > tmp = ocelot_ace_rule_get_rule_index(block, index); > tmp->stats.pkts = 0; >- is2_entry_set(ocelot, index, tmp); >+ vcap_entry_set(ocelot, index, tmp, block_id); > > return 0; > } >@@ -968,7 +984,7 @@ static void vcap_init(struct ocelot *ocelot, const struct vcap_props *vcap) > > int ocelot_ace_init(struct ocelot *ocelot) > { >- struct ocelot_acl_block *block = &ocelot->acl_block; >+ struct ocelot_acl_block *block; > > vcap_init(ocelot, &ocelot->vcap[VCAP_IS2]); > >@@ -987,6 +1003,7 @@ int ocelot_ace_init(struct ocelot *ocelot) > ocelot_write_gix(ocelot, 0x3fffff, ANA_POL_CIR_STATE, > OCELOT_POLICER_DISCARD); > >+ block = &ocelot->acl_block[VCAP_IS2]; > block->pol_lpr = OCELOT_POLICER_DISCARD - 1; > INIT_LIST_HEAD(&block->rules); > >diff --git a/drivers/net/ethernet/mscc/ocelot_ace.h b/drivers/net/ethernet/mscc/ocelot_ace.h >index 099e177f2617..a9fd99401a65 100644 >--- a/drivers/net/ethernet/mscc/ocelot_ace.h >+++ b/drivers/net/ethernet/mscc/ocelot_ace.h >@@ -175,6 +175,7 @@ struct ocelot_ace_frame_ipv6 { > }; > > enum ocelot_ace_action { >+ OCELOT_ACL_ACTION_NULL, > OCELOT_ACL_ACTION_DROP, > OCELOT_ACL_ACTION_TRAP, > OCELOT_ACL_ACTION_POLICE, >@@ -214,12 +215,12 @@ struct ocelot_ace_rule { > u32 pol_ix; > }; > >-int ocelot_ace_rule_offload_add(struct ocelot *ocelot, >+int ocelot_ace_rule_offload_add(struct ocelot *ocelot, int block_id, > struct ocelot_ace_rule *rule, > struct netlink_ext_ack *extack); >-int ocelot_ace_rule_offload_del(struct ocelot *ocelot, >+int ocelot_ace_rule_offload_del(struct ocelot *ocelot, int block_id, > struct ocelot_ace_rule *rule); >-int ocelot_ace_rule_stats_update(struct ocelot *ocelot, >+int ocelot_ace_rule_stats_update(struct ocelot *ocelot, int block_id, > struct ocelot_ace_rule *rule); > > int ocelot_ace_init(struct ocelot *ocelot); >diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c >index 891925f73cbc..a1f7b6b28170 100644 >--- a/drivers/net/ethernet/mscc/ocelot_flower.c >+++ b/drivers/net/ethernet/mscc/ocelot_flower.c >@@ -9,13 +9,26 @@ > > #include "ocelot_ace.h" > >+static int ocelot_block_id_get(int chain, bool ingress) >+{ >+ /* Select TCAM blocks by using chain index. Rules in chain 0 are >+ * implemented on IS1, chain 1 are implemented on IS2, and egress >+ * chain corresponds to ES0 block. >+ */ >+ if (ingress) >+ return chain ? VCAP_IS2 : VCAP_IS1; >+ else >+ return VCAP_ES0; >+} >+ > static int ocelot_flower_parse_action(struct flow_cls_offload *f, > struct ocelot_ace_rule *ace) > { >+ struct netlink_ext_ack *extack = f->common.extack; > const struct flow_action_entry *a; >+ int i, allowed_chain = 0; > s64 burst; > u64 rate; >- int i; > > if (!flow_offload_has_one_action(&f->rule->action)) > return -EOPNOTSUPP; >@@ -28,9 +41,11 @@ static int ocelot_flower_parse_action(struct flow_cls_offload *f, > switch (a->id) { > case FLOW_ACTION_DROP: > ace->action = OCELOT_ACL_ACTION_DROP; >+ allowed_chain = 1; > break; > case FLOW_ACTION_TRAP: > ace->action = OCELOT_ACL_ACTION_TRAP; >+ allowed_chain = 1; > break; > case FLOW_ACTION_POLICE: > ace->action = OCELOT_ACL_ACTION_POLICE; >@@ -38,10 +53,23 @@ static int ocelot_flower_parse_action(struct flow_cls_offload *f, > ace->pol.rate = div_u64(rate, 1000) * 8; > burst = rate * PSCHED_NS2TICKS(a->police.burst); > ace->pol.burst = div_u64(burst, PSCHED_TICKS_PER_SEC); >+ allowed_chain = 1; >+ break; >+ case FLOW_ACTION_GOTO: >+ if (a->chain_index != f->common.chain_index + 1) { >+ NL_SET_ERR_MSG_MOD(extack, "HW only support goto next chain\n"); >+ return -EOPNOTSUPP; >+ } >+ ace->action = OCELOT_ACL_ACTION_NULL; >+ allowed_chain = f->common.chain_index; > break; > default: > return -EOPNOTSUPP; > } >+ if (f->common.chain_index != allowed_chain) { >+ NL_SET_ERR_MSG_MOD(extack, "Action is not supported on this chain\n"); >+ return -EOPNOTSUPP; >+ } > } > > return 0; >@@ -205,7 +233,7 @@ int ocelot_cls_flower_replace(struct ocelot *ocelot, int port, > struct flow_cls_offload *f, bool ingress) > { > struct ocelot_ace_rule *ace; >- int ret; >+ int ret, block_id; > > ace = ocelot_ace_rule_create(ocelot, port, f); > if (!ace) >@@ -216,8 +244,10 @@ int ocelot_cls_flower_replace(struct ocelot *ocelot, int port, > kfree(ace); > return ret; > } >+ block_id = ocelot_block_id_get(f->common.chain_index, ingress); > >- return ocelot_ace_rule_offload_add(ocelot, ace, f->common.extack); >+ return ocelot_ace_rule_offload_add(ocelot, block_id, ace, >+ f->common.extack); > } > EXPORT_SYMBOL_GPL(ocelot_cls_flower_replace); > >@@ -225,11 +255,13 @@ int ocelot_cls_flower_destroy(struct ocelot *ocelot, int port, > struct flow_cls_offload *f, bool ingress) > { > struct ocelot_ace_rule ace; >+ int block_id; > > ace.prio = f->common.prio; > ace.id = f->cookie; >+ block_id = ocelot_block_id_get(f->common.chain_index, ingress); > >- return ocelot_ace_rule_offload_del(ocelot, &ace); >+ return ocelot_ace_rule_offload_del(ocelot, block_id, &ace); > } > EXPORT_SYMBOL_GPL(ocelot_cls_flower_destroy); > >@@ -237,11 +269,13 @@ int ocelot_cls_flower_stats(struct ocelot *ocelot, int port, > struct flow_cls_offload *f, bool ingress) > { > struct ocelot_ace_rule ace; >- int ret; >+ int ret, block_id; > > ace.prio = f->common.prio; > ace.id = f->cookie; >- ret = ocelot_ace_rule_stats_update(ocelot, &ace); >+ block_id = ocelot_block_id_get(f->common.chain_index, ingress); >+ >+ ret = ocelot_ace_rule_stats_update(ocelot, block_id, &ace); > if (ret) > return ret; > >diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h >index 91357b1c8f31..4b2320bdc036 100644 >--- a/include/soc/mscc/ocelot.h >+++ b/include/soc/mscc/ocelot.h >@@ -540,7 +540,7 @@ struct ocelot { > > struct list_head multicast; > >- struct ocelot_acl_block acl_block; >+ struct ocelot_acl_block acl_block[3]; > > const struct vcap_props *vcap; > >diff --git a/include/soc/mscc/ocelot_vcap.h b/include/soc/mscc/ocelot_vcap.h >index 26d9384b3657..495847a40490 100644 >--- a/include/soc/mscc/ocelot_vcap.h >+++ b/include/soc/mscc/ocelot_vcap.h >@@ -14,9 +14,9 @@ > */ > > enum { >- /* VCAP_IS1, */ >+ VCAP_IS1, > VCAP_IS2, >- /* VCAP_ES0, */ >+ VCAP_ES0, > }; > > struct vcap_props { >-- >2.17.1 > /Allan
Hi Allan, On Tue, 2 Jun 2020 at 11:38, Allan W. Nielsen <allan.nielsen@microchip.com> wrote: > > Hi Xiaoliang, > > Happy to see that you are moving in the directions of multi chain - this > seems ilke a much better fit to me. > > > On 02.06.2020 13:18, Xiaoliang Yang wrote: > >There are three hardware TCAMs for ocelot chips: IS1, IS2 and ES0. Each > >one supports different actions. The hardware flow order is: IS1->IS2->ES0. > > > >This patch add three blocks to store rules according to chain index. > >chain 0 is offloaded to IS1, chain 1 is offloaded to IS2, and egress chain > >0 is offloaded to ES0. > > Using "static" allocation to to say chain-X goes to TCAM Y, also seems > like the right approach to me. Given the capabilities of the HW, this > will most likely be the easiest scheme to implement and to explain to > the end-user. > > But I think we should make some adjustments to this mapping schema. > > Here are some important "things" I would like to consider when defining > this schema: > > - As you explain, we have 3 TCAMs (IS1, IS2 and ES0), but we have 3 > parallel lookups in IS1 and 2 parallel lookups in IS2 - and also these > TCAMs has a wide verity of keys. > > - We can utilize these multiple parallel lookups such that it seems like > they are done in serial (that is if they do not touch the same > actions), but as they are done in parallel they can not influence each > other. > > - We can let IS1 influence the IS2 lookup (like the GOTO actions was > intended to be used). > > - The chip also has other QoS classification facilities which sits > before the TCAM (take a look at 3.7.3 QoS, DP, and DSCP Classification > in vsc7514 datasheet). It we at some point in time want to enable > this, then I think we need to do that in the same tc-flower framework. > > Here is my initial suggestion for an alternative chain-schema: > > Chain 0: The default chain - today this is in IS2. If we proceed > with this as is - then this will change. > Chain 1-9999: These are offloaded by "basic" classification. > Chain 10000-19999: These are offloaded in IS1 > Chain 10000: Lookup-0 in IS1, and here we could limit the > action to do QoS related stuff (priority > update) > Chain 11000: Lookup-1 in IS1, here we could do VLAN > stuff > Chain 12000: Lookup-2 in IS1, here we could apply the > "PAG" which is essentially a GOTO. > > Chain 20000-29999: These are offloaded in IS2 > Chain 20000-20255: Lookup-0 in IS2, where CHAIN-ID - > 20000 is the PAG value. > Chain 21000-21000: Lookup-1 in IS2. > > All these chains should be optional - users should only need to > configure the chains they need. To make this work, we need to configure > both the desired actions (could be priority update) and the goto action. > Remember in HW, all packets goes through this process, while in SW they > only follow the "goto" path. > > An example could be (I have not tested this yet - sorry): > > tc qdisc add dev eth0 ingress > > # Activate lookup 11000. We can not do any other rules in chain 0, also > # this implicitly means that we do not want any chains <11000. > tc filter add dev eth0 parent ffff: chain 0 > action > matchall goto 11000 > > tc filter add dev eth0 parent ffff: chain 11000 \ > flower src_mac 00:01:00:00:00:00/00:ff:00:00:00:00 \ > action \ > vlan modify id 1234 \ > pipe \ > goto 20001 > > tc filter add dev eth0 parent ffff: chain 20001 ... > > Maybe it would be an idea to create some use-cases, implement them in a > test which can pass with today's SW, and then once we have a common > understanding of what we want, we can implement it? > > /Allan > > >Using action goto chain to express flow order as follows: > > tc filter add dev swp0 chain 0 parent ffff: flower skip_sw \ > > action goto chain 1 > > > >Signed-off-by: Xiaoliang Yang <xiaoliang.yang_1@nxp.com> > >--- > > drivers/net/ethernet/mscc/ocelot_ace.c | 51 +++++++++++++++-------- > > drivers/net/ethernet/mscc/ocelot_ace.h | 7 ++-- > > drivers/net/ethernet/mscc/ocelot_flower.c | 46 +++++++++++++++++--- > > include/soc/mscc/ocelot.h | 2 +- > > include/soc/mscc/ocelot_vcap.h | 4 +- > > 5 files changed, 81 insertions(+), 29 deletions(-) > /Allan What would be the advantage, from a user perspective, in exposing the 3 IS1 lookups as separate chains with orthogonal actions? If the user wants to add an IS1 action that performs QoS classification, VLAN classification and selects a custom PAG, they would have to install 3 separate filters with the same key, each into its own chain. Then the driver would be smart enough to figure out that the 3 keys are actually the same, so it could merge them. In comparison, we could just add a single filter to the IS1 chain, with 3 actions (skbedit priority, vlan modify, goto is2). Thanks, -Vladimir
On 03.06.2020 13:04, Vladimir Oltean wrote: >On Tue, 2 Jun 2020 at 11:38, Allan W. Nielsen ><allan.nielsen@microchip.com> wrote: >> >> Hi Xiaoliang, >> >> Happy to see that you are moving in the directions of multi chain - this >> seems ilke a much better fit to me. >> >> >> On 02.06.2020 13:18, Xiaoliang Yang wrote: >> >There are three hardware TCAMs for ocelot chips: IS1, IS2 and ES0. Each >> >one supports different actions. The hardware flow order is: IS1->IS2->ES0. >> > >> >This patch add three blocks to store rules according to chain index. >> >chain 0 is offloaded to IS1, chain 1 is offloaded to IS2, and egress chain >> >0 is offloaded to ES0. >> >> Using "static" allocation to to say chain-X goes to TCAM Y, also seems >> like the right approach to me. Given the capabilities of the HW, this >> will most likely be the easiest scheme to implement and to explain to >> the end-user. >> >> But I think we should make some adjustments to this mapping schema. >> >> Here are some important "things" I would like to consider when defining >> this schema: >> >> - As you explain, we have 3 TCAMs (IS1, IS2 and ES0), but we have 3 >> parallel lookups in IS1 and 2 parallel lookups in IS2 - and also these >> TCAMs has a wide verity of keys. >> >> - We can utilize these multiple parallel lookups such that it seems like >> they are done in serial (that is if they do not touch the same >> actions), but as they are done in parallel they can not influence each >> other. >> >> - We can let IS1 influence the IS2 lookup (like the GOTO actions was >> intended to be used). >> >> - The chip also has other QoS classification facilities which sits >> before the TCAM (take a look at 3.7.3 QoS, DP, and DSCP Classification >> in vsc7514 datasheet). It we at some point in time want to enable >> this, then I think we need to do that in the same tc-flower framework. >> >> Here is my initial suggestion for an alternative chain-schema: >> >> Chain 0: The default chain - today this is in IS2. If we proceed >> with this as is - then this will change. >> Chain 1-9999: These are offloaded by "basic" classification. >> Chain 10000-19999: These are offloaded in IS1 >> Chain 10000: Lookup-0 in IS1, and here we could limit the >> action to do QoS related stuff (priority >> update) >> Chain 11000: Lookup-1 in IS1, here we could do VLAN >> stuff >> Chain 12000: Lookup-2 in IS1, here we could apply the >> "PAG" which is essentially a GOTO. >> >> Chain 20000-29999: These are offloaded in IS2 >> Chain 20000-20255: Lookup-0 in IS2, where CHAIN-ID - >> 20000 is the PAG value. >> Chain 21000-21000: Lookup-1 in IS2. >> >> All these chains should be optional - users should only need to >> configure the chains they need. To make this work, we need to configure >> both the desired actions (could be priority update) and the goto action. >> Remember in HW, all packets goes through this process, while in SW they >> only follow the "goto" path. >> >> An example could be (I have not tested this yet - sorry): >> >> tc qdisc add dev eth0 ingress >> >> # Activate lookup 11000. We can not do any other rules in chain 0, also >> # this implicitly means that we do not want any chains <11000. >> tc filter add dev eth0 parent ffff: chain 0 >> action >> matchall goto 11000 >> >> tc filter add dev eth0 parent ffff: chain 11000 \ >> flower src_mac 00:01:00:00:00:00/00:ff:00:00:00:00 \ >> action \ >> vlan modify id 1234 \ >> pipe \ >> goto 20001 >> >> tc filter add dev eth0 parent ffff: chain 20001 ... >> >> Maybe it would be an idea to create some use-cases, implement them in a >> test which can pass with today's SW, and then once we have a common >> understanding of what we want, we can implement it? >> >> /Allan >> >> >Using action goto chain to express flow order as follows: >> > tc filter add dev swp0 chain 0 parent ffff: flower skip_sw \ >> > action goto chain 1 >> > >> >Signed-off-by: Xiaoliang Yang <xiaoliang.yang_1@nxp.com> >> >--- >> > drivers/net/ethernet/mscc/ocelot_ace.c | 51 +++++++++++++++-------- >> > drivers/net/ethernet/mscc/ocelot_ace.h | 7 ++-- >> > drivers/net/ethernet/mscc/ocelot_flower.c | 46 +++++++++++++++++--- >> > include/soc/mscc/ocelot.h | 2 +- >> > include/soc/mscc/ocelot_vcap.h | 4 +- >> > 5 files changed, 81 insertions(+), 29 deletions(-) > >> /Allan > >What would be the advantage, from a user perspective, in exposing the >3 IS1 lookups as separate chains with orthogonal actions? >If the user wants to add an IS1 action that performs QoS >classification, VLAN classification and selects a custom PAG, they >would have to install 3 separate filters with the same key, each into >its own chain. Then the driver would be smart enough to figure out >that the 3 keys are actually the same, so it could merge them. >In comparison, we could just add a single filter to the IS1 chain, >with 3 actions (skbedit priority, vlan modify, goto is2). Hi, I realize I forgot to answer this one - sorry. The reason for this design is that we have use-cases where the rules to do QoS classification must not impact VLAN classification. The easiest way to do that, it to have it in separated lookups. But we could make this more flexible to support your use-case better. A alternative approach would be to assign exclusive-right-to-action on first use. If the user choose to use the VLAN update in a given loopup, then it cannot be used in others. If the user attempt to use a given action across different lookups we need to return an error. Would that work for you? Any downside in such approach? /Allan
Hi Allan, On Mon, 8 Jun 2020 at 16:56, Allan W. Nielsen <allan.nielsen@microchip.com> wrote: > > On 03.06.2020 13:04, Vladimir Oltean wrote: > >On Tue, 2 Jun 2020 at 11:38, Allan W. Nielsen > ><allan.nielsen@microchip.com> wrote: > >> > >> Hi Xiaoliang, > >> > >> Happy to see that you are moving in the directions of multi chain - this > >> seems ilke a much better fit to me. > >> > >> > >> On 02.06.2020 13:18, Xiaoliang Yang wrote: > >> >There are three hardware TCAMs for ocelot chips: IS1, IS2 and ES0. Each > >> >one supports different actions. The hardware flow order is: IS1->IS2->ES0. > >> > > >> >This patch add three blocks to store rules according to chain index. > >> >chain 0 is offloaded to IS1, chain 1 is offloaded to IS2, and egress chain > >> >0 is offloaded to ES0. > >> > >> Using "static" allocation to to say chain-X goes to TCAM Y, also seems > >> like the right approach to me. Given the capabilities of the HW, this > >> will most likely be the easiest scheme to implement and to explain to > >> the end-user. > >> > >> But I think we should make some adjustments to this mapping schema. > >> > >> Here are some important "things" I would like to consider when defining > >> this schema: > >> > >> - As you explain, we have 3 TCAMs (IS1, IS2 and ES0), but we have 3 > >> parallel lookups in IS1 and 2 parallel lookups in IS2 - and also these > >> TCAMs has a wide verity of keys. > >> > >> - We can utilize these multiple parallel lookups such that it seems like > >> they are done in serial (that is if they do not touch the same > >> actions), but as they are done in parallel they can not influence each > >> other. > >> > >> - We can let IS1 influence the IS2 lookup (like the GOTO actions was > >> intended to be used). > >> > >> - The chip also has other QoS classification facilities which sits > >> before the TCAM (take a look at 3.7.3 QoS, DP, and DSCP Classification > >> in vsc7514 datasheet). It we at some point in time want to enable > >> this, then I think we need to do that in the same tc-flower framework. > >> > >> Here is my initial suggestion for an alternative chain-schema: > >> > >> Chain 0: The default chain - today this is in IS2. If we proceed > >> with this as is - then this will change. > >> Chain 1-9999: These are offloaded by "basic" classification. > >> Chain 10000-19999: These are offloaded in IS1 > >> Chain 10000: Lookup-0 in IS1, and here we could limit the > >> action to do QoS related stuff (priority > >> update) > >> Chain 11000: Lookup-1 in IS1, here we could do VLAN > >> stuff > >> Chain 12000: Lookup-2 in IS1, here we could apply the > >> "PAG" which is essentially a GOTO. > >> > >> Chain 20000-29999: These are offloaded in IS2 > >> Chain 20000-20255: Lookup-0 in IS2, where CHAIN-ID - > >> 20000 is the PAG value. > >> Chain 21000-21000: Lookup-1 in IS2. > >> > >> All these chains should be optional - users should only need to > >> configure the chains they need. To make this work, we need to configure > >> both the desired actions (could be priority update) and the goto action. > >> Remember in HW, all packets goes through this process, while in SW they > >> only follow the "goto" path. > >> > >> An example could be (I have not tested this yet - sorry): > >> > >> tc qdisc add dev eth0 ingress > >> > >> # Activate lookup 11000. We can not do any other rules in chain 0, also > >> # this implicitly means that we do not want any chains <11000. > >> tc filter add dev eth0 parent ffff: chain 0 > >> action > >> matchall goto 11000 > >> > >> tc filter add dev eth0 parent ffff: chain 11000 \ > >> flower src_mac 00:01:00:00:00:00/00:ff:00:00:00:00 \ > >> action \ > >> vlan modify id 1234 \ > >> pipe \ > >> goto 20001 > >> > >> tc filter add dev eth0 parent ffff: chain 20001 ... > >> > >> Maybe it would be an idea to create some use-cases, implement them in a > >> test which can pass with today's SW, and then once we have a common > >> understanding of what we want, we can implement it? > >> > >> /Allan > >> > >> >Using action goto chain to express flow order as follows: > >> > tc filter add dev swp0 chain 0 parent ffff: flower skip_sw \ > >> > action goto chain 1 > >> > > >> >Signed-off-by: Xiaoliang Yang <xiaoliang.yang_1@nxp.com> > >> >--- > >> > drivers/net/ethernet/mscc/ocelot_ace.c | 51 +++++++++++++++-------- > >> > drivers/net/ethernet/mscc/ocelot_ace.h | 7 ++-- > >> > drivers/net/ethernet/mscc/ocelot_flower.c | 46 +++++++++++++++++--- > >> > include/soc/mscc/ocelot.h | 2 +- > >> > include/soc/mscc/ocelot_vcap.h | 4 +- > >> > 5 files changed, 81 insertions(+), 29 deletions(-) > > > >> /Allan > > > >What would be the advantage, from a user perspective, in exposing the > >3 IS1 lookups as separate chains with orthogonal actions? > >If the user wants to add an IS1 action that performs QoS > >classification, VLAN classification and selects a custom PAG, they > >would have to install 3 separate filters with the same key, each into > >its own chain. Then the driver would be smart enough to figure out > >that the 3 keys are actually the same, so it could merge them. > >In comparison, we could just add a single filter to the IS1 chain, > >with 3 actions (skbedit priority, vlan modify, goto is2). > > Hi, I realize I forgot to answer this one - sorry. > > The reason for this design is that we have use-cases where the rules to > do QoS classification must not impact VLAN classification. The easiest > way to do that, it to have it in separated lookups. > Impact in the sense that an IS1 rule for VLAN classification might 'steal' packets from an IS1 rule for QoS classification (since the TCAM stops after the first matching entry in each lookup)? Such as: tc filter add dev swp0 ingress protocol 802.1Q flower vlan_ethtype ipv4 action skbedit priority 3 tc filter add dev swp0 ingress protocol 802.1Q flower vlan_id 0 vlan_prio 2 action vlan modify id 12 priority 1 The trouble with this TCAM seems to be that it doesn't support the 'pipe' control, so rules on overlapping keys will have unpredictable results. So if we had these 2 rules in the same lookup in hardware, then an IPv4 packet tagged with VID 0 and PCP 2 would get classified to QoS class 3 but would not get retagged to VID 12 PCP 1, right? So we should error out if the rule's control is 'pipe' (I haven't checked if we can do that, I hope we can), and basically only 'goto' something. Which brings the discussion to "goto where?". From the IS1 chain for QoS classification, you could goto the IS1 chain for VLAN classification, ok, that's your example. Is this goto optional? (i.e. can you goto a PAG in IS2 directly, and in that case would VLAN classification be skipped in hardware for the packets that skip it in software?) Your suggestion of having an entire chain for PAG selection suggests to me that the rules in chain 10000 (IS1 for QoS classification) would need to have a non-optional goto to chain 11000 (IS1 for VLAN classification), which in turn would have a non-optional goto to chain 12000, where there would be a bunch of rules with just a goto action that selects the IS2 policy. You can't have a goto from chains 10000 or 11000 directly into an IS2 chain (either the default policy of 0 or a custom one). > But we could make this more flexible to support your use-case better. A > alternative approach would be to assign exclusive-right-to-action on > first use. If the user choose to use the VLAN update in a given loopup, > then it cannot be used in others. > > If the user attempt to use a given action across different lookups we > need to return an error. > > Would that work for you? Any downside in such approach? > > /Allan > No, I definitely wasn't trying to suggest that. If I understand correctly what you're proposing, then I don't necessarily have anything against breaking compatibility with the current single-chain layout. Our users will definitely need an extra book for filling in TCAM filters (the other one being on how to use tc in general), but on the other hand, at the end of the day, having things like the actions rigidly assigned to static id's per chain is the more sane thing to do given the TCAM design. My only concern was literally that duplicating filter keys in a bunch of different chains (all of which eventually land in the same TCAM, just in different lookups) might result in waste of TCAM space. In Felix, VCAP IS1 has space for 512 half key entries. Consider just the trivial example when the user wants to do something like: tc filter add dev swp0 ingress protocol 802.1Q flower \ vlan_ethtype ipv4 src_ip 192.168.1.1 \ action skbedit priority 1 \ action vlan modify id 101 priority 1 tc filter add dev swp0 ingress protocol 802.1Q flower \ vlan_ethtype ipv4 src_ip 192.168.1.2 \ action skbedit priority 2 \ action vlan modify id 101 priority 2 and 100 more rules like that. With a forced "1-action-per-chain" split, the rules would have to look like this: Chain 10000: tc filter add dev swp0 ingress protocol 802.1Q flower \ vlan_ethtype ipv4 src_ip 192.168.1.1 \ action skbedit priority 1 tc filter add dev swp0 ingress protocol 802.1Q flower \ vlan_ethtype ipv4 src_ip 192.168.1.2 \ action skbedit priority 2 Chain 11000: tc filter add dev swp0 ingress protocol 802.1Q flower \ vlan_ethtype ipv4 src_ip 192.168.1.1 \ action vlan modify id 101 priority 1 tc filter add dev swp0 ingress protocol 802.1Q flower \ vlan_ethtype ipv4 src_ip 192.168.1.2 \ action vlan modify id 101 priority 2 So I would just like to avoid this latter approach consuming 200 entries. It sounds to me like it would need pretty complicated bookkeeping in the driver. Also, another topic: would the lookups in VCAP IS2 be ever exposed to the user? Looking at the VSC7514 datasheet Table 60 "MASK_MODE and PORT_MASK Combinations" (for how the action vectors resulted from different lookups combine into a single result), I think it's relatively difficult to make sense out of it. And last question: why the large spacing between chain IDs in your example? Thanks, -Vladimir
On 09.06.2020 15:55, Vladimir Oltean wrote: >EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >Hi Allan, > >On Mon, 8 Jun 2020 at 16:56, Allan W. Nielsen ><allan.nielsen@microchip.com> wrote: >> >> On 03.06.2020 13:04, Vladimir Oltean wrote: >> >On Tue, 2 Jun 2020 at 11:38, Allan W. Nielsen >> ><allan.nielsen@microchip.com> wrote: >> >> >> >> Hi Xiaoliang, >> >> >> >> Happy to see that you are moving in the directions of multi chain - this >> >> seems ilke a much better fit to me. >> >> >> >> >> >> On 02.06.2020 13:18, Xiaoliang Yang wrote: >> >> >There are three hardware TCAMs for ocelot chips: IS1, IS2 and ES0. Each >> >> >one supports different actions. The hardware flow order is: IS1->IS2->ES0. >> >> > >> >> >This patch add three blocks to store rules according to chain index. >> >> >chain 0 is offloaded to IS1, chain 1 is offloaded to IS2, and egress chain >> >> >0 is offloaded to ES0. >> >> >> >> Using "static" allocation to to say chain-X goes to TCAM Y, also seems >> >> like the right approach to me. Given the capabilities of the HW, this >> >> will most likely be the easiest scheme to implement and to explain to >> >> the end-user. >> >> >> >> But I think we should make some adjustments to this mapping schema. >> >> >> >> Here are some important "things" I would like to consider when defining >> >> this schema: >> >> >> >> - As you explain, we have 3 TCAMs (IS1, IS2 and ES0), but we have 3 >> >> parallel lookups in IS1 and 2 parallel lookups in IS2 - and also these >> >> TCAMs has a wide verity of keys. >> >> >> >> - We can utilize these multiple parallel lookups such that it seems like >> >> they are done in serial (that is if they do not touch the same >> >> actions), but as they are done in parallel they can not influence each >> >> other. >> >> >> >> - We can let IS1 influence the IS2 lookup (like the GOTO actions was >> >> intended to be used). >> >> >> >> - The chip also has other QoS classification facilities which sits >> >> before the TCAM (take a look at 3.7.3 QoS, DP, and DSCP Classification >> >> in vsc7514 datasheet). It we at some point in time want to enable >> >> this, then I think we need to do that in the same tc-flower framework. >> >> >> >> Here is my initial suggestion for an alternative chain-schema: >> >> >> >> Chain 0: The default chain - today this is in IS2. If we proceed >> >> with this as is - then this will change. >> >> Chain 1-9999: These are offloaded by "basic" classification. >> >> Chain 10000-19999: These are offloaded in IS1 >> >> Chain 10000: Lookup-0 in IS1, and here we could limit the >> >> action to do QoS related stuff (priority >> >> update) >> >> Chain 11000: Lookup-1 in IS1, here we could do VLAN >> >> stuff >> >> Chain 12000: Lookup-2 in IS1, here we could apply the >> >> "PAG" which is essentially a GOTO. >> >> >> >> Chain 20000-29999: These are offloaded in IS2 >> >> Chain 20000-20255: Lookup-0 in IS2, where CHAIN-ID - >> >> 20000 is the PAG value. >> >> Chain 21000-21000: Lookup-1 in IS2. >> >> >> >> All these chains should be optional - users should only need to >> >> configure the chains they need. To make this work, we need to configure >> >> both the desired actions (could be priority update) and the goto action. >> >> Remember in HW, all packets goes through this process, while in SW they >> >> only follow the "goto" path. >> >> >> >> An example could be (I have not tested this yet - sorry): >> >> >> >> tc qdisc add dev eth0 ingress >> >> >> >> # Activate lookup 11000. We can not do any other rules in chain 0, also >> >> # this implicitly means that we do not want any chains <11000. >> >> tc filter add dev eth0 parent ffff: chain 0 >> >> action >> >> matchall goto 11000 >> >> >> >> tc filter add dev eth0 parent ffff: chain 11000 \ >> >> flower src_mac 00:01:00:00:00:00/00:ff:00:00:00:00 \ >> >> action \ >> >> vlan modify id 1234 \ >> >> pipe \ >> >> goto 20001 >> >> >> >> tc filter add dev eth0 parent ffff: chain 20001 ... >> >> >> >> Maybe it would be an idea to create some use-cases, implement them in a >> >> test which can pass with today's SW, and then once we have a common >> >> understanding of what we want, we can implement it? >> >> >> >> /Allan >> >> >> >> >Using action goto chain to express flow order as follows: >> >> > tc filter add dev swp0 chain 0 parent ffff: flower skip_sw \ >> >> > action goto chain 1 >> >> > >> >> >Signed-off-by: Xiaoliang Yang <xiaoliang.yang_1@nxp.com> >> >> >--- >> >> > drivers/net/ethernet/mscc/ocelot_ace.c | 51 +++++++++++++++-------- >> >> > drivers/net/ethernet/mscc/ocelot_ace.h | 7 ++-- >> >> > drivers/net/ethernet/mscc/ocelot_flower.c | 46 +++++++++++++++++--- >> >> > include/soc/mscc/ocelot.h | 2 +- >> >> > include/soc/mscc/ocelot_vcap.h | 4 +- >> >> > 5 files changed, 81 insertions(+), 29 deletions(-) >> > >> >> /Allan >> > >> >What would be the advantage, from a user perspective, in exposing the >> >3 IS1 lookups as separate chains with orthogonal actions? >> >If the user wants to add an IS1 action that performs QoS >> >classification, VLAN classification and selects a custom PAG, they >> >would have to install 3 separate filters with the same key, each into >> >its own chain. Then the driver would be smart enough to figure out >> >that the 3 keys are actually the same, so it could merge them. >> >In comparison, we could just add a single filter to the IS1 chain, >> >with 3 actions (skbedit priority, vlan modify, goto is2). >> >> Hi, I realize I forgot to answer this one - sorry. >> >> The reason for this design is that we have use-cases where the rules to >> do QoS classification must not impact VLAN classification. The easiest >> way to do that, it to have it in separated lookups. >> > >Impact in the sense that an IS1 rule for VLAN classification might >'steal' packets from an IS1 rule for QoS classification (since the >TCAM stops after the first matching entry in each lookup)? >Such as: > >tc filter add dev swp0 ingress protocol 802.1Q flower vlan_ethtype >ipv4 action skbedit priority 3 >tc filter add dev swp0 ingress protocol 802.1Q flower vlan_id 0 >vlan_prio 2 action vlan modify id 12 priority 1 Yes, this is a good example >The trouble with this TCAM seems to be that it doesn't support the >'pipe' control, so rules on overlapping keys will have unpredictable >results. I think we may have a mis-understanding in what the pipe action does. It does not continue evaluting rules in the chain - it only continue evaluating the list of actions assiciated to the matched rule. At least, this is my understanding and it seems to align with the experiments I have done. >So if we had these 2 rules in the same lookup in hardware, >then an IPv4 packet tagged with VID 0 and PCP 2 would get classified >to QoS class 3 but would not get retagged to VID 12 PCP 1, right? Yes. And you will get the same in "pure" SW. >So we should error out if the rule's control is 'pipe' (I haven't >checked if we can do that, I hope we can), and basically only 'goto' >something. No - I think this is the "pipe" misunderstanding from above. But we cannot support "reclassify", "continue" and "pass". We should error out on that. >Which brings the discussion to "goto where?". >From the IS1 chain for QoS classification, you could goto the IS1 >chain for VLAN classification, ok, that's your example. Is this goto >optional? (i.e. can you goto a PAG in IS2 directly, and in that case >would VLAN classification be skipped in hardware for the packets that >skip it in software?) You are right, these GOTOs are not really optional, and not as flexible as the SW implementation. The HW take the same path every time. But if we do not put any rules in IS1-lookup2, then it is the same as if IS1-lookup2 is skipped. This mean: If we want to use IS1, lookup 0, 1 and 2 - then we need a match-all in chain 0 to goto 10000 - then all rules in IS1/0 (10000) must goto IS1/1 (11000). - then all rules in IS1/1 (11000) must goto IS1/2 (12000). If we want to use IS1, lookup 0, 1 - then we need a match-all in chain 0 to goto 10000 - then all rules in IS1/0 (10000) must goto IS1/1 (11000). If we want to use IS1, lookup 0 - then we need a match-all in chain 0 to goto 10000 I skiped the "goto" IS2 on purpose - we will get to it with your next question. >Your suggestion of having an entire chain for PAG selection suggests >to me that the rules in chain 10000 (IS1 for QoS classification) would >need to have a non-optional goto to chain 11000 (IS1 for VLAN >classification), which in turn would have a non-optional goto to chain >12000, where there would be a bunch of rules with just a goto action >that selects the IS2 policy. You can't have a goto from chains 10000 >or 11000 directly into an IS2 chain (either the default policy of 0 or >a custom one). I suggest a dedicated lookup for PAG because in the TCAM/HW implementation this is an action, and we need the 3 parallel lookups not to use the same actions. This also means that this needs to be the last-is1-look chain, which can goto IS2 (with a dedicated PAG). >> But we could make this more flexible to support your use-case better. A >> alternative approach would be to assign exclusive-right-to-action on >> first use. If the user choose to use the VLAN update in a given loopup, >> then it cannot be used in others. >> >> If the user attempt to use a given action across different lookups we >> need to return an error. >> >> Would that work for you? Any downside in such approach? > >No, I definitely wasn't trying to suggest that. But we may want to consider that, as it would be more flexible. >If I understand correctly what you're proposing, then I don't >necessarily have anything against breaking compatibility with the >current single-chain layout. Our users will definitely need an extra >book for filling in TCAM filters (the other one being on how to use tc >in general), but on the other hand, at the end of the day, having >things like the actions rigidly assigned to static id's per chain is >the more sane thing to do given the TCAM design. I agree. Documenting this to an extend that is actually understandable is a challenge. >My only concern was literally that duplicating filter keys in a bunch >of different chains (all of which eventually land in the same TCAM, >just in different lookups) might result in waste of TCAM space. In >Felix, VCAP IS1 has space for 512 half key entries. >Consider just the trivial example when the user wants to do something like: > >tc filter add dev swp0 ingress protocol 802.1Q flower \ > vlan_ethtype ipv4 src_ip 192.168.1.1 \ > action skbedit priority 1 \ > action vlan modify id 101 priority 1 >tc filter add dev swp0 ingress protocol 802.1Q flower \ > vlan_ethtype ipv4 src_ip 192.168.1.2 \ > action skbedit priority 2 \ > action vlan modify id 101 priority 2 > >and 100 more rules like that. >With a forced "1-action-per-chain" split, the rules would have to look >like this: > >Chain 10000: >tc filter add dev swp0 ingress protocol 802.1Q flower \ > vlan_ethtype ipv4 src_ip 192.168.1.1 \ > action skbedit priority 1 >tc filter add dev swp0 ingress protocol 802.1Q flower \ > vlan_ethtype ipv4 src_ip 192.168.1.2 \ > action skbedit priority 2 > >Chain 11000: >tc filter add dev swp0 ingress protocol 802.1Q flower \ > vlan_ethtype ipv4 src_ip 192.168.1.1 \ > action vlan modify id 101 priority 1 >tc filter add dev swp0 ingress protocol 802.1Q flower \ > vlan_ethtype ipv4 src_ip 192.168.1.2 \ > action vlan modify id 101 priority 2 > >So I would just like to avoid this latter approach consuming 200 >entries. It sounds to me like it would need pretty complicated >bookkeeping in the driver. Yes, I think it is a valid twist which we should address. I see no problem in "dynamic" assign what chain own what action. >Also, another topic: would the lookups in VCAP IS2 be ever exposed to >the user? Looking at the VSC7514 datasheet Table 60 "MASK_MODE and >PORT_MASK Combinations" (for how the action vectors resulted from >different lookups combine into a single result), I think it's >relatively difficult to make sense out of it. It is not too easy to understand - but this is basically just the "hooks" to assign actions to a give lookup. >And last question: why the large spacing between chain IDs in your example? It is an attempt to make the validation and documentation easier. TCAM (1, 2) |Lookup number (0, 1, 2) || PAG ID || | vv/-\ 20123 The following chains will be valid: 0, 10000, 11000, 12000, 20000-20255, 21000. - Rules in chain 10000 must point to a higher number, but can not "skip any numbers" - Rules in chain 11000 must point to a higher number, but can not "skip any numbers" - Rules in chain 12000 must point to a higher number, but can not "skip any numbers" - Rules in chain 20000-20255 must point to none, or 21000 - Rules in 21000 can not have any goto. The user decide how many chains to use. Example could be only to use 12000, 20010, 20020. Maybe there is a better way - I'm open for other suggestions. But be aware, this is still not fully thought through. Before implementing I will need to understand how the templates and the shared blocks works as well. /Allan
Hi Allan, On 11.06.2002 2:18, Allan W. Nielsen <allan.nielsen@microchip.com> wrote: >> >> Here is my initial suggestion for an alternative chain-schema: >> >> >> >> Chain 0: The default chain - today this is in IS2. If we proceed >> >> with this as is - then this will change. >> >> Chain 1-9999: These are offloaded by "basic" classification. >> >> Chain 10000-19999: These are offloaded in IS1 >> >> Chain 10000: Lookup-0 in IS1, and here we could limit the >> >> action to do QoS related stuff (priority >> >> update) >> >> Chain 11000: Lookup-1 in IS1, here we could do VLAN >> >> stuff >> >> Chain 12000: Lookup-2 in IS1, here we could apply the >> >> "PAG" which is essentially a GOTO. >> >> >> >> Chain 20000-29999: These are offloaded in IS2 >> >> Chain 20000-20255: Lookup-0 in IS2, where CHAIN-ID - >> >> 20000 is the PAG value. >> >> Chain 21000-21000: Lookup-1 in IS2. >> >> >> >> All these chains should be optional - users should only need to >> >> configure the chains they need. To make this work, we need to >> >> configure both the desired actions (could be priority update) and the goto action. >> >> Remember in HW, all packets goes through this process, while in SW >> >> they only follow the "goto" path. >> >> I agree with this chain assignment, following is an example to set rules: 1. Set a matchall rule for each chain, the last chain do not need goto chain action. # tc filter add dev swp0 chain 0 flower skip_sw action goto chain 10000 # tc filter add dev swp0 chain 10000 flower skip_sw action goto chain 21000 In driver, use these rules to register the chain. 2. Set normal rules. # tc filter add dev swp0 chain 10000 protocol 802.1Q parent ffff: flower skip_sw vlan_id 1 vlan_prio 1 action skbedit priority 1 action goto chain 21000 # tc filter add dev swp0 chain 21000 protocol 802.1Q parent ffff: flower skip_sw vlan_id 1 vlan_prio 1 action drop In driver, we check if the chain ID has been registered, and goto chain is the same as first matchall rule, if is not, then return error. Each rule need has goto action except last chain. I also have check about chain template, it can not set an action template for each chain, so I think it's no use for our case. If this way to set rules is OK, I will update the patch to do as this. Thanks, Xiaoliang Yang
Hi Xiaoliang, The 07/16/2020 06:49, Xiaoliang Yang wrote: > Hi Allan, > > On 11.06.2002 2:18, Allan W. Nielsen <allan.nielsen@microchip.com> wrote: > >> >> Here is my initial suggestion for an alternative chain-schema: > >> >> > >> >> Chain 0: The default chain - today this is in IS2. If we proceed > >> >> with this as is - then this will change. > >> >> Chain 1-9999: These are offloaded by "basic" classification. > >> >> Chain 10000-19999: These are offloaded in IS1 > >> >> Chain 10000: Lookup-0 in IS1, and here we could limit the > >> >> action to do QoS related stuff (priority > >> >> update) > >> >> Chain 11000: Lookup-1 in IS1, here we could do VLAN > >> >> stuff > >> >> Chain 12000: Lookup-2 in IS1, here we could apply the > >> >> "PAG" which is essentially a GOTO. > >> >> > >> >> Chain 20000-29999: These are offloaded in IS2 > >> >> Chain 20000-20255: Lookup-0 in IS2, where CHAIN-ID - > >> >> 20000 is the PAG value. > >> >> Chain 21000-21000: Lookup-1 in IS2. > >> >> > >> >> All these chains should be optional - users should only need to > >> >> configure the chains they need. To make this work, we need to > >> >> configure both the desired actions (could be priority update) and the goto action. > >> >> Remember in HW, all packets goes through this process, while in SW > >> >> they only follow the "goto" path. > >> >> > > I agree with this chain assignment, following is an example to set rules: > > 1. Set a matchall rule for each chain, the last chain do not need goto chain action. > # tc filter add dev swp0 chain 0 flower skip_sw action goto chain 10000 > # tc filter add dev swp0 chain 10000 flower skip_sw action goto chain 21000 > In driver, use these rules to register the chain. > > 2. Set normal rules. > # tc filter add dev swp0 chain 10000 protocol 802.1Q parent ffff: flower skip_sw vlan_id 1 vlan_prio 1 action skbedit priority 1 action goto chain 21000 > # tc filter add dev swp0 chain 21000 protocol 802.1Q parent ffff: flower skip_sw vlan_id 1 vlan_prio 1 action drop > > In driver, we check if the chain ID has been registered, and goto chain is the same as first matchall rule, if is not, then return error. Each rule need has goto action except last chain. > > I also have check about chain template, it can not set an action template for each chain, so I think it's no use for our case. If this way to set rules is OK, I will update the patch to do as this. > > Thanks, > Xiaoliang Yang > I agree that you cannot set an action template for each chain but you can set a match template which for example can be used for setting up which IS1 key to generate for the device/port. The template ensures that you cannot add an illegal match. I have attached a snippet from a testcase I wrote in order to test these ideas. Note that not all actions are valid for the hardware. SMAC = "00:00:00:11:11:11" DMAC = "00:00:00:dd:dd:dd" VID1 = 0x10 VID2 = 0x20 PCP1 = 3 PCP2 = 5 DEI = 1 SIP = "10.10.0.1" DIP = "10.10.0.2" IS1_L0 = 10000 # IS1 lookup 0 IS1_L1 = 11000 # IS1 lookup 1 IS1_L2 = 12000 # IS1 lookup 2 IS2_L0 = 20000 # IS2 lookup 0 # IS2 20000 - 20255 -> pag 0-255 IS2_L0_P1 = 20001 # IS2 lookup 0 pag 1 IS2_L0_P2 = 20002 # IS2 lookup 0 pag 2 IS2_L1 = 21000 # IS2 lookup 1 $skip = "skip_hw" # or "skip_sw" test "Chain templates and goto" do t_i "'prio #' sets the sequence of filters. Lowest number = highest priority = checked first. 0..0xffff" t_i "'handle #' is a reference to the filter. Use this is if you need to reference the filter later. 0..0xffffffff" t_i "'chain #' is the chain to use. Chain 0 is the default. Different chains can have different templates. 0..0xffffffff" $ts.dut.run "tc qdisc add dev #{$dp[0]} clsact" t_i "Add templates" t_i "Configure the VCAP port configuration to match the shortest key that fulfill the purpose" t_i "Create a template that sets IS1 lookup 0 to generate S1_NORMAL with S1_DMAC_DIP_ENA" t_i "If you match on both src and dst you will generate S1_7TUPLE" $ts.dut.run "tc chain add dev #{$dp[0]} ingress protocol ip chain #{IS1_L0} flower #{$skip} "\ "dst_mac 00:00:00:00:00:00 "\ "dst_ip 0.0.0.0 " t_i "Create a template that sets IS1 lookup 1 to generate S1_5TUPLE_IP4" $ts.dut.run "tc chain add dev #{$dp[0]} ingress protocol ip chain #{IS1_L1} flower #{$skip} "\ "src_ip 0.0.0.0 "\ "dst_ip 0.0.0.0 " t_i "Create a template that sets IS1 lookup 2 to generate S1_DBL_VID" $ts.dut.run "tc chain add dev #{$dp[0]} ingress protocol 802.1ad chain #{IS1_L2} flower #{$skip} "\ "vlan_id 0 "\ "vlan_prio 0 "\ "vlan_ethtype 802.1q "\ "cvlan_id 0 "\ "cvlan_prio 0 " $ts.dut.run "tc chain show dev #{$dp[0]} ingress" t_i "Start the chaining party. We can have other matchall rules here but the last one must goto IS1" $ts.dut.run "tc filter add dev #{$dp[0]} ingress protocol all prio 0xffff handle 0x1 matchall #{$skip} "\ "action goto chain #{IS1_L0} " t_i "Insert catch all last in chain IS1_L0. Note: Protocol == all and prio = max" t_i "flower must be used here in order to satisfy the template although it is used as a 'matchall' filter." t_i "Driver must enforce that every filter in chain IS1_L0 ends with a goto chain IS1_L1" $ts.dut.run "tc filter add dev #{$dp[0]} ingress protocol all prio 0xffff handle 0x199 chain #{IS1_L0} flower #{$skip} "\ "action mirred egress mirror dev #{$dp[2]} "\ "action goto chain #{IS1_L1} " t_i "Insert catch all last in chain IS1_L1. Note: Protocol == all and prio = max" t_i "flower must be used here in order to satisfy the template although it is used as a 'matchall' filter." t_i "Driver must enforce that every filter in chain IS1_L1 ends with a goto chain IS1_L2" $ts.dut.run "tc filter add dev #{$dp[0]} ingress protocol all prio 0xffff handle 0x299 chain #{IS1_L1} flower #{$skip} "\ "action goto chain #{IS1_L2} " t_i "Insert catch all last in chain IS1_L2. Note: Protocol == all and prio = max" t_i "flower must be used here in order to satisfy the template although it is used as a 'matchall' filter." t_i "Driver must enforce that every filter in chain IS1_L2 ends with a goto chain IS2_L0 + PAG value 0..255" $ts.dut.run "tc filter add dev #{$dp[0]} ingress protocol all prio 0xffff handle 0x399 chain #{IS1_L2} flower #{$skip} "\ "action continue " # goto IS2! t_i "Insert in chain IS1_L0" $ts.dut.run "tc filter add dev #{$dp[0]} ingress protocol ip prio 10 handle 0x100 chain #{IS1_L0} flower #{$skip} "\ "dst_mac #{DMAC} "\ "dst_ip #{DIP} "\ "action goto chain #{IS1_L1} " t_i "Insert in chain IS1_L1" $ts.dut.run "tc filter add dev #{$dp[0]} ingress protocol ip prio 11 handle 0x200 chain #{IS1_L1} flower #{$skip} "\ "src_ip #{SIP} "\ "dst_ip #{DIP} "\ "action goto chain #{IS1_L2} " t_i "Insert in chain IS1_L1" $ts.dut.run "tc filter add dev #{$dp[0]} ingress protocol ip prio 12 handle 0x201 chain #{IS1_L1} flower #{$skip} "\ "dst_ip #{DIP} "\ "action goto chain #{IS1_L2} " t_i "Insert in chain IS1_L2" $ts.dut.run "tc filter add dev #{$dp[0]} ingress protocol 802.1ad prio 11 handle 0x300 chain #{IS1_L2} flower #{$skip} "\ "vlan_id 10 "\ "vlan_prio 1 "\ "vlan_ethtype 802.1q "\ "cvlan_id 20 "\ "cvlan_prio 2 "\ "action pass " # TODO: goto IS2! # TODO: Add IS2 t_i "Test invalid inserts that must fail" $ts.dut.run_err "tc filter add dev #{$dp[0]} ingress protocol ip chain #{IS1_L0} flower #{$skip} "\ "src_ip 10.10.0.0/16 "\ "action drop" $ts.dut.run_err "tc filter add dev #{$dp[0]} ingress protocol ip chain #{IS1_L1} flower #{$skip} "\ "dst_mac aa:11:22:33:44:55/00:00:ff:00:00:00 "\ "action drop" $ts.dut.run_err "tc filter add dev #{$dp[0]} ingress protocol ip chain #{IS1_L2} flower #{$skip} "\ "ip_proto udp "\ "action drop" end
Hi Joergen, -----Original Message----- From: Joergen Andreasen <joergen.andreasen@microchip.com> Sent: 2020年7月16日 16:51 > >> >> Chain 0: The default chain - today this is in IS2. If we proceed > >> >> with this as is - then this will change. > >> >> Chain 1-9999: These are offloaded by "basic" classification. > >> >> Chain 10000-19999: These are offloaded in IS1 > >> >> Chain 10000: Lookup-0 in IS1, and here we could limit the > >> >> action to do QoS related stuff (priority > >> >> update) > >> >> Chain 11000: Lookup-1 in IS1, here we could do VLAN > >> >> stuff > >> >> Chain 12000: Lookup-2 in IS1, here we could apply the > >> >> "PAG" which is essentially a GOTO. > >> >> > >> >> Chain 20000-29999: These are offloaded in IS2 > >> >> Chain 20000-20255: Lookup-0 in IS2, where CHAIN-ID - > >> >> 20000 is the PAG value. > >> >> Chain 21000-21000: Lookup-1 in IS2. > >> >> > >> >> All these chains should be optional - users should only need to > >> >> configure the chains they need. To make this work, we need to > >> >> configure both the desired actions (could be priority update) and the goto action. > >> >> Remember in HW, all packets goes through this process, while in > >> >> SW they only follow the "goto" path. > >> >> >> >> I agree with this chain assignment, following is an example to set rules: >> >> 1. Set a matchall rule for each chain, the last chain do not need goto chain action. >> # tc filter add dev swp0 chain 0 flower skip_sw action goto chain >> 10000 # tc filter add dev swp0 chain 10000 flower skip_sw action goto >> chain 21000 In driver, use these rules to register the chain. >> >> 2. Set normal rules. >> # tc filter add dev swp0 chain 10000 protocol 802.1Q parent ffff: >> flower skip_sw vlan_id 1 vlan_prio 1 action skbedit priority 1 action >> goto chain 21000 # tc filter add dev swp0 chain 21000 protocol 802.1Q >> parent ffff: flower skip_sw vlan_id 1 vlan_prio 1 action drop >> >> In driver, we check if the chain ID has been registered, and goto chain is the same as first matchall rule, if is not, then return error. Each rule need has goto action except last chain. >> >> I also have check about chain template, it can not set an action template for each chain, so I think it's no use for our case. If this way to set rules is OK, I will update the patch to do as this. >> >> Thanks, >> Xiaoliang Yang > > I agree that you cannot set an action template for each chain but you can set a match template which for example can be used for setting up which IS1 key to generate for the device/port. > The template ensures that you cannot add an illegal match. > I have attached a snippet from a testcase I wrote in order to test these ideas. > Note that not all actions are valid for the hardware. > > SMAC = "00:00:00:11:11:11" > DMAC = "00:00:00:dd:dd:dd" > VID1 = 0x10 > VID2 = 0x20 > PCP1 = 3 > PCP2 = 5 > DEI = 1 > SIP = "10.10.0.1" > DIP = "10.10.0.2" > > IS1_L0 = 10000 # IS1 lookup 0 > IS1_L1 = 11000 # IS1 lookup 1 > IS1_L2 = 12000 # IS1 lookup 2 > > IS2_L0 = 20000 # IS2 lookup 0 # IS2 20000 - 20255 -> pag 0-255 > IS2_L0_P1 = 20001 # IS2 lookup 0 pag 1 > IS2_L0_P2 = 20002 # IS2 lookup 0 pag 2 > > IS2_L1 = 21000 # IS2 lookup 1 > > $skip = "skip_hw" # or "skip_sw" > > test "Chain templates and goto" do > t_i "'prio #' sets the sequence of filters. Lowest number = highest priority = checked first. 0..0xffff" > t_i "'handle #' is a reference to the filter. Use this is if you need to reference the filter later. 0..0xffffffff" > t_i "'chain #' is the chain to use. Chain 0 is the default. Different chains can have different templates. 0..0xffffffff" > $ts.dut.run "tc qdisc add dev #{$dp[0]} clsact" > > t_i "Add templates" > t_i "Configure the VCAP port configuration to match the shortest key that fulfill the purpose" > t_i "Create a template that sets IS1 lookup 0 to generate S1_NORMAL with S1_DMAC_DIP_ENA" > t_i "If you match on both src and dst you will generate S1_7TUPLE" > $ts.dut.run "tc chain add dev #{$dp[0]} ingress protocol ip chain #{IS1_L0} flower #{$skip} "\ > "dst_mac 00:00:00:00:00:00 "\ > "dst_ip 0.0.0.0 " > > t_i "Create a template that sets IS1 lookup 1 to generate S1_5TUPLE_IP4" > $ts.dut.run "tc chain add dev #{$dp[0]} ingress protocol ip chain #{IS1_L1} flower #{$skip} "\ > "src_ip 0.0.0.0 "\ > "dst_ip 0.0.0.0 " > > t_i "Create a template that sets IS1 lookup 2 to generate S1_DBL_VID" > $ts.dut.run "tc chain add dev #{$dp[0]} ingress protocol 802.1ad chain #{IS1_L2} flower #{$skip} "\ > "vlan_id 0 "\ > "vlan_prio 0 "\ > "vlan_ethtype 802.1q "\ > "cvlan_id 0 "\ > "cvlan_prio 0 " > > $ts.dut.run "tc chain show dev #{$dp[0]} ingress" Why you set different filter keys on different lookup? Each lookup only filter one type of keys? If I want to filter a same key like dst_mac and do both QoS classified action and vlan modify action, how to implement this in the same chain #{IS1_L0} ? I think it's more reasonable to distinguish different lookup by different action like this: IS1_L0 = 10000 # IS1 lookup 0 # do QoS classified action IS1_L1 = 11000 # IS1 lookup 1 # do vlan modify action IS1_L2 = 12000 # IS1 lookup 2 # do goto PAG action IS2_L0 = 20000 # IS2 lookup 0 # IS2 20000 - 20255 -> pag 0-255 IS2_L1 = 21000 # IS2 lookup 1 So it’s no need to add templates, each lookup can support filtering mac, IP or vlan tag, but only support one action. Thanks, Xiaoliang
On Thu, Jul 16, 2020 at 10:37:40AM +0000, Xiaoliang Yang wrote: > Hi Joergen, > > > -----Original Message----- > From: Joergen Andreasen <joergen.andreasen@microchip.com> > Sent: 2020年7月16日 16:51 > > > >> >> Chain 0: The default chain - today this is in IS2. If we proceed > > >> >> with this as is - then this will change. > > >> >> Chain 1-9999: These are offloaded by "basic" classification. > > >> >> Chain 10000-19999: These are offloaded in IS1 > > >> >> Chain 10000: Lookup-0 in IS1, and here we could limit the > > >> >> action to do QoS related stuff (priority > > >> >> update) > > >> >> Chain 11000: Lookup-1 in IS1, here we could do VLAN > > >> >> stuff > > >> >> Chain 12000: Lookup-2 in IS1, here we could apply the > > >> >> "PAG" which is essentially a GOTO. > > >> >> > > >> >> Chain 20000-29999: These are offloaded in IS2 > > >> >> Chain 20000-20255: Lookup-0 in IS2, where CHAIN-ID - > > >> >> 20000 is the PAG value. > > >> >> Chain 21000-21000: Lookup-1 in IS2. > > >> >> > > >> >> All these chains should be optional - users should only need to > > >> >> configure the chains they need. To make this work, we need to > > >> >> configure both the desired actions (could be priority update) and the goto action. > > >> >> Remember in HW, all packets goes through this process, while in > > >> >> SW they only follow the "goto" path. > > >> >> > >> > >> I agree with this chain assignment, following is an example to set rules: > >> > >> 1. Set a matchall rule for each chain, the last chain do not need goto chain action. > >> # tc filter add dev swp0 chain 0 flower skip_sw action goto chain 10000 > >> # tc filter add dev swp0 chain 10000 flower skip_sw action goto chain 21000 > >> In driver, use these rules to register the chain. > >> > >> 2. Set normal rules. > >> # tc filter add dev swp0 chain 10000 protocol 802.1Q parent ffff: flower skip_sw vlan_id 1 vlan_prio 1 action skbedit priority 1 action goto chain 21000 > >> # tc filter add dev swp0 chain 21000 protocol 802.1Q parent ffff: flower skip_sw vlan_id 1 vlan_prio 1 action drop > >> > >> In driver, we check if the chain ID has been registered, and goto > >> chain is the same as first matchall rule, if is not, then return > >> error. Each rule need has goto action except last chain. > >> > >> I also have check about chain template, it can not set an action > >> template for each chain, so I think it's no use for our case. If > >> this way to set rules is OK, I will update the patch to do as this. > >> > >> Thanks, > >> Xiaoliang Yang > > > > > I agree that you cannot set an action template for each chain but > > you can set a match template which for example can be used for > > setting up which IS1 key to generate for the device/port. > > The template ensures that you cannot add an illegal match. > > I have attached a snippet from a testcase I wrote in order to test these ideas. > > Note that not all actions are valid for the hardware. > > > > SMAC = "00:00:00:11:11:11" > > DMAC = "00:00:00:dd:dd:dd" > > VID1 = 0x10 > > VID2 = 0x20 > > PCP1 = 3 > > PCP2 = 5 > > DEI = 1 > > SIP = "10.10.0.1" > > DIP = "10.10.0.2" > > > > IS1_L0 = 10000 # IS1 lookup 0 > > IS1_L1 = 11000 # IS1 lookup 1 > > IS1_L2 = 12000 # IS1 lookup 2 > > > > IS2_L0 = 20000 # IS2 lookup 0 # IS2 20000 - 20255 -> pag 0-255 > > IS2_L0_P1 = 20001 # IS2 lookup 0 pag 1 > > IS2_L0_P2 = 20002 # IS2 lookup 0 pag 2 > > > > IS2_L1 = 21000 # IS2 lookup 1 > > > > $skip = "skip_hw" # or "skip_sw" > > > > test "Chain templates and goto" do > > t_i "'prio #' sets the sequence of filters. Lowest number = highest priority = checked first. 0..0xffff" > > t_i "'handle #' is a reference to the filter. Use this is if you need to reference the filter later. 0..0xffffffff" > > t_i "'chain #' is the chain to use. Chain 0 is the default. Different chains can have different templates. 0..0xffffffff" > > $ts.dut.run "tc qdisc add dev #{$dp[0]} clsact" > > > > t_i "Add templates" > > t_i "Configure the VCAP port configuration to match the shortest key that fulfill the purpose" > > > t_i "Create a template that sets IS1 lookup 0 to generate S1_NORMAL with S1_DMAC_DIP_ENA" > > t_i "If you match on both src and dst you will generate S1_7TUPLE" > > $ts.dut.run "tc chain add dev #{$dp[0]} ingress protocol ip chain #{IS1_L0} flower #{$skip} "\ > > "dst_mac 00:00:00:00:00:00 "\ > > "dst_ip 0.0.0.0 " > > > > t_i "Create a template that sets IS1 lookup 1 to generate S1_5TUPLE_IP4" > > $ts.dut.run "tc chain add dev #{$dp[0]} ingress protocol ip chain #{IS1_L1} flower #{$skip} "\ > > "src_ip 0.0.0.0 "\ > > "dst_ip 0.0.0.0 " > > > > t_i "Create a template that sets IS1 lookup 2 to generate S1_DBL_VID" > > $ts.dut.run "tc chain add dev #{$dp[0]} ingress protocol 802.1ad chain #{IS1_L2} flower #{$skip} "\ > > "vlan_id 0 "\ > > "vlan_prio 0 "\ > > "vlan_ethtype 802.1q "\ > > "cvlan_id 0 "\ > > "cvlan_prio 0 " > > > > $ts.dut.run "tc chain show dev #{$dp[0]} ingress" > > Why you set different filter keys on different lookup? Each lookup > only filter one type of keys? > If I want to filter a same key like dst_mac and do both QoS classified > action and vlan modify action, how to implement this in the same chain > #{IS1_L0} ? > > I think it's more reasonable to distinguish different lookup by different action like this: > IS1_L0 = 10000 # IS1 lookup 0 # do QoS classified action > IS1_L1 = 11000 # IS1 lookup 1 # do vlan modify action > IS1_L2 = 12000 # IS1 lookup 2 # do goto PAG action > > IS2_L0 = 20000 # IS2 lookup 0 # IS2 20000 - 20255 -> pag 0-255 > IS2_L1 = 21000 # IS2 lookup 1 > > So it’s no need to add templates, each lookup can support filtering > mac, IP or vlan tag, but only support one action. > > Thanks, > Xiaoliang As far as I understand, he's still using the static chain numbers exactly for that, even though he didn't explicitly mention the action for each individual IS1 lookup. The reason why he's also adding templates on each individual chain is to be able to configure VCAP_S1_KEY_CFG and VCAP_S2_CFG. The configuration of key type is per lookup. Honestly, Joergen, I would take dynamic key configuration per lookup as a separate item. Xiaoliang's patch series for IS1 support is pretty large already. Right now we have: - In mainline: S2_IP6_CFG S2_IP6_CFG controls the key generation for IPv6 frames. Bits 1:0 control the first lookup and bits 3:2 control the second lookup. 0: IPv6 frames are matched against IP6_TCP_UDP or IP6_OTHER entries 1: IPv6 frames are matched against IP6_STD entries 2: IPv6 frames are matched against IP4_TCP_UDP or IP4_OTHER entries 3: IPv6 frames are matched against MAC_ETYPE entries We set this field to 0xa (0b1010, aka 2 for both lookups: IP4_TCP_UDP). Although we don't really parse IPv6 keys coming from tc. Also there are these fields which we're managing dynamically through ocelot_match_all_as_mac_etype, depending on whether there is any MAC_ETYPE key added by the user: S2_SNAP_DIS S2_ARP_DIS S2_IP_TCPUDP_DIS S2_IP_OTHER_DIS - In Xiaoliang's patchset: S1_KEY_IP6_CFG Selects key per lookup in S1 for IPv6 frames. 0: Use key S1_NORMAL 1: Use key S1_7TUPLE 2: Use key S1_5TUPLE_IP4 3: Use key S1_NORMAL_IP6 4: Use key S1_5TUPLE_IP6 5: Use key S1_DBL_VID We set this to 2. S1_KEY_IP4_CFG Selects key per lookup in S1 for IPv4 frames. 0: Use key S1_NORMAL 1: Use key S1_7TUPLE 2: Use key S1_5TUPLE_IP4 3: Use key S1_DBL_VID We set this to 2. Your input on which tc chain template could be used for each key type is valuable, we should create a table with all the options and associated key sizes (and therefore, number of available filters) and post it somewhere. I'm not completely sure that chains will be enough to describe every key type, at least not intuitively, For example if I just want to match on EtherType (protocol), I'll need an ETYPE (IS1) or MAC_ETYPE (IS2) rule, but the template for that will need to be formulated in terms of dst_mac because I don't think there's a way to use only the protocol in a template. But I expect we keep using some default values (perhaps even the current ones, or deduce a valid key type from the first added rule, which is exactly what ocelot_match_all_as_mac_etype tries to do now) and don't expect the user to open the datasheet unless some advanced configuration is required. Otherwise I'm not sure who is going to use this. If the user sees a template shell script with the chains already set up, chances are it won't be too hard to add the right actions to the right chains. But if that is going to involve fiddling with templates to set up the right key type, when all they want is a source IPv4 address match, well, no chance. If we agree that templates won't be strictly necessary for basic functionality, we can resubmit what we have already and think more about the best way to expose all key types. I don't honestly know about using a flower filter with 'protocol all' and no other key as a matchall replacement. This is going to be really, really restrictive, and this particular restriction could even be perhaps lifted in the meantime (I don't see a reason why 'matchall' wouldn't be allowed in a chain with a template installed). But Xiaoliang has a point though: there is something which can never be supported: if I want to do QoS based on a list of filters, some of which need a S1_7TUPLE key, and others need a S1_NORMAL_IP6 key, then I can never do that, because in our model, there's only one chain/lookup reserved for QoS classification (a software constraint) but we need 2 chains/lookups for the 2 different key types (a hardware constraint). Yes, this is something hypothetical at this point, but it bothers me that the model would be limiting us. The hardware should support QoS classification in more than 1 IS1 lookup, no? It isn't limited to IS1_L0. Maybe, after all, we should permit dynamic assignment of actions to chains. This way, "the QoS on multiple key types" use case could be supported. What do you think? Allan also mentioned shared blocks. Do we see anything particularly difficult with those, that we should address first? Thanks, -Vladimir
The 07/16/2020 17:45, Vladimir Oltean wrote: > Hi Vladimir, > > On Thu, Jul 16, 2020 at 10:37:40AM +0000, Xiaoliang Yang wrote: > > Hi Joergen, > > > > > > -----Original Message----- > > From: Joergen Andreasen <joergen.andreasen@microchip.com> > > Sent: 2020年7月16日 16:51 > > > > > >> >> Chain 0: The default chain - today this is in IS2. If we proceed > > > >> >> with this as is - then this will change. > > > >> >> Chain 1-9999: These are offloaded by "basic" classification. > > > >> >> Chain 10000-19999: These are offloaded in IS1 > > > >> >> Chain 10000: Lookup-0 in IS1, and here we could limit the > > > >> >> action to do QoS related stuff (priority > > > >> >> update) > > > >> >> Chain 11000: Lookup-1 in IS1, here we could do VLAN > > > >> >> stuff > > > >> >> Chain 12000: Lookup-2 in IS1, here we could apply the > > > >> >> "PAG" which is essentially a GOTO. > > > >> >> > > > >> >> Chain 20000-29999: These are offloaded in IS2 > > > >> >> Chain 20000-20255: Lookup-0 in IS2, where CHAIN-ID - > > > >> >> 20000 is the PAG value. > > > >> >> Chain 21000-21000: Lookup-1 in IS2. > > > >> >> > > > >> >> All these chains should be optional - users should only need to > > > >> >> configure the chains they need. To make this work, we need to > > > >> >> configure both the desired actions (could be priority update) and the goto action. > > > >> >> Remember in HW, all packets goes through this process, while in > > > >> >> SW they only follow the "goto" path. > > > >> >> > > >> > > >> I agree with this chain assignment, following is an example to set rules: > > >> > > >> 1. Set a matchall rule for each chain, the last chain do not need goto chain action. > > >> # tc filter add dev swp0 chain 0 flower skip_sw action goto chain 10000 > > >> # tc filter add dev swp0 chain 10000 flower skip_sw action goto chain 21000 > > >> In driver, use these rules to register the chain. > > >> > > >> 2. Set normal rules. > > >> # tc filter add dev swp0 chain 10000 protocol 802.1Q parent ffff: flower skip_sw vlan_id 1 vlan_prio 1 action skbedit priority 1 action goto chain 21000 > > >> # tc filter add dev swp0 chain 21000 protocol 802.1Q parent ffff: flower skip_sw vlan_id 1 vlan_prio 1 action drop > > >> > > >> In driver, we check if the chain ID has been registered, and goto > > >> chain is the same as first matchall rule, if is not, then return > > >> error. Each rule need has goto action except last chain. > > >> > > >> I also have check about chain template, it can not set an action > > >> template for each chain, so I think it's no use for our case. If > > >> this way to set rules is OK, I will update the patch to do as this. > > >> > > >> Thanks, > > >> Xiaoliang Yang > > > > > > > > I agree that you cannot set an action template for each chain but > > > you can set a match template which for example can be used for > > > setting up which IS1 key to generate for the device/port. > > > The template ensures that you cannot add an illegal match. > > > I have attached a snippet from a testcase I wrote in order to test these ideas. > > > Note that not all actions are valid for the hardware. > > > > > > SMAC = "00:00:00:11:11:11" > > > DMAC = "00:00:00:dd:dd:dd" > > > VID1 = 0x10 > > > VID2 = 0x20 > > > PCP1 = 3 > > > PCP2 = 5 > > > DEI = 1 > > > SIP = "10.10.0.1" > > > DIP = "10.10.0.2" > > > > > > IS1_L0 = 10000 # IS1 lookup 0 > > > IS1_L1 = 11000 # IS1 lookup 1 > > > IS1_L2 = 12000 # IS1 lookup 2 > > > > > > IS2_L0 = 20000 # IS2 lookup 0 # IS2 20000 - 20255 -> pag 0-255 > > > IS2_L0_P1 = 20001 # IS2 lookup 0 pag 1 > > > IS2_L0_P2 = 20002 # IS2 lookup 0 pag 2 > > > > > > IS2_L1 = 21000 # IS2 lookup 1 > > > > > > $skip = "skip_hw" # or "skip_sw" > > > > > > test "Chain templates and goto" do > > > t_i "'prio #' sets the sequence of filters. Lowest number = highest priority = checked first. 0..0xffff" > > > t_i "'handle #' is a reference to the filter. Use this is if you need to reference the filter later. 0..0xffffffff" > > > t_i "'chain #' is the chain to use. Chain 0 is the default. Different chains can have different templates. 0..0xffffffff" > > > $ts.dut.run "tc qdisc add dev #{$dp[0]} clsact" > > > > > > t_i "Add templates" > > > t_i "Configure the VCAP port configuration to match the shortest key that fulfill the purpose" > > > > > t_i "Create a template that sets IS1 lookup 0 to generate S1_NORMAL with S1_DMAC_DIP_ENA" > > > t_i "If you match on both src and dst you will generate S1_7TUPLE" > > > $ts.dut.run "tc chain add dev #{$dp[0]} ingress protocol ip chain #{IS1_L0} flower #{$skip} "\ > > > "dst_mac 00:00:00:00:00:00 "\ > > > "dst_ip 0.0.0.0 " > > > > > > t_i "Create a template that sets IS1 lookup 1 to generate S1_5TUPLE_IP4" > > > $ts.dut.run "tc chain add dev #{$dp[0]} ingress protocol ip chain #{IS1_L1} flower #{$skip} "\ > > > "src_ip 0.0.0.0 "\ > > > "dst_ip 0.0.0.0 " > > > > > > t_i "Create a template that sets IS1 lookup 2 to generate S1_DBL_VID" > > > $ts.dut.run "tc chain add dev #{$dp[0]} ingress protocol 802.1ad chain #{IS1_L2} flower #{$skip} "\ > > > "vlan_id 0 "\ > > > "vlan_prio 0 "\ > > > "vlan_ethtype 802.1q "\ > > > "cvlan_id 0 "\ > > > "cvlan_prio 0 " > > > > > > $ts.dut.run "tc chain show dev #{$dp[0]} ingress" > > > > Why you set different filter keys on different lookup? Each lookup > > only filter one type of keys? > > If I want to filter a same key like dst_mac and do both QoS classified > > action and vlan modify action, how to implement this in the same chain > > #{IS1_L0} ? > > > > I think it's more reasonable to distinguish different lookup by different action like this: > > IS1_L0 = 10000 # IS1 lookup 0 # do QoS classified action > > IS1_L1 = 11000 # IS1 lookup 1 # do vlan modify action > > IS1_L2 = 12000 # IS1 lookup 2 # do goto PAG action > > > > IS2_L0 = 20000 # IS2 lookup 0 # IS2 20000 - 20255 -> pag 0-255 > > IS2_L1 = 21000 # IS2 lookup 1 > > > > So it’s no need to add templates, each lookup can support filtering > > mac, IP or vlan tag, but only support one action. > > > > Thanks, > > Xiaoliang > > As far as I understand, he's still using the static chain numbers > exactly for that, even though he didn't explicitly mention the action > for each individual IS1 lookup. > > The reason why he's also adding templates on each individual chain is to > be able to configure VCAP_S1_KEY_CFG and VCAP_S2_CFG. The configuration > of key type is per lookup. > > Honestly, Joergen, I would take dynamic key configuration per lookup as > a separate item. Xiaoliang's patch series for IS1 support is pretty > large already. > > Right now we have: > > - In mainline: > > S2_IP6_CFG > S2_IP6_CFG controls the key generation for IPv6 frames. Bits 1:0 > control the first lookup and bits 3:2 control the second lookup. > 0: IPv6 frames are matched against IP6_TCP_UDP or IP6_OTHER entries > 1: IPv6 frames are matched against IP6_STD entries > 2: IPv6 frames are matched against IP4_TCP_UDP or IP4_OTHER entries > 3: IPv6 frames are matched against MAC_ETYPE entries > > We set this field to 0xa (0b1010, aka 2 for both lookups: IP4_TCP_UDP). > Although we don't really parse IPv6 keys coming from tc. > > Also there are these fields which we're managing dynamically through > ocelot_match_all_as_mac_etype, depending on whether there is any > MAC_ETYPE key added by the user: > S2_SNAP_DIS > S2_ARP_DIS > S2_IP_TCPUDP_DIS > S2_IP_OTHER_DIS > > - In Xiaoliang's patchset: > > S1_KEY_IP6_CFG > Selects key per lookup in S1 for IPv6 frames. > 0: Use key S1_NORMAL > 1: Use key S1_7TUPLE > 2: Use key S1_5TUPLE_IP4 > 3: Use key S1_NORMAL_IP6 > 4: Use key S1_5TUPLE_IP6 > 5: Use key S1_DBL_VID > > We set this to 2. > > S1_KEY_IP4_CFG > Selects key per lookup in S1 for IPv4 frames. > 0: Use key S1_NORMAL > 1: Use key S1_7TUPLE > 2: Use key S1_5TUPLE_IP4 > 3: Use key S1_DBL_VID > > We set this to 2. > > Your input on which tc chain template could be used for each key type is > valuable, we should create a table with all the options and associated > key sizes (and therefore, number of available filters) and post it > somewhere. I'm not completely sure that chains will be enough to > describe every key type, at least not intuitively, For example if I just > want to match on EtherType (protocol), I'll need an ETYPE (IS1) or > MAC_ETYPE (IS2) rule, but the template for that will need to be > formulated in terms of dst_mac because I don't think there's a way to > use only the protocol in a template. > > But I expect we keep using some default values (perhaps even the current > ones, or deduce a valid key type from the first added rule, which is > exactly what ocelot_match_all_as_mac_etype tries to do now) and don't > expect the user to open the datasheet unless some advanced configuration > is required. Otherwise I'm not sure who is going to use this. If the > user sees a template shell script with the chains already set up, > chances are it won't be too hard to add the right actions to the right > chains. But if that is going to involve fiddling with templates to set > up the right key type, when all they want is a source IPv4 address > match, well, no chance. > > If we agree that templates won't be strictly necessary for basic > functionality, we can resubmit what we have already and think more about > the best way to expose all key types. I don't honestly know about using > a flower filter with 'protocol all' and no other key as a matchall > replacement. This is going to be really, really restrictive, and this > particular restriction could even be perhaps lifted in the meantime (I > don't see a reason why 'matchall' wouldn't be allowed in a chain with a > template installed). > > But Xiaoliang has a point though: there is something which can never be > supported: if I want to do QoS based on a list of filters, some of which > need a S1_7TUPLE key, and others need a S1_NORMAL_IP6 key, then I can > never do that, because in our model, there's only one chain/lookup > reserved for QoS classification (a software constraint) but we need 2 > chains/lookups for the 2 different key types (a hardware constraint). > Yes, this is something hypothetical at this point, but it bothers me > that the model would be limiting us. The hardware should support QoS > classification in more than 1 IS1 lookup, no? It isn't limited to > IS1_L0. Maybe, after all, we should permit dynamic assignment of actions > to chains. This way, "the QoS on multiple key types" use case could be > supported. What do you think? > > Allan also mentioned shared blocks. Do we see anything particularly > difficult with those, that we should address first? > > Thanks, > -Vladimir I agree that dynamic key configuration per lookup should be taken as a separate item. I was just mentioning it because I think it is the only way to derive per port key type generation with the current 'tc' command set. The hardware supports all kind of actions in all lookups but mixing QoS and VLAN actions in same lookup could be hard to understand for the user. Let us say we want to apply a QoS action to <dip,dport> and a VLAN action to <dip>. The most specific rule (the QoS action) must be specified first in the TCAM and if it matches, the lookup terminates and no VLAN action is applied to <dip>. To solve this, the user must assign both QoS and VLAN action in the <dip,dport> rule which is probably not very intuitive. The raseon for using a flower filter with an empty key as the final 'catch all' is that you cannot add a matchall filter in a chain configured with flower. I haven't checked how ineffective this is in the sw path but it works as expected. I don't see any problems with shared blocks. They could be used for setting IGR_PORT_MASK if you want to apply a filter to more than one interface.
On Fri, Jul 17, 2020 at 09:34:11AM +0200, Joergen Andreasen wrote: > The 07/16/2020 17:45, Vladimir Oltean wrote: > > Hi Vladimir, > > > > On Thu, Jul 16, 2020 at 10:37:40AM +0000, Xiaoliang Yang wrote: > > > Hi Joergen, > > > > > > > > > -----Original Message----- > > > From: Joergen Andreasen <joergen.andreasen@microchip.com> > > > Sent: 2020年7月16日 16:51 > > > > > > > >> >> Chain 0: The default chain - today this is in IS2. If we proceed > > > > >> >> with this as is - then this will change. > > > > >> >> Chain 1-9999: These are offloaded by "basic" classification. > > > > >> >> Chain 10000-19999: These are offloaded in IS1 > > > > >> >> Chain 10000: Lookup-0 in IS1, and here we could limit the > > > > >> >> action to do QoS related stuff (priority > > > > >> >> update) > > > > >> >> Chain 11000: Lookup-1 in IS1, here we could do VLAN > > > > >> >> stuff > > > > >> >> Chain 12000: Lookup-2 in IS1, here we could apply the > > > > >> >> "PAG" which is essentially a GOTO. > > > > >> >> > > > > >> >> Chain 20000-29999: These are offloaded in IS2 > > > > >> >> Chain 20000-20255: Lookup-0 in IS2, where CHAIN-ID - > > > > >> >> 20000 is the PAG value. > > > > >> >> Chain 21000-21000: Lookup-1 in IS2. > > > > >> >> > > > > >> >> All these chains should be optional - users should only need to > > > > >> >> configure the chains they need. To make this work, we need to > > > > >> >> configure both the desired actions (could be priority update) and the goto action. > > > > >> >> Remember in HW, all packets goes through this process, while in > > > > >> >> SW they only follow the "goto" path. > > > > >> >> > > > >> > > > >> I agree with this chain assignment, following is an example to set rules: > > > >> > > > >> 1. Set a matchall rule for each chain, the last chain do not need goto chain action. > > > >> # tc filter add dev swp0 chain 0 flower skip_sw action goto chain 10000 > > > >> # tc filter add dev swp0 chain 10000 flower skip_sw action goto chain 21000 > > > >> In driver, use these rules to register the chain. > > > >> > > > >> 2. Set normal rules. > > > >> # tc filter add dev swp0 chain 10000 protocol 802.1Q parent ffff: flower skip_sw vlan_id 1 vlan_prio 1 action skbedit priority 1 action goto chain 21000 > > > >> # tc filter add dev swp0 chain 21000 protocol 802.1Q parent ffff: flower skip_sw vlan_id 1 vlan_prio 1 action drop > > > >> > > > >> In driver, we check if the chain ID has been registered, and goto > > > >> chain is the same as first matchall rule, if is not, then return > > > >> error. Each rule need has goto action except last chain. > > > >> > > > >> I also have check about chain template, it can not set an action > > > >> template for each chain, so I think it's no use for our case. If > > > >> this way to set rules is OK, I will update the patch to do as this. > > > >> > > > >> Thanks, > > > >> Xiaoliang Yang > > > > > > > > > > > I agree that you cannot set an action template for each chain but > > > > you can set a match template which for example can be used for > > > > setting up which IS1 key to generate for the device/port. > > > > The template ensures that you cannot add an illegal match. > > > > I have attached a snippet from a testcase I wrote in order to test these ideas. > > > > Note that not all actions are valid for the hardware. > > > > > > > > SMAC = "00:00:00:11:11:11" > > > > DMAC = "00:00:00:dd:dd:dd" > > > > VID1 = 0x10 > > > > VID2 = 0x20 > > > > PCP1 = 3 > > > > PCP2 = 5 > > > > DEI = 1 > > > > SIP = "10.10.0.1" > > > > DIP = "10.10.0.2" > > > > > > > > IS1_L0 = 10000 # IS1 lookup 0 > > > > IS1_L1 = 11000 # IS1 lookup 1 > > > > IS1_L2 = 12000 # IS1 lookup 2 > > > > > > > > IS2_L0 = 20000 # IS2 lookup 0 # IS2 20000 - 20255 -> pag 0-255 > > > > IS2_L0_P1 = 20001 # IS2 lookup 0 pag 1 > > > > IS2_L0_P2 = 20002 # IS2 lookup 0 pag 2 > > > > > > > > IS2_L1 = 21000 # IS2 lookup 1 > > > > > > > > $skip = "skip_hw" # or "skip_sw" > > > > > > > > test "Chain templates and goto" do > > > > t_i "'prio #' sets the sequence of filters. Lowest number = highest priority = checked first. 0..0xffff" > > > > t_i "'handle #' is a reference to the filter. Use this is if you need to reference the filter later. 0..0xffffffff" > > > > t_i "'chain #' is the chain to use. Chain 0 is the default. Different chains can have different templates. 0..0xffffffff" > > > > $ts.dut.run "tc qdisc add dev #{$dp[0]} clsact" > > > > > > > > t_i "Add templates" > > > > t_i "Configure the VCAP port configuration to match the shortest key that fulfill the purpose" > > > > > > > t_i "Create a template that sets IS1 lookup 0 to generate S1_NORMAL with S1_DMAC_DIP_ENA" > > > > t_i "If you match on both src and dst you will generate S1_7TUPLE" > > > > $ts.dut.run "tc chain add dev #{$dp[0]} ingress protocol ip chain #{IS1_L0} flower #{$skip} "\ > > > > "dst_mac 00:00:00:00:00:00 "\ > > > > "dst_ip 0.0.0.0 " > > > > > > > > t_i "Create a template that sets IS1 lookup 1 to generate S1_5TUPLE_IP4" > > > > $ts.dut.run "tc chain add dev #{$dp[0]} ingress protocol ip chain #{IS1_L1} flower #{$skip} "\ > > > > "src_ip 0.0.0.0 "\ > > > > "dst_ip 0.0.0.0 " > > > > > > > > t_i "Create a template that sets IS1 lookup 2 to generate S1_DBL_VID" > > > > $ts.dut.run "tc chain add dev #{$dp[0]} ingress protocol 802.1ad chain #{IS1_L2} flower #{$skip} "\ > > > > "vlan_id 0 "\ > > > > "vlan_prio 0 "\ > > > > "vlan_ethtype 802.1q "\ > > > > "cvlan_id 0 "\ > > > > "cvlan_prio 0 " > > > > > > > > $ts.dut.run "tc chain show dev #{$dp[0]} ingress" > > > > > > Why you set different filter keys on different lookup? Each lookup > > > only filter one type of keys? > > > If I want to filter a same key like dst_mac and do both QoS classified > > > action and vlan modify action, how to implement this in the same chain > > > #{IS1_L0} ? > > > > > > I think it's more reasonable to distinguish different lookup by different action like this: > > > IS1_L0 = 10000 # IS1 lookup 0 # do QoS classified action > > > IS1_L1 = 11000 # IS1 lookup 1 # do vlan modify action > > > IS1_L2 = 12000 # IS1 lookup 2 # do goto PAG action > > > > > > IS2_L0 = 20000 # IS2 lookup 0 # IS2 20000 - 20255 -> pag 0-255 > > > IS2_L1 = 21000 # IS2 lookup 1 > > > > > > So it’s no need to add templates, each lookup can support filtering > > > mac, IP or vlan tag, but only support one action. > > > > > > Thanks, > > > Xiaoliang > > > > As far as I understand, he's still using the static chain numbers > > exactly for that, even though he didn't explicitly mention the action > > for each individual IS1 lookup. > > > > The reason why he's also adding templates on each individual chain is to > > be able to configure VCAP_S1_KEY_CFG and VCAP_S2_CFG. The configuration > > of key type is per lookup. > > > > Honestly, Joergen, I would take dynamic key configuration per lookup as > > a separate item. Xiaoliang's patch series for IS1 support is pretty > > large already. > > > > Right now we have: > > > > - In mainline: > > > > S2_IP6_CFG > > S2_IP6_CFG controls the key generation for IPv6 frames. Bits 1:0 > > control the first lookup and bits 3:2 control the second lookup. > > 0: IPv6 frames are matched against IP6_TCP_UDP or IP6_OTHER entries > > 1: IPv6 frames are matched against IP6_STD entries > > 2: IPv6 frames are matched against IP4_TCP_UDP or IP4_OTHER entries > > 3: IPv6 frames are matched against MAC_ETYPE entries > > > > We set this field to 0xa (0b1010, aka 2 for both lookups: IP4_TCP_UDP). > > Although we don't really parse IPv6 keys coming from tc. > > > > Also there are these fields which we're managing dynamically through > > ocelot_match_all_as_mac_etype, depending on whether there is any > > MAC_ETYPE key added by the user: > > S2_SNAP_DIS > > S2_ARP_DIS > > S2_IP_TCPUDP_DIS > > S2_IP_OTHER_DIS > > > > - In Xiaoliang's patchset: > > > > S1_KEY_IP6_CFG > > Selects key per lookup in S1 for IPv6 frames. > > 0: Use key S1_NORMAL > > 1: Use key S1_7TUPLE > > 2: Use key S1_5TUPLE_IP4 > > 3: Use key S1_NORMAL_IP6 > > 4: Use key S1_5TUPLE_IP6 > > 5: Use key S1_DBL_VID > > > > We set this to 2. > > > > S1_KEY_IP4_CFG > > Selects key per lookup in S1 for IPv4 frames. > > 0: Use key S1_NORMAL > > 1: Use key S1_7TUPLE > > 2: Use key S1_5TUPLE_IP4 > > 3: Use key S1_DBL_VID > > > > We set this to 2. > > > > Your input on which tc chain template could be used for each key type is > > valuable, we should create a table with all the options and associated > > key sizes (and therefore, number of available filters) and post it > > somewhere. I'm not completely sure that chains will be enough to > > describe every key type, at least not intuitively, For example if I just > > want to match on EtherType (protocol), I'll need an ETYPE (IS1) or > > MAC_ETYPE (IS2) rule, but the template for that will need to be > > formulated in terms of dst_mac because I don't think there's a way to > > use only the protocol in a template. > > > > But I expect we keep using some default values (perhaps even the current > > ones, or deduce a valid key type from the first added rule, which is > > exactly what ocelot_match_all_as_mac_etype tries to do now) and don't > > expect the user to open the datasheet unless some advanced configuration > > is required. Otherwise I'm not sure who is going to use this. If the > > user sees a template shell script with the chains already set up, > > chances are it won't be too hard to add the right actions to the right > > chains. But if that is going to involve fiddling with templates to set > > up the right key type, when all they want is a source IPv4 address > > match, well, no chance. > > > > If we agree that templates won't be strictly necessary for basic > > functionality, we can resubmit what we have already and think more about > > the best way to expose all key types. I don't honestly know about using > > a flower filter with 'protocol all' and no other key as a matchall > > replacement. This is going to be really, really restrictive, and this > > particular restriction could even be perhaps lifted in the meantime (I > > don't see a reason why 'matchall' wouldn't be allowed in a chain with a > > template installed). > > > > But Xiaoliang has a point though: there is something which can never be > > supported: if I want to do QoS based on a list of filters, some of which > > need a S1_7TUPLE key, and others need a S1_NORMAL_IP6 key, then I can > > never do that, because in our model, there's only one chain/lookup > > reserved for QoS classification (a software constraint) but we need 2 > > chains/lookups for the 2 different key types (a hardware constraint). > > Yes, this is something hypothetical at this point, but it bothers me > > that the model would be limiting us. The hardware should support QoS > > classification in more than 1 IS1 lookup, no? It isn't limited to > > IS1_L0. Maybe, after all, we should permit dynamic assignment of actions > > to chains. This way, "the QoS on multiple key types" use case could be > > supported. What do you think? > > > > Allan also mentioned shared blocks. Do we see anything particularly > > difficult with those, that we should address first? > > > > Thanks, > > -Vladimir > > I agree that dynamic key configuration per lookup should be taken as a separate item. > I was just mentioning it because I think it is the only way to derive per port > key type generation with the current 'tc' command set. > > The hardware supports all kind of actions in all lookups but mixing QoS and VLAN > actions in same lookup could be hard to understand for the user. > > Let us say we want to apply a QoS action to <dip,dport> and a VLAN action to <dip>. > The most specific rule (the QoS action) must be specified first in the TCAM and Why must the rule with the most specific key be added first to the TCAM? Is this in reply to my comment about deriving an appropriate key type from the first filter introduced in each lookup? > if it matches, the lookup terminates and no VLAN action is applied to <dip>. > To solve this, the user must assign both QoS and VLAN action in the <dip,dport> > rule which is probably not very intuitive. > If so, can't we simply alter the priority of the filters, such that the order in which they match on packets is not the same order as which they were introduced by the user? Supporting multiple actions per rule could be interesting, in the sense that the hardware supports this feature and it would be therefore nice if it could be exposed. I've been re-reading the discussion with you and Allan, and to be honest, I don't completely internalize why we are doing the 1-action-per-chain now. Sorry. It has been said that the TCAM stops on first match in each lookup, and I understand that. So we must have, in each filter's list of actions, not only the action we want (vlan, skbedit priority, trap, drop, police), but also an explicit 'goto' action to the next lookup or TCAM. But if 'stealing' frames is the problem, then it's not just a QoS action that can steal frames from a VLAN action. But instead, a QoS action can steal frames even from another QoS action. I mean, in your example above, with a more generic <dip> key and another more specific <dip,dport> key, there's nothing about the action itself that is causing this 'stealing' to occur. So, I don't really see why we are forcing a unique action per chain/lookup, and how that, in itself, helps with anything. With my (certainly not as deep as yours) understanding, just chaining a goto after each action should be enough to keep the software and the hardware behavior identical. I would really appreciate if you could clarify my misunderstanding. > The raseon for using a flower filter with an empty key as the final 'catch all' > is that you cannot add a matchall filter in a chain configured with flower. > I haven't checked how ineffective this is in the sw path but it works as expected. > I realize now I didn't explicitly mention why the 'flower' instead of 'matchall' concerns me if we use templates, and that is because one of the use cases we are interested in (which we are not covering in this patch series) is masked matching on the raw first 4 bytes after EtherType. This could be done with an u32 filter, and in hardware it would use the L3_IP4_SIP field of an S1_NORMAL half key: L3_IP4_SIP Overloaded fields for different frame types: LLC frame: L3_IP4_SIP = [CTRL, PAYLOAD[0:2]] SNAP frame: L3_IP4_SIP = [PID[2:0], PAYLOAD[0]] IPv4 or IPv6 frame: L3_IP4_SIP = source IP address, bits [31:0] OAM, ARP or ETYPE frame: L3_IP4_SIP = PAYLOAD[0:3] <- we have ETYPE frame For IPv4 or IPv6 frames, use destination IP address if VCAP_CFG.S1_DMAC_DIP_ENA is set for ingress port. But if tc chain templates require us to not mix different types of filters (such as matchall and flower, u32 and flower), I think it could become very challenging. > I don't see any problems with shared blocks. They could be used for setting > IGR_PORT_MASK if you want to apply a filter to more than one interface. > Ok, that's what I figured too. > -- > Joergen Andreasen, Microchip Thanks, -Vladimir
On 17.07.2020 12:08, Vladimir Oltean wrote: >EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > >On Fri, Jul 17, 2020 at 09:34:11AM +0200, Joergen Andreasen wrote: >> The 07/16/2020 17:45, Vladimir Oltean wrote: >> > Hi Vladimir, >> > >> > On Thu, Jul 16, 2020 at 10:37:40AM +0000, Xiaoliang Yang wrote: >> > > Hi Joergen, >> > > >> > > >> > > -----Original Message----- >> > > From: Joergen Andreasen <joergen.andreasen@microchip.com> >> > > Sent: 2020年7月16日 16:51 >> > > >> > > > >> >> Chain 0: The default chain - today this is in IS2. If we proceed >> > > > >> >> with this as is - then this will change. >> > > > >> >> Chain 1-9999: These are offloaded by "basic" classification. >> > > > >> >> Chain 10000-19999: These are offloaded in IS1 >> > > > >> >> Chain 10000: Lookup-0 in IS1, and here we could limit the >> > > > >> >> action to do QoS related stuff (priority >> > > > >> >> update) >> > > > >> >> Chain 11000: Lookup-1 in IS1, here we could do VLAN >> > > > >> >> stuff >> > > > >> >> Chain 12000: Lookup-2 in IS1, here we could apply the >> > > > >> >> "PAG" which is essentially a GOTO. >> > > > >> >> >> > > > >> >> Chain 20000-29999: These are offloaded in IS2 >> > > > >> >> Chain 20000-20255: Lookup-0 in IS2, where CHAIN-ID - >> > > > >> >> 20000 is the PAG value. >> > > > >> >> Chain 21000-21000: Lookup-1 in IS2. >> > > > >> >> >> > > > >> >> All these chains should be optional - users should only need to >> > > > >> >> configure the chains they need. To make this work, we need to >> > > > >> >> configure both the desired actions (could be priority update) and the goto action. >> > > > >> >> Remember in HW, all packets goes through this process, while in >> > > > >> >> SW they only follow the "goto" path. >> > > > >> >> >> > > >> >> > > >> I agree with this chain assignment, following is an example to set rules: >> > > >> >> > > >> 1. Set a matchall rule for each chain, the last chain do not need goto chain action. >> > > >> # tc filter add dev swp0 chain 0 flower skip_sw action goto chain 10000 >> > > >> # tc filter add dev swp0 chain 10000 flower skip_sw action goto chain 21000 >> > > >> In driver, use these rules to register the chain. >> > > >> >> > > >> 2. Set normal rules. >> > > >> # tc filter add dev swp0 chain 10000 protocol 802.1Q parent ffff: flower skip_sw vlan_id 1 vlan_prio 1 action skbedit priority 1 action goto chain 21000 >> > > >> # tc filter add dev swp0 chain 21000 protocol 802.1Q parent ffff: flower skip_sw vlan_id 1 vlan_prio 1 action drop >> > > >> >> > > >> In driver, we check if the chain ID has been registered, and goto >> > > >> chain is the same as first matchall rule, if is not, then return >> > > >> error. Each rule need has goto action except last chain. >> > > >> >> > > >> I also have check about chain template, it can not set an action >> > > >> template for each chain, so I think it's no use for our case. If >> > > >> this way to set rules is OK, I will update the patch to do as this. >> > > >> >> > > >> Thanks, >> > > >> Xiaoliang Yang >> > > > >> > > >> > > > I agree that you cannot set an action template for each chain but >> > > > you can set a match template which for example can be used for >> > > > setting up which IS1 key to generate for the device/port. >> > > > The template ensures that you cannot add an illegal match. >> > > > I have attached a snippet from a testcase I wrote in order to test these ideas. >> > > > Note that not all actions are valid for the hardware. >> > > > >> > > > SMAC = "00:00:00:11:11:11" >> > > > DMAC = "00:00:00:dd:dd:dd" >> > > > VID1 = 0x10 >> > > > VID2 = 0x20 >> > > > PCP1 = 3 >> > > > PCP2 = 5 >> > > > DEI = 1 >> > > > SIP = "10.10.0.1" >> > > > DIP = "10.10.0.2" >> > > > >> > > > IS1_L0 = 10000 # IS1 lookup 0 >> > > > IS1_L1 = 11000 # IS1 lookup 1 >> > > > IS1_L2 = 12000 # IS1 lookup 2 >> > > > >> > > > IS2_L0 = 20000 # IS2 lookup 0 # IS2 20000 - 20255 -> pag 0-255 >> > > > IS2_L0_P1 = 20001 # IS2 lookup 0 pag 1 >> > > > IS2_L0_P2 = 20002 # IS2 lookup 0 pag 2 >> > > > >> > > > IS2_L1 = 21000 # IS2 lookup 1 >> > > > >> > > > $skip = "skip_hw" # or "skip_sw" >> > > > >> > > > test "Chain templates and goto" do >> > > > t_i "'prio #' sets the sequence of filters. Lowest number = highest priority = checked first. 0..0xffff" >> > > > t_i "'handle #' is a reference to the filter. Use this is if you need to reference the filter later. 0..0xffffffff" >> > > > t_i "'chain #' is the chain to use. Chain 0 is the default. Different chains can have different templates. 0..0xffffffff" >> > > > $ts.dut.run "tc qdisc add dev #{$dp[0]} clsact" >> > > > >> > > > t_i "Add templates" >> > > > t_i "Configure the VCAP port configuration to match the shortest key that fulfill the purpose" >> > > >> > > > t_i "Create a template that sets IS1 lookup 0 to generate S1_NORMAL with S1_DMAC_DIP_ENA" >> > > > t_i "If you match on both src and dst you will generate S1_7TUPLE" >> > > > $ts.dut.run "tc chain add dev #{$dp[0]} ingress protocol ip chain #{IS1_L0} flower #{$skip} "\ >> > > > "dst_mac 00:00:00:00:00:00 "\ >> > > > "dst_ip 0.0.0.0 " >> > > > >> > > > t_i "Create a template that sets IS1 lookup 1 to generate S1_5TUPLE_IP4" >> > > > $ts.dut.run "tc chain add dev #{$dp[0]} ingress protocol ip chain #{IS1_L1} flower #{$skip} "\ >> > > > "src_ip 0.0.0.0 "\ >> > > > "dst_ip 0.0.0.0 " >> > > > >> > > > t_i "Create a template that sets IS1 lookup 2 to generate S1_DBL_VID" >> > > > $ts.dut.run "tc chain add dev #{$dp[0]} ingress protocol 802.1ad chain #{IS1_L2} flower #{$skip} "\ >> > > > "vlan_id 0 "\ >> > > > "vlan_prio 0 "\ >> > > > "vlan_ethtype 802.1q "\ >> > > > "cvlan_id 0 "\ >> > > > "cvlan_prio 0 " >> > > > >> > > > $ts.dut.run "tc chain show dev #{$dp[0]} ingress" >> > > >> > > Why you set different filter keys on different lookup? Each lookup >> > > only filter one type of keys? >> > > If I want to filter a same key like dst_mac and do both QoS classified >> > > action and vlan modify action, how to implement this in the same chain >> > > #{IS1_L0} ? >> > > >> > > I think it's more reasonable to distinguish different lookup by different action like this: >> > > IS1_L0 = 10000 # IS1 lookup 0 # do QoS classified action >> > > IS1_L1 = 11000 # IS1 lookup 1 # do vlan modify action >> > > IS1_L2 = 12000 # IS1 lookup 2 # do goto PAG action >> > > >> > > IS2_L0 = 20000 # IS2 lookup 0 # IS2 20000 - 20255 -> pag 0-255 >> > > IS2_L1 = 21000 # IS2 lookup 1 >> > > >> > > So it’s no need to add templates, each lookup can support filtering >> > > mac, IP or vlan tag, but only support one action. >> > > >> > > Thanks, >> > > Xiaoliang >> > >> > As far as I understand, he's still using the static chain numbers >> > exactly for that, even though he didn't explicitly mention the action >> > for each individual IS1 lookup. >> > >> > The reason why he's also adding templates on each individual chain is to >> > be able to configure VCAP_S1_KEY_CFG and VCAP_S2_CFG. The configuration >> > of key type is per lookup. >> > >> > Honestly, Joergen, I would take dynamic key configuration per lookup as >> > a separate item. Xiaoliang's patch series for IS1 support is pretty >> > large already. I agree, it is already really long, and I would also like if we can focus on as small incremental steps as possible. Maybe it would be an idea to do the ES0 independently. As there are only 1 ES0 look-up, and only 1 egress tcam this seems a lot more straight forward. I can not remember the details of that patchs, but I remember it as largely okay. >> > Right now we have: >> > - In mainline: >> > S2_IP6_CFG >> > S2_IP6_CFG controls the key generation for IPv6 frames. Bits 1:0 >> > control the first lookup and bits 3:2 control the second lookup. >> > 0: IPv6 frames are matched against IP6_TCP_UDP or IP6_OTHER entries >> > 1: IPv6 frames are matched against IP6_STD entries >> > 2: IPv6 frames are matched against IP4_TCP_UDP or IP4_OTHER entries >> > 3: IPv6 frames are matched against MAC_ETYPE entries >> > >> > We set this field to 0xa (0b1010, aka 2 for both lookups: IP4_TCP_UDP). >> > Although we don't really parse IPv6 keys coming from tc. >> > >> > Also there are these fields which we're managing dynamically through >> > ocelot_match_all_as_mac_etype, depending on whether there is any >> > MAC_ETYPE key added by the user: >> > S2_SNAP_DIS >> > S2_ARP_DIS >> > S2_IP_TCPUDP_DIS >> > S2_IP_OTHER_DIS As far as I recall with what we have in main-line today, we only use 1 lookup in IS2. You added some functionallity to dynamic select the KEY depending on what rules was installed, and some better error handling to reject rules that could not be applied correctly. >> > - In Xiaoliang's patchset: >> > >> > S1_KEY_IP6_CFG >> > Selects key per lookup in S1 for IPv6 frames. >> > 0: Use key S1_NORMAL >> > 1: Use key S1_7TUPLE >> > 2: Use key S1_5TUPLE_IP4 >> > 3: Use key S1_NORMAL_IP6 >> > 4: Use key S1_5TUPLE_IP6 >> > 5: Use key S1_DBL_VID >> > >> > We set this to 2. >> > >> > S1_KEY_IP4_CFG >> > Selects key per lookup in S1 for IPv4 frames. >> > 0: Use key S1_NORMAL >> > 1: Use key S1_7TUPLE >> > 2: Use key S1_5TUPLE_IP4 >> > 3: Use key S1_DBL_VID >> > >> > We set this to 2. Any reason you prefer S1_5TUPLE_IP4 over S1_7TUPLE? >> > Your input on which tc chain template could be used for each key type is >> > valuable, we should create a table with all the options and associated >> > key sizes (and therefore, number of available filters) and post it >> > somewhere. I'm not completely sure that chains will be enough to >> > describe every key type, at least not intuitively, For example if I just >> > want to match on EtherType (protocol), I'll need an ETYPE (IS1) or >> > MAC_ETYPE (IS2) rule, but the template for that will need to be >> > formulated in terms of dst_mac because I don't think there's a way to >> > use only the protocol in a template. Agree. >> > But I expect we keep using some default values (perhaps even the current >> > ones, or deduce a valid key type from the first added rule, which is >> > exactly what ocelot_match_all_as_mac_etype tries to do now) and don't >> > expect the user to open the datasheet unless some advanced configuration >> > is required. Otherwise I'm not sure who is going to use this. If the >> > user sees a template shell script with the chains already set up, >> > chances are it won't be too hard to add the right actions to the right >> > chains. But if that is going to involve fiddling with templates to set >> > up the right key type, when all they want is a source IPv4 address >> > match, well, no chance. My preference would be to pick the "largest" key by default. This will mean that we will be wasting some TCAM resources - but we have more features. We can then at a later point add support for templates, and with the templates we can choose the smallest key which fullfill the requirement stated by the template. >> > If we agree that templates won't be strictly necessary for basic >> > functionality, we can resubmit what we have already and think more about >> > the best way to expose all key types. I don't honestly know about using >> > a flower filter with 'protocol all' and no other key as a matchall >> > replacement. This is going to be really, really restrictive, and this >> > particular restriction could even be perhaps lifted in the meantime (I >> > don't see a reason why 'matchall' wouldn't be allowed in a chain with a >> > template installed). >> > >> > But Xiaoliang has a point though: there is something which can never be >> > supported: if I want to do QoS based on a list of filters, some of which >> > need a S1_7TUPLE key, and others need a S1_NORMAL_IP6 key, then I can >> > never do that, because in our model, there's only one chain/lookup >> > reserved for QoS classification (a software constraint) but we need 2 >> > chains/lookups for the 2 different key types (a hardware constraint). >> > Yes, this is something hypothetical at this point, but it bothers me >> > that the model would be limiting us. The hardware should support QoS >> > classification in more than 1 IS1 lookup, no? It isn't limited to >> > IS1_L0. Maybe, after all, we should permit dynamic assignment of actions >> > to chains. This way, "the QoS on multiple key types" use case could be >> > supported. What do you think? >> > >> > Allan also mentioned shared blocks. Do we see anything particularly >> > difficult with those, that we should address first? >> > >> > Thanks, >> > -Vladimir >> >> I agree that dynamic key configuration per lookup should be taken as a separate item. >> I was just mentioning it because I think it is the only way to derive per port >> key type generation with the current 'tc' command set. >> >> The hardware supports all kind of actions in all lookups but mixing QoS and VLAN >> actions in same lookup could be hard to understand for the user. >> >> Let us say we want to apply a QoS action to <dip,dport> and a VLAN action to <dip>. >> The most specific rule (the QoS action) must be specified first in the TCAM and > >Why must the rule with the most specific key be added first to the TCAM? >Is this in reply to my comment about deriving an appropriate key type >from the first filter introduced in each lookup? > >> if it matches, the lookup terminates and no VLAN action is applied to <dip>. >> To solve this, the user must assign both QoS and VLAN action in the <dip,dport> >> rule which is probably not very intuitive. >> > >If so, can't we simply alter the priority of the filters, such that the >order in which they match on packets is not the same order as which they >were introduced by the user? > >Supporting multiple actions per rule could be interesting, in the sense >that the hardware supports this feature and it would be therefore nice >if it could be exposed. > >I've been re-reading the discussion with you and Allan, and to be >honest, I don't completely internalize why we are doing the >1-action-per-chain now. Sorry. It has been said that the TCAM stops on >first match in each lookup, and I understand that. So we must have, >in each filter's list of actions, not only the action we want (vlan, >skbedit priority, trap, drop, police), but also an explicit 'goto' >action to the next lookup or TCAM. But if 'stealing' frames is the >problem, then it's not just a QoS action that can steal frames from a >VLAN action. But instead, a QoS action can steal frames even from >another QoS action. I mean, in your example above, with a more generic ><dip> key and another more specific <dip,dport> key, there's nothing >about the action itself that is causing this 'stealing' to occur. So, I >don't really see why we are forcing a unique action per chain/lookup, >and how that, in itself, helps with anything. With my (certainly not as >deep as yours) understanding, just chaining a goto after each action >should be enough to keep the software and the hardware behavior >identical. I would really appreciate if you could clarify my >misunderstanding. The reason why I that we should make a given action exclusive owned by a given lookup, is that then it is trivial to guarantee that the 3 look-ups which in HW is done in parallel will give the same result as in SW where they are done in sequence. But if we can do something less restrictive, then I'm open for it (we might be able to do that). >> The raseon for using a flower filter with an empty key as the final 'catch all' >> is that you cannot add a matchall filter in a chain configured with flower. >> I haven't checked how ineffective this is in the sw path but it works as expected. >> > >I realize now I didn't explicitly mention why the 'flower' instead of >'matchall' concerns me if we use templates, and that is because one of >the use cases we are interested in (which we are not covering in this >patch series) is masked matching on the raw first 4 bytes after >EtherType. This could be done with an u32 filter, and in hardware it >would use the L3_IP4_SIP field of an S1_NORMAL half key: > >L3_IP4_SIP >Overloaded fields for different frame types: >LLC frame: L3_IP4_SIP = [CTRL, PAYLOAD[0:2]] >SNAP frame: L3_IP4_SIP = [PID[2:0], PAYLOAD[0]] >IPv4 or IPv6 frame: L3_IP4_SIP = source IP address, bits [31:0] >OAM, ARP or ETYPE frame: L3_IP4_SIP = PAYLOAD[0:3] <- we have ETYPE frame >For IPv4 or IPv6 frames, use destination IP address if >VCAP_CFG.S1_DMAC_DIP_ENA is set for ingress port. > >But if tc chain templates require us to not mix different types of >filters (such as matchall and flower, u32 and flower), I think it could >become very challenging. Okay - I will need to look deeper into to this to really understand the consequences of mixing different types of filters. As far as Joergens example goes, "matchall" is really the same as a flower without any matches. Long story short, to me the most important step here is that we come up with a design where we can expose the 3 lookups in IS1 as separate chains, and that we have something which behaves the same in HW and SW. Once we have that, we can add templates, shared blocks, shared actions etc. in the future. I know I have not been very active on this thread for the past couple of days, but I'm certainly interesting in continue working/reviewing this. I will be OOO for the next 3 weeks, with very limited options for reviewing/commenting on this, but after that I'm back again. /Allan
18.07.2020 3:10, Allan wrote: > > Okay - I will need to look deeper into to this to really understand the consequences of mixing different types of filters. As far as Joergens example goes, "matchall" is really the same as a flower without any matches. > > Long story short, to me the most important step here is that we come up with a design where we can expose the 3 lookups in IS1 as separate chains, and that we have something which behaves the same in HW and SW. > > Once we have that, we can add templates, shared blocks, shared actions etc. in the future. > > I know I have not been very active on this thread for the past couple of days, but I'm certainly interesting in continue working/reviewing this. > I will be OOO for the next 3 weeks, with very limited options for reviewing/commenting on this, but after that I'm back again. > >/Allan > So chain template is used to configure key type on IS1, we can set one key type for each of the three lookups. In order to support all key types, we need to add half keys, full keys and quard keys support. If there is no template set, using a default "S1_7TUPLE" key type, which can cover most keys. In general, using a default key type for each of the three lookups, and limited one action on one lookup chain, these can support three parallel lookup on IS1. Add PAG support as two lookups on IS2, then templates and shared blocks can be supported after that. Thanks, Xiaoliang
diff --git a/drivers/net/ethernet/mscc/ocelot_ace.c b/drivers/net/ethernet/mscc/ocelot_ace.c index 748c618db7d8..b76593b40097 100644 --- a/drivers/net/ethernet/mscc/ocelot_ace.c +++ b/drivers/net/ethernet/mscc/ocelot_ace.c @@ -341,6 +341,8 @@ static void is2_action_set(struct ocelot *ocelot, struct vcap_data *data, vcap_action_set(vcap, data, VCAP_IS2_ACT_CPU_QU_NUM, 0); vcap_action_set(vcap, data, VCAP_IS2_ACT_CPU_COPY_ENA, 0); break; + default: + break; } } @@ -644,9 +646,9 @@ static void is2_entry_set(struct ocelot *ocelot, int ix, } static void vcap_entry_get(struct ocelot *ocelot, struct ocelot_ace_rule *rule, - int ix) + int ix, int block_id) { - const struct vcap_props *vcap = &ocelot->vcap[VCAP_IS2]; + const struct vcap_props *vcap = &ocelot->vcap[block_id]; struct vcap_data data; int row, count; u32 cnt; @@ -663,6 +665,19 @@ static void vcap_entry_get(struct ocelot *ocelot, struct ocelot_ace_rule *rule, rule->stats.pkts = cnt; } +static void vcap_entry_set(struct ocelot *ocelot, int ix, + struct ocelot_ace_rule *ace, + int block_id) +{ + switch (block_id) { + case VCAP_IS2: + is2_entry_set(ocelot, ix, ace); + break; + default: + break; + } +} + static void ocelot_ace_rule_add(struct ocelot *ocelot, struct ocelot_acl_block *block, struct ocelot_ace_rule *rule) @@ -790,7 +805,7 @@ static bool ocelot_ace_is_problematic_non_mac_etype(struct ocelot_ace_rule *ace) static bool ocelot_exclusive_mac_etype_ace_rules(struct ocelot *ocelot, struct ocelot_ace_rule *ace) { - struct ocelot_acl_block *block = &ocelot->acl_block; + struct ocelot_acl_block *block = &ocelot->acl_block[VCAP_IS2]; struct ocelot_ace_rule *tmp; unsigned long port; int i; @@ -824,15 +839,16 @@ static bool ocelot_exclusive_mac_etype_ace_rules(struct ocelot *ocelot, return true; } -int ocelot_ace_rule_offload_add(struct ocelot *ocelot, +int ocelot_ace_rule_offload_add(struct ocelot *ocelot, int block_id, struct ocelot_ace_rule *rule, struct netlink_ext_ack *extack) { - struct ocelot_acl_block *block = &ocelot->acl_block; + struct ocelot_acl_block *block = &ocelot->acl_block[block_id]; struct ocelot_ace_rule *ace; int i, index; - if (!ocelot_exclusive_mac_etype_ace_rules(ocelot, rule)) { + if (block_id == VCAP_IS2 && + !ocelot_exclusive_mac_etype_ace_rules(ocelot, rule)) { NL_SET_ERR_MSG_MOD(extack, "Cannot mix MAC_ETYPE with non-MAC_ETYPE rules"); return -EBUSY; @@ -847,11 +863,11 @@ int ocelot_ace_rule_offload_add(struct ocelot *ocelot, /* Move down the rules to make place for the new rule */ for (i = block->count - 1; i > index; i--) { ace = ocelot_ace_rule_get_rule_index(block, i); - is2_entry_set(ocelot, i, ace); + vcap_entry_set(ocelot, i, ace, block_id); } /* Now insert the new rule */ - is2_entry_set(ocelot, index, rule); + vcap_entry_set(ocelot, index, rule, block_id); return 0; } @@ -902,10 +918,10 @@ static void ocelot_ace_rule_del(struct ocelot *ocelot, block->count--; } -int ocelot_ace_rule_offload_del(struct ocelot *ocelot, +int ocelot_ace_rule_offload_del(struct ocelot *ocelot, int block_id, struct ocelot_ace_rule *rule) { - struct ocelot_acl_block *block = &ocelot->acl_block; + struct ocelot_acl_block *block = &ocelot->acl_block[block_id]; struct ocelot_ace_rule del_ace; struct ocelot_ace_rule *ace; int i, index; @@ -921,29 +937,29 @@ int ocelot_ace_rule_offload_del(struct ocelot *ocelot, /* Move up all the blocks over the deleted rule */ for (i = index; i < block->count; i++) { ace = ocelot_ace_rule_get_rule_index(block, i); - is2_entry_set(ocelot, i, ace); + vcap_entry_set(ocelot, i, ace, block_id); } /* Now delete the last rule, because it is duplicated */ - is2_entry_set(ocelot, block->count, &del_ace); + vcap_entry_set(ocelot, block->count, &del_ace, block_id); return 0; } -int ocelot_ace_rule_stats_update(struct ocelot *ocelot, +int ocelot_ace_rule_stats_update(struct ocelot *ocelot, int block_id, struct ocelot_ace_rule *rule) { - struct ocelot_acl_block *block = &ocelot->acl_block; + struct ocelot_acl_block *block = &ocelot->acl_block[block_id]; struct ocelot_ace_rule *tmp; int index; index = ocelot_ace_rule_get_index_id(block, rule); - vcap_entry_get(ocelot, rule, index); + vcap_entry_get(ocelot, rule, index, block_id); /* After we get the result we need to clear the counters */ tmp = ocelot_ace_rule_get_rule_index(block, index); tmp->stats.pkts = 0; - is2_entry_set(ocelot, index, tmp); + vcap_entry_set(ocelot, index, tmp, block_id); return 0; } @@ -968,7 +984,7 @@ static void vcap_init(struct ocelot *ocelot, const struct vcap_props *vcap) int ocelot_ace_init(struct ocelot *ocelot) { - struct ocelot_acl_block *block = &ocelot->acl_block; + struct ocelot_acl_block *block; vcap_init(ocelot, &ocelot->vcap[VCAP_IS2]); @@ -987,6 +1003,7 @@ int ocelot_ace_init(struct ocelot *ocelot) ocelot_write_gix(ocelot, 0x3fffff, ANA_POL_CIR_STATE, OCELOT_POLICER_DISCARD); + block = &ocelot->acl_block[VCAP_IS2]; block->pol_lpr = OCELOT_POLICER_DISCARD - 1; INIT_LIST_HEAD(&block->rules); diff --git a/drivers/net/ethernet/mscc/ocelot_ace.h b/drivers/net/ethernet/mscc/ocelot_ace.h index 099e177f2617..a9fd99401a65 100644 --- a/drivers/net/ethernet/mscc/ocelot_ace.h +++ b/drivers/net/ethernet/mscc/ocelot_ace.h @@ -175,6 +175,7 @@ struct ocelot_ace_frame_ipv6 { }; enum ocelot_ace_action { + OCELOT_ACL_ACTION_NULL, OCELOT_ACL_ACTION_DROP, OCELOT_ACL_ACTION_TRAP, OCELOT_ACL_ACTION_POLICE, @@ -214,12 +215,12 @@ struct ocelot_ace_rule { u32 pol_ix; }; -int ocelot_ace_rule_offload_add(struct ocelot *ocelot, +int ocelot_ace_rule_offload_add(struct ocelot *ocelot, int block_id, struct ocelot_ace_rule *rule, struct netlink_ext_ack *extack); -int ocelot_ace_rule_offload_del(struct ocelot *ocelot, +int ocelot_ace_rule_offload_del(struct ocelot *ocelot, int block_id, struct ocelot_ace_rule *rule); -int ocelot_ace_rule_stats_update(struct ocelot *ocelot, +int ocelot_ace_rule_stats_update(struct ocelot *ocelot, int block_id, struct ocelot_ace_rule *rule); int ocelot_ace_init(struct ocelot *ocelot); diff --git a/drivers/net/ethernet/mscc/ocelot_flower.c b/drivers/net/ethernet/mscc/ocelot_flower.c index 891925f73cbc..a1f7b6b28170 100644 --- a/drivers/net/ethernet/mscc/ocelot_flower.c +++ b/drivers/net/ethernet/mscc/ocelot_flower.c @@ -9,13 +9,26 @@ #include "ocelot_ace.h" +static int ocelot_block_id_get(int chain, bool ingress) +{ + /* Select TCAM blocks by using chain index. Rules in chain 0 are + * implemented on IS1, chain 1 are implemented on IS2, and egress + * chain corresponds to ES0 block. + */ + if (ingress) + return chain ? VCAP_IS2 : VCAP_IS1; + else + return VCAP_ES0; +} + static int ocelot_flower_parse_action(struct flow_cls_offload *f, struct ocelot_ace_rule *ace) { + struct netlink_ext_ack *extack = f->common.extack; const struct flow_action_entry *a; + int i, allowed_chain = 0; s64 burst; u64 rate; - int i; if (!flow_offload_has_one_action(&f->rule->action)) return -EOPNOTSUPP; @@ -28,9 +41,11 @@ static int ocelot_flower_parse_action(struct flow_cls_offload *f, switch (a->id) { case FLOW_ACTION_DROP: ace->action = OCELOT_ACL_ACTION_DROP; + allowed_chain = 1; break; case FLOW_ACTION_TRAP: ace->action = OCELOT_ACL_ACTION_TRAP; + allowed_chain = 1; break; case FLOW_ACTION_POLICE: ace->action = OCELOT_ACL_ACTION_POLICE; @@ -38,10 +53,23 @@ static int ocelot_flower_parse_action(struct flow_cls_offload *f, ace->pol.rate = div_u64(rate, 1000) * 8; burst = rate * PSCHED_NS2TICKS(a->police.burst); ace->pol.burst = div_u64(burst, PSCHED_TICKS_PER_SEC); + allowed_chain = 1; + break; + case FLOW_ACTION_GOTO: + if (a->chain_index != f->common.chain_index + 1) { + NL_SET_ERR_MSG_MOD(extack, "HW only support goto next chain\n"); + return -EOPNOTSUPP; + } + ace->action = OCELOT_ACL_ACTION_NULL; + allowed_chain = f->common.chain_index; break; default: return -EOPNOTSUPP; } + if (f->common.chain_index != allowed_chain) { + NL_SET_ERR_MSG_MOD(extack, "Action is not supported on this chain\n"); + return -EOPNOTSUPP; + } } return 0; @@ -205,7 +233,7 @@ int ocelot_cls_flower_replace(struct ocelot *ocelot, int port, struct flow_cls_offload *f, bool ingress) { struct ocelot_ace_rule *ace; - int ret; + int ret, block_id; ace = ocelot_ace_rule_create(ocelot, port, f); if (!ace) @@ -216,8 +244,10 @@ int ocelot_cls_flower_replace(struct ocelot *ocelot, int port, kfree(ace); return ret; } + block_id = ocelot_block_id_get(f->common.chain_index, ingress); - return ocelot_ace_rule_offload_add(ocelot, ace, f->common.extack); + return ocelot_ace_rule_offload_add(ocelot, block_id, ace, + f->common.extack); } EXPORT_SYMBOL_GPL(ocelot_cls_flower_replace); @@ -225,11 +255,13 @@ int ocelot_cls_flower_destroy(struct ocelot *ocelot, int port, struct flow_cls_offload *f, bool ingress) { struct ocelot_ace_rule ace; + int block_id; ace.prio = f->common.prio; ace.id = f->cookie; + block_id = ocelot_block_id_get(f->common.chain_index, ingress); - return ocelot_ace_rule_offload_del(ocelot, &ace); + return ocelot_ace_rule_offload_del(ocelot, block_id, &ace); } EXPORT_SYMBOL_GPL(ocelot_cls_flower_destroy); @@ -237,11 +269,13 @@ int ocelot_cls_flower_stats(struct ocelot *ocelot, int port, struct flow_cls_offload *f, bool ingress) { struct ocelot_ace_rule ace; - int ret; + int ret, block_id; ace.prio = f->common.prio; ace.id = f->cookie; - ret = ocelot_ace_rule_stats_update(ocelot, &ace); + block_id = ocelot_block_id_get(f->common.chain_index, ingress); + + ret = ocelot_ace_rule_stats_update(ocelot, block_id, &ace); if (ret) return ret; diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h index 91357b1c8f31..4b2320bdc036 100644 --- a/include/soc/mscc/ocelot.h +++ b/include/soc/mscc/ocelot.h @@ -540,7 +540,7 @@ struct ocelot { struct list_head multicast; - struct ocelot_acl_block acl_block; + struct ocelot_acl_block acl_block[3]; const struct vcap_props *vcap; diff --git a/include/soc/mscc/ocelot_vcap.h b/include/soc/mscc/ocelot_vcap.h index 26d9384b3657..495847a40490 100644 --- a/include/soc/mscc/ocelot_vcap.h +++ b/include/soc/mscc/ocelot_vcap.h @@ -14,9 +14,9 @@ */ enum { - /* VCAP_IS1, */ + VCAP_IS1, VCAP_IS2, - /* VCAP_ES0, */ + VCAP_ES0, }; struct vcap_props {
There are three hardware TCAMs for ocelot chips: IS1, IS2 and ES0. Each one supports different actions. The hardware flow order is: IS1->IS2->ES0. This patch add three blocks to store rules according to chain index. chain 0 is offloaded to IS1, chain 1 is offloaded to IS2, and egress chain 0 is offloaded to ES0. Using action goto chain to express flow order as follows: tc filter add dev swp0 chain 0 parent ffff: flower skip_sw \ action goto chain 1 Signed-off-by: Xiaoliang Yang <xiaoliang.yang_1@nxp.com> --- drivers/net/ethernet/mscc/ocelot_ace.c | 51 +++++++++++++++-------- drivers/net/ethernet/mscc/ocelot_ace.h | 7 ++-- drivers/net/ethernet/mscc/ocelot_flower.c | 46 +++++++++++++++++--- include/soc/mscc/ocelot.h | 2 +- include/soc/mscc/ocelot_vcap.h | 4 +- 5 files changed, 81 insertions(+), 29 deletions(-)