Re: [PATCH] qmp: Stabilize preconfig

[Paolo Bonzini]

On 11/11/21 15:37, Markus Armbruster wrote:
> > 1) PHASE_NO_MACHINE - backends can already be created here, but no
> > machine exists yet
> >
> > 2) PHASE_MACHINE_CREATED - the machine object has been created.  It's
> > not initialized, but it's there.
> >
> > 3) PHASE_ACCEL_CREATED - the accelerator object has been created.  The
> > accelerator needs the machine object, because for example KVM might
> > not support all machine types.  So the accelerator queries the machine
> > object and fails creation in case of incompatibility.  This enables
> > e.g. fallback to TCG.  -preconfig starts the monitor here.
>
> We should be able to start monitors first, if we put in the work.

The monitor starts, the question is the availability of the event loop. 
 This requires a command (or a something) to advance to the next phase. 
  x-exit-preconfig is such a command.

In addition, one thing I don't like of preconfig is that command line 
arguments linger until they are triggered by x-exit-preconfig.  Adding 
more such commands makes things worse.

> > 4) PHASE_MACHINE_INIT - machine initialization consists mostly in
> > creating the onboard devices.  For this to happen, the machine needs
> > to learn about the accelerator, because onboard devices include CPUs
> > and other accelerator-dependent devices.  Devices plugged in this
> > phase are cold-plugged.
> >
> > 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.
>
> Remind us: what work is done in the machine init done notifiers?

It depends, but---generally speaking---what they do applies only to 
cold-plugged devices.  For example, fw_cfg gathers the boot order in the 
machine-init-done notifier (via get_boot_devices_list).

> What exactly necessitates "count as hot-plugged"?  Is it something done
> in these notifiers?

It depends on the bus.  It boils down to this code in device_initfn:

    if (phase_check(PHASE_MACHINE_READY)) {
        dev->hotplugged = 1;
        qdev_hot_added = true;
    }

For example, hotplugged PCI devices must define function 0 last; 
coldplugged PCI devices can define functions in any order 
(do_pci_register_device, called by pci_qdev_realize).

Another example, a device_add after machine-done causes an ACPI hotplug 
event, because acpi_pcihp_device_plug_cb checks dev->hotplugged.

> > x-exit-preconfig goes straight from PHASE_ACCEL_CREATED to
> > PHASE_MACHINE_READY.  Devices can only be created after
> > PHASE_MACHINE_INIT, so device_add cannot be enabled at preconfig
> > stage.
>
> Now I am confused again.  Can you cold plug devices with device_add in
> presence of -preconfig, and if yes, how?

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).

> 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.

> > 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
>              v
>      PHASE_MACHINE_CREATED
>              |
>              |  -accel or accel-set
>              v
>      PHASE_ACCEL_CREATED
>              |
>              |  ???

qmp_x_exit_preconfig() -> qemu_init_board() -> machine_run_board_init()

>              v
>      PHASE_MACHINE_INIT
>              |
>              |  ???

qmp_x_exit_preconfig() -> qemu_machine_creation_done() -> 
qdev_machine_creation_done()

The steps in qmp_x_exit_preconfig() are pretty self-explanatory:

    qemu_init_board();
    qemu_create_cli_devices();
    qemu_machine_creation_done();

qemu_init() calls qmp_x_exit_preconfig() if -preconfig is not passed on 
the command line.

>              v
>      PHASE_MACHINE_READY
>              |
>              |  cont
>              v
>             ???

No phases anymore, it's always PHASE_MACHINE_READY.  cont simply changes 
the runstate to RUNNING.

> 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.

> 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).  allow-preconfig is just a 
quick way to avoid writing

    if (!phase_check(PHASE_MACHINE_READY)) {
        error_setg(errp, "Please configure the machine first");
        return;
    }

over and over in many commands.

Paolo