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

[De Lara Guarch, Pablo]

> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal at nxp.com]
> Sent: Thursday, May 04, 2017 12:23 PM
> To: Gonzalez Monroy, Sergio; De Lara Guarch, Pablo; dev at dpdk.org
> Cc: Doherty, Declan; hemant.agrawal at nxp.com;
> zbigniew.bodek at caviumnetworks.com; jerin.jacob at caviumnetworks.com
> Subject: Re: [PATCH] [RFC] cryptodev: crypto operation restructuring
> 
> 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?

Hi Akhil,

Thanks for your comments!

No, AAD can be anywhere, as we are passing a pointer to the data:

union {
          struct {
                    uint32_t offset;
                    uint32_t length;
          } data;
         struct {
                   uint32_t length;
                   uint8_t *data;
                   phys_addr_t phys_addr;
        } aad;
};

We don't need auth data offset/length, because data to authenticate and cipher
is passed on cipher structure, plus the aad structure for additional data to authenticate.

Another proposal would be creating an structure in that crypto operation for AEAD algorithms,
and have the union with that structure and the other two structures (auth and cipher).
In this case, it is more explicit that AAD cannot be used in authentication algorithms such as SHA1.

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 {
                uint32_t offset;
        } iv;

        union {
                struct {
                        struct {
                                uint32_t offset;
                                uint32_t length;
                        } data;
		/**< Data offset and length for encryption/authentication */
                        struct {
                                uint32_t length;        
                                uint8_t *data;
                                phys_addr_t phys_addr; 
                        } aad;
                        /**< Additional authentication parameters */
                        struct {
                                uint8_t *data;
                                phys_addr_t phys_addr;
                        } digest;
		/**< Digest parameters (length is fixed in session) */
                } aead;

                struct {
                        struct {
                                struct {
                                        uint32_t offset;
                                        uint32_t length;
                                } data;
		    /**< Data offset and length for authentication */
                                struct {
                                        uint8_t *data;
                                        phys_addr_t phys_addr;
                                } digest;
		    /**< Digest parameters (length is fixed in session) */
                        } auth;

                        struct {
                                struct {
                                        uint32_t offset;
                                        uint32_t length; 
                                } data;
		    /**< Data offset and length for ciphering */
                        } cipher;
                };
        };

        __extension__ char _private[0];
        /**< Private crypto operation material */
};


Thanks,
Pablo