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

[Cornelia Huck]

On Tue, 28 Feb 2017 14:46:10 +0100
Paolo Bonzini <address@hidden> wrote:

> On 28/02/2017 13:48, Cornelia Huck wrote:
> > 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.
> 
> What if virtio_queue_set_addr called virtio_queue_set_notification(vq,
> vq->notification)?  In fact the RCU-protected part of
> virtio_queue_set_notification could become its own function, something
> like virtio_queue_update_notification or perhaps a better name.
> 
> virtio_queue_set_addr is only called by the virtio transports, not e.g.
> on migration, so it seems to be the right spot.

And virtio_queue_set_rings() for virtio-1. This sounds like a plan;
I'll play with it a bit.