Recently, two new messages have been added to the vhost-user protocol that make the device initialization more robust. VHOST_VIRTIO_SET_STATUS allows the driver to set the device status VHOST_VIRTIO_GET_STATUS allows the driver to read the status back from the device This series implements these features in the virtio-user pmd --- v2: - Drop "net/virtio: split virtio-net and virtio status" An identical patch has already been merged - [Chenbo] Log when set-status fails Adrian Moreno (3): net/virtio: add DEVICE_NEEDS_RESET status bit net/virtio: add GET_STATUS support to virtio-user net/virtio: enable feature checking on virtio-user Maxime Coquelin (1): net/virtio: add VIRTIO_SET_STATUS support to Virtio-user drivers/net/virtio/virtio_ethdev.c | 13 ++-- drivers/net/virtio/virtio_pci.h | 13 ++-- drivers/net/virtio/virtio_user/vhost.h | 6 ++ drivers/net/virtio/virtio_user/vhost_user.c | 12 ++++ .../net/virtio/virtio_user/virtio_user_dev.c | 68 ++++++++++++++++++- .../net/virtio/virtio_user/virtio_user_dev.h | 2 + drivers/net/virtio/virtio_user_ethdev.c | 3 + 7 files changed, 104 insertions(+), 13 deletions(-) -- 2.26.2
For the sake of completeness, add the definition of the missing status bit in accordance with the virtio spec Signed-off-by: Adrian Moreno <amorenoz@redhat.com> --- drivers/net/virtio/virtio_pci.h | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h index 74ed77e33..ab61e911b 100644 --- a/drivers/net/virtio/virtio_pci.h +++ b/drivers/net/virtio/virtio_pci.h @@ -57,12 +57,13 @@ struct virtnet_ctl; #define VIRTIO_ID_9P 0x09 /* Status byte for guest to report progress. */ -#define VIRTIO_CONFIG_STATUS_RESET 0x00 -#define VIRTIO_CONFIG_STATUS_ACK 0x01 -#define VIRTIO_CONFIG_STATUS_DRIVER 0x02 -#define VIRTIO_CONFIG_STATUS_DRIVER_OK 0x04 -#define VIRTIO_CONFIG_STATUS_FEATURES_OK 0x08 -#define VIRTIO_CONFIG_STATUS_FAILED 0x80 +#define VIRTIO_CONFIG_STATUS_RESET 0x00 +#define VIRTIO_CONFIG_STATUS_ACK 0x01 +#define VIRTIO_CONFIG_STATUS_DRIVER 0x02 +#define VIRTIO_CONFIG_STATUS_DRIVER_OK 0x04 +#define VIRTIO_CONFIG_STATUS_FEATURES_OK 0x08 +#define VIRTIO_CONFIG_STATUS_DEV_NEED_RESET 0x40 +#define VIRTIO_CONFIG_STATUS_FAILED 0x80 /* * Each virtqueue indirect descriptor list must be physically contiguous. -- 2.26.2
From: Maxime Coquelin <maxime.coquelin@redhat.com> This patch adds support for VHOST_USER_SET_STATUS request. It is used to make the backend aware of Virtio devices status update. It is useful for the backend to know when the Virtio driver is done with the Virtio device configuration. Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> --- drivers/net/virtio/virtio_user/vhost.h | 6 +++++ drivers/net/virtio/virtio_user/vhost_user.c | 10 +++++++ .../net/virtio/virtio_user/virtio_user_dev.c | 26 ++++++++++++++++++- .../net/virtio/virtio_user/virtio_user_dev.h | 1 + drivers/net/virtio/virtio_user_ethdev.c | 1 + 5 files changed, 43 insertions(+), 1 deletion(-) diff --git a/drivers/net/virtio/virtio_user/vhost.h b/drivers/net/virtio/virtio_user/vhost.h index 260e1c308..8f49ef457 100644 --- a/drivers/net/virtio/virtio_user/vhost.h +++ b/drivers/net/virtio/virtio_user/vhost.h @@ -57,6 +57,10 @@ struct vhost_vring_addr { #define VHOST_USER_PROTOCOL_F_REPLY_ACK 3 #endif +#ifndef VHOST_USER_PROTOCOL_F_STATUS +#define VHOST_USER_PROTOCOL_F_STATUS 16 +#endif + enum vhost_user_request { VHOST_USER_NONE = 0, VHOST_USER_GET_FEATURES = 1, @@ -77,6 +81,8 @@ enum vhost_user_request { VHOST_USER_SET_PROTOCOL_FEATURES = 16, VHOST_USER_GET_QUEUE_NUM = 17, VHOST_USER_SET_VRING_ENABLE = 18, + VHOST_USER_SET_STATUS = 39, + VHOST_USER_GET_STATUS = 40, VHOST_USER_MAX }; diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c index ad48bafd4..337e51199 100644 --- a/drivers/net/virtio/virtio_user/vhost_user.c +++ b/drivers/net/virtio/virtio_user/vhost_user.c @@ -244,6 +244,8 @@ const char * const vhost_msg_strings[] = { [VHOST_USER_SET_VRING_ENABLE] = "VHOST_SET_VRING_ENABLE", [VHOST_USER_GET_PROTOCOL_FEATURES] = "VHOST_USER_GET_PROTOCOL_FEATURES", [VHOST_USER_SET_PROTOCOL_FEATURES] = "VHOST_USER_SET_PROTOCOL_FEATURES", + [VHOST_USER_SET_STATUS] = "VHOST_SET_STATUS", + [VHOST_USER_GET_STATUS] = "VHOST_GET_STATUS", }; static int @@ -280,6 +282,14 @@ vhost_user_sock(struct virtio_user_dev *dev, need_reply = 1; break; + case VHOST_USER_SET_STATUS: + if (!(dev->protocol_features & + (1ULL << VHOST_USER_PROTOCOL_F_STATUS))) + return 0; + + if (has_reply_ack) + msg.flags |= VHOST_USER_NEED_REPLY_MASK; + /* Fallthrough */ case VHOST_USER_SET_FEATURES: case VHOST_USER_SET_PROTOCOL_FEATURES: case VHOST_USER_SET_LOG_BASE: diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c index 0a6991bcc..670fc9d40 100644 --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c @@ -424,7 +424,8 @@ virtio_user_dev_setup(struct virtio_user_dev *dev) #define VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES \ (1ULL << VHOST_USER_PROTOCOL_F_MQ | \ - 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK) + 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK | \ + 1ULL << VHOST_USER_PROTOCOL_F_STATUS) int virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, @@ -783,3 +784,26 @@ virtio_user_handle_cq(struct virtio_user_dev *dev, uint16_t queue_idx) __atomic_add_fetch(&vring->used->idx, 1, __ATOMIC_RELAXED); } } + +int +virtio_user_send_status_update(struct virtio_user_dev *dev, uint8_t status) +{ + int ret; + uint64_t arg = status; + + /* Vhost-user only for now */ + if (!is_vhost_user_by_type(dev->path)) + return 0; + + if (!(dev->protocol_features & (1ULL << VHOST_USER_PROTOCOL_F_STATUS))) + return 0; + + ret = dev->ops->send_request(dev, VHOST_USER_SET_STATUS, &arg); + if (ret) { + PMD_INIT_LOG(ERR, "VHOST_USER_SET_STATUS failed (%d): %s", ret, + strerror(errno)); + return -1; + } + + return 0; +} diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h index 554174e81..835ab64ae 100644 --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h @@ -72,4 +72,5 @@ void virtio_user_handle_cq(struct virtio_user_dev *dev, uint16_t queue_idx); void virtio_user_handle_cq_packed(struct virtio_user_dev *dev, uint16_t queue_idx); uint8_t virtio_user_handle_mq(struct virtio_user_dev *dev, uint16_t q_pairs); +int virtio_user_send_status_update(struct virtio_user_dev *dev, uint8_t status); #endif diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c index 6003f6d50..785882e06 100644 --- a/drivers/net/virtio/virtio_user_ethdev.c +++ b/drivers/net/virtio/virtio_user_ethdev.c @@ -272,6 +272,7 @@ virtio_user_set_status(struct virtio_hw *hw, uint8_t status) else if (status == VIRTIO_CONFIG_STATUS_RESET) virtio_user_reset(hw); dev->status = status; + virtio_user_send_status_update(dev, status); } static uint8_t -- 2.26.2
This patch adds support for VHOST_USER_GET_STATUS request. Only vhost-user backed is supported for now Signed-off-by: Adrian Moreno <amorenoz@redhat.com> --- drivers/net/virtio/virtio_user/vhost_user.c | 2 + .../net/virtio/virtio_user/virtio_user_dev.c | 42 +++++++++++++++++++ .../net/virtio/virtio_user/virtio_user_dev.h | 1 + drivers/net/virtio/virtio_user_ethdev.c | 2 + 4 files changed, 47 insertions(+) diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c index 337e51199..12b6c7dbc 100644 --- a/drivers/net/virtio/virtio_user/vhost_user.c +++ b/drivers/net/virtio/virtio_user/vhost_user.c @@ -279,6 +279,7 @@ vhost_user_sock(struct virtio_user_dev *dev, switch (req) { case VHOST_USER_GET_FEATURES: case VHOST_USER_GET_PROTOCOL_FEATURES: + case VHOST_USER_GET_STATUS: need_reply = 1; break; @@ -373,6 +374,7 @@ vhost_user_sock(struct virtio_user_dev *dev, switch (req) { case VHOST_USER_GET_FEATURES: + case VHOST_USER_GET_STATUS: case VHOST_USER_GET_PROTOCOL_FEATURES: if (msg.size != sizeof(m.payload.u64)) { PMD_DRV_LOG(ERR, "Received bad msg size"); diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c index 670fc9d40..a5b2d7057 100644 --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c @@ -807,3 +807,45 @@ virtio_user_send_status_update(struct virtio_user_dev *dev, uint8_t status) return 0; } + +int +virtio_user_update_status(struct virtio_user_dev *dev) +{ + uint64_t ret; + int err; + + /* Vhost-user only for now */ + if (!is_vhost_user_by_type(dev->path)) + return 0; + + if (!(dev->protocol_features & (1UL << VHOST_USER_PROTOCOL_F_STATUS))) + return 0; + + err = dev->ops->send_request(dev, VHOST_USER_GET_STATUS, &ret); + if (err) { + PMD_INIT_LOG(ERR, "VHOST_USER_GET_STATUS failed (%d): %s", err, + strerror(errno)); + return -1; + } + if (ret > UINT8_MAX) { + PMD_INIT_LOG(ERR, "Invalid VHOST_USER_GET_STATUS response 0x%" PRIx64 "\n", ret); + return -1; + } + + dev->status = ret; + PMD_INIT_LOG(DEBUG, "Updated Device Status(0x%08x):\n" + "\t-ACKNOWLEDGE: %u\n" + "\t-DRIVER: %u\n" + "\t-DRIVER_OK: %u\n" + "\t-FEATURES_OK: %u\n" + "\t-DEVICE_NEED_RESET: %u\n" + "\t-FAILED: %u\n", + dev->status, + !!(dev->status & VIRTIO_CONFIG_STATUS_ACK), + !!(dev->status & VIRTIO_CONFIG_STATUS_DRIVER), + !!(dev->status & VIRTIO_CONFIG_STATUS_DRIVER_OK), + !!(dev->status & VIRTIO_CONFIG_STATUS_FEATURES_OK), + !!(dev->status & VIRTIO_CONFIG_STATUS_DEV_NEED_RESET), + !!(dev->status & VIRTIO_CONFIG_STATUS_FAILED)); + return 0; +} diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h index 835ab64ae..9377d5ba6 100644 --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h @@ -73,4 +73,5 @@ void virtio_user_handle_cq_packed(struct virtio_user_dev *dev, uint16_t queue_idx); uint8_t virtio_user_handle_mq(struct virtio_user_dev *dev, uint16_t q_pairs); int virtio_user_send_status_update(struct virtio_user_dev *dev, uint8_t status); +int virtio_user_update_status(struct virtio_user_dev *dev); #endif diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c index 785882e06..87f6cb695 100644 --- a/drivers/net/virtio/virtio_user_ethdev.c +++ b/drivers/net/virtio/virtio_user_ethdev.c @@ -280,6 +280,8 @@ virtio_user_get_status(struct virtio_hw *hw) { struct virtio_user_dev *dev = virtio_user_get_dev(hw); + virtio_user_update_status(dev); + return dev->status; } -- 2.26.2
virtio 1.0 introduced a mechanism for the driver to verify that the feature bits it sets are accepted by the device. This mechanism consists in setting the VIRTIO_STATUS_FEATURE_OK status bit and re-reading it, whitch gives a chance for the device to clear it if the the features were not accepted. This is currently being done only in modern virtio-pci devices but since the appropriate vhost-user messages have been added, it can also be done in virtio-user (vhost-user only). This patch activates this mechanism on virtio-user. Signed-off-by: Adrian Moreno <amorenoz@redhat.com> --- drivers/net/virtio/virtio_ethdev.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c index dc0093bdf..9063bfeb2 100644 --- a/drivers/net/virtio/virtio_ethdev.c +++ b/drivers/net/virtio/virtio_ethdev.c @@ -1355,12 +1355,13 @@ virtio_negotiate_features(struct virtio_hw *hw, uint64_t req_features) PMD_INIT_LOG(DEBUG, "features after negotiate = %" PRIx64, hw->guest_features); - if (hw->modern) { - if (!vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) { - PMD_INIT_LOG(ERR, - "VIRTIO_F_VERSION_1 features is not enabled."); - return -1; - } + if (hw->modern && !vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) { + PMD_INIT_LOG(ERR, + "VIRTIO_F_VERSION_1 features is not enabled."); + return -1; + } + + if (hw->modern || hw->virtio_user_dev) { vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK); if (!(vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_FEATURES_OK)) { PMD_INIT_LOG(ERR, -- 2.26.2
Hi Adrian, > -----Original Message----- > From: Adrian Moreno <amorenoz@redhat.com> > Sent: Tuesday, August 4, 2020 3:24 PM > To: dev@dpdk.org > Cc: Wang, Zhihong <zhihong.wang@intel.com>; Xia, Chenbo > <chenbo.xia@intel.com>; maxime.coquelin@redhat.com; Adrian Moreno > <amorenoz@redhat.com> > Subject: [PATCH v2 1/4] net/virtio: add DEVICE_NEEDS_RESET status bit > > For the sake of completeness, add the definition of the missing status > bit in accordance with the virtio spec > > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > --- > drivers/net/virtio/virtio_pci.h | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/virtio/virtio_pci.h > b/drivers/net/virtio/virtio_pci.h > index 74ed77e33..ab61e911b 100644 > --- a/drivers/net/virtio/virtio_pci.h > +++ b/drivers/net/virtio/virtio_pci.h > @@ -57,12 +57,13 @@ struct virtnet_ctl; > #define VIRTIO_ID_9P 0x09 > > /* Status byte for guest to report progress. */ > -#define VIRTIO_CONFIG_STATUS_RESET 0x00 > -#define VIRTIO_CONFIG_STATUS_ACK 0x01 > -#define VIRTIO_CONFIG_STATUS_DRIVER 0x02 > -#define VIRTIO_CONFIG_STATUS_DRIVER_OK 0x04 > -#define VIRTIO_CONFIG_STATUS_FEATURES_OK 0x08 > -#define VIRTIO_CONFIG_STATUS_FAILED 0x80 > +#define VIRTIO_CONFIG_STATUS_RESET 0x00 > +#define VIRTIO_CONFIG_STATUS_ACK 0x01 > +#define VIRTIO_CONFIG_STATUS_DRIVER 0x02 > +#define VIRTIO_CONFIG_STATUS_DRIVER_OK 0x04 > +#define VIRTIO_CONFIG_STATUS_FEATURES_OK 0x08 > +#define VIRTIO_CONFIG_STATUS_DEV_NEED_RESET 0x40 > +#define VIRTIO_CONFIG_STATUS_FAILED 0x80 > > /* > * Each virtqueue indirect descriptor list must be physically contiguous. Should we also define RESET(0x00) in vhost lib? I don't see the definition there. Correct me if I'm wrong. For this patch: Reviewed-by: Chenbo Xia <chenbo.xia@intel.com> > -- > 2.26.2
> -----Original Message-----
> From: Adrian Moreno <amorenoz@redhat.com>
> Sent: Tuesday, August 4, 2020 3:24 PM
> To: dev@dpdk.org
> Cc: Wang, Zhihong <zhihong.wang@intel.com>; Xia, Chenbo
> <chenbo.xia@intel.com>; maxime.coquelin@redhat.com; Adrian Moreno
> <amorenoz@redhat.com>
> Subject: [PATCH v2 2/4] net/virtio: add VIRTIO_SET_STATUS support to
> Virtio-user
>
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>
> This patch adds support for VHOST_USER_SET_STATUS
> request. It is used to make the backend aware of
> Virtio devices status update.
>
> It is useful for the backend to know when the Virtio
> driver is done with the Virtio device configuration.
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
> drivers/net/virtio/virtio_user/vhost.h | 6 +++++
> drivers/net/virtio/virtio_user/vhost_user.c | 10 +++++++
> .../net/virtio/virtio_user/virtio_user_dev.c | 26 ++++++++++++++++++-
> .../net/virtio/virtio_user/virtio_user_dev.h | 1 +
> drivers/net/virtio/virtio_user_ethdev.c | 1 +
> 5 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio/virtio_user/vhost.h
> b/drivers/net/virtio/virtio_user/vhost.h
> index 260e1c308..8f49ef457 100644
> --- a/drivers/net/virtio/virtio_user/vhost.h
> +++ b/drivers/net/virtio/virtio_user/vhost.h
> @@ -57,6 +57,10 @@ struct vhost_vring_addr {
> #define VHOST_USER_PROTOCOL_F_REPLY_ACK 3
> #endif
>
> +#ifndef VHOST_USER_PROTOCOL_F_STATUS
> +#define VHOST_USER_PROTOCOL_F_STATUS 16
> +#endif
> +
> enum vhost_user_request {
> VHOST_USER_NONE = 0,
> VHOST_USER_GET_FEATURES = 1,
> @@ -77,6 +81,8 @@ enum vhost_user_request {
> VHOST_USER_SET_PROTOCOL_FEATURES = 16,
> VHOST_USER_GET_QUEUE_NUM = 17,
> VHOST_USER_SET_VRING_ENABLE = 18,
> + VHOST_USER_SET_STATUS = 39,
> + VHOST_USER_GET_STATUS = 40,
> VHOST_USER_MAX
> };
>
> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c
> b/drivers/net/virtio/virtio_user/vhost_user.c
> index ad48bafd4..337e51199 100644
> --- a/drivers/net/virtio/virtio_user/vhost_user.c
> +++ b/drivers/net/virtio/virtio_user/vhost_user.c
> @@ -244,6 +244,8 @@ const char * const vhost_msg_strings[] = {
> [VHOST_USER_SET_VRING_ENABLE] = "VHOST_SET_VRING_ENABLE",
> [VHOST_USER_GET_PROTOCOL_FEATURES] =
> "VHOST_USER_GET_PROTOCOL_FEATURES",
> [VHOST_USER_SET_PROTOCOL_FEATURES] =
> "VHOST_USER_SET_PROTOCOL_FEATURES",
> + [VHOST_USER_SET_STATUS] = "VHOST_SET_STATUS",
> + [VHOST_USER_GET_STATUS] = "VHOST_GET_STATUS",
> };
>
> static int
> @@ -280,6 +282,14 @@ vhost_user_sock(struct virtio_user_dev *dev,
> need_reply = 1;
> break;
>
> + case VHOST_USER_SET_STATUS:
> + if (!(dev->protocol_features &
> + (1ULL << VHOST_USER_PROTOCOL_F_STATUS)))
> + return 0;
> +
> + if (has_reply_ack)
> + msg.flags |= VHOST_USER_NEED_REPLY_MASK;
> + /* Fallthrough */
> case VHOST_USER_SET_FEATURES:
> case VHOST_USER_SET_PROTOCOL_FEATURES:
> case VHOST_USER_SET_LOG_BASE:
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index 0a6991bcc..670fc9d40 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -424,7 +424,8 @@ virtio_user_dev_setup(struct virtio_user_dev *dev)
>
> #define VIRTIO_USER_SUPPORTED_PROTOCOL_FEATURES \
> (1ULL << VHOST_USER_PROTOCOL_F_MQ | \
> - 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK)
> + 1ULL << VHOST_USER_PROTOCOL_F_REPLY_ACK | \
> + 1ULL << VHOST_USER_PROTOCOL_F_STATUS)
>
> int
> virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
> @@ -783,3 +784,26 @@ virtio_user_handle_cq(struct virtio_user_dev *dev,
> uint16_t queue_idx)
> __atomic_add_fetch(&vring->used->idx, 1, __ATOMIC_RELAXED);
> }
> }
> +
> +int
> +virtio_user_send_status_update(struct virtio_user_dev *dev, uint8_t
> status)
> +{
> + int ret;
> + uint64_t arg = status;
> +
> + /* Vhost-user only for now */
> + if (!is_vhost_user_by_type(dev->path))
> + return 0;
> +
> + if (!(dev->protocol_features & (1ULL <<
> VHOST_USER_PROTOCOL_F_STATUS)))
> + return 0;
> +
> + ret = dev->ops->send_request(dev, VHOST_USER_SET_STATUS, &arg);
> + if (ret) {
> + PMD_INIT_LOG(ERR, "VHOST_USER_SET_STATUS failed (%d): %s", ret,
> + strerror(errno));
> + return -1;
> + }
> +
> + return 0;
> +}
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> index 554174e81..835ab64ae 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> @@ -72,4 +72,5 @@ void virtio_user_handle_cq(struct virtio_user_dev *dev,
> uint16_t queue_idx);
> void virtio_user_handle_cq_packed(struct virtio_user_dev *dev,
> uint16_t queue_idx);
> uint8_t virtio_user_handle_mq(struct virtio_user_dev *dev, uint16_t
> q_pairs);
> +int virtio_user_send_status_update(struct virtio_user_dev *dev, uint8_t
> status);
> #endif
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> b/drivers/net/virtio/virtio_user_ethdev.c
> index 6003f6d50..785882e06 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -272,6 +272,7 @@ virtio_user_set_status(struct virtio_hw *hw, uint8_t
> status)
> else if (status == VIRTIO_CONFIG_STATUS_RESET)
> virtio_user_reset(hw);
> dev->status = status;
> + virtio_user_send_status_update(dev, status);
> }
>
> static uint8_t
> --
> 2.26.2
Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
Hi Adrian, > -----Original Message----- > From: Adrian Moreno <amorenoz@redhat.com> > Sent: Tuesday, August 4, 2020 3:25 PM > To: dev@dpdk.org > Cc: Wang, Zhihong <zhihong.wang@intel.com>; Xia, Chenbo > <chenbo.xia@intel.com>; maxime.coquelin@redhat.com; Adrian Moreno > <amorenoz@redhat.com> > Subject: [PATCH v2 3/4] net/virtio: add GET_STATUS support to virtio-user > > This patch adds support for VHOST_USER_GET_STATUS request. > > Only vhost-user backed is supported for now > > Signed-off-by: Adrian Moreno <amorenoz@redhat.com> > --- > drivers/net/virtio/virtio_user/vhost_user.c | 2 + > .../net/virtio/virtio_user/virtio_user_dev.c | 42 +++++++++++++++++++ > .../net/virtio/virtio_user/virtio_user_dev.h | 1 + > drivers/net/virtio/virtio_user_ethdev.c | 2 + > 4 files changed, 47 insertions(+) > > diff --git a/drivers/net/virtio/virtio_user/vhost_user.c > b/drivers/net/virtio/virtio_user/vhost_user.c > index 337e51199..12b6c7dbc 100644 > --- a/drivers/net/virtio/virtio_user/vhost_user.c > +++ b/drivers/net/virtio/virtio_user/vhost_user.c > @@ -279,6 +279,7 @@ vhost_user_sock(struct virtio_user_dev *dev, > switch (req) { > case VHOST_USER_GET_FEATURES: > case VHOST_USER_GET_PROTOCOL_FEATURES: > + case VHOST_USER_GET_STATUS: > need_reply = 1; > break; > > @@ -373,6 +374,7 @@ vhost_user_sock(struct virtio_user_dev *dev, > > switch (req) { > case VHOST_USER_GET_FEATURES: > + case VHOST_USER_GET_STATUS: > case VHOST_USER_GET_PROTOCOL_FEATURES: > if (msg.size != sizeof(m.payload.u64)) { > PMD_DRV_LOG(ERR, "Received bad msg size"); > diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c > b/drivers/net/virtio/virtio_user/virtio_user_dev.c > index 670fc9d40..a5b2d7057 100644 > --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c > +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c > @@ -807,3 +807,45 @@ virtio_user_send_status_update(struct virtio_user_dev > *dev, uint8_t status) > > return 0; > } > + > +int > +virtio_user_update_status(struct virtio_user_dev *dev) > +{ > + uint64_t ret; > + int err; > + > + /* Vhost-user only for now */ > + if (!is_vhost_user_by_type(dev->path)) > + return 0; > + > + if (!(dev->protocol_features & (1UL << > VHOST_USER_PROTOCOL_F_STATUS))) > + return 0; > + > + err = dev->ops->send_request(dev, VHOST_USER_GET_STATUS, &ret); > + if (err) { > + PMD_INIT_LOG(ERR, "VHOST_USER_GET_STATUS failed (%d): %s", err, > + strerror(errno)); > + return -1; > + } > + if (ret > UINT8_MAX) { > + PMD_INIT_LOG(ERR, "Invalid VHOST_USER_GET_STATUS response 0x%" > PRIx64 "\n", ret); > + return -1; > + } > + > + dev->status = ret; > + PMD_INIT_LOG(DEBUG, "Updated Device Status(0x%08x):\n" > + "\t-ACKNOWLEDGE: %u\n" > + "\t-DRIVER: %u\n" > + "\t-DRIVER_OK: %u\n" > + "\t-FEATURES_OK: %u\n" > + "\t-DEVICE_NEED_RESET: %u\n" > + "\t-FAILED: %u\n", > + dev->status, > + !!(dev->status & VIRTIO_CONFIG_STATUS_ACK), > + !!(dev->status & VIRTIO_CONFIG_STATUS_DRIVER), > + !!(dev->status & VIRTIO_CONFIG_STATUS_DRIVER_OK), > + !!(dev->status & VIRTIO_CONFIG_STATUS_FEATURES_OK), > + !!(dev->status & VIRTIO_CONFIG_STATUS_DEV_NEED_RESET), > + !!(dev->status & VIRTIO_CONFIG_STATUS_FAILED)); > + return 0; > +} Should we also add RESET in this log? I just did a simple test and it shows status could be updated to 0x00. Thanks! Chenbo > diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h > b/drivers/net/virtio/virtio_user/virtio_user_dev.h > index 835ab64ae..9377d5ba6 100644 > --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h > +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h > @@ -73,4 +73,5 @@ void virtio_user_handle_cq_packed(struct virtio_user_dev > *dev, > uint16_t queue_idx); > uint8_t virtio_user_handle_mq(struct virtio_user_dev *dev, uint16_t > q_pairs); > int virtio_user_send_status_update(struct virtio_user_dev *dev, uint8_t > status); > +int virtio_user_update_status(struct virtio_user_dev *dev); > #endif > diff --git a/drivers/net/virtio/virtio_user_ethdev.c > b/drivers/net/virtio/virtio_user_ethdev.c > index 785882e06..87f6cb695 100644 > --- a/drivers/net/virtio/virtio_user_ethdev.c > +++ b/drivers/net/virtio/virtio_user_ethdev.c > @@ -280,6 +280,8 @@ virtio_user_get_status(struct virtio_hw *hw) > { > struct virtio_user_dev *dev = virtio_user_get_dev(hw); > > + virtio_user_update_status(dev); > + > return dev->status; > } > > -- > 2.26.2
> -----Original Message-----
> From: Adrian Moreno <amorenoz@redhat.com>
> Sent: Tuesday, August 4, 2020 3:25 PM
> To: dev@dpdk.org
> Cc: Wang, Zhihong <zhihong.wang@intel.com>; Xia, Chenbo
> <chenbo.xia@intel.com>; maxime.coquelin@redhat.com; Adrian Moreno
> <amorenoz@redhat.com>
> Subject: [PATCH v2 4/4] net/virtio: enable feature checking on virtio-user
>
> virtio 1.0 introduced a mechanism for the driver to verify that the
> feature bits it sets are accepted by the device. This mechanism consists
> in setting the VIRTIO_STATUS_FEATURE_OK status bit and re-reading it,
> whitch gives a chance for the device to clear it if the the features
> were not accepted.
>
> This is currently being done only in modern virtio-pci devices but since
> the appropriate vhost-user messages have been added, it can also be done
> in virtio-user (vhost-user only).
>
> This patch activates this mechanism on virtio-user.
>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
> drivers/net/virtio/virtio_ethdev.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/virtio/virtio_ethdev.c
> b/drivers/net/virtio/virtio_ethdev.c
> index dc0093bdf..9063bfeb2 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1355,12 +1355,13 @@ virtio_negotiate_features(struct virtio_hw *hw,
> uint64_t req_features)
> PMD_INIT_LOG(DEBUG, "features after negotiate = %" PRIx64,
> hw->guest_features);
>
> - if (hw->modern) {
> - if (!vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) {
> - PMD_INIT_LOG(ERR,
> - "VIRTIO_F_VERSION_1 features is not enabled.");
> - return -1;
> - }
> + if (hw->modern && !vtpci_with_feature(hw, VIRTIO_F_VERSION_1)) {
> + PMD_INIT_LOG(ERR,
> + "VIRTIO_F_VERSION_1 features is not enabled.");
> + return -1;
> + }
> +
> + if (hw->modern || hw->virtio_user_dev) {
> vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
> if (!(vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_FEATURES_OK))
> {
> PMD_INIT_LOG(ERR,
> --
> 2.26.2
Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
On 8/5/20 12:55 PM, Xia, Chenbo wrote: > Hi Adrian, > >> -----Original Message----- >> From: Adrian Moreno <amorenoz@redhat.com> >> Sent: Tuesday, August 4, 2020 3:25 PM >> To: dev@dpdk.org >> Cc: Wang, Zhihong <zhihong.wang@intel.com>; Xia, Chenbo >> <chenbo.xia@intel.com>; maxime.coquelin@redhat.com; Adrian Moreno >> <amorenoz@redhat.com> >> Subject: [PATCH v2 3/4] net/virtio: add GET_STATUS support to virtio-user >> >> This patch adds support for VHOST_USER_GET_STATUS request. >> >> Only vhost-user backed is supported for now >> >> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >> --- >> drivers/net/virtio/virtio_user/vhost_user.c | 2 + >> .../net/virtio/virtio_user/virtio_user_dev.c | 42 +++++++++++++++++++ >> .../net/virtio/virtio_user/virtio_user_dev.h | 1 + >> drivers/net/virtio/virtio_user_ethdev.c | 2 + >> 4 files changed, 47 insertions(+) >> >> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c >> b/drivers/net/virtio/virtio_user/vhost_user.c >> index 337e51199..12b6c7dbc 100644 >> --- a/drivers/net/virtio/virtio_user/vhost_user.c >> +++ b/drivers/net/virtio/virtio_user/vhost_user.c >> @@ -279,6 +279,7 @@ vhost_user_sock(struct virtio_user_dev *dev, >> switch (req) { >> case VHOST_USER_GET_FEATURES: >> case VHOST_USER_GET_PROTOCOL_FEATURES: >> + case VHOST_USER_GET_STATUS: >> need_reply = 1; >> break; >> >> @@ -373,6 +374,7 @@ vhost_user_sock(struct virtio_user_dev *dev, >> >> switch (req) { >> case VHOST_USER_GET_FEATURES: >> + case VHOST_USER_GET_STATUS: >> case VHOST_USER_GET_PROTOCOL_FEATURES: >> if (msg.size != sizeof(m.payload.u64)) { >> PMD_DRV_LOG(ERR, "Received bad msg size"); >> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c >> b/drivers/net/virtio/virtio_user/virtio_user_dev.c >> index 670fc9d40..a5b2d7057 100644 >> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c >> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c >> @@ -807,3 +807,45 @@ virtio_user_send_status_update(struct virtio_user_dev >> *dev, uint8_t status) >> >> return 0; >> } >> + >> +int >> +virtio_user_update_status(struct virtio_user_dev *dev) >> +{ >> + uint64_t ret; >> + int err; >> + >> + /* Vhost-user only for now */ >> + if (!is_vhost_user_by_type(dev->path)) >> + return 0; >> + >> + if (!(dev->protocol_features & (1UL << >> VHOST_USER_PROTOCOL_F_STATUS))) >> + return 0; >> + >> + err = dev->ops->send_request(dev, VHOST_USER_GET_STATUS, &ret); >> + if (err) { >> + PMD_INIT_LOG(ERR, "VHOST_USER_GET_STATUS failed (%d): %s", err, >> + strerror(errno)); >> + return -1; >> + } >> + if (ret > UINT8_MAX) { >> + PMD_INIT_LOG(ERR, "Invalid VHOST_USER_GET_STATUS response 0x%" >> PRIx64 "\n", ret); >> + return -1; >> + } >> + >> + dev->status = ret; >> + PMD_INIT_LOG(DEBUG, "Updated Device Status(0x%08x):\n" >> + "\t-ACKNOWLEDGE: %u\n" >> + "\t-DRIVER: %u\n" >> + "\t-DRIVER_OK: %u\n" >> + "\t-FEATURES_OK: %u\n" >> + "\t-DEVICE_NEED_RESET: %u\n" >> + "\t-FAILED: %u\n", >> + dev->status, >> + !!(dev->status & VIRTIO_CONFIG_STATUS_ACK), >> + !!(dev->status & VIRTIO_CONFIG_STATUS_DRIVER), >> + !!(dev->status & VIRTIO_CONFIG_STATUS_DRIVER_OK), >> + !!(dev->status & VIRTIO_CONFIG_STATUS_FEATURES_OK), >> + !!(dev->status & VIRTIO_CONFIG_STATUS_DEV_NEED_RESET), >> + !!(dev->status & VIRTIO_CONFIG_STATUS_FAILED)); >> + return 0; >> +} > > Should we also add RESET in this log? I just did a simple test and it shows > status could be updated to 0x00. > Yep. I'll add it. Thanks > Thanks! > Chenbo > >> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h >> b/drivers/net/virtio/virtio_user/virtio_user_dev.h >> index 835ab64ae..9377d5ba6 100644 >> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h >> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h >> @@ -73,4 +73,5 @@ void virtio_user_handle_cq_packed(struct virtio_user_dev >> *dev, >> uint16_t queue_idx); >> uint8_t virtio_user_handle_mq(struct virtio_user_dev *dev, uint16_t >> q_pairs); >> int virtio_user_send_status_update(struct virtio_user_dev *dev, uint8_t >> status); >> +int virtio_user_update_status(struct virtio_user_dev *dev); >> #endif >> diff --git a/drivers/net/virtio/virtio_user_ethdev.c >> b/drivers/net/virtio/virtio_user_ethdev.c >> index 785882e06..87f6cb695 100644 >> --- a/drivers/net/virtio/virtio_user_ethdev.c >> +++ b/drivers/net/virtio/virtio_user_ethdev.c >> @@ -280,6 +280,8 @@ virtio_user_get_status(struct virtio_hw *hw) >> { >> struct virtio_user_dev *dev = virtio_user_get_dev(hw); >> >> + virtio_user_update_status(dev); >> + >> return dev->status; >> } >> >> -- >> 2.26.2 > -- Adrián Moreno
On 8/5/20 12:52 PM, Xia, Chenbo wrote: > Hi Adrian, > >> -----Original Message----- >> From: Adrian Moreno <amorenoz@redhat.com> >> Sent: Tuesday, August 4, 2020 3:24 PM >> To: dev@dpdk.org >> Cc: Wang, Zhihong <zhihong.wang@intel.com>; Xia, Chenbo >> <chenbo.xia@intel.com>; maxime.coquelin@redhat.com; Adrian Moreno >> <amorenoz@redhat.com> >> Subject: [PATCH v2 1/4] net/virtio: add DEVICE_NEEDS_RESET status bit >> >> For the sake of completeness, add the definition of the missing status >> bit in accordance with the virtio spec >> >> Signed-off-by: Adrian Moreno <amorenoz@redhat.com> >> --- >> drivers/net/virtio/virtio_pci.h | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/net/virtio/virtio_pci.h >> b/drivers/net/virtio/virtio_pci.h >> index 74ed77e33..ab61e911b 100644 >> --- a/drivers/net/virtio/virtio_pci.h >> +++ b/drivers/net/virtio/virtio_pci.h >> @@ -57,12 +57,13 @@ struct virtnet_ctl; >> #define VIRTIO_ID_9P 0x09 >> >> /* Status byte for guest to report progress. */ >> -#define VIRTIO_CONFIG_STATUS_RESET 0x00 >> -#define VIRTIO_CONFIG_STATUS_ACK 0x01 >> -#define VIRTIO_CONFIG_STATUS_DRIVER 0x02 >> -#define VIRTIO_CONFIG_STATUS_DRIVER_OK 0x04 >> -#define VIRTIO_CONFIG_STATUS_FEATURES_OK 0x08 >> -#define VIRTIO_CONFIG_STATUS_FAILED 0x80 >> +#define VIRTIO_CONFIG_STATUS_RESET 0x00 >> +#define VIRTIO_CONFIG_STATUS_ACK 0x01 >> +#define VIRTIO_CONFIG_STATUS_DRIVER 0x02 >> +#define VIRTIO_CONFIG_STATUS_DRIVER_OK 0x04 >> +#define VIRTIO_CONFIG_STATUS_FEATURES_OK 0x08 >> +#define VIRTIO_CONFIG_STATUS_DEV_NEED_RESET 0x40 >> +#define VIRTIO_CONFIG_STATUS_FAILED 0x80 >> >> /* >> * Each virtqueue indirect descriptor list must be physically contiguous. > > Should we also define RESET(0x00) in vhost lib? I don't see the definition > there. Correct me if I'm wrong. > I'll add it in a separate patch if that's OK. > For this patch: > > Reviewed-by: Chenbo Xia <chenbo.xia@intel.com> > >> -- >> 2.26.2 > Thanks -- Adrián Moreno