DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net/virtio: fix crash when device reconnecting
@ 2020-04-14 12:55 Marvin Liu
  2020-04-15  1:06 ` Wang, Yinan
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Marvin Liu @ 2020-04-14 12:55 UTC (permalink / raw)
  To: maxime.coquelin, zhihong.wang, xiaolong.ye; +Cc: dev, xuan.ding, Marvin Liu

When doing virtio device initialization, virtqueues will be reset in
server mode if ring type is packed. This will cause issue because queues
have been freed in the beginning of device initialization.

Fix this issue by splitting device initial process and device reinit
process. Virt queues won't be freed or realloc in reinit process. Also
moved virtio device initialization from configuration to start stage,
which can reduce number of reinitialization times.

Fixes: 6ebbf4109f35 ("net/virtio-user: fix packed ring server mode")

Signed-off-by: Marvin Liu <yong.liu@intel.com>

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 21570e5cf..8c84bfe91 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1670,7 +1670,9 @@ virtio_configure_intr(struct rte_eth_dev *dev)
 
 /* reset device and renegotiate features if needed */
 static int
-virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
+virtio_init_device(struct rte_eth_dev *eth_dev,
+		   uint64_t req_features,
+		   bool reinit)
 {
 	struct virtio_hw *hw = eth_dev->data->dev_private;
 	struct virtio_net_config *config;
@@ -1681,7 +1683,7 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
 	/* Reset the device although not necessary at startup */
 	vtpci_reset(hw);
 
-	if (hw->vqs) {
+	if (hw->vqs && !reinit) {
 		virtio_dev_free_mbufs(eth_dev);
 		virtio_free_queues(hw);
 	}
@@ -1794,9 +1796,11 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
 			VLAN_TAG_LEN - hw->vtnet_hdr_size;
 	}
 
-	ret = virtio_alloc_queues(eth_dev);
-	if (ret < 0)
-		return ret;
+	if (!reinit) {
+		ret = virtio_alloc_queues(eth_dev);
+		if (ret < 0)
+			return ret;
+	}
 
 	if (eth_dev->data->dev_conf.intr_conf.rxq) {
 		if (virtio_configure_intr(eth_dev) < 0) {
@@ -1925,7 +1929,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
 	rte_spinlock_init(&hw->state_lock);
 
 	/* reset device and negotiate default features */
-	ret = virtio_init_device(eth_dev, VIRTIO_PMD_DEFAULT_GUEST_FEATURES);
+	ret = virtio_init_device(eth_dev, VIRTIO_PMD_DEFAULT_GUEST_FEATURES,
+			false);
 	if (ret < 0)
 		goto err_virtio_init;
 
@@ -2091,12 +2096,6 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 		return -EINVAL;
 	}
 
-	if (dev->data->dev_conf.intr_conf.rxq) {
-		ret = virtio_init_device(dev, hw->req_guest_features);
-		if (ret < 0)
-			return ret;
-	}
-
 	if (rxmode->max_rx_pkt_len > hw->max_mtu + ether_hdr_len)
 		req_features &= ~(1ULL << VIRTIO_NET_F_MTU);
 
@@ -2120,7 +2119,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
 
 	/* if request features changed, reinit the device */
 	if (req_features != hw->req_guest_features) {
-		ret = virtio_init_device(dev, req_features);
+		ret = virtio_negotiate_features(hw, req_features);
 		if (ret < 0)
 			return ret;
 	}
@@ -2235,6 +2234,11 @@ virtio_dev_start(struct rte_eth_dev *dev)
 	struct virtio_hw *hw = dev->data->dev_private;
 	int ret;
 
+	/* reinit the device */
+	ret = virtio_init_device(dev, hw->req_guest_features, true);
+	if (ret < 0)
+		return ret;
+
 	/* Finish the initialization of the queues */
 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
 		ret = virtio_dev_rx_queue_setup_finish(dev, i);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH] net/virtio: fix crash when device reconnecting
  2020-04-14 12:55 [dpdk-dev] [PATCH] net/virtio: fix crash when device reconnecting Marvin Liu
@ 2020-04-15  1:06 ` Wang, Yinan
  2020-04-15  7:24 ` Ye Xiaolong
  2020-05-06 15:07 ` [dpdk-dev] [PATCH v2] " Marvin Liu
  2 siblings, 0 replies; 11+ messages in thread
