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

[David Gibson]

On Tue, Mar 07, 2017 at 09:02:37AM -0300, Eduardo Habkost wrote:
> On Tue, Mar 07, 2017 at 02:31:05PM +1100, David Gibson wrote:
> > On Mon, Mar 06, 2017 at 08:47:52AM -0300, Eduardo Habkost wrote:
> > > 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.
> > 
> > So, the thing is we have some properties that logically belong in the
> > CPU class, rather than instance, and that's correct for all TCG CPUs,
> > but not for the KVM host CPU.  It seems nasty to have to force all
> > those things into the instance just because of KVM.
> 
> You can still register any class properties you want, without
> querying KVM first. Are there properties that you are able to
> register only after querying KVM? Is the set of class properties
> on TYPE_HOST_POWERPC_CPU different depending on the host
> capabilities?

Sorry, I used "properties" sloppily, not meaning QOM properties.

There are several fields in the CPU class which describe CPU
capabilities - bitmasks indicating which instructions are available,
and L1 cacheline sizes.  There are some other things that are in the
CPU instance at the moment, but arguably would belong better in the
class: available page sizes and PTE encodings, for example.

At the real hardware level these things are dependent only the model
of CPU - hence belonging in cpu class, not instance.  But, because of
the way virtualization works on POWER, some of the features may not be
available to guests, due to configuration of the hypervisor.  So for
the "host" cpu we need to query KVM to see which CPU features are
actually available.

> I'm looking at target/ppc/cpu-models.c, and I only see the
> class_init function setting PowerPCCPUClass::pvr and
> PowerPCCPUClass::svr. Am I missing anything?

Yes.  Notice that the .parent set there is not the base powerpc cpu
class, but a "family" class.  Those families are defined in
translate_init.c by another hairy macro POWERPC_FAMILY().  The family
class_init function (generally done as a block below the macro
invocation) initializes several other things, including insns_flags.

kvmppc_host_cpu_class_init(), uses the family's copy of insns_flags as
a base, but needs to query the host and adjust it for a couple of
instruction groups that can be disabled from the hypervisor.

[Aside: we also update the cache sizes based on host information.
The reasons for that are complicated.  This particular case doesn't
require a late class init though, because although it's queried from
the host, it's not queried from kvm specifically]

> You can still keep pvr and svr on the CPU class. The only thing
> that's different on TYPE_HOST_POWERPC_CPU is that it won't have
> the preset values on PowerPCCPUClass. But it can initialize the
> instance values on ->instance_init() or ->realize() instead.
> 
> I even see ppc_cpu_class_by_pvr*() treating TYPE_HOST_POWERPC_CPU
> as special on PVR lookup, already. So it looks like the code is
> almost ready for that?

PVR is a red herring.  insn_flags is the real issue.

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

signature.asc
Description: PGP signature