Re: [Qemu-devel] [PATCH v6 01/17] Introduce stub routine cpu_desc_avail

[Michael Mueller]

On Tue, 5 May 2015 14:41:01 -0300
Eduardo Habkost <address@hidden> wrote:

> On Tue, May 05, 2015 at 06:12:16PM +0200, Michael Mueller wrote:
> > On Tue, 5 May 2015 10:55:47 -0300
> > Eduardo Habkost <address@hidden> wrote:
> > 
> > > On Mon, Apr 27, 2015 at 04:53:15PM +0200, Michael Mueller wrote:
> > > > This patch introduces the function cpu_desc_avail() which returns by
> > > > default true if not architecture specific implemented. Its intention
> > > > is to indicate if the cpu model description is available for display
> > > > by list_cpus(). This change allows cpu model descriptions to become
> > > > dynamically created by evaluating the runtime context instead of
> > > > putting static cpu model information at display.
> > > 
> > > Why are you deliberately breaking "-cpu ?" when cpu_desc_avail() is
> > > false?
> > 
> > In the s390x case cpu_desc_avail() is per se false in this code section of 
> > vl.c:
> > 
> >   /* Init CPU def lists, based on config                         
> >    * - Must be called after all the qemu_read_config_file() calls
> >    * - Must be called before list_cpu()
> >    * - Must be called before machine->init()
> >    */
> 
> (Side note: I believe the above outdated, I will send a patch to update
> it.)

Will be interesting to see what the change is, master is currently showing this 
code.

> 
> >    cpudef_init();
> > 
> >    if (cpu_model && cpu_desc_avail() && is_help_option(cpu_model)) {
> >        list_cpus(stdout, &fprintf, cpu_model);
> >        exit(0);
> >    }
> > 
> > That is because the output does not solely depend on static definitions
> > but also on runtime context. Here the host machine type this instance of
> > QEMU is running on, at least for the KVM case.
> 
> Is this a required feature? I would prefer to have the main() code
> simple even if it means not having runnable information in "-cpu ?" by
> now (about possible ways to implement this without cpu_desc_avail(), see
> below).

I think it is more than a desired feature because one might end up with a failed
CPU object instantiation although the help screen claims to CPU model to be 
valid. 

> 
> 
> > 
> > Once the accelerator has been initialized AND the S390 cpu classes have
> > been setup by means of the following code:
> > 
> > static void kvm_setup_cpu_classes(KVMState *s)
> > {
> >     S390MachineProps mach;
> > 
> >     if (!kvm_s390_get_machine_props(s, &mach)) {
> >         s390_setup_cpu_classes(ACCEL_CURRENT, &mach,
> >                                s390_current_fac_list_mask());
> >     s390_setup_cpu_aliases();
> >         cpu_classes_initialized = true;
> >     }
> > }
> > 
> > cpu_desc_avail() becomes true. In case the selceted mode was "?"
> > the list_cpu() is now done right before the cpu model is used as part
> > of the cpu initialization (hw/s390-virtio.c):
> > 
> > void s390_init_cpus(const char *cpu_model, uint8_t *storage_keys)
> > {
> >     int i;
> > 
> >     if (cpu_model == NULL) {
> >         cpu_model = "none";
> >     }
> > 
> >     if (is_help_option(cpu_model)) {
> >         list_cpus(stdout, &fprintf, cpu_model);
> >         exit(0);
> >     }
> > 
> >     ...
> >     for (i = 0; i < smp_cpus; i++) {
> >         ...
> >         cpu = cpu_s390x_init(cpu_model);
> >         ...
> >     }
> > }
> 
> 
> In other words, you just need to ensure that s390_cpu_list() run after
> kvm_setup_cpu_classes().

... which is part of the KVM/accel init process but it could of course make use
of the ACCEL_TMP use case as query-cpu-definitions does.

> 
> Can't you simply call s390_setup_cpu_classes(ACCEL_TEMP) inside
> s390_init_cpus(), just like arch_query_cpu_definitions()? You could even
> share code between both functions.

