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

[Stefan Hajnoczi]

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.

signature.asc
Description: PGP signature