[Qemu-devel] [PATCH] virtio-net: properly check the vhost status during status set

[Qemu-devel] [PATCH] virtio-net: properly check the vhost status during status set

[Jason Wang]

Commit 32993698 (vhost: disable on tap link down) tries to disable the vhost
also when the peer's link is down. But the check was not done properly, the
vhost were only started when:

1) peer's link is not down
2) virtio-net has already been started.

Since == have a higher precedence than &&, place a brace to make sure both the
conditions were met then does the check. This fixes the crash when doing a savem
after set the link off which let qemu crash and complains:

virtio_net_save: Assertion `!n->vhost_started' failed.

Cc: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/virtio-net.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 4d2cdd2..6222039 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -114,8 +114,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
         return;
     }
 
-    if (!!n->vhost_started == virtio_net_started(n, status) &&
-                              !nc->peer->link_down) {
+    if (!!n->vhost_started ==
+        (virtio_net_started(n, status) && !nc->peer->link_down)) {
         return;
     }
     if (!n->vhost_started) {
-- 
1.7.1

Re: [Qemu-devel] [PATCH] virtio-net: properly check the vhost status during status set

[Michael S. Tsirkin]

On Fri, Apr 26, 2013 at 06:27:40PM +0800, Jason Wang wrote:
> Commit 32993698 (vhost: disable on tap link down) tries to disable the vhost
> also when the peer's link is down. But the check was not done properly, the
> vhost were only started when:
> 
> 1) peer's link is not down
> 2) virtio-net has already been started.
> 
> Since == have a higher precedence than &&, place a brace to make sure both the
> conditions were met then does the check. This fixes the crash when doing a savem
> after set the link off which let qemu crash and complains:
> 
> virtio_net_save: Assertion `!n->vhost_started' failed.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Hmm okay, but now that I think about this,
e.g. if link is up later, vhost will not be started.

So the correct thing is maybe to start vhost but use
some backend that will drop all packets.
And add a callback so we know peer state changed.
Hmm do we need a kernel change for this?

> ---
>  hw/net/virtio-net.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 4d2cdd2..6222039 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -114,8 +114,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
>          return;
>      }
>  
> -    if (!!n->vhost_started == virtio_net_started(n, status) &&
> -                              !nc->peer->link_down) {
> +    if (!!n->vhost_started ==
> +        (virtio_net_started(n, status) && !nc->peer->link_down)) {
>          return;
>      }
>      if (!n->vhost_started) {
> -- 
> 1.7.1

Re: [Qemu-devel] [PATCH] virtio-net: properly check the vhost status during status set

[Jason Wang]

On 04/26/2013 08:26 PM, Michael S. Tsirkin wrote:
> On Fri, Apr 26, 2013 at 06:27:40PM +0800, Jason Wang wrote:
>> Commit 32993698 (vhost: disable on tap link down) tries to disable the vhost
>> also when the peer's link is down. But the check was not done properly, the
>> vhost were only started when:
>>
>> 1) peer's link is not down
>> 2) virtio-net has already been started.
>>
>> Since == have a higher precedence than &&, place a brace to make sure both the
>> conditions were met then does the check. This fixes the crash when doing a savem
>> after set the link off which let qemu crash and complains:
>>
>> virtio_net_save: Assertion `!n->vhost_started' failed.
>>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> Hmm okay, but now that I think about this,
> e.g. if link is up later, vhost will not be started.

If vm has been stopeed, and the link is up later, vhost won't be
started. this is expected.
If vm has been started, and the link is up later, since n->vhost_started
is false but both virtio_net_started() and !nc->peer->link_down is true,
so the vhost will be started.

Looks ok?

> So the correct thing is maybe to start vhost but use
> some backend that will drop all packets.
> And add a callback so we know peer state changed.
> Hmm do we need a kernel change for this?
>
>> ---
>>  hw/net/virtio-net.c |    4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>> index 4d2cdd2..6222039 100644
>> --- a/hw/net/virtio-net.c
>> +++ b/hw/net/virtio-net.c
>> @@ -114,8 +114,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
>>          return;
>>      }
>>  
>> -    if (!!n->vhost_started == virtio_net_started(n, status) &&
>> -                              !nc->peer->link_down) {
>> +    if (!!n->vhost_started ==
>> +        (virtio_net_started(n, status) && !nc->peer->link_down)) {
>>          return;
>>      }
>>      if (!n->vhost_started) {
>> -- 
>> 1.7.1

Re: [Qemu-devel] [PATCH] virtio-net: properly check the vhost status during status set

[Michael S. Tsirkin]

On Sat, Apr 27, 2013 at 01:11:16PM +0800, Jason Wang wrote:
> On 04/26/2013 08:26 PM, Michael S. Tsirkin wrote:
> > On Fri, Apr 26, 2013 at 06:27:40PM +0800, Jason Wang wrote:
> >> Commit 32993698 (vhost: disable on tap link down) tries to disable the vhost
> >> also when the peer's link is down. But the check was not done properly, the
> >> vhost were only started when:
> >>
> >> 1) peer's link is not down
> >> 2) virtio-net has already been started.
> >>
> >> Since == have a higher precedence than &&, place a brace to make sure both the
> >> conditions were met then does the check. This fixes the crash when doing a savem
> >> after set the link off which let qemu crash and complains:
> >>
> >> virtio_net_save: Assertion `!n->vhost_started' failed.
> >>
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Signed-off-by: Jason Wang <jasowang@redhat.com>
> > Hmm okay, but now that I think about this,
> > e.g. if link is up later, vhost will not be started.
> 
> If vm has been stopeed, and the link is up later, vhost won't be
> started. this is expected.
> If vm has been started, and the link is up later, since n->vhost_started
> is false but both virtio_net_started() and !nc->peer->link_down is true,
> so the vhost will be started.
> 
> Looks ok?

