[PATCH] examples/vhost: fix qemu abort

[PATCH] examples/vhost: fix qemu abort

[Marvin Liu]

When start vhost sample with builtin-net-driver argument, vhost feature
bit will be zero. If VHOST_USER_F_PROTOCOL_FEATURES is not set, vhost
net start will be failed in qemu. This occasion will cause device stop
action was skipped. Consequently, same ioevent fd will be added second
time after reload driver and then cause qemu abort. Add feature bit
which has been supported by vhost library can fix this error.

Fixes: ca059fa5 ("examples/vhost: demonstrate the new generic APIs")

Signed-off-by: Marvin Liu <yong.liu@intel.com>

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index 2175c1186..44aec2f47 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -1520,7 +1520,8 @@ main(int argc, char *argv[])
 		}
 
 		if (builtin_net_driver)
-			rte_vhost_driver_set_features(file, VIRTIO_NET_FEATURES);
+			rte_vhost_driver_set_features(file,
+				1ULL << VHOST_USER_F_PROTOCOL_FEATURES);
 
 		if (mergeable == 0) {
 			rte_vhost_driver_disable_features(file,
-- 
2.17.0

Re: [PATCH] examples/vhost: fix qemu abort

[Tiwei Bie]

On Tue, Jul 24, 2018 at 11:16:49PM +0800, Marvin Liu wrote:
> When start vhost sample with builtin-net-driver argument, vhost feature
> bit will be zero. If VHOST_USER_F_PROTOCOL_FEATURES is not set, vhost
> net start will be failed in qemu. This occasion will cause device stop
> action was skipped. Consequently, same ioevent fd will be added second
> time after reload driver and then cause qemu abort. Add feature bit
> which has been supported by vhost library can fix this error.
> 
> Fixes: ca059fa5 ("examples/vhost: demonstrate the new generic APIs")

It's a bug in QEMU, this is a workaround in DPDK.
So there is no need to fix any commit in DPDK.

> 
> Signed-off-by: Marvin Liu <yong.liu@intel.com>
> 
> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> index 2175c1186..44aec2f47 100644
> --- a/examples/vhost/main.c
> +++ b/examples/vhost/main.c
> @@ -1520,7 +1520,8 @@ main(int argc, char *argv[])
>  		}
>  
>  		if (builtin_net_driver)
> -			rte_vhost_driver_set_features(file, VIRTIO_NET_FEATURES);

There is no need to remove VIRTIO_NET_FEATURES
although it's defined to 0 (as it's helpful for
users to understand this example).

> +			rte_vhost_driver_set_features(file,
> +				1ULL << VHOST_USER_F_PROTOCOL_FEATURES);

It's better to add some comments that this is
a workaround and why this workaround is needed.

>  
>  		if (mergeable == 0) {
>  			rte_vhost_driver_disable_features(file,
> -- 
> 2.17.0
> 

[PATCH v2] examples/vhost: workaround qemu abort

[Marvin Liu]

Current qemu vhost net ring start has a dependency on feature bit
VHOST_USER_F_PROTOCOL_FEATURES. If vhost device start without it, stop
action will be skipped. Consequently, same ioevent fd will be added
twice after reloading driver and then cause qemu abort. However, ring
should be initialized in an enabled state when this feature bit not
negotiated. Work around qemu issue by enabling this feature bit in vhost
user backend.

Signed-off-by: Marvin Liu <yong.liu@intel.com>

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index 2175c1186..4b87331fc 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -1519,8 +1519,12 @@ main(int argc, char *argv[])
 				"vhost driver register failure.\n");
 		}
 
