[dpdk-dev] [RFC v2 0/2] vhost: Support external backend only vhost-user requests

[dpdk-dev] [RFC v2 0/2] vhost: Support external backend only vhost-user requests

[Maxime Coquelin]

The goals of this series is to provide more flexibility to external
backends to implement their specific vhost-user request handling
without having to patch vhost-user library.

First patch implements a new API for external backend to advertize
its specific protocol features to vhost-user master.

Second patch ensures a request not handled by the vhost-user library
but by the external backend only will not be treated as an error or
make the vhost lib to crash.

Changes in RFC v2:
==================
 - Fix build & git message
 - Add new enum for not handled messages & remove skip_master (Ilya)

Maxime Coquelin (2):
  vhost: add API to set protocol features flags
  vhost: support requests only handled by external backend

 lib/librte_vhost/rte_vhost.h           | 30 +++++++++--
 lib/librte_vhost/rte_vhost_version.map |  1 +
 lib/librte_vhost/socket.c              | 14 +++++
 lib/librte_vhost/vhost_user.c          | 75 +++++++++++++++++---------
 4 files changed, 89 insertions(+), 31 deletions(-)

-- 
2.20.1

[dpdk-dev] [RFC v2 1/2] vhost: add API to set protocol features flags

[Maxime Coquelin]

rte_vhost_driver_set_protocol_features API is to be used
by external backends to advertize vhost-user protocol
features it supports.

It has to be called after rte_vhost_driver_register() and
before rte_vhost_driver_start().

Example of usage to advertize VHOST_USER_PROTOCOL_F_FOOBAR
protocol feature:

const char *path = "/tmp/vhost-user";
uint64_t protocol_features;
rte_vhost_driver_register(path, 0);
rte_vhost_driver_get_protocol_features(path, &protocol_features);
protocol_features |= VHOST_USER_PROTOCOL_F_FOOBAR;
rte_vhost_driver_set_protocol_features(path, protocol_features);
rte_vhost_driver_start(path);

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/rte_vhost.h           | 14 ++++++++++++++
 lib/librte_vhost/rte_vhost_version.map |  1 +
 lib/librte_vhost/socket.c              | 14 ++++++++++++++
 3 files changed, 29 insertions(+)

diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
index 2753670a2..c9c392975 100644
--- a/lib/librte_vhost/rte_vhost.h
+++ b/lib/librte_vhost/rte_vhost.h
@@ -405,6 +405,20 @@ int rte_vhost_driver_disable_features(const char *path, uint64_t features);
  */
 int rte_vhost_driver_get_features(const char *path, uint64_t *features);
 
+/**
+ * Set the protocol feature bits before feature negotiation.
+ *
+ * @param path
+ *  The vhost-user socket file path
+ * @param protocol_features
+ *  Supported protocol features
+ * @return
+ *  0 on success, -1 on failure
+ */
+int __rte_experimental
+rte_vhost_driver_set_protocol_features(const char *path,
+		uint64_t protocol_features);
+
 /**
  * Get the protocol feature bits before feature negotiation.
  *
diff --git a/lib/librte_vhost/rte_vhost_version.map b/lib/librte_vhost/rte_vhost_version.map
index 8a3bc19e0..5f1d4a75c 100644
--- a/lib/librte_vhost/rte_vhost_version.map
+++ b/lib/librte_vhost/rte_vhost_version.map
@@ -86,4 +86,5 @@ EXPERIMENTAL {
 	rte_vhost_host_notifier_ctrl;
 	rte_vdpa_relay_vring_used;
 	rte_vhost_extern_callback_register;
+	rte_vhost_driver_set_protocol_features;
 };
diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
index 9883b0491..3da9de62c 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -707,6 +707,20 @@ rte_vhost_driver_get_features(const char *path, uint64_t *features)
 	return ret;
 }
 
+int
+rte_vhost_driver_set_protocol_features(const char *path,
+		uint64_t protocol_features)
+{
+	struct vhost_user_socket *vsocket;
+
+	pthread_mutex_lock(&vhost_user.mutex);
+	vsocket = find_vhost_user_socket(path);
+	if (vsocket)
+		vsocket->protocol_features = protocol_features;
+	pthread_mutex_unlock(&vhost_user.mutex);
+	return vsocket ? 0 : -1;
+}
+
 int
 rte_vhost_driver_get_protocol_features(const char *path,
 		uint64_t *protocol_features)
-- 
2.20.1

[dpdk-dev] [RFC v2 2/2] vhost: support requests only handled by external backend

[Maxime Coquelin]

External backends may have specific requests to handle, and so
we don't want the vhost-user lib to handle these requests as
errors.

This patch also changes the experimental API by introducing
RTE_VHOST_MSG_RESULT_NOT_HANDLED so that vhost-user lib
can report an error if a message is handled neither by
the vhost-user library nor by the external backend.

The logic changes a bit so that if the callback returns
with ERR, OK or REPLY, it is considered the message
is handled by the external backend so it won't be
handled by the vhost-user library.
It is still possible for an external backend to listen
to requests that have to be handled by the vhost-user
library like SET_MEM_TABLE, but the callback have to
return NOT_HANDLED in that case.

Suggested-by: Ilya Maximets <i.maximets@samsung.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/rte_vhost.h  | 16 +++++---
 lib/librte_vhost/vhost_user.c | 75 +++++++++++++++++++++++------------
 2 files changed, 60 insertions(+), 31 deletions(-)

diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
index c9c392975..b1c5a0908 100644
--- a/lib/librte_vhost/rte_vhost.h
+++ b/lib/librte_vhost/rte_vhost.h
@@ -121,6 +121,8 @@ enum rte_vhost_msg_result {
 	RTE_VHOST_MSG_RESULT_OK =  0,
 	/* Message handling successful and reply prepared */
 	RTE_VHOST_MSG_RESULT_REPLY =  1,
+	/* Message not handled */
+	RTE_VHOST_MSG_RESULT_NOT_HANDLED,
 };
 
 /**
@@ -135,11 +137,13 @@ enum rte_vhost_msg_result {
  *  If the handler requires skipping the master message handling, this variable
  *  shall be written 1, otherwise 0.
  * @return
- *  VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply,
- *  VH_RESULT_ERR on failure
+ *  RTE_VHOST_MSG_RESULT_OK on success,
+ *  RTE_VHOST_MSG_RESULT_REPLY on success with reply,
+ *  RTE_VHOST_MSG_RESULT_ERR on failure,
+ *  RTE_VHOST_MSG_RESULT_NOT_HANDLED if message was not handled.
  */
 typedef enum rte_vhost_msg_result (*rte_vhost_msg_pre_handle)(int vid,
-		void *msg, uint32_t *skip_master);
+		void *msg);
 
 /**
  * Function prototype for the vhost backend to handler specific vhost user
@@ -150,8 +154,10 @@ typedef enum rte_vhost_msg_result (*rte_vhost_msg_pre_handle)(int vid,
  * @param msg
  *  Message pointer.
  * @return
- *  VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply,
- *  VH_RESULT_ERR on failure
+ *  RTE_VHOST_MSG_RESULT_OK on success,
+ *  RTE_VHOST_MSG_RESULT_REPLY on success with reply,
+ *  RTE_VHOST_MSG_RESULT_ERR on failure,
+ *  RTE_VHOST_MSG_RESULT_NOT_HANDLED if message was not handled.
  */
 typedef enum rte_vhost_msg_result (*rte_vhost_msg_post_handle)(int vid,
 		void *msg);
diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 36c0c676d..ca9167f1d 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1906,7 +1906,7 @@ vhost_user_msg_handler(int vid, int fd)
 	int did = -1;
 	int ret;
 	int unlock_required = 0;
-	uint32_t skip_master = 0;
+	bool handled;
 	int request;
 
 	dev = get_device(vid);
@@ -1924,27 +1924,29 @@ vhost_user_msg_handler(int vid, int fd)
 	}
 
 	ret = read_vhost_message(fd, &msg);
-	if (ret <= 0 || msg.request.master >= VHOST_USER_MAX) {
+	if (ret <= 0) {
 		if (ret < 0)
 			RTE_LOG(ERR, VHOST_CONFIG,
 				"vhost read message failed\n");
-		else if (ret == 0)
+		else
 			RTE_LOG(INFO, VHOST_CONFIG,
 				"vhost peer closed\n");
-		else
-			RTE_LOG(ERR, VHOST_CONFIG,
-				"vhost read incorrect message\n");
 
 		return -1;
 	}
 
 	ret = 0;
-	if (msg.request.master != VHOST_USER_IOTLB_MSG)
-		RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n",
-			vhost_message_str[msg.request.master]);
-	else
-		RTE_LOG(DEBUG, VHOST_CONFIG, "read message %s\n",
-			vhost_message_str[msg.request.master]);
+	request = msg.request.master;
+	if (request < VHOST_USER_MAX && vhost_message_str[request]) {
+		if (request != VHOST_USER_IOTLB_MSG)
+			RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n",
+				vhost_message_str[request]);
+		else
+			RTE_LOG(DEBUG, VHOST_CONFIG, "read message %s\n",
+				vhost_message_str[request]);
+	} else {
+		RTE_LOG(INFO, VHOST_CONFIG, "External request %d\n", request);
+	}
 
 	ret = vhost_user_check_and_alloc_queue_pair(dev, &msg);
 	if (ret < 0) {
@@ -1960,7 +1962,7 @@ vhost_user_msg_handler(int vid, int fd)
 	 * inactive, so it is safe. Otherwise taking the access_lock
 	 * would cause a dead lock.
 	 */
-	switch (msg.request.master) {
+	switch (request) {
 	case VHOST_USER_SET_FEATURES:
 	case VHOST_USER_SET_PROTOCOL_FEATURES:
 	case VHOST_USER_SET_OWNER:
@@ -1985,19 +1987,23 @@ vhost_user_msg_handler(int vid, int fd)
 
 	}
 
+	handled = false;
 	if (dev->extern_ops.pre_msg_handle) {
 		ret = (*dev->extern_ops.pre_msg_handle)(dev->vid,
-				(void *)&msg, &skip_master);
-		if (ret == RTE_VHOST_MSG_RESULT_ERR)
-			goto skip_to_reply;
-		else if (ret == RTE_VHOST_MSG_RESULT_REPLY)
+				(void *)&msg);
+		switch (ret) {
+		case RTE_VHOST_MSG_RESULT_REPLY:
 			send_vhost_reply(fd, &msg);
-
-		if (skip_master)
+		case RTE_VHOST_MSG_RESULT_ERR:
+		case RTE_VHOST_MSG_RESULT_OK:
+			handled = true;
 			goto skip_to_post_handle;
+		case RTE_VHOST_MSG_RESULT_NOT_HANDLED:
+		default:
+			break;
+		}
 	}
 
-	request = msg.request.master;
 	if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) {
 		if (!vhost_message_handlers[request])
 			goto skip_to_post_handle;
@@ -2008,17 +2014,22 @@ vhost_user_msg_handler(int vid, int fd)
 			RTE_LOG(ERR, VHOST_CONFIG,
 				"Processing %s failed.\n",
 				vhost_message_str[request]);
+			handled = true;
 			break;
 		case RTE_VHOST_MSG_RESULT_OK:
 			RTE_LOG(DEBUG, VHOST_CONFIG,
 				"Processing %s succeeded.\n",
 				vhost_message_str[request]);
+			handled = true;
 			break;
 		case RTE_VHOST_MSG_RESULT_REPLY:
 			RTE_LOG(DEBUG, VHOST_CONFIG,
 				"Processing %s succeeded and needs reply.\n",
 				vhost_message_str[request]);
 			send_vhost_reply(fd, &msg);
+			handled = true;
+			break;
+		default:
 			break;
 		}
 	} else {
@@ -2030,18 +2041,30 @@ vhost_user_msg_handler(int vid, int fd)
 skip_to_post_handle:
 	if (ret != RTE_VHOST_MSG_RESULT_ERR &&
 			dev->extern_ops.post_msg_handle) {
-		ret = (*dev->extern_ops.post_msg_handle)(
-				dev->vid, (void *)&msg);
-		if (ret == RTE_VHOST_MSG_RESULT_ERR)
-			goto skip_to_reply;
-		else if (ret == RTE_VHOST_MSG_RESULT_REPLY)
+		ret = (*dev->extern_ops.post_msg_handle)(dev->vid,
+				(void *)&msg);
+		switch (ret) {
+		case RTE_VHOST_MSG_RESULT_REPLY:
 			send_vhost_reply(fd, &msg);
+		case RTE_VHOST_MSG_RESULT_ERR:
+		case RTE_VHOST_MSG_RESULT_OK:
+			handled = true;
+		case RTE_VHOST_MSG_RESULT_NOT_HANDLED:
+		default:
+			break;
+		}
 	}
 
-skip_to_reply:
 	if (unlock_required)
 		vhost_user_unlock_all_queue_pairs(dev);
 
+	/* If message was not handled at this stage, treat it as an error */
+	if (!handled) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"vhost message (req: %d) was not handled.\n", request);
+		ret = RTE_VHOST_MSG_RESULT_ERR;
+	}
+
 	/*
 	 * If the request required a reply that was already sent,
 	 * this optional reply-ack won't be sent as the
-- 
2.20.1

Re: [dpdk-dev] [RFC v2 2/2] vhost: support requests only handled by external backend

[Maxime Coquelin]

On 2/28/19 4:31 PM, Maxime Coquelin wrote:
> +	handled = false;
>   	if (dev->extern_ops.pre_msg_handle) {
>   		ret = (*dev->extern_ops.pre_msg_handle)(dev->vid,
> -				(void *)&msg, &skip_master);
> -		if (ret == RTE_VHOST_MSG_RESULT_ERR)
> -			goto skip_to_reply;
> -		else if (ret == RTE_VHOST_MSG_RESULT_REPLY)
> +				(void *)&msg);
> +		switch (ret) {
> +		case RTE_VHOST_MSG_RESULT_REPLY:

Note that I missed to add "/* Fall-through */" so that it builds with
newer GCCs.

That will be fixed in v1.

>   			send_vhost_reply(fd, &msg);
> -
> -		if (skip_master)
> +		case RTE_VHOST_MSG_RESULT_ERR:
> +		case RTE_VHOST_MSG_RESULT_OK:
> +			handled = true;
>   			goto skip_to_post_handle;
> +		case RTE_VHOST_MSG_RESULT_NOT_HANDLED:
> +		default:
> +			break;
> +		}

Re: [dpdk-dev] [RFC v2 2/2] vhost: support requests only handled by external backend

[Ilya Maximets]

