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

[Stefan Hajnoczi]

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.

signature.asc
Description: PGP signature