[2/2] lib/security: add SA lifetime configuration

  Add SA lifetime configuration to register soft and hard expiry limits.
Expiry can be in units of number of packets or bytes. Crypto op
status is also updated to cover warnings indicating soft expiry in case
of lookaside protocol operations.

In case of soft expiry, the packets are successfully IPsec processed but
the soft expiry would indicate that SA needs to be reconfigured. For
inline protocol capable ethdev, this would result in an eth event while
for lookaside protocol capable cryptodev, this can be communicated via
`rte_crypto_op.status` field.

In case of hard expiry, the packets will not be IPsec processed and
would result in error.

Signed-off-by: Anoob Joseph <anoobj@marvell.com>
---
 examples/ipsec-secgw/ipsec.c |  2 +-
 examples/ipsec-secgw/ipsec.h |  2 +-
 lib/cryptodev/rte_crypto.h   |  7 +++++++
 lib/security/rte_security.h  | 28 ++++++++++++++++++++++++++--
 4 files changed, 35 insertions(+), 4 deletions(-)
  

Hi Akhil, Declan, Fan, Hemant, Konstantin,

This patch & and a patch submitted by Archana earlier (http://patches.dpdk.org/project/dpdk/patch/20210630111248.746-1-marchana@marvell.com/), aims at extending rte_crypto_op so that it can be used to communicate any warnings from the rte_security offload, such as,

1. Soft expiry : application requires a notification to renegotiate SA
2. L3/L4 checksum : when application offloads checksum verification of plain packet after IPsec processing. This need not be treated as an error as IPsec operation was successful and checksum generation/verification can be redone in software, especially if the checksum operation failed due to some limitations of the underlying device.

Both the above will be an IPsec operation completed successfully but with additional information that PMD can pass on to application for indicating status of offloads.

There are two options that we considered,
1. Extend the enum, rte_crypto_op_status,  to cover warnings [1]
2. There are reserved fields in rte_cryto_op structure. So we can use bits in them to indicate various cases. [2]

Both the submitted patches follow approach 1 (following how it's done currently), but we can switch to approach 2 if we think there can be more such "warnings" that can occur simultaneously. Can you share your thoughts on how we should extend the library to handle such cases?

[1] https://doc.dpdk.org/api/rte__crypto_8h.html#afe16508b77c2a8dc5caf74a4e9850171
[2] https://doc.dpdk.org/api/rte__crypto_8h_source.html

Thanks,
Anoob

> -----Original Message-----
> From: Anoob Joseph <anoobj@marvell.com>
> Sent: Tuesday, July 20, 2021 11:16 AM
> To: Akhil Goyal <gakhil@marvell.com>; Declan Doherty
> <declan.doherty@intel.com>; Fan Zhang <roy.fan.zhang@intel.com>;
> Konstantin Ananyev <konstantin.ananyev@intel.com>
> Cc: Anoob Joseph <anoobj@marvell.com>; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>; Tejasree
> Kondoj <ktejasree@marvell.com>; dev@dpdk.org
> Subject: [PATCH 2/2] lib/security: add SA lifetime configuration
> 
> Add SA lifetime configuration to register soft and hard expiry limits.
> Expiry can be in units of number of packets or bytes. Crypto op status is also
> updated to cover warnings indicating soft expiry in case of lookaside protocol
> operations.
> 
> In case of soft expiry, the packets are successfully IPsec processed but the
> soft expiry would indicate that SA needs to be reconfigured. For inline
> protocol capable ethdev, this would result in an eth event while for lookaside
> protocol capable cryptodev, this can be communicated via
> `rte_crypto_op.status` field.
> 
> In case of hard expiry, the packets will not be IPsec processed and would
> result in error.
> 
> Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> ---
>  examples/ipsec-secgw/ipsec.c |  2 +-
>  examples/ipsec-secgw/ipsec.h |  2 +-
>  lib/cryptodev/rte_crypto.h   |  7 +++++++
>  lib/security/rte_security.h  | 28 ++++++++++++++++++++++++++--
>  4 files changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c
> index 5b032fe..4868294 100644
> --- a/examples/ipsec-secgw/ipsec.c
> +++ b/examples/ipsec-secgw/ipsec.c
> @@ -49,7 +49,7 @@ set_ipsec_conf(struct ipsec_sa *sa, struct
> rte_security_ipsec_xform *ipsec)
>  		}
>  		/* TODO support for Transport */
>  	}
> -	ipsec->esn_soft_limit = IPSEC_OFFLOAD_ESN_SOFTLIMIT;
> +	ipsec->life.packets_soft_limit = IPSEC_OFFLOAD_PKTS_SOFTLIMIT;
>  	ipsec->replay_win_sz = app_sa_prm.window_size;
>  	ipsec->options.esn = app_sa_prm.enable_esn;
>  	ipsec->options.udp_encap = sa->udp_encap; diff --git
> a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h index
> ae5058d..90c81c1 100644
> --- a/examples/ipsec-secgw/ipsec.h
> +++ b/examples/ipsec-secgw/ipsec.h
> @@ -23,7 +23,7 @@
> 
>  #define MAX_DIGEST_SIZE 32 /* Bytes -- 256 bits */
> 
> -#define IPSEC_OFFLOAD_ESN_SOFTLIMIT 0xffffff00
> +#define IPSEC_OFFLOAD_PKTS_SOFTLIMIT 0xffffff00
> 
>  #define IV_OFFSET		(sizeof(struct rte_crypto_op) + \
>  				sizeof(struct rte_crypto_sym_op))
> diff --git a/lib/cryptodev/rte_crypto.h b/lib/cryptodev/rte_crypto.h index
> fd5ef3a..c5a0897 100644
> --- a/lib/cryptodev/rte_crypto.h
> +++ b/lib/cryptodev/rte_crypto.h
> @@ -52,6 +52,13 @@ enum rte_crypto_op_status {
>  	/**< Operation failed due to invalid arguments in request */
>  	RTE_CRYPTO_OP_STATUS_ERROR,
>  	/**< Error handling operation */
> +	RTE_CRYPTO_OP_STATUS_WAR = 128,
> +	/**<
> +	 * Operation completed successfully with warnings.
> +	 * Note: All the warnings starts from here.
> +	 */
> +	RTE_CRYPTO_OPSTATUS_WAR_SOFT_EXPIRY,
> +	/**< Operation completed successfully with soft expiry of lifetime */
>  };
> 
>  /**
> diff --git a/lib/security/rte_security.h b/lib/security/rte_security.h index
> d61a55d..d633c8d 100644
> --- a/lib/security/rte_security.h
> +++ b/lib/security/rte_security.h
> @@ -206,6 +206,30 @@ enum rte_security_ipsec_sa_direction {  };
> 
>  /**
> + * Configure soft and hard lifetime of an IPsec SA
> + *
> + * Lifetime of an IPsec SA would specify the maximum number of packets
> +or bytes
> + * that can be processed. IPsec operations would start failing once any
> +hard
> + * limit is reached.
> + *
> + * Soft limits can be specified to generate notification when the SA is
> + * approaching hard limits for lifetime. For inline operations,
> +reaching soft
> + * expiry limit would result in raising an eth event for the same. For
> +lookaside
> + * operations, this would result in a warning returned in
> + * ``rte_crypto_op.status``.
> + */
> +struct rte_security_ipsec_lifetime {
> +	uint64_t packets_soft_limit;
> +	/**< Soft expiry limit in number of packets */
> +	uint64_t bytes_soft_limit;
> +	/**< Soft expiry limit in bytes */
> +	uint64_t packets_hard_limit;
> +	/**< Soft expiry limit in number of packets */
> +	uint64_t bytes_hard_limit;
> +	/**< Soft expiry limit in bytes */
> +};
> +
> +/**
>   * IPsec security association configuration data.
>   *
>   * This structure contains data required to create an IPsec SA security
> session.
> @@ -225,8 +249,8 @@ struct rte_security_ipsec_xform {
>  	/**< IPsec SA Mode - transport/tunnel */
>  	struct rte_security_ipsec_tunnel_param tunnel;
>  	/**< Tunnel parameters, NULL for transport mode */
> -	uint64_t esn_soft_limit;
> -	/**< ESN for which the overflow event need to be raised */
> +	struct rte_security_ipsec_lifetime life;
> +	/**< IPsec SA lifetime */
>  	uint32_t replay_win_sz;
>  	/**< Anti replay window size to enable sequence replay attack
> handling.
>  	 * replay checking is disabled if the window size is 0.
> --
> 2.7.4
  

Hi Anoob,

> 
> Hi Akhil, Declan, Fan, Hemant, Konstantin,
> 
> This patch & and a patch submitted by Archana earlier (http://patches.dpdk.org/project/dpdk/patch/20210630111248.746-1-
> marchana@marvell.com/), aims at extending rte_crypto_op so that it can be used to communicate any warnings from the rte_security
> offload, such as,
> 
> 1. Soft expiry : application requires a notification to renegotiate SA
> 2. L3/L4 checksum : when application offloads checksum verification of plain packet after IPsec processing. This need not be treated as an
> error as IPsec operation was successful and checksum generation/verification can be redone in software, especially if the checksum
> operation failed due to some limitations of the underlying device.
> 
> Both the above will be an IPsec operation completed successfully but with additional information that PMD can pass on to application for
> indicating status of offloads.
> 
> There are two options that we considered,
> 1. Extend the enum, rte_crypto_op_status,  to cover warnings [1]
> 2. There are reserved fields in rte_cryto_op structure. So we can use bits in them to indicate various cases. [2]
> 
> Both the submitted patches follow approach 1 (following how it's done currently), but we can switch to approach 2 if we think there can be
> more such "warnings" that can occur simultaneously. Can you share your thoughts on how we should extend the library to handle such
> cases?
> 
> [1] https://doc.dpdk.org/api/rte__crypto_8h.html#afe16508b77c2a8dc5caf74a4e9850171
> [2] https://doc.dpdk.org/api/rte__crypto_8h_source.html

My vote would probably be for option #2 (use one of the reserved fields for it).
That way - existing code wouldn't need to be changed.
Again these warnings, it probably needs to be a bit-flags, correct?
Konstantin

> Thanks,
> Anoob
> 
> > -----Original Message-----
> > From: Anoob Joseph <anoobj@marvell.com>
> > Sent: Tuesday, July 20, 2021 11:16 AM
> > To: Akhil Goyal <gakhil@marvell.com>; Declan Doherty
> > <declan.doherty@intel.com>; Fan Zhang <roy.fan.zhang@intel.com>;
> > Konstantin Ananyev <konstantin.ananyev@intel.com>
> > Cc: Anoob Joseph <anoobj@marvell.com>; Jerin Jacob Kollanukkaran
> > <jerinj@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>; Tejasree
> > Kondoj <ktejasree@marvell.com>; dev@dpdk.org
> > Subject: [PATCH 2/2] lib/security: add SA lifetime configuration
> >
> > Add SA lifetime configuration to register soft and hard expiry limits.
> > Expiry can be in units of number of packets or bytes. Crypto op status is also
> > updated to cover warnings indicating soft expiry in case of lookaside protocol
> > operations.
> >
> > In case of soft expiry, the packets are successfully IPsec processed but the
> > soft expiry would indicate that SA needs to be reconfigured. For inline
> > protocol capable ethdev, this would result in an eth event while for lookaside
> > protocol capable cryptodev, this can be communicated via
> > `rte_crypto_op.status` field.
> >
> > In case of hard expiry, the packets will not be IPsec processed and would
> > result in error.
> >
> > Signed-off-by: Anoob Joseph <anoobj@marvell.com>
> > ---
> >  examples/ipsec-secgw/ipsec.c |  2 +-
> >  examples/ipsec-secgw/ipsec.h |  2 +-
> >  lib/cryptodev/rte_crypto.h   |  7 +++++++
> >  lib/security/rte_security.h  | 28 ++++++++++++++++++++++++++--
> >  4 files changed, 35 insertions(+), 4 deletions(-)
> >
> > diff --git a/examples/ipsec-secgw/ipsec.c b/examples/ipsec-secgw/ipsec.c
> > index 5b032fe..4868294 100644
> > --- a/examples/ipsec-secgw/ipsec.c
> > +++ b/examples/ipsec-secgw/ipsec.c
> > @@ -49,7 +49,7 @@ set_ipsec_conf(struct ipsec_sa *sa, struct
> > rte_security_ipsec_xform *ipsec)
> >  		}
> >  		/* TODO support for Transport */
> >  	}
> > -	ipsec->esn_soft_limit = IPSEC_OFFLOAD_ESN_SOFTLIMIT;
> > +	ipsec->life.packets_soft_limit = IPSEC_OFFLOAD_PKTS_SOFTLIMIT;
> >  	ipsec->replay_win_sz = app_sa_prm.window_size;
> >  	ipsec->options.esn = app_sa_prm.enable_esn;
> >  	ipsec->options.udp_encap = sa->udp_encap; diff --git
> > a/examples/ipsec-secgw/ipsec.h b/examples/ipsec-secgw/ipsec.h index
> > ae5058d..90c81c1 100644
> > --- a/examples/ipsec-secgw/ipsec.h
> > +++ b/examples/ipsec-secgw/ipsec.h
> > @@ -23,7 +23,7 @@
> >
> >  #define MAX_DIGEST_SIZE 32 /* Bytes -- 256 bits */
> >
> > -#define IPSEC_OFFLOAD_ESN_SOFTLIMIT 0xffffff00
> > +#define IPSEC_OFFLOAD_PKTS_SOFTLIMIT 0xffffff00
> >
> >  #define IV_OFFSET		(sizeof(struct rte_crypto_op) + \
> >  				sizeof(struct rte_crypto_sym_op))
> > diff --git a/lib/cryptodev/rte_crypto.h b/lib/cryptodev/rte_crypto.h index
> > fd5ef3a..c5a0897 100644
> > --- a/lib/cryptodev/rte_crypto.h
> > +++ b/lib/cryptodev/rte_crypto.h
> > @@ -52,6 +52,13 @@ enum rte_crypto_op_status {
> >  	/**< Operation failed due to invalid arguments in request */
> >  	RTE_CRYPTO_OP_STATUS_ERROR,
> >  	/**< Error handling operation */
> > +	RTE_CRYPTO_OP_STATUS_WAR = 128,
> > +	/**<
> > +	 * Operation completed successfully with warnings.
> > +	 * Note: All the warnings starts from here.
> > +	 */
> > +	RTE_CRYPTO_OPSTATUS_WAR_SOFT_EXPIRY,
> > +	/**< Operation completed successfully with soft expiry of lifetime */
> >  };
> >
> >  /**
> > diff --git a/lib/security/rte_security.h b/lib/security/rte_security.h index
> > d61a55d..d633c8d 100644
> > --- a/lib/security/rte_security.h
> > +++ b/lib/security/rte_security.h
> > @@ -206,6 +206,30 @@ enum rte_security_ipsec_sa_direction {  };
> >
> >  /**
> > + * Configure soft and hard lifetime of an IPsec SA
> > + *
> > + * Lifetime of an IPsec SA would specify the maximum number of packets
> > +or bytes
> > + * that can be processed. IPsec operations would start failing once any
> > +hard
> > + * limit is reached.
> > + *
> > + * Soft limits can be specified to generate notification when the SA is
> > + * approaching hard limits for lifetime. For inline operations,
> > +reaching soft
> > + * expiry limit would result in raising an eth event for the same. For
> > +lookaside
> > + * operations, this would result in a warning returned in
> > + * ``rte_crypto_op.status``.
> > + */
> > +struct rte_security_ipsec_lifetime {
> > +	uint64_t packets_soft_limit;
> > +	/**< Soft expiry limit in number of packets */
> > +	uint64_t bytes_soft_limit;
> > +	/**< Soft expiry limit in bytes */
> > +	uint64_t packets_hard_limit;
> > +	/**< Soft expiry limit in number of packets */
> > +	uint64_t bytes_hard_limit;
> > +	/**< Soft expiry limit in bytes */
> > +};
> > +
> > +/**
> >   * IPsec security association configuration data.
> >   *
> >   * This structure contains data required to create an IPsec SA security
> > session.
> > @@ -225,8 +249,8 @@ struct rte_security_ipsec_xform {
> >  	/**< IPsec SA Mode - transport/tunnel */
> >  	struct rte_security_ipsec_tunnel_param tunnel;
> >  	/**< Tunnel parameters, NULL for transport mode */
> > -	uint64_t esn_soft_limit;
> > -	/**< ESN for which the overflow event need to be raised */
> > +	struct rte_security_ipsec_lifetime life;
> > +	/**< IPsec SA lifetime */
> >  	uint32_t replay_win_sz;
> >  	/**< Anti replay window size to enable sequence replay attack
> > handling.
> >  	 * replay checking is disabled if the window size is 0.
> > --
> > 2.7.4
  

Hi Konstantin,
> > There are two options that we considered,
> > 1. Extend the enum, rte_crypto_op_status,  to cover warnings [1]
> > 2. There are reserved fields in rte_cryto_op structure. So we can use bits in
> them to indicate various cases. [2]
> >
> > Both the submitted patches follow approach 1 (following how it's done
> currently), but we can switch to approach 2 if we think there can be
> > more such "warnings" that can occur simultaneously. Can you share your
> thoughts on how we should extend the library to handle such
> > cases?
> >
> > [1] https://doc.dpdk.org/api/rte__crypto_8h.html#afe16508b77c2a8dc5caf74a4e9850171
> > [2] https://doc.dpdk.org/api/rte__crypto_8h_source.html
> 
> My vote would probably be for option #2 (use one of the reserved fields for
> it).
> That way - existing code wouldn't need to be changed.

Adding a single enum or multiple enums is the same thing. Right wrt code changes?
However, if the check is something like 
If (status != RTE_CRYPTO_OP_STATUS_SUCCESS)
	Report appropriate error number
App code will need to be updated to take care the warnings in both options.
It will be something like
Option #1
If (status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
	If (status < RTE_CRYPTO_OP_STATUS_SUCCESS)
		Report appropriate error number.
	Else
		Report appropriate warning number probably in debug prints.
}
Option #2	
If (op->status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
	If (op->status == RTE_CRYPTO_OP_STATUS_WARNING) {
		Report appropriate warning based on op->reserved[0]
	} else {
		Report appropriate error number
	}
}
Here both the options are same wrt performance.
But in option #2, driver and app need to fill and decode 2 separate variables
As against 1 variable in option #1

In both the options, there will be similar code changes.
Do you suspect any other code change?

> Again these warnings, it probably needs to be a bit-flags, correct?

We can deal with both bit flags as well as new enums in the status.
I believe both are same and in fact using enum in application is more convenient
for user, instead of decoding bit flags.
However, it is personal choice. People may differ on that.

Regards,
Akhil
  

Hi Akhil,
 
> Hi Konstantin,
> > > There are two options that we considered,
> > > 1. Extend the enum, rte_crypto_op_status,  to cover warnings [1]
> > > 2. There are reserved fields in rte_cryto_op structure. So we can use bits in
> > them to indicate various cases. [2]
> > >
> > > Both the submitted patches follow approach 1 (following how it's done
> > currently), but we can switch to approach 2 if we think there can be
> > > more such "warnings" that can occur simultaneously. Can you share your
> > thoughts on how we should extend the library to handle such
> > > cases?
> > >
> > > [1] https://doc.dpdk.org/api/rte__crypto_8h.html#afe16508b77c2a8dc5caf74a4e9850171
> > > [2] https://doc.dpdk.org/api/rte__crypto_8h_source.html
> >
> > My vote would probably be for option #2 (use one of the reserved fields for
> > it).
> > That way - existing code wouldn't need to be changed.
> 
> Adding a single enum or multiple enums is the same thing. Right wrt code changes?
> However, if the check is something like
> If (status != RTE_CRYPTO_OP_STATUS_SUCCESS)
> 	Report appropriate error number
> App code will need to be updated to take care the warnings in both options.
> It will be something like
> Option #1
> If (status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
> 	If (status < RTE_CRYPTO_OP_STATUS_SUCCESS)
> 		Report appropriate error number.
> 	Else
> 		Report appropriate warning number probably in debug prints.
> }
> Option #2
> If (op->status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
> 	If (op->status == RTE_CRYPTO_OP_STATUS_WARNING) {
> 		Report appropriate warning based on op->reserved[0]
> 	} else {
> 		Report appropriate error number
> 	}
> }
> Here both the options are same wrt performance.
> But in option #2, driver and app need to fill and decode 2 separate variables
> As against 1 variable in option #1
> 
> In both the options, there will be similar code changes.
> Do you suspect any other code change?

Hmm, I think there is some sort of contradiction here.
From Anoob original mail:
"Both the above will be an IPsec operation completed successfully but with additional information
that PMD can pass on to application for indicating status of offloads."
So my understanding after reading Anoob mail was :
a) warnings will be set when crypto-op completed successfully, i.e:
     op->status == RTE_CRYPTO_OP_STATUS_SUCCESS
b) It is not mandatory for the application to process the warnings.
    Yes it is a recommended but still an optional.

Though from your mail it seems visa-versa:
Warnings are just some extra error codes (op->status != RTE_CRYPTO_OP_STATUS_SUCCESS)
and obviously each app have to handle them.

So could you tell me which approach did you mean?
If these 'warnings' are just new error codes and app is required to handle them,
then why do we need to introduce 'warnings' at all?
Lets treat them as error - add new  RTE_CRYPTO_OP_STATUS_ error codes for them
and that's would be it. 
 
If processing them is optional, then I think we better have a new field for them
So app code will look like:
if (op->status == RTE_CRYPTO_OP_STATUS_SUCCESS) {
    if (op->warning != 0) {
        /* handle warning conditions here */ 
    }
    /* do normal success processing */
}

