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

[Eduardo Habkost]

On Wed, May 06, 2015 at 11:17:20AM +0200, Michael Mueller wrote:
> 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.

We don't (and shouldn't) have the qemu_read_config_file() requirement,
as CPU models are not loaded from config files anymore.

And we should be able to eliminate cpudef_init() completely, soon. I
think the only user of cpudef_init() is x86.

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

I think you are more likely to confuse users by not showing information
on "-cpu ?" when -machine is not present. I believe most people use
"-cpu ?" with no other arguments, to see what the QEMU binary is capable
of.

Anyway, whatever we decide to do, I believe we should start with
something simple to get things working, and after that we can look for
ways improve the help output with "runnable" info.

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

Exactly. I don't see a reason to not share code between
query-cpu-definitions and "-cpu ?".

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

I understand you are just delaying it. But it is requiring a hack inside
generic main() code that could be avoided. About the actual reasons to
delay it, see below.

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

About query-cpu-definitions: its code shouldn't depend on -machine
options to work. Maybe it can use current_machine to get the default
accel and machine argument, but it should always output the same data
for a machine+accel combination even if you use "-machine none".

About "-cpu ?": do we really want it to depend on -machine processing?
Today, help output shows what the QEMU binary is capable of, not just
what the host system and -machine option are capable of.

If we decide to change that assumption, let's do it in a generic way and
not as a arch-specific hack. The options I see are:

1) Continue with the current policy where "-cpu ?" does not depend on
   -machine arguments, and show all CPU models on "-cpu ?".
2) Deciding that, yes, it is OK to make "-cpu ?" depend on -machine
   arguments, and move the list_cpus() call after machine initialization
   inside generic main() code for all arches.
   2.1) We could delay the list_cpus() call inside main() on all cases.
   2.2) We could delay the list_cpus() call inside main() only if
        an explicit -machine option is present.

I prefer (1) and my second choice would be (2.2), but the main point is
that none of the options above require making s390 special and
introducing cpu_desc_avail().

-- 
Eduardo