Add ACL Mirror Copy Type and Mirror Session ID match fields by mobinmohan · Pull Request #2274 · opencomputeproject/SAI

mobinmohan · 2026-04-10T00:52:05Z

This PR introduces two new ACL match fields to the SAI ACL headers:

SAI_ACL_TABLE_ATTR_FIELD_ACL_MIRROR_COPY_TYPE: Allows ACL rules to match packets based on whether they are mirror copies and their specific type (Ingress, Egress, or Ingress Or Egress).
SAI_ACL_TABLE_ATTR_FIELD_ACL_MIRROR_SESSION_ID: Allows ACL rules to match based on a specific mirror session ID.

JaiOCP · 2026-04-13T21:42:54Z

_ No description provided. _

Pleas provide description of the PR

JaiOCP · 2026-04-13T21:43:48Z

@mobinmohan Is it an ingress mirror or egress mirror copy match condition.

mobinmohan · 2026-04-14T00:05:48Z

@mobinmohan Is it an ingress mirror or egress mirror copy match condition.

The attribute was to match both ingress and egress mirror copies. However, I can change the type from bool to sai_acl_mirror_copy_type_t to allow selective matching, if the underlying hardware supports it. Please let me know your recommendation.

+/**
+ * @brief Mirror copy type for ACL matching
+ */
+typedef enum _sai_acl_mirror_copy_type_t
+{
+    SAI_ACL_MIRROR_COPY_TYPE_NONE,
+    SAI_ACL_MIRROR_COPY_TYPE_INGRESS,
+    SAI_ACL_MIRROR_COPY_TYPE_EGRESS,
+    SAI_ACL_MIRROR_COPY_TYPE_ANY
+} sai_acl_mirror_copy_type_t;
+
+

JaiOCP · 2026-04-14T02:58:23Z

@mobinmohan Is it an ingress mirror or egress mirror copy match condition.

The attribute was to match both ingress and egress mirror copies. However, I can change the type from bool to sai_acl_mirror_copy_type_t to allow selective matching, if the underlying hardware supports it. Please let me know your recommendation.
+/**
+ * @brief Mirror copy type for ACL matching
+ */
+typedef enum _sai_acl_mirror_copy_type_t
+{
+    SAI_ACL_MIRROR_COPY_TYPE_NONE,
+    SAI_ACL_MIRROR_COPY_TYPE_INGRESS,
+    SAI_ACL_MIRROR_COPY_TYPE_EGRESS,
+    SAI_ACL_MIRROR_COPY_TYPE_ANY
+} sai_acl_mirror_copy_type_t;
+
+

I don't think we need to have type_t.
If the ACL table stage is ingress then the match action would apply to ingress mirror.
If the ACL table stage is egress then the match action would apply to egress mirror.

Typically there is no case where ingress mirror copy needs an egress ACL rule for match action as the mirror copy is taken out to the mirror port directly but I am not very sure about it. Is that a use case for you?

mobinmohan · 2026-04-23T06:03:30Z

_ No description provided. _

Pleas provide description of the PR

Added description.

mobinmohan · 2026-04-23T06:06:58Z

@mobinmohan Is it an ingress mirror or egress mirror copy match condition.

The attribute was to match both ingress and egress mirror copies. However, I can change the type from bool to sai_acl_mirror_copy_type_t to allow selective matching, if the underlying hardware supports it. Please let me know your recommendation.
+/**
+ * @brief Mirror copy type for ACL matching
+ */
+typedef enum _sai_acl_mirror_copy_type_t
+{
+    SAI_ACL_MIRROR_COPY_TYPE_NONE,
+    SAI_ACL_MIRROR_COPY_TYPE_INGRESS,
+    SAI_ACL_MIRROR_COPY_TYPE_EGRESS,
+    SAI_ACL_MIRROR_COPY_TYPE_ANY
+} sai_acl_mirror_copy_type_t;
+
+
I don't think we need to have type_t. If the ACL table stage is ingress then the match action would apply to ingress mirror. If the ACL table stage is egress then the match action would apply to egress mirror.

