diff mbox series

[v3] examples/vhost: workaround qemu abort

Message ID 20180725102556.68604-1-yong.liu@intel.com (mailing list archive)
State Rejected, archived
Headers show
Series [v3] examples/vhost: workaround qemu abort | expand

Checks

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

Commit Message

Liu, Yong July 25, 2018, 10:25 a.m. UTC
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>

Comments

Tiwei Bie July 25, 2018, 6:39 a.m. UTC | #1
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
>
Liu, Yong July 26, 2018, 5:42 a.m. UTC | #2
> -----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
> >
diff mbox series

Patch

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,