[Qemu-devel] [PATCH 0/2] ppc: Fix problems with duplicated CPU family types

[Qemu-devel] [PATCH 0/2] ppc: Fix problems with duplicated CPU family types

[Thomas Huth]

The following two patches fix two rather cosmetic than severe issues
that we currently have with registering the CPU "family" types:

1) In the 'query-cpu-definitions' QMP call, there are currently two
   conflicting "POWER8" entries in the results list.

2) When running QEMU with "-cpu ?" on POWER8E or POWER8NVL, you always
   get a wrong and confusing entry for the "POWER8" alias.

Please see the description of the first patch for more details.

Thomas Huth (2):
  ppc/kvm: Handle the "family" CPU via alias instead of registering new
    types
  vl: Print CPU help after we've registered the CPU accelerators

 target/ppc/kvm.c | 36 +++++++++++++++++++++++-------------
 vl.c             | 10 +++++-----
 2 files changed, 28 insertions(+), 18 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] ppc/kvm: Handle the "family" CPU via alias instead of registering new types

[Thomas Huth]

When running with KVM on POWER, we are registering a "family" CPU
type for the host CPU that we are running on. For example, on all
POWER8-compatible hosts, we register a "POWER8" CPU type, so that
you can always start QEMU with "-cpu POWER8" there, without the
need to know whether you are running on a POWER8, POWER8E or POWER8NVL
host machine.
However, we also have a "POWER8" CPU alias in the ppc_cpu_aliases list
(that is mainly useful for TCG). This leads to two cosmetical drawbacks:
If the user runs QEMU with "-cpu ?", we always claim that POWER8 is an
"alias for POWER8_v2.0" - which is simply not true when running with
KVM on POWER. And when using the 'query-cpu-definitions' QMP call,
there are currently two entries for "POWER8", one for the alias, and
one for the additional registered type.
To solve the two problems, we should rather update the "family" alias
instead of registering a new types. We then only have one "POWER8"
CPU definition around, an alias, which also points to the right
destination.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1396536
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 target/ppc/kvm.c | 36 +++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index ec92c64..f58c260 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -24,6 +24,7 @@
 #include "qemu-common.h"
 #include "qemu/error-report.h"
 #include "cpu.h"
+#include "cpu-models.h"
 #include "qemu/timer.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/hw_accel.h"
@@ -2412,6 +2413,7 @@ static int kvm_ppc_register_host_cpu_type(void)
     };
     PowerPCCPUClass *pvr_pcc;
     DeviceClass *dc;
+    int i;
 
     pvr_pcc = kvm_ppc_get_host_cpu_class();
     if (pvr_pcc == NULL) {
@@ -2420,13 +2422,6 @@ static int kvm_ppc_register_host_cpu_type(void)
     type_info.parent = object_class_get_name(OBJECT_CLASS(pvr_pcc));
     type_register(&type_info);
 
-    /* Register generic family CPU class for a family */
-    pvr_pcc = ppc_cpu_get_family_class(pvr_pcc);
-    dc = DEVICE_CLASS(pvr_pcc);
-    type_info.parent = object_class_get_name(OBJECT_CLASS(pvr_pcc));
-    type_info.name = g_strdup_printf("%s-"TYPE_POWERPC_CPU, dc->desc);
-    type_register(&type_info);
-
 #if defined(TARGET_PPC64)
     type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, "host");
     type_info.parent = TYPE_SPAPR_CPU_CORE,
@@ -2436,14 +2431,29 @@ static int kvm_ppc_register_host_cpu_type(void)
     type_info.class_data = (void *) "host";
     type_register(&type_info);
     g_free((void *)type_info.name);
-
-    /* Register generic spapr CPU family class for current host CPU type */
-    type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, dc->desc);
-    type_info.class_data = (void *) dc->desc;
-    type_register(&type_info);
-    g_free((void *)type_info.name);
 #endif
 
+    /*
+     * Update generic CPU family class alias (e.g. on a POWER8NVL host,
+     * we want "POWER8" to be a "family" alias that points to the current
+     * host CPU type, too)
+     */
+    dc = DEVICE_CLASS(ppc_cpu_get_family_class(pvr_pcc));
+    for (i = 0; ppc_cpu_aliases[i].alias != NULL; i++) {
+        if (strcmp(ppc_cpu_aliases[i].alias, dc->desc) == 0) {
+            ObjectClass *oc = OBJECT_CLASS(pvr_pcc);
+            char *suffix;
+
+            ppc_cpu_aliases[i].model = g_strdup(object_class_get_name(oc));
+            suffix = strstr(ppc_cpu_aliases[i].model, "-"TYPE_POWERPC_CPU);
+            if (suffix) {
+                *suffix = 0;
+            }
+            ppc_cpu_aliases[i].oc = oc;
+            break;
+        }
+    }
+
     return 0;
 }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/2] ppc/kvm: Handle the "family" CPU via alias instead of registering new types

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4468 bytes --]

On Tue, Jan 31, 2017 at 02:11:58PM +0100, Thomas Huth wrote:
> When running with KVM on POWER, we are registering a "family" CPU
> type for the host CPU that we are running on. For example, on all
> POWER8-compatible hosts, we register a "POWER8" CPU type, so that
> you can always start QEMU with "-cpu POWER8" there, without the
> need to know whether you are running on a POWER8, POWER8E or POWER8NVL
> host machine.
> However, we also have a "POWER8" CPU alias in the ppc_cpu_aliases list
> (that is mainly useful for TCG). This leads to two cosmetical drawbacks:
> If the user runs QEMU with "-cpu ?", we always claim that POWER8 is an
> "alias for POWER8_v2.0" - which is simply not true when running with
> KVM on POWER. And when using the 'query-cpu-definitions' QMP call,
> there are currently two entries for "POWER8", one for the alias, and
> one for the additional registered type.
> To solve the two problems, we should rather update the "family" alias
> instead of registering a new types. We then only have one "POWER8"
> CPU definition around, an alias, which also points to the right
> destination.
> 
> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1396536
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

Updating the otherwise static table of aliases is kind of ugly, but
then so is registering an extra full type as we do now.

Is this safe to apply without the follow up patch to vl.c.

> ---
>  target/ppc/kvm.c | 36 +++++++++++++++++++++++-------------
>  1 file changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index ec92c64..f58c260 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -24,6 +24,7 @@
>  #include "qemu-common.h"
>  #include "qemu/error-report.h"
>  #include "cpu.h"
> +#include "cpu-models.h"
>  #include "qemu/timer.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/hw_accel.h"
> @@ -2412,6 +2413,7 @@ static int kvm_ppc_register_host_cpu_type(void)
>      };
>      PowerPCCPUClass *pvr_pcc;
>      DeviceClass *dc;
> +    int i;
>  
>      pvr_pcc = kvm_ppc_get_host_cpu_class();
>      if (pvr_pcc == NULL) {
> @@ -2420,13 +2422,6 @@ static int kvm_ppc_register_host_cpu_type(void)
>      type_info.parent = object_class_get_name(OBJECT_CLASS(pvr_pcc));
>      type_register(&type_info);
>  
> -    /* Register generic family CPU class for a family */
> -    pvr_pcc = ppc_cpu_get_family_class(pvr_pcc);
> -    dc = DEVICE_CLASS(pvr_pcc);
> -    type_info.parent = object_class_get_name(OBJECT_CLASS(pvr_pcc));
> -    type_info.name = g_strdup_printf("%s-"TYPE_POWERPC_CPU, dc->desc);
> -    type_register(&type_info);
> -
>  #if defined(TARGET_PPC64)
>      type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, "host");
>      type_info.parent = TYPE_SPAPR_CPU_CORE,
> @@ -2436,14 +2431,29 @@ static int kvm_ppc_register_host_cpu_type(void)
>      type_info.class_data = (void *) "host";
>      type_register(&type_info);
>      g_free((void *)type_info.name);
> -
> -    /* Register generic spapr CPU family class for current host CPU type */
> -    type_info.name = g_strdup_printf("%s-"TYPE_SPAPR_CPU_CORE, dc->desc);
> -    type_info.class_data = (void *) dc->desc;
> -    type_register(&type_info);
> -    g_free((void *)type_info.name);
>  #endif
>  
> +    /*
> +     * Update generic CPU family class alias (e.g. on a POWER8NVL host,
> +     * we want "POWER8" to be a "family" alias that points to the current
> +     * host CPU type, too)
> +     */
> +    dc = DEVICE_CLASS(ppc_cpu_get_family_class(pvr_pcc));
> +    for (i = 0; ppc_cpu_aliases[i].alias != NULL; i++) {
> +        if (strcmp(ppc_cpu_aliases[i].alias, dc->desc) == 0) {
> +            ObjectClass *oc = OBJECT_CLASS(pvr_pcc);
> +            char *suffix;
> +
> +            ppc_cpu_aliases[i].model = g_strdup(object_class_get_name(oc));
> +            suffix = strstr(ppc_cpu_aliases[i].model, "-"TYPE_POWERPC_CPU);
> +            if (suffix) {
> +                *suffix = 0;
> +            }
> +            ppc_cpu_aliases[i].oc = oc;
> +            break;
> +        }
> +    }
> +
>      return 0;
>  }
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 1/2] ppc/kvm: Handle the "family" CPU via alias instead of registering new types

