On Tue, 28 Feb 2017 14:46:10 +0100 Paolo Bonzini <[email protected]> wrote:
> 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. And virtio_queue_set_rings() for virtio-1. This sounds like a plan; I'll play with it a bit.
