Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Separate XPN configuration attribute from read-only attribute #1169

Merged
merged 4 commits into from
Jan 14, 2021

Conversation

dipankar-ba
Copy link
Contributor

Added a new read-only current XPN (available for both ingress and egress) and separate from the egress attribute for setting initial XPN value.
Renamed ingress set-and-clear attribute MINIMUM_XPN to MINIMUM_INGRESS_XPN.

@kcudnik
Copy link
Collaborator

kcudnik commented Dec 8, 2020

We discussed about this whether we want to separate attributes, or add a flag "@dynamic" for existing attribute. There are pros and cons for both of them, we didn't decided yet for that. Will let you know.

@lguohan yfi

@qbdwlr
Copy link
Contributor

qbdwlr commented Dec 8, 2020

I think it makes sense to have a separate operational vs configuration attribute to avoid multiple writers (SAI and MKA) to the same attribute.
There is no precedent in this SAI MACsec definition for including the keywords "INGRESS" and "EGRESS" even though there are other attributes that are specific to a single direction. I don't think changing the naming is warranted as the comments make the usage clear and this will keep the naming convention consistent.
Since there is no longer a scenario for their values to change post-creation, I would suggest updating the configuration attributes to CREATE_ONLY.

inc/saimacsec.h Outdated
@@ -758,18 +758,27 @@ typedef enum _sai_macsec_sa_attr_t
* @default 0
* @validonly SAI_MACSEC_SA_ATTR_MACSEC_DIRECTION == SAI_MACSEC_DIRECTION_EGRESS
*/
SAI_MACSEC_SA_ATTR_XPN,
SAI_MACSEC_SA_ATTR_INITIAL_EGRESS_XPN,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we can do alias here so that we can maintain backward compatible here.

Copy link
Contributor

@prafull-brcm prafull-brcm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes looks good to me.

@dipankar-ba dipankar-ba changed the title Xpn changes Separate XPN configuration attribute from read-only attribute Jan 7, 2021
@lguohan lguohan merged commit 5d5784d into opencomputeproject:master Jan 14, 2021
SAI_MACSEC_SA_ATTR_CURRENT_XPN,

/** @ignore - for backward compatibility */
SAI_MACSEC_SA_ATTR_XPN = SAI_MACSEC_SA_ATTR_CURRENT_XPN,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dipankar-ba this does not look correct. SAI_MACSEC_SA_ATTR_XPN should be alias SAI_MACSEC_SA_ATTR_CONFIGURED_EGRESS_XPN ? This has broken backward compatibility as SAI_MACSEC_SA_ATTR_XPN was Create and Set and now it became Read only.

Here is the error when we are moving v1.8.0
https://dev.azure.com/mssonic/build/_build/results?buildId=9116&view=logs&jobId=83516c17-6666-5250-abde-63983ce72a49&j=83516c17-6666-5250-abde-63983ce72a49&t=6177235f-d4f1-5f72-835a-90ebb93a1784

can you please check again

cc @rlhui @kcudnik

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The underlying hardware has a counter whose value has to be periodically read by NOS to run MKA protocol through SAI_MACSEC_SA_ATTR_CURRENT_XPN. Since this read is mandatory for MACsec operation, I tried to keep this backward compatible.
For counter should be initialized to 0 by the driver and normally never be modified by NOS for normal operation. So I do not expect SAI_MACSEC_SA_ATTR_CONFIGURED_EGRESS_XPN to be read or written except perhaps for synthetic tests for testing counter saturation.

lguohan pushed a commit to sonic-net/sonic-swss that referenced this pull request Jul 15, 2021
What I did
Rename XPN attributes from SAI_MACSEC_SA_ATTR_MINIMUM_XPN and SAI_MACSEC_SA_ATTR_XPN to SAI_MACSEC_SA_ATTR_CURRENT_XPN

Why I did it
Due to opencomputeproject/SAI#1169 that refactors the attributes about XPN, a new attributes SAI_MACSEC_SA_ATTR_CURRENT_XPN for both ingress and egress was introduced for reading the Packet number. So move the original attributes to the new one.

Signed-off-by: Ze Gan <[email protected]>
raphaelt-nvidia pushed a commit to raphaelt-nvidia/sonic-swss that referenced this pull request Oct 5, 2021
What I did
Rename XPN attributes from SAI_MACSEC_SA_ATTR_MINIMUM_XPN and SAI_MACSEC_SA_ATTR_XPN to SAI_MACSEC_SA_ATTR_CURRENT_XPN

Why I did it
Due to opencomputeproject/SAI#1169 that refactors the attributes about XPN, a new attributes SAI_MACSEC_SA_ATTR_CURRENT_XPN for both ingress and egress was introduced for reading the Packet number. So move the original attributes to the new one.

Signed-off-by: Ze Gan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants