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

[Igor Mammedov]

On Mon, 4 Jun 2018 21:35:46 -0300
Eduardo Habkost <address@hidden> wrote:

> 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?
Agreed, we can discuss and settle this internal to QEMU implementation
detail independently on top of actual fix.

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

I'll post a proper patch with it.