diff mbox series

[v3,09/15] crypto/mlx5: adjust to the multiple data unit API

Message ID 20210504210857.3398397-10-matan@nvidia.com (mailing list archive)
State Changes Requested, archived
Delegated to: akhil goyal
Headers show
Series drivers: introduce mlx5 crypto PMD | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Matan Azrad May 4, 2021, 9:08 p.m. UTC
From: Shiri Kuzin <shirik@nvidia.com>

In AES-XTS the data to be encrypted\decrypted does not have to be
in multiples of 16B size, the unit of data is called data-unit.

As a result of patch [1] a new field is added to the cipher capability,
called dataunit_set, where the devices can report the range of
supported data-unit sizes.

The new field enables saving the data-unit size in the session
structure to the block size pointer variable in order to support
several data-unit sizes.

[1] https://www.mail-archive.com/dev@dpdk.org/msg205337.html

Signed-off-by: Shiri Kuzin <shirik@nvidia.com>
---
 drivers/crypto/mlx5/mlx5_crypto.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

Comments

Akhil Goyal May 8, 2021, 12:27 p.m. UTC | #1
> From: Shiri Kuzin <shirik@nvidia.com>
> 
> In AES-XTS the data to be encrypted\decrypted does not have to be
> in multiples of 16B size, the unit of data is called data-unit.
> 
> As a result of patch [1] a new field is added to the cipher capability,
> called dataunit_set, where the devices can report the range of
> supported data-unit sizes.
> 
> The new field enables saving the data-unit size in the session
> structure to the block size pointer variable in order to support
> several data-unit sizes.
> 
> [1] https://www.mail-archive.com/dev@dpdk.org/msg205337.html 
> 
Since [1] patch is already merged, no need to refer it in the patch description.
Title can be rephrased as
Crypto/mlx5: support multiple data units

Also set feature flag in the code and the documentation in this patch.
I see that you are setting all of them in a single patch in the end.
This is not correct. It should be added where the feature is supported.
Please fix this for all the feature flags.