On 28.02.2019 18:31, Maxime Coquelin wrote:
> External backends may have specific requests to handle, and so
> we don't want the vhost-user lib to handle these requests as
> errors.
> 
> This patch also changes the experimental API by introducing
> RTE_VHOST_MSG_RESULT_NOT_HANDLED so that vhost-user lib
> can report an error if a message is handled neither by
> the vhost-user library nor by the external backend.
> 
> The logic changes a bit so that if the callback returns
> with ERR, OK or REPLY, it is considered the message
> is handled by the external backend so it won't be
> handled by the vhost-user library.
> It is still possible for an external backend to listen
> to requests that have to be handled by the vhost-user
> library like SET_MEM_TABLE, but the callback have to
> return NOT_HANDLED in that case.
> 
> Suggested-by: Ilya Maximets <i.maximets@samsung.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/librte_vhost/rte_vhost.h  | 16 +++++---
>  lib/librte_vhost/vhost_user.c | 75 +++++++++++++++++++++++------------
>  2 files changed, 60 insertions(+), 31 deletions(-)
> 
> diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
> index c9c392975..b1c5a0908 100644
> --- a/lib/librte_vhost/rte_vhost.h
> +++ b/lib/librte_vhost/rte_vhost.h
> @@ -121,6 +121,8 @@ enum rte_vhost_msg_result {
>  	RTE_VHOST_MSG_RESULT_OK =  0,
>  	/* Message handling successful and reply prepared */
>  	RTE_VHOST_MSG_RESULT_REPLY =  1,
> +	/* Message not handled */
> +	RTE_VHOST_MSG_RESULT_NOT_HANDLED,
>  };
>  
>  /**
> @@ -135,11 +137,13 @@ enum rte_vhost_msg_result {
>   *  If the handler requires skipping the master message handling, this variable
>   *  shall be written 1, otherwise 0.
>   * @return
> - *  VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply,
> - *  VH_RESULT_ERR on failure
> + *  RTE_VHOST_MSG_RESULT_OK on success,
> + *  RTE_VHOST_MSG_RESULT_REPLY on success with reply,
> + *  RTE_VHOST_MSG_RESULT_ERR on failure,
> + *  RTE_VHOST_MSG_RESULT_NOT_HANDLED if message was not handled.
>   */
>  typedef enum rte_vhost_msg_result (*rte_vhost_msg_pre_handle)(int vid,
> -		void *msg, uint32_t *skip_master);
> +		void *msg);
>  
>  /**
>   * Function prototype for the vhost backend to handler specific vhost user
> @@ -150,8 +154,10 @@ typedef enum rte_vhost_msg_result (*rte_vhost_msg_pre_handle)(int vid,
>   * @param msg
>   *  Message pointer.
>   * @return
> - *  VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply,
> - *  VH_RESULT_ERR on failure
> + *  RTE_VHOST_MSG_RESULT_OK on success,
> + *  RTE_VHOST_MSG_RESULT_REPLY on success with reply,
> + *  RTE_VHOST_MSG_RESULT_ERR on failure,
> + *  RTE_VHOST_MSG_RESULT_NOT_HANDLED if message was not handled.
>   */
>  typedef enum rte_vhost_msg_result (*rte_vhost_msg_post_handle)(int vid,
>  		void *msg);

According to above definition, we should make corresponding change in vhost_crypto.
Something like this:
---
diff --git a/lib/librte_vhost/vhost_crypto.c b/lib/librte_vhost/vhost_crypto.c
index 0f437c4a1..f0eedd422 100644
--- a/lib/librte_vhost/vhost_crypto.c
+++ b/lib/librte_vhost/vhost_crypto.c
@@ -453,14 +453,20 @@ vhost_crypto_msg_post_handler(int vid, void *msg)
 		return RTE_VHOST_MSG_RESULT_ERR;
 	}
 
