[RFC] um: virtio: Workaround for using virtio_net with snabb

From: Erel Geron <erelx.geron@intel.com>

Workaround a crash of snabb when using virtio_uml.

Signed-off-by: Erel Geron <erelx.geron@intel.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 arch/um/drivers/virtio_uml.c | 9 +++++++++
 1 file changed, 9 insertions(+)

On 31/07/2019 21:30, Johannes Berg wrote:
> From: Erel Geron <erelx.geron@intel.com>
> 
> Workaround a crash of snabb when using virtio_uml.
> 
> Signed-off-by: Erel Geron <erelx.geron@intel.com>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>   arch/um/drivers/virtio_uml.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/arch/um/drivers/virtio_uml.c b/arch/um/drivers/virtio_uml.c
> index 30eeb907f42a..9c1308f56b93 100644
> --- a/arch/um/drivers/virtio_uml.c
> +++ b/arch/um/drivers/virtio_uml.c
> @@ -31,6 +31,10 @@
>   #include <os.h>
>   #include "vhost_user.h"
>   
> +/* TODO for the workaround in vhost_user_init */
> +#include <linux/virtio_ids.h>
> +#include <linux/virtio_net.h>
> +
>   /* Workaround due to a conflict between irq_user.h and irqreturn.h */
>   #ifdef IRQ_NONE
>   #undef IRQ_NONE
> @@ -200,6 +204,11 @@ static int vhost_user_init(struct virtio_uml_device *vu_dev)
>   	if (rc)
>   		return rc;
>   
> +	/* TODO workaround for some implementations */
> +	if (vu_dev->vdev.id.device == VIRTIO_ID_NET)
> +		vu_dev->features &= ~BIT_ULL(VIRTIO_NET_F_CTRL_VQ) &
> +				~BIT_ULL(VIRTIO_NET_F_MQ);

If it is needed only for some implementations, then it should be an option.

Have you tried it vs let's say dpdk (IIRC it supports vhost_user as an 
interface).

> +
>   	if (vu_dev->features & BIT_ULL(VHOST_USER_F_PROTOCOL_FEATURES)) {
>   		rc = vhost_user_get_protocol_features(vu_dev,
>   				&vu_dev->protocol_features);
>

On Wed, 2019-08-07 at 17:14 +0100, Anton Ivanov wrote:
  
> > +	/* TODO workaround for some implementations */
> > +	if (vu_dev->vdev.id.device == VIRTIO_ID_NET)
> > +		vu_dev->features &= ~BIT_ULL(VIRTIO_NET_F_CTRL_VQ) &
> > +				~BIT_ULL(VIRTIO_NET_F_MQ);
> 
> If it is needed only for some implementations, then it should be an option.

Yeah, I guess it should be. Then again, will anyone really even want to
use snabb? I don't know, perhaps, but perhaps it should just be fixed on
the snabb side? It stands to reason that regardless of what we do here,
it shouldn't just crash.

> Have you tried it vs let's say dpdk (IIRC it supports vhost_user as an 
> interface).

No, and we probably won't, we're not really particularly interested in
the network aspect of it. Trying it out here with existing device
implementations was really just for development, ultimately our plan is
to have our own device implementation(s) and driver(s). I'm hoping to
publish and upstream something that will let you run multiple UML
machines in "synchronized time travel" mode over vhost-user though.

johannes

On 08/08/2019 08:26, Johannes Berg wrote:
> On Wed, 2019-08-07 at 17:14 +0100, Anton Ivanov wrote:
>    
>>> +	/* TODO workaround for some implementations */
>>> +	if (vu_dev->vdev.id.device == VIRTIO_ID_NET)
>>> +		vu_dev->features &= ~BIT_ULL(VIRTIO_NET_F_CTRL_VQ) &
>>> +				~BIT_ULL(VIRTIO_NET_F_MQ);
>>
>> If it is needed only for some implementations, then it should be an option.
> 
> Yeah, I guess it should be. Then again, will anyone really even want to
> use snabb? I don't know, perhaps, but perhaps it should just be fixed on
> the snabb side? It stands to reason that regardless of what we do here,
> it shouldn't just crash.

True.

However, a cursory search shows these being turned off in a few other 
cases as well including vs some DPDK based software.

