Improvements to rte_security

-----Original Message-----
From: Anoob Joseph <[email protected]>
Sent: Tuesday, July 20, 2021 11:16 AM
To: Akhil Goyal <[email protected]>; Declan Doherty <[email protected]>; Fan Zhang <[email protected]>; Konstantin Ananyev <[email protected]>
Cc: Anoob Joseph <[email protected]>; Jerin Jacob Kollanukkaran <[email protected]>; Ankur Dwivedi <[email protected]>; Tejasree Kondoj <[email protected]>; [email protected]
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 <[email protected]>

---
 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(-)

examples/ipsec-secgw/ipsec.c +1 -1

index 5b032fe..4868294 100644
@@ -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

examples/ipsec-secgw/ipsec.h +1 -1

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

lib/cryptodev/rte_crypto.h index renamed from lib/cryptodev/rte_crypto.h +0 -0

fd5ef3a..c5a0897 100644

lib/cryptodev/rte_crypto.h +7 -0

@@ -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 */
 };
 
 /**

lib/security/rte_security.h index renamed from lib/security/rte_security.h +0 -0

d61a55d..d633c8d 100644

lib/security/rte_security.h +28 -0

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

lib/security/rte_security.h +2 -2

@@ -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 Akhil, Declan, Fan, Hemant, Konstantin,

This patch & and a patch submitted by Archana earlier (http://patches.dpdk.org/project/dpdk/patch/20210630111248.746-1-
[email protected]/), 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

Quoted 131 lines

-----Original Message-----
From: Anoob Joseph <[email protected]>
Sent: Tuesday, July 20, 2021 11:16 AM
To: Akhil Goyal <[email protected]>; Declan Doherty <[email protected]>; Fan Zhang <[email protected]>; Konstantin Ananyev <[email protected]>
Cc: Anoob Joseph <[email protected]>; Jerin Jacob Kollanukkaran <[email protected]>; Ankur Dwivedi <[email protected]>; Tejasree Kondoj <[email protected]>; [email protected]
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 <[email protected]>

---
 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(-)

examples/ipsec-secgw/ipsec.c +1 -1

index 5b032fe..4868294 100644
@@ -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

examples/ipsec-secgw/ipsec.h +1 -1

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

lib/cryptodev/rte_crypto.h index renamed from lib/cryptodev/rte_crypto.h +0 -0

fd5ef3a..c5a0897 100644

lib/cryptodev/rte_crypto.h +7 -0

@@ -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 */
 };
 
 /**

lib/security/rte_security.h index renamed from lib/security/rte_security.h +0 -0

d61a55d..d633c8d 100644

lib/security/rte_security.h +28 -0

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

lib/security/rte_security.h +2 -2

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

Quoted 3 lines

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]

Quoted 2 lines

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

Quoted 1 line

more such "warnings" that can occur simultaneously. Can you share your

thoughts on how we should extend the library to handle such

Quoted 4 lines

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?

Hi Konstantin,

Quoted 9 lines

Quoted 3 lines

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]

Quoted 2 lines

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

Quoted 1 line

more such "warnings" that can occur simultaneously. Can you share your

thoughts on how we should extend the library to handle such

Quoted 8 lines

Quoted 4 lines

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?

Quoted 1 line

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.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

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

Quoted 5 lines

Quoted 5 lines

them to indicate various cases. [2]

Quoted 2 lines

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

Quoted 1 line

more such "warnings" that can occur simultaneously. Can you share

your

Quoted 7 lines

Quoted 7 lines

thoughts on how we should extend the library to handle such

Quoted 4 lines

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

Quoted 4 lines

Quoted 2 lines

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?

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 11 lines

}
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

Quoted 4 lines

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.

Quoted 4 lines

Quoted 1 line

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

Quoted 2 lines

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.

Quoted 60 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

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

Quoted 5 lines

Quoted 5 lines

them to indicate various cases. [2]

Quoted 2 lines

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

Quoted 1 line

more such "warnings" that can occur simultaneously. Can you share

your

Quoted 7 lines

Quoted 7 lines

thoughts on how we should extend the library to handle such

Quoted 4 lines

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

Quoted 4 lines

Quoted 2 lines

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?

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 11 lines

}
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

Quoted 4 lines

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:

Quoted 3 lines

     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

Quoted 6 lines

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.

Quoted 7 lines

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.

Quoted 11 lines

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 */
}

Quoted 3 lines

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.

Quoted 8 lines

Quoted 4 lines

Quoted 1 line

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

Quoted 2 lines

for user, instead of decoding bit flags.
However, it is personal choice. People may differ on that.

Quoted 4 lines

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,

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

