[sai-gen] Create SAI spec for SAI API generation.

[r12f]

Problem

Currently, the SAI API generation is facing 2 issues: ABI compatibility and code review with SAI API change.

To maintain the ABI compatibility, it is required to maintain the order of API, attributes, stats in the order of its creation. The newer ones must comes later. Before the "order" annotation is introduced, this problem was never solved. With "order" annotation, although it kinda helps some cases, but it is still not good - it is hacky, and it is not generically applicable. E.g., currently, the keys and action parameters are not sorted together, counters can be introduced in multiple files making "order" not intuitive.

Furthermore, with P4 updates, it is hard to review the SAI API changes, because these changes are not captured in the PR.

What we are doing in this change

The SAI spec YAML file is introduced for helping these cases.

First all, all changes to the SAI API will be reflected in the SAI spec YAML file, allow us to review the PR without building the SAI branch. Once the CI passes, we will know how the SAI APIs are being changed, and it works.

Secondly, it helps maintaining the ABI compatibility in future. Instead of using "order" to explicitly order the attributes, we can merge the new API spec with the old API spec by insert any new things in the end of the spec. This ensures any new changes will be put in the end, and no hacky solution needs to be introduced again.

This change will not change any generated SAI headers and libs.

[KrisNey-MSFT]

Need to ensure attributes we generate are always in order.

@kcudnik commented: Custom range: could vendor ranges overlap? Would this be compatible?
@marian-pritsak: could we carve out the bottom half of the range?
@r12f: carve part of extension range out of (end of) custom range?
@marian-pritsak: define this value in the extension file? Define this in order to not interfere with the custom range.
@kcudnik: current parser does now allow..? Fixed value vs relative value? Parser may need to be updated. Attribute switching/serialization may need logic removal to ignore that attributes are not increasing 1x1...

[kcudnik]

custom range was intended to be used by vendor for private internal attributes, if we want to have extensions in separate range, we could add a new range const based like:

SAI_PORT_POOL_ATTR_EXTENSIONS_RANGE_START = 0x20000000,

SAI_PORT_POOL_ATTR_EXTENSIONS_RANGE_END

but be aware that adding those extensions could be not backward compatible, since for example dash attributes are changing values and names many times already, some were added, some removed, many shifted, that's why they were in extensions/experimental directory

[r12f]

Thanks a lot for sharing the recommended change, Kamil!

And yes, I am totally agree with you. All the things that mentioned by you have happened before, which is reason I am proposing this change. As we are moving towards production, it will be great to have these issues avoided at the closest place to the source.

With the yaml file, all changes will be captured and shown directly in the PR. Later on, we can easily make the new attribute being added to the end, old attributes being marked as deprecated instead of being removed, and rename will be an add + deprecate.

Additionally, we can use this file to generate the SAI headers, which will greatly simplify the logic in the j2 template. (Honestly, it is so hard to figure out which if and endif are paired, because there is no indent...)

[kcudnik]

so if we go with this approach, do we still need support for enums/attributes to be continued from XX_ATTR_END ? or all extensions should be starting from EXTENSIONS_RANGE_START ?

can you prepare example on existing files in one of refular enum and one of attribute

btw. there will be problem with SAI_OBJECT_TYPE, since so far object type is 1 bylte, and object type in sonic and probably other verndors is encoded inside OID VALUE, this is very handy to use in sonic/sairedis since we alrady know what we are deling with

moving SAI_OBJECT_TYPE in extensions to range starting frox 0x2000000, encoding that into OID will be impossible and there is no easy way to mitigate that note that SAI_OBJECT_TYPE in extensions starts from SAI_OBJECT_TYPE_MAX which also can shift when new values are added

we need to have discussion on that how to solve this

similar issue is with SAI_API_XX, since we use that at index in api table, but this could be easly mitigated

also for object type we have easy checks like this if obejct type is valid:

return (ot > SAI_OBJECT_TYPE_NULL) && (ot < SAI_OBJECT_TYPE_EXTENSIONS_MAX);

moving exstensions to separate range, will force to iterare over entire table of enums to validate whether values is correct or to check 2 ranges

• @lguohan @r12f @marian-pritsak

[r12f]

so if we go with this approach, do we still need support for enums/attributes to be continued from XX_ATTR_END ? or all extensions should be starting from EXTENSIONS_RANGE_START ?

IMHO, it will be better for all extensions to start with EXTENSIONS_RANGE_START, so we will have only 1 pattern to follow. However, this will make other extensions to be updated, such as bmtor, which is not part of DASH. I am not sure if this is acceptable.

can you prepare example on existing files in one of refular enum and one of attribute

Yea, I can create a manually updated DASH SAI API change in SAI repo. The port stats should be a great example. It might not pass the CI, but we can use it as reference.

btw. there will be problem with SAI_OBJECT_TYPE, since so far object type is 1 bylte, and object type in sonic and probably other verndors is encoded inside OID VALUE, this is very handy to use in sonic/sairedis since we alrady know what we are deling with

moving SAI_OBJECT_TYPE in extensions to range starting frox 0x2000000, encoding that into OID will be impossible and there is no easy way to mitigate that note that SAI_OBJECT_TYPE in extensions starts from SAI_OBJECT_TYPE_MAX which also can shift when new values are added

we need to have discussion on that how to solve this

similar issue is with SAI_API_XX, since we use that at index in api table, but this could be easly mitigated

also for object type we have easy checks like this if obejct type is valid:

return (ot > SAI_OBJECT_TYPE_NULL) && (ot < SAI_OBJECT_TYPE_EXTENSIONS_MAX);

moving exstensions to separate range, will force to iterare over entire table of enums to validate whether values is correct or to check 2 ranges

yea, we will need to discuss on the object type and API type. Since they are less frequently to be updated comparing to attributes, we can probably do it as step 2. And get the attribute working first.

And for now, I am thinking maybe using a much smaller number for object types, like 512 or 1024, since the custom range start is also much smaller. And checking 2 ranges seems to be a much better way to me. And for devices that only supports regular object types not custom range. It will still work as it is.

And I think API type is the hardest one, since it is used as the index of the API table... but I feel you might already have some idea on that.

[kcudnik]

that object type custom range 256 was added there just to satisfy gcc error when casting, since object type and extended object type is more than 7 bits, and there was compiler warning on that

[r12f]

that object type custom range 256 was added there just to satisfy gcc error when casting, since object type and extended object type is more than 7 bits, and there was compiler warning on that

I see. i will schedule a meeting for further discussion on the object types and api types.

And I wonder if you mind we moving this PR forward first, since I believe they are 2 orthogonal issues? :D

[r12f]

thank you so much, Kamil! see you next week!