[Thomas Huth]

[-- Attachment #1: Type: text/plain, Size: 1732 bytes --]

On 01.02.2017 01:10, David Gibson wrote:
> On Tue, Jan 31, 2017 at 02:11:58PM +0100, Thomas Huth wrote:
>> When running with KVM on POWER, we are registering a "family" CPU
>> type for the host CPU that we are running on. For example, on all
>> POWER8-compatible hosts, we register a "POWER8" CPU type, so that
>> you can always start QEMU with "-cpu POWER8" there, without the
>> need to know whether you are running on a POWER8, POWER8E or POWER8NVL
>> host machine.
>> However, we also have a "POWER8" CPU alias in the ppc_cpu_aliases list
>> (that is mainly useful for TCG). This leads to two cosmetical drawbacks:
>> If the user runs QEMU with "-cpu ?", we always claim that POWER8 is an
>> "alias for POWER8_v2.0" - which is simply not true when running with
>> KVM on POWER. And when using the 'query-cpu-definitions' QMP call,
>> there are currently two entries for "POWER8", one for the alias, and
>> one for the additional registered type.
>> To solve the two problems, we should rather update the "family" alias
>> instead of registering a new types. We then only have one "POWER8"
>> CPU definition around, an alias, which also points to the right
>> destination.
>>
>> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1396536
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> Updating the otherwise static table of aliases is kind of ugly, but
> then so is registering an extra full type as we do now.
> 
> Is this safe to apply without the follow up patch to vl.c.

Yes. It fixes the problem with "query-cpu-definitions" already. You just
need the other patch to get the output of "-cpu ?" right, too.

 Thomas



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [Qemu-devel] [PATCH 1/2] ppc/kvm: Handle the "family" CPU via alias instead of registering new types

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2074 bytes --]

On Wed, Feb 01, 2017 at 08:39:47AM +0100, Thomas Huth wrote:
> On 01.02.2017 01:10, David Gibson wrote:
> > On Tue, Jan 31, 2017 at 02:11:58PM +0100, Thomas Huth wrote:
> >> When running with KVM on POWER, we are registering a "family" CPU
> >> type for the host CPU that we are running on. For example, on all
> >> POWER8-compatible hosts, we register a "POWER8" CPU type, so that
> >> you can always start QEMU with "-cpu POWER8" there, without the
> >> need to know whether you are running on a POWER8, POWER8E or POWER8NVL
> >> host machine.
> >> However, we also have a "POWER8" CPU alias in the ppc_cpu_aliases list
> >> (that is mainly useful for TCG). This leads to two cosmetical drawbacks:
> >> If the user runs QEMU with "-cpu ?", we always claim that POWER8 is an
> >> "alias for POWER8_v2.0" - which is simply not true when running with
> >> KVM on POWER. And when using the 'query-cpu-definitions' QMP call,
> >> there are currently two entries for "POWER8", one for the alias, and
> >> one for the additional registered type.
> >> To solve the two problems, we should rather update the "family" alias
> >> instead of registering a new types. We then only have one "POWER8"
> >> CPU definition around, an alias, which also points to the right
> >> destination.
> >>
> >> Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1396536
> >> Signed-off-by: Thomas Huth <thuth@redhat.com>
> > 
> > Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> > 
> > Updating the otherwise static table of aliases is kind of ugly, but
> > then so is registering an extra full type as we do now.
> > 
> > Is this safe to apply without the follow up patch to vl.c.
> 
> Yes. It fixes the problem with "query-cpu-definitions" already. You just
> need the other patch to get the output of "-cpu ?" right, too.

Great.  Applied to ppc-for-2.9.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[Qemu-devel] [PATCH 2/2] vl: Print CPU help after we've registered the CPU accelerators

[Thomas Huth]

When running with KVM on POWER, we register some CPU types during
the initialization function of the ppc64 KVM code (which unfortunately
also can not be done via a type_init() like it is done on x86). So to
be able to see these updates in the CPU help text, the code that calls
list_cpus() has to be run after configure_accelerator(). This move should
be fine since the "cpu_model" variable is also never used before the call
to configure_accelerator(), and thus there should not be any unwanted
side effects in the code before configure_accelerator() if the user
started QEMU with "-cpu ?" or "-cpu help".

Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 vl.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/vl.c b/vl.c
index 0b72b12..315c5c3 100644
--- a/vl.c
+++ b/vl.c
@@ -4055,11 +4055,6 @@ int main(int argc, char **argv, char **envp)
         qemu_set_hw_version(machine_class->hw_version);
     }
 
-    if (cpu_model && is_help_option(cpu_model)) {
-        list_cpus(stdout, &fprintf, cpu_model);
-        exit(0);
-    }
-
     if (!trace_init_backends()) {
         exit(1);
     }
@@ -4298,6 +4293,11 @@ int main(int argc, char **argv, char **envp)
 
     configure_accelerator(current_machine);
 
+    if (cpu_model && is_help_option(cpu_model)) {
+        list_cpus(stdout, &fprintf, cpu_model);
+        exit(0);
+    }
+
     if (qtest_chrdev) {
         qtest_init(qtest_chrdev, qtest_log, &error_fatal);
     }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/2] vl: Print CPU help after we've registered the CPU accelerators

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2072 bytes --]

On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote:
> When running with KVM on POWER, we register some CPU types during
> the initialization function of the ppc64 KVM code (which unfortunately
> also can not be done via a type_init() like it is done on x86). So to
> be able to see these updates in the CPU help text, the code that calls
> list_cpus() has to be run after configure_accelerator(). This move should
> be fine since the "cpu_model" variable is also never used before the call
> to configure_accelerator(), and thus there should not be any unwanted
> side effects in the code before configure_accelerator() if the user
> started QEMU with "-cpu ?" or "-cpu help".
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

This looks fine to me, but it's not within my area to apply.  Paolo,
if you want to ack I'm happy to take it through my tree if that's
convenient for you.

> ---
>  vl.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index 0b72b12..315c5c3 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4055,11 +4055,6 @@ int main(int argc, char **argv, char **envp)
>          qemu_set_hw_version(machine_class->hw_version);
>      }
>  
> -    if (cpu_model && is_help_option(cpu_model)) {
> -        list_cpus(stdout, &fprintf, cpu_model);
> -        exit(0);
> -    }
> -
>      if (!trace_init_backends()) {
>          exit(1);
>      }
> @@ -4298,6 +4293,11 @@ int main(int argc, char **argv, char **envp)
>  
>      configure_accelerator(current_machine);
>  
> +    if (cpu_model && is_help_option(cpu_model)) {
> +        list_cpus(stdout, &fprintf, cpu_model);
> +        exit(0);
> +    }
> +
>      if (qtest_chrdev) {
>          qtest_init(qtest_chrdev, qtest_log, &error_fatal);
>      }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 2/2] vl: Print CPU help after we've registered the CPU accelerators

