tap-win32: don't abort in tap_enable(); enables -netdev tap

[Qemu-devel] [PATCH] tap-win32: don't abort in tap_enable(); enables -netdev tap

Posted by Andrew Baumann 7 years ago

The docs generally steer users away from using the legacy -net
parameter, however on win32 attempting to enable a tap device using
-netdev tap fails at an abort() in tap_enable(). Removing the abort()s
seems to be enough to get everything working, so do that.

Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
---
 net/tap-win32.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/tap-win32.c b/net/tap-win32.c
index 662f9b6..3620843 100644
--- a/net/tap-win32.c
+++ b/net/tap-win32.c
@@ -811,10 +811,10 @@ int net_init_tap(const Netdev *netdev, const char *name,
 
 int tap_enable(NetClientState *nc)
 {
-    abort();
+    return 0;
 }
 
 int tap_disable(NetClientState *nc)
 {
-    abort();
+    return 0;
 }
-- 
2.8.3

Re: [Qemu-devel] [PATCH for 2.9?] tap-win32: don't abort in tap_enable(); enables -netdev tap

Posted by Stefan Weil 7 years ago

Am 25.03.2017 um 00:46 schrieb Andrew Baumann:
> The docs generally steer users away from using the legacy -net
> parameter, however on win32 attempting to enable a tap device using
> -netdev tap fails at an abort() in tap_enable(). Removing the abort()s
> seems to be enough to get everything working, so do that.
>
> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> ---
>  net/tap-win32.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/tap-win32.c b/net/tap-win32.c
> index 662f9b6..3620843 100644
> --- a/net/tap-win32.c
> +++ b/net/tap-win32.c
> @@ -811,10 +811,10 @@ int net_init_tap(const Netdev *netdev, const char *name,
>
>  int tap_enable(NetClientState *nc)
>  {
> -    abort();
> +    return 0;
>  }
>
>  int tap_disable(NetClientState *nc)
>  {
> -    abort();
> +    return 0;
>  }

As I never worked with TAP on Windows, I cannot say much to this fix.

Jason, what is the use of tap_enable, tap_disable? Is it fine
to simply do nothing on Windows here?

And is this something for QEMU‌ 2.9 (I added question to subject line)?

Stefan

Re: [Qemu-devel] [PATCH for 2.9?] tap-win32: don't abort in tap_enable(); enables -netdev tap

Posted by Andrew Baumann via Qemu-devel 7 years ago

> From: Stefan Weil [mailto:sw@weilnetz.de]
> Sent: Tuesday, 28 March 2017 11:28

> Am 25.03.2017 um 00:46 schrieb Andrew Baumann:
> > The docs generally steer users away from using the legacy -net
> > parameter, however on win32 attempting to enable a tap device using
> > -netdev tap fails at an abort() in tap_enable(). Removing the abort()s
> > seems to be enough to get everything working, so do that.
> >
> > Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
> > ---
> >  net/tap-win32.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/tap-win32.c b/net/tap-win32.c
> > index 662f9b6..3620843 100644
> > --- a/net/tap-win32.c
> > +++ b/net/tap-win32.c
> > @@ -811,10 +811,10 @@ int net_init_tap(const Netdev *netdev, const char
> *name,
> >
> >  int tap_enable(NetClientState *nc)
> >  {
> > -    abort();
> > +    return 0;
> >  }
> >
> >  int tap_disable(NetClientState *nc)
> >  {
> > -    abort();
> > +    return 0;
> >  }
> 
> As I never worked with TAP on Windows, I cannot say much to this fix.
> 
> Jason, what is the use of tap_enable, tap_disable? Is it fine
> to simply do nothing on Windows here?

I was also hoping for a review -- I'm no expert on this stuff either, but my quick reading of those code paths is that they issue ioctls to enable/disable packet reception on the underlying tap device. As win32 TAP is implemented, that is already enabled from start of day.

It's possible this patch still does not permit dynamic reconfiguration of tap devices (e.g. from the monitor console). However, it does work with the -netdev tap option on the command-line.

> And is this something for QEMU‌ 2.9 (I added question to subject line)?

Ideally, yes. If not, -netdev tap will continue to blow up in the abort as it does today...

Andrew

Re: [Qemu-devel] [PATCH for 2.9?] tap-win32: don't abort in tap_enable(); enables -netdev tap

Posted by Jason Wang 7 years ago


On 2017年03月29日 02:55, Andrew Baumann wrote:
>> From: Stefan Weil [mailto:sw@weilnetz.de]
>> Sent: Tuesday, 28 March 2017 11:28
>> Am 25.03.2017 um 00:46 schrieb Andrew Baumann:
>>> The docs generally steer users away from using the legacy -net
>>> parameter, however on win32 attempting to enable a tap device using
>>> -netdev tap fails at an abort() in tap_enable(). Removing the abort()s
>>> seems to be enough to get everything working, so do that.
>>>
>>> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
>>> ---
>>>   net/tap-win32.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/tap-win32.c b/net/tap-win32.c
>>> index 662f9b6..3620843 100644
>>> --- a/net/tap-win32.c
>>> +++ b/net/tap-win32.c
>>> @@ -811,10 +811,10 @@ int net_init_tap(const Netdev *netdev, const char
>> *name,
>>>   int tap_enable(NetClientState *nc)
>>>   {
>>> -    abort();
>>> +    return 0;
>>>   }
>>>
>>>   int tap_disable(NetClientState *nc)
>>>   {
>>> -    abort();
>>> +    return 0;
>>>   }
>> As I never worked with TAP on Windows, I cannot say much to this fix.
>>
>> Jason, what is the use of tap_enable, tap_disable?

It should be only used when we want to enable and disable a specific 
queue of a multiqueue supported tap.

>>   Is it fine
>> to simply do nothing on Windows here?

Unless windows support multiqueue tap, we should keep the assert here.

> I was also hoping for a review -- I'm no expert on this stuff either, but my quick reading of those code paths is that they issue ioctls to enable/disable packet reception on the underlying tap device. As win32 TAP is implemented, that is already enabled from start of day.
>
> It's possible this patch still does not permit dynamic reconfiguration of tap devices (e.g. from the monitor console). However, it does work with the -netdev tap option on the command-line.
>
>> And is this something for QEMU‌ 2.9 (I added question to subject line)?
> Ideally, yes. If not, -netdev tap will continue to blow up in the abort as it does today...
>
> Andrew

Yes, so the problem is we should prevent tap_enable() and tap_disable() 
from being called if multiqueue is disabled.

I believe the following patch can fix this issue, could you give a try 
on this?

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index c321680..7d091c9 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -510,6 +510,10 @@ static int peer_attach(VirtIONet *n, int index)
          return 0;
      }

+    if (n->max_queues == 1) {
+        return 0;
+    }
+
      return tap_enable(nc->peer);
  }

Re: [Qemu-devel] [PATCH for 2.9?] tap-win32: don't abort in tap_enable(); enables -netdev tap

Posted by Andrew Baumann via Qemu-devel 7 years ago

> From: Jason Wang [mailto:jasowang@redhat.com]
> Sent: Tuesday, 28 March 2017 19:39
> 
> On 2017年03月29日 02:55, Andrew Baumann wrote:
> >> From: Stefan Weil [mailto:sw@weilnetz.de]
> >> Sent: Tuesday, 28 March 2017 11:28
> >> Am 25.03.2017 um 00:46 schrieb Andrew Baumann:
> >>> The docs generally steer users away from using the legacy -net
> >>> parameter, however on win32 attempting to enable a tap device using
> >>> -netdev tap fails at an abort() in tap_enable(). Removing the abort()s
> >>> seems to be enough to get everything working, so do that.
> >>>
> >>> Signed-off-by: Andrew Baumann <Andrew.Baumann@microsoft.com>
[...]
> >> Jason, what is the use of tap_enable, tap_disable?
> 
> It should be only used when we want to enable and disable a specific
> queue of a multiqueue supported tap.
> 
> >>   Is it fine
> >> to simply do nothing on Windows here?
> 
> Unless windows support multiqueue tap, we should keep the assert here.
> 
> > I was also hoping for a review -- I'm no expert on this stuff either, but my
> quick reading of those code paths is that they issue ioctls to enable/disable
> packet reception on the underlying tap device. As win32 TAP is implemented,
> that is already enabled from start of day.
> >
> > It's possible this patch still does not permit dynamic reconfiguration of tap
> devices (e.g. from the monitor console). However, it does work with the -
> netdev tap option on the command-line.
> >
> >> And is this something for QEMU‌ 2.9 (I added question to subject line)?
> > Ideally, yes. If not, -netdev tap will continue to blow up in the abort as it does
> today...
> >
> > Andrew
> 
> Yes, so the problem is we should prevent tap_enable() and tap_disable()
> from being called if multiqueue is disabled.
> 
> I believe the following patch can fix this issue, could you give a try
> on this?
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index c321680..7d091c9 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -510,6 +510,10 @@ static int peer_attach(VirtIONet *n, int index)
>           return 0;
>       }
> 
> +    if (n->max_queues == 1) {
> +        return 0;
> +    }
> +
>       return tap_enable(nc->peer);
>   }
> 