In that case existing apps will be continue to work without any modifications.
Yes, they would just ignore these new warnings, but nothing will be broken.

> > Again these warnings, it probably needs to be a bit-flags, correct?
> 
> We can deal with both bit flags as well as new enums in the status.
> I believe both are same and in fact using enum in application is more convenient
> for user, instead of decoding bit flags.
> However, it is personal choice. People may differ on that.

From what I understand from previous mails: same op can have multiple warnings set.
Let say both SOFT_LIMIT can be reached and L4 checksum is not correct.
That's why I presumed that warnings have to be a bit-flag. 

Konstantin
  

Hi Konstantin,
> > > > There are two options that we considered,
> > > > 1. Extend the enum, rte_crypto_op_status,  to cover warnings [1]
> > > > 2. There are reserved fields in rte_cryto_op structure. So we can use
> bits in
> > > them to indicate various cases. [2]
> > > >
> > > > Both the submitted patches follow approach 1 (following how it's done
> > > currently), but we can switch to approach 2 if we think there can be
> > > > more such "warnings" that can occur simultaneously. Can you share
> your
> > > thoughts on how we should extend the library to handle such
> > > > cases?
> > > >
> > > > [1] https://doc.dpdk.org/api/rte__crypto_8h.html#afe16508b77c2a8dc5caf74a4e9850171
> > > > [2] https://doc.dpdk.org/api/rte__crypto_8h_source.html
> > >
> > > My vote would probably be for option #2 (use one of the reserved fields
> for
> > > it).
> > > That way - existing code wouldn't need to be changed.
> >
> > Adding a single enum or multiple enums is the same thing. Right wrt code
> changes?
> > However, if the check is something like
> > If (status != RTE_CRYPTO_OP_STATUS_SUCCESS)
> > 	Report appropriate error number
> > App code will need to be updated to take care the warnings in both
> options.
> > It will be something like
> > Option #1
> > If (status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
> > 	If (status < RTE_CRYPTO_OP_STATUS_SUCCESS)
> > 		Report appropriate error number.
> > 	Else
> > 		Report appropriate warning number probably in debug
> prints.
> > }
> > Option #2
> > If (op->status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
> > 	If (op->status == RTE_CRYPTO_OP_STATUS_WARNING) {
> > 		Report appropriate warning based on op->reserved[0]
> > 	} else {
> > 		Report appropriate error number
> > 	}
> > }
> > Here both the options are same wrt performance.
> > But in option #2, driver and app need to fill and decode 2 separate
> variables
> > As against 1 variable in option #1
> >
> > In both the options, there will be similar code changes.
> > Do you suspect any other code change?
> 
> Hmm, I think there is some sort of contradiction here.
> From Anoob original mail:
> "Both the above will be an IPsec operation completed successfully but with
> additional information
> that PMD can pass on to application for indicating status of offloads."
> So my understanding after reading Anoob mail was :
> a) warnings will be set when crypto-op completed successfully, i.e:
>      op->status == RTE_CRYPTO_OP_STATUS_SUCCESS
> b) It is not mandatory for the application to process the warnings.
>     Yes it is a recommended but still an optional.