[Paolo Bonzini]

On 31/01/2017 16:08, David Gibson wrote:
> On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote:
>> When running with KVM on POWER, we register some CPU types during
>> the initialization function of the ppc64 KVM code (which unfortunately
>> also can not be done via a type_init() like it is done on x86). So to
>> be able to see these updates in the CPU help text, the code that calls
>> list_cpus() has to be run after configure_accelerator(). This move should
>> be fine since the "cpu_model" variable is also never used before the call
>> to configure_accelerator(), and thus there should not be any unwanted
>> side effects in the code before configure_accelerator() if the user
>> started QEMU with "-cpu ?" or "-cpu help".
>>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> 
> This looks fine to me, but it's not within my area to apply.  Paolo,
> if you want to ack I'm happy to take it through my tree if that's
> convenient for you.
> 
>> ---
>>  vl.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/vl.c b/vl.c
>> index 0b72b12..315c5c3 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -4055,11 +4055,6 @@ int main(int argc, char **argv, char **envp)
>>          qemu_set_hw_version(machine_class->hw_version);
>>      }
>>  
>> -    if (cpu_model && is_help_option(cpu_model)) {
>> -        list_cpus(stdout, &fprintf, cpu_model);
>> -        exit(0);
>> -    }
>> -
>>      if (!trace_init_backends()) {
>>          exit(1);
>>      }
>> @@ -4298,6 +4293,11 @@ int main(int argc, char **argv, char **envp)
>>  
>>      configure_accelerator(current_machine);
>>  
>> +    if (cpu_model && is_help_option(cpu_model)) {
>> +        list_cpus(stdout, &fprintf, cpu_model);
>> +        exit(0);
>> +    }
>> +
>>      if (qtest_chrdev) {
>>          qtest_init(qtest_chrdev, qtest_log, &error_fatal);
>>      }
> 

Eduardo is the one who did most of the accel and CPU definitions work.

Paolo

Re: [Qemu-devel] [PATCH 2/2] vl: Print CPU help after we've registered the CPU accelerators

[Eduardo Habkost]

On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote:
> When running with KVM on POWER, we register some CPU types during
> the initialization function of the ppc64 KVM code (which unfortunately
> also can not be done via a type_init() like it is done on x86).

Can you elaborate why it can't be done via type_init()? If the
QOM type hierarchy depends on any runtime data unavailable at
type_init(), we should fix that.


>                                                                 So to
> be able to see these updates in the CPU help text, the code that calls
> list_cpus() has to be run after configure_accelerator(). This move should
> be fine since the "cpu_model" variable is also never used before the call
> to configure_accelerator(), and thus there should not be any unwanted
> side effects in the code before configure_accelerator() if the user
> started QEMU with "-cpu ?" or "-cpu help".
> 
> Signed-off-by: Thomas Huth <thuth@redhat.com>

I am not convinced that the output of "-cpu help" and
"-cpu help -machine accel=kvm" should look different. Do you have
an example of what exactly is wrong with the output currently?

I actually believe list_cpus() needs to be called _earlier_, not
later. Otherwise we won't be able to fix this bug:

  $ qemu-system-arm -cpu help
  qemu-system-arm:/usr/local/etc/qemu/qemu.conf:1: No machine specified, and there is no default
  Use -machine help to list supported machines
  $ 

> ---
>  vl.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index 0b72b12..315c5c3 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4055,11 +4055,6 @@ int main(int argc, char **argv, char **envp)
>          qemu_set_hw_version(machine_class->hw_version);
>      }
>  
> -    if (cpu_model && is_help_option(cpu_model)) {
> -        list_cpus(stdout, &fprintf, cpu_model);
> -        exit(0);
> -    }
> -
>      if (!trace_init_backends()) {
>          exit(1);
>      }
> @@ -4298,6 +4293,11 @@ int main(int argc, char **argv, char **envp)
>  
>      configure_accelerator(current_machine);
>  
> +    if (cpu_model && is_help_option(cpu_model)) {
> +        list_cpus(stdout, &fprintf, cpu_model);
> +        exit(0);
> +    }
> +
>      if (qtest_chrdev) {
>          qtest_init(qtest_chrdev, qtest_log, &error_fatal);
>      }
> -- 
> 1.8.3.1
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/2] vl: Print CPU help after we've registered the CPU accelerators

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3081 bytes --]

On Fri, Mar 03, 2017 at 11:58:07AM -0300, Eduardo Habkost wrote:
> On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote:
> > When running with KVM on POWER, we register some CPU types during
> > the initialization function of the ppc64 KVM code (which unfortunately
> > also can not be done via a type_init() like it is done on x86).
> 
> Can you elaborate why it can't be done via type_init()? If the
> QOM type hierarchy depends on any runtime data unavailable at
> type_init(), we should fix that.

Hmm.. how?  This is specifically for the special 'host' cpu in the
case of KVM.  We can't use a static configuration here, because there
are things on the host that could limit what features of the CPU are
available for guest use.  So, we need KVM to be initialized in order
to query that information.

> >                                                                 So to
> > be able to see these updates in the CPU help text, the code that calls
> > list_cpus() has to be run after configure_accelerator(). This move should
> > be fine since the "cpu_model" variable is also never used before the call
> > to configure_accelerator(), and thus there should not be any unwanted
> > side effects in the code before configure_accelerator() if the user
> > started QEMU with "-cpu ?" or "-cpu help".
> > 
> > Signed-off-by: Thomas Huth <thuth@redhat.com>
> 
> I am not convinced that the output of "-cpu help" and
> "-cpu help -machine accel=kvm" should look different. Do you have
> an example of what exactly is wrong with the output currently?
> 
> I actually believe list_cpus() needs to be called _earlier_, not
> later. Otherwise we won't be able to fix this bug:
> 
>   $ qemu-system-arm -cpu help
>   qemu-system-arm:/usr/local/etc/qemu/qemu.conf:1: No machine specified, and there is no default
>   Use -machine help to list supported machines
>   $ 
> 
> > ---
> >  vl.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/vl.c b/vl.c
> > index 0b72b12..315c5c3 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -4055,11 +4055,6 @@ int main(int argc, char **argv, char **envp)
> >          qemu_set_hw_version(machine_class->hw_version);
> >      }
> >  
> > -    if (cpu_model && is_help_option(cpu_model)) {
> > -        list_cpus(stdout, &fprintf, cpu_model);
> > -        exit(0);
> > -    }
> > -
> >      if (!trace_init_backends()) {
> >          exit(1);
> >      }
> > @@ -4298,6 +4293,11 @@ int main(int argc, char **argv, char **envp)
> >  
> >      configure_accelerator(current_machine);
> >  
> > +    if (cpu_model && is_help_option(cpu_model)) {
> > +        list_cpus(stdout, &fprintf, cpu_model);
> > +        exit(0);
> > +    }
> > +
> >      if (qtest_chrdev) {
> >          qtest_init(qtest_chrdev, qtest_log, &error_fatal);
> >      }
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 2/2] vl: Print CPU help after we've registered the CPU accelerators

[Eduardo Habkost]

On Mon, Mar 06, 2017 at 10:06:27AM +1100, David Gibson wrote:
> On Fri, Mar 03, 2017 at 11:58:07AM -0300, Eduardo Habkost wrote:
> > On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote:
> > > When running with KVM on POWER, we register some CPU types during
> > > the initialization function of the ppc64 KVM code (which unfortunately
> > > also can not be done via a type_init() like it is done on x86).
> > 
> > Can you elaborate why it can't be done via type_init()? If the
> > QOM type hierarchy depends on any runtime data unavailable at
> > type_init(), we should fix that.
> 
> Hmm.. how?  This is specifically for the special 'host' cpu in the
> case of KVM.  We can't use a static configuration here, because there
> are things on the host that could limit what features of the CPU are
> available for guest use.  So, we need KVM to be initialized in order
> to query that information.

