diff mbox series

[v7,08/13] qapi: Add a 'coroutine' flag for commands

Message ID 20200909151149.490589-9-kwolf@redhat.com
State New
Headers show
Series monitor: Optionally run handlers in coroutines | expand

Commit Message

Kevin Wolf Sept. 9, 2020, 3:11 p.m. UTC
This patch adds a new 'coroutine' flag to QMP command definitions that
tells the QMP dispatcher that the command handler is safe to be run in a
coroutine.

The documentation of the new flag pretends that this flag is already
used as intended, which it isn't yet after this patch. We'll implement
this in another patch in this series.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 docs/devel/qapi-code-gen.txt            | 12 ++++++++++++
 include/qapi/qmp/dispatch.h             |  1 +
 tests/test-qmp-cmds.c                   |  4 ++++
 scripts/qapi/commands.py                | 10 +++++++---
 scripts/qapi/doc.py                     |  2 +-
 scripts/qapi/expr.py                    | 10 ++++++++--
 scripts/qapi/introspect.py              |  2 +-
 scripts/qapi/schema.py                  | 12 ++++++++----
 tests/qapi-schema/test-qapi.py          |  7 ++++---
 tests/qapi-schema/meson.build           |  1 +
 tests/qapi-schema/oob-coroutine.err     |  2 ++
 tests/qapi-schema/oob-coroutine.json    |  2 ++
 tests/qapi-schema/oob-coroutine.out     |  0
 tests/qapi-schema/qapi-schema-test.json |  1 +
 tests/qapi-schema/qapi-schema-test.out  |  2 ++
 15 files changed, 54 insertions(+), 14 deletions(-)
 create mode 100644 tests/qapi-schema/oob-coroutine.err
 create mode 100644 tests/qapi-schema/oob-coroutine.json
 create mode 100644 tests/qapi-schema/oob-coroutine.out

Comments

Markus Armbruster Sept. 14, 2020, 3:15 p.m. UTC | #1
Kevin Wolf <kwolf@redhat.com> writes:

> This patch adds a new 'coroutine' flag to QMP command definitions that

> tells the QMP dispatcher that the command handler is safe to be run in a

> coroutine.

>

> The documentation of the new flag pretends that this flag is already

> used as intended, which it isn't yet after this patch. We'll implement

> this in another patch in this series.

>

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> Reviewed-by: Markus Armbruster <armbru@redhat.com>

> ---

>  docs/devel/qapi-code-gen.txt            | 12 ++++++++++++

>  include/qapi/qmp/dispatch.h             |  1 +

>  tests/test-qmp-cmds.c                   |  4 ++++

>  scripts/qapi/commands.py                | 10 +++++++---

>  scripts/qapi/doc.py                     |  2 +-

>  scripts/qapi/expr.py                    | 10 ++++++++--

>  scripts/qapi/introspect.py              |  2 +-

>  scripts/qapi/schema.py                  | 12 ++++++++----

>  tests/qapi-schema/test-qapi.py          |  7 ++++---

>  tests/qapi-schema/meson.build           |  1 +

>  tests/qapi-schema/oob-coroutine.err     |  2 ++

>  tests/qapi-schema/oob-coroutine.json    |  2 ++

>  tests/qapi-schema/oob-coroutine.out     |  0

>  tests/qapi-schema/qapi-schema-test.json |  1 +

>  tests/qapi-schema/qapi-schema-test.out  |  2 ++

>  15 files changed, 54 insertions(+), 14 deletions(-)

>  create mode 100644 tests/qapi-schema/oob-coroutine.err

>  create mode 100644 tests/qapi-schema/oob-coroutine.json

>  create mode 100644 tests/qapi-schema/oob-coroutine.out

>

> diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt

> index f3e7ced212..36daa9b5f8 100644

> --- a/docs/devel/qapi-code-gen.txt

> +++ b/docs/devel/qapi-code-gen.txt

> @@ -472,6 +472,7 @@ Syntax:

>                  '*gen': false,

>                  '*allow-oob': true,

>                  '*allow-preconfig': true,

> +                '*coroutine': true,

>                  '*if': COND,

>                  '*features': FEATURES }

>  

> @@ -596,6 +597,17 @@ before the machine is built.  It defaults to false.  For example:

>  QMP is available before the machine is built only when QEMU was

>  started with --preconfig.

>  

