DPDK patches and discussions
 help / color / mirror / Atom feed
From: Adrian Moreno <amorenoz@redhat.com>
To: "Xia, Chenbo" <chenbo.xia@intel.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	"shahafs@mellanox.com" <shahafs@mellanox.com>,
	"matan@mellanox.com" <matan@mellanox.com>,
	"maxime.coquelin@redhat.com" <maxime.coquelin@redhat.com>,
	"Wang, Xiao W" <xiao.w.wang@intel.com>,
	"viacheslavo@mellanox.com" <viacheslavo@mellanox.com>
Cc: "jasowang@redhat.com" <jasowang@redhat.com>,
	"lulu@redhat.com" <lulu@redhat.com>
Subject: Re: [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status message
Date: Mon, 6 Jul 2020 12:45:11 +0200	[thread overview]
Message-ID: <eefa0a37-865d-cc62-18f8-b4d7a26e76a5@redhat.com> (raw)
In-Reply-To: <MN2PR11MB406343FCEBD40278204B80B29C690@MN2PR11MB4063.namprd11.prod.outlook.com>



On 7/6/20 12:29 PM, Xia, Chenbo wrote:
> Hi Adrian,
> 
>> -----Original Message-----
>> From: Adrian Moreno <amorenoz@redhat.com>
>> Sent: Monday, July 6, 2020 4:49 PM
>> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org;
>> shahafs@mellanox.com; matan@mellanox.com; maxime.coquelin@redhat.com;
>> Wang, Xiao W <xiao.w.wang@intel.com>; viacheslavo@mellanox.com
>> Cc: jasowang@redhat.com; lulu@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@dpdk.org> On Behalf Of Adrian Moreno
>>>> Sent: Thursday, July 2, 2020 4:33 PM
>>>> To: dev@dpdk.org; Ye, Xiaolong <xiaolong.ye@intel.com>;
>>>> shahafs@mellanox.com; matan@mellanox.com;
>> maxime.coquelin@redhat.com;
>>>> Wang, Xiao W <xiao.w.wang@intel.com>; viacheslavo@mellanox.com
>>>> Cc: jasowang@redhat.com; lulu@redhat.com; Adrian Moreno
>>>> <amorenoz@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@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@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


  reply	other threads:[~2020-07-06 10:45 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-02  8:32 [dpdk-dev] [PATCH (v20.08) v2 0/8] vhost: improve Vhost/vDPA device init Adrian Moreno
2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 1/8] vhost: fix virtio ready flag check Adrian Moreno
2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 2/8] vhost: refactor Virtio ready check Adrian Moreno
2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 3/8] vhost: make some vDPA callbacks mandatory Adrian Moreno
2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 4/8] vhost: check vDPA configuration succeed Adrian Moreno
2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 5/8] vhost: add support for virtio status Adrian Moreno
2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 6/8] vhost: add support for virtio get status message Adrian Moreno
2020-07-05 13:15   ` Xia, Chenbo
2020-07-06  8:15     ` Adrian Moreno
2020-07-06  3:22   ` Xia, Chenbo
2020-07-06  8:48     ` Adrian Moreno
2020-07-06 10:29       ` Xia, Chenbo
2020-07-06 10:45         ` Adrian Moreno [this message]
2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 7/8] vdpa/ifc: enable status protocol feature Adrian Moreno
2020-07-02  8:32 ` [dpdk-dev] [PATCH v2 8/8] vdpa/mlx5: " Adrian Moreno

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=eefa0a37-865d-cc62-18f8-b4d7a26e76a5@redhat.com \
    --to=amorenoz@redhat.com \
    --cc=chenbo.xia@intel.com \
    --cc=dev@dpdk.org \
    --cc=jasowang@redhat.com \
    --cc=lulu@redhat.com \
    --cc=matan@mellanox.com \
    --cc=maxime.coquelin@redhat.com \
    --cc=shahafs@mellanox.com \
    --cc=viacheslavo@mellanox.com \
    --cc=xiao.w.wang@intel.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).