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

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Damien Hedde <damien.hedde@greensocs.com> writes:

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

Be careful, the RunState is API and things watch for events on it, so
any changes to it are delicate.

Dave

> > + 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.
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK