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

[Paolo Bonzini <pbonzini@redhat.com>]

On 28/02/2017 13:48, Cornelia Huck wrote:
> On Mon, 27 Feb 2017 16:41:09 +0100
> Paolo Bonzini <pbonzini@redhat.com> 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.

Paolo

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