| Message ID | 20170227163708.796ab27f.cornelia.huck@de.ibm.com |
|---|---|
| State | New |
| Headers | show |
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?
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. 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...
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... >
On Tue, 28 Feb 2017 14:46:10 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > 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. And virtio_queue_set_rings() for virtio-1. This sounds like a plan; I'll play with it a bit.
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)) {