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

[Jason Wang <jasowang@redhat.com>]

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