If we set op->status = RTE_CRYPTO_OP_STATUS_SUCCESS
And then check for warnings with a separate variable there will be an
extra check for every packet even for a success case with no warning.
This may not be acceptable.
Now, if  we introduce RTE_CRYPTO_OP_STATUS_WARNING or any other warning,
Then it would mean a SUCCESS but with a specific warning which application can decide
to ignore or process. All the enum fields > RTE_CRYPTO_OP_STATUS_SUCCESS Should be
treated as success.
Status is a uint8_t which can hold 255 values, we can start the warning from say 128,
Leaving behind scope for more errors which can be added before
RTE_CRYPTO_OP_STATUS_SUCCESS

> 
> Though from your mail it seems visa-versa:
> Warnings are just some extra error codes (op->status !=
> RTE_CRYPTO_OP_STATUS_SUCCESS)
> and obviously each app have to handle them.
> 
> So could you tell me which approach did you mean?
> If these 'warnings' are just new error codes and app is required to handle
> them,
> then why do we need to introduce 'warnings' at all?
> Lets treat them as error - add new  RTE_CRYPTO_OP_STATUS_ error codes
> for them
> and that's would be it.

We cannot treat warnings as error codes. These are success cases with some
caution to inform user that there may be some issue in coming packets, eg soft expiry.
The patch that Anoob sent and the options that I specified are inline.
There may be some confusion with the wordings. I hope all your doubts gets clarified
After this mail.