There's nothing saying you need to query that information before
type_register() or during class_init, does it? The behavior of
TYPE_HOST_POWERPC_CPU after object_new() is called can be as
dynamic as you want it to, but the QOM type hierarchy is supposed
to be static.

Registering QOM types outside type_init() is only causing us
problems and we should stop doing that.

> 
> > >                                                                 So to
> > > be able to see these updates in the CPU help text, the code that calls
> > > list_cpus() has to be run after configure_accelerator(). This move should
> > > be fine since the "cpu_model" variable is also never used before the call
> > > to configure_accelerator(), and thus there should not be any unwanted
> > > side effects in the code before configure_accelerator() if the user
> > > started QEMU with "-cpu ?" or "-cpu help".
> > > 
> > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > 
> > I am not convinced that the output of "-cpu help" and
> > "-cpu help -machine accel=kvm" should look different. Do you have
> > an example of what exactly is wrong with the output currently?
> > 
> > I actually believe list_cpus() needs to be called _earlier_, not
> > later. Otherwise we won't be able to fix this bug:
> > 
> >   $ qemu-system-arm -cpu help
> >   qemu-system-arm:/usr/local/etc/qemu/qemu.conf:1: No machine specified, and there is no default
> >   Use -machine help to list supported machines
> >   $ 
> > 
> > > ---
> > >  vl.c | 10 +++++-----
> > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/vl.c b/vl.c
> > > index 0b72b12..315c5c3 100644
> > > --- a/vl.c
> > > +++ b/vl.c
> > > @@ -4055,11 +4055,6 @@ int main(int argc, char **argv, char **envp)
> > >          qemu_set_hw_version(machine_class->hw_version);
> > >      }
> > >  
> > > -    if (cpu_model && is_help_option(cpu_model)) {
> > > -        list_cpus(stdout, &fprintf, cpu_model);
> > > -        exit(0);
> > > -    }
> > > -
> > >      if (!trace_init_backends()) {
> > >          exit(1);
> > >      }
> > > @@ -4298,6 +4293,11 @@ int main(int argc, char **argv, char **envp)
> > >  
> > >      configure_accelerator(current_machine);
> > >  
> > > +    if (cpu_model && is_help_option(cpu_model)) {
> > > +        list_cpus(stdout, &fprintf, cpu_model);
> > > +        exit(0);
> > > +    }
> > > +
> > >      if (qtest_chrdev) {
> > >          qtest_init(qtest_chrdev, qtest_log, &error_fatal);
> > >      }
> > 
> 
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson



-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/2] vl: Print CPU help after we've registered the CPU accelerators

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4180 bytes --]

On Mon, Mar 06, 2017 at 08:47:52AM -0300, Eduardo Habkost wrote:
> On Mon, Mar 06, 2017 at 10:06:27AM +1100, David Gibson wrote:
> > On Fri, Mar 03, 2017 at 11:58:07AM -0300, Eduardo Habkost wrote:
> > > On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote:
> > > > When running with KVM on POWER, we register some CPU types during
> > > > the initialization function of the ppc64 KVM code (which unfortunately
> > > > also can not be done via a type_init() like it is done on x86).
> > > 
> > > Can you elaborate why it can't be done via type_init()? If the
> > > QOM type hierarchy depends on any runtime data unavailable at
> > > type_init(), we should fix that.
> > 
> > Hmm.. how?  This is specifically for the special 'host' cpu in the
> > case of KVM.  We can't use a static configuration here, because there
> > are things on the host that could limit what features of the CPU are
> > available for guest use.  So, we need KVM to be initialized in order
> > to query that information.
> 
> There's nothing saying you need to query that information before
> type_register() or during class_init, does it? The behavior of
> TYPE_HOST_POWERPC_CPU after object_new() is called can be as
> dynamic as you want it to, but the QOM type hierarchy is supposed
> to be static.

So, the thing is we have some properties that logically belong in the
CPU class, rather than instance, and that's correct for all TCG CPUs,
but not for the KVM host CPU.  It seems nasty to have to force all
those things into the instance just because of KVM.

> Registering QOM types outside type_init() is only causing us
> problems and we should stop doing that.
> 
> > 
> > > >                                                                 So to
> > > > be able to see these updates in the CPU help text, the code that calls
> > > > list_cpus() has to be run after configure_accelerator(). This move should
> > > > be fine since the "cpu_model" variable is also never used before the call
> > > > to configure_accelerator(), and thus there should not be any unwanted
> > > > side effects in the code before configure_accelerator() if the user
> > > > started QEMU with "-cpu ?" or "-cpu help".
> > > > 
> > > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > 
> > > I am not convinced that the output of "-cpu help" and
> > > "-cpu help -machine accel=kvm" should look different. Do you have
> > > an example of what exactly is wrong with the output currently?
> > > 
> > > I actually believe list_cpus() needs to be called _earlier_, not
> > > later. Otherwise we won't be able to fix this bug:
> > > 
> > >   $ qemu-system-arm -cpu help
> > >   qemu-system-arm:/usr/local/etc/qemu/qemu.conf:1: No machine specified, and there is no default
> > >   Use -machine help to list supported machines
> > >   $ 
> > > 
> > > > ---
> > > >  vl.c | 10 +++++-----
> > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/vl.c b/vl.c
> > > > index 0b72b12..315c5c3 100644
> > > > --- a/vl.c
> > > > +++ b/vl.c
> > > > @@ -4055,11 +4055,6 @@ int main(int argc, char **argv, char **envp)
> > > >          qemu_set_hw_version(machine_class->hw_version);
> > > >      }
> > > >  
> > > > -    if (cpu_model && is_help_option(cpu_model)) {
> > > > -        list_cpus(stdout, &fprintf, cpu_model);
> > > > -        exit(0);
> > > > -    }
> > > > -
> > > >      if (!trace_init_backends()) {
> > > >          exit(1);
> > > >      }
> > > > @@ -4298,6 +4293,11 @@ int main(int argc, char **argv, char **envp)
> > > >  
> > > >      configure_accelerator(current_machine);
> > > >  
> > > > +    if (cpu_model && is_help_option(cpu_model)) {
> > > > +        list_cpus(stdout, &fprintf, cpu_model);
> > > > +        exit(0);
> > > > +    }
> > > > +
> > > >      if (qtest_chrdev) {
> > > >          qtest_init(qtest_chrdev, qtest_log, &error_fatal);
> > > >      }
> > > 
> > 
> 
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 2/2] vl: Print CPU help after we've registered the CPU accelerators

[Eduardo Habkost]

On Tue, Mar 07, 2017 at 02:31:05PM +1100, David Gibson wrote:
> On Mon, Mar 06, 2017 at 08:47:52AM -0300, Eduardo Habkost wrote:
> > On Mon, Mar 06, 2017 at 10:06:27AM +1100, David Gibson wrote:
> > > On Fri, Mar 03, 2017 at 11:58:07AM -0300, Eduardo Habkost wrote:
> > > > On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote:
> > > > > When running with KVM on POWER, we register some CPU types during
> > > > > the initialization function of the ppc64 KVM code (which unfortunately
> > > > > also can not be done via a type_init() like it is done on x86).
> > > > 
> > > > Can you elaborate why it can't be done via type_init()? If the
> > > > QOM type hierarchy depends on any runtime data unavailable at
> > > > type_init(), we should fix that.
> > > 
> > > Hmm.. how?  This is specifically for the special 'host' cpu in the
> > > case of KVM.  We can't use a static configuration here, because there
> > > are things on the host that could limit what features of the CPU are
> > > available for guest use.  So, we need KVM to be initialized in order
> > > to query that information.
> > 
> > There's nothing saying you need to query that information before
> > type_register() or during class_init, does it? The behavior of
> > TYPE_HOST_POWERPC_CPU after object_new() is called can be as
> > dynamic as you want it to, but the QOM type hierarchy is supposed
> > to be static.
> 
> So, the thing is we have some properties that logically belong in the
> CPU class, rather than instance, and that's correct for all TCG CPUs,
> but not for the KVM host CPU.  It seems nasty to have to force all
> those things into the instance just because of KVM.

