Re: [dpdk-stable] [dpdk-dev] vhost/crypto: fix initialization.

["Zhang, Roy Fan" <roy.fan.zhang@intel.com>]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Friday, October 2, 2020 1:18 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] vhost/crypto: fix initialization.
> 
> Hi Fan,
> 
> Thanks for working on this.
> 
> The commit message should not contain dot, please remove it in v2.
> 
> On 10/2/20 10:36 AM, Fan Zhang wrote:
> > This patch fixes the problem that vhost crypto cannot be
> > initialized due to the different requirement between
> > built-in virtio-net and virtio-crypto. The fix includes
> > the following change:
> >
> > - Added new internal enum type virtio_backend_type to
> > distinguish virtio-net, virtio-crypto, and external
> > device types.
> > - Added new API rte_vhost_crypto_driver_start to
> > distinguish between virtio-net and virtio-crypto built-in
> > drivers initialization.
> > - Added new internal function for the vhost library
> > to use different feature flags when initializing
> > virtio-crypto.
> 
> This last one should be part of a dedicated patch.
Will do.
> 
> 
> > Fixes: 2ab58f20db51 ("vhost: refactor virtio ready check")
> Please remove that Fixes tag.
> Looking in this patch, we can see it worked by luck. Thanks to the
> v20.08 refactoring, we spotted that Vhost crypto was broken.
> 
> > Cc: maxime.coquelin@redhat.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/socket.c              | 44 +++++++++++++++++++++-----
> >  lib/librte_vhost/vhost.h               |  1 -
> >  lib/librte_vhost/vhost_crypto.c        | 35 ++++++++++++--------
> >  lib/librte_vhost/vhost_user.h          | 12 +++++++
> >  7 files changed, 84 insertions(+), 24 deletions(-)
> >
> > diff --git a/examples/vhost_crypto/main.c
> b/examples/vhost_crypto/main.c
> > index 11b022e81..ef64e96de 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 20b4abcb4..a454d5f41 100644
> > --- a/lib/librte_vhost/rte_vhost_version.map
> > +++ b/lib/librte_vhost/rte_vhost_version.map
> > @@ -48,6 +48,7 @@ EXPERIMENTAL {
> >  	rte_vhost_get_vring_base;
> >  	rte_vhost_set_vring_base;
> >  	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/socket.c b/lib/librte_vhost/socket.c
> > index 73e1dca95..3de2da836 100644
> > --- a/lib/librte_vhost/socket.c
> > +++ b/lib/librte_vhost/socket.c
> > @@ -39,7 +39,7 @@ struct vhost_user_socket {
> >  	bool reconnect;
> >  	bool dequeue_zero_copy;
> >  	bool iommu_support;
> > -	bool use_builtin_virtio_net;
> > +	enum virtio_backend_type backend_type;
> >  	bool extbuf;
> >  	bool linearbuf;
> >  	bool async_copy;
> > @@ -225,7 +225,15 @@ vhost_user_add_connection(int fd, struct
> vhost_user_socket *vsocket)
> >  	size = strnlen(vsocket->path, PATH_MAX);
> >  	vhost_set_ifname(vid, vsocket->path, size);
> >
> > -	vhost_set_builtin_virtio_net(vid, vsocket->use_builtin_virtio_net);
> > +	vhost_set_builtin_virtio_net(vid,
> > +			vsocket->backend_type ==
> VIRTIO_DEV_BUILTIN_NET ?
> > +					true : false);
> > +
> > +	if (vsocket->backend_type == VIRTIO_DEV_BUILTIN_CRYPTO) {
> > +		vhost_crypto_set_feature_flags(&vsocket-
> >supported_features,
> > +				&vsocket->protocol_features);
> 
> It should not be done like that, we have API for that.
> We don't want to call vhost_crypto API in socket.c
> 
> Features supported by the application/backend have to be set between
> rte_vhost_driver_register() and rte_vhost_driver_start() calls.

I agree. This is actually the place I am not certain for the patch.

> 
> It think it can be done in rte_vhost_crypto_driver_start() function,
> please see below.


> 
> > +		vsocket->features = vsocket->supported_features;
> > +	}
> >
> >  	vhost_attach_vdpa_device(vid, vsocket->vdpa_dev);
> >
> > @@ -636,7 +644,7 @@ rte_vhost_driver_disable_features(const char
> *path, uint64_t features)
> >  	pthread_mutex_lock(&vhost_user.mutex);
> >  	vsocket = find_vhost_user_socket(path);
> >
> > -	/* Note that use_builtin_virtio_net is not affected by this function
> > +	/* Note that backend type is not affected by this function
> >  	 * since callers may want to selectively disable features of the
> >  	 * built-in vhost net device backend.
> >  	 */
> > @@ -685,7 +693,7 @@ rte_vhost_driver_set_features(const char *path,
> uint64_t features)
> >  		/* Anyone setting feature bits is implementing their own
> vhost
> >  		 * device backend.
> >  		 */
> > -		vsocket->use_builtin_virtio_net = false;
> > +		vsocket->backend_type = VIRTIO_DEV_UNKNOWN;
> >  	}
> >  	pthread_mutex_unlock(&vhost_user.mutex);
> >
> > @@ -913,7 +921,7 @@ rte_vhost_driver_register(const char *path,
> uint64_t flags)
> >  	 * rte_vhost_driver_set_features(), which will overwrite following
> >  	 * two values.
> >  	 */
> > -	vsocket->use_builtin_virtio_net = true;
> > +	vsocket->backend_type = VIRTIO_DEV_BUILTIN_NET;
> >  	vsocket->supported_features =
> VIRTIO_NET_SUPPORTED_FEATURES;
> >  	vsocket->features           = VIRTIO_NET_SUPPORTED_FEATURES;
> >  	vsocket->protocol_features  = VHOST_USER_PROTOCOL_FEATURES;
> > @@ -1164,10 +1172,17 @@ vhost_driver_callback_get(const char *path)
> >  }
> >
> >  int
> > -rte_vhost_driver_start(const char *path)
> > +vhost_driver_start(const char *path, enum virtio_backend_type
> backend_type)
> >  {
> >  	struct vhost_user_socket *vsocket;
> >  	static pthread_t fdset_tid;
> > +	int ret;
> > +
> > +	if (backend_type <= VIRTIO_DEV_UNKNOWN ||
> > +			backend_type > VIRTIO_DEV_BUILTIN_CRYPTO) {
> > +		VHOST_LOG_CONFIG(ERR, "Wrong backend type\n");
> > +		return -1;
> > +	}
> >
> >  	pthread_mutex_lock(&vhost_user.mutex);
> >  	vsocket = find_vhost_user_socket(path);
> > @@ -1200,7 +1215,20 @@ rte_vhost_driver_start(const char *path)
> >  	}
> >
> >  	if (vsocket->is_server)
> > -		return vhost_user_start_server(vsocket);
> > +		ret = vhost_user_start_server(vsocket);
> >  	else
> > -		return vhost_user_start_client(vsocket);
> > +		ret = vhost_user_start_client(vsocket);
> > +
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	vsocket->backend_type = backend_type;
> > +
> > +	return 0;
> > +}
> > +
> > +int
> > +rte_vhost_driver_start(const char *path)
> > +{
> > +	return vhost_driver_start(path, VIRTIO_DEV_BUILTIN_NET);
> >  }
> > diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
> > index 73a1ed889..1da9ed871 100644
> > --- a/lib/librte_vhost/vhost.h
> > +++ b/lib/librte_vhost/vhost.h
> > @@ -352,7 +352,6 @@ struct vring_packed_desc_event {
> >  				(1ULL << VIRTIO_F_IOMMU_PLATFORM) | \
> >  				(1ULL << VIRTIO_F_RING_PACKED))
> >
> > -
> >  struct guest_page {
> >  	uint64_t guest_phys_addr;
> >  	uint64_t host_phys_addr;
> > diff --git a/lib/librte_vhost/vhost_crypto.c
> b/lib/librte_vhost/vhost_crypto.c
> > index e08f9c6d7..e9c6702c2 100644
> > --- a/lib/librte_vhost/vhost_crypto.c
> > +++ b/lib/librte_vhost/vhost_crypto.c
> > @@ -35,13 +35,13 @@
> >  #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) |
> 	\
> 
> The two above lines are not Virtio features, how can it have possibly
> work in the past?

Not sure - amazingly it worked  :-). Sorry for made  the mistake in the
first place. By then it was my first virtio task.

