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

[Nicholas Piggin]

On Tue May 2, 2023 at 3:00 PM AEST, Harsh Prateek Bora wrote:
>
>
> 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.

Yeah one patch would be 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.

I did say that before realising the other functions there use as
much varied and inconsistent terminology as possible, sigh.

I *think* ppc_get|set_reg() is the best naming. store is used a lot but
it means something else too, so set is better. But if you have strong
feelings another way I don't mind.

Thanks,
Nick