Re: [Qemu-ppc] [Qemu-devel] [PATCH 2/2] vl: Print CPU help after we've registered the CPU accelerators

[Eduardo Habkost]

On Mon, Mar 06, 2017 at 10:06:27AM +1100, David Gibson wrote:
> On Fri, Mar 03, 2017 at 11:58:07AM -0300, Eduardo Habkost wrote:
> > On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote:
> > > When running with KVM on POWER, we register some CPU types during
> > > the initialization function of the ppc64 KVM code (which unfortunately
> > > also can not be done via a type_init() like it is done on x86).
> > 
> > Can you elaborate why it can't be done via type_init()? If the
> > QOM type hierarchy depends on any runtime data unavailable at
> > type_init(), we should fix that.
> 
> Hmm.. how?  This is specifically for the special 'host' cpu in the
> case of KVM.  We can't use a static configuration here, because there
> are things on the host that could limit what features of the CPU are
> available for guest use.  So, we need KVM to be initialized in order
> to query that information.

There's nothing saying you need to query that information before
type_register() or during class_init, does it? The behavior of
TYPE_HOST_POWERPC_CPU after object_new() is called can be as
dynamic as you want it to, but the QOM type hierarchy is supposed
to be static.

Registering QOM types outside type_init() is only causing us
problems and we should stop doing that.

> 
> > >                                                                 So to
> > > be able to see these updates in the CPU help text, the code that calls
> > > list_cpus() has to be run after configure_accelerator(). This move should
> > > be fine since the "cpu_model" variable is also never used before the call
> > > to configure_accelerator(), and thus there should not be any unwanted
> > > side effects in the code before configure_accelerator() if the user
> > > started QEMU with "-cpu ?" or "-cpu help".
> > > 
> > > Signed-off-by: Thomas Huth <address@hidden>
> > 
> > I am not convinced that the output of "-cpu help" and
> > "-cpu help -machine accel=kvm" should look different. Do you have
> > an example of what exactly is wrong with the output currently?
> > 
> > I actually believe list_cpus() needs to be called _earlier_, not
> > later. Otherwise we won't be able to fix this bug:
> > 
> >   $ qemu-system-arm -cpu help
> >   qemu-system-arm:/usr/local/etc/qemu/qemu.conf:1: No machine specified, 
> > and there is no default
> >   Use -machine help to list supported machines
> >   $ 
> > 
> > > ---
> > >  vl.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/vl.c b/vl.c
> > > index 0b72b12..315c5c3 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -4055,11 +4055,6 @@ int main(int argc, char **argv, char **envp)
> > >          qemu_set_hw_version(machine_class->hw_version);
> > >      }
> > >  
> > > -    if (cpu_model && is_help_option(cpu_model)) {
> > > -        list_cpus(stdout, &fprintf, cpu_model);
> > > -        exit(0);
> > > -    }
> > > -
> > >      if (!trace_init_backends()) {
> > >          exit(1);
> > >      }
> > > @@ -4298,6 +4293,11 @@ int main(int argc, char **argv, char **envp)
> > >  
> > >      configure_accelerator(current_machine);
> > >  
> > > +    if (cpu_model && is_help_option(cpu_model)) {
> > > +        list_cpus(stdout, &fprintf, cpu_model);
> > > +        exit(0);
> > > +    }
> > > +
> > >      if (qtest_chrdev) {
> > >          qtest_init(qtest_chrdev, qtest_log, &error_fatal);
> > >      }
> > 
> 
> -- 
> David Gibson                  | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au        | minimalist, thank you.  NOT _the_ 
> _other_
>                               | _way_ _around_!
> http://www.ozlabs.org/~dgibson



-- 
Eduardo