| Message ID | 20170301165656.80456-1-cornelia.huck@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 01, 2017 at 05:56:56PM +0100, Cornelia Huck wrote: > Switching to vring caches exposed an existing bug in > virtio_queue_set_notification(): We can't access vring structures > if they have not been set up yet. This may happen, for example, > for virtio-blk devices with multiple queues: The code will try to > switch notifiers for every queue, but the guest may have only set up > a subset of them. > > Fix this by (1) guarding access to the vring memory by checking > for vring.desc and (2) triggering an update to the vring flags > for consistency with the configured notification state once the > queue is actually configured AND the device is in a state that > the rings may be actually accessed (i.e. DRIVER_OK has been set > or a legacy device kicks for the first time). I am still puzzled about 2. drivers pre-zero rings as they are set up so notification is on by default. Why do you want to poke at it one extra time? You might get an extra notification if you tried to disable too early but is this really so bad? > Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> > --- > v1->v2: > - Don't touch queues before DRIVER_OK or first kick > - Migration stuff > > Only very lightly tested. > > --- > hw/virtio/virtio.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++--- > 1 file changed, 71 insertions(+), 3 deletions(-) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index e487e36..b467290 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -99,6 +99,9 @@ struct VirtQueue > /* Notification enabled? */ > bool notification; > > + /* Delayed setup of flags for notifications */ > + bool delayed_notification_setup; > + > uint16_t queue_index; > > unsigned int inuse; > @@ -284,10 +287,15 @@ static inline void vring_set_avail_event(VirtQueue *vq, uint16_t val) > virtio_stw_phys_cached(vq->vdev, &caches->used, pa, val); > } > > -void virtio_queue_set_notification(VirtQueue *vq, int enable) > +static void vring_set_notification(VirtQueue *vq, int enable) > { > - vq->notification = enable; > - > + if (!vq->vring.desc || > + (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_VERSION_1) && > + !(vq->vdev->status & VIRTIO_CONFIG_S_DRIVER_OK))) { > + vq->delayed_notification_setup = true; > + return; > + } > + vq->delayed_notification_setup = false; > rcu_read_lock(); > if (virtio_vdev_has_feature(vq->vdev, VIRTIO_RING_F_EVENT_IDX)) { > vring_set_avail_event(vq, vring_avail_idx(vq)); > @@ -303,6 +311,13 @@ void virtio_queue_set_notification(VirtQueue *vq, int enable) > rcu_read_unlock(); > } > > +void virtio_queue_set_notification(VirtQueue *vq, int enable) > +{ > + vq->notification = enable; > + > + vring_set_notification(vq, enable); > +} > + > int virtio_queue_ready(VirtQueue *vq) > { > return vq->vring.avail != 0; > @@ -1087,6 +1102,17 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val) > if (k->set_status) { > k->set_status(vdev, val); > } > + if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) && > + val & VIRTIO_CONFIG_S_FEATURES_OK) { > + int i; > + > + for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { > + if (!virtio_queue_get_num(vdev, i)) { > + break; > + } > + vring_set_notification(&vdev->vq[i], vdev->vq[i].notification); > + } > + } > vdev->status = val; > return 0; > } > @@ -1152,6 +1178,7 @@ void virtio_reset(void *opaque) > vdev->vq[i].notification = true; > vdev->vq[i].vring.num = vdev->vq[i].vring.num_default; > vdev->vq[i].inuse = 0; > + vdev->vq[i].delayed_notification_setup = false; > } > } > > @@ -1671,6 +1698,19 @@ static bool virtio_broken_needed(void *opaque) > return vdev->broken; > } > > +static bool virtio_delayed_notification_needed(void *opaque) > +{ > + VirtIODevice *vdev = opaque; > + int i; > + > + for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { > + if (vdev->vq[i].delayed_notification_setup) { > + return true; > + } > + } > + return false; > +} > + > static const VMStateDescription vmstate_virtqueue = { > .name = "virtqueue_state", > .version_id = 1, > @@ -1799,6 +1839,29 @@ static const VMStateDescription vmstate_virtio_broken = { > } > }; > > +static const VMStateDescription vmstate_delayed_notification = { > + .name = "delayed_notification_state", > + .version_id = 1, > + .minimum_version_id = 1, > + .fields = (VMStateField[]) { > + VMSTATE_BOOL(delayed_notification_setup, struct VirtQueue), > + VMSTATE_END_OF_LIST() > + } > +}; > + > +static const VMStateDescription vmstate_virtio_delayed_notification = { > + .name = "virtio/delayed_notification", > + .version_id = 1, > + .minimum_version_id = 1, > + .needed = &virtio_delayed_notification_needed, > + .fields = (VMStateField[]) { > + VMSTATE_STRUCT_VARRAY_POINTER_KNOWN(vq, struct VirtIODevice, > + VIRTIO_QUEUE_MAX, 0, vmstate_delayed_notification, > + VirtQueue), > + VMSTATE_END_OF_LIST() > + } > +}; > + > static const VMStateDescription vmstate_virtio = { > .name = "virtio", > .version_id = 1, > @@ -1814,6 +1877,7 @@ static const VMStateDescription vmstate_virtio = { > &vmstate_virtio_ringsize, > &vmstate_virtio_broken, > &vmstate_virtio_extra_state, > + &vmstate_virtio_delayed_notification, > NULL > } > }; > @@ -2273,6 +2337,10 @@ EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq) > static void virtio_queue_host_notifier_aio_read(EventNotifier *n) > { > VirtQueue *vq = container_of(n, VirtQueue, host_notifier); > + > + if (unlikely(vq->delayed_notification_setup)) { > + vring_set_notification(vq, vq->notification); > + } > if (event_notifier_test_and_clear(n)) { > virtio_queue_notify_aio_vq(vq); > } > -- > 2.8.4
On Wed, 1 Mar 2017 19:07:14 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > On Wed, Mar 01, 2017 at 05:56:56PM +0100, Cornelia Huck wrote: > > Switching to vring caches exposed an existing bug in > > virtio_queue_set_notification(): We can't access vring structures > > if they have not been set up yet. This may happen, for example, > > for virtio-blk devices with multiple queues: The code will try to > > switch notifiers for every queue, but the guest may have only set up > > a subset of them. > > > > Fix this by (1) guarding access to the vring memory by checking > > for vring.desc and (2) triggering an update to the vring flags > > for consistency with the configured notification state once the > > queue is actually configured AND the device is in a state that > > the rings may be actually accessed (i.e. DRIVER_OK has been set > > or a legacy device kicks for the first time). > > I am still puzzled about 2. drivers pre-zero rings as they > are set up so notification is on by default. Why do you want to poke > at it one extra time? You might get an extra notification > if you tried to disable too early but is this really so bad? Just checking for !desc and then leaving the queues alone might be the right thing. See also my reply in the other thread.
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index e487e36..b467290 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -99,6 +99,9 @@ struct VirtQueue /* Notification enabled? */ bool notification; + /* Delayed setup of flags for notifications */ + bool delayed_notification_setup; + uint16_t queue_index; unsigned int inuse; @@ -284,10 +287,15 @@ static inline void vring_set_avail_event(VirtQueue *vq, uint16_t val) virtio_stw_phys_cached(vq->vdev, &caches->used, pa, val); } -void virtio_queue_set_notification(VirtQueue *vq, int enable) +static void vring_set_notification(VirtQueue *vq, int enable) { - vq->notification = enable; - + if (!vq->vring.desc || + (virtio_vdev_has_feature(vq->vdev, VIRTIO_F_VERSION_1) && + !(vq->vdev->status & VIRTIO_CONFIG_S_DRIVER_OK))) { + vq->delayed_notification_setup = true; + return; + } + vq->delayed_notification_setup = false; rcu_read_lock(); if (virtio_vdev_has_feature(vq->vdev, VIRTIO_RING_F_EVENT_IDX)) { vring_set_avail_event(vq, vring_avail_idx(vq)); @@ -303,6 +311,13 @@ void virtio_queue_set_notification(VirtQueue *vq, int enable) rcu_read_unlock(); } +void virtio_queue_set_notification(VirtQueue *vq, int enable) +{ + vq->notification = enable; + + vring_set_notification(vq, enable); +} + int virtio_queue_ready(VirtQueue *vq) { return vq->vring.avail != 0; @@ -1087,6 +1102,17 @@ int virtio_set_status(VirtIODevice *vdev, uint8_t val) if (k->set_status) { k->set_status(vdev, val); } + if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) && + val & VIRTIO_CONFIG_S_FEATURES_OK) { + int i; + + for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { + if (!virtio_queue_get_num(vdev, i)) { + break; + } + vring_set_notification(&vdev->vq[i], vdev->vq[i].notification); + } + } vdev->status = val; return 0; } @@ -1152,6 +1178,7 @@ void virtio_reset(void *opaque) vdev->vq[i].notification = true; vdev->vq[i].vring.num = vdev->vq[i].vring.num_default; vdev->vq[i].inuse = 0; + vdev->vq[i].delayed_notification_setup = false; } } @@ -1671,6 +1698,19 @@ static bool virtio_broken_needed(void *opaque) return vdev->broken; } +static bool virtio_delayed_notification_needed(void *opaque) +{ + VirtIODevice *vdev = opaque; + int i; + + for (i = 0; i < VIRTIO_QUEUE_MAX; i++) { + if (vdev->vq[i].delayed_notification_setup) { + return true; + } + } + return false; +} + static const VMStateDescription vmstate_virtqueue = { .name = "virtqueue_state", .version_id = 1, @@ -1799,6 +1839,29 @@ static const VMStateDescription vmstate_virtio_broken = { } }; +static const VMStateDescription vmstate_delayed_notification = { + .name = "delayed_notification_state", + .version_id = 1, + .minimum_version_id = 1, + .fields = (VMStateField[]) { + VMSTATE_BOOL(delayed_notification_setup, struct VirtQueue), + VMSTATE_END_OF_LIST() + } +}; + +static const VMStateDescription vmstate_virtio_delayed_notification = { + .name = "virtio/delayed_notification", + .version_id = 1, + .minimum_version_id = 1, + .needed = &virtio_delayed_notification_needed, + .fields = (VMStateField[]) { + VMSTATE_STRUCT_VARRAY_POINTER_KNOWN(vq, struct VirtIODevice, + VIRTIO_QUEUE_MAX, 0, vmstate_delayed_notification, + VirtQueue), + VMSTATE_END_OF_LIST() + } +}; + static const VMStateDescription vmstate_virtio = { .name = "virtio", .version_id = 1, @@ -1814,6 +1877,7 @@ static const VMStateDescription vmstate_virtio = { &vmstate_virtio_ringsize, &vmstate_virtio_broken, &vmstate_virtio_extra_state, + &vmstate_virtio_delayed_notification, NULL } }; @@ -2273,6 +2337,10 @@ EventNotifier *virtio_queue_get_guest_notifier(VirtQueue *vq) static void virtio_queue_host_notifier_aio_read(EventNotifier *n) { VirtQueue *vq = container_of(n, VirtQueue, host_notifier); + + if (unlikely(vq->delayed_notification_setup)) { + vring_set_notification(vq, vq->notification); + } if (event_notifier_test_and_clear(n)) { virtio_queue_notify_aio_vq(vq); }
Switching to vring caches exposed an existing bug in virtio_queue_set_notification(): We can't access vring structures if they have not been set up yet. This may happen, for example, for virtio-blk devices with multiple queues: The code will try to switch notifiers for every queue, but the guest may have only set up a subset of them. Fix this by (1) guarding access to the vring memory by checking for vring.desc and (2) triggering an update to the vring flags for consistency with the configured notification state once the queue is actually configured AND the device is in a state that the rings may be actually accessed (i.e. DRIVER_OK has been set or a legacy device kicks for the first time). Signed-off-by: Cornelia Huck <cornelia.huck@de.ibm.com> --- v1->v2: - Don't touch queues before DRIVER_OK or first kick - Migration stuff Only very lightly tested. --- hw/virtio/virtio.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 71 insertions(+), 3 deletions(-)