Yep, this works. Thanks!

Andrew

Re: [Qemu-devel] [PATCH for 2.9?] tap-win32: don't abort in tap_enable(); enables -netdev tap

Posted by Jason Wang 7 years ago


On 2017年03月29日 12:13, Andrew Baumann via Qemu-devel wrote:
>> From: Jason Wang [mailto:jasowang@redhat.com]
>> Sent: Tuesday, 28 March 2017 19:39
>>
>> On 2017年03月29日 02:55, Andrew Baumann wrote:
>>>> From: Stefan Weil [mailto:sw@weilnetz.de]
>>>> Sent: Tuesday, 28 March 2017 11:28
>>>> Am 25.03.2017 um 00:46 schrieb Andrew Baumann:
>>>>> The docs generally steer users away from using the legacy -net
>>>>> parameter, however on win32 attempting to enable a tap device using
>>>>> -netdev tap fails at an abort() in tap_enable(). Removing the abort()s
>>>>> seems to be enough to get everything working, so do that.
>>>>>
>>>>> Signed-off-by: Andrew Baumann<Andrew.Baumann@microsoft.com>
> [...]
>>>> Jason, what is the use of tap_enable, tap_disable?
>> It should be only used when we want to enable and disable a specific
>> queue of a multiqueue supported tap.
>>
>>>>    Is it fine
>>>> to simply do nothing on Windows here?
>> Unless windows support multiqueue tap, we should keep the assert here.
>>
>>> I was also hoping for a review -- I'm no expert on this stuff either, but my
>> quick reading of those code paths is that they issue ioctls to enable/disable
>> packet reception on the underlying tap device. As win32 TAP is implemented,
>> that is already enabled from start of day.
>>> It's possible this patch still does not permit dynamic reconfiguration of tap
>> devices (e.g. from the monitor console). However, it does work with the -
>> netdev tap option on the command-line.
>>>> And is this something for QEMU‌ 2.9 (I added question to subject line)?
>>> Ideally, yes. If not, -netdev tap will continue to blow up in the abort as it does
>> today...
>>> Andrew
>> Yes, so the problem is we should prevent tap_enable() and tap_disable()
>> from being called if multiqueue is disabled.
>>
>> I believe the following patch can fix this issue, could you give a try
>> on this?
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index c321680..7d091c9 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -510,6 +510,10 @@ static int peer_attach(VirtIONet *n, int index)
>>            return 0;
>>        }
>>
>> +    if (n->max_queues == 1) {
>> +        return 0;
>> +    }
>> +
>>        return tap_enable(nc->peer);
>>    }
>>
> Yep, this works. Thanks!
>
> Andrew

Thanks, queued this for 2.9.