[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] segfault use VRingMemoryRegionCaches for avail and used
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] segfault use VRingMemoryRegionCaches for avail and used ring vs num-queues |
Date: |
Tue, 28 Feb 2017 14:46:10 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 |
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.
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...
>
Re: [Qemu-devel] segfault use VRingMemoryRegionCaches for avail and used ring vs num-queues, Paolo Bonzini, 2017/02/27