From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yuanhan Liu Subject: Re: [PATCH 3/3] net/virtio_user: fix wrongly set features Date: Thu, 8 Dec 2016 16:32:33 +0800 Message-ID: <20161208083233.GC30698@yliu-dev.sh.intel.com> References: <1480689075-66977-1-git-send-email-jianfeng.tan@intel.com> <1480689075-66977-4-git-send-email-jianfeng.tan@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: dev@dpdk.org, ferruh.yigit@intel.com, cunming.liang@intel.com To: Jianfeng Tan Return-path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id BCE1B370 for ; Thu, 8 Dec 2016 09:31:51 +0100 (CET) Content-Disposition: inline In-Reply-To: <1480689075-66977-4-git-send-email-jianfeng.tan@intel.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Fri, Dec 02, 2016 at 02:31:15PM +0000, Jianfeng Tan wrote: > Before the commit 86d59b21468a ("net/virtio: support LRO"), features > in virtio PMD, is decided and properly set at device initialization > and will not be changed. But afterward, features could be changed in > virtio_dev_configure(), and will be re-negotiated if it's changed. > > In virtio_user, host features is obtained at device initialization > only once, but we did not store it. So the added feature bits in > re-negotiation will fail. I think you misunderstood the features negotiation logic here, since the beginning. The feature negotiation is a work of 3 parts (the virtio device, the virtio driver and the vhost). Each part has it's own feature bits to claim what kind of features it supports. On the init stage, the driver will have a sync with the device, and that's what the "get_features" and "set_features" for. Once they have come to an agreement on the negotiated features, it will have a last sync with the vhost backend, through the GET_FEATURES and SET_FEATURES request. Last, we would have a features bits that would work for all of the 3 parts. That said, you should not mix the driver's features bits with the device one. It's Okay to introduce host_features, but it should not be in the driver layer, it should be in the device layer. And naming it to something like "device_features" is better, IMO. And maybe you should re-visit the whole virtio_user features negotiation logic, to see what can be improved. Besides, I would put the bug fixing patch in the first of a series, so that it has minimal chance to introduce conflics while backporting it to a stable branch. --yliu > > Fixes: e9efa4d93821 ("net/virtio-user: add new virtual PCI driver") > > Signed-off-by: Jianfeng Tan > --- > drivers/net/virtio/virtio_user/virtio_user_dev.c | 2 +- > drivers/net/virtio/virtio_user/virtio_user_dev.h | 1 + > drivers/net/virtio/virtio_user_ethdev.c | 4 ++-- > 3 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c > index 3aef5f6..a9157a9 100644 > --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c > +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c > @@ -224,7 +224,7 @@ virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues, > } > > if (vhost_call(dev->vid, VHOST_USER_GET_FEATURES, > - &dev->features) < 0) { > + &dev->host_features) < 0) { > PMD_INIT_LOG(ERR, "get_features failed: %s", strerror(errno)); > return -1; > } > diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.h b/drivers/net/virtio/virtio_user/virtio_user_dev.h > index 80efb6e..d219432 100644 > --- a/drivers/net/virtio/virtio_user/virtio_user_dev.h > +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.h > @@ -47,6 +47,7 @@ struct virtio_user_dev { > uint32_t queue_pairs; > uint32_t queue_size; > uint64_t features; > + uint64_t host_features; > uint8_t status; > uint8_t mac_addr[ETHER_ADDR_LEN]; > char path[PATH_MAX]; > diff --git a/drivers/net/virtio/virtio_user_ethdev.c b/drivers/net/virtio/virtio_user_ethdev.c > index 406beea..cfe2bfc 100644 > --- a/drivers/net/virtio/virtio_user_ethdev.c > +++ b/drivers/net/virtio/virtio_user_ethdev.c > @@ -117,7 +117,7 @@ virtio_user_get_features(struct virtio_hw *hw) > { > struct virtio_user_dev *dev = virtio_user_get_dev(hw); > > - return dev->features; > + return dev->host_features; > } > > static void > @@ -125,7 +125,7 @@ virtio_user_set_features(struct virtio_hw *hw, uint64_t features) > { > struct virtio_user_dev *dev = virtio_user_get_dev(hw); > > - dev->features = features; > + dev->features = features & dev->host_features; > } > > static uint8_t > -- > 2.7.4