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 <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.

> >
> >
> >>> 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