> 
> If processing them is optional, then I think we better have a new field for
> them
> So app code will look like:
> if (op->status == RTE_CRYPTO_OP_STATUS_SUCCESS) {
>     if (op->warning != 0) {
>         /* handle warning conditions here */
>     }
>     /* do normal success processing */
> }
> 
> In that case existing apps will be continue to work without any modifications.
> Yes, they would just ignore these new warnings, but nothing will be broken.
> 
The existing apps can still work and but they would treat warnings as error for
the PMDs which can return these warnings. For all other PMDs, it will work as is.
But the application writer knows the features of the PMD which it is using
And hence would need to take care of the warnings eventually.
Eg: it will configure the soft/hard expiry limits while configuring the session.
Hence it will expect the warning to come.

Moreover as I said above also, there will be one extra check for each packet even
for success cases without any warning which may not be desirable.
As I suggested in both the options, the extra check will be there only in case
there is error or warning and not on the success case.

> > > Again these warnings, it probably needs to be a bit-flags, correct?
> >
> > We can deal with both bit flags as well as new enums in the status.
> > I believe both are same and in fact using enum in application is more
> convenient
> > for user, instead of decoding bit flags.
> > However, it is personal choice. People may differ on that.
> 
> From what I understand from previous mails: same op can have multiple
> warnings set.
> Let say both SOFT_LIMIT can be reached and L4 checksum is not correct.
> That's why I presumed that warnings have to be a bit-flag.

We can specify enum names to combine the possible combination of warnings.
Eg: RTE_CRYPTO_OP_STATUS_WAR_SE_L4_CSUM
  

Hi Akhil,

> > > > > There are two options that we considered,
> > > > > 1. Extend the enum, rte_crypto_op_status,  to cover warnings [1]
> > > > > 2. There are reserved fields in rte_cryto_op structure. So we can use
> > bits in
> > > > them to indicate various cases. [2]
> > > > >
> > > > > Both the submitted patches follow approach 1 (following how it's done
> > > > currently), but we can switch to approach 2 if we think there can be
> > > > > more such "warnings" that can occur simultaneously. Can you share
> > your
> > > > thoughts on how we should extend the library to handle such
> > > > > cases?
> > > > >
> > > > > [1] https://doc.dpdk.org/api/rte__crypto_8h.html#afe16508b77c2a8dc5caf74a4e9850171
> > > > > [2] https://doc.dpdk.org/api/rte__crypto_8h_source.html
> > > >
> > > > My vote would probably be for option #2 (use one of the reserved fields
> > for
> > > > it).
> > > > That way - existing code wouldn't need to be changed.
> > >
> > > Adding a single enum or multiple enums is the same thing. Right wrt code
> > changes?
> > > However, if the check is something like
> > > If (status != RTE_CRYPTO_OP_STATUS_SUCCESS)
> > > 	Report appropriate error number
> > > App code will need to be updated to take care the warnings in both
> > options.
> > > It will be something like
> > > Option #1
> > > If (status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
> > > 	If (status < RTE_CRYPTO_OP_STATUS_SUCCESS)
> > > 		Report appropriate error number.
> > > 	Else
> > > 		Report appropriate warning number probably in debug
> > prints.
> > > }
> > > Option #2
> > > If (op->status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
> > > 	If (op->status == RTE_CRYPTO_OP_STATUS_WARNING) {
> > > 		Report appropriate warning based on op->reserved[0]
> > > 	} else {
> > > 		Report appropriate error number
> > > 	}
> > > }
> > > Here both the options are same wrt performance.
> > > But in option #2, driver and app need to fill and decode 2 separate
> > variables
> > > As against 1 variable in option #1
> > >
> > > In both the options, there will be similar code changes.
> > > Do you suspect any other code change?
> >
> > Hmm, I think there is some sort of contradiction here.
> > From Anoob original mail:
> > "Both the above will be an IPsec operation completed successfully but with
> > additional information
> > that PMD can pass on to application for indicating status of offloads."
> > So my understanding after reading Anoob mail was :
> > a) warnings will be set when crypto-op completed successfully, i.e:
> >      op->status == RTE_CRYPTO_OP_STATUS_SUCCESS
> > b) It is not mandatory for the application to process the warnings.
> >     Yes it is a recommended but still an optional.
> 
> If we set op->status = RTE_CRYPTO_OP_STATUS_SUCCESS
> And then check for warnings with a separate variable there will be an
> extra check for every packet even for a success case with no warning.

