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

["Xia, Chenbo" <chenbo.xia@intel.com>]

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.

Thanks,
Chenbo

> 
> 
> > 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