Typically there is no case where ingress mirror copy needs an egress ACL rule for match action as the mirror copy is taken out to the mirror port directly but I am not very sure about it. Is that a use case for you?

Sounds good. We can continue with the default(bool).

Gnanapriya27 · 2026-04-24T09:44:05Z

_sai_acl_mirror_copy_type_t

The current bool definition still leaves ambiguity on what exactly is being matched.
Interpreting this based on the stage as per ACL table,
Ingress ACL stage → match ingress-generated mirror copies only
Egress ACL stage → match egress-generated mirror copies only

then SAI_ACL_ENTRY_ATTR_FIELD_MIRROR_COPY=true in an egress ACL table, does it mean only egress mirror copies, not any mirrored packet ? please clarify.

Modeling this stage/direction explicitly using sai_acl_mirror_copy_type_t (as proposed in this conversation) would make the semantics unambiguous and future-proof the design, enum capability query allows the vendor to announce their capabilities.

Gnanapriya27 · 2026-04-24T09:46:56Z

+     * @flags CREATE_ONLY
+     * @default false
+     */
+    SAI_ACL_TABLE_ATTR_FIELD_MIRROR_COPY = SAI_ACL_TABLE_ATTR_FIELD_START + 0x167,


Can you clarify whether SAI_ACL_ENTRY_ATTR_FIELD_MIRROR_COPY matches mirror copies generated by any mirror session (i.e. a generic “mirrored packet” qualifier), or whether there is intent to support classification of mirror copies generated by a specific mirror session?

A potential use case is applying different ACL treatment to traffic mirrored by different mirror sessions (for example, Mirror session A vs session B). With the current boolean qualifier, it is not clear how per-session discrimination would be achieved.

JaiOCP · 2026-04-24T16:00:31Z

_sai_acl_mirror_copy_type_t

The current bool definition still leaves ambiguity on what exactly is being matched. Interpreting this based on the stage as per ACL table, Ingress ACL stage → match ingress-generated mirror copies only Egress ACL stage → match egress-generated mirror copies only

then SAI_ACL_ENTRY_ATTR_FIELD_MIRROR_COPY=true in an egress ACL table, does it mean only egress mirror copies, not any mirrored packet ? please clarify.

Modeling this stage/direction explicitly using sai_acl_mirror_copy_type_t (as proposed in this conversation) would make the semantics unambiguous and future-proof the design, enum capability query allows the vendor to announce their capabilities.

I agree and take my earlier statement back.
Just providing the stage is not enough. Lets use the type_t

mobinmohan · 2026-04-27T17:15:33Z

_sai_acl_mirror_copy_type_t

The current bool definition still leaves ambiguity on what exactly is being matched. Interpreting this based on the stage as per ACL table, Ingress ACL stage → match ingress-generated mirror copies only Egress ACL stage → match egress-generated mirror copies only

then SAI_ACL_ENTRY_ATTR_FIELD_MIRROR_COPY=true in an egress ACL table, does it mean only egress mirror copies, not any mirrored packet ? please clarify.

Modeling this stage/direction explicitly using sai_acl_mirror_copy_type_t (as proposed in this conversation) would make the semantics unambiguous and future-proof the design, enum capability query allows the vendor to announce their capabilities.

I received a similar feedback during the weekly review meeting. I agree using sai_acl_mirror_copy_type_t makes the design future proof. I have updated the PR to use the enum type, though proper distinction will depend on the ASIC support.

mobinmohan · 2026-04-27T17:16:47Z

_sai_acl_mirror_copy_type_t

The current bool definition still leaves ambiguity on what exactly is being matched. Interpreting this based on the stage as per ACL table, Ingress ACL stage → match ingress-generated mirror copies only Egress ACL stage → match egress-generated mirror copies only
then SAI_ACL_ENTRY_ATTR_FIELD_MIRROR_COPY=true in an egress ACL table, does it mean only egress mirror copies, not any mirrored packet ? please clarify.
Modeling this stage/direction explicitly using sai_acl_mirror_copy_type_t (as proposed in this conversation) would make the semantics unambiguous and future-proof the design, enum capability query allows the vendor to announce their capabilities.

