[dpdk-dev] [PATCH 0/5] Add support for GET/SET_STATUS on virtio-user pmd

[dpdk-dev] [PATCH 0/5] Add support for GET/SET_STATUS on virtio-user pmd

[Adrian Moreno]

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

Adrian Moreno (4):
  net/virtio: split virtio-net and virtio status
  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  | 62 ++++++++++++++++++-
 .../net/virtio/virtio_user/virtio_user_dev.h  |  7 ++-
 drivers/net/virtio/virtio_user_ethdev.c       | 14 +++--
 7 files changed, 108 insertions(+), 19 deletions(-)

-- 
2.26.2

[dpdk-dev] [PATCH 1/5] net/virtio: split virtio-net and virtio status

[Adrian Moreno]

Currently, the same field is being used to store both the generic virtio
device status and the virtio-net-specific status.

This can become problematic since they have different sets of status
bits that may collide (e.g: VIRTIO_CONFIG_STATUS_ACK and
VIRTIO_NET_S_LINK_UP).

Split them in different fields.

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 drivers/net/virtio/virtio_user/virtio_user_dev.h |  5 ++++-
 drivers/net/virtio/virtio_user_ethdev.c          | 11 ++++++-----
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h
index 56e638f8a..10b274d19 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
@@ -43,7 +43,10 @@ struct virtio_user_dev {
 	uint64_t	protocol_features; /* negotiated protocol features
 					    * (Vhost-user only)
 					    */
-	uint8_t		status;
+
+	uint8_t		virtio_net_status; /* virtio-net device status */
+	uint8_t		status;		   /* virtio device status */
+
 	uint16_t	port_id;
 	uint8_t		mac_addr[RTE_ETHER_ADDR_LEN];
 	char		path[PATH_MAX];
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index e51425c4f..5b06d8e89 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -205,7 +205,8 @@ virtio_user_read_dev_config(struct virtio_hw *hw, size_t offset,
 			}
 			r = recv(dev->vhostfd, buf, 128, MSG_PEEK);
 			if (r == 0 || (r < 0 && errno != EAGAIN)) {
-				dev->status &= (~VIRTIO_NET_S_LINK_UP);
+				dev->virtio_net_status &=
+					(~VIRTIO_NET_S_LINK_UP);
 				PMD_DRV_LOG(ERR, "virtio-user port %u is down",
 					    hw->port_id);
 
@@ -217,7 +218,7 @@ virtio_user_read_dev_config(struct virtio_hw *hw, size_t offset,
 						  virtio_user_delayed_handler,
 						  (void *)hw);
 			} else {
-				dev->status |= VIRTIO_NET_S_LINK_UP;
+				dev->virtio_net_status |= VIRTIO_NET_S_LINK_UP;
 			}
 			if (fcntl(dev->vhostfd, F_SETFL,
 					flags & ~O_NONBLOCK) == -1) {
@@ -225,12 +226,12 @@ virtio_user_read_dev_config(struct virtio_hw *hw, size_t offset,
 				return;
 			}
 		} else if (dev->is_server) {
-			dev->status &= (~VIRTIO_NET_S_LINK_UP);
+			dev->virtio_net_status &= (~VIRTIO_NET_S_LINK_UP);
 			if (virtio_user_server_reconnect(dev) >= 0)
-				dev->status |= VIRTIO_NET_S_LINK_UP;
+				dev->virtio_net_status |= VIRTIO_NET_S_LINK_UP;
 		}
 
-		*(uint16_t *)dst = dev->status;
+		*(uint16_t *)dst = dev->virtio_net_status;
 	}
 
 	if (offset == offsetof(struct virtio_net_config, max_virtqueue_pairs))
-- 
2.26.2

[dpdk-dev] [PATCH 2/5] net/virtio: add DEVICE_NEEDS_RESET status bit

[Adrian Moreno]

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

Re: [dpdk-dev] [PATCH 2/5] net/virtio: add DEVICE_NEEDS_RESET status bit

[Xia, Chenbo]

Hi Adrian,

> -----Original Message-----
> From: Adrian Moreno <amorenoz@redhat.com>
> Sent: Thursday, July 16, 2020 1:18 AM
> To: dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>;
> amorenoz@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
> Subject: [PATCH 2/5] 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

