Re: [Qemu-devel] [PATCH v6 01/17] Introduce stub routine cpu_desc_avail
On Wed, 6 May 2015 14:06:16 -0300
Eduardo Habkost wrote:
> On Wed, May 06, 2015 at 06:23:05PM +0200, Michael Mueller wrote:
> > On Wed, 6 May 2015 08:23:32 -0300
> > Eduardo Habkost wrote:
> [...]
> > > > > >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.
>
> Well, we could simply add a "KVM required" note (maybe just an asterisk beside
> the CPU model description). But maybe we have a reasonable alternative below:
>
> >
> > >
> > > 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...
>
> What bothers me in cpu_desc_avail() is that it depends on global state that is
> non-trivial (one needs to follow the whole KVM initialization path to find out
> if cpu_desc_avail() will be true or false).
>
> We could instead simply skip the cpu_list() call unconditionally on s390.
> e.g.:
>
> target-s390x/cpu.h:
> /* Delete the existing cpu_list macro */
>
> cpus.c:
> int list_cpus(FILE *f, fprintf_function cpu_fprintf, const char *optarg)
> {
> #if defined(cpu_list)
> cpu_list(f, cpu_fprintf);
> return 1;
> #else
> return 0;
> #endif
> }
>
> vl.c:
> if (cpu_model && is_help_option(cpu_model)) {
> /* zero list_cpus() return value means "-cpu ?" will be
> * handled later by machine initialization code */
> if (list_cpus(stdout, &fprintf, cpu_model)) {
> exit(0);
> }
> }
That approach is will do the job as well. I will prepare a patch for the next
version.
Thanks!
>
> [...]
> > >
> > > 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.
>
> That's true, on s390 the set of available CPU models is very different on both
> cases. It breaks assumptions in the existing "-cpu ?" handling code in main().
>
> >
> > >
> > > 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 -mach
Re: [Qemu-devel] [PATCH v6 01/17] Introduce stub routine cpu_desc_avail
On Wed, May 06, 2015 at 06:23:05PM +0200, Michael Mueller wrote:
> On Wed, 6 May 2015 08:23:32 -0300
> Eduardo Habkost wrote:
[...]
> > > > >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.
Well, we could simply add a "KVM required" note (maybe just an asterisk beside
the CPU model description). But maybe we have a reasonable alternative below:
>
> >
> > 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...
What bothers me in cpu_desc_avail() is that it depends on global state that is
non-trivial (one needs to follow the whole KVM initialization path to find out
if cpu_desc_avail() will be true or false).
We could instead simply skip the cpu_list() call unconditionally on s390. e.g.:
target-s390x/cpu.h:
/* Delete the existing cpu_list macro */
cpus.c:
int list_cpus(FILE *f, fprintf_function cpu_fprintf, const char *optarg)
{
#if defined(cpu_list)
cpu_list(f, cpu_fprintf);
return 1;
#else
return 0;
#endif
}
vl.c:
if (cpu_model && is_help_option(cpu_model)) {
/* zero list_cpus() return value means "-cpu ?" will be
* handled later by machine initialization code */
if (list_cpus(stdout, &fprintf, cpu_model)) {
exit(0);
}
}
[...]
> >
> > 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.
That's true, on s390 the set of available CPU models is very different on both
cases. It breaks assumptions in the existing "-cpu ?" handling code in main().
>
> >
> > 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.
The problem with 2.1 is that some machine init functions require that
additional command-line parameters are set and will abort (e.g. mips machines).
So we can't do that unconditionally for all architectures.
The proposal above is like 2.1, but conditional: it will delay "-cpu ?"
handling only on architectures that don't define cpu_
Re: [Qemu-devel] [PATCH v6 01/17] Introduce stub routine cpu_desc_avail
On Wed, 6 May 2015 08:23:32 -0300
Eduardo Habkost 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 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 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_mode
Re: [Qemu-devel] [PATCH v6 01/17] Introduce stub routine cpu_desc_avail
On Wed, May 06, 2015 at 11:17:20AM +0200, Michael Mueller wrote:
> On Tue, 5 May 2015 14:41:01 -0300
> Eduardo Habkost 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 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 requi
Re: [Qemu-devel] [PATCH v6 01/17] Introduce stub routine cpu_desc_avail
On Tue, 5 May 2015 14:41:01 -0300
Eduardo Habkost 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 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 ,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 99
Re: [Qemu-devel] [PATCH v6 01/17] Introduce stub routine cpu_desc_avail
On Tue, May 05, 2015 at 06:12:16PM +0200, Michael Mueller wrote:
> On Tue, 5 May 2015 10:55:47 -0300
> Eduardo Habkost 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.)
>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).
>
> 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().
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.
(In the future, we should be able to implement "-cpu ?" by simply
calling the query-cpu-definitions implementation.)
>
> >
> > 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 GA
Re: [Qemu-devel] [PATCH v6 01/17] Introduce stub routine cpu_desc_avail
On Tue, 5 May 2015 10:55:47 -0300
Eduardo Habkost 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()
*/
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.
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);
...
}
}
>
> 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)
> >
> > Signed-off-by: Michael Mueller
> > Reviewed-by: Thomas Huth
> > Acked-by: Christian Borntraeger
> > ---
> > include/qemu-common.h | 2 ++
> > stubs/Makefile.objs| 1 +
> > stubs/cpu-desc-avail.c | 6 ++
> > vl.c | 2 +-
> > 4 files changed, 10 insertions(+), 1 deletion(-)
> > create mode 100644 stubs/
Re: [Qemu-devel] [PATCH v6 01/17] Introduce stub routine cpu_desc_avail
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?
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.
>
> Signed-off-by: Michael Mueller
> Reviewed-by: Thomas Huth
> Acked-by: Christian Borntraeger
> ---
> include/qemu-common.h | 2 ++
> stubs/Makefile.objs| 1 +
> stubs/cpu-desc-avail.c | 6 ++
> vl.c | 2 +-
> 4 files changed, 10 insertions(+), 1 deletion(-)
> create mode 100644 stubs/cpu-desc-avail.c
>
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index 1b5cffb..386750f 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -484,4 +484,6 @@ int parse_debug_env(const char *name, int max, int
> initial);
>
> const char *qemu_ether_ntoa(const MACAddr *mac);
>
> +bool cpu_desc_avail(void);
> +
> #endif
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 8beff4c..dce9cd2 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -39,3 +39,4 @@ stub-obj-$(CONFIG_WIN32) += fd-register.o
> stub-obj-y += cpus.o
> stub-obj-y += kvm.o
> stub-obj-y += qmp_pc_dimm_device_list.o
> +stub-obj-y += cpu-desc-avail.o
> diff --git a/stubs/cpu-desc-avail.c b/stubs/cpu-desc-avail.c
> new file mode 100644
> index 000..0cd594e
> --- /dev/null
> +++ b/stubs/cpu-desc-avail.c
> @@ -0,0 +1,6 @@
> +#include "qemu-common.h"
> +
> +bool cpu_desc_avail(void)
> +{
> +return true;
> +}
> diff --git a/vl.c b/vl.c
> index 74c2681..c552561 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3820,7 +3820,7 @@ int main(int argc, char **argv, char **envp)
> */
> cpudef_init();
>
> -if (cpu_model && is_help_option(cpu_model)) {
> +if (cpu_model && cpu_desc_avail() && is_help_option(cpu_model)) {
> list_cpus(stdout, &fprintf, cpu_model);
> exit(0);
> }
> --
> 1.8.3.1
>
--
Eduardo
[Qemu-devel] [PATCH v6 01/17] Introduce stub routine cpu_desc_avail
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.
Signed-off-by: Michael Mueller
Reviewed-by: Thomas Huth
Acked-by: Christian Borntraeger
---
include/qemu-common.h | 2 ++
stubs/Makefile.objs| 1 +
stubs/cpu-desc-avail.c | 6 ++
vl.c | 2 +-
4 files changed, 10 insertions(+), 1 deletion(-)
create mode 100644 stubs/cpu-desc-avail.c
diff --git a/include/qemu-common.h b/include/qemu-common.h
index 1b5cffb..386750f 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -484,4 +484,6 @@ int parse_debug_env(const char *name, int max, int initial);
const char *qemu_ether_ntoa(const MACAddr *mac);
+bool cpu_desc_avail(void);
+
#endif
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 8beff4c..dce9cd2 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -39,3 +39,4 @@ stub-obj-$(CONFIG_WIN32) += fd-register.o
stub-obj-y += cpus.o
stub-obj-y += kvm.o
stub-obj-y += qmp_pc_dimm_device_list.o
+stub-obj-y += cpu-desc-avail.o
diff --git a/stubs/cpu-desc-avail.c b/stubs/cpu-desc-avail.c
new file mode 100644
index 000..0cd594e
--- /dev/null
+++ b/stubs/cpu-desc-avail.c
@@ -0,0 +1,6 @@
+#include "qemu-common.h"
+
+bool cpu_desc_avail(void)
+{
+return true;
+}
diff --git a/vl.c b/vl.c
index 74c2681..c552561 100644
--- a/vl.c
+++ b/vl.c
@@ -3820,7 +3820,7 @@ int main(int argc, char **argv, char **envp)
*/
cpudef_init();
-if (cpu_model && is_help_option(cpu_model)) {
+if (cpu_model && cpu_desc_avail() && is_help_option(cpu_model)) {
list_cpus(stdout, &fprintf, cpu_model);
exit(0);
}
--
1.8.3.1