I agree and take my earlier statement back. Just providing the stage is not enough. Lets use the type_t

Thanks for reviewing this during the weekly meeting. I have update the PR now.

tjchadaga · 2026-05-05T19:29:17Z

/azp run

azure-pipelines · 2026-05-05T19:29:27Z

Azure Pipelines successfully started running 1 pipeline(s).

This change introduces a new ACL match field `ACL_MIRROR_COPY_TYPE` to allow ACLs to match based on whether the packet is a mirror copy. It allows ACLs to match based on the type of mirror copy (Ingress, Egress, or Ingress or Egress). Change-Id: I351b2aba82b10ce52425615920eb07e7fd964f96 Signed-off-by: Mobin Mohan <mobinmohan@google.com>

Gnanapriya27 · 2026-05-20T04:35:27Z

+     * @flags CREATE_AND_SET
+     * @default disabled
+     */
+    SAI_ACL_ENTRY_ATTR_FIELD_ACL_MIRROR_COPY_TYPE = SAI_ACL_ENTRY_ATTR_FIELD_START + 0x167,


Semantics related:
Remove ACL from the new attr to be inlined with existing attr SAI_ACL_ENTRY_ATTR_FIELD_ACL_MIRROR_COPY_TYPE to
SAI_ACL_ENTRY_ATTR_FIELD_MIRROR_COPY_TYPE

Same for other attr as well

The SAI metadata checker enforces a convention for attributes with custom enum type. It requires the attribute name suffix match the enum type name (excluding the sai_ prefix and _t suffix). Here the type name is sai_acl_mirror_copy_type_t. Removing acl keyword from the type_t would not be preferred here.

The naming is also similar to SAI_ACL_ENTRY_ATTR_FIELD_ACL_IP_TYPE and SAI_ACL_ENTRY_ATTR_FIELD_ACL_IP_FRAG.

Thanks for the details, Agree.

Gnanapriya27 · 2026-05-20T04:43:04Z

