[dpdk-dev] [PATCH] [RFC] cryptodev: crypto operation restructuring

[Akhil Goyal]

Hi Sergio,

On 5/3/2017 7:48 PM, Sergio Gonzalez Monroy wrote:
> 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
>

The problem will come when we run on systems with different endianness. 
The bit field positioning will be different for LE and BE.
It would be like in LE
uint64_t type:8;
uint64_t status:8;
uint64_t sess_type:8;
uint64_t reserved:40;

and on BE it would be
uint64_t reserved:40;
uint64_t sess_type:8;
uint64_t status:8;
uint64_t type:8;

So it would be better to use uint8_t for each of the field.

>>>
>>> - 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 at intel.com>
>>> ---
>>
>> Comments inline.
>>
>> Regards,
>> Akhil
>>
>>
>
>