diff mbox

[dpdk-dev,v2] net/virtio-user: fix cannot get initialized

Message ID 1492078287-138040-1-git-send-email-jianfeng.tan@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Yuanhan Liu
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Jianfeng Tan April 13, 2017, 10:11 a.m. UTC 
  The feature negotiation in virtio-user is proven to be broken,
which results in device initialization failure.

Originally, we get features from vhost backend, and remove those
that are not supported. But when new feature is added, for example,
VIRTIO_NET_F_MTU, we fail to remove this new feature. Then, this
new feature will be negotiated, as both frontend and backend claim
to support this feature.

To fix it, we add a macro to record supported featues, as a filter
to remove newly added features.

Fixes: 37a7eb2ae816 ("net/virtio-user: add device emulation layer")

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Comments

Yuanhan Liu April 14, 2017, 4:24 a.m. UTC | #1 
On Thu, Apr 13, 2017 at 10:11:27AM +0000, Jianfeng Tan wrote:
> The feature negotiation in virtio-user is proven to be broken,
> which results in device initialization failure.
> 
> Originally, we get features from vhost backend, and remove those
> that are not supported. But when new feature is added, for example,
> VIRTIO_NET_F_MTU, we fail to remove this new feature. Then, this
> new feature will be negotiated, as both frontend and backend claim
> to support this feature.
> 
> To fix it, we add a macro to record supported featues, as a filter
> to remove newly added features.

Yes, this is much better! You now don't have to worry that virtio-user
will be broken every time we add a new feature.

Applied to dpdk-next-virtio, with the title changed to "fix feature
negotitation".

Thanks.

	--yliu
Jianfeng Tan April 14, 2017, 4:35 a.m. UTC | #2 
> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Friday, April 14, 2017 12:24 PM
> To: Tan, Jianfeng
> Cc: dev@dpdk.org; maxime.coquelin@redhat.com
> Subject: Re: [PATCH v2] net/virtio-user: fix cannot get initialized
> 
> On Thu, Apr 13, 2017 at 10:11:27AM +0000, Jianfeng Tan wrote:
> > The feature negotiation in virtio-user is proven to be broken,
> > which results in device initialization failure.
> >
> > Originally, we get features from vhost backend, and remove those
> > that are not supported. But when new feature is added, for example,
> > VIRTIO_NET_F_MTU, we fail to remove this new feature. Then, this
> > new feature will be negotiated, as both frontend and backend claim
> > to support this feature.
> >
> > To fix it, we add a macro to record supported featues, as a filter
> > to remove newly added features.
> 
> Yes, this is much better! You now don't have to worry that virtio-user
> will be broken every time we add a new feature.
> 
> Applied to dpdk-next-virtio, with the title changed to "fix feature
> negotitation".
> 

Thanks. By the way, it's better backporting it to stable branches, however, I forget to CC stable@dpdk.org.
Yuanhan Liu April 14, 2017, 5:37 a.m. UTC | #3 
On Fri, Apr 14, 2017 at 04:35:57AM +0000, Tan, Jianfeng wrote:
> 
> 
> > -----Original Message-----
> > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > Sent: Friday, April 14, 2017 12:24 PM
> > To: Tan, Jianfeng
> > Cc: dev@dpdk.org; maxime.coquelin@redhat.com
> > Subject: Re: [PATCH v2] net/virtio-user: fix cannot get initialized
> > 
> > On Thu, Apr 13, 2017 at 10:11:27AM +0000, Jianfeng Tan wrote:
> > > The feature negotiation in virtio-user is proven to be broken,
> > > which results in device initialization failure.
> > >
> > > Originally, we get features from vhost backend, and remove those
> > > that are not supported. But when new feature is added, for example,
> > > VIRTIO_NET_F_MTU, we fail to remove this new feature. Then, this
> > > new feature will be negotiated, as both frontend and backend claim
> > > to support this feature.
> > >
> > > To fix it, we add a macro to record supported featues, as a filter
> > > to remove newly added features.
> > 
> > Yes, this is much better! You now don't have to worry that virtio-user
> > will be broken every time we add a new feature.
> > 
> > Applied to dpdk-next-virtio, with the title changed to "fix feature
> > negotitation".
> > 
> 
> Thanks. By the way, it's better backporting it to stable branches, however, I forget to CC stable@dpdk.org.

As long as we don't backport new features to a stable release, there
should be no problem.