> Signed-off-by: Shiri Kuzin <shirik@nvidia.com>
> ---
>  drivers/crypto/mlx5/mlx5_crypto.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/crypto/mlx5/mlx5_crypto.c
> b/drivers/crypto/mlx5/mlx5_crypto.c
> index 18b1a6be88..8cc29ced21 100644
> --- a/drivers/crypto/mlx5/mlx5_crypto.c
> +++ b/drivers/crypto/mlx5/mlx5_crypto.c
> @@ -48,6 +48,11 @@ struct mlx5_crypto_session {
>  	 * bsf_size, bsf_p_type, encryption_order and encryption standard,
>  	 * saved in big endian format.
>  	 */
> +	uint32_t bsp_res;
> +	/*
> +	 * crypto_block_size_pointer and reserved 24 bits saved in big endian
> +	 * format.
> +	 */
>  	uint32_t iv_offset:16;
>  	/* Starting point for Initialisation Vector. */
>  	struct mlx5_crypto_dek *dek; /* Pointer to dek struct. */
> @@ -171,6 +176,24 @@ mlx5_crypto_sym_session_configure(struct
> rte_cryptodev *dev,
>  			 MLX5_BSF_P_TYPE_CRYPTO <<
> MLX5_BSF_P_TYPE_OFFSET |
>  			 encryption_order <<
> MLX5_ENCRYPTION_ORDER_OFFSET |
>  			 MLX5_ENCRYPTION_STANDARD_AES_XTS);
> +	switch (xform->cipher.dataunit_len) {
> +	case 0:
> +		sess_private_data->bsp_res = 0;
> +		break;
> +	case 512:
> +		sess_private_data->bsp_res = rte_cpu_to_be_32
> +
> ((uint32_t)MLX5_BLOCK_SIZE_512B <<
> +					     MLX5_BLOCK_SIZE_OFFSET);
> +		break;
> +	case 4096:
> +		sess_private_data->bsp_res = rte_cpu_to_be_32
> +
> ((uint32_t)MLX5_BLOCK_SIZE_4096B <<
> +					     MLX5_BLOCK_SIZE_OFFSET);
> +		break;
> +	default:
> +		DRV_LOG(ERR, "Cipher data unit length is not supported.");
> +		return -ENOTSUP;
> +	}
>  	sess_private_data->iv_offset = cipher->iv.offset;
>  	sess_private_data->dek_id =
>  			rte_cpu_to_be_32(sess_private_data->dek->obj->id
> &
> --
> 2.25.1
Matan Azrad May 9, 2021, 8:05 a.m. UTC | #2
From: Akhil Goyal
> > From: Shiri Kuzin <shirik@nvidia.com>
> >
> > In AES-XTS the data to be encrypted\decrypted does not have to be in
> > multiples of 16B size, the unit of data is called data-unit.
> >
> > As a result of patch [1] a new field is added to the cipher
> > capability, called dataunit_set, where the devices can report the
> > range of supported data-unit sizes.
> >
> > The new field enables saving the data-unit size in the session
> > structure to the block size pointer variable in order to support
> > several data-unit sizes.
> >
> > [1] https://www.mail-archive.com/dev@dpdk.org/msg205337.html
> >
> Since [1] patch is already merged, no need to refer it in the patch description.
> Title can be rephrased as
> Crypto/mlx5: support multiple data units

Ok.

 
> Also set feature flag in the code and the documentation in this patch.
> I see that you are setting all of them in a single patch in the end.
> This is not correct. It should be added where the feature is supported.
> Please fix this for all the feature flags.

No, in this stage no feature is really supported, the actual time it will be supported is after the datapath patches and capabilities set.

> 
> > Signed-off-by: Shiri Kuzin <shirik@nvidia.com>
> > ---
> >  drivers/crypto/mlx5/mlx5_crypto.c | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >
> > diff --git a/drivers/crypto/mlx5/mlx5_crypto.c
> > b/drivers/crypto/mlx5/mlx5_crypto.c
> > index 18b1a6be88..8cc29ced21 100644
> > --- a/drivers/crypto/mlx5/mlx5_crypto.c
> > +++ b/drivers/crypto/mlx5/mlx5_crypto.c
> > @@ -48,6 +48,11 @@ struct mlx5_crypto_session {
> >        * bsf_size, bsf_p_type, encryption_order and encryption standard,
> >        * saved in big endian format.
> >        */
> > +     uint32_t bsp_res;
> > +     /*
> > +      * crypto_block_size_pointer and reserved 24 bits saved in big endian
> > +      * format.
> > +      */
> >       uint32_t iv_offset:16;
> >       /* Starting point for Initialisation Vector. */
> >       struct mlx5_crypto_dek *dek; /* Pointer to dek struct. */ @@
> > -171,6 +176,24 @@ mlx5_crypto_sym_session_configure(struct
> > rte_cryptodev *dev,
> >                        MLX5_BSF_P_TYPE_CRYPTO <<
> > MLX5_BSF_P_TYPE_OFFSET |
> >                        encryption_order <<
> > MLX5_ENCRYPTION_ORDER_OFFSET |
> >                        MLX5_ENCRYPTION_STANDARD_AES_XTS);
> > +     switch (xform->cipher.dataunit_len) {
> > +     case 0:
> > +             sess_private_data->bsp_res = 0;
> > +             break;
> > +     case 512:
> > +             sess_private_data->bsp_res = rte_cpu_to_be_32
> > +
> > ((uint32_t)MLX5_BLOCK_SIZE_512B <<
> > +                                          MLX5_BLOCK_SIZE_OFFSET);
> > +             break;
> > +     case 4096:
> > +             sess_private_data->bsp_res = rte_cpu_to_be_32
> > +
> > ((uint32_t)MLX5_BLOCK_SIZE_4096B <<
> > +                                          MLX5_BLOCK_SIZE_OFFSET);
> > +             break;
> > +     default:
> > +             DRV_LOG(ERR, "Cipher data unit length is not supported.");
> > +             return -ENOTSUP;
> > +     }
> >       sess_private_data->iv_offset = cipher->iv.offset;
> >       sess_private_data->dek_id =
> >                       rte_cpu_to_be_32(sess_private_data->dek->obj->id
> > &
> > --
> > 2.25.1
Akhil Goyal May 9, 2021, 9:19 a.m. UTC | #3
> 
> > Also set feature flag in the code and the documentation in this patch.
> > I see that you are setting all of them in a single patch in the end.
> > This is not correct. It should be added where the feature is supported.
> > Please fix this for all the feature flags.
> 
> No, in this stage no feature is really supported, the actual time it will be
> supported is after the datapath patches and capabilities set.
> 
Move the patch after adding data path, but documentation should be part
Of this patch.
Matan Azrad May 9, 2021, 2:24 p.m. UTC | #4
> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Sunday, May 9, 2021 12:19 PM
> To: Matan Azrad <matan@nvidia.com>; dev@dpdk.org
> Cc: Suanming Mou <suanmingm@nvidia.com>; NBU-Contact-Thomas
> Monjalon <thomas@monjalon.net>; Shiri Kuzin <shirik@nvidia.com>
> Subject: RE: [EXT] [PATCH v3 09/15] crypto/mlx5: adjust to the multiple data
> unit API
> 
> External email: Use caution opening links or attachments
> 
> 
> >
> > > Also set feature flag in the code and the documentation in this patch.
> > > I see that you are setting all of them in a single patch in the end.
> > > This is not correct. It should be added where the feature is supported.
> > > Please fix this for all the feature flags.
> >
> > No, in this stage no feature is really supported, the actual time it
> > will be supported is after the datapath patches and capabilities set.
> >
> Move the patch after adding data path, but documentation should be part Of
> this patch.

I will squash this patch to the session commit.
Akhil Goyal May 11, 2021, 5:34 p.m. UTC | #5
> > >
> > > > Also set feature flag in the code and the documentation in this patch.
> > > > I see that you are setting all of them in a single patch in the end.
> > > > This is not correct. It should be added where the feature is supported.
> > > > Please fix this for all the feature flags.
> > >
> > > No, in this stage no feature is really supported, the actual time it
> > > will be supported is after the datapath patches and capabilities set.
> > >
> > Move the patch after adding data path, but documentation should be part
> Of
> > this patch.
> 
> I will squash this patch to the session commit.

I am not asking you to squash the patch to the session commit.
The point is documentation patch should be part of code patch which
Introduced that feature. This methodology has been discussed in the past
and is being followed now. Please discuss this in techboard in case of deviation.

Regards,
Akhil
Matan Azrad May 12, 2021, 5:53 a.m. UTC | #6
From: Akhil Goyal
> > > > > Also set feature flag in the code and the documentation in this patch.
> > > > > I see that you are setting all of them in a single patch in the end.
> > > > > This is not correct. It should be added where the feature is supported.
> > > > > Please fix this for all the feature flags.
> > > >
> > > > No, in this stage no feature is really supported, the actual time
> > > > it will be supported is after the datapath patches and capabilities set.
> > > >
> > > Move the patch after adding data path, but documentation should be
> > > part
> > Of
> > > this patch.
> >
> > I will squash this patch to the session commit.
> 
> I am not asking you to squash the patch to the session commit.
> The point is documentation patch should be part of code patch which
> Introduced that feature. This methodology has been discussed in the past
> and is being followed now. Please discuss this in techboard in case of
> deviation.

Not sure this is mandatory when you add a new PMD.
For sure you right in general.

Anyway, the session commit is the better place to this code.
 
> Regards,
> Akhil
Akhil Goyal May 12, 2021, 6:49 a.m. UTC | #7
>  From: Akhil Goyal
> > > > > > Also set feature flag in the code and the documentation in this
> patch.
> > > > > > I see that you are setting all of them in a single patch in the end.
> > > > > > This is not correct. It should be added where the feature is
> supported.
> > > > > > Please fix this for all the feature flags.
> > > > >
> > > > > No, in this stage no feature is really supported, the actual time
> > > > > it will be supported is after the datapath patches and capabilities set.
> > > > >
> > > > Move the patch after adding data path, but documentation should be
> > > > part
> > > Of
> > > > this patch.
> > >
> > > I will squash this patch to the session commit.
> >
> > I am not asking you to squash the patch to the session commit.
> > The point is documentation patch should be part of code patch which
> > Introduced that feature. This methodology has been discussed in the past
> > and is being followed now. Please discuss this in techboard in case of
> > deviation.
> 
> Not sure this is mandatory when you add a new PMD.
In case of a new PMD also we try to follow this as much as we can.
IMO, this can be done here.

> For sure you right in general.
> 
> Anyway, the session commit is the better place to this code.
> 
> > Regards,
> > Akhil
diff mbox series

Patch

diff --git a/drivers/crypto/mlx5/mlx5_crypto.c b/drivers/crypto/mlx5/mlx5_crypto.c
index 18b1a6be88..8cc29ced21 100644
--- a/drivers/crypto/mlx5/mlx5_crypto.c
+++ b/drivers/crypto/mlx5/mlx5_crypto.c
@@ -48,6 +48,11 @@  struct mlx5_crypto_session {
 	 * bsf_size, bsf_p_type, encryption_order and encryption standard,
 	 * saved in big endian format.
 	 */
+	uint32_t bsp_res;
+	/*
+	 * crypto_block_size_pointer and reserved 24 bits saved in big endian
+	 * format.
+	 */
 	uint32_t iv_offset:16;
 	/* Starting point for Initialisation Vector. */
 	struct mlx5_crypto_dek *dek; /* Pointer to dek struct. */
@@ -171,6 +176,24 @@  mlx5_crypto_sym_session_configure(struct rte_cryptodev *dev,
 			 MLX5_BSF_P_TYPE_CRYPTO << MLX5_BSF_P_TYPE_OFFSET |
 			 encryption_order << MLX5_ENCRYPTION_ORDER_OFFSET |
 			 MLX5_ENCRYPTION_STANDARD_AES_XTS);
+	switch (xform->cipher.dataunit_len) {
+	case 0:
+		sess_private_data->bsp_res = 0;
+		break;
+	case 512:
+		sess_private_data->bsp_res = rte_cpu_to_be_32
+					     ((uint32_t)MLX5_BLOCK_SIZE_512B <<
+					     MLX5_BLOCK_SIZE_OFFSET);
+		break;
+	case 4096:
+		sess_private_data->bsp_res = rte_cpu_to_be_32
+					     ((uint32_t)MLX5_BLOCK_SIZE_4096B <<
+					     MLX5_BLOCK_SIZE_OFFSET);
+		break;
+	default:
+		DRV_LOG(ERR, "Cipher data unit length is not supported.");
+		return -ENOTSUP;
+	}
 	sess_private_data->iv_offset = cipher->iv.offset;
 	sess_private_data->dek_id =
 			rte_cpu_to_be_32(sess_private_data->dek->obj->id &