Re: [PATCH 2/2] virtio: Drop out of coroutine context in virtio_load()

[Stefan Hajnoczi]

On Tue, Sep 05, 2023 at 04:50:02PM +0200, Kevin Wolf wrote:
> virtio_load() as a whole should run in coroutine context because it
> reads from the migration stream and we don't want this to block.

Is that "should" a "must" or a "can"?

If it's a "must" then virtio_load() needs assert(qemu_in_coroutine()).

But the previous patch mentioned that loadvm for snapshots calls it
outside coroutine context. So maybe it's a "can"?

> 
> However, it calls virtio_set_features_nocheck() and devices don't
> expect their .set_features callback to run in a coroutine and therefore
> call functions that may not be called in coroutine context. To fix this,
> drop out of coroutine context for calling virtio_set_features_nocheck().
> 
> Without this fix, the following crash was reported:
> 
>   #0  __pthread_kill_implementation (threadid=<optimized out>, 
> signo=signo@entry=6, no_tid=no_tid@entry=0) at pthread_kill.c:44
>   #1  0x00007efc738c05d3 in __pthread_kill_internal (signo=6, 
> threadid=<optimized out>) at pthread_kill.c:78
>   #2  0x00007efc73873d26 in __GI_raise (sig=sig@entry=6) at 
> ../sysdeps/posix/raise.c:26
>   #3  0x00007efc738477f3 in __GI_abort () at abort.c:79
>   #4  0x00007efc7384771b in __assert_fail_base (fmt=0x7efc739dbcb8 "", 
> assertion=assertion@entry=0x560aebfbf5cf "!qemu_in_coroutine()",
>      file=file@entry=0x560aebfcd2d4 "../block/graph-lock.c", 
> line=line@entry=275, function=function@entry=0x560aebfcd34d "void 
> bdrv_graph_rdlock_main_loop(void)") at assert.c:92
>   #5  0x00007efc7386ccc6 in __assert_fail (assertion=0x560aebfbf5cf 
> "!qemu_in_coroutine()", file=0x560aebfcd2d4 "../block/graph-lock.c", line=275,
>      function=0x560aebfcd34d "void bdrv_graph_rdlock_main_loop(void)") at 
> assert.c:101
>   #6  0x0000560aebcd8dd6 in bdrv_register_buf ()
>   #7  0x0000560aeb97ed97 in ram_block_added.llvm ()
>   #8  0x0000560aebb8303f in ram_block_add.llvm ()
>   #9  0x0000560aebb834fa in qemu_ram_alloc_internal.llvm ()
>   #10 0x0000560aebb2ac98 in vfio_region_mmap ()
>   #11 0x0000560aebb3ea0f in vfio_bars_register ()
>   #12 0x0000560aebb3c628 in vfio_realize ()
>   #13 0x0000560aeb90f0c2 in pci_qdev_realize ()
>   #14 0x0000560aebc40305 in device_set_realized ()
>   #15 0x0000560aebc48e07 in property_set_bool.llvm ()
>   #16 0x0000560aebc46582 in object_property_set ()
>   #17 0x0000560aebc4cd58 in object_property_set_qobject ()
>   #18 0x0000560aebc46ba7 in object_property_set_bool ()
>   #19 0x0000560aeb98b3ca in qdev_device_add_from_qdict ()
>   #20 0x0000560aebb1fbaf in virtio_net_set_features ()
>   #21 0x0000560aebb46b51 in virtio_set_features_nocheck ()
>   #22 0x0000560aebb47107 in virtio_load ()
>   #23 0x0000560aeb9ae7ce in vmstate_load_state ()
>   #24 0x0000560aeb9d2ee9 in qemu_loadvm_state_main ()
>   #25 0x0000560aeb9d45e1 in qemu_loadvm_state ()
>   #26 0x0000560aeb9bc32c in process_incoming_migration_co.llvm ()
>   #27 0x0000560aebeace56 in coroutine_trampoline.llvm ()
> 
> Cc: qemu-stable@nongnu.org
> Buglink: https://issues.redhat.com/browse/RHEL-832
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  hw/virtio/virtio.c | 45 ++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 40 insertions(+), 5 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

signature.asc
Description: PGP signature