That would not help with the current placement of list_cpus() in main() as it 
happens
way to early. Not s390_init_cpus() is the issue, the context information has 
been
processed already at that time. Currently I just kind of delay the list_cpus() 
until
all required information is available.

> 
> (In the future, we should be able to implement "-cpu ?" by simply
> calling the query-cpu-definitions implementation.)

Right but the -machine <name>,accel=<accel> options have to be processed 
already.

> 
> > 
> > > 
> > > What exactly could cause cpu_desc_avail() to be false? If CPU model
> > > information is not yet available when cpu_list() is called, it is a bug.
> > > 
> > 
> > Here an example output that shows only runnable cpu models:
> > 
> > $ ./s390x-softmmu/qemu-system-s390x -machine s390,accel=kvm -cpu ?
> > s390 none       
> > s390 2064-ga1   IBM zSeries 900 GA1
> > s390 2064-ga2   IBM zSeries 900 GA2
> > s390 2064-ga3   IBM zSeries 900 GA3
> > s390 2064       (alias for 2064-ga3)
> > s390 z900       (alias for 2064-ga3)
> > s390 2066-ga1   IBM zSeries 800 GA1
> > s390 2066       (alias for 2066-ga1)
> > s390 z800       (alias for 2066-ga1)
> > s390 2084-ga1   IBM zSeries 990 GA1
> > s390 2084-ga2   IBM zSeries 990 GA2
> > s390 2084-ga3   IBM zSeries 990 GA3
> > s390 2084-ga4   IBM zSeries 990 GA4
> > s390 2084-ga5   IBM zSeries 990 GA5
> > s390 2084       (alias for 2084-ga5)
> > s390 z990       (alias for 2084-ga5)
> > s390 2086-ga1   IBM zSeries 890 GA1
> > s390 2086-ga2   IBM zSeries 890 GA2
> > s390 2086-ga3   IBM zSeries 890 GA3
> > s390 2086       (alias for 2086-ga3)
> > s390 z890       (alias for 2086-ga3)
> > s390 2094-ga1   IBM System z9 EC GA1
> > s390 z9-109     (alias for 2094-ga1)
> > s390 2094-ga2   IBM System z9 EC GA2
> > s390 2094-ga3   IBM System z9 EC GA3
> > s390 2094       (alias for 2094-ga3)
> > s390 z9         (alias for 2094-ga3)
> > s390 z9-ec      (alias for 2094-ga3)
> > s390 2096-ga1   IBM System z9 BC GA1
> > s390 2096-ga2   IBM System z9 BC GA2
> > s390 2096       (alias for 2096-ga2)
> > s390 z9-bc      (alias for 2096-ga2)
> > s390 2097-ga1   IBM System z10 EC GA1
> > s390 2097-ga2   IBM System z10 EC GA2
> > s390 2097-ga3   IBM System z10 EC GA3
> > s390 2097       (alias for 2097-ga3)
> > s390 z10        (alias for 2097-ga3)
> > s390 z10-ec     (alias for 2097-ga3)
> > s390 2098-ga1   IBM System z10 BC GA1
> > s390 2098-ga2   IBM System z10 BC GA2
> > s390 2098       (alias for 2098-ga2)
> > s390 z10-bc     (alias for 2098-ga2)
> > s390 2817-ga1   IBM zEnterprise 196 GA1
> > s390 2817-ga2   IBM zEnterprise 196 GA2
> > s390 2817       (alias for 2817-ga2)
> > s390 z196       (alias for 2817-ga2)
> > s390 2818-ga1   IBM zEnterprise 114 GA1
> > s390 2818       (alias for 2818-ga1)
> > s390 z114       (alias for 2818-ga1)
> > s390 2827-ga1   IBM zEnterprise EC12 GA1
> > s390 2827-ga2   IBM zEnterprise EC12 GA2
> > s390 2827       (alias for 2827-ga2)
> > s390 zEC12      (alias for 2827-ga2)
> > s390 host       (alias for 2827-ga2)
> > s390 2828-ga1   IBM zEnterprise BC12 GA1
> > s390 2828       (alias for 2828-ga1)
> > s390 zBC12      (alias for 2828-ga1)
> > 
> > 
> [...]
>