[dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status message

[Adrian Moreno]

On 7/5/20 3:15 PM, Xia, Chenbo wrote:
> Hi Adrian,
> 
>> -----Original Message-----
>> From: dev <dev-bounces at dpdk.org> On Behalf Of Adrian Moreno
>> Sent: Thursday, July 2, 2020 4:33 PM
>> To: dev at dpdk.org; Ye, Xiaolong <xiaolong.ye at intel.com>;
>> shahafs at mellanox.com; matan at mellanox.com; maxime.coquelin at redhat.com;
>> Wang, Xiao W <xiao.w.wang at intel.com>; viacheslavo at mellanox.com
>> Cc: jasowang at redhat.com; lulu at redhat.com; Adrian Moreno
>> <amorenoz at redhat.com>
>> Subject: [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status
>> message
>>
>> This patch adds support to the new Virtio device get status Vhost-user message.
>>
>> The driver can send this new message to read the device status.
>>
>> One of the uses of this message is to ensure the feature negotiation has
>> succeded.  According to the virtio spec, after completing the feature negotiation,
>> the driver sets the FEATURE_OK status bit and re-reads it to ensure the device
>> has accepted the features.
>>
>> This patch also clears the FEATURE_OK status bit if the feature negotiation has
>> failed to let the driver know about his failure.
>>
>> Signed-off-by: Adrian Moreno <amorenoz at redhat.com>
>> ---
>>  lib/librte_vhost/vhost.h      |  2 ++
>>  lib/librte_vhost/vhost_user.c | 32 ++++++++++++++++++++++++++++++++
>> lib/librte_vhost/vhost_user.h |  1 +
>>  3 files changed, 35 insertions(+)
>>
>> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h index
>> 25d31c71b..e743821cc 100644
>> --- a/lib/librte_vhost/vhost.h
>> +++ b/lib/librte_vhost/vhost.h
>> @@ -32,6 +32,8 @@
>>  #define VIRTIO_DEV_BUILTIN_VIRTIO_NET 4
>>  /* Used to indicate that the device has its own data path and configured */
>> #define VIRTIO_DEV_VDPA_CONFIGURED 8
>> +/* Used to indicate that the feature negotiation failed */ #define
>> +VIRTIO_DEV_FEATURES_FAILED 16
>>
>>  /* Backend value set by guest. */
>>  #define VIRTIO_DEV_STOPPED -1
>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c index
>> 8d3d13913..00da7bf18 100644
>> --- a/lib/librte_vhost/vhost_user.c
>> +++ b/lib/librte_vhost/vhost_user.c
>> @@ -88,6 +88,7 @@ static const char *vhost_message_str[VHOST_USER_MAX]
>> = {
>>  	[VHOST_USER_GET_INFLIGHT_FD] = "VHOST_USER_GET_INFLIGHT_FD",
>>  	[VHOST_USER_SET_INFLIGHT_FD] = "VHOST_USER_SET_INFLIGHT_FD",
>>  	[VHOST_USER_SET_STATUS] = "VHOST_USER_SET_STATUS",
>> +	[VHOST_USER_GET_STATUS] = "VHOST_USER_GET_STATUS",
>>  };
>>
>>  static int send_vhost_reply(int sockfd, struct VhostUserMsg *msg); @@ -339,6
>> +340,9 @@ vhost_user_set_features(struct virtio_net **pdev, struct
>> VhostUserMsg *msg,
>>  		VHOST_LOG_CONFIG(ERR,
>>  			"(%d) received invalid negotiated features.\n",
>>  			dev->vid);
>> +		dev->flags |= VIRTIO_DEV_FEATURES_FAILED;
>> +		dev->status &= ~VIRTIO_DEVICE_STATUS_FEATURES_OK;
>> +
>>  		return RTE_VHOST_MSG_RESULT_ERR;
>>  	}
>>
>> @@ -402,6 +406,7 @@ vhost_user_set_features(struct virtio_net **pdev, struct
>> VhostUserMsg *msg,
>>  	if (vdpa_dev)
>>  		vdpa_dev->ops->set_features(dev->vid);
>>
>> +	dev->flags &= ~VIRTIO_DEV_FEATURES_FAILED;
>>  	return RTE_VHOST_MSG_RESULT_OK;
>>  }
>>
>> @@ -2458,6 +2463,22 @@ vhost_user_postcopy_end(struct virtio_net **pdev,
>> struct VhostUserMsg *msg,
>>  	return RTE_VHOST_MSG_RESULT_REPLY;
>>  }
>>
>> +static int
>> +vhost_user_get_status(struct virtio_net **pdev, struct VhostUserMsg *msg,
>> +		      int main_fd __rte_unused)
>> +{
>> +	struct virtio_net *dev = *pdev;
>> +
>> +	if (validate_msg_fds(msg, 0) != 0)
>> +		return RTE_VHOST_MSG_RESULT_ERR;
>> +
>> +	msg->payload.u64 = dev->status;
>> +	msg->size = sizeof(msg->payload.u64);
>> +	msg->fd_num = 0;
>> +
>> +	return RTE_VHOST_MSG_RESULT_OK;
> 
> Should this 'RESULT_OK' be 'RESULT_REPLY' since get_status msg needs a reply?
> 
Right! Thanks for catching it.
I'll update.

> Thanks!
> Chenbo
> 
>> +}
>> +
>>  static int
>>  vhost_user_set_status(struct virtio_net **pdev, struct VhostUserMsg *msg,
>>  			int main_fd __rte_unused)
>> @@ -2476,6 +2497,16 @@ vhost_user_set_status(struct virtio_net **pdev,
>> struct VhostUserMsg *msg,
>>
>>  	dev->status = msg->payload.u64;
>>
>> +	if ((dev->status & VIRTIO_DEVICE_STATUS_FEATURES_OK) &&
>> +	    (dev->flags & VIRTIO_DEV_FEATURES_FAILED)) {
>> +		VHOST_LOG_CONFIG(ERR, "FEATURES_OK bit is set but feature
>> negotiation failed\n");
>> +		/*
>> +		 * Clear the bit to let the driver know about the feature
>> +		 * negotiation failure
>> +		 */
>> +		dev->status &= ~VIRTIO_DEVICE_STATUS_FEATURES_OK;
>> +	    }
>> +
>>  	VHOST_LOG_CONFIG(INFO, "New device status(0x%08x):\n"
>>  			"\t-ACKNOWLEDGE: %u\n"
>>  			"\t-DRIVER: %u\n"
>> @@ -2527,6 +2558,7 @@ static vhost_message_handler_t
>> vhost_message_handlers[VHOST_USER_MAX] = {
>>  	[VHOST_USER_GET_INFLIGHT_FD] = vhost_user_get_inflight_fd,
>>  	[VHOST_USER_SET_INFLIGHT_FD] = vhost_user_set_inflight_fd,
>>  	[VHOST_USER_SET_STATUS] = vhost_user_set_status,
>> +	[VHOST_USER_GET_STATUS] = vhost_user_get_status,
>>  };
>>
>>  /* return bytes# of read on success or negative val on failure. */ diff --git
>> a/lib/librte_vhost/vhost_user.h b/lib/librte_vhost/vhost_user.h index
>> 82885ab5e..16fe03f88 100644
>> --- a/lib/librte_vhost/vhost_user.h
>> +++ b/lib/librte_vhost/vhost_user.h
>> @@ -58,6 +58,7 @@ typedef enum VhostUserRequest {
>>  	VHOST_USER_GET_INFLIGHT_FD = 31,
>>  	VHOST_USER_SET_INFLIGHT_FD = 32,
>>  	VHOST_USER_SET_STATUS = 39,
>> +	VHOST_USER_GET_STATUS = 40,
>>  	VHOST_USER_MAX = 41
>>  } VhostUserRequest;
>>
>> --
>> 2.26.2
> 

-- 
Adrián Moreno