Let me clarify: virtio link is up but peer link is down.
So guest will send packets. Will they never be
completed?


> > So the correct thing is maybe to start vhost but use
> > some backend that will drop all packets.
> > And add a callback so we know peer state changed.
> > Hmm do we need a kernel change for this?
> >
> >> ---
> >>  hw/net/virtio-net.c |    4 ++--
> >>  1 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >> index 4d2cdd2..6222039 100644
> >> --- a/hw/net/virtio-net.c
> >> +++ b/hw/net/virtio-net.c
> >> @@ -114,8 +114,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
> >>          return;
> >>      }
> >>  
> >> -    if (!!n->vhost_started == virtio_net_started(n, status) &&
> >> -                              !nc->peer->link_down) {
> >> +    if (!!n->vhost_started ==
> >> +        (virtio_net_started(n, status) && !nc->peer->link_down)) {
> >>          return;
> >>      }
> >>      if (!n->vhost_started) {
> >> -- 
> >> 1.7.1

Re: [Qemu-devel] [PATCH] virtio-net: properly check the vhost status during status set

[Jason Wang]

On 04/28/2013 03:32 AM, Michael S. Tsirkin wrote:
> On Sat, Apr 27, 2013 at 01:11:16PM +0800, Jason Wang wrote:
>> On 04/26/2013 08:26 PM, Michael S. Tsirkin wrote:
>>> On Fri, Apr 26, 2013 at 06:27:40PM +0800, Jason Wang wrote:
>>>> Commit 32993698 (vhost: disable on tap link down) tries to disable the vhost
>>>> also when the peer's link is down. But the check was not done properly, the
>>>> vhost were only started when:
>>>>
>>>> 1) peer's link is not down
>>>> 2) virtio-net has already been started.
>>>>
>>>> Since == have a higher precedence than &&, place a brace to make sure both the
>>>> conditions were met then does the check. This fixes the crash when doing a savem
>>>> after set the link off which let qemu crash and complains:
>>>>
>>>> virtio_net_save: Assertion `!n->vhost_started' failed.
>>>>
>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> Hmm okay, but now that I think about this,
>>> e.g. if link is up later, vhost will not be started.
>> If vm has been stopeed, and the link is up later, vhost won't be
>> started. this is expected.
>> If vm has been started, and the link is up later, since n->vhost_started
>> is false but both virtio_net_started() and !nc->peer->link_down is true,
>> so the vhost will be started.
>>
>> Looks ok?
> Let me clarify: virtio link is up but peer link is down.
> So guest will send packets. Will they never be
> completed?

qemu_deliver_packet_iov() will assume the packet were sent when peer
link is down. So we are still ok?
>
>
>>> So the correct thing is maybe to start vhost but use
>>> some backend that will drop all packets.
>>> And add a callback so we know peer state changed.
>>> Hmm do we need a kernel change for this?
>>>
>>>> ---
>>>>  hw/net/virtio-net.c |    4 ++--
>>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>> index 4d2cdd2..6222039 100644
>>>> --- a/hw/net/virtio-net.c
>>>> +++ b/hw/net/virtio-net.c
>>>> @@ -114,8 +114,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
>>>>          return;
>>>>      }
>>>>  
>>>> -    if (!!n->vhost_started == virtio_net_started(n, status) &&
>>>> -                              !nc->peer->link_down) {
>>>> +    if (!!n->vhost_started ==
>>>> +        (virtio_net_started(n, status) && !nc->peer->link_down)) {
>>>>          return;
>>>>      }
>>>>      if (!n->vhost_started) {
>>>> -- 
>>>> 1.7.1

Re: [Qemu-devel] [PATCH] virtio-net: properly check the vhost status during status set

[Michael S. Tsirkin]

On Sun, Apr 28, 2013 at 03:51:32PM +0800, Jason Wang wrote:
> On 04/28/2013 03:32 AM, Michael S. Tsirkin wrote:
> > On Sat, Apr 27, 2013 at 01:11:16PM +0800, Jason Wang wrote:
> >> On 04/26/2013 08:26 PM, Michael S. Tsirkin wrote:
> >>> On Fri, Apr 26, 2013 at 06:27:40PM +0800, Jason Wang wrote:
> >>>> Commit 32993698 (vhost: disable on tap link down) tries to disable the vhost
> >>>> also when the peer's link is down. But the check was not done properly, the
> >>>> vhost were only started when:
> >>>>
> >>>> 1) peer's link is not down
> >>>> 2) virtio-net has already been started.
> >>>>
> >>>> Since == have a higher precedence than &&, place a brace to make sure both the
> >>>> conditions were met then does the check. This fixes the crash when doing a savem
> >>>> after set the link off which let qemu crash and complains:
> >>>>
> >>>> virtio_net_save: Assertion `!n->vhost_started' failed.
> >>>>
> >>>> Cc: Michael S. Tsirkin <mst@redhat.com>
> >>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>> Hmm okay, but now that I think about this,
> >>> e.g. if link is up later, vhost will not be started.
> >> If vm has been stopeed, and the link is up later, vhost won't be
> >> started. this is expected.
> >> If vm has been started, and the link is up later, since n->vhost_started
> >> is false but both virtio_net_started() and !nc->peer->link_down is true,
> >> so the vhost will be started.
> >>
> >> Looks ok?
> > Let me clarify: virtio link is up but peer link is down.
> > So guest will send packets. Will they never be
> > completed?
> 
> qemu_deliver_packet_iov() will assume the packet were sent when peer
> link is down. So we are still ok?

