Re: [Qemu-devel] [PATCH 2/4] vl: move global property, migrate init earlier

[Peter Xu]

On Mon, Jul 03, 2017 at 11:59:03AM -0300, Eduardo Habkost wrote:
> On Mon, Jul 03, 2017 at 10:44:06AM +0800, Peter Xu wrote:
> > Currently drive_init_func() may call migrate_get_current() while the
> > migrate object is still not ready yet at that time. Move the migration
> > object init earlier, along with the global properties, right after
> > acceleration init.
> > 
> > This fixes a breakage for iotest 055, which caused an assertion failure.
> > 
> > Reported-by: Max Reitz <address@hidden>
> > Reported-by: Philippe Mathieu-Daudé <address@hidden>
> > Fixes: 3df663 ("migration: move only_migratable to MigrationState")
> > Signed-off-by: Peter Xu <address@hidden>
> > ---
> >  vl.c | 24 ++++++++++++------------
> >  1 file changed, 12 insertions(+), 12 deletions(-)
> > 
> > diff --git a/vl.c b/vl.c
> > index 0c497a3..2ae4313 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -4414,6 +4414,18 @@ int main(int argc, char **argv, char **envp)
> >  
> >      configure_accelerator(current_machine);
> >  
> > +    /*
> > +     * Register all the global properties, including accel properties,
> > +     * machine properties, and user-specified ones.
> > +     */
> > +    register_global_properties(current_machine);
> > +
> > +    /*
> > +     * Migration object can only be created after global properties
> > +     * are applied correctly.
> > +     */
> > +    migration_object_init();
> > +
> 
> So, things that might introduce bugs here are:
> 1) Unexpected qdev_prop_register_global() calls between this place and
>    the original register_global_properties() call (that would now happen
>    in a different order).
> 2) register_global_properties() seeing a different global property list
>    because it is being called earlier.
>    2.1) AccelClass::global_props is statically defined and will be the
>         same here. Not a problem.
>    2.2) MachineClass::compat_props: same as above.
>    2.3) user-provided global properties: we need to ensure all
>         properties in the "global" QemuOpts section are already
>         available at this point.
> 
> 
> To ensure (1) is not a problem, we need to check all calls for
> qdev_prop_register_global().  The callers are:
> 
> * configure_rtc()
>   - Called very early, when parsing command-line options.  Not a problem.[1]
> * global_init_func()
>   * Called by user_register_global_props()
>     * Called by register_global_properties().
>       - That's the code we're moving.  Not a problem.
> * QEMU_OPTION_rtc_td_hack case in main()
>   - Called very early, when parsing command-line options.  Not a problem.[1]
> * QEMU_OPTION_no_kvm_pit_reinjection case in main()
>   - Called very early, when parsing command-line options.  Not a problem.[1]
> * register_compat_prop()
>   * Called by machine_register_compat_props()
>     * Called by register_global_properties().
>       - That's the code we're moving.  Not a problem.
>   * Called by machine_register_compat_for_subclass()
>     * Called by machine_register_compat_props() (see above)
>   * Called by register_compat_props_array()
>     * Called by accel_register_compat_props()
>       * Called by register_global_properties().
>         - That's the code we're moving.  Not a problem.
> * qdev_prop_register_global_list()
>   - Used only by unit test code.
> * cpu_common_parse_features()
>   - Used when initializing CPUs, which is done much later, when
>     machine_run_board_init() is called (or when -device is handled by
>     device_init_func()).  Not a problem.[2]
> * x86_cpu_parse_featurestr()
>   - Same as above.
> 
> 
> To ensure (2.3) is not a problem, we need to look for references to
> qemu_find_opts("global") or qemu_global_opts.  They are:
> 
> * user_register_global_props()
>   - This is the code we're moving.  Not a problem.
> * default_driver_check() call at main()
>   - This happens earlier, before the code we're moving.  Not a problem.
> * qemu_global_option()
>   - Called very early, when parsing command-line options.  Not a problem.
> 
> 
> So the code reordering looks OK.
> 
> Reviewed-by: Eduardo Habkost <address@hidden>

I really appreciate for such an in-depth review on this patch.

> 
> 
> Notes about things we need to fix in the future:
> 
> [1] I think they should be replaced by qemu_global_option() calls, though.
> [2] This is a fragile portion of the global property code and should be
>     eventually moved to the command-line parsing section of main().

Agree on both.

Thanks again!

-- 
Peter Xu