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

[Michael Mueller]

On Wed, 6 May 2015 08:23:32 -0300
Eduardo Habkost <address@hidden> wrote:

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

right.

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

I don't disagree with that, both cases are to some extend confusing...
But the accelerator makes a big difference and a tended user should really be 
aware
of that.

Also that TCG is the default:

$ ./s390x-softmmu/qemu-system-s390x -cpu ?
s390             host

And I don't see a way to make a user belief that all the defined CPU models are 
available to a 
TCG user in the S390 case where most of the CPU facilities are not implemented.

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

I don't see how to solve this without cpu_desc_avail() or some other comparable 
mechanism, the
aliases e.g. are also dynamic...

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

No it should not but it takes the same parameters.

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

I think we have to take it into account because the available CPU models might
deviate substantially as in the case for S390 for KVM and TCG.
 
> 
> 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:

welcome

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

My take here is 2.1 because omitting option -machine is a decision to some
defaults for machine type and accelerator type already.  

>