[Qemu-devel] [PATCH v3 0/7] ppc: compute cpu_dt_id in the machine code

[Qemu-devel] [PATCH v3 0/7] ppc: compute cpu_dt_id in the machine code

[Greg Kurz]

This series is a sequel to the discussion on a patch from Ben's powernv
patchset:

        http://patchwork.ozlabs.org/patch/597153/

Indeed, since the DT is a machine abstraction, it should definitely sit
under hw/ppc and not in the target code:
- all machine types are forced to share the same numbering logic
- user mode does not need that => there are #ifdef everywhere

So this series moves all the current numbering logic to the machine
code.

This v3 is nearly a total rewrite (only patches 1/7 and 4/7 come from
v2). The most notable changes are:
- vcpu_dt_id is computed earlier instead of passing a cpu_index and
  a function pointer to the generic code
- parse cpu features only once (*)
- consolidate the vcpu_dt_id logic in sPAPR

(*) this depends on recent work by Igor. The code is not upstream yet,
    but should be in Eduardo Habkost's next pull request, later this
    week according to:

https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg00543.html

In the meantime, I've merged Eduardo's branch into master, added my
patches and pushed that to:

git://github.com/gkurz/qemu.git ppc-vcpu-dt-id-rework

Please comment.

---

Greg Kurz (7):
      ppc: different creation paths for cpus in system and user mode
      ppc: move smp_threads sanity checks to spapr
      ppc: parse cpu features once
      ppc: open code cpu creation for machine types
      ppc: each machine type to provide vcpu_dt_id
      ppc: drop vcpu_idt_id bits from the target code
      spapr: consolidate the logic of core cpu_dt_id


 hw/ppc/e500.c                   |    4 ++
 hw/ppc/mac_newworld.c           |    3 +-
 hw/ppc/mac_oldworld.c           |    3 +-
 hw/ppc/ppc.c                    |   64 +++++++++++++++++++++++++++++++++++++++
 hw/ppc/ppc440_bamboo.c          |    3 +-
 hw/ppc/ppc4xx_devs.c            |    3 +-
 hw/ppc/prep.c                   |    3 +-
 hw/ppc/spapr.c                  |   30 +++++++++++++-----
 hw/ppc/spapr_cpu_core.c         |   33 +++++++++++++++-----
 hw/ppc/virtex_ml507.c           |    3 +-
 include/hw/ppc/ppc.h            |    2 +
 include/hw/ppc/spapr_cpu_core.h |    9 +++++
 target-ppc/cpu.h                |    5 ++-
 target-ppc/translate_init.c     |   35 ---------------------
 14 files changed, 139 insertions(+), 61 deletions(-)

--
Greg

[Qemu-devel] [PATCH v3 1/7] ppc: different creation paths for cpus in system and user mode

[Greg Kurz]

The machine code currently uses the same cpu_ppc_init() function to
create cpus as the user mode. This function also triggers the cpu
realization.

It is okay for user mode but with system mode we may want to do other
things between initialization and realization, like generating cpu
ids for the DT for example.

With this patch, each mode has its own creation helper:
- ppc_cpu_init() is for system mode only
- cpu_init() is for user mode only


Suggested-by: Igor Mammedov <imammedo@redhat.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
v3: fixed ppc_cpu_init() invocation for virtex and ppc4xx (Igor)
---
 hw/ppc/e500.c               |    2 +-
 hw/ppc/mac_newworld.c       |    2 +-
 hw/ppc/mac_oldworld.c       |    2 +-
 hw/ppc/ppc.c                |    5 +++++
 hw/ppc/ppc440_bamboo.c      |    2 +-
 hw/ppc/ppc4xx_devs.c        |    2 +-
 hw/ppc/prep.c               |    2 +-
 hw/ppc/spapr.c              |    2 +-
 hw/ppc/virtex_ml507.c       |    2 +-
 include/hw/ppc/ppc.h        |    1 +
 target-ppc/cpu.h            |    5 +++--
 target-ppc/translate_init.c |    5 -----
 12 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 0cd534df55f8..ff5d92e48dd9 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -821,7 +821,7 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
         CPUState *cs;
         qemu_irq *input;
 