> +Member 'coroutine' tells the QMP dispatcher whether the command handler

> +is safe to be run in a coroutine.


We need to document what exactly makes a command handler coroutine safe
/ unsafe.  We discussed this at some length in review of PATCH v4 1/4:

    Message-ID: <874kwnvgad.fsf@dusky.pond.sub.org>
    https://lists.nongnu.org/archive/html/qemu-devel/2020-01/msg05015.html

I'd be willing to accept a follow-up patch, if that's more convenient
for you.

>                                     It defaults to false.  If it is true,

> +the command handler is called from coroutine context and may yield while


Is it *always* called from coroutine context, or could it be called
outside coroutine context, too?  I guess the former, thanks to PATCH 10,
and as long as we diligently mark HMP commands that such call QMP
handlers, like you do in PATCH 13.

What's the worst than can happen when we neglect to mark such an HMP
command?

> +waiting for an external event (such as I/O completion) in order to avoid

> +blocking the guest and other background operations.

> +

> +It is an error to specify both 'coroutine': true and 'allow-oob': true

> +for a command.  We don't currently have a use case for both together and

> +without a use case, it's not entirely clear what the semantics should

> +be.

> +

>  The optional 'if' member specifies a conditional.  See "Configuring

>  the schema" below for more on this.

[...]
Kevin Wolf Sept. 25, 2020, 3:37 p.m. UTC | #2
Am 14.09.2020 um 17:15 hat Markus Armbruster geschrieben:
> Kevin Wolf <kwolf@redhat.com> writes:
> 
> > This patch adds a new 'coroutine' flag to QMP command definitions that
> > tells the QMP dispatcher that the command handler is safe to be run in a
> > coroutine.
> >
> > The documentation of the new flag pretends that this flag is already
> > used as intended, which it isn't yet after this patch. We'll implement
> > this in another patch in this series.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> > Reviewed-by: Markus Armbruster <armbru@redhat.com>
> > ---
> >  docs/devel/qapi-code-gen.txt            | 12 ++++++++++++
> >  include/qapi/qmp/dispatch.h             |  1 +
> >  tests/test-qmp-cmds.c                   |  4 ++++
> >  scripts/qapi/commands.py                | 10 +++++++---
> >  scripts/qapi/doc.py                     |  2 +-
> >  scripts/qapi/expr.py                    | 10 ++++++++--
> >  scripts/qapi/introspect.py              |  2 +-
> >  scripts/qapi/schema.py                  | 12 ++++++++----
> >  tests/qapi-schema/test-qapi.py          |  7 ++++---
> >  tests/qapi-schema/meson.build           |  1 +
> >  tests/qapi-schema/oob-coroutine.err     |  2 ++
> >  tests/qapi-schema/oob-coroutine.json    |  2 ++
> >  tests/qapi-schema/oob-coroutine.out     |  0
> >  tests/qapi-schema/qapi-schema-test.json |  1 +
> >  tests/qapi-schema/qapi-schema-test.out  |  2 ++
> >  15 files changed, 54 insertions(+), 14 deletions(-)
> >  create mode 100644 tests/qapi-schema/oob-coroutine.err
> >  create mode 100644 tests/qapi-schema/oob-coroutine.json
> >  create mode 100644 tests/qapi-schema/oob-coroutine.out
> >
> > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
> > index f3e7ced212..36daa9b5f8 100644
> > --- a/docs/devel/qapi-code-gen.txt
> > +++ b/docs/devel/qapi-code-gen.txt
> > @@ -472,6 +472,7 @@ Syntax:
> >                  '*gen': false,
> >                  '*allow-oob': true,
> >                  '*allow-preconfig': true,
> > +                '*coroutine': true,
> >                  '*if': COND,
> >                  '*features': FEATURES }
> >  
> > @@ -596,6 +597,17 @@ before the machine is built.  It defaults to false.  For example:
> >  QMP is available before the machine is built only when QEMU was
> >  started with --preconfig.
> >  
> > +Member 'coroutine' tells the QMP dispatcher whether the command handler
> > +is safe to be run in a coroutine.
> 
> We need to document what exactly makes a command handler coroutine safe
> / unsafe.  We discussed this at some length in review of PATCH v4 1/4:
> 
>     Message-ID: <874kwnvgad.fsf@dusky.pond.sub.org>
>     https://lists.nongnu.org/archive/html/qemu-devel/2020-01/msg05015.html
> 
> I'd be willing to accept a follow-up patch, if that's more convenient
> for you.