You can still register any class properties you want, without
querying KVM first. Are there properties that you are able to
register only after querying KVM? Is the set of class properties
on TYPE_HOST_POWERPC_CPU different depending on the host
capabilities?

I'm looking at target/ppc/cpu-models.c, and I only see the
class_init function setting PowerPCCPUClass::pvr and
PowerPCCPUClass::svr. Am I missing anything?

You can still keep pvr and svr on the CPU class. The only thing
that's different on TYPE_HOST_POWERPC_CPU is that it won't have
the preset values on PowerPCCPUClass. But it can initialize the
instance values on ->instance_init() or ->realize() instead.

I even see ppc_cpu_class_by_pvr*() treating TYPE_HOST_POWERPC_CPU
as special on PVR lookup, already. So it looks like the code is
almost ready for that?

> 
> > Registering QOM types outside type_init() is only causing us
> > problems and we should stop doing that.
> > 
> > > 
> > > > >                                                                 So to
> > > > > be able to see these updates in the CPU help text, the code that calls
> > > > > list_cpus() has to be run after configure_accelerator(). This move should
> > > > > be fine since the "cpu_model" variable is also never used before the call
> > > > > to configure_accelerator(), and thus there should not be any unwanted
> > > > > side effects in the code before configure_accelerator() if the user
> > > > > started QEMU with "-cpu ?" or "-cpu help".
> > > > > 
> > > > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > > 
> > > > I am not convinced that the output of "-cpu help" and
> > > > "-cpu help -machine accel=kvm" should look different. Do you have
> > > > an example of what exactly is wrong with the output currently?
> > > > 
> > > > I actually believe list_cpus() needs to be called _earlier_, not
> > > > later. Otherwise we won't be able to fix this bug:
> > > > 
> > > >   $ qemu-system-arm -cpu help
> > > >   qemu-system-arm:/usr/local/etc/qemu/qemu.conf:1: No machine specified, and there is no default
> > > >   Use -machine help to list supported machines
> > > >   $ 
> > > > 
> > > > > ---
> > > > >  vl.c | 10 +++++-----
> > > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > > > 
> > > > > diff --git a/vl.c b/vl.c
> > > > > index 0b72b12..315c5c3 100644
> > > > > --- a/vl.c
> > > > > +++ b/vl.c
> > > > > @@ -4055,11 +4055,6 @@ int main(int argc, char **argv, char **envp)
> > > > >          qemu_set_hw_version(machine_class->hw_version);
> > > > >      }
> > > > >  
> > > > > -    if (cpu_model && is_help_option(cpu_model)) {
> > > > > -        list_cpus(stdout, &fprintf, cpu_model);
> > > > > -        exit(0);
> > > > > -    }
> > > > > -
> > > > >      if (!trace_init_backends()) {
> > > > >          exit(1);
> > > > >      }
> > > > > @@ -4298,6 +4293,11 @@ int main(int argc, char **argv, char **envp)
> > > > >  
> > > > >      configure_accelerator(current_machine);
> > > > >  
> > > > > +    if (cpu_model && is_help_option(cpu_model)) {
> > > > > +        list_cpus(stdout, &fprintf, cpu_model);
> > > > > +        exit(0);
> > > > > +    }
> > > > > +
> > > > >      if (qtest_chrdev) {
> > > > >          qtest_init(qtest_chrdev, qtest_log, &error_fatal);
> > > > >      }
> > > > 
> > > 
> > 
> > 
> > 
> 
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson



-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/2] vl: Print CPU help after we've registered the CPU accelerators

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 7213 bytes --]

On Tue, Mar 07, 2017 at 09:02:37AM -0300, Eduardo Habkost wrote:
> On Tue, Mar 07, 2017 at 02:31:05PM +1100, David Gibson wrote:
> > On Mon, Mar 06, 2017 at 08:47:52AM -0300, Eduardo Habkost wrote:
> > > On Mon, Mar 06, 2017 at 10:06:27AM +1100, David Gibson wrote:
> > > > On Fri, Mar 03, 2017 at 11:58:07AM -0300, Eduardo Habkost wrote:
> > > > > On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote:
> > > > > > When running with KVM on POWER, we register some CPU types during
> > > > > > the initialization function of the ppc64 KVM code (which unfortunately
> > > > > > also can not be done via a type_init() like it is done on x86).
> > > > > 
> > > > > Can you elaborate why it can't be done via type_init()? If the
> > > > > QOM type hierarchy depends on any runtime data unavailable at
> > > > > type_init(), we should fix that.
> > > > 
> > > > Hmm.. how?  This is specifically for the special 'host' cpu in the
> > > > case of KVM.  We can't use a static configuration here, because there
> > > > are things on the host that could limit what features of the CPU are
> > > > available for guest use.  So, we need KVM to be initialized in order
> > > > to query that information.
> > > 
> > > There's nothing saying you need to query that information before
> > > type_register() or during class_init, does it? The behavior of
> > > TYPE_HOST_POWERPC_CPU after object_new() is called can be as
> > > dynamic as you want it to, but the QOM type hierarchy is supposed
> > > to be static.
> > 
> > So, the thing is we have some properties that logically belong in the
> > CPU class, rather than instance, and that's correct for all TCG CPUs,
> > but not for the KVM host CPU.  It seems nasty to have to force all
> > those things into the instance just because of KVM.
> 
> You can still register any class properties you want, without
> querying KVM first. Are there properties that you are able to
> register only after querying KVM? Is the set of class properties
> on TYPE_HOST_POWERPC_CPU different depending on the host
> capabilities?

Sorry, I used "properties" sloppily, not meaning QOM properties.

There are several fields in the CPU class which describe CPU
capabilities - bitmasks indicating which instructions are available,
and L1 cacheline sizes.  There are some other things that are in the
CPU instance at the moment, but arguably would belong better in the
class: available page sizes and PTE encodings, for example.

At the real hardware level these things are dependent only the model
of CPU - hence belonging in cpu class, not instance.  But, because of
the way virtualization works on POWER, some of the features may not be
available to guests, due to configuration of the hypervisor.  So for
the "host" cpu we need to query KVM to see which CPU features are
actually available.

> I'm looking at target/ppc/cpu-models.c, and I only see the
> class_init function setting PowerPCCPUClass::pvr and
> PowerPCCPUClass::svr. Am I missing anything?

Yes.  Notice that the .parent set there is not the base powerpc cpu
class, but a "family" class.  Those families are defined in
translate_init.c by another hairy macro POWERPC_FAMILY().  The family
class_init function (generally done as a block below the macro
invocation) initializes several other things, including insns_flags.

kvmppc_host_cpu_class_init(), uses the family's copy of insns_flags as
a base, but needs to query the host and adjust it for a couple of
instruction groups that can be disabled from the hypervisor.