So snabb is not alone having issues with MQ and VQ flags.

> 
>> Have you tried it vs let's say dpdk (IIRC it supports vhost_user as an
>> interface).
> 
> No, and we probably won't, we're not really particularly interested in
> the network aspect of it. Trying it out here with existing device
> implementations was really just for development, ultimately our plan is
> to have our own device implementation(s) and driver(s). 

Understood, however for it to be upstreamed it should work with 
something else which is publicly available. That is why I am going to 
try it vs Bess/DPDK (it is also lowest effort for me as I already have 
that set-up).

> I'm hoping to
> publish and upstream something that will let you run multiple UML
> machines in "synchronized time travel" mode over vhost-user though.

vhost-user clock? Interesting idea.

> 
> johannes
> 
> 
> _______________________________________________
> linux-um mailing list
> linux-um@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-um
>

On Thu, 2019-08-08 at 08:56 +0100, Anton Ivanov wrote:

> However, a cursory search shows these being turned off in a few other 
> cases as well including vs some DPDK based software.
> 
> So snabb is not alone having issues with MQ and VQ flags.

Oh. I hadn't really researched it much more.

> > > Have you tried it vs let's say dpdk (IIRC it supports vhost_user as an
> > > interface).
> > 
> > No, and we probably won't, we're not really particularly interested in
> > the network aspect of it. Trying it out here with existing device
> > implementations was really just for development, ultimately our plan is
> > to have our own device implementation(s) and driver(s). 
> 
> Understood, however for it to be upstreamed it should work with 
> something else which is publicly available. That is why I am going to 
> try it vs Bess/DPDK (it is also lowest effort for me as I already have 
> that set-up).

True. Erel tried some of the samples from qemu contrib as well as snabb.
But if you have a DPDK setup then that's certainly an interesting option
to try, one that would likely also be useful for others to use this
with.

If needed, we can respin this as a real patch to add options to the
command line, I suppose.

> > I'm hoping to
> > publish and upstream something that will let you run multiple UML
> > machines in "synchronized time travel" mode over vhost-user though.
> 
> vhost-user clock? Interesting idea.

I'm on vacation right now, but once I'm back I'll start looking into
that. Not sure how to really tie that in though since it needs the idle
hook too, and that's arch specific. So I guess it just has to be
integrated with all the time-travel mode code, rather than being more
generic/separate.

johannes

On 09/08/2019 08:19, Johannes Berg wrote:
> On Thu, 2019-08-08 at 08:56 +0100, Anton Ivanov wrote:
> 
>> However, a cursory search shows these being turned off in a few other
>> cases as well including vs some DPDK based software.
>>
>> So snabb is not alone having issues with MQ and VQ flags.
> 
> Oh. I hadn't really researched it much more.
> 
>>>> Have you tried it vs let's say dpdk (IIRC it supports vhost_user as an
>>>> interface).
>>>
>>> No, and we probably won't, we're not really particularly interested in
>>> the network aspect of it. Trying it out here with existing device
>>> implementations was really just for development, ultimately our plan is
>>> to have our own device implementation(s) and driver(s).
>>
>> Understood, however for it to be upstreamed it should work with
>> something else which is publicly available. That is why I am going to
>> try it vs Bess/DPDK (it is also lowest effort for me as I already have
>> that set-up).
> 
> True. Erel tried some of the samples from qemu contrib as well as snabb.
> But if you have a DPDK setup then that's certainly an interesting option
> to try, one that would likely also be useful for others to use this
> with.

I am working on this at the moment: https://github.com/NetSys/bess

It is a simple DPDK based L2/L3 forwarder.

I have it set-up already so I can try to use it as a test case.

> 
> If needed, we can respin this as a real patch to add options to the
> command line, I suppose.

I hope to have some answers to that by early next week.

> 
>>> I'm hoping to
>>> publish and upstream something that will let you run multiple UML
>>> machines in "synchronized time travel" mode over vhost-user though.
>>
>> vhost-user clock? Interesting idea.
> 
> I'm on vacation right now, but once I'm back I'll start looking into
> that. Not sure how to really tie that in though since it needs the idle
> hook too, and that's arch specific. So I guess it just has to be
> integrated with all the time-travel mode code, rather than being more
> generic/separate.
> 
> johannes
> 
>