On Wed, Jun 7, 2023 at 5:20 PM Akhil Goyal <gakhil@marvell.com> wrote:
>
> MACsec SC/SA ids are created based on direction of the flow.
> Hence, added the missing field for configuration and cleanup
> of the SCs and SAs.
>
> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> ---
> devtools/libabigail.abignore | 7 +++++++
> lib/security/rte_security.c | 16 ++++++++++------
> lib/security/rte_security.h | 14 ++++++++++----
> lib/security/rte_security_driver.h | 12 ++++++++++--
> 4 files changed, 37 insertions(+), 12 deletions(-)
>
Looking at the report with no supression rule:
$ abidiff --suppr .../next-cryptodev/devtools/libabigail.abignore
--no-added-syms --headers-dir1
.../abi/v23.03/build-gcc-shared/usr/local/include --headers-dir2
.../next-cryptodev/build-gcc-shared/install/usr/local/include
.../abi/v23.03/build-gcc-shared/usr/local/lib64/librte_security.so.23.1
.../next-cryptodev/build-gcc-shared/install/usr/local/lib64/librte_security.so.23.2
Functions changes summary: 0 Removed, 1 Changed (13 filtered out), 0
Added functions
Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
1 function with some indirect sub-type change:
[C] 'function const rte_security_capability*
rte_security_capabilities_get(rte_security_ctx*)' at
rte_security.c:241:1 has some indirect sub-type changes:
parameter 1 of type 'rte_security_ctx*' has sub-type changes:
in pointed to type 'struct rte_security_ctx' at rte_security.h:69:1:
type size hasn't changed
1 data member change:
type of 'const rte_security_ops* ops' changed:
in pointed to type 'const rte_security_ops':
in unqualified underlying type 'struct rte_security_ops'
at rte_security_driver.h:230:1:
type size hasn't changed
4 data member changes (4 filtered):
type of 'security_macsec_sc_create_t
macsec_sc_create' changed:
underlying type 'int (void*,
rte_security_macsec_sc*)*' changed:
in pointed to type 'function type int (void*,
rte_security_macsec_sc*)':
parameter 2 of type 'rte_security_macsec_sc*'
has sub-type changes:
in pointed to type 'struct
rte_security_macsec_sc' at rte_security.h:399:1:
type size changed from 256 to 320 (in bits)
1 data member insertion:
'union {struct {uint16_t sa_id[4];
uint8_t sa_in_use[4]; uint8_t active; uint8_t is_xpn; uint8_t
reserved;} sc_rx; struct {uint16_t sa_id; uint16_t sa_id_rekey;
uint64_t sci; uint8_t active; uint8_t re_key_en; uint8_t is_xpn;
uint8_t reserved;} sc_tx;}', at offset 128 (in bits)
1 data member change:
anonymous data member union {struct
{uint16_t sa_id[4]; uint8_t sa_in_use[4]; uint8_t active; uint8_t
reserved;} sc_rx; struct {uint16_t sa_id; uint16_t sa_id_rekey;
uint64_t sci; uint8_t active; uint8_t re_key_en; uint8_t reserved;}
sc_tx;} at offset 64 (in bits) became data member 'uint64_t
pn_threshold'
and size changed from 192 to 64 (in
bits) (by -128 bits)
type of 'security_macsec_sc_destroy_t
macsec_sc_destroy' changed:
underlying type 'int (void*, typedef uint16_t)*' changed:
in pointed to type 'function type int (void*,
typedef uint16_t)':
parameter 3 of type 'enum
rte_security_macsec_direction' was added
type of 'security_macsec_sc_stats_get_t
macsec_sc_stats_get' changed:
underlying type 'int (void*, typedef uint16_t,
rte_security_macsec_sc_stats*)*' changed:
in pointed to type 'function type int (void*,
typedef uint16_t, rte_security_macsec_sc_stats*)':
parameter 3 of type
'rte_security_macsec_sc_stats*' changed:
entity changed from
'rte_security_macsec_sc_stats*' to 'enum
rte_security_macsec_direction' at rte_security.h:361:1
type size changed from 64 to 32 (in bits)
type alignment changed from 0 to 32
parameter 4 of type
'rte_security_macsec_sc_stats*' was added
type of 'security_macsec_sa_stats_get_t
macsec_sa_stats_get' changed:
underlying type 'int (void*, typedef uint16_t,
rte_security_macsec_sa_stats*)*' changed:
in pointed to type 'function type int (void*,
typedef uint16_t, rte_security_macsec_sa_stats*)':
parameter 3 of type
'rte_security_macsec_sa_stats*' changed:
entity changed from
'rte_security_macsec_sa_stats*' to 'enum
rte_security_macsec_direction' at rte_security.h:361:1
type size changed from 64 to 32 (in bits)
type alignment changed from 0 to 32
parameter 4 of type
'rte_security_macsec_sa_stats*' was added
The report complains about the macsec ops type changes.
> diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
> index c0361bfc7b..14d8fa4293 100644
> --- a/devtools/libabigail.abignore
> +++ b/devtools/libabigail.abignore
> @@ -37,6 +37,13 @@
> [suppress_type]
> type_kind = enum
> changed_enumerators = RTE_CRYPTO_ASYM_XFORM_ECPM, RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> +; Ignore changes to rte_security_ops MACsec APIs which are experimental
> +[suppress_type]
> + name = rte_security_ops
> + has_data_member_inserted_between =
> + {
> + offset_of(security_macsec_sc_create_t), offset_of(security_macsec_sa_stats_get_t)
> + }
So I don't get the intent with this rule.
There is no field named neither security_macsec_sc_create_t nor
security_macsec_sa_stats_get_t in the rte_security_ops struct.
Now.. why is this rule making the check pass... it is a mystery to me.
I already hit a case when libabigail ignored statements that are
invalid or make no sense, so my guess is that this rule is actually
applied as a simple:
[suppress_type]
name = rte_security_ops
And well, this rule is ok from my pov: this rte_security_ops struct
does not change in size.
An application is not supposed to know about its content (that is
defined in a driver header) and all accesses to those ops are supposed
to be through symbols from the security library.
So I would go with this "larger" and simpler rule.
Just a small addition to this, as discussed offlist, this is going to
be reworked in v23.11 and this rule on rte_security_ops will be
unneccesary, so please move it in the relevant block (at the end) of
the libabigail.abignore file.
> On Wed, Jun 7, 2023 at 5:20 PM Akhil Goyal <gakhil@marvell.com> wrote:
> >
> > MACsec SC/SA ids are created based on direction of the flow.
> > Hence, added the missing field for configuration and cleanup
> > of the SCs and SAs.
> >
> > Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> > ---
> > devtools/libabigail.abignore | 7 +++++++
> > lib/security/rte_security.c | 16 ++++++++++------
> > lib/security/rte_security.h | 14 ++++++++++----
> > lib/security/rte_security_driver.h | 12 ++++++++++--
> > 4 files changed, 37 insertions(+), 12 deletions(-)
> >
>
> Looking at the report with no supression rule:
> $ abidiff --suppr .../next-cryptodev/devtools/libabigail.abignore
> --no-added-syms --headers-dir1
> .../abi/v23.03/build-gcc-shared/usr/local/include --headers-dir2
> .../next-cryptodev/build-gcc-shared/install/usr/local/include
> .../abi/v23.03/build-gcc-shared/usr/local/lib64/librte_security.so.23.1
> .../next-cryptodev/build-gcc-
> shared/install/usr/local/lib64/librte_security.so.23.2
> Functions changes summary: 0 Removed, 1 Changed (13 filtered out), 0
> Added functions
> Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
>
> 1 function with some indirect sub-type change:
>
> [C] 'function const rte_security_capability*
> rte_security_capabilities_get(rte_security_ctx*)' at
> rte_security.c:241:1 has some indirect sub-type changes:
> parameter 1 of type 'rte_security_ctx*' has sub-type changes:
> in pointed to type 'struct rte_security_ctx' at rte_security.h:69:1:
> type size hasn't changed
> 1 data member change:
> type of 'const rte_security_ops* ops' changed:
> in pointed to type 'const rte_security_ops':
> in unqualified underlying type 'struct rte_security_ops'
> at rte_security_driver.h:230:1:
> type size hasn't changed
> 4 data member changes (4 filtered):
> type of 'security_macsec_sc_create_t
> macsec_sc_create' changed:
> underlying type 'int (void*,
> rte_security_macsec_sc*)*' changed:
> in pointed to type 'function type int (void*,
> rte_security_macsec_sc*)':
> parameter 2 of type 'rte_security_macsec_sc*'
> has sub-type changes:
> in pointed to type 'struct
> rte_security_macsec_sc' at rte_security.h:399:1:
> type size changed from 256 to 320 (in bits)
> 1 data member insertion:
> 'union {struct {uint16_t sa_id[4];
> uint8_t sa_in_use[4]; uint8_t active; uint8_t is_xpn; uint8_t
> reserved;} sc_rx; struct {uint16_t sa_id; uint16_t sa_id_rekey;
> uint64_t sci; uint8_t active; uint8_t re_key_en; uint8_t is_xpn;
> uint8_t reserved;} sc_tx;}', at offset 128 (in bits)
> 1 data member change:
> anonymous data member union {struct
> {uint16_t sa_id[4]; uint8_t sa_in_use[4]; uint8_t active; uint8_t
> reserved;} sc_rx; struct {uint16_t sa_id; uint16_t sa_id_rekey;
> uint64_t sci; uint8_t active; uint8_t re_key_en; uint8_t reserved;}
> sc_tx;} at offset 64 (in bits) became data member 'uint64_t
> pn_threshold'
> and size changed from 192 to 64 (in
> bits) (by -128 bits)
> type of 'security_macsec_sc_destroy_t
> macsec_sc_destroy' changed:
> underlying type 'int (void*, typedef uint16_t)*' changed:
> in pointed to type 'function type int (void*,
> typedef uint16_t)':
> parameter 3 of type 'enum
> rte_security_macsec_direction' was added
> type of 'security_macsec_sc_stats_get_t
> macsec_sc_stats_get' changed:
> underlying type 'int (void*, typedef uint16_t,
> rte_security_macsec_sc_stats*)*' changed:
> in pointed to type 'function type int (void*,
> typedef uint16_t, rte_security_macsec_sc_stats*)':
> parameter 3 of type
> 'rte_security_macsec_sc_stats*' changed:
> entity changed from
> 'rte_security_macsec_sc_stats*' to 'enum
> rte_security_macsec_direction' at rte_security.h:361:1
> type size changed from 64 to 32 (in bits)
> type alignment changed from 0 to 32
> parameter 4 of type
> 'rte_security_macsec_sc_stats*' was added
> type of 'security_macsec_sa_stats_get_t
> macsec_sa_stats_get' changed:
> underlying type 'int (void*, typedef uint16_t,
> rte_security_macsec_sa_stats*)*' changed:
> in pointed to type 'function type int (void*,
> typedef uint16_t, rte_security_macsec_sa_stats*)':
> parameter 3 of type
> 'rte_security_macsec_sa_stats*' changed:
> entity changed from
> 'rte_security_macsec_sa_stats*' to 'enum
> rte_security_macsec_direction' at rte_security.h:361:1
> type size changed from 64 to 32 (in bits)
> type alignment changed from 0 to 32
> parameter 4 of type
> 'rte_security_macsec_sa_stats*' was added
>
> The report complains about the macsec ops type changes.
>
> > diff --git a/devtools/libabigail.abignore b/devtools/libabigail.abignore
> > index c0361bfc7b..14d8fa4293 100644
> > --- a/devtools/libabigail.abignore
> > +++ b/devtools/libabigail.abignore
> > @@ -37,6 +37,13 @@
> > [suppress_type]
> > type_kind = enum
> > changed_enumerators = RTE_CRYPTO_ASYM_XFORM_ECPM,
> RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
> > +; Ignore changes to rte_security_ops MACsec APIs which are experimental
> > +[suppress_type]
> > + name = rte_security_ops
> > + has_data_member_inserted_between =
> > + {
> > + offset_of(security_macsec_sc_create_t),
> offset_of(security_macsec_sa_stats_get_t)
> > + }
>
> So I don't get the intent with this rule.
> There is no field named neither security_macsec_sc_create_t nor
> security_macsec_sa_stats_get_t in the rte_security_ops struct.
>
> Now.. why is this rule making the check pass... it is a mystery to me.
> I already hit a case when libabigail ignored statements that are
> invalid or make no sense, so my guess is that this rule is actually
> applied as a simple:
> [suppress_type]
> name = rte_security_ops
>
> And well, this rule is ok from my pov: this rte_security_ops struct
> does not change in size.
> An application is not supposed to know about its content (that is
> defined in a driver header) and all accesses to those ops are supposed
> to be through symbols from the security library.
> So I would go with this "larger" and simpler rule.
The intent was to make it specific to MACsec APIs only.
I am ok with both as these are internal thing and would change it in next release.
Updated the v3 as per your suggestion.
>
>
> Just a small addition to this, as discussed offlist, this is going to
> be reworked in v23.11 and this rule on rte_security_ops will be
> unneccesary, so please move it in the relevant block (at the end) of
> the libabigail.abignore file.
Ack
@@ -37,6 +37,13 @@
[suppress_type]
type_kind = enum
changed_enumerators = RTE_CRYPTO_ASYM_XFORM_ECPM, RTE_CRYPTO_ASYM_XFORM_TYPE_LIST_END
+; Ignore changes to rte_security_ops MACsec APIs which are experimental
+[suppress_type]
+ name = rte_security_ops
+ has_data_member_inserted_between =
+ {
+ offset_of(security_macsec_sc_create_t), offset_of(security_macsec_sa_stats_get_t)
+ }
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
; Temporary exceptions till next major ABI version ;
@@ -164,13 +164,14 @@ rte_security_macsec_sa_create(struct rte_security_ctx *instance,
}
int
-rte_security_macsec_sc_destroy(struct rte_security_ctx *instance, uint16_t sc_id)
+rte_security_macsec_sc_destroy(struct rte_security_ctx *instance, uint16_t sc_id,
+ enum rte_security_macsec_direction dir)
{
int ret;
RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, macsec_sc_destroy, -EINVAL, -ENOTSUP);
- ret = instance->ops->macsec_sc_destroy(instance->device, sc_id);
+ ret = instance->ops->macsec_sc_destroy(instance->device, sc_id, dir);
if (ret != 0)
return ret;
@@ -181,13 +182,14 @@ rte_security_macsec_sc_destroy(struct rte_security_ctx *instance, uint16_t sc_id
}
int
-rte_security_macsec_sa_destroy(struct rte_security_ctx *instance, uint16_t sa_id)
+rte_security_macsec_sa_destroy(struct rte_security_ctx *instance, uint16_t sa_id,
+ enum rte_security_macsec_direction dir)
{
int ret;
RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, macsec_sa_destroy, -EINVAL, -ENOTSUP);
- ret = instance->ops->macsec_sa_destroy(instance->device, sa_id);
+ ret = instance->ops->macsec_sa_destroy(instance->device, sa_id, dir);
if (ret != 0)
return ret;
@@ -199,22 +201,24 @@ rte_security_macsec_sa_destroy(struct rte_security_ctx *instance, uint16_t sa_id
int
rte_security_macsec_sc_stats_get(struct rte_security_ctx *instance, uint16_t sc_id,
+ enum rte_security_macsec_direction dir,
struct rte_security_macsec_sc_stats *stats)
{
RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, macsec_sc_stats_get, -EINVAL, -ENOTSUP);
RTE_PTR_OR_ERR_RET(stats, -EINVAL);
- return instance->ops->macsec_sc_stats_get(instance->device, sc_id, stats);
+ return instance->ops->macsec_sc_stats_get(instance->device, sc_id, dir, stats);
}
int
rte_security_macsec_sa_stats_get(struct rte_security_ctx *instance, uint16_t sa_id,
+ enum rte_security_macsec_direction dir,
struct rte_security_macsec_sa_stats *stats)
{
RTE_PTR_CHAIN3_OR_ERR_RET(instance, ops, macsec_sa_stats_get, -EINVAL, -ENOTSUP);
RTE_PTR_OR_ERR_RET(stats, -EINVAL);
- return instance->ops->macsec_sa_stats_get(instance->device, sa_id, stats);
+ return instance->ops->macsec_sa_stats_get(instance->device, sa_id, dir, stats);
}
int
@@ -761,6 +761,7 @@ rte_security_macsec_sc_create(struct rte_security_ctx *instance,
*
* @param instance security instance
* @param sc_id SC ID to be destroyed
+ * @param dir direction of the SC
* @return
* - 0 if successful.
* - -EINVAL if sc_id is invalid or instance is NULL.
@@ -768,7 +769,8 @@ rte_security_macsec_sc_create(struct rte_security_ctx *instance,
*/
__rte_experimental
int
-rte_security_macsec_sc_destroy(struct rte_security_ctx *instance, uint16_t sc_id);
+rte_security_macsec_sc_destroy(struct rte_security_ctx *instance, uint16_t sc_id,
+ enum rte_security_macsec_direction dir);
/**
* @warning
@@ -798,6 +800,7 @@ rte_security_macsec_sa_create(struct rte_security_ctx *instance,
*
* @param instance security instance
* @param sa_id SA ID to be destroyed
+ * @param dir direction of the SA
* @return
* - 0 if successful.
* - -EINVAL if sa_id is invalid or instance is NULL.
@@ -805,7 +808,8 @@ rte_security_macsec_sa_create(struct rte_security_ctx *instance,
*/
__rte_experimental
int
-rte_security_macsec_sa_destroy(struct rte_security_ctx *instance, uint16_t sa_id);
+rte_security_macsec_sa_destroy(struct rte_security_ctx *instance, uint16_t sa_id,
+ enum rte_security_macsec_direction dir);
/** Device-specific metadata field type */
typedef uint64_t rte_security_dynfield_t;
@@ -1077,6 +1081,7 @@ rte_security_session_stats_get(struct rte_security_ctx *instance,
*
* @param instance security instance
* @param sa_id SA ID for which stats are needed
+ * @param dir direction of the SA
* @param stats statistics
* @return
* - On success, return 0.
@@ -1085,7 +1090,7 @@ rte_security_session_stats_get(struct rte_security_ctx *instance,
__rte_experimental
int
rte_security_macsec_sa_stats_get(struct rte_security_ctx *instance,
- uint16_t sa_id,
+ uint16_t sa_id, enum rte_security_macsec_direction dir,
struct rte_security_macsec_sa_stats *stats);
/**
@@ -1096,6 +1101,7 @@ rte_security_macsec_sa_stats_get(struct rte_security_ctx *instance,
*
* @param instance security instance
* @param sc_id SC ID for which stats are needed
+ * @param dir direction of the SC
* @param stats SC statistics
* @return
* - On success, return 0.
@@ -1104,7 +1110,7 @@ rte_security_macsec_sa_stats_get(struct rte_security_ctx *instance,
__rte_experimental
int
rte_security_macsec_sc_stats_get(struct rte_security_ctx *instance,
- uint16_t sc_id,
+ uint16_t sc_id, enum rte_security_macsec_direction dir,
struct rte_security_macsec_sc_stats *stats);
/**
@@ -106,8 +106,10 @@ typedef int (*security_macsec_sc_create_t)(void *device, struct rte_security_mac
*
* @param device Crypto/eth device pointer
* @param sc_id MACsec SC ID
+ * @param dir Direction of SC
*/
-typedef int (*security_macsec_sc_destroy_t)(void *device, uint16_t sc_id);
+typedef int (*security_macsec_sc_destroy_t)(void *device, uint16_t sc_id,
+ enum rte_security_macsec_direction dir);
/**
* Configure a MACsec security Association (SA) on a device.
@@ -128,8 +130,10 @@ typedef int (*security_macsec_sa_create_t)(void *device, struct rte_security_mac
*
* @param device Crypto/eth device pointer
* @param sa_id MACsec SA ID
+ * @param dir Direction of SA
*/
-typedef int (*security_macsec_sa_destroy_t)(void *device, uint16_t sa_id);
+typedef int (*security_macsec_sa_destroy_t)(void *device, uint16_t sa_id,
+ enum rte_security_macsec_direction dir);
/**
* Get the size of a security session
@@ -162,6 +166,7 @@ typedef int (*security_session_stats_get_t)(void *device,
*
* @param device Crypto/eth device pointer
* @param sc_id secure channel ID created by rte_security_macsec_sc_create()
+ * @param dir direction of SC
* @param stats SC stats of the driver
*
* @return
@@ -169,6 +174,7 @@ typedef int (*security_session_stats_get_t)(void *device,
* - -EINVAL if sc_id or device is invalid.
*/
typedef int (*security_macsec_sc_stats_get_t)(void *device, uint16_t sc_id,
+ enum rte_security_macsec_direction dir,
struct rte_security_macsec_sc_stats *stats);
/**
@@ -176,6 +182,7 @@ typedef int (*security_macsec_sc_stats_get_t)(void *device, uint16_t sc_id,
*
* @param device Crypto/eth device pointer
* @param sa_id secure channel ID created by rte_security_macsec_sc_create()
+ * @param dir direction of SA
* @param stats SC stats of the driver
*
* @return
@@ -183,6 +190,7 @@ typedef int (*security_macsec_sc_stats_get_t)(void *device, uint16_t sc_id,
* - -EINVAL if sa_id or device is invalid.
*/
typedef int (*security_macsec_sa_stats_get_t)(void *device, uint16_t sa_id,
+ enum rte_security_macsec_direction dir,
struct rte_security_macsec_sa_stats *stats);