[v2,3/4] examples/fips_validation: remove illegal usage of APIs
Checks
Commit Message
Some of the cryptodev APIs are not allowed to be used
by application directly. Hence removing the usage of
1. queue_pair_release: it is not required, as configure
of queue pair release the previous queue pairs and the
dev is not directly exposed to application, hence cannot
use its ops from app.
2. rte_cryptodev_stop: it can be used directly without
checking if the device is started or not.
3. rte_cryptodev_pmd_destroy: application should use
rte_cryptodev_close instead.
Signed-off-by: Akhil Goyal <gakhil@marvell.com>
---
examples/fips_validation/fips_dev_self_test.c | 19 ++-----------------
examples/fips_validation/main.c | 7 ++-----
2 files changed, 4 insertions(+), 22 deletions(-)
Comments
From: Akhil Goyal
> Some of the cryptodev APIs are not allowed to be used by application
> directly. Hence removing the usage of 1. queue_pair_release: it is not
> required, as configure
> of queue pair release the previous queue pairs and the
> dev is not directly exposed to application, hence cannot
> use its ops from app.
> 2. rte_cryptodev_stop: it can be used directly without
> checking if the device is started or not.
> 3. rte_cryptodev_pmd_destroy: application should use
> rte_cryptodev_close instead.
>
> Signed-off-by: Akhil Goyal <gakhil@marvell.com>
Look's like it should be backported to stable releases with a Fixes reference.
What do you think?
Besides,
Acked-by: Matan Azrad <matan@nvidia.com>
> ---
> examples/fips_validation/fips_dev_self_test.c | 19 ++-----------------
> examples/fips_validation/main.c | 7 ++-----
> 2 files changed, 4 insertions(+), 22 deletions(-)
>
> diff --git a/examples/fips_validation/fips_dev_self_test.c
> b/examples/fips_validation/fips_dev_self_test.c
> index 17e85973c0..b4eab05a98 100644
> --- a/examples/fips_validation/fips_dev_self_test.c
> +++ b/examples/fips_validation/fips_dev_self_test.c
> @@ -3,7 +3,7 @@
> */
>
> #include <rte_cryptodev.h>
> -#include <rte_cryptodev_pmd.h>
> +#include <rte_malloc.h>
>
> #include "fips_dev_self_test.h"
>
> @@ -1523,12 +1523,6 @@ static void
> fips_dev_auto_test_uninit(uint8_t dev_id,
> struct fips_dev_auto_test_env *env) {
> - struct rte_cryptodev *dev = rte_cryptodev_pmd_get_dev(dev_id);
> - uint32_t i;
> -
> - if (!dev)
> - return;
> -
> if (env->mbuf)
> rte_pktmbuf_free(env->mbuf);
> if (env->op)
> @@ -1542,16 +1536,7 @@ fips_dev_auto_test_uninit(uint8_t dev_id,
> if (env->sess_priv_pool)
> rte_mempool_free(env->sess_priv_pool);
>
> - if (dev->data->dev_started)
> - rte_cryptodev_stop(dev_id);
> -
> - if (dev->data->nb_queue_pairs) {
> - for (i = 0; i < dev->data->nb_queue_pairs; i++)
> - (*dev->dev_ops->queue_pair_release)(dev, i);
> - dev->data->nb_queue_pairs = 0;
> - rte_free(dev->data->queue_pairs);
> - dev->data->queue_pairs = NULL;
> - }
> + rte_cryptodev_stop(dev_id);
> }
>
> static int
> diff --git a/examples/fips_validation/main.c
> b/examples/fips_validation/main.c index e892078f0e..a8daad1f48 100644
> --- a/examples/fips_validation/main.c
> +++ b/examples/fips_validation/main.c
> @@ -7,7 +7,7 @@
> #include <dirent.h>
>
> #include <rte_cryptodev.h>
> -#include <rte_cryptodev_pmd.h>
> +#include <rte_malloc.h>
> #include <rte_mempool.h>
> #include <rte_mbuf.h>
> #include <rte_string_fns.h>
> @@ -73,10 +73,7 @@ cryptodev_fips_validate_app_int(void)
> if (env.self_test) {
> ret = fips_dev_self_test(env.dev_id, env.broken_test_config);
> if (ret < 0) {
> - struct rte_cryptodev *cryptodev =
> - rte_cryptodev_pmd_get_dev(env.dev_id);
> -
> - rte_cryptodev_pmd_destroy(cryptodev);
> + rte_cryptodev_close(env.dev_id);
>
> return ret;
> }
> --
> 2.25.1
>
> From: Akhil Goyal
> > Some of the cryptodev APIs are not allowed to be used by application
> > directly. Hence removing the usage of 1. queue_pair_release: it is not
> > required, as configure
> > of queue pair release the previous queue pairs and the
> > dev is not directly exposed to application, hence cannot
> > use its ops from app.
> > 2. rte_cryptodev_stop: it can be used directly without
> > checking if the device is started or not.
> > 3. rte_cryptodev_pmd_destroy: application should use
> > rte_cryptodev_close instead.
> >
> > Signed-off-by: Akhil Goyal <gakhil@marvell.com>
>
> Look's like it should be backported to stable releases with a Fixes reference.
> What do you think?
>
> Besides,
> Acked-by: Matan Azrad <matan@nvidia.com>
>
Yes, Agreed.
I wanted to get opinion from the Maintainer of fips app first if the changes are
Correct or not.
> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Wednesday, August 11, 2021 9:20 AM
> To: Matan Azrad <matan@nvidia.com>; dev@dpdk.org
> Cc: Anoob Joseph <anoobj@marvell.com>; Nicolau, Radu
> <radu.nicolau@intel.com>; Doherty, Declan <declan.doherty@intel.com>;
> hemant.agrawal@nxp.com; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; Zhang, Roy Fan <roy.fan.zhang@intel.com>;
> asomalap@amd.com; ruifeng.wang@arm.com;
> ajit.khaparde@broadcom.com; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; Trahe, Fiona <fiona.trahe@intel.com>;
> Ankur Dwivedi <adwivedi@marvell.com>; Michael Shamis
> <michaelsh@marvell.com>; Nagadheeraj Rottela
> <rnagadheeraj@marvell.com>; jianjay.zhou@huawei.com
> Subject: RE: [PATCH v2 3/4] examples/fips_validation: remove illegal usage of
> APIs
>
> >
> > From: Akhil Goyal
> > > Some of the cryptodev APIs are not allowed to be used by application
> > > directly. Hence removing the usage of 1. queue_pair_release: it is not
> > > required, as configure
> > > of queue pair release the previous queue pairs and the
> > > dev is not directly exposed to application, hence cannot
> > > use its ops from app.
> > > 2. rte_cryptodev_stop: it can be used directly without
> > > checking if the device is started or not.
> > > 3. rte_cryptodev_pmd_destroy: application should use
> > > rte_cryptodev_close instead.
> > >
> > > Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> >
> > Look's like it should be backported to stable releases with a Fixes reference.
> > What do you think?
> >
> > Besides,
> > Acked-by: Matan Azrad <matan@nvidia.com>
> >
> Yes, Agreed.
> I wanted to get opinion from the Maintainer of fips app first if the changes
> are
> Correct or not.
Hi Akhil,
FIPS requirement is upon failure of running self-test the device memory
shall be destroyed completely and not visible by the application at all -
rte_cryptodev_close does not provide this functionality.
In this case we may need new API rte_cryptodev_destroy() to replace
rte_cryptodev_pmd_destroy().
Regards,
Fan
Hi Fan,
> > > > Some of the cryptodev APIs are not allowed to be used by application
> > > > directly. Hence removing the usage of 1. queue_pair_release: it is not
> > > > required, as configure
> > > > of queue pair release the previous queue pairs and the
> > > > dev is not directly exposed to application, hence cannot
> > > > use its ops from app.
> > > > 2. rte_cryptodev_stop: it can be used directly without
> > > > checking if the device is started or not.
> > > > 3. rte_cryptodev_pmd_destroy: application should use
> > > > rte_cryptodev_close instead.
> > > >
> > > > Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> > >
> > > Look's like it should be backported to stable releases with a Fixes
> reference.
> > > What do you think?
> > >
> > > Besides,
> > > Acked-by: Matan Azrad <matan@nvidia.com>
> > >
> > Yes, Agreed.
> > I wanted to get opinion from the Maintainer of fips app first if the changes
> > are
> > Correct or not.
>
> Hi Akhil,
>
> FIPS requirement is upon failure of running self-test the device memory
> shall be destroyed completely and not visible by the application at all -
> rte_cryptodev_close does not provide this functionality.
>
> In this case we may need new API rte_cryptodev_destroy() to replace
> rte_cryptodev_pmd_destroy().
>
_create and _destroy are called from device probe and remove.
If the requirement is to use pmd_destroy, how is the driver created again?
I do not see the use of rte_cryptodev_pmd_create() function.
The use of pmd APIs is not allowed from the application and it looks something
Is not correct here for the use of destroy API.
Please check and fix as required.
Regards,
Akhil
Hi Akhil
> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Tuesday, August 31, 2021 8:04 AM
> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; Matan Azrad
> <matan@nvidia.com>; dev@dpdk.org
> Cc: Anoob Joseph <anoobj@marvell.com>; Nicolau, Radu
> <radu.nicolau@intel.com>; Doherty, Declan <declan.doherty@intel.com>;
> hemant.agrawal@nxp.com; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; asomalap@amd.com; ruifeng.wang@arm.com;
> ajit.khaparde@broadcom.com; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; Trahe, Fiona <fiona.trahe@intel.com>;
> Ankur Dwivedi <adwivedi@marvell.com>; Michael Shamis
> <michaelsh@marvell.com>; Nagadheeraj Rottela
> <rnagadheeraj@marvell.com>; jianjay.zhou@huawei.com
> Subject: RE: [PATCH v2 3/4] examples/fips_validation: remove illegal usage of
> APIs
>
> Hi Fan,
> > > > > Some of the cryptodev APIs are not allowed to be used by application
> > > > > directly. Hence removing the usage of 1. queue_pair_release: it is not
> > > > > required, as configure
> > > > > of queue pair release the previous queue pairs and the
> > > > > dev is not directly exposed to application, hence cannot
> > > > > use its ops from app.
> > > > > 2. rte_cryptodev_stop: it can be used directly without
> > > > > checking if the device is started or not.
> > > > > 3. rte_cryptodev_pmd_destroy: application should use
> > > > > rte_cryptodev_close instead.
> > > > >
> > > > > Signed-off-by: Akhil Goyal <gakhil@marvell.com>
> > > >
> > > > Look's like it should be backported to stable releases with a Fixes
> > reference.
> > > > What do you think?
> > > >
> > > > Besides,
> > > > Acked-by: Matan Azrad <matan@nvidia.com>
> > > >
> > > Yes, Agreed.
> > > I wanted to get opinion from the Maintainer of fips app first if the
> changes
> > > are
> > > Correct or not.
> >
> > Hi Akhil,
> >
> > FIPS requirement is upon failure of running self-test the device memory
> > shall be destroyed completely and not visible by the application at all -
> > rte_cryptodev_close does not provide this functionality.
> >
> > In this case we may need new API rte_cryptodev_destroy() to replace
> > rte_cryptodev_pmd_destroy().
> >
> _create and _destroy are called from device probe and remove.
> If the requirement is to use pmd_destroy, how is the driver created again?
> I do not see the use of rte_cryptodev_pmd_create() function.
>
> The use of pmd APIs is not allowed from the application and it looks
> something
> Is not correct here for the use of destroy API.
> Please check and fix as required.
>
> Regards,
> Akhil
Will fix for Intel PMDs.
Regards,
Fan
@@ -3,7 +3,7 @@
*/
#include <rte_cryptodev.h>
-#include <rte_cryptodev_pmd.h>
+#include <rte_malloc.h>
#include "fips_dev_self_test.h"
@@ -1523,12 +1523,6 @@ static void
fips_dev_auto_test_uninit(uint8_t dev_id,
struct fips_dev_auto_test_env *env)
{
- struct rte_cryptodev *dev = rte_cryptodev_pmd_get_dev(dev_id);
- uint32_t i;
-
- if (!dev)
- return;
-
if (env->mbuf)
rte_pktmbuf_free(env->mbuf);
if (env->op)
@@ -1542,16 +1536,7 @@ fips_dev_auto_test_uninit(uint8_t dev_id,
if (env->sess_priv_pool)
rte_mempool_free(env->sess_priv_pool);
- if (dev->data->dev_started)
- rte_cryptodev_stop(dev_id);
-
- if (dev->data->nb_queue_pairs) {
- for (i = 0; i < dev->data->nb_queue_pairs; i++)
- (*dev->dev_ops->queue_pair_release)(dev, i);
- dev->data->nb_queue_pairs = 0;
- rte_free(dev->data->queue_pairs);
- dev->data->queue_pairs = NULL;
- }
+ rte_cryptodev_stop(dev_id);
}
static int
@@ -7,7 +7,7 @@
#include <dirent.h>
#include <rte_cryptodev.h>
-#include <rte_cryptodev_pmd.h>
+#include <rte_malloc.h>
#include <rte_mempool.h>
#include <rte_mbuf.h>
#include <rte_string_fns.h>
@@ -73,10 +73,7 @@ cryptodev_fips_validate_app_int(void)
if (env.self_test) {
ret = fips_dev_self_test(env.dev_id, env.broken_test_config);
if (ret < 0) {
- struct rte_cryptodev *cryptodev =
- rte_cryptodev_pmd_get_dev(env.dev_id);
-
- rte_cryptodev_pmd_destroy(cryptodev);
+ rte_cryptodev_close(env.dev_id);
return ret;
}