* [Qemu-devel] [PATCH] Fix for crash after migration in virtio-rng on bi-endian targets @ 2014-11-27 5:48 David Gibson 2014-11-27 9:08 ` Amit Shah 2014-11-27 9:26 ` Markus Armbruster 0 siblings, 2 replies; 11+ messages in thread From: David Gibson @ 2014-11-27 5:48 UTC (permalink / raw) To: qemu-devel; +Cc: quintela, rusty, agraf, mst, pbonzini, David Gibson VirtIO devices now remember which endianness they're operating in in order to support targets which may have guests of either endianness, such as powerpc. This endianness state is transferred in a subsection of the virtio device's information. With virtio-rng this can lead to an abort after a loadvm hitting the assert() in virtio_is_big_endian(). This can be reproduced by doing a migrate and load from file on a bi-endian target with a virtio-rng device. The actual guest state isn't particularly important to triggering this. The cause is that virtio_rng_load_device() calls virtio_rng_process() which accesses the ring and thus needs the endianness. However, virtio_rng_process() is called via virtio_load() before it loads the subsections. Essentially the ->load callback in VirtioDeviceClass should only be used for actually reading the device state from the stream, not for post-load re-initialization. This patch fixes the bug by moving the virtio_rng_process() after the call to virtio_load(). Better yet would be to convert virtio to use vmsd and have the virtio_rng_process() as a post_load callback, but that's a bigger project for another day. This is bugfix, and should be considered for the 2.2 branch. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/virtio/virtio-rng.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c index e85a979..473c044 100644 --- a/hw/virtio/virtio-rng.c +++ b/hw/virtio/virtio-rng.c @@ -113,20 +113,22 @@ static void virtio_rng_save(QEMUFile *f, void *opaque) static int virtio_rng_load(QEMUFile *f, void *opaque, int version_id) { + VirtIORNG *vrng = opaque; + int ret; + if (version_id != 1) { return -EINVAL; } - return virtio_load(VIRTIO_DEVICE(opaque), f, version_id); -} + ret = virtio_load(VIRTIO_DEVICE(vrng), f, version_id); + if (ret != 0) { + return ret; + } -static int virtio_rng_load_device(VirtIODevice *vdev, QEMUFile *f, - int version_id) -{ /* We may have an element ready but couldn't process it due to a quota * limit. Make sure to try again after live migration when the quota may * have been reset. */ - virtio_rng_process(VIRTIO_RNG(vdev)); + virtio_rng_process(vrng); return 0; } @@ -231,7 +233,6 @@ static void virtio_rng_class_init(ObjectClass *klass, void *data) vdc->realize = virtio_rng_device_realize; vdc->unrealize = virtio_rng_device_unrealize; vdc->get_features = get_features; - vdc->load = virtio_rng_load_device; } static void virtio_rng_initfn(Object *obj) -- 1.9.3 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix for crash after migration in virtio-rng on bi-endian targets 2014-11-27 5:48 [Qemu-devel] [PATCH] Fix for crash after migration in virtio-rng on bi-endian targets David Gibson @ 2014-11-27 9:08 ` Amit Shah 2014-11-27 11:10 ` Amit Shah ` (2 more replies) 2014-11-27 9:26 ` Markus Armbruster 1 sibling, 3 replies; 11+ messages in thread From: Amit Shah @ 2014-11-27 9:08 UTC (permalink / raw) To: David Gibson; +Cc: mst, rusty, agraf, qemu-devel, quintela, pbonzini On (Thu) 27 Nov 2014 [16:48:10], David Gibson wrote: > VirtIO devices now remember which endianness they're operating in in order > to support targets which may have guests of either endianness, such as > powerpc. This endianness state is transferred in a subsection of the > virtio device's information. > > With virtio-rng this can lead to an abort after a loadvm hitting the > assert() in virtio_is_big_endian(). This can be reproduced by doing a > migrate and load from file on a bi-endian target with a virtio-rng device. > The actual guest state isn't particularly important to triggering this. > > The cause is that virtio_rng_load_device() calls virtio_rng_process() which > accesses the ring and thus needs the endianness. However, > virtio_rng_process() is called via virtio_load() before it loads the > subsections. Essentially the ->load callback in VirtioDeviceClass should > only be used for actually reading the device state from the stream, not for > post-load re-initialization. Agreed. > This patch fixes the bug by moving the virtio_rng_process() after the call > to virtio_load(). Better yet would be to convert virtio to use vmsd and > have the virtio_rng_process() as a post_load callback, but that's a bigger > project for another day. > > This is bugfix, and should be considered for the 2.2 branch. This is undoing most of 3902d49e13c2428bd6381cfdf183103ca4477c1f , added Greg to CC list. Did you try this on x86 guests, or with multiple rng devices? (keeping context for Greg) > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/virtio/virtio-rng.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c > index e85a979..473c044 100644 > --- a/hw/virtio/virtio-rng.c > +++ b/hw/virtio/virtio-rng.c > @@ -113,20 +113,22 @@ static void virtio_rng_save(QEMUFile *f, void *opaque) > > static int virtio_rng_load(QEMUFile *f, void *opaque, int version_id) > { > + VirtIORNG *vrng = opaque; > + int ret; > + > if (version_id != 1) { > return -EINVAL; > } > - return virtio_load(VIRTIO_DEVICE(opaque), f, version_id); > -} > + ret = virtio_load(VIRTIO_DEVICE(vrng), f, version_id); > + if (ret != 0) { > + return ret; > + } > > -static int virtio_rng_load_device(VirtIODevice *vdev, QEMUFile *f, > - int version_id) > -{ > /* We may have an element ready but couldn't process it due to a quota > * limit. Make sure to try again after live migration when the quota may > * have been reset. > */ > - virtio_rng_process(VIRTIO_RNG(vdev)); > + virtio_rng_process(vrng); > > return 0; > } > @@ -231,7 +233,6 @@ static void virtio_rng_class_init(ObjectClass *klass, void *data) > vdc->realize = virtio_rng_device_realize; > vdc->unrealize = virtio_rng_device_unrealize; > vdc->get_features = get_features; > - vdc->load = virtio_rng_load_device; > } > > static void virtio_rng_initfn(Object *obj) Thanks, Amit ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix for crash after migration in virtio-rng on bi-endian targets 2014-11-27 9:08 ` Amit Shah @ 2014-11-27 11:10 ` Amit Shah 2014-11-27 14:15 ` Greg Kurz 2014-11-28 0:50 ` David Gibson 2 siblings, 0 replies; 11+ messages in thread From: Amit Shah @ 2014-11-27 11:10 UTC (permalink / raw) To: David Gibson; +Cc: mst, rusty, agraf, qemu-devel, quintela, pbonzini (actually adding Greg) On (Thu) 27 Nov 2014 [14:38:42], Amit Shah wrote: > On (Thu) 27 Nov 2014 [16:48:10], David Gibson wrote: > > VirtIO devices now remember which endianness they're operating in in order > > to support targets which may have guests of either endianness, such as > > powerpc. This endianness state is transferred in a subsection of the > > virtio device's information. > > > > With virtio-rng this can lead to an abort after a loadvm hitting the > > assert() in virtio_is_big_endian(). This can be reproduced by doing a > > migrate and load from file on a bi-endian target with a virtio-rng device. > > The actual guest state isn't particularly important to triggering this. > > > > The cause is that virtio_rng_load_device() calls virtio_rng_process() which > > accesses the ring and thus needs the endianness. However, > > virtio_rng_process() is called via virtio_load() before it loads the > > subsections. Essentially the ->load callback in VirtioDeviceClass should > > only be used for actually reading the device state from the stream, not for > > post-load re-initialization. > > Agreed. > > > This patch fixes the bug by moving the virtio_rng_process() after the call > > to virtio_load(). Better yet would be to convert virtio to use vmsd and > > have the virtio_rng_process() as a post_load callback, but that's a bigger > > project for another day. > > > > This is bugfix, and should be considered for the 2.2 branch. > > This is undoing most of 3902d49e13c2428bd6381cfdf183103ca4477c1f , > added Greg to CC list. > > Did you try this on x86 guests, or with multiple rng devices? > > (keeping context for Greg) > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > hw/virtio/virtio-rng.c | 15 ++++++++------- > > 1 file changed, 8 insertions(+), 7 deletions(-) > > > > diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c > > index e85a979..473c044 100644 > > --- a/hw/virtio/virtio-rng.c > > +++ b/hw/virtio/virtio-rng.c > > @@ -113,20 +113,22 @@ static void virtio_rng_save(QEMUFile *f, void *opaque) > > > > static int virtio_rng_load(QEMUFile *f, void *opaque, int version_id) > > { > > + VirtIORNG *vrng = opaque; > > + int ret; > > + > > if (version_id != 1) { > > return -EINVAL; > > } > > - return virtio_load(VIRTIO_DEVICE(opaque), f, version_id); > > -} > > + ret = virtio_load(VIRTIO_DEVICE(vrng), f, version_id); > > + if (ret != 0) { > > + return ret; > > + } > > > > -static int virtio_rng_load_device(VirtIODevice *vdev, QEMUFile *f, > > - int version_id) > > -{ > > /* We may have an element ready but couldn't process it due to a quota > > * limit. Make sure to try again after live migration when the quota may > > * have been reset. > > */ > > - virtio_rng_process(VIRTIO_RNG(vdev)); > > + virtio_rng_process(vrng); > > > > return 0; > > } > > @@ -231,7 +233,6 @@ static void virtio_rng_class_init(ObjectClass *klass, void *data) > > vdc->realize = virtio_rng_device_realize; > > vdc->unrealize = virtio_rng_device_unrealize; > > vdc->get_features = get_features; > > - vdc->load = virtio_rng_load_device; > > } > > > > static void virtio_rng_initfn(Object *obj) > > > Thanks, > > Amit Amit ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix for crash after migration in virtio-rng on bi-endian targets 2014-11-27 9:08 ` Amit Shah 2014-11-27 11:10 ` Amit Shah @ 2014-11-27 14:15 ` Greg Kurz 2014-11-28 0:50 ` David Gibson 2 siblings, 0 replies; 11+ messages in thread From: Greg Kurz @ 2014-11-27 14:15 UTC (permalink / raw) To: Amit Shah; +Cc: quintela, rusty, qemu-devel, agraf, mst, pbonzini, David Gibson On Thu, 27 Nov 2014 14:38:42 +0530 Amit Shah <amit.shah@redhat.com> wrote: > On (Thu) 27 Nov 2014 [16:48:10], David Gibson wrote: > > VirtIO devices now remember which endianness they're operating in in order > > to support targets which may have guests of either endianness, such as > > powerpc. This endianness state is transferred in a subsection of the > > virtio device's information. > > > > With virtio-rng this can lead to an abort after a loadvm hitting the > > assert() in virtio_is_big_endian(). This can be reproduced by doing a > > migrate and load from file on a bi-endian target with a virtio-rng device. > > The actual guest state isn't particularly important to triggering this. > > > > The cause is that virtio_rng_load_device() calls virtio_rng_process() which > > accesses the ring and thus needs the endianness. However, > > virtio_rng_process() is called via virtio_load() before it loads the > > subsections. Essentially the ->load callback in VirtioDeviceClass should > > only be used for actually reading the device state from the stream, not for > > post-load re-initialization. > > Agreed. > > > This patch fixes the bug by moving the virtio_rng_process() after the call > > to virtio_load(). Better yet would be to convert virtio to use vmsd and > > have the virtio_rng_process() as a post_load callback, but that's a bigger > > project for another day. > > I remember discussions on IRC last spring where I agreed I would work on it. :) > > This is bugfix, and should be considered for the 2.2 branch. > > This is undoing most of 3902d49e13c2428bd6381cfdf183103ca4477c1f , > added Greg to CC list. > This commit is indeed completely wrong: the load callback only makes sense when there's something to read which is obviously not the case here... This is definitely post load stuff :-\ Thanks ! :) Reviewed-by: Greg Kurz <gkurz@linux.vnet.ibm.com> -- Greg > Did you try this on x86 guests, or with multiple rng devices? > > (keeping context for Greg) > > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > > --- > > hw/virtio/virtio-rng.c | 15 ++++++++------- > > 1 file changed, 8 insertions(+), 7 deletions(-) > > > > diff --git a/hw/virtio/virtio-rng.c b/hw/virtio/virtio-rng.c > > index e85a979..473c044 100644 > > --- a/hw/virtio/virtio-rng.c > > +++ b/hw/virtio/virtio-rng.c > > @@ -113,20 +113,22 @@ static void virtio_rng_save(QEMUFile *f, void *opaque) > > > > static int virtio_rng_load(QEMUFile *f, void *opaque, int version_id) > > { > > + VirtIORNG *vrng = opaque; > > + int ret; > > + > > if (version_id != 1) { > > return -EINVAL; > > } > > - return virtio_load(VIRTIO_DEVICE(opaque), f, version_id); > > -} > > + ret = virtio_load(VIRTIO_DEVICE(vrng), f, version_id); > > + if (ret != 0) { > > + return ret; > > + } > > > > -static int virtio_rng_load_device(VirtIODevice *vdev, QEMUFile *f, > > - int version_id) > > -{ > > /* We may have an element ready but couldn't process it due to a quota > > * limit. Make sure to try again after live migration when the quota may > > * have been reset. > > */ > > - virtio_rng_process(VIRTIO_RNG(vdev)); > > + virtio_rng_process(vrng); > > > > return 0; > > } > > @@ -231,7 +233,6 @@ static void virtio_rng_class_init(ObjectClass *klass, void *data) > > vdc->realize = virtio_rng_device_realize; > > vdc->unrealize = virtio_rng_device_unrealize; > > vdc->get_features = get_features; > > - vdc->load = virtio_rng_load_device; > > } > > > > static void virtio_rng_initfn(Object *obj) > > > Thanks, > > Amit > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix for crash after migration in virtio-rng on bi-endian targets 2014-11-27 9:08 ` Amit Shah 2014-11-27 11:10 ` Amit Shah 2014-11-27 14:15 ` Greg Kurz @ 2014-11-28 0:50 ` David Gibson 2014-11-28 4:14 ` Amit Shah 2 siblings, 1 reply; 11+ messages in thread From: David Gibson @ 2014-11-28 0:50 UTC (permalink / raw) To: Amit Shah; +Cc: mst, rusty, agraf, qemu-devel, quintela, pbonzini [-- Attachment #1: Type: text/plain, Size: 1876 bytes --] On Thu, Nov 27, 2014 at 02:38:42PM +0530, Amit Shah wrote: > On (Thu) 27 Nov 2014 [16:48:10], David Gibson wrote: > > VirtIO devices now remember which endianness they're operating in in order > > to support targets which may have guests of either endianness, such as > > powerpc. This endianness state is transferred in a subsection of the > > virtio device's information. > > > > With virtio-rng this can lead to an abort after a loadvm hitting the > > assert() in virtio_is_big_endian(). This can be reproduced by doing a > > migrate and load from file on a bi-endian target with a virtio-rng device. > > The actual guest state isn't particularly important to triggering this. > > > > The cause is that virtio_rng_load_device() calls virtio_rng_process() which > > accesses the ring and thus needs the endianness. However, > > virtio_rng_process() is called via virtio_load() before it loads the > > subsections. Essentially the ->load callback in VirtioDeviceClass should > > only be used for actually reading the device state from the stream, not for > > post-load re-initialization. > > Agreed. > > > This patch fixes the bug by moving the virtio_rng_process() after the call > > to virtio_load(). Better yet would be to convert virtio to use vmsd and > > have the virtio_rng_process() as a post_load callback, but that's a bigger > > project for another day. > > > > This is bugfix, and should be considered for the 2.2 branch. > > This is undoing most of 3902d49e13c2428bd6381cfdf183103ca4477c1f , > added Greg to CC list. > > Did you try this on x86 guests, or with multiple rng devices? Not so far, I'll see what I can do. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix for crash after migration in virtio-rng on bi-endian targets 2014-11-28 0:50 ` David Gibson @ 2014-11-28 4:14 ` Amit Shah 0 siblings, 0 replies; 11+ messages in thread From: Amit Shah @ 2014-11-28 4:14 UTC (permalink / raw) To: David Gibson; +Cc: mst, rusty, agraf, qemu-devel, quintela, pbonzini On (Fri) 28 Nov 2014 [11:50:51], David Gibson wrote: > On Thu, Nov 27, 2014 at 02:38:42PM +0530, Amit Shah wrote: > > On (Thu) 27 Nov 2014 [16:48:10], David Gibson wrote: > > > VirtIO devices now remember which endianness they're operating in in order > > > to support targets which may have guests of either endianness, such as > > > powerpc. This endianness state is transferred in a subsection of the > > > virtio device's information. > > > > > > With virtio-rng this can lead to an abort after a loadvm hitting the > > > assert() in virtio_is_big_endian(). This can be reproduced by doing a > > > migrate and load from file on a bi-endian target with a virtio-rng device. > > > The actual guest state isn't particularly important to triggering this. > > > > > > The cause is that virtio_rng_load_device() calls virtio_rng_process() which > > > accesses the ring and thus needs the endianness. However, > > > virtio_rng_process() is called via virtio_load() before it loads the > > > subsections. Essentially the ->load callback in VirtioDeviceClass should > > > only be used for actually reading the device state from the stream, not for > > > post-load re-initialization. > > > > Agreed. > > > > > This patch fixes the bug by moving the virtio_rng_process() after the call > > > to virtio_load(). Better yet would be to convert virtio to use vmsd and > > > have the virtio_rng_process() as a post_load callback, but that's a bigger > > > project for another day. > > > > > > This is bugfix, and should be considered for the 2.2 branch. > > > > This is undoing most of 3902d49e13c2428bd6381cfdf183103ca4477c1f , > > added Greg to CC list. > > > > Did you try this on x86 guests, or with multiple rng devices? > > Not so far, I'll see what I can do. Thanks. Amit ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix for crash after migration in virtio-rng on bi-endian targets 2014-11-27 5:48 [Qemu-devel] [PATCH] Fix for crash after migration in virtio-rng on bi-endian targets David Gibson 2014-11-27 9:08 ` Amit Shah @ 2014-11-27 9:26 ` Markus Armbruster 2014-11-28 9:14 ` Peter Maydell 1 sibling, 1 reply; 11+ messages in thread From: Markus Armbruster @ 2014-11-27 9:26 UTC (permalink / raw) To: David Gibson; +Cc: mst, rusty, agraf, qemu-devel, quintela, pbonzini David Gibson <david@gibson.dropbear.id.au> writes: > VirtIO devices now remember which endianness they're operating in in order > to support targets which may have guests of either endianness, such as > powerpc. This endianness state is transferred in a subsection of the > virtio device's information. > > With virtio-rng this can lead to an abort after a loadvm hitting the > assert() in virtio_is_big_endian(). This can be reproduced by doing a > migrate and load from file on a bi-endian target with a virtio-rng device. > The actual guest state isn't particularly important to triggering this. > > The cause is that virtio_rng_load_device() calls virtio_rng_process() which > accesses the ring and thus needs the endianness. However, > virtio_rng_process() is called via virtio_load() before it loads the > subsections. Essentially the ->load callback in VirtioDeviceClass should > only be used for actually reading the device state from the stream, not for > post-load re-initialization. > > This patch fixes the bug by moving the virtio_rng_process() after the call > to virtio_load(). Better yet would be to convert virtio to use vmsd and > have the virtio_rng_process() as a post_load callback, but that's a bigger > project for another day. > > This is bugfix, and should be considered for the 2.2 branch. "[PATCH for-2.2]" would have been a good idea then. Next time :) ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix for crash after migration in virtio-rng on bi-endian targets 2014-11-27 9:26 ` Markus Armbruster @ 2014-11-28 9:14 ` Peter Maydell 2014-11-28 11:30 ` David Gibson 2014-11-28 11:47 ` Greg Kurz 0 siblings, 2 replies; 11+ messages in thread From: Peter Maydell @ 2014-11-28 9:14 UTC (permalink / raw) To: Markus Armbruster Cc: Juan Quintela, Rusty Russell, QEMU Developers, Alexander Graf, Michael S. Tsirkin, Paolo Bonzini, David Gibson On 27 November 2014 at 09:26, Markus Armbruster <armbru@redhat.com> wrote: > David Gibson <david@gibson.dropbear.id.au> writes: > >> VirtIO devices now remember which endianness they're operating in in order >> to support targets which may have guests of either endianness, such as >> powerpc. This endianness state is transferred in a subsection of the >> virtio device's information. >> >> With virtio-rng this can lead to an abort after a loadvm hitting the >> assert() in virtio_is_big_endian(). This can be reproduced by doing a >> migrate and load from file on a bi-endian target with a virtio-rng device. >> The actual guest state isn't particularly important to triggering this. >> >> The cause is that virtio_rng_load_device() calls virtio_rng_process() which >> accesses the ring and thus needs the endianness. However, >> virtio_rng_process() is called via virtio_load() before it loads the >> subsections. Essentially the ->load callback in VirtioDeviceClass should >> only be used for actually reading the device state from the stream, not for >> post-load re-initialization. >> >> This patch fixes the bug by moving the virtio_rng_process() after the call >> to virtio_load(). Better yet would be to convert virtio to use vmsd and >> have the virtio_rng_process() as a post_load callback, but that's a bigger >> project for another day. >> >> This is bugfix, and should be considered for the 2.2 branch. > > "[PATCH for-2.2]" would have been a good idea then. Next time :) So do you want this patch in 2.2? I was planning to put in the virtio-vs-xen fixes today and tag rc4, so it's not too late if you're confident this patch is good. Let me know if you think it should go in, and I can apply it to master directly. -- PMM ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix for crash after migration in virtio-rng on bi-endian targets 2014-11-28 9:14 ` Peter Maydell @ 2014-11-28 11:30 ` David Gibson 2014-11-28 11:47 ` Greg Kurz 1 sibling, 0 replies; 11+ messages in thread From: David Gibson @ 2014-11-28 11:30 UTC (permalink / raw) To: Peter Maydell Cc: Juan Quintela, Markus Armbruster, Rusty Russell, QEMU Developers, Alexander Graf, Michael S. Tsirkin, Paolo Bonzini [-- Attachment #1: Type: text/plain, Size: 2135 bytes --] On Fri, Nov 28, 2014 at 09:14:46AM +0000, Peter Maydell wrote: > On 27 November 2014 at 09:26, Markus Armbruster <armbru@redhat.com> wrote: > > David Gibson <david@gibson.dropbear.id.au> writes: > > > >> VirtIO devices now remember which endianness they're operating in in order > >> to support targets which may have guests of either endianness, such as > >> powerpc. This endianness state is transferred in a subsection of the > >> virtio device's information. > >> > >> With virtio-rng this can lead to an abort after a loadvm hitting the > >> assert() in virtio_is_big_endian(). This can be reproduced by doing a > >> migrate and load from file on a bi-endian target with a virtio-rng device. > >> The actual guest state isn't particularly important to triggering this. > >> > >> The cause is that virtio_rng_load_device() calls virtio_rng_process() which > >> accesses the ring and thus needs the endianness. However, > >> virtio_rng_process() is called via virtio_load() before it loads the > >> subsections. Essentially the ->load callback in VirtioDeviceClass should > >> only be used for actually reading the device state from the stream, not for > >> post-load re-initialization. > >> > >> This patch fixes the bug by moving the virtio_rng_process() after the call > >> to virtio_load(). Better yet would be to convert virtio to use vmsd and > >> have the virtio_rng_process() as a post_load callback, but that's a bigger > >> project for another day. > >> > >> This is bugfix, and should be considered for the 2.2 branch. > > > > "[PATCH for-2.2]" would have been a good idea then. Next time :) > > So do you want this patch in 2.2? I was planning to put in the > virtio-vs-xen fixes today and tag rc4, so it's not too late if you're > confident this patch is good. Let me know if you think it should go in, > and I can apply it to master directly. Yes, I think it should be applied. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson [-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix for crash after migration in virtio-rng on bi-endian targets 2014-11-28 9:14 ` Peter Maydell 2014-11-28 11:30 ` David Gibson @ 2014-11-28 11:47 ` Greg Kurz 2014-11-28 14:59 ` Peter Maydell 1 sibling, 1 reply; 11+ messages in thread From: Greg Kurz @ 2014-11-28 11:47 UTC (permalink / raw) To: Peter Maydell Cc: Michael S. Tsirkin, Markus Armbruster, Rusty Russell, QEMU Developers, Alexander Graf, Juan Quintela, Paolo Bonzini, David Gibson On Fri, 28 Nov 2014 09:14:46 +0000 Peter Maydell <peter.maydell@linaro.org> wrote: > On 27 November 2014 at 09:26, Markus Armbruster <armbru@redhat.com> wrote: > > David Gibson <david@gibson.dropbear.id.au> writes: > > > >> VirtIO devices now remember which endianness they're operating in in order > >> to support targets which may have guests of either endianness, such as > >> powerpc. This endianness state is transferred in a subsection of the > >> virtio device's information. > >> > >> With virtio-rng this can lead to an abort after a loadvm hitting the > >> assert() in virtio_is_big_endian(). This can be reproduced by doing a > >> migrate and load from file on a bi-endian target with a virtio-rng device. > >> The actual guest state isn't particularly important to triggering this. > >> > >> The cause is that virtio_rng_load_device() calls virtio_rng_process() which > >> accesses the ring and thus needs the endianness. However, > >> virtio_rng_process() is called via virtio_load() before it loads the > >> subsections. Essentially the ->load callback in VirtioDeviceClass should > >> only be used for actually reading the device state from the stream, not for > >> post-load re-initialization. > >> > >> This patch fixes the bug by moving the virtio_rng_process() after the call > >> to virtio_load(). Better yet would be to convert virtio to use vmsd and > >> have the virtio_rng_process() as a post_load callback, but that's a bigger > >> project for another day. > >> > >> This is bugfix, and should be considered for the 2.2 branch. > > > > "[PATCH for-2.2]" would have been a good idea then. Next time :) > > So do you want this patch in 2.2? I was planning to put in the > virtio-vs-xen fixes today and tag rc4, so it's not too late if you're > confident this patch is good. Let me know if you think it should go in, > and I can apply it to master directly. > > -- PMM > Peter, FWIW I think it should. Commit 3902d49e13c2428bd6381cfdf183103ca4477c1f is clearly bad: virtio-rng does not need the .load callback obviously... and the fact it breaks migration makes it even worse... :( Please apply to 2.2. Cheers. -- Greg ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH] Fix for crash after migration in virtio-rng on bi-endian targets 2014-11-28 11:47 ` Greg Kurz @ 2014-11-28 14:59 ` Peter Maydell 0 siblings, 0 replies; 11+ messages in thread From: Peter Maydell @ 2014-11-28 14:59 UTC (permalink / raw) To: Greg Kurz Cc: Michael S. Tsirkin, Markus Armbruster, Rusty Russell, QEMU Developers, Alexander Graf, Juan Quintela, Paolo Bonzini, David Gibson On 28 November 2014 at 11:47, Greg Kurz <gkurz@linux.vnet.ibm.com> wrote: > FWIW I think it should. Commit 3902d49e13c2428bd6381cfdf183103ca4477c1f is > clearly bad: virtio-rng does not need the .load callback obviously... and > the fact it breaks migration makes it even worse... :( > > Please apply to 2.2. Applied to master, thanks. -- PMM ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2014-11-28 14:59 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-11-27 5:48 [Qemu-devel] [PATCH] Fix for crash after migration in virtio-rng on bi-endian targets David Gibson 2014-11-27 9:08 ` Amit Shah 2014-11-27 11:10 ` Amit Shah 2014-11-27 14:15 ` Greg Kurz 2014-11-28 0:50 ` David Gibson 2014-11-28 4:14 ` Amit Shah 2014-11-27 9:26 ` Markus Armbruster 2014-11-28 9:14 ` Peter Maydell 2014-11-28 11:30 ` David Gibson 2014-11-28 11:47 ` Greg Kurz 2014-11-28 14:59 ` Peter Maydell
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).