| Message ID | 1430146411-34632-2-git-send-email-mimu@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 <mimu@linux.vnet.ibm.com> > Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com> > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > 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 0000000..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 >
On Tue, 5 May 2015 10:55:47 -0300 Eduardo Habkost <ehabkost@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() */ 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 <mimu@linux.vnet.ibm.com> > > Reviewed-by: Thomas Huth <thuth@linux.vnet.ibm.com> > > Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> > > --- > > 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 0000000..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 > > >
On Tue, May 05, 2015 at 06:12:16PM +0200, Michael Mueller wrote: > On Tue, 5 May 2015 10:55:47 -0300 > Eduardo Habkost <ehabkost@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.) > 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 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) > > [...]
On Tue, 5 May 2015 14:41:01 -0300 Eduardo Habkost <ehabkost@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 <ehabkost@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. > > > 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) > > > > > [...] >
On Wed, May 06, 2015 at 11:17:20AM +0200, Michael Mueller wrote: > On Tue, 5 May 2015 14:41:01 -0300 > Eduardo Habkost <ehabkost@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 <ehabkost@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. > > > > > > 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().
On Wed, 6 May 2015 08:23:32 -0300 Eduardo Habkost <ehabkost@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 <ehabkost@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 <ehabkost@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. >
On Wed, May 06, 2015 at 06:23:05PM +0200, Michael Mueller wrote: > On Wed, 6 May 2015 08:23:32 -0300 > Eduardo Habkost <ehabkost@redhat.com> 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_list().
On Wed, 6 May 2015 14:06:16 -0300 Eduardo Habkost <ehabkost@redhat.com> 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 <ehabkost@redhat.com> 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 -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_list(). perfect. Michael >
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 0000000..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); }