Re: [Qemu-devel] [PATCH] clean up callback when del virtqueue

[Jason Wang]

On 2018年09月14日 21:14, liujunjie (A) wrote:
> > -----Original Message-----
> > From: Jason Wang [mailto:address@hidden
> > Sent: Friday, September 14, 2018 8:45 PM
> > To: liujunjie (A) <address@hidden>; address@hidden
> > Cc: Huangweidong (C) <address@hidden>; wangxin (U)
> > <address@hidden>; address@hidden; Gonglei (Arei)
> > <address@hidden>; Zhoujian (jay) <address@hidden>
> > Subject: Re: [Qemu-devel] [PATCH] clean up callback when del virtqueue
> >
> >
> >
> > On 2018年09月08日 21:04, liujunjie wrote:
> > > Before, we did not clear callback like handle_output when delete the
> > > virtqueue which may result be segmentfault.
> > > The scene is as follows:
> > > 1. Start a vm with multiqueue vhost-net, 2. then we write
> > > VIRTIO_PCI_GUEST_FEATURES in PCI configuration to triger multiqueue
> > > disable in this vm which will delete the virtqueue.
> > > In this step, the tx_bh is deleted but the callback
> > > virtio_net_handle_tx_bh still exist.
> > > 3. Finally, we write VIRTIO_PCI_QUEUE_NOTIFY in PCI configuration to
> > > notify the deleted virtqueue. In this way, virtio_net_handle_tx_bh
> > > will be called and qemu will be crashed.
> > Good catch.
> >
> > > Although the way described above is uncommon, we had better reinforce it.
> > >
> > > Signed-off-by: liujunjie <address@hidden>
> > > ---
> > >    hw/net/virtio-net.c | 4 +++-
> > >    hw/virtio/virtio.c  | 3 +++
> > >    2 files changed, 6 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index
> > > f154756..9bb20e3 100644
> > > --- a/hw/net/virtio-net.c
> > > +++ b/hw/net/virtio-net.c
> > > @@ -1467,7 +1467,9 @@ static void virtio_net_handle_tx_bh(VirtIODevice
> > *vdev, VirtQueue *vq)
> > >            return;
> > >        }
> > >        virtio_queue_set_notification(vq, 0);
> > > -    qemu_bh_schedule(q->tx_bh);
> > > +    if (q->tx_bh) {
> > > +        qemu_bh_schedule(q->tx_bh);
> > > +    }
> > I believe this is not necessary since handle_output is NULL now?
> Yes, it is not necessary.
>
> > >    }
> > >
> > >    static void virtio_net_tx_timer(void *opaque) diff --git
> > > a/hw/virtio/virtio.c b/hw/virtio/virtio.c index d4e4d98..7577518
> > > 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -1604,6 +1604,9 @@ void virtio_del_queue(VirtIODevice *vdev, int n)
> > >
> > >        vdev->vq[n].vring.num = 0;
> > >        vdev->vq[n].vring.num_default = 0;
> > > +    vdev->vq[n].vring.align = 0;
> > Is this related or fix for another bug?
> It's not related for another bug. So it is also unnecessary to add this, too.
> The reason why add it is only trying to correspond to the function 
> <virtio_add_queue> in the same file.

Ok, please send V2 and cc qemu stable.

Thanks

> > Thanks
> >
> > > +    vdev->vq[n].handle_output = NULL;
> > > +    vdev->vq[n].handle_aio_output = NULL;
> > >    }
> > >
> > >    static void virtio_set_isr(VirtIODevice *vdev, int value)