Re: [PATCH] [RFC] cryptodev: crypto operation restructuring

[Sergio Gonzalez Monroy <sergio.gonzalez.monroy@intel.com>]

On 03/05/2017 12:01, Akhil Goyal wrote:
> Hi Pablo,
>
> On 4/28/2017 11:33 PM, Pablo de Lara wrote:
>> This is a proposal to correct and improve the current crypto 
>> operation (rte_crypto_op)
>> and symmetric crypto operation (rte_crypto_sym_op) structures, shrinking
>> their sizes to fit both structures into two 64-byte cache lines as 
>> one of the goals.
>>
>> The following changes are proposed:
>>
>> In rte_crypto_op:
>>
>> - Move session type (with session/sessionless) from symmetric op to 
>> crypto op,
>>   as this could be used for other types
>>
>> - Combine operation type, operation status and session type into a 
>> 64-bit flag (each one taking 1 byte),
>>   instead of having enums taking 4 bytes each
> [Akhil] wouldn't this be a problem? Bit fields create endianness 
> issues. Can we have uint8_t for each of the field.

Sure, as it is proposed it would be the same as having 3 uint8_t fields. 
The idea was to possibly compact those fields (ie. we do not need 8 bits 
for sess_type) to make better use of the bits and add asym fields there 
if needed.

I don't think bitfields would be a problem in this case. Agree, we 
should not use both bitmask and bitfields, but we would use just bitfields.
Can you elaborate on the issue you see?

Regards,
Sergio

>>
>> - Remove opaque data from crypto operation, as private data can be 
>> allocated
>>   just after the symmetric (or other type) crypto operation
>>
>> - Modify symmetric operation pointer to zero-array, as the symmetric 
>> op should be always after the crypto operation
>>
>> In rte_crypto_sym_xform:
>>
>> - Remove AAD length from sym_xform (will be taken from operation only)
>>
>> - Add IV length in sym_xform, so this length will be fixed for all 
>> the operations in a session
> A much needed change. This would remove hard codings for iv length 
> while configuring sessions.
>>
>> In rte_crypto_sym_op:
>>
>> - Separate IV from cipher structure in symmetric crypto operation, as 
>> it is also used in authentication, for some algorithms
>>
>> - Remove IV pointer and length from sym crypto op, and leave just the 
>> offset (from the beginning of the crypto operation),
>>   as the IV can reside after the crypto operation
>>
>> - Create union with authentication data and AAD, as these two values 
>> cannot be used at the same time
> [Akhil] Does this mean, in case of AEAD, additional authentication 
> data and auth data are contiguous as we do not have explicit auth data 
> offset here.
>>
>> - Remove digest length from sym crypto op, so this length will be 
>> fixed for all the operations in a session
>>
>> - Add zero-array at the end of sym crypto op to be used to get extra 
>> allocated memory (IV + other user data)
>>
>> Previous rte_crypto_op (40 bytes) and rte_crypto_sym_op (114 bytes) 
>> structures:
>>
>> struct rte_crypto_op {
>>         enum rte_crypto_op_type type;
>>
>>         enum rte_crypto_op_status status;
>>
>>         struct rte_mempool *mempool;
>>
>>         phys_addr_t phys_addr;
>>
>>         void *opaque_data;
>>
>>         union {
>>                 struct rte_crypto_sym_op *sym;
>>         };
>> } __rte_cache_aligned;
>>
>> struct rte_crypto_sym_op {
>>         struct rte_mbuf *m_src;
>>         struct rte_mbuf *m_dst;
>>
>>         enum rte_crypto_sym_op_sess_type sess_type;
>>
>>         RTE_STD_C11
>>         union {
>>                 struct rte_cryptodev_sym_session *session;
>>                 struct rte_crypto_sym_xform *xform;
>>         };
>>
>>         struct {
>>                 struct {
>>                         uint32_t offset;
>>                         uint32_t length;
>>                 } data;
>>
>>                 struct {
>>                         uint8_t *data;
>>                         phys_addr_t phys_addr;
>>                         uint16_t length;
>>                 } iv;
>>         } cipher;
>>
>>         struct {
>>                 struct {
>>                         uint32_t offset;
>>                         uint32_t length;
>>                 } data;
>>                 struct {
>>                         uint8_t *data;
>>                         phys_addr_t phys_addr;
>>                         uint16_t length;
>>                 } digest; /**< Digest parameters */
>>
>>                 struct {
>>                         uint8_t *data;
>>                         phys_addr_t phys_addr;
>>                         uint16_t length;
>>                 } aad;
>>
>>         } auth;
>> } __rte_cache_aligned;
>>
>> New rte_crypto_op (24 bytes) and rte_crypto_sym_op (72 bytes) 
>> structures:
>>
>> struct rte_crypto_op {
>>         uint64_t type: 8;
>>         uint64_t status: 8;
>>         uint64_t sess_type: 8;
>>
>>         struct rte_mempool *mempool;
>>
>>         phys_addr_t phys_addr;
>>
>>         RTE_STD_C11
>>         union {
>>                 struct rte_crypto_sym_op sym[0];
>>         };
>> } __rte_cache_aligned;
>>
>> struct rte_crypto_sym_op {
>>         struct rte_mbuf *m_src;
>>         struct rte_mbuf *m_dst;
>>
>>         RTE_STD_C11
>>         union {
>>                 struct rte_cryptodev_sym_session *session;
>>                 struct rte_crypto_sym_xform *xform;
>>         };
>>
>>         struct {
>>                 uint8_t offset;
>>         } iv;
>>
>>         struct {
>>                 union {
>>                         struct {
>>                                 uint32_t offset;
>>                                 uint32_t length;
>>                         } data;
>>                         struct {
>>                                 uint32_t length;
>>                                 uint8_t *data;
>>                                 phys_addr_t phys_addr;
>>                         } aad;
>>                 };
>>
>>                 struct {
>>                         uint8_t *data;
>>                         phys_addr_t phys_addr;
>>                 } digest;
>>
>>         } auth;
>>         struct {
>>                 struct {
>>                         uint32_t offset;
>>                         uint32_t length;
>>                 } data;
>>
>>         } cipher;
>>
>>         __extension__ char _private[0];
>>        };
>>
>> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>
>> ---
>
> Comments inline.
>
> Regards,
> Akhil
>
>