test/crypto: change cipher offset for esn vector

  In the IPSEC ESN test vector the cipher offset for encryption
should be 8 bytes which is the size of esp header.
This patch also changes the ciphertext and the digest reference
data against which the operaton result is validated.

Fixes: 699741912d71 ("test/crypto: add case for auth only trailer")

Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
---
 app/test/test_cryptodev.c                  |   8 +-
 app/test/test_cryptodev_aes_test_vectors.h | 126 ++++++++++++++---------------
 2 files changed, 67 insertions(+), 67 deletions(-)
  

Hi All,

Can you please review this patch?

Thanks
Ankur

>-----Original Message-----
>From: Ankur Dwivedi <adwivedi@marvell.com>
>Sent: Friday, June 5, 2020 2:01 PM
>To: dev@dpdk.org
>Cc: declan.doherty@intel.com; akhil.goyal@nxp.com;
>hemant.agrawal@nxp.com; vakul.garg@nxp.com; Anoob Joseph
><anoobj@marvell.com>; Ankur Dwivedi <adwivedi@marvell.com>
>Subject: [PATCH] test/crypto: change cipher offset for esn vector
>
>In the IPSEC ESN test vector the cipher offset for encryption should be 8 bytes
>which is the size of esp header.
>This patch also changes the ciphertext and the digest reference data against
>which the operaton result is validated.
>
>Fixes: 699741912d71 ("test/crypto: add case for auth only trailer")
>
>Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
>---
> app/test/test_cryptodev.c                  |   8 +-
> app/test/test_cryptodev_aes_test_vectors.h | 126 ++++++++++++++-------------
>--
> 2 files changed, 67 insertions(+), 67 deletions(-)
>
>diff --git a/app/test/test_cryptodev.c b/app/test/test_cryptodev.c index
>8f63146..c812f8c 100644
>--- a/app/test/test_cryptodev.c
>+++ b/app/test/test_cryptodev.c
>@@ -10204,7 +10204,7 @@ struct test_crypto_vector {  static const struct
>test_crypto_vector  aes128cbc_hmac_sha1_aad_test_vector = {
> 	.crypto_algo = RTE_CRYPTO_CIPHER_AES_CBC,
>-	.cipher_offset = 12,
>+	.cipher_offset = 8,
> 	.cipher_len = 496,
> 	.cipher_key = {
> 		.data = {
>@@ -10240,9 +10240,9 @@ struct test_crypto_vector {
> 	},
> 	.digest = {
> 		.data = {
>-			0x1F, 0x6A, 0xD2, 0x8B, 0x4B, 0xB3, 0xC0, 0x9E,
>-			0x86, 0x9B, 0x3A, 0xF2, 0x00, 0x5B, 0x4F, 0x08,
>-			0x62, 0x8D, 0x62, 0x65
>+			0x6D, 0xF3, 0x50, 0x79, 0x7A, 0x2A, 0xAC, 0x7F,
>+			0xA6, 0xF0, 0xC6, 0x38, 0x1F, 0xA4, 0xDD, 0x9B,
>+			0x62, 0x0F, 0xFB, 0x10
> 		},
> 		.len = 20
> 	}
>diff --git a/app/test/test_cryptodev_aes_test_vectors.h
>b/app/test/test_cryptodev_aes_test_vectors.h
>index 3c38eee..f56bbdd 100644
>--- a/app/test/test_cryptodev_aes_test_vectors.h
>+++ b/app/test/test_cryptodev_aes_test_vectors.h
>@@ -368,69 +368,69 @@
>
> static const uint8_t ciphertext512_aes128cbc_aad[] = {
> 	0x57, 0x68, 0x61, 0x74, 0x20, 0x61, 0x20, 0x6C,
>-	0x6F, 0x75, 0x73, 0x79, 0x6D, 0x70, 0xB4, 0xAD,
>-	0x09, 0x7C, 0xD7, 0x52, 0xD6, 0xF2, 0xBF, 0xD1,
>-	0x9D, 0x79, 0xC6, 0xB6, 0x8F, 0x94, 0xEB, 0xD8,
>-	0xBA, 0x5E, 0x01, 0x49, 0x7D, 0xB3, 0xC5, 0xFE,
>-	0x18, 0xF4, 0xE3, 0x60, 0x8C, 0x84, 0x68, 0x13,
>-	0x33, 0x06, 0x85, 0x60, 0xD3, 0xE7, 0x8A, 0xB5,
>-	0x23, 0xA2, 0xDE, 0x52, 0x5C, 0xB6, 0x26, 0x37,
>-	0xBB, 0x23, 0x8A, 0x38, 0x07, 0x85, 0xB6, 0x2E,
>-	0xC3, 0x69, 0x57, 0x79, 0x6B, 0xE4, 0xD7, 0x86,
>-	0x23, 0x72, 0x4C, 0x65, 0x49, 0x08, 0x1E, 0xF3,
>-	0xCC, 0x71, 0x4C, 0x45, 0x97, 0x03, 0xBC, 0xA0,
>-	0x9D, 0xF0, 0x4F, 0x5D, 0xEC, 0x40, 0x6C, 0xC6,
>-	0x52, 0xC0, 0x9D, 0x1C, 0xDC, 0x8B, 0xC2, 0xFA,
>-	0x35, 0xA7, 0x3A, 0x00, 0x04, 0x1C, 0xA6, 0x91,
>-	0x5D, 0xEB, 0x07, 0xA1, 0xB9, 0x3E, 0xD1, 0xB6,
>-	0xCA, 0x96, 0xEC, 0x71, 0xF7, 0x7D, 0xB6, 0x09,
>-	0x3D, 0x19, 0x6E, 0x75, 0x03, 0xC3, 0x1A, 0x4E,
>-	0x5B, 0x4D, 0xEA, 0xD9, 0x92, 0x96, 0x01, 0xFB,
>-	0xA3, 0xC2, 0x6D, 0xC4, 0x17, 0x6B, 0xB4, 0x3B,
>-	0x1E, 0x87, 0x54, 0x26, 0x95, 0x63, 0x07, 0x73,
>-	0xB6, 0xBA, 0x52, 0xD7, 0xA7, 0xD0, 0x9C, 0x75,
>-	0x8A, 0xCF, 0xC4, 0x3C, 0x4A, 0x55, 0x0E, 0x53,
>-	0xEC, 0xE0, 0x31, 0x51, 0xB7, 0xB7, 0xD2, 0xB4,
>-	0xF3, 0x2B, 0x70, 0x6D, 0x15, 0x9E, 0x57, 0x30,
>-	0x72, 0xE5, 0xA4, 0x71, 0x5F, 0xA4, 0xE8, 0x7C,
>-	0x46, 0x58, 0x36, 0x71, 0x91, 0x55, 0xAA, 0x99,
>-	0x3B, 0x3F, 0xF6, 0xA2, 0x9D, 0x27, 0xBF, 0xC2,
>-	0x62, 0x2C, 0x85, 0xB7, 0x51, 0xDD, 0xFD, 0x7B,
>-	0x8B, 0xB5, 0xDD, 0x2A, 0x73, 0xF8, 0x93, 0x9A,
>-	0x3F, 0xAD, 0x1D, 0xF0, 0x46, 0xD1, 0x76, 0x83,
>-	0x71, 0x4E, 0xD3, 0x0D, 0x64, 0x8C, 0xC3, 0xE6,
>-	0x03, 0xED, 0xE8, 0x53, 0x23, 0x1A, 0xC7, 0x86,
>-	0xEB, 0x87, 0xD6, 0x78, 0xF9, 0xFB, 0x9C, 0x1D,
>-	0xE7, 0x4E, 0xC0, 0x70, 0x27, 0x7A, 0x43, 0xE2,
>-	0x5D, 0xA4, 0x10, 0x40, 0xBE, 0x61, 0x0D, 0x2B,
>-	0x25, 0x08, 0x75, 0x91, 0xB5, 0x5A, 0x26, 0xC8,
>-	0x32, 0xA7, 0xC6, 0x88, 0xBF, 0x75, 0x94, 0xCC,
>-	0x58, 0xA4, 0xFE, 0x2F, 0xF7, 0x5C, 0xD2, 0x36,
>-	0x66, 0x55, 0xF0, 0xEA, 0xF5, 0x64, 0x43, 0xE7,
>-	0x6D, 0xE0, 0xED, 0xA1, 0x10, 0x0A, 0x84, 0x07,
>-	0x11, 0x88, 0xFA, 0xA1, 0xD3, 0xA0, 0x00, 0x5D,
>-	0xEB, 0xB5, 0x62, 0x01, 0x72, 0xC1, 0x9B, 0x39,
>-	0x0B, 0xD3, 0xAF, 0x04, 0x19, 0x42, 0xEC, 0xFF,
>-	0x4B, 0xB3, 0x5E, 0x87, 0x27, 0xE4, 0x26, 0x57,
>-	0x76, 0xCD, 0x36, 0x31, 0x5B, 0x94, 0x74, 0xFF,
>-	0x33, 0x91, 0xAA, 0xD1, 0x45, 0x34, 0xC2, 0x11,
>-	0xF0, 0x35, 0x44, 0xC9, 0xD5, 0xA2, 0x5A, 0xC2,
>-	0xE9, 0x9E, 0xCA, 0xE2, 0x6F, 0xD2, 0x40, 0xB4,
>-	0x93, 0x42, 0x78, 0x20, 0x92, 0x88, 0xC7, 0x16,
>-	0xCF, 0x15, 0x54, 0x7B, 0xE1, 0x46, 0x38, 0x69,
>-	0xB8, 0xE4, 0xF1, 0x81, 0xF0, 0x08, 0x6F, 0x92,
>-	0x6D, 0x1A, 0xD9, 0x93, 0xFA, 0xD7, 0x35, 0xFE,
>-	0x7F, 0x59, 0x43, 0x1D, 0x3A, 0x3B, 0xFC, 0xD0,
>-	0x14, 0x95, 0x1E, 0xB2, 0x04, 0x08, 0x4F, 0xC6,
>-	0xEA, 0xE8, 0x22, 0xF3, 0xD7, 0x66, 0x93, 0xAA,
>-	0xFD, 0xA0, 0xFE, 0x03, 0x96, 0x54, 0x78, 0x35,
>-	0x18, 0xED, 0xB7, 0x2F, 0x40, 0xE3, 0x8E, 0x22,
>-	0xC6, 0xDA, 0xB0, 0x8E, 0xA0, 0xA1, 0x62, 0x03,
>-	0x63, 0x34, 0x11, 0xF5, 0x9E, 0xAA, 0x6B, 0xC4,
>-	0x14, 0x75, 0x4C, 0xF4, 0xD8, 0xD9, 0xF1, 0x76,
>-	0xE3, 0xD3, 0x55, 0xCE, 0x22, 0x7D, 0x4A, 0xB7,
>-	0xBB, 0x7F, 0x4F, 0x09, 0x88, 0x70, 0x6E, 0x09,
>-	0x84, 0x6B, 0x24, 0x19, 0x2C, 0x20, 0x73, 0x75
>+	0x1D, 0x7C, 0x76, 0xED, 0xC2, 0x10, 0x3C, 0xB5,
>+	0x14, 0x07, 0x3C, 0x33, 0x7B, 0xBE, 0x9E, 0xA9,
>+	0x01, 0xC5, 0xAA, 0xA6, 0xB6, 0x7A, 0xE1, 0xDB,
>+	0x39, 0xAA, 0xAA, 0xF4, 0xEE, 0xA7, 0x71, 0x71,
>+	0x78, 0x0D, 0x5A, 0xD4, 0xF9, 0xCD, 0x75, 0xD1,
>+	0x9C, 0x7F, 0xC8, 0x58, 0x46, 0x7A, 0xD1, 0x81,
>+	0xEA, 0xCC, 0x08, 0xDC, 0x82, 0x73, 0x22, 0x08,
>+	0x11, 0x73, 0x7C, 0xB1, 0x84, 0x6A, 0x8E, 0x67,
>+	0x3F, 0x5D, 0xDB, 0x0E, 0xE2, 0xC2, 0xCB, 0x6D,
>+	0x88, 0xEC, 0x3F, 0x50, 0x44, 0xD3, 0x47, 0x6E,
>+	0xDD, 0x42, 0xDC, 0x2A, 0x5E, 0x5C, 0x50, 0x24,
>+	0x57, 0x8A, 0xE7, 0xC5, 0x53, 0x6D, 0x89, 0x33,
>+	0x21, 0x65, 0x82, 0xD6, 0xE9, 0xE7, 0x77, 0x10,
>+	0xC2, 0x09, 0x91, 0xC1, 0x42, 0x62, 0x36, 0xF4,
>+	0x43, 0x37, 0x95, 0xB3, 0x7E, 0x21, 0xC5, 0x3E,
>+	0x65, 0xCB, 0xB6, 0xAA, 0xEC, 0xA5, 0xC6, 0x5C,
>+	0x4D, 0xBE, 0x14, 0xF1, 0x98, 0xBF, 0x6C, 0x8A,
>+	0x9E, 0x9F, 0xD4, 0xB4, 0xF2, 0x22, 0x96, 0x99,
>+	0x37, 0x32, 0xB6, 0xC1, 0x04, 0x66, 0x52, 0x37,
>+	0x5D, 0x5F, 0x58, 0x92, 0xC9, 0x97, 0xEA, 0x60,
>+	0x60, 0x27, 0x57, 0xF9, 0x47, 0x4F, 0xBC, 0xDF,
>+	0x05, 0xBD, 0x37, 0x87, 0xBB, 0x09, 0xA5, 0xBE,
>+	0xC1, 0xFC, 0x32, 0x86, 0x6A, 0xB7, 0x8B, 0x1E,
>+	0x6B, 0xCE, 0x8D, 0x81, 0x63, 0x4C, 0xF2, 0x7F,
>+	0xD1, 0x45, 0x82, 0xE8, 0x0D, 0x1C, 0x4D, 0xA8,
>+	0xBF, 0x2D, 0x2B, 0x52, 0xE5, 0xDB, 0xAB, 0xFD,
>+	0x04, 0xA2, 0xA1, 0x1E, 0x21, 0x1D, 0x06, 0x9A,
>+	0xC2, 0x7D, 0x99, 0xFC, 0xB4, 0x72, 0x89, 0x41,
>+	0x55, 0x69, 0xFA, 0x1F, 0x78, 0x2F, 0x35, 0x59,
>+	0xD7, 0x59, 0x6D, 0xA6, 0x45, 0xC9, 0x2B, 0x06,
>+	0x6C, 0xEC, 0x83, 0x34, 0xA5, 0x08, 0xDB, 0x6F,
>+	0xDE, 0x75, 0x21, 0x9B, 0xB0, 0xCB, 0x0A, 0xAE,
>+	0x22, 0x99, 0x74, 0x1C, 0x9D, 0x37, 0x0E, 0xC6,
>+	0x3A, 0x45, 0x49, 0xE5, 0xE3, 0x21, 0x11, 0xEA,
>+	0x34, 0x25, 0xD5, 0x76, 0xB0, 0x30, 0x19, 0x87,
>+	0x14, 0x3A, 0x10, 0x6F, 0x6D, 0xDD, 0xE9, 0x60,
>+	0x6A, 0x00, 0x6A, 0x4C, 0x5B, 0x85, 0x3E, 0x1A,
>+	0x41, 0xFA, 0xDE, 0x2D, 0x2F, 0x2E, 0x5B, 0x79,
>+	0x09, 0x66, 0x65, 0xD0, 0xDB, 0x32, 0x05, 0xB5,
>+	0xEA, 0xFB, 0x6A, 0xD5, 0x43, 0xF8, 0xBD, 0x98,
>+	0x7B, 0x8E, 0x3B, 0x85, 0x89, 0x5D, 0xC5, 0x59,
>+	0x54, 0x22, 0x75, 0xA8, 0x60, 0xDC, 0x0A, 0x37,
>+	0x8C, 0xD8, 0x05, 0xEA, 0x62, 0x62, 0x71, 0x98,
>+	0x0C, 0xCB, 0xCE, 0x0A, 0xD9, 0xE6, 0xE8, 0xA7,
>+	0xB3, 0x2D, 0x89, 0xA7, 0x60, 0xF0, 0x42, 0xA7,
>+	0x3D, 0x80, 0x44, 0xE7, 0xC1, 0xA6, 0x88, 0xB1,
>+	0x4F, 0xC0, 0xB1, 0xAF, 0x40, 0xF3, 0x54, 0x72,
>+	0x8F, 0xAF, 0x47, 0x96, 0x19, 0xEB, 0xA5, 0x5C,
>+	0x00, 0x3B, 0x36, 0xC8, 0x3F, 0x1E, 0x63, 0x54,
>+	0xF3, 0x3D, 0x85, 0x44, 0x9B, 0x9B, 0x20, 0xE3,
>+	0x9D, 0xEF, 0x62, 0x21, 0xA1, 0x0B, 0x78, 0xF4,
>+	0x2B, 0x89, 0x66, 0x5E, 0x97, 0xC6, 0xC4, 0x55,
>+	0x35, 0x32, 0xD7, 0x44, 0x95, 0x9A, 0xE7, 0xF2,
>+	0x57, 0x52, 0x5B, 0x92, 0x86, 0x8F, 0x8B, 0xCF,
>+	0x41, 0x89, 0xF6, 0x2A, 0xD3, 0x42, 0x87, 0x43,
>+	0x56, 0x1F, 0x0E, 0x49, 0xF1, 0x32, 0x6D, 0xA8,
>+	0x62, 0xDF, 0x47, 0xBB, 0xB6, 0x53, 0xF8, 0x5C,
>+	0x36, 0xDA, 0x34, 0x34, 0x2D, 0x2E, 0x1D, 0x33,
>+	0xAF, 0x6A, 0x1E, 0xF1, 0xC9, 0x72, 0xB5, 0x3C,
>+	0x64, 0x4C, 0x96, 0x12, 0x78, 0x67, 0x6A, 0xE5,
>+	0x8B, 0x05, 0x80, 0xAE, 0x7D, 0xE5, 0x9B, 0x24,
>+	0xDB, 0xFF, 0x1E, 0xB8, 0x36, 0x6D, 0x3D, 0x5D,
>+	0x73, 0x65, 0x72, 0x73, 0x2C, 0x20, 0x73, 0x75
> };
>
> /* AES128-CTR-SHA1 test vector */
>--
>1.9.3
  

Hi Ankur,
> 
> In the IPSEC ESN test vector the cipher offset for encryption
> should be 8 bytes which is the size of esp header.
> This patch also changes the ciphertext and the digest reference
> data against which the operaton result is validated.
> 
> Fixes: 699741912d71 ("test/crypto: add case for auth only trailer")
> 
> Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
> ---

Why do we need this change?
The existing test case is to demonstrate a generic case where we can
have an auth only trailer as well. It is similar to a case of IPSEC ESN
but not exactly IPSec. Cipher offset can be anything as per the app
requirement.
I don't think there is anything wrong in the vector. It should pass in
every hardware without any issue.

Regards,
Akhil
  

Hi Akhil,

Please see my reply inline.

Thanks
Ankur

>-----Original Message-----
>From: Akhil Goyal <akhil.goyal@nxp.com>
>Sent: Monday, July 6, 2020 5:54 PM
>To: Ankur Dwivedi <adwivedi@marvell.com>; dev@dpdk.org
>Cc: declan.doherty@intel.com; Hemant Agrawal
><hemant.agrawal@nxp.com>; Vakul Garg <vakul.garg@nxp.com>; Anoob
>Joseph <anoobj@marvell.com>
>Subject: [EXT] RE: [PATCH] test/crypto: change cipher offset for esn vector
>
>External Email
>
>----------------------------------------------------------------------
>Hi Ankur,
>>
>> In the IPSEC ESN test vector the cipher offset for encryption should
>> be 8 bytes which is the size of esp header.
>> This patch also changes the ciphertext and the digest reference data
>> against which the operaton result is validated.
>>
>> Fixes: 699741912d71 ("test/crypto: add case for auth only trailer")
>>
>> Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
>> ---
>
>Why do we need this change?
>The existing test case is to demonstrate a generic case where we can have an
>auth only trailer as well. It is similar to a case of IPSEC ESN but not exactly
>IPSec. Cipher offset can be anything as per the app requirement.
>I don't think there is anything wrong in the vector. It should pass in every
>hardware without any issue.
[Ankur] It's a limitation in OCTEON TX PMDs that the (encr offset - auth offset) should be 8 bytes aligned.
In the IPSEC ESN scenario generally the offsets will be such.
But in the above IPSEC ESN test vector, this requirement is not met and hence the associated test cases fails on the PMD.
>
>Regards,
>Akhil
  

> >Why do we need this change?
> >The existing test case is to demonstrate a generic case where we can have an
> >auth only trailer as well. It is similar to a case of IPSEC ESN but not exactly
> >IPSec. Cipher offset can be anything as per the app requirement.
> >I don't think there is anything wrong in the vector. It should pass in every
> >hardware without any issue.
> [Ankur] It's a limitation in OCTEON TX PMDs that the (encr offset - auth offset)
> should be 8 bytes aligned.
> In the IPSEC ESN scenario generally the offsets will be such.
> But in the above IPSEC ESN test vector, this requirement is not met and hence
> the associated test cases fails on the PMD.

In that case, I think it is better to have a separate test vector and both should
be executed. With the previous one as not supported in your case and this
one will be supported.
  

Hi Akhil,

Please see my reply inline.

Thanks
Ankur

>-----Original Message-----
>From: Akhil Goyal <akhil.goyal@nxp.com>
>Sent: Tuesday, July 7, 2020 12:24 AM
>To: Ankur Dwivedi <adwivedi@marvell.com>; dev@dpdk.org
>Cc: declan.doherty@intel.com; Hemant Agrawal
><hemant.agrawal@nxp.com>; Vakul Garg <vakul.garg@nxp.com>; Anoob
>Joseph <anoobj@marvell.com>
>Subject: [EXT] RE: [PATCH] test/crypto: change cipher offset for esn vector
>
>External Email
>
>----------------------------------------------------------------------
>> >Why do we need this change?
>> >The existing test case is to demonstrate a generic case where we can
>> >have an auth only trailer as well. It is similar to a case of IPSEC
>> >ESN but not exactly IPSec. Cipher offset can be anything as per the app
>requirement.
>> >I don't think there is anything wrong in the vector. It should pass
>> >in every hardware without any issue.
>> [Ankur] It's a limitation in OCTEON TX PMDs that the (encr offset -
>> auth offset) should be 8 bytes aligned.
>> In the IPSEC ESN scenario generally the offsets will be such.
>> But in the above IPSEC ESN test vector, this requirement is not met
>> and hence the associated test cases fails on the PMD.
>
>In that case, I think it is better to have a separate test vector and both should
>be executed. With the previous one as not supported in your case and this
>one will be supported.
[Ankur] The offsets values are present per crypto operation. So to make these tests
as unsupported the pmd datapath needs to be changed.
Is there an alternative to make these tests unsupported?
  

> >> >Why do we need this change?
> >> >The existing test case is to demonstrate a generic case where we can
> >> >have an auth only trailer as well. It is similar to a case of IPSEC
> >> >ESN but not exactly IPSec. Cipher offset can be anything as per the app
> >requirement.
> >> >I don't think there is anything wrong in the vector. It should pass
> >> >in every hardware without any issue.
> >> [Ankur] It's a limitation in OCTEON TX PMDs that the (encr offset -
> >> auth offset) should be 8 bytes aligned.
> >> In the IPSEC ESN scenario generally the offsets will be such.
> >> But in the above IPSEC ESN test vector, this requirement is not met
> >> and hence the associated test cases fails on the PMD.
> >
> >In that case, I think it is better to have a separate test vector and both should
> >be executed. With the previous one as not supported in your case and this
> >one will be supported.
> [Ankur] The offsets values are present per crypto operation. So to make these
> tests
> as unsupported the pmd datapath needs to be changed.
> Is there an alternative to make these tests unsupported?

I believe this is a data path error and a limitation in your PMD.
You can not stop the application writer from using unaligned cipher offsets(non-8 byte aligned)

This is just a test application, which may hide your PMD limitation by accepting this patch
But in actual the scenario will fail when some user configures a 12B cipher offset.
IMO, you should add a new test vector instead of replacing this one and it should
Be OK to have the existing one fail in your case.
  

Hi Akhil, 

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Wednesday, July 8, 2020 12:51 AM
> To: Ankur Dwivedi <adwivedi@marvell.com>; dev@dpdk.org
> Cc: declan.doherty@intel.com; Hemant Agrawal
> <hemant.agrawal@nxp.com>; Vakul Garg <vakul.garg@nxp.com>; Anoob
> Joseph <anoobj@marvell.com>
> Subject: [EXT] RE: [PATCH] test/crypto: change cipher offset for esn vector
> 
> External Email
> 
> ----------------------------------------------------------------------
> 
> > >> >Why do we need this change?
> > >> >The existing test case is to demonstrate a generic case where we
> > >> >can have an auth only trailer as well. It is similar to a case of
> > >> >IPSEC ESN but not exactly IPSec. Cipher offset can be anything as
> > >> >per the app
> > >requirement.
> > >> >I don't think there is anything wrong in the vector. It should
> > >> >pass in every hardware without any issue.
> > >> [Ankur] It's a limitation in OCTEON TX PMDs that the (encr offset -
> > >> auth offset) should be 8 bytes aligned.
> > >> In the IPSEC ESN scenario generally the offsets will be such.
> > >> But in the above IPSEC ESN test vector, this requirement is not met
> > >> and hence the associated test cases fails on the PMD.
> > >
> > >In that case, I think it is better to have a separate test vector and
> > >both should be executed. With the previous one as not supported in
> > >your case and this one will be supported.
> > [Ankur] The offsets values are present per crypto operation. So to
> > make these tests as unsupported the pmd datapath needs to be changed.
> > Is there an alternative to make these tests unsupported?
> 
> I believe this is a data path error and a limitation in your PMD.
> You can not stop the application writer from using unaligned cipher
> offsets(non-8 byte aligned)

[Anoob] Yes. But the typical case with IPsec is what is supported in the PMD. 

> 
> This is just a test application, which may hide your PMD limitation by
> accepting this patch But in actual the scenario will fail when some user
> configures a 12B cipher offset.

[Anoob] Agreed. But autotest having a failure is not an ideal situation to be it. Especially when it's not the typical usage. Can I propose to add a field like "Know Issues:" in the summary field? We can add a check for OCTEONTX PMDs in the test case and mark it as a known case. I do understand that the vision was to remove all driver specific tests and have generic tests for all PMDs, but here we are left with no other option. Chances are, other PMDs also could have similar limitations when moving to generic framework.

If you have suggestions to skip this test in any other way, that would also work for us.
 
> IMO, you should add a new test vector instead of replacing this one and it
> should Be OK to have the existing one fail in your case.
>
  

Hi Akhil,

Did you get a chance to look at my comments?

Thanks,
Anoob

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Anoob Joseph
> Sent: Wednesday, July 8, 2020 3:21 PM
> To: Akhil Goyal <akhil.goyal@nxp.com>; Ankur Dwivedi
> <adwivedi@marvell.com>; dev@dpdk.org
> Cc: declan.doherty@intel.com; Hemant Agrawal
> <hemant.agrawal@nxp.com>; Vakul Garg <vakul.garg@nxp.com>
> Subject: Re: [dpdk-dev] [PATCH] test/crypto: change cipher offset for esn
> vector
> 
> Hi Akhil,
> 
> Please see inline.
> 
> Thanks,
> Anoob
> 
> > -----Original Message-----
> > From: Akhil Goyal <akhil.goyal@nxp.com>
> > Sent: Wednesday, July 8, 2020 12:51 AM
> > To: Ankur Dwivedi <adwivedi@marvell.com>; dev@dpdk.org
> > Cc: declan.doherty@intel.com; Hemant Agrawal
> <hemant.agrawal@nxp.com>;
> > Vakul Garg <vakul.garg@nxp.com>; Anoob Joseph <anoobj@marvell.com>
> > Subject: [EXT] RE: [PATCH] test/crypto: change cipher offset for esn
> > vector
> >
> > External Email
> >
> > ----------------------------------------------------------------------
> >
> > > >> >Why do we need this change?
> > > >> >The existing test case is to demonstrate a generic case where we
> > > >> >can have an auth only trailer as well. It is similar to a case
> > > >> >of IPSEC ESN but not exactly IPSec. Cipher offset can be
> > > >> >anything as per the app
> > > >requirement.
> > > >> >I don't think there is anything wrong in the vector. It should
> > > >> >pass in every hardware without any issue.
> > > >> [Ankur] It's a limitation in OCTEON TX PMDs that the (encr offset
> > > >> - auth offset) should be 8 bytes aligned.
> > > >> In the IPSEC ESN scenario generally the offsets will be such.
> > > >> But in the above IPSEC ESN test vector, this requirement is not
> > > >> met and hence the associated test cases fails on the PMD.
> > > >
> > > >In that case, I think it is better to have a separate test vector
> > > >and both should be executed. With the previous one as not supported
> > > >in your case and this one will be supported.
> > > [Ankur] The offsets values are present per crypto operation. So to
> > > make these tests as unsupported the pmd datapath needs to be
> changed.
> > > Is there an alternative to make these tests unsupported?
> >
> > I believe this is a data path error and a limitation in your PMD.
> > You can not stop the application writer from using unaligned cipher
> > offsets(non-8 byte aligned)
> 
> [Anoob] Yes. But the typical case with IPsec is what is supported in the PMD.
> 
> >
> > This is just a test application, which may hide your PMD limitation by
> > accepting this patch But in actual the scenario will fail when some
> > user configures a 12B cipher offset.
> 
> [Anoob] Agreed. But autotest having a failure is not an ideal situation to be it.
> Especially when it's not the typical usage. Can I propose to add a field like
> "Know Issues:" in the summary field? We can add a check for OCTEONTX
> PMDs in the test case and mark it as a known case. I do understand that the
> vision was to remove all driver specific tests and have generic tests for all
> PMDs, but here we are left with no other option. Chances are, other PMDs
> also could have similar limitations when moving to generic framework.
> 
> If you have suggestions to skip this test in any other way, that would also
> work for us.
> 
> > IMO, you should add a new test vector instead of replacing this one
> > and it should Be OK to have the existing one fail in your case.
> >
  