My vote would probably be for option #2 (use one of the reserved

fields

Quoted 5 lines

Quoted 5 lines

for

Quoted 4 lines

Quoted 2 lines

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

Quoted 26 lines

Quoted 26 lines

changes?

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 11 lines

}
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

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

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

Quoted 11 lines

Quoted 7 lines

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

Quoted 1 line

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;}
};

-----Original Message-----
From: Akhil Goyal <[email protected]>
Sent: Wednesday, July 28, 2021 6:29 PM
To: Ananyev, Konstantin <[email protected]>; Anoob Joseph <[email protected]>; Doherty, Declan <[email protected]>; Zhang, Roy Fan <[email protected]>; [email protected]
Cc: Jerin Jacob Kollanukkaran <[email protected]>; Ankur Dwivedi <[email protected]>; Tejasree Kondoj <[email protected]>; [email protected]; Archana Muniganti <[email protected]>
Subject: RE: [PATCH 2/2] lib/security: add SA lifetime configuration

Hi Konstantin,

Quoted 41 lines

Hi Akhil,

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

My vote would probably be for option #2 (use one of the reserved

fields

Quoted 6 lines

Quoted 6 lines

for

Quoted 5 lines

Quoted 2 lines

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

Quoted 29 lines

Quoted 29 lines

changes?

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 9 lines

}
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
	}
}

Quoted 3 lines

Here both the options are same wrt performance. But in option #2, driver and app need to fill and decode 2 separate

variables

Quoted 2 lines

As against 1 variable in option #1

Quoted 17 lines

Quoted 7 lines

Quoted 7 lines

Quoted 2 lines

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

Quoted 9 lines

Quoted 7 lines

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

Quoted 17 lines

Quoted 2 lines

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

Quoted 1 line

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:

Quoted 3 lines

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.

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

Subject: [EXT] RE: [PATCH 2/2] lib/security: add SA lifetime configuration

External Email

----------------------------------------------------------------------
Hi Anoob,

Quoted 2 lines

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

Quoted 2 lines

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

CSUM_UNKOWN)

Quoted 5 lines

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

Quoted 4 lines

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

Quoted 8 lines

Sequence in the application would be,

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

#define RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECKSUM_COMPUTED (1 << 0)

#define

Quoted 3 lines

RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECSUM_GOOD (1 << 1)

if (op.aux_flags &

RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECKSUM_COMPUTED) {

Quoted 1 line

if (op.aux_flags &

RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECSUM_GOOD)

Quoted 10 lines

		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;
}

Quoted 3 lines

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

Quoted 4 lines

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

Quoted 55 lines

Quoted 2 lines

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

Quoted 2 lines

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

CSUM_UNKOWN)

Quoted 5 lines

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

Quoted 4 lines

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

Quoted 8 lines

Sequence in the application would be,

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

#define RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECKSUM_COMPUTED (1 << 0)

#define

Quoted 3 lines

RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECSUM_GOOD (1 << 1)

if (op.aux_flags &

RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECKSUM_COMPUTED) {

Quoted 1 line

if (op.aux_flags &

RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECSUM_GOOD)

Quoted 10 lines

		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;
}

Quoted 3 lines

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

Quoted 4 lines

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

Quoted 7 lines

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?

Subject: [EXT] RE: [PATCH 2/2] lib/security: add SA lifetime configuration

Hi Anoob,

Quoted 21 lines

Quoted 21 lines

Quoted 3 lines

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

Quoted 2 lines

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

CSUM_UNKOWN)

Quoted 5 lines

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

Quoted 5 lines

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

Quoted 1 line

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Sequence in the application would be,

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

#define RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECKSUM_COMPUTED (1

<< 0)

Quoted 17 lines

Quoted 17 lines

#define

Quoted 3 lines

RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECSUM_GOOD (1 << 1)

if (op.aux_flags &

RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECKSUM_COMPUTED) {

Quoted 1 line

if (op.aux_flags &

RTE_SEC_IPSEC_AUX_FLAGS_L4_CHECSUM_GOOD)

Quoted 10 lines

		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;
}

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

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.

Quoted 7 lines

Quoted 7 lines

For applications interested in checksum, it can follow above sequence (kinds, for demonstration purpose only).

Quoted 5 lines

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

Quoted 6 lines

Quoted 6 lines

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?

Quoted 6 lines

Quoted 4 lines

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.

Quoted 6 lines

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

Quoted 8 lines

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

Quoted 18 lines

/**
* 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)

Quoted 1 line

Do you think above might require some modifications to document

behavior with lookaside IPsec?

Quoted 3 lines

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?