[Qemu-devel] [PATCH for-2.12] monitor: bind dispatch bh to iohandler context

Peter Xu posted 1 patch 5 years, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20180403050115.6037-1-peterx@redhat.com
Test checkpatch passed
Test docker-build@min-glib passed
Test docker-mingw@fedora passed
Test s390x passed
There is a newer version of this series
monitor.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[Qemu-devel] [PATCH for-2.12] monitor: bind dispatch bh to iohandler context
Posted by Peter Xu 5 years, 11 months ago
Eric Auger reported the problem days ago that OOB broke ARM when running
with libvirt:

http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html

This patch fixes the problem.

It's not really needed now since we have turned OOB off now, but it's
still a bug fix, and it'll start to work when we turn OOB on for ARM.

The problem was that the monitor dispatcher bottom half was bound to
qemu_aio_context, but that context seems to be for block only.  For the
rest of the QEMU world we should be using iohandler context.  So
assigning monitor dispatcher bottom half to that context.

If without this change, QMP dispatcher might be run even before reaching
main loop in block IO path, for example, in a stack like:

        #0  qmp_cont ()
        #1  0x00000000006bd210 in qmp_marshal_cont ()
        #2  0x0000000000ac05c4 in do_qmp_dispatch ()
        #3  0x0000000000ac07a0 in qmp_dispatch ()
        #4  0x0000000000472d60 in monitor_qmp_dispatch_one ()
        #5  0x000000000047302c in monitor_qmp_bh_dispatcher ()
        #6  0x0000000000acf374 in aio_bh_call ()
        #7  0x0000000000acf428 in aio_bh_poll ()
        #8  0x0000000000ad5110 in aio_poll ()
        #9  0x0000000000a08ab8 in blk_prw ()
        #10 0x0000000000a091c4 in blk_pread ()
        #11 0x0000000000734f94 in pflash_cfi01_realize ()
        #12 0x000000000075a3a4 in device_set_realized ()
        #13 0x00000000009a26cc in property_set_bool ()
        #14 0x00000000009a0a40 in object_property_set ()
        #15 0x00000000009a3a08 in object_property_set_qobject ()
        #16 0x00000000009a0c8c in object_property_set_bool ()
        #17 0x0000000000758f94 in qdev_init_nofail ()
        #18 0x000000000058e190 in create_one_flash ()
        #19 0x000000000058e2f4 in create_flash ()
        #20 0x00000000005902f0 in machvirt_init ()
        #21 0x00000000007635cc in machine_run_board_init ()
        #22 0x00000000006b135c in main ()

This can cause ARM to crash when used with both OOB capability enabled
and libvirt as upper layer, since libvirt will start QEMU with "-S" and
the first "cont" command will arrive very early if the context is not
correct (which is what above stack shows).  Then, the vcpu threads will
start to run right after the qmp_cont() call, even when GICs have not
been setup correctly yet (which is done in kvm_arm_machine_init_done()).

My sincere thanks to Eric Auger who offered great help during both
debugging and verifying the problem.  The ARM test was carried out by
applying this patch upon QEMU 2.12.0-rc0 and problem is gone after the
patch.

A quick test of mine shows that after this patch applied we can pass all
raw iotests even with OOB on by default.

CC: Eric Blake <eblake@redhat.com>
CC: Markus Armbruster <armbru@redhat.com>
CC: Stefan Hajnoczi <stefanha@redhat.com>
CC: Fam Zheng <famz@redhat.com>
Reported-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---

This patch will fix all known OOB breakages I know so far, but I think
for better safety I'll still keep OOB off, and I'll send another patch
to turn default OOB on after 2.12 release.
---
 monitor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/monitor.c b/monitor.c
index 51f4cf480f..39f8ee17ba 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4467,7 +4467,7 @@ static void monitor_iothread_init(void)
      * have assumption to be run on main loop thread.  It would be
      * nice that one day we can remove this assumption in the future.
      */
