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

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