Not really. warning will be within the same 32/64 bits as status.
Compilers these days are smart enough to generate code that would
read an check them as one value:
https://godbolt.org/z/M3f9891zq

> This may not be acceptable.

I don't think there would be any performance regression, see above.
If you are still nervous about possibility of this extra load, I think we can go even one step
further and reorder crypto_op fields a bit to have 'status' and 'warning'
fields consequent, and put them into one struct to make such optimizations explicit.
I.E:
union {
    uint16_t status_warning;
    struct {uint8_t status; uint8_t warning;}
}; 
 
> Now, if  we introduce RTE_CRYPTO_OP_STATUS_WARNING or any other warning,
> Then it would mean a SUCCESS but with a specific warning which application can decide
> to ignore or process. All the enum fields > RTE_CRYPTO_OP_STATUS_SUCCESS Should be
> treated as success.
> Status is a uint8_t which can hold 255 values, we can start the warning from say 128,
> Leaving behind scope for more errors which can be added before
> RTE_CRYPTO_OP_STATUS_SUCCESS
> 
> >
> > Though from your mail it seems visa-versa:
> > Warnings are just some extra error codes (op->status !=
> > RTE_CRYPTO_OP_STATUS_SUCCESS)
> > and obviously each app have to handle them.
> >
> > So could you tell me which approach did you mean?
> > If these 'warnings' are just new error codes and app is required to handle
> > them,
> > then why do we need to introduce 'warnings' at all?
> > Lets treat them as error - add new  RTE_CRYPTO_OP_STATUS_ error codes
> > for them
> > and that's would be it.
> 
> We cannot treat warnings as error codes. These are success cases with some
> caution to inform user that there may be some issue in coming packets, eg soft expiry.
> The patch that Anoob sent and the options that I specified are inline.
> There may be some confusion with the wordings. I hope all your doubts gets clarified
> After this mail.
> 
> >
> > If processing them is optional, then I think we better have a new field for
> > them
> > So app code will look like:
> > if (op->status == RTE_CRYPTO_OP_STATUS_SUCCESS) {
> >     if (op->warning != 0) {
> >         /* handle warning conditions here */
> >     }
> >     /* do normal success processing */
> > }
> >
> > In that case existing apps will be continue to work without any modifications.
> > Yes, they would just ignore these new warnings, but nothing will be broken.
> >
> The existing apps can still work and but they would treat warnings as error for
> the PMDs which can return these warnings. 
> For all other PMDs, it will work as is.
> But the application writer knows the features of the PMD which it is using
> And hence would need to take care of the warnings eventually.
> Eg: it will configure the soft/hard expiry limits while configuring the session.
> Hence it will expect the warning to come.

So, PMD will generate warnings only when particular offloads will be enabled,  
and existing apps wouldn't need to be changed to keep working, right?
That's a good thing.
Though I still don't like the idea to implicitly re-define op->status behaviour,
depending on some offloads enable/disable. 
Warning as separate filed looks much more sane and clear to me.

> Moreover as I said above also, there will be one extra check for each packet even
> for success cases without any warning which may not be desirable.
> As I suggested in both the options, the extra check will be there only in case
> there is error or warning and not on the success case.
> 
> > > > Again these warnings, it probably needs to be a bit-flags, correct?
> > >
> > > We can deal with both bit flags as well as new enums in the status.
> > > I believe both are same and in fact using enum in application is more
> > convenient
> > > for user, instead of decoding bit flags.
> > > However, it is personal choice. People may differ on that.
> >
> > From what I understand from previous mails: same op can have multiple
> > warnings set.
> > Let say both SOFT_LIMIT can be reached and L4 checksum is not correct.
> > That's why I presumed that warnings have to be a bit-flag.
> 
> We can specify enum names to combine the possible combination of warnings.
> Eg: RTE_CRYPTO_OP_STATUS_WAR_SE_L4_CSUM

With just 2 warnings defined it is ok, but imagine in future there would be
let say 5 or 6 different warnings, and nearly all combinations will be possible.
With enum it would become a real pain.
  

Hi Konstantin,
> Hi Akhil,
> 
> > > > > My vote would probably be for option #2 (use one of the reserved
> fields
> > > for
> > > > > it).
> > > > > That way - existing code wouldn't need to be changed.
> > > >
> > > > Adding a single enum or multiple enums is the same thing. Right wrt
> code
> > > changes?
> > > > However, if the check is something like
> > > > If (status != RTE_CRYPTO_OP_STATUS_SUCCESS)
> > > > 	Report appropriate error number
> > > > App code will need to be updated to take care the warnings in both
> > > options.
> > > > It will be something like
> > > > Option #1
> > > > If (status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
> > > > 	If (status < RTE_CRYPTO_OP_STATUS_SUCCESS)
> > > > 		Report appropriate error number.
> > > > 	Else
> > > > 		Report appropriate warning number probably in debug
> > > prints.
> > > > }
> > > > Option #2
> > > > If (op->status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
> > > > 	If (op->status == RTE_CRYPTO_OP_STATUS_WARNING) {
> > > > 		Report appropriate warning based on op->reserved[0]
> > > > 	} else {
> > > > 		Report appropriate error number
> > > > 	}
> > > > }
> > > > Here both the options are same wrt performance.
> > > > But in option #2, driver and app need to fill and decode 2 separate
> > > variables
> > > > As against 1 variable in option #1
> > > >
> > > > In both the options, there will be similar code changes.
> > > > Do you suspect any other code change?
> > >
> > > Hmm, I think there is some sort of contradiction here.
> > > From Anoob original mail:
> > > "Both the above will be an IPsec operation completed successfully but
> with
> > > additional information
> > > that PMD can pass on to application for indicating status of offloads."
> > > So my understanding after reading Anoob mail was :
> > > a) warnings will be set when crypto-op completed successfully, i.e:
> > >      op->status == RTE_CRYPTO_OP_STATUS_SUCCESS
> > > b) It is not mandatory for the application to process the warnings.
> > >     Yes it is a recommended but still an optional.
> >
> > If we set op->status = RTE_CRYPTO_OP_STATUS_SUCCESS
> > And then check for warnings with a separate variable there will be an
> > extra check for every packet even for a success case with no warning.
> 
> Not really. warning will be within the same 32/64 bits as status.
> Compilers these days are smart enough to generate code that would
> read an check them as one value:
> https://godbolt.org/z/M3f9891zq
> 
> > This may not be acceptable.
> 
> I don't think there would be any performance regression, see above.
> If you are still nervous about possibility of this extra load, I think we can go
> even one step
> further and reorder crypto_op fields a bit to have 'status' and 'warning'
> fields consequent, and put them into one struct to make such optimizations
> explicit.
> I.E:
> union {
>     uint16_t status_warning;
>     struct {uint8_t status; uint8_t warning;}
> };

