[Qemu-devel] [PATCH qom-cpu 0/4] CPU cleanup and PPC subclasses

[Qemu-devel] [PATCH qom-cpu 0/4] CPU cleanup and PPC subclasses

[Andreas Färber]

Hello,

This series starts with unifying the various structs for -cpu ? implementation.

I'm guessing the second patch will be necessary for CPU-as-a-device,
but it breaks the paradigm of having only typedefs in qemu-types.h.

Based on that, by demand from David, here's a quick and dirty introduction of
CPU subclasses as proposed some time ago. It's been redone, so no change log.
My proposal is to leave ppc_def_t in place for now, adding a pointer to it in
the CPU class for instance_init and for David.

Plus, it seems that my "POWER5+ (gs)" CPU is #ifdef TODO'ed out, so lacking
an immediate fix how to fall back to another CPU model I'm proposing an error.

The series is based on the current qom-cpu queue and will need to be slightly
rebased when I apply the KVM CPUState series.

Regards,
Andreas

Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Richard Henderson <rth@twiddle.net>
Cc: Peter Maydell <peter.maydell@linaro.org>

Cc: Alexander Graf <agraf@suse.de>
Cc: qemu-ppc <qemu-ppc@nongnu.org>
Cc: David Gibson <david@gibson.dropbear.id.au>

Andreas Färber (4):
  cpu: Introduce CPUListState struct
  qemu-common.h: Move fprintf_function to qemu-types.h
  target-ppc: Slim conversion of model definitions to QOM subclasses
  target-ppc: Error out for -cpu host on unknown PVR

 include/qemu/cpu.h          |   12 ++
 qemu-common.h               |    5 -
 qemu-types.h                |    6 +
 target-alpha/cpu.c          |    9 +-
 target-arm/helper.c         |    9 +-
 target-m68k/helper.c        |    9 +-
 target-ppc/Makefile.objs    |    3 +-
 target-ppc/cpu-qom.h        |    5 +
 target-ppc/cpu.h            |    4 -
 target-ppc/helper.c         |   50 -------
 target-ppc/kvm.c            |   44 +++++-
 target-ppc/kvm_ppc.h        |    8 +-
 target-ppc/translate_init.c |  344 +++++++++++++++++++++++++++++--------------
 13 Dateien geändert, 306 Zeilen hinzugefügt(+), 202 Zeilen entfernt(-)
 delete mode 100644 target-ppc/helper.c

-- 
1.7.10.4

[Qemu-devel] [PATCH qom-cpu 1/4] cpu: Introduce CPUListState struct

[Andreas Färber]

This generalizes {ARM,M68k,Alpha}CPUListState to avoid declaring it for
each target.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 include/qemu/cpu.h   |   12 ++++++++++++
 target-alpha/cpu.c   |    9 ++-------
 target-arm/helper.c  |    9 ++-------
 target-m68k/helper.c |    9 ++-------
 4 Dateien geändert, 18 Zeilen hinzugefügt(+), 21 Zeilen entfernt(-)

diff --git a/include/qemu/cpu.h b/include/qemu/cpu.h
index 61b7698..5fbb3f9 100644
--- a/include/qemu/cpu.h
+++ b/include/qemu/cpu.h
@@ -21,6 +21,7 @@
 #define QEMU_CPU_H
 
 #include "qemu/object.h"
+#include "qemu-common.h"
 #include "qemu-thread.h"
 
 /**
@@ -80,6 +81,17 @@ struct CPUState {
     /* TODO Move common fields from CPUArchState here. */
 };
 