Besides, the virtio-user features supported in this patch may not match
those supported by a stable release (say 16.11 LTS). For example, F_STATUS
is just added in this feature. Thus, I don't think it's a good candidate
for a stable release (though a backport could fix it).

	--yliu
Jianfeng Tan April 14, 2017, 5:55 a.m. UTC | #4 
> -----Original Message-----
> From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> Sent: Friday, April 14, 2017 1:38 PM
> To: Tan, Jianfeng
> Cc: dev@dpdk.org; maxime.coquelin@redhat.com
> Subject: Re: [PATCH v2] net/virtio-user: fix cannot get initialized
> 
> On Fri, Apr 14, 2017 at 04:35:57AM +0000, Tan, Jianfeng wrote:
> >
> >
> > > -----Original Message-----
> > > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > > Sent: Friday, April 14, 2017 12:24 PM
> > > To: Tan, Jianfeng
> > > Cc: dev@dpdk.org; maxime.coquelin@redhat.com
> > > Subject: Re: [PATCH v2] net/virtio-user: fix cannot get initialized
> > >
> > > On Thu, Apr 13, 2017 at 10:11:27AM +0000, Jianfeng Tan wrote:
> > > > The feature negotiation in virtio-user is proven to be broken,
> > > > which results in device initialization failure.
> > > >
> > > > Originally, we get features from vhost backend, and remove those
> > > > that are not supported. But when new feature is added, for example,
> > > > VIRTIO_NET_F_MTU, we fail to remove this new feature. Then, this
> > > > new feature will be negotiated, as both frontend and backend claim
> > > > to support this feature.
> > > >
> > > > To fix it, we add a macro to record supported featues, as a filter
> > > > to remove newly added features.
> > >
> > > Yes, this is much better! You now don't have to worry that virtio-user
> > > will be broken every time we add a new feature.
> > >
> > > Applied to dpdk-next-virtio, with the title changed to "fix feature
> > > negotitation".
> > >
> >
> > Thanks. By the way, it's better backporting it to stable branches, however, I
> forget to CC stable@dpdk.org.
> 
> As long as we don't backport new features to a stable release, there
> should be no problem.
> 
> Besides, the virtio-user features supported in this patch may not match
> those supported by a stable release (say 16.11 LTS). For example, F_STATUS
> is just added in this feature. Thus, I don't think it's a good candidate
> for a stable release (though a backport could fix it).

Make sense. Thanks!

Jianfeng
diff mbox

Patch

diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 6871cd4..299ee16 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -311,6 +311,25 @@  virtio_user_dev_setup(struct virtio_user_dev *dev)
 	return dev->ops->setup(dev);
 }
 
+/* Use below macro to filter features from vhost backend */
+#define VIRTIO_USER_SUPPORTED_FEATURES			\
+	(1ULL << VIRTIO_NET_F_MAC		|	\
+	 1ULL << VIRTIO_NET_F_STATUS		|	\
+	 1ULL << VIRTIO_NET_F_MQ		|	\
+	 1ULL << VIRTIO_NET_F_CTRL_MAC_ADDR	|	\
+	 1ULL << VIRTIO_NET_F_CTRL_VQ		|	\
+	 1ULL << VIRTIO_NET_F_CTRL_RX		|	\
+	 1ULL << VIRTIO_NET_F_CTRL_VLAN		|	\
+	 1ULL << VIRTIO_NET_F_CSUM		|	\
+	 1ULL << VIRTIO_NET_F_HOST_TSO4		|	\
+	 1ULL << VIRTIO_NET_F_HOST_TSO6		|	\
+	 1ULL << VIRTIO_NET_F_MRG_RXBUF		|	\
+	 1ULL << VIRTIO_RING_F_INDIRECT_DESC	|	\
+	 1ULL << VIRTIO_NET_F_GUEST_CSUM	|	\
+	 1ULL << VIRTIO_NET_F_GUEST_TSO4	|	\
+	 1ULL << VIRTIO_NET_F_GUEST_TSO6	|	\
+	 1ULL << VIRTIO_F_VERSION_1)
+
 int
 virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 		     int cq, int queue_size, const char *mac, char **ifname)
@@ -362,6 +381,8 @@  virtio_user_dev_init(struct virtio_user_dev *dev, char *path, int queues,
 	/* The backend will not report this feature, we add it explicitly */
 	dev->device_features |= (1ull << VIRTIO_NET_F_STATUS);
 
+	dev->device_features &= VIRTIO_USER_SUPPORTED_FEATURES;
+
 	return 0;
 }