Right so I think userspace will start dropping packets.
I think this is unnecessarily fragile, I think it's best
to make sure vhost=on means userspace does not
process tx/rx rings.

> >
> >
> >>> So the correct thing is maybe to start vhost but use
> >>> some backend that will drop all packets.
> >>> And add a callback so we know peer state changed.
> >>> Hmm do we need a kernel change for this?
> >>>
> >>>> ---
> >>>>  hw/net/virtio-net.c |    4 ++--
> >>>>  1 files changed, 2 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> >>>> index 4d2cdd2..6222039 100644
> >>>> --- a/hw/net/virtio-net.c
> >>>> +++ b/hw/net/virtio-net.c
> >>>> @@ -114,8 +114,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
> >>>>          return;
> >>>>      }
> >>>>  
> >>>> -    if (!!n->vhost_started == virtio_net_started(n, status) &&
> >>>> -                              !nc->peer->link_down) {
> >>>> +    if (!!n->vhost_started ==
> >>>> +        (virtio_net_started(n, status) && !nc->peer->link_down)) {
> >>>>          return;
> >>>>      }
> >>>>      if (!n->vhost_started) {
> >>>> -- 
> >>>> 1.7.1

Re: [Qemu-devel] [PATCH] virtio-net: properly check the vhost status during status set

[Jason Wang]

On 04/28/2013 04:25 PM, Michael S. Tsirkin wrote:
> On Sun, Apr 28, 2013 at 03:51:32PM +0800, Jason Wang wrote:
>> On 04/28/2013 03:32 AM, Michael S. Tsirkin wrote:
>>> On Sat, Apr 27, 2013 at 01:11:16PM +0800, Jason Wang wrote:
>>>> On 04/26/2013 08:26 PM, Michael S. Tsirkin wrote:
>>>>> On Fri, Apr 26, 2013 at 06:27:40PM +0800, Jason Wang wrote:
>>>>>> Commit 32993698 (vhost: disable on tap link down) tries to disable the vhost
>>>>>> also when the peer's link is down. But the check was not done properly, the
>>>>>> vhost were only started when:
>>>>>>
>>>>>> 1) peer's link is not down
>>>>>> 2) virtio-net has already been started.
>>>>>>
>>>>>> Since == have a higher precedence than &&, place a brace to make sure both the
>>>>>> conditions were met then does the check. This fixes the crash when doing a savem
>>>>>> after set the link off which let qemu crash and complains:
>>>>>>
>>>>>> virtio_net_save: Assertion `!n->vhost_started' failed.
>>>>>>
>>>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>> Hmm okay, but now that I think about this,
>>>>> e.g. if link is up later, vhost will not be started.
>>>> If vm has been stopeed, and the link is up later, vhost won't be
>>>> started. this is expected.
>>>> If vm has been started, and the link is up later, since n->vhost_started
>>>> is false but both virtio_net_started() and !nc->peer->link_down is true,
>>>> so the vhost will be started.
>>>>
>>>> Looks ok?
>>> Let me clarify: virtio link is up but peer link is down.
>>> So guest will send packets. Will they never be
>>> completed?
>> qemu_deliver_packet_iov() will assume the packet were sent when peer
>> link is down. So we are still ok?
> Right so I think userspace will start dropping packets.
> I think this is unnecessarily fragile, I think it's best
> to make sure vhost=on means userspace does not
> process tx/rx rings.

But this is also what other nic does such as e1000. Anyway it's harmless
so we can just keep it.
>
>>>
>>>>> So the correct thing is maybe to start vhost but use
>>>>> some backend that will drop all packets.
>>>>> And add a callback so we know peer state changed.
>>>>> Hmm do we need a kernel change for this?
>>>>>
>>>>>> ---
>>>>>>  hw/net/virtio-net.c |    4 ++--
>>>>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>>>> index 4d2cdd2..6222039 100644
>>>>>> --- a/hw/net/virtio-net.c
>>>>>> +++ b/hw/net/virtio-net.c
>>>>>> @@ -114,8 +114,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
>>>>>>          return;
>>>>>>      }
>>>>>>  
>>>>>> -    if (!!n->vhost_started == virtio_net_started(n, status) &&
>>>>>> -                              !nc->peer->link_down) {
>>>>>> +    if (!!n->vhost_started ==
>>>>>> +        (virtio_net_started(n, status) && !nc->peer->link_down)) {
>>>>>>          return;
>>>>>>      }
>>>>>>      if (!n->vhost_started) {
>>>>>> -- 
>>>>>> 1.7.1

Re: [Qemu-devel] [PATCH] virtio-net: properly check the vhost status during status set

[Jason Wang]

On 04/28/2013 04:25 PM, Michael S. Tsirkin wrote:
> On Sun, Apr 28, 2013 at 03:51:32PM +0800, Jason Wang wrote:
>> On 04/28/2013 03:32 AM, Michael S. Tsirkin wrote:
>>> On Sat, Apr 27, 2013 at 01:11:16PM +0800, Jason Wang wrote:
>>>> On 04/26/2013 08:26 PM, Michael S. Tsirkin wrote:
>>>>> On Fri, Apr 26, 2013 at 06:27:40PM +0800, Jason Wang wrote:
>>>>>> Commit 32993698 (vhost: disable on tap link down) tries to disable the vhost
>>>>>> also when the peer's link is down. But the check was not done properly, the
>>>>>> vhost were only started when:
>>>>>>
>>>>>> 1) peer's link is not down
>>>>>> 2) virtio-net has already been started.
>>>>>>
>>>>>> Since == have a higher precedence than &&, place a brace to make sure both the
>>>>>> conditions were met then does the check. This fixes the crash when doing a savem
>>>>>> after set the link off which let qemu crash and complains:
>>>>>>
>>>>>> virtio_net_save: Assertion `!n->vhost_started' failed.
>>>>>>
>>>>>> Cc: Michael S. Tsirkin <mst@redhat.com>
>>>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>>> Hmm okay, but now that I think about this,
>>>>> e.g. if link is up later, vhost will not be started.
>>>> If vm has been stopeed, and the link is up later, vhost won't be
>>>> started. this is expected.
>>>> If vm has been started, and the link is up later, since n->vhost_started
>>>> is false but both virtio_net_started() and !nc->peer->link_down is true,
>>>> so the vhost will be started.
>>>>
>>>> Looks ok?
>>> Let me clarify: virtio link is up but peer link is down.
>>> So guest will send packets. Will they never be
>>> completed?
>> qemu_deliver_packet_iov() will assume the packet were sent when peer
>> link is down. So we are still ok?
> Right so I think userspace will start dropping packets.
> I think this is unnecessarily fragile, I think it's best
> to make sure vhost=on means userspace does not
> process tx/rx rings.

