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 <odp/visibility_begin.h>
>>
>> #include <odp/api/deprecated.h>
>> +#include <odp/api/support.h>
>>
>> #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 crypto processed packet
>> (ODP_EVENT_PACKET_CRYPTO)
>> + * @param[out] result Pointer to operation result for output
>> + *
>> + * @retval 0 On success
>> + * @retval <0 On failure
>> + */
>> +int odp_crypto_packet_result(odp_crypto_packet_op_result_t *result,
>> + odp_packet_t packet);
>> +
>> +/**
>> + * Crypto packet operation
>> + *
>> + * Performs the SYNC cryptographic operations specified during session
>> creation
>> + * on the packets. Caller should initialize pkt_out either with desired
>> output
>> + * packet handles or with ODP_PACKET_INVALID to make ODP allocate new
>> packets
>> + * from provided pool. All arrays should be of num_pkt size.
>> + *
>> + * @param pkt_in Packets to be processed
>> + * @param[in,out] pkt_out Packet handle array specifyint resulting
>> packets
>> + * @param param Operation parameters array
>> + * @param num_pkt Number of packets to be processed
>> + *
>> + * @return Number of input packets consumed (0 ... num_pkt)
>> + * @retval <0 on failure
>> + */
>> +int odp_crypto_packet_op(const odp_packet_t pkt_in[],
>> + odp_packet_t pkt_out[],
>> + const odp_crypto_packet_op_param_t
>> param[],
>> + int num_pkt);
>
>
> Again, "packet" not needed in my opinion. odp_crypto_op() ?
I wanted to show that this call is tightly connected with
odp_crypto_packet_result(), etc. Would you really like to remove
_packet_ part here?
>> +
>> +/**
>> + * Crypto packet operation
>> + *
>> + * Performs the ASYNC cryptographic operations specified during session
>> creation
>> + * on the packets. Caller should initialize pkt_out either with desired
>> output
>> + * packet handles or with ODP_PACKET_INVALID to make ODP allocate new
>> packets
>> + * from provided pool. All arrays should be of num_pkt size. Resulting
>> packets
>> + * are returned through events.
>> + *
>> + * @param pkt_in Packets to be processed
>> + * @param pkt_out Packet handle array specifying resulting packets
>> + * @param param Operation parameters array
>> + * @param num_pkt Number of packets to be processed
>> + *
>> + * @return Number of input packets consumed (0 ... num_pkt)
>> + * @retval <0 on failure
>> + */
>> +int odp_crypto_packet_op_enq(const odp_packet_t pkt_in[],
>> + const odp_packet_t pkt_out[],
>> + const odp_crypto_packet_op_param_t
>> param[],
>> + int num_pkt);
>
> odp_crypto_op_enq() ?
Same as above.
--
With best wishes
Dmitry
