Re: [lng-odp] [PATCH API-NEXT v5 6/10] api: crypto: add crypto packet operation interface
Petri,
Thank you for the review!
I corrected some of the issues you pointed. For the rest
(packet_op_mode, odp_crypto_packet_op()/odp_crypto_packet_op_enq()) I
would like to keep them as is, as this shows tighter connection between
API. If you'd absolutely insist on that, I can remove _packet_ parts of
function/data names.
On 03.07.2017 17:25, Savolainen, Petri (Nokia - FI/Espoo) wrote:
>
>> diff --git a/include/odp/api/spec/crypto.h b/include/odp/api/spec/crypto.h
>> index 454855ea..9dd60749 100644
>> --- a/include/odp/api/spec/crypto.h
>> +++ b/include/odp/api/spec/crypto.h
>> @@ -16,6 +16,7 @@
>> #include
>>
>> #include
>> +#include
>>
>> #ifdef __cplusplus
>> extern "C" {
>> @@ -276,6 +277,9 @@ typedef struct odp_crypto_session_param_t {
>> * data in non-posted mode */
>> odp_crypto_op_mode_t ODP_DEPRECATE(pref_mode);
>>
>> +/** Operation mode when using packet interface: sync or async
>> */
>> +odp_crypto_op_mode_t packet_op_mode;
>> +
>
> I think "packet" is not needed here, just "op_mode".
I wanted to emphasize that this only covers odp_crypto_packet_* API, not
odp_crypto_operation().
>> /** Cipher algorithm
>> *
>> * Use odp_crypto_capability() for supported algorithms.
>> @@ -311,16 +315,15 @@ typedef struct odp_crypto_session_param_t {
>>
>> /** Async mode completion event queue
>> *
>> - * When odp_crypto_operation() is asynchronous, the completion
>> queue is
>> - * used to return the completion status of the operation to
>> the
>> - * application.
>> + * The completion queue is used to return
>> odp_crypto_packet_op_enq()
>> + * results to the application.
>
> "... return results from asynchronous operation ..." is sufficient. We don't
> need to update if there will be other functions sending events into this
> queue later on.
Ack.
>
>
>> */
>> odp_queue_t compl_queue;
>>
>> /** Output pool
>> *
>> * When the output packet is not specified during the call to
>> - * odp_crypto_operation(), the output packet will be allocated
>> + * crypto operation, the output packet will be allocated
>> * from this pool.
>> */
>> odp_pool_t output_pool;
>> @@ -400,6 +403,44 @@ typedef struct odp_crypto_op_param_t {
>> typedef odp_crypto_op_param_t ODP_DEPRECATE(odp_crypto_op_params_t);
>>
>> /**
>> + * Crypto packet API per packet operation parameters
>> + */
>> +typedef struct odp_crypto_packet_op_param_t {
>> +/** Session handle from creation */
>> +odp_crypto_session_t session;
>> +
>> +/** Override session IV pointer */
>> +uint8_t *override_iv_ptr;
>> +
>> +/** Offset from start of packet for hash result
>> + *
>> + * Specifies the offset where the hash result is to be stored.
>> In case
>> + * of decode sessions, input hash values will be read from
>> this offset,
>> + * and overwritten with hash results. If this offset lies
>> within
>> + * specified 'auth_range', implementation will mute this field
>> before
>> + * calculating the hash result.
>> + */
>> +uint32_t hash_result_offset;
>> +
>> +/** Additional Authenticated Data (AAD) */
>> +struct {
>> +/** Pointer to ADD */
>> +uint8_t *ptr;
>> +
>> +/** AAD length in bytes. Use
>> odp_crypto_auth_capability() for
>> + * supported AAD lengths. */
>> +uint32_t length;
>> +} aad;
>> +
>> +/** Data range to apply cipher */
>> +odp_packet_data_range_t cipher_range;
>> +
>> +/** Data range to authenticate */
>> +odp_packet_data_range_t auth_range;
>> +
>> +} odp_crypto_packet_op_param_t;
>
> Again, I'd have it simply "odp_crypto_op_param_t", but that's reserved by old
> API. This would need some creativity to remove "packet" in this case.
I'd suggest to merge this as odp_crypto_packet_op_param_t and rename it
to just odp_crypto_op_param_t later, if we ever remove current
odp_crypto_op_param_t/odp_crypto_operation().
>>
>> /**
>> + * Crypto packet API operation result
>> + */
>> +typedef struct odp_crypto_packet_op_result_t {
>> +/** Request completed successfully */
>> +odp_bool_t ok;
>> +
>> +/** Cipher status */
>> +odp_crypto_op_status_t cipher_status;
>> +
>> +/** Authentication status */
>> +odp_crypto_op_status_t auth_status;
>> +
>> +} odp_crypto_packet_op_result_t;
>
>
> Here odp_crypto_packet_result_t would match IPsec API.
Ack
>> +
>> +/**
>> + * Get crypto operation results from an crypto processed packet
>> + *
>> + * Successful crypto operations of all types (SYNC and ASYNC) produce
>> packets
>> + * which contain crypto result metadata. This function copies the
>> operation
>> + * results from an crypto processed packet. Event subtype of this kind of
>> + * packet is ODP_EVENT_PACKET_crypto. Results are undefined if a non-
>> crypto
>> + * processed packet is passed as input.
>> + *
>> + * @param packet An
Re: [lng-odp] [PATCH API-NEXT v5 6/10] api: crypto: add crypto packet operation interface
> diff --git a/include/odp/api/spec/crypto.h b/include/odp/api/spec/crypto.h
> index 454855ea..9dd60749 100644
> --- a/include/odp/api/spec/crypto.h
> +++ b/include/odp/api/spec/crypto.h
> @@ -16,6 +16,7 @@
> #include
>
> #include
> +#include
>
> #ifdef __cplusplus
> extern "C" {
> @@ -276,6 +277,9 @@ typedef struct odp_crypto_session_param_t {
>* data in non-posted mode */
> odp_crypto_op_mode_t ODP_DEPRECATE(pref_mode);
>
> + /** Operation mode when using packet interface: sync or async
> */
> + odp_crypto_op_mode_t packet_op_mode;
> +
I think "packet" is not needed here, just "op_mode".
> /** Cipher algorithm
>*
>* Use odp_crypto_capability() for supported algorithms.
> @@ -311,16 +315,15 @@ typedef struct odp_crypto_session_param_t {
>
> /** Async mode completion event queue
>*
> - * When odp_crypto_operation() is asynchronous, the completion
> queue is
> - * used to return the completion status of the operation to
> the
> - * application.
> + * The completion queue is used to return
> odp_crypto_packet_op_enq()
> + * results to the application.
"... return results from asynchronous operation ..." is sufficient. We don't
need to update if there will be other functions sending events into this queue
later on.
>*/
> odp_queue_t compl_queue;
>
> /** Output pool
>*
>* When the output packet is not specified during the call to
> - * odp_crypto_operation(), the output packet will be allocated
> + * crypto operation, the output packet will be allocated
>* from this pool.
>*/
> odp_pool_t output_pool;
> @@ -400,6 +403,44 @@ typedef struct odp_crypto_op_param_t {
> typedef odp_crypto_op_param_t ODP_DEPRECATE(odp_crypto_op_params_t);
>
> /**
> + * Crypto packet API per packet operation parameters
> + */
> +typedef struct odp_crypto_packet_op_param_t {
> + /** Session handle from creation */
> + odp_crypto_session_t session;
> +
> + /** Override session IV pointer */
> + uint8_t *override_iv_ptr;
> +
> + /** Offset from start of packet for hash result
> + *
> + * Specifies the offset where the hash result is to be stored.
> In case
> + * of decode sessions, input hash values will be read from
> this offset,
> + * and overwritten with hash results. If this offset lies
> within
> + * specified 'auth_range', implementation will mute this field
> before
> + * calculating the hash result.
> + */
> + uint32_t hash_result_offset;
> +
> + /** Additional Authenticated Data (AAD) */
> + struct {
> + /** Pointer to ADD */
> + uint8_t *ptr;
> +
> + /** AAD length in bytes. Use
> odp_crypto_auth_capability() for
> + * supported AAD lengths. */
> + uint32_t length;
> + } aad;
> +
> + /** Data range to apply cipher */
> + odp_packet_data_range_t cipher_range;
> +
> + /** Data range to authenticate */
> + odp_packet_data_range_t auth_range;
> +
> +} odp_crypto_packet_op_param_t;
Again, I'd have it simply "odp_crypto_op_param_t", but that's reserved by old
API. This would need some creativity to remove "packet" in this case.
> +
> +/**
> * Crypto API session creation return code
> */
> typedef enum {
> @@ -444,14 +485,17 @@ typedef enum {
> /**
> * Cryto API per packet operation completion status
> */
> -typedef struct odp_crypto_compl_status {
> +typedef struct odp_crypto_op_status {
> /** Algorithm specific return code */
> odp_crypto_alg_err_t alg_err;
>
> /** Hardware specific return code */
> odp_crypto_hw_err_t hw_err;
>
> -} odp_crypto_compl_status_t;
> +} odp_crypto_op_status_t;
> +
> +/** @deprecated Use ODP_DEPRECATE(odp_crypto_op_status_t) instead */
> +typedef odp_crypto_op_status_t ODP_DEPRECATE(odp_crypto_compl_status_t);
>
> /**
> * Crypto API operation result
> @@ -460,27 +504,51 @@ typedef struct odp_crypto_op_result {
> /** Request completed successfully */
> odp_bool_t ok;
>
> - /** User context from request */
> - void *ctx;
> + /** User context from request
> + *
> + * @deprecated No need to pass context around sync calls
> + * */
> + void *ODP_DEPRECATE(ctx);
>
> /** Output packet */
> odp_packet_t pkt;
>
> /** Cipher status */
> - odp_crypto_compl_status_t cipher_status;
> + odp_crypto_op_status_t cipher_status;
>
> /** Authentication status */
> - odp_crypto_compl_status_t auth_status;
> + odp_crypto_op_status_t auth_status;
>
> } odp_crypto_op_result_t;
>
> /**
> + * Crypto packet API operation result
> + */
> +typedef struct odp_crypto_packet_op_result_t {
> + /** Request completed successfully */
> + odp_bool_t ok;
> +
> + /** Cipher status */
> + odp_crypto_op_status_t cipher_status;
> +
> + /** Authentication sta