-	if (vmsg->request.master == VHOST_USER_CRYPTO_CREATE_SESS) {
+	switch (vmsg->request.master) {
+	VHOST_USER_CRYPTO_CREATE_SESS:
 		vhost_crypto_create_sess(vcrypto,
 				&vmsg->payload.crypto_session);
 		vmsg->fd_num = 0;
 		ret = RTE_VHOST_MSG_RESULT_REPLY;
-	} else if (vmsg->request.master == VHOST_USER_CRYPTO_CLOSE_SESS) {
+		break;
+	VHOST_USER_CRYPTO_CLOSE_SESS:
 		if (vhost_crypto_close_sess(vcrypto, vmsg->payload.u64))
 			ret = RTE_VHOST_MSG_RESULT_ERR;
+		break;
+	default:
+		ret = RTE_VHOST_MSG_RESULT_NOT_HANDLED;
+		break;
 	}
 
 	return ret;
---


> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 36c0c676d..ca9167f1d 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -1906,7 +1906,7 @@ vhost_user_msg_handler(int vid, int fd)
>  	int did = -1;
>  	int ret;
>  	int unlock_required = 0;
> -	uint32_t skip_master = 0;
> +	bool handled;

In below code 'handled' equals to 'false' only if 'ret' equals to
'RTE_VHOST_MSG_RESULT_NOT_HANDLED'. Looks like we don't need this
variable.

>  	int request;
>  
>  	dev = get_device(vid);
> @@ -1924,27 +1924,29 @@ vhost_user_msg_handler(int vid, int fd)
>  	}
>  
>  	ret = read_vhost_message(fd, &msg);
> -	if (ret <= 0 || msg.request.master >= VHOST_USER_MAX) {
> +	if (ret <= 0) {
>  		if (ret < 0)
>  			RTE_LOG(ERR, VHOST_CONFIG,
>  				"vhost read message failed\n");
> -		else if (ret == 0)
> +		else
>  			RTE_LOG(INFO, VHOST_CONFIG,
>  				"vhost peer closed\n");
> -		else
> -			RTE_LOG(ERR, VHOST_CONFIG,
> -				"vhost read incorrect message\n");
>  
>  		return -1;
>  	}
>  
>  	ret = 0;
> -	if (msg.request.master != VHOST_USER_IOTLB_MSG)
> -		RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n",
> -			vhost_message_str[msg.request.master]);
> -	else
> -		RTE_LOG(DEBUG, VHOST_CONFIG, "read message %s\n",
> -			vhost_message_str[msg.request.master]);
> +	request = msg.request.master;
> +	if (request < VHOST_USER_MAX && vhost_message_str[request]) {

We probably need to check for 'request > VHOST_USER_NONE' because it
has signed type.

BTW, do we heed to allow requests out of (VHOST_USER_NONE, VHOST_USER_MAX)
range? This 'if' statement reports them as 'External' requests.
However, the 'master' 'if' statement will treat them as error, printing
"Requested invalid message type".

If we don't need to handle messages out of our range, we could check the
range once at the top of this function and never check again.

> +		if (request != VHOST_USER_IOTLB_MSG)
> +			RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n",
> +				vhost_message_str[request]);
> +		else
> +			RTE_LOG(DEBUG, VHOST_CONFIG, "read message %s\n",
> +				vhost_message_str[request]);
> +	} else {
> +		RTE_LOG(INFO, VHOST_CONFIG, "External request %d\n", request);
> +	}
>  
>  	ret = vhost_user_check_and_alloc_queue_pair(dev, &msg);
>  	if (ret < 0) {
> @@ -1960,7 +1962,7 @@ vhost_user_msg_handler(int vid, int fd)
>  	 * inactive, so it is safe. Otherwise taking the access_lock
>  	 * would cause a dead lock.
>  	 */
> -	switch (msg.request.master) {
> +	switch (request) {
>  	case VHOST_USER_SET_FEATURES:
>  	case VHOST_USER_SET_PROTOCOL_FEATURES:
>  	case VHOST_USER_SET_OWNER:
> @@ -1985,19 +1987,23 @@ vhost_user_msg_handler(int vid, int fd)
>  
>  	}
>  
> +	handled = false;

'ret = RTE_VHOST_MSG_RESULT_NOT_HANDLED' instead.

>  	if (dev->extern_ops.pre_msg_handle) {
>  		ret = (*dev->extern_ops.pre_msg_handle)(dev->vid,
> -				(void *)&msg, &skip_master);
> -		if (ret == RTE_VHOST_MSG_RESULT_ERR)
> -			goto skip_to_reply;
> -		else if (ret == RTE_VHOST_MSG_RESULT_REPLY)
> +				(void *)&msg);
> +		switch (ret) {
> +		case RTE_VHOST_MSG_RESULT_REPLY:
>  			send_vhost_reply(fd, &msg);
> -
> -		if (skip_master)
> +		case RTE_VHOST_MSG_RESULT_ERR:
> +		case RTE_VHOST_MSG_RESULT_OK:
> +			handled = true;
>  			goto skip_to_post_handle;
> +		case RTE_VHOST_MSG_RESULT_NOT_HANDLED:
> +		default:
> +			break;
> +		}
>  	}
>  
> -	request = msg.request.master;
>  	if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) {
>  		if (!vhost_message_handlers[request])
>  			goto skip_to_post_handle;
> @@ -2008,17 +2014,22 @@ vhost_user_msg_handler(int vid, int fd)
>  			RTE_LOG(ERR, VHOST_CONFIG,
>  				"Processing %s failed.\n",
>  				vhost_message_str[request]);
> +			handled = true;
>  			break;
>  		case RTE_VHOST_MSG_RESULT_OK:
>  			RTE_LOG(DEBUG, VHOST_CONFIG,
>  				"Processing %s succeeded.\n",
>  				vhost_message_str[request]);
> +			handled = true;
>  			break;
>  		case RTE_VHOST_MSG_RESULT_REPLY:
>  			RTE_LOG(DEBUG, VHOST_CONFIG,
>  				"Processing %s succeeded and needs reply.\n",
>  				vhost_message_str[request]);
>  			send_vhost_reply(fd, &msg);
> +			handled = true;
> +			break;
> +		default:
>  			break;
>  		}
>  	} else {
> @@ -2030,18 +2041,30 @@ vhost_user_msg_handler(int vid, int fd)
>  skip_to_post_handle:
>  	if (ret != RTE_VHOST_MSG_RESULT_ERR &&
>  			dev->extern_ops.post_msg_handle) {
> -		ret = (*dev->extern_ops.post_msg_handle)(
> -				dev->vid, (void *)&msg);
> -		if (ret == RTE_VHOST_MSG_RESULT_ERR)
> -			goto skip_to_reply;
> -		else if (ret == RTE_VHOST_MSG_RESULT_REPLY)
> +		ret = (*dev->extern_ops.post_msg_handle)(dev->vid,
> +				(void *)&msg);
> +		switch (ret) {
> +		case RTE_VHOST_MSG_RESULT_REPLY:
>  			send_vhost_reply(fd, &msg);
> +		case RTE_VHOST_MSG_RESULT_ERR:
> +		case RTE_VHOST_MSG_RESULT_OK:
> +			handled = true;
> +		case RTE_VHOST_MSG_RESULT_NOT_HANDLED:
> +		default:
> +			break;
> +		}
>  	}
>  
> -skip_to_reply:
>  	if (unlock_required)
>  		vhost_user_unlock_all_queue_pairs(dev);
>  
> +	/* If message was not handled at this stage, treat it as an error */
> +	if (!handled) {

if (ret == RTE_VHOST_MSG_RESULT_NOT_HANDLED)

> +		RTE_LOG(ERR, VHOST_CONFIG,
> +			"vhost message (req: %d) was not handled.\n", request);
> +		ret = RTE_VHOST_MSG_RESULT_ERR;
> +	}
> +
>  	/*
>  	 * If the request required a reply that was already sent,
>  	 * this optional reply-ack won't be sent as the
> 

Re: [dpdk-dev] [RFC v2 2/2] vhost: support requests only handled by external backend

[Maxime Coquelin]

On 3/4/19 4:25 PM, Ilya Maximets wrote:
> On 28.02.2019 18:31, Maxime Coquelin wrote:
>> External backends may have specific requests to handle, and so
>> we don't want the vhost-user lib to handle these requests as
>> errors.
>>
>> This patch also changes the experimental API by introducing
>> RTE_VHOST_MSG_RESULT_NOT_HANDLED so that vhost-user lib
>> can report an error if a message is handled neither by
>> the vhost-user library nor by the external backend.
>>
>> The logic changes a bit so that if the callback returns
>> with ERR, OK or REPLY, it is considered the message
>> is handled by the external backend so it won't be
>> handled by the vhost-user library.
>> It is still possible for an external backend to listen
>> to requests that have to be handled by the vhost-user
>> library like SET_MEM_TABLE, but the callback have to
>> return NOT_HANDLED in that case.
>>
>> Suggested-by: Ilya Maximets <i.maximets@samsung.com>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>   lib/librte_vhost/rte_vhost.h  | 16 +++++---
>>   lib/librte_vhost/vhost_user.c | 75 +++++++++++++++++++++++------------
>>   2 files changed, 60 insertions(+), 31 deletions(-)
>>
>> diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
>> index c9c392975..b1c5a0908 100644
>> --- a/lib/librte_vhost/rte_vhost.h
>> +++ b/lib/librte_vhost/rte_vhost.h
>> @@ -121,6 +121,8 @@ enum rte_vhost_msg_result {
>>   	RTE_VHOST_MSG_RESULT_OK =  0,
>>   	/* Message handling successful and reply prepared */
>>   	RTE_VHOST_MSG_RESULT_REPLY =  1,
>> +	/* Message not handled */
>> +	RTE_VHOST_MSG_RESULT_NOT_HANDLED,
>>   };
>>   
>>   /**
>> @@ -135,11 +137,13 @@ enum rte_vhost_msg_result {
>>    *  If the handler requires skipping the master message handling, this variable
>>    *  shall be written 1, otherwise 0.
>>    * @return
>> - *  VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply,
>> - *  VH_RESULT_ERR on failure
>> + *  RTE_VHOST_MSG_RESULT_OK on success,
>> + *  RTE_VHOST_MSG_RESULT_REPLY on success with reply,
>> + *  RTE_VHOST_MSG_RESULT_ERR on failure,
>> + *  RTE_VHOST_MSG_RESULT_NOT_HANDLED if message was not handled.
>>    */
>>   typedef enum rte_vhost_msg_result (*rte_vhost_msg_pre_handle)(int vid,
>> -		void *msg, uint32_t *skip_master);
>> +		void *msg);
>>   
>>   /**
>>    * Function prototype for the vhost backend to handler specific vhost user
>> @@ -150,8 +154,10 @@ typedef enum rte_vhost_msg_result (*rte_vhost_msg_pre_handle)(int vid,
>>    * @param msg
>>    *  Message pointer.
>>    * @return
>> - *  VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply,
>> - *  VH_RESULT_ERR on failure
>> + *  RTE_VHOST_MSG_RESULT_OK on success,
>> + *  RTE_VHOST_MSG_RESULT_REPLY on success with reply,
>> + *  RTE_VHOST_MSG_RESULT_ERR on failure,
>> + *  RTE_VHOST_MSG_RESULT_NOT_HANDLED if message was not handled.
>>    */
>>   typedef enum rte_vhost_msg_result (*rte_vhost_msg_post_handle)(int vid,
>>   		void *msg);
> 
> According to above definition, we should make corresponding change in vhost_crypto.
> Something like this:
> ---
> diff --git a/lib/librte_vhost/vhost_crypto.c b/lib/librte_vhost/vhost_crypto.c
> index 0f437c4a1..f0eedd422 100644
> --- a/lib/librte_vhost/vhost_crypto.c
> +++ b/lib/librte_vhost/vhost_crypto.c
> @@ -453,14 +453,20 @@ vhost_crypto_msg_post_handler(int vid, void *msg)
>   		return RTE_VHOST_MSG_RESULT_ERR;
>   	}
>   
> -	if (vmsg->request.master == VHOST_USER_CRYPTO_CREATE_SESS) {
> +	switch (vmsg->request.master) {
> +	VHOST_USER_CRYPTO_CREATE_SESS:
>   		vhost_crypto_create_sess(vcrypto,
>   				&vmsg->payload.crypto_session);
>   		vmsg->fd_num = 0;
>   		ret = RTE_VHOST_MSG_RESULT_REPLY;
> -	} else if (vmsg->request.master == VHOST_USER_CRYPTO_CLOSE_SESS) {
> +		break;
> +	VHOST_USER_CRYPTO_CLOSE_SESS:
>   		if (vhost_crypto_close_sess(vcrypto, vmsg->payload.u64))
>   			ret = RTE_VHOST_MSG_RESULT_ERR;
> +		break;
> +	default:
> +		ret = RTE_VHOST_MSG_RESULT_NOT_HANDLED;
> +		break;
>   	}
>   
>   	return ret;
> ---

Indeed, it will be part of v1 if Changpeng confirms this RFC is working
for his usecase.

> 
> 
>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>> index 36c0c676d..ca9167f1d 100644
>> --- a/lib/librte_vhost/vhost_user.c
>> +++ b/lib/librte_vhost/vhost_user.c
>> @@ -1906,7 +1906,7 @@ vhost_user_msg_handler(int vid, int fd)
>>   	int did = -1;
>>   	int ret;
>>   	int unlock_required = 0;
>> -	uint32_t skip_master = 0;
>> +	bool handled;
> 
> In below code 'handled' equals to 'false' only if 'ret' equals to
> 'RTE_VHOST_MSG_RESULT_NOT_HANDLED'. Looks like we don't need this
> variable.

Actually I think it is necessary, more below.

> 
>>   	int request;
>>   
>>   	dev = get_device(vid);
>> @@ -1924,27 +1924,29 @@ vhost_user_msg_handler(int vid, int fd)
>>   	}
>>   
>>   	ret = read_vhost_message(fd, &msg);
>> -	if (ret <= 0 || msg.request.master >= VHOST_USER_MAX) {
>> +	if (ret <= 0) {
>>   		if (ret < 0)
>>   			RTE_LOG(ERR, VHOST_CONFIG,
>>   				"vhost read message failed\n");
>> -		else if (ret == 0)
>> +		else
>>   			RTE_LOG(INFO, VHOST_CONFIG,
>>   				"vhost peer closed\n");
>> -		else
>> -			RTE_LOG(ERR, VHOST_CONFIG,
>> -				"vhost read incorrect message\n");
>>   
>>   		return -1;
>>   	}
>>   
>>   	ret = 0;
>> -	if (msg.request.master != VHOST_USER_IOTLB_MSG)
>> -		RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n",
>> -			vhost_message_str[msg.request.master]);
>> -	else
>> -		RTE_LOG(DEBUG, VHOST_CONFIG, "read message %s\n",
>> -			vhost_message_str[msg.request.master]);
>> +	request = msg.request.master;
>> +	if (request < VHOST_USER_MAX && vhost_message_str[request]) {
> 
> We probably need to check for 'request > VHOST_USER_NONE' because it
> has signed type.

Agree.

> BTW, do we heed to allow requests out of (VHOST_USER_NONE, VHOST_USER_MAX)
> range? This 'if' statement reports them as 'External' requests.
> However, the 'master' 'if' statement will treat them as error, printing
> "Requested invalid message type".
> 
> If we don't need to handle messages out of our range, we could check the
> range once at the top of this function and never check again.

I think we need to handle messages out of range, otherwise external
backend may not implement new requests without patch dpdk first.

Regarding "Requested invalid message type", I think it should just be
removed. This version assumes the external backend will implement the
'pre' callback for its specific requests, but this is an uneeded
limitation and could implmeent the 'post' callback only.

>> +		if (request != VHOST_USER_IOTLB_MSG)
>> +			RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n",
>> +				vhost_message_str[request]);
>> +		else
>> +			RTE_LOG(DEBUG, VHOST_CONFIG, "read message %s\n",
>> +				vhost_message_str[request]);
>> +	} else {
>> +		RTE_LOG(INFO, VHOST_CONFIG, "External request %d\n", request);
>> +	}
>>   
>>   	ret = vhost_user_check_and_alloc_queue_pair(dev, &msg);
>>   	if (ret < 0) {
>> @@ -1960,7 +1962,7 @@ vhost_user_msg_handler(int vid, int fd)
>>   	 * inactive, so it is safe. Otherwise taking the access_lock
>>   	 * would cause a dead lock.
>>   	 */
>> -	switch (msg.request.master) {
>> +	switch (request) {
>>   	case VHOST_USER_SET_FEATURES:
>>   	case VHOST_USER_SET_PROTOCOL_FEATURES:
>>   	case VHOST_USER_SET_OWNER:
>> @@ -1985,19 +1987,23 @@ vhost_user_msg_handler(int vid, int fd)
>>   
>>   	}
>>   
>> +	handled = false;
> 
> 'ret = RTE_VHOST_MSG_RESULT_NOT_HANDLED' instead.
> 
>>   	if (dev->extern_ops.pre_msg_handle) {
>>   		ret = (*dev->extern_ops.pre_msg_handle)(dev->vid,
>> -				(void *)&msg, &skip_master);
>> -		if (ret == RTE_VHOST_MSG_RESULT_ERR)
>> -			goto skip_to_reply;
>> -		else if (ret == RTE_VHOST_MSG_RESULT_REPLY)
>> +				(void *)&msg);
>> +		switch (ret) {
>> +		case RTE_VHOST_MSG_RESULT_REPLY:
>>   			send_vhost_reply(fd, &msg);
>> -
>> -		if (skip_master)
>> +		case RTE_VHOST_MSG_RESULT_ERR:
>> +		case RTE_VHOST_MSG_RESULT_OK:
>> +			handled = true;
>>   			goto skip_to_post_handle;
>> +		case RTE_VHOST_MSG_RESULT_NOT_HANDLED:
>> +		default:
>> +			break;
>> +		}
>>   	}
>>   
>> -	request = msg.request.master;
>>   	if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) {
>>   		if (!vhost_message_handlers[request])
>>   			goto skip_to_post_handle;
>> @@ -2008,17 +2014,22 @@ vhost_user_msg_handler(int vid, int fd)
>>   			RTE_LOG(ERR, VHOST_CONFIG,
>>   				"Processing %s failed.\n",
>>   				vhost_message_str[request]);
>> +			handled = true;
>>   			break;
>>   		case RTE_VHOST_MSG_RESULT_OK:
>>   			RTE_LOG(DEBUG, VHOST_CONFIG,
>>   				"Processing %s succeeded.\n",
>>   				vhost_message_str[request]);
>> +			handled = true;
>>   			break;
>>   		case RTE_VHOST_MSG_RESULT_REPLY:
>>   			RTE_LOG(DEBUG, VHOST_CONFIG,
>>   				"Processing %s succeeded and needs reply.\n",
>>   				vhost_message_str[request]);
>>   			send_vhost_reply(fd, &msg);
>> +			handled = true;
>> +			break;
>> +		default:
>>   			break;
>>   		}
>>   	} else {
>> @@ -2030,18 +2041,30 @@ vhost_user_msg_handler(int vid, int fd)
>>   skip_to_post_handle:
>>   	if (ret != RTE_VHOST_MSG_RESULT_ERR &&
>>   			dev->extern_ops.post_msg_handle) {
>> -		ret = (*dev->extern_ops.post_msg_handle)(
>> -				dev->vid, (void *)&msg);
>> -		if (ret == RTE_VHOST_MSG_RESULT_ERR)
>> -			goto skip_to_reply;
>> -		else if (ret == RTE_VHOST_MSG_RESULT_REPLY)
>> +		ret = (*dev->extern_ops.post_msg_handle)(dev->vid,
>> +				(void *)&msg);
>> +		switch (ret) {
>> +		case RTE_VHOST_MSG_RESULT_REPLY:
>>   			send_vhost_reply(fd, &msg);
>> +		case RTE_VHOST_MSG_RESULT_ERR:
>> +		case RTE_VHOST_MSG_RESULT_OK:
>> +			handled = true;
>> +		case RTE_VHOST_MSG_RESULT_NOT_HANDLED:
>> +		default:
>> +			break;
>> +		}
>>   	}
>>   
>> -skip_to_reply:
>>   	if (unlock_required)
>>   		vhost_user_unlock_all_queue_pairs(dev);
>>   
>> +	/* If message was not handled at this stage, treat it as an error */
>> +	if (!handled) {
> 
> if (ret == RTE_VHOST_MSG_RESULT_NOT_HANDLED)

I added 'handled' variable because ret can be
RTE_VHOST_MSG_RESULT_NOT_HANDLED at this stage but the request has been
handled.

For example, vhost-user library handles the request and the external
backend implements post_msg_handle callback. If the external backend
callback does not handle this psecific request, results will be
RTE_VHOST_MSG_RESULT_NOT_HANDLED.

So 'handled' is set to true as soon as one of the 3 possible ways to
handle the request (.pre, vhost-lib, .post) handles it.

>> +		RTE_LOG(ERR, VHOST_CONFIG,
>> +			"vhost message (req: %d) was not handled.\n", request);
>> +		ret = RTE_VHOST_MSG_RESULT_ERR;
>> +	}
>> +
>>   	/*
>>   	 * If the request required a reply that was already sent,
>>   	 * this optional reply-ack won't be sent as the
>>

Re: [dpdk-dev] [RFC v2 2/2] vhost: support requests only handled by external backend

[Ilya Maximets]

On 04.03.2019 19:02, Maxime Coquelin wrote:
> 
> 
> On 3/4/19 4:25 PM, Ilya Maximets wrote:
>> On 28.02.2019 18:31, Maxime Coquelin wrote:
>>> External backends may have specific requests to handle, and so
>>> we don't want the vhost-user lib to handle these requests as
>>> errors.
>>>
>>> This patch also changes the experimental API by introducing
>>> RTE_VHOST_MSG_RESULT_NOT_HANDLED so that vhost-user lib
>>> can report an error if a message is handled neither by
>>> the vhost-user library nor by the external backend.
>>>
>>> The logic changes a bit so that if the callback returns
>>> with ERR, OK or REPLY, it is considered the message
>>> is handled by the external backend so it won't be
>>> handled by the vhost-user library.
>>> It is still possible for an external backend to listen
>>> to requests that have to be handled by the vhost-user
>>> library like SET_MEM_TABLE, but the callback have to
>>> return NOT_HANDLED in that case.
>>>
>>> Suggested-by: Ilya Maximets <i.maximets@samsung.com>
>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> ---
>>>   lib/librte_vhost/rte_vhost.h  | 16 +++++---
>>>   lib/librte_vhost/vhost_user.c | 75 +++++++++++++++++++++++------------
>>>   2 files changed, 60 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/lib/librte_vhost/rte_vhost.h b/lib/librte_vhost/rte_vhost.h
>>> index c9c392975..b1c5a0908 100644
>>> --- a/lib/librte_vhost/rte_vhost.h
>>> +++ b/lib/librte_vhost/rte_vhost.h
>>> @@ -121,6 +121,8 @@ enum rte_vhost_msg_result {
>>>       RTE_VHOST_MSG_RESULT_OK =  0,
>>>       /* Message handling successful and reply prepared */
>>>       RTE_VHOST_MSG_RESULT_REPLY =  1,
>>> +    /* Message not handled */
>>> +    RTE_VHOST_MSG_RESULT_NOT_HANDLED,
>>>   };
>>>     /**
>>> @@ -135,11 +137,13 @@ enum rte_vhost_msg_result {
>>>    *  If the handler requires skipping the master message handling, this variable
>>>    *  shall be written 1, otherwise 0.
>>>    * @return
>>> - *  VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply,
>>> - *  VH_RESULT_ERR on failure
>>> + *  RTE_VHOST_MSG_RESULT_OK on success,
>>> + *  RTE_VHOST_MSG_RESULT_REPLY on success with reply,
>>> + *  RTE_VHOST_MSG_RESULT_ERR on failure,
>>> + *  RTE_VHOST_MSG_RESULT_NOT_HANDLED if message was not handled.
>>>    */
>>>   typedef enum rte_vhost_msg_result (*rte_vhost_msg_pre_handle)(int vid,
>>> -        void *msg, uint32_t *skip_master);
>>> +        void *msg);
>>>     /**
>>>    * Function prototype for the vhost backend to handler specific vhost user
>>> @@ -150,8 +154,10 @@ typedef enum rte_vhost_msg_result (*rte_vhost_msg_pre_handle)(int vid,
>>>    * @param msg
>>>    *  Message pointer.
>>>    * @return
>>> - *  VH_RESULT_OK on success, VH_RESULT_REPLY on success with reply,
>>> - *  VH_RESULT_ERR on failure
>>> + *  RTE_VHOST_MSG_RESULT_OK on success,
>>> + *  RTE_VHOST_MSG_RESULT_REPLY on success with reply,
>>> + *  RTE_VHOST_MSG_RESULT_ERR on failure,
>>> + *  RTE_VHOST_MSG_RESULT_NOT_HANDLED if message was not handled.
>>>    */
>>>   typedef enum rte_vhost_msg_result (*rte_vhost_msg_post_handle)(int vid,
>>>           void *msg);
>>
>> According to above definition, we should make corresponding change in vhost_crypto.
>> Something like this:
>> ---
>> diff --git a/lib/librte_vhost/vhost_crypto.c b/lib/librte_vhost/vhost_crypto.c
>> index 0f437c4a1..f0eedd422 100644
>> --- a/lib/librte_vhost/vhost_crypto.c
>> +++ b/lib/librte_vhost/vhost_crypto.c
>> @@ -453,14 +453,20 @@ vhost_crypto_msg_post_handler(int vid, void *msg)
>>           return RTE_VHOST_MSG_RESULT_ERR;
>>       }
>>   -    if (vmsg->request.master == VHOST_USER_CRYPTO_CREATE_SESS) {
>> +    switch (vmsg->request.master) {
>> +    VHOST_USER_CRYPTO_CREATE_SESS:
>>           vhost_crypto_create_sess(vcrypto,
>>                   &vmsg->payload.crypto_session);
>>           vmsg->fd_num = 0;
>>           ret = RTE_VHOST_MSG_RESULT_REPLY;
>> -    } else if (vmsg->request.master == VHOST_USER_CRYPTO_CLOSE_SESS) {
>> +        break;
>> +    VHOST_USER_CRYPTO_CLOSE_SESS:
>>           if (vhost_crypto_close_sess(vcrypto, vmsg->payload.u64))
>>               ret = RTE_VHOST_MSG_RESULT_ERR;
>> +        break;
>> +    default:
>> +        ret = RTE_VHOST_MSG_RESULT_NOT_HANDLED;
>> +        break;
>>       }
>>         return ret;
>> ---
> 
> Indeed, it will be part of v1 if Changpeng confirms this RFC is working
> for his usecase.
> 
>>
>>
>>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>>> index 36c0c676d..ca9167f1d 100644
>>> --- a/lib/librte_vhost/vhost_user.c
>>> +++ b/lib/librte_vhost/vhost_user.c
>>> @@ -1906,7 +1906,7 @@ vhost_user_msg_handler(int vid, int fd)
>>>       int did = -1;
>>>       int ret;
>>>       int unlock_required = 0;
>>> -    uint32_t skip_master = 0;
>>> +    bool handled;
>>
>> In below code 'handled' equals to 'false' only if 'ret' equals to
>> 'RTE_VHOST_MSG_RESULT_NOT_HANDLED'. Looks like we don't need this
>> variable.
> 
> Actually I think it is necessary, more below.
> 
>>
>>>       int request;
>>>         dev = get_device(vid);
>>> @@ -1924,27 +1924,29 @@ vhost_user_msg_handler(int vid, int fd)
>>>       }
>>>         ret = read_vhost_message(fd, &msg);
>>> -    if (ret <= 0 || msg.request.master >= VHOST_USER_MAX) {
>>> +    if (ret <= 0) {
>>>           if (ret < 0)
>>>               RTE_LOG(ERR, VHOST_CONFIG,
>>>                   "vhost read message failed\n");
>>> -        else if (ret == 0)
>>> +        else
>>>               RTE_LOG(INFO, VHOST_CONFIG,
>>>                   "vhost peer closed\n");
>>> -        else
>>> -            RTE_LOG(ERR, VHOST_CONFIG,
>>> -                "vhost read incorrect message\n");
>>>             return -1;
>>>       }
>>>         ret = 0;
>>> -    if (msg.request.master != VHOST_USER_IOTLB_MSG)
>>> -        RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n",
>>> -            vhost_message_str[msg.request.master]);
>>> -    else
>>> -        RTE_LOG(DEBUG, VHOST_CONFIG, "read message %s\n",
>>> -            vhost_message_str[msg.request.master]);
>>> +    request = msg.request.master;
>>> +    if (request < VHOST_USER_MAX && vhost_message_str[request]) {
>>
>> We probably need to check for 'request > VHOST_USER_NONE' because it
>> has signed type.
> 
> Agree.
> 
>> BTW, do we heed to allow requests out of (VHOST_USER_NONE, VHOST_USER_MAX)
>> range? This 'if' statement reports them as 'External' requests.
>> However, the 'master' 'if' statement will treat them as error, printing
>> "Requested invalid message type".
>>
>> If we don't need to handle messages out of our range, we could check the
>> range once at the top of this function and never check again.
> 
> I think we need to handle messages out of range, otherwise external
> backend may not implement new requests without patch dpdk first.
> 
> Regarding "Requested invalid message type", I think it should just be
> removed. 

Agree.

> This version assumes the external backend will implement the
> 'pre' callback for its specific requests, but this is an uneeded
> limitation and could implmeent the 'post' callback only.

Sure. vhost_crypto has only post handler.

> 
>>> +        if (request != VHOST_USER_IOTLB_MSG)
>>> +            RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n",
>>> +                vhost_message_str[request]);
>>> +        else
>>> +            RTE_LOG(DEBUG, VHOST_CONFIG, "read message %s\n",
>>> +                vhost_message_str[request]);
>>> +    } else {
>>> +        RTE_LOG(INFO, VHOST_CONFIG, "External request %d\n", request);
>>> +    }
>>>         ret = vhost_user_check_and_alloc_queue_pair(dev, &msg);
>>>       if (ret < 0) {
>>> @@ -1960,7 +1962,7 @@ vhost_user_msg_handler(int vid, int fd)
>>>        * inactive, so it is safe. Otherwise taking the access_lock
>>>        * would cause a dead lock.
>>>        */
>>> -    switch (msg.request.master) {
>>> +    switch (request) {
>>>       case VHOST_USER_SET_FEATURES:
>>>       case VHOST_USER_SET_PROTOCOL_FEATURES:
>>>       case VHOST_USER_SET_OWNER:
>>> @@ -1985,19 +1987,23 @@ vhost_user_msg_handler(int vid, int fd)
>>>         }
>>>   +    handled = false;
>>
>> 'ret = RTE_VHOST_MSG_RESULT_NOT_HANDLED' instead.
>>
>>>       if (dev->extern_ops.pre_msg_handle) {
>>>           ret = (*dev->extern_ops.pre_msg_handle)(dev->vid,
>>> -                (void *)&msg, &skip_master);
>>> -        if (ret == RTE_VHOST_MSG_RESULT_ERR)
>>> -            goto skip_to_reply;
>>> -        else if (ret == RTE_VHOST_MSG_RESULT_REPLY)
>>> +                (void *)&msg);
>>> +        switch (ret) {
>>> +        case RTE_VHOST_MSG_RESULT_REPLY:
>>>               send_vhost_reply(fd, &msg);
>>> -
>>> -        if (skip_master)
>>> +        case RTE_VHOST_MSG_RESULT_ERR:
>>> +        case RTE_VHOST_MSG_RESULT_OK:
>>> +            handled = true;
>>>               goto skip_to_post_handle;
>>> +        case RTE_VHOST_MSG_RESULT_NOT_HANDLED:
>>> +        default:
>>> +            break;
>>> +        }
>>>       }
>>>   -    request = msg.request.master;
>>>       if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) {
>>>           if (!vhost_message_handlers[request])
>>>               goto skip_to_post_handle;
>>> @@ -2008,17 +2014,22 @@ vhost_user_msg_handler(int vid, int fd)
>>>               RTE_LOG(ERR, VHOST_CONFIG,
>>>                   "Processing %s failed.\n",
>>>                   vhost_message_str[request]);
>>> +            handled = true;
>>>               break;
>>>           case RTE_VHOST_MSG_RESULT_OK:
>>>               RTE_LOG(DEBUG, VHOST_CONFIG,
>>>                   "Processing %s succeeded.\n",
>>>                   vhost_message_str[request]);
>>> +            handled = true;
>>>               break;
>>>           case RTE_VHOST_MSG_RESULT_REPLY:
>>>               RTE_LOG(DEBUG, VHOST_CONFIG,
>>>                   "Processing %s succeeded and needs reply.\n",
>>>                   vhost_message_str[request]);
>>>               send_vhost_reply(fd, &msg);
>>> +            handled = true;
>>> +            break;
>>> +        default:
>>>               break;
>>>           }
>>>       } else {
>>> @@ -2030,18 +2041,30 @@ vhost_user_msg_handler(int vid, int fd)
>>>   skip_to_post_handle:
>>>       if (ret != RTE_VHOST_MSG_RESULT_ERR &&
>>>               dev->extern_ops.post_msg_handle) {
>>> -        ret = (*dev->extern_ops.post_msg_handle)(
>>> -                dev->vid, (void *)&msg);
>>> -        if (ret == RTE_VHOST_MSG_RESULT_ERR)
>>> -            goto skip_to_reply;
>>> -        else if (ret == RTE_VHOST_MSG_RESULT_REPLY)
>>> +        ret = (*dev->extern_ops.post_msg_handle)(dev->vid,
>>> +                (void *)&msg);
>>> +        switch (ret) {
>>> +        case RTE_VHOST_MSG_RESULT_REPLY:
>>>               send_vhost_reply(fd, &msg);
>>> +        case RTE_VHOST_MSG_RESULT_ERR:
>>> +        case RTE_VHOST_MSG_RESULT_OK:
>>> +            handled = true;
>>> +        case RTE_VHOST_MSG_RESULT_NOT_HANDLED:
>>> +        default:
>>> +            break;
>>> +        }
>>>       }
>>>   -skip_to_reply:
>>>       if (unlock_required)
>>>           vhost_user_unlock_all_queue_pairs(dev);
>>>   +    /* If message was not handled at this stage, treat it as an error */
>>> +    if (!handled) {
>>
>> if (ret == RTE_VHOST_MSG_RESULT_NOT_HANDLED)
> 
> I added 'handled' variable because ret can be
> RTE_VHOST_MSG_RESULT_NOT_HANDLED at this stage but the request has been
> handled.
> 
> For example, vhost-user library handles the request and the external
> backend implements post_msg_handle callback. If the external backend
> callback does not handle this psecific request, results will be
> RTE_VHOST_MSG_RESULT_NOT_HANDLED.
> 
> So 'handled' is set to true as soon as one of the 3 possible ways to
> handle the request (.pre, vhost-lib, .post) handles it.

Oh. I see. Thanks.

> 
>>> +        RTE_LOG(ERR, VHOST_CONFIG,
>>> +            "vhost message (req: %d) was not handled.\n", request);
>>> +        ret = RTE_VHOST_MSG_RESULT_ERR;
>>> +    }
>>> +
>>>       /*
>>>        * If the request required a reply that was already sent,
>>>        * this optional reply-ack won't be sent as the
>>>
> 
> 

Re: [dpdk-dev] [RFC v2 1/2] vhost: add API to set protocol features flags

[Stojaczyk, Dariusz]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maxime Coquelin
> Sent: Thursday, February 28, 2019 4:32 PM
> To: dev@dpdk.org; Liu, Changpeng <changpeng.liu@intel.com>; Bie, Tiwei
> <tiwei.bie@intel.com>; i.maximets@samsung.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [dpdk-dev] [RFC v2 1/2] vhost: add API to set protocol features flags
> 
> rte_vhost_driver_set_protocol_features API is to be used
> by external backends to advertize vhost-user protocol
> features it supports.
> 
> It has to be called after rte_vhost_driver_register() and
> before rte_vhost_driver_start().
> 
> Example of usage to advertize VHOST_USER_PROTOCOL_F_FOOBAR
> protocol feature:
> 
> const char *path = "/tmp/vhost-user";
> uint64_t protocol_features;
> rte_vhost_driver_register(path, 0);
> rte_vhost_driver_get_protocol_features(path, &protocol_features);
> protocol_features |= VHOST_USER_PROTOCOL_F_FOOBAR;
> rte_vhost_driver_set_protocol_features(path, protocol_features);
> rte_vhost_driver_start(path);
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---

Tested-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>

Thanks!

Re: [dpdk-dev] [RFC v2 2/2] vhost: support requests only handled by external backend

[Stojaczyk, Dariusz]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maxime Coquelin
> Sent: Thursday, February 28, 2019 4:32 PM
> To: dev@dpdk.org; Liu, Changpeng <changpeng.liu@intel.com>; Bie, Tiwei
> <tiwei.bie@intel.com>; i.maximets@samsung.com
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Subject: [dpdk-dev] [RFC v2 2/2] vhost: support requests only handled by
> external backend
> 
> External backends may have specific requests to handle, and so
> we don't want the vhost-user lib to handle these requests as
> errors.
> 
> This patch also changes the experimental API by introducing
> RTE_VHOST_MSG_RESULT_NOT_HANDLED so that vhost-user lib
> can report an error if a message is handled neither by
> the vhost-user library nor by the external backend.
> 
> The logic changes a bit so that if the callback returns
> with ERR, OK or REPLY, it is considered the message
> is handled by the external backend so it won't be
> handled by the vhost-user library.
> It is still possible for an external backend to listen
> to requests that have to be handled by the vhost-user
> library like SET_MEM_TABLE, but the callback have to
> return NOT_HANDLED in that case.
> 
> Suggested-by: Ilya Maximets <i.maximets@samsung.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---

Besides the fall-through compilation issue,

Tested-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>

Thanks!

Re: [dpdk-dev] [RFC v2 2/2] vhost: support requests only handled by external backend

[Maxime Coquelin]

On 3/8/19 10:18 AM, Stojaczyk, Dariusz wrote:
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maxime Coquelin
>> Sent: Thursday, February 28, 2019 4:32 PM
>> To: dev@dpdk.org; Liu, Changpeng <changpeng.liu@intel.com>; Bie, Tiwei
>> <tiwei.bie@intel.com>; i.maximets@samsung.com
>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Subject: [dpdk-dev] [RFC v2 2/2] vhost: support requests only handled by
>> external backend
>>
>> External backends may have specific requests to handle, and so
>> we don't want the vhost-user lib to handle these requests as
>> errors.
>>
>> This patch also changes the experimental API by introducing
>> RTE_VHOST_MSG_RESULT_NOT_HANDLED so that vhost-user lib
>> can report an error if a message is handled neither by
>> the vhost-user library nor by the external backend.
>>
>> The logic changes a bit so that if the callback returns
>> with ERR, OK or REPLY, it is considered the message
>> is handled by the external backend so it won't be
>> handled by the vhost-user library.
>> It is still possible for an external backend to listen
>> to requests that have to be handled by the vhost-user
>> library like SET_MEM_TABLE, but the callback have to
>> return NOT_HANDLED in that case.
>>
>> Suggested-by: Ilya Maximets <i.maximets@samsung.com>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
> 
> Besides the fall-through compilation issue,
> 
> Tested-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> 
> Thanks!
> 

Great, thanks Darek.
I was waiting for your feedback, I'll post v1 early next week.

Maxime