On Wed, 6 May 2015 08:23:32 -0300
Eduardo Habkost <ehabk...@redhat.com> 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 <ehabk...@redhat.com> 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 <ehabk...@redhat.com> 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.
>
