From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 6B598A0563; Wed, 15 Apr 2020 09:30:52 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 71E601D163; Wed, 15 Apr 2020 09:30:51 +0200 (CEST) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 9198D1C220 for ; Wed, 15 Apr 2020 09:30:48 +0200 (CEST) IronPort-SDR: URb9AuTRv39vajVvXhwrN/RnAH6EPx4mudzmMBychUGCBVP6suWj5QPCbKDgq4Ut3Z5gjW/pG/ 7unjzmecGsEA== X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 15 Apr 2020 00:30:47 -0700 IronPort-SDR: SWr6i8MAXo4RIu/Up1XLF/AReRk7A0CpiXFQ1Ei4JpiIMujESotJyy65f8DLbYlOzM9SeZluil HX8kfEU2JyZA== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.72,386,1580803200"; d="scan'208";a="271651784" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga002.jf.intel.com with ESMTP; 15 Apr 2020 00:30:46 -0700 Received: from fmsmsx152.amr.corp.intel.com (10.18.125.5) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 15 Apr 2020 00:30:44 -0700 Received: from shsmsx105.ccr.corp.intel.com (10.239.4.158) by FMSMSX152.amr.corp.intel.com (10.18.125.5) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 15 Apr 2020 00:30:43 -0700 Received: from shsmsx103.ccr.corp.intel.com ([169.254.4.146]) by SHSMSX105.ccr.corp.intel.com ([169.254.11.213]) with mapi id 14.03.0439.000; Wed, 15 Apr 2020 15:30:40 +0800 From: "Liu, Yong" To: "Ye, Xiaolong" CC: "maxime.coquelin@redhat.com" , "Wang, Zhihong" , "dev@dpdk.org" , "Ding, Xuan" Thread-Topic: [PATCH] net/virtio: fix crash when device reconnecting Thread-Index: AQHWEhyTclpe89ykXEKgR6QgDJjjHqh5Q2iAgACHcWA= Date: Wed, 15 Apr 2020 07:30:40 +0000 Message-ID: <86228AFD5BCD8E4EBFD2B90117B5E81E635351F0@SHSMSX103.ccr.corp.intel.com> References: <20200414125555.86601-1-yong.liu@intel.com> <20200415072417.GB33551@intel.com> In-Reply-To: <20200415072417.GB33551@intel.com> Accept-Language: zh-CN, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH] net/virtio: fix crash when device reconnecting X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" > -----Original Message----- > From: Ye, Xiaolong > Sent: Wednesday, April 15, 2020 3:24 PM > To: Liu, Yong > Cc: maxime.coquelin@redhat.com; Wang, Zhihong > ; dev@dpdk.org; Ding, Xuan > > Subject: Re: [PATCH] net/virtio: fix crash when device reconnecting >=20 > Hi, Marvin >=20 > 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") >=20 > I think it also needs to cc stable. >=20 Hi Xiaolong, Packed ring server mode fix was merged in 20.02 release. So there's no stab= le branch for it.=20 Thanks, Marvin > > > >Signed-off-by: Marvin Liu > > > >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 =3D 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 =3D virtio_alloc_queues(eth_dev); > >- if (ret < 0) > >- return ret; > >+ if (!reinit) { > >+ ret =3D 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 =3D virtio_init_device(eth_dev, > VIRTIO_PMD_DEFAULT_GUEST_FEATURES); > >+ ret =3D 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 =3D 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 &=3D ~(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 !=3D hw->req_guest_features) { > >- ret =3D virtio_init_device(dev, req_features); > >+ ret =3D virtio_negotiate_features(hw, req_features); >=20 > Why do we need to change virtio_init_device to virtio_negotiate_features > here? >=20 > Thanks, > Xiaolong >=20 > > if (ret < 0) > > return ret; > > } > >@@ -2235,6 +2234,11 @@ virtio_dev_start(struct rte_eth_dev *dev) > > struct virtio_hw *hw =3D dev->data->dev_private; > > int ret; > > > >+ /* reinit the device */ > >+ ret =3D virtio_init_device(dev, hw->req_guest_features, true); > >+ if (ret < 0) > >+ return ret; > >+ > > /* Finish the initialization of the queues */ > > for (i =3D 0; i < dev->data->nb_rx_queues; i++) { > > ret =3D virtio_dev_rx_queue_setup_finish(dev, i); > >-- > >2.17.1 > >