[PATCH v3 0/2] Cleanup ppc cr get/set with helper routines

[PATCH v3 0/2] Cleanup ppc cr get/set with helper routines

[Harsh Prateek Bora]

This patchset introduces helper routines to get/set cr reg for better code
readability / maintenance. No functional changes intended with this patchset.

Changelog:

changes from v2:
 - addressed Nick's review comments
 - holding changes from 2/4, 3/4 for future
changes from v1:
 - addressed Fabiano's review comments
 - folding Reviewed-by tags in patch 1/4, 4/4


*** BLURB HERE ***

Harsh Prateek Bora (2):
  ppc: spapr: cleanup cr get/set with helpers.
  MAINTAINERS: Adding myself in the list for ppc/spapr

 MAINTAINERS               |  1 +
 hw/ppc/spapr_hcall.c      | 18 ++----------------
 linux-user/elfload.c      |  4 +---
 linux-user/ppc/signal.c   |  9 ++-------
 target/ppc/cpu.c          | 17 +++++++++++++++++
 target/ppc/cpu.h          |  2 ++
 target/ppc/gdbstub.c      | 22 ++++------------------
 target/ppc/kvm.c          | 13 ++-----------
 target/ppc/ppc-qmp-cmds.c |  6 +-----
 9 files changed, 32 insertions(+), 60 deletions(-)

-- 
2.31.1

[PATCH v3 1/2] ppc: spapr: cleanup cr get/set with helpers.

[Harsh Prateek Bora]

The bits in cr reg are grouped into eight 4-bit fields represented
by env->crf[8] and the related calculations should be abstracted to
keep the calling routines simpler to read. This is a step towards
cleaning up the related/calling code for better readability.

Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
---
 hw/ppc/spapr_hcall.c      | 18 ++----------------
 linux-user/elfload.c      |  4 +---
 linux-user/ppc/signal.c   |  9 ++-------
 target/ppc/cpu.c          | 17 +++++++++++++++++
 target/ppc/cpu.h          |  2 ++
 target/ppc/gdbstub.c      | 22 ++++------------------
 target/ppc/kvm.c          | 13 ++-----------
 target/ppc/ppc-qmp-cmds.c |  6 +-----
 8 files changed, 31 insertions(+), 60 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index ec4def62f8..1c102c8c0d 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1566,8 +1566,6 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
     struct kvmppc_hv_guest_state hv_state;
     struct kvmppc_pt_regs *regs;
     hwaddr len;
