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

[Adrian Moreno]

On 7/6/20 12:29 PM, Xia, Chenbo wrote:
> Hi Adrian,
> 
>> -----Original Message-----
>> From: Adrian Moreno <amorenoz at redhat.com>
>> Sent: Monday, July 6, 2020 4:49 PM
>> To: Xia, Chenbo <chenbo.xia at intel.com>; dev at dpdk.org;
>> 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
>> Subject: Re: [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status
>> message
>>
>>
>>
>> On 7/6/20 5:22 AM, 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;
>>>> +}
>>>> +
>>>>  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;
>>>> +	    }
>>>> +
>>>
>>> There's a coding style issue because of above '}' alignment. Could you fix this?
>>>
>>> Another thing I'm not sure: if above condition happens, should it be
>>> treated as err? If set status is with replay-ack (this will happen,
>>> right?), would QEMU like to know this status is not set? As QEMU
>>> should know it during SET_FEATURES, I'm not sure whether this will
>>> also need NACK when reply-ack enabled. What's your opinion?
>>>
>>
>> My interpretation was that, since we have already NACKed SET_FEATURES,
>> SET_STATUS should only NACK if we were unable to set the status (device is not
>> present, invalid message, etc), and according to the virtio standard the driver
>> must read again and verify FEATURES_OK is still set, therefore NACKing the
>> SET_STATUS would only hide the real problem.
>>
>> Besides, for a driver (e.g: qemu) that implements the virtio/vhost logic agnostic
>> of the underlying vhost type (vhost-net or vhost-user) a spec-oriented way of
>> expressing errors is preferred. See as an example a (still unmerged) use of this
>> feature in function "static int vhost_vdpa_set_features()" in:
>>
>> https://patchwork.ozlabs.org/project/qemu-
>> devel/patch/20200701145538.22333-14-lulu at redhat.com/
>>
>> Having said all this, I realize this should be a rare case. This mechanism is in place
>> to prevent the driver from configuring an incompatible combination of features.
>> However, the vhost backend only checks that qemu has honored it's original
>> feature set which the driver must do according to the spec. So I'm happy to
>> change it if you have a strong opinion on this.
> 
> Yeah, it makes sense that we should NACK SET_FEATURES for this. So I'm fine with
> current implementation. BTW, about REPLY_ACK, does spec say something about
> which messages should set NEED_REPLY if REPLY_ACK is supported? I only see some
> msg like SET_SLAVE_REQ_FD has description about REPLY_ACK.
> 

OK. I'll resend the series addressing the other comments.

With regards to the VHOST_USER_PROTOCOL_F_REPLY_ACK feature bit, the spec says:

"With this protocol extension negotiated, the sender (QEMU) can set the
need_reply [Bit 3] flag to any command."

So, in general, the vhost backend which usually acts as "slave" (until we find a
better word), only needs to send the response if qemu has requested it.

Now, I said in general because there are some messages that are originated by
the backend if VHOST_USER_PROTOCOL_F_SLAVE_REQ is negotiated [1]. Those messages
start with "VHOST_USER_SLAVE_*". That's why you will find code that sets the
need_reply bit on those messages.

Thanks,
Adrián

[1]
https://github.com/qemu/qemu/blob/master/docs/interop/vhost-user.rst#slave-communication





>>
>>
>>> Thanks!
>>> Chenbo
>>>
>>>>  	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
> 

-- 
Adrián Moreno