Re: [Qemu-devel] [PATCH v2 1/2] vl: don't use RUN_STATE_PRECONFIG as initial state

[Eduardo Habkost]

On Mon, Jun 04, 2018 at 07:37:15PM +0200, Igor Mammedov wrote:
> On Mon, 4 Jun 2018 17:11:57 +0100
> Daniel P. Berrangé <address@hidden> wrote:
> 
> > On Mon, Jun 04, 2018 at 05:40:32PM +0200, Igor Mammedov wrote:
> > > On Mon,  4 Jun 2018 13:03:44 +0100
> > > Daniel P. Berrangé <address@hidden> wrote:
> > >   
> > > > The RUN_STATE_PRECONFIG state is not supposed to be reachable unless the
> > > > --preconfig argument is given to QEMU, but when it was introduced in:
> > > > 
> > > >   commit 047f7038f586d2150f16c6d9ba9cfd0479f0f6ac
> > > >   Author: Igor Mammedov <address@hidden>
> > > >   Date:   Fri May 11 19:24:43 2018 +0200
> > > > 
> > > >     cli: add --preconfig option
> > > > 
> > > > ...the global 'current_run_state' variable was changed to have an 
> > > > initial
> > > > value of RUN_STATE_PRECONFIG regardless of whether --preconfig is given.
> > > > 
> > > > A second invokation of main_loop() was added which then toggles it back
> > > > to RUN_STATE_PRELAUNCH when --preconfig is not given. This is racy
> > > > because it leaves a window where QEMU is in RUN_STATE_PRECONFIG despite
> > > > --preconfig not being given.  
> > > Above statements isn't exactly correct, PRECONFIG were supposed to be
> > > the new state of QEMU from start up till machine_run_board_init(),
> > > that would separate stage of initial configuration out from all
> > > encompassing PRELAUNCH state. So I'd scratch out above part.  
> > 
> > That doesn't really make sense, given that --preconfig is *not* the
> > default and thus not supposed to be an active state unless the app
> > has explicitly opted in.
> >
> > IMHO running PRECONFIG state for any period of time when the app
> > has not requested --preconfig is simply broken, and a recipe for
> > obscure bugs like the ones we've seen right now.
> mgmt hasn't opted in for default PRELAUNCH either, for it default
> PRECONFIG runstate is not visible unless it opts in so nothing
> is broken in regards to this.
> Default runstate selection is QEMU's internal impl. detail.

Daniel's description is how he expects it to work, but your
description reflects the way the state machine actually works
today (and how it worked befor the --preconfig series).

However, I agree with Daniel that moving to PRECONFIG or
PRELAUNCH if neither --preconfig or -S were specified is
confusing, and I would prefer to change it the way he suggests.

But:

[...]
> >    $START ------------->  PRELAUNCH {-> INMIGRATE]
> >      |              ^
> >      |              |
> >      +-- PRECONFIG -+
> > 
> > By marking the current state as PRECONFIG by default, we've essentially
> > given 2 meanings to  PRECONFIG - sometimes it means stuff  that can be
> > unconditionally run in early startup, and sometimes it means stuff that
> > can only be run if --preconfig is given. Introducing the separate NONE
> > state clarifies that inherant contradiction in startup phases.
> Yep, one can say it this way (as merged PRECONFIG was early
> configuration stage regardless of if it's unconditional early
> initialization or early CLI parsing/QMP configuration).
> 
> Maybe adding NONE state makes sense but I'm not quite sure,
> that's why I'd not rush it in and discuss if we really need
> fragment early configuration into more stages.
> (we can do it later as both stages aren't visible to user by default).

Is it possible to fix the bugs first, and discuss how to refactor
the state machine later?

In the meantime, we could even document preconfig more accurately
to avoid additional confusion:

# @preconfig: Board initialization was not run yet.  The state is
#             visible to the outside only if the --preconfig CLI
#             option is used.  (Since 3.0)

-- 
Eduardo