-        cpu = cpu_ppc_init(machine->cpu_model);
+        cpu = ppc_cpu_init(machine->cpu_model);
         if (cpu == NULL) {
             fprintf(stderr, "Unable to initialize CPU!\n");
             exit(1);
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 7d2510658d0f..ecd40914be45 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -193,7 +193,7 @@ static void ppc_core99_init(MachineState *machine)
 #endif
     }
     for (i = 0; i < smp_cpus; i++) {
-        cpu = cpu_ppc_init(machine->cpu_model);
+        cpu = ppc_cpu_init(machine->cpu_model);
         if (cpu == NULL) {
             fprintf(stderr, "Unable to find PowerPC CPU definition\n");
             exit(1);
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 447948746b1a..77fbdfffd4e2 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -113,7 +113,7 @@ static void ppc_heathrow_init(MachineState *machine)
     if (machine->cpu_model == NULL)
         machine->cpu_model = "G3";
     for (i = 0; i < smp_cpus; i++) {
-        cpu = cpu_ppc_init(machine->cpu_model);
+        cpu = ppc_cpu_init(machine->cpu_model);
         if (cpu == NULL) {
             fprintf(stderr, "Unable to find PowerPC CPU definition\n");
             exit(1);
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index e4252528a69d..dc3d214009c5 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -1350,3 +1350,8 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
 
     return NULL;
 }
+
+PowerPCCPU *ppc_cpu_init(const char *cpu_model)
+{
+    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model));
+}
diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
index 5c535b18a20d..7f22433c8e91 100644
--- a/hw/ppc/ppc440_bamboo.c
+++ b/hw/ppc/ppc440_bamboo.c
@@ -186,7 +186,7 @@ static void bamboo_init(MachineState *machine)
     if (machine->cpu_model == NULL) {
         machine->cpu_model = "440EP";
     }
-    cpu = cpu_ppc_init(machine->cpu_model);
+    cpu = ppc_cpu_init(machine->cpu_model);
     if (cpu == NULL) {
         fprintf(stderr, "Unable to initialize CPU!\n");
         exit(1);
diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
index e7f413e49d08..d48b61f04910 100644
--- a/hw/ppc/ppc4xx_devs.c
+++ b/hw/ppc/ppc4xx_devs.c
@@ -56,7 +56,7 @@ PowerPCCPU *ppc4xx_init(const char *cpu_model,
     CPUPPCState *env;
 
     /* init CPUs */
-    cpu = cpu_ppc_init(cpu_model);
+    cpu = ppc_cpu_init(cpu_model);
     if (cpu == NULL) {
         fprintf(stderr, "Unable to find PowerPC %s CPU definition\n",
                 cpu_model);
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 054af1e8b481..e62fe643f492 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -509,7 +509,7 @@ static void ppc_prep_init(MachineState *machine)
     if (machine->cpu_model == NULL)
         machine->cpu_model = "602";
     for (i = 0; i < smp_cpus; i++) {
-        cpu = cpu_ppc_init(machine->cpu_model);
+        cpu = ppc_cpu_init(machine->cpu_model);
         if (cpu == NULL) {
             fprintf(stderr, "Unable to find PowerPC CPU definition\n");
             exit(1);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 7f33a1b2b57d..d134eb2f338e 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1835,7 +1835,7 @@ static void ppc_spapr_init(MachineState *machine)
         g_free(type);
     } else {
         for (i = 0; i < smp_cpus; i++) {
-            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
+            PowerPCCPU *cpu = ppc_cpu_init(machine->cpu_model);
             if (cpu == NULL) {
                 error_report("Unable to find PowerPC CPU definition");
                 exit(1);
diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
index b97d96685cf1..7e4445b6879a 100644
--- a/hw/ppc/virtex_ml507.c
+++ b/hw/ppc/virtex_ml507.c
@@ -96,7 +96,7 @@ static PowerPCCPU *ppc440_init_xilinx(ram_addr_t *ram_size,
     CPUPPCState *env;
     qemu_irq *irqs;
 
-    cpu = cpu_ppc_init(cpu_model);
+    cpu = ppc_cpu_init(cpu_model);
     if (cpu == NULL) {
         fprintf(stderr, "Unable to initialize CPU!\n");
         exit(1);
diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
index 5617dc4a2c04..a4db1db82e1b 100644
--- a/include/hw/ppc/ppc.h
+++ b/include/hw/ppc/ppc.h
@@ -106,4 +106,5 @@ enum {
 /* ppc_booke.c */
 void ppc_booke_timers_init(PowerPCCPU *cpu, uint32_t freq, uint32_t flags);
 
+PowerPCCPU *ppc_cpu_init(const char *cpu_model);
 #endif
diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
index 2666a3f80d00..63e586298296 100644
--- a/target-ppc/cpu.h
+++ b/target-ppc/cpu.h
@@ -1198,7 +1198,6 @@ extern const struct VMStateDescription vmstate_ppc_cpu;
 #endif
 
 /*****************************************************************************/
-PowerPCCPU *cpu_ppc_init(const char *cpu_model);
 void ppc_translate_init(void);
 void gen_update_current_nip(void *opaque);
 /* you can call this signal handler from your SIGBUS and SIGSEGV
@@ -1277,7 +1276,9 @@ static inline uint64_t ppc_dump_gpr(CPUPPCState *env, int gprn)
 int ppc_dcr_read (ppc_dcr_t *dcr_env, int dcrn, uint32_t *valp);
 int ppc_dcr_write (ppc_dcr_t *dcr_env, int dcrn, uint32_t val);
 
-#define cpu_init(cpu_model) CPU(cpu_ppc_init(cpu_model))
+#if defined(CONFIG_USER_ONLY)
+#define cpu_init(cpu_model) cpu_generic_init(TYPE_POWERPC_CPU, cpu_model)
+#endif
 
 #define cpu_signal_handler cpu_ppc_signal_handler
 #define cpu_list ppc_cpu_list
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 8f257fb74aa7..31120a5aaf33 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -10012,11 +10012,6 @@ static ObjectClass *ppc_cpu_class_by_name(const char *name)
     return NULL;
 }
 
-PowerPCCPU *cpu_ppc_init(const char *cpu_model)
-{
-    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model));
-}
-
 /* Sort by PVR, ordering special case "host" last. */
 static gint ppc_cpu_list_compare(gconstpointer a, gconstpointer b)
 {

Re: [Qemu-devel] [PATCH v3 1/7] ppc: different creation paths for cpus in system and user mode

[David Gibson]

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

On Wed, Jul 06, 2016 at 02:13:14PM +0200, Greg Kurz wrote:
> The machine code currently uses the same cpu_ppc_init() function to
> create cpus as the user mode. This function also triggers the cpu
> realization.
> 
> It is okay for user mode but with system mode we may want to do other
> things between initialization and realization, like generating cpu
> ids for the DT for example.
> 
> With this patch, each mode has its own creation helper:
> - ppc_cpu_init() is for system mode only
> - cpu_init() is for user mode only
> 
> 
> Suggested-by: Igor Mammedov <imammedo@redhat.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>

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

> ---
> v3: fixed ppc_cpu_init() invocation for virtex and ppc4xx (Igor)
> ---
>  hw/ppc/e500.c               |    2 +-
>  hw/ppc/mac_newworld.c       |    2 +-
>  hw/ppc/mac_oldworld.c       |    2 +-
>  hw/ppc/ppc.c                |    5 +++++
>  hw/ppc/ppc440_bamboo.c      |    2 +-
>  hw/ppc/ppc4xx_devs.c        |    2 +-
>  hw/ppc/prep.c               |    2 +-
>  hw/ppc/spapr.c              |    2 +-
>  hw/ppc/virtex_ml507.c       |    2 +-
>  include/hw/ppc/ppc.h        |    1 +
>  target-ppc/cpu.h            |    5 +++--
>  target-ppc/translate_init.c |    5 -----
>  12 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 0cd534df55f8..ff5d92e48dd9 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -821,7 +821,7 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
>          CPUState *cs;
>          qemu_irq *input;
>  
> -        cpu = cpu_ppc_init(machine->cpu_model);
> +        cpu = ppc_cpu_init(machine->cpu_model);
>          if (cpu == NULL) {
>              fprintf(stderr, "Unable to initialize CPU!\n");
>              exit(1);
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index 7d2510658d0f..ecd40914be45 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -193,7 +193,7 @@ static void ppc_core99_init(MachineState *machine)
>  #endif
>      }
>      for (i = 0; i < smp_cpus; i++) {
> -        cpu = cpu_ppc_init(machine->cpu_model);
> +        cpu = ppc_cpu_init(machine->cpu_model);
>          if (cpu == NULL) {
>              fprintf(stderr, "Unable to find PowerPC CPU definition\n");
>              exit(1);
> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> index 447948746b1a..77fbdfffd4e2 100644
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -113,7 +113,7 @@ static void ppc_heathrow_init(MachineState *machine)
>      if (machine->cpu_model == NULL)
>          machine->cpu_model = "G3";
>      for (i = 0; i < smp_cpus; i++) {
> -        cpu = cpu_ppc_init(machine->cpu_model);
> +        cpu = ppc_cpu_init(machine->cpu_model);
>          if (cpu == NULL) {
>              fprintf(stderr, "Unable to find PowerPC CPU definition\n");
>              exit(1);
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index e4252528a69d..dc3d214009c5 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -1350,3 +1350,8 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
>  
>      return NULL;
>  }
> +
> +PowerPCCPU *ppc_cpu_init(const char *cpu_model)
> +{
> +    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model));
> +}
> diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
> index 5c535b18a20d..7f22433c8e91 100644
> --- a/hw/ppc/ppc440_bamboo.c
> +++ b/hw/ppc/ppc440_bamboo.c
> @@ -186,7 +186,7 @@ static void bamboo_init(MachineState *machine)
>      if (machine->cpu_model == NULL) {
>          machine->cpu_model = "440EP";
>      }
> -    cpu = cpu_ppc_init(machine->cpu_model);
> +    cpu = ppc_cpu_init(machine->cpu_model);
>      if (cpu == NULL) {
>          fprintf(stderr, "Unable to initialize CPU!\n");
>          exit(1);
> diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
> index e7f413e49d08..d48b61f04910 100644
> --- a/hw/ppc/ppc4xx_devs.c
> +++ b/hw/ppc/ppc4xx_devs.c
> @@ -56,7 +56,7 @@ PowerPCCPU *ppc4xx_init(const char *cpu_model,
>      CPUPPCState *env;
>  
>      /* init CPUs */
> -    cpu = cpu_ppc_init(cpu_model);
> +    cpu = ppc_cpu_init(cpu_model);
>      if (cpu == NULL) {
>          fprintf(stderr, "Unable to find PowerPC %s CPU definition\n",
>                  cpu_model);
> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index 054af1e8b481..e62fe643f492 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -509,7 +509,7 @@ static void ppc_prep_init(MachineState *machine)
>      if (machine->cpu_model == NULL)
>          machine->cpu_model = "602";
>      for (i = 0; i < smp_cpus; i++) {
> -        cpu = cpu_ppc_init(machine->cpu_model);
> +        cpu = ppc_cpu_init(machine->cpu_model);
>          if (cpu == NULL) {
>              fprintf(stderr, "Unable to find PowerPC CPU definition\n");
>              exit(1);
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 7f33a1b2b57d..d134eb2f338e 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1835,7 +1835,7 @@ static void ppc_spapr_init(MachineState *machine)
>          g_free(type);
>      } else {
>          for (i = 0; i < smp_cpus; i++) {
> -            PowerPCCPU *cpu = cpu_ppc_init(machine->cpu_model);
> +            PowerPCCPU *cpu = ppc_cpu_init(machine->cpu_model);
>              if (cpu == NULL) {
>                  error_report("Unable to find PowerPC CPU definition");
>                  exit(1);
> diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
> index b97d96685cf1..7e4445b6879a 100644
> --- a/hw/ppc/virtex_ml507.c
> +++ b/hw/ppc/virtex_ml507.c
> @@ -96,7 +96,7 @@ static PowerPCCPU *ppc440_init_xilinx(ram_addr_t *ram_size,
>      CPUPPCState *env;
>      qemu_irq *irqs;
>  
> -    cpu = cpu_ppc_init(cpu_model);
> +    cpu = ppc_cpu_init(cpu_model);
>      if (cpu == NULL) {
>          fprintf(stderr, "Unable to initialize CPU!\n");
>          exit(1);
> diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
> index 5617dc4a2c04..a4db1db82e1b 100644
> --- a/include/hw/ppc/ppc.h
> +++ b/include/hw/ppc/ppc.h
> @@ -106,4 +106,5 @@ enum {
>  /* ppc_booke.c */
>  void ppc_booke_timers_init(PowerPCCPU *cpu, uint32_t freq, uint32_t flags);
>  
> +PowerPCCPU *ppc_cpu_init(const char *cpu_model);
>  #endif
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 2666a3f80d00..63e586298296 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1198,7 +1198,6 @@ extern const struct VMStateDescription vmstate_ppc_cpu;
>  #endif
>  
>  /*****************************************************************************/
> -PowerPCCPU *cpu_ppc_init(const char *cpu_model);
>  void ppc_translate_init(void);
>  void gen_update_current_nip(void *opaque);
>  /* you can call this signal handler from your SIGBUS and SIGSEGV
> @@ -1277,7 +1276,9 @@ static inline uint64_t ppc_dump_gpr(CPUPPCState *env, int gprn)
>  int ppc_dcr_read (ppc_dcr_t *dcr_env, int dcrn, uint32_t *valp);
>  int ppc_dcr_write (ppc_dcr_t *dcr_env, int dcrn, uint32_t val);
>  
> -#define cpu_init(cpu_model) CPU(cpu_ppc_init(cpu_model))
> +#if defined(CONFIG_USER_ONLY)
> +#define cpu_init(cpu_model) cpu_generic_init(TYPE_POWERPC_CPU, cpu_model)
> +#endif
>  
>  #define cpu_signal_handler cpu_ppc_signal_handler
>  #define cpu_list ppc_cpu_list
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 8f257fb74aa7..31120a5aaf33 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -10012,11 +10012,6 @@ static ObjectClass *ppc_cpu_class_by_name(const char *name)
>      return NULL;
>  }
>  
> -PowerPCCPU *cpu_ppc_init(const char *cpu_model)
> -{
> -    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model));
> -}
> -
>  /* Sort by PVR, ordering special case "host" last. */
>  static gint ppc_cpu_list_compare(gconstpointer a, gconstpointer b)
>  {
> 

-- 
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 v3 2/7] ppc: move smp_threads sanity checks to spapr

[Greg Kurz]

Only POWER5 and newer PowerPC cpus from IBM have SMT capabilities.
Since they are only supported by pseries, let's move the checks to
ppc_spapr_init().

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c              |   12 ++++++++++++
 target-ppc/translate_init.c |   14 --------------
 2 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index d134eb2f338e..09600fee19b2 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1739,6 +1739,18 @@ static void ppc_spapr_init(MachineState *machine)
         }
     }
 
+    if (smp_threads > smt) {
+        error_report("Cannot support more than %d threads on PPC with %s",
+                     smt, kvm_enabled() ? "KVM" : "TCG");
+        exit(1);
+    }
+    if (!is_power_of_2(smp_threads)) {
+        error_report("Cannot support %d threads on PPC with %s, "
+                     "threads count must be a power of 2.",
+                     smp_threads, kvm_enabled() ? "KVM" : "TCG");
+        exit(1);
+    }
+
     msi_nonbroken = true;
 
     QLIST_INIT(&spapr->phbs);
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 31120a5aaf33..775df72cf6c2 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -9531,20 +9531,6 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
     int max_smt = kvmppc_smt_threads();
 #endif
 
-#if !defined(CONFIG_USER_ONLY)
-    if (smp_threads > max_smt) {
-        error_setg(errp, "Cannot support more than %d threads on PPC with %s",
-                   max_smt, kvm_enabled() ? "KVM" : "TCG");
-        return;
-    }
-    if (!is_power_of_2(smp_threads)) {
-        error_setg(errp, "Cannot support %d threads on PPC with %s, "
-                   "threads count must be a power of 2.",
-                   smp_threads, kvm_enabled() ? "KVM" : "TCG");
-        return;
-    }
-#endif
-
     cpu_exec_init(cs, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);

Re: [Qemu-devel] [PATCH v3 2/7] ppc: move smp_threads sanity checks to spapr

[David Gibson]

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

On Wed, Jul 06, 2016 at 02:13:30PM +0200, Greg Kurz wrote:
> Only POWER5 and newer PowerPC cpus from IBM have SMT capabilities.
> Since they are only supported by pseries, let's move the checks to
> ppc_spapr_init().
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>

I'm not comfortable with this.  SMT may only be used for pseries at
the moment, but the SMT possibilities are absolutely a constraint of
the CPU itself, not the machine type.  It seems more logical to me to
check this in the CPU code.

> ---
>  hw/ppc/spapr.c              |   12 ++++++++++++
>  target-ppc/translate_init.c |   14 --------------
>  2 files changed, 12 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index d134eb2f338e..09600fee19b2 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1739,6 +1739,18 @@ static void ppc_spapr_init(MachineState *machine)
>          }
>      }
>  
> +    if (smp_threads > smt) {
> +        error_report("Cannot support more than %d threads on PPC with %s",
> +                     smt, kvm_enabled() ? "KVM" : "TCG");
> +        exit(1);
> +    }
> +    if (!is_power_of_2(smp_threads)) {
> +        error_report("Cannot support %d threads on PPC with %s, "
> +                     "threads count must be a power of 2.",
> +                     smp_threads, kvm_enabled() ? "KVM" : "TCG");
> +        exit(1);
> +    }
> +
>      msi_nonbroken = true;
>  
>      QLIST_INIT(&spapr->phbs);
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 31120a5aaf33..775df72cf6c2 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -9531,20 +9531,6 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
>      int max_smt = kvmppc_smt_threads();
>  #endif
>  
> -#if !defined(CONFIG_USER_ONLY)
> -    if (smp_threads > max_smt) {
> -        error_setg(errp, "Cannot support more than %d threads on PPC with %s",
> -                   max_smt, kvm_enabled() ? "KVM" : "TCG");
> -        return;
> -    }
> -    if (!is_power_of_2(smp_threads)) {
> -        error_setg(errp, "Cannot support %d threads on PPC with %s, "
> -                   "threads count must be a power of 2.",
> -                   smp_threads, kvm_enabled() ? "KVM" : "TCG");
> -        return;
> -    }
> -#endif
> -
>      cpu_exec_init(cs, &local_err);
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
> 

-- 
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 v3 2/7] ppc: move smp_threads sanity checks to spapr

[Benjamin Herrenschmidt]

On Thu, 2016-07-07 at 11:12 +1000, David Gibson wrote:
> I'm not comfortable with this.  SMT may only be used for pseries at
> the moment, but the SMT possibilities are absolutely a constraint of
> the CPU itself, not the machine type.  It seems more logical to me to
> check this in the CPU code.

Also powernv uses them too no ?

Ben.

Re: [Qemu-devel] [PATCH v3 2/7] ppc: move smp_threads sanity checks to spapr

[David Gibson]

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

On Thu, Jul 07, 2016 at 11:57:32AM +1000, Benjamin Herrenschmidt wrote:
> On Thu, 2016-07-07 at 11:12 +1000, David Gibson wrote:
> > I'm not comfortable with this.  SMT may only be used for pseries at
> > the moment, but the SMT possibilities are absolutely a constraint of
> > the CPU itself, not the machine type.  It seems more logical to me to
> > check this in the CPU code.
> 
> Also powernv uses them too no ?

Well, powernv isn't in yet, but yes it will.

-- 
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 v3 2/7] ppc: move smp_threads sanity checks to spapr

[Greg Kurz]

On Thu, 07 Jul 2016 11:57:32 +1000
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> On Thu, 2016-07-07 at 11:12 +1000, David Gibson wrote:
> > I'm not comfortable with this.  SMT may only be used for pseries at
> > the moment, but the SMT possibilities are absolutely a constraint of
> > the CPU itself, not the machine type.  It seems more logical to me to
> > check this in the CPU code.  
> 
> Also powernv uses them too no ?
> 
> Ben.
> 

Hi Ben !

I've started to play with Cedric's tree on github. I'll work on adapting
powernv to this series.

Cheers.

--
Greg

Re: [Qemu-devel] [PATCH v3 2/7] ppc: move smp_threads sanity checks to spapr

[Greg Kurz]

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

On Thu, 7 Jul 2016 11:12:42 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Jul 06, 2016 at 02:13:30PM +0200, Greg Kurz wrote:
> > Only POWER5 and newer PowerPC cpus from IBM have SMT capabilities.
> > Since they are only supported by pseries, let's move the checks to
> > ppc_spapr_init().
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>  
> 
> I'm not comfortable with this.  SMT may only be used for pseries at
> the moment, but the SMT possibilities are absolutely a constraint of
> the CPU itself, not the machine type.  It seems more logical to me to
> check this in the CPU code.
> 

Yes, you're right. Ans since this isn't a prerequisite to compute cpu_dt_id,
it can stay in ppc_cpu_realizefn().

I'll drop this patch in v4.

> > ---
> >  hw/ppc/spapr.c              |   12 ++++++++++++
> >  target-ppc/translate_init.c |   14 --------------
> >  2 files changed, 12 insertions(+), 14 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index d134eb2f338e..09600fee19b2 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1739,6 +1739,18 @@ static void ppc_spapr_init(MachineState *machine)
> >          }
> >      }
> >  
> > +    if (smp_threads > smt) {
> > +        error_report("Cannot support more than %d threads on PPC with %s",
> > +                     smt, kvm_enabled() ? "KVM" : "TCG");
> > +        exit(1);
> > +    }
> > +    if (!is_power_of_2(smp_threads)) {
> > +        error_report("Cannot support %d threads on PPC with %s, "
> > +                     "threads count must be a power of 2.",
> > +                     smp_threads, kvm_enabled() ? "KVM" : "TCG");
> > +        exit(1);
> > +    }
> > +
> >      msi_nonbroken = true;
> >  
> >      QLIST_INIT(&spapr->phbs);
> > diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> > index 31120a5aaf33..775df72cf6c2 100644
> > --- a/target-ppc/translate_init.c
> > +++ b/target-ppc/translate_init.c
> > @@ -9531,20 +9531,6 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
> >      int max_smt = kvmppc_smt_threads();
> >  #endif
> >  
> > -#if !defined(CONFIG_USER_ONLY)
> > -    if (smp_threads > max_smt) {
> > -        error_setg(errp, "Cannot support more than %d threads on PPC with %s",
> > -                   max_smt, kvm_enabled() ? "KVM" : "TCG");
> > -        return;
> > -    }
> > -    if (!is_power_of_2(smp_threads)) {
> > -        error_setg(errp, "Cannot support %d threads on PPC with %s, "
> > -                   "threads count must be a power of 2.",
> > -                   smp_threads, kvm_enabled() ? "KVM" : "TCG");
> > -        return;
> > -    }
> > -#endif
> > -
> >      cpu_exec_init(cs, &local_err);
> >      if (local_err != NULL) {
> >          error_propagate(errp, local_err);
> >   
> 


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

[Qemu-devel] [PATCH v3 3/7] ppc: parse cpu features once

[Greg Kurz]

Considering that features are converted to global properties and
global properties are automatically applied to every new instance
of created CPU (at object_new() time), there is no point in
parsing cpu_model string every time a CPU created. So move
parsing outside CPU creation loop and do it only once.

Parsing also should be done before any CPU is created so that
features would affect the first CPU a well.

This patch does that for all PowerPC machine types.

It is based on previous work from Bharata:

https://lists.nongnu.org/archive/html/qemu-devel/2016-06/msg07564.html

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/e500.c          |    2 ++
 hw/ppc/mac_newworld.c  |    1 +
 hw/ppc/mac_oldworld.c  |    1 +
 hw/ppc/ppc.c           |   26 ++++++++++++++++++++++++++
 hw/ppc/ppc440_bamboo.c |    1 +
 hw/ppc/ppc4xx_devs.c   |    1 +
 hw/ppc/prep.c          |    1 +
 hw/ppc/spapr.c         |    2 ++
 hw/ppc/virtex_ml507.c  |    1 +
 include/hw/ppc/ppc.h   |    1 +
 10 files changed, 37 insertions(+)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index ff5d92e48dd9..b9221cc2c14a 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -814,6 +814,8 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
         machine->cpu_model = "e500v2_v30";
     }
 
+    ppc_cpu_parse_features(machine->cpu_model);
+
     irqs = g_malloc0(smp_cpus * sizeof(qemu_irq *));
     irqs[0] = g_malloc0(smp_cpus * sizeof(qemu_irq) * OPENPIC_OUTPUT_NB);
     for (i = 0; i < smp_cpus; i++) {
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index ecd40914be45..366089085844 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -192,6 +192,7 @@ static void ppc_core99_init(MachineState *machine)
         machine->cpu_model = "G4";
 #endif
     }
+    ppc_cpu_parse_features(machine->cpu_model);
     for (i = 0; i < smp_cpus; i++) {
         cpu = ppc_cpu_init(machine->cpu_model);
         if (cpu == NULL) {
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 77fbdfffd4e2..5c2dc53cb584 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -112,6 +112,7 @@ static void ppc_heathrow_init(MachineState *machine)
     /* init CPUs */
     if (machine->cpu_model == NULL)
         machine->cpu_model = "G3";
+    ppc_cpu_parse_features(machine->cpu_model);
     for (i = 0; i < smp_cpus; i++) {
         cpu = ppc_cpu_init(machine->cpu_model);
         if (cpu == NULL) {
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index dc3d214009c5..313b3f0b9a51 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -33,6 +33,7 @@
 #include "hw/timer/m48t59.h"
 #include "qemu/log.h"
 #include "qemu/error-report.h"
+#include "qapi/error.h"
 #include "hw/loader.h"
 #include "sysemu/kvm.h"
 #include "kvm_ppc.h"
@@ -1355,3 +1356,28 @@ PowerPCCPU *ppc_cpu_init(const char *cpu_model)
 {
     return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model));
 }
+
+void ppc_cpu_parse_features(const char *cpu_model)
+{
+    CPUClass *cc;
+    ObjectClass *oc;
+    const char *typename;
+    gchar **model_pieces;
+
+    model_pieces = g_strsplit(cpu_model, ",", 2);
+    if (!model_pieces[0]) {
+        error_report("Invalid/empty CPU model name");
+        exit(1);
+    }
+
+    oc = cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]);
+    if (oc == NULL) {
+        error_report("Unable to find CPU definition: %s", model_pieces[0]);
+        exit(1);
+    }
+
+    typename = object_class_get_name(oc);
+    cc = CPU_CLASS(oc);
+    cc->parse_features(typename, model_pieces[1], &error_fatal);
+    g_strfreev(model_pieces);
+}
diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
index 7f22433c8e91..709a779cba7e 100644
--- a/hw/ppc/ppc440_bamboo.c
+++ b/hw/ppc/ppc440_bamboo.c
@@ -186,6 +186,7 @@ static void bamboo_init(MachineState *machine)
     if (machine->cpu_model == NULL) {
         machine->cpu_model = "440EP";
     }
+    ppc_cpu_parse_features(machine->cpu_model);
     cpu = ppc_cpu_init(machine->cpu_model);
     if (cpu == NULL) {
         fprintf(stderr, "Unable to initialize CPU!\n");
diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
index d48b61f04910..8cbe50361e86 100644
--- a/hw/ppc/ppc4xx_devs.c
+++ b/hw/ppc/ppc4xx_devs.c
@@ -56,6 +56,7 @@ PowerPCCPU *ppc4xx_init(const char *cpu_model,
     CPUPPCState *env;
 
     /* init CPUs */
+    ppc_cpu_parse_features(cpu_model);
     cpu = ppc_cpu_init(cpu_model);
     if (cpu == NULL) {
         fprintf(stderr, "Unable to find PowerPC %s CPU definition\n",
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index e62fe643f492..99409a09f0ef 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -508,6 +508,7 @@ static void ppc_prep_init(MachineState *machine)
     /* init CPUs */
     if (machine->cpu_model == NULL)
         machine->cpu_model = "602";
+    ppc_cpu_parse_features(machine->cpu_model);
     for (i = 0; i < smp_cpus; i++) {
         cpu = ppc_cpu_init(machine->cpu_model);
         if (cpu == NULL) {
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 09600fee19b2..8ca80812f16a 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1815,6 +1815,8 @@ static void ppc_spapr_init(MachineState *machine)
         machine->cpu_model = kvm_enabled() ? "host" : "POWER7";
     }
 
+    ppc_cpu_parse_features(machine->cpu_model);
+
     if (smc->dr_cpu_enabled) {
         char *type = spapr_get_cpu_core_type(machine->cpu_model);
 
diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
index 7e4445b6879a..a19dd17addda 100644
--- a/hw/ppc/virtex_ml507.c
+++ b/hw/ppc/virtex_ml507.c
@@ -96,6 +96,7 @@ static PowerPCCPU *ppc440_init_xilinx(ram_addr_t *ram_size,
     CPUPPCState *env;
     qemu_irq *irqs;
 
+    ppc_cpu_parse_features(cpu_model);
     cpu = ppc_cpu_init(cpu_model);
     if (cpu == NULL) {
         fprintf(stderr, "Unable to initialize CPU!\n");
diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
index a4db1db82e1b..2ed063e09bec 100644
--- a/include/hw/ppc/ppc.h
+++ b/include/hw/ppc/ppc.h
@@ -107,4 +107,5 @@ enum {
 void ppc_booke_timers_init(PowerPCCPU *cpu, uint32_t freq, uint32_t flags);
 
 PowerPCCPU *ppc_cpu_init(const char *cpu_model);
+void ppc_cpu_parse_features(const char *cpu_model);
 #endif

[Qemu-devel] [PATCH v3 4/7] ppc: open code cpu creation for machine types

[Greg Kurz]

If we want to generate cpu_dt_id in the machine code, this must occur
before the cpu gets realized. We must open code the cpu creation to be
able to do this.

This patch just does that.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
v3: don't parse cpu features as it is done in a previous patch
---
 hw/ppc/ppc.c |   32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 313b3f0b9a51..0df32a9b3965 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -32,6 +32,7 @@
 #include "sysemu/cpus.h"
 #include "hw/timer/m48t59.h"
 #include "qemu/log.h"
+#include "qapi/error.h"
 #include "qemu/error-report.h"
 #include "qapi/error.h"
 #include "hw/loader.h"
@@ -1354,7 +1355,36 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
 
 PowerPCCPU *ppc_cpu_init(const char *cpu_model)
 {
-    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model));
+    PowerPCCPU *cpu;
+    ObjectClass *oc;
+    gchar **model_pieces;
+    Error *err = NULL;
+
+    model_pieces = g_strsplit(cpu_model, ",", 2);
+    if (!model_pieces[0]) {
+        error_report("Invalid/empty CPU model name");
+        return NULL;
+    }
+
+    oc = cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]);
+    if (oc == NULL) {
+        error_report("Unable to find CPU definition: %s", model_pieces[0]);
+        goto out;
+    }
+
+    cpu = POWERPC_CPU(object_new(object_class_get_name(oc)));
+    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
+
+out:
+    g_strfreev(model_pieces);
+
+    if (err != NULL) {
+        error_report_err(err);
+        object_unref(OBJECT(cpu));
+        return NULL;
+    }
+
+    return cpu;
 }
 
 void ppc_cpu_parse_features(const char *cpu_model)

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 4/7] ppc: open code cpu creation for machine types

[Greg Kurz]

On Wed, 06 Jul 2016 14:14:25 +0200
Greg Kurz <groug@kaod.org> wrote:

> If we want to generate cpu_dt_id in the machine code, this must occur
> before the cpu gets realized. We must open code the cpu creation to be
> able to do this.
> 
> This patch just does that.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> v3: don't parse cpu features as it is done in a previous patch
> ---
>  hw/ppc/ppc.c |   32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index 313b3f0b9a51..0df32a9b3965 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -32,6 +32,7 @@
>  #include "sysemu/cpus.h"
>  #include "hw/timer/m48t59.h"
>  #include "qemu/log.h"
> +#include "qapi/error.h"
>  #include "qemu/error-report.h"
>  #include "qapi/error.h"
>  #include "hw/loader.h"
> @@ -1354,7 +1355,36 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
>  
>  PowerPCCPU *ppc_cpu_init(const char *cpu_model)
>  {
> -    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model));
> +    PowerPCCPU *cpu;
> +    ObjectClass *oc;
> +    gchar **model_pieces;
> +    Error *err = NULL;
> +
> +    model_pieces = g_strsplit(cpu_model, ",", 2);
> +    if (!model_pieces[0]) {
> +        error_report("Invalid/empty CPU model name");
> +        return NULL;
> +    }
> +
> +    oc = cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]);
> +    if (oc == NULL) {
> +        error_report("Unable to find CPU definition: %s", model_pieces[0]);
> +        goto out;
> +    }
> +
> +    cpu = POWERPC_CPU(object_new(object_class_get_name(oc)));
> +    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
> +
> +out:
> +    g_strfreev(model_pieces);
> +
> +    if (err != NULL) {
> +        error_report_err(err);
> +        object_unref(OBJECT(cpu));

cpu can be uninitialized here (thanks travis)

I will initialize cpu to NULL and do this:

        if (cpu != NULL) {
            object_unref(OBJECT(cpu));
        }

> +        return NULL;
> +    }
> +
> +    return cpu;
>  }
>  
>  void ppc_cpu_parse_features(const char *cpu_model)
> 
> 

Re: [Qemu-devel] [Qemu-ppc] [PATCH v3 4/7] ppc: open code cpu creation for machine types

[David Gibson]

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

On Wed, Jul 06, 2016 at 06:06:16PM +0200, Greg Kurz wrote:
> On Wed, 06 Jul 2016 14:14:25 +0200
> Greg Kurz <groug@kaod.org> wrote:
> 
> > If we want to generate cpu_dt_id in the machine code, this must occur
> > before the cpu gets realized. We must open code the cpu creation to be
> > able to do this.
> > 
> > This patch just does that.
> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > v3: don't parse cpu features as it is done in a previous patch
> > ---
> >  hw/ppc/ppc.c |   32 +++++++++++++++++++++++++++++++-
> >  1 file changed, 31 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> > index 313b3f0b9a51..0df32a9b3965 100644
> > --- a/hw/ppc/ppc.c
> > +++ b/hw/ppc/ppc.c
> > @@ -32,6 +32,7 @@
> >  #include "sysemu/cpus.h"
> >  #include "hw/timer/m48t59.h"
> >  #include "qemu/log.h"
> > +#include "qapi/error.h"
> >  #include "qemu/error-report.h"
> >  #include "qapi/error.h"
> >  #include "hw/loader.h"
> > @@ -1354,7 +1355,36 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
> >  
> >  PowerPCCPU *ppc_cpu_init(const char *cpu_model)
> >  {
> > -    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model));
> > +    PowerPCCPU *cpu;
> > +    ObjectClass *oc;
> > +    gchar **model_pieces;
> > +    Error *err = NULL;
> > +
> > +    model_pieces = g_strsplit(cpu_model, ",", 2);
> > +    if (!model_pieces[0]) {
> > +        error_report("Invalid/empty CPU model name");
> > +        return NULL;
> > +    }
> > +
> > +    oc = cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]);
> > +    if (oc == NULL) {
> > +        error_report("Unable to find CPU definition: %s", model_pieces[0]);
> > +        goto out;
> > +    }
> > +
> > +    cpu = POWERPC_CPU(object_new(object_class_get_name(oc)));
> > +    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
> > +
> > +out:
> > +    g_strfreev(model_pieces);
> > +
> > +    if (err != NULL) {
> > +        error_report_err(err);
> > +        object_unref(OBJECT(cpu));
> 
> cpu can be uninitialized here (thanks travis)
> 
> I will initialize cpu to NULL and do this:
> 
>         if (cpu != NULL) {
>             object_unref(OBJECT(cpu));
>         }

Good catch.

> > +        return NULL;
> > +    }
> > +
> > +    return cpu;
> >  }
> >  
> >  void ppc_cpu_parse_features(const char *cpu_model)
> > 
> > 
> 

-- 
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 v3 4/7] ppc: open code cpu creation for machine types

[David Gibson]

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

On Wed, Jul 06, 2016 at 02:14:25PM +0200, Greg Kurz wrote:
> If we want to generate cpu_dt_id in the machine code, this must occur
> before the cpu gets realized. We must open code the cpu creation to be
> able to do this.
> 
> This patch just does that.
> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> v3: don't parse cpu features as it is done in a previous patch

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

> ---
>  hw/ppc/ppc.c |   32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index 313b3f0b9a51..0df32a9b3965 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -32,6 +32,7 @@
>  #include "sysemu/cpus.h"
>  #include "hw/timer/m48t59.h"
>  #include "qemu/log.h"
> +#include "qapi/error.h"
>  #include "qemu/error-report.h"
>  #include "qapi/error.h"
>  #include "hw/loader.h"
> @@ -1354,7 +1355,36 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
>  
>  PowerPCCPU *ppc_cpu_init(const char *cpu_model)
>  {
> -    return POWERPC_CPU(cpu_generic_init(TYPE_POWERPC_CPU, cpu_model));
> +    PowerPCCPU *cpu;
> +    ObjectClass *oc;
> +    gchar **model_pieces;
> +    Error *err = NULL;
> +
> +    model_pieces = g_strsplit(cpu_model, ",", 2);
> +    if (!model_pieces[0]) {
> +        error_report("Invalid/empty CPU model name");
> +        return NULL;
> +    }
> +
> +    oc = cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]);
> +    if (oc == NULL) {
> +        error_report("Unable to find CPU definition: %s", model_pieces[0]);
> +        goto out;
> +    }
> +
> +    cpu = POWERPC_CPU(object_new(object_class_get_name(oc)));
> +    object_property_set_bool(OBJECT(cpu), true, "realized", &err);
> +
> +out:
> +    g_strfreev(model_pieces);
> +
> +    if (err != NULL) {
> +        error_report_err(err);
> +        object_unref(OBJECT(cpu));
> +        return NULL;
> +    }
> +
> +    return cpu;
>  }
>  
>  void ppc_cpu_parse_features(const char *cpu_model)
> 

-- 
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 v3 5/7] ppc: each machine type to provide vcpu_dt_id

[Greg Kurz]

This patch switches machine types to provide device-tree cpu ids.

We have three cases to handle:

- pseries < 2.7 call ppc_cpu_init() and should compute the DT id as it is
  currently done in the target code.

- pseries 2.7 don't call ppc_cpu_init() and compute the DT as the sum of
  the core DT id and the the thread index within the core

- all other machine types don't support SMT and just need to provide a
  different index for each vcpu

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/e500.c           |    2 +-
 hw/ppc/mac_newworld.c   |    2 +-
 hw/ppc/mac_oldworld.c   |    2 +-
 hw/ppc/ppc.c            |    5 ++++-
 hw/ppc/ppc440_bamboo.c  |    2 +-
 hw/ppc/ppc4xx_devs.c    |    2 +-
 hw/ppc/prep.c           |    2 +-
 hw/ppc/spapr.c          |    5 ++++-
 hw/ppc/spapr_cpu_core.c |    7 +++++--
 hw/ppc/virtex_ml507.c   |    2 +-
 include/hw/ppc/ppc.h    |    2 +-
 11 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index b9221cc2c14a..513a42a3f952 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -823,7 +823,7 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
         CPUState *cs;
         qemu_irq *input;
 
-        cpu = ppc_cpu_init(machine->cpu_model);
+        cpu = ppc_cpu_init(machine->cpu_model, i);
         if (cpu == NULL) {
             fprintf(stderr, "Unable to initialize CPU!\n");
             exit(1);
diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
index 366089085844..9ef9504ff29c 100644
--- a/hw/ppc/mac_newworld.c
+++ b/hw/ppc/mac_newworld.c
@@ -194,7 +194,7 @@ static void ppc_core99_init(MachineState *machine)
     }
     ppc_cpu_parse_features(machine->cpu_model);
     for (i = 0; i < smp_cpus; i++) {
-        cpu = ppc_cpu_init(machine->cpu_model);
+        cpu = ppc_cpu_init(machine->cpu_model, i);
         if (cpu == NULL) {
             fprintf(stderr, "Unable to find PowerPC CPU definition\n");
             exit(1);
diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
index 5c2dc53cb584..0b09a8da8bf2 100644
--- a/hw/ppc/mac_oldworld.c
+++ b/hw/ppc/mac_oldworld.c
@@ -114,7 +114,7 @@ static void ppc_heathrow_init(MachineState *machine)
         machine->cpu_model = "G3";
     ppc_cpu_parse_features(machine->cpu_model);
     for (i = 0; i < smp_cpus; i++) {
-        cpu = ppc_cpu_init(machine->cpu_model);
+        cpu = ppc_cpu_init(machine->cpu_model, i);
         if (cpu == NULL) {
             fprintf(stderr, "Unable to find PowerPC CPU definition\n");
             exit(1);
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index 0df32a9b3965..3d7739b9540e 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -1353,7 +1353,7 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
     return NULL;
 }
 
-PowerPCCPU *ppc_cpu_init(const char *cpu_model)
+PowerPCCPU *ppc_cpu_init(const char *cpu_model, unsigned vcpu_dt_id)
 {
     PowerPCCPU *cpu;
     ObjectClass *oc;
@@ -1373,6 +1373,9 @@ PowerPCCPU *ppc_cpu_init(const char *cpu_model)
     }
 
     cpu = POWERPC_CPU(object_new(object_class_get_name(oc)));
+
+    cpu->cpu_dt_id = vcpu_dt_id;
+
     object_property_set_bool(OBJECT(cpu), true, "realized", &err);
 
 out:
diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
index 709a779cba7e..9dbe87207304 100644
--- a/hw/ppc/ppc440_bamboo.c
+++ b/hw/ppc/ppc440_bamboo.c
@@ -187,7 +187,7 @@ static void bamboo_init(MachineState *machine)
         machine->cpu_model = "440EP";
     }
     ppc_cpu_parse_features(machine->cpu_model);
-    cpu = ppc_cpu_init(machine->cpu_model);
+    cpu = ppc_cpu_init(machine->cpu_model, 0);
     if (cpu == NULL) {
         fprintf(stderr, "Unable to initialize CPU!\n");
         exit(1);
diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
index 8cbe50361e86..ac39202c0d1c 100644
--- a/hw/ppc/ppc4xx_devs.c
+++ b/hw/ppc/ppc4xx_devs.c
@@ -57,7 +57,7 @@ PowerPCCPU *ppc4xx_init(const char *cpu_model,
 
     /* init CPUs */
     ppc_cpu_parse_features(cpu_model);
-    cpu = ppc_cpu_init(cpu_model);
+    cpu = ppc_cpu_init(cpu_model, 0);
     if (cpu == NULL) {
         fprintf(stderr, "Unable to find PowerPC %s CPU definition\n",
                 cpu_model);
diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
index 99409a09f0ef..9e8cf233d98d 100644
--- a/hw/ppc/prep.c
+++ b/hw/ppc/prep.c
@@ -510,7 +510,7 @@ static void ppc_prep_init(MachineState *machine)
         machine->cpu_model = "602";
     ppc_cpu_parse_features(machine->cpu_model);
     for (i = 0; i < smp_cpus; i++) {
-        cpu = ppc_cpu_init(machine->cpu_model);
+        cpu = ppc_cpu_init(machine->cpu_model, i);
         if (cpu == NULL) {
             fprintf(stderr, "Unable to find PowerPC CPU definition\n");
             exit(1);
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 8ca80812f16a..baefc7bd279c 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1849,7 +1849,10 @@ static void ppc_spapr_init(MachineState *machine)
         g_free(type);
     } else {
         for (i = 0; i < smp_cpus; i++) {
-            PowerPCCPU *cpu = ppc_cpu_init(machine->cpu_model);
+            unsigned core_index = i / smp_threads;
+            unsigned thread_index = i % smp_threads;
+            PowerPCCPU *cpu = ppc_cpu_init(machine->cpu_model,
+                                           core_index * smt + thread_index);
             if (cpu == NULL) {
                 error_report("Unable to find PowerPC CPU definition");
                 exit(1);
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index 70b6b0b5ee17..e3a3024baf32 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -259,13 +259,16 @@ out:
     error_propagate(errp, local_err);
 }
 
-static void spapr_cpu_core_realize_child(Object *child, Error **errp)
+static void spapr_cpu_core_realize_child(Object *child, unsigned vcpu_dt_id,
+                                         Error **errp)
 {
     Error *local_err = NULL;
     sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
     CPUState *cs = CPU(child);
     PowerPCCPU *cpu = POWERPC_CPU(cs);
 
+    cpu->cpu_dt_id = vcpu_dt_id;
+
     object_property_set_bool(child, true, "realized", &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
@@ -306,7 +309,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
     for (j = 0; j < cc->nr_threads; j++) {
         obj = sc->threads + j * size;
 
-        spapr_cpu_core_realize_child(obj, &local_err);
+        spapr_cpu_core_realize_child(obj, cc->core_id + j, &local_err);
         if (local_err) {
             goto err;
         }
diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
index a19dd17addda..efbc58137df4 100644
--- a/hw/ppc/virtex_ml507.c
+++ b/hw/ppc/virtex_ml507.c
@@ -97,7 +97,7 @@ static PowerPCCPU *ppc440_init_xilinx(ram_addr_t *ram_size,
     qemu_irq *irqs;
 
     ppc_cpu_parse_features(cpu_model);
-    cpu = ppc_cpu_init(cpu_model);
+    cpu = ppc_cpu_init(cpu_model, 0);
     if (cpu == NULL) {
         fprintf(stderr, "Unable to initialize CPU!\n");
         exit(1);
diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
index 2ed063e09bec..379b2f5d8e3d 100644
--- a/include/hw/ppc/ppc.h
+++ b/include/hw/ppc/ppc.h
@@ -106,6 +106,6 @@ enum {
 /* ppc_booke.c */
 void ppc_booke_timers_init(PowerPCCPU *cpu, uint32_t freq, uint32_t flags);
 
-PowerPCCPU *ppc_cpu_init(const char *cpu_model);
+PowerPCCPU *ppc_cpu_init(const char *cpu_model, unsigned vcpu_dt_id);
 void ppc_cpu_parse_features(const char *cpu_model);
 #endif

Re: [Qemu-devel] [PATCH v3 5/7] ppc: each machine type to provide vcpu_dt_id

[David Gibson]

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

On Wed, Jul 06, 2016 at 02:14:36PM +0200, Greg Kurz wrote:
> This patch switches machine types to provide device-tree cpu ids.
> 
> We have three cases to handle:
> 
> - pseries < 2.7 call ppc_cpu_init() and should compute the DT id as it is
>   currently done in the target code.
> 
> - pseries 2.7 don't call ppc_cpu_init() and compute the DT as the sum of
>   the core DT id and the the thread index within the core
> 
> - all other machine types don't support SMT and just need to provide a
>   different index for each vcpu

Would it instead make sense to expose the dt_id as a vcpu property and
have the machine type set it if it wants to override?

dt_id is probably not a good name though, since there's no guarantee
the machine type actually uses a dt, which is part of the point of
this series.

> 
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/ppc/e500.c           |    2 +-
>  hw/ppc/mac_newworld.c   |    2 +-
>  hw/ppc/mac_oldworld.c   |    2 +-
>  hw/ppc/ppc.c            |    5 ++++-
>  hw/ppc/ppc440_bamboo.c  |    2 +-
>  hw/ppc/ppc4xx_devs.c    |    2 +-
>  hw/ppc/prep.c           |    2 +-
>  hw/ppc/spapr.c          |    5 ++++-
>  hw/ppc/spapr_cpu_core.c |    7 +++++--
>  hw/ppc/virtex_ml507.c   |    2 +-
>  include/hw/ppc/ppc.h    |    2 +-
>  11 files changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index b9221cc2c14a..513a42a3f952 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -823,7 +823,7 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
>          CPUState *cs;
>          qemu_irq *input;
>  
> -        cpu = ppc_cpu_init(machine->cpu_model);
> +        cpu = ppc_cpu_init(machine->cpu_model, i);
>          if (cpu == NULL) {
>              fprintf(stderr, "Unable to initialize CPU!\n");
>              exit(1);
> diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> index 366089085844..9ef9504ff29c 100644
> --- a/hw/ppc/mac_newworld.c
> +++ b/hw/ppc/mac_newworld.c
> @@ -194,7 +194,7 @@ static void ppc_core99_init(MachineState *machine)
>      }
>      ppc_cpu_parse_features(machine->cpu_model);
>      for (i = 0; i < smp_cpus; i++) {
> -        cpu = ppc_cpu_init(machine->cpu_model);
> +        cpu = ppc_cpu_init(machine->cpu_model, i);
>          if (cpu == NULL) {
>              fprintf(stderr, "Unable to find PowerPC CPU definition\n");
>              exit(1);
> diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> index 5c2dc53cb584..0b09a8da8bf2 100644
> --- a/hw/ppc/mac_oldworld.c
> +++ b/hw/ppc/mac_oldworld.c
> @@ -114,7 +114,7 @@ static void ppc_heathrow_init(MachineState *machine)
>          machine->cpu_model = "G3";
>      ppc_cpu_parse_features(machine->cpu_model);
>      for (i = 0; i < smp_cpus; i++) {
> -        cpu = ppc_cpu_init(machine->cpu_model);
> +        cpu = ppc_cpu_init(machine->cpu_model, i);
>          if (cpu == NULL) {
>              fprintf(stderr, "Unable to find PowerPC CPU definition\n");
>              exit(1);
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index 0df32a9b3965..3d7739b9540e 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -1353,7 +1353,7 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
>      return NULL;
>  }
>  
> -PowerPCCPU *ppc_cpu_init(const char *cpu_model)
> +PowerPCCPU *ppc_cpu_init(const char *cpu_model, unsigned vcpu_dt_id)
>  {
>      PowerPCCPU *cpu;
>      ObjectClass *oc;
> @@ -1373,6 +1373,9 @@ PowerPCCPU *ppc_cpu_init(const char *cpu_model)
>      }
>  
>      cpu = POWERPC_CPU(object_new(object_class_get_name(oc)));
> +
> +    cpu->cpu_dt_id = vcpu_dt_id;
> +
>      object_property_set_bool(OBJECT(cpu), true, "realized", &err);
>  
>  out:
> diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
> index 709a779cba7e..9dbe87207304 100644
> --- a/hw/ppc/ppc440_bamboo.c
> +++ b/hw/ppc/ppc440_bamboo.c
> @@ -187,7 +187,7 @@ static void bamboo_init(MachineState *machine)
>          machine->cpu_model = "440EP";
>      }
>      ppc_cpu_parse_features(machine->cpu_model);
> -    cpu = ppc_cpu_init(machine->cpu_model);
> +    cpu = ppc_cpu_init(machine->cpu_model, 0);
>      if (cpu == NULL) {
>          fprintf(stderr, "Unable to initialize CPU!\n");
>          exit(1);
> diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
> index 8cbe50361e86..ac39202c0d1c 100644
> --- a/hw/ppc/ppc4xx_devs.c
> +++ b/hw/ppc/ppc4xx_devs.c
> @@ -57,7 +57,7 @@ PowerPCCPU *ppc4xx_init(const char *cpu_model,
>  
>      /* init CPUs */
>      ppc_cpu_parse_features(cpu_model);
> -    cpu = ppc_cpu_init(cpu_model);
> +    cpu = ppc_cpu_init(cpu_model, 0);
>      if (cpu == NULL) {
>          fprintf(stderr, "Unable to find PowerPC %s CPU definition\n",
>                  cpu_model);
> diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> index 99409a09f0ef..9e8cf233d98d 100644
> --- a/hw/ppc/prep.c
> +++ b/hw/ppc/prep.c
> @@ -510,7 +510,7 @@ static void ppc_prep_init(MachineState *machine)
>          machine->cpu_model = "602";
>      ppc_cpu_parse_features(machine->cpu_model);
>      for (i = 0; i < smp_cpus; i++) {
> -        cpu = ppc_cpu_init(machine->cpu_model);
> +        cpu = ppc_cpu_init(machine->cpu_model, i);
>          if (cpu == NULL) {
>              fprintf(stderr, "Unable to find PowerPC CPU definition\n");
>              exit(1);
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 8ca80812f16a..baefc7bd279c 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -1849,7 +1849,10 @@ static void ppc_spapr_init(MachineState *machine)
>          g_free(type);
>      } else {
>          for (i = 0; i < smp_cpus; i++) {
> -            PowerPCCPU *cpu = ppc_cpu_init(machine->cpu_model);
> +            unsigned core_index = i / smp_threads;
> +            unsigned thread_index = i % smp_threads;
> +            PowerPCCPU *cpu = ppc_cpu_init(machine->cpu_model,
> +                                           core_index * smt + thread_index);
>              if (cpu == NULL) {
>                  error_report("Unable to find PowerPC CPU definition");
>                  exit(1);
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index 70b6b0b5ee17..e3a3024baf32 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -259,13 +259,16 @@ out:
>      error_propagate(errp, local_err);
>  }
>  
> -static void spapr_cpu_core_realize_child(Object *child, Error **errp)
> +static void spapr_cpu_core_realize_child(Object *child, unsigned vcpu_dt_id,
> +                                         Error **errp)
>  {
>      Error *local_err = NULL;
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      CPUState *cs = CPU(child);
>      PowerPCCPU *cpu = POWERPC_CPU(cs);
>  
> +    cpu->cpu_dt_id = vcpu_dt_id;
> +
>      object_property_set_bool(child, true, "realized", &local_err);
>      if (local_err) {
>          error_propagate(errp, local_err);
> @@ -306,7 +309,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
>      for (j = 0; j < cc->nr_threads; j++) {
>          obj = sc->threads + j * size;
>  
> -        spapr_cpu_core_realize_child(obj, &local_err);
> +        spapr_cpu_core_realize_child(obj, cc->core_id + j, &local_err);
>          if (local_err) {
>              goto err;
>          }
> diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
> index a19dd17addda..efbc58137df4 100644
> --- a/hw/ppc/virtex_ml507.c
> +++ b/hw/ppc/virtex_ml507.c
> @@ -97,7 +97,7 @@ static PowerPCCPU *ppc440_init_xilinx(ram_addr_t *ram_size,
>      qemu_irq *irqs;
>  
>      ppc_cpu_parse_features(cpu_model);
> -    cpu = ppc_cpu_init(cpu_model);
> +    cpu = ppc_cpu_init(cpu_model, 0);
>      if (cpu == NULL) {
>          fprintf(stderr, "Unable to initialize CPU!\n");
>          exit(1);
> diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
> index 2ed063e09bec..379b2f5d8e3d 100644
> --- a/include/hw/ppc/ppc.h
> +++ b/include/hw/ppc/ppc.h
> @@ -106,6 +106,6 @@ enum {
>  /* ppc_booke.c */
>  void ppc_booke_timers_init(PowerPCCPU *cpu, uint32_t freq, uint32_t flags);
>  
> -PowerPCCPU *ppc_cpu_init(const char *cpu_model);
> +PowerPCCPU *ppc_cpu_init(const char *cpu_model, unsigned vcpu_dt_id);
>  void ppc_cpu_parse_features(const char *cpu_model);
>  #endif
> 

-- 
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 v3 5/7] ppc: each machine type to provide vcpu_dt_id

[Greg Kurz]

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

On Thu, 7 Jul 2016 12:01:51 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Jul 06, 2016 at 02:14:36PM +0200, Greg Kurz wrote:
> > This patch switches machine types to provide device-tree cpu ids.
> > 
> > We have three cases to handle:
> > 
> > - pseries < 2.7 call ppc_cpu_init() and should compute the DT id as it is
> >   currently done in the target code.
> > 
> > - pseries 2.7 don't call ppc_cpu_init() and compute the DT as the sum of
> >   the core DT id and the the thread index within the core
> > 
> > - all other machine types don't support SMT and just need to provide a
> >   different index for each vcpu  
> 
> Would it instead make sense to expose the dt_id as a vcpu property and
> have the machine type set it if it wants to override?
> 

And the default would be cs->cpu_index I guess, correct ?

> dt_id is probably not a good name though, since there's no guarantee
> the machine type actually uses a dt, which is part of the point of
> this series.
> 

machine_cpu_index ?

> > 
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/ppc/e500.c           |    2 +-
> >  hw/ppc/mac_newworld.c   |    2 +-
> >  hw/ppc/mac_oldworld.c   |    2 +-
> >  hw/ppc/ppc.c            |    5 ++++-
> >  hw/ppc/ppc440_bamboo.c  |    2 +-
> >  hw/ppc/ppc4xx_devs.c    |    2 +-
> >  hw/ppc/prep.c           |    2 +-
> >  hw/ppc/spapr.c          |    5 ++++-
> >  hw/ppc/spapr_cpu_core.c |    7 +++++--
> >  hw/ppc/virtex_ml507.c   |    2 +-
> >  include/hw/ppc/ppc.h    |    2 +-
> >  11 files changed, 21 insertions(+), 12 deletions(-)
> > 
> > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> > index b9221cc2c14a..513a42a3f952 100644
> > --- a/hw/ppc/e500.c
> > +++ b/hw/ppc/e500.c
> > @@ -823,7 +823,7 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
> >          CPUState *cs;
> >          qemu_irq *input;
> >  
> > -        cpu = ppc_cpu_init(machine->cpu_model);
> > +        cpu = ppc_cpu_init(machine->cpu_model, i);
> >          if (cpu == NULL) {
> >              fprintf(stderr, "Unable to initialize CPU!\n");
> >              exit(1);
> > diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> > index 366089085844..9ef9504ff29c 100644
> > --- a/hw/ppc/mac_newworld.c
> > +++ b/hw/ppc/mac_newworld.c
> > @@ -194,7 +194,7 @@ static void ppc_core99_init(MachineState *machine)
> >      }
> >      ppc_cpu_parse_features(machine->cpu_model);
> >      for (i = 0; i < smp_cpus; i++) {
> > -        cpu = ppc_cpu_init(machine->cpu_model);
> > +        cpu = ppc_cpu_init(machine->cpu_model, i);
> >          if (cpu == NULL) {
> >              fprintf(stderr, "Unable to find PowerPC CPU definition\n");
> >              exit(1);
> > diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> > index 5c2dc53cb584..0b09a8da8bf2 100644
> > --- a/hw/ppc/mac_oldworld.c
> > +++ b/hw/ppc/mac_oldworld.c
> > @@ -114,7 +114,7 @@ static void ppc_heathrow_init(MachineState *machine)
> >          machine->cpu_model = "G3";
> >      ppc_cpu_parse_features(machine->cpu_model);
> >      for (i = 0; i < smp_cpus; i++) {
> > -        cpu = ppc_cpu_init(machine->cpu_model);
> > +        cpu = ppc_cpu_init(machine->cpu_model, i);
> >          if (cpu == NULL) {
> >              fprintf(stderr, "Unable to find PowerPC CPU definition\n");
> >              exit(1);
> > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> > index 0df32a9b3965..3d7739b9540e 100644
> > --- a/hw/ppc/ppc.c
> > +++ b/hw/ppc/ppc.c
> > @@ -1353,7 +1353,7 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
> >      return NULL;
> >  }
> >  
> > -PowerPCCPU *ppc_cpu_init(const char *cpu_model)
> > +PowerPCCPU *ppc_cpu_init(const char *cpu_model, unsigned vcpu_dt_id)
> >  {
> >      PowerPCCPU *cpu;
> >      ObjectClass *oc;
> > @@ -1373,6 +1373,9 @@ PowerPCCPU *ppc_cpu_init(const char *cpu_model)
> >      }
> >  
> >      cpu = POWERPC_CPU(object_new(object_class_get_name(oc)));
> > +
> > +    cpu->cpu_dt_id = vcpu_dt_id;
> > +
> >      object_property_set_bool(OBJECT(cpu), true, "realized", &err);
> >  
> >  out:
> > diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
> > index 709a779cba7e..9dbe87207304 100644
> > --- a/hw/ppc/ppc440_bamboo.c
> > +++ b/hw/ppc/ppc440_bamboo.c
> > @@ -187,7 +187,7 @@ static void bamboo_init(MachineState *machine)
> >          machine->cpu_model = "440EP";
> >      }
> >      ppc_cpu_parse_features(machine->cpu_model);
> > -    cpu = ppc_cpu_init(machine->cpu_model);
> > +    cpu = ppc_cpu_init(machine->cpu_model, 0);
> >      if (cpu == NULL) {
> >          fprintf(stderr, "Unable to initialize CPU!\n");
> >          exit(1);
> > diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
> > index 8cbe50361e86..ac39202c0d1c 100644
> > --- a/hw/ppc/ppc4xx_devs.c
> > +++ b/hw/ppc/ppc4xx_devs.c
> > @@ -57,7 +57,7 @@ PowerPCCPU *ppc4xx_init(const char *cpu_model,
> >  
> >      /* init CPUs */
> >      ppc_cpu_parse_features(cpu_model);
> > -    cpu = ppc_cpu_init(cpu_model);
> > +    cpu = ppc_cpu_init(cpu_model, 0);
> >      if (cpu == NULL) {
> >          fprintf(stderr, "Unable to find PowerPC %s CPU definition\n",
> >                  cpu_model);
> > diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> > index 99409a09f0ef..9e8cf233d98d 100644
> > --- a/hw/ppc/prep.c
> > +++ b/hw/ppc/prep.c
> > @@ -510,7 +510,7 @@ static void ppc_prep_init(MachineState *machine)
> >          machine->cpu_model = "602";
> >      ppc_cpu_parse_features(machine->cpu_model);
> >      for (i = 0; i < smp_cpus; i++) {
> > -        cpu = ppc_cpu_init(machine->cpu_model);
> > +        cpu = ppc_cpu_init(machine->cpu_model, i);
> >          if (cpu == NULL) {
> >              fprintf(stderr, "Unable to find PowerPC CPU definition\n");
> >              exit(1);
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 8ca80812f16a..baefc7bd279c 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -1849,7 +1849,10 @@ static void ppc_spapr_init(MachineState *machine)
> >          g_free(type);
> >      } else {
> >          for (i = 0; i < smp_cpus; i++) {
> > -            PowerPCCPU *cpu = ppc_cpu_init(machine->cpu_model);
> > +            unsigned core_index = i / smp_threads;
> > +            unsigned thread_index = i % smp_threads;
> > +            PowerPCCPU *cpu = ppc_cpu_init(machine->cpu_model,
> > +                                           core_index * smt + thread_index);
> >              if (cpu == NULL) {
> >                  error_report("Unable to find PowerPC CPU definition");
> >                  exit(1);
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index 70b6b0b5ee17..e3a3024baf32 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -259,13 +259,16 @@ out:
> >      error_propagate(errp, local_err);
> >  }
> >  
> > -static void spapr_cpu_core_realize_child(Object *child, Error **errp)
> > +static void spapr_cpu_core_realize_child(Object *child, unsigned vcpu_dt_id,
> > +                                         Error **errp)
> >  {
> >      Error *local_err = NULL;
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> >      CPUState *cs = CPU(child);
> >      PowerPCCPU *cpu = POWERPC_CPU(cs);
> >  
> > +    cpu->cpu_dt_id = vcpu_dt_id;
> > +
> >      object_property_set_bool(child, true, "realized", &local_err);
> >      if (local_err) {
> >          error_propagate(errp, local_err);
> > @@ -306,7 +309,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> >      for (j = 0; j < cc->nr_threads; j++) {
> >          obj = sc->threads + j * size;
> >  
> > -        spapr_cpu_core_realize_child(obj, &local_err);
> > +        spapr_cpu_core_realize_child(obj, cc->core_id + j, &local_err);
> >          if (local_err) {
> >              goto err;
> >          }
> > diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
> > index a19dd17addda..efbc58137df4 100644
> > --- a/hw/ppc/virtex_ml507.c
> > +++ b/hw/ppc/virtex_ml507.c
> > @@ -97,7 +97,7 @@ static PowerPCCPU *ppc440_init_xilinx(ram_addr_t *ram_size,
> >      qemu_irq *irqs;
> >  
> >      ppc_cpu_parse_features(cpu_model);
> > -    cpu = ppc_cpu_init(cpu_model);
> > +    cpu = ppc_cpu_init(cpu_model, 0);
> >      if (cpu == NULL) {
> >          fprintf(stderr, "Unable to initialize CPU!\n");
> >          exit(1);
> > diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
> > index 2ed063e09bec..379b2f5d8e3d 100644
> > --- a/include/hw/ppc/ppc.h
> > +++ b/include/hw/ppc/ppc.h
> > @@ -106,6 +106,6 @@ enum {
> >  /* ppc_booke.c */
> >  void ppc_booke_timers_init(PowerPCCPU *cpu, uint32_t freq, uint32_t flags);
> >  
> > -PowerPCCPU *ppc_cpu_init(const char *cpu_model);
> > +PowerPCCPU *ppc_cpu_init(const char *cpu_model, unsigned vcpu_dt_id);
> >  void ppc_cpu_parse_features(const char *cpu_model);
> >  #endif
> >   
> 


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

Re: [Qemu-devel] [PATCH v3 5/7] ppc: each machine type to provide vcpu_dt_id

[David Gibson]

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

On Thu, Jul 07, 2016 at 10:55:02AM +0200, Greg Kurz wrote:
> On Thu, 7 Jul 2016 12:01:51 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Jul 06, 2016 at 02:14:36PM +0200, Greg Kurz wrote:
> > > This patch switches machine types to provide device-tree cpu ids.
> > > 
> > > We have three cases to handle:
> > > 
> > > - pseries < 2.7 call ppc_cpu_init() and should compute the DT id as it is
> > >   currently done in the target code.
> > > 
> > > - pseries 2.7 don't call ppc_cpu_init() and compute the DT as the sum of
> > >   the core DT id and the the thread index within the core
> > > 
> > > - all other machine types don't support SMT and just need to provide a
> > >   different index for each vcpu  
> > 
> > Would it instead make sense to expose the dt_id as a vcpu property and
> > have the machine type set it if it wants to override?
> > 
> 
> And the default would be cs->cpu_index I guess, correct ?

That was my intention.  However Bharata has since pointed out that
this doesn't work, because the cpu_index is only computed at realize time.

> > dt_id is probably not a good name though, since there's no guarantee
> > the machine type actually uses a dt, which is part of the point of
> > this series.
> > 
> 
> machine_cpu_index ?
> 
> > > 
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > >  hw/ppc/e500.c           |    2 +-
> > >  hw/ppc/mac_newworld.c   |    2 +-
> > >  hw/ppc/mac_oldworld.c   |    2 +-
> > >  hw/ppc/ppc.c            |    5 ++++-
> > >  hw/ppc/ppc440_bamboo.c  |    2 +-
> > >  hw/ppc/ppc4xx_devs.c    |    2 +-
> > >  hw/ppc/prep.c           |    2 +-
> > >  hw/ppc/spapr.c          |    5 ++++-
> > >  hw/ppc/spapr_cpu_core.c |    7 +++++--
> > >  hw/ppc/virtex_ml507.c   |    2 +-
> > >  include/hw/ppc/ppc.h    |    2 +-
> > >  11 files changed, 21 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> > > index b9221cc2c14a..513a42a3f952 100644
> > > --- a/hw/ppc/e500.c
> > > +++ b/hw/ppc/e500.c
> > > @@ -823,7 +823,7 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
> > >          CPUState *cs;
> > >          qemu_irq *input;
> > >  
> > > -        cpu = ppc_cpu_init(machine->cpu_model);
> > > +        cpu = ppc_cpu_init(machine->cpu_model, i);
> > >          if (cpu == NULL) {
> > >              fprintf(stderr, "Unable to initialize CPU!\n");
> > >              exit(1);
> > > diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> > > index 366089085844..9ef9504ff29c 100644
> > > --- a/hw/ppc/mac_newworld.c
> > > +++ b/hw/ppc/mac_newworld.c
> > > @@ -194,7 +194,7 @@ static void ppc_core99_init(MachineState *machine)
> > >      }
> > >      ppc_cpu_parse_features(machine->cpu_model);
> > >      for (i = 0; i < smp_cpus; i++) {
> > > -        cpu = ppc_cpu_init(machine->cpu_model);
> > > +        cpu = ppc_cpu_init(machine->cpu_model, i);
> > >          if (cpu == NULL) {
> > >              fprintf(stderr, "Unable to find PowerPC CPU definition\n");
> > >              exit(1);
> > > diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> > > index 5c2dc53cb584..0b09a8da8bf2 100644
> > > --- a/hw/ppc/mac_oldworld.c
> > > +++ b/hw/ppc/mac_oldworld.c
> > > @@ -114,7 +114,7 @@ static void ppc_heathrow_init(MachineState *machine)
> > >          machine->cpu_model = "G3";
> > >      ppc_cpu_parse_features(machine->cpu_model);
> > >      for (i = 0; i < smp_cpus; i++) {
> > > -        cpu = ppc_cpu_init(machine->cpu_model);
> > > +        cpu = ppc_cpu_init(machine->cpu_model, i);
> > >          if (cpu == NULL) {
> > >              fprintf(stderr, "Unable to find PowerPC CPU definition\n");
> > >              exit(1);
> > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> > > index 0df32a9b3965..3d7739b9540e 100644
> > > --- a/hw/ppc/ppc.c
> > > +++ b/hw/ppc/ppc.c
> > > @@ -1353,7 +1353,7 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
> > >      return NULL;
> > >  }
> > >  
> > > -PowerPCCPU *ppc_cpu_init(const char *cpu_model)
> > > +PowerPCCPU *ppc_cpu_init(const char *cpu_model, unsigned vcpu_dt_id)
> > >  {
> > >      PowerPCCPU *cpu;
> > >      ObjectClass *oc;
> > > @@ -1373,6 +1373,9 @@ PowerPCCPU *ppc_cpu_init(const char *cpu_model)
> > >      }
> > >  
> > >      cpu = POWERPC_CPU(object_new(object_class_get_name(oc)));
> > > +
> > > +    cpu->cpu_dt_id = vcpu_dt_id;
> > > +
> > >      object_property_set_bool(OBJECT(cpu), true, "realized", &err);
> > >  
> > >  out:
> > > diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
> > > index 709a779cba7e..9dbe87207304 100644
> > > --- a/hw/ppc/ppc440_bamboo.c
> > > +++ b/hw/ppc/ppc440_bamboo.c
> > > @@ -187,7 +187,7 @@ static void bamboo_init(MachineState *machine)
> > >          machine->cpu_model = "440EP";
> > >      }
> > >      ppc_cpu_parse_features(machine->cpu_model);
> > > -    cpu = ppc_cpu_init(machine->cpu_model);
> > > +    cpu = ppc_cpu_init(machine->cpu_model, 0);
> > >      if (cpu == NULL) {
> > >          fprintf(stderr, "Unable to initialize CPU!\n");
> > >          exit(1);
> > > diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
> > > index 8cbe50361e86..ac39202c0d1c 100644
> > > --- a/hw/ppc/ppc4xx_devs.c
> > > +++ b/hw/ppc/ppc4xx_devs.c
> > > @@ -57,7 +57,7 @@ PowerPCCPU *ppc4xx_init(const char *cpu_model,
> > >  
> > >      /* init CPUs */
> > >      ppc_cpu_parse_features(cpu_model);
> > > -    cpu = ppc_cpu_init(cpu_model);
> > > +    cpu = ppc_cpu_init(cpu_model, 0);
> > >      if (cpu == NULL) {
> > >          fprintf(stderr, "Unable to find PowerPC %s CPU definition\n",
> > >                  cpu_model);
> > > diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> > > index 99409a09f0ef..9e8cf233d98d 100644
> > > --- a/hw/ppc/prep.c
> > > +++ b/hw/ppc/prep.c
> > > @@ -510,7 +510,7 @@ static void ppc_prep_init(MachineState *machine)
> > >          machine->cpu_model = "602";
> > >      ppc_cpu_parse_features(machine->cpu_model);
> > >      for (i = 0; i < smp_cpus; i++) {
> > > -        cpu = ppc_cpu_init(machine->cpu_model);
> > > +        cpu = ppc_cpu_init(machine->cpu_model, i);
> > >          if (cpu == NULL) {
> > >              fprintf(stderr, "Unable to find PowerPC CPU definition\n");
> > >              exit(1);
> > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > index 8ca80812f16a..baefc7bd279c 100644
> > > --- a/hw/ppc/spapr.c
> > > +++ b/hw/ppc/spapr.c
> > > @@ -1849,7 +1849,10 @@ static void ppc_spapr_init(MachineState *machine)
> > >          g_free(type);
> > >      } else {
> > >          for (i = 0; i < smp_cpus; i++) {
> > > -            PowerPCCPU *cpu = ppc_cpu_init(machine->cpu_model);
> > > +            unsigned core_index = i / smp_threads;
> > > +            unsigned thread_index = i % smp_threads;
> > > +            PowerPCCPU *cpu = ppc_cpu_init(machine->cpu_model,
> > > +                                           core_index * smt + thread_index);
> > >              if (cpu == NULL) {
> > >                  error_report("Unable to find PowerPC CPU definition");
> > >                  exit(1);
> > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > index 70b6b0b5ee17..e3a3024baf32 100644
> > > --- a/hw/ppc/spapr_cpu_core.c
> > > +++ b/hw/ppc/spapr_cpu_core.c
> > > @@ -259,13 +259,16 @@ out:
> > >      error_propagate(errp, local_err);
> > >  }
> > >  
> > > -static void spapr_cpu_core_realize_child(Object *child, Error **errp)
> > > +static void spapr_cpu_core_realize_child(Object *child, unsigned vcpu_dt_id,
> > > +                                         Error **errp)
> > >  {
> > >      Error *local_err = NULL;
> > >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > >      CPUState *cs = CPU(child);
> > >      PowerPCCPU *cpu = POWERPC_CPU(cs);
> > >  
> > > +    cpu->cpu_dt_id = vcpu_dt_id;
> > > +
> > >      object_property_set_bool(child, true, "realized", &local_err);
> > >      if (local_err) {
> > >          error_propagate(errp, local_err);
> > > @@ -306,7 +309,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > >      for (j = 0; j < cc->nr_threads; j++) {
> > >          obj = sc->threads + j * size;
> > >  
> > > -        spapr_cpu_core_realize_child(obj, &local_err);
> > > +        spapr_cpu_core_realize_child(obj, cc->core_id + j, &local_err);
> > >          if (local_err) {
> > >              goto err;
> > >          }
> > > diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
> > > index a19dd17addda..efbc58137df4 100644
> > > --- a/hw/ppc/virtex_ml507.c
> > > +++ b/hw/ppc/virtex_ml507.c
> > > @@ -97,7 +97,7 @@ static PowerPCCPU *ppc440_init_xilinx(ram_addr_t *ram_size,
> > >      qemu_irq *irqs;
> > >  
> > >      ppc_cpu_parse_features(cpu_model);
> > > -    cpu = ppc_cpu_init(cpu_model);
> > > +    cpu = ppc_cpu_init(cpu_model, 0);
> > >      if (cpu == NULL) {
> > >          fprintf(stderr, "Unable to initialize CPU!\n");
> > >          exit(1);
> > > diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
> > > index 2ed063e09bec..379b2f5d8e3d 100644
> > > --- a/include/hw/ppc/ppc.h
> > > +++ b/include/hw/ppc/ppc.h
> > > @@ -106,6 +106,6 @@ enum {
> > >  /* ppc_booke.c */
> > >  void ppc_booke_timers_init(PowerPCCPU *cpu, uint32_t freq, uint32_t flags);
> > >  
> > > -PowerPCCPU *ppc_cpu_init(const char *cpu_model);
> > > +PowerPCCPU *ppc_cpu_init(const char *cpu_model, unsigned vcpu_dt_id);
> > >  void ppc_cpu_parse_features(const char *cpu_model);
> > >  #endif
> > >   
> > 
> 



-- 
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 v3 5/7] ppc: each machine type to provide vcpu_dt_id

[Greg Kurz]

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

On Fri, 8 Jul 2016 11:57:07 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jul 07, 2016 at 10:55:02AM +0200, Greg Kurz wrote:
> > On Thu, 7 Jul 2016 12:01:51 +1000
> > David Gibson <david@gibson.dropbear.id.au> wrote:
> >   
> > > On Wed, Jul 06, 2016 at 02:14:36PM +0200, Greg Kurz wrote:  
> > > > This patch switches machine types to provide device-tree cpu ids.
> > > > 
> > > > We have three cases to handle:
> > > > 
> > > > - pseries < 2.7 call ppc_cpu_init() and should compute the DT id as it is
> > > >   currently done in the target code.
> > > > 
> > > > - pseries 2.7 don't call ppc_cpu_init() and compute the DT as the sum of
> > > >   the core DT id and the the thread index within the core
> > > > 
> > > > - all other machine types don't support SMT and just need to provide a
> > > >   different index for each vcpu    
> > > 
> > > Would it instead make sense to expose the dt_id as a vcpu property and
> > > have the machine type set it if it wants to override?
> > >   
> > 
> > And the default would be cs->cpu_index I guess, correct ?  
> 
> That was my intention.  However Bharata has since pointed out that
> this doesn't work, because the cpu_index is only computed at realize time.
> 

Yes. That's why Igor had suggested to stop using cpu_index to machine
specific ids, like x86 already does (see pc_cpus_init()).

I followed a similar logic in this patch. The resulting DT ids are exactly
the same.

Do we really need a property then ?

> > > dt_id is probably not a good name though, since there's no guarantee
> > > the machine type actually uses a dt, which is part of the point of
> > > this series.
> > >   
> > 
> > machine_cpu_index ?
> >   
> > > > 
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > ---
> > > >  hw/ppc/e500.c           |    2 +-
> > > >  hw/ppc/mac_newworld.c   |    2 +-
> > > >  hw/ppc/mac_oldworld.c   |    2 +-
> > > >  hw/ppc/ppc.c            |    5 ++++-
> > > >  hw/ppc/ppc440_bamboo.c  |    2 +-
> > > >  hw/ppc/ppc4xx_devs.c    |    2 +-
> > > >  hw/ppc/prep.c           |    2 +-
> > > >  hw/ppc/spapr.c          |    5 ++++-
> > > >  hw/ppc/spapr_cpu_core.c |    7 +++++--
> > > >  hw/ppc/virtex_ml507.c   |    2 +-
> > > >  include/hw/ppc/ppc.h    |    2 +-
> > > >  11 files changed, 21 insertions(+), 12 deletions(-)
> > > > 
> > > > diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> > > > index b9221cc2c14a..513a42a3f952 100644
> > > > --- a/hw/ppc/e500.c
> > > > +++ b/hw/ppc/e500.c
> > > > @@ -823,7 +823,7 @@ void ppce500_init(MachineState *machine, PPCE500Params *params)
> > > >          CPUState *cs;
> > > >          qemu_irq *input;
> > > >  
> > > > -        cpu = ppc_cpu_init(machine->cpu_model);
> > > > +        cpu = ppc_cpu_init(machine->cpu_model, i);
> > > >          if (cpu == NULL) {
> > > >              fprintf(stderr, "Unable to initialize CPU!\n");
> > > >              exit(1);
> > > > diff --git a/hw/ppc/mac_newworld.c b/hw/ppc/mac_newworld.c
> > > > index 366089085844..9ef9504ff29c 100644
> > > > --- a/hw/ppc/mac_newworld.c
> > > > +++ b/hw/ppc/mac_newworld.c
> > > > @@ -194,7 +194,7 @@ static void ppc_core99_init(MachineState *machine)
> > > >      }
> > > >      ppc_cpu_parse_features(machine->cpu_model);
> > > >      for (i = 0; i < smp_cpus; i++) {
> > > > -        cpu = ppc_cpu_init(machine->cpu_model);
> > > > +        cpu = ppc_cpu_init(machine->cpu_model, i);
> > > >          if (cpu == NULL) {
> > > >              fprintf(stderr, "Unable to find PowerPC CPU definition\n");
> > > >              exit(1);
> > > > diff --git a/hw/ppc/mac_oldworld.c b/hw/ppc/mac_oldworld.c
> > > > index 5c2dc53cb584..0b09a8da8bf2 100644
> > > > --- a/hw/ppc/mac_oldworld.c
> > > > +++ b/hw/ppc/mac_oldworld.c
> > > > @@ -114,7 +114,7 @@ static void ppc_heathrow_init(MachineState *machine)
> > > >          machine->cpu_model = "G3";
> > > >      ppc_cpu_parse_features(machine->cpu_model);
> > > >      for (i = 0; i < smp_cpus; i++) {
> > > > -        cpu = ppc_cpu_init(machine->cpu_model);
> > > > +        cpu = ppc_cpu_init(machine->cpu_model, i);
> > > >          if (cpu == NULL) {
> > > >              fprintf(stderr, "Unable to find PowerPC CPU definition\n");
> > > >              exit(1);
> > > > diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> > > > index 0df32a9b3965..3d7739b9540e 100644
> > > > --- a/hw/ppc/ppc.c
> > > > +++ b/hw/ppc/ppc.c
> > > > @@ -1353,7 +1353,7 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
> > > >      return NULL;
> > > >  }
> > > >  
> > > > -PowerPCCPU *ppc_cpu_init(const char *cpu_model)
> > > > +PowerPCCPU *ppc_cpu_init(const char *cpu_model, unsigned vcpu_dt_id)
> > > >  {
> > > >      PowerPCCPU *cpu;
> > > >      ObjectClass *oc;
> > > > @@ -1373,6 +1373,9 @@ PowerPCCPU *ppc_cpu_init(const char *cpu_model)
> > > >      }
> > > >  
> > > >      cpu = POWERPC_CPU(object_new(object_class_get_name(oc)));
> > > > +
> > > > +    cpu->cpu_dt_id = vcpu_dt_id;
> > > > +
> > > >      object_property_set_bool(OBJECT(cpu), true, "realized", &err);
> > > >  
> > > >  out:
> > > > diff --git a/hw/ppc/ppc440_bamboo.c b/hw/ppc/ppc440_bamboo.c
> > > > index 709a779cba7e..9dbe87207304 100644
> > > > --- a/hw/ppc/ppc440_bamboo.c
> > > > +++ b/hw/ppc/ppc440_bamboo.c
> > > > @@ -187,7 +187,7 @@ static void bamboo_init(MachineState *machine)
> > > >          machine->cpu_model = "440EP";
> > > >      }
> > > >      ppc_cpu_parse_features(machine->cpu_model);
> > > > -    cpu = ppc_cpu_init(machine->cpu_model);
> > > > +    cpu = ppc_cpu_init(machine->cpu_model, 0);
> > > >      if (cpu == NULL) {
> > > >          fprintf(stderr, "Unable to initialize CPU!\n");
> > > >          exit(1);
> > > > diff --git a/hw/ppc/ppc4xx_devs.c b/hw/ppc/ppc4xx_devs.c
> > > > index 8cbe50361e86..ac39202c0d1c 100644
> > > > --- a/hw/ppc/ppc4xx_devs.c
> > > > +++ b/hw/ppc/ppc4xx_devs.c
> > > > @@ -57,7 +57,7 @@ PowerPCCPU *ppc4xx_init(const char *cpu_model,
> > > >  
> > > >      /* init CPUs */
> > > >      ppc_cpu_parse_features(cpu_model);
> > > > -    cpu = ppc_cpu_init(cpu_model);
> > > > +    cpu = ppc_cpu_init(cpu_model, 0);
> > > >      if (cpu == NULL) {
> > > >          fprintf(stderr, "Unable to find PowerPC %s CPU definition\n",
> > > >                  cpu_model);
> > > > diff --git a/hw/ppc/prep.c b/hw/ppc/prep.c
> > > > index 99409a09f0ef..9e8cf233d98d 100644
> > > > --- a/hw/ppc/prep.c
> > > > +++ b/hw/ppc/prep.c
> > > > @@ -510,7 +510,7 @@ static void ppc_prep_init(MachineState *machine)
> > > >          machine->cpu_model = "602";
> > > >      ppc_cpu_parse_features(machine->cpu_model);
> > > >      for (i = 0; i < smp_cpus; i++) {
> > > > -        cpu = ppc_cpu_init(machine->cpu_model);
> > > > +        cpu = ppc_cpu_init(machine->cpu_model, i);
> > > >          if (cpu == NULL) {
> > > >              fprintf(stderr, "Unable to find PowerPC CPU definition\n");
> > > >              exit(1);
> > > > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > > > index 8ca80812f16a..baefc7bd279c 100644
> > > > --- a/hw/ppc/spapr.c
> > > > +++ b/hw/ppc/spapr.c
> > > > @@ -1849,7 +1849,10 @@ static void ppc_spapr_init(MachineState *machine)
> > > >          g_free(type);
> > > >      } else {
> > > >          for (i = 0; i < smp_cpus; i++) {
> > > > -            PowerPCCPU *cpu = ppc_cpu_init(machine->cpu_model);
> > > > +            unsigned core_index = i / smp_threads;
> > > > +            unsigned thread_index = i % smp_threads;
> > > > +            PowerPCCPU *cpu = ppc_cpu_init(machine->cpu_model,
> > > > +                                           core_index * smt + thread_index);
> > > >              if (cpu == NULL) {
> > > >                  error_report("Unable to find PowerPC CPU definition");
> > > >                  exit(1);
> > > > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > > > index 70b6b0b5ee17..e3a3024baf32 100644
> > > > --- a/hw/ppc/spapr_cpu_core.c
> > > > +++ b/hw/ppc/spapr_cpu_core.c
> > > > @@ -259,13 +259,16 @@ out:
> > > >      error_propagate(errp, local_err);
> > > >  }
> > > >  
> > > > -static void spapr_cpu_core_realize_child(Object *child, Error **errp)
> > > > +static void spapr_cpu_core_realize_child(Object *child, unsigned vcpu_dt_id,
> > > > +                                         Error **errp)
> > > >  {
> > > >      Error *local_err = NULL;
> > > >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> > > >      CPUState *cs = CPU(child);
> > > >      PowerPCCPU *cpu = POWERPC_CPU(cs);
> > > >  
> > > > +    cpu->cpu_dt_id = vcpu_dt_id;
> > > > +
> > > >      object_property_set_bool(child, true, "realized", &local_err);
> > > >      if (local_err) {
> > > >          error_propagate(errp, local_err);
> > > > @@ -306,7 +309,7 @@ static void spapr_cpu_core_realize(DeviceState *dev, Error **errp)
> > > >      for (j = 0; j < cc->nr_threads; j++) {
> > > >          obj = sc->threads + j * size;
> > > >  
> > > > -        spapr_cpu_core_realize_child(obj, &local_err);
> > > > +        spapr_cpu_core_realize_child(obj, cc->core_id + j, &local_err);
> > > >          if (local_err) {
> > > >              goto err;
> > > >          }
> > > > diff --git a/hw/ppc/virtex_ml507.c b/hw/ppc/virtex_ml507.c
> > > > index a19dd17addda..efbc58137df4 100644
> > > > --- a/hw/ppc/virtex_ml507.c
> > > > +++ b/hw/ppc/virtex_ml507.c
> > > > @@ -97,7 +97,7 @@ static PowerPCCPU *ppc440_init_xilinx(ram_addr_t *ram_size,
> > > >      qemu_irq *irqs;
> > > >  
> > > >      ppc_cpu_parse_features(cpu_model);
> > > > -    cpu = ppc_cpu_init(cpu_model);
> > > > +    cpu = ppc_cpu_init(cpu_model, 0);
> > > >      if (cpu == NULL) {
> > > >          fprintf(stderr, "Unable to initialize CPU!\n");
> > > >          exit(1);
> > > > diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
> > > > index 2ed063e09bec..379b2f5d8e3d 100644
> > > > --- a/include/hw/ppc/ppc.h
> > > > +++ b/include/hw/ppc/ppc.h
> > > > @@ -106,6 +106,6 @@ enum {
> > > >  /* ppc_booke.c */
> > > >  void ppc_booke_timers_init(PowerPCCPU *cpu, uint32_t freq, uint32_t flags);
> > > >  
> > > > -PowerPCCPU *ppc_cpu_init(const char *cpu_model);
> > > > +PowerPCCPU *ppc_cpu_init(const char *cpu_model, unsigned vcpu_dt_id);
> > > >  void ppc_cpu_parse_features(const char *cpu_model);
> > > >  #endif
> > > >     
> > >   
> >   
> 
> 
> 


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

[Qemu-devel] [PATCH v3 6/7] ppc: drop vcpu_idt_id bits from the target code

[Greg Kurz]

Now that all machine types provide vcpu_dt_id, we can safely drop these
bits from the target code.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 target-ppc/translate_init.c |   16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 775df72cf6c2..976a38bd30f8 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -9527,9 +9527,6 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
     PowerPCCPU *cpu = POWERPC_CPU(dev);
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     Error *local_err = NULL;
-#if !defined(CONFIG_USER_ONLY)
-    int max_smt = kvmppc_smt_threads();
-#endif
 
     cpu_exec_init(cs, &local_err);
     if (local_err != NULL) {
@@ -9537,19 +9534,6 @@ static void ppc_cpu_realizefn(DeviceState *dev, Error **errp)
         return;
     }
 
-#if !defined(CONFIG_USER_ONLY)
-    cpu->cpu_dt_id = (cs->cpu_index / smp_threads) * max_smt
-        + (cs->cpu_index % smp_threads);
-
-    if (kvm_enabled() && !kvm_vcpu_id_is_valid(cpu->cpu_dt_id)) {
-        error_setg(errp, "Can't create CPU with id %d in KVM", cpu->cpu_dt_id);
-        error_append_hint(errp, "Adjust the number of cpus to %d "
-                          "or try to raise the number of threads per core\n",
-                          cpu->cpu_dt_id * smp_threads / max_smt);
-        return;
-    }
-#endif
-
     if (tcg_enabled()) {
         if (ppc_fixup_cpu(cpu) != 0) {
             error_setg(errp, "Unable to emulate selected CPU with TCG");

[Qemu-devel] [PATCH v3 7/7] spapr: consolidate the logic of core cpu_dt_id

[Greg Kurz]

POWER5 and newer cpus from IBM have a specific numbering scheme for
DT ids. This is currently open coded in several places.

This patch consolidates the logic in helpers.

Suggested-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/ppc/spapr.c                  |   11 ++++-------
 hw/ppc/spapr_cpu_core.c         |   26 +++++++++++++++++++-------
 include/hw/ppc/spapr_cpu_core.h |    9 +++++++++
 3 files changed, 32 insertions(+), 14 deletions(-)

diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index baefc7bd279c..89e61b976c60 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -199,7 +199,6 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPRMachineState *spapr)
     int ret = 0, offset, cpus_offset;
     CPUState *cs;
     char cpu_model[32];
-    int smt = kvmppc_smt_threads();
     uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
 
     CPU_FOREACH(cs) {
@@ -207,7 +206,7 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPRMachineState *spapr)
         DeviceClass *dc = DEVICE_GET_CLASS(cs);
         int index = ppc_get_vcpu_dt_id(cpu);
 
-        if ((index % smt) != 0) {
+        if (!spapr_core_dt_id_is_valid(index)) {
             continue;
         }
 
@@ -735,7 +734,6 @@ static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr)
     CPUState *cs;
     int cpus_offset;
     char *nodename;
-    int smt = kvmppc_smt_threads();
 
     cpus_offset = fdt_add_subnode(fdt, 0, "cpus");
     _FDT(cpus_offset);
@@ -753,7 +751,7 @@ static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr)
         DeviceClass *dc = DEVICE_GET_CLASS(cs);
         int offset;
 
-        if ((index % smt) != 0) {
+        if (!spapr_core_dt_id_is_valid(index)) {
             continue;
         }
 
@@ -1822,7 +1820,7 @@ static void ppc_spapr_init(MachineState *machine)
 
         spapr->cores = g_new0(Object *, spapr_max_cores);
         for (i = 0; i < spapr_max_cores; i++) {
-            int core_dt_id = i * smt;
+            int core_dt_id = spapr_core_index_to_dt_id(i);
             sPAPRDRConnector *drc =
                 spapr_dr_connector_new(OBJECT(spapr),
                                        SPAPR_DR_CONNECTOR_TYPE_CPU, core_dt_id);
@@ -2386,7 +2384,6 @@ static HotpluggableCPUList *spapr_query_hotpluggable_cpus(MachineState *machine)
     HotpluggableCPUList *head = NULL;
     sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
     int spapr_max_cores = max_cpus / smp_threads;
-    int smt = kvmppc_smt_threads();
 
     for (i = 0; i < spapr_max_cores; i++) {
         HotpluggableCPUList *list_item = g_new0(typeof(*list_item), 1);
@@ -2396,7 +2393,7 @@ static HotpluggableCPUList *spapr_query_hotpluggable_cpus(MachineState *machine)
         cpu_item->type = spapr_get_cpu_core_type(machine->cpu_model);
         cpu_item->vcpus_count = smp_threads;
         cpu_props->has_core_id = true;
-        cpu_props->core_id = i * smt;
+        cpu_props->core_id = spapr_core_index_to_dt_id(i);
         /* TODO: add 'has_node/node' here to describe
            to which node core belongs */
 
diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
index e3a3024baf32..b104778350df 100644
--- a/hw/ppc/spapr_cpu_core.c
+++ b/hw/ppc/spapr_cpu_core.c
@@ -103,7 +103,6 @@ static void spapr_core_release(DeviceState *dev, void *opaque)
     size_t size = object_type_get_instance_size(typename);
     sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
     CPUCore *cc = CPU_CORE(dev);
-    int smt = kvmppc_smt_threads();
     int i;
 
     for (i = 0; i < cc->nr_threads; i++) {
@@ -117,7 +116,7 @@ static void spapr_core_release(DeviceState *dev, void *opaque)
         object_unparent(obj);
     }
 
-    spapr->cores[cc->core_id / smt] = NULL;
+    spapr->cores[spapr_dt_id_to_core_index(cc->core_id)] = NULL;
 
     g_free(sc->threads);
     object_unparent(OBJECT(dev));
@@ -160,10 +159,9 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     void *fdt = NULL;
     int fdt_offset = 0;
     int index;
-    int smt = kvmppc_smt_threads();
 
     drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, cc->core_id);
-    index = cc->core_id / smt;
+    index = spapr_dt_id_to_core_index(cc->core_id);
     spapr->cores[index] = OBJECT(dev);
 
     if (!smc->dr_cpu_enabled) {
@@ -217,7 +215,6 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
     sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
     int spapr_max_cores = max_cpus / smp_threads;
     int index;
-    int smt = kvmppc_smt_threads();
     Error *local_err = NULL;
     CPUCore *cc = CPU_CORE(dev);
     char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
@@ -238,12 +235,12 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
         goto out;
     }
 
-    if (cc->core_id % smt) {
+    if (!spapr_core_dt_id_is_valid(cc->core_id)) {
         error_setg(&local_err, "invalid core id %d\n", cc->core_id);
         goto out;
     }
 
-    index = cc->core_id / smt;
+    index = spapr_dt_id_to_core_index(cc->core_id);
     if (index < 0 || index >= spapr_max_cores) {
         error_setg(&local_err, "core id %d out of range", cc->core_id);
         goto out;
@@ -331,6 +328,21 @@ static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
     dc->realize = spapr_cpu_core_realize;
 }
 
+unsigned spapr_core_index_to_dt_id(unsigned index)
+{
+    return index * kvmppc_smt_threads();
+}
+
+unsigned spapr_dt_id_to_core_index(unsigned dt_id)
+{
+    return dt_id / kvmppc_smt_threads();
+}
+
+unsigned spapr_dt_id_to_thread_index(unsigned dt_id)
+{
+    return dt_id % kvmppc_smt_threads();
+}
+
 /*
  * instance_init routines from different flavours of sPAPR CPU cores.
  */
diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
index 1c9b3195cce9..be7be91308ce 100644
--- a/include/hw/ppc/spapr_cpu_core.h
+++ b/include/hw/ppc/spapr_cpu_core.h
@@ -33,4 +33,13 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
                      Error **errp);
 void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
                        Error **errp);
+
+unsigned spapr_core_index_to_dt_id(unsigned index);
+unsigned spapr_dt_id_to_core_index(unsigned dt_id);
+unsigned spapr_dt_id_to_thread_index(unsigned dt_id);
+
+static inline bool spapr_core_dt_id_is_valid(unsigned dt_id)
+{
+    return spapr_dt_id_to_thread_index(dt_id) == 0;
+}
 #endif

Re: [Qemu-devel] [PATCH v3 7/7] spapr: consolidate the logic of core cpu_dt_id

[David Gibson]

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

On Wed, Jul 06, 2016 at 02:14:59PM +0200, Greg Kurz wrote:
> POWER5 and newer cpus from IBM have a specific numbering scheme for
> DT ids. This is currently open coded in several places.
> 
> This patch consolidates the logic in helpers.
> 
> Suggested-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>

This seems backwards to me.  I think we should be constructing the
vcpu ids from the core id, not the other way around.  Doing things
that way will take a bit longer, but this seems like execessive work
as an interim cleanup until it's obsoleted by core based numbering.

> ---
>  hw/ppc/spapr.c                  |   11 ++++-------
>  hw/ppc/spapr_cpu_core.c         |   26 +++++++++++++++++++-------
>  include/hw/ppc/spapr_cpu_core.h |    9 +++++++++
>  3 files changed, 32 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index baefc7bd279c..89e61b976c60 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -199,7 +199,6 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPRMachineState *spapr)
>      int ret = 0, offset, cpus_offset;
>      CPUState *cs;
>      char cpu_model[32];
> -    int smt = kvmppc_smt_threads();
>      uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
>  
>      CPU_FOREACH(cs) {
> @@ -207,7 +206,7 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPRMachineState *spapr)
>          DeviceClass *dc = DEVICE_GET_CLASS(cs);
>          int index = ppc_get_vcpu_dt_id(cpu);
>  
> -        if ((index % smt) != 0) {
> +        if (!spapr_core_dt_id_is_valid(index)) {
>              continue;
>          }
>  
> @@ -735,7 +734,6 @@ static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr)
>      CPUState *cs;
>      int cpus_offset;
>      char *nodename;
> -    int smt = kvmppc_smt_threads();
>  
>      cpus_offset = fdt_add_subnode(fdt, 0, "cpus");
>      _FDT(cpus_offset);
> @@ -753,7 +751,7 @@ static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr)
>          DeviceClass *dc = DEVICE_GET_CLASS(cs);
>          int offset;
>  
> -        if ((index % smt) != 0) {
> +        if (!spapr_core_dt_id_is_valid(index)) {
>              continue;
>          }
>  
> @@ -1822,7 +1820,7 @@ static void ppc_spapr_init(MachineState *machine)
>  
>          spapr->cores = g_new0(Object *, spapr_max_cores);
>          for (i = 0; i < spapr_max_cores; i++) {
> -            int core_dt_id = i * smt;
> +            int core_dt_id = spapr_core_index_to_dt_id(i);
>              sPAPRDRConnector *drc =
>                  spapr_dr_connector_new(OBJECT(spapr),
>                                         SPAPR_DR_CONNECTOR_TYPE_CPU, core_dt_id);
> @@ -2386,7 +2384,6 @@ static HotpluggableCPUList *spapr_query_hotpluggable_cpus(MachineState *machine)
>      HotpluggableCPUList *head = NULL;
>      sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
>      int spapr_max_cores = max_cpus / smp_threads;
> -    int smt = kvmppc_smt_threads();
>  
>      for (i = 0; i < spapr_max_cores; i++) {
>          HotpluggableCPUList *list_item = g_new0(typeof(*list_item), 1);
> @@ -2396,7 +2393,7 @@ static HotpluggableCPUList *spapr_query_hotpluggable_cpus(MachineState *machine)
>          cpu_item->type = spapr_get_cpu_core_type(machine->cpu_model);
>          cpu_item->vcpus_count = smp_threads;
>          cpu_props->has_core_id = true;
> -        cpu_props->core_id = i * smt;
> +        cpu_props->core_id = spapr_core_index_to_dt_id(i);
>          /* TODO: add 'has_node/node' here to describe
>             to which node core belongs */
>  
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index e3a3024baf32..b104778350df 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -103,7 +103,6 @@ static void spapr_core_release(DeviceState *dev, void *opaque)
>      size_t size = object_type_get_instance_size(typename);
>      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
>      CPUCore *cc = CPU_CORE(dev);
> -    int smt = kvmppc_smt_threads();
>      int i;
>  
>      for (i = 0; i < cc->nr_threads; i++) {
> @@ -117,7 +116,7 @@ static void spapr_core_release(DeviceState *dev, void *opaque)
>          object_unparent(obj);
>      }
>  
> -    spapr->cores[cc->core_id / smt] = NULL;
> +    spapr->cores[spapr_dt_id_to_core_index(cc->core_id)] = NULL;
>  
>      g_free(sc->threads);
>      object_unparent(OBJECT(dev));
> @@ -160,10 +159,9 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      void *fdt = NULL;
>      int fdt_offset = 0;
>      int index;
> -    int smt = kvmppc_smt_threads();
>  
>      drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, cc->core_id);
> -    index = cc->core_id / smt;
> +    index = spapr_dt_id_to_core_index(cc->core_id);
>      spapr->cores[index] = OBJECT(dev);
>  
>      if (!smc->dr_cpu_enabled) {
> @@ -217,7 +215,6 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>      sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
>      int spapr_max_cores = max_cpus / smp_threads;
>      int index;
> -    int smt = kvmppc_smt_threads();
>      Error *local_err = NULL;
>      CPUCore *cc = CPU_CORE(dev);
>      char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
> @@ -238,12 +235,12 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>          goto out;
>      }
>  
> -    if (cc->core_id % smt) {
> +    if (!spapr_core_dt_id_is_valid(cc->core_id)) {
>          error_setg(&local_err, "invalid core id %d\n", cc->core_id);
>          goto out;
>      }
>  
> -    index = cc->core_id / smt;
> +    index = spapr_dt_id_to_core_index(cc->core_id);
>      if (index < 0 || index >= spapr_max_cores) {
>          error_setg(&local_err, "core id %d out of range", cc->core_id);
>          goto out;
> @@ -331,6 +328,21 @@ static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
>      dc->realize = spapr_cpu_core_realize;
>  }
>  
> +unsigned spapr_core_index_to_dt_id(unsigned index)
> +{
> +    return index * kvmppc_smt_threads();
> +}
> +
> +unsigned spapr_dt_id_to_core_index(unsigned dt_id)
> +{
> +    return dt_id / kvmppc_smt_threads();
> +}
> +
> +unsigned spapr_dt_id_to_thread_index(unsigned dt_id)
> +{
> +    return dt_id % kvmppc_smt_threads();
> +}
> +
>  /*
>   * instance_init routines from different flavours of sPAPR CPU cores.
>   */
> diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> index 1c9b3195cce9..be7be91308ce 100644
> --- a/include/hw/ppc/spapr_cpu_core.h
> +++ b/include/hw/ppc/spapr_cpu_core.h
> @@ -33,4 +33,13 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                       Error **errp);
>  void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
>                         Error **errp);
> +
> +unsigned spapr_core_index_to_dt_id(unsigned index);
> +unsigned spapr_dt_id_to_core_index(unsigned dt_id);
> +unsigned spapr_dt_id_to_thread_index(unsigned dt_id);
> +
> +static inline bool spapr_core_dt_id_is_valid(unsigned dt_id)
> +{
> +    return spapr_dt_id_to_thread_index(dt_id) == 0;
> +}
>  #endif
> 

-- 
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 v3 7/7] spapr: consolidate the logic of core cpu_dt_id

[Greg Kurz]

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

On Thu, 7 Jul 2016 12:05:02 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Jul 06, 2016 at 02:14:59PM +0200, Greg Kurz wrote:
> > POWER5 and newer cpus from IBM have a specific numbering scheme for
> > DT ids. This is currently open coded in several places.
> > 
> > This patch consolidates the logic in helpers.
> > 
> > Suggested-by: Bharata B Rao <bharata@linux.vnet.ibm.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>  
> 
> This seems backwards to me.  I think we should be constructing the
> vcpu ids from the core id, not the other way around.  Doing things

Just to be sure: you're talking about the spapr_dt_id_to_*_index() helpers ?

> that way will take a bit longer, but this seems like execessive work
> as an interim cleanup until it's obsoleted by core based numbering.
> 

I agree this patch isn't needed anyway, especially if the numbering is about to
evolve. I'll drop this patch in v4.

> > ---
> >  hw/ppc/spapr.c                  |   11 ++++-------
> >  hw/ppc/spapr_cpu_core.c         |   26 +++++++++++++++++++-------
> >  include/hw/ppc/spapr_cpu_core.h |    9 +++++++++
> >  3 files changed, 32 insertions(+), 14 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index baefc7bd279c..89e61b976c60 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -199,7 +199,6 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPRMachineState *spapr)
> >      int ret = 0, offset, cpus_offset;
> >      CPUState *cs;
> >      char cpu_model[32];
> > -    int smt = kvmppc_smt_threads();
> >      uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
> >  
> >      CPU_FOREACH(cs) {
> > @@ -207,7 +206,7 @@ static int spapr_fixup_cpu_dt(void *fdt, sPAPRMachineState *spapr)
> >          DeviceClass *dc = DEVICE_GET_CLASS(cs);
> >          int index = ppc_get_vcpu_dt_id(cpu);
> >  
> > -        if ((index % smt) != 0) {
> > +        if (!spapr_core_dt_id_is_valid(index)) {
> >              continue;
> >          }
> >  
> > @@ -735,7 +734,6 @@ static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr)
> >      CPUState *cs;
> >      int cpus_offset;
> >      char *nodename;
> > -    int smt = kvmppc_smt_threads();
> >  
> >      cpus_offset = fdt_add_subnode(fdt, 0, "cpus");
> >      _FDT(cpus_offset);
> > @@ -753,7 +751,7 @@ static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr)
> >          DeviceClass *dc = DEVICE_GET_CLASS(cs);
> >          int offset;
> >  
> > -        if ((index % smt) != 0) {
> > +        if (!spapr_core_dt_id_is_valid(index)) {
> >              continue;
> >          }
> >  
> > @@ -1822,7 +1820,7 @@ static void ppc_spapr_init(MachineState *machine)
> >  
> >          spapr->cores = g_new0(Object *, spapr_max_cores);
> >          for (i = 0; i < spapr_max_cores; i++) {
> > -            int core_dt_id = i * smt;
> > +            int core_dt_id = spapr_core_index_to_dt_id(i);
> >              sPAPRDRConnector *drc =
> >                  spapr_dr_connector_new(OBJECT(spapr),
> >                                         SPAPR_DR_CONNECTOR_TYPE_CPU, core_dt_id);
> > @@ -2386,7 +2384,6 @@ static HotpluggableCPUList *spapr_query_hotpluggable_cpus(MachineState *machine)
> >      HotpluggableCPUList *head = NULL;
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(machine);
> >      int spapr_max_cores = max_cpus / smp_threads;
> > -    int smt = kvmppc_smt_threads();
> >  
> >      for (i = 0; i < spapr_max_cores; i++) {
> >          HotpluggableCPUList *list_item = g_new0(typeof(*list_item), 1);
> > @@ -2396,7 +2393,7 @@ static HotpluggableCPUList *spapr_query_hotpluggable_cpus(MachineState *machine)
> >          cpu_item->type = spapr_get_cpu_core_type(machine->cpu_model);
> >          cpu_item->vcpus_count = smp_threads;
> >          cpu_props->has_core_id = true;
> > -        cpu_props->core_id = i * smt;
> > +        cpu_props->core_id = spapr_core_index_to_dt_id(i);
> >          /* TODO: add 'has_node/node' here to describe
> >             to which node core belongs */
> >  
> > diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> > index e3a3024baf32..b104778350df 100644
> > --- a/hw/ppc/spapr_cpu_core.c
> > +++ b/hw/ppc/spapr_cpu_core.c
> > @@ -103,7 +103,6 @@ static void spapr_core_release(DeviceState *dev, void *opaque)
> >      size_t size = object_type_get_instance_size(typename);
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> >      CPUCore *cc = CPU_CORE(dev);
> > -    int smt = kvmppc_smt_threads();
> >      int i;
> >  
> >      for (i = 0; i < cc->nr_threads; i++) {
> > @@ -117,7 +116,7 @@ static void spapr_core_release(DeviceState *dev, void *opaque)
> >          object_unparent(obj);
> >      }
> >  
> > -    spapr->cores[cc->core_id / smt] = NULL;
> > +    spapr->cores[spapr_dt_id_to_core_index(cc->core_id)] = NULL;
> >  
> >      g_free(sc->threads);
> >      object_unparent(OBJECT(dev));
> > @@ -160,10 +159,9 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >      void *fdt = NULL;
> >      int fdt_offset = 0;
> >      int index;
> > -    int smt = kvmppc_smt_threads();
> >  
> >      drc = spapr_dr_connector_by_id(SPAPR_DR_CONNECTOR_TYPE_CPU, cc->core_id);
> > -    index = cc->core_id / smt;
> > +    index = spapr_dt_id_to_core_index(cc->core_id);
> >      spapr->cores[index] = OBJECT(dev);
> >  
> >      if (!smc->dr_cpu_enabled) {
> > @@ -217,7 +215,6 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >      sPAPRMachineState *spapr = SPAPR_MACHINE(OBJECT(hotplug_dev));
> >      int spapr_max_cores = max_cpus / smp_threads;
> >      int index;
> > -    int smt = kvmppc_smt_threads();
> >      Error *local_err = NULL;
> >      CPUCore *cc = CPU_CORE(dev);
> >      char *base_core_type = spapr_get_cpu_core_type(machine->cpu_model);
> > @@ -238,12 +235,12 @@ void spapr_core_pre_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >          goto out;
> >      }
> >  
> > -    if (cc->core_id % smt) {
> > +    if (!spapr_core_dt_id_is_valid(cc->core_id)) {
> >          error_setg(&local_err, "invalid core id %d\n", cc->core_id);
> >          goto out;
> >      }
> >  
> > -    index = cc->core_id / smt;
> > +    index = spapr_dt_id_to_core_index(cc->core_id);
> >      if (index < 0 || index >= spapr_max_cores) {
> >          error_setg(&local_err, "core id %d out of range", cc->core_id);
> >          goto out;
> > @@ -331,6 +328,21 @@ static void spapr_cpu_core_class_init(ObjectClass *oc, void *data)
> >      dc->realize = spapr_cpu_core_realize;
> >  }
> >  
> > +unsigned spapr_core_index_to_dt_id(unsigned index)
> > +{
> > +    return index * kvmppc_smt_threads();
> > +}
> > +
> > +unsigned spapr_dt_id_to_core_index(unsigned dt_id)
> > +{
> > +    return dt_id / kvmppc_smt_threads();
> > +}
> > +
> > +unsigned spapr_dt_id_to_thread_index(unsigned dt_id)
> > +{
> > +    return dt_id % kvmppc_smt_threads();
> > +}
> > +
> >  /*
> >   * instance_init routines from different flavours of sPAPR CPU cores.
> >   */
> > diff --git a/include/hw/ppc/spapr_cpu_core.h b/include/hw/ppc/spapr_cpu_core.h
> > index 1c9b3195cce9..be7be91308ce 100644
> > --- a/include/hw/ppc/spapr_cpu_core.h
> > +++ b/include/hw/ppc/spapr_cpu_core.h
> > @@ -33,4 +33,13 @@ void spapr_core_plug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >                       Error **errp);
> >  void spapr_core_unplug(HotplugHandler *hotplug_dev, DeviceState *dev,
> >                         Error **errp);
> > +
> > +unsigned spapr_core_index_to_dt_id(unsigned index);
> > +unsigned spapr_dt_id_to_core_index(unsigned dt_id);
> > +unsigned spapr_dt_id_to_thread_index(unsigned dt_id);
> > +
> > +static inline bool spapr_core_dt_id_is_valid(unsigned dt_id)
> > +{
> > +    return spapr_dt_id_to_thread_index(dt_id) == 0;
> > +}
> >  #endif
> >   
> 


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