> >
> > > >> >Why do we need this change?
> > > >> >The existing test case is to demonstrate a generic case where we
> > > >> >can have an auth only trailer as well. It is similar to a case of
> > > >> >IPSEC ESN but not exactly IPSec. Cipher offset can be anything as
> > > >> >per the app
> > > >requirement.
> > > >> >I don't think there is anything wrong in the vector. It should
> > > >> >pass in every hardware without any issue.
> > > >> [Ankur] It's a limitation in OCTEON TX PMDs that the (encr offset -
> > > >> auth offset) should be 8 bytes aligned.
> > > >> In the IPSEC ESN scenario generally the offsets will be such.
> > > >> But in the above IPSEC ESN test vector, this requirement is not met
> > > >> and hence the associated test cases fails on the PMD.
> > > >
> > > >In that case, I think it is better to have a separate test vector and
> > > >both should be executed. With the previous one as not supported in
> > > >your case and this one will be supported.
> > > [Ankur] The offsets values are present per crypto operation. So to
> > > make these tests as unsupported the pmd datapath needs to be changed.
> > > Is there an alternative to make these tests unsupported?
> >
> > I believe this is a data path error and a limitation in your PMD.
> > You can not stop the application writer from using unaligned cipher
> > offsets(non-8 byte aligned)
> 
> [Anoob] Yes. But the typical case with IPsec is what is supported in the PMD.
> 
> >
> > This is just a test application, which may hide your PMD limitation by
> > accepting this patch But in actual the scenario will fail when some user
> > configures a 12B cipher offset.
> 
> [Anoob] Agreed. But autotest having a failure is not an ideal situation to be it.
> Especially when it's not the typical usage. Can I propose to add a field like "Know
> Issues:" in the summary field? We can add a check for OCTEONTX PMDs in the
> test case and mark it as a known case. I do understand that the vision was to
> remove all driver specific tests and have generic tests for all PMDs, but here we
> are left with no other option. Chances are, other PMDs also could have similar
> limitations when moving to generic framework.
> 
> If you have suggestions to skip this test in any other way, that would also work
> for us.
> 

I don't see there is a straight forward way for skipping this test, but we cannot allow
PMD specific checks going further.
If you are not ready to accept this failure case, I am ok to take this patch as is.
I believe it will hide your PMD limitation and may come up
As a surprise to your customers if they start using non-8 byte aligned cipher offset.

Applied to dpdk-next-crypto

Thanks.

> > IMO, you should add a new test vector instead of replacing this one and it
> > should Be OK to have the existing one fail in your case.
> >