Re: [Qemu-devel] [PATCH 04/14] ppc: introduce ppc_get_cr and ppc_set_cr

[Tom Musta <tommusta@gmail.com>]

On 9/15/2014 10:03 AM, Paolo Bonzini wrote:
> New functions to put together all 32 CR bits.  Avoids easy
> off-by-one mistakes such as the one fixed by commit f13f529
> (ppc: fix monitor access to CR, 2014-08-28).
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> 	v1->v2: replaces "ppc: use ARRAY_SIZE in gdbstub.c"
> 
>  linux-user/elfload.c |  4 +---
>  linux-user/signal.c  |  8 ++------
>  monitor.c            |  9 +--------
>  target-ppc/cpu.h     | 20 ++++++++++++++++++++
>  target-ppc/gdbstub.c | 42 ++++++++----------------------------------
>  target-ppc/kvm.c     | 11 ++---------
>  6 files changed, 34 insertions(+), 60 deletions(-)
> 
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index bea803b..a7d1714 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -857,9 +857,7 @@ static void elf_core_copy_regs(target_elf_gregset_t *regs, const CPUPPCState *en
>      (*regs)[36] = tswapreg(env->lr);
>      (*regs)[37] = tswapreg(env->xer);
>  
> -    for (i = 0; i < ARRAY_SIZE(env->crf); i++) {
> -        ccr |= env->crf[i] << (32 - ((i + 1) * 4));
> -    }
> +    ccr = ppc_get_cr(env);
>      (*regs)[38] = tswapreg(ccr);
>  }
>  
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index e11b208..97c3107 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -4534,9 +4534,7 @@ static void save_user_regs(CPUPPCState *env, struct target_mcontext *frame)
>      __put_user(env->lr, &frame->mc_gregs[TARGET_PT_LNK]);
>      __put_user(env->xer, &frame->mc_gregs[TARGET_PT_XER]);
>  
> -    for (i = 0; i < ARRAY_SIZE(env->crf); i++) {
> -        ccr |= env->crf[i] << (32 - ((i + 1) * 4));
> -    }
> +    ccr = ppc_get_cr(env);
>      __put_user(ccr, &frame->mc_gregs[TARGET_PT_CCR]);
>  
>      /* Save Altivec registers if necessary.  */
> @@ -4616,9 +4614,7 @@ static void restore_user_regs(CPUPPCState *env,
>      __get_user(env->xer, &frame->mc_gregs[TARGET_PT_XER]);
>      __get_user(ccr, &frame->mc_gregs[TARGET_PT_CCR]);
>  
> -    for (i = 0; i < ARRAY_SIZE(env->crf); i++) {
> -        env->crf[i] = (ccr >> (32 - ((i + 1) * 4))) & 0xf;
> -    }
> +    ppc_set_cr(env, ccr);
>  
>      if (!sig) {
>          env->gpr[2] = save_r2;
> diff --git a/monitor.c b/monitor.c
> index ec73dd4..80acf25 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -2963,14 +2963,7 @@ static target_long monitor_get_pc (const struct MonitorDef *md, int val)
>  static target_long monitor_get_ccr (const struct MonitorDef *md, int val)
>  {
>      CPUArchState *env = mon_get_cpu();
> -    unsigned int u;
> -    int i;
> -
> -    u = 0;
> -    for (i = 0; i < 8; i++)
> -        u |= env->crf[i] << (32 - (4 * (i + 1)));
> -
> -    return u;
> +    return ppc_get_cr(env);
>  }
>  
>  static target_long monitor_get_msr (const struct MonitorDef *md, int val)
> diff --git a/target-ppc/cpu.h b/target-ppc/cpu.h
> index c29ce3b..0c0196d 100644
> --- a/target-ppc/cpu.h
> +++ b/target-ppc/cpu.h
> @@ -1197,6 +1197,26 @@ void ppc_tlb_invalidate_one (CPUPPCState *env, target_ulong addr);
>  
>  void store_fpscr(CPUPPCState *env, uint64_t arg, uint32_t mask);
>  
> +static inline uint32_t ppc_get_cr(const CPUPPCState *env)
> +{
> +    uint32_t cr = 0;
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(env->crf); i++) {
> +        cr |= env->crf[i] << (32 - ((i + 1) * 4));
> +    }
> +    return cr;
> +}
> +
> +static inline void ppc_set_cr(CPUPPCState *env, uint32_t cr)
> +{
> +    int i;
> +
> +    for (i = 0; i < ARRAY_SIZE(env->crf); i++) {
> +        env->crf[i] = (cr >> (32 - ((i + 1) * 4))) & 0xF;
> +    }
> +}
> +
>  static inline uint64_t ppc_dump_gpr(CPUPPCState *env, int gprn)
>  {
>      uint64_t gprv;
> diff --git a/target-ppc/gdbstub.c b/target-ppc/gdbstub.c
> index 14675f4..11d138e 100644
> --- a/target-ppc/gdbstub.c
> +++ b/target-ppc/gdbstub.c
> @@ -135,15 +135,8 @@ int ppc_cpu_gdb_read_register(CPUState *cs, uint8_t *mem_buf, int n)
>              gdb_get_regl(mem_buf, env->msr);
>              break;
>          case 66:
> -            {
> -                uint32_t cr = 0;
> -                int i;
> -                for (i = 0; i < 8; i++) {
> -                    cr |= env->crf[i] << (32 - ((i + 1) * 4));
> -                }
> -                gdb_get_reg32(mem_buf, cr);
> -                break;
> -            }
> +            gdb_get_reg32(mem_buf, ppc_get_cr(env));
> +            break;
>          case 67:
>              gdb_get_regl(mem_buf, env->lr);
>              break;
> @@ -191,15 +184,8 @@ int ppc_cpu_gdb_read_register_apple(CPUState *cs, uint8_t *mem_buf, int n)
>              gdb_get_reg64(mem_buf, env->msr);
>              break;
>          case 66 + 32:
> -            {
> -                uint32_t cr = 0;
> -                int i;
> -                for (i = 0; i < 8; i++) {
> -                    cr |= env->crf[i] << (32 - ((i + 1) * 4));
> -                }
> -                gdb_get_reg32(mem_buf, cr);
> -                break;
> -            }
> +            gdb_get_reg32(mem_buf, ppc_get_cr(env));
> +            break;
>          case 67 + 32:
>              gdb_get_reg64(mem_buf, env->lr);
>              break;
> @@ -243,14 +229,8 @@ int ppc_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
>              ppc_store_msr(env, ldtul_p(mem_buf));
>              break;
>          case 66:
> -            {
> -                uint32_t cr = ldl_p(mem_buf);
> -                int i;
> -                for (i = 0; i < 8; i++) {
> -                    env->crf[i] = (cr >> (32 - ((i + 1) * 4))) & 0xF;
> -                }
> -                break;
> -            }
> +            ppc_set_cr(env, ldl_p(mem_buf));
> +            break;
>          case 67:
>              env->lr = ldtul_p(mem_buf);
>              break;
> @@ -293,14 +273,8 @@ int ppc_cpu_gdb_write_register_apple(CPUState *cs, uint8_t *mem_buf, int n)
>              ppc_store_msr(env, ldq_p(mem_buf));
>              break;
>          case 66 + 32:
> -            {
> -                uint32_t cr = ldl_p(mem_buf);
> -                int i;
> -                for (i = 0; i < 8; i++) {
> -                    env->crf[i] = (cr >> (32 - ((i + 1) * 4))) & 0xF;
> -                }
> -                break;
> -            }
> +            ppc_set_cr(env, ldl_p(mem_buf));
> +            break;
>          case 67 + 32:
>              env->lr = ldq_p(mem_buf);
>              break;
> diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
> index 9c23c6b..e541b9e 100644
> --- a/target-ppc/kvm.c
> +++ b/target-ppc/kvm.c
> @@ -831,10 +831,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>      for (i = 0;i < 32; i++)
>          regs.gpr[i] = env->gpr[i];
>  
> -    regs.cr = 0;
> -    for (i = 0; i < 8; i++) {
> -        regs.cr |= (env->crf[i] & 15) << (4 * (7 - i));
> -    }
> +    regs.cr = ppc_get_cr(env);
>  
>      ret = kvm_vcpu_ioctl(cs, KVM_SET_REGS, &regs);
>      if (ret < 0)
> @@ -955,11 +952,7 @@ int kvm_arch_get_registers(CPUState *cs)
>      if (ret < 0)
>          return ret;
>  
> -    cr = regs.cr;
> -    for (i = 7; i >= 0; i--) {
> -        env->crf[i] = cr & 15;
> -        cr >>= 4;
> -    }
> +    ppc_set_cr(env, regs.cr);
>  
>      env->ctr = regs.ctr;
>      env->lr = regs.lr;
> 

One minor issue with this patch:

  CC    ppc64-softmmu/target-ppc/kvm.o
/bghome/tmusta/powerisa/qemu/qemu/target-ppc/kvm.c: In function ?kvm_arch_get_registers?:
/bghome/tmusta/powerisa/qemu/qemu/target-ppc/kvm.c:948: warning: unused variable ?cr?

which, of course, can be fixed like this:

> git diff
diff --git a/target-ppc/kvm.c b/target-ppc/kvm.c
index e541b9e..74c1324 100644
--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -945,7 +945,6 @@ int kvm_arch_get_registers(CPUState *cs)
     CPUPPCState *env = &cpu->env;
     struct kvm_regs regs;
     struct kvm_sregs sregs;
-    uint32_t cr;
     int i, ret;

     ret = kvm_vcpu_ioctl(cs, KVM_GET_REGS, &regs);


Otherwise ...
Reviewed-by: Tom Musta <tommusta@gmail.com>