virtio-net not working with the latest qemu-kvm git

virtio-net not working with the latest qemu-kvm git

[Sridhar Samudrala]

When i moved to the latest qemu-kvm git tree from kvm-85, i noticed that
networking stopped working between the host and the guest.
It started working when i put the device in promiscuos mode by running
tcpdump in background on the guest. 

After browsing through the recent patches, i found that the following commit
is causing the regression.

Remove stray GSO code from virtio_net (Mark McLoughlin)
http://git.kernel.org/?p=virt/kvm/qemu-kvm.git;a=commitdiff;h=559a8f45f34cc50d1a60b4f67a06614d506b2e01

The comment doesn't seem to match with the code that is removed with this patch.

Thanks
Sridhar

Re: virtio-net not working with the latest qemu-kvm git

[Alex Williamson]

On Mon, 2009-05-04 at 09:50 -0700, Sridhar Samudrala wrote:
> When i moved to the latest qemu-kvm git tree from kvm-85, i noticed that
> networking stopped working between the host and the guest.
> It started working when i put the device in promiscuos mode by running
> tcpdump in background on the guest. 
> 
> After browsing through the recent patches, i found that the following commit
> is causing the regression.
> 
> Remove stray GSO code from virtio_net (Mark McLoughlin)
> http://git.kernel.org/?p=virt/kvm/qemu-kvm.git;a=commitdiff;h=559a8f45f34cc50d1a60b4f67a06614d506b2e01
> 
> The comment doesn't seem to match with the code that is removed with this patch.

Yep, I agree, the removed code is not bogus.  We have to skip the vnet
header to to get to the ethernet header, which we do the filtering on.

Alex

[PATCH 1/1] qemu-kvm: virtio-net: Re-instate GSO code removed upstream

[Mark McLoughlin]

This commit:

   commit 559a8f45f34cc50d1a60b4f67a06614d506b2e01
   Subject: Remove stray GSO code from virtio_net (Mark McLoughlin)

Removed some GSO code from upstream qemu.git, but it needs to
be re-instated in qemu-kvm.git.

Reported-by: Sridhar Samudrala <sri@us.ibm.com>
Signed-off-by: Mark McLoughlin <markmc@redhat.com>
---
 hw/virtio-net.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/hw/virtio-net.c b/hw/virtio-net.c
index ac8e030..e5d7add 100644
--- a/hw/virtio-net.c
+++ b/hw/virtio-net.c
@@ -424,6 +424,11 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
     if (n->promisc)
         return 1;
 
+#ifdef TAP_VNET_HDR
+    if (tap_has_vnet_hdr(n->vc->vlan->first_client))
+        ptr += sizeof(struct virtio_net_hdr);
+#endif
+
     if (!memcmp(&ptr[12], vlan, sizeof(vlan))) {
         int vid = be16_to_cpup((uint16_t *)(ptr + 14)) & 0xfff;
         if (!(n->vlans[vid >> 5] & (1U << (vid & 0x1f))))
-- 
1.6.0.6

Re: virtio-net not working with the latest qemu-kvm git

[Mark McLoughlin]

On Mon, 2009-05-04 at 11:44 -0600, Alex Williamson wrote:
> On Mon, 2009-05-04 at 09:50 -0700, Sridhar Samudrala wrote:
> > When i moved to the latest qemu-kvm git tree from kvm-85, i noticed that
> > networking stopped working between the host and the guest.
> > It started working when i put the device in promiscuos mode by running
> > tcpdump in background on the guest. 
> > 
> > After browsing through the recent patches, i found that the following commit
> > is causing the regression.
> > 
> > Remove stray GSO code from virtio_net (Mark McLoughlin)
> > http://git.kernel.org/?p=virt/kvm/qemu-kvm.git;a=commitdiff;h=559a8f45f34cc50d1a60b4f67a06614d506b2e01
> > 
> > The comment doesn't seem to match with the code that is removed with this patch.
> 
> Yep, I agree, the removed code is not bogus.  We have to skip the vnet
> header to to get to the ethernet header, which we do the filtering on.

The code was removed in upstream qemu.git, but we need it re-instated in
qemu-kvm.git. Just sent a patch to do that.

Thanks,
Mark.

Re: [PATCH 1/1] qemu-kvm: virtio-net: Re-instate GSO code removed upstream

[Avi Kivity]

Mark McLoughlin wrote:
> This commit:
>
>    commit 559a8f45f34cc50d1a60b4f67a06614d506b2e01
>    Subject: Remove stray GSO code from virtio_net (Mark McLoughlin)
>
> Removed some GSO code from upstream qemu.git, but it needs to
> be re-instated in qemu-kvm.git.
>
>   

Applied, thanks.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: virtio-net not working with the latest qemu-kvm git

[Avi Kivity]

Sridhar Samudrala wrote:
> When i moved to the latest qemu-kvm git tree from kvm-85, i noticed that
> networking stopped working between the host and the guest.
> It started working when i put the device in promiscuos mode by running
> tcpdump in background on the guest. 
>
> After browsing through the recent patches, i found that the following commit
> is causing the regression.
>
> Remove stray GSO code from virtio_net (Mark McLoughlin)
> http://git.kernel.org/?p=virt/kvm/qemu-kvm.git;a=commitdiff;h=559a8f45f34cc50d1a60b4f67a06614d506b2e01
>
> The comment doesn't seem to match with the code that is removed with this patch.
>   

I applied a patch from Mark that fixes this.  Thanks for reporting.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH 1/1] qemu-kvm: virtio-net: Re-instate GSO code removed upstream

