Add ACL Mirror Copy Type and Mirror Session ID match fields

[mobinmohan]

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

1. 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).

2. SAI_ACL_TABLE_ATTR_FIELD_ACL_MIRROR_SESSION_ID: Allows ACL rules to match based on a specific mirror session ID.

[JaiOCP]

_ No description provided. _

Pleas provide description of the PR

[JaiOCP]

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

[mobinmohan]

@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]

@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]

_ No description provided. _

Pleas provide description of the PR

Added description.

[mobinmohan]

@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]

_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.

[JaiOCP]

_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]

_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]

_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]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[Gnanapriya27]

Looks good to me