On 17 May 2017 at 22:29, Dmitry Eremin-Solenikov <
[email protected]> wrote:
> Balakrishna,
>
> On 17.05.2017 13:49, Balakrishna Garapati wrote:
> > dpdk pmd's require to minatain min, max, inc fields for all
> > crypto capabilitites. By making iv_len to feature like dpdk pmd's
> > it is easier to create cipher entires in odp based on key_len.
>
> Unless you'd like to express something more comlex than
> { .key_len = 128, .iv_len = 8 },
> { .key_len = 128, .iv_len = 12 },
> { .key_len = 192, .iv_len = 8 },
> { .key_len = 192, .iv_len = 12 },
> { .key_len = 256, .iv_len = 8 },
> { .key_len = 256, .iv_len = 12 },
>
> your suggestion adds more complexity.
>
instead of duplicating key_len's for different iv's I like to make a key
based array where the no.of entries can be limited by key_len index and
also limit the need to validate key_len for each entry.
>
> > Signed-off-by: Balakrishna Garapati <[email protected]>
> > ---
> > since v1: fixed comments from v1,
> > - updated the description about the patch
> > - Fixed the bug in crypto validation
> > since v2: if statement correction in validation "check_cipher_options"
> >
> > include/odp/api/spec/crypto.h | 12 +++-
> > platform/linux-generic/odp_crypto.c | 8 +--
> > .../validation/api/crypto/odp_crypto_test_inp.c | 81
> ++++++----------------
> > 3 files changed, 37 insertions(+), 64 deletions(-)
> >
> > diff --git a/include/odp/api/spec/crypto.h
> b/include/odp/api/spec/crypto.h
> > index c47d314..a1b540a 100644
> > --- a/include/odp/api/spec/crypto.h
> > +++ b/include/odp/api/spec/crypto.h
> > @@ -497,7 +497,17 @@ typedef struct odp_crypto_cipher_capability_t {
> > uint32_t key_len;
> >
> > /** IV length in bytes */
> > - uint32_t iv_len;
> > + struct {
> > + /** Minimum iv length in bytes */
> > + uint32_t min;
> > +
> > + /** Maximum iv length in bytes */
> > + uint32_t max;
> > +
> > + /** Increment of supported lengths between min and max
> > + * (in bytes) */
> > + uint32_t inc;
> > + } iv_len;
> >
> > } odp_crypto_cipher_capability_t;
> >
> > diff --git a/platform/linux-generic/odp_crypto.c
> b/platform/linux-generic/odp_crypto.c
> > index a0f3f7e..3e9b607 100644
> > --- a/platform/linux-generic/odp_crypto.c
> > +++ b/platform/linux-generic/odp_crypto.c
> > @@ -35,16 +35,16 @@
> > * Keep sorted: first by key length, then by IV length
> > */
> > static const odp_crypto_cipher_capability_t cipher_capa_des[] = {
> > -{.key_len = 24, .iv_len = 8} };
> > +{.key_len = 24, .iv_len = {.min = 8, .max = 8, .inc = 0} } };
> >
> > static const odp_crypto_cipher_capability_t cipher_capa_trides_cbc[] = {
> > -{.key_len = 24, .iv_len = 8} };
> > +{.key_len = 24, .iv_len = {.min = 8, .max = 8, .inc = 0} } };
> >
> > static const odp_crypto_cipher_capability_t cipher_capa_aes_cbc[] = {
> > -{.key_len = 16, .iv_len = 16} };
> > +{.key_len = 16, .iv_len = {.min = 16, .max = 16, .inc = 0} } };
> >
> > static const odp_crypto_cipher_capability_t cipher_capa_aes_gcm[] = {
> > -{.key_len = 16, .iv_len = 12} };
> > +{.key_len = 16, .iv_len = {.min = 12, .max = 12, .inc = 0} } };
> >
> > /*
> > * Authentication algorithm capabilities
> > diff --git a/test/common_plat/validation/api/crypto/odp_crypto_test_inp.c
> b/test/common_plat/validation/api/crypto/odp_crypto_test_inp.c
> > index 24ea493..0f98fe5 100644
> > --- a/test/common_plat/validation/api/crypto/odp_crypto_test_inp.c
> > +++ b/test/common_plat/validation/api/crypto/odp_crypto_test_inp.c
> > @@ -93,10 +93,6 @@ static void alg_test(odp_crypto_op_t op,
> > odp_crypto_op_param_t op_params;
> > uint8_t *data_addr;
> > int data_off;
> > - odp_crypto_cipher_capability_t cipher_capa[MAX_ALG_CAPA];
> > - odp_crypto_auth_capability_t auth_capa[MAX_ALG_CAPA];
> > - int num, i;
> > - int found;
> >
> > rc = odp_crypto_capability(&capa);
> > CU_ASSERT(!rc);
> > @@ -136,59 +132,6 @@ static void alg_test(odp_crypto_op_t op,
> > CU_ASSERT(!rc);
> > CU_ASSERT((~capa.auths.all_bits & capa.hw_auths.all_bits) == 0);
> >
> > - num = odp_crypto_cipher_capability(cipher_alg, cipher_capa,
> > - MAX_ALG_CAPA);
> > -
> > - if (cipher_alg != ODP_CIPHER_ALG_NULL) {
> > - CU_ASSERT(num > 0);
> > - found = 0;
> > - } else {
> > - CU_ASSERT(num == 0);
> > - found = 1;
> > - }
> > -
> > - CU_ASSERT(num <= MAX_ALG_CAPA);
> > -
> > - if (num > MAX_ALG_CAPA)
> > - num = MAX_ALG_CAPA;
> > -
> > - /* Search for the test case */
> > - for (i = 0; i < num; i++) {
> > - if (cipher_capa[i].key_len == cipher_key.length &&
> > - cipher_capa[i].iv_len == ses_iv.length) {
> > - found = 1;
> > - break;
> > - }
> > - }
> > -
> > - CU_ASSERT(found);
> > -
> > - num = odp_crypto_auth_capability(auth_alg, auth_capa,
> MAX_ALG_CAPA);
> > -
> > - if (auth_alg != ODP_AUTH_ALG_NULL) {
> > - CU_ASSERT(num > 0);
> > - found = 0;
> > - } else {
> > - CU_ASSERT(num == 0);
> > - found = 1;
> > - }
> > -
> > - CU_ASSERT(num <= MAX_ALG_CAPA);
> > -
> > - if (num > MAX_ALG_CAPA)
> > - num = MAX_ALG_CAPA;
> > -
> > - /* Search for the test case */
> > - for (i = 0; i < num; i++) {
> > - if (auth_capa[i].digest_len == digest_len &&
> > - auth_capa[i].key_len == auth_key.length) {
> > - found = 1;
> > - break;
> > - }
> > - }
> > -
> > - CU_ASSERT(found);
> > -
>
> Why did you remove auth checks?
>
> > /* Create a crypto session */
> > odp_crypto_session_param_init(&ses_params);
> > ses_params.op = op;
> > @@ -379,14 +322,34 @@ static int check_cipher_options(odp_cipher_alg_t
> cipher, uint32_t key_len,
> > {
> > int i;
> > int num;
> > + int found = 0;
> > +
> > odp_crypto_cipher_capability_t cipher_capa[MAX_ALG_CAPA];
> >
> > num = odp_crypto_cipher_capability(cipher, cipher_capa,
> MAX_ALG_CAPA);
> > CU_ASSERT_FATAL(num >= 1);
> >
> > for (i = 0; i < num; i++) {
> > - if (key_len == cipher_capa[i].key_len &&
> > - iv_len == cipher_capa[i].iv_len)
> > + uint8_t inc = cipher_capa[i].iv_len.inc;
> > + uint8_t iv_min = cipher_capa[i].iv_len.min;
> > + uint8_t iv_max = cipher_capa[i].iv_len.max;
> > +
> > + if (inc == 0) {
> > + if (iv_len == iv_min &&
> > + key_len == cipher_capa[i].key_len) {
> > + found = 1;
> > + }
> > + } else {
> > + for (uint8_t iv = iv_min; iv <= iv_max; iv = iv +
> inc) {
> > + if (iv_len == iv &&
> > + key_len == cipher_capa[i].key_len) {
> > + found = 1;
> > + break;
> > + }
> > + }
> > + }
> > +
> > + if (found == 1)
> > break;
> > }
>
> Ugh. So where is actual skip/error if not found check?
>
it comes after the loop "if (i == num) and the test is skipped before
calling alg_test() inside each individual test.
>
> Also, see, here is added complexity. Instead of a single loop over
> iv/key lengths array, you now have two nested loops with an additional
> condition. Bad idea.
>
I suppose you will have the nested loops while creating the entries
otherwise. The above loop can be even more simplified by moving the key_len
check outside of nested if's so it doesn't need to be checked for all iv
entries.
/Krishna
>
> --
> With best wishes
> Dmitry
>