Yes this looks a good option and as you checked, the compiled code look
Same for both the cases, we can explore this option.
With this  union, it will be a single variable also.
The major concern I had was performance hit. But if that is not an issue,
We can work on this one.

Thanks.
  

Hi Akhil, Konstantin,

Now that we have an agreement on bitfields (hoping no one else has an objection), I would like to discuss one more topic. It is more related to checksum offload, but it's better that we discuss along with other similar items (like soft expiry).

L3 & L4 checksum can be tristate (CSUM_OK, CSUM_ERROR, CSUM_UNKOWN)

1. Application didn't request. Nothing computed. 
2. Application requested. Checksum verification success.
3. Application requested. Checksum verification failed.
4. Application requested. Checksum could not be computed (PMD limitations etc).

How would we indicate each case?

My proposal would be, let's call the field that we called "warning" as "aux_flags" (auxiliary or secondary information from the operation).

Sequence in the application would be,

if (op.status != SUCCESS) {
    /* handle errors */
}

#define RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECKSUM_COMPUTED (1 << 0)
#define RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECSUM_GOOD (1 << 1)

if (op.aux_flags & RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECKSUM_COMPUTED) {
	if (op.aux_flags & RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECSUM_GOOD)
		mbuf->l4_checksum_good = 1;
	else
		mbuf->l4_checksum_good = 0;
} else {
	if (verify_l4_checksum(mbuf) == SUCCESS) {
		mbuf->l4_checksum_good = 1;
	else
		mbuf->l4_checksum_good = 0;
}

For an application not worried about aux_flags (ex: ipsec-secgw), additional checks are not required. For applications not interested in checksum, a blind check on op.aux_flags would be enough to bail out early. For applications interested in checksum, it can follow above sequence (kinds, for demonstration purpose only). 

Would something like above fine? Or if we want to restrict additional fields for just warnings, (L4_CHECKSUM_ERROR), how would application differentiate between checksum good & checksum not computed? In that case, what should be PMDs treatment of "could not compute" v/s "computed and wrong".

For soft expiry, warning would work fine.

Thanks,
Anoob

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Wednesday, July 28, 2021 6:29 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Anoob Joseph
> <anoobj@marvell.com>; Doherty, Declan <declan.doherty@intel.com>;
> Zhang, Roy Fan <roy.fan.zhang@intel.com>; hemant.agrawal@nxp.com
> Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Ankur Dwivedi
> <adwivedi@marvell.com>; Tejasree Kondoj <ktejasree@marvell.com>;
> dev@dpdk.org; Archana Muniganti <marchana@marvell.com>
> Subject: RE: [PATCH 2/2] lib/security: add SA lifetime configuration
> 
> Hi Konstantin,
> > Hi Akhil,
> >
> > > > > > My vote would probably be for option #2 (use one of the
> > > > > > reserved
> > fields
> > > > for
> > > > > > it).
> > > > > > That way - existing code wouldn't need to be changed.
> > > > >
> > > > > Adding a single enum or multiple enums is the same thing. Right
> > > > > wrt
> > code
> > > > changes?
> > > > > However, if the check is something like If (status !=
> > > > > RTE_CRYPTO_OP_STATUS_SUCCESS)
> > > > > 	Report appropriate error number App code will need to be
> > > > > updated to take care the warnings in both
> > > > options.
> > > > > It will be something like
> > > > > Option #1
> > > > > If (status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
> > > > > 	If (status < RTE_CRYPTO_OP_STATUS_SUCCESS)
> > > > > 		Report appropriate error number.
> > > > > 	Else
> > > > > 		Report appropriate warning number probably in debug
> > > > prints.
> > > > > }
> > > > > Option #2
> > > > > If (op->status != RTE_CRYPTO_OP_STATUS_SUCCESS) {
> > > > > 	If (op->status == RTE_CRYPTO_OP_STATUS_WARNING) {
> > > > > 		Report appropriate warning based on op->reserved[0]
> > > > > 	} else {
> > > > > 		Report appropriate error number
> > > > > 	}
> > > > > }
> > > > > Here both the options are same wrt performance.
> > > > > But in option #2, driver and app need to fill and decode 2
> > > > > separate
> > > > variables
> > > > > As against 1 variable in option #1
> > > > >
> > > > > In both the options, there will be similar code changes.
> > > > > Do you suspect any other code change?
> > > >
> > > > Hmm, I think there is some sort of contradiction here.
> > > > From Anoob original mail:
> > > > "Both the above will be an IPsec operation completed successfully
> > > > but
> > with
> > > > additional information
> > > > that PMD can pass on to application for indicating status of offloads."
> > > > So my understanding after reading Anoob mail was :
> > > > a) warnings will be set when crypto-op completed successfully, i.e:
> > > >      op->status == RTE_CRYPTO_OP_STATUS_SUCCESS
> > > > b) It is not mandatory for the application to process the warnings.
> > > >     Yes it is a recommended but still an optional.
> > >
> > > If we set op->status = RTE_CRYPTO_OP_STATUS_SUCCESS And then
> check
> > > for warnings with a separate variable there will be an extra check
> > > for every packet even for a success case with no warning.
> >
> > Not really. warning will be within the same 32/64 bits as status.
> > Compilers these days are smart enough to generate code that would read
> > an check them as one value:
> > https://godbolt.org/z/M3f9891zq
> >
> > > This may not be acceptable.
> >
> > I don't think there would be any performance regression, see above.
> > If you are still nervous about possibility of this extra load, I think
> > we can go even one step further and reorder crypto_op fields a bit to
> > have 'status' and 'warning'
> > fields consequent, and put them into one struct to make such
> > optimizations explicit.
> > I.E:
> > union {
> >     uint16_t status_warning;
> >     struct {uint8_t status; uint8_t warning;} };
> 
> Yes this looks a good option and as you checked, the compiled code look
> Same for both the cases, we can explore this option.
> With this  union, it will be a single variable also.
> The major concern I had was performance hit. But if that is not an issue, We
> can work on this one.
> 
> Thanks.
>
  

Hi Anoob,