+typedef enum _sai_acl_mirror_copy_type_t
+{
+    /** Match packets that are not mirror copies */
+    SAI_ACL_MIRROR_COPY_TYPE_NONE,


what does NONE signifies ?
MIRROR_COPY is a match criteria that is specifically meant for matching on the mirrored copy of the packet alone and not the original packet.

`NONE' description indicated not mirror copies
NONE enum description and the primary intent of this qualifier still raises ambiguity

what does NONE signifies ?

Match on original packet is specified using NONE.

MIRROR_COPY is a match criteria that is specifically meant for matching on the mirrored copy of the packet alone and not the original packet.

I do not think that aligns with the intended behavior. If a user shares the same ACL group for multiple features, they need control to differentiate between original vs mirror traffic. If a feature should not apply to mirror copies, they need a way to specify that. NONE will help to specify the rule should not match on mirror copies.

Gnanapriya27 · 2026-05-20T04:54:40Z

+    SAI_ACL_ENTRY_ATTR_FIELD_ACL_MIRROR_COPY_TYPE = SAI_ACL_ENTRY_ATTR_FIELD_START + 0x167,
+
+    /**
+     * @brief Match on mirror session ID


it would be better to brief it like Match on mirror copies generated by this mirror session ID

Sure. I have updated it.

This change introduces a new ACL match field `ACL_MIRROR_SESSION_ID` to allow ACLs to match based on a specific mirror session ID. * Added support to match on `SAI_OBJECT_TYPE_MIRROR_SESSION` objects in ACL entries. * Added Table and Entry attributes with updated end markers. Change-Id: Iea1a664dbe8fca6e22a9b3f9c50ecb0fc9f29b75 Signed-off-by: Mobin Mohan <mobinmohan@google.com>

Gnanapriya27

Looks good to me

mobinmohan force-pushed the add_mirror_copy branch 2 times, most recently from cd28e05 to 0ae367c Compare April 10, 2026 23:08

kcudnik reviewed Apr 14, 2026

View reviewed changes

Comment thread inc/saiacl.h Outdated

mobinmohan closed this Apr 23, 2026

mobinmohan reopened this Apr 23, 2026

Gnanapriya27 reviewed Apr 24, 2026

View reviewed changes

mobinmohan force-pushed the add_mirror_copy branch from 0ae367c to 1fc21bb Compare April 27, 2026 16:59

mobinmohan changed the title ~~Add SAI_ACL_TABLE_ATTR_FIELD_MIRROR_COPY~~ Add SAI_ACL_TABLE_ATTR_FIELD_MIRROR_COPY_TYPE Apr 27, 2026

mobinmohan force-pushed the add_mirror_copy branch from 1fc21bb to 6923c55 Compare April 27, 2026 18:02

mobinmohan changed the title ~~Add SAI_ACL_TABLE_ATTR_FIELD_MIRROR_COPY_TYPE~~ Add SAI_ACL_TABLE_ATTR_FIELD_ACL_MIRROR_COPY_TYPE Apr 27, 2026

mobinmohan force-pushed the add_mirror_copy branch from 6923c55 to d56bb79 Compare April 27, 2026 18:07

JaiOCP approved these changes Apr 28, 2026

View reviewed changes

mikeberesford approved these changes Apr 28, 2026

View reviewed changes

tjchadaga added the reviewed PR is discussed in SAI Meeting label Apr 28, 2026

tjchadaga requested a review from Gnanapriya27 May 5, 2026 19:28

Gnanapriya27 reviewed May 6, 2026

View reviewed changes

Comment thread inc/saiacl.h

mobinmohan force-pushed the add_mirror_copy branch from d56bb79 to b31131d Compare May 19, 2026 06:27

mobinmohan changed the title ~~Add SAI_ACL_TABLE_ATTR_FIELD_ACL_MIRROR_COPY_TYPE~~ Add ACL Mirror Copy Type and Mirror Session ID match fields May 19, 2026

Gnanapriya27 reviewed May 20, 2026

View reviewed changes

mobinmohan force-pushed the add_mirror_copy branch from 73aa841 to d33e34b Compare May 21, 2026 02:13

Gnanapriya27 approved these changes May 22, 2026

View reviewed changes

Conversation

mobinmohan commented Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JaiOCP commented Apr 13, 2026

Uh oh!

JaiOCP commented Apr 13, 2026

Uh oh!

mobinmohan commented Apr 14, 2026

Uh oh!

JaiOCP commented Apr 14, 2026

Uh oh!

Uh oh!

mobinmohan commented Apr 23, 2026

Uh oh!

mobinmohan commented Apr 23, 2026

Uh oh!

Gnanapriya27 commented Apr 24, 2026

Uh oh!

Gnanapriya27 Apr 24, 2026

Choose a reason for hiding this comment

Uh oh!

JaiOCP commented Apr 24, 2026

Uh oh!

mobinmohan commented Apr 27, 2026

Uh oh!

mobinmohan commented Apr 27, 2026

Uh oh!

tjchadaga commented May 5, 2026

Uh oh!

azure-pipelines Bot commented May 5, 2026

Uh oh!

Uh oh!

Uh oh!

Gnanapriya27 May 20, 2026

Choose a reason for hiding this comment

Uh oh!

mobinmohan May 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gnanapriya27 May 21, 2026

Choose a reason for hiding this comment

Uh oh!

Gnanapriya27 May 20, 2026

Choose a reason for hiding this comment

Uh oh!

mobinmohan May 21, 2026

Choose a reason for hiding this comment

Uh oh!

Gnanapriya27 May 20, 2026

Choose a reason for hiding this comment

Uh oh!

mobinmohan May 21, 2026

Choose a reason for hiding this comment

Uh oh!

Gnanapriya27 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mobinmohan commented Apr 10, 2026 •

edited

Loading

mobinmohan May 21, 2026 •

edited

Loading