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

[dpdk-dev] [RFC 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.

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

 lib/librte_vhost/rte_vhost.h           | 14 +++++++++++++
 lib/librte_vhost/rte_vhost_version.map |  1 +
 lib/librte_vhost/socket.c              | 15 ++++++++++++++
 lib/librte_vhost/vhost_user.c          | 28 ++++++++++++++------------
 4 files changed, 45 insertions(+), 13 deletions(-)

-- 
2.20.1

[dpdk-dev] [RFC 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              | 15 +++++++++++++++
 3 files changed, 30 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..129a047f6 100644
--- a/lib/librte_vhost/socket.c
+++ b/lib/librte_vhost/socket.c
@@ -707,6 +707,21 @@ 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;
+	int ret = 0;
+
+	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 2/2] vhost: support vhost-user request 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 catch the case where a request is neither handled
by the external backend nor by the vhost library.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost_user.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 36c0c676d..bae5ef1cc 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -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[req]) {
+		if (request != VHOST_USER_IOTLB_MSG)
+			RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n",
+				vhost_message_str[request]);
+		else if (
+			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,6 +1987,7 @@ vhost_user_msg_handler(int vid, int fd)
 
 	}
 
+	ret = RTE_VHOST_MSG_RESULT_ERR;
 	if (dev->extern_ops.pre_msg_handle) {
 		ret = (*dev->extern_ops.pre_msg_handle)(dev->vid,
 				(void *)&msg, &skip_master);
@@ -1997,7 +2000,6 @@ vhost_user_msg_handler(int vid, int fd)
 			goto skip_to_post_handle;
 	}
 
-	request = msg.request.master;
 	if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) {
 		if (!vhost_message_handlers[request])
 			goto skip_to_post_handle;
-- 
2.20.1

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

[Ilya Maximets]

On 27.02.2019 13:02, 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 catch the case where a request is neither handled
> by the external backend nor by the vhost library.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/librte_vhost/vhost_user.c | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 36c0c676d..bae5ef1cc 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -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[req]) {
> +		if (request != VHOST_USER_IOTLB_MSG)
> +			RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n",
> +				vhost_message_str[request]);
> +		else if (
> +			RTE_LOG(DEBUG, VHOST_CONFIG, "read message %s\n",
> +				vhost_message_str[request]);

There is no need for the 'if' without the body.

> +	} else {
> +		RTE_LOG(INFO, VHOST_CONFIG, "External request %d\n", request);

External requests could be annoying. Maybe we'll need to move them under DBG ?
I'm not sure.

> +	}
>  
>  	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,6 +1987,7 @@ vhost_user_msg_handler(int vid, int fd)
>  
>  	}
>  
> +	ret = RTE_VHOST_MSG_RESULT_ERR;

This will break the 'vhost_crypto', because it has no 'pre_msg_handler'
and master will skip to 'post_msg_handler', but it will not be called
because current status is ERR.

Maybe it's easier to introduce RTE_VHOST_MSG_RESULT_NOT_HANDLED and convert
it to ERR before the reply ?
This will require changing the pre_msg_handlers to return
RTE_VHOST_MSG_RESULT_NOT_HANDLED if message wasn't recognized.
And we'll possibly be able to drop the 'skip_master' in this case.

>  	if (dev->extern_ops.pre_msg_handle) {
>  		ret = (*dev->extern_ops.pre_msg_handle)(dev->vid,
>  				(void *)&msg, &skip_master);
> @@ -1997,7 +2000,6 @@ vhost_user_msg_handler(int vid, int fd)
>  			goto skip_to_post_handle;
>  	}
>  
> -	request = msg.request.master;
>  	if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) {
>  		if (!vhost_message_handlers[request])
>  			goto skip_to_post_handle;
> 

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

[Maxime Coquelin]

On 2/27/19 2:15 PM, Ilya Maximets wrote:
> On 27.02.2019 13:02, 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 catch the case where a request is neither handled
>> by the external backend nor by the vhost library.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>   lib/librte_vhost/vhost_user.c | 28 +++++++++++++++-------------
>>   1 file changed, 15 insertions(+), 13 deletions(-)
>>
>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>> index 36c0c676d..bae5ef1cc 100644
>> --- a/lib/librte_vhost/vhost_user.c
>> +++ b/lib/librte_vhost/vhost_user.c
>> @@ -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[req]) {
>> +		if (request != VHOST_USER_IOTLB_MSG)
>> +			RTE_LOG(INFO, VHOST_CONFIG, "read message %s\n",
>> +				vhost_message_str[request]);
>> +		else if (
>> +			RTE_LOG(DEBUG, VHOST_CONFIG, "read message %s\n",
>> +				vhost_message_str[request]);
> 
> There is no need for the 'if' without the body.

Oops, indeed. I was pretty sure I did compile test it, but looking at
the history I didn't.

>> +	} else {
>> +		RTE_LOG(INFO, VHOST_CONFIG, "External request %d\n", request);
> 
> External requests could be annoying. Maybe we'll need to move them under DBG ?
> I'm not sure.

Fair point. I'll change to DBG.

>> +	}
>>   
>>   	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,6 +1987,7 @@ vhost_user_msg_handler(int vid, int fd)
>>   
>>   	}
>>   
>> +	ret = RTE_VHOST_MSG_RESULT_ERR;
> 
> This will break the 'vhost_crypto', because it has no 'pre_msg_handler'
> and master will skip to 'post_msg_handler', but it will not be called
> because current status is ERR.

Thanks for catching it.

> Maybe it's easier to introduce RTE_VHOST_MSG_RESULT_NOT_HANDLED and convert
> it to ERR before the reply ?
> This will require changing the pre_msg_handlers to return
> RTE_VHOST_MSG_RESULT_NOT_HANDLED if message wasn't recognized.
> And we'll possibly be able to drop the 'skip_master' in this case.

Ok, that means breaking the API, but it is still experimental so not a
blocker.

I like the idea because it would also make it possible to add some debug 
prints.

I'll post new iteration this morning.

Thanks,
Maxime

>>   	if (dev->extern_ops.pre_msg_handle) {
>>   		ret = (*dev->extern_ops.pre_msg_handle)(dev->vid,
>>   				(void *)&msg, &skip_master);
>> @@ -1997,7 +2000,6 @@ vhost_user_msg_handler(int vid, int fd)
>>   			goto skip_to_post_handle;
>>   	}
>>   
>> -	request = msg.request.master;
>>   	if (request > VHOST_USER_NONE && request < VHOST_USER_MAX) {
>>   		if (!vhost_message_handlers[request])
>>   			goto skip_to_post_handle;
>>