Re: [PATCH] qmp: Stabilize preconfig

[Paolo Bonzini]

On 11/12/21 12:48, Markus Armbruster wrote:
> > The monitor starts, the question is the availability of the event loop.
>
> What does the event loop depend on?

It depends on moving the relevant code out of qemu_init (at least 
conditionally, as is the case for what is in qmp_x_exit_preconfig). 
This in turn has the problem that it's ugly to have lingering unapplied 
settings from the command line.

> > > > 5) PHASE_MACHINE_READY - machine init done notifiers have been called
> > > > and the VM is ready.  Devices plugged in this phase already count as
> > > > hot-plugged.  -S starts the monitor here.
>
> Why would anyone *want* to plug a device in PHASE_MACHINE_READY (when
> the plug is hot) instead of earlier (when it's cold)?

Well, PHASE_MACHINE_READY includes the whole time the guest is running. 
 So the simplest thing to do is to tell the user "if it hurts, don't do 
it".  If you want a cold-plugged device, plug it during 
PHASE_MACHINE_INIT, which right now means on the command line.

> > > Related question: when exactly in these phases do we create devices
> > > specified with -device?
> >
> > In PHASE_MACHINE_INIT---that is, after the machine has been initialized
> > and before machine-done-notifiers have been called.
>
> In other words, you should never use device_add where -device would do,
> because the latter gives you cold plug (which is simple and reliable),
> and the former hot plug (which is the opposite).

Exactly.

> > No, because the monitor goes directly from a point where device_add 
> > fails (PHASE_ACCEL_CREATED) to a point where devices are hotplugged 
> > (PHASE_MACHINE_READY).
>
> Bummer.

True, but consider that these "phases" were reconstructed ex post.  It's 
not like x-exit-preconfig was designed to skip PHASE_MACHINE_INIT; it's 
just that preconfig used to call qemu_main_loop() at the point which is 
now known as PHASE_ACCEL_CREATED.

> > > > With a pure-QMP configuration flow, PHASE_MACHINE_CREATED would be
> > > > reached with a machine-set command (corresponding to the
> > > > non-deprecated parts of -machine) and PHASE_ACCEL_CREATED would be
> > > > reached with an accel-set command (corresponding to -accel).
> > >
> > > I don't think this depends on "pure-QMP configuration flow".  -machine
> > > and -accel could advance the phase just like their buddies machine-set
> > > and accel-set.
> >
> > They already do (see qemu_init's calls to phase_advance).
> >
> > > State transition diagram:
> > >
> > >       PHASE_NO_MACHINE (initial state)
> > >               |  -machine or machine-set
> > >       PHASE_MACHINE_CREATED
> > >               |  -accel or accel-set
> > >       PHASE_ACCEL_CREATED
> > >               |
> >
> > qmp_x_exit_preconfig() -> qemu_init_board() -> machine_run_board_init()
>
> I read this as "the state transition happens in
> machine_run_board_init(), called from qmp_x_exit_preconfig() via
> qemu_init_board()".

Exactly.  And in turn qmp_x_exit_preconfig() is reached from either the 
monitor (with -preconfig) or qemu_init (without -preconfig).

> > >       PHASE_MACHINE_INIT
> > >               |
> >
> > qmp_x_exit_preconfig() -> qemu_machine_creation_done() ->
> > qdev_machine_creation_done()
>
> I read this as "the state transition happens in
> qdev_machine_creation_done(), called from qmp_x_exit_preconfig() via
> qemu_machine_creation_done()".

Right again.  In both cases, just grep for calls of "phase_advance".

> > > The earlier the monitor becomes available, the better.
> > > Ideally, we'd process the command line strictly left to right, and fail
> > > options that are "out of phase".  Make the monitor available right when
> > > we process its -mon.  The -chardev for its character device must precede
> > > it.
> >
> > The boat for this has sailed.  The only sane way to do this is a new binary.
>
> "Ideally" still applies to any new binary.

Well, "ideally" any new binary would only have a few command line 
options, and ordering would be mostly irrelevant.  For example I'd 
expect a QMP binary to only have a few options, mostly for 
debugging/development (-L, -trace) and for process-wide settings (such 
as -name).

> > > Likewise, we'd fail QMP commands that are "out of phase".
> > > @allow-preconfig is a crutch that only exists because we're afraid (with
> > > reason) of hidden assumptions in QMP commands.
> >
> > At this point, it's not even like that anymore (except for block devices
> > because my patches haven't been applied).
>
> My point is that we still have quite a few commands without
> 'allow-preconfig' mostly because we are afraid of allowing them in
> preconfig state, not because of true phase dependencies.

I think there's very few of them, if any (outside the block layer for 
which patches exist), and those are due to distraction more than fear.

Paolo