[Aside: we also update the cache sizes based on host information.
The reasons for that are complicated.  This particular case doesn't
require a late class init though, because although it's queried from
the host, it's not queried from kvm specifically]

> You can still keep pvr and svr on the CPU class. The only thing
> that's different on TYPE_HOST_POWERPC_CPU is that it won't have
> the preset values on PowerPCCPUClass. But it can initialize the
> instance values on ->instance_init() or ->realize() instead.
> 
> I even see ppc_cpu_class_by_pvr*() treating TYPE_HOST_POWERPC_CPU
> as special on PVR lookup, already. So it looks like the code is
> almost ready for that?

PVR is a red herring.  insn_flags is the real issue.

> 
> > 
> > > Registering QOM types outside type_init() is only causing us
> > > problems and we should stop doing that.
> > > 
> > > > 
> > > > > >                                                                 So to
> > > > > > be able to see these updates in the CPU help text, the code that calls
> > > > > > list_cpus() has to be run after configure_accelerator(). This move should
> > > > > > be fine since the "cpu_model" variable is also never used before the call
> > > > > > to configure_accelerator(), and thus there should not be any unwanted
> > > > > > side effects in the code before configure_accelerator() if the user
> > > > > > started QEMU with "-cpu ?" or "-cpu help".
> > > > > > 
> > > > > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > > > 
> > > > > I am not convinced that the output of "-cpu help" and
> > > > > "-cpu help -machine accel=kvm" should look different. Do you have
> > > > > an example of what exactly is wrong with the output currently?
> > > > > 
> > > > > I actually believe list_cpus() needs to be called _earlier_, not
> > > > > later. Otherwise we won't be able to fix this bug:
> > > > > 
> > > > >   $ qemu-system-arm -cpu help
> > > > >   qemu-system-arm:/usr/local/etc/qemu/qemu.conf:1: No machine specified, and there is no default
> > > > >   Use -machine help to list supported machines
> > > > >   $ 
> > > > > 
> > > > > > ---
> > > > > >  vl.c | 10 +++++-----
> > > > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > > > > 
> > > > > > diff --git a/vl.c b/vl.c
> > > > > > index 0b72b12..315c5c3 100644
> > > > > > --- a/vl.c
> > > > > > +++ b/vl.c
> > > > > > @@ -4055,11 +4055,6 @@ int main(int argc, char **argv, char **envp)
> > > > > >          qemu_set_hw_version(machine_class->hw_version);
> > > > > >      }
> > > > > >  
> > > > > > -    if (cpu_model && is_help_option(cpu_model)) {
> > > > > > -        list_cpus(stdout, &fprintf, cpu_model);
> > > > > > -        exit(0);
> > > > > > -    }
> > > > > > -
> > > > > >      if (!trace_init_backends()) {
> > > > > >          exit(1);
> > > > > >      }
> > > > > > @@ -4298,6 +4293,11 @@ int main(int argc, char **argv, char **envp)
> > > > > >  
> > > > > >      configure_accelerator(current_machine);
> > > > > >  
> > > > > > +    if (cpu_model && is_help_option(cpu_model)) {
> > > > > > +        list_cpus(stdout, &fprintf, cpu_model);
> > > > > > +        exit(0);
> > > > > > +    }
> > > > > > +
> > > > > >      if (qtest_chrdev) {
> > > > > >          qtest_init(qtest_chrdev, qtest_log, &error_fatal);
> > > > > >      }
> > > > > 
> > > > 
> > > 
> > > 
> > > 
> > 
> 
> 
> 

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 2/2] vl: Print CPU help after we've registered the CPU accelerators

[Eduardo Habkost]

On Wed, Mar 08, 2017 at 01:33:45PM +1100, David Gibson wrote:
> On Tue, Mar 07, 2017 at 09:02:37AM -0300, Eduardo Habkost wrote:
> > On Tue, Mar 07, 2017 at 02:31:05PM +1100, David Gibson wrote:
> > > On Mon, Mar 06, 2017 at 08:47:52AM -0300, Eduardo Habkost wrote:
> > > > On Mon, Mar 06, 2017 at 10:06:27AM +1100, David Gibson wrote:
> > > > > On Fri, Mar 03, 2017 at 11:58:07AM -0300, Eduardo Habkost wrote:
> > > > > > On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote:
> > > > > > > When running with KVM on POWER, we register some CPU types during
> > > > > > > the initialization function of the ppc64 KVM code (which unfortunately
> > > > > > > also can not be done via a type_init() like it is done on x86).
> > > > > > 
> > > > > > Can you elaborate why it can't be done via type_init()? If the
> > > > > > QOM type hierarchy depends on any runtime data unavailable at
> > > > > > type_init(), we should fix that.
> > > > > 
> > > > > Hmm.. how?  This is specifically for the special 'host' cpu in the
> > > > > case of KVM.  We can't use a static configuration here, because there
> > > > > are things on the host that could limit what features of the CPU are
> > > > > available for guest use.  So, we need KVM to be initialized in order
> > > > > to query that information.
> > > > 
> > > > There's nothing saying you need to query that information before
> > > > type_register() or during class_init, does it? The behavior of
> > > > TYPE_HOST_POWERPC_CPU after object_new() is called can be as
> > > > dynamic as you want it to, but the QOM type hierarchy is supposed
> > > > to be static.
> > > 
> > > So, the thing is we have some properties that logically belong in the
> > > CPU class, rather than instance, and that's correct for all TCG CPUs,
> > > but not for the KVM host CPU.  It seems nasty to have to force all
> > > those things into the instance just because of KVM.
> > 
> > You can still register any class properties you want, without
> > querying KVM first. Are there properties that you are able to
> > register only after querying KVM? Is the set of class properties
> > on TYPE_HOST_POWERPC_CPU different depending on the host
> > capabilities?
> 
> Sorry, I used "properties" sloppily, not meaning QOM properties.
> 
> There are several fields in the CPU class which describe CPU
> capabilities - bitmasks indicating which instructions are available,
> and L1 cacheline sizes.  There are some other things that are in the
> CPU instance at the moment, but arguably would belong better in the
> class: available page sizes and PTE encodings, for example.
> 
> At the real hardware level these things are dependent only the model
> of CPU - hence belonging in cpu class, not instance.  But, because of
> the way virtualization works on POWER, some of the features may not be
> available to guests, due to configuration of the hypervisor.  So for
> the "host" cpu we need to query KVM to see which CPU features are
> actually available.
> 

I see. If there is data that is available only at PowerPCCPUClass
and you don't want to duplicate it at PowerPCCPU, we can have a
solution for that: instead of late-registration of the class, we
could leave those fields to be populated after KVM is
initialized.

Anyway, I don't think this is urgent: the code already treats
"host" as an exception in ppc_cpu_list(), and (AFAICS) the
original problem this patch addresses is related to the
inaccurate alias information only (which is not a consequence of
the late type_register() call).

> > I'm looking at target/ppc/cpu-models.c, and I only see the
> > class_init function setting PowerPCCPUClass::pvr and
> > PowerPCCPUClass::svr. Am I missing anything?
> 
> Yes.  Notice that the .parent set there is not the base powerpc cpu
> class, but a "family" class.  Those families are defined in
> translate_init.c by another hairy macro POWERPC_FAMILY().  The family
> class_init function (generally done as a block below the macro
> invocation) initializes several other things, including insns_flags.
> 
> kvmppc_host_cpu_class_init(), uses the family's copy of insns_flags as
> a base, but needs to query the host and adjust it for a couple of
> instruction groups that can be disabled from the hypervisor.
> 
> [Aside: we also update the cache sizes based on host information.
> The reasons for that are complicated.  This particular case doesn't
> require a late class init though, because although it's queried from
> the host, it's not queried from kvm specifically]
> 
> > You can still keep pvr and svr on the CPU class. The only thing
> > that's different on TYPE_HOST_POWERPC_CPU is that it won't have
> > the preset values on PowerPCCPUClass. But it can initialize the
> > instance values on ->instance_init() or ->realize() instead.
> > 
> > I even see ppc_cpu_class_by_pvr*() treating TYPE_HOST_POWERPC_CPU
> > as special on PVR lookup, already. So it looks like the code is
> > almost ready for that?
> 
> PVR is a red herring.  insn_flags is the real issue.
> 
> > 
> > > 
> > > > Registering QOM types outside type_init() is only causing us
> > > > problems and we should stop doing that.
> > > > 
> > > > > 
> > > > > > >                                                                 So to
> > > > > > > be able to see these updates in the CPU help text, the code that calls
> > > > > > > list_cpus() has to be run after configure_accelerator(). This move should
> > > > > > > be fine since the "cpu_model" variable is also never used before the call
> > > > > > > to configure_accelerator(), and thus there should not be any unwanted
> > > > > > > side effects in the code before configure_accelerator() if the user
> > > > > > > started QEMU with "-cpu ?" or "-cpu help".
> > > > > > > 
> > > > > > > Signed-off-by: Thomas Huth <thuth@redhat.com>
> > > > > > 
> > > > > > I am not convinced that the output of "-cpu help" and
> > > > > > "-cpu help -machine accel=kvm" should look different. Do you have
> > > > > > an example of what exactly is wrong with the output currently?
> > > > > > 
> > > > > > I actually believe list_cpus() needs to be called _earlier_, not
> > > > > > later. Otherwise we won't be able to fix this bug:
> > > > > > 
> > > > > >   $ qemu-system-arm -cpu help
> > > > > >   qemu-system-arm:/usr/local/etc/qemu/qemu.conf:1: No machine specified, and there is no default
> > > > > >   Use -machine help to list supported machines
> > > > > >   $ 
> > > > > > 
> > > > > > > ---
> > > > > > >  vl.c | 10 +++++-----
> > > > > > >  1 file changed, 5 insertions(+), 5 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/vl.c b/vl.c
> > > > > > > index 0b72b12..315c5c3 100644
> > > > > > > --- a/vl.c
> > > > > > > +++ b/vl.c
> > > > > > > @@ -4055,11 +4055,6 @@ int main(int argc, char **argv, char **envp)
> > > > > > >          qemu_set_hw_version(machine_class->hw_version);
> > > > > > >      }
> > > > > > >  
> > > > > > > -    if (cpu_model && is_help_option(cpu_model)) {
> > > > > > > -        list_cpus(stdout, &fprintf, cpu_model);
> > > > > > > -        exit(0);
> > > > > > > -    }
> > > > > > > -
> > > > > > >      if (!trace_init_backends()) {
> > > > > > >          exit(1);
> > > > > > >      }
> > > > > > > @@ -4298,6 +4293,11 @@ int main(int argc, char **argv, char **envp)
> > > > > > >  
> > > > > > >      configure_accelerator(current_machine);
> > > > > > >  
> > > > > > > +    if (cpu_model && is_help_option(cpu_model)) {
> > > > > > > +        list_cpus(stdout, &fprintf, cpu_model);
> > > > > > > +        exit(0);
> > > > > > > +    }
> > > > > > > +
> > > > > > >      if (qtest_chrdev) {
> > > > > > >          qtest_init(qtest_chrdev, qtest_log, &error_fatal);
> > > > > > >      }
> > > > > > 
> > > > > 
> > > > 
> > > > 
> > > > 
> > > 
> > 
> > 
> > 
> 
> -- 
> David Gibson			| I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
> 				| _way_ _around_!
> http://www.ozlabs.org/~dgibson



