From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:42219) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWArd-00071e-0M for qemu-devel@nongnu.org; Sat, 27 Apr 2013 15:32:55 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UWArZ-00051K-IO for qemu-devel@nongnu.org; Sat, 27 Apr 2013 15:32:52 -0400 Received: from mx1.redhat.com ([209.132.183.28]:3458) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UWArZ-00050s-B4 for qemu-devel@nongnu.org; Sat, 27 Apr 2013 15:32:49 -0400 Date: Sat, 27 Apr 2013 22:32:40 +0300 From: "Michael S. Tsirkin" Message-ID: <20130427193239.GB30188@redhat.com> References: <1366972060-21606-1-git-send-email-jasowang@redhat.com> <20130426122628.GA15119@redhat.com> <517B5DF4.5000508@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <517B5DF4.5000508@redhat.com> Subject: Re: [Qemu-devel] [PATCH] virtio-net: properly check the vhost status during status set List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Jason Wang Cc: aliguori@us.ibm.com, qemu-devel@nongnu.org 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 > >> Signed-off-by: Jason Wang > > 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