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

[Fabiano Rosas <farosas@suse.de>]

"Nicholas Piggin" <npiggin@gmail.com> writes:

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

+1 for get/set

Best to save load/store for the code emulating the actual guest ld/st
instructions.