[dpdk-dev] [PATCH] [RFC] cryptodev: crypto operation restructuring
Akhil Goyal
akhil.goyal at nxp.com
Thu May 4 13:23:14 CEST 2017
On 5/4/2017 1:49 PM, Sergio Gonzalez Monroy wrote:
> 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
>
I am not against bitfields. But we should take care of the endianness
using the compile time flags for byte ordering or we can use the uint8_t.
I am ok with both.
Thanks,
Akhil
>> 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.
Pablo/Sergio,
Is my understanding correct here?
Akhil
>>>>>>>
>>>>>>> - 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
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
>
>
More information about the dev
mailing list