diff mbox series

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

Message ID 20230424144712.1985425-2-harshpb@linux.ibm.com
State New
Headers show
Series Cleanup [h_enter|spapr_exit]_nested routines | expand

Commit Message

Harsh Prateek Bora April 24, 2023, 2:47 p.m. UTC 
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 ++----------------
 target/ppc/cpu.c     | 17 +++++++++++++++++
 target/ppc/cpu.h     |  2 ++
 3 files changed, 21 insertions(+), 16 deletions(-)

Comments

Nicholas Piggin May 2, 2023, 4:37 a.m. UTC | #1 
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
Harsh Prateek Bora May 2, 2023, 5 a.m. UTC | #2 
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
>
Nicholas Piggin May 2, 2023, 2:39 p.m. UTC | #3 
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
Fabiano Rosas May 2, 2023, 2:46 p.m. UTC | #4 
"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.
diff mbox series

Patch

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
@@ -1566,8 +1566,6 @@  static target_ulong h_enter_nested(PowerPCCPU *cpu,
     struct kvmppc_hv_guest_state hv_state;
     struct kvmppc_pt_regs *regs;
     hwaddr len;
-    uint64_t cr;
-    int i;
 
     if (spapr->nested_ptcr == 0) {
         return H_NOT_AVAILABLE;
@@ -1616,12 +1614,7 @@  static target_ulong h_enter_nested(PowerPCCPU *cpu,
     env->lr = regs->link;
     env->ctr = regs->ctr;
     cpu_write_xer(env, regs->xer);
-
-    cr = regs->ccr;
-    for (i = 7; i >= 0; i--) {
-        env->crf[i] = cr & 15;
-        cr >>= 4;
-    }
+    ppc_store_cr(env, regs->ccr);
 
     env->msr = regs->msr;
     env->nip = regs->nip;
@@ -1698,8 +1691,6 @@  void spapr_exit_nested(PowerPCCPU *cpu, int excp)
     struct kvmppc_hv_guest_state *hvstate;
     struct kvmppc_pt_regs *regs;
     hwaddr len;
-    uint64_t cr;
-    int i;
 
     assert(spapr_cpu->in_nested);
 
@@ -1757,12 +1748,7 @@  void spapr_exit_nested(PowerPCCPU *cpu, int excp)
     regs->link = env->lr;
     regs->ctr = env->ctr;
     regs->xer = cpu_read_xer(env);
-
-    cr = 0;
-    for (i = 0; i < 8; i++) {
-        cr |= (env->crf[i] & 15) << (4 * (7 - i));
-    }
-    regs->ccr = cr;
+    regs->ccr = ppc_get_cr(env);
 
     if (excp == POWERPC_EXCP_MCHECK ||
         excp == POWERPC_EXCP_RESET ||
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)
+{
+    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                                            */