-		if (builtin_net_driver)
+		if (builtin_net_driver) {
 			rte_vhost_driver_set_features(file, VIRTIO_NET_FEATURES);
+			/* Workaround for qemu vhost net device startup */
+			rte_vhost_driver_set_features(file,
+				1ULL << VHOST_USER_F_PROTOCOL_FEATURES);
+		}
 
 		if (mergeable == 0) {
 			rte_vhost_driver_disable_features(file,
-- 
2.17.0

Re: [PATCH v2] examples/vhost: workaround qemu abort

[Tiwei Bie]

On Wed, Jul 25, 2018 at 05:56:54PM +0800, Marvin Liu wrote:
> Current qemu vhost net ring start has a dependency on feature bit
> VHOST_USER_F_PROTOCOL_FEATURES. If vhost device start without it, stop
> action will be skipped. Consequently, same ioevent fd will be added
> twice after reloading driver and then cause qemu abort. However, ring
> should be initialized in an enabled state when this feature bit not
> negotiated. Work around qemu issue by enabling this feature bit in vhost
> user backend.
> 
> Signed-off-by: Marvin Liu <yong.liu@intel.com>
> 
> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> index 2175c1186..4b87331fc 100644
> --- a/examples/vhost/main.c
> +++ b/examples/vhost/main.c
> @@ -1519,8 +1519,12 @@ main(int argc, char *argv[])
>  				"vhost driver register failure.\n");
>  		}
>  
> -		if (builtin_net_driver)
> +		if (builtin_net_driver) {
>  			rte_vhost_driver_set_features(file, VIRTIO_NET_FEATURES);
> +			/* Workaround for qemu vhost net device startup */
> +			rte_vhost_driver_set_features(file,
> +				1ULL << VHOST_USER_F_PROTOCOL_FEATURES);

rte_vhost_driver_set_features() will overwrite the
existing value. You need to OR VIRTIO_NET_FEATURES
and (1ULL << VHOST_USER_F_PROTOCOL_FEATURES).



> +		}
>  
>  		if (mergeable == 0) {
>  			rte_vhost_driver_disable_features(file,
> -- 
> 2.17.0
> 

Re: [PATCH v2] examples/vhost: workaround qemu abort

[Liu, Yong]

> -----Original Message-----
> From: Bie, Tiwei
> Sent: Wednesday, July 25, 2018 10:19 AM
> To: Liu, Yong <yong.liu@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v2] examples/vhost: workaround qemu abort
> 
> On Wed, Jul 25, 2018 at 05:56:54PM +0800, Marvin Liu wrote:
> > Current qemu vhost net ring start has a dependency on feature bit
> > VHOST_USER_F_PROTOCOL_FEATURES. If vhost device start without it, stop
> > action will be skipped. Consequently, same ioevent fd will be added
> > twice after reloading driver and then cause qemu abort. However, ring
> > should be initialized in an enabled state when this feature bit not
> > negotiated. Work around qemu issue by enabling this feature bit in vhost
> > user backend.
> >
> > Signed-off-by: Marvin Liu <yong.liu@intel.com>
> >
> > diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> > index 2175c1186..4b87331fc 100644
> > --- a/examples/vhost/main.c
> > +++ b/examples/vhost/main.c
> > @@ -1519,8 +1519,12 @@ main(int argc, char *argv[])
> >  				"vhost driver register failure.\n");
> >  		}
> >
> > -		if (builtin_net_driver)
> > +		if (builtin_net_driver) {
> >  			rte_vhost_driver_set_features(file, VIRTIO_NET_FEATURES);
> > +			/* Workaround for qemu vhost net device startup */
> > +			rte_vhost_driver_set_features(file,
> > +				1ULL << VHOST_USER_F_PROTOCOL_FEATURES);
> 
> rte_vhost_driver_set_features() will overwrite the
> existing value. You need to OR VIRTIO_NET_FEATURES
> and (1ULL << VHOST_USER_F_PROTOCOL_FEATURES).
> 
> 

Tiwei,
Thanks for point out the error, I have sent v3 for correcting this.

Regards,
Marvin
> 
> > +		}
> >
> >  		if (mergeable == 0) {
> >  			rte_vhost_driver_disable_features(file,
> > --
> > 2.17.0
> >

[PATCH v3] examples/vhost: workaround qemu abort

[Marvin Liu]

Current qemu vhost net ring start has a dependency on feature bit
VHOST_USER_F_PROTOCOL_FEATURES. Without this feature, vhost ring can't
enabled and ioevent fd won't be deleted after vhost device stop. That
will cause qemu abort when reloading driver. Work around qemu issues by
enabling feature bit in vhost user backend.

Signed-off-by: Marvin Liu <yong.liu@intel.com>

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index 2175c1186..8573004dd 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c
@@ -1520,7 +1520,10 @@ main(int argc, char *argv[])
 		}
 
 		if (builtin_net_driver)
-			rte_vhost_driver_set_features(file, VIRTIO_NET_FEATURES);
+			/* Workaround for qemu vhost net device startup */
+			rte_vhost_driver_set_features(file,
+				VIRTIO_NET_FEATURES |
+				1ULL << VHOST_USER_F_PROTOCOL_FEATURES);
 
 		if (mergeable == 0) {
 			rte_vhost_driver_disable_features(file,
-- 
2.17.0

Re: [PATCH v3] examples/vhost: workaround qemu abort

[Tiwei Bie]

On Wed, Jul 25, 2018 at 06:25:56PM +0800, Marvin Liu wrote:
> Current qemu vhost net ring start has a dependency on feature bit
> VHOST_USER_F_PROTOCOL_FEATURES. Without this feature, vhost ring can't
> enabled and ioevent fd won't be deleted after vhost device stop. That
> will cause qemu abort when reloading driver. Work around qemu issues by
> enabling feature bit in vhost user backend.
> 
> Signed-off-by: Marvin Liu <yong.liu@intel.com>
> 
> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> index 2175c1186..8573004dd 100644
> --- a/examples/vhost/main.c
> +++ b/examples/vhost/main.c
> @@ -1520,7 +1520,10 @@ main(int argc, char *argv[])
>  		}
>  
>  		if (builtin_net_driver)
> -			rte_vhost_driver_set_features(file, VIRTIO_NET_FEATURES);
> +			/* Workaround for qemu vhost net device startup */
> +			rte_vhost_driver_set_features(file,
> +				VIRTIO_NET_FEATURES |
> +				1ULL << VHOST_USER_F_PROTOCOL_FEATURES);

I have taken a closer look. I think it's not really right
to just offer this bit like this. Because once this bit is
negotiated, QEMU will ask the builtin_net_driver for the
protocol features it supports. Currently, there is no API
to allow external backends like the builtin_net_driver in
this example to set the protocol features it supports, and
instead, the protocol features VHOST_USER_PROTOCOL_FEATURES
defined in librte_vhost will be returned to QEMU. This will
cause problems because the builtin_net_driver doesn't really
support those protocol features.


>  
>  		if (mergeable == 0) {
>  			rte_vhost_driver_disable_features(file,
> -- 
> 2.17.0
> 

Re: [PATCH v3] examples/vhost: workaround qemu abort

[Liu, Yong]

> -----Original Message-----
> From: Bie, Tiwei
> Sent: Wednesday, July 25, 2018 2:39 PM
> To: Liu, Yong <yong.liu@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v3] examples/vhost: workaround qemu abort
> 
> On Wed, Jul 25, 2018 at 06:25:56PM +0800, Marvin Liu wrote:
> > Current qemu vhost net ring start has a dependency on feature bit
> > VHOST_USER_F_PROTOCOL_FEATURES. Without this feature, vhost ring can't
> > enabled and ioevent fd won't be deleted after vhost device stop. That
> > will cause qemu abort when reloading driver. Work around qemu issues by
> > enabling feature bit in vhost user backend.
> >
> > Signed-off-by: Marvin Liu <yong.liu@intel.com>
> >
> > diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> > index 2175c1186..8573004dd 100644
> > --- a/examples/vhost/main.c
> > +++ b/examples/vhost/main.c
> > @@ -1520,7 +1520,10 @@ main(int argc, char *argv[])
> >  		}
> >
> >  		if (builtin_net_driver)
> > -			rte_vhost_driver_set_features(file, VIRTIO_NET_FEATURES);
> > +			/* Workaround for qemu vhost net device startup */
> > +			rte_vhost_driver_set_features(file,
> > +				VIRTIO_NET_FEATURES |
> > +				1ULL << VHOST_USER_F_PROTOCOL_FEATURES);
> 
> I have taken a closer look. I think it's not really right
> to just offer this bit like this. Because once this bit is
> negotiated, QEMU will ask the builtin_net_driver for the
> protocol features it supports. Currently, there is no API
> to allow external backends like the builtin_net_driver in
> this example to set the protocol features it supports, and
> instead, the protocol features VHOST_USER_PROTOCOL_FEATURES
> defined in librte_vhost will be returned to QEMU. This will
> cause problems because the builtin_net_driver doesn't really
> support those protocol features.
> 

Agreed, just announce protocol feature may have potential problem. Due to protocol features bits is fixed when no vdpa backend existing. IMHO, it has no benefit that adapt builtin_net_driver into vdpa framework. So just updated this sample's doc about builtin_net_driver compatible issue with QEMU.
Please kindly review that patch.

Regards,
Marvin

> 
> >
> >  		if (mergeable == 0) {
> >  			rte_vhost_driver_disable_features(file,
> > --
> > 2.17.0
> >