Did we ever arrive at a conclusion for a good definition?

I mean I can give a definition like "it's coroutine safe if it results
in the right behaviour even when called in coroutine context", but
that's not really helpful.

FWIW, I would also have a hard time giving a much better definition than
this for thread safety.

> >                                     It defaults to false.  If it is true,
> > +the command handler is called from coroutine context and may yield while
> 
> Is it *always* called from coroutine context, or could it be called
> outside coroutine context, too?  I guess the former, thanks to PATCH 10,
> and as long as we diligently mark HMP commands that such call QMP
> handlers, like you do in PATCH 13.

Yes, it must *always* be called from coroutine context. This is the
reason why I included HMP after all, even though I'm really mostly just
interested in QMP.

> What's the worst than can happen when we neglect to mark such an HMP
> command?

When the command handler tries to yield and it's not in coroutine
context, it will abort(). If it never tries to yield, I think it would
just work - but of course, the ability to yield is the only reason why
you would want to run a command handler in a coroutine.

Kevin
Markus Armbruster Sept. 28, 2020, 8:23 a.m. UTC | #3
Kevin Wolf <kwolf@redhat.com> writes:

> Am 14.09.2020 um 17:15 hat Markus Armbruster geschrieben:
>> Kevin Wolf <kwolf@redhat.com> writes:
>> 
>> > This patch adds a new 'coroutine' flag to QMP command definitions that
>> > tells the QMP dispatcher that the command handler is safe to be run in a
>> > coroutine.
>> >
>> > The documentation of the new flag pretends that this flag is already
>> > used as intended, which it isn't yet after this patch. We'll implement
>> > this in another patch in this series.
>> >
>> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > Reviewed-by: Markus Armbruster <armbru@redhat.com>
>> > ---
>> >  docs/devel/qapi-code-gen.txt            | 12 ++++++++++++
>> >  include/qapi/qmp/dispatch.h             |  1 +
>> >  tests/test-qmp-cmds.c                   |  4 ++++
>> >  scripts/qapi/commands.py                | 10 +++++++---
>> >  scripts/qapi/doc.py                     |  2 +-
>> >  scripts/qapi/expr.py                    | 10 ++++++++--
>> >  scripts/qapi/introspect.py              |  2 +-
>> >  scripts/qapi/schema.py                  | 12 ++++++++----
>> >  tests/qapi-schema/test-qapi.py          |  7 ++++---
>> >  tests/qapi-schema/meson.build           |  1 +
>> >  tests/qapi-schema/oob-coroutine.err     |  2 ++
>> >  tests/qapi-schema/oob-coroutine.json    |  2 ++
>> >  tests/qapi-schema/oob-coroutine.out     |  0
>> >  tests/qapi-schema/qapi-schema-test.json |  1 +
>> >  tests/qapi-schema/qapi-schema-test.out  |  2 ++
>> >  15 files changed, 54 insertions(+), 14 deletions(-)
>> >  create mode 100644 tests/qapi-schema/oob-coroutine.err
>> >  create mode 100644 tests/qapi-schema/oob-coroutine.json
>> >  create mode 100644 tests/qapi-schema/oob-coroutine.out
>> >
>> > diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
>> > index f3e7ced212..36daa9b5f8 100644
>> > --- a/docs/devel/qapi-code-gen.txt
>> > +++ b/docs/devel/qapi-code-gen.txt
>> > @@ -472,6 +472,7 @@ Syntax:
>> >                  '*gen': false,
>> >                  '*allow-oob': true,
>> >                  '*allow-preconfig': true,
>> > +                '*coroutine': true,
>> >                  '*if': COND,
>> >                  '*features': FEATURES }
>> >  
>> > @@ -596,6 +597,17 @@ before the machine is built.  It defaults to false.  For example:
>> >  QMP is available before the machine is built only when QEMU was
>> >  started with --preconfig.
>> >  
>> > +Member 'coroutine' tells the QMP dispatcher whether the command handler
>> > +is safe to be run in a coroutine.
>> 
>> We need to document what exactly makes a command handler coroutine safe
>> / unsafe.  We discussed this at some length in review of PATCH v4 1/4:
>> 
>>     Message-ID: <874kwnvgad.fsf@dusky.pond.sub.org>
>>     https://lists.nongnu.org/archive/html/qemu-devel/2020-01/msg05015.html
>> 
>> I'd be willing to accept a follow-up patch, if that's more convenient
>> for you.
>
> Did we ever arrive at a conclusion for a good definition?
>
> I mean I can give a definition like "it's coroutine safe if it results
> in the right behaviour even when called in coroutine context", but
> that's not really helpful.