[Mark McLoughlin]

On Tue, 2009-05-05 at 09:56 +0100, Mark McLoughlin wrote:
> This commit:
> 
>    commit 559a8f45f34cc50d1a60b4f67a06614d506b2e01
>    Subject: Remove stray GSO code from virtio_net (Mark McLoughlin)
> 
> Removed some GSO code from upstream qemu.git, but it needs to
> be re-instated in qemu-kvm.git.
> 
> Reported-by: Sridhar Samudrala <sri@us.ibm.com>
> Signed-off-by: Mark McLoughlin <markmc@redhat.com>
> ---
>  hw/virtio-net.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> index ac8e030..e5d7add 100644
> --- a/hw/virtio-net.c
> +++ b/hw/virtio-net.c
> @@ -424,6 +424,11 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
>      if (n->promisc)
>          return 1;
>  
> +#ifdef TAP_VNET_HDR
> +    if (tap_has_vnet_hdr(n->vc->vlan->first_client))
> +        ptr += sizeof(struct virtio_net_hdr);
> +#endif
> +
>      if (!memcmp(&ptr[12], vlan, sizeof(vlan))) {
>          int vid = be16_to_cpup((uint16_t *)(ptr + 14)) & 0xfff;
>          if (!(n->vlans[vid >> 5] & (1U << (vid & 0x1f))))

I'm not sure[1] how we didn't notice, but this has been broken on the
stable-0.10 branch since 0.10.3; please apply there too

See:

  https://bugzilla.redhat.com/522994

Cheers,
Mark.

[1] - well, one reason is that libvirt doesn't seem to be enabling
vnet_hdr at the moment. That's the next thing to look at

Re: [PATCH 1/1] qemu-kvm: virtio-net: Re-instate GSO code removed upstream

[Mark McLoughlin]

On Tue, 2009-09-29 at 21:45 +0100, Mark McLoughlin wrote:
> On Tue, 2009-05-05 at 09:56 +0100, Mark McLoughlin wrote:
> > This commit:
> > 
> >    commit 559a8f45f34cc50d1a60b4f67a06614d506b2e01
> >    Subject: Remove stray GSO code from virtio_net (Mark McLoughlin)
> > 
> > Removed some GSO code from upstream qemu.git, but it needs to
> > be re-instated in qemu-kvm.git.
> > 
> > Reported-by: Sridhar Samudrala <sri@us.ibm.com>
> > Signed-off-by: Mark McLoughlin <markmc@redhat.com>
> > ---
> >  hw/virtio-net.c |    5 +++++
> >  1 files changed, 5 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> > index ac8e030..e5d7add 100644
> > --- a/hw/virtio-net.c
> > +++ b/hw/virtio-net.c
> > @@ -424,6 +424,11 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
> >      if (n->promisc)
> >          return 1;
> >  
> > +#ifdef TAP_VNET_HDR
> > +    if (tap_has_vnet_hdr(n->vc->vlan->first_client))
> > +        ptr += sizeof(struct virtio_net_hdr);
> > +#endif
> > +
> >      if (!memcmp(&ptr[12], vlan, sizeof(vlan))) {
> >          int vid = be16_to_cpup((uint16_t *)(ptr + 14)) & 0xfff;
> >          if (!(n->vlans[vid >> 5] & (1U << (vid & 0x1f))))
> 
> I'm not sure[1] how we didn't notice, but this has been broken on the
> stable-0.10 branch since 0.10.3; please apply there too
> 
> See:
> 
>   https://bugzilla.redhat.com/522994
> 
> Cheers,
> Mark.
> 
> [1] - well, one reason is that libvirt doesn't seem to be enabling
> vnet_hdr at the moment. That's the next thing to look at

Oh, another reason is that this is only a problem with 2.6.30 guests
which enable MAC based receive filtering

Cheers,
Mark.

Re: [PATCH 1/1] qemu-kvm: virtio-net: Re-instate GSO code removed upstream

[Avi Kivity]

On 09/29/2009 10:45 PM, Mark McLoughlin wrote:
> On Tue, 2009-05-05 at 09:56 +0100, Mark McLoughlin wrote:
>    
>> This commit:
>>
>>     commit 559a8f45f34cc50d1a60b4f67a06614d506b2e01
>>     Subject: Remove stray GSO code from virtio_net (Mark McLoughlin)
>>
>> Removed some GSO code from upstream qemu.git, but it needs to
>> be re-instated in qemu-kvm.git.
>>
>> Reported-by: Sridhar Samudrala<sri@us.ibm.com>
>> Signed-off-by: Mark McLoughlin<markmc@redhat.com>
>> ---
>>   hw/virtio-net.c |    5 +++++
>>   1 files changed, 5 insertions(+), 0 deletions(-)
>>
>> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
>> index ac8e030..e5d7add 100644
>> --- a/hw/virtio-net.c
>> +++ b/hw/virtio-net.c
>> @@ -424,6 +424,11 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
>>       if (n->promisc)
>>           return 1;
>>
>> +#ifdef TAP_VNET_HDR
>> +    if (tap_has_vnet_hdr(n->vc->vlan->first_client))
>> +        ptr += sizeof(struct virtio_net_hdr);
>> +#endif
>> +
>>       if (!memcmp(&ptr[12], vlan, sizeof(vlan))) {
>>           int vid = be16_to_cpup((uint16_t *)(ptr + 14))&  0xfff;
>>           if (!(n->vlans[vid>>  5]&  (1U<<  (vid&  0x1f))))
>>      
> I'm not sure[1] how we didn't notice, but this has been broken on the
> stable-0.10 branch since 0.10.3; please apply there too
>
>    

Thanks, we'll queue it on stable-0.10.

Anthony/Glauber, is 0.10.7 in the works?  If not, we'll release it as 
0.10.6.1.


-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH 1/1] qemu-kvm: virtio-net: Re-instate GSO code removed upstream

[Glauber Costa]

On Wed, Sep 30, 2009 at 08:24:18AM +0200, Avi Kivity wrote:
> On 09/29/2009 10:45 PM, Mark McLoughlin wrote:
>> On Tue, 2009-05-05 at 09:56 +0100, Mark McLoughlin wrote:
>>    
>>> This commit:
>>>
>>>     commit 559a8f45f34cc50d1a60b4f67a06614d506b2e01
>>>     Subject: Remove stray GSO code from virtio_net (Mark McLoughlin)
>>>
>>> Removed some GSO code from upstream qemu.git, but it needs to
>>> be re-instated in qemu-kvm.git.
>>>
>>> Reported-by: Sridhar Samudrala<sri@us.ibm.com>
>>> Signed-off-by: Mark McLoughlin<markmc@redhat.com>
>>> ---
>>>   hw/virtio-net.c |    5 +++++
>>>   1 files changed, 5 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
>>> index ac8e030..e5d7add 100644
>>> --- a/hw/virtio-net.c
>>> +++ b/hw/virtio-net.c
>>> @@ -424,6 +424,11 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
>>>       if (n->promisc)
>>>           return 1;
>>>
>>> +#ifdef TAP_VNET_HDR
>>> +    if (tap_has_vnet_hdr(n->vc->vlan->first_client))
>>> +        ptr += sizeof(struct virtio_net_hdr);
>>> +#endif
>>> +
>>>       if (!memcmp(&ptr[12], vlan, sizeof(vlan))) {
>>>           int vid = be16_to_cpup((uint16_t *)(ptr + 14))&  0xfff;
>>>           if (!(n->vlans[vid>>  5]&  (1U<<  (vid&  0x1f))))
>>>      
>> I'm not sure[1] how we didn't notice, but this has been broken on the
>> stable-0.10 branch since 0.10.3; please apply there too
>>
>>    
>
> Thanks, we'll queue it on stable-0.10.
>
> Anthony/Glauber, is 0.10.7 in the works?  If not, we'll release it as  
> 0.10.6.1.
Since it is just one patch, I don't see a problem in anthony picking it directly
and making a new release.

Re: [PATCH 1/1] qemu-kvm: virtio-net: Re-instate GSO code removed upstream

[Mark McLoughlin]

On Wed, 2009-09-30 at 08:24 -0300, Glauber Costa wrote:
> On Wed, Sep 30, 2009 at 08:24:18AM +0200, Avi Kivity wrote:
> > On 09/29/2009 10:45 PM, Mark McLoughlin wrote:
> >> On Tue, 2009-05-05 at 09:56 +0100, Mark McLoughlin wrote:
> >>    
> >>> This commit:
> >>>
> >>>     commit 559a8f45f34cc50d1a60b4f67a06614d506b2e01
> >>>     Subject: Remove stray GSO code from virtio_net (Mark McLoughlin)
> >>>
> >>> Removed some GSO code from upstream qemu.git, but it needs to
> >>> be re-instated in qemu-kvm.git.
> >>>
> >>> Reported-by: Sridhar Samudrala<sri@us.ibm.com>
> >>> Signed-off-by: Mark McLoughlin<markmc@redhat.com>
> >>> ---
> >>>   hw/virtio-net.c |    5 +++++
> >>>   1 files changed, 5 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/hw/virtio-net.c b/hw/virtio-net.c
> >>> index ac8e030..e5d7add 100644
> >>> --- a/hw/virtio-net.c
> >>> +++ b/hw/virtio-net.c
> >>> @@ -424,6 +424,11 @@ static int receive_filter(VirtIONet *n, const uint8_t *buf, int size)
> >>>       if (n->promisc)
> >>>           return 1;
> >>>
> >>> +#ifdef TAP_VNET_HDR
> >>> +    if (tap_has_vnet_hdr(n->vc->vlan->first_client))
> >>> +        ptr += sizeof(struct virtio_net_hdr);
> >>> +#endif
> >>> +
> >>>       if (!memcmp(&ptr[12], vlan, sizeof(vlan))) {
> >>>           int vid = be16_to_cpup((uint16_t *)(ptr + 14))&  0xfff;
> >>>           if (!(n->vlans[vid>>  5]&  (1U<<  (vid&  0x1f))))
> >>>      
> >> I'm not sure[1] how we didn't notice, but this has been broken on the
> >> stable-0.10 branch since 0.10.3; please apply there too
> >>
> >>    
> >
> > Thanks, we'll queue it on stable-0.10.
> >
> > Anthony/Glauber, is 0.10.7 in the works?  If not, we'll release it as  
> > 0.10.6.1.
> Since it is just one patch, I don't see a problem in anthony picking it directly
> and making a new release.

It's not for qemu.git, it's for qemu-kvm.git - see the changelog

Cheers,
Mark.

Re: [PATCH 1/1] qemu-kvm: virtio-net: Re-instate GSO code removed upstream

[Anthony Liguori]

Avi Kivity wrote:
> Anthony/Glauber, is 0.10.7 in the works?  If not, we'll release it as 
> 0.10.6.1.

Yes.  I can release it very soon.

-- 
Regards,

Anthony Liguori

Re: [PATCH 1/1] qemu-kvm: virtio-net: Re-instate GSO code removed upstream

[Christoph Hellwig]

I might sound like a broken record, but why isn't the full GSO support
for virtio-net upstream in qemu?

Re: [PATCH 1/1] qemu-kvm: virtio-net: Re-instate GSO code removed upstream

[Avi Kivity]

On 09/30/2009 03:51 PM, Christoph Hellwig wrote:
> I might sound like a broken record, but why isn't the full GSO support
> for virtio-net upstream in qemu?
>    

IIRC the current hacks are not upstream quality.

The problem (again IIRC) is that the guest and host negotiate a 
protocol, but the qemu vlan model doesn't have a guest and a host, it 
has peers (possibly more than two), so a lot of translation has to take 
place if you have one peer supporting a guest feature and another not.

IMO the best way out is to drop the vlan model.  It has its uses, but 
they can all be implemented in other ways, and are all have minor usage 
compared to the business of getting data into and out of a guest.

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/1] qemu-kvm: virtio-net: Re-instate GSO code removed upstream

[Mark McLoughlin]

On Wed, 2009-09-30 at 15:55 +0200, Avi Kivity wrote:
> On 09/30/2009 03:51 PM, Christoph Hellwig wrote:
> > I might sound like a broken record, but why isn't the full GSO support
> > for virtio-net upstream in qemu?
> >    
> 
> IIRC the current hacks are not upstream quality.
> 
> The problem (again IIRC) is that the guest and host negotiate a 
> protocol, but the qemu vlan model doesn't have a guest and a host, it 
> has peers (possibly more than two), so a lot of translation has to take 
> place if you have one peer supporting a guest feature and another not.

Right.

> IMO the best way out is to drop the vlan model.  It has its uses, but 
> they can all be implemented in other ways, and are all have minor usage 
> compared to the business of getting data into and out of a guest.

I think we should keep the vlan stuff, just de-emphasise it.

I'm planning on adding -hostnet and -nic arguments, which would not use
vlans by default but rather connect the nic directly to the host side.

The QemuOpts conversion of -net which is waiting to be merged is the
first stage of that.

Cheers,
Mark.

Re: [PATCH 1/1] qemu-kvm: virtio-net: Re-instate GSO code removed upstream

[Gerd Hoffmann]

On 09/30/09 15:59, Mark McLoughlin wrote:
> I'm planning on adding -hostnet and -nic arguments, which would not use
> vlans by default but rather connect the nic directly to the host side.

No new -nic argument please.  We should just finalize the qdev-ifycation 
of the nic drivers, then you'll do either

   -device e1000,vlan=<nr>

or

   -device e1000,hostnet=<name>

and be done with it.

cheers,
   Gerd

Re: [Qemu-devel] Re: [PATCH 1/1] qemu-kvm: virtio-net: Re-instate GSO code removed upstream

[Mark McLoughlin]

On Wed, 2009-09-30 at 21:15 +0200, Gerd Hoffmann wrote:
> On 09/30/09 15:59, Mark McLoughlin wrote:
> > I'm planning on adding -hostnet and -nic arguments, which would not use
> > vlans by default but rather connect the nic directly to the host side.
> 
> No new -nic argument please.  We should just finalize the qdev-ifycation 
> of the nic drivers, then you'll do either
> 
>    -device e1000,vlan=<nr>
> 
> or
> 
>    -device e1000,hostnet=<name>
> 
> and be done with it.

Yeah, that makes sense.

Cheers,
Mark.

Re: [PATCH 1/1] qemu-kvm: virtio-net: Re-instate GSO code removed upstream

[Avi Kivity]

On 09/30/2009 03:59 PM, Mark McLoughlin wrote:
> I think we should keep the vlan stuff, just de-emphasise it.
>    

Maybe we should do what X.org does, break it silently and remove it some 
time later when no one complains.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH 1/1] qemu-kvm: virtio-net: Re-instate GSO code removed upstream

[Mark McLoughlin]

On Thu, 2009-10-01 at 18:49 +0200, Avi Kivity wrote:
> On 09/30/2009 03:59 PM, Mark McLoughlin wrote:
> > I think we should keep the vlan stuff, just de-emphasise it.
> >    
> 
> Maybe we should do what X.org does, break it silently and remove it some 
> time later when no one complains.

Well, the 'silently' part isn't going to work now, is it?

Anyway, I'm sure at least Jan would notice if his 'dump' backend stopped
working

Cheers,
Mark.

Re: [PATCH 1/1] qemu-kvm: virtio-net: Re-instate GSO code removed upstream

[Avi Kivity]

On 10/01/2009 07:00 PM, Mark McLoughlin wrote:
> On Thu, 2009-10-01 at 18:49 +0200, Avi Kivity wrote:
>    
>> On 09/30/2009 03:59 PM, Mark McLoughlin wrote:
>>      
>>> I think we should keep the vlan stuff, just de-emphasise it.
>>>
>>>        
>> Maybe we should do what X.org does, break it silently and remove it some
>> time later when no one complains.
>>      
> Well, the 'silently' part isn't going to work now, is it?
>    

Me and my big mouth.

> Anyway, I'm sure at least Jan would notice if his 'dump' backend stopped
> working
>    

Patch it to use libpcap?

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.