diff mbox series

[v2,2/2] vhost/crypto: fix feature negotiation

Message ID 20201002153601.84097-3-roy.fan.zhang@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Maxime Coquelin
Headers
Series vhost/crypto: fix initialization |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/Intel-compilation success Compilation OK
ci/travis-robot success Travis build: passed

Commit Message

Fan Zhang Oct. 2, 2020, 3:36 p.m. UTC 
  This patch fixes the feature negotiation for vhost crypto during
initialization. The patch uses the newly created driver start
function to inform the driver type with the fixed vhost features.
In addtion the patch provides a new API specifically used by
the application to start a vhost-crypto driver.

Fixes: 939066d96563 ("vhost/crypto: add public function implementation")
Cc: roy.fan.zhang@intel.com

Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>
---
 examples/vhost_crypto/main.c           |  3 +-
 lib/librte_vhost/rte_vhost_crypto.h    | 12 ++++++++
 lib/librte_vhost/rte_vhost_version.map |  1 +
 lib/librte_vhost/vhost_crypto.c        | 41 +++++++++++++++++---------
 4 files changed, 42 insertions(+), 15 deletions(-)

Comments

Maxime Coquelin Oct. 6, 2020, 8:09 a.m. UTC | #1 
On 10/2/20 5:36 PM, Fan Zhang wrote:
> This patch fixes the feature negotiation for vhost crypto during
> initialization. The patch uses the newly created driver start
> function to inform the driver type with the fixed vhost features.
> In addtion the patch provides a new API specifically used by
> the application to start a vhost-crypto driver.
> 
> Fixes: 939066d96563 ("vhost/crypto: add public function implementation")
> Cc: roy.fan.zhang@intel.com
> 
> Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>
> ---
>  examples/vhost_crypto/main.c           |  3 +-
>  lib/librte_vhost/rte_vhost_crypto.h    | 12 ++++++++
>  lib/librte_vhost/rte_vhost_version.map |  1 +
>  lib/librte_vhost/vhost_crypto.c        | 41 +++++++++++++++++---------
>  4 files changed, 42 insertions(+), 15 deletions(-)
> 
> diff --git a/examples/vhost_crypto/main.c b/examples/vhost_crypto/main.c
> index d78fd9b81..11ad49159 100644
> --- a/examples/vhost_crypto/main.c
> +++ b/examples/vhost_crypto/main.c
> @@ -598,7 +598,8 @@ main(int argc, char *argv[])
>  			rte_vhost_driver_callback_register(lo->socket_files[j],
>  				&virtio_crypto_device_ops);
>  
> -			ret = rte_vhost_driver_start(lo->socket_files[j]);
> +			ret = rte_vhost_crypto_driver_start(
> +					lo->socket_files[j]);
>  			if (ret < 0)  {
>  				RTE_LOG(ERR, USER1, "failed to start vhost.\n");
>  				goto error_exit;
> diff --git a/lib/librte_vhost/rte_vhost_crypto.h b/lib/librte_vhost/rte_vhost_crypto.h
> index b54d61db6..c809c46a2 100644
> --- a/lib/librte_vhost/rte_vhost_crypto.h
> +++ b/lib/librte_vhost/rte_vhost_crypto.h
> @@ -20,6 +20,18 @@ enum rte_vhost_crypto_zero_copy {
>  	RTE_VHOST_CRYPTO_MAX_ZERO_COPY_OPTIONS
>  };
>  
> +/**
> + * Start vhost crypto driver
> + *
> + * @param path
> + *  The vhost-user socket file path
> + * @return
> + *  0 on success, -1 on failure
> + */
> +__rte_experimental
> +int
> +rte_vhost_crypto_driver_start(const char *path);
> +
>  /**
>   *  Create Vhost-crypto instance
>   *
> diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
> index 55e98e557..9183d6f2f 100644
> --- a/lib/librte_vhost/rte_vhost_version.map
> +++ b/lib/librte_vhost/rte_vhost_version.map
> @@ -55,6 +55,7 @@ EXPERIMENTAL {
>  	rte_vhost_driver_get_protocol_features;
>  	rte_vhost_driver_get_queue_num;
>  	rte_vhost_crypto_create;
> +	rte_vhost_crypto_driver_start;
>  	rte_vhost_crypto_free;
>  	rte_vhost_crypto_fetch_requests;
>  	rte_vhost_crypto_finalize_requests;
> diff --git a/lib/librte_vhost/vhost_crypto.c b/lib/librte_vhost/vhost_crypto.c
> index e08f9c6d7..6195958d2 100644
> --- a/lib/librte_vhost/vhost_crypto.c
> +++ b/lib/librte_vhost/vhost_crypto.c
> @@ -35,13 +35,12 @@
>  #define VC_LOG_DBG(fmt, args...)
>  #endif
>  
> -#define VIRTIO_CRYPTO_FEATURES ((1 << VIRTIO_F_NOTIFY_ON_EMPTY) |	\
> -		(1 << VIRTIO_RING_F_INDIRECT_DESC) |			\
> -		(1 << VIRTIO_RING_F_EVENT_IDX) |			\
> -		(1 << VIRTIO_CRYPTO_SERVICE_CIPHER) |			\
> -		(1 << VIRTIO_CRYPTO_SERVICE_MAC) |			\
> -		(1 << VIRTIO_NET_F_CTRL_VQ) |				\
> -		(1 << VHOST_USER_PROTOCOL_F_CONFIG))
> +#define VIRTIO_CRYPTO_FEATURES ((1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) |	\
> +		(1ULL << VIRTIO_RING_F_INDIRECT_DESC) |			\
> +		(1ULL << VIRTIO_RING_F_EVENT_IDX) |			\
> +		(1ULL << VIRTIO_NET_F_CTRL_VQ) |			\
> +		(1ULL << VIRTIO_F_VERSION_1) |				\
> +		(1ULL << VHOST_USER_F_PROTOCOL_FEATURES))
>  
>  #define IOVA_TO_VVA(t, r, a, l, p)					\
>  	((t)(uintptr_t)vhost_iova_to_vva(r->dev, r->vq, a, l, p))
> @@ -1400,6 +1399,27 @@ vhost_crypto_complete_one_vm_requests(struct rte_crypto_op **ops,
>  	return processed;
>  }
>  
> +int
> +rte_vhost_crypto_driver_start(const char *path)
> +{
> +	uint64_t protocol_features;
> +	int ret;
> +
> +	ret = rte_vhost_driver_set_features(path, VIRTIO_CRYPTO_FEATURES);
> +	if (ret)
> +		return -1;

As rte_vhost_driver_set_features is now called on time,
use_builtin_virtio_net is set to false before the connection is
established.

So it should be enough.

> +	ret = rte_vhost_driver_get_protocol_features(path, &protocol_features);
> +	if (ret)
> +		return -1;
> +	protocol_features |= (1ULL << VHOST_USER_PROTOCOL_F_CONFIG);
> +	ret = rte_vhost_driver_set_protocol_features(path, protocol_features);
> +	if (ret)
> +		return -1;
> +
> +	return vhost_driver_start(path, VIRTIO_DEV_BUILTIN_CRYPTO);

We just have to remove the extra param.

I will to the change and apply it today so that you can test.

Thanks,
Maxime

> +}
> +
>  int
>  rte_vhost_crypto_create(int vid, uint8_t cryptodev_id,
>  		struct rte_mempool *sess_pool,
> @@ -1417,13 +1437,6 @@ rte_vhost_crypto_create(int vid, uint8_t cryptodev_id,
>  		return -EINVAL;
>  	}
>  
> -	ret = rte_vhost_driver_set_features(dev->ifname,
> -			VIRTIO_CRYPTO_FEATURES);
> -	if (ret < 0) {
> -		VC_LOG_ERR("Error setting features");
> -		return -1;
> -	}
> -
>  	vcrypto = rte_zmalloc_socket(NULL, sizeof(*vcrypto),
>  			RTE_CACHE_LINE_SIZE, socket_id);
>  	if (!vcrypto) {
>
Fan Zhang Oct. 6, 2020, 8:37 a.m. UTC | #2 
Hi Maxime,

Thanks I will verify it after you applied the patch.

Regards,
Fan

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, October 6, 2020 9:10 AM
> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Liu, Changpeng
> <changpeng.liu@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> stable@dpdk.org
> Subject: Re: [dpdk-dev v2 2/2] vhost/crypto: fix feature negotiation
> 
> 
> 
> On 10/2/20 5:36 PM, Fan Zhang wrote:
> > This patch fixes the feature negotiation for vhost crypto during
> > initialization. The patch uses the newly created driver start
> > function to inform the driver type with the fixed vhost features.
> > In addtion the patch provides a new API specifically used by
> > the application to start a vhost-crypto driver.
> >
> > Fixes: 939066d96563 ("vhost/crypto: add public function implementation")
> > Cc: roy.fan.zhang@intel.com
> >
> > Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>
> > ---
> >  examples/vhost_crypto/main.c           |  3 +-
> >  lib/librte_vhost/rte_vhost_crypto.h    | 12 ++++++++
> >  lib/librte_vhost/rte_vhost_version.map |  1 +
> >  lib/librte_vhost/vhost_crypto.c        | 41 +++++++++++++++++---------
> >  4 files changed, 42 insertions(+), 15 deletions(-)
> >
> > diff --git a/examples/vhost_crypto/main.c
> b/examples/vhost_crypto/main.c
> > index d78fd9b81..11ad49159 100644
> > --- a/examples/vhost_crypto/main.c
> > +++ b/examples/vhost_crypto/main.c
> > @@ -598,7 +598,8 @@ main(int argc, char *argv[])
> >  			rte_vhost_driver_callback_register(lo-
> >socket_files[j],
> >  				&virtio_crypto_device_ops);
> >
> > -			ret = rte_vhost_driver_start(lo->socket_files[j]);
> > +			ret = rte_vhost_crypto_driver_start(
> > +					lo->socket_files[j]);
> >  			if (ret < 0)  {
> >  				RTE_LOG(ERR, USER1, "failed to start
> vhost.\n");
> >  				goto error_exit;
> > diff --git a/lib/librte_vhost/rte_vhost_crypto.h
> b/lib/librte_vhost/rte_vhost_crypto.h
> > index b54d61db6..c809c46a2 100644
> > --- a/lib/librte_vhost/rte_vhost_crypto.h
> > +++ b/lib/librte_vhost/rte_vhost_crypto.h
> > @@ -20,6 +20,18 @@ enum rte_vhost_crypto_zero_copy {
> >  	RTE_VHOST_CRYPTO_MAX_ZERO_COPY_OPTIONS
> >  };
> >
> > +/**
> > + * Start vhost crypto driver
> > + *
> > + * @param path
> > + *  The vhost-user socket file path
> > + * @return
> > + *  0 on success, -1 on failure
> > + */
> > +__rte_experimental
> > +int
> > +rte_vhost_crypto_driver_start(const char *path);
> > +
> >  /**
> >   *  Create Vhost-crypto instance
> >   *
> > diff --git a/lib/librte_vhost/rte_vhost_version.map
> b/lib/librte_vhost/rte_vhost_version.map
> > index 55e98e557..9183d6f2f 100644
> > --- a/lib/librte_vhost/rte_vhost_version.map
> > +++ b/lib/librte_vhost/rte_vhost_version.map
> > @@ -55,6 +55,7 @@ EXPERIMENTAL {
> >  	rte_vhost_driver_get_protocol_features;
> >  	rte_vhost_driver_get_queue_num;
> >  	rte_vhost_crypto_create;
> > +	rte_vhost_crypto_driver_start;
> >  	rte_vhost_crypto_free;
> >  	rte_vhost_crypto_fetch_requests;
> >  	rte_vhost_crypto_finalize_requests;
> > diff --git a/lib/librte_vhost/vhost_crypto.c
> b/lib/librte_vhost/vhost_crypto.c
> > index e08f9c6d7..6195958d2 100644
> > --- a/lib/librte_vhost/vhost_crypto.c
> > +++ b/lib/librte_vhost/vhost_crypto.c
> > @@ -35,13 +35,12 @@
> >  #define VC_LOG_DBG(fmt, args...)
> >  #endif
> >
> > -#define VIRTIO_CRYPTO_FEATURES ((1 << VIRTIO_F_NOTIFY_ON_EMPTY)
> |	\
> > -		(1 << VIRTIO_RING_F_INDIRECT_DESC) |
> 	\
> > -		(1 << VIRTIO_RING_F_EVENT_IDX) |			\
> > -		(1 << VIRTIO_CRYPTO_SERVICE_CIPHER) |
> 	\
> > -		(1 << VIRTIO_CRYPTO_SERVICE_MAC) |
> 	\
> > -		(1 << VIRTIO_NET_F_CTRL_VQ) |
> 	\
> > -		(1 << VHOST_USER_PROTOCOL_F_CONFIG))
> > +#define VIRTIO_CRYPTO_FEATURES ((1ULL <<
> VIRTIO_F_NOTIFY_ON_EMPTY) |	\
> > +		(1ULL << VIRTIO_RING_F_INDIRECT_DESC) |
> 	\
> > +		(1ULL << VIRTIO_RING_F_EVENT_IDX) |
> 	\
> > +		(1ULL << VIRTIO_NET_F_CTRL_VQ) |			\
> > +		(1ULL << VIRTIO_F_VERSION_1) |
> 	\
> > +		(1ULL << VHOST_USER_F_PROTOCOL_FEATURES))
> >
> >  #define IOVA_TO_VVA(t, r, a, l, p)					\
> >  	((t)(uintptr_t)vhost_iova_to_vva(r->dev, r->vq, a, l, p))
> > @@ -1400,6 +1399,27 @@
> vhost_crypto_complete_one_vm_requests(struct rte_crypto_op **ops,
> >  	return processed;
> >  }
> >
> > +int
> > +rte_vhost_crypto_driver_start(const char *path)
> > +{
> > +	uint64_t protocol_features;
> > +	int ret;
> > +
> > +	ret = rte_vhost_driver_set_features(path,
> VIRTIO_CRYPTO_FEATURES);
> > +	if (ret)
> > +		return -1;
> 
> As rte_vhost_driver_set_features is now called on time,
> use_builtin_virtio_net is set to false before the connection is
> established.
> 
> So it should be enough.
> 
> > +	ret = rte_vhost_driver_get_protocol_features(path,
> &protocol_features);
> > +	if (ret)
> > +		return -1;
> > +	protocol_features |= (1ULL << VHOST_USER_PROTOCOL_F_CONFIG);
> > +	ret = rte_vhost_driver_set_protocol_features(path,
> protocol_features);
> > +	if (ret)
> > +		return -1;
> > +
> > +	return vhost_driver_start(path, VIRTIO_DEV_BUILTIN_CRYPTO);
> 
> We just have to remove the extra param.
> 
> I will to the change and apply it today so that you can test.
> 
> Thanks,
> Maxime
> 
> > +}
> > +
> >  int
> >  rte_vhost_crypto_create(int vid, uint8_t cryptodev_id,
> >  		struct rte_mempool *sess_pool,
> > @@ -1417,13 +1437,6 @@ rte_vhost_crypto_create(int vid, uint8_t
> cryptodev_id,
> >  		return -EINVAL;
> >  	}
> >
> > -	ret = rte_vhost_driver_set_features(dev->ifname,
> > -			VIRTIO_CRYPTO_FEATURES);
> > -	if (ret < 0) {
> > -		VC_LOG_ERR("Error setting features");
> > -		return -1;
> > -	}
> > -
> >  	vcrypto = rte_zmalloc_socket(NULL, sizeof(*vcrypto),
> >  			RTE_CACHE_LINE_SIZE, socket_id);
> >  	if (!vcrypto) {
> >
Maxime Coquelin Oct. 9, 2020, 6:39 a.m. UTC | #3 
On 10/2/20 5:36 PM, Fan Zhang wrote:
> This patch fixes the feature negotiation for vhost crypto during
> initialization. The patch uses the newly created driver start
> function to inform the driver type with the fixed vhost features.
> In addtion the patch provides a new API specifically used by
> the application to start a vhost-crypto driver.
> 
> Fixes: 939066d96563 ("vhost/crypto: add public function implementation")
> Cc: roy.fan.zhang@intel.com
> 
> Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>
> ---
>  examples/vhost_crypto/main.c           |  3 +-
>  lib/librte_vhost/rte_vhost_crypto.h    | 12 ++++++++
>  lib/librte_vhost/rte_vhost_version.map |  1 +
>  lib/librte_vhost/vhost_crypto.c        | 41 +++++++++++++++++---------
>  4 files changed, 42 insertions(+), 15 deletions(-)
> 
> diff --git a/examples/vhost_crypto/main.c b/examples/vhost_crypto/main.c
> index d78fd9b81..11ad49159 100644
> --- a/examples/vhost_crypto/main.c
> +++ b/examples/vhost_crypto/main.c
> @@ -598,7 +598,8 @@ main(int argc, char *argv[])
>  			rte_vhost_driver_callback_register(lo->socket_files[j],
>  				&virtio_crypto_device_ops);
>  
> -			ret = rte_vhost_driver_start(lo->socket_files[j]);
> +			ret = rte_vhost_crypto_driver_start(
> +					lo->socket_files[j]);
>  			if (ret < 0)  {
>  				RTE_LOG(ERR, USER1, "failed to start vhost.\n");
>  				goto error_exit;
> diff --git a/lib/librte_vhost/rte_vhost_crypto.h b/lib/librte_vhost/rte_vhost_crypto.h
> index b54d61db6..c809c46a2 100644
> --- a/lib/librte_vhost/rte_vhost_crypto.h
> +++ b/lib/librte_vhost/rte_vhost_crypto.h
> @@ -20,6 +20,18 @@ enum rte_vhost_crypto_zero_copy {
>  	RTE_VHOST_CRYPTO_MAX_ZERO_COPY_OPTIONS
>  };
>  
> +/**
> + * Start vhost crypto driver
> + *
> + * @param path
> + *  The vhost-user socket file path
> + * @return
> + *  0 on success, -1 on failure
> + */
> +__rte_experimental
> +int
> +rte_vhost_crypto_driver_start(const char *path);
> +
>  /**
>   *  Create Vhost-crypto instance
>   *
> diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
> index 55e98e557..9183d6f2f 100644
> --- a/lib/librte_vhost/rte_vhost_version.map
> +++ b/lib/librte_vhost/rte_vhost_version.map
> @@ -55,6 +55,7 @@ EXPERIMENTAL {
>  	rte_vhost_driver_get_protocol_features;
>  	rte_vhost_driver_get_queue_num;
>  	rte_vhost_crypto_create;
> +	rte_vhost_crypto_driver_start;
>  	rte_vhost_crypto_free;
>  	rte_vhost_crypto_fetch_requests;
>  	rte_vhost_crypto_finalize_requests;
> diff --git a/lib/librte_vhost/vhost_crypto.c b/lib/librte_vhost/vhost_crypto.c
> index e08f9c6d7..6195958d2 100644
> --- a/lib/librte_vhost/vhost_crypto.c
> +++ b/lib/librte_vhost/vhost_crypto.c
> @@ -35,13 +35,12 @@
>  #define VC_LOG_DBG(fmt, args...)
>  #endif
>  
> -#define VIRTIO_CRYPTO_FEATURES ((1 << VIRTIO_F_NOTIFY_ON_EMPTY) |	\
> -		(1 << VIRTIO_RING_F_INDIRECT_DESC) |			\
> -		(1 << VIRTIO_RING_F_EVENT_IDX) |			\
> -		(1 << VIRTIO_CRYPTO_SERVICE_CIPHER) |			\
> -		(1 << VIRTIO_CRYPTO_SERVICE_MAC) |			\
> -		(1 << VIRTIO_NET_F_CTRL_VQ) |				\
> -		(1 << VHOST_USER_PROTOCOL_F_CONFIG))
> +#define VIRTIO_CRYPTO_FEATURES ((1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) |	\
> +		(1ULL << VIRTIO_RING_F_INDIRECT_DESC) |			\
> +		(1ULL << VIRTIO_RING_F_EVENT_IDX) |			\
> +		(1ULL << VIRTIO_NET_F_CTRL_VQ) |			\
> +		(1ULL << VIRTIO_F_VERSION_1) |				\
> +		(1ULL << VHOST_USER_F_PROTOCOL_FEATURES))
>  
>  #define IOVA_TO_VVA(t, r, a, l, p)					\
>  	((t)(uintptr_t)vhost_iova_to_vva(r->dev, r->vq, a, l, p))
> @@ -1400,6 +1399,27 @@ vhost_crypto_complete_one_vm_requests(struct rte_crypto_op **ops,
>  	return processed;
>  }
>  
> +int
> +rte_vhost_crypto_driver_start(const char *path)
> +{
> +	uint64_t protocol_features;
> +	int ret;
> +
> +	ret = rte_vhost_driver_set_features(path, VIRTIO_CRYPTO_FEATURES);
> +	if (ret)
> +		return -1;
> +
> +	ret = rte_vhost_driver_get_protocol_features(path, &protocol_features);
> +	if (ret)
> +		return -1;
> +	protocol_features |= (1ULL << VHOST_USER_PROTOCOL_F_CONFIG);
> +	ret = rte_vhost_driver_set_protocol_features(path, protocol_features);
> +	if (ret)
> +		return -1;
> +
> +	return vhost_driver_start(path, VIRTIO_DEV_BUILTIN_CRYPTO);
> +}
> +
>  int
>  rte_vhost_crypto_create(int vid, uint8_t cryptodev_id,
>  		struct rte_mempool *sess_pool,
> @@ -1417,13 +1437,6 @@ rte_vhost_crypto_create(int vid, uint8_t cryptodev_id,
>  		return -EINVAL;
>  	}
>  
> -	ret = rte_vhost_driver_set_features(dev->ifname,
> -			VIRTIO_CRYPTO_FEATURES);
> -	if (ret < 0) {
> -		VC_LOG_ERR("Error setting features");
> -		return -1;
> -	}
> -
>  	vcrypto = rte_zmalloc_socket(NULL, sizeof(*vcrypto),
>  			RTE_CACHE_LINE_SIZE, socket_id);
>  	if (!vcrypto) {
> 

Only for this patch:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime
Maxime Coquelin Oct. 9, 2020, 7:24 a.m. UTC | #4 
On 10/2/20 5:36 PM, Fan Zhang wrote:
> This patch fixes the feature negotiation for vhost crypto during
> initialization. The patch uses the newly created driver start
> function to inform the driver type with the fixed vhost features.
> In addtion the patch provides a new API specifically used by
> the application to start a vhost-crypto driver.
> 
> Fixes: 939066d96563 ("vhost/crypto: add public function implementation")
> Cc: roy.fan.zhang@intel.com
> 
> Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>
> ---
>  examples/vhost_crypto/main.c           |  3 +-
>  lib/librte_vhost/rte_vhost_crypto.h    | 12 ++++++++
>  lib/librte_vhost/rte_vhost_version.map |  1 +
>  lib/librte_vhost/vhost_crypto.c        | 41 +++++++++++++++++---------
>  4 files changed, 42 insertions(+), 15 deletions(-)


Applied to dpdk-next-virtio/main.

Thanks,
Maxime
Maxime Coquelin Oct. 9, 2020, 7:36 a.m. UTC | #5 
On 10/6/20 10:37 AM, Zhang, Roy Fan wrote:
> Hi Maxime,
> 
> Thanks I will verify it after you applied the patch.


Thanks,
Your patch is now on dpdk-next-virtio/main.
I would be glad if you could test it before Ferruh merges it.

Thanks,
Maxime
> Regards,
> Fan
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Tuesday, October 6, 2020 9:10 AM
>> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; dev@dpdk.org
>> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Liu, Changpeng
>> <changpeng.liu@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
>> stable@dpdk.org
>> Subject: Re: [dpdk-dev v2 2/2] vhost/crypto: fix feature negotiation
>>
>>
>>
>> On 10/2/20 5:36 PM, Fan Zhang wrote:
>>> This patch fixes the feature negotiation for vhost crypto during
>>> initialization. The patch uses the newly created driver start
>>> function to inform the driver type with the fixed vhost features.
>>> In addtion the patch provides a new API specifically used by
>>> the application to start a vhost-crypto driver.
>>>
>>> Fixes: 939066d96563 ("vhost/crypto: add public function implementation")
>>> Cc: roy.fan.zhang@intel.com
>>>
>>> Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>
>>> ---
>>>  examples/vhost_crypto/main.c           |  3 +-
>>>  lib/librte_vhost/rte_vhost_crypto.h    | 12 ++++++++
>>>  lib/librte_vhost/rte_vhost_version.map |  1 +
>>>  lib/librte_vhost/vhost_crypto.c        | 41 +++++++++++++++++---------
>>>  4 files changed, 42 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/examples/vhost_crypto/main.c
>> b/examples/vhost_crypto/main.c
>>> index d78fd9b81..11ad49159 100644
>>> --- a/examples/vhost_crypto/main.c
>>> +++ b/examples/vhost_crypto/main.c
>>> @@ -598,7 +598,8 @@ main(int argc, char *argv[])
>>>  			rte_vhost_driver_callback_register(lo-
>>> socket_files[j],
>>>  				&virtio_crypto_device_ops);
>>>
>>> -			ret = rte_vhost_driver_start(lo->socket_files[j]);
>>> +			ret = rte_vhost_crypto_driver_start(
>>> +					lo->socket_files[j]);
>>>  			if (ret < 0)  {
>>>  				RTE_LOG(ERR, USER1, "failed to start
>> vhost.\n");
>>>  				goto error_exit;
>>> diff --git a/lib/librte_vhost/rte_vhost_crypto.h
>> b/lib/librte_vhost/rte_vhost_crypto.h
>>> index b54d61db6..c809c46a2 100644
>>> --- a/lib/librte_vhost/rte_vhost_crypto.h
>>> +++ b/lib/librte_vhost/rte_vhost_crypto.h
>>> @@ -20,6 +20,18 @@ enum rte_vhost_crypto_zero_copy {
>>>  	RTE_VHOST_CRYPTO_MAX_ZERO_COPY_OPTIONS
>>>  };
>>>
>>> +/**
>>> + * Start vhost crypto driver
>>> + *
>>> + * @param path
>>> + *  The vhost-user socket file path
>>> + * @return
>>> + *  0 on success, -1 on failure
>>> + */
>>> +__rte_experimental
>>> +int
>>> +rte_vhost_crypto_driver_start(const char *path);
>>> +
>>>  /**
>>>   *  Create Vhost-crypto instance
>>>   *
>>> diff --git a/lib/librte_vhost/rte_vhost_version.map
>> b/lib/librte_vhost/rte_vhost_version.map
>>> index 55e98e557..9183d6f2f 100644
>>> --- a/lib/librte_vhost/rte_vhost_version.map
>>> +++ b/lib/librte_vhost/rte_vhost_version.map
>>> @@ -55,6 +55,7 @@ EXPERIMENTAL {
>>>  	rte_vhost_driver_get_protocol_features;
>>>  	rte_vhost_driver_get_queue_num;
>>>  	rte_vhost_crypto_create;
>>> +	rte_vhost_crypto_driver_start;
>>>  	rte_vhost_crypto_free;
>>>  	rte_vhost_crypto_fetch_requests;
>>>  	rte_vhost_crypto_finalize_requests;
>>> diff --git a/lib/librte_vhost/vhost_crypto.c
>> b/lib/librte_vhost/vhost_crypto.c
>>> index e08f9c6d7..6195958d2 100644
>>> --- a/lib/librte_vhost/vhost_crypto.c
>>> +++ b/lib/librte_vhost/vhost_crypto.c
>>> @@ -35,13 +35,12 @@
>>>  #define VC_LOG_DBG(fmt, args...)
>>>  #endif
>>>
>>> -#define VIRTIO_CRYPTO_FEATURES ((1 << VIRTIO_F_NOTIFY_ON_EMPTY)
>> |	\
>>> -		(1 << VIRTIO_RING_F_INDIRECT_DESC) |
>> 	\
>>> -		(1 << VIRTIO_RING_F_EVENT_IDX) |			\
>>> -		(1 << VIRTIO_CRYPTO_SERVICE_CIPHER) |
>> 	\
>>> -		(1 << VIRTIO_CRYPTO_SERVICE_MAC) |
>> 	\
>>> -		(1 << VIRTIO_NET_F_CTRL_VQ) |
>> 	\
>>> -		(1 << VHOST_USER_PROTOCOL_F_CONFIG))
>>> +#define VIRTIO_CRYPTO_FEATURES ((1ULL <<
>> VIRTIO_F_NOTIFY_ON_EMPTY) |	\
>>> +		(1ULL << VIRTIO_RING_F_INDIRECT_DESC) |
>> 	\
>>> +		(1ULL << VIRTIO_RING_F_EVENT_IDX) |
>> 	\
>>> +		(1ULL << VIRTIO_NET_F_CTRL_VQ) |			\
>>> +		(1ULL << VIRTIO_F_VERSION_1) |
>> 	\
>>> +		(1ULL << VHOST_USER_F_PROTOCOL_FEATURES))
>>>
>>>  #define IOVA_TO_VVA(t, r, a, l, p)					\
>>>  	((t)(uintptr_t)vhost_iova_to_vva(r->dev, r->vq, a, l, p))
>>> @@ -1400,6 +1399,27 @@
>> vhost_crypto_complete_one_vm_requests(struct rte_crypto_op **ops,
>>>  	return processed;
>>>  }
>>>
>>> +int
>>> +rte_vhost_crypto_driver_start(const char *path)
>>> +{
>>> +	uint64_t protocol_features;
>>> +	int ret;
>>> +
>>> +	ret = rte_vhost_driver_set_features(path,
>> VIRTIO_CRYPTO_FEATURES);
>>> +	if (ret)
>>> +		return -1;
>>
>> As rte_vhost_driver_set_features is now called on time,
>> use_builtin_virtio_net is set to false before the connection is
>> established.
>>
>> So it should be enough.
>>
>>> +	ret = rte_vhost_driver_get_protocol_features(path,
>> &protocol_features);
>>> +	if (ret)
>>> +		return -1;
>>> +	protocol_features |= (1ULL << VHOST_USER_PROTOCOL_F_CONFIG);
>>> +	ret = rte_vhost_driver_set_protocol_features(path,
>> protocol_features);
>>> +	if (ret)
>>> +		return -1;
>>> +
>>> +	return vhost_driver_start(path, VIRTIO_DEV_BUILTIN_CRYPTO);
>>
>> We just have to remove the extra param.
>>
>> I will to the change and apply it today so that you can test.
>>
>> Thanks,
>> Maxime
>>
>>> +}
>>> +
>>>  int
>>>  rte_vhost_crypto_create(int vid, uint8_t cryptodev_id,
>>>  		struct rte_mempool *sess_pool,
>>> @@ -1417,13 +1437,6 @@ rte_vhost_crypto_create(int vid, uint8_t
>> cryptodev_id,
>>>  		return -EINVAL;
>>>  	}
>>>
>>> -	ret = rte_vhost_driver_set_features(dev->ifname,
>>> -			VIRTIO_CRYPTO_FEATURES);
>>> -	if (ret < 0) {
>>> -		VC_LOG_ERR("Error setting features");
>>> -		return -1;
>>> -	}
>>> -
>>>  	vcrypto = rte_zmalloc_socket(NULL, sizeof(*vcrypto),
>>>  			RTE_CACHE_LINE_SIZE, socket_id);
>>>  	if (!vcrypto) {
>>>
>
Yu Jiang Oct. 10, 2020, 9:28 a.m. UTC | #6 
Tested-by: Jiang, YuX <yux.jiang@intel.com>

    Best Regards
    Jiang yu

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maxime Coquelin
> Sent: Friday, October 9, 2020 3:36 PM
> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Liu, Changpeng
> <changpeng.liu@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> stable@dpdk.org
> Subject: Re: [dpdk-dev] [dpdk-dev v2 2/2] vhost/crypto: fix feature
> negotiation
> 
> 
> 
> On 10/6/20 10:37 AM, Zhang, Roy Fan wrote:
> > Hi Maxime,
> >
> > Thanks I will verify it after you applied the patch.
> 
> 
> Thanks,
> Your patch is now on dpdk-next-virtio/main.
> I would be glad if you could test it before Ferruh merges it.
> 
> Thanks,
> Maxime
> > Regards,
> > Fan
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Tuesday, October 6, 2020 9:10 AM
> >> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; dev@dpdk.org
> >> Cc: Xia, Chenbo <chenbo.xia@intel.com>; Liu, Changpeng
> >> <changpeng.liu@intel.com>; Yigit, Ferruh <ferruh.yigit@intel.com>;
> >> stable@dpdk.org
> >> Subject: Re: [dpdk-dev v2 2/2] vhost/crypto: fix feature negotiation
> >>
> >>
> >>
> >> On 10/2/20 5:36 PM, Fan Zhang wrote:
> >>> This patch fixes the feature negotiation for vhost crypto during
> >>> initialization. The patch uses the newly created driver start
> >>> function to inform the driver type with the fixed vhost features.
> >>> In addtion the patch provides a new API specifically used by the
> >>> application to start a vhost-crypto driver.
> >>>
> >>> Fixes: 939066d96563 ("vhost/crypto: add public function
> >>> implementation")
> >>> Cc: roy.fan.zhang@intel.com
> >>>
> >>> Signed-off-by: Fan Zhang <roy.fan.zhang@intel.com>
> >>> ---
> >>>  examples/vhost_crypto/main.c           |  3 +-
> >>>  lib/librte_vhost/rte_vhost_crypto.h    | 12 ++++++++
> >>>  lib/librte_vhost/rte_vhost_version.map |  1 +
> >>>  lib/librte_vhost/vhost_crypto.c        | 41 +++++++++++++++++---------
> >>>  4 files changed, 42 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/examples/vhost_crypto/main.c
> >> b/examples/vhost_crypto/main.c
> >>> index d78fd9b81..11ad49159 100644
> >>> --- a/examples/vhost_crypto/main.c
> >>> +++ b/examples/vhost_crypto/main.c
> >>> @@ -598,7 +598,8 @@ main(int argc, char *argv[])
> >>>  			rte_vhost_driver_callback_register(lo-
> >>> socket_files[j],
> >>>  				&virtio_crypto_device_ops);
> >>>
> >>> -			ret = rte_vhost_driver_start(lo->socket_files[j]);
> >>> +			ret = rte_vhost_crypto_driver_start(
> >>> +					lo->socket_files[j]);
> >>>  			if (ret < 0)  {
> >>>  				RTE_LOG(ERR, USER1, "failed to start
> >> vhost.\n");
> >>>  				goto error_exit;
> >>> diff --git a/lib/librte_vhost/rte_vhost_crypto.h
> >> b/lib/librte_vhost/rte_vhost_crypto.h
> >>> index b54d61db6..c809c46a2 100644
> >>> --- a/lib/librte_vhost/rte_vhost_crypto.h
> >>> +++ b/lib/librte_vhost/rte_vhost_crypto.h
> >>> @@ -20,6 +20,18 @@ enum rte_vhost_crypto_zero_copy {
> >>>  	RTE_VHOST_CRYPTO_MAX_ZERO_COPY_OPTIONS
> >>>  };
> >>>
> >>> +/**
> >>> + * Start vhost crypto driver
> >>> + *
> >>> + * @param path
> >>> + *  The vhost-user socket file path
> >>> + * @return
> >>> + *  0 on success, -1 on failure
> >>> + */
> >>> +__rte_experimental
> >>> +int
> >>> +rte_vhost_crypto_driver_start(const char *path);
> >>> +
> >>>  /**
> >>>   *  Create Vhost-crypto instance
> >>>   *
> >>> diff --git a/lib/librte_vhost/rte_vhost_version.map
> >> b/lib/librte_vhost/rte_vhost_version.map
> >>> index 55e98e557..9183d6f2f 100644
> >>> --- a/lib/librte_vhost/rte_vhost_version.map
> >>> +++ b/lib/librte_vhost/rte_vhost_version.map
> >>> @@ -55,6 +55,7 @@ EXPERIMENTAL {
> >>>  	rte_vhost_driver_get_protocol_features;
> >>>  	rte_vhost_driver_get_queue_num;
> >>>  	rte_vhost_crypto_create;
> >>> +	rte_vhost_crypto_driver_start;
> >>>  	rte_vhost_crypto_free;
> >>>  	rte_vhost_crypto_fetch_requests;
> >>>  	rte_vhost_crypto_finalize_requests;
> >>> diff --git a/lib/librte_vhost/vhost_crypto.c
> >> b/lib/librte_vhost/vhost_crypto.c
> >>> index e08f9c6d7..6195958d2 100644
> >>> --- a/lib/librte_vhost/vhost_crypto.c
> >>> +++ b/lib/librte_vhost/vhost_crypto.c
> >>> @@ -35,13 +35,12 @@
> >>>  #define VC_LOG_DBG(fmt, args...)
> >>>  #endif
> >>>
> >>> -#define VIRTIO_CRYPTO_FEATURES ((1 <<
> VIRTIO_F_NOTIFY_ON_EMPTY)
> >> |	\
> >>> -		(1 << VIRTIO_RING_F_INDIRECT_DESC) |
> >> 	\
> >>> -		(1 << VIRTIO_RING_F_EVENT_IDX) |			\
> >>> -		(1 << VIRTIO_CRYPTO_SERVICE_CIPHER) |
> >> 	\
> >>> -		(1 << VIRTIO_CRYPTO_SERVICE_MAC) |
> >> 	\
> >>> -		(1 << VIRTIO_NET_F_CTRL_VQ) |
> >> 	\
> >>> -		(1 << VHOST_USER_PROTOCOL_F_CONFIG))
> >>> +#define VIRTIO_CRYPTO_FEATURES ((1ULL <<
> >> VIRTIO_F_NOTIFY_ON_EMPTY) |	\
> >>> +		(1ULL << VIRTIO_RING_F_INDIRECT_DESC) |
> >> 	\
> >>> +		(1ULL << VIRTIO_RING_F_EVENT_IDX) |
> >> 	\
> >>> +		(1ULL << VIRTIO_NET_F_CTRL_VQ) |			\
> >>> +		(1ULL << VIRTIO_F_VERSION_1) |
> >> 	\
> >>> +		(1ULL << VHOST_USER_F_PROTOCOL_FEATURES))
> >>>
> >>>  #define IOVA_TO_VVA(t, r, a, l, p)
> 	\
> >>>  	((t)(uintptr_t)vhost_iova_to_vva(r->dev, r->vq, a, l, p)) @@
> >>> -1400,6 +1399,27 @@
> >> vhost_crypto_complete_one_vm_requests(struct rte_crypto_op **ops,
> >>>  	return processed;
> >>>  }
> >>>
> >>> +int
> >>> +rte_vhost_crypto_driver_start(const char *path) {
> >>> +	uint64_t protocol_features;
> >>> +	int ret;
> >>> +
> >>> +	ret = rte_vhost_driver_set_features(path,
> >> VIRTIO_CRYPTO_FEATURES);
> >>> +	if (ret)
> >>> +		return -1;
> >>
> >> As rte_vhost_driver_set_features is now called on time,
> >> use_builtin_virtio_net is set to false before the connection is
> >> established.
> >>
> >> So it should be enough.
> >>
> >>> +	ret = rte_vhost_driver_get_protocol_features(path,
> >> &protocol_features);
> >>> +	if (ret)
> >>> +		return -1;
> >>> +	protocol_features |= (1ULL << VHOST_USER_PROTOCOL_F_CONFIG);
> >>> +	ret = rte_vhost_driver_set_protocol_features(path,
> >> protocol_features);
> >>> +	if (ret)
> >>> +		return -1;
> >>> +
> >>> +	return vhost_driver_start(path, VIRTIO_DEV_BUILTIN_CRYPTO);
> >>
> >> We just have to remove the extra param.
> >>
> >> I will to the change and apply it today so that you can test.
> >>
> >> Thanks,
> >> Maxime
> >>
> >>> +}
> >>> +
> >>>  int
> >>>  rte_vhost_crypto_create(int vid, uint8_t cryptodev_id,
> >>>  		struct rte_mempool *sess_pool,
> >>> @@ -1417,13 +1437,6 @@ rte_vhost_crypto_create(int vid, uint8_t
> >> cryptodev_id,
> >>>  		return -EINVAL;
> >>>  	}
> >>>
> >>> -	ret = rte_vhost_driver_set_features(dev->ifname,
> >>> -			VIRTIO_CRYPTO_FEATURES);
> >>> -	if (ret < 0) {
> >>> -		VC_LOG_ERR("Error setting features");
> >>> -		return -1;
> >>> -	}
> >>> -
> >>>  	vcrypto = rte_zmalloc_socket(NULL, sizeof(*vcrypto),
> >>>  			RTE_CACHE_LINE_SIZE, socket_id);
> >>>  	if (!vcrypto) {
> >>>
> >
diff mbox series

Patch

diff --git a/examples/vhost_crypto/main.c b/examples/vhost_crypto/main.c
index d78fd9b81..11ad49159 100644
--- a/examples/vhost_crypto/main.c
+++ b/examples/vhost_crypto/main.c
@@ -598,7 +598,8 @@  main(int argc, char *argv[])
 			rte_vhost_driver_callback_register(lo->socket_files[j],
 				&virtio_crypto_device_ops);
 
-			ret = rte_vhost_driver_start(lo->socket_files[j]);
+			ret = rte_vhost_crypto_driver_start(
+					lo->socket_files[j]);
 			if (ret < 0)  {
 				RTE_LOG(ERR, USER1, "failed to start vhost.\n");
 				goto error_exit;
diff --git a/lib/librte_vhost/rte_vhost_crypto.h b/lib/librte_vhost/rte_vhost_crypto.h
index b54d61db6..c809c46a2 100644
--- a/lib/librte_vhost/rte_vhost_crypto.h
+++ b/lib/librte_vhost/rte_vhost_crypto.h
@@ -20,6 +20,18 @@  enum rte_vhost_crypto_zero_copy {
 	RTE_VHOST_CRYPTO_MAX_ZERO_COPY_OPTIONS
 };
 
+/**
+ * Start vhost crypto driver
+ *
+ * @param path
+ *  The vhost-user socket file path
+ * @return
+ *  0 on success, -1 on failure
+ */
+__rte_experimental
+int
+rte_vhost_crypto_driver_start(const char *path);
+
 /**
  *  Create Vhost-crypto instance
  *
diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
index 55e98e557..9183d6f2f 100644
--- a/lib/librte_vhost/rte_vhost_version.map
+++ b/lib/librte_vhost/rte_vhost_version.map
@@ -55,6 +55,7 @@  EXPERIMENTAL {
 	rte_vhost_driver_get_protocol_features;
 	rte_vhost_driver_get_queue_num;
 	rte_vhost_crypto_create;
+	rte_vhost_crypto_driver_start;
 	rte_vhost_crypto_free;
 	rte_vhost_crypto_fetch_requests;
 	rte_vhost_crypto_finalize_requests;
diff --git a/lib/librte_vhost/vhost_crypto.c b/lib/librte_vhost/vhost_crypto.c
index e08f9c6d7..6195958d2 100644
--- a/lib/librte_vhost/vhost_crypto.c
+++ b/lib/librte_vhost/vhost_crypto.c
@@ -35,13 +35,12 @@ 
 #define VC_LOG_DBG(fmt, args...)
 #endif
 
-#define VIRTIO_CRYPTO_FEATURES ((1 << VIRTIO_F_NOTIFY_ON_EMPTY) |	\
-		(1 << VIRTIO_RING_F_INDIRECT_DESC) |			\
-		(1 << VIRTIO_RING_F_EVENT_IDX) |			\
-		(1 << VIRTIO_CRYPTO_SERVICE_CIPHER) |			\
-		(1 << VIRTIO_CRYPTO_SERVICE_MAC) |			\
-		(1 << VIRTIO_NET_F_CTRL_VQ) |				\
-		(1 << VHOST_USER_PROTOCOL_F_CONFIG))
+#define VIRTIO_CRYPTO_FEATURES ((1ULL << VIRTIO_F_NOTIFY_ON_EMPTY) |	\
+		(1ULL << VIRTIO_RING_F_INDIRECT_DESC) |			\
+		(1ULL << VIRTIO_RING_F_EVENT_IDX) |			\
+		(1ULL << VIRTIO_NET_F_CTRL_VQ) |			\
+		(1ULL << VIRTIO_F_VERSION_1) |				\
+		(1ULL << VHOST_USER_F_PROTOCOL_FEATURES))
 
 #define IOVA_TO_VVA(t, r, a, l, p)					\
 	((t)(uintptr_t)vhost_iova_to_vva(r->dev, r->vq, a, l, p))
@@ -1400,6 +1399,27 @@  vhost_crypto_complete_one_vm_requests(struct rte_crypto_op **ops,
 	return processed;
 }
 
+int
+rte_vhost_crypto_driver_start(const char *path)
+{
+	uint64_t protocol_features;
+	int ret;
+
+	ret = rte_vhost_driver_set_features(path, VIRTIO_CRYPTO_FEATURES);
+	if (ret)
+		return -1;
+
+	ret = rte_vhost_driver_get_protocol_features(path, &protocol_features);
+	if (ret)
+		return -1;
+	protocol_features |= (1ULL << VHOST_USER_PROTOCOL_F_CONFIG);
+	ret = rte_vhost_driver_set_protocol_features(path, protocol_features);
+	if (ret)
+		return -1;
+
+	return vhost_driver_start(path, VIRTIO_DEV_BUILTIN_CRYPTO);
+}
+
 int
 rte_vhost_crypto_create(int vid, uint8_t cryptodev_id,
 		struct rte_mempool *sess_pool,
@@ -1417,13 +1437,6 @@  rte_vhost_crypto_create(int vid, uint8_t cryptodev_id,
 		return -EINVAL;
 	}
 
-	ret = rte_vhost_driver_set_features(dev->ifname,
-			VIRTIO_CRYPTO_FEATURES);
-	if (ret < 0) {
-		VC_LOG_ERR("Error setting features");
-		return -1;
-	}
-
 	vcrypto = rte_zmalloc_socket(NULL, sizeof(*vcrypto),
 			RTE_CACHE_LINE_SIZE, socket_id);
 	if (!vcrypto) {