If an actual definition is out of reach, we should still say
*something*.  Even a mere "it's complicated" would help, because it
warns the reader he's on thin ice.

Can we give examples for "unsafe"?  You mentioned nested event loops.

> FWIW, I would also have a hard time giving a much better definition than
> this for thread safety.

For what it's worth, here's how POSIX.1-2017 defined "thread-safe":

    A thread-safe function can be safely invoked concurrently with other
    calls to the same function, or with calls to any other thread-safe
    functions, by multiple threads.  Each function defined in the System
    Interfaces volume of POSIX.1-2017 is thread-safe unless explicitly
    stated otherwise.  Examples are any "pure" function, a function
    which holds a mutex locked while it is accessing static storage, or
    objects shared among threads.

https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_407

>> >                                     It defaults to false.  If it is true,
>> > +the command handler is called from coroutine context and may yield while
>> 
>> Is it *always* called from coroutine context, or could it be called
>> outside coroutine context, too?  I guess the former, thanks to PATCH 10,
>> and as long as we diligently mark HMP commands that such call QMP
>> handlers, like you do in PATCH 13.
>
> Yes, it must *always* be called from coroutine context. This is the
> reason why I included HMP after all, even though I'm really mostly just
> interested in QMP.
>
>> What's the worst than can happen when we neglect to mark such an HMP
>> command?
>
> When the command handler tries to yield and it's not in coroutine
> context, it will abort(). If it never tries to yield, I think it would
> just work - but of course, the ability to yield is the only reason why
> you would want to run a command handler in a coroutine.

Let's spell this out.  After your paragraph

    Member 'coroutine' tells the QMP dispatcher whether the command handler
    is safe to be run in a coroutine.  It defaults to false.  If it is true,
    the command handler is called from coroutine context and may yield while
    waiting for an external event (such as I/O completion) in order to avoid
    blocking the guest and other background operations.

add something like

    Since the command handler may assume coroutine-context, any callers
    other than the QMP dispatcher must also call it in coroutine
    context.  In particular, HMP commands calling such a QMP command
    handler must be marked .coroutine = true in hmp-commands.hx.

Patch ordering issue: this becomes possible only in PATCH 10.
Possible solutions:

* Add the last sentence only in PATCH 10.

* Write "HMP commands cannot call such a QMP command handler" in PATCH
  8, replace it in PATCH 10.

* Add a "TODO make that possible" line here, drop it in PATCH 10.

* Reorder patches.

I don't like the first one much.  Anyway, up to you.
Markus Armbruster Oct. 2, 2020, 7:53 a.m. UTC | #4
Additional nitpick detail on Kevin's request.

Kevin Wolf <kwolf@redhat.com> writes:

> This patch adds a new 'coroutine' flag to QMP command definitions that

> tells the QMP dispatcher that the command handler is safe to be run in a

> coroutine.

>

> The documentation of the new flag pretends that this flag is already

> used as intended, which it isn't yet after this patch. We'll implement

> this in another patch in this series.

>

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> Reviewed-by: Markus Armbruster <armbru@redhat.com>

[...]
> diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py

> index 2942520399..928cd1eb5c 100644

> --- a/scripts/qapi/expr.py

> +++ b/scripts/qapi/expr.py

> @@ -88,10 +88,16 @@ def check_flags(expr, info):

>          if key in expr and expr[key] is not False:

>              raise QAPISemError(

>                  info, "flag '%s' may only use false value" % key)

> -    for key in ['boxed', 'allow-oob', 'allow-preconfig']:

> +    for key in ['boxed', 'allow-oob', 'allow-preconfig', 'coroutine']:

>          if key in expr and expr[key] is not True:

>              raise QAPISemError(

>                  info, "flag '%s' may only use true value" % key)

> +    if 'allow-oob' in expr and 'coroutine' in expr:

> +        # This is not necessarily a fundamental incompatibility, but we don't

> +        # have a use case and the desired semantics isn't obvious. The simplest

