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