From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id E04363DC for ; Mon, 26 Dec 2016 09:00:27 +0100 (CET) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga104.fm.intel.com with ESMTP; 26 Dec 2016 00:00:21 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,404,1477983600"; d="scan'208";a="916179553" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.67.162]) by orsmga003.jf.intel.com with ESMTP; 26 Dec 2016 00:00:20 -0800 Date: Mon, 26 Dec 2016 16:02:08 +0800 From: Yuanhan Liu To: "Tan, Jianfeng" Cc: "dev@dpdk.org" , "Yigit, Ferruh" , "Liang, Cunming" Message-ID: <20161226080208.GG19288@yliu-dev.sh.intel.com> References: <1480689075-66977-1-git-send-email-jianfeng.tan@intel.com> <1482477266-39199-1-git-send-email-jianfeng.tan@intel.com> <1482477266-39199-3-git-send-email-jianfeng.tan@intel.com> <20161226062719.GA19288@yliu-dev.sh.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH v2 2/7] net/virtio_user: postpone DRIVER OK notification 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: , X-List-Received-Date: Mon, 26 Dec 2016 08:00:28 -0000 On Mon, Dec 26, 2016 at 06:55:16AM +0000, Tan, Jianfeng wrote: > > > > -----Original Message----- > > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com] > > Sent: Monday, December 26, 2016 2:27 PM > > To: Tan, Jianfeng > > Cc: dev@dpdk.org; Yigit, Ferruh; Liang, Cunming > > Subject: Re: [PATCH v2 2/7] net/virtio_user: postpone DRIVER OK notification > > > > On Fri, Dec 23, 2016 at 07:14:21AM +0000, Jianfeng Tan wrote: > > > In driver probe phase, we obtain device information; and then virtio > > > driver will initialize device and stores info, like negotiated > > > features, in vhost_user layer; finally, vhost_user gets DRIVER_OK > > > notification from virtio driver, and then sync with backend device. > > > > > > Previously, DRIVER_OK could be sent twice: 1. when ether layer invokes > > > eth_device_init to initialize device; 2. when user configures it with > > > different configuration from that of previous. > > > > Yes, but that's wrong. Now only 1) will be taken. > > >From code in virtio_dev_configure() as below, if hw_ip_checksum or enable_lro is configured, the device will be reset and re-initialized. > if (rxmode->hw_ip_checksum) > req_features |= (1ULL << VIRTIO_NET_F_GUEST_CSUM); > if (rxmode->enable_lro) > req_features |= > (1ULL << VIRTIO_NET_F_GUEST_TSO4) | > (1ULL << VIRTIO_NET_F_GUEST_TSO6); > > /* if request features changed, reinit the device */ > if (req_features != hw->req_guest_features) { > ret = virtio_init_device(dev, req_features); > if (ret < 0) > return ret; > } Yes, I know. But virtio_init_device() will call vtpci_reinit_complete() at the end, doesn't it? > > > Since we can only depend on DRIVER_OK notification to sync with backend > > > device, we postpone it to virtio_dev_start when everything is settled. > > > > I don't quite understand what you were going to fix here; you don't > > state it in the commit log after all. Normally, when everything is > > negotiated (when DRIVER_OK is set), we should not set it again, unless > > a reset has been happened. > > I agree that DRIVER_OK should be set only if everything is settled. Under the case of hw_ip_checksum or enable_lro, a reset will be sent and the device will be re-initialized. > > So my point is that since the configuration might be changed in dev_configure(), why not just send DRIVER_OK after everything is done. > > > > > If you look at QEMU, qemu will simply ignore it once vhost has already > > started. > > It does not apply here. The configuration is changed, and it cannot be ignored. Why it doesn't apply here? What makes virtio_user special? If it works to QEMU and it doesn't to virtio_user, it basically means the virtio_user is broken ... --yliu