> +        # solution is to forbid it until we get a use case for it.

> +        raise QAPISemError(info, "flags 'allow-oob' and 'coroutine' "

> +                                 "are incompatible")


PEP 8: For flowing long blocks of text with fewer structural
restrictions (docstrings or comments), the line length should be limited
to 72 characters.

PEP 8: You should use two spaces after a sentence-ending period in
multi- sentence comments, except after the final sentence.

>  

>  

>  def check_if(expr, info, source):

[...]
Markus Armbruster Oct. 2, 2020, 7:59 a.m. UTC | #5
Hit send too fast.

Kevin Wolf <kwolf@redhat.com> writes:

> This patch adds a new 'coroutine' flag to QMP command definitions that

> tells the QMP dispatcher that the command handler is safe to be run in a

> coroutine.

>

> The documentation of the new flag pretends that this flag is already

> used as intended, which it isn't yet after this patch. We'll implement

> this in another patch in this series.

>

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> Reviewed-by: Markus Armbruster <armbru@redhat.com>

[...]
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py

> index 78309a00f0..c44d391c3f 100644

> --- a/scripts/qapi/schema.py

> +++ b/scripts/qapi/schema.py

> @@ -128,7 +128,7 @@ class QAPISchemaVisitor:

>  

>      def visit_command(self, name, info, ifcond, features,

>                        arg_type, ret_type, gen, success_response, boxed,

> -                      allow_oob, allow_preconfig):

> +                      allow_oob, allow_preconfig, coroutine):

>          pass

>  

>      def visit_event(self, name, info, ifcond, features, arg_type, boxed):

> @@ -713,7 +713,8 @@ class QAPISchemaCommand(QAPISchemaEntity):

>  

>      def __init__(self, name, info, doc, ifcond, features,

>                   arg_type, ret_type,

> -                 gen, success_response, boxed, allow_oob, allow_preconfig):

> +                 gen, success_response, boxed, allow_oob, allow_preconfig,

> +                 coroutine):

>          super().__init__(name, info, doc, ifcond, features)

>          assert not arg_type or isinstance(arg_type, str)

>          assert not ret_type or isinstance(ret_type, str)

> @@ -726,6 +727,7 @@ class QAPISchemaCommand(QAPISchemaEntity):

>          self.boxed = boxed

>          self.allow_oob = allow_oob

>          self.allow_preconfig = allow_preconfig

> +        self.coroutine = coroutine

>  

>      def check(self, schema):

>          super().check(schema)

> @@ -768,7 +770,7 @@ class QAPISchemaCommand(QAPISchemaEntity):

>          visitor.visit_command(

>              self.name, self.info, self.ifcond, self.features,

>              self.arg_type, self.ret_type, self.gen, self.success_response,

> -            self.boxed, self.allow_oob, self.allow_preconfig)

> +            self.boxed, self.allow_oob, self.allow_preconfig, self.coroutine)


Recommend to break the line after preconfig, like you do elsewhere.

>  

>  

>  class QAPISchemaEvent(QAPISchemaEntity):

> @@ -1074,6 +1076,7 @@ class QAPISchema:

>          boxed = expr.get('boxed', False)

>          allow_oob = expr.get('allow-oob', False)

>          allow_preconfig = expr.get('allow-preconfig', False)

> +        coroutine = expr.get('coroutine', False)

>          ifcond = expr.get('if')

>          features = self._make_features(expr.get('features'), info)

>          if isinstance(data, OrderedDict):

> @@ -1086,7 +1089,8 @@ class QAPISchema:

>          self._def_entity(QAPISchemaCommand(name, info, doc, ifcond, features,

>                                             data, rets,

>                                             gen, success_response,

> -                                           boxed, allow_oob, allow_preconfig))

> +                                           boxed, allow_oob, allow_preconfig,

> +                                           coroutine))


Preexisting: the arguments are kind of squeezed onto the right margin.
Hanging indent would avoid that.  Feel free to ignore.

>  

>      def _def_event(self, expr, info, doc):

>          name = expr['event']

[...]
diff mbox series

Patch

diff --git a/docs/devel/qapi-code-gen.txt b/docs/devel/qapi-code-gen.txt
index f3e7ced212..36daa9b5f8 100644
--- a/docs/devel/qapi-code-gen.txt
+++ b/docs/devel/qapi-code-gen.txt
@@ -472,6 +472,7 @@  Syntax:
                 '*gen': false,
                 '*allow-oob': true,
                 '*allow-preconfig': true,
