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

[Sergio Gonzalez Monroy]

On 04/05/2017 07:09, Akhil Goyal wrote:
> 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.

Understood, but why is that an issue? Those fields are used by 
application code and PMD, same system.
Do you have a use case where you are offloading crypto ops to a 
different arch/system?

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