> Now that we have an agreement on bitfields (hoping no one else has an objection), I would like to discuss one more topic. It is more related
> to checksum offload, but it's better that we discuss along with other similar items (like soft expiry).
> 
> L3 & L4 checksum can be tristate (CSUM_OK, CSUM_ERROR, CSUM_UNKOWN)
> 
> 1. Application didn't request. Nothing computed.
> 2. Application requested. Checksum verification success.
> 3. Application requested. Checksum verification failed.
> 4. Application requested. Checksum could not be computed (PMD limitations etc).
> 
> How would we indicate each case?
> 
> My proposal would be, let's call the field that we called "warning" as "aux_flags" (auxiliary or secondary information from the operation).
> 
> Sequence in the application would be,
> 
> if (op.status != SUCCESS) {
>     /* handle errors */
> }
> 
> #define RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECKSUM_COMPUTED (1 << 0)
> #define RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECSUM_GOOD (1 << 1)
> 
> if (op.aux_flags & RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECKSUM_COMPUTED) {
> 	if (op.aux_flags & RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECSUM_GOOD)
> 		mbuf->l4_checksum_good = 1;
> 	else
> 		mbuf->l4_checksum_good = 0;
> } else {
> 	if (verify_l4_checksum(mbuf) == SUCCESS) {
> 		mbuf->l4_checksum_good = 1;
> 	else
> 		mbuf->l4_checksum_good = 0;
> }
> 
> For an application not worried about aux_flags (ex: ipsec-secgw), additional checks are not required. For applications not interested in
> checksum, a blind check on op.aux_flags would be enough to bail out early. For applications interested in checksum, it can follow above
> sequence (kinds, for demonstration purpose only).
> 
> Would something like above fine? Or if we want to restrict additional fields for just warnings, (L4_CHECKSUM_ERROR), how would
> application differentiate between checksum good & checksum not computed? In that case, what should be PMDs treatment of "could not
> compute" v/s "computed and wrong".

I am ok with what you suggest.
My only thought - we already have CSUM flags in mbuf itself,
so why not to use them instead to pass this information from crypto PMD to user?
That way it would be compliant with ethdev CSUM approach and no need to spend 
2 bits in 'aux_flags'.
Konstantin
  

Hi Konstantin,

> Subject: [EXT] RE: [PATCH 2/2] lib/security: add SA lifetime configuration
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hi Anoob,
> 
> > Now that we have an agreement on bitfields (hoping no one else has an
> > objection), I would like to discuss one more topic. It is more related to
> checksum offload, but it's better that we discuss along with other similar
> items (like soft expiry).
> >
> > L3 & L4 checksum can be tristate (CSUM_OK, CSUM_ERROR,
> CSUM_UNKOWN)
> >
> > 1. Application didn't request. Nothing computed.
> > 2. Application requested. Checksum verification success.
> > 3. Application requested. Checksum verification failed.
> > 4. Application requested. Checksum could not be computed (PMD
> limitations etc).
> >
> > How would we indicate each case?
> >
> > My proposal would be, let's call the field that we called "warning" as
> "aux_flags" (auxiliary or secondary information from the operation).
> >
> > Sequence in the application would be,
> >
> > if (op.status != SUCCESS) {
> >     /* handle errors */
> > }
> >
> > #define RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECKSUM_COMPUTED (1 << 0)
> #define
> > RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECSUM_GOOD (1 << 1)
> >
> > if (op.aux_flags &
> RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECKSUM_COMPUTED) {
> > 	if (op.aux_flags &
> RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECSUM_GOOD)
> > 		mbuf->l4_checksum_good = 1;
> > 	else
> > 		mbuf->l4_checksum_good = 0;
> > } else {
> > 	if (verify_l4_checksum(mbuf) == SUCCESS) {
> > 		mbuf->l4_checksum_good = 1;
> > 	else
> > 		mbuf->l4_checksum_good = 0;
> > }
> >
> > For an application not worried about aux_flags (ex: ipsec-secgw),
> > additional checks are not required. For applications not interested in
> > checksum, a blind check on op.aux_flags would be enough to bail out early.
> For applications interested in checksum, it can follow above sequence (kinds,
> for demonstration purpose only).
> >
> > Would something like above fine? Or if we want to restrict additional
> > fields for just warnings, (L4_CHECKSUM_ERROR), how would application
> > differentiate between checksum good & checksum not computed? In that
> case, what should be PMDs treatment of "could not compute" v/s
> "computed and wrong".
> 
> I am ok with what you suggest.
> My only thought - we already have CSUM flags in mbuf itself, so why not to
> use them instead to pass this information from crypto PMD to user?
> That way it would be compliant with ethdev CSUM approach and no need to
> spend
> 2 bits in 'aux_flags'.
> Konstantin

[Anoob] You are right. We do have CSUM flags in mbuf and that would fully suite our requirement here. 

Our problem was, it's called PKT_RX_ and the description text refers to RX.

/**
 * Mask of bits used to determine the status of RX IP checksum.
 * - PKT_RX_IP_CKSUM_UNKNOWN: no information about the RX IP checksum
 * - PKT_RX_IP_CKSUM_BAD: the IP checksum in the packet is wrong
 * - PKT_RX_IP_CKSUM_GOOD: the IP checksum in the packet is valid
 * - PKT_RX_IP_CKSUM_NONE: the IP checksum is not correct in the packet
 *   data, but the integrity of the IP header is verified.
 */

But if we overlook that (& may be update documentation), it's a rather great idea. We could use similar PKT_TX_* flags for requesting checksum generation with outbound operations (checksum generation for plain packet before IPsec processing).

/**
 * Offload the IP checksum in the hardware. The flag PKT_TX_IPV4 should
 * also be set by the application, although a PMD will only check
 * PKT_TX_IP_CKSUM.
 *  - fill the mbuf offload information: l2_len, l3_len
 */
#define PKT_TX_IP_CKSUM      (1ULL << 54)

/**
 * Packet is IPv4. This flag must be set when using any offload feature
 * (TSO, L3 or L4 checksum) to tell the NIC that the packet is an IPv4
 * packet. If the packet is a tunneled packet, this flag is related to
 * the inner headers.
 */
#define PKT_TX_IPV4          (1ULL << 55)

Do you think above might require some modifications to document behavior with lookaside IPsec?

Also, these flags are probably the best way for checksum for inner packet with inline IPsec. So this looks like overall better idea. Do you agree?

Thanks,
Anoob
  

Hi Anoob,

> > > Now that we have an agreement on bitfields (hoping no one else has an
> > > objection), I would like to discuss one more topic. It is more related to
> > checksum offload, but it's better that we discuss along with other similar
> > items (like soft expiry).
> > >
> > > L3 & L4 checksum can be tristate (CSUM_OK, CSUM_ERROR,
> > CSUM_UNKOWN)
> > >
> > > 1. Application didn't request. Nothing computed.
> > > 2. Application requested. Checksum verification success.
> > > 3. Application requested. Checksum verification failed.
> > > 4. Application requested. Checksum could not be computed (PMD
> > limitations etc).
> > >
> > > How would we indicate each case?
> > >
> > > My proposal would be, let's call the field that we called "warning" as
> > "aux_flags" (auxiliary or secondary information from the operation).
> > >
> > > Sequence in the application would be,
> > >
> > > if (op.status != SUCCESS) {
> > >     /* handle errors */
> > > }
> > >
> > > #define RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECKSUM_COMPUTED (1 << 0)
> > #define
> > > RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECSUM_GOOD (1 << 1)
> > >
> > > if (op.aux_flags &
> > RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECKSUM_COMPUTED) {
> > > 	if (op.aux_flags &
> > RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECSUM_GOOD)
> > > 		mbuf->l4_checksum_good = 1;
> > > 	else
> > > 		mbuf->l4_checksum_good = 0;
> > > } else {
> > > 	if (verify_l4_checksum(mbuf) == SUCCESS) {
> > > 		mbuf->l4_checksum_good = 1;
> > > 	else
> > > 		mbuf->l4_checksum_good = 0;
> > > }
> > >
> > > For an application not worried about aux_flags (ex: ipsec-secgw),
> > > additional checks are not required. For applications not interested in
> > > checksum, a blind check on op.aux_flags would be enough to bail out early.
> > For applications interested in checksum, it can follow above sequence (kinds,
> > for demonstration purpose only).
> > >
> > > Would something like above fine? Or if we want to restrict additional
> > > fields for just warnings, (L4_CHECKSUM_ERROR), how would application
> > > differentiate between checksum good & checksum not computed? In that
> > case, what should be PMDs treatment of "could not compute" v/s
> > "computed and wrong".
> >
> > I am ok with what you suggest.
> > My only thought - we already have CSUM flags in mbuf itself, so why not to
> > use them instead to pass this information from crypto PMD to user?
> > That way it would be compliant with ethdev CSUM approach and no need to
> > spend
> > 2 bits in 'aux_flags'.
> > Konstantin
> 
> [Anoob] You are right. We do have CSUM flags in mbuf and that would fully suite our requirement here.
> 
> Our problem was, it's called PKT_RX_ and the description text refers to RX.
> 
> /**
>  * Mask of bits used to determine the status of RX IP checksum.
>  * - PKT_RX_IP_CKSUM_UNKNOWN: no information about the RX IP checksum
>  * - PKT_RX_IP_CKSUM_BAD: the IP checksum in the packet is wrong
>  * - PKT_RX_IP_CKSUM_GOOD: the IP checksum in the packet is valid
>  * - PKT_RX_IP_CKSUM_NONE: the IP checksum is not correct in the packet
>  *   data, but the integrity of the IP header is verified.
>  */
> 
> But if we overlook that (& may be update documentation), it's a rather great idea. We could use similar PKT_TX_* flags for requesting
> checksum generation with outbound operations (checksum generation for plain packet before IPsec processing).
> 
> /**
>  * Offload the IP checksum in the hardware. The flag PKT_TX_IPV4 should
>  * also be set by the application, although a PMD will only check
>  * PKT_TX_IP_CKSUM.
>  *  - fill the mbuf offload information: l2_len, l3_len
>  */
> #define PKT_TX_IP_CKSUM      (1ULL << 54)
> 
> /**
>  * Packet is IPv4. This flag must be set when using any offload feature
>  * (TSO, L3 or L4 checksum) to tell the NIC that the packet is an IPv4
>  * packet. If the packet is a tunneled packet, this flag is related to
>  * the inner headers.
>  */
> #define PKT_TX_IPV4          (1ULL << 55)
> 
> Do you think above might require some modifications to document behavior with lookaside IPsec?
> 
> Also, these flags are probably the best way for checksum for inner packet with inline IPsec. So this looks like overall better idea. Do you
> agree?

Not sure I understand your proposal fully.
Yes, right now inside mbuf we have different set of flags for checksum offloads: RX and TX.
RX flags - indicate was checksum calculated/checked for incoming packet and what was the result, 
While TX flags define which CSUM calculations have to be done by HW.
Yes, I suppose same flags can be reused by crypto-dev, if it capable to implement these HW offloads.
Though not sure what changes do you think will be required inside mbuf?

Konstantin
  

Hi Konstantin,

> Subject: [EXT] RE: [PATCH 2/2] lib/security: add SA lifetime configuration
> 
> Hi Anoob,
> 
> > > > Now that we have an agreement on bitfields (hoping no one else has
> > > > an objection), I would like to discuss one more topic. It is more
> > > > related to
> > > checksum offload, but it's better that we discuss along with other
> > > similar items (like soft expiry).
> > > >
> > > > L3 & L4 checksum can be tristate (CSUM_OK, CSUM_ERROR,
> > > CSUM_UNKOWN)
> > > >
> > > > 1. Application didn't request. Nothing computed.
> > > > 2. Application requested. Checksum verification success.
> > > > 3. Application requested. Checksum verification failed.
> > > > 4. Application requested. Checksum could not be computed (PMD
> > > limitations etc).
> > > >
> > > > How would we indicate each case?
> > > >
> > > > My proposal would be, let's call the field that we called
> > > > "warning" as
> > > "aux_flags" (auxiliary or secondary information from the operation).
> > > >
> > > > Sequence in the application would be,
> > > >
> > > > if (op.status != SUCCESS) {
> > > >     /* handle errors */
> > > > }
> > > >
> > > > #define RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECKSUM_COMPUTED (1
> << 0)
> > > #define
> > > > RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECSUM_GOOD (1 << 1)
> > > >
> > > > if (op.aux_flags &
> > > RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECKSUM_COMPUTED) {
> > > > 	if (op.aux_flags &
> > > RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECSUM_GOOD)
> > > > 		mbuf->l4_checksum_good = 1;
> > > > 	else
> > > > 		mbuf->l4_checksum_good = 0;
> > > > } else {
> > > > 	if (verify_l4_checksum(mbuf) == SUCCESS) {
> > > > 		mbuf->l4_checksum_good = 1;
> > > > 	else
> > > > 		mbuf->l4_checksum_good = 0;
> > > > }
> > > >
> > > > For an application not worried about aux_flags (ex: ipsec-secgw),
> > > > additional checks are not required. For applications not
> > > > interested in checksum, a blind check on op.aux_flags would be enough
> to bail out early.
> > > For applications interested in checksum, it can follow above
> > > sequence (kinds, for demonstration purpose only).
> > > >
> > > > Would something like above fine? Or if we want to restrict
> > > > additional fields for just warnings, (L4_CHECKSUM_ERROR), how
> > > > would application differentiate between checksum good & checksum
> > > > not computed? In that
> > > case, what should be PMDs treatment of "could not compute" v/s
> > > "computed and wrong".
> > >
> > > I am ok with what you suggest.
> > > My only thought - we already have CSUM flags in mbuf itself, so why
> > > not to use them instead to pass this information from crypto PMD to
> user?
> > > That way it would be compliant with ethdev CSUM approach and no need
> > > to spend
> > > 2 bits in 'aux_flags'.
> > > Konstantin
> >
> > [Anoob] You are right. We do have CSUM flags in mbuf and that would fully
> suite our requirement here.
> >
> > Our problem was, it's called PKT_RX_ and the description text refers to RX.
> >
> > /**
> >  * Mask of bits used to determine the status of RX IP checksum.
> >  * - PKT_RX_IP_CKSUM_UNKNOWN: no information about the RX IP
> checksum
> >  * - PKT_RX_IP_CKSUM_BAD: the IP checksum in the packet is wrong
> >  * - PKT_RX_IP_CKSUM_GOOD: the IP checksum in the packet is valid
> >  * - PKT_RX_IP_CKSUM_NONE: the IP checksum is not correct in the packet
> >  *   data, but the integrity of the IP header is verified.
> >  */
> >
> > But if we overlook that (& may be update documentation), it's a rather
> > great idea. We could use similar PKT_TX_* flags for requesting checksum
> generation with outbound operations (checksum generation for plain packet
> before IPsec processing).
> >
> > /**
> >  * Offload the IP checksum in the hardware. The flag PKT_TX_IPV4
> > should
> >  * also be set by the application, although a PMD will only check
> >  * PKT_TX_IP_CKSUM.
> >  *  - fill the mbuf offload information: l2_len, l3_len  */
> > #define PKT_TX_IP_CKSUM      (1ULL << 54)
> >
> > /**
> >  * Packet is IPv4. This flag must be set when using any offload
> > feature
> >  * (TSO, L3 or L4 checksum) to tell the NIC that the packet is an IPv4
> >  * packet. If the packet is a tunneled packet, this flag is related to
> >  * the inner headers.
> >  */
> > #define PKT_TX_IPV4          (1ULL << 55)
> >
> > Do you think above might require some modifications to document
> behavior with lookaside IPsec?
> >
> > Also, these flags are probably the best way for checksum for inner
> > packet with inline IPsec. So this looks like overall better idea. Do you agree?
> 
> Not sure I understand your proposal fully.
> Yes, right now inside mbuf we have different set of flags for checksum
> offloads: RX and TX.
> RX flags - indicate was checksum calculated/checked for incoming packet and
> what was the result, While TX flags define which CSUM calculations have to
> be done by HW.
> Yes, I suppose same flags can be reused by crypto-dev, if it capable to
> implement these HW offloads.
> Though not sure what changes do you think will be required inside mbuf?

[Anoob] My concern was regarding "RX" & "TX" in the comments, which are more applicable for ethdev than cryptodev. But then, I guess it's fine this way itself.

Will submit a new version for all the proposals with some unit tests so that we can discuss if there is any ambiguity in the usage.

Appreciate your feedback and suggestions.

Thanks,
Anoob