It may make sense, but let's do it in the future. So ack or apply this
patch to fix the bug first?

Thanks
>>>
>>>>> So the correct thing is maybe to start vhost but use
>>>>> some backend that will drop all packets.
>>>>> And add a callback so we know peer state changed.
>>>>> Hmm do we need a kernel change for this?
>>>>>
>>>>>> ---
>>>>>>  hw/net/virtio-net.c |    4 ++--
>>>>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
>>>>>> index 4d2cdd2..6222039 100644
>>>>>> --- a/hw/net/virtio-net.c
>>>>>> +++ b/hw/net/virtio-net.c
>>>>>> @@ -114,8 +114,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
>>>>>>          return;
>>>>>>      }
>>>>>>  
>>>>>> -    if (!!n->vhost_started == virtio_net_started(n, status) &&
>>>>>> -                              !nc->peer->link_down) {
>>>>>> +    if (!!n->vhost_started ==
>>>>>> +        (virtio_net_started(n, status) && !nc->peer->link_down)) {
>>>>>>          return;
>>>>>>      }
>>>>>>      if (!n->vhost_started) {
>>>>>> -- 
>>>>>> 1.7.1

Re: [Qemu-devel] [PATCH] virtio-net: properly check the vhost status during status set

[Michael S. Tsirkin]

On Fri, Apr 26, 2013 at 06:27:40PM +0800, Jason Wang wrote:
> Commit 32993698 (vhost: disable on tap link down) tries to disable the vhost
> also when the peer's link is down. But the check was not done properly, the
> vhost were only started when:
> 
> 1) peer's link is not down
> 2) virtio-net has already been started.
> 
> Since == have a higher precedence than &&, place a brace to make sure both the
> conditions were met then does the check. This fixes the crash when doing a savem
> after set the link off which let qemu crash and complains:
> 
> virtio_net_save: Assertion `!n->vhost_started' failed.
> 
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

Too late for 1.5?

> ---
>  hw/net/virtio-net.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 4d2cdd2..6222039 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -114,8 +114,8 @@ static void virtio_net_vhost_status(VirtIONet *n, uint8_t status)
>          return;
>      }
>  
> -    if (!!n->vhost_started == virtio_net_started(n, status) &&
> -                              !nc->peer->link_down) {
> +    if (!!n->vhost_started ==
> +        (virtio_net_started(n, status) && !nc->peer->link_down)) {
>          return;
>      }
>      if (!n->vhost_started) {
> -- 
> 1.7.1

Re: [Qemu-devel] [PATCH] virtio-net: properly check the vhost status during status set

[Anthony Liguori]

Applied.  Thanks.

Regards,

Anthony Liguori