Re: [Qemu-ppc] [RFC 08/17] ppc: Rewrite ppc_get_compat_smt_threads()

[Alexey Kardashevskiy]

On 30/10/16 22:11, David Gibson wrote:
> To continue consolidation of compatibility mode information, this rewrites
> the ppc_get_compat_smt_threads() function using the table of compatiblity
> modes in target-ppc/compat.c.
> 
> It's not a direct replacement, the new ppc_compat_max_threads() function
> has simpler semantics - it just returns the number of threads the cpu
> model has, taking into account any compatiblity mode it is in.
> 
> This no longer takes into account kvmppc_smt_threads() as the previous
> version did.  That check wasn't useful because we check elsewhere that

Nit: s/elsewhere/in ppc_cpu_realizefn()/


Reviewed-by: Alexey Kardashevskiy <address@hidden>




> CPUs aren't instantiated with more threads than kvm allows (or if we didn't
> things will already be broken and this won't make it any worse).
> 
> Signed-off-by: David Gibson <address@hidden>
> ---
>  hw/ppc/spapr.c              |  8 ++++----
>  target-ppc/compat.c         | 18 ++++++++++++++++++
>  target-ppc/cpu.h            |  2 +-
>  target-ppc/translate_init.c | 20 --------------------
>  4 files changed, 23 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 276cefa..6c78889 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -207,6 +207,7 @@ static int spapr_fixup_cpu_dt(void *fdt, 
> sPAPRMachineState *spapr)
>          PowerPCCPU *cpu = POWERPC_CPU(cs);
>          DeviceClass *dc = DEVICE_GET_CLASS(cs);
>          int index = ppc_get_vcpu_dt_id(cpu);
> +        int compat_smt = MIN(smp_threads, ppc_compat_max_threads(cpu));
>  
>          if ((index % smt) != 0) {
>              continue;
> @@ -241,8 +242,7 @@ static int spapr_fixup_cpu_dt(void *fdt, 
> sPAPRMachineState *spapr)
>              return ret;
>          }
>  
> -        ret = spapr_fixup_cpu_smt_dt(fdt, offset, cpu,
> -                                     ppc_get_compat_smt_threads(cpu));
> +        ret = spapr_fixup_cpu_smt_dt(fdt, offset, cpu, compat_smt);
>          if (ret < 0) {
>              return ret;
>          }
> @@ -408,6 +408,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void 
> *fdt, int offset,
>      size_t page_sizes_prop_size;
>      uint32_t vcpus_per_socket = smp_threads * smp_cores;
>      uint32_t pft_size_prop[] = {0, cpu_to_be32(spapr->htab_shift)};
> +    int compat_smt = MIN(smp_threads, ppc_compat_max_threads(cpu));
>      sPAPRDRConnector *drc;
>      sPAPRDRConnectorClass *drck;
>      int drc_index;
> @@ -495,8 +496,7 @@ static void spapr_populate_cpu_dt(CPUState *cs, void 
> *fdt, int offset,
>  
>      _FDT(spapr_fixup_cpu_numa_dt(fdt, offset, cs));
>  
> -    _FDT(spapr_fixup_cpu_smt_dt(fdt, offset, cpu,
> -                                ppc_get_compat_smt_threads(cpu)));
> +    _FDT(spapr_fixup_cpu_smt_dt(fdt, offset, cpu, compat_smt));
>  }
>  
>  static void spapr_populate_cpus_dt_node(void *fdt, sPAPRMachineState *spapr)
> diff --git a/target-ppc/compat.c b/target-ppc/compat.c
> index f3fd9c6..66529a6 100644
> --- a/target-ppc/compat.c
> +++ b/target-ppc/compat.c
> @@ -28,6 +28,7 @@
>  typedef struct {
>      uint32_t pvr;
>      uint64_t pcr;
> +    int max_threads;
>  } CompatInfo;
>  
>  static const CompatInfo compat_table[] = {
> @@ -35,18 +36,22 @@ static const CompatInfo compat_table[] = {
>          .pvr = CPU_POWERPC_LOGICAL_2_05,
>          .pcr = PCR_COMPAT_2_07 | PCR_COMPAT_2_06 | PCR_COMPAT_2_05
>                 | PCR_TM_DIS | PCR_VSX_DIS,
> +        .max_threads = 2,
>      },
>      { /* POWER7, ISA2.06 */
>          .pvr = CPU_POWERPC_LOGICAL_2_06,
>          .pcr = PCR_COMPAT_2_07 | PCR_COMPAT_2_06 | PCR_TM_DIS,
> +        .max_threads = 4,
>      },
>      {
>          .pvr = CPU_POWERPC_LOGICAL_2_06_PLUS,
>          .pcr = PCR_COMPAT_2_07 | PCR_COMPAT_2_06 | PCR_TM_DIS,
> +        .max_threads = 4,
>      },
>      { /* POWER8, ISA2.07 */
>          .pvr = CPU_POWERPC_LOGICAL_2_07,
>          .pcr = PCR_COMPAT_2_07,
> +        .max_threads = 8,
>      },
>  };
>  
> @@ -89,3 +94,16 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, 
> Error **errp)
>          }
>      }
>  }
> +
> +int ppc_compat_max_threads(PowerPCCPU *cpu)
> +{
> +    const CompatInfo *compat = compat_by_pvr(cpu->compat_pvr);
> +    int n_threads = CPU(cpu)->nr_threads;
> +
> +    if (cpu->compat_pvr) {
> +        g_assert(compat);
> +        n_threads = MIN(n_threads, compat->max_threads);
> +    }
> +
> +    return n_threads;
> +}
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index 15d5e4b..cfda7b2 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1241,7 +1241,6 @@ void ppc_store_sdr1 (CPUPPCState *env, target_ulong 
> value);
>  void ppc_store_msr (CPUPPCState *env, target_ulong value);
>  
>  void ppc_cpu_list (FILE *f, fprintf_function cpu_fprintf);
> -int ppc_get_compat_smt_threads(PowerPCCPU *cpu);
>  #if defined(TARGET_PPC64)
>  #endif
>  
> @@ -1316,6 +1315,7 @@ static inline int cpu_mmu_index (CPUPPCState *env, bool 
> ifetch)
>  /* Compatibility modes */
>  #if defined(TARGET_PPC64)
>  void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp);
> +int ppc_compat_max_threads(PowerPCCPU *cpu);
>  #endif /* defined(TARGET_PPC64) */
>  
>  #include "exec/cpu-all.h"
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index a70eafb..ba48242 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -9952,26 +9952,6 @@ static void ppc_cpu_unrealizefn(DeviceState *dev, 
> Error **errp)
>      }
>  }
>  
> -int ppc_get_compat_smt_threads(PowerPCCPU *cpu)
> -{
> -    CPUState *cs = CPU(cpu);
> -    int ret = MIN(cs->nr_threads, kvmppc_smt_threads());
> -
> -    switch (cpu->compat_pvr) {
> -    case CPU_POWERPC_LOGICAL_2_05:
> -        ret = MIN(ret, 2);
> -        break;
> -    case CPU_POWERPC_LOGICAL_2_06:
> -        ret = MIN(ret, 4);
> -        break;
> -    case CPU_POWERPC_LOGICAL_2_07:
> -        ret = MIN(ret, 8);
> -        break;
> -    }
> -
> -    return ret;
> -}
> -
>  static gint ppc_cpu_compare_class_pvr(gconstpointer a, gconstpointer b)
>  {
>      ObjectClass *oc = (ObjectClass *)a;
> 


-- 
Alexey