Re: [RFC PATCH v3 0/5] QMP support for cold-plugging devices

[Markus Armbruster]

Damien Hedde <damien.hedde@greensocs.com> writes:

> Hi all,
>
> This series adds support for cold-plugging devices using QMP
> commands. It is a step towards machine configuration using QMP, but
> it does not allow the user to add more devices than he could do with
> the CLI options before.
>
> Right now we can add a device using 2 ways:
> + giving "-device" CLI option. In that case the device is
>   cold-plugged: it is created before the machine becomes ready.
> + issuing device_add QMP command. In that case the device is
>   hot-plugged (the command can not be issued before the machine is
>   ready).
>
> This series allows to issue device_add QMP to cold-plug a device
> like we do with "-device" CLI option. All added QMP commands are
> marked as 'unstable' in qapi as they are part of preconfig.
> We achieve this by introducing a new 'x-machine-init' command to
> stop the VM creation at a point where we can cold-plug devices.
>
> We are aware of the ongoing discussion about preconfig future (see
> [1]). This patchset includes no major modifications from the v2 (but
> the scope is reduced) and "x-machine-init" simply stops the
> configuration between qemu_board_init() and qemu_create_cli_devices()
> function calls.
>
> As a consequence, in the current state, the timeline is:

"current state" = with this series applied?

> + "x-machine-init" command
> + "device_add" cold-plug commands (no fw_cfg legacy order support)
> + "x-exit-preconfig" command will then trigger the following
> + "-soundhw" CLI options
> + "-fw_cfg" CLI options
> + usb devices creation
> + "-device" CLI options (with fw_cfg legacy order support)
> + some other devices creation (with fw_cfg legacy order support)
>
> We don't know if the differences between -device/device_add are
> acceptable or not. To reduce/remove them we could move the
> "x-machine-init" stopping point. What do you think ?

I'm not sure I understand this paragraph.

I understand the difference between -device and device_add in master:
cold vs. hot plug.

Your patch series makes "cold" device_add possible, i.e. it reduces
(eliminates?) the difference between -device and device_add when the
latter is "cold".

What difference remains that moving 'the "x-machine-init" stopping
point' would 'reduce/remove'?

> Patches 1, 3 and 5 miss a review.
>
> The series is organized as follow:
>
> + Patches 1 and 2 converts the MachinePhase enum to a qapi definition
>   and add the 'query-machine-phase'. It allows to introspect the
>   current machine phase during preconfig as we will now be able to
>   reach several machine phases using QMP.

If we fold MachinePhase into RunState, we can reuse query-status.

Having two state machines run one after the other feels like one too
many.

> + Patch 3 adds the 'x-machine-init' QMP command to stop QEMU at
>   machine-initialized phase during preconfig.
> + Patch 4 allows issuing device_add QMP command during the
>   machine-initialized phase.
> + Patch 5 improves the doc about preconfig in consequence. 

I understand you want to make progress towards machine configuration
with QMP.  However, QEMU startup is (in my educated opinion) a hole, and
we should be wary of digging deeper.

The "timeline" you gave above illustrates this.  It's a complicated
shuffling of command line options and QMP commands that basically nobody
can keep in working memory.  We have reshuffled it / made it more
complicated quite a few times already to support new features.  Based on
your cover letter, I figure you're making it more complicated once more.

At some point, we need to stop digging us deeper into the hole.  This is
not an objection to merging your work.  It's a call to stop and think.

Let me quote the sketch I posted to the "Stabilize preconfig" thread:

1. Start event loop

2. Feed it CLI left to right.  Each option runs a handler just like each
   QMP command does.

   Options that read a configuration file inject the file into the feed.

   Options that create a monitor create it suspended.

   Options may advance the phase / run state, and they may require
   certain phase(s).

3. When we're done with CLI, resume any monitors we created.

4. Monitors now feed commands to the event loop.  Commands may advance
   the phase / run state, and they may require certain phase(s).

Users can do as much or as little with the CLI as they want.  You'd
probably want to set up a QMP monitor and no more.

device_add becomes possible at a certain state of the phase / run state
machine.  It changes from cold to hot plug at a certain later state.

> [1]: 
> https://lore.kernel.org/qemu-devel/b31f442d28920447690a6b8cee865bdbacde1283.1635160056.git.mprivozn@redhat.com
>
> Thanks for your feedback.