Message ID | 1330601288-3558-1-git-send-email-owasserm@redhat.com |
---|---|
State | New |
Headers | show |
On (Thu) 01 Mar 2012 [13:28:08], Orit Wasserman wrote: > Otherwise we crash on error. > Instruction to reporduce the crash with migration: > 1) run a guest with -device virtio-blk-pci,drive=drive_name,scsi=on > 2) run destination with > -device virtio-blk-pci,drive=drive_name,scsi=off ... -incoming ... > 3) migrate from 1 to 2. Thanks. Acked-by: Amit Shah <amit.shah@redhat.com> Amit
On Thu, Mar 01, 2012 at 01:28:08PM +0200, Orit Wasserman wrote: > Otherwise we crash on error. > Instruction to reporduce the crash with migration: > 1) run a guest with -device virtio-blk-pci,drive=drive_name,scsi=on > 2) run destination with > -device virtio-blk-pci,drive=drive_name,scsi=off ... -incoming ... > 3) migrate from 1 to 2. > > Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com> > Signed-off-by: Orit Wasserman <owasserm@redhat.com> Acked-by: Michael S. Tsirkin <mst@redhat.com> > --- > hw/virtio-balloon.c | 6 +++++- > hw/virtio-blk.c | 7 ++++++- > hw/virtio-net.c | 6 +++++- > hw/virtio-scsi.c | 7 ++++++- > hw/virtio-serial-bus.c | 6 +++++- > 5 files changed, 27 insertions(+), 5 deletions(-) > > diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c > index ce9d2c9..075ed87 100644 > --- a/hw/virtio-balloon.c > +++ b/hw/virtio-balloon.c > @@ -211,11 +211,15 @@ static void virtio_balloon_save(QEMUFile *f, void *opaque) > static int virtio_balloon_load(QEMUFile *f, void *opaque, int version_id) > { > VirtIOBalloon *s = opaque; > + int ret; > > if (version_id != 1) > return -EINVAL; > > - virtio_load(&s->vdev, f); > + ret = virtio_load(&s->vdev, f); > + if (ret) { > + return ret; > + } > > s->num_pages = qemu_get_be32(f); > s->actual = qemu_get_be32(f); > diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c > index 49990f8..d4bb400 100644 > --- a/hw/virtio-blk.c > +++ b/hw/virtio-blk.c > @@ -537,11 +537,16 @@ static void virtio_blk_save(QEMUFile *f, void *opaque) > static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id) > { > VirtIOBlock *s = opaque; > + int ret; > > if (version_id != 2) > return -EINVAL; > > - virtio_load(&s->vdev, f); > + ret = virtio_load(&s->vdev, f); > + if (ret) { > + return ret; > + } > + > while (qemu_get_sbyte(f)) { > VirtIOBlockReq *req = virtio_blk_alloc_request(s); > qemu_get_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem)); > diff --git a/hw/virtio-net.c b/hw/virtio-net.c > index bc5e3a8..3f190d4 100644 > --- a/hw/virtio-net.c > +++ b/hw/virtio-net.c > @@ -891,11 +891,15 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id) > { > VirtIONet *n = opaque; > int i; > + int ret; > > if (version_id < 2 || version_id > VIRTIO_NET_VM_VERSION) > return -EINVAL; > > - virtio_load(&n->vdev, f); > + ret = virtio_load(&n->vdev, f); > + if (ret) { > + return ret; > + } > > qemu_get_buffer(f, n->mac, ETH_ALEN); > n->tx_waiting = qemu_get_be32(f); > diff --git a/hw/virtio-scsi.c b/hw/virtio-scsi.c > index e607edc..9797847 100644 > --- a/hw/virtio-scsi.c > +++ b/hw/virtio-scsi.c > @@ -558,7 +558,12 @@ static void virtio_scsi_save(QEMUFile *f, void *opaque) > static int virtio_scsi_load(QEMUFile *f, void *opaque, int version_id) > { > VirtIOSCSI *s = opaque; > - virtio_load(&s->vdev, f); > + int ret; > + > + ret = virtio_load(&s->vdev, f); > + if (ret) { > + return ret; > + } > return 0; > } > > diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c > index e22940e..4a33872 100644 > --- a/hw/virtio-serial-bus.c > +++ b/hw/virtio-serial-bus.c > @@ -590,13 +590,17 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id) > VirtIOSerialPort *port; > uint32_t max_nr_ports, nr_active_ports, ports_map; > unsigned int i; > + int ret; > > if (version_id > 3) { > return -EINVAL; > } > > /* The virtio device */ > - virtio_load(&s->vdev, f); > + ret = virtio_load(&s->vdev, f); > + if (ret) { > + return ret; > + } > > if (version_id < 2) { > return 0; > -- > 1.7.6.5
While I investigated and reproduced the problem, the qemu process itself did not crash/abort. I observed that a Linux guest (KVM virtual machine) became unresponsive after a migration because requests to a virtio disk did not complete. If virtio_load() returns via the following section of code ... if (virtio_set_features(vdev, features) < 0) { supported_features = vdev->binding->get_features(vdev->binding_opaque); error_report("Features 0x%x unsupported. Allowed features: 0x%x", features, supported_features); return -1; } ... various data structures are not fully set up. Execution of the above code is - for example - triggered by passing "scsi=on" to the source qemu and "scsi=off" to the destination qemu process. Since the data structures are in an incomplete state, virtio disk requests from the guest cannot be processed. Regards, Uli ----- Original Message ----- > From: "Orit Wasserman" <owasserm@redhat.com> ... > Sent: Thursday, March 1, 2012 12:28:08 PM > Subject: [PATCH v2] We should check the virtio_load return code > > Otherwise we crash on error. > Instruction to reporduce the crash with migration: > 1) run a guest with -device virtio-blk-pci,drive=drive_name,scsi=on > 2) run destination with > -device virtio-blk-pci,drive=drive_name,scsi=off ... -incoming ... > 3) migrate from 1 to 2. > > Signed-off-by: Ulrich Obergfell <uobergfe@redhat.com> > Signed-off-by: Orit Wasserman <owasserm@redhat.com> ...
diff --git a/hw/virtio-balloon.c b/hw/virtio-balloon.c index ce9d2c9..075ed87 100644 --- a/hw/virtio-balloon.c +++ b/hw/virtio-balloon.c @@ -211,11 +211,15 @@ static void virtio_balloon_save(QEMUFile *f, void *opaque) static int virtio_balloon_load(QEMUFile *f, void *opaque, int version_id) { VirtIOBalloon *s = opaque; + int ret; if (version_id != 1) return -EINVAL; - virtio_load(&s->vdev, f); + ret = virtio_load(&s->vdev, f); + if (ret) { + return ret; + } s->num_pages = qemu_get_be32(f); s->actual = qemu_get_be32(f); diff --git a/hw/virtio-blk.c b/hw/virtio-blk.c index 49990f8..d4bb400 100644 --- a/hw/virtio-blk.c +++ b/hw/virtio-blk.c @@ -537,11 +537,16 @@ static void virtio_blk_save(QEMUFile *f, void *opaque) static int virtio_blk_load(QEMUFile *f, void *opaque, int version_id) { VirtIOBlock *s = opaque; + int ret; if (version_id != 2) return -EINVAL; - virtio_load(&s->vdev, f); + ret = virtio_load(&s->vdev, f); + if (ret) { + return ret; + } + while (qemu_get_sbyte(f)) { VirtIOBlockReq *req = virtio_blk_alloc_request(s); qemu_get_buffer(f, (unsigned char*)&req->elem, sizeof(req->elem)); diff --git a/hw/virtio-net.c b/hw/virtio-net.c index bc5e3a8..3f190d4 100644 --- a/hw/virtio-net.c +++ b/hw/virtio-net.c @@ -891,11 +891,15 @@ static int virtio_net_load(QEMUFile *f, void *opaque, int version_id) { VirtIONet *n = opaque; int i; + int ret; if (version_id < 2 || version_id > VIRTIO_NET_VM_VERSION) return -EINVAL; - virtio_load(&n->vdev, f); + ret = virtio_load(&n->vdev, f); + if (ret) { + return ret; + } qemu_get_buffer(f, n->mac, ETH_ALEN); n->tx_waiting = qemu_get_be32(f); diff --git a/hw/virtio-scsi.c b/hw/virtio-scsi.c index e607edc..9797847 100644 --- a/hw/virtio-scsi.c +++ b/hw/virtio-scsi.c @@ -558,7 +558,12 @@ static void virtio_scsi_save(QEMUFile *f, void *opaque) static int virtio_scsi_load(QEMUFile *f, void *opaque, int version_id) { VirtIOSCSI *s = opaque; - virtio_load(&s->vdev, f); + int ret; + + ret = virtio_load(&s->vdev, f); + if (ret) { + return ret; + } return 0; } diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index e22940e..4a33872 100644 --- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -590,13 +590,17 @@ static int virtio_serial_load(QEMUFile *f, void *opaque, int version_id) VirtIOSerialPort *port; uint32_t max_nr_ports, nr_active_ports, ports_map; unsigned int i; + int ret; if (version_id > 3) { return -EINVAL; } /* The virtio device */ - virtio_load(&s->vdev, f); + ret = virtio_load(&s->vdev, f); + if (ret) { + return ret; + } if (version_id < 2) { return 0;