| Message ID | 1459342088-24311-4-git-send-email-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 30 Mar 2016 14:48:02 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > The missing check on dataplane_disabled caused a segmentation > fault in notify_guest_bh, because s->guest_notifier was NULL. I think this patch description could be improved :) "We must not call virtio_blk_data_plane_notify if dataplane is disabled: we would hit a segmentation fault in notify_guest_bh as s->guest_notifier has not been setup and is NULL." ? > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/block/dataplane/virtio-blk.c | 7 +++---- > hw/block/virtio-blk.c | 2 +- > include/hw/virtio/virtio-blk.h | 1 + > 3 files changed, 5 insertions(+), 5 deletions(-) Patch looks good: Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
Hi Paolo: On 03/30/2016 08:48 PM, Paolo Bonzini wrote: > The missing check on dataplane_disabled caused a segmentation > fault in notify_guest_bh, because s->guest_notifier was NULL. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/block/dataplane/virtio-blk.c | 7 +++---- > hw/block/virtio-blk.c | 2 +- > include/hw/virtio/virtio-blk.h | 1 + > 3 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c > index 0d76110..378feb3 100644 > --- a/hw/block/dataplane/virtio-blk.c > +++ b/hw/block/dataplane/virtio-blk.c > @@ -28,7 +28,6 @@ > struct VirtIOBlockDataPlane { > bool starting; > bool stopping; > - bool disabled; > > VirtIOBlkConf *conf; > > @@ -233,7 +232,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) > fail_host_notifier: > k->set_guest_notifiers(qbus->parent, 1, false); > fail_guest_notifiers: > - s->disabled = true; > + vblk->dataplane_disabled = true; > s->starting = false; > vblk->dataplane_started = true; > } > @@ -250,8 +249,8 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s) > } > > /* Better luck next time. */ > - if (s->disabled) { > - s->disabled = false; > + if (vblk->dataplane_disabled) { > + vblk->dataplane_disabled = false; > vblk->dataplane_started = false; > return; > } > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c > index d0b8248..77221c1 100644 > --- a/hw/block/virtio-blk.c > +++ b/hw/block/virtio-blk.c > @@ -53,7 +53,7 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status) > > stb_p(&req->in->status, status); > virtqueue_push(s->vq, &req->elem, req->in_len); > - if (s->dataplane) { > + if (s->dataplane_started && !s->dataplane_disabled) { > virtio_blk_data_plane_notify(s->dataplane); > } else { > virtio_notify(vdev, s->vq); > diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h > index 5cb66cd..073c632 100644 > --- a/include/hw/virtio/virtio-blk.h > +++ b/include/hw/virtio/virtio-blk.h > @@ -53,6 +53,7 @@ typedef struct VirtIOBlock { > unsigned short sector_mask; > bool original_wce; > VMChangeStateEntry *change; > + bool dataplane_disabled; > bool dataplane_started; > int reentrancy_test; There is no "int reentrancy_test;" in the latest qemu master. thx > struct VirtIOBlockDataPlane *dataplane; >
On 31/03/2016 10:32, tu bo wrote: >> >> diff --git a/include/hw/virtio/virtio-blk.h >> b/include/hw/virtio/virtio-blk.h >> index 5cb66cd..073c632 100644 >> --- a/include/hw/virtio/virtio-blk.h >> +++ b/include/hw/virtio/virtio-blk.h >> @@ -53,6 +53,7 @@ typedef struct VirtIOBlock { >> unsigned short sector_mask; >> bool original_wce; >> VMChangeStateEntry *change; >> + bool dataplane_disabled; >> bool dataplane_started; >> int reentrancy_test; > > There is no "int reentrancy_test;" in the latest qemu master. thx Indeed, I will resend a fix version soon. Paolo
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c index 0d76110..378feb3 100644 --- a/hw/block/dataplane/virtio-blk.c +++ b/hw/block/dataplane/virtio-blk.c @@ -28,7 +28,6 @@ struct VirtIOBlockDataPlane { bool starting; bool stopping; - bool disabled; VirtIOBlkConf *conf; @@ -233,7 +232,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) fail_host_notifier: k->set_guest_notifiers(qbus->parent, 1, false); fail_guest_notifiers: - s->disabled = true; + vblk->dataplane_disabled = true; s->starting = false; vblk->dataplane_started = true; } @@ -250,8 +249,8 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s) } /* Better luck next time. */ - if (s->disabled) { - s->disabled = false; + if (vblk->dataplane_disabled) { + vblk->dataplane_disabled = false; vblk->dataplane_started = false; return; } diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c index d0b8248..77221c1 100644 --- a/hw/block/virtio-blk.c +++ b/hw/block/virtio-blk.c @@ -53,7 +53,7 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status) stb_p(&req->in->status, status); virtqueue_push(s->vq, &req->elem, req->in_len); - if (s->dataplane) { + if (s->dataplane_started && !s->dataplane_disabled) { virtio_blk_data_plane_notify(s->dataplane); } else { virtio_notify(vdev, s->vq); diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h index 5cb66cd..073c632 100644 --- a/include/hw/virtio/virtio-blk.h +++ b/include/hw/virtio/virtio-blk.h @@ -53,6 +53,7 @@ typedef struct VirtIOBlock { unsigned short sector_mask; bool original_wce; VMChangeStateEntry *change; + bool dataplane_disabled; bool dataplane_started; int reentrancy_test; struct VirtIOBlockDataPlane *dataplane;
The missing check on dataplane_disabled caused a segmentation fault in notify_guest_bh, because s->guest_notifier was NULL. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/block/dataplane/virtio-blk.c | 7 +++---- hw/block/virtio-blk.c | 2 +- include/hw/virtio/virtio-blk.h | 1 + 3 files changed, 5 insertions(+), 5 deletions(-)