Do we need to delete ' VIRTIO_CONFIG_STATUS_RESET'? I see vhost lib does not
have it now. And I read virtio 1.1 spec and find the description of writing 0 to
reset device is deleted. I think virtio 1.1 spec is not very clear about reset status.
Does 'DEV_NEED_RESET' equal old 'RESET'?

Thanks!
Chenbo 

> 
>  /*
>   * Each virtqueue indirect descriptor list must be physically contiguous.
> --
> 2.26.2

Re: [dpdk-dev] [PATCH 2/5] net/virtio: add DEVICE_NEEDS_RESET status bit

[Adrian Moreno]

Hi,

On 7/16/20 4:26 AM, Xia, Chenbo wrote:
> Hi Adrian,
> 
>> -----Original Message-----
>> From: Adrian Moreno <amorenoz@redhat.com>
>> Sent: Thursday, July 16, 2020 1:18 AM
>> To: dev@dpdk.org
>> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>;
>> amorenoz@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
>> Subject: [PATCH 2/5] 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
> 
> Do we need to delete ' VIRTIO_CONFIG_STATUS_RESET'? I see vhost lib does not
> have it now. And I read virtio 1.1 spec and find the description of writing 0 to
> reset device is deleted. I think virtio 1.1 spec is not very clear about reset status.
> Does 'DEV_NEED_RESET' equal old 'RESET'?
> 
In virtio 1.1
"2.1.2 Device Requirements: Device Status Field

The device MUST initialize device status to 0 upon reset.
...
"
So I think having "#define VIRTIO_CONFIG_STATUS_RESET 0x00" is still useful to
understand what is going on in:

void
vtpci_reset(struct virtio_hw *hw)
{
	VTPCI_OPS(hw)->set_status(hw, VIRTIO_CONFIG_STATUS_RESET);
	/* flush status write */
	VTPCI_OPS(hw)->get_status(hw);
}

DEV_NEED_RESET is used for the device to notify that it has encountered an
unrecoverable error. Therefore, the driver would never
"set_status(VIRTIO_CONFIG_STATUS_DEV_NEED_RESET)"; instead, it should read the
status and if this bit is set, reset the device.


> Thanks!
> Chenbo 
> 
>>
>>  /*
>>   * Each virtqueue indirect descriptor list must be physically contiguous.
>> --
>> 2.26.2
> 

-- 
Adrián Moreno

Re: [dpdk-dev] [PATCH 2/5] net/virtio: add DEVICE_NEEDS_RESET status bit

[Xia, Chenbo]

Hi Adrian,

> -----Original Message-----
> From: Adrian Moreno <amorenoz@redhat.com>
> Sent: Thursday, July 16, 2020 3:34 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>;
> Li, Miao <miao.li@intel.com>
> Subject: Re: [PATCH 2/5] net/virtio: add DEVICE_NEEDS_RESET status bit
> 
> Hi,
> 
> On 7/16/20 4:26 AM, Xia, Chenbo wrote:
> > Hi Adrian,
> >
> >> -----Original Message-----
> >> From: Adrian Moreno <amorenoz@redhat.com>
> >> Sent: Thursday, July 16, 2020 1:18 AM
> >> To: dev@dpdk.org
> >> Cc: maxime.coquelin@redhat.com; Wang, Zhihong
> >> <zhihong.wang@intel.com>; amorenoz@redhat.com; Xia, Chenbo
> >> <chenbo.xia@intel.com>
> >> Subject: [PATCH 2/5] 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
> >
> > Do we need to delete ' VIRTIO_CONFIG_STATUS_RESET'? I see vhost lib
> > does not have it now. And I read virtio 1.1 spec and find the
> > description of writing 0 to reset device is deleted. I think virtio 1.1 spec is not
> very clear about reset status.
> > Does 'DEV_NEED_RESET' equal old 'RESET'?
> >
> In virtio 1.1
> "2.1.2 Device Requirements: Device Status Field
> 
> The device MUST initialize device status to 0 upon reset.
> ...
> "
> So I think having "#define VIRTIO_CONFIG_STATUS_RESET 0x00" is still useful to
> understand what is going on in:
> 
> void
> vtpci_reset(struct virtio_hw *hw)
> {
> 	VTPCI_OPS(hw)->set_status(hw, VIRTIO_CONFIG_STATUS_RESET);
> 	/* flush status write */
> 	VTPCI_OPS(hw)->get_status(hw);
> }
> 
> DEV_NEED_RESET is used for the device to notify that it has encountered an
> unrecoverable error. Therefore, the driver would never
> "set_status(VIRTIO_CONFIG_STATUS_DEV_NEED_RESET)"; instead, it should
> read the status and if this bit is set, reset the device.

