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

["Nicholas Piggin" <npiggin@gmail.com>]

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 <harshpb@linux.ibm.com>
> Reviewed-by: Fabiano Rosas <farosas@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.

>  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.

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

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