[dpdk-dev] [PATCH v2 0/3] virtio-user: fix virtio-user server mode

[dpdk-dev] [PATCH v2 0/3] virtio-user: fix virtio-user server mode

[Adrian Moreno]

This series addresses some issues identified in the virtio-user driver
when configured in server mode.

Firstly, properly identify the backend type in this mode.

Secondly, ensure no get/set status commands are sent before protocol
feature negotiation to avoid sending them to a backend that does not support them

And finally, add get/set status as per virtio spec so that it can work with
modern vhost-user backends that rely on this message to start the device.

--
v1->v2:
Added patch 2 and 3 addressing additional issues
Check errno to select vhost-user backend and log the detected backend
type


Adrian Moreno (3):
  virtio-user: fix backend selection if stat fails
  virtio_user: don't set/get_status until FEATURES_OK
  virtio-user: set status on virtio-user reconnect

 drivers/net/virtio/virtio_user/vhost_user.c   | 10 ++++++----
 .../net/virtio/virtio_user/virtio_user_dev.c  |  7 +++++++
 .../net/virtio/virtio_user/virtio_user_dev.h  |  1 +
 drivers/net/virtio/virtio_user_ethdev.c       | 20 ++++++++++++++++---
 4 files changed, 31 insertions(+), 7 deletions(-)

-- 
2.26.2

[dpdk-dev] [PATCH v2 1/3] virtio-user: fix backend selection if stat fails

[Adrian Moreno]

If stat fails because the file does not exist, it means that
the backend must be vhost-user in server mode.

Also, log the detected backend type.

Bugzilla ID: 559
Fixes: f908b22ea47a ("net/virtio: move backend type selection to ethdev")
Cc: stable@dpdk.org

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 7 +++++++
 drivers/net/virtio/virtio_user/virtio_user_dev.h | 1 +
 drivers/net/virtio/virtio_user_ethdev.c          | 6 +++++-
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 3681758ef..27814eadb 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -22,6 +22,13 @@
 
 #define VIRTIO_USER_MEM_EVENT_CLB_NAME "virtio_user_mem_event_clb"
 
+const char * const virtio_user_backend_strings[] = {
+	[VIRTIO_USER_BACKEND_UNKNOWN] = "VIRTIO_USER_BACKEND_UNKNOWN",
+	[VIRTIO_USER_BACKEND_VHOST_USER] = "VHOST_USER",
+	[VIRTIO_USER_BACKEND_VHOST_KERNEL] = "VHOST_NET",
+	[VIRTIO_USER_BACKEND_VHOST_VDPA] = "VHOST_VDPA",
+};
+
 static int
 virtio_user_create_queue(struct virtio_user_dev *dev, uint32_t queue_sel)
 {
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h
index 3e9d1a1eb..998986875 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.h
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h
@@ -83,4 +83,5 @@ void virtio_user_handle_cq_packed(struct virtio_user_dev *dev,
 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);
+extern const char * const virtio_user_backend_strings[];
 #endif
diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index 042665bc0..e870fb2ff 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -560,6 +560,9 @@ virtio_user_backend_type(const char *path)
 	struct stat sb;
 
 	if (stat(path, &sb) == -1) {
+		if (errno == ENOENT)
+			return VIRTIO_USER_BACKEND_VHOST_USER;
+
 		PMD_INIT_LOG(ERR, "Stat fails: %s (%s)\n", path,
 			     strerror(errno));
 		return VIRTIO_USER_BACKEND_UNKNOWN;
@@ -697,7 +700,8 @@ virtio_user_pmd_probe(struct rte_vdev_device *dev)
 			path);
 		goto end;
 	}
