Re: [Qemu-devel] segfault use VRingMemoryRegionCaches for avail and used ring vs num-queues

[Cornelia Huck]

On Mon, 27 Feb 2017 16:41:09 +0100
Paolo Bonzini <address@hidden> wrote:

> On 27/02/2017 16:37, Cornelia Huck wrote:
> > With the following applied (probably whitespace damaged), my guest
> > starts:
> > 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index e487e36..28906e5 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -287,6 +287,9 @@ static inline void vring_set_avail_event(VirtQueue *vq, 
> > uint16_t val)
> >  void virtio_queue_set_notification(VirtQueue *vq, int enable)
> >  {
> >      vq->notification = enable;
> > +    if (!vq->vring.desc) {
> > +        return;
> > +    }
> >  
> >      rcu_read_lock();
> >      if (virtio_vdev_has_feature(vq->vdev, VIRTIO_RING_F_EVENT_IDX)) {
> > 
> > Maybe introduction of caches just exposed bugs that were already there
> > (trying to muck with vring state for queues that have not been setup?)
> 
> Yes, it did.  I had caught a few while writing the patches, but it does
> feel like whack-a-mole...
> 
> Paolo
> 
> > Should we stick some asserts into the respective functions to help
> > flush out the remaining bugs?

I've been staring at the code some more and I'm not really sure how to
fix this properly.

The dataplane code tries to switch handlers for all virtqueues,
regardless whether they are configured or not. My hack above leaves the
notification in a bit of an ambiguous state, as it cannot
enable/disable notifications on the real queues.

This is ok for this particular case, where we hand over from the bios
(which only enables the first queue) to the Linux kernel (which uses
multiple queues) - but with a virtio reset before additional queues are
configured. I don't think the spec prohibits configuring extra queues
(if present) on the fly, however...