Yes, you are correct! I missed that part. Btw, should we add STATUS_RESET to
Vhost lib? I see there's no reset status now.

Thanks!
Chenbo

> 
> 
> > Thanks!
> > Chenbo
> >
> >>
> >>  /*
> >>   * Each virtqueue indirect descriptor list must be physically contiguous.
> >> --
> >> 2.26.2
> >
> 
> --
> Adrián Moreno

[dpdk-dev] [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-user

[Adrian Moreno]

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  | 23 ++++++++++++++++++-
 .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
 drivers/net/virtio/virtio_user_ethdev.c       |  1 +
 5 files changed, 40 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 631d0e353..2332e01d1 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..2c400a448 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,23 @@ 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)
+		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 10b274d19..a76d7cfaf 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
@@ -74,4 +74,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 5b06d8e89..e52f11341 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -273,6 +273,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

Re: [dpdk-dev] [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-user

[Xia, Chenbo]

Hi Adrian,

> -----Original Message-----
> From: Adrian Moreno <amorenoz@redhat.com>
> Sent: Thursday, July 16, 2020 1:18 AM
> To: dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>;
> amorenoz@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
> Subject: [PATCH 3/5] 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  | 23 ++++++++++++++++++-
>   .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
>  drivers/net/virtio/virtio_user_ethdev.c       |  1 +
>  5 files changed, 40 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 631d0e353..2332e01d1 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 */

There's a coding style issue here:
WARNING:PREFER_FALLTHROUGH: Prefer 'fallthrough;' over fallthrough comment.
Could you fix this?

>  	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..2c400a448 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,23 @@ 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)
> +		return -1;

Do you think we should add a log here to show if SET_STAUTS failed?

Thanks!
Chenbo

> +
> +	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 10b274d19..a76d7cfaf 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> @@ -74,4 +74,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 5b06d8e89..e52f11341 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -273,6 +273,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

Re: [dpdk-dev] [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-user

[Adrian Moreno]

On 7/16/20 5:15 AM, Xia, Chenbo wrote:
> Hi Adrian,
> 
>> -----Original Message-----
>> From: Adrian Moreno <amorenoz@redhat.com>
>> Sent: Thursday, July 16, 2020 1:18 AM
>> To: dev@dpdk.org
>> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>;
>> amorenoz@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
>> Subject: [PATCH 3/5] 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  | 23 ++++++++++++++++++-
>>   .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
>>  drivers/net/virtio/virtio_user_ethdev.c       |  1 +
>>  5 files changed, 40 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 631d0e353..2332e01d1 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 */
> 
> There's a coding style issue here:
> WARNING:PREFER_FALLTHROUGH: Prefer 'fallthrough;' over fallthrough comment.
> Could you fix this?
> 
"fallthrough" is not defined. I think this macro is defined in the linux kernel
(where checkpatches.pl comes from?). We could define the same macro that would
depend in compiler support for __falthrough__ attribute.

In any case, this is not the only case:

$ find -name *.c | xargs grep -i "/\* fallthrough \*/" | wc -l
62

So maybe this warning is new?
Should I change all of them together in another patch?

>>  	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..2c400a448 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,23 @@ 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)
>> +		return -1;
> 
> Do you think we should add a log here to show if SET_STAUTS failed?
> 
Good idea! Will do in the next version. Thanks

> Thanks!
> Chenbo
> 
>> +
>> +	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 10b274d19..a76d7cfaf 100644
>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> @@ -74,4 +74,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 5b06d8e89..e52f11341 100644
>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>> @@ -273,6 +273,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
> 

-- 
Adrián Moreno

