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

[Sergio Gonzalez Monroy]

On 04/05/2017 08:38, Akhil Goyal wrote:
> On 5/4/2017 1:01 PM, Sergio Gonzalez Monroy wrote:
>> 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
> same application may run on LE or BE machines. So if we use masks for 
> accessing these fields and take the complete field as uint64_t, then 
> LE and BE machine would interpret it differently as the code is same.
>

Right, either you use bitfields or you define macros and work on the 
complete uint64_t. The proposal here was to just use bitfields and for 
the given use case it would not be an issue while IMHO allowing better 
packing and readability than defining each field as a Byte.

Anyway, if you feel strongly against bitfields, we can just define it as 
uint8_t as you suggest or single uint64_t with macros.

Sergio

> Akhil
>>
>>>
>>>>>>
>>>>>> - 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
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>
>