[v2,6/8] vhost: add support for virtio get status message
Checks
Commit 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(+)
Comments
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;
Should this 'RESULT_OK' be 'RESULT_REPLY' since get_status msg needs a reply?
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
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?
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
On 7/5/20 3:15 PM, 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;
>
> 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
>
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.
> 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
>
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
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
>
@@ -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
@@ -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;
+ }
+
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. */
@@ -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;