-    mon_global.qmp_dispatcher_bh = aio_bh_new(qemu_get_aio_context(),
+    mon_global.qmp_dispatcher_bh = aio_bh_new(iohandler_get_aio_context(),
                                               monitor_qmp_bh_dispatcher,
                                               NULL);
 
-- 
2.14.3


Re: [Qemu-devel] [PATCH for-2.12] monitor: bind dispatch bh to iohandler context
Posted by Peter Xu 5 years, 11 months ago
On Tue, Apr 03, 2018 at 01:01:15PM +0800, Peter Xu wrote:
> Eric Auger reported the problem days ago that OOB broke ARM when running
> with libvirt:
> 
> http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html
> 
> This patch fixes the problem.
> 
> It's not really needed now since we have turned OOB off now, but it's
> still a bug fix, and it'll start to work when we turn OOB on for ARM.
> 
> The problem was that the monitor dispatcher bottom half was bound to
> qemu_aio_context, but that context seems to be for block only.  For the
> rest of the QEMU world we should be using iohandler context.  So
> assigning monitor dispatcher bottom half to that context.
> 
> If without this change, QMP dispatcher might be run even before reaching
> main loop in block IO path, for example, in a stack like:
> 
>         #0  qmp_cont ()
>         #1  0x00000000006bd210 in qmp_marshal_cont ()
>         #2  0x0000000000ac05c4 in do_qmp_dispatch ()
>         #3  0x0000000000ac07a0 in qmp_dispatch ()
>         #4  0x0000000000472d60 in monitor_qmp_dispatch_one ()
>         #5  0x000000000047302c in monitor_qmp_bh_dispatcher ()
>         #6  0x0000000000acf374 in aio_bh_call ()
>         #7  0x0000000000acf428 in aio_bh_poll ()
>         #8  0x0000000000ad5110 in aio_poll ()
>         #9  0x0000000000a08ab8 in blk_prw ()
>         #10 0x0000000000a091c4 in blk_pread ()
>         #11 0x0000000000734f94 in pflash_cfi01_realize ()
>         #12 0x000000000075a3a4 in device_set_realized ()
>         #13 0x00000000009a26cc in property_set_bool ()
>         #14 0x00000000009a0a40 in object_property_set ()
>         #15 0x00000000009a3a08 in object_property_set_qobject ()
>         #16 0x00000000009a0c8c in object_property_set_bool ()
>         #17 0x0000000000758f94 in qdev_init_nofail ()
>         #18 0x000000000058e190 in create_one_flash ()
>         #19 0x000000000058e2f4 in create_flash ()
>         #20 0x00000000005902f0 in machvirt_init ()
>         #21 0x00000000007635cc in machine_run_board_init ()
>         #22 0x00000000006b135c in main ()
> 
> This can cause ARM to crash when used with both OOB capability enabled
> and libvirt as upper layer, since libvirt will start QEMU with "-S" and
> the first "cont" command will arrive very early if the context is not
> correct (which is what above stack shows).  Then, the vcpu threads will
> start to run right after the qmp_cont() call, even when GICs have not
> been setup correctly yet (which is done in kvm_arm_machine_init_done()).
> 
> My sincere thanks to Eric Auger who offered great help during both
> debugging and verifying the problem.  The ARM test was carried out by
> applying this patch upon QEMU 2.12.0-rc0 and problem is gone after the
> patch.
> 
> A quick test of mine shows that after this patch applied we can pass all
> raw iotests even with OOB on by default.
> 
> CC: Eric Blake <eblake@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Fam Zheng <famz@redhat.com>
> Reported-by: Eric Auger <eric.auger@redhat.com>
> Tested-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

It seems that Reported-by and Tested-by didn't really trigger the
add-cc operation.  Cc Eric Auger too.

-- 
Peter Xu

Re: [Qemu-devel] [PATCH for-2.12] monitor: bind dispatch bh to iohandler context
Posted by Fam Zheng 5 years, 11 months ago
On Tue, 04/03 13:01, Peter Xu wrote:
> Eric Auger reported the problem days ago that OOB broke ARM when running
> with libvirt:
> 
> http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html
> 
> This patch fixes the problem.
> 
> It's not really needed now since we have turned OOB off now, but it's
> still a bug fix, and it'll start to work when we turn OOB on for ARM.
> 
> The problem was that the monitor dispatcher bottom half was bound to
> qemu_aio_context, but that context seems to be for block only.  For the
> rest of the QEMU world we should be using iohandler context.  So
> assigning monitor dispatcher bottom half to that context.
> 
> If without this change, QMP dispatcher might be run even before reaching
> main loop in block IO path, for example, in a stack like:
> 
>         #0  qmp_cont ()
>         #1  0x00000000006bd210 in qmp_marshal_cont ()
>         #2  0x0000000000ac05c4 in do_qmp_dispatch ()
>         #3  0x0000000000ac07a0 in qmp_dispatch ()
>         #4  0x0000000000472d60 in monitor_qmp_dispatch_one ()
>         #5  0x000000000047302c in monitor_qmp_bh_dispatcher ()
>         #6  0x0000000000acf374 in aio_bh_call ()
>         #7  0x0000000000acf428 in aio_bh_poll ()
>         #8  0x0000000000ad5110 in aio_poll ()
>         #9  0x0000000000a08ab8 in blk_prw ()
>         #10 0x0000000000a091c4 in blk_pread ()
>         #11 0x0000000000734f94 in pflash_cfi01_realize ()
>         #12 0x000000000075a3a4 in device_set_realized ()
>         #13 0x00000000009a26cc in property_set_bool ()
>         #14 0x00000000009a0a40 in object_property_set ()
>         #15 0x00000000009a3a08 in object_property_set_qobject ()
>         #16 0x00000000009a0c8c in object_property_set_bool ()
>         #17 0x0000000000758f94 in qdev_init_nofail ()
>         #18 0x000000000058e190 in create_one_flash ()
>         #19 0x000000000058e2f4 in create_flash ()
>         #20 0x00000000005902f0 in machvirt_init ()
>         #21 0x00000000007635cc in machine_run_board_init ()
>         #22 0x00000000006b135c in main ()
> 
> This can cause ARM to crash when used with both OOB capability enabled
> and libvirt as upper layer, since libvirt will start QEMU with "-S" and
> the first "cont" command will arrive very early if the context is not
> correct (which is what above stack shows).  Then, the vcpu threads will
> start to run right after the qmp_cont() call, even when GICs have not
> been setup correctly yet (which is done in kvm_arm_machine_init_done()).
> 
> My sincere thanks to Eric Auger who offered great help during both
> debugging and verifying the problem.  The ARM test was carried out by
> applying this patch upon QEMU 2.12.0-rc0 and problem is gone after the
> patch.
> 
> A quick test of mine shows that after this patch applied we can pass all
> raw iotests even with OOB on by default.
> 
> CC: Eric Blake <eblake@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Fam Zheng <famz@redhat.com>
> Reported-by: Eric Auger <eric.auger@redhat.com>
> Tested-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> 
> This patch will fix all known OOB breakages I know so far, but I think
> for better safety I'll still keep OOB off, and I'll send another patch
> to turn default OOB on after 2.12 release.
> ---
>  monitor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 51f4cf480f..39f8ee17ba 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4467,7 +4467,7 @@ static void monitor_iothread_init(void)
>       * have assumption to be run on main loop thread.  It would be
>       * nice that one day we can remove this assumption in the future.
>       */
> -    mon_global.qmp_dispatcher_bh = aio_bh_new(qemu_get_aio_context(),
> +    mon_global.qmp_dispatcher_bh = aio_bh_new(iohandler_get_aio_context(),
>                                                monitor_qmp_bh_dispatcher,
>                                                NULL);

Makes sense to me.

Reviewed-by: Fam Zheng <famz@redhat.com>


Re: [Qemu-devel] [PATCH for-2.12] monitor: bind dispatch bh to iohandler context
Posted by Eric Blake 5 years, 11 months ago
On 04/03/2018 12:01 AM, Peter Xu wrote:
> Eric Auger reported the problem days ago that OOB broke ARM when running
> with libvirt:
> 
> http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html
> 
> This patch fixes the problem.
> 
> It's not really needed now since we have turned OOB off now, but it's
> still a bug fix, and it'll start to work when we turn OOB on for ARM.

It may be more than just ARM affected, and just that ARM happened to
trigger the problem easier.

I'm queuing this on my qapi tree, and will send a pull request,
hopefully for -rc2, but even if I miss that, this qualifies as -rc3
material.

> 
> The problem was that the monitor dispatcher bottom half was bound to
> qemu_aio_context, but that context seems to be for block only.  For the
> rest of the QEMU world we should be using iohandler context.  So
> assigning monitor dispatcher bottom half to that context.
> 

> 
> A quick test of mine shows that after this patch applied we can pass all
> raw iotests even with OOB on by default.
> 
> CC: Eric Blake <eblake@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Fam Zheng <famz@redhat.com>
> Reported-by: Eric Auger <eric.auger@redhat.com>
> Tested-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> 
> This patch will fix all known OOB breakages I know so far, but I think
> for better safety I'll still keep OOB off, and I'll send another patch
> to turn default OOB on after 2.12 release.

Yes, leaving OOB defaulting to off unless you use x-oob=on for 2.12 is
the most conservative action, although we still want to fix all known
bugs that you can trigger by using that.

How did you run iotests with OOB on by default, by recompiling the
binary to undo commit be933ffc2, or by tweaking the iotests scripts to
pass x-oob=on by default?

> ---
>  monitor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 51f4cf480f..39f8ee17ba 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4467,7 +4467,7 @@ static void monitor_iothread_init(void)
>       * have assumption to be run on main loop thread.  It would be
>       * nice that one day we can remove this assumption in the future.
>       */
> -    mon_global.qmp_dispatcher_bh = aio_bh_new(qemu_get_aio_context(),
> +    mon_global.qmp_dispatcher_bh = aio_bh_new(iohandler_get_aio_context(),
>                                                monitor_qmp_bh_dispatcher,
>                                                NULL);
>  
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH for-2.12] monitor: bind dispatch bh to iohandler context
Posted by Peter Xu 5 years, 11 months ago
On Tue, Apr 03, 2018 at 08:02:44AM -0500, Eric Blake wrote:
> On 04/03/2018 12:01 AM, Peter Xu wrote:
> > Eric Auger reported the problem days ago that OOB broke ARM when running
> > with libvirt:
> > 
> > http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html
> > 
> > This patch fixes the problem.
> > 
> > It's not really needed now since we have turned OOB off now, but it's
> > still a bug fix, and it'll start to work when we turn OOB on for ARM.
> 
> It may be more than just ARM affected, and just that ARM happened to
> trigger the problem easier.

Yes.  Please feel free to add something to the commit message if you
think necessary.

[...]

> > This patch will fix all known OOB breakages I know so far, but I think
> > for better safety I'll still keep OOB off, and I'll send another patch
> > to turn default OOB on after 2.12 release.
> 
> Yes, leaving OOB defaulting to off unless you use x-oob=on for 2.12 is
> the most conservative action, although we still want to fix all known
> bugs that you can trigger by using that.
> 
> How did you run iotests with OOB on by default, by recompiling the
> binary to undo commit be933ffc2, or by tweaking the iotests scripts to
> pass x-oob=on by default?

Not really undo be933ffc2, but pick up 3fd2457d18 manually.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH for-2.12] monitor: bind dispatch bh to iohandler context
Posted by Stefan Hajnoczi 5 years, 11 months ago
On Tue, Apr 03, 2018 at 01:01:15PM +0800, Peter Xu wrote:
> Eric Auger reported the problem days ago that OOB broke ARM when running
> with libvirt:
> 
> http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html
> 
> This patch fixes the problem.
> 
> It's not really needed now since we have turned OOB off now, but it's
> still a bug fix, and it'll start to work when we turn OOB on for ARM.
> 
> The problem was that the monitor dispatcher bottom half was bound to
> qemu_aio_context, but that context seems to be for block only.

No, it is not block-only.  iohandler_ctx is for the legacy
qemu_set_fd_handler() API only and modern code should use
qemu_aio_context.

The difference between qemu_aio_context and iohandler_ctx is that
aio_poll(qemu_aio_context) does not process iohandler_ctx (since it's a
difference context).  That is the legacy behavior that
qemu_set_fd_handler() expects and it's implemented by keeping a separate
iohandler_ctx.

> For the
> rest of the QEMU world we should be using iohandler context.  So
> assigning monitor dispatcher bottom half to that context.

This patch relies on the side-effect that iohandler_ctx is only called
later by the main loop, which seems to prevent the crash below.

What is the actual crash/problem?  You mentioned the GIC, but what does
that have to do with monitor code crashing?

> 
> If without this change, QMP dispatcher might be run even before reaching
> main loop in block IO path, for example, in a stack like:
> 
>         #0  qmp_cont ()
>         #1  0x00000000006bd210 in qmp_marshal_cont ()
>         #2  0x0000000000ac05c4 in do_qmp_dispatch ()
>         #3  0x0000000000ac07a0 in qmp_dispatch ()
>         #4  0x0000000000472d60 in monitor_qmp_dispatch_one ()
>         #5  0x000000000047302c in monitor_qmp_bh_dispatcher ()
>         #6  0x0000000000acf374 in aio_bh_call ()
>         #7  0x0000000000acf428 in aio_bh_poll ()
>         #8  0x0000000000ad5110 in aio_poll ()
>         #9  0x0000000000a08ab8 in blk_prw ()
>         #10 0x0000000000a091c4 in blk_pread ()
>         #11 0x0000000000734f94 in pflash_cfi01_realize ()
>         #12 0x000000000075a3a4 in device_set_realized ()
>         #13 0x00000000009a26cc in property_set_bool ()
>         #14 0x00000000009a0a40 in object_property_set ()
>         #15 0x00000000009a3a08 in object_property_set_qobject ()
>         #16 0x00000000009a0c8c in object_property_set_bool ()
>         #17 0x0000000000758f94 in qdev_init_nofail ()
>         #18 0x000000000058e190 in create_one_flash ()
>         #19 0x000000000058e2f4 in create_flash ()
>         #20 0x00000000005902f0 in machvirt_init ()
>         #21 0x00000000007635cc in machine_run_board_init ()
>         #22 0x00000000006b135c in main ()
> 
> This can cause ARM to crash when used with both OOB capability enabled
> and libvirt as upper layer, since libvirt will start QEMU with "-S" and
> the first "cont" command will arrive very early if the context is not
> correct (which is what above stack shows).  Then, the vcpu threads will
> start to run right after the qmp_cont() call, even when GICs have not
> been setup correctly yet (which is done in kvm_arm_machine_init_done()).
>
> My sincere thanks to Eric Auger who offered great help during both
> debugging and verifying the problem.  The ARM test was carried out by
> applying this patch upon QEMU 2.12.0-rc0 and problem is gone after the
> patch.
> 
> A quick test of mine shows that after this patch applied we can pass all
> raw iotests even with OOB on by default.
> 
> CC: Eric Blake <eblake@redhat.com>
> CC: Markus Armbruster <armbru@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Fam Zheng <famz@redhat.com>
> Reported-by: Eric Auger <eric.auger@redhat.com>
> Tested-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
> 
> This patch will fix all known OOB breakages I know so far, but I think
> for better safety I'll still keep OOB off, and I'll send another patch
> to turn default OOB on after 2.12 release.
> ---
>  monitor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/monitor.c b/monitor.c
> index 51f4cf480f..39f8ee17ba 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4467,7 +4467,7 @@ static void monitor_iothread_init(void)
>       * have assumption to be run on main loop thread.  It would be
>       * nice that one day we can remove this assumption in the future.
>       */
> -    mon_global.qmp_dispatcher_bh = aio_bh_new(qemu_get_aio_context(),
> +    mon_global.qmp_dispatcher_bh = aio_bh_new(iohandler_get_aio_context(),
>                                                monitor_qmp_bh_dispatcher,
>                                                NULL);
>  
> -- 
> 2.14.3
> 
> 
Re: [Qemu-devel] [PATCH for-2.12] monitor: bind dispatch bh to iohandler context
Posted by Eric Blake 5 years, 11 months ago
On 04/04/2018 09:22 AM, Stefan Hajnoczi wrote:
> On Tue, Apr 03, 2018 at 01:01:15PM +0800, Peter Xu wrote:
>> Eric Auger reported the problem days ago that OOB broke ARM when running
>> with libvirt:
>>
>> http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html
>>
>> This patch fixes the problem.
>>
>> It's not really needed now since we have turned OOB off now, but it's
>> still a bug fix, and it'll start to work when we turn OOB on for ARM.
>>
>> The problem was that the monitor dispatcher bottom half was bound to
>> qemu_aio_context, but that context seems to be for block only.
> 
> No, it is not block-only.  iohandler_ctx is for the legacy
> qemu_set_fd_handler() API only and modern code should use
> qemu_aio_context.
> 
> The difference between qemu_aio_context and iohandler_ctx is that
> aio_poll(qemu_aio_context) does not process iohandler_ctx (since it's a
> difference context).  That is the legacy behavior that
> qemu_set_fd_handler() expects and it's implemented by keeping a separate
> iohandler_ctx.

Do I need to put a hold on my pull request while we come to a better
understanding of root cause, or is this patch still okay to include?

> 
>> For the
>> rest of the QEMU world we should be using iohandler context.  So
>> assigning monitor dispatcher bottom half to that context.
> 
> This patch relies on the side-effect that iohandler_ctx is only called
> later by the main loop, which seems to prevent the crash below.
> 
> What is the actual crash/problem?  You mentioned the GIC, but what does
> that have to do with monitor code crashing?
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH for-2.12] monitor: bind dispatch bh to iohandler context
Posted by Stefan Hajnoczi 5 years, 11 months ago
On Wed, Apr 04, 2018 at 10:07:34AM -0500, Eric Blake wrote:
> On 04/04/2018 09:22 AM, Stefan Hajnoczi wrote:
> > On Tue, Apr 03, 2018 at 01:01:15PM +0800, Peter Xu wrote:
> >> Eric Auger reported the problem days ago that OOB broke ARM when running
> >> with libvirt:
> >>
> >> http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html
> >>
> >> This patch fixes the problem.
> >>
> >> It's not really needed now since we have turned OOB off now, but it's
> >> still a bug fix, and it'll start to work when we turn OOB on for ARM.
> >>
> >> The problem was that the monitor dispatcher bottom half was bound to
> >> qemu_aio_context, but that context seems to be for block only.
> > 
> > No, it is not block-only.  iohandler_ctx is for the legacy
> > qemu_set_fd_handler() API only and modern code should use
> > qemu_aio_context.
> > 
> > The difference between qemu_aio_context and iohandler_ctx is that
> > aio_poll(qemu_aio_context) does not process iohandler_ctx (since it's a
> > difference context).  That is the legacy behavior that
> > qemu_set_fd_handler() expects and it's implemented by keeping a separate
> > iohandler_ctx.
> 
> Do I need to put a hold on my pull request while we come to a better
> understanding of root cause, or is this patch still okay to include?

The cover letter says this patch is not critical for 2.12:

"It's not really needed now since we have turned OOB off now, but it's
still a bug fix, and it'll start to work when we turn OOB on for ARM."

Please hold off until the nature of the bug is understood.

Thanks,
Stefan
Re: [Qemu-devel] [PATCH for-2.12] monitor: bind dispatch bh to iohandler context
Posted by Peter Xu 5 years, 11 months ago
On Thu, Apr 05, 2018 at 01:34:31PM +0100, Stefan Hajnoczi wrote:
> On Wed, Apr 04, 2018 at 10:07:34AM -0500, Eric Blake wrote:
> > On 04/04/2018 09:22 AM, Stefan Hajnoczi wrote:
> > > On Tue, Apr 03, 2018 at 01:01:15PM +0800, Peter Xu wrote:
> > >> Eric Auger reported the problem days ago that OOB broke ARM when running
> > >> with libvirt:
> > >>
> > >> http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html
> > >>
> > >> This patch fixes the problem.
> > >>
> > >> It's not really needed now since we have turned OOB off now, but it's
> > >> still a bug fix, and it'll start to work when we turn OOB on for ARM.
> > >>
> > >> The problem was that the monitor dispatcher bottom half was bound to
> > >> qemu_aio_context, but that context seems to be for block only.
> > > 
> > > No, it is not block-only.  iohandler_ctx is for the legacy
> > > qemu_set_fd_handler() API only and modern code should use
> > > qemu_aio_context.
> > > 
> > > The difference between qemu_aio_context and iohandler_ctx is that
> > > aio_poll(qemu_aio_context) does not process iohandler_ctx (since it's a
> > > difference context).  That is the legacy behavior that
> > > qemu_set_fd_handler() expects and it's implemented by keeping a separate
> > > iohandler_ctx.
> > 
> > Do I need to put a hold on my pull request while we come to a better
> > understanding of root cause, or is this patch still okay to include?
> 
> The cover letter says this patch is not critical for 2.12:
> 
> "It's not really needed now since we have turned OOB off now, but it's
> still a bug fix, and it'll start to work when we turn OOB on for ARM."
> 
> Please hold off until the nature of the bug is understood.

But frankly speaking I would still prefer the fix be ready for 2.12
since that's still a bugfix and after all we have x-oob=on parameter
to enable that (that's why I mentioned for-2.12 in subject).  The fix
can be something better than this patch, but still it'll be good 2.12
has that.

Let's first discuss which's the best way to fix this up, then we'll
see whether this can be fixed for 2.12, or later.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH for-2.12] monitor: bind dispatch bh to iohandler context
Posted by Peter Xu 5 years, 11 months ago
On Wed, Apr 04, 2018 at 03:22:20PM +0100, Stefan Hajnoczi wrote:
> On Tue, Apr 03, 2018 at 01:01:15PM +0800, Peter Xu wrote:
> > Eric Auger reported the problem days ago that OOB broke ARM when running
> > with libvirt:
> > 
> > http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html

[1]

> > 
> > This patch fixes the problem.
> > 
> > It's not really needed now since we have turned OOB off now, but it's
> > still a bug fix, and it'll start to work when we turn OOB on for ARM.
> > 
> > The problem was that the monitor dispatcher bottom half was bound to
> > qemu_aio_context, but that context seems to be for block only.
> 
> No, it is not block-only.  iohandler_ctx is for the legacy
> qemu_set_fd_handler() API only and modern code should use
> qemu_aio_context.
> 
> The difference between qemu_aio_context and iohandler_ctx is that
> aio_poll(qemu_aio_context) does not process iohandler_ctx (since it's a
> difference context).  That is the legacy behavior that
> qemu_set_fd_handler() expects and it's implemented by keeping a separate
> iohandler_ctx.
> 
> > For the
> > rest of the QEMU world we should be using iohandler context.  So
> > assigning monitor dispatcher bottom half to that context.
> 
> This patch relies on the side-effect that iohandler_ctx is only called
> later by the main loop, which seems to prevent the crash below.

Actually I thought that's why we need that iohandler_ctx, no?

Any better suggestion would be welcomed...

> 
> What is the actual crash/problem?  You mentioned the GIC, but what does
> that have to do with monitor code crashing?

The actually crash is mentioned above [1].  Please have a look on that
thread for details.

Basic idea is that the QMP command "cont" is run earlier than before,
which breaks the ordering of past, so vcpu threads are running
earlier, even if they are not really fully setup.  Strange things
happen when we want to run un-prepared vcpus.

> 
> > 
> > If without this change, QMP dispatcher might be run even before reaching
> > main loop in block IO path, for example, in a stack like:
> > 
> >         #0  qmp_cont ()
> >         #1  0x00000000006bd210 in qmp_marshal_cont ()
> >         #2  0x0000000000ac05c4 in do_qmp_dispatch ()
> >         #3  0x0000000000ac07a0 in qmp_dispatch ()
> >         #4  0x0000000000472d60 in monitor_qmp_dispatch_one ()
> >         #5  0x000000000047302c in monitor_qmp_bh_dispatcher ()
> >         #6  0x0000000000acf374 in aio_bh_call ()
> >         #7  0x0000000000acf428 in aio_bh_poll ()
> >         #8  0x0000000000ad5110 in aio_poll ()
> >         #9  0x0000000000a08ab8 in blk_prw ()
> >         #10 0x0000000000a091c4 in blk_pread ()
> >         #11 0x0000000000734f94 in pflash_cfi01_realize ()
> >         #12 0x000000000075a3a4 in device_set_realized ()
> >         #13 0x00000000009a26cc in property_set_bool ()
> >         #14 0x00000000009a0a40 in object_property_set ()
> >         #15 0x00000000009a3a08 in object_property_set_qobject ()
> >         #16 0x00000000009a0c8c in object_property_set_bool ()
> >         #17 0x0000000000758f94 in qdev_init_nofail ()
> >         #18 0x000000000058e190 in create_one_flash ()
> >         #19 0x000000000058e2f4 in create_flash ()
> >         #20 0x00000000005902f0 in machvirt_init ()
> >         #21 0x00000000007635cc in machine_run_board_init ()
> >         #22 0x00000000006b135c in main ()
> > 
> > This can cause ARM to crash when used with both OOB capability enabled
> > and libvirt as upper layer, since libvirt will start QEMU with "-S" and
> > the first "cont" command will arrive very early if the context is not
> > correct (which is what above stack shows).  Then, the vcpu threads will
> > start to run right after the qmp_cont() call, even when GICs have not
> > been setup correctly yet (which is done in kvm_arm_machine_init_done()).
> >
> > My sincere thanks to Eric Auger who offered great help during both
> > debugging and verifying the problem.  The ARM test was carried out by
> > applying this patch upon QEMU 2.12.0-rc0 and problem is gone after the
> > patch.
> > 
> > A quick test of mine shows that after this patch applied we can pass all
> > raw iotests even with OOB on by default.
> > 
> > CC: Eric Blake <eblake@redhat.com>
> > CC: Markus Armbruster <armbru@redhat.com>
> > CC: Stefan Hajnoczi <stefanha@redhat.com>
> > CC: Fam Zheng <famz@redhat.com>
> > Reported-by: Eric Auger <eric.auger@redhat.com>
> > Tested-by: Eric Auger <eric.auger@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> > 
> > This patch will fix all known OOB breakages I know so far, but I think
> > for better safety I'll still keep OOB off, and I'll send another patch
> > to turn default OOB on after 2.12 release.
> > ---
> >  monitor.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/monitor.c b/monitor.c
> > index 51f4cf480f..39f8ee17ba 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4467,7 +4467,7 @@ static void monitor_iothread_init(void)
> >       * have assumption to be run on main loop thread.  It would be
> >       * nice that one day we can remove this assumption in the future.
> >       */
> > -    mon_global.qmp_dispatcher_bh = aio_bh_new(qemu_get_aio_context(),
> > +    mon_global.qmp_dispatcher_bh = aio_bh_new(iohandler_get_aio_context(),
> >                                                monitor_qmp_bh_dispatcher,
> >                                                NULL);
> >  
> > -- 
> > 2.14.3
> > 
> > 



-- 
Peter Xu

Re: [Qemu-devel] [PATCH for-2.12] monitor: bind dispatch bh to iohandler context
Posted by Stefan Hajnoczi 5 years, 11 months ago
On Sun, Apr 08, 2018 at 01:12:07PM +0800, Peter Xu wrote:
> On Wed, Apr 04, 2018 at 03:22:20PM +0100, Stefan Hajnoczi wrote:
> > On Tue, Apr 03, 2018 at 01:01:15PM +0800, Peter Xu wrote:
> > > Eric Auger reported the problem days ago that OOB broke ARM when running
> > > with libvirt:
> > > 
> > > http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html
> 
> [1]
> 
> > > 
> > > This patch fixes the problem.
> > > 
> > > It's not really needed now since we have turned OOB off now, but it's
> > > still a bug fix, and it'll start to work when we turn OOB on for ARM.
> > > 
> > > The problem was that the monitor dispatcher bottom half was bound to
> > > qemu_aio_context, but that context seems to be for block only.
> > 
> > No, it is not block-only.  iohandler_ctx is for the legacy
> > qemu_set_fd_handler() API only and modern code should use
> > qemu_aio_context.
> > 
> > The difference between qemu_aio_context and iohandler_ctx is that
> > aio_poll(qemu_aio_context) does not process iohandler_ctx (since it's a
> > difference context).  That is the legacy behavior that
> > qemu_set_fd_handler() expects and it's implemented by keeping a separate
> > iohandler_ctx.
> > 
> > > For the
> > > rest of the QEMU world we should be using iohandler context.  So
> > > assigning monitor dispatcher bottom half to that context.
> > 
> > This patch relies on the side-effect that iohandler_ctx is only called
> > later by the main loop, which seems to prevent the crash below.
> 
> Actually I thought that's why we need that iohandler_ctx, no?
> 
> Any better suggestion would be welcomed...

iohandler_ctx is for the qemu_set_fd_handler() API.  Monitor code
doesn't use that API, it uses the chardev API.

The problem you've described is that monitor chardev handlers are being
invoked too early.  The straightforward fix is for monitor.c to set up
handlers at the right time - when we want the monitor to begin
processing commands.

> > 
> > What is the actual crash/problem?  You mentioned the GIC, but what does
> > that have to do with monitor code crashing?
> 
> The actually crash is mentioned above [1].  Please have a look on that
> thread for details.

The email you linked says there is a bug and git-bisect led to a commit.
It doesn't explain the bug.

If you want to get code merged, please make life easy for reviewers.
Don't link to email threads and expect reviewers to research the bug.

Put the root cause into the commit description so there's enough
information to understand the issue in your commit.
Re: [Qemu-devel] [PATCH for-2.12] monitor: bind dispatch bh to iohandler context
Posted by Stefan Hajnoczi 5 years, 11 months ago
On Tue, Apr 03, 2018 at 01:01:15PM +0800, Peter Xu wrote:

I have changed my mind about this: this patch is necessary.  It is
needed in QEMU 2.12.

> Eric Auger reported the problem days ago that OOB broke ARM when running
> with libvirt:
> 
> http://lists.gnu.org/archive/html/qemu-devel/2018-03/msg06231.html
> 
> This patch fixes the problem.

The general problem is that the monitor should only dispatch commands
from main_loop_wait().  That's how the code worked before OOB.  After
OOB commands are also dispatched from aio_poll().  This causes
unexpected behavior.

Here is another scenario that this patch fixes:

If the monitor IOThread parses a command while a monitor command is in
aio_poll() then qmp_dispatch() is re-entered.  Monitor command code is
not designed to handle this!

> It's not really needed now since we have turned OOB off now, but it's
> still a bug fix, and it'll start to work when we turn OOB on for ARM.

No, it is needed even when OOB is disabled.  Consider the case where the
chardev handler is invoked by main_loop_wait() and command parsing
completes.  qmp_dispatcher_bh will be marked scheduled=1.

Before qmp_dispatcher_bh executes another fd handler runs and invokes
aio_poll().  Now qmp_dispatcher_bh runs from aio_poll() instead of from
main_loop_wait().  Before OOB this was impossible and it's likely to
hang or crash.

> The problem was that the monitor dispatcher bottom half was bound to
> qemu_aio_context, but that context seems to be for block only.

s/but that context seems to be for block only/which is used for nested
event loops/

It's not true that qemu_aio_context is used for block only.  All
qemu_bh_new() users (many emulated devices, for example) use
qemu_aio_context, as well as TPM and 9p.

> For the
> rest of the QEMU world we should be using iohandler context.  So
> assigning monitor dispatcher bottom half to that context.

TPM and 9p also use nested event loops, they need to use
qemu_aio_context.

The choice depends on whether or not nested event loops are needed.
Code that isn't designed for nested event loops has to use iohandler_ctx
(usually via qemu_set_fd_handler()).  Code that is designed for nested
event loops has to use qemu_aio_context.