[PATCH] net/virtio-user: fix cannot get initialized

[PATCH] net/virtio-user: fix cannot get initialized

[Jianfeng Tan]

After the introduction of vhost MTU, VIRTIO_NET_F_MTU is enabled
by default. However, virtio-user vtpci does not support to get
MTU from device yet, i.e., vtpci_read_dev_config(MTU) fails.
Plus, struct virtio_net_config is defined as a uninitialized
variable, and could be different values in
virtio_negotiate_features() and virtio_init_device().

In some cases, it passes the check in virtio_negotiate_features()
but fails the check in virtio_init_device(). As a result,
virtio-user canno be initialized.

To fix it, (1) accessing uninitialized variable is not a good
practice, so initialize it as zero; (2) explicitly disable MTU
feature in virtio-user.

Fixes: 49d26d9e3f47 ("net/virtio: support MTU feature")
Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
Cc: Yuanhan Liu <yuanhan.liu@linux.intel.com>

Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
---
 drivers/net/virtio/virtio_ethdev.c               | 4 ++--
 drivers/net/virtio/virtio_user/virtio_user_dev.c | 6 ++++++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
index 78cb3e8..4c43784 100644
--- a/drivers/net/virtio/virtio_ethdev.c
+++ b/drivers/net/virtio/virtio_ethdev.c
@@ -1163,7 +1163,7 @@ virtio_negotiate_features(struct virtio_hw *hw, uint64_t req_features)
 
 	/* If supported, ensure MTU value is valid before acknowledging it. */
 	if (host_features & req_features & (1ULL << VIRTIO_NET_F_MTU)) {
-		struct virtio_net_config config;
+		struct virtio_net_config config = {0};
 
 		vtpci_read_dev_config(hw,
 			offsetof(struct virtio_net_config, mtu),
@@ -1332,7 +1332,7 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
 {
 	struct virtio_hw *hw = eth_dev->data->dev_private;
 	struct virtio_net_config *config;
-	struct virtio_net_config local_config;
+	struct virtio_net_config local_config = {0};
 	struct rte_pci_device *pci_dev = NULL;
 	int ret;
 
diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
index 6871cd4..529b3d7 100644
--- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
+++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
@@ -362,6 +362,12 @@ 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);
 
+	/* TODO: VIRTIO_NET_F_MTU is for QEMU to advertise MTU to both frontend
+	 * and backend driver. For virtio-user, disable it for now, until we
+	 * have a parameter to specify the MTU.
+	 */
+	dev->device_features &= ~(1ull << VIRTIO_NET_F_MTU);
+
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH] net/virtio-user: fix cannot get initialized

[Yuanhan Liu]

On Fri, Apr 07, 2017 at 07:57:40AM +0000, Jianfeng Tan wrote:
> After the introduction of vhost MTU, VIRTIO_NET_F_MTU is enabled
> by default. However, virtio-user vtpci does not support to get
> MTU from device yet, i.e., vtpci_read_dev_config(MTU) fails.
> Plus, struct virtio_net_config is defined as a uninitialized
> variable, and could be different values in
> virtio_negotiate_features() and virtio_init_device().
> 
> In some cases, it passes the check in virtio_negotiate_features()
> but fails the check in virtio_init_device(). As a result,
> virtio-user canno be initialized.
> 
> To fix it, (1) accessing uninitialized variable is not a good
> practice, so initialize it as zero; (2) explicitly disable MTU
> feature in virtio-user.
> 
> Fixes: 49d26d9e3f47 ("net/virtio: support MTU feature")
> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> Cc: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> 
> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> ---
>  drivers/net/virtio/virtio_ethdev.c               | 4 ++--
>  drivers/net/virtio/virtio_user/virtio_user_dev.c | 6 ++++++
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> index 78cb3e8..4c43784 100644
> --- a/drivers/net/virtio/virtio_ethdev.c
> +++ b/drivers/net/virtio/virtio_ethdev.c
> @@ -1163,7 +1163,7 @@ virtio_negotiate_features(struct virtio_hw *hw, uint64_t req_features)
>  
>  	/* If supported, ensure MTU value is valid before acknowledging it. */
>  	if (host_features & req_features & (1ULL << VIRTIO_NET_F_MTU)) {
> -		struct virtio_net_config config;
> +		struct virtio_net_config config = {0};

virtio-user does not support the MTU feature, this patch should not be
reached. The virtio-user feature negotiation should be broken.

	--yliu

>  
>  		vtpci_read_dev_config(hw,
>  			offsetof(struct virtio_net_config, mtu),
> @@ -1332,7 +1332,7 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features)
>  {
>  	struct virtio_hw *hw = eth_dev->data->dev_private;
>  	struct virtio_net_config *config;
> -	struct virtio_net_config local_config;
> +	struct virtio_net_config local_config = {0};
>  	struct rte_pci_device *pci_dev = NULL;
>  	int ret;
>  
> diff --git a/drivers/net/virtio/virtio_user/virtio_user_dev.c b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> index 6871cd4..529b3d7 100644
> --- a/drivers/net/virtio/virtio_user/virtio_user_dev.c
> +++ b/drivers/net/virtio/virtio_user/virtio_user_dev.c
> @@ -362,6 +362,12 @@ 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);
>  
> +	/* TODO: VIRTIO_NET_F_MTU is for QEMU to advertise MTU to both frontend
> +	 * and backend driver. For virtio-user, disable it for now, until we
> +	 * have a parameter to specify the MTU.
> +	 */
> +	dev->device_features &= ~(1ull << VIRTIO_NET_F_MTU);
> +
>  	return 0;
>  }
>  
> -- 
> 2.7.4

Re: [PATCH] net/virtio-user: fix cannot get initialized

[Tan, Jianfeng]

On 4/13/2017 9:58 AM, Yuanhan Liu wrote:
> On Fri, Apr 07, 2017 at 07:57:40AM +0000, Jianfeng Tan wrote:
>> After the introduction of vhost MTU, VIRTIO_NET_F_MTU is enabled
>> by default. However, virtio-user vtpci does not support to get
>> MTU from device yet, i.e., vtpci_read_dev_config(MTU) fails.
>> Plus, struct virtio_net_config is defined as a uninitialized
>> variable, and could be different values in
>> virtio_negotiate_features() and virtio_init_device().
>>
>> In some cases, it passes the check in virtio_negotiate_features()
>> but fails the check in virtio_init_device(). As a result,
>> virtio-user canno be initialized.
>>
>> To fix it, (1) accessing uninitialized variable is not a good
>> practice, so initialize it as zero; (2) explicitly disable MTU
>> feature in virtio-user.
>>
>> Fixes: 49d26d9e3f47 ("net/virtio: support MTU feature")
>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Cc: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>>
>> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
>> ---
>>   drivers/net/virtio/virtio_ethdev.c               | 4 ++--
>>   drivers/net/virtio/virtio_user/virtio_user_dev.c | 6 ++++++
>>   2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
>> index 78cb3e8..4c43784 100644
>> --- a/drivers/net/virtio/virtio_ethdev.c
>> +++ b/drivers/net/virtio/virtio_ethdev.c
>> @@ -1163,7 +1163,7 @@ virtio_negotiate_features(struct virtio_hw *hw, uint64_t req_features)
>>   
>>   	/* If supported, ensure MTU value is valid before acknowledging it. */
>>   	if (host_features & req_features & (1ULL << VIRTIO_NET_F_MTU)) {
>> -		struct virtio_net_config config;
>> +		struct virtio_net_config config = {0};
> virtio-user does not support the MTU feature, this patch should not be
> reached. The virtio-user feature negotiation should be broken.
>
> 	--yliu
>

Yes, it will not come to here anyway. But some static code analyzer 
might report this as an error: there's chance to read uninitialized 
variable.

Thanks,
Jianfeng

Re: [PATCH] net/virtio-user: fix cannot get initialized

[Yuanhan Liu]

On Thu, Apr 13, 2017 at 10:18:43AM +0800, Tan, Jianfeng wrote:
> 
> 
> On 4/13/2017 9:58 AM, Yuanhan Liu wrote:
> >On Fri, Apr 07, 2017 at 07:57:40AM +0000, Jianfeng Tan wrote:
> >>After the introduction of vhost MTU, VIRTIO_NET_F_MTU is enabled
> >>by default. However, virtio-user vtpci does not support to get
> >>MTU from device yet, i.e., vtpci_read_dev_config(MTU) fails.
> >>Plus, struct virtio_net_config is defined as a uninitialized
> >>variable, and could be different values in
> >>virtio_negotiate_features() and virtio_init_device().
> >>
> >>In some cases, it passes the check in virtio_negotiate_features()
> >>but fails the check in virtio_init_device(). As a result,
> >>virtio-user canno be initialized.
> >>
> >>To fix it, (1) accessing uninitialized variable is not a good
> >>practice, so initialize it as zero; (2) explicitly disable MTU
> >>feature in virtio-user.
> >>
> >>Fixes: 49d26d9e3f47 ("net/virtio: support MTU feature")
> >>Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>Cc: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> >>
> >>Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> >>---
> >>  drivers/net/virtio/virtio_ethdev.c               | 4 ++--
> >>  drivers/net/virtio/virtio_user/virtio_user_dev.c | 6 ++++++
> >>  2 files changed, 8 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> >>index 78cb3e8..4c43784 100644
> >>--- a/drivers/net/virtio/virtio_ethdev.c
> >>+++ b/drivers/net/virtio/virtio_ethdev.c
> >>@@ -1163,7 +1163,7 @@ virtio_negotiate_features(struct virtio_hw *hw, uint64_t req_features)
> >>  	/* If supported, ensure MTU value is valid before acknowledging it. */
> >>  	if (host_features & req_features & (1ULL << VIRTIO_NET_F_MTU)) {
> >>-		struct virtio_net_config config;
> >>+		struct virtio_net_config config = {0};
> >virtio-user does not support the MTU feature, this patch should not be
> >reached. The virtio-user feature negotiation should be broken.
> >
> >	--yliu
> >
> 
> Yes, it will not come to here anyway. But some static code analyzer might
> report this as an error: there's chance to read uninitialized variable.

If so, fix it in another patch, with the detailed (and real) errors in
the commit log.

	--yliu

Re: [PATCH] net/virtio-user: fix cannot get initialized

[Yuanhan Liu]

On Thu, Apr 13, 2017 at 10:18:43AM +0800, Tan, Jianfeng wrote:
> 
> 
> On 4/13/2017 9:58 AM, Yuanhan Liu wrote:
> >On Fri, Apr 07, 2017 at 07:57:40AM +0000, Jianfeng Tan wrote:
> >>After the introduction of vhost MTU, VIRTIO_NET_F_MTU is enabled
> >>by default.

Actually, that's my point. The vhost lib just claim he supports MTU
feature and MTU is not enabled by the virtio-user device.  Meaning,
the MTU feature bit should not be set after the negoitation.

You were seeing the bug because you found the bit is set, right?
If so, that means the virtio-user feature negotiation is broken.


	--yliu

> However, virtio-user vtpci does not support to get
> >>MTU from device yet, i.e., vtpci_read_dev_config(MTU) fails.
> >>Plus, struct virtio_net_config is defined as a uninitialized
> >>variable, and could be different values in
> >>virtio_negotiate_features() and virtio_init_device().
> >>
> >>In some cases, it passes the check in virtio_negotiate_features()
> >>but fails the check in virtio_init_device(). As a result,
> >>virtio-user canno be initialized.
> >>
> >>To fix it, (1) accessing uninitialized variable is not a good
> >>practice, so initialize it as zero; (2) explicitly disable MTU
> >>feature in virtio-user.
> >>
> >>Fixes: 49d26d9e3f47 ("net/virtio: support MTU feature")
> >>Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>Cc: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> >>
> >>Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
> >>---
> >>  drivers/net/virtio/virtio_ethdev.c               | 4 ++--
> >>  drivers/net/virtio/virtio_user/virtio_user_dev.c | 6 ++++++
> >>  2 files changed, 8 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> >>index 78cb3e8..4c43784 100644
> >>--- a/drivers/net/virtio/virtio_ethdev.c
> >>+++ b/drivers/net/virtio/virtio_ethdev.c
> >>@@ -1163,7 +1163,7 @@ virtio_negotiate_features(struct virtio_hw *hw, uint64_t req_features)
> >>  	/* If supported, ensure MTU value is valid before acknowledging it. */
> >>  	if (host_features & req_features & (1ULL << VIRTIO_NET_F_MTU)) {
> >>-		struct virtio_net_config config;
> >>+		struct virtio_net_config config = {0};
> >virtio-user does not support the MTU feature, this patch should not be
> >reached. The virtio-user feature negotiation should be broken.
> >
> >	--yliu
> >
> 
> Yes, it will not come to here anyway. But some static code analyzer might
> report this as an error: there's chance to read uninitialized variable.
> 
> Thanks,
> Jianfeng

Re: [PATCH] net/virtio-user: fix cannot get initialized

[Tan, Jianfeng]

On 4/13/2017 10:21 AM, Yuanhan Liu wrote:
> On Thu, Apr 13, 2017 at 10:18:43AM +0800, Tan, Jianfeng wrote:
>>
>> On 4/13/2017 9:58 AM, Yuanhan Liu wrote:
>>> On Fri, Apr 07, 2017 at 07:57:40AM +0000, Jianfeng Tan wrote:
>>>> After the introduction of vhost MTU, VIRTIO_NET_F_MTU is enabled
>>>> by default. However, virtio-user vtpci does not support to get
>>>> MTU from device yet, i.e., vtpci_read_dev_config(MTU) fails.
>>>> Plus, struct virtio_net_config is defined as a uninitialized
>>>> variable, and could be different values in
>>>> virtio_negotiate_features() and virtio_init_device().
>>>>
>>>> In some cases, it passes the check in virtio_negotiate_features()
>>>> but fails the check in virtio_init_device(). As a result,
>>>> virtio-user canno be initialized.
>>>>
>>>> To fix it, (1) accessing uninitialized variable is not a good
>>>> practice, so initialize it as zero; (2) explicitly disable MTU
>>>> feature in virtio-user.
>>>>
>>>> Fixes: 49d26d9e3f47 ("net/virtio: support MTU feature")
>>>> Cc: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> Cc: Yuanhan Liu <yuanhan.liu@linux.intel.com>
>>>>
>>>> Signed-off-by: Jianfeng Tan <jianfeng.tan@intel.com>
>>>> ---
>>>>   drivers/net/virtio/virtio_ethdev.c               | 4 ++--
>>>>   drivers/net/virtio/virtio_user/virtio_user_dev.c | 6 ++++++
>>>>   2 files changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
>>>> index 78cb3e8..4c43784 100644
>>>> --- a/drivers/net/virtio/virtio_ethdev.c
>>>> +++ b/drivers/net/virtio/virtio_ethdev.c
>>>> @@ -1163,7 +1163,7 @@ virtio_negotiate_features(struct virtio_hw *hw, uint64_t req_features)
>>>>   	/* If supported, ensure MTU value is valid before acknowledging it. */
>>>>   	if (host_features & req_features & (1ULL << VIRTIO_NET_F_MTU)) {
>>>> -		struct virtio_net_config config;
>>>> +		struct virtio_net_config config = {0};
>>> virtio-user does not support the MTU feature, this patch should not be
>>> reached. The virtio-user feature negotiation should be broken.
>>>
>>> 	--yliu
>>>
>> Yes, it will not come to here anyway. But some static code analyzer might
>> report this as an error: there's chance to read uninitialized variable.
> If so, fix it in another patch, with the detailed (and real) errors in
> the commit log.
>

OK, I'll drop this fix from this patch.

Re: [PATCH] net/virtio-user: fix cannot get initialized

[Tan, Jianfeng]

On 4/13/2017 10:36 AM, Yuanhan Liu wrote:
> On Thu, Apr 13, 2017 at 10:18:43AM +0800, Tan, Jianfeng wrote:
>>
>> On 4/13/2017 9:58 AM, Yuanhan Liu wrote:
>>> On Fri, Apr 07, 2017 at 07:57:40AM +0000, Jianfeng Tan wrote:
>>>> After the introduction of vhost MTU, VIRTIO_NET_F_MTU is enabled
>>>> by default.
> Actually, that's my point. The vhost lib just claim he supports MTU
> feature and MTU is not enabled by the virtio-user device.  Meaning,
> the MTU feature bit should not be set after the negoitation.
>
> You were seeing the bug because you found the bit is set, right?
> If so, that means the virtio-user feature negotiation is broken.

Make sense. Then I'll use a macro to define all supported features by 
virtio-user as a filter, so that any new features will be disabled in 
virtio-user.

Thanks,
Jianfeng

[PATCH v2] net/virtio-user: fix cannot get initialized

[Jianfeng Tan]

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(+)

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;
 }
 
-- 
2.7.4

Re: [PATCH v2] net/virtio-user: fix cannot get initialized

[Yuanhan Liu]

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

Re: [PATCH v2] net/virtio-user: fix cannot get initialized

[Tan, Jianfeng]

> -----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.

Re: [PATCH v2] net/virtio-user: fix cannot get initialized

[Yuanhan Liu]

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

Re: [PATCH v2] net/virtio-user: fix cannot get initialized

[Tan, Jianfeng]

> -----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