Hi Maxime,
> -----Original Message-----
> From: Maxime Coquelin <maxime.coque...@redhat.com>
> Sent: Friday, October 2, 2020 1:18 PM
> To: Zhang, Roy Fan <roy.fan.zh...@intel.com>; dev@dpdk.org
> Cc: Xia, Chenbo <chenbo....@intel.com>; Liu, Changpeng
> <changpeng....@intel.com>; Yigit, Ferruh <ferruh.yi...@intel.com>;
> sta...@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.coque...@redhat.com
> >
> > Signed-off-by: Fan Zhang <roy.fan.zh...@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
> >
