On 28/02/2017 13:48, Cornelia Huck wrote: > On Mon, 27 Feb 2017 16:41:09 +0100 > Paolo Bonzini <[email protected]> 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... >