-    uint64_t cr;
-    int i;
 
     if (spapr->nested_ptcr == 0) {
         return H_NOT_AVAILABLE;
@@ -1616,12 +1614,7 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
     env->lr = regs->link;
     env->ctr = regs->ctr;
     cpu_write_xer(env, regs->xer);
-
-    cr = regs->ccr;
-    for (i = 7; i >= 0; i--) {
-        env->crf[i] = cr & 15;
-        cr >>= 4;
-    }
+    ppc_set_cr(env, regs->ccr);
 
     env->msr = regs->msr;
     env->nip = regs->nip;
@@ -1698,8 +1691,6 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
     struct kvmppc_hv_guest_state *hvstate;
     struct kvmppc_pt_regs *regs;
     hwaddr len;
-    uint64_t cr;
-    int i;
 
     assert(spapr_cpu->in_nested);
 
@@ -1757,12 +1748,7 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
     regs->link = env->lr;
     regs->ctr = env->ctr;
     regs->xer = cpu_read_xer(env);
-
-    cr = 0;
-    for (i = 0; i < 8; i++) {
-        cr |= (env->crf[i] & 15) << (4 * (7 - i));
-    }
-    regs->ccr = cr;
+    regs->ccr = ppc_get_cr(env);
 
     if (excp == POWERPC_EXCP_MCHECK ||
         excp == POWERPC_EXCP_RESET ||
diff --git a/linux-user/elfload.c b/linux-user/elfload.c
index f1370a7a8b..703f7434a0 100644
--- a/linux-user/elfload.c
+++ b/linux-user/elfload.c
@@ -961,9 +961,7 @@ static void elf_core_copy_regs(target_elf_gregset_t *regs, const CPUPPCState *en
     (*regs)[36] = tswapreg(env->lr);
     (*regs)[37] = tswapreg(cpu_read_xer(env));
 
-    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/ppc/signal.c b/linux-user/ppc/signal.c
index 07729c1653..a616f20efb 100644
--- a/linux-user/ppc/signal.c
+++ b/linux-user/ppc/signal.c
@@ -243,9 +243,7 @@ static void save_user_regs(CPUPPCState *env, struct target_mcontext *frame)
     __put_user(env->lr, &frame->mc_gregs[TARGET_PT_LNK]);
     __put_user(cpu_read_xer(env), &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.  */
@@ -335,10 +333,7 @@ static void restore_user_regs(CPUPPCState *env,
     cpu_write_xer(env, 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/target/ppc/cpu.c b/target/ppc/cpu.c
index 1a97b41c6b..241d9e27e5 100644
--- a/target/ppc/cpu.c
+++ b/target/ppc/cpu.c
@@ -67,6 +67,23 @@ uint32_t ppc_get_vscr(CPUPPCState *env)
     return env->vscr | (sat << VSCR_SAT);
 }
 
+void ppc_set_cr(CPUPPCState *env, uint64_t cr)
+{
+    for (int i = 7; i >= 0; i--) {
+        env->crf[i] = cr & 0xf;
+        cr >>= 4;
+    }
+}
+
+uint64_t ppc_get_cr(CPUPPCState *env)
+{
+    uint64_t cr = 0;
+    for (int i = 0; i < 8; i++) {
+        cr |= (env->crf[i] & 0xf) << (4 * (7 - i));
+    }
+    return cr;
+}
+
 /* GDBstub can read and write MSR... */
 void ppc_store_msr(CPUPPCState *env, target_ulong value)
 {
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 557d736dab..0af94170d0 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2773,6 +2773,8 @@ void dump_mmu(CPUPPCState *env);
 void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len);
 void ppc_store_vscr(CPUPPCState *env, uint32_t vscr);
 uint32_t ppc_get_vscr(CPUPPCState *env);
+void ppc_set_cr(CPUPPCState *env, uint64_t cr);
+uint64_t ppc_get_cr(CPUPPCState *env);
 
 /*****************************************************************************/
 /* Power management enable checks                                            */
diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
index d2bc1d7c53..63c9abe4f1 100644
--- a/target/ppc/gdbstub.c
+++ b/target/ppc/gdbstub.c
@@ -145,11 +145,7 @@ int ppc_cpu_gdb_read_register(CPUState *cs, GByteArray *buf, int n)
             break;
         case 66:
             {
-                uint32_t cr = 0;
-                int i;
-                for (i = 0; i < 8; i++) {
-                    cr |= env->crf[i] << (32 - ((i + 1) * 4));
-                }
+                uint32_t cr = ppc_get_cr(env);
                 gdb_get_reg32(buf, cr);
                 break;
             }
@@ -203,11 +199,7 @@ int ppc_cpu_gdb_read_register_apple(CPUState *cs, GByteArray *buf, int n)
             break;
         case 66 + 32:
             {
-                uint32_t cr = 0;
-                int i;
-                for (i = 0; i < 8; i++) {
-                    cr |= env->crf[i] << (32 - ((i + 1) * 4));
-                }
+                uint32_t cr = ppc_get_cr(env);
                 gdb_get_reg32(buf, cr);
                 break;
             }
@@ -257,10 +249,7 @@ int ppc_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
         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;
-                }
+                ppc_set_cr(env, cr);
                 break;
             }
         case 67:
@@ -307,10 +296,7 @@ int ppc_cpu_gdb_write_register_apple(CPUState *cs, uint8_t *mem_buf, int n)
         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;
-                }
+                ppc_set_cr(env, cr);
                 break;
             }
         case 67 + 32:
diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
index 78f6fc50cd..336e663bc3 100644
--- a/target/ppc/kvm.c
+++ b/target/ppc/kvm.c
@@ -927,10 +927,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
         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) {
@@ -1205,7 +1202,6 @@ int kvm_arch_get_registers(CPUState *cs)
     PowerPCCPU *cpu = POWERPC_CPU(cs);
     CPUPPCState *env = &cpu->env;
     struct kvm_regs regs;
-    uint32_t cr;
     int i, ret;
 
     ret = kvm_vcpu_ioctl(cs, KVM_GET_REGS, &regs);
@@ -1213,12 +1209,7 @@ int kvm_arch_get_registers(CPUState *cs)
         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;
     cpu_write_xer(env, regs.xer);
diff --git a/target/ppc/ppc-qmp-cmds.c b/target/ppc/ppc-qmp-cmds.c
index 36e5b5eff8..f9acc21056 100644
--- a/target/ppc/ppc-qmp-cmds.c
+++ b/target/ppc/ppc-qmp-cmds.c
@@ -37,12 +37,8 @@ static target_long monitor_get_ccr(Monitor *mon, const struct MonitorDef *md,
 {
     CPUArchState *env = mon_get_cpu_env(mon);
     unsigned int u;
-    int i;
 
-    u = 0;
-    for (i = 0; i < 8; i++) {
-        u |= env->crf[i] << (32 - (4 * (i + 1)));
-    }
+    u = ppc_get_cr(env);
 
     return u;
 }
-- 
2.31.1

Re: [PATCH v3 1/2] ppc: spapr: cleanup cr get/set with helpers.

[Richard Henderson]

On 5/3/23 10:36, Harsh Prateek Bora wrote:
> The bits in cr reg are grouped into eight 4-bit fields represented
> by env->crf[8] and the related calculations should be abstracted to
> keep the calling routines simpler to read. This is a step towards
> cleaning up the related/calling code for better readability.
> 
> Signed-off-by: Harsh Prateek Bora<harshpb@linux.ibm.com>
> Reviewed-by: Fabiano Rosas<farosas@suse.de>
> ---
>   hw/ppc/spapr_hcall.c      | 18 ++----------------
>   linux-user/elfload.c      |  4 +---
>   linux-user/ppc/signal.c   |  9 ++-------
>   target/ppc/cpu.c          | 17 +++++++++++++++++
>   target/ppc/cpu.h          |  2 ++
>   target/ppc/gdbstub.c      | 22 ++++------------------
>   target/ppc/kvm.c          | 13 ++-----------
>   target/ppc/ppc-qmp-cmds.c |  6 +-----
>   8 files changed, 31 insertions(+), 60 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v3 1/2] ppc: spapr: cleanup cr get/set with helpers.

[Daniel Henrique Barboza]

This patch breaks linux-user build as follows:

[23/214] Compiling C object libqemu-ppc64-linux-user.fa.p/linux-user_elfload.c.o
FAILED: libqemu-ppc64-linux-user.fa.p/linux-user_elfload.c.o
cc -m64 -mcx16 -Ilibqemu-ppc64-linux-user.fa.p -I. -I.. -Itarget/ppc -I../target/ppc -I../common-user/host/x86_64 -I../linux-user/include/host/x86_64 -I../linux-user/include -Ilinux-user -I../linux-user -Ilinux-user/ppc -I../linux-user/ppc -Iqapi -Itrace -Iui -Iui/shader -I/usr/include/glib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/sysprof-4 -fdiagnostics-color=auto -Wall -Winvalid-pch -Werror -std=gnu11 -O2 -g -isystem /home/danielhb/powerpc/qemu/linux-headers -isystem linux-headers -iquote . -iquote /home/danielhb/powerpc/qemu -iquote /home/danielhb/powerpc/qemu/include -iquote /home/danielhb/powerpc/qemu/tcg/i386 -pthread -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -fno-strict-aliasing -fno-common -fwrapv -Wundef -Wwrite-strings -Wmissing-prototypes -Wstrict-prototypes -Wredundant-decls -Wold-style-declaration -Wold-style-definition -Wtype-limits -Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers -Wempty-body -Wnested-externs -Wendif-labels -Wexpansion-to-defined -Wimplicit-fallthrough=2 -Wmissing-format-attribute -Wno-missing-include-dirs -Wno-shift-negative-value -Wno-psabi -fstack-protector-strong -fPIE -isystem../linux-headers -isystemlinux-headers -DNEED_CPU_H '-DCONFIG_TARGET="ppc64-linux-user-config-target.h"' '-DCONFIG_DEVICES="ppc64-linux-user-config-devices.h"' -MD -MQ libqemu-ppc64-linux-user.fa.p/linux-user_elfload.c.o -MF libqemu-ppc64-linux-user.fa.p/linux-user_elfload.c.o.d -o libqemu-ppc64-linux-user.fa.p/linux-user_elfload.c.o -c ../linux-user/elfload.c
../linux-user/elfload.c: In function ‘elf_core_copy_regs’:
../linux-user/elfload.c:964:22: error: passing argument 1 of ‘ppc_get_cr’ discards ‘const’ qualifier from pointer target type [-Werror=discarded-qualifiers]
    964 |     ccr = ppc_get_cr(env);
        |                      ^~~
In file included from ../linux-user/qemu.h:4,
                   from ../linux-user/elfload.c:8:
../target/ppc/cpu.h:2777:34: note: expected ‘CPUPPCState *’ {aka ‘struct CPUArchState *’} but argument is of type ‘const CPUPPCState *’ {aka ‘const struct CPUArchState *’}
   2777 | uint64_t ppc_get_cr(CPUPPCState *env);
        |                     ~~~~~~~~~~~~~^~~
cc1: all warnings being treated as errors
[24/214] Compiling C object libqemu-ppc-softmmu.fa.p/target_ppc_power8-pmu.c.o


I squashed in this change to fix it:


diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
index 241d9e27e5..424f2e1741 100644
--- a/target/ppc/cpu.c
+++ b/target/ppc/cpu.c
@@ -75,7 +75,7 @@ void ppc_set_cr(CPUPPCState *env, uint64_t cr)
       }
   }
   