> 
> > -		(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_PROTOCOL_F_CONFIG) |
> 	\
> 
> Ouch, VHOST_USER_PROTOCOL_F_CONFIG is not a Virtio feature, but a
> Vhost-
> user protocol feature. Please remove that in a dedicated patch.
> 
> > +		(1ULL << VHOST_USER_F_PROTOCOL_FEATURES))
> 
> Why is this now necessary? I don't see the link with the purpose of the
> patch.

Without this line 1 out of 2 times the feature negotiation will fail. I was
using Qemu 3.0.1 for my test.

> 
> I think I understand now! I bet that it just means that previously,
> VHOST_USER_GET_FEATURES was sent before Vhost-crypto would call
> rte_vhost_driver_set_features(), which makes a lot of sense as I said
> when I was surprised .new_device() was called without features not being
> negotiated.
> 
> It was really working by luck.


You are right. 

> 
> >
> >  #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 +1400,20 @@
> vhost_crypto_complete_one_vm_requests(struct rte_crypto_op **ops,
> >  	return processed;
> >  }
> >
> > +void
> > +vhost_crypto_set_feature_flags(uint64_t *feature_flags,
> > +		uint64_t *protocol_feature_flags)
> > +{
> > +	*feature_flags = VIRTIO_CRYPTO_FEATURES;
> > +	*protocol_feature_flags |= (1ULL <<
> VHOST_USER_PROTOCOL_F_CONFIG);
> > +}
> 
> So above function can be removed.
> 
> > +int
> > +rte_vhost_crypto_driver_start(const char *path)
> > +{
> Here we set supported Virtio and Vhost-usuer 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 |= 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);

