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 <address@hidden>
>>>>>> Signed-off-by: Jason Wang <address@hidden>
>>>>> 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