-
+	PMD_INIT_LOG(INFO, "Backend type detected: %s",
+		     virtio_user_backend_strings[backend_type]);
 
 	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_INTERFACE_NAME) == 1) {
 		if (backend_type != VIRTIO_USER_BACKEND_VHOST_KERNEL) {
-- 
2.26.2

Re: [dpdk-dev] [PATCH v2 1/3] virtio-user: fix backend selection if stat fails

[Maxime Coquelin]

On 10/20/20 5:20 PM, Adrian Moreno wrote:
> If stat fails because the file does not exist, it means that
> the backend must be vhost-user in server mode.
> 
> Also, log the detected backend type.
> 
> Bugzilla ID: 559
> Fixes: f908b22ea47a ("net/virtio: move backend type selection to ethdev")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  drivers/net/virtio/virtio_user/virtio_user_dev.c | 7 +++++++
>  drivers/net/virtio/virtio_user/virtio_user_dev.h | 1 +
>  drivers/net/virtio/virtio_user_ethdev.c          | 6 +++++-
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [dpdk-dev] [PATCH v2 1/3] virtio-user: fix backend selection if stat fails

[Kevin Traynor]

On 20/10/2020 16:20, Adrian Moreno wrote:
> If stat fails because the file does not exist, it means that
> the backend must be vhost-user in server mode.
> 
> Also, log the detected backend type.
> 
> Bugzilla ID: 559
> Fixes: f908b22ea47a ("net/virtio: move backend type selection to ethdev")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  drivers/net/virtio/virtio_user/virtio_user_dev.c | 7 +++++++
>  drivers/net/virtio/virtio_user/virtio_user_dev.h | 1 +
>  drivers/net/virtio/virtio_user_ethdev.c          | 6 +++++-
>  3 files changed, 13 insertions(+), 1 deletion(-)

Acked-by: Kevin Traynor <ktraynor@redhat.com>

[dpdk-dev] [PATCH v2 2/3] virtio_user: don't set/get_status until FEATURES_OK

[Adrian Moreno]

According to the virtio spec, ACK and DRIVER status bits should be set
before feature negotiation.

However, until the protocol features are negotiated, the driver does not
know if the device actually supports the those vhost-user messages.
Therefore, until FEATURES_OK is set, the GET/SET_STATUS messages should
not be sent.

Fixes: 57912824615f ("net/virtio-user: support vhost status setting")
Cc: maxime.coquelin@redhat.com
Cc: stable@dpdk.org

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 drivers/net/virtio/virtio_user/vhost_user.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c
index ef290c357..450d77e92 100644
--- a/drivers/net/virtio/virtio_user/vhost_user.c
+++ b/drivers/net/virtio/virtio_user/vhost_user.c
@@ -278,8 +278,9 @@ vhost_user_sock(struct virtio_user_dev *dev,
 
 	switch (req) {
 	case VHOST_USER_GET_STATUS:
-		if (!(dev->protocol_features &
-				(1ULL << VHOST_USER_PROTOCOL_F_STATUS)))
+		if (!(dev->status & VIRTIO_CONFIG_STATUS_FEATURES_OK) ||
+		    (!(dev->protocol_features &
+				(1ULL << VHOST_USER_PROTOCOL_F_STATUS))))
 			return 0;
 		/* Fallthrough */
 	case VHOST_USER_GET_FEATURES:
@@ -288,8 +289,9 @@ vhost_user_sock(struct virtio_user_dev *dev,
 		break;
 
 	case VHOST_USER_SET_STATUS:
-		if (!(dev->protocol_features &
-				(1ULL << VHOST_USER_PROTOCOL_F_STATUS)))
+		if (!(dev->status & VIRTIO_CONFIG_STATUS_FEATURES_OK) ||
+		    (!(dev->protocol_features &
+				(1ULL << VHOST_USER_PROTOCOL_F_STATUS))))
 			return 0;
 
 		if (has_reply_ack)
-- 
2.26.2

Re: [dpdk-dev] [PATCH v2 2/3] virtio_user: don't set/get_status until FEATURES_OK

[Maxime Coquelin]

On 10/20/20 5:20 PM, Adrian Moreno wrote:
> According to the virtio spec, ACK and DRIVER status bits should be set
> before feature negotiation.
> 
> However, until the protocol features are negotiated, the driver does not
> know if the device actually supports the those vhost-user messages.

s/the those/those/

> Therefore, until FEATURES_OK is set, the GET/SET_STATUS messages should
> not be sent.
> 
> Fixes: 57912824615f ("net/virtio-user: support vhost status setting")
> Cc: maxime.coquelin@redhat.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  drivers/net/virtio/virtio_user/vhost_user.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_user/vhost_user.c b/drivers/net/virtio/virtio_user/vhost_user.c
> index ef290c357..450d77e92 100644
> --- a/drivers/net/virtio/virtio_user/vhost_user.c
> +++ b/drivers/net/virtio/virtio_user/vhost_user.c
> @@ -278,8 +278,9 @@ vhost_user_sock(struct virtio_user_dev *dev,
>  
>  	switch (req) {
>  	case VHOST_USER_GET_STATUS:
> -		if (!(dev->protocol_features &
> -				(1ULL << VHOST_USER_PROTOCOL_F_STATUS)))
> +		if (!(dev->status & VIRTIO_CONFIG_STATUS_FEATURES_OK) ||
> +		    (!(dev->protocol_features &
> +				(1ULL << VHOST_USER_PROTOCOL_F_STATUS))))
>  			return 0;
>  		/* Fallthrough */
>  	case VHOST_USER_GET_FEATURES:
> @@ -288,8 +289,9 @@ vhost_user_sock(struct virtio_user_dev *dev,
>  		break;
>  
>  	case VHOST_USER_SET_STATUS:
> -		if (!(dev->protocol_features &
> -				(1ULL << VHOST_USER_PROTOCOL_F_STATUS)))
> +		if (!(dev->status & VIRTIO_CONFIG_STATUS_FEATURES_OK) ||
> +		    (!(dev->protocol_features &
> +				(1ULL << VHOST_USER_PROTOCOL_F_STATUS))))
>  			return 0;
>  
>  		if (has_reply_ack)
> 


Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

[dpdk-dev] [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect

[Adrian Moreno]

Newer vhost-user backends will rely on SET_STATUS to start the device
so this required to support them.

Fixes: 57912824615f ("net/virtio-user: support vhost status setting")
Cc: maxime.coquelin@redhat.com
Cc: stable@dpdk.org

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 drivers/net/virtio/virtio_user_ethdev.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index e870fb2ff..d8bea4537 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -78,6 +78,13 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
 		return -1;
 
 	dev->vhostfd = connectfd;
+
+	vtpci_reset(hw);
+
+	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_ACK);
+
+	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
+
 	if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
 				   &dev->device_features) < 0) {
 		PMD_INIT_LOG(ERR, "get_features failed: %s",
@@ -111,6 +118,8 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
 
 	dev->features &= dev->device_features;
 
+	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
+
 	/* For packed ring, resetting queues is required in reconnection. */
 	if (vtpci_packed_queue(hw) &&
 	   (vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_DRIVER_OK)) {
@@ -119,8 +128,9 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
 		virtio_user_reset_queues_packed(eth_dev);
 	}
 
-	ret = virtio_user_start_device(dev);
-	if (ret < 0)
+	/*  Start the device */
+	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER_OK);
+	if (!dev->started)
 		return -1;
 
 	if (dev->queue_pairs > 1) {
-- 
2.26.2

Re: [dpdk-dev] [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect

[Maxime Coquelin]

On 10/20/20 5:20 PM, Adrian Moreno wrote:
> Newer vhost-user backends will rely on SET_STATUS to start the device
> so this required to support them.
> 
> Fixes: 57912824615f ("net/virtio-user: support vhost status setting")
> Cc: maxime.coquelin@redhat.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> ---
>  drivers/net/virtio/virtio_user_ethdev.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
> index e870fb2ff..d8bea4537 100644
> --- a/drivers/net/virtio/virtio_user_ethdev.c
> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> @@ -78,6 +78,13 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
>  		return -1;
>  
>  	dev->vhostfd = connectfd;
> +
> +	vtpci_reset(hw);
> +
> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_ACK);
> +
> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
> +
>  	if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
>  				   &dev->device_features) < 0) {
>  		PMD_INIT_LOG(ERR, "get_features failed: %s",
> @@ -111,6 +118,8 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
>  
>  	dev->features &= dev->device_features;
>  
> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
> +
>  	/* For packed ring, resetting queues is required in reconnection. */
>  	if (vtpci_packed_queue(hw) &&
>  	   (vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_DRIVER_OK)) {
> @@ -119,8 +128,9 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
>  		virtio_user_reset_queues_packed(eth_dev);
>  	}
>  
> -	ret = virtio_user_start_device(dev);
> -	if (ret < 0)
> +	/*  Start the device */
> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER_OK);
> +	if (!dev->started)
>  		return -1;
>  
>  	if (dev->queue_pairs > 1) {
> 

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

Re: [dpdk-dev] [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect

[Wang, Yinan]

Hi Maxime/ Adrian，

Thanks for the patch.  we can launch vhost-user with client mode with this fix patch.
But still fail to get throughput with basic vhost/virtio-user server mode loopback test. This is another problem which introduced by 57912824615fd7787a48a7b18e40661466.
Bugzilla: https://bugs.dpdk.org/show_bug.cgi?id=541

BR,
Yinan
> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: 2020年10月21日 0:43
> To: Adrian Moreno <amorenoz@redhat.com>; dev@dpdk.org
> Cc: Wang, Yinan <yinan.wang@intel.com>; Fu, Patrick <patrick.fu@intel.com>;
> stable@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Wang, Zhihong
> <zhihong.wang@intel.com>
> Subject: Re: [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect
> 
> 
> 
> On 10/20/20 5:20 PM, Adrian Moreno wrote:
> > Newer vhost-user backends will rely on SET_STATUS to start the device
> > so this required to support them.
> >
> > Fixes: 57912824615f ("net/virtio-user: support vhost status setting")
> > Cc: maxime.coquelin@redhat.com
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> > ---
> >  drivers/net/virtio/virtio_user_ethdev.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> b/drivers/net/virtio/virtio_user_ethdev.c
> > index e870fb2ff..d8bea4537 100644
> > --- a/drivers/net/virtio/virtio_user_ethdev.c
> > +++ b/drivers/net/virtio/virtio_user_ethdev.c
> > @@ -78,6 +78,13 @@ virtio_user_server_reconnect(struct virtio_user_dev
> *dev)
> >  		return -1;
> >
> >  	dev->vhostfd = connectfd;
> > +
> > +	vtpci_reset(hw);
> > +
> > +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_ACK);
> > +
> > +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
> > +
> >  	if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
> >  				   &dev->device_features) < 0) {
> >  		PMD_INIT_LOG(ERR, "get_features failed: %s",
> > @@ -111,6 +118,8 @@ virtio_user_server_reconnect(struct virtio_user_dev
> *dev)
> >
> >  	dev->features &= dev->device_features;
> >
> > +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
> > +
> >  	/* For packed ring, resetting queues is required in reconnection. */
> >  	if (vtpci_packed_queue(hw) &&
> >  	   (vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_DRIVER_OK)) {
> > @@ -119,8 +128,9 @@ virtio_user_server_reconnect(struct virtio_user_dev
> *dev)
> >  		virtio_user_reset_queues_packed(eth_dev);
> >  	}
> >
> > -	ret = virtio_user_start_device(dev);
> > -	if (ret < 0)
> > +	/*  Start the device */
> > +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER_OK);
> > +	if (!dev->started)
> >  		return -1;
> >
> >  	if (dev->queue_pairs > 1) {
> >
> 
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> Thanks,
> Maxime

Re: [dpdk-dev] [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect

[Maxime Coquelin]

Hi Yinan,

On 10/22/20 6:01 AM, Wang, Yinan wrote:
> Hi Maxime/ Adrian，
> 
> Thanks for the patch.  we can launch vhost-user with client mode with this fix patch.
> But still fail to get throughput with basic vhost/virtio-user server mode loopback test. This is another problem which introduced by 57912824615fd7787a48a7b18e40661466.
> Bugzilla: https://bugs.dpdk.org/show_bug.cgi?id=541

Thanks for reporting the issue, I will look at it this morning.

BTW, this virtio-user server mode is broken by design, we should really
fix it.

For example, it assumes features to be supported by the backend before
the negotiation took place:
https://git.dpdk.org/dpdk/tree/drivers/net/virtio/virtio_user/virtio_user_dev.c#n513

As part of my rework, I suggest we implement the same behaviour as QEMU
does, which will be reliable and consistent with QEMU. It means that if
server mode is enabled in device command-line, the driver waits until
the socket is ready.

Chenbo, Adrian, what do you think?

Thanks,
Maxime

> BR,
> Yinan
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: 2020年10月21日 0:43
>> To: Adrian Moreno <amorenoz@redhat.com>; dev@dpdk.org
>> Cc: Wang, Yinan <yinan.wang@intel.com>; Fu, Patrick <patrick.fu@intel.com>;
>> stable@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Wang, Zhihong
>> <zhihong.wang@intel.com>
>> Subject: Re: [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect
>>
>>
>>
>> On 10/20/20 5:20 PM, Adrian Moreno wrote:
>>> Newer vhost-user backends will rely on SET_STATUS to start the device
>>> so this required to support them.
>>>
>>> Fixes: 57912824615f ("net/virtio-user: support vhost status setting")
>>> Cc: maxime.coquelin@redhat.com
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>>> ---
>>>  drivers/net/virtio/virtio_user_ethdev.c | 14 ++++++++++++--
>>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
>> b/drivers/net/virtio/virtio_user_ethdev.c
>>> index e870fb2ff..d8bea4537 100644
>>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>>> @@ -78,6 +78,13 @@ virtio_user_server_reconnect(struct virtio_user_dev
>> *dev)
>>>  		return -1;
>>>
>>>  	dev->vhostfd = connectfd;
>>> +
>>> +	vtpci_reset(hw);
>>> +
>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_ACK);
>>> +
>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
>>> +
>>>  	if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
>>>  				   &dev->device_features) < 0) {
>>>  		PMD_INIT_LOG(ERR, "get_features failed: %s",
>>> @@ -111,6 +118,8 @@ virtio_user_server_reconnect(struct virtio_user_dev
>> *dev)
>>>
>>>  	dev->features &= dev->device_features;
>>>
>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
>>> +
>>>  	/* For packed ring, resetting queues is required in reconnection. */
>>>  	if (vtpci_packed_queue(hw) &&
>>>  	   (vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_DRIVER_OK)) {
>>> @@ -119,8 +128,9 @@ virtio_user_server_reconnect(struct virtio_user_dev
>> *dev)
>>>  		virtio_user_reset_queues_packed(eth_dev);
>>>  	}
>>>
>>> -	ret = virtio_user_start_device(dev);
>>> -	if (ret < 0)
>>> +	/*  Start the device */
>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER_OK);
>>> +	if (!dev->started)
>>>  		return -1;
>>>
>>>  	if (dev->queue_pairs > 1) {
>>>
>>
>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>
>> Thanks,
>> Maxime
> 

Re: [dpdk-dev] [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect

[Xia, Chenbo]

Hi Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, October 22, 2020 3:14 PM
> To: Wang, Yinan <yinan.wang@intel.com>; Adrian Moreno
> <amorenoz@redhat.com>; dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>
> Cc: Fu, Patrick <patrick.fu@intel.com>; stable@dpdk.org; Wang, Zhihong
> <zhihong.wang@intel.com>; Xu, Qian Q <qian.q.xu@intel.com>
> Subject: Re: [PATCH v2 3/3] virtio-user: set status on virtio-user
> reconnect
> 
> Hi Yinan,
> 
> On 10/22/20 6:01 AM, Wang, Yinan wrote:
> > Hi Maxime/ Adrian，
> >
> > Thanks for the patch.  we can launch vhost-user with client mode with
> this fix patch.
> > But still fail to get throughput with basic vhost/virtio-user server
> mode loopback test. This is another problem which introduced by
> 57912824615fd7787a48a7b18e40661466.
> > Bugzilla: https://bugs.dpdk.org/show_bug.cgi?id=541
> 
> Thanks for reporting the issue, I will look at it this morning.
> 
> BTW, this virtio-user server mode is broken by design, we should really
> fix it.
> 
> For example, it assumes features to be supported by the backend before
> the negotiation took place:
> https://git.dpdk.org/dpdk/tree/drivers/net/virtio/virtio_user/virtio_user_
> dev.c#n513
> 
> As part of my rework, I suggest we implement the same behaviour as QEMU
> does, which will be reliable and consistent with QEMU. It means that if
> server mode is enabled in device command-line, the driver waits until
> the socket is ready.
> 
> Chenbo, Adrian, what do you think?

Yes! I totally agree and have the same opinion for a long time. As I remember, this
feature assumption has caused problems before and I notice the new STATUS feature is
also affected. It's good to make this consistent with QEMU 😊.

Cheers,
Chenbo

> 
> Thanks,
> Maxime
> 
> > BR,
> > Yinan
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: 2020年10月21日 0:43
> >> To: Adrian Moreno <amorenoz@redhat.com>; dev@dpdk.org
> >> Cc: Wang, Yinan <yinan.wang@intel.com>; Fu, Patrick
> <patrick.fu@intel.com>;
> >> stable@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Wang, Zhihong
> >> <zhihong.wang@intel.com>
> >> Subject: Re: [PATCH v2 3/3] virtio-user: set status on virtio-user
> reconnect
> >>
> >>
> >>
> >> On 10/20/20 5:20 PM, Adrian Moreno wrote:
> >>> Newer vhost-user backends will rely on SET_STATUS to start the device
> >>> so this required to support them.
> >>>
> >>> Fixes: 57912824615f ("net/virtio-user: support vhost status setting")
> >>> Cc: maxime.coquelin@redhat.com
> >>> Cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> >>> ---
> >>>  drivers/net/virtio/virtio_user_ethdev.c | 14 ++++++++++++--
> >>>  1 file changed, 12 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> >> b/drivers/net/virtio/virtio_user_ethdev.c
> >>> index e870fb2ff..d8bea4537 100644
> >>> --- a/drivers/net/virtio/virtio_user_ethdev.c
> >>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> >>> @@ -78,6 +78,13 @@ virtio_user_server_reconnect(struct virtio_user_dev
> >> *dev)
> >>>  		return -1;
> >>>
> >>>  	dev->vhostfd = connectfd;
> >>> +
> >>> +	vtpci_reset(hw);
> >>> +
> >>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_ACK);
> >>> +
> >>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
> >>> +
> >>>  	if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
> >>>  				   &dev->device_features) < 0) {
> >>>  		PMD_INIT_LOG(ERR, "get_features failed: %s",
> >>> @@ -111,6 +118,8 @@ virtio_user_server_reconnect(struct
> virtio_user_dev
> >> *dev)
> >>>
> >>>  	dev->features &= dev->device_features;
> >>>
> >>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
> >>> +
> >>>  	/* For packed ring, resetting queues is required in reconnection. */
> >>>  	if (vtpci_packed_queue(hw) &&
> >>>  	   (vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_DRIVER_OK)) {
> >>> @@ -119,8 +128,9 @@ virtio_user_server_reconnect(struct
> virtio_user_dev
> >> *dev)
> >>>  		virtio_user_reset_queues_packed(eth_dev);
> >>>  	}
> >>>
> >>> -	ret = virtio_user_start_device(dev);
> >>> -	if (ret < 0)
> >>> +	/*  Start the device */
> >>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER_OK);
> >>> +	if (!dev->started)
> >>>  		return -1;
> >>>
> >>>  	if (dev->queue_pairs > 1) {
> >>>
> >>
> >> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>
> >> Thanks,
> >> Maxime
> >

Re: [dpdk-dev] [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect

[Adrian Moreno]

On 10/22/20 9:37 AM, Xia, Chenbo wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Thursday, October 22, 2020 3:14 PM
>> To: Wang, Yinan <yinan.wang@intel.com>; Adrian Moreno
>> <amorenoz@redhat.com>; dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>
>> Cc: Fu, Patrick <patrick.fu@intel.com>; stable@dpdk.org; Wang, Zhihong
>> <zhihong.wang@intel.com>; Xu, Qian Q <qian.q.xu@intel.com>
>> Subject: Re: [PATCH v2 3/3] virtio-user: set status on virtio-user
>> reconnect
>>
>> Hi Yinan,
>>
>> On 10/22/20 6:01 AM, Wang, Yinan wrote:
>>> Hi Maxime/ Adrian，
>>>
>>> Thanks for the patch.  we can launch vhost-user with client mode with
>> this fix patch.
>>> But still fail to get throughput with basic vhost/virtio-user server
>> mode loopback test. This is another problem which introduced by
>> 57912824615fd7787a48a7b18e40661466.
>>> Bugzilla: https://bugs.dpdk.org/show_bug.cgi?id=541
>>
>> Thanks for reporting the issue, I will look at it this morning.
>>
>> BTW, this virtio-user server mode is broken by design, we should really
>> fix it.
>>
>> For example, it assumes features to be supported by the backend before
>> the negotiation took place:
>> https://git.dpdk.org/dpdk/tree/drivers/net/virtio/virtio_user/virtio_user_
>> dev.c#n513
>>
>> As part of my rework, I suggest we implement the same behaviour as QEMU
>> does, which will be reliable and consistent with QEMU. It means that if
>> server mode is enabled in device command-line, the driver waits until
>> the socket is ready.
>>
>> Chenbo, Adrian, what do you think?
> 
> Yes! I totally agree and have the same opinion for a long time. As I remember, this
> feature assumption has caused problems before and I notice the new STATUS feature is
> also affected. It's good to make this consistent with QEMU 😊.
> 
+1

In fact, I was looking into how to get rid of some error messages originated
during the initialization of the virtio-pci layer when the socket is still not
ready.


> Cheers,
> Chenbo
> 
>>
>> Thanks,
>> Maxime
>>
>>> BR,
>>> Yinan
>>>> -----Original Message-----
>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> Sent: 2020年10月21日 0:43
>>>> To: Adrian Moreno <amorenoz@redhat.com>; dev@dpdk.org
>>>> Cc: Wang, Yinan <yinan.wang@intel.com>; Fu, Patrick
>> <patrick.fu@intel.com>;
>>>> stable@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Wang, Zhihong
>>>> <zhihong.wang@intel.com>
>>>> Subject: Re: [PATCH v2 3/3] virtio-user: set status on virtio-user
>> reconnect
>>>>
>>>>
>>>>
>>>> On 10/20/20 5:20 PM, Adrian Moreno wrote:
>>>>> Newer vhost-user backends will rely on SET_STATUS to start the device
>>>>> so this required to support them.
>>>>>
>>>>> Fixes: 57912824615f ("net/virtio-user: support vhost status setting")
>>>>> Cc: maxime.coquelin@redhat.com
>>>>> Cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>>>>> ---
>>>>>  drivers/net/virtio/virtio_user_ethdev.c | 14 ++++++++++++--
>>>>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
>>>> b/drivers/net/virtio/virtio_user_ethdev.c
>>>>> index e870fb2ff..d8bea4537 100644
>>>>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>>>>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>>>>> @@ -78,6 +78,13 @@ virtio_user_server_reconnect(struct virtio_user_dev
>>>> *dev)
>>>>>  		return -1;
>>>>>
>>>>>  	dev->vhostfd = connectfd;
>>>>> +
>>>>> +	vtpci_reset(hw);
>>>>> +
>>>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_ACK);
>>>>> +
>>>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
>>>>> +
>>>>>  	if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
>>>>>  				   &dev->device_features) < 0) {
>>>>>  		PMD_INIT_LOG(ERR, "get_features failed: %s",
>>>>> @@ -111,6 +118,8 @@ virtio_user_server_reconnect(struct
>> virtio_user_dev
>>>> *dev)
>>>>>
>>>>>  	dev->features &= dev->device_features;
>>>>>
>>>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
>>>>> +
>>>>>  	/* For packed ring, resetting queues is required in reconnection. */
>>>>>  	if (vtpci_packed_queue(hw) &&
>>>>>  	   (vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_DRIVER_OK)) {
>>>>> @@ -119,8 +128,9 @@ virtio_user_server_reconnect(struct
>> virtio_user_dev
>>>> *dev)
>>>>>  		virtio_user_reset_queues_packed(eth_dev);
>>>>>  	}
>>>>>
>>>>> -	ret = virtio_user_start_device(dev);
>>>>> -	if (ret < 0)
>>>>> +	/*  Start the device */
>>>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER_OK);
>>>>> +	if (!dev->started)
>>>>>  		return -1;
>>>>>
>>>>>  	if (dev->queue_pairs > 1) {
>>>>>
>>>>
>>>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>
>>>> Thanks,
>>>> Maxime
>>>
> 

-- 
Adrián Moreno

Re: [dpdk-dev] [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect

[Maxime Coquelin]

On 10/22/20 10:12 AM, Adrian Moreno wrote:
> 
> 
> On 10/22/20 9:37 AM, Xia, Chenbo wrote:
>> Hi Maxime,
>>
>>> -----Original Message-----
>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Sent: Thursday, October 22, 2020 3:14 PM
>>> To: Wang, Yinan <yinan.wang@intel.com>; Adrian Moreno
>>> <amorenoz@redhat.com>; dev@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>
>>> Cc: Fu, Patrick <patrick.fu@intel.com>; stable@dpdk.org; Wang, Zhihong
>>> <zhihong.wang@intel.com>; Xu, Qian Q <qian.q.xu@intel.com>
>>> Subject: Re: [PATCH v2 3/3] virtio-user: set status on virtio-user
>>> reconnect
>>>
>>> Hi Yinan,
>>>
>>> On 10/22/20 6:01 AM, Wang, Yinan wrote:
>>>> Hi Maxime/ Adrian，
>>>>
>>>> Thanks for the patch.  we can launch vhost-user with client mode with
>>> this fix patch.
>>>> But still fail to get throughput with basic vhost/virtio-user server
>>> mode loopback test. This is another problem which introduced by
>>> 57912824615fd7787a48a7b18e40661466.
>>>> Bugzilla: https://bugs.dpdk.org/show_bug.cgi?id=541
>>>
>>> Thanks for reporting the issue, I will look at it this morning.
>>>
>>> BTW, this virtio-user server mode is broken by design, we should really
>>> fix it.
>>>
>>> For example, it assumes features to be supported by the backend before
>>> the negotiation took place:
>>> https://git.dpdk.org/dpdk/tree/drivers/net/virtio/virtio_user/virtio_user_
>>> dev.c#n513
>>>
>>> As part of my rework, I suggest we implement the same behaviour as QEMU
>>> does, which will be reliable and consistent with QEMU. It means that if
>>> server mode is enabled in device command-line, the driver waits until
>>> the socket is ready.
>>>
>>> Chenbo, Adrian, what do you think?
>>
>> Yes! I totally agree and have the same opinion for a long time. As I remember, this
>> feature assumption has caused problems before and I notice the new STATUS feature is
>> also affected. It's good to make this consistent with QEMU 😊.
>>
> +1
> 
> In fact, I was looking into how to get rid of some error messages originated
> during the initialization of the virtio-pci layer when the socket is still not
> ready.

Is that the error messages you meant?


virtio_user_send_status_update(): VHOST_USER_SET_STATUS failed (-1): Success
virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
virtio_user_send_status_update(): VHOST_USER_SET_STATUS failed (-1): Success
virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
virtio_user_send_status_update(): VHOST_USER_SET_STATUS failed (-1): Success
virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
virtio_user_send_status_update(): VHOST_USER_SET_STATUS failed (-1): Success
virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
virtio_user_send_status_update(): VHOST_USER_SET_STATUS failed (-1): Success


That would be good to have them fixed

> 
>> Cheers,
>> Chenbo
>>
>>>
>>> Thanks,
>>> Maxime
>>>
>>>> BR,
>>>> Yinan
>>>>> -----Original Message-----
>>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>> Sent: 2020年10月21日 0:43
>>>>> To: Adrian Moreno <amorenoz@redhat.com>; dev@dpdk.org
>>>>> Cc: Wang, Yinan <yinan.wang@intel.com>; Fu, Patrick
>>> <patrick.fu@intel.com>;
>>>>> stable@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Wang, Zhihong
>>>>> <zhihong.wang@intel.com>
>>>>> Subject: Re: [PATCH v2 3/3] virtio-user: set status on virtio-user
>>> reconnect
>>>>>
>>>>>
>>>>>
>>>>> On 10/20/20 5:20 PM, Adrian Moreno wrote:
>>>>>> Newer vhost-user backends will rely on SET_STATUS to start the device
>>>>>> so this required to support them.
>>>>>>
>>>>>> Fixes: 57912824615f ("net/virtio-user: support vhost status setting")
>>>>>> Cc: maxime.coquelin@redhat.com
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
>>>>>> ---
>>>>>>  drivers/net/virtio/virtio_user_ethdev.c | 14 ++++++++++++--
>>>>>>  1 file changed, 12 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
>>>>> b/drivers/net/virtio/virtio_user_ethdev.c
>>>>>> index e870fb2ff..d8bea4537 100644
>>>>>> --- a/drivers/net/virtio/virtio_user_ethdev.c
>>>>>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
>>>>>> @@ -78,6 +78,13 @@ virtio_user_server_reconnect(struct virtio_user_dev
>>>>> *dev)
>>>>>>  		return -1;
>>>>>>
>>>>>>  	dev->vhostfd = connectfd;
>>>>>> +
>>>>>> +	vtpci_reset(hw);
>>>>>> +
>>>>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_ACK);
>>>>>> +
>>>>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
>>>>>> +
>>>>>>  	if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES,
>>>>>>  				   &dev->device_features) < 0) {
>>>>>>  		PMD_INIT_LOG(ERR, "get_features failed: %s",
>>>>>> @@ -111,6 +118,8 @@ virtio_user_server_reconnect(struct
>>> virtio_user_dev
>>>>> *dev)
>>>>>>
>>>>>>  	dev->features &= dev->device_features;
>>>>>>
>>>>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_FEATURES_OK);
>>>>>> +
>>>>>>  	/* For packed ring, resetting queues is required in reconnection. */
>>>>>>  	if (vtpci_packed_queue(hw) &&
>>>>>>  	   (vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_DRIVER_OK)) {
>>>>>> @@ -119,8 +128,9 @@ virtio_user_server_reconnect(struct
>>> virtio_user_dev
>>>>> *dev)
>>>>>>  		virtio_user_reset_queues_packed(eth_dev);
>>>>>>  	}
>>>>>>
>>>>>> -	ret = virtio_user_start_device(dev);
>>>>>> -	if (ret < 0)
>>>>>> +	/*  Start the device */
>>>>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER_OK);
>>>>>> +	if (!dev->started)
>>>>>>  		return -1;
>>>>>>
>>>>>>  	if (dev->queue_pairs > 1) {
>>>>>>
>>>>>
>>>>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>
>>>>> Thanks,
>>>>> Maxime
>>>>
>>
> 

Re: [dpdk-dev] [PATCH v2 3/3] virtio-user: set status on virtio-user reconnect

[Jiang, YuX]

Tested-by: JiangYuX <yux.jiang@intel.com>

    Best Regards
    Jiang yu


> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maxime Coquelin
> Sent: Thursday, October 22, 2020 4:32 PM
> To: Adrian Moreno <amorenoz@redhat.com>; Xia, Chenbo
> <chenbo.xia@intel.com>; Wang, Yinan <yinan.wang@intel.com>;
> dev@dpdk.org
> Cc: Fu, Patrick <patrick.fu@intel.com>; stable@dpdk.org; Wang, Zhihong
> <zhihong.wang@intel.com>; Xu, Qian Q <qian.q.xu@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2 3/3] virtio-user: set status on virtio-user
> reconnect
> 
> 
> 
> On 10/22/20 10:12 AM, Adrian Moreno wrote:
> >
> >
> > On 10/22/20 9:37 AM, Xia, Chenbo wrote:
> >> Hi Maxime,
> >>
> >>> -----Original Message-----
> >>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>> Sent: Thursday, October 22, 2020 3:14 PM
> >>> To: Wang, Yinan <yinan.wang@intel.com>; Adrian Moreno
> >>> <amorenoz@redhat.com>; dev@dpdk.org; Xia, Chenbo
> >>> <chenbo.xia@intel.com>
> >>> Cc: Fu, Patrick <patrick.fu@intel.com>; stable@dpdk.org; Wang,
> >>> Zhihong <zhihong.wang@intel.com>; Xu, Qian Q <qian.q.xu@intel.com>
> >>> Subject: Re: [PATCH v2 3/3] virtio-user: set status on virtio-user
> >>> reconnect
> >>>
> >>> Hi Yinan,
> >>>
> >>> On 10/22/20 6:01 AM, Wang, Yinan wrote:
> >>>> Hi Maxime/ Adrian，
> >>>>
> >>>> Thanks for the patch.  we can launch vhost-user with client mode
> >>>> with
> >>> this fix patch.
> >>>> But still fail to get throughput with basic vhost/virtio-user
> >>>> server
> >>> mode loopback test. This is another problem which introduced by
> >>> 57912824615fd7787a48a7b18e40661466.
> >>>> Bugzilla: https://bugs.dpdk.org/show_bug.cgi?id=541
> >>>
> >>> Thanks for reporting the issue, I will look at it this morning.
> >>>
> >>> BTW, this virtio-user server mode is broken by design, we should
> >>> really fix it.
> >>>
> >>> For example, it assumes features to be supported by the backend
> >>> before the negotiation took place:
> >>> https://git.dpdk.org/dpdk/tree/drivers/net/virtio/virtio_user/virtio
> >>> _user_
> >>> dev.c#n513
> >>>
> >>> As part of my rework, I suggest we implement the same behaviour as
> >>> QEMU does, which will be reliable and consistent with QEMU. It means
> >>> that if server mode is enabled in device command-line, the driver
> >>> waits until the socket is ready.
> >>>
> >>> Chenbo, Adrian, what do you think?
> >>
> >> Yes! I totally agree and have the same opinion for a long time. As I
> >> remember, this feature assumption has caused problems before and I
> >> notice the new STATUS feature is also affected. It's good to make this
> consistent with QEMU 😊.
> >>
> > +1
> >
> > In fact, I was looking into how to get rid of some error messages
> > originated during the initialization of the virtio-pci layer when the
> > socket is still not ready.
> 
> Is that the error messages you meant?
> 
> 
> virtio_user_send_status_update(): VHOST_USER_SET_STATUS failed (-1):
> Success
> virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
> virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
> virtio_user_send_status_update(): VHOST_USER_SET_STATUS failed (-1):
> Success
> virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
> virtio_user_send_status_update(): VHOST_USER_SET_STATUS failed (-1):
> Success
> virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
> virtio_user_send_status_update(): VHOST_USER_SET_STATUS failed (-1):
> Success
> virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
> virtio_user_update_status(): VHOST_USER_GET_STATUS failed (-1): Success
> virtio_user_send_status_update(): VHOST_USER_SET_STATUS failed (-1):
> Success
> 
> 
> That would be good to have them fixed
> 
> >
> >> Cheers,
> >> Chenbo
> >>
> >>>
> >>> Thanks,
> >>> Maxime
> >>>
> >>>> BR,
> >>>> Yinan
> >>>>> -----Original Message-----
> >>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>>> Sent: 2020年10月21日 0:43
> >>>>> To: Adrian Moreno <amorenoz@redhat.com>; dev@dpdk.org
> >>>>> Cc: Wang, Yinan <yinan.wang@intel.com>; Fu, Patrick
> >>> <patrick.fu@intel.com>;
> >>>>> stable@dpdk.org; Xia, Chenbo <chenbo.xia@intel.com>; Wang,
> Zhihong
> >>>>> <zhihong.wang@intel.com>
> >>>>> Subject: Re: [PATCH v2 3/3] virtio-user: set status on virtio-user
> >>> reconnect
> >>>>>
> >>>>>
> >>>>>
> >>>>> On 10/20/20 5:20 PM, Adrian Moreno wrote:
> >>>>>> Newer vhost-user backends will rely on SET_STATUS to start the
> >>>>>> device so this required to support them.
> >>>>>>
> >>>>>> Fixes: 57912824615f ("net/virtio-user: support vhost status
> >>>>>> setting")
> >>>>>> Cc: maxime.coquelin@redhat.com
> >>>>>> Cc: stable@dpdk.org
> >>>>>>
> >>>>>> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
> >>>>>> ---
> >>>>>>  drivers/net/virtio/virtio_user_ethdev.c | 14 ++++++++++++--
> >>>>>>  1 file changed, 12 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> >>>>> b/drivers/net/virtio/virtio_user_ethdev.c
> >>>>>> index e870fb2ff..d8bea4537 100644
> >>>>>> --- a/drivers/net/virtio/virtio_user_ethdev.c
> >>>>>> +++ b/drivers/net/virtio/virtio_user_ethdev.c
> >>>>>> @@ -78,6 +78,13 @@ virtio_user_server_reconnect(struct
> >>>>>> virtio_user_dev
> >>>>> *dev)
> >>>>>>  		return -1;
> >>>>>>
> >>>>>>  	dev->vhostfd = connectfd;
> >>>>>> +
> >>>>>> +	vtpci_reset(hw);
> >>>>>> +
> >>>>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_ACK);
> >>>>>> +
> >>>>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER);
> >>>>>> +
> >>>>>>  	if (dev->ops->send_request(dev,
> VHOST_USER_GET_FEATURES,
> >>>>>>  				   &dev->device_features) < 0) {
> >>>>>>  		PMD_INIT_LOG(ERR, "get_features failed: %s", @@ -
> 111,6 +118,8
> >>>>>> @@ virtio_user_server_reconnect(struct
> >>> virtio_user_dev
> >>>>> *dev)
> >>>>>>
> >>>>>>  	dev->features &= dev->device_features;
> >>>>>>
> >>>>>> +	vtpci_set_status(hw,
> VIRTIO_CONFIG_STATUS_FEATURES_OK);
> >>>>>> +
> >>>>>>  	/* For packed ring, resetting queues is required in
> reconnection. */
> >>>>>>  	if (vtpci_packed_queue(hw) &&
> >>>>>>  	   (vtpci_get_status(hw) &
> VIRTIO_CONFIG_STATUS_DRIVER_OK)) {
> >>>>>> @@ -119,8 +128,9 @@ virtio_user_server_reconnect(struct
> >>> virtio_user_dev
> >>>>> *dev)
> >>>>>>  		virtio_user_reset_queues_packed(eth_dev);
> >>>>>>  	}
> >>>>>>
> >>>>>> -	ret = virtio_user_start_device(dev);
> >>>>>> -	if (ret < 0)
> >>>>>> +	/*  Start the device */
> >>>>>> +	vtpci_set_status(hw, VIRTIO_CONFIG_STATUS_DRIVER_OK);
> >>>>>> +	if (!dev->started)
> >>>>>>  		return -1;
> >>>>>>
> >>>>>>  	if (dev->queue_pairs > 1) {
> >>>>>>
> >>>>>
> >>>>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>>>
> >>>>> Thanks,
> >>>>> Maxime
> >>>>
> >>
> >