From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 43FFA1B8A3 for ; Thu, 10 May 2018 07:04:12 +0200 (CEST) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 May 2018 22:04:11 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.49,384,1520924400"; d="scan'208";a="198167761" Received: from debian.sh.intel.com (HELO debian) ([10.67.104.164]) by orsmga004.jf.intel.com with ESMTP; 09 May 2018 22:04:10 -0700 Date: Thu, 10 May 2018 13:04:43 +0800 From: Tiwei Bie To: Jiayu Hu Cc: dev@dpdk.org, zhiyong.yang@intel.com, maxime.coquelin@redhat.com Message-ID: <20180510050443.mzibwgqnp7ixwa3m@debian> References: <1525675806-22387-1-git-send-email-jiayu.hu@intel.com> <20180508024950.7rqyxjihdfilvovf@debian> <20180508074421.GA57422@dpdk15.sh.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20180508074421.GA57422@dpdk15.sh.intel.com> User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH] net/virtio-user: fix feature setting with vhost-net backend 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: Thu, 10 May 2018 05:04:12 -0000 On Tue, May 08, 2018 at 03:44:22PM +0800, Jiayu Hu wrote: > Hi Tiwei, > > On Tue, May 08, 2018 at 10:49:50AM +0800, Tiwei Bie wrote: > > On Mon, May 07, 2018 at 02:50:06PM +0800, Jiayu Hu wrote: > > > Currently, when the backend is vhost-net, which implies > > > virtio-user works in client mode, virtio-user is assigned > > > to the default feature VIRTIO_USER_SUPPORTED_FEATURES. > > > However, virtio-user should request features from the > > > backend in this case. > > > > > > Fixes: bd8f50a45d0f ("net/virtio-user: support server mode") > > > Signed-off-by: Jiayu Hu > > > --- > > > drivers/net/virtio/virtio_user/virtio_user_dev.c | 15 +++++++-------- > > > 1 file changed, 7 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c > > > index 38b8bc9..f745163 100644 > > > --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c > > > +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c > > > @@ -353,20 +353,22 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, > > > return -1; > > > } > > > > > > - if (dev->vhostfd >= 0) { > > > + if (!dev->is_server) { > > > > Server mode also needs to run below code when the > > connection to the backend is established. > > For server mode, the below code is called by virtio_user_start_device(), > instead of virtio_user_dev_init(). Only client mode virtio-user > needs to call it when init. Okay. So in server mode, virtio-user never get a chance to do GET_FEATURES. > > > > > > if (dev->ops->send_request(dev, VHOST_USER_SET_OWNER, > > > - NULL) < 0) { > > > + NULL) < 0) { > > > > There is no need to change the indent. > > Thanks, change in the next patch. > > > > > > PMD_INIT_LOG(ERR, "set_owner fails: %s", > > > - strerror(errno)); > > > + strerror(errno)); > > > > There is no need to change the indent. > > ditto. > > > > > > return -1; > > > } > > > > > > if (dev->ops->send_request(dev, VHOST_USER_GET_FEATURES, > > > - &dev->device_features) < 0) { > > > + &dev->device_features) < 0) { > > > > There is no need to change the indent. > > ditto. > > > > > > PMD_INIT_LOG(ERR, "get_features failed: %s", > > > - strerror(errno)); > > > + strerror(errno)); > > > > There is no need to change the indent. > > ditto. > > > > > > return -1; > > > } > > > + if (dev->mac_specified) > > > + dev->device_features |= (1ull << VIRTIO_NET_F_MAC); > > > > Why move above code? > > The server mode virtio-user is assigned the default features, which include > VIRTIO_NET_F_MAC. So checking if support VIRTIO_NET_F_MAC is only necessary > for client mode. I think we should set this bit only when mac is specified. And in server mode, we need to clear this bit when mac isn't specified. So instead of moving it, it should be changed to something like this: if (dev->mac_specified) dev->device_features |= (1ull << VIRTIO_NET_F_MAC); else dev->device_features &= ~(1ull << VIRTIO_NET_F_MAC); Thanks > > Thanks, > Jiayu > > > > > > } else { > > > /* We just pretend vhost-user can support all these features. > > > * Note that this could be problematic that if some feature is > > > @@ -376,9 +378,6 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, > > > dev->device_features = VIRTIO_USER_SUPPORTED_FEATURES; > > > } > > > > > > - if (dev->mac_specified) > > > - dev->device_features |= (1ull << VIRTIO_NET_F_MAC); > > > - > > > if (cq) { > > > /* device does not really need to know anything about CQ, > > > * so if necessary, we just claim to support CQ > > > -- > > > 2.7.4 > > >