Re: [PATCH v2 1/4] ppc: spapr: cleanup cr get/store in [h_enter|spapr_exit]_nested with helpers.

[Harsh Prateek Bora]

On 5/2/23 10:07, Nicholas Piggin wrote:
On Tue Apr 25, 2023 at 12:47 AM AEST, 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 [h_enter|spapr_exit]_nested calls for better readability.

Signed-off-by: Harsh Prateek Bora <hars...@linux.ibm.com>
Reviewed-by: Fabiano Rosas <faro...@suse.de>
---
  hw/ppc/spapr_hcall.c | 18 ++----------------

Could you either convert all callers, or do implementation and
conversion as separate patches. Preference for former if you can
be bothered.

save_user_regs(), restore_user_regs(), gdb read/write register * 2,
kvm_arch_get/put_registers, monitor_get_ccr, at a quick glance.

Sure, I can include other consumers as well in the patches.
I usually prefer separate patches for implementation/conversion but 
since the implementation is a small change, I hope either approach is fine.


  target/ppc/cpu.c     | 17 +++++++++++++++++
  target/ppc/cpu.h     |  2 ++
  3 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index ec4def62f8..124cee5e53 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c

[snip]

diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
index 1a97b41c6b..3b444e58b5 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_store_cr(CPUPPCState *env, uint64_t cr)

Set is normal counterpart to get. Or load and store, but
I think set and get is probably better.

Sure, make sense.

Good refactoring though, it shouldn't be open-coded everywhere.

Thanks,
Harsh

Thanks,
Nick

+{
+    for (int i = 7; i >= 0; i--) {
+        env->crf[i] = cr & 15;
+        cr >>= 4;
+    }
+}
+
+uint64_t ppc_get_cr(CPUPPCState *env)
+{
+    uint64_t cr = 0;
+    for (int i = 0; i < 8; i++) {
+        cr |= (env->crf[i] & 15) << (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..b4c21459f1 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_store_cr(CPUPPCState *env, uint64_t cr);
+uint64_t ppc_get_cr(CPUPPCState *env);
  
  /*****************************************************************************/
  /* Power management enable checks                                            
*/
--
2.31.1