-uint64_t ppc_get_cr(CPUPPCState *env)
+uint64_t ppc_get_cr(const CPUPPCState *env)
   {
       uint64_t cr = 0;
       for (int i = 0; i < 8; i++) {
diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
index 0af94170d0..1c02596d9f 100644
--- a/target/ppc/cpu.h
+++ b/target/ppc/cpu.h
@@ -2774,7 +2774,7 @@ void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len);
   void ppc_store_vscr(CPUPPCState *env, uint32_t vscr);
   uint32_t ppc_get_vscr(CPUPPCState *env);
   void ppc_set_cr(CPUPPCState *env, uint64_t cr);
-uint64_t ppc_get_cr(CPUPPCState *env);
+uint64_t ppc_get_cr(const CPUPPCState *env);
   
   /*****************************************************************************/


Just a FYI,. No need to re-send.



Daniel



On 5/3/23 06:36, Harsh Prateek Bora wrote:
> The bits in cr reg are grouped into eight 4-bit fields represented
> by env->crf[8] and the related calculations should be abstracted to
> keep the calling routines simpler to read. This is a step towards
> cleaning up the related/calling code for better readability.
> 
> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> Reviewed-by: Fabiano Rosas <farosas@suse.de>
> ---
>   hw/ppc/spapr_hcall.c      | 18 ++----------------
>   linux-user/elfload.c      |  4 +---
>   linux-user/ppc/signal.c   |  9 ++-------
>   target/ppc/cpu.c          | 17 +++++++++++++++++
>   target/ppc/cpu.h          |  2 ++
>   target/ppc/gdbstub.c      | 22 ++++------------------
>   target/ppc/kvm.c          | 13 ++-----------
>   target/ppc/ppc-qmp-cmds.c |  6 +-----
>   8 files changed, 31 insertions(+), 60 deletions(-)
> 
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index ec4def62f8..1c102c8c0d 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1566,8 +1566,6 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
>       struct kvmppc_hv_guest_state hv_state;
>       struct kvmppc_pt_regs *regs;
>       hwaddr len;
> -    uint64_t cr;
> -    int i;
>   
>       if (spapr->nested_ptcr == 0) {
>           return H_NOT_AVAILABLE;
> @@ -1616,12 +1614,7 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
>       env->lr = regs->link;
>       env->ctr = regs->ctr;
>       cpu_write_xer(env, regs->xer);
> -
> -    cr = regs->ccr;
> -    for (i = 7; i >= 0; i--) {
> -        env->crf[i] = cr & 15;
> -        cr >>= 4;
> -    }
> +    ppc_set_cr(env, regs->ccr);
>   
>       env->msr = regs->msr;
>       env->nip = regs->nip;
> @@ -1698,8 +1691,6 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
>       struct kvmppc_hv_guest_state *hvstate;
>       struct kvmppc_pt_regs *regs;
>       hwaddr len;
> -    uint64_t cr;
> -    int i;
>   
>       assert(spapr_cpu->in_nested);
>   
> @@ -1757,12 +1748,7 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
>       regs->link = env->lr;
>       regs->ctr = env->ctr;
>       regs->xer = cpu_read_xer(env);
> -
> -    cr = 0;
> -    for (i = 0; i < 8; i++) {
> -        cr |= (env->crf[i] & 15) << (4 * (7 - i));
> -    }
> -    regs->ccr = cr;
> +    regs->ccr = ppc_get_cr(env);
>   
>       if (excp == POWERPC_EXCP_MCHECK ||
>           excp == POWERPC_EXCP_RESET ||
> diff --git a/linux-user/elfload.c b/linux-user/elfload.c
> index f1370a7a8b..703f7434a0 100644
> --- a/linux-user/elfload.c
> +++ b/linux-user/elfload.c
> @@ -961,9 +961,7 @@ static void elf_core_copy_regs(target_elf_gregset_t *regs, const CPUPPCState *en
>       (*regs)[36] = tswapreg(env->lr);
>       (*regs)[37] = tswapreg(cpu_read_xer(env));
>   
> -    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/ppc/signal.c b/linux-user/ppc/signal.c
> index 07729c1653..a616f20efb 100644
> --- a/linux-user/ppc/signal.c
> +++ b/linux-user/ppc/signal.c
> @@ -243,9 +243,7 @@ static void save_user_regs(CPUPPCState *env, struct target_mcontext *frame)
>       __put_user(env->lr, &frame->mc_gregs[TARGET_PT_LNK]);
>       __put_user(cpu_read_xer(env), &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.  */
> @@ -335,10 +333,7 @@ static void restore_user_regs(CPUPPCState *env,
>       cpu_write_xer(env, 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/target/ppc/cpu.c b/target/ppc/cpu.c
> index 1a97b41c6b..241d9e27e5 100644
> --- a/target/ppc/cpu.c
> +++ b/target/ppc/cpu.c
> @@ -67,6 +67,23 @@ uint32_t ppc_get_vscr(CPUPPCState *env)
>       return env->vscr | (sat << VSCR_SAT);
>   }
>   
> +void ppc_set_cr(CPUPPCState *env, uint64_t cr)
> +{
> +    for (int i = 7; i >= 0; i--) {
> +        env->crf[i] = cr & 0xf;
> +        cr >>= 4;
> +    }
> +}
> +
> +uint64_t ppc_get_cr(CPUPPCState *env)
> +{
> +    uint64_t cr = 0;
> +    for (int i = 0; i < 8; i++) {
> +        cr |= (env->crf[i] & 0xf) << (4 * (7 - i));
> +    }
> +    return cr;
> +}
> +
>   /* GDBstub can read and write MSR... */
>   void ppc_store_msr(CPUPPCState *env, target_ulong value)
>   {
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 557d736dab..0af94170d0 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2773,6 +2773,8 @@ void dump_mmu(CPUPPCState *env);
>   void ppc_maybe_bswap_register(CPUPPCState *env, uint8_t *mem_buf, int len);
>   void ppc_store_vscr(CPUPPCState *env, uint32_t vscr);
>   uint32_t ppc_get_vscr(CPUPPCState *env);
> +void ppc_set_cr(CPUPPCState *env, uint64_t cr);
> +uint64_t ppc_get_cr(CPUPPCState *env);
>   
>   /*****************************************************************************/
>   /* Power management enable checks                                            */
> diff --git a/target/ppc/gdbstub.c b/target/ppc/gdbstub.c
> index d2bc1d7c53..63c9abe4f1 100644
> --- a/target/ppc/gdbstub.c
> +++ b/target/ppc/gdbstub.c
> @@ -145,11 +145,7 @@ int ppc_cpu_gdb_read_register(CPUState *cs, GByteArray *buf, int n)
>               break;
>           case 66:
>               {
> -                uint32_t cr = 0;
> -                int i;
> -                for (i = 0; i < 8; i++) {
> -                    cr |= env->crf[i] << (32 - ((i + 1) * 4));
> -                }
> +                uint32_t cr = ppc_get_cr(env);
>                   gdb_get_reg32(buf, cr);
>                   break;
>               }
> @@ -203,11 +199,7 @@ int ppc_cpu_gdb_read_register_apple(CPUState *cs, GByteArray *buf, int n)
>               break;
>           case 66 + 32:
>               {
> -                uint32_t cr = 0;
> -                int i;
> -                for (i = 0; i < 8; i++) {
> -                    cr |= env->crf[i] << (32 - ((i + 1) * 4));
> -                }
> +                uint32_t cr = ppc_get_cr(env);
>                   gdb_get_reg32(buf, cr);
>                   break;
>               }
> @@ -257,10 +249,7 @@ int ppc_cpu_gdb_write_register(CPUState *cs, uint8_t *mem_buf, int n)
>           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;
> -                }
> +                ppc_set_cr(env, cr);
>                   break;
>               }
>           case 67:
> @@ -307,10 +296,7 @@ int ppc_cpu_gdb_write_register_apple(CPUState *cs, uint8_t *mem_buf, int n)
>           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;
> -                }
> +                ppc_set_cr(env, cr);
>                   break;
>               }
>           case 67 + 32:
> diff --git a/target/ppc/kvm.c b/target/ppc/kvm.c
> index 78f6fc50cd..336e663bc3 100644
> --- a/target/ppc/kvm.c
> +++ b/target/ppc/kvm.c
> @@ -927,10 +927,7 @@ int kvm_arch_put_registers(CPUState *cs, int level)
>           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) {
> @@ -1205,7 +1202,6 @@ int kvm_arch_get_registers(CPUState *cs)
>       PowerPCCPU *cpu = POWERPC_CPU(cs);
>       CPUPPCState *env = &cpu->env;
>       struct kvm_regs regs;
> -    uint32_t cr;
>       int i, ret;
>   
>       ret = kvm_vcpu_ioctl(cs, KVM_GET_REGS, &regs);
> @@ -1213,12 +1209,7 @@ int kvm_arch_get_registers(CPUState *cs)
>           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;
>       cpu_write_xer(env, regs.xer);
> diff --git a/target/ppc/ppc-qmp-cmds.c b/target/ppc/ppc-qmp-cmds.c
> index 36e5b5eff8..f9acc21056 100644
> --- a/target/ppc/ppc-qmp-cmds.c
> +++ b/target/ppc/ppc-qmp-cmds.c
> @@ -37,12 +37,8 @@ static target_long monitor_get_ccr(Monitor *mon, const struct MonitorDef *md,
>   {
>       CPUArchState *env = mon_get_cpu_env(mon);
>       unsigned int u;
> -    int i;
>   
> -    u = 0;
> -    for (i = 0; i < 8; i++) {
> -        u |= env->crf[i] << (32 - (4 * (i + 1)));
> -    }
> +    u = ppc_get_cr(env);
>   
>       return u;
>   }

Re: [PATCH v3 1/2] ppc: spapr: cleanup cr get/set with helpers.

[Harsh Prateek Bora]

On 5/4/23 02:19, Daniel Henrique Barboza wrote:
> I squashed in this change to fix it:
> 
> 
> diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
> index 241d9e27e5..424f2e1741 100644
> --- a/target/ppc/cpu.c
> +++ b/target/ppc/cpu.c
> @@ -75,7 +75,7 @@ void ppc_set_cr(CPUPPCState *env, uint64_t cr)
>        }
>    }
> -uint64_t ppc_get_cr(CPUPPCState *env)
> +uint64_t ppc_get_cr(const CPUPPCState *env)
>    {
>        uint64_t cr = 0;
>        for (int i = 0; i < 8; i++) {
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 0af94170d0..1c02596d9f 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -2774,7 +2774,7 @@ void ppc_maybe_bswap_register(CPUPPCState *env, 
> uint8_t *mem_buf, int len);
>    void ppc_store_vscr(CPUPPCState *env, uint32_t vscr);
>    uint32_t ppc_get_vscr(CPUPPCState *env);
>    void ppc_set_cr(CPUPPCState *env, uint64_t cr);
> -uint64_t ppc_get_cr(CPUPPCState *env);
> +uint64_t ppc_get_cr(const CPUPPCState *env);
>    
> /*****************************************************************************/
> 
> 
> Just a FYI,. No need to re-send.
> 
Oh ok, thanks Daniel.

> 
> 
> Daniel

[PATCH v3 2/2] MAINTAINERS: Adding myself in the list for ppc/spapr

[Harsh Prateek Bora]

Would like to get notified of changes in this area and review them.

Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 2c2068ea5c..b5d290cf92 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1420,6 +1420,7 @@ M: Daniel Henrique Barboza <danielhb413@gmail.com>
 R: Cédric Le Goater <clg@kaod.org>
 R: David Gibson <david@gibson.dropbear.id.au>
 R: Greg Kurz <groug@kaod.org>
+R: Harsh Prateek Bora <harshpb@linux.ibm.com>
 L: qemu-ppc@nongnu.org
 S: Odd Fixes
 F: hw/*/spapr*
-- 
2.31.1

Re: [PATCH v3 0/2] Cleanup ppc cr get/set with helper routines

[Daniel Henrique Barboza]

Queued in gitlab.com/danielhb/qemu/tree/ppc-next. Thanks,


Daniel

On 5/3/23 06:36, Harsh Prateek Bora wrote:
> This patchset introduces helper routines to get/set cr reg for better code
> readability / maintenance. No functional changes intended with this patchset.
> 
> Changelog:
> 
> changes from v2:
>   - addressed Nick's review comments
>   - holding changes from 2/4, 3/4 for future
> changes from v1:
>   - addressed Fabiano's review comments
>   - folding Reviewed-by tags in patch 1/4, 4/4
> 
> 
> *** BLURB HERE ***
> 
> Harsh Prateek Bora (2):
>    ppc: spapr: cleanup cr get/set with helpers.
>    MAINTAINERS: Adding myself in the list for ppc/spapr
> 
>   MAINTAINERS               |  1 +
>   hw/ppc/spapr_hcall.c      | 18 ++----------------
>   linux-user/elfload.c      |  4 +---
>   linux-user/ppc/signal.c   |  9 ++-------
>   target/ppc/cpu.c          | 17 +++++++++++++++++
>   target/ppc/cpu.h          |  2 ++
>   target/ppc/gdbstub.c      | 22 ++++------------------
>   target/ppc/kvm.c          | 13 ++-----------
>   target/ppc/ppc-qmp-cmds.c |  6 +-----
>   9 files changed, 32 insertions(+), 60 deletions(-)
>