+                '*coroutine': true,
                 '*if': COND,
                 '*features': FEATURES }
 
@@ -596,6 +597,17 @@  before the machine is built.  It defaults to false.  For example:
 QMP is available before the machine is built only when QEMU was
 started with --preconfig.
 
+Member 'coroutine' tells the QMP dispatcher whether the command handler
+is safe to be run in a coroutine.  It defaults to false.  If it is true,
+the command handler is called from coroutine context and may yield while
+waiting for an external event (such as I/O completion) in order to avoid
+blocking the guest and other background operations.
+
+It is an error to specify both 'coroutine': true and 'allow-oob': true
+for a command.  We don't currently have a use case for both together and
+without a use case, it's not entirely clear what the semantics should
+be.
+
 The optional 'if' member specifies a conditional.  See "Configuring
 the schema" below for more on this.
 
diff --git a/include/qapi/qmp/dispatch.h b/include/qapi/qmp/dispatch.h
index 0c2f467028..9fd2b720a7 100644
--- a/include/qapi/qmp/dispatch.h
+++ b/include/qapi/qmp/dispatch.h
@@ -25,6 +25,7 @@  typedef enum QmpCommandOptions
     QCO_NO_SUCCESS_RESP       =  (1U << 0),
     QCO_ALLOW_OOB             =  (1U << 1),
     QCO_ALLOW_PRECONFIG       =  (1U << 2),
+    QCO_COROUTINE             =  (1U << 3),
 } QmpCommandOptions;
 
 typedef struct QmpCommand
diff --git a/tests/test-qmp-cmds.c b/tests/test-qmp-cmds.c
index 5f1b245e19..d3413bfef0 100644
--- a/tests/test-qmp-cmds.c
+++ b/tests/test-qmp-cmds.c
@@ -36,6 +36,10 @@  void qmp_cmd_success_response(Error **errp)
 {
 }
 
+void qmp_coroutine_cmd(Error **errp)
+{
+}
+
 Empty2 *qmp_user_def_cmd0(Error **errp)
 {
     return g_new0(Empty2, 1);
diff --git a/scripts/qapi/commands.py b/scripts/qapi/commands.py
index 3cf9e1110b..6e6fc94a14 100644
--- a/scripts/qapi/commands.py
+++ b/scripts/qapi/commands.py
@@ -176,7 +176,8 @@  out:
     return ret
 
 
-def gen_register_command(name, success_response, allow_oob, allow_preconfig):
+def gen_register_command(name, success_response, allow_oob, allow_preconfig,
+                         coroutine):
     options = []
 
     if not success_response:
@@ -185,6 +186,8 @@  def gen_register_command(name, success_response, allow_oob, allow_preconfig):
         options += ['QCO_ALLOW_OOB']
     if allow_preconfig:
         options += ['QCO_ALLOW_PRECONFIG']
+    if coroutine:
+        options += ['QCO_COROUTINE']
 
     if not options:
         options = ['QCO_NO_OPTIONS']
@@ -267,7 +270,7 @@  void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
 
     def visit_command(self, name, info, ifcond, features,
                       arg_type, ret_type, gen, success_response, boxed,
-                      allow_oob, allow_preconfig):
+                      allow_oob, allow_preconfig, coroutine):
         if not gen:
             return
         # FIXME: If T is a user-defined type, the user is responsible
@@ -285,7 +288,8 @@  void %(c_prefix)sqmp_init_marshal(QmpCommandList *cmds);
             self._genh.add(gen_marshal_decl(name))
             self._genc.add(gen_marshal(name, arg_type, boxed, ret_type))
             self._regy.add(gen_register_command(name, success_response,
-                                                allow_oob, allow_preconfig))
+                                                allow_oob, allow_preconfig,
+                                                coroutine))
 
 
 def gen_commands(schema, output_dir, prefix):