+/**
+ * CPUListState:
+ * @cpu_fprintf: Print function.
+ * @file: File to print to using @cpu_fprint.
+ *
+ * State commonly used for iterating over CPU models.
+ */
+typedef struct CPUListState {
+    fprintf_function cpu_fprintf;
+    FILE *file;
+} CPUListState;
 
 /**
  * cpu_reset:
diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
index d065085..915278f 100644
--- a/target-alpha/cpu.c
+++ b/target-alpha/cpu.c
@@ -33,11 +33,6 @@ static void alpha_cpu_realize(Object *obj, Error **err)
 #endif
 }
 
-typedef struct AlphaCPUListState {
-    fprintf_function cpu_fprintf;
-    FILE *file;
-} AlphaCPUListState;
-
 /* Sort alphabetically by type name. */
 static gint alpha_cpu_list_compare(gconstpointer a, gconstpointer b)
 {
@@ -53,7 +48,7 @@ static gint alpha_cpu_list_compare(gconstpointer a, gconstpointer b)
 static void alpha_cpu_list_entry(gpointer data, gpointer user_data)
 {
     ObjectClass *oc = data;
-    AlphaCPUListState *s = user_data;
+    CPUListState *s = user_data;
 
     (*s->cpu_fprintf)(s->file, "  %s\n",
                       object_class_get_name(oc));
@@ -61,7 +56,7 @@ static void alpha_cpu_list_entry(gpointer data, gpointer user_data)
 
 void alpha_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 {
-    AlphaCPUListState s = {
+    CPUListState s = {
         .file = f,
         .cpu_fprintf = cpu_fprintf,
     };
diff --git a/target-arm/helper.c b/target-arm/helper.c
index ab8b734..d2f2fb4 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1291,11 +1291,6 @@ ARMCPU *cpu_arm_init(const char *cpu_model)
     return cpu;
 }
 
-typedef struct ARMCPUListState {
-    fprintf_function cpu_fprintf;
-    FILE *file;
-} ARMCPUListState;
-
 /* Sort alphabetically by type name, except for "any". */
 static gint arm_cpu_list_compare(gconstpointer a, gconstpointer b)
 {
@@ -1317,7 +1312,7 @@ static gint arm_cpu_list_compare(gconstpointer a, gconstpointer b)
 static void arm_cpu_list_entry(gpointer data, gpointer user_data)
 {
     ObjectClass *oc = data;
-    ARMCPUListState *s = user_data;
+    CPUListState *s = user_data;
 
     (*s->cpu_fprintf)(s->file, "  %s\n",
                       object_class_get_name(oc));
@@ -1325,7 +1320,7 @@ static void arm_cpu_list_entry(gpointer data, gpointer user_data)
 
 void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 {
-    ARMCPUListState s = {
+    CPUListState s = {
         .file = f,
         .cpu_fprintf = cpu_fprintf,
     };
diff --git a/target-m68k/helper.c b/target-m68k/helper.c
index a5d0100..875a71a 100644
--- a/target-m68k/helper.c
+++ b/target-m68k/helper.c
@@ -25,11 +25,6 @@
 
 #define SIGNBIT (1u << 31)
 
-typedef struct M68kCPUListState {
-    fprintf_function cpu_fprintf;
-    FILE *file;
-} M68kCPUListState;
-
 /* Sort alphabetically, except for "any". */
 static gint m68k_cpu_list_compare(gconstpointer a, gconstpointer b)
 {
@@ -51,7 +46,7 @@ static gint m68k_cpu_list_compare(gconstpointer a, gconstpointer b)
 static void m68k_cpu_list_entry(gpointer data, gpointer user_data)
 {
     ObjectClass *c = data;
-    M68kCPUListState *s = user_data;
+    CPUListState *s = user_data;
 
     (*s->cpu_fprintf)(s->file, "%s\n",
                       object_class_get_name(c));
@@ -59,7 +54,7 @@ static void m68k_cpu_list_entry(gpointer data, gpointer user_data)
 
 void m68k_cpu_list(FILE *f, fprintf_function cpu_fprintf)
 {
-    M68kCPUListState s = {
+    CPUListState s = {
         .file = f,
         .cpu_fprintf = cpu_fprintf,
     };
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH qom-cpu 1/4] cpu: Introduce CPUListState struct

[Igor Mammedov]

On Tue, 18 Dec 2012 08:53:40 +0100
Andreas Färber <afaerber@suse.de> wrote:

> This generalizes {ARM,M68k,Alpha}CPUListState to avoid declaring it for
> each target.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  include/qemu/cpu.h   |   12 ++++++++++++
>  target-alpha/cpu.c   |    9 ++-------
>  target-arm/helper.c  |    9 ++-------
>  target-m68k/helper.c |    9 ++-------
>  4 Dateien geändert, 18 Zeilen hinzugefügt(+), 21 Zeilen entfernt(-)
> 

Could we use cpus.h for CPUListState?

It has related list_cpus() and it doesn't included in any headers yet.
And we won't pollute base CPU qom definition with utility structures.

> diff --git a/include/qemu/cpu.h b/include/qemu/cpu.h
> index 61b7698..5fbb3f9 100644
> --- a/include/qemu/cpu.h
> +++ b/include/qemu/cpu.h
> @@ -21,6 +21,7 @@
>  #define QEMU_CPU_H
>  
>  #include "qemu/object.h"
> +#include "qemu-common.h"
>  #include "qemu-thread.h"
>  
>  /**
> @@ -80,6 +81,17 @@ struct CPUState {
>      /* TODO Move common fields from CPUArchState here. */
>  };
>  
> +/**
> + * CPUListState:
> + * @cpu_fprintf: Print function.
> + * @file: File to print to using @cpu_fprint.
> + *
> + * State commonly used for iterating over CPU models.
> + */
> +typedef struct CPUListState {
> +    fprintf_function cpu_fprintf;
> +    FILE *file;
> +} CPUListState;
>  
>  /**
>   * cpu_reset:
> diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
> index d065085..915278f 100644
> --- a/target-alpha/cpu.c
> +++ b/target-alpha/cpu.c
> @@ -33,11 +33,6 @@ static void alpha_cpu_realize(Object *obj, Error **err)
>  #endif
>  }
>  
> -typedef struct AlphaCPUListState {
> -    fprintf_function cpu_fprintf;
> -    FILE *file;
> -} AlphaCPUListState;
> -
>  /* Sort alphabetically by type name. */
>  static gint alpha_cpu_list_compare(gconstpointer a, gconstpointer b)
>  {
> @@ -53,7 +48,7 @@ static gint alpha_cpu_list_compare(gconstpointer a,
> gconstpointer b) static void alpha_cpu_list_entry(gpointer data, gpointer
Perhaps *cpu_list_entry() could be generalized too, they all look alike.

> user_data) {
>      ObjectClass *oc = data;
> -    AlphaCPUListState *s = user_data;
> +    CPUListState *s = user_data;
>  
>      (*s->cpu_fprintf)(s->file, "  %s\n",
>                        object_class_get_name(oc));
> @@ -61,7 +56,7 @@ static void alpha_cpu_list_entry(gpointer data, gpointer
> user_data) 
>  void alpha_cpu_list(FILE *f, fprintf_function cpu_fprintf)
>  {
> -    AlphaCPUListState s = {
> +    CPUListState s = {
>          .file = f,
>          .cpu_fprintf = cpu_fprintf,
>      };
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index ab8b734..d2f2fb4 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1291,11 +1291,6 @@ ARMCPU *cpu_arm_init(const char *cpu_model)
>      return cpu;
>  }
>  
> -typedef struct ARMCPUListState {
> -    fprintf_function cpu_fprintf;
> -    FILE *file;
> -} ARMCPUListState;
> -
>  /* Sort alphabetically by type name, except for "any". */
>  static gint arm_cpu_list_compare(gconstpointer a, gconstpointer b)
>  {
> @@ -1317,7 +1312,7 @@ static gint arm_cpu_list_compare(gconstpointer a,
> gconstpointer b) static void arm_cpu_list_entry(gpointer data, gpointer
> user_data) {
>      ObjectClass *oc = data;
> -    ARMCPUListState *s = user_data;
> +    CPUListState *s = user_data;
>  
>      (*s->cpu_fprintf)(s->file, "  %s\n",
>                        object_class_get_name(oc));
> @@ -1325,7 +1320,7 @@ static void arm_cpu_list_entry(gpointer data,
> gpointer user_data) 
>  void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf)
>  {
> -    ARMCPUListState s = {
> +    CPUListState s = {
>          .file = f,
>          .cpu_fprintf = cpu_fprintf,
>      };
> diff --git a/target-m68k/helper.c b/target-m68k/helper.c
> index a5d0100..875a71a 100644
> --- a/target-m68k/helper.c
> +++ b/target-m68k/helper.c
> @@ -25,11 +25,6 @@
>  
>  #define SIGNBIT (1u << 31)
>  
> -typedef struct M68kCPUListState {
> -    fprintf_function cpu_fprintf;
> -    FILE *file;
> -} M68kCPUListState;
> -
>  /* Sort alphabetically, except for "any". */
>  static gint m68k_cpu_list_compare(gconstpointer a, gconstpointer b)
>  {
> @@ -51,7 +46,7 @@ static gint m68k_cpu_list_compare(gconstpointer a,
> gconstpointer b) static void m68k_cpu_list_entry(gpointer data, gpointer
> user_data) {
>      ObjectClass *c = data;
> -    M68kCPUListState *s = user_data;
> +    CPUListState *s = user_data;
>  
>      (*s->cpu_fprintf)(s->file, "%s\n",
>                        object_class_get_name(c));
> @@ -59,7 +54,7 @@ static void m68k_cpu_list_entry(gpointer data, gpointer
> user_data) 
>  void m68k_cpu_list(FILE *f, fprintf_function cpu_fprintf)
>  {
> -    M68kCPUListState s = {
> +    CPUListState s = {
>          .file = f,
>          .cpu_fprintf = cpu_fprintf,
>      };

Re: [Qemu-devel] [PATCH qom-cpu 1/4] cpu: Introduce CPUListState struct

[Andreas Färber]

Am 18.12.2012 16:59, schrieb Igor Mammedov:
> On Tue, 18 Dec 2012 08:53:40 +0100
> Andreas Färber <afaerber@suse.de> wrote:
> 
>> This generalizes {ARM,M68k,Alpha}CPUListState to avoid declaring it for
>> each target.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>>  include/qemu/cpu.h   |   12 ++++++++++++
>>  target-alpha/cpu.c   |    9 ++-------
>>  target-arm/helper.c  |    9 ++-------
>>  target-m68k/helper.c |    9 ++-------
>>  4 Dateien geändert, 18 Zeilen hinzugefügt(+), 21 Zeilen entfernt(-)
>>
> 
> Could we use cpus.h for CPUListState?
> 
> It has related list_cpus() and it doesn't included in any headers yet.
> And we won't pollute base CPU qom definition with utility structures.

I'd be open for another header, but it's probably orthogonal to the
qemu-common.h dependency issue.

Note however that I have been moving the QOM-cleaned-up declarations of
CPU functions to qemu/cpu.h as the canonical CPU header as well.

CPUListState does not contain CPUState in any way, it's more for the
target than for the actual CPU, so that's a good point to investigate.

>> diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
>> index d065085..915278f 100644
>> --- a/target-alpha/cpu.c
>> +++ b/target-alpha/cpu.c
>> @@ -33,11 +33,6 @@ static void alpha_cpu_realize(Object *obj, Error **err)
>>  #endif
>>  }
>>  
>> -typedef struct AlphaCPUListState {
>> -    fprintf_function cpu_fprintf;
>> -    FILE *file;
>> -} AlphaCPUListState;
>> -
>>  /* Sort alphabetically by type name. */
>>  static gint alpha_cpu_list_compare(gconstpointer a, gconstpointer b)
>>  {
>> @@ -53,7 +48,7 @@ static gint alpha_cpu_list_compare(gconstpointer a,
>> gconstpointer b) static void alpha_cpu_list_entry(gpointer data, gpointer
> Perhaps *cpu_list_entry() could be generalized too, they all look alike.

Actually, apart from varying indentation and whether or not they're
prefixed with the architecture, some print more information than just
the class name. In particular other targets print just the base name for
backwards compatibility (something that I could optionally change for
alpha, where we don't have any to keep).

What I was considering was to move to a general location a GCompareFunc
comparing just the ObjectClass names. But so far there's always
exceptions to that rule (any, host, PVR order, new vs. old naming
scheme, ...) so that no two are identical yet.

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH qom-cpu 1/4] cpu: Introduce CPUListState struct

[Eduardo Habkost]

On Tue, Dec 18, 2012 at 08:53:40AM +0100, Andreas Färber wrote:
> This generalizes {ARM,M68k,Alpha}CPUListState to avoid declaring it for
> each target.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  include/qemu/cpu.h   |   12 ++++++++++++
>  target-alpha/cpu.c   |    9 ++-------
>  target-arm/helper.c  |    9 ++-------
>  target-m68k/helper.c |    9 ++-------
>  4 Dateien geändert, 18 Zeilen hinzugefügt(+), 21 Zeilen entfernt(-)
> 
> diff --git a/include/qemu/cpu.h b/include/qemu/cpu.h
> index 61b7698..5fbb3f9 100644
> --- a/include/qemu/cpu.h
> +++ b/include/qemu/cpu.h
> @@ -21,6 +21,7 @@
>  #define QEMU_CPU_H
>  
>  #include "qemu/object.h"
> +#include "qemu-common.h"
>  #include "qemu-thread.h"

Please, don't add more "#include qemu-common.h" lines to header files.
This introduces yet another circular dependency:

qemu-common.h -> target-*/cpu.h -> target-*/cpu-qom.h -> qemu/cpu.h -> qemu-common.h

You could just reverse the order of patches 1/4 and 2/4, and include
"qemu-types.h" instead.

The rest of the patch is an obvious removal of duplicate code, that
would get a Reviewed-By line from me.

>  
>  /**
> @@ -80,6 +81,17 @@ struct CPUState {
>      /* TODO Move common fields from CPUArchState here. */
>  };
>  
> +/**
> + * CPUListState:
> + * @cpu_fprintf: Print function.
> + * @file: File to print to using @cpu_fprint.
> + *
> + * State commonly used for iterating over CPU models.
> + */
> +typedef struct CPUListState {
> +    fprintf_function cpu_fprintf;
> +    FILE *file;
> +} CPUListState;
>  
>  /**
>   * cpu_reset:
> diff --git a/target-alpha/cpu.c b/target-alpha/cpu.c
> index d065085..915278f 100644
> --- a/target-alpha/cpu.c
> +++ b/target-alpha/cpu.c
> @@ -33,11 +33,6 @@ static void alpha_cpu_realize(Object *obj, Error **err)
>  #endif
>  }
>  
> -typedef struct AlphaCPUListState {
> -    fprintf_function cpu_fprintf;
> -    FILE *file;
> -} AlphaCPUListState;
> -
>  /* Sort alphabetically by type name. */
>  static gint alpha_cpu_list_compare(gconstpointer a, gconstpointer b)
>  {
> @@ -53,7 +48,7 @@ static gint alpha_cpu_list_compare(gconstpointer a, gconstpointer b)
>  static void alpha_cpu_list_entry(gpointer data, gpointer user_data)
>  {
>      ObjectClass *oc = data;
> -    AlphaCPUListState *s = user_data;
> +    CPUListState *s = user_data;
>  
>      (*s->cpu_fprintf)(s->file, "  %s\n",
>                        object_class_get_name(oc));
> @@ -61,7 +56,7 @@ static void alpha_cpu_list_entry(gpointer data, gpointer user_data)
>  
>  void alpha_cpu_list(FILE *f, fprintf_function cpu_fprintf)
>  {
> -    AlphaCPUListState s = {
> +    CPUListState s = {
>          .file = f,
>          .cpu_fprintf = cpu_fprintf,
>      };
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index ab8b734..d2f2fb4 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1291,11 +1291,6 @@ ARMCPU *cpu_arm_init(const char *cpu_model)
>      return cpu;
>  }
>  
> -typedef struct ARMCPUListState {
> -    fprintf_function cpu_fprintf;
> -    FILE *file;
> -} ARMCPUListState;
> -
>  /* Sort alphabetically by type name, except for "any". */
>  static gint arm_cpu_list_compare(gconstpointer a, gconstpointer b)
>  {
> @@ -1317,7 +1312,7 @@ static gint arm_cpu_list_compare(gconstpointer a, gconstpointer b)
>  static void arm_cpu_list_entry(gpointer data, gpointer user_data)
>  {
>      ObjectClass *oc = data;
> -    ARMCPUListState *s = user_data;
> +    CPUListState *s = user_data;
>  
>      (*s->cpu_fprintf)(s->file, "  %s\n",
>                        object_class_get_name(oc));
> @@ -1325,7 +1320,7 @@ static void arm_cpu_list_entry(gpointer data, gpointer user_data)
>  
>  void arm_cpu_list(FILE *f, fprintf_function cpu_fprintf)
>  {
> -    ARMCPUListState s = {
> +    CPUListState s = {
>          .file = f,
>          .cpu_fprintf = cpu_fprintf,
>      };
> diff --git a/target-m68k/helper.c b/target-m68k/helper.c
> index a5d0100..875a71a 100644
> --- a/target-m68k/helper.c
> +++ b/target-m68k/helper.c
> @@ -25,11 +25,6 @@
>  
>  #define SIGNBIT (1u << 31)
>  
> -typedef struct M68kCPUListState {
> -    fprintf_function cpu_fprintf;
> -    FILE *file;
> -} M68kCPUListState;
> -
>  /* Sort alphabetically, except for "any". */
>  static gint m68k_cpu_list_compare(gconstpointer a, gconstpointer b)
>  {
> @@ -51,7 +46,7 @@ static gint m68k_cpu_list_compare(gconstpointer a, gconstpointer b)
>  static void m68k_cpu_list_entry(gpointer data, gpointer user_data)
>  {
>      ObjectClass *c = data;
> -    M68kCPUListState *s = user_data;
> +    CPUListState *s = user_data;
>  
>      (*s->cpu_fprintf)(s->file, "%s\n",
>                        object_class_get_name(c));
> @@ -59,7 +54,7 @@ static void m68k_cpu_list_entry(gpointer data, gpointer user_data)
>  
>  void m68k_cpu_list(FILE *f, fprintf_function cpu_fprintf)
>  {
> -    M68kCPUListState s = {
> +    CPUListState s = {
>          .file = f,
>          .cpu_fprintf = cpu_fprintf,
>      };
> -- 
> 1.7.10.4
> 
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH qom-cpu 1/4] cpu: Introduce CPUListState struct

[Andreas Färber]

Am 18.12.2012 18:42, schrieb Eduardo Habkost:
> On Tue, Dec 18, 2012 at 08:53:40AM +0100, Andreas Färber wrote:
>> diff --git a/include/qemu/cpu.h b/include/qemu/cpu.h
>> index 61b7698..5fbb3f9 100644
>> --- a/include/qemu/cpu.h
>> +++ b/include/qemu/cpu.h
>> @@ -21,6 +21,7 @@
>>  #define QEMU_CPU_H
>>  
>>  #include "qemu/object.h"
>> +#include "qemu-common.h"
>>  #include "qemu-thread.h"
> 
> Please, don't add more "#include qemu-common.h" lines to header files.
> This introduces yet another circular dependency:
> 
> qemu-common.h -> target-*/cpu.h -> target-*/cpu-qom.h -> qemu/cpu.h -> qemu-common.h

That's what 2/4 resolves. My reasoning was that this should be an
uncontroversial code-sharing change since, for good or bad,
qemu-common.h happens to be the place where this is defined today.

Whether to move it to qemu-types.h as proposed or to a new qemu-stdio.h
affects more than just the core CPU and thus me as maintainer and
requires careful mingw32 etc. testing.

> You could just reverse the order of patches 1/4 and 2/4, and include
> "qemu-types.h" instead.

If we find an agreeable solution by tomorrow for how/where to do it, sure!

Andreas

> The rest of the patch is an obvious removal of duplicate code, that
> would get a Reviewed-By line from me.

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH RFC qom-cpu 2/4] qemu-common.h: Move fprintf_function to qemu-types.h

[Andreas Färber]

This avoids a dependency on qemu-common.h from qemu/cpu.h.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 include/qemu/cpu.h |    2 +-
 qemu-common.h      |    5 -----
 qemu-types.h       |    6 ++++++
 3 Dateien geändert, 7 Zeilen hinzugefügt(+), 6 Zeilen entfernt(-)

diff --git a/include/qemu/cpu.h b/include/qemu/cpu.h
index 5fbb3f9..d737fcd 100644
--- a/include/qemu/cpu.h
+++ b/include/qemu/cpu.h
@@ -21,7 +21,7 @@
 #define QEMU_CPU_H
 
 #include "qemu/object.h"
-#include "qemu-common.h"
+#include "qemu-types.h"
 #include "qemu-thread.h"
 
 /**
diff --git a/qemu-common.h b/qemu-common.h
index e674786..98ab78d 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -12,7 +12,6 @@
 #ifndef QEMU_COMMON_H
 #define QEMU_COMMON_H
 
-#include "compiler.h"
 #include "config-host.h"
 #include "qemu-types.h"
 
@@ -24,7 +23,6 @@
 
 /* we put basic includes here to avoid repeating them in device drivers */
 #include <stdlib.h>
-#include <stdio.h>
 #include <stdarg.h>
 #include <stdbool.h>
 #include <string.h>
@@ -95,9 +93,6 @@ struct iovec {
 #include <sys/uio.h>
 #endif
 
-typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
-    GCC_FMT_ATTR(2, 3);
-
 #ifdef _WIN32
 #define fsync _commit
 #if !defined(lseek)
diff --git a/qemu-types.h b/qemu-types.h
index fd532a2..f7a7194 100644
--- a/qemu-types.h
+++ b/qemu-types.h
@@ -1,6 +1,12 @@
 #ifndef QEMU_TYPEDEFS_H
 #define QEMU_TYPEDEFS_H
 
+#include "compiler.h"
+#include <stdio.h>
+
+typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
+    GCC_FMT_ATTR(2, 3);
+
 /* A load of opaque types so that device init declarations don't have to
    pull in all the real definitions.  */
 typedef struct QEMUTimer QEMUTimer;
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH RFC qom-cpu 2/4] qemu-common.h: Move fprintf_function to qemu-types.h

[Eduardo Habkost]

On Tue, Dec 18, 2012 at 08:53:41AM +0100, Andreas Färber wrote:
> This avoids a dependency on qemu-common.h from qemu/cpu.h.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>

When I introduced qemu-types.h, I just planned to use it for the trivial
"typedef struct Foobar Foobar" typedefs that can't be anywhere else
because it would cause circular header dependencies.

I am not against this patch, but I wonder if osdep.h (or even a new
qemu-stdio.h header) isn't more appropriate for this (and for other
I/O-related definitions currently on qemu-common.h).

> ---
>  include/qemu/cpu.h |    2 +-
>  qemu-common.h      |    5 -----
>  qemu-types.h       |    6 ++++++
>  3 Dateien geändert, 7 Zeilen hinzugefügt(+), 6 Zeilen entfernt(-)
> 
> diff --git a/include/qemu/cpu.h b/include/qemu/cpu.h
> index 5fbb3f9..d737fcd 100644
> --- a/include/qemu/cpu.h
> +++ b/include/qemu/cpu.h
> @@ -21,7 +21,7 @@
>  #define QEMU_CPU_H
>  
>  #include "qemu/object.h"
> -#include "qemu-common.h"
> +#include "qemu-types.h"
>  #include "qemu-thread.h"
>  
>  /**
> diff --git a/qemu-common.h b/qemu-common.h
> index e674786..98ab78d 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -12,7 +12,6 @@
>  #ifndef QEMU_COMMON_H
>  #define QEMU_COMMON_H
>  
> -#include "compiler.h"
>  #include "config-host.h"
>  #include "qemu-types.h"
>  
> @@ -24,7 +23,6 @@
>  
>  /* we put basic includes here to avoid repeating them in device drivers */
>  #include <stdlib.h>
> -#include <stdio.h>
>  #include <stdarg.h>
>  #include <stdbool.h>
>  #include <string.h>
> @@ -95,9 +93,6 @@ struct iovec {
>  #include <sys/uio.h>
>  #endif
>  
> -typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
> -    GCC_FMT_ATTR(2, 3);
> -
>  #ifdef _WIN32
>  #define fsync _commit
>  #if !defined(lseek)
> diff --git a/qemu-types.h b/qemu-types.h
> index fd532a2..f7a7194 100644
> --- a/qemu-types.h
> +++ b/qemu-types.h
> @@ -1,6 +1,12 @@
>  #ifndef QEMU_TYPEDEFS_H
>  #define QEMU_TYPEDEFS_H
>  
> +#include "compiler.h"
> +#include <stdio.h>
> +
> +typedef int (*fprintf_function)(FILE *f, const char *fmt, ...)
> +    GCC_FMT_ATTR(2, 3);
> +
>  /* A load of opaque types so that device init declarations don't have to
>     pull in all the real definitions.  */
>  typedef struct QEMUTimer QEMUTimer;
> -- 
> 1.7.10.4
> 
> 

-- 
Eduardo

[Qemu-devel] [PATCH qom-cpu 3/4] target-ppc: Slim conversion of model definitions to QOM subclasses

[Andreas Färber]

Since the model list is highly macrofied, keep ppc_def_t for now and
save a pointer to it in PowerPCCPUClass. This results in a flat list of
subclasses including aliases, to be refined later.

Move cpu_ppc_init() to translate_init.c and drop helper.c.
Long-term the idea is to turn translate_init.c into a standalone cpu.c.

Inline cpu_ppc_usable() into type registration.

Split cpu_ppc_register() in two by code movement into the initfn and
by turning the remaining part into a realizefn.
Move qemu_init_vcpu() call into the new realizefn and adapt
create_ppc_opcodes() to return an Error.

Change ppc_find_by_pvr() -> ppc_cpu_class_by_pvr().
Change ppc_find_by_name() -> ppc_cpu_class_by_name().

Turn -cpu host into its own subclass. This requires to move the
kvm_enabled() check in ppc_cpu_class_by_name() to avoid the class being
found via the normal name lookup in the !kvm_enabled() case.
Turn kvmppc_host_cpu_def() into the class_init and add an initfn that
asserts KVM is in fact enabled.

Implement -cpu ? and the QMP equivalent in terms of subclasses.
This newly exposes -cpu host to the user, ordered last for -cpu ?.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 target-ppc/Makefile.objs    |    3 +-
 target-ppc/cpu-qom.h        |    5 +
 target-ppc/cpu.h            |    4 -
 target-ppc/helper.c         |   50 -------
 target-ppc/kvm.c            |   37 ++++-
 target-ppc/kvm_ppc.h        |    8 +-
 target-ppc/translate_init.c |  344 +++++++++++++++++++++++++++++--------------
 7 Dateien geändert, 275 Zeilen hinzugefügt(+), 176 Zeilen entfernt(-)
 delete mode 100644 target-ppc/helper.c

diff --git a/target-ppc/Makefile.objs b/target-ppc/Makefile.objs
index 237a0ed..15d2a3c 100644
--- a/target-ppc/Makefile.objs
+++ b/target-ppc/Makefile.objs
@@ -1,7 +1,6 @@
-obj-y += translate.o helper.o
+obj-y += translate.o
 obj-$(CONFIG_SOFTMMU) += machine.o
 obj-$(CONFIG_KVM) += kvm.o kvm_ppc.o
-obj-y += helper.o
 obj-y += excp_helper.o
 obj-y += fpu_helper.o
 obj-y += int_helper.o
diff --git a/target-ppc/cpu-qom.h b/target-ppc/cpu-qom.h
index fef6f95..f5c757f 100644
--- a/target-ppc/cpu-qom.h
+++ b/target-ppc/cpu-qom.h
@@ -50,6 +50,9 @@ typedef struct PowerPCCPUClass {
     /*< public >*/
 
     void (*parent_reset)(CPUState *cpu);
+
+    /* TODO inline fields here */
+    ppc_def_t *info;
 } PowerPCCPUClass;
 
 /**
@@ -73,5 +76,7 @@ static inline PowerPCCPU *ppc_env_get_cpu(CPUPPCState *env)
 
 #define ENV_GET_CPU(e) CPU(ppc_env_get_cpu(e))
 
+PowerPCCPUClass *ppc_cpu_class_by_pvr(uint32_t pvr);
+
 
 #endif
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 742d4f8..556080a 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1156,10 +1156,6 @@ void ppc_store_msr (CPUPPCState *env, target_ulong value);
 
 void ppc_cpu_list (FILE *f, fprintf_function cpu_fprintf);
 
-const ppc_def_t *ppc_find_by_pvr(uint32_t pvr);
-const ppc_def_t *cpu_ppc_find_by_name (const char *name);
-int cpu_ppc_register_internal (CPUPPCState *env, const ppc_def_t *def);
-
 /* Time-base and decrementer management */
 #ifndef NO_CPU_IO_DEFS
 uint64_t cpu_ppc_load_tbl (CPUPPCState *env);
diff --git a/target-ppc/helper.c b/target-ppc/helper.c
deleted file mode 100644
index 48b19a7..0000000
--- a/target-ppc/helper.c
+++ /dev/null
@@ -1,50 +0,0 @@
-/*
- *  PowerPC emulation helpers for QEMU.
- *
- *  Copyright (c) 2003-2007 Jocelyn Mayer
- *
- * This library is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public
- * License as published by the Free Software Foundation; either
- * version 2 of the License, or (at your option) any later version.
- *
- * This library is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public
- * License along with this library; if not, see <http://www.gnu.org/licenses/>.
- */
-
-#include "cpu.h"
-#include "helper_regs.h"
-#include "kvm.h"
-#include "kvm_ppc.h"
-#include "cpus.h"
-
-PowerPCCPU *cpu_ppc_init(const char *cpu_model)
-{
-    PowerPCCPU *cpu;
-    CPUPPCState *env;
-    const ppc_def_t *def;
-
-    def = cpu_ppc_find_by_name(cpu_model);
-    if (!def) {
-        return NULL;
-    }
-
-    cpu = POWERPC_CPU(object_new(TYPE_POWERPC_CPU));
-    env = &cpu->env;
-
-    if (tcg_enabled()) {
-        ppc_translate_init();
-    }
-
-    env->cpu_model_str = cpu_model;
-    cpu_ppc_register_internal(env, def);
-
-    qemu_init_vcpu(env);
-
-    return cpu;
-}
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index 3f5df57..f115892 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -1184,18 +1184,29 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on)
     }
 }
 
-const ppc_def_t *kvmppc_host_cpu_def(void)
+static void kvmppc_host_cpu_initfn(Object *obj)
 {
+    assert(kvm_enabled());
+}
+
+static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
+{
+    PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
     uint32_t host_pvr = mfpvr();
-    const ppc_def_t *base_spec;
+    PowerPCCPUClass *pvr_pcc;
     ppc_def_t *spec;
     uint32_t vmx = kvmppc_get_vmx();
     uint32_t dfp = kvmppc_get_dfp();
 
-    base_spec = ppc_find_by_pvr(host_pvr);
-
     spec = g_malloc0(sizeof(*spec));
-    memcpy(spec, base_spec, sizeof(*spec));
+
+    pvr_pcc = ppc_cpu_class_by_pvr(host_pvr);
+    if (pvr_pcc != NULL) {
+        memcpy(spec, pvr_pcc->info, sizeof(*spec));
+    }
+    pcc->info = spec;
+    /* Override the display name for -cpu ? and QMP */
+    pcc->info->name = "host";
 
     /* Now fix up the spec with information we can query from the host */
 
@@ -1208,8 +1219,6 @@ const ppc_def_t *kvmppc_host_cpu_def(void)
         /* Only override when we know what the host supports */
         alter_insns(&spec->insns_flags2, PPC2_DFP, dfp);
     }
-
-    return spec;
 }
 
 int kvmppc_fixup_cpu(CPUPPCState *env)
@@ -1239,3 +1248,17 @@ int kvm_arch_on_sigbus(int code, void *addr)
 {
     return 1;
 }
+
+static const TypeInfo kvm_host_cpu_type_info = {
+    .name = TYPE_HOST_POWERPC_CPU,
+    .parent = TYPE_POWERPC_CPU,
+    .instance_init = kvmppc_host_cpu_initfn,
+    .class_init = kvmppc_host_cpu_class_init,
+};
+
+static void kvm_ppc_register_types(void)
+{
+    type_register_static(&kvm_host_cpu_type_info);
+}
+
+type_init(kvm_ppc_register_types)
diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
index baad6eb..33d8cc2 100644
--- a/target-ppc/kvm_ppc.h
+++ b/target-ppc/kvm_ppc.h
@@ -11,6 +11,8 @@
 
 #include "memory.h"
 
+#define TYPE_HOST_POWERPC_CPU "host-" TYPE_POWERPC_CPU
+
 void kvmppc_init(void);
 
 #ifdef CONFIG_KVM
@@ -30,7 +32,6 @@ int kvmppc_remove_spapr_tce(void *table, int pfd, uint32_t window_size);
 int kvmppc_reset_htab(int shift_hint);
 uint64_t kvmppc_rma_size(uint64_t current_size, unsigned int hash_shift);
 #endif /* !CONFIG_USER_ONLY */
-const ppc_def_t *kvmppc_host_cpu_def(void);
 int kvmppc_fixup_cpu(CPUPPCState *env);
 
 #else
@@ -115,11 +116,6 @@ static inline int kvmppc_update_sdr1(CPUPPCState *env)
 
 #endif /* !CONFIG_USER_ONLY */
 
-static inline const ppc_def_t *kvmppc_host_cpu_def(void)
-{
-    return NULL;
-}
-
 static inline int kvmppc_fixup_cpu(CPUPPCState *env)
 {
     return -1;
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index e63627c..908a558 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -9797,8 +9797,11 @@ static void fix_opcode_tables (opc_handler_t **ppc_opcodes)
 }
 
 /*****************************************************************************/
-static int create_ppc_opcodes (CPUPPCState *env, const ppc_def_t *def)
+static void create_ppc_opcodes(PowerPCCPU *cpu, Error **errp)
 {
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+    CPUPPCState *env = &cpu->env;
+    const ppc_def_t *def = pcc->info;
     opcode_t *opc;
 
     fill_new_table(env->opcodes, 0x40);
@@ -9806,18 +9809,16 @@ static int create_ppc_opcodes (CPUPPCState *env, const ppc_def_t *def)
         if (((opc->handler.type & def->insns_flags) != 0) ||
             ((opc->handler.type2 & def->insns_flags2) != 0)) {
             if (register_insn(env->opcodes, opc) < 0) {
-                printf("*** ERROR initializing PowerPC instruction "
-                       "0x%02x 0x%02x 0x%02x\n", opc->opc1, opc->opc2,
-                       opc->opc3);
-                return -1;
+                error_setg(errp, "ERROR initializing PowerPC instruction "
+                           "0x%02x 0x%02x 0x%02x\n", opc->opc1, opc->opc2,
+                           opc->opc3);
+                return;
             }
         }
     }
     fix_opcode_tables(env->opcodes);
     fflush(stdout);
     fflush(stderr);
-
-    return 0;
 }
 
 #if defined(PPC_DUMP_CPU)
@@ -10031,53 +10032,31 @@ static int ppc_fixup_cpu(CPUPPCState *env)
     return 0;
 }
 
-int cpu_ppc_register_internal (CPUPPCState *env, const ppc_def_t *def)
+static void ppc_cpu_realize(Object *obj, Error **errp)
 {
-    env->msr_mask = def->msr_mask;
-    env->mmu_model = def->mmu_model;
-    env->excp_model = def->excp_model;
-    env->bus_model = def->bus_model;
-    env->insns_flags = def->insns_flags;
-    env->insns_flags2 = def->insns_flags2;
-    env->flags = def->flags;
-    env->bfd_mach = def->bfd_mach;
-    env->check_pow = def->check_pow;
-
-#if defined(TARGET_PPC64)
-    if (def->sps)
-        env->sps = *def->sps;
-    else if (env->mmu_model & POWERPC_MMU_64) {
-        /* Use default sets of page sizes */
-        static const struct ppc_segment_page_sizes defsps = {
-            .sps = {
-                { .page_shift = 12, /* 4K */
-                  .slb_enc = 0,
-                  .enc = { { .page_shift = 12, .pte_enc = 0 } }
-                },
-                { .page_shift = 24, /* 16M */
-                  .slb_enc = 0x100,
-                  .enc = { { .page_shift = 24, .pte_enc = 0 } }
-                },
-            },
-        };
-        env->sps = defsps;
-    }
-#endif /* defined(TARGET_PPC64) */
+    PowerPCCPU *cpu = POWERPC_CPU(obj);
+    CPUPPCState *env = &cpu->env;
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+    ppc_def_t *def = pcc->info;
+    Error *local_err = NULL;
 
     if (kvm_enabled()) {
         if (kvmppc_fixup_cpu(env) != 0) {
-            fprintf(stderr, "Unable to virtualize selected CPU with KVM\n");
-            exit(1);
+            error_setg(errp, "Unable to virtualize selected CPU with KVM");
+            return;
         }
     } else {
         if (ppc_fixup_cpu(env) != 0) {
-            fprintf(stderr, "Unable to emulate selected CPU with TCG\n");
-            exit(1);
+            error_setg(errp, "Unable to emulate selected CPU with TCG");
+            return;
         }
     }
 
-    if (create_ppc_opcodes(env, def) < 0)
-        return -1;
+    create_ppc_opcodes(cpu, &local_err);
+    if (local_err != NULL) {
+        error_propagate(errp, local_err);
+        return;
+    }
     init_ppc_proc(env, def);
 
     if (def->insns_flags & PPC_FLOAT) {
@@ -10093,6 +10072,8 @@ int cpu_ppc_register_internal (CPUPPCState *env, const ppc_def_t *def)
                                  34, "power-spe.xml", 0);
     }
 
+    qemu_init_vcpu(env);
+
 #if defined(PPC_DUMP_CPU)
     {
         const char *mmu_model, *excp_model, *bus_model;
@@ -10254,50 +10235,65 @@ int cpu_ppc_register_internal (CPUPPCState *env, const ppc_def_t *def)
     dump_ppc_sprs(env);
     fflush(stdout);
 #endif
-
-    return 0;
 }
 
-static bool ppc_cpu_usable(const ppc_def_t *def)
+static gint ppc_cpu_compare_class_pvr(gconstpointer a, gconstpointer b)
 {
-#if defined(TARGET_PPCEMB)
-    /* When using the ppcemb target, we only support 440 style cores */
-    if (def->mmu_model != POWERPC_MMU_BOOKE) {
-        return false;
+    ObjectClass *oc = (ObjectClass *)a;
+    uint32_t pvr = *(uint32_t *)b;
+    PowerPCCPUClass *pcc = (PowerPCCPUClass *)a;
+
+    /* -cpu host does a PVR lookup during construction */
+    if (unlikely(strcmp(object_class_get_name(oc),
+                        TYPE_HOST_POWERPC_CPU) == 0)) {
+        return -1;
     }
-#endif
 
-    return true;
+    return pcc->info->pvr == pvr ? 0 : -1;
 }
 
-const ppc_def_t *ppc_find_by_pvr(uint32_t pvr)
+PowerPCCPUClass *ppc_cpu_class_by_pvr(uint32_t pvr)
 {
-    int i;
+    GSList *list, *item;
+    PowerPCCPUClass *pcc = NULL;
 
-    for (i = 0; i < ARRAY_SIZE(ppc_defs); i++) {
-        if (!ppc_cpu_usable(&ppc_defs[i])) {
-            continue;
-        }
-
-        /* If we have an exact match, we're done */
-        if (pvr == ppc_defs[i].pvr) {
-            return &ppc_defs[i];
-        }
+    list = object_class_get_list(TYPE_POWERPC_CPU, false);
+    item = g_slist_find_custom(list, &pvr, ppc_cpu_compare_class_pvr);
+    if (item != NULL) {
+        pcc = POWERPC_CPU_CLASS(item->data);
     }
+    g_slist_free(list);
+
+    return pcc;
+}
+
+static gint ppc_cpu_compare_class_name(gconstpointer a, gconstpointer b)
+{
+    ObjectClass *oc = (ObjectClass *)a;
+    const char *name = b;
 
-    return NULL;
+    if (strncasecmp(name, object_class_get_name(oc), strlen(name)) == 0 &&
+        strcmp(object_class_get_name(oc) + strlen(name),
+               "-" TYPE_POWERPC_CPU) == 0) {
+        return 0;
+    }
+    return -1;
 }
 
 #include <ctype.h>
 
-const ppc_def_t *cpu_ppc_find_by_name (const char *name)
+static ObjectClass *ppc_cpu_class_by_name(const char *name)
 {
-    const ppc_def_t *ret;
+    GSList *list, *item;
+    ObjectClass *ret = NULL;
     const char *p;
-    int i, max, len;
+    int i, len;
 
-    if (kvm_enabled() && (strcasecmp(name, "host") == 0)) {
-        return kvmppc_host_cpu_def();
+    if (strcasecmp(name, "host") == 0) {
+        if (kvm_enabled()) {
+            ret = object_class_by_name(TYPE_HOST_POWERPC_CPU);
+        }
+        return ret;
     }
 
     /* Check if the given name is a PVR */
@@ -10312,63 +10308,151 @@ const ppc_def_t *cpu_ppc_find_by_name (const char *name)
             if (!qemu_isxdigit(*p++))
                 break;
         }
-        if (i == 8)
-            return ppc_find_by_pvr(strtoul(name, NULL, 16));
-    }
-    ret = NULL;
-    max = ARRAY_SIZE(ppc_defs);
-    for (i = 0; i < max; i++) {
-        if (!ppc_cpu_usable(&ppc_defs[i])) {
-            continue;
+        if (i == 8) {
+            ret = OBJECT_CLASS(ppc_cpu_class_by_pvr(strtoul(name, NULL, 16)));
+            return ret;
         }
+    }
 
-        if (strcasecmp(name, ppc_defs[i].name) == 0) {
-            ret = &ppc_defs[i];
-            break;
-        }
+    list = object_class_get_list(TYPE_POWERPC_CPU, false);
+    item = g_slist_find_custom(list, name, ppc_cpu_compare_class_name);
+    if (item != NULL) {
+        ret = OBJECT_CLASS(item->data);
     }
+    g_slist_free(list);
 
     return ret;
 }
 
-void ppc_cpu_list (FILE *f, fprintf_function cpu_fprintf)
+PowerPCCPU *cpu_ppc_init(const char *cpu_model)
 {
-    int i, max;
+    PowerPCCPU *cpu;
+    CPUPPCState *env;
+    ObjectClass *oc;
+    Error *err = NULL;
 
-    max = ARRAY_SIZE(ppc_defs);
-    for (i = 0; i < max; i++) {
-        if (!ppc_cpu_usable(&ppc_defs[i])) {
-            continue;
-        }
+    oc = ppc_cpu_class_by_name(cpu_model);
+    if (oc == NULL) {
+        return NULL;
+    }
 
-        (*cpu_fprintf)(f, "PowerPC %-16s PVR %08x\n",
-                       ppc_defs[i].name, ppc_defs[i].pvr);
+    cpu = POWERPC_CPU(object_new(object_class_get_name(oc)));
+    env = &cpu->env;
+
+    if (tcg_enabled()) {
+        ppc_translate_init();
     }
+
+    env->cpu_model_str = cpu_model;
+
+    ppc_cpu_realize(OBJECT(cpu), &err);
+    if (err != NULL) {
+        fprintf(stderr, "%s\n", error_get_pretty(err));
+        error_free(err);
+        object_delete(OBJECT(cpu));
+        return NULL;
+    }
+
+    return cpu;
+}
+
+/* Sort by PVR, ordering special case "host" last. */
+static gint ppc_cpu_list_compare(gconstpointer a, gconstpointer b)
+{
+    ObjectClass *oc_a = (ObjectClass *)a;
+    ObjectClass *oc_b = (ObjectClass *)b;
+    PowerPCCPUClass *pcc_a = POWERPC_CPU_CLASS(oc_a);
+    PowerPCCPUClass *pcc_b = POWERPC_CPU_CLASS(oc_b);
+    const char *name_a = object_class_get_name(oc_a);
+    const char *name_b = object_class_get_name(oc_b);
+
+    if (strcmp(name_a, TYPE_HOST_POWERPC_CPU) == 0) {
+        return 1;
+    } else if (strcmp(name_b, TYPE_HOST_POWERPC_CPU) == 0) {
+        return -1;
+    } else {
+        /* Avoid an integer overflow during subtraction */
+        if (pcc_a->info->pvr < pcc_b->info->pvr) {
+            return -1;
+        } else if (pcc_a->info->pvr > pcc_b->info->pvr) {
+            return 1;
+        } else {
+            return 0;
+        }
+    }
+}
+
+static void ppc_cpu_list_entry(gpointer data, gpointer user_data)
+{
+    ObjectClass *oc = data;
+    CPUListState *s = user_data;
+    PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
+
+    (*s->cpu_fprintf)(s->file, "PowerPC %-16s PVR %08x\n",
+                      pcc->info->name, pcc->info->pvr);
+}
+
+void ppc_cpu_list(FILE *f, fprintf_function cpu_fprintf)
+{
+    CPUListState s = {
+        .file = f,
+        .cpu_fprintf = cpu_fprintf,
+    };
+    GSList *list;
+
+    list = object_class_get_list(TYPE_POWERPC_CPU, false);
+    list = g_slist_sort(list, ppc_cpu_list_compare);
+    g_slist_foreach(list, ppc_cpu_list_entry, &s);
+    g_slist_free(list);
+}
+
+static void ppc_cpu_defs_entry(gpointer data, gpointer user_data)
+{
+    ObjectClass *oc = data;
+    CpuDefinitionInfoList **first = user_data;
+    PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
+    CpuDefinitionInfoList *entry;
+    CpuDefinitionInfo *info;
+
+    info = g_malloc0(sizeof(*info));
+    info->name = g_strdup(pcc->info->name);
+
+    entry = g_malloc0(sizeof(*entry));
+    entry->value = info;
+    entry->next = *first;
+    *first = entry;
 }
 
 CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
 {
     CpuDefinitionInfoList *cpu_list = NULL;
-    int i;
+    GSList *list;
 
-    for (i = 0; i < ARRAY_SIZE(ppc_defs); i++) {
-        CpuDefinitionInfoList *entry;
-        CpuDefinitionInfo *info;
+    list = object_class_get_list(TYPE_POWERPC_CPU, false);
+    g_slist_foreach(list, ppc_cpu_defs_entry, &cpu_list);
+    g_slist_free(list);
 
-        if (!ppc_cpu_usable(&ppc_defs[i])) {
-            continue;
-        }
+    return cpu_list;
+}
 
-        info = g_malloc0(sizeof(*info));
-        info->name = g_strdup(ppc_defs[i].name);
+static void ppc_cpu_def_class_init(ObjectClass *oc, void *data)
+{
+    PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
+    ppc_def_t *info = data;
 
-        entry = g_malloc0(sizeof(*entry));
-        entry->value = info;
-        entry->next = cpu_list;
-        cpu_list = entry;
-    }
+    pcc->info = info;
+}
 
-    return cpu_list;
+static void ppc_cpu_register_model(const ppc_def_t *def)
+{
+    TypeInfo type_info = {
+        .name = def->name,
+        .parent = TYPE_POWERPC_CPU,
+        .class_init = ppc_cpu_def_class_init,
+        .class_data = (void *)def,
+    };
+
+    type_register(&type_info);
 }
 
 /* CPUClass::reset() */
@@ -10439,9 +10523,42 @@ static void ppc_cpu_reset(CPUState *s)
 static void ppc_cpu_initfn(Object *obj)
 {
     PowerPCCPU *cpu = POWERPC_CPU(obj);
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     CPUPPCState *env = &cpu->env;
+    ppc_def_t *def = pcc->info;
 
     cpu_exec_init(env);
+
+    env->msr_mask = def->msr_mask;
+    env->mmu_model = def->mmu_model;
+    env->excp_model = def->excp_model;
+    env->bus_model = def->bus_model;
+    env->insns_flags = def->insns_flags;
+    env->insns_flags2 = def->insns_flags2;
+    env->flags = def->flags;
+    env->bfd_mach = def->bfd_mach;
+    env->check_pow = def->check_pow;
+
+#if defined(TARGET_PPC64)
+    if (def->sps) {
+        env->sps = *def->sps;
+    } else if (env->mmu_model & POWERPC_MMU_64) {
+        /* Use default sets of page sizes */
+        static const struct ppc_segment_page_sizes defsps = {
+            .sps = {
+                { .page_shift = 12, /* 4K */
+                  .slb_enc = 0,
+                  .enc = { { .page_shift = 12, .pte_enc = 0 } }
+                },
+                { .page_shift = 24, /* 16M */
+                  .slb_enc = 0x100,
+                  .enc = { { .page_shift = 24, .pte_enc = 0 } }
+                },
+            },
+        };
+        env->sps = defsps;
+    }
+#endif /* defined(TARGET_PPC64) */
 }
 
 static void ppc_cpu_class_init(ObjectClass *oc, void *data)
@@ -10458,14 +10575,27 @@ static const TypeInfo ppc_cpu_type_info = {
     .parent = TYPE_CPU,
     .instance_size = sizeof(PowerPCCPU),
     .instance_init = ppc_cpu_initfn,
-    .abstract = false,
+    .abstract = true,
     .class_size = sizeof(PowerPCCPUClass),
     .class_init = ppc_cpu_class_init,
 };
 
 static void ppc_cpu_register_types(void)
 {
+    int i;
+
     type_register_static(&ppc_cpu_type_info);
+
+    for (i = 0; i < ARRAY_SIZE(ppc_defs); i++) {
+        const ppc_def_t *def = &ppc_defs[i];
+#if defined(TARGET_PPCEMB)
+        /* When using the ppcemb target, we only support 440 style cores */
+        if (def->mmu_model != POWERPC_MMU_BOOKE) {
+            continue;
+        }
+#endif
+        ppc_cpu_register_model(def);
+    }
 }
 
 type_init(ppc_cpu_register_types)
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH qom-cpu 3/4] target-ppc: Slim conversion of model definitions to QOM subclasses

[Eduardo Habkost]

On Tue, Dec 18, 2012 at 08:53:42AM +0100, Andreas Färber wrote:
> Since the model list is highly macrofied, keep ppc_def_t for now and
> save a pointer to it in PowerPCCPUClass. This results in a flat list of
> subclasses including aliases, to be refined later.
> 
> Move cpu_ppc_init() to translate_init.c and drop helper.c.
> Long-term the idea is to turn translate_init.c into a standalone cpu.c.
> 
> Inline cpu_ppc_usable() into type registration.
> 
> Split cpu_ppc_register() in two by code movement into the initfn and
> by turning the remaining part into a realizefn.
> Move qemu_init_vcpu() call into the new realizefn and adapt
> create_ppc_opcodes() to return an Error.
> 
> Change ppc_find_by_pvr() -> ppc_cpu_class_by_pvr().
> Change ppc_find_by_name() -> ppc_cpu_class_by_name().
> 
> Turn -cpu host into its own subclass. This requires to move the
> kvm_enabled() check in ppc_cpu_class_by_name() to avoid the class being
> found via the normal name lookup in the !kvm_enabled() case.
> Turn kvmppc_host_cpu_def() into the class_init and add an initfn that
> asserts KVM is in fact enabled.
> 
> Implement -cpu ? and the QMP equivalent in terms of subclasses.
> This newly exposes -cpu host to the user, ordered last for -cpu ?.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
[...]
> +static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
> +{
> +    PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
>      uint32_t host_pvr = mfpvr();
> -    const ppc_def_t *base_spec;
> +    PowerPCCPUClass *pvr_pcc;
>      ppc_def_t *spec;
>      uint32_t vmx = kvmppc_get_vmx();
>      uint32_t dfp = kvmppc_get_dfp();
>  
> -    base_spec = ppc_find_by_pvr(host_pvr);
> -
>      spec = g_malloc0(sizeof(*spec));
> -    memcpy(spec, base_spec, sizeof(*spec));
> +
> +    pvr_pcc = ppc_cpu_class_by_pvr(host_pvr);

I wonder if it's really safe to do this inside class_init.

ppc_cpu_class_by_pvr() calls object_class_get_list(TYPE_POWERPC_CPU),
and TYPE_HOST_POWERPC_CPU will be included in that list, but hasn't
finished its initialization yet.

I see that you took care of checking for TYPE_HOST_POWERPC_CPU inside
ppc_cpu_compare_class_pvr(). So, it looks like this is OK today, but I
am not 100% sure this doesn't break any assumptions/requirements of QOM.



> +    if (pvr_pcc != NULL) {
> +        memcpy(spec, pvr_pcc->info, sizeof(*spec));
> +    }
> +    pcc->info = spec;
> +    /* Override the display name for -cpu ? and QMP */
> +    pcc->info->name = "host";
>  
>      /* Now fix up the spec with information we can query from the host */
>  
> @@ -1208,8 +1219,6 @@ const ppc_def_t *kvmppc_host_cpu_def(void)
>          /* Only override when we know what the host supports */
>          alter_insns(&spec->insns_flags2, PPC2_DFP, dfp);
>      }
> -
> -    return spec;
>  }
>  
>  int kvmppc_fixup_cpu(CPUPPCState *env)
[...]
> +static void ppc_cpu_list_entry(gpointer data, gpointer user_data)
> +{
> +    ObjectClass *oc = data;
> +    CPUListState *s = user_data;
> +    PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> +
> +    (*s->cpu_fprintf)(s->file, "PowerPC %-16s PVR %08x\n",
> +                      pcc->info->name, pcc->info->pvr);
> +}
> +
> +void ppc_cpu_list(FILE *f, fprintf_function cpu_fprintf)
> +{
> +    CPUListState s = {
> +        .file = f,
> +        .cpu_fprintf = cpu_fprintf,
> +    };
> +    GSList *list;
> +
> +    list = object_class_get_list(TYPE_POWERPC_CPU, false);
> +    list = g_slist_sort(list, ppc_cpu_list_compare);
> +    g_slist_foreach(list, ppc_cpu_list_entry, &s);
> +    g_slist_free(list);
> +}
> +
> +static void ppc_cpu_defs_entry(gpointer data, gpointer user_data)
> +{
> +    ObjectClass *oc = data;
> +    CpuDefinitionInfoList **first = user_data;
> +    PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> +    CpuDefinitionInfoList *entry;
> +    CpuDefinitionInfo *info;
> +
> +    info = g_malloc0(sizeof(*info));
> +    info->name = g_strdup(pcc->info->name);
> +
> +    entry = g_malloc0(sizeof(*entry));
> +    entry->value = info;
> +    entry->next = *first;
> +    *first = entry;
>  }
>  
>  CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
>  {
>      CpuDefinitionInfoList *cpu_list = NULL;
> -    int i;
> +    GSList *list;
>  
> -    for (i = 0; i < ARRAY_SIZE(ppc_defs); i++) {
> -        CpuDefinitionInfoList *entry;
> -        CpuDefinitionInfo *info;
> +    list = object_class_get_list(TYPE_POWERPC_CPU, false);
> +    g_slist_foreach(list, ppc_cpu_defs_entry, &cpu_list);
> +    g_slist_free(list);
>  
> -        if (!ppc_cpu_usable(&ppc_defs[i])) {
> -            continue;
> -        }
> +    return cpu_list;
> +}

I still think we could reuse arch_query_cpu_definitions() for both
"-cpu ?" and "query-cpu-definitions". But, anyway: we can unify that one
step at a time. This at least matches the same pattern used in other
architectures.

[...]

I didn't review the ppc-specific code that got moved around. I will try
to take a deeper look today, but I hope this gets reviewed by somebody
with more experience dealing with the target-ppc code.

-- 
Eduardo

Re: [Qemu-devel] [PATCH qom-cpu 3/4] target-ppc: Slim conversion of model definitions to QOM subclasses

[Andreas Färber]

Am 18.12.2012 08:53, schrieb Andreas Färber:
> +static gint ppc_cpu_compare_class_name(gconstpointer a, gconstpointer b)
> +{
> +    ObjectClass *oc = (ObjectClass *)a;
> +    const char *name = b;
>  
> -    return NULL;
> +    if (strncasecmp(name, object_class_get_name(oc), strlen(name)) == 0 &&
> +        strcmp(object_class_get_name(oc) + strlen(name),
> +               "-" TYPE_POWERPC_CPU) == 0) {
> +        return 0;
> +    }
> +    return -1;
>  }
>  
>  #include <ctype.h>
>  
> -const ppc_def_t *cpu_ppc_find_by_name (const char *name)
> +static ObjectClass *ppc_cpu_class_by_name(const char *name)
[...]
> +    list = object_class_get_list(TYPE_POWERPC_CPU, false);
> +    item = g_slist_find_custom(list, name, ppc_cpu_compare_class_name);
> +    if (item != NULL) {
> +        ret = OBJECT_CLASS(item->data);
>      }
> +    g_slist_free(list);
>  
>      return ret;
>  }
[...]
> +static void ppc_cpu_register_model(const ppc_def_t *def)
> +{
> +    TypeInfo type_info = {
> +        .name = def->name,
> +        .parent = TYPE_POWERPC_CPU,
> +        .class_init = ppc_cpu_def_class_init,
> +        .class_data = (void *)def,
> +    };
> +
> +    type_register(&type_info);
>  }

Err, needs the following fix:

diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 908a558..ad0763e 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -10446,13 +10446,14 @@ static void ppc_cpu_def_class_init(ObjectClass
*oc, void *data)
 static void ppc_cpu_register_model(const ppc_def_t *def)
 {
     TypeInfo type_info = {
-        .name = def->name,
         .parent = TYPE_POWERPC_CPU,
         .class_init = ppc_cpu_def_class_init,
         .class_data = (void *)def,
     };

+    type_info.name = g_strdup_printf("%s-" TYPE_POWERPC_CPU, def->name),
     type_register(&type_info);
+    g_free((gpointer)type_info.name);
 }

 /* CPUClass::reset() */

Not sure how I managed to screw this up so badly, seems I tested corner
cases and -cpu host too much...

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [PATCH qom-cpu 4/4] target-ppc: Error out for -cpu host on unknown PVR

[Andreas Färber]

Previously we silently exited, with subclasses we got an opcode warning.
Instead explicitly tell the user what's wrong.

An indication for this is -cpu ? showing "host" with an all-zero PVR.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 target-ppc/kvm.c |    7 +++++++
 1 Datei geändert, 7 Zeilen hinzugefügt(+)

diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index f115892..8998d0f 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -1186,7 +1186,14 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on)
 
 static void kvmppc_host_cpu_initfn(Object *obj)
 {
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(obj);
+
     assert(kvm_enabled());
+
+    if (pcc->info->pvr != mfpvr()) {
+        fprintf(stderr, "Host PVR unsupported.\n");
+        exit(1);
+    }
 }
 
 static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH qom-cpu 4/4] target-ppc: Error out for -cpu host on unknown PVR

[Alexander Graf]

On 18.12.2012, at 08:53, Andreas Färber wrote:

> Previously we silently exited, with subclasses we got an opcode warning.
> Instead explicitly tell the user what's wrong.
> 
> An indication for this is -cpu ? showing "host" with an all-zero PVR.
> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
> target-ppc/kvm.c |    7 +++++++
> 1 Datei geändert, 7 Zeilen hinzugefügt(+)
> 
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index f115892..8998d0f 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -1186,7 +1186,14 @@ static void alter_insns(uint64_t *word, uint64_t flags, bool on)
> 
> static void kvmppc_host_cpu_initfn(Object *obj)
> {
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(obj);
> +
>     assert(kvm_enabled());
> +
> +    if (pcc->info->pvr != mfpvr()) {
> +        fprintf(stderr, "Host PVR unsupported.\n");

This should probably rather say "Host CPU unsupported for -cpu host" or so :). Not everyone who invokes qemu-system-ppc knows what a PVR is.


Alex