Re: [Qemu-devel] [RFC 06/17] ppc: Rename cpu_version to compat_pvr

[Alexey Kardashevskiy]

On 30/10/16 22:11, David Gibson wrote:
> The 'cpu_version' field in PowerPCCPU is badly named.  It's named after the
> 'cpu-version' device tree property where it is advertised, but that meaning
> may not be obvious in most places it appears.
> 
> Worse, it doesn't even really correspond to that device tree property.  The
> property contains either the processor's PVR, or, if the CPU is running in
> a compatibility mode, a special "logical PVR" representing which mode.
> 
> Rename the cpu_version field, and a number of related variables to
> compat_pvr to make this clearer.
> 
> Signed-off-by: David Gibson <address@hidden>
> ---
>  hw/ppc/spapr.c              |  4 ++--
>  hw/ppc/spapr_hcall.c        | 30 +++++++++++++++---------------
>  target-ppc/cpu.h            |  6 +++---
>  target-ppc/kvm.c            |  4 ++--
>  target-ppc/kvm_ppc.h        |  4 ++--
>  target-ppc/translate_init.c | 10 +++++-----
>  6 files changed, 29 insertions(+), 29 deletions(-)
> 
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index b7762ee..276cefa 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -149,8 +149,8 @@ static int spapr_fixup_cpu_smt_dt(void *fdt, int offset, 
> PowerPCCPU *cpu,
>      uint32_t gservers_prop[smt_threads * 2];
>      int index = ppc_get_vcpu_dt_id(cpu);
>  
> -    if (cpu->cpu_version) {
> -        ret = fdt_setprop_cell(fdt, offset, "cpu-version", cpu->cpu_version);
> +    if (cpu->compat_pvr) {


Nit: g_assert(cpu->compat_pvr & 0x0F000000); may be?


Reviewed-by: Alexey Kardashevskiy <address@hidden>




> +        ret = fdt_setprop_cell(fdt, offset, "cpu-version", cpu->compat_pvr);
>          if (ret < 0) {
>              return ret;
>          }
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index b5544cb..d93f580 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -882,7 +882,7 @@ static target_ulong h_set_mode(PowerPCCPU *cpu, 
> sPAPRMachineState *spapr,
>  }
>  
>  typedef struct {
> -    uint32_t cpu_version;
> +    uint32_t compat_pvr;
>      Error *err;
>  } SetCompatState;
>  
> @@ -892,7 +892,7 @@ static void do_set_compat(CPUState *cs, void *arg)
>      SetCompatState *s = arg;
>  
>      cpu_synchronize_state(cs);
> -    ppc_set_compat(cpu, s->cpu_version, &s->err);
> +    ppc_set_compat(cpu, s->compat_pvr, &s->err);
>  }
>  
>  #define get_compat_level(cpuver) ( \
> @@ -903,7 +903,7 @@ static void do_set_compat(CPUState *cs, void *arg)
>  
>  static void cas_handle_compat_cpu(PowerPCCPUClass *pcc, uint32_t pvr,
>                                    unsigned max_lvl, unsigned *compat_lvl,
> -                                  unsigned *cpu_version)
> +                                  unsigned *compat_pvr)
>  {
>      unsigned lvl = get_compat_level(pvr);
>      bool is205, is206, is207;
> @@ -926,12 +926,12 @@ static void cas_handle_compat_cpu(PowerPCCPUClass *pcc, 
> uint32_t pvr,
>              /* User did not set the level, choose the highest */
>              if (*compat_lvl <= lvl) {
>                  *compat_lvl = lvl;
> -                *cpu_version = pvr;
> +                *compat_pvr = pvr;
>              }
>          } else if (max_lvl >= lvl) {
>              /* User chose the level, don't set higher than this */
>              *compat_lvl = lvl;
> -            *cpu_version = pvr;
> +            *compat_pvr = pvr;
>          }
>      }
>  }
> @@ -946,8 +946,8 @@ static target_ulong 
> h_client_architecture_support(PowerPCCPU *cpu_,
>      PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu_);
>      CPUState *cs;
>      bool cpu_match = false;
> -    unsigned old_cpu_version = cpu_->cpu_version;
> -    unsigned compat_lvl = 0, cpu_version = 0;
> +    unsigned old_compat_pvr = cpu_->compat_pvr;
> +    unsigned compat_lvl = 0, compat_pvr = 0;
>      unsigned max_lvl = get_compat_level(cpu_->max_compat);
>      int counter;
>      sPAPROptionVector *ov5_guest, *ov5_cas_old, *ov5_updates;
> @@ -965,12 +965,12 @@ static target_ulong 
> h_client_architecture_support(PowerPCCPU *cpu_,
>          if (!max_lvl &&
>              ((cpu_->env.spr[SPR_PVR] & pvr_mask) == (pvr & pvr_mask))) {
>              cpu_match = true;
> -            cpu_version = 0;
> -        } else if (pvr == cpu_->cpu_version) {
> +            compat_pvr = 0;
> +        } else if (pvr == cpu_->compat_pvr) {
>              cpu_match = true;
> -            cpu_version = cpu_->cpu_version;
> +            compat_pvr = cpu_->compat_pvr;
>          } else if (!cpu_match) {
> -            cas_handle_compat_cpu(pcc, pvr, max_lvl, &compat_lvl, 
> &cpu_version);
> +            cas_handle_compat_cpu(pcc, pvr, max_lvl, &compat_lvl, 
> &compat_pvr);
>          }
>          /* Terminator record */
>          if (~pvr_mask & pvr) {
> @@ -979,14 +979,14 @@ static target_ulong 
> h_client_architecture_support(PowerPCCPU *cpu_,
>      }
>  
>      /* Parsing finished */
> -    trace_spapr_cas_pvr(cpu_->cpu_version, cpu_match,
> -                        cpu_version, pcc->pcr_mask);
> +    trace_spapr_cas_pvr(cpu_->compat_pvr, cpu_match,
> +                        compat_pvr, pcc->pcr_mask);
>  
>      /* Update CPUs */
> -    if (old_cpu_version != cpu_version) {
> +    if (old_compat_pvr != compat_pvr) {
>          CPU_FOREACH(cs) {
>              SetCompatState s = {
> -                .cpu_version = cpu_version,
> +                .compat_pvr = compat_pvr,
>                  .err = NULL,
>              };
>  
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index a655105..f7656cb 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1156,7 +1156,7 @@ typedef struct PPCVirtualHypervisorClass 
> PPCVirtualHypervisorClass;
>   * @env: #CPUPPCState
>   * @cpu_dt_id: CPU index used in the device tree. KVM uses this index too
>   * @max_compat: Maximal supported logical PVR from the command line
> - * @cpu_version: Current logical PVR, zero if in "raw" mode
> + * @compat_pvr: Current logical PVR, zero if in "raw" mode
>   *
>   * A PowerPC CPU.
>   */
> @@ -1168,7 +1168,7 @@ struct PowerPCCPU {
>      CPUPPCState env;
>      int cpu_dt_id;
>      uint32_t max_compat;
> -    uint32_t cpu_version;
> +    uint32_t compat_pvr;
>      PPCVirtualHypervisor *vhyp;
>  };
>  
> @@ -1243,7 +1243,7 @@ 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)
> -void ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version, Error **errp);
> +void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp);
>  #endif
>  
>  /* Time-base and decrementer management */
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 9c4834c..15e12f3 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -2103,9 +2103,9 @@ void kvmppc_set_papr(PowerPCCPU *cpu)
>      cap_papr = 1;
>  }
>  
> -int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version)
> +int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr)
>  {
> -    return kvm_set_one_reg(CPU(cpu), KVM_REG_PPC_ARCH_COMPAT, &cpu_version);
> +    return kvm_set_one_reg(CPU(cpu), KVM_REG_PPC_ARCH_COMPAT, &compat_pvr);
>  }
>  
>  void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy)
> diff --git a/target-ppc/kvm_ppc.h b/target-ppc/kvm_ppc.h
> index bd1d78b..841a29b 100644
> --- a/target-ppc/kvm_ppc.h
> +++ b/target-ppc/kvm_ppc.h
> @@ -26,7 +26,7 @@ void kvmppc_enable_logical_ci_hcalls(void);
>  void kvmppc_enable_set_mode_hcall(void);
>  void kvmppc_enable_clear_ref_mod_hcalls(void);
>  void kvmppc_set_papr(PowerPCCPU *cpu);
> -int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version);
> +int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr);
>  void kvmppc_set_mpic_proxy(PowerPCCPU *cpu, int mpic_proxy);
>  int kvmppc_smt_threads(void);
>  int kvmppc_clear_tsr_bits(PowerPCCPU *cpu, uint32_t tsr_bits);
> @@ -123,7 +123,7 @@ static inline void kvmppc_set_papr(PowerPCCPU *cpu)
>  {
>  }
>  
> -static inline int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version)
> +static inline int kvmppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr)
>  {
>      return 0;
>  }
> diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
> index 21899a4..c35065d 100644
> --- a/target-ppc/translate_init.c
> +++ b/target-ppc/translate_init.c
> @@ -9957,7 +9957,7 @@ int ppc_get_compat_smt_threads(PowerPCCPU *cpu)
>      CPUState *cs = CPU(cpu);
>      int ret = MIN(cs->nr_threads, kvmppc_smt_threads());
>  
> -    switch (cpu->cpu_version) {
> +    switch (cpu->compat_pvr) {
>      case CPU_POWERPC_LOGICAL_2_05:
>          ret = MIN(ret, 2);
>          break;
> @@ -9973,15 +9973,15 @@ int ppc_get_compat_smt_threads(PowerPCCPU *cpu)
>  }
>  
>  #ifdef TARGET_PPC64
> -void ppc_set_compat(PowerPCCPU *cpu, uint32_t cpu_version, Error **errp)
> +void ppc_set_compat(PowerPCCPU *cpu, uint32_t compat_pvr, Error **errp)
>  {
>      int ret = 0;
>      CPUPPCState *env = &cpu->env;
>      PowerPCCPUClass *host_pcc;
>  
> -    cpu->cpu_version = cpu_version;
> +    cpu->compat_pvr = compat_pvr;
>  
> -    switch (cpu_version) {
> +    switch (compat_pvr) {
>      case CPU_POWERPC_LOGICAL_2_05:
>          env->spr[SPR_PCR] = PCR_TM_DIS | PCR_VSX_DIS | PCR_COMPAT_2_07 |
>                              PCR_COMPAT_2_06 | PCR_COMPAT_2_05;
> @@ -10004,7 +10004,7 @@ void ppc_set_compat(PowerPCCPU *cpu, uint32_t 
> cpu_version, Error **errp)
>      }
>  
>      if (kvm_enabled()) {
> -        ret = kvmppc_set_compat(cpu, cpu->cpu_version);
> +        ret = kvmppc_set_compat(cpu, cpu->compat_pvr);
>          if (ret < 0) {
>              error_setg_errno(errp, -ret,
>                               "Unable to set CPU compatibility mode in KVM");
> 


-- 
Alexey