From: "Zhang, Roy Fan" <roy.fan.zhang@intel.com> To: Maxime Coquelin <maxime.coquelin@redhat.com>, "dev@dpdk.org" <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" <stable@dpdk.org> Subject: Re: [dpdk-stable] [dpdk-dev] vhost/crypto: fix initialization. Date: Fri, 2 Oct 2020 12:38:34 +0000 Message-ID: <BL0PR11MB304309DC27F84A86114FBB6FB8310@BL0PR11MB3043.namprd11.prod.outlook.com> (raw) In-Reply-To: <28378483-f252-2482-19f0-ff7dc21dcb06@redhat.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 > >
next prev parent reply other threads:[~2020-10-02 12:38 UTC|newest] Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-02 8:36 Fan Zhang 2020-10-02 12:17 ` Maxime Coquelin 2020-10-02 12:38 ` Zhang, Roy Fan [this message] 2020-10-02 15:35 ` [dpdk-stable] [dpdk-dev v2 0/2] " Fan Zhang 2020-10-02 15:36 ` [dpdk-stable] [dpdk-dev v2 1/2] vhost: add backend type in driver start Fan Zhang 2020-10-06 7:53 ` Maxime Coquelin 2020-10-06 8:56 ` Zhang, Roy Fan 2020-10-02 15:36 ` [dpdk-stable] [dpdk-dev v2 2/2] vhost/crypto: fix feature negotiation Fan Zhang 2020-10-06 8:09 ` Maxime Coquelin 2020-10-06 8:37 ` Zhang, Roy Fan 2020-10-09 7:36 ` Maxime Coquelin 2020-10-10 9:28 ` [dpdk-stable] [dpdk-dev] " Jiang, YuX 2020-10-09 6:39 ` [dpdk-stable] " Maxime Coquelin 2020-10-09 7:24 ` Maxime Coquelin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=BL0PR11MB304309DC27F84A86114FBB6FB8310@BL0PR11MB3043.namprd11.prod.outlook.com \ --to=roy.fan.zhang@intel.com \ --cc=changpeng.liu@intel.com \ --cc=chenbo.xia@intel.com \ --cc=dev@dpdk.org \ --cc=ferruh.yigit@intel.com \ --cc=maxime.coquelin@redhat.com \ --cc=stable@dpdk.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
patches for DPDK stable branches This inbox may be cloned and mirrored by anyone: git clone --mirror https://inbox.dpdk.org/stable/0 stable/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 stable stable/ https://inbox.dpdk.org/stable \ stable@dpdk.org public-inbox-index stable Example config snippet for mirrors. Newsgroup available over NNTP: nntp://inbox.dpdk.org/inbox.dpdk.stable AGPL code for this site: git clone https://public-inbox.org/public-inbox.git