Thanks. I will prepare v2 right away with your change proposed.

Regards,
Fan

> > +}
> > +
> >  int
> >  rte_vhost_crypto_create(int vid, uint8_t cryptodev_id,
> >  		struct rte_mempool *sess_pool,
> > @@ -1417,13 +1431,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 --git a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h
> > index 16fe03f88..d28da17dd 100644
> > --- a/lib/librte_vhost/vhost_user.h
> > +++ b/lib/librte_vhost/vhost_user.h
> > @@ -158,6 +158,12 @@ typedef struct VhostUserMsg {
> >  /* The version of the protocol we support */
> >  #define VHOST_USER_VERSION    0x1
> >
> > +/* virtio backend types */
> > +enum virtio_backend_type {
> > +	VIRTIO_DEV_UNKNOWN = 0, /* Likely external */
> > +	VIRTIO_DEV_BUILTIN_NET, /* Virtio-net device */
> > +	VIRTIO_DEV_BUILTIN_CRYPTO, /* Virtio-crypto device */
> > +};
> >
> >  /* vhost_user.c */
> >  int vhost_user_msg_handler(int vid, int fd);
> > @@ -167,5 +173,11 @@ int vhost_user_iotlb_miss(struct virtio_net *dev,
> uint64_t iova, uint8_t perm);
> >  int read_fd_message(int sockfd, char *buf, int buflen, int *fds, int
> max_fds,
> >  		int *fd_num);
> >  int send_fd_message(int sockfd, char *buf, int buflen, int *fds, int
> fd_num);
> > +int vhost_driver_start(const char *path,
> > +		enum virtio_backend_type backend_type);
> > +
> > +/* vhost_crypto.c */
> > +void vhost_crypto_set_feature_flags(uint64_t *feature_flags,
> > +		uint64_t *protocol_feature_flags);
> >
> >  #endif
> >