Re: [dpdk-dev] [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-user

[Xia, Chenbo]

Hi Adrian,

> -----Original Message-----
> From: Adrian Moreno <amorenoz@redhat.com>
> Sent: Thursday, July 16, 2020 3:43 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>
> Subject: Re: [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-
> user
> 
> 
> 
> On 7/16/20 5:15 AM, Xia, Chenbo wrote:
> > Hi Adrian,
> >
> >> -----Original Message-----
> >> From: Adrian Moreno <amorenoz@redhat.com>
> >> Sent: Thursday, July 16, 2020 1:18 AM
> >> To: dev@dpdk.org
> >> Cc: maxime.coquelin@redhat.com; Wang, Zhihong
> >> <zhihong.wang@intel.com>; amorenoz@redhat.com; Xia, Chenbo
> >> <chenbo.xia@intel.com>
> >> Subject: [PATCH 3/5] 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  | 23 ++++++++++++++++++-
> >>   .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
> >>  drivers/net/virtio/virtio_user_ethdev.c       |  1 +
> >>  5 files changed, 40 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 631d0e353..2332e01d1 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 */
> >
> > There's a coding style issue here:
> > WARNING:PREFER_FALLTHROUGH: Prefer 'fallthrough;' over fallthrough
> comment.
> > Could you fix this?
> >
> "fallthrough" is not defined. I think this macro is defined in the linux kernel
> (where checkpatches.pl comes from?). We could define the same macro that
> would depend in compiler support for __falthrough__ attribute.
> 
> In any case, this is not the only case:
> 
> $ find -name *.c | xargs grep -i "/\* fallthrough \*/" | wc -l
> 62
> 
> So maybe this warning is new?
> Should I change all of them together in another patch?

checkpatches.pl is in linux kernel. I think it prefers 'fallthrough' rather than
'Fallthrough'. I think it's a linux style, right?

Thanks!
Chenbo

> 
> >>  	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..2c400a448 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,23 @@ 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)
> >> +		return -1;
> >
> > Do you think we should add a log here to show if SET_STAUTS failed?
> >
> Good idea! Will do in the next version. Thanks
> 
> > Thanks!
> > Chenbo
> >
> >> +
> >> +	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 10b274d19..a76d7cfaf 100644
> >> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> >> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> >> @@ -74,4 +74,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 5b06d8e89..e52f11341 100644
> >> --- a/drivers/net/virtio/virtio_user_ethdev.c
> >> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> >> @@ -273,6 +273,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
> >
> 
> --
> Adrián Moreno

Re: [dpdk-dev] [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-user

[Adrian Moreno]

On 7/16/20 10:58 AM, Xia, Chenbo wrote:
> Hi Adrian,
> 
>> -----Original Message-----
>> From: Adrian Moreno <amorenoz@redhat.com>
>> Sent: Thursday, July 16, 2020 3:43 PM
>> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
>> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>
>> Subject: Re: [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-
>> user
>>
>>
>>
>> On 7/16/20 5:15 AM, Xia, Chenbo wrote:
>>> Hi Adrian,
>>>
>>>> -----Original Message-----
>>>> From: Adrian Moreno <amorenoz@redhat.com>
>>>> Sent: Thursday, July 16, 2020 1:18 AM
>>>> To: dev@dpdk.org
>>>> Cc: maxime.coquelin@redhat.com; Wang, Zhihong
>>>> <zhihong.wang@intel.com>; amorenoz@redhat.com; Xia, Chenbo
>>>> <chenbo.xia@intel.com>
>>>> Subject: [PATCH 3/5] 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  | 23 ++++++++++++++++++-
>>>>   .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
>>>>  drivers/net/virtio/virtio_user_ethdev.c       |  1 +
>>>>  5 files changed, 40 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 631d0e353..2332e01d1 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 */
>>>
>>> There's a coding style issue here:
>>> WARNING:PREFER_FALLTHROUGH: Prefer 'fallthrough;' over fallthrough
>> comment.
>>> Could you fix this?
>>>
>> "fallthrough" is not defined. I think this macro is defined in the linux kernel
>> (where checkpatches.pl comes from?). We could define the same macro that
>> would depend in compiler support for __falthrough__ attribute.
>>
>> In any case, this is not the only case:
>>
>> $ find -name *.c | xargs grep -i "/\* fallthrough \*/" | wc -l
>> 62
>>
>> So maybe this warning is new?
>> Should I change all of them together in another patch?
> 
> checkpatches.pl is in linux kernel. I think it prefers 'fallthrough' rather than
> 'Fallthrough'. I think it's a linux style, right?
> 
It refers to this:
https://github.com/torvalds/linux/blob/91a9a90d040e8b9ff63d48ea71468e0f4db764ff/include/linux/compiler_attributes.h#L201

Since we don't have that pseudo-keyword, we should see if we can disable this in
checkpatches.


> Thanks!
> Chenbo
> 
>>
>>>>  	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..2c400a448 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,23 @@ 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)
>>>> +		return -1;
>>>
>>> Do you think we should add a log here to show if SET_STAUTS failed?
>>>
>> Good idea! Will do in the next version. Thanks
>>
>>> Thanks!
>>> Chenbo
>>>
>>>> +
>>>> +	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 10b274d19..a76d7cfaf 100644
>>>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
>>>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
>>>> @@ -74,4 +74,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 5b06d8e89..e52f11341 100644
>>>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>>>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>>>> @@ -273,6 +273,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
>>>
>>
>> --
>> Adrián Moreno
> 

-- 
Adrián Moreno

Re: [dpdk-dev] [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-user

[Xia, Chenbo]

Hi Adrian,

> -----Original Message-----
> From: Adrian Moreno <amorenoz@redhat.com>
> Sent: Thursday, July 16, 2020 5:51 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>
> Subject: Re: [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to Virtio-
> user
> 
> 
> 
> On 7/16/20 10:58 AM, Xia, Chenbo wrote:
> > Hi Adrian,
> >
> >> -----Original Message-----
> >> From: Adrian Moreno <amorenoz@redhat.com>
> >> Sent: Thursday, July 16, 2020 3:43 PM
> >> To: Xia, Chenbo <chenbo.xia@intel.com>; dev@dpdk.org
> >> Cc: maxime.coquelin@redhat.com; Wang, Zhihong
> >> <zhihong.wang@intel.com>
> >> Subject: Re: [PATCH 3/5] net/virtio: add VIRTIO_SET_STATUS support to
> >> Virtio- user
> >>
> >>
> >>
> >> On 7/16/20 5:15 AM, Xia, Chenbo wrote:
> >>> Hi Adrian,
> >>>
> >>>> -----Original Message-----
> >>>> From: Adrian Moreno <amorenoz@redhat.com>
> >>>> Sent: Thursday, July 16, 2020 1:18 AM
> >>>> To: dev@dpdk.org
> >>>> Cc: maxime.coquelin@redhat.com; Wang, Zhihong
> >>>> <zhihong.wang@intel.com>; amorenoz@redhat.com; Xia, Chenbo
> >>>> <chenbo.xia@intel.com>
> >>>> Subject: [PATCH 3/5] 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  | 23 ++++++++++++++++++-
> >>>>   .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
> >>>>  drivers/net/virtio/virtio_user_ethdev.c       |  1 +
> >>>>  5 files changed, 40 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 631d0e353..2332e01d1 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 */
> >>>
> >>> There's a coding style issue here:
> >>> WARNING:PREFER_FALLTHROUGH: Prefer 'fallthrough;' over fallthrough
> >> comment.
> >>> Could you fix this?
> >>>
> >> "fallthrough" is not defined. I think this macro is defined in the
> >> linux kernel (where checkpatches.pl comes from?). We could define the
> >> same macro that would depend in compiler support for __falthrough__
> attribute.
> >>
> >> In any case, this is not the only case:
> >>
> >> $ find -name *.c | xargs grep -i "/\* fallthrough \*/" | wc -l
> >> 62
> >>
> >> So maybe this warning is new?
> >> Should I change all of them together in another patch?
> >
> > checkpatches.pl is in linux kernel. I think it prefers 'fallthrough'
> > rather than 'Fallthrough'. I think it's a linux style, right?
> >
> It refers to this:
> https://github.com/torvalds/linux/blob/91a9a90d040e8b9ff63d48ea71468e0f4
> db764ff/include/linux/compiler_attributes.h#L201
> 
> Since we don't have that pseudo-keyword, we should see if we can disable this
> in checkpatches.

Yeah, I confirmed this could be ignored. So you could change it or keep it.
Both fine.

Cheers,
Chenbo

> 
> 
> > Thanks!
> > Chenbo
> >
> >>
> >>>>  	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..2c400a448 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,23 @@ 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)
> >>>> +		return -1;
> >>>
> >>> Do you think we should add a log here to show if SET_STAUTS failed?
> >>>
> >> Good idea! Will do in the next version. Thanks
> >>
> >>> Thanks!
> >>> Chenbo
> >>>
> >>>> +
> >>>> +	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 10b274d19..a76d7cfaf 100644
> >>>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> >>>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> >>>> @@ -74,4 +74,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 5b06d8e89..e52f11341 100644
> >>>> --- a/drivers/net/virtio/virtio_user_ethdev.c
> >>>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> >>>> @@ -273,6 +273,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
> >>>
> >>
> >> --
> >> Adrián Moreno
> >
> 
> --
> Adrián Moreno

[dpdk-dev] [PATCH 4/5] net/virtio: add GET_STATUS support to virtio-user

[Adrian Moreno]

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  | 39 +++++++++++++++++++
 .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
 drivers/net/virtio/virtio_user_ethdev.c       |  2 +
 4 files changed, 44 insertions(+)

diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c
index 2332e01d1..43c630e47 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 2c400a448..613caef56 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -804,3 +804,42 @@ 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;
+
+	/* 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;
+
+	if (dev->ops->send_request(dev, VHOST_USER_GET_STATUS, &ret) < 0) {
+		PMD_INIT_LOG(ERR, "GET_STATUS failed: %s", 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 a76d7cfaf..1a045af6f 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
@@ -75,4 +75,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 e52f11341..40330deed 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -281,6 +281,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

Re: [dpdk-dev] [PATCH 4/5] net/virtio: add GET_STATUS support to virtio-user

[Xia, Chenbo]

Hi Adrian,

> -----Original Message-----
> From: Adrian Moreno <amorenoz@redhat.com>
> Sent: Thursday, July 16, 2020 1:18 AM
> To: dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>;
> amorenoz@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
> Subject: [PATCH 4/5] 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  | 39
> +++++++++++++++++++  .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
>  drivers/net/virtio/virtio_user_ethdev.c       |  2 +
>  4 files changed, 44 insertions(+)
> 
> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c
> b/drivers/net/virtio/virtio_user/vhost_user.c
> index 2332e01d1..43c630e47 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 2c400a448..613caef56 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -804,3 +804,42 @@ 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;
> +
> +	/* 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;
> +
> +	if (dev->ops->send_request(dev, VHOST_USER_GET_STATUS, &ret) < 0) {
> +		PMD_INIT_LOG(ERR, "GET_STATUS failed: %s", strerror(errno));
> +		return -1;
> +	}
> +	if (ret > UINT8_MAX) {
> +		PMD_INIT_LOG(ERR, "Invalid VHOST_USER_GET_STATUS
> response 0x%" PRIx64 "\n", ret);

There's a coding style issue here:
CHECK:CAMELCASE: Avoid CamelCase: <PRIx64>
Could you fix this?

Btw, in case you don't know, there's a script in dpdk for check these issues:
devtools/checkpatches.sh. 😊

Thanks!
Chenbo

> +		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 a76d7cfaf..1a045af6f 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
> @@ -75,4 +75,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 e52f11341..40330deed 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -281,6 +281,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

Re: [dpdk-dev] [PATCH 4/5] net/virtio: add GET_STATUS support to virtio-user

[Adrian Moreno]

On 7/16/20 5:31 AM, Xia, Chenbo wrote:
> Hi Adrian,
> 
>> -----Original Message-----
>> From: Adrian Moreno <amorenoz@redhat.com>
>> Sent: Thursday, July 16, 2020 1:18 AM
>> To: dev@dpdk.org
>> Cc: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>;
>> amorenoz@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>
>> Subject: [PATCH 4/5] 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  | 39
>> +++++++++++++++++++  .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
>>  drivers/net/virtio/virtio_user_ethdev.c       |  2 +
>>  4 files changed, 44 insertions(+)
>>
>> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c
>> b/drivers/net/virtio/virtio_user/vhost_user.c
>> index 2332e01d1..43c630e47 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 2c400a448..613caef56 100644
>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
>> @@ -804,3 +804,42 @@ 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;
>> +
>> +	/* 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;
>> +
>> +	if (dev->ops->send_request(dev, VHOST_USER_GET_STATUS, &ret) < 0) {
>> +		PMD_INIT_LOG(ERR, "GET_STATUS failed: %s", strerror(errno));
>> +		return -1;
>> +	}
>> +	if (ret > UINT8_MAX) {
>> +		PMD_INIT_LOG(ERR, "Invalid VHOST_USER_GET_STATUS
>> response 0x%" PRIx64 "\n", ret);
> 
> There's a coding style issue here:
> CHECK:CAMELCASE: Avoid CamelCase: <PRIx64>
> Could you fix this?

I did see this one as well. It also seems new because PRIx64 is widely used in DPDK.

find -iname *.c | xargs grep -i "PRIx64" | wc -l
520

So, ether we have to modify the script to ignore this one or change all 520
cases, right?

+ David, what do you think?

> 
> Btw, in case you don't know, there's a script in dpdk for check these issues:
> devtools/checkpatches.sh. 😊
> 
> Thanks!
> Chenbo
> 
>> +		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 a76d7cfaf..1a045af6f 100644
>> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
>> @@ -75,4 +75,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 e52f11341..40330deed 100644
>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>> @@ -281,6 +281,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

Re: [dpdk-dev] [PATCH 4/5] net/virtio: add GET_STATUS support to virtio-user

[David Marchand]

On Thu, Jul 16, 2020 at 9:53 AM Adrian Moreno <amorenoz@redhat.com> wrote:
> >> +    if (ret > UINT8_MAX) {
> >> +            PMD_INIT_LOG(ERR, "Invalid VHOST_USER_GET_STATUS
> >> response 0x%" PRIx64 "\n", ret);
> >
> > There's a coding style issue here:
> > CHECK:CAMELCASE: Avoid CamelCase: <PRIx64>
> > Could you fix this?
>
> I did see this one as well. It also seems new because PRIx64 is widely used in DPDK.
>
> find -iname *.c | xargs grep -i "PRIx64" | wc -l
> 520
>
> So, ether we have to modify the script to ignore this one or change all 520
> cases, right?

This is a false positive.
We won't drop use of this standard macro because checkpatch is not
happy with it.

I don't see how to silence this warning but I can see that the kernel
uses this macro: so I guess people are simply ignoring this one.


-- 
David Marchand

Re: [dpdk-dev] [PATCH 4/5] net/virtio: add GET_STATUS support to virtio-user

[Adrian Moreno]

On 7/16/20 10:18 AM, David Marchand wrote:
> On Thu, Jul 16, 2020 at 9:53 AM Adrian Moreno <amorenoz@redhat.com> wrote:
>>>> +    if (ret > UINT8_MAX) {
>>>> +            PMD_INIT_LOG(ERR, "Invalid VHOST_USER_GET_STATUS
>>>> response 0x%" PRIx64 "\n", ret);
>>>
>>> There's a coding style issue here:
>>> CHECK:CAMELCASE: Avoid CamelCase: <PRIx64>
>>> Could you fix this?
>>
>> I did see this one as well. It also seems new because PRIx64 is widely used in DPDK.
>>
>> find -iname *.c | xargs grep -i "PRIx64" | wc -l
>> 520
>>
>> So, ether we have to modify the script to ignore this one or change all 520
>> cases, right?
> 
> This is a false positive.
> We won't drop use of this standard macro because checkpatch is not
> happy with it.
> 
> I don't see how to silence this warning but I can see that the kernel
> uses this macro: so I guess people are simply ignoring this one.
> 
> 

Thanks David!

-- 
Adrián Moreno

[dpdk-dev] [PATCH 5/5] net/virtio: enable feature checking on virtio-user

[Adrian Moreno]

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