diff --git a/scripts/qapi/doc.py b/scripts/qapi/doc.py
index 92f584edcf..11771de923 100644
--- a/scripts/qapi/doc.py
+++ b/scripts/qapi/doc.py
@@ -264,7 +264,7 @@  class QAPISchemaGenDocVisitor(QAPISchemaVisitor):
 
     def visit_command(self, name, info, ifcond, features,
                       arg_type, ret_type, gen, success_response, boxed,
-                      allow_oob, allow_preconfig):
+                      allow_oob, allow_preconfig, coroutine):
         doc = self.cur_doc
         self._gen.add(texi_msg('Command', doc, ifcond,
                                texi_arguments(doc,
diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index 2942520399..928cd1eb5c 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -88,10 +88,16 @@  def check_flags(expr, info):
         if key in expr and expr[key] is not False:
             raise QAPISemError(
                 info, "flag '%s' may only use false value" % key)
-    for key in ['boxed', 'allow-oob', 'allow-preconfig']:
+    for key in ['boxed', 'allow-oob', 'allow-preconfig', 'coroutine']:
         if key in expr and expr[key] is not True:
             raise QAPISemError(
                 info, "flag '%s' may only use true value" % key)
+    if 'allow-oob' in expr and 'coroutine' in expr:
+        # This is not necessarily a fundamental incompatibility, but we don't
+        # have a use case and the desired semantics isn't obvious. The simplest
+        # solution is to forbid it until we get a use case for it.
+        raise QAPISemError(info, "flags 'allow-oob' and 'coroutine' "
+                                 "are incompatible")
 
 
 def check_if(expr, info, source):
@@ -342,7 +348,7 @@  def check_exprs(exprs):
                        ['command'],
                        ['data', 'returns', 'boxed', 'if', 'features',
                         'gen', 'success-response', 'allow-oob',
-                        'allow-preconfig'])
+                        'allow-preconfig', 'coroutine'])
             normalize_members(expr.get('data'))
             check_command(expr, info)
         elif meta == 'event':
diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 23652be810..5907b09cd5 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -216,7 +216,7 @@  const QLitObject %(c_name)s = %(c_string)s;
 
     def visit_command(self, name, info, ifcond, features,
                       arg_type, ret_type, gen, success_response, boxed,
-                      allow_oob, allow_preconfig):
+                      allow_oob, allow_preconfig, coroutine):
         arg_type = arg_type or self._schema.the_empty_object_type
         ret_type = ret_type or self._schema.the_empty_object_type
         obj = {'arg-type': self._use_type(arg_type),
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 78309a00f0..c44d391c3f 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -128,7 +128,7 @@  class QAPISchemaVisitor:
 
     def visit_command(self, name, info, ifcond, features,
                       arg_type, ret_type, gen, success_response, boxed,
-                      allow_oob, allow_preconfig):
+                      allow_oob, allow_preconfig, coroutine):
         pass
 
     def visit_event(self, name, info, ifcond, features, arg_type, boxed):
@@ -713,7 +713,8 @@  class QAPISchemaCommand(QAPISchemaEntity):
 
     def __init__(self, name, info, doc, ifcond, features,
                  arg_type, ret_type,
-                 gen, success_response, boxed, allow_oob, allow_preconfig):
+                 gen, success_response, boxed, allow_oob, allow_preconfig,
+                 coroutine):
         super().__init__(name, info, doc, ifcond, features)
         assert not arg_type or isinstance(arg_type, str)
         assert not ret_type or isinstance(ret_type, str)
@@ -726,6 +727,7 @@  class QAPISchemaCommand(QAPISchemaEntity):
         self.boxed = boxed
         self.allow_oob = allow_oob
         self.allow_preconfig = allow_preconfig
+        self.coroutine = coroutine
 
     def check(self, schema):
         super().check(schema)
@@ -768,7 +770,7 @@  class QAPISchemaCommand(QAPISchemaEntity):
         visitor.visit_command(
             self.name, self.info, self.ifcond, self.features,
             self.arg_type, self.ret_type, self.gen, self.success_response,
-            self.boxed, self.allow_oob, self.allow_preconfig)
+            self.boxed, self.allow_oob, self.allow_preconfig, self.coroutine)
 
 
 class QAPISchemaEvent(QAPISchemaEntity):
@@ -1074,6 +1076,7 @@  class QAPISchema:
         boxed = expr.get('boxed', False)
         allow_oob = expr.get('allow-oob', False)
         allow_preconfig = expr.get('allow-preconfig', False)
+        coroutine = expr.get('coroutine', False)
         ifcond = expr.get('if')
         features = self._make_features(expr.get('features'), info)
         if isinstance(data, OrderedDict):