From: Wang, Yinan @ 2020-04-15  1:06 UTC (permalink / raw)
  To: Liu, Yong, maxime.coquelin, Wang, Zhihong, Ye, Xiaolong
  Cc: dev, Ding, Xuan, Liu, Yong

Tested-by: Wang, Yinan <yinan.wang@intel.com>

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Marvin Liu
> Sent: 2020年4月14日 20:56
> To: maxime.coquelin@redhat.com; Wang, Zhihong <zhihong.wang@intel.com>;
> Ye, Xiaolong <xiaolong.ye@intel.com>
> Cc: dev@dpdk.org; Ding, Xuan <xuan.ding@intel.com>; Liu, Yong
> <yong.liu@intel.com>
> Subject: [dpdk-dev] [PATCH] net/virtio: fix crash when device reconnecting
> 
> When doing virtio device initialization, virtqueues will be reset in server mode if
> ring type is packed. This will cause issue because queues have been freed in the
> beginning of device initialization.
> 
> Fix this issue by splitting device initial process and device reinit process. Virt
> queues won't be freed or realloc in reinit process. Also moved virtio device
> initialization from configuration to start stage, which can reduce number of
> reinitialization times.
> 
> Fixes: 6ebbf4109f35 ("net/virtio-user: fix packed ring server mode")
> 
> Signed-off-by: Marvin Liu <yong.liu@intel.com>
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 21570e5cf..8c84bfe91 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1670,7 +1670,9 @@ virtio_configure_intr(struct rte_eth_dev *dev)
> 
>  /* reset device and renegotiate features if needed */  static int -
> virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
> +virtio_init_device(struct rte_eth_dev *eth_dev,
> +		   uint64_t req_features,
> +		   bool reinit)
>  {
>  	struct virtio_hw *hw = eth_dev->data->dev_private;
>  	struct virtio_net_config *config;
> @@ -1681,7 +1683,7 @@ virtio_init_device(struct rte_eth_dev *eth_dev,
> uint64_t req_features)
>  	/* Reset the device although not necessary at startup */
>  	vtpci_reset(hw);
> 
> -	if (hw->vqs) {
> +	if (hw->vqs && !reinit) {
>  		virtio_dev_free_mbufs(eth_dev);
>  		virtio_free_queues(hw);
>  	}
> @@ -1794,9 +1796,11 @@ virtio_init_device(struct rte_eth_dev *eth_dev,
> uint64_t req_features)
>  			VLAN_TAG_LEN - hw->vtnet_hdr_size;
>  	}
> 
> -	ret = virtio_alloc_queues(eth_dev);
> -	if (ret < 0)
> -		return ret;
> +	if (!reinit) {
> +		ret = virtio_alloc_queues(eth_dev);
> +		if (ret < 0)
> +			return ret;
> +	}
> 
>  	if (eth_dev->data->dev_conf.intr_conf.rxq) {
>  		if (virtio_configure_intr(eth_dev) < 0) { @@ -1925,7 +1929,8
> @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
>  	rte_spinlock_init(&hw->state_lock);
> 
>  	/* reset device and negotiate default features */
> -	ret = virtio_init_device(eth_dev,
> VIRTIO_PMD_DEFAULT_GUEST_FEATURES);
> +	ret = virtio_init_device(eth_dev,
> VIRTIO_PMD_DEFAULT_GUEST_FEATURES,
> +			false);
>  	if (ret < 0)
>  		goto err_virtio_init;
> 
> @@ -2091,12 +2096,6 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>  		return -EINVAL;
>  	}
> 
> -	if (dev->data->dev_conf.intr_conf.rxq) {
> -		ret = virtio_init_device(dev, hw->req_guest_features);
> -		if (ret < 0)
> -			return ret;
> -	}
> -
>  	if (rxmode->max_rx_pkt_len > hw->max_mtu + ether_hdr_len)
>  		req_features &= ~(1ULL << VIRTIO_NET_F_MTU);
> 
> @@ -2120,7 +2119,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
> 
>  	/* if request features changed, reinit the device */
>  	if (req_features != hw->req_guest_features) {
> -		ret = virtio_init_device(dev, req_features);
> +		ret = virtio_negotiate_features(hw, req_features);
>  		if (ret < 0)
>  			return ret;
>  	}
> @@ -2235,6 +2234,11 @@ virtio_dev_start(struct rte_eth_dev *dev)
>  	struct virtio_hw *hw = dev->data->dev_private;
>  	int ret;
> 
> +	/* reinit the device */
> +	ret = virtio_init_device(dev, hw->req_guest_features, true);
> +	if (ret < 0)
> +		return ret;
> +
>  	/* Finish the initialization of the queues */
>  	for (i = 0; i < dev->data->nb_rx_queues; i++) {
>  		ret = virtio_dev_rx_queue_setup_finish(dev, i);
> --
> 2.17.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH] net/virtio: fix crash when device reconnecting
  2020-04-14 12:55 [dpdk-dev] [PATCH] net/virtio: fix crash when device reconnecting Marvin Liu
  2020-04-15  1:06 ` Wang, Yinan
@ 2020-04-15  7:24 ` Ye Xiaolong
  2020-04-15  7:30   ` Liu, Yong
  2020-05-06 15:07 ` [dpdk-dev] [PATCH v2] " Marvin Liu
  2 siblings, 1 reply; 11+ messages in thread
From: Ye Xiaolong @ 2020-04-15  7:24 UTC (permalink / raw)
  To: Marvin Liu; +Cc: maxime.coquelin, zhihong.wang, dev, xuan.ding

Hi, Marvin

On 04/14, Marvin Liu wrote:
>When doing virtio device initialization, virtqueues will be reset in
>server mode if ring type is packed. This will cause issue because queues
>have been freed in the beginning of device initialization.
>
>Fix this issue by splitting device initial process and device reinit
>process. Virt queues won't be freed or realloc in reinit process. Also
>moved virtio device initialization from configuration to start stage,
>which can reduce number of reinitialization times.
>
>Fixes: 6ebbf4109f35 ("net/virtio-user: fix packed ring server mode")

I think it also needs to cc stable.

>
>Signed-off-by: Marvin Liu <yong.liu@intel.com>
>
>diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
>index 21570e5cf..8c84bfe91 100644
>--- a/drivers/net/virtio/virtio_ethdev.c
>+++ b/drivers/net/virtio/virtio_ethdev.c
>@@ -1670,7 +1670,9 @@ virtio_configure_intr(struct rte_eth_dev *dev)
> 
> /* reset device and renegotiate features if needed */
> static int
>-virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
>+virtio_init_device(struct rte_eth_dev *eth_dev,
>+		   uint64_t req_features,
>+		   bool reinit)
> {
> 	struct virtio_hw *hw = eth_dev->data->dev_private;
> 	struct virtio_net_config *config;
>@@ -1681,7 +1683,7 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
> 	/* Reset the device although not necessary at startup */
> 	vtpci_reset(hw);
> 
>-	if (hw->vqs) {
>+	if (hw->vqs && !reinit) {
> 		virtio_dev_free_mbufs(eth_dev);
> 		virtio_free_queues(hw);
> 	}
>@@ -1794,9 +1796,11 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
> 			VLAN_TAG_LEN - hw->vtnet_hdr_size;
> 	}
> 
>-	ret = virtio_alloc_queues(eth_dev);
>-	if (ret < 0)
>-		return ret;
>+	if (!reinit) {
>+		ret = virtio_alloc_queues(eth_dev);
>+		if (ret < 0)
>+			return ret;
>+	}
> 
> 	if (eth_dev->data->dev_conf.intr_conf.rxq) {
> 		if (virtio_configure_intr(eth_dev) < 0) {
>@@ -1925,7 +1929,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> 	rte_spinlock_init(&hw->state_lock);
> 
> 	/* reset device and negotiate default features */
>-	ret = virtio_init_device(eth_dev, VIRTIO_PMD_DEFAULT_GUEST_FEATURES);
>+	ret = virtio_init_device(eth_dev, VIRTIO_PMD_DEFAULT_GUEST_FEATURES,
>+			false);
> 	if (ret < 0)
> 		goto err_virtio_init;
> 
>@@ -2091,12 +2096,6 @@ virtio_dev_configure(struct rte_eth_dev *dev)
> 		return -EINVAL;
> 	}
> 
>-	if (dev->data->dev_conf.intr_conf.rxq) {
>-		ret = virtio_init_device(dev, hw->req_guest_features);
>-		if (ret < 0)
>-			return ret;
>-	}
>-
> 	if (rxmode->max_rx_pkt_len > hw->max_mtu + ether_hdr_len)
> 		req_features &= ~(1ULL << VIRTIO_NET_F_MTU);
> 
>@@ -2120,7 +2119,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
> 
> 	/* if request features changed, reinit the device */
> 	if (req_features != hw->req_guest_features) {
>-		ret = virtio_init_device(dev, req_features);
>+		ret = virtio_negotiate_features(hw, req_features);

Why do we need to change virtio_init_device to virtio_negotiate_features here? 

Thanks,
Xiaolong

> 		if (ret < 0)
> 			return ret;
> 	}
>@@ -2235,6 +2234,11 @@ virtio_dev_start(struct rte_eth_dev *dev)
> 	struct virtio_hw *hw = dev->data->dev_private;
> 	int ret;
> 
>+	/* reinit the device */
>+	ret = virtio_init_device(dev, hw->req_guest_features, true);
>+	if (ret < 0)
>+		return ret;
>+
> 	/* Finish the initialization of the queues */
> 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> 		ret = virtio_dev_rx_queue_setup_finish(dev, i);
>-- 
>2.17.1
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH] net/virtio: fix crash when device reconnecting
  2020-04-15  7:24 ` Ye Xiaolong
@ 2020-04-15  7:30   ` Liu, Yong
  2020-04-17 15:17     ` Maxime Coquelin
  0 siblings, 1 reply; 11+ messages in thread
From: Liu, Yong @ 2020-04-15  7:30 UTC (permalink / raw)
  To: Ye, Xiaolong; +Cc: maxime.coquelin, Wang, Zhihong, dev, Ding, Xuan



> -----Original Message-----
> From: Ye, Xiaolong <xiaolong.ye@intel.com>
> Sent: Wednesday, April 15, 2020 3:24 PM
> To: Liu, Yong <yong.liu@intel.com>
> Cc: maxime.coquelin@redhat.com; Wang, Zhihong
> <zhihong.wang@intel.com>; dev@dpdk.org; Ding, Xuan
> <xuan.ding@intel.com>
> Subject: Re: [PATCH] net/virtio: fix crash when device reconnecting
> 
> Hi, Marvin
> 
> On 04/14, Marvin Liu wrote:
> >When doing virtio device initialization, virtqueues will be reset in
> >server mode if ring type is packed. This will cause issue because queues
> >have been freed in the beginning of device initialization.
> >
> >Fix this issue by splitting device initial process and device reinit
> >process. Virt queues won't be freed or realloc in reinit process. Also
> >moved virtio device initialization from configuration to start stage,
> >which can reduce number of reinitialization times.
> >
> >Fixes: 6ebbf4109f35 ("net/virtio-user: fix packed ring server mode")
> 
> I think it also needs to cc stable.
> 

Hi Xiaolong,
Packed ring server mode fix was merged in 20.02 release. So there's no stable branch for it. 

Thanks,
Marvin

> >
> >Signed-off-by: Marvin Liu <yong.liu@intel.com>
> >
> >diff --git a/drivers/net/virtio/virtio_ethdev.c
> b/drivers/net/virtio/virtio_ethdev.c
> >index 21570e5cf..8c84bfe91 100644
> >--- a/drivers/net/virtio/virtio_ethdev.c
> >+++ b/drivers/net/virtio/virtio_ethdev.c
> >@@ -1670,7 +1670,9 @@ virtio_configure_intr(struct rte_eth_dev *dev)
> >
> > /* reset device and renegotiate features if needed */
> > static int
> >-virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
> >+virtio_init_device(struct rte_eth_dev *eth_dev,
> >+		   uint64_t req_features,
> >+		   bool reinit)
> > {
> > 	struct virtio_hw *hw = eth_dev->data->dev_private;
> > 	struct virtio_net_config *config;
> >@@ -1681,7 +1683,7 @@ virtio_init_device(struct rte_eth_dev *eth_dev,
> uint64_t req_features)
> > 	/* Reset the device although not necessary at startup */
> > 	vtpci_reset(hw);
> >
> >-	if (hw->vqs) {
> >+	if (hw->vqs && !reinit) {
> > 		virtio_dev_free_mbufs(eth_dev);
> > 		virtio_free_queues(hw);
> > 	}
> >@@ -1794,9 +1796,11 @@ virtio_init_device(struct rte_eth_dev *eth_dev,
> uint64_t req_features)
> > 			VLAN_TAG_LEN - hw->vtnet_hdr_size;
> > 	}
> >
> >-	ret = virtio_alloc_queues(eth_dev);
> >-	if (ret < 0)
> >-		return ret;
> >+	if (!reinit) {
> >+		ret = virtio_alloc_queues(eth_dev);
> >+		if (ret < 0)
> >+			return ret;
> >+	}
> >
> > 	if (eth_dev->data->dev_conf.intr_conf.rxq) {
> > 		if (virtio_configure_intr(eth_dev) < 0) {
> >@@ -1925,7 +1929,8 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> > 	rte_spinlock_init(&hw->state_lock);
> >
> > 	/* reset device and negotiate default features */
> >-	ret = virtio_init_device(eth_dev,
> VIRTIO_PMD_DEFAULT_GUEST_FEATURES);
> >+	ret = virtio_init_device(eth_dev,
> VIRTIO_PMD_DEFAULT_GUEST_FEATURES,
> >+			false);
> > 	if (ret < 0)
> > 		goto err_virtio_init;
> >
> >@@ -2091,12 +2096,6 @@ virtio_dev_configure(struct rte_eth_dev *dev)
> > 		return -EINVAL;
> > 	}
> >
> >-	if (dev->data->dev_conf.intr_conf.rxq) {
> >-		ret = virtio_init_device(dev, hw->req_guest_features);
> >-		if (ret < 0)
> >-			return ret;
> >-	}
> >-
> > 	if (rxmode->max_rx_pkt_len > hw->max_mtu + ether_hdr_len)
> > 		req_features &= ~(1ULL << VIRTIO_NET_F_MTU);
> >
> >@@ -2120,7 +2119,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
> >
> > 	/* if request features changed, reinit the device */
> > 	if (req_features != hw->req_guest_features) {
> >-		ret = virtio_init_device(dev, req_features);
> >+		ret = virtio_negotiate_features(hw, req_features);
> 
> Why do we need to change virtio_init_device to virtio_negotiate_features
> here?
> 
> Thanks,
> Xiaolong
> 
> > 		if (ret < 0)
> > 			return ret;
> > 	}
> >@@ -2235,6 +2234,11 @@ virtio_dev_start(struct rte_eth_dev *dev)
> > 	struct virtio_hw *hw = dev->data->dev_private;
> > 	int ret;
> >
> >+	/* reinit the device */
> >+	ret = virtio_init_device(dev, hw->req_guest_features, true);
> >+	if (ret < 0)
> >+		return ret;
> >+
> > 	/* Finish the initialization of the queues */
> > 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
> > 		ret = virtio_dev_rx_queue_setup_finish(dev, i);
> >--
> >2.17.1
> >

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH] net/virtio: fix crash when device reconnecting
  2020-04-15  7:30   ` Liu, Yong
@ 2020-04-17 15:17     ` Maxime Coquelin
  2020-04-19  1:35       ` Liu, Yong
  0 siblings, 1 reply; 11+ messages in thread
From: Maxime Coquelin @ 2020-04-17 15:17 UTC (permalink / raw)
  To: Liu, Yong, Ye, Xiaolong; +Cc: Wang, Zhihong, dev, Ding, Xuan

Hi Marvin,

On 4/15/20 9:30 AM, Liu, Yong wrote:
>> @@ -2120,7 +2119,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>>
>> 	/* if request features changed, reinit the device */
>> 	if (req_features != hw->req_guest_features) {
>> -		ret = virtio_init_device(dev, req_features);
>> +		ret = virtio_negotiate_features(hw, req_features);
> Why do we need to change virtio_init_device to virtio_negotiate_features
> here?


You missed to reply to that question from Xiaolong.

Regards,
Maxime


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH] net/virtio: fix crash when device reconnecting
  2020-04-17 15:17     ` Maxime Coquelin
@ 2020-04-19  1:35       ` Liu, Yong
  2020-04-19  3:08         ` Ye Xiaolong
  0 siblings, 1 reply; 11+ messages in thread
From: Liu, Yong @ 2020-04-19  1:35 UTC (permalink / raw)
  To: Maxime Coquelin, Ye, Xiaolong; +Cc: Wang, Zhihong, dev, Ding, Xuan

Sorry for missed this question.   The purpose of change function is to skip device initialization which is not needed in configuration stage.
When features not matched, can just do feature negotiation in configuration stage and do related actions when virtio device start.

Regards,
Marvin

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Friday, April 17, 2020 11:18 PM
> To: Liu, Yong <yong.liu@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>
> Cc: Wang, Zhihong <zhihong.wang@intel.com>; dev@dpdk.org; Ding, Xuan
> <xuan.ding@intel.com>
> Subject: Re: [PATCH] net/virtio: fix crash when device reconnecting
> 
> Hi Marvin,
> 
> On 4/15/20 9:30 AM, Liu, Yong wrote:
> >> @@ -2120,7 +2119,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
> >>
> >> 	/* if request features changed, reinit the device */
> >> 	if (req_features != hw->req_guest_features) {
> >> -		ret = virtio_init_device(dev, req_features);
> >> +		ret = virtio_negotiate_features(hw, req_features);
> > Why do we need to change virtio_init_device to virtio_negotiate_features
> > here?
> 
> 
> You missed to reply to that question from Xiaolong.
> 
> Regards,
> Maxime


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH] net/virtio: fix crash when device reconnecting
  2020-04-19  1:35       ` Liu, Yong
@ 2020-04-19  3:08         ` Ye Xiaolong
  0 siblings, 0 replies; 11+ messages in thread
From: Ye Xiaolong @ 2020-04-19  3:08 UTC (permalink / raw)
  To: Liu, Yong; +Cc: Maxime Coquelin, Wang, Zhihong, dev, Ding, Xuan

On 04/19, Liu, Yong wrote:
>Sorry for missed this question.   The purpose of change function is to skip device initialization which is not needed in configuration stage.
>When features not matched, can just do feature negotiation in configuration stage and do related actions when virtio device start.

Thanks for the explanation, then seems we needs to adjust the comment above accordingly.

Thanks,
Xiaolong
>
>Regards,
>Marvin
>
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Friday, April 17, 2020 11:18 PM
>> To: Liu, Yong <yong.liu@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>
>> Cc: Wang, Zhihong <zhihong.wang@intel.com>; dev@dpdk.org; Ding, Xuan
>> <xuan.ding@intel.com>
>> Subject: Re: [PATCH] net/virtio: fix crash when device reconnecting
>> 
>> Hi Marvin,
>> 
>> On 4/15/20 9:30 AM, Liu, Yong wrote:
>> >> @@ -2120,7 +2119,7 @@ virtio_dev_configure(struct rte_eth_dev *dev)
>> >>
>> >> 	/* if request features changed, reinit the device */
>> >> 	if (req_features != hw->req_guest_features) {
>> >> -		ret = virtio_init_device(dev, req_features);
>> >> +		ret = virtio_negotiate_features(hw, req_features);
>> > Why do we need to change virtio_init_device to virtio_negotiate_features
>> > here?
>> 
>> 
>> You missed to reply to that question from Xiaolong.
>> 
>> Regards,
>> Maxime
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [dpdk-dev] [PATCH v2] net/virtio: fix crash when device reconnecting
  2020-04-14 12:55 [dpdk-dev] [PATCH] net/virtio: fix crash when device reconnecting Marvin Liu
  2020-04-15  1:06 ` Wang, Yinan
  2020-04-15  7:24 ` Ye Xiaolong
@ 2020-05-06 15:07 ` Marvin Liu
  2020-05-07 10:53   ` Maxime Coquelin
  2020-05-07 14:18   ` Maxime Coquelin
  2 siblings, 2 replies; 11+ messages in thread
From: Marvin Liu @ 2020-05-06 15:07 UTC (permalink / raw)
  To: maxime.coquelin, xiaolong.ye, zhihong.wang; +Cc: dev, Marvin Liu

When doing virtio device initialization, virtqueues will be reset in
server mode if ring type is packed. It will cause issue because queues
have been freed in the beginning of device initialization.

Fix this issue by checking whether device has been initialized before
reset. If device hasn't been initialized, there's no need to reset
queues.

Fixes: 6ebbf4109f35 ("net/virtio-user: fix packed ring server mode")

Signed-off-by: Marvin Liu <yong.liu@intel.com>

diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c
index c54698ad1..55bd81f0b 100644
--- a/drivers/net/virtio/virtio_user_ethdev.c
+++ b/drivers/net/virtio/virtio_user_ethdev.c
@@ -89,7 +89,8 @@ virtio_user_server_reconnect(struct virtio_user_dev *dev)
 	dev->features &= dev->device_features;
 
 	/* For packed ring, resetting queues is required in reconnection. */
-	if (vtpci_packed_queue(hw)) {
+	if (vtpci_packed_queue(hw) &&
+	   (vtpci_get_status(hw) & VIRTIO_CONFIG_STATUS_DRIVER_OK)) {
 		PMD_INIT_LOG(NOTICE, "Packets on the fly will be dropped"
 				" when packed ring reconnecting.");
 		virtio_user_reset_queues_packed(eth_dev);
-- 
2.17.1


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/virtio: fix crash when device reconnecting
  2020-05-06 15:07 ` [dpdk-dev] [PATCH v2] " Marvin Liu
@ 2020-05-07 10:53   ` Maxime Coquelin
  2020-05-07 14:18   ` Maxime Coquelin
  1 sibling, 0 replies; 11+ messages in thread
From: Maxime Coquelin @ 2020-05-07 10:53 UTC (permalink / raw)
  To: Marvin Liu, xiaolong.ye, zhihong.wang; +Cc: dev



On 5/6/20 5:07 PM, Marvin Liu wrote:
> When doing virtio device initialization, virtqueues will be reset in
> server mode if ring type is packed. It will cause issue because queues
> have been freed in the beginning of device initialization.
> 
> Fix this issue by checking whether device has been initialized before
> reset. If device hasn't been initialized, there's no need to reset
> queues.
> 
> Fixes: 6ebbf4109f35 ("net/virtio-user: fix packed ring server mode")
> 
> Signed-off-by: Marvin Liu <yong.liu@intel.com>

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

Thanks,
Maxime


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/virtio: fix crash when device reconnecting
  2020-05-06 15:07 ` [dpdk-dev] [PATCH v2] " Marvin Liu
  2020-05-07 10:53   ` Maxime Coquelin
@ 2020-05-07 14:18   ` Maxime Coquelin
  2020-05-08  1:54     ` Wang, Yinan
  1 sibling, 1 reply; 11+ messages in thread
From: Maxime Coquelin @ 2020-05-07 14:18 UTC (permalink / raw)
  To: Marvin Liu, xiaolong.ye, zhihong.wang; +Cc: dev



On 5/6/20 5:07 PM, Marvin Liu wrote:
> When doing virtio device initialization, virtqueues will be reset in
> server mode if ring type is packed. It will cause issue because queues
> have been freed in the beginning of device initialization.
> 
> Fix this issue by checking whether device has been initialized before
> reset. If device hasn't been initialized, there's no need to reset
> queues.
> 
> Fixes: 6ebbf4109f35 ("net/virtio-user: fix packed ring server mode")
> 
> Signed-off-by: Marvin Liu <yong.liu@intel.com>

Applied to dpdk-next-virtio/master

Thanks,
Maxime


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net/virtio: fix crash when device reconnecting
  2020-05-07 14:18   ` Maxime Coquelin
@ 2020-05-08  1:54     ` Wang, Yinan
  0 siblings, 0 replies; 11+ messages in thread
From: Wang, Yinan @ 2020-05-08  1:54 UTC (permalink / raw)
  To: Maxime Coquelin, Liu, Yong, Ye, Xiaolong, Wang, Zhihong; +Cc: dev

Tested-by: Wang, Yinan <yinan.wang@intel.com>


> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Maxime Coquelin
> Sent: 2020年5月7日 22:18
> To: Liu, Yong <yong.liu@intel.com>; Ye, Xiaolong <xiaolong.ye@intel.com>;
> Wang, Zhihong <zhihong.wang@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] net/virtio: fix crash when device
> reconnecting
> 
> 
> 
> On 5/6/20 5:07 PM, Marvin Liu wrote:
> > When doing virtio device initialization, virtqueues will be reset in
> > server mode if ring type is packed. It will cause issue because queues
> > have been freed in the beginning of device initialization.
> >
> > Fix this issue by checking whether device has been initialized before
> > reset. If device hasn't been initialized, there's no need to reset
> > queues.
> >
> > Fixes: 6ebbf4109f35 ("net/virtio-user: fix packed ring server mode")
> >
> > Signed-off-by: Marvin Liu <yong.liu@intel.com>
> 
> Applied to dpdk-next-virtio/master
> 
> Thanks,
> Maxime


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2020-05-08  1:54 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-14 12:55 [dpdk-dev] [PATCH] net/virtio: fix crash when device reconnecting Marvin Liu
2020-04-15  1:06 ` Wang, Yinan
2020-04-15  7:24 ` Ye Xiaolong
2020-04-15  7:30   ` Liu, Yong
2020-04-17 15:17     ` Maxime Coquelin
2020-04-19  1:35       ` Liu, Yong
2020-04-19  3:08         ` Ye Xiaolong
2020-05-06 15:07 ` [dpdk-dev] [PATCH v2] " Marvin Liu
2020-05-07 10:53   ` Maxime Coquelin
2020-05-07 14:18   ` Maxime Coquelin
2020-05-08  1:54     ` Wang, Yinan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).