Hi Adrian,
> -----Original Message-----
> From: Adrian Moreno <amore...@redhat.com>
> Sent: Monday, July 6, 2020 4:49 PM
> To: Xia, Chenbo <chenbo....@intel.com>; dev@dpdk.org;
> shah...@mellanox.com; ma...@mellanox.com; maxime.coque...@redhat.com;
> Wang, Xiao W <xiao.w.w...@intel.com>; viachesl...@mellanox.com
> Cc: jasow...@redhat.com; l...@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-boun...@dpdk.org> On Behalf Of Adrian Moreno
> >> Sent: Thursday, July 2, 2020 4:33 PM
> >> To: dev@dpdk.org; Ye, Xiaolong <xiaolong...@intel.com>;
> >> shah...@mellanox.com; ma...@mellanox.com;
> maxime.coque...@redhat.com;
> >> Wang, Xiao W <xiao.w.w...@intel.com>; viachesl...@mellanox.com
> >> Cc: jasow...@redhat.com; l...@redhat.com; Adrian Moreno
> >> <amore...@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 <amore...@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-l...@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