@@ -1086,7 +1089,8 @@  class QAPISchema:
         self._def_entity(QAPISchemaCommand(name, info, doc, ifcond, features,
                                            data, rets,
                                            gen, success_response,
-                                           boxed, allow_oob, allow_preconfig))
+                                           boxed, allow_oob, allow_preconfig,
+                                           coroutine))
 
     def _def_event(self, expr, info, doc):
         name = expr['event']
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index f396b471eb..e8db9d09d9 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -68,12 +68,13 @@  class QAPISchemaTestVisitor(QAPISchemaVisitor):
 
     def visit_command(self, name, info, ifcond, features,
                       arg_type, ret_type, gen, success_response, boxed,
-                      allow_oob, allow_preconfig):
+                      allow_oob, allow_preconfig, coroutine):
         print('command %s %s -> %s'
               % (name, arg_type and arg_type.name,
                  ret_type and ret_type.name))
-        print('    gen=%s success_response=%s boxed=%s oob=%s preconfig=%s'
-              % (gen, success_response, boxed, allow_oob, allow_preconfig))
+        print('    gen=%s success_response=%s boxed=%s oob=%s preconfig=%s%s'
+              % (gen, success_response, boxed, allow_oob, allow_preconfig,
+                 " coroutine=True" if coroutine else ""))
         self._print_if(ifcond)
         self._print_features(features)
 
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index c87d141417..7a9bc69cc0 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -141,6 +141,7 @@  schemas = [
   'nested-struct-data.json',
   'nested-struct-data-invalid-dict.json',
   'non-objects.json',
+  'oob-coroutine.json',
   'oob-test.json',
   'allow-preconfig-test.json',
   'pragma-doc-required-crap.json',
diff --git a/tests/qapi-schema/oob-coroutine.err b/tests/qapi-schema/oob-coroutine.err
new file mode 100644
index 0000000000..c01a4992bd
--- /dev/null
+++ b/tests/qapi-schema/oob-coroutine.err
@@ -0,0 +1,2 @@ 
+oob-coroutine.json: In command 'oob-command-1':
+oob-coroutine.json:2: flags 'allow-oob' and 'coroutine' are incompatible
diff --git a/tests/qapi-schema/oob-coroutine.json b/tests/qapi-schema/oob-coroutine.json
new file mode 100644
index 0000000000..0f67663bcd
--- /dev/null
+++ b/tests/qapi-schema/oob-coroutine.json
@@ -0,0 +1,2 @@ 
+# Check that incompatible flags allow-oob and coroutine are rejected
+{ 'command': 'oob-command-1', 'allow-oob': true, 'coroutine': true }
diff --git a/tests/qapi-schema/oob-coroutine.out b/tests/qapi-schema/oob-coroutine.out
new file mode 100644
index 0000000000..e69de29bb2
diff --git a/tests/qapi-schema/qapi-schema-test.json b/tests/qapi-schema/qapi-schema-test.json
index 3a9f2cbb33..63f92adf68 100644
--- a/tests/qapi-schema/qapi-schema-test.json
+++ b/tests/qapi-schema/qapi-schema-test.json
@@ -148,6 +148,7 @@ 
   'returns': 'UserDefTwo' }
 
 { 'command': 'cmd-success-response', 'data': {}, 'success-response': false }
+{ 'command': 'coroutine-cmd', 'data': {}, 'coroutine': true }
 
 # Returning a non-dictionary requires a name from the whitelist
 { 'command': 'guest-get-time', 'data': {'a': 'int', '*b': 'int' },
diff --git a/tests/qapi-schema/qapi-schema-test.out b/tests/qapi-schema/qapi-schema-test.out
index 891b4101e0..8868ca0dca 100644
--- a/tests/qapi-schema/qapi-schema-test.out
+++ b/tests/qapi-schema/qapi-schema-test.out
@@ -203,6 +203,8 @@  command user_def_cmd2 q_obj_user_def_cmd2-arg -> UserDefTwo
     gen=True success_response=True boxed=False oob=False preconfig=False
 command cmd-success-response None -> None
     gen=True success_response=False boxed=False oob=False preconfig=False
+command coroutine-cmd None -> None
+    gen=True success_response=True boxed=False oob=False preconfig=False coroutine=True
 object q_obj_guest-get-time-arg
     member a: int optional=False
     member b: int optional=True