-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/2] vl: Print CPU help after we've registered the CPU accelerators

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4017 bytes --]

On Wed, Mar 08, 2017 at 09:08:50AM -0300, Eduardo Habkost wrote:
> On Wed, Mar 08, 2017 at 01:33:45PM +1100, David Gibson wrote:
> > On Tue, Mar 07, 2017 at 09:02:37AM -0300, Eduardo Habkost wrote:
> > > On Tue, Mar 07, 2017 at 02:31:05PM +1100, David Gibson wrote:
> > > > On Mon, Mar 06, 2017 at 08:47:52AM -0300, Eduardo Habkost wrote:
> > > > > On Mon, Mar 06, 2017 at 10:06:27AM +1100, David Gibson wrote:
> > > > > > On Fri, Mar 03, 2017 at 11:58:07AM -0300, Eduardo Habkost wrote:
> > > > > > > On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote:
> > > > > > > > When running with KVM on POWER, we register some CPU types during
> > > > > > > > the initialization function of the ppc64 KVM code (which unfortunately
> > > > > > > > also can not be done via a type_init() like it is done on x86).
> > > > > > > 
> > > > > > > Can you elaborate why it can't be done via type_init()? If the
> > > > > > > QOM type hierarchy depends on any runtime data unavailable at
> > > > > > > type_init(), we should fix that.
> > > > > > 
> > > > > > Hmm.. how?  This is specifically for the special 'host' cpu in the
> > > > > > case of KVM.  We can't use a static configuration here, because there
> > > > > > are things on the host that could limit what features of the CPU are
> > > > > > available for guest use.  So, we need KVM to be initialized in order
> > > > > > to query that information.
> > > > > 
> > > > > There's nothing saying you need to query that information before
> > > > > type_register() or during class_init, does it? The behavior of
> > > > > TYPE_HOST_POWERPC_CPU after object_new() is called can be as
> > > > > dynamic as you want it to, but the QOM type hierarchy is supposed
> > > > > to be static.
> > > > 
> > > > So, the thing is we have some properties that logically belong in the
> > > > CPU class, rather than instance, and that's correct for all TCG CPUs,
> > > > but not for the KVM host CPU.  It seems nasty to have to force all
> > > > those things into the instance just because of KVM.
> > > 
> > > You can still register any class properties you want, without
> > > querying KVM first. Are there properties that you are able to
> > > register only after querying KVM? Is the set of class properties
> > > on TYPE_HOST_POWERPC_CPU different depending on the host
> > > capabilities?
> > 
> > Sorry, I used "properties" sloppily, not meaning QOM properties.
> > 
> > There are several fields in the CPU class which describe CPU
> > capabilities - bitmasks indicating which instructions are available,
> > and L1 cacheline sizes.  There are some other things that are in the
> > CPU instance at the moment, but arguably would belong better in the
> > class: available page sizes and PTE encodings, for example.
> > 
> > At the real hardware level these things are dependent only the model
> > of CPU - hence belonging in cpu class, not instance.  But, because of
> > the way virtualization works on POWER, some of the features may not be
> > available to guests, due to configuration of the hypervisor.  So for
> > the "host" cpu we need to query KVM to see which CPU features are
> > actually available.
> > 
> 
> I see. If there is data that is available only at PowerPCCPUClass
> and you don't want to duplicate it at PowerPCCPU, we can have a
> solution for that: instead of late-registration of the class, we
> could leave those fields to be populated after KVM is
> initialized.

Ok, that sounds workable.

> Anyway, I don't think this is urgent: the code already treats
> "host" as an exception in ppc_cpu_list(), and (AFAICS) the
> original problem this patch addresses is related to the
> inaccurate alias information only (which is not a consequence of
> the late type_register() call).

I agree.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [Qemu-devel] [PATCH 2/2] vl: Print CPU help after we've registered the CPU accelerators

[Thomas Huth]

On 03.03.2017 15:58, Eduardo Habkost wrote:
[...]
> I am not convinced that the output of "-cpu help" and
> "-cpu help -machine accel=kvm" should look different. Do you have
> an example of what exactly is wrong with the output currently?

The problem is that on POWER, we've got a "family" of CPUs with
different sub-types, e.g. for POWER8:

$ qemu-system-ppc64 -cpu ? | grep POWER8
PowerPC POWER8E_v2.1     PVR 004b0201
PowerPC POWER8E          (alias for POWER8E_v2.1)
PowerPC POWER8NVL_v1.0   PVR 004c0100
PowerPC POWER8NVL        (alias for POWER8NVL_v1.0)
PowerPC POWER8_v2.0      PVR 004d0200
PowerPC POWER8           (alias for POWER8_v2.0)

Most of the users don't know about the current subtype that they are
using, and just want to use "-cpu POWER8" - and for example we've also
got an agreement with the libvirt folks that they can always use "-cpu
POWER8" for any kind of POWER8 system, no matter whether the host is
using a POWER8E or POWER8NVL chip.
So the "POWER8" alias now gets updated internally in QEMU to the correct
host CPU type ... but the output of "-cpu help" is then still wrong.
I agree that it's kind of ugly to have different help texts depending on
whether "accel=kvm" has been used or not, but that sounds still better
to me than printing wrong information here.
Thinking about this again ... maybe it would be better if we'd rework
the help text to print out something like this instead:

PowerPC POWER8           (alias for any POWER8 chip)

... so that we simply get rid of the version/subtype information here
completely?

 Thomas

Re: [Qemu-devel] [PATCH 2/2] vl: Print CPU help after we've registered the CPU accelerators

[Eduardo Habkost]

