Re: [PATCH net-next v1 2/2] net: dsa: microchip: Add partial ACL support for ksz9477 switches

[Vladimir Oltean]

On Mon, Apr 17, 2023 at 01:03:11PM +0200, Oleksij Rempel wrote:
> Certain aspects of the chip specification appeared ambiguous, leading me
> to decide to allocate a separate time slot for investigating the counter
> topic if necessary.
> 
> For example, according to the
> KSZ9477 4.4.18 ACCESS CONTROL LIST (ACL) FILTERING:
> 
> "It is also possible to configure the ACL table so that multiple processing
> entries specify the same action rule. In this way, the final matching result is
> the OR of the matching results from each of the multiple RuleSets.
> The 16 ACL rules represent an ordered list, with entry #0 having the highest
> priority and entry #15 having the lowest priority. All matching rules are
> evaluated. If there are multiple true match results and multiple corresponding
> actions, the highest priority (lowest numbered) of those actions will be the
> one taken."
> 
> A summary of this part of documentation is:
> 1. ACL table can have multiple entries specifying the same action rule.
> 2. Final matching result is the OR of multiple RuleSets' results.
> 3. 16 ACL rules form an ordered list, with priority descending from #0 to #15.
> 4. All matching rules are evaluated.
> 5. When multiple true matches and actions occur, the highest priority action is
>    executed.
> 
> Considering this, there is a possibility that separate action rules would not
> be executed, as they might not be the highest priority match.  Since counters
> would have separation action rules, they would not be executed or prevent other
> action rules from execution.
> 
> To confirm my hypothesis, additional time and testing will be required.
> Nonetheless, I hope this issue does not impede the progress of this patch.

This is the kind of stuff you'd have to know when adding a software model
for the rules, right? Could you consider writing a selftest that
precisely illustrates the matching pattern of the hardware? It would be
good if the same test could then be run on a software-only implementation
and if the behavior would match. The tc tool should be more than a
vendor agnostic tool of doing vendor specific stuff. It should offload
as faithfully as possible the software data path. It would also be good,
but I haven't studied or used this test personally, if the test could be
based on the existing tools/testing/selftests/net/forwarding/skbedit_priority.sh.

> > > > Have you considered the "skbedit priority" action as opposed to hw_tc?
> > > 
> > > I had already thought of that, but since bridging is offloaded in the HW
> > > no skbs are involved, i thought it will be confusing. Since tc-flower seems to
> > > already support hw_tc remapping, I decided to use it. I hope it will not harm,
> > > to use it for now as mandatory option and make it optional later if other
> > > actions are added, including skbedit.
> > 
> > Well, skbedit is offloadable, so in that sense, its behavior is defined
> > even when no skbs are involved. OTOH, skbedit also has a software data
> > path (sets skb->priority), as opposed to hw_tc, which last time I checked,
> > did not.
> 
> Alright, having tc rules be portable is certainly a benefit. I presume
> that in this situation, it's not an exclusive "either...or" choice. Both
> variants can coexist, and the skbedit action can be incorporated at a
> later time. Is that accurate?

I believe Petr Machata (now copied) could have an opinion here too.