On Tue, Mar 07, 2017 at 10:02:26AM +0100, Thomas Huth wrote:
> On 03.03.2017 15:58, Eduardo Habkost wrote:
> [...]
> > I am not convinced that the output of "-cpu help" and
> > "-cpu help -machine accel=kvm" should look different. Do you have
> > an example of what exactly is wrong with the output currently?
> 
> The problem is that on POWER, we've got a "family" of CPUs with
> different sub-types, e.g. for POWER8:
> 
> $ qemu-system-ppc64 -cpu ? | grep POWER8
> PowerPC POWER8E_v2.1     PVR 004b0201
> PowerPC POWER8E          (alias for POWER8E_v2.1)
> PowerPC POWER8NVL_v1.0   PVR 004c0100
> PowerPC POWER8NVL        (alias for POWER8NVL_v1.0)
> PowerPC POWER8_v2.0      PVR 004d0200
> PowerPC POWER8           (alias for POWER8_v2.0)
> 
> Most of the users don't know about the current subtype that they are
> using, and just want to use "-cpu POWER8" - and for example we've also
> got an agreement with the libvirt folks that they can always use "-cpu
> POWER8" for any kind of POWER8 system, no matter whether the host is
> using a POWER8E or POWER8NVL chip.
> So the "POWER8" alias now gets updated internally in QEMU to the correct
> host CPU type ... but the output of "-cpu help" is then still wrong.
> I agree that it's kind of ugly to have different help texts depending on
> whether "accel=kvm" has been used or not, but that sounds still better
> to me than printing wrong information here.

I agree that incorrect information is even worse than showing
different help information depending on accel=kvm, but:

> Thinking about this again ... maybe it would be better if we'd rework
> the help text to print out something like this instead:
> 
> PowerPC POWER8           (alias for any POWER8 chip)
> 
> ... so that we simply get rid of the version/subtype information here
> completely?

Yes, making help output not depend on accel=kvm sounds better to
me.

This seems to be affected only by the alias table, so it can be
fixed even before we address the late-type_register() issue I was
discussing with David?

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/2] vl: Print CPU help after we've registered the CPU accelerators

[Andrea Bolognani]

On Tue, 2017-03-07 at 10:02 +0100, Thomas Huth wrote:
> The problem is that on POWER, we've got a "family" of CPUs with
> different sub-types, e.g. for POWER8:
> 
> $ qemu-system-ppc64 -cpu ? | grep POWER8
> PowerPC POWER8E_v2.1     PVR 004b0201
> PowerPC POWER8E          (alias for POWER8E_v2.1)
> PowerPC POWER8NVL_v1.0   PVR 004c0100
> PowerPC POWER8NVL        (alias for POWER8NVL_v1.0)
> PowerPC POWER8_v2.0      PVR 004d0200
> PowerPC POWER8           (alias for POWER8_v2.0)
> 
> Most of the users don't know about the current subtype that they are
> using, and just want to use "-cpu POWER8" - and for example we've also
> got an agreement with the libvirt folks that they can always use "-cpu
> POWER8" for any kind of POWER8 system, no matter whether the host is
> using a POWER8E or POWER8NVL chip.

Yup, thanks for bringing that up and keeping an eye out
to make sure it keeps working :)

> So the "POWER8" alias now gets updated internally in QEMU to the correct
> host CPU type ... but the output of "-cpu help" is then still wrong.
> I agree that it's kind of ugly to have different help texts depending on
> whether "accel=kvm" has been used or not, but that sounds still better
> to me than printing wrong information here.
> Thinking about this again ... maybe it would be better if we'd rework
> the help text to print out something like this instead:
> 
> PowerPC POWER8           (alias for any POWER8 chip)
> 
> ... so that we simply get rid of the version/subtype information here
> completely?

I was initially concerned about this, because I remembered
libvirt parsing the string after "(alias for ", but I
checked and it looks like we do so only for machine types,
not for CPU models.

So the change should be safe, and since we're using QMP to
fetch CPU definitions these days, I don't expect we'll need
to worry about this change even in the future.

Still, did you try changing the description and using the
resulting QEMU binary along with libvirt?

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [Qemu-devel] [PATCH 2/2] vl: Print CPU help after we've registered the CPU accelerators

[Peter Maydell]

On 3 March 2017 at 15:58, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote:
>> When running with KVM on POWER, we register some CPU types during
>> the initialization function of the ppc64 KVM code (which unfortunately
>> also can not be done via a type_init() like it is done on x86).
>
> Can you elaborate why it can't be done via type_init()? If the
> QOM type hierarchy depends on any runtime data unavailable at
> type_init(), we should fix that.

On ARM we (currently) have the KVM-only 'host' CPU type be
added to the type hierarchy only at runtime in kvm_init(),
but we deal with the '-help' problem by having arm_cpu_list() do

#ifdef CONFIG_KVM
    /* The 'host' CPU type is dynamically registered only if KVM is
     * enabled, so we have to special-case it here:
     */
    (*cpu_fprintf)(f, "  host (only available in KVM mode)\n");
#endif

thanks
-- PMM

Re: [Qemu-devel] [PATCH 2/2] vl: Print CPU help after we've registered the CPU accelerators

[Eduardo Habkost]

On Wed, Mar 08, 2017 at 03:31:01PM +0100, Peter Maydell wrote:
> On 3 March 2017 at 15:58, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote:
> >> When running with KVM on POWER, we register some CPU types during
> >> the initialization function of the ppc64 KVM code (which unfortunately
> >> also can not be done via a type_init() like it is done on x86).
> >
> > Can you elaborate why it can't be done via type_init()? If the
> > QOM type hierarchy depends on any runtime data unavailable at
> > type_init(), we should fix that.
> 
> On ARM we (currently) have the KVM-only 'host' CPU type be
> added to the type hierarchy only at runtime in kvm_init(),
> but we deal with the '-help' problem by having arm_cpu_list() do
> 
> #ifdef CONFIG_KVM
>     /* The 'host' CPU type is dynamically registered only if KVM is
>      * enabled, so we have to special-case it here:
>      */
>     (*cpu_fprintf)(f, "  host (only available in KVM mode)\n");
> #endif
> 

We already have similar code at ppc_cpu_list():

#ifdef CONFIG_KVM
    cpu_fprintf(f, "\n");
    cpu_fprintf(f, "PowerPC %-16s\n", "host");
#endif

If I understood it correctly, the current problem is just
inaccurate alias information being printed because the alias
table is modified by the accelerator.

My current suggestion is to avoid printing anything that depends
on the current machine/accelerator at the help output.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/2] vl: Print CPU help after we've registered the CPU accelerators

[Thomas Huth]

On 08.03.2017 15:50, Eduardo Habkost wrote:
> On Wed, Mar 08, 2017 at 03:31:01PM +0100, Peter Maydell wrote:
>> On 3 March 2017 at 15:58, Eduardo Habkost <ehabkost@redhat.com> wrote:
>>> On Tue, Jan 31, 2017 at 02:11:59PM +0100, Thomas Huth wrote:
>>>> When running with KVM on POWER, we register some CPU types during
>>>> the initialization function of the ppc64 KVM code (which unfortunately
>>>> also can not be done via a type_init() like it is done on x86).
>>>
>>> Can you elaborate why it can't be done via type_init()? If the
>>> QOM type hierarchy depends on any runtime data unavailable at
>>> type_init(), we should fix that.
>>
>> On ARM we (currently) have the KVM-only 'host' CPU type be
>> added to the type hierarchy only at runtime in kvm_init(),
>> but we deal with the '-help' problem by having arm_cpu_list() do
>>
>> #ifdef CONFIG_KVM
>>     /* The 'host' CPU type is dynamically registered only if KVM is
>>      * enabled, so we have to special-case it here:
>>      */
>>     (*cpu_fprintf)(f, "  host (only available in KVM mode)\n");
>> #endif
>>
> 
> We already have similar code at ppc_cpu_list():
> 
> #ifdef CONFIG_KVM
>     cpu_fprintf(f, "\n");
>     cpu_fprintf(f, "PowerPC %-16s\n", "host");
> #endif
> 
> If I understood it correctly, the current problem is just
> inaccurate alias information being printed because the alias
> table is modified by the accelerator.

Yes.

> My current suggestion is to avoid printing anything that depends
> on the current machine/accelerator at the help output.

I'll have a look at that...

 Thomas