[PATCH v2 0/4] Cleanup [h_enter|spapr_exit]_nested routines

[PATCH v2 0/4] Cleanup [h_enter|spapr_exit]_nested routines

[Harsh Prateek Bora]

This patchset introduces helper routines to enable (and does) cleaning
up of h_enter_nested() and spapr_exit_nested() routines in existing api
for nested virtualization on Power/SPAPR for better code readability /
maintenance. No functional changes intended with this patchset.

Changelog:

changes from v1:
 - addressed Fabiano's review comments
 - folding Reviewed-by tags in patch 1/4, 4/4

Harsh Prateek Bora (4):
  ppc: spapr: cleanup cr get/store in [h_enter|spapr_exit]_nested with
    helpers.
  ppc: spapr: cleanup h_enter_nested() with helper routines.
  ppc: spapr: cleanup spapr_exit_nested() with helper routines.
  MAINTAINERS: Adding myself in the list for ppc/spapr

 MAINTAINERS          |   1 +
 hw/ppc/spapr_hcall.c | 251 ++++++++++++++++++++++++++-----------------
 target/ppc/cpu.c     |  17 +++
 target/ppc/cpu.h     |   2 +
 4 files changed, 170 insertions(+), 101 deletions(-)

-- 
2.31.1

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

[Harsh Prateek Bora]

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(-)

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                                            */
-- 
2.31.1

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

[Nicholas Piggin]

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

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
> 

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

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

[Fabiano Rosas]

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

[PATCH v2 2/4] ppc: spapr: cleanup h_enter_nested() with helper routines.

[Harsh Prateek Bora]

h_enter_nested() currently does a lot of register specific operations
which should be abstracted logically to simplify the code for better
readability. This patch breaks down relevant blocks into respective
helper routines to make use of them for better readability/maintenance.

Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
---
 hw/ppc/spapr_hcall.c | 117 ++++++++++++++++++++++++++++---------------
 1 file changed, 78 insertions(+), 39 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index 124cee5e53..f24d4b368e 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1544,6 +1544,81 @@ static target_ulong h_copy_tofrom_guest(PowerPCCPU *cpu,
     return H_FUNCTION;
 }
 
+static void restore_hdec_from_hvstate(CPUPPCState *dst,
+                                      struct kvmppc_hv_guest_state *hv_state,
+                                      target_ulong now)
+{
+    target_ulong hdec;
+
+    assert(hv_state);
+    hdec = hv_state->hdec_expiry - now;
+    cpu_ppc_hdecr_init(dst);
+    cpu_ppc_store_hdecr(dst, hdec);
+}
+
+static void restore_lpcr_from_hvstate(PowerPCCPU *cpu,
+                                      struct kvmppc_hv_guest_state *hv_state)
+{
+    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
+    CPUPPCState *dst = &cpu->env;
+    target_ulong lpcr, lpcr_mask;
+
+    assert(hv_state);
+    lpcr_mask = LPCR_DPFD | LPCR_ILE | LPCR_AIL | LPCR_LD | LPCR_MER;
+    lpcr = (dst->spr[SPR_LPCR] & ~lpcr_mask) | (hv_state->lpcr & lpcr_mask);
+    lpcr |= LPCR_HR | LPCR_UPRT | LPCR_GTSE | LPCR_HVICE | LPCR_HDICE;
+    lpcr &= ~LPCR_LPES0;
+    dst->spr[SPR_LPCR] = lpcr & pcc->lpcr_mask;
+}
+
+static void restore_env_from_ptregs(CPUPPCState *env,
+                                    struct kvmppc_pt_regs *regs)
+{
+    assert(env);
+    assert(regs);
+    assert(sizeof(env->gpr) == sizeof(regs->gpr));
+    memcpy(env->gpr, regs->gpr, sizeof(env->gpr));
+    env->nip = regs->nip;
+    env->msr = regs->msr;
+    env->lr = regs->link;
+    env->ctr = regs->ctr;
+    cpu_write_xer(env, regs->xer);
+    ppc_store_cr(env, regs->ccr);
+}
+
+static void restore_env_from_hvstate(CPUPPCState *env,
+                                     struct kvmppc_hv_guest_state *hv_state)
+{
+    assert(env);
+    assert(hv_state);
+    env->spr[SPR_HFSCR] = hv_state->hfscr;
+    /* TCG does not implement DAWR*, CIABR, PURR, SPURR, IC, VTB, HEIR SPRs*/
+    env->cfar = hv_state->cfar;
+    env->spr[SPR_PCR] = hv_state->pcr;
+    env->spr[SPR_DPDES] = hv_state->dpdes;
+    env->spr[SPR_SRR0] = hv_state->srr0;
+    env->spr[SPR_SRR1] = hv_state->srr1;
+    env->spr[SPR_SPRG0] = hv_state->sprg[0];
+    env->spr[SPR_SPRG1] = hv_state->sprg[1];
+    env->spr[SPR_SPRG2] = hv_state->sprg[2];
+    env->spr[SPR_SPRG3] = hv_state->sprg[3];
+    env->spr[SPR_BOOKS_PID] = hv_state->pidr;
+    env->spr[SPR_PPR] = hv_state->ppr;
+}
+
+static inline void restore_l2_env(PowerPCCPU *cpu,
+		                  struct kvmppc_hv_guest_state *hv_state,
+				  struct kvmppc_pt_regs *regs,
+				  target_ulong now)
+{
+    CPUPPCState *env = &cpu->env;
+
+    restore_env_from_ptregs(env, regs);
+    restore_env_from_hvstate(env, hv_state);
+    restore_lpcr_from_hvstate(cpu, hv_state);
+    restore_hdec_from_hvstate(env, hv_state, now);
+}
+
 /*
  * When this handler returns, the environment is switched to the L2 guest
  * and TCG begins running that. spapr_exit_nested() performs the switch from
@@ -1554,14 +1629,12 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
                                    target_ulong opcode,
                                    target_ulong *args)
 {
-    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     CPUState *cs = CPU(cpu);
     CPUPPCState *env = &cpu->env;
     SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
     target_ulong hv_ptr = args[0];
     target_ulong regs_ptr = args[1];
-    target_ulong hdec, now = cpu_ppc_load_tbl(env);
-    target_ulong lpcr, lpcr_mask;
+    target_ulong now = cpu_ppc_load_tbl(env);
     struct kvmppc_hv_guest_state *hvstate;
     struct kvmppc_hv_guest_state hv_state;
     struct kvmppc_pt_regs *regs;
@@ -1607,49 +1680,15 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
         return H_P2;
     }
 
-    len = sizeof(env->gpr);
-    assert(len == sizeof(regs->gpr));
-    memcpy(env->gpr, regs->gpr, len);
-
-    env->lr = regs->link;
-    env->ctr = regs->ctr;
-    cpu_write_xer(env, regs->xer);
-    ppc_store_cr(env, regs->ccr);
-
-    env->msr = regs->msr;
-    env->nip = regs->nip;
+    /* restore L2 env from hv_state and ptregs */
+    restore_l2_env(cpu, &hv_state, regs, now);
 
     address_space_unmap(CPU(cpu)->as, regs, len, len, false);
 
-    env->cfar = hv_state.cfar;
-
     assert(env->spr[SPR_LPIDR] == 0);
     env->spr[SPR_LPIDR] = hv_state.lpid;
 
-    lpcr_mask = LPCR_DPFD | LPCR_ILE | LPCR_AIL | LPCR_LD | LPCR_MER;
-    lpcr = (env->spr[SPR_LPCR] & ~lpcr_mask) | (hv_state.lpcr & lpcr_mask);
-    lpcr |= LPCR_HR | LPCR_UPRT | LPCR_GTSE | LPCR_HVICE | LPCR_HDICE;
-    lpcr &= ~LPCR_LPES0;
-    env->spr[SPR_LPCR] = lpcr & pcc->lpcr_mask;
-
-    env->spr[SPR_PCR] = hv_state.pcr;
-    /* hv_state.amor is not used */
-    env->spr[SPR_DPDES] = hv_state.dpdes;
-    env->spr[SPR_HFSCR] = hv_state.hfscr;
-    hdec = hv_state.hdec_expiry - now;
     spapr_cpu->nested_tb_offset = hv_state.tb_offset;
-    /* TCG does not implement DAWR*, CIABR, PURR, SPURR, IC, VTB, HEIR SPRs*/
-    env->spr[SPR_SRR0] = hv_state.srr0;
-    env->spr[SPR_SRR1] = hv_state.srr1;
-    env->spr[SPR_SPRG0] = hv_state.sprg[0];
-    env->spr[SPR_SPRG1] = hv_state.sprg[1];
-    env->spr[SPR_SPRG2] = hv_state.sprg[2];
-    env->spr[SPR_SPRG3] = hv_state.sprg[3];
-    env->spr[SPR_BOOKS_PID] = hv_state.pidr;
-    env->spr[SPR_PPR] = hv_state.ppr;
-
-    cpu_ppc_hdecr_init(env);
-    cpu_ppc_store_hdecr(env, hdec);
 
     /*
      * The hv_state.vcpu_token is not needed. It is used by the KVM
-- 
2.31.1

Re: [PATCH v2 2/4] ppc: spapr: cleanup h_enter_nested() with helper routines.

[Nicholas Piggin]

On Tue Apr 25, 2023 at 12:47 AM AEST, Harsh Prateek Bora wrote:
> h_enter_nested() currently does a lot of register specific operations
> which should be abstracted logically to simplify the code for better
> readability. This patch breaks down relevant blocks into respective
> helper routines to make use of them for better readability/maintenance.
>
> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> ---
>  hw/ppc/spapr_hcall.c | 117 ++++++++++++++++++++++++++++---------------
>  1 file changed, 78 insertions(+), 39 deletions(-)
>
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index 124cee5e53..f24d4b368e 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1544,6 +1544,81 @@ static target_ulong h_copy_tofrom_guest(PowerPCCPU *cpu,
>      return H_FUNCTION;
>  }
>  
> +static void restore_hdec_from_hvstate(CPUPPCState *dst,
> +                                      struct kvmppc_hv_guest_state *hv_state,
> +                                      target_ulong now)
> +{
> +    target_ulong hdec;
> +
> +    assert(hv_state);
> +    hdec = hv_state->hdec_expiry - now;
> +    cpu_ppc_hdecr_init(dst);
> +    cpu_ppc_store_hdecr(dst, hdec);
> +}
> +
> +static void restore_lpcr_from_hvstate(PowerPCCPU *cpu,
> +                                      struct kvmppc_hv_guest_state *hv_state)
> +{
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +    CPUPPCState *dst = &cpu->env;
> +    target_ulong lpcr, lpcr_mask;
> +
> +    assert(hv_state);
> +    lpcr_mask = LPCR_DPFD | LPCR_ILE | LPCR_AIL | LPCR_LD | LPCR_MER;
> +    lpcr = (dst->spr[SPR_LPCR] & ~lpcr_mask) | (hv_state->lpcr & lpcr_mask);
> +    lpcr |= LPCR_HR | LPCR_UPRT | LPCR_GTSE | LPCR_HVICE | LPCR_HDICE;
> +    lpcr &= ~LPCR_LPES0;
> +    dst->spr[SPR_LPCR] = lpcr & pcc->lpcr_mask;
> +}
> +
> +static void restore_env_from_ptregs(CPUPPCState *env,
> +                                    struct kvmppc_pt_regs *regs)
> +{
> +    assert(env);
> +    assert(regs);
> +    assert(sizeof(env->gpr) == sizeof(regs->gpr));
> +    memcpy(env->gpr, regs->gpr, sizeof(env->gpr));
> +    env->nip = regs->nip;
> +    env->msr = regs->msr;
> +    env->lr = regs->link;
> +    env->ctr = regs->ctr;
> +    cpu_write_xer(env, regs->xer);
> +    ppc_store_cr(env, regs->ccr);
> +}
> +
> +static void restore_env_from_hvstate(CPUPPCState *env,
> +                                     struct kvmppc_hv_guest_state *hv_state)
> +{
> +    assert(env);
> +    assert(hv_state);
> +    env->spr[SPR_HFSCR] = hv_state->hfscr;
> +    /* TCG does not implement DAWR*, CIABR, PURR, SPURR, IC, VTB, HEIR SPRs*/
> +    env->cfar = hv_state->cfar;
> +    env->spr[SPR_PCR] = hv_state->pcr;
> +    env->spr[SPR_DPDES] = hv_state->dpdes;
> +    env->spr[SPR_SRR0] = hv_state->srr0;
> +    env->spr[SPR_SRR1] = hv_state->srr1;
> +    env->spr[SPR_SPRG0] = hv_state->sprg[0];
> +    env->spr[SPR_SPRG1] = hv_state->sprg[1];
> +    env->spr[SPR_SPRG2] = hv_state->sprg[2];
> +    env->spr[SPR_SPRG3] = hv_state->sprg[3];
> +    env->spr[SPR_BOOKS_PID] = hv_state->pidr;
> +    env->spr[SPR_PPR] = hv_state->ppr;
> +}
> +
> +static inline void restore_l2_env(PowerPCCPU *cpu,
> +		                  struct kvmppc_hv_guest_state *hv_state,
> +				  struct kvmppc_pt_regs *regs,
> +				  target_ulong now)
> +{
> +    CPUPPCState *env = &cpu->env;
> +
> +    restore_env_from_ptregs(env, regs);
> +    restore_env_from_hvstate(env, hv_state);
> +    restore_lpcr_from_hvstate(cpu, hv_state);
> +    restore_hdec_from_hvstate(env, hv_state, now);
> +}
> +
>  /*
>   * When this handler returns, the environment is switched to the L2 guest
>   * and TCG begins running that. spapr_exit_nested() performs the switch from
> @@ -1554,14 +1629,12 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
>                                     target_ulong opcode,
>                                     target_ulong *args)
>  {
> -    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>      CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
>      SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>      target_ulong hv_ptr = args[0];
>      target_ulong regs_ptr = args[1];
> -    target_ulong hdec, now = cpu_ppc_load_tbl(env);
> -    target_ulong lpcr, lpcr_mask;
> +    target_ulong now = cpu_ppc_load_tbl(env);
>      struct kvmppc_hv_guest_state *hvstate;
>      struct kvmppc_hv_guest_state hv_state;
>      struct kvmppc_pt_regs *regs;
> @@ -1607,49 +1680,15 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
>          return H_P2;
>      }
>  
> -    len = sizeof(env->gpr);
> -    assert(len == sizeof(regs->gpr));
> -    memcpy(env->gpr, regs->gpr, len);
> -
> -    env->lr = regs->link;
> -    env->ctr = regs->ctr;
> -    cpu_write_xer(env, regs->xer);
> -    ppc_store_cr(env, regs->ccr);
> -
> -    env->msr = regs->msr;
> -    env->nip = regs->nip;
> +    /* restore L2 env from hv_state and ptregs */
> +    restore_l2_env(cpu, &hv_state, regs, now);
>  
>      address_space_unmap(CPU(cpu)->as, regs, len, len, false);

I don't agree this improves readability. It also does more with the
guest address space mapped, which may not be a big deal is strictly
not an improvement.

The comment needn't just repeat what the function says, and it does
not actually restore the l2 environment. It sets some registers to
L2 values, but it also leaves other state.

I would like to see this in a larger series if it's going somewhere,
but at the moment I'd rather leave it as is.

Thanks,
Nick

Re: [PATCH v2 2/4] ppc: spapr: cleanup h_enter_nested() with helper routines.

[Harsh Prateek Bora]

On 5/2/23 10:19, Nicholas Piggin wrote:
> On Tue Apr 25, 2023 at 12:47 AM AEST, Harsh Prateek Bora wrote:
>> h_enter_nested() currently does a lot of register specific operations
>> which should be abstracted logically to simplify the code for better
>> readability. This patch breaks down relevant blocks into respective
>> helper routines to make use of them for better readability/maintenance.
>>
>> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>> ---
>>   hw/ppc/spapr_hcall.c | 117 ++++++++++++++++++++++++++++---------------
>>   1 file changed, 78 insertions(+), 39 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 124cee5e53..f24d4b368e 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1544,6 +1544,81 @@ static target_ulong h_copy_tofrom_guest(PowerPCCPU *cpu,
>>       return H_FUNCTION;
>>   }
>>   
>> +static void restore_hdec_from_hvstate(CPUPPCState *dst,
>> +                                      struct kvmppc_hv_guest_state *hv_state,
>> +                                      target_ulong now)
>> +{
>> +    target_ulong hdec;
>> +
>> +    assert(hv_state);
>> +    hdec = hv_state->hdec_expiry - now;
>> +    cpu_ppc_hdecr_init(dst);
>> +    cpu_ppc_store_hdecr(dst, hdec);
>> +}
>> +
>> +static void restore_lpcr_from_hvstate(PowerPCCPU *cpu,
>> +                                      struct kvmppc_hv_guest_state *hv_state)
>> +{
>> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>> +    CPUPPCState *dst = &cpu->env;
>> +    target_ulong lpcr, lpcr_mask;
>> +
>> +    assert(hv_state);
>> +    lpcr_mask = LPCR_DPFD | LPCR_ILE | LPCR_AIL | LPCR_LD | LPCR_MER;
>> +    lpcr = (dst->spr[SPR_LPCR] & ~lpcr_mask) | (hv_state->lpcr & lpcr_mask);
>> +    lpcr |= LPCR_HR | LPCR_UPRT | LPCR_GTSE | LPCR_HVICE | LPCR_HDICE;
>> +    lpcr &= ~LPCR_LPES0;
>> +    dst->spr[SPR_LPCR] = lpcr & pcc->lpcr_mask;
>> +}
>> +
>> +static void restore_env_from_ptregs(CPUPPCState *env,
>> +                                    struct kvmppc_pt_regs *regs)
>> +{
>> +    assert(env);
>> +    assert(regs);
>> +    assert(sizeof(env->gpr) == sizeof(regs->gpr));
>> +    memcpy(env->gpr, regs->gpr, sizeof(env->gpr));
>> +    env->nip = regs->nip;
>> +    env->msr = regs->msr;
>> +    env->lr = regs->link;
>> +    env->ctr = regs->ctr;
>> +    cpu_write_xer(env, regs->xer);
>> +    ppc_store_cr(env, regs->ccr);
>> +}
>> +
>> +static void restore_env_from_hvstate(CPUPPCState *env,
>> +                                     struct kvmppc_hv_guest_state *hv_state)
>> +{
>> +    assert(env);
>> +    assert(hv_state);
>> +    env->spr[SPR_HFSCR] = hv_state->hfscr;
>> +    /* TCG does not implement DAWR*, CIABR, PURR, SPURR, IC, VTB, HEIR SPRs*/
>> +    env->cfar = hv_state->cfar;
>> +    env->spr[SPR_PCR] = hv_state->pcr;
>> +    env->spr[SPR_DPDES] = hv_state->dpdes;
>> +    env->spr[SPR_SRR0] = hv_state->srr0;
>> +    env->spr[SPR_SRR1] = hv_state->srr1;
>> +    env->spr[SPR_SPRG0] = hv_state->sprg[0];
>> +    env->spr[SPR_SPRG1] = hv_state->sprg[1];
>> +    env->spr[SPR_SPRG2] = hv_state->sprg[2];
>> +    env->spr[SPR_SPRG3] = hv_state->sprg[3];
>> +    env->spr[SPR_BOOKS_PID] = hv_state->pidr;
>> +    env->spr[SPR_PPR] = hv_state->ppr;
>> +}
>> +
>> +static inline void restore_l2_env(PowerPCCPU *cpu,
>> +		                  struct kvmppc_hv_guest_state *hv_state,
>> +				  struct kvmppc_pt_regs *regs,
>> +				  target_ulong now)
>> +{
>> +    CPUPPCState *env = &cpu->env;
>> +
>> +    restore_env_from_ptregs(env, regs);
>> +    restore_env_from_hvstate(env, hv_state);
>> +    restore_lpcr_from_hvstate(cpu, hv_state);
>> +    restore_hdec_from_hvstate(env, hv_state, now);
>> +}
>> +
>>   /*
>>    * When this handler returns, the environment is switched to the L2 guest
>>    * and TCG begins running that. spapr_exit_nested() performs the switch from
>> @@ -1554,14 +1629,12 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
>>                                      target_ulong opcode,
>>                                      target_ulong *args)
>>   {
>> -    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>>       CPUState *cs = CPU(cpu);
>>       CPUPPCState *env = &cpu->env;
>>       SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>>       target_ulong hv_ptr = args[0];
>>       target_ulong regs_ptr = args[1];
>> -    target_ulong hdec, now = cpu_ppc_load_tbl(env);
>> -    target_ulong lpcr, lpcr_mask;
>> +    target_ulong now = cpu_ppc_load_tbl(env);
>>       struct kvmppc_hv_guest_state *hvstate;
>>       struct kvmppc_hv_guest_state hv_state;
>>       struct kvmppc_pt_regs *regs;
>> @@ -1607,49 +1680,15 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
>>           return H_P2;
>>       }
>>   
>> -    len = sizeof(env->gpr);
>> -    assert(len == sizeof(regs->gpr));
>> -    memcpy(env->gpr, regs->gpr, len);
>> -
>> -    env->lr = regs->link;
>> -    env->ctr = regs->ctr;
>> -    cpu_write_xer(env, regs->xer);
>> -    ppc_store_cr(env, regs->ccr);
>> -
>> -    env->msr = regs->msr;
>> -    env->nip = regs->nip;
>> +    /* restore L2 env from hv_state and ptregs */
>> +    restore_l2_env(cpu, &hv_state, regs, now);
>>   
>>       address_space_unmap(CPU(cpu)->as, regs, len, len, false);
> 
> I don't agree this improves readability. It also does more with the
> guest address space mapped, which may not be a big deal is strictly
> not an improvement.
> 
> The comment needn't just repeat what the function says, and it does
> not actually restore the l2 environment. It sets some registers to
> L2 values, but it also leaves other state.
> 
> I would like to see this in a larger series if it's going somewhere,
> but at the moment I'd rather leave it as is.
> 
While I agree the routine could be named restore_l2_hvstate_ptregs() as 
more appropriate, I think it still makes sense to have the body of 
enter/exit routines with as minimum LOC as possible, with the help of 
minimum helper routines possible. Giving semantics to the set of 
operations related to ptregs/hvstate register load/store is the first 
step towards it.

As you have guessed, this is certainly a precursor to another API 
version that we have been working on (still a WIP), and helps isolating 
the code flows for backward compatibiility. Having such changes early 
upstream helps stablising changes which are not a really a API/design 
change.

regards,
Harsh

> Thanks,
> Nick

Re: [PATCH v2 2/4] ppc: spapr: cleanup h_enter_nested() with helper routines.

[Nicholas Piggin]

On Tue May 2, 2023 at 4:13 PM AEST, Harsh Prateek Bora wrote:
> On 5/2/23 10:19, Nicholas Piggin wrote:
> > On Tue Apr 25, 2023 at 12:47 AM AEST, Harsh Prateek Bora wrote:
> >> @@ -1607,49 +1680,15 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
> >>           return H_P2;
> >>       }
> >>   
> >> -    len = sizeof(env->gpr);
> >> -    assert(len == sizeof(regs->gpr));
> >> -    memcpy(env->gpr, regs->gpr, len);
> >> -
> >> -    env->lr = regs->link;
> >> -    env->ctr = regs->ctr;
> >> -    cpu_write_xer(env, regs->xer);
> >> -    ppc_store_cr(env, regs->ccr);
> >> -
> >> -    env->msr = regs->msr;
> >> -    env->nip = regs->nip;
> >> +    /* restore L2 env from hv_state and ptregs */
> >> +    restore_l2_env(cpu, &hv_state, regs, now);
> >>   
> >>       address_space_unmap(CPU(cpu)->as, regs, len, len, false);
> > 
> > I don't agree this improves readability. It also does more with the
> > guest address space mapped, which may not be a big deal is strictly
> > not an improvement.
> > 
> > The comment needn't just repeat what the function says, and it does
> > not actually restore the l2 environment. It sets some registers to
> > L2 values, but it also leaves other state.
> > 
> > I would like to see this in a larger series if it's going somewhere,
> > but at the moment I'd rather leave it as is.
> > 
> While I agree the routine could be named restore_l2_hvstate_ptregs() as 
> more appropriate, I think it still makes sense to have the body of 
> enter/exit routines with as minimum LOC as possible, with the help of 
> minimum helper routines possible.

I don't think that's a good goal. The entirity of entering and exiting
from a nested guest is 279 lines including comments and no more than
one level of control flow. It's tricky code and has worts, but not
because the number of lines.

> Giving semantics to the set of 
> operations related to ptregs/hvstate register load/store is the first 
> step towards it.

Those structures are entirely the domain of the hcall API though, so
if anything belongs in the handler functions it is the handling of
those IMO.

> As you have guessed, this is certainly a precursor to another API 
> version that we have been working on (still a WIP), and helps isolating 
> the code flows for backward compatibiility. Having such changes early 
> upstream helps stablising changes which are not a really a API/design 
> change.

Right. Some more abstracting could certainly make sense here, I just
think at this point we need to see the bigger picture.

Thanks,
Nick

Re: [PATCH v2 2/4] ppc: spapr: cleanup h_enter_nested() with helper routines.

[Harsh Prateek Bora]

On 5/2/23 12:11, Nicholas Piggin wrote:
> On Tue May 2, 2023 at 4:13 PM AEST, Harsh Prateek Bora wrote:
>> On 5/2/23 10:19, Nicholas Piggin wrote:
>>> On Tue Apr 25, 2023 at 12:47 AM AEST, Harsh Prateek Bora wrote:
>>>> @@ -1607,49 +1680,15 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
>>>>            return H_P2;
>>>>        }
>>>>    
>>>> -    len = sizeof(env->gpr);
>>>> -    assert(len == sizeof(regs->gpr));
>>>> -    memcpy(env->gpr, regs->gpr, len);
>>>> -
>>>> -    env->lr = regs->link;
>>>> -    env->ctr = regs->ctr;
>>>> -    cpu_write_xer(env, regs->xer);
>>>> -    ppc_store_cr(env, regs->ccr);
>>>> -
>>>> -    env->msr = regs->msr;
>>>> -    env->nip = regs->nip;
>>>> +    /* restore L2 env from hv_state and ptregs */
>>>> +    restore_l2_env(cpu, &hv_state, regs, now);
>>>>    
>>>>        address_space_unmap(CPU(cpu)->as, regs, len, len, false);
>>>
>>> I don't agree this improves readability. It also does more with the
>>> guest address space mapped, which may not be a big deal is strictly
>>> not an improvement.
>>>
>>> The comment needn't just repeat what the function says, and it does
>>> not actually restore the l2 environment. It sets some registers to
>>> L2 values, but it also leaves other state.
>>>
>>> I would like to see this in a larger series if it's going somewhere,
>>> but at the moment I'd rather leave it as is.
>>>
>> While I agree the routine could be named restore_l2_hvstate_ptregs() as
>> more appropriate, I think it still makes sense to have the body of
>> enter/exit routines with as minimum LOC as possible, with the help of
>> minimum helper routines possible.
> 
> I don't think that's a good goal. The entirity of entering and exiting
> from a nested guest is 279 lines including comments and no more than
> one level of control flow. It's tricky code and has worts, but not
> because the number of lines.
> 
Yes, It's a tricky code, and this patch was an attempt to simplify the 
tricky-ness by giving names to set of related ops with helper routines.

>> Giving semantics to the set of
>> operations related to ptregs/hvstate register load/store is the first
>> step towards it.
> 
> Those structures are entirely the domain of the hcall API though, so
> if anything belongs in the handler functions it is the handling of
> those IMO.
> 
Absolutely, ideally we would want to contain everything inside the 
handler, but if a logical name could be given to a set of related ops 
(ptregs/hvstate specific), that certainly helps the reader to look into 
bigger picture at first and then get into specific details as needed.

>> As you have guessed, this is certainly a precursor to another API
>> version that we have been working on (still a WIP), and helps isolating
>> the code flows for backward compatibiility. Having such changes early
>> upstream helps stablising changes which are not a really a API/design
>> change.
> 
> Right. Some more abstracting could certainly make sense here, I just
> think at this point we need to see the bigger picture.

I think I am fine holding the cleanup for enter/exit nested for now 
until we bring the next set of API changes upstream, as that will 
provide a better context to the value these changes would bring along.

Meanwhile, I shall address your comments on 1/4 and post a v3.
Thanks for all your review inputs.

regards,
Harsh
> 
> Thanks,
> Nick
> 

Re: [PATCH v2 2/4] ppc: spapr: cleanup h_enter_nested() with helper routines.

[Nicholas Piggin]

On Tue May 2, 2023 at 5:36 PM AEST, Harsh Prateek Bora wrote:
>
>
> On 5/2/23 12:11, Nicholas Piggin wrote:
> > On Tue May 2, 2023 at 4:13 PM AEST, Harsh Prateek Bora wrote:
> >> On 5/2/23 10:19, Nicholas Piggin wrote:
> >>> On Tue Apr 25, 2023 at 12:47 AM AEST, Harsh Prateek Bora wrote:
> >>>> @@ -1607,49 +1680,15 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
> >>>>            return H_P2;
> >>>>        }
> >>>>    
> >>>> -    len = sizeof(env->gpr);
> >>>> -    assert(len == sizeof(regs->gpr));
> >>>> -    memcpy(env->gpr, regs->gpr, len);
> >>>> -
> >>>> -    env->lr = regs->link;
> >>>> -    env->ctr = regs->ctr;
> >>>> -    cpu_write_xer(env, regs->xer);
> >>>> -    ppc_store_cr(env, regs->ccr);
> >>>> -
> >>>> -    env->msr = regs->msr;
> >>>> -    env->nip = regs->nip;
> >>>> +    /* restore L2 env from hv_state and ptregs */
> >>>> +    restore_l2_env(cpu, &hv_state, regs, now);
> >>>>    
> >>>>        address_space_unmap(CPU(cpu)->as, regs, len, len, false);
> >>>
> >>> I don't agree this improves readability. It also does more with the
> >>> guest address space mapped, which may not be a big deal is strictly
> >>> not an improvement.
> >>>
> >>> The comment needn't just repeat what the function says, and it does
> >>> not actually restore the l2 environment. It sets some registers to
> >>> L2 values, but it also leaves other state.
> >>>
> >>> I would like to see this in a larger series if it's going somewhere,
> >>> but at the moment I'd rather leave it as is.
> >>>
> >> While I agree the routine could be named restore_l2_hvstate_ptregs() as
> >> more appropriate, I think it still makes sense to have the body of
> >> enter/exit routines with as minimum LOC as possible, with the help of
> >> minimum helper routines possible.
> > 
> > I don't think that's a good goal. The entirity of entering and exiting
> > from a nested guest is 279 lines including comments and no more than
> > one level of control flow. It's tricky code and has worts, but not
> > because the number of lines.
> > 
> Yes, It's a tricky code, and this patch was an attempt to simplify the 
> tricky-ness by giving names to set of related ops with helper routines.

The H_ENTER_NESTED hcall says "here are a bunch of registers, set the
environment to that and switch to the L2 guest.

So having a long list of registers may be a bit tedious but it's at the
same level of abstraction as the call itself. Nothing really wrong with
it. And you have to put that somewhere.

It can help to read tricky logic by factoring out something, but in this
case the entire hcall just about is switching state, so

   switch_some_state();
   ... switch other state ...

Isn't *necessarily* an improvement over

   ... switch some state...
   ... switch other state...

There is no complicated logic around enter/exit, so there's really no
additional clarity you get by being able to abstract some of it. The
difficult part is how switching that state is entirely what causes the
hcall interrupt to return to the L2 guest.

> >> Giving semantics to the set of
> >> operations related to ptregs/hvstate register load/store is the first
> >> step towards it.
> > 
> > Those structures are entirely the domain of the hcall API though, so
> > if anything belongs in the handler functions it is the handling of
> > those IMO.
> > 
> Absolutely, ideally we would want to contain everything inside the 
> handler, but if a logical name could be given to a set of related ops 
> (ptregs/hvstate specific), that certainly helps the reader to look into 
> bigger picture at first and then get into specific details as needed.

But those related ops don't necesarily make sense to pull out like this,
because they are tied to the API. So depending on what the bigger series
is, it might not make sense. If you are to add another hcall API for
nested HV, then I would say it's probably wrong. What you want to
abstract is the switching between L1 and L2, not moving register values
in and out of the hcall structs.

> >> As you have guessed, this is certainly a precursor to another API
> >> version that we have been working on (still a WIP), and helps isolating
> >> the code flows for backward compatibiility. Having such changes early
> >> upstream helps stablising changes which are not a really a API/design
> >> change.
> > 
> > Right. Some more abstracting could certainly make sense here, I just
> > think at this point we need to see the bigger picture.
>
> I think I am fine holding the cleanup for enter/exit nested for now 
> until we bring the next set of API changes upstream, as that will 
> provide a better context to the value these changes would bring along.
>
> Meanwhile, I shall address your comments on 1/4 and post a v3.
> Thanks for all your review inputs.

Sounds good.

Thanks,
Nick

Re: [PATCH v2 2/4] ppc: spapr: cleanup h_enter_nested() with helper routines.

[Harsh Prateek Bora]

On 5/2/23 14:09, Nicholas Piggin wrote:
> On Tue May 2, 2023 at 5:36 PM AEST, Harsh Prateek Bora wrote:
>>
>>
>> On 5/2/23 12:11, Nicholas Piggin wrote:
>>> On Tue May 2, 2023 at 4:13 PM AEST, Harsh Prateek Bora wrote:
>>>> On 5/2/23 10:19, Nicholas Piggin wrote:
>>>>> On Tue Apr 25, 2023 at 12:47 AM AEST, Harsh Prateek Bora wrote:
>>>>>> @@ -1607,49 +1680,15 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
>>>>>>             return H_P2;
>>>>>>         }
>>>>>>     
>>>>>> -    len = sizeof(env->gpr);
>>>>>> -    assert(len == sizeof(regs->gpr));
>>>>>> -    memcpy(env->gpr, regs->gpr, len);
>>>>>> -
>>>>>> -    env->lr = regs->link;
>>>>>> -    env->ctr = regs->ctr;
>>>>>> -    cpu_write_xer(env, regs->xer);
>>>>>> -    ppc_store_cr(env, regs->ccr);
>>>>>> -
>>>>>> -    env->msr = regs->msr;
>>>>>> -    env->nip = regs->nip;
>>>>>> +    /* restore L2 env from hv_state and ptregs */
>>>>>> +    restore_l2_env(cpu, &hv_state, regs, now);
>>>>>>     
>>>>>>         address_space_unmap(CPU(cpu)->as, regs, len, len, false);
>>>>>
>>>>> I don't agree this improves readability. It also does more with the
>>>>> guest address space mapped, which may not be a big deal is strictly
>>>>> not an improvement.
>>>>>
>>>>> The comment needn't just repeat what the function says, and it does
>>>>> not actually restore the l2 environment. It sets some registers to
>>>>> L2 values, but it also leaves other state.
>>>>>
>>>>> I would like to see this in a larger series if it's going somewhere,
>>>>> but at the moment I'd rather leave it as is.
>>>>>
>>>> While I agree the routine could be named restore_l2_hvstate_ptregs() as
>>>> more appropriate, I think it still makes sense to have the body of
>>>> enter/exit routines with as minimum LOC as possible, with the help of
>>>> minimum helper routines possible.
>>>
>>> I don't think that's a good goal. The entirity of entering and exiting
>>> from a nested guest is 279 lines including comments and no more than
>>> one level of control flow. It's tricky code and has worts, but not
>>> because the number of lines.
>>>
>> Yes, It's a tricky code, and this patch was an attempt to simplify the
>> tricky-ness by giving names to set of related ops with helper routines.
> 
> The H_ENTER_NESTED hcall says "here are a bunch of registers, set the
> environment to that and switch to the L2 guest.
> 
> So having a long list of registers may be a bit tedious but it's at the
> same level of abstraction as the call itself. Nothing really wrong with
> it. And you have to put that somewhere.
> 
> It can help to read tricky logic by factoring out something, but in this
> case the entire hcall just about is switching state, so
> 
>     switch_some_state();
>     ... switch other state ...
> 
> Isn't *necessarily* an improvement over
> 
>     ... switch some state...
>     ... switch other state...
> 
> There is no complicated logic around enter/exit, so there's really no
> additional clarity you get by being able to abstract some of it. The
> difficult part is how switching that state is entirely what causes the
> hcall interrupt to return to the L2 guest.
> 
I think the cleanup may look more appropriate when we have the new 
incoming changes in the same set of enter/exit routines, to ensure it 
doesn't look bloated then.

>>>> Giving semantics to the set of
>>>> operations related to ptregs/hvstate register load/store is the first
>>>> step towards it.
>>>
>>> Those structures are entirely the domain of the hcall API though, so
>>> if anything belongs in the handler functions it is the handling of
>>> those IMO.
>>>
>> Absolutely, ideally we would want to contain everything inside the
>> handler, but if a logical name could be given to a set of related ops
>> (ptregs/hvstate specific), that certainly helps the reader to look into
>> bigger picture at first and then get into specific details as needed.
> 
> But those related ops don't necesarily make sense to pull out like this,
> because they are tied to the API. So depending on what the bigger series
> is, it might not make sense. If you are to add another hcall API for
> nested HV, then I would say it's probably wrong. What you want to
> abstract is the switching between L1 and L2, not moving register values
> in and out of the hcall structs.
> 
There will be a set of new hcalls (to provide more capabilities) and it 
does reuse most of the existing logic/code in enter/exit path as well. 
As suggested, focus of cleanup shall remain on abstracting the switching 
between L1/L2 for common routines. We can discuss more later when we 
have the newer API changes ready for upstream.

>>>> As you have guessed, this is certainly a precursor to another API
>>>> version that we have been working on (still a WIP), and helps isolating
>>>> the code flows for backward compatibiility. Having such changes early
>>>> upstream helps stablising changes which are not a really a API/design
>>>> change.
>>>
>>> Right. Some more abstracting could certainly make sense here, I just
>>> think at this point we need to see the bigger picture.
>>
>> I think I am fine holding the cleanup for enter/exit nested for now
>> until we bring the next set of API changes upstream, as that will
>> provide a better context to the value these changes would bring along.
>>
>> Meanwhile, I shall address your comments on 1/4 and post a v3.
>> Thanks for all your review inputs.
> 
> Sounds good.

Thanks
Harsh
> 
> Thanks,
> Nick

[PATCH v2 3/4] ppc: spapr: cleanup spapr_exit_nested() with helper routines.

[Harsh Prateek Bora]

Currently, in spapr_exit_nested(), it does a lot of register state
restoring from ptregs/hvstate after mapping each of those before
restoring the L1 host state. This patch breaks down those set of ops
to respective helper routines for better code readability/maintenance.

Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
---
 hw/ppc/spapr_hcall.c | 120 ++++++++++++++++++++++++++-----------------
 1 file changed, 72 insertions(+), 48 deletions(-)

diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
index f24d4b368e..e69634bc22 100644
--- a/hw/ppc/spapr_hcall.c
+++ b/hw/ppc/spapr_hcall.c
@@ -1719,45 +1719,14 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
     return env->gpr[3];
 }
 
-void spapr_exit_nested(PowerPCCPU *cpu, int excp)
+static void restore_hvstate_from_env(struct kvmppc_hv_guest_state *hvstate,
+                                     CPUPPCState *env, int excp)
 {
-    CPUState *cs = CPU(cpu);
-    CPUPPCState *env = &cpu->env;
-    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
-    target_ulong r3_return = env->excp_vectors[excp]; /* hcall return value */
-    target_ulong hv_ptr = spapr_cpu->nested_host_state->gpr[4];
-    target_ulong regs_ptr = spapr_cpu->nested_host_state->gpr[5];
-    struct kvmppc_hv_guest_state *hvstate;
-    struct kvmppc_pt_regs *regs;
-    hwaddr len;
-
-    assert(spapr_cpu->in_nested);
-
-    cpu_ppc_hdecr_exit(env);
-
-    len = sizeof(*hvstate);
-    hvstate = address_space_map(CPU(cpu)->as, hv_ptr, &len, true,
-                                MEMTXATTRS_UNSPECIFIED);
-    if (len != sizeof(*hvstate)) {
-        address_space_unmap(CPU(cpu)->as, hvstate, len, 0, true);
-        r3_return = H_PARAMETER;
-        goto out_restore_l1;
-    }
-
     hvstate->cfar = env->cfar;
     hvstate->lpcr = env->spr[SPR_LPCR];
     hvstate->pcr = env->spr[SPR_PCR];
     hvstate->dpdes = env->spr[SPR_DPDES];
     hvstate->hfscr = env->spr[SPR_HFSCR];
-
-    if (excp == POWERPC_EXCP_HDSI) {
-        hvstate->hdar = env->spr[SPR_HDAR];
-        hvstate->hdsisr = env->spr[SPR_HDSISR];
-        hvstate->asdr = env->spr[SPR_ASDR];
-    } else if (excp == POWERPC_EXCP_HISI) {
-        hvstate->asdr = env->spr[SPR_ASDR];
-    }
-
     /* HEIR should be implemented for HV mode and saved here. */
     hvstate->srr0 = env->spr[SPR_SRR0];
     hvstate->srr1 = env->spr[SPR_SRR1];
@@ -1768,27 +1737,43 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
     hvstate->pidr = env->spr[SPR_BOOKS_PID];
     hvstate->ppr = env->spr[SPR_PPR];
 
-    /* Is it okay to specify write length larger than actual data written? */
-    address_space_unmap(CPU(cpu)->as, hvstate, len, len, true);
+    if (excp == POWERPC_EXCP_HDSI) {
+        hvstate->hdar = env->spr[SPR_HDAR];
+        hvstate->hdsisr = env->spr[SPR_HDSISR];
+        hvstate->asdr = env->spr[SPR_ASDR];
+    } else if (excp == POWERPC_EXCP_HISI) {
+        hvstate->asdr = env->spr[SPR_ASDR];
+    }
+}
 
-    len = sizeof(*regs);
-    regs = address_space_map(CPU(cpu)->as, regs_ptr, &len, true,
+static int map_and_restore_l2_hvstate(PowerPCCPU *cpu, int excp, target_ulong *r3)
+{
+    CPUPPCState *env = &cpu->env;
+    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
+    target_ulong hv_ptr = spapr_cpu->nested_host_state->gpr[4];
+    struct kvmppc_hv_guest_state *hvstate;
+    hwaddr len = sizeof(*hvstate);
+
+    hvstate = address_space_map(CPU(cpu)->as, hv_ptr, &len, true,
                                 MEMTXATTRS_UNSPECIFIED);
-    if (!regs || len != sizeof(*regs)) {
-        address_space_unmap(CPU(cpu)->as, regs, len, 0, true);
-        r3_return = H_P2;
-        goto out_restore_l1;
+    if (len != sizeof(*hvstate)) {
+        address_space_unmap(CPU(cpu)->as, hvstate, len, 0, true);
+        *r3 = H_PARAMETER;
+        return -1;
     }
+    restore_hvstate_from_env(hvstate, env, excp);
+    /* Is it okay to specify write length larger than actual data written? */
+    address_space_unmap(CPU(cpu)->as, hvstate, len, len, true);
+    return 0;
+}
 
+static void restore_ptregs_from_env(struct kvmppc_pt_regs *regs,
+                                    CPUPPCState *env, int excp)
+{
+    hwaddr len;
     len = sizeof(env->gpr);
     assert(len == sizeof(regs->gpr));
     memcpy(regs->gpr, env->gpr, len);
-
-    regs->link = env->lr;
-    regs->ctr = env->ctr;
-    regs->xer = cpu_read_xer(env);
-    regs->ccr = ppc_get_cr(env);
-
     if (excp == POWERPC_EXCP_MCHECK ||
         excp == POWERPC_EXCP_RESET ||
         excp == POWERPC_EXCP_SYSCALL) {
@@ -1798,11 +1783,50 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
         regs->nip = env->spr[SPR_HSRR0];
         regs->msr = env->spr[SPR_HSRR1] & env->msr_mask;
     }
+    regs->link = env->lr;
+    regs->ctr = env->ctr;
+    regs->xer = cpu_read_xer(env);
+    regs->ccr = ppc_get_cr(env);
+}
 
+static int map_and_restore_l2_ptregs(PowerPCCPU *cpu, int excp, target_ulong *r3)
+{
+    CPUPPCState *env = &cpu->env;
+    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
+    target_ulong regs_ptr = spapr_cpu->nested_host_state->gpr[5];
+    hwaddr len;
+    struct kvmppc_pt_regs *regs = NULL;
+
+    len = sizeof(*regs);
+    regs = address_space_map(CPU(cpu)->as, regs_ptr, &len, true,
+                             MEMTXATTRS_UNSPECIFIED);
+    if (!regs || len != sizeof(*regs)) {
+        address_space_unmap(CPU(cpu)->as, regs, len, 0, true);
+        *r3 = H_P2;
+        return -1;
+    }
+    restore_ptregs_from_env(regs, env, excp);
     /* Is it okay to specify write length larger than actual data written? */
     address_space_unmap(CPU(cpu)->as, regs, len, len, true);
+    return 0;
+}
+
+void spapr_exit_nested(PowerPCCPU *cpu, int excp)
+{
+    CPUState *cs = CPU(cpu);
+    CPUPPCState *env = &cpu->env;
+    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
+    target_ulong r3_return = env->excp_vectors[excp]; /* hcall return value */
+
+    assert(spapr_cpu->in_nested);
+
+    cpu_ppc_hdecr_exit(env);
+
+   if (!map_and_restore_l2_hvstate(cpu, excp, &r3_return)) {
+       map_and_restore_l2_ptregs (cpu, excp, &r3_return);
+   }
 
-out_restore_l1:
+    /* out_restore_l1 */
     memcpy(env->gpr, spapr_cpu->nested_host_state->gpr, sizeof(env->gpr));
     env->lr = spapr_cpu->nested_host_state->lr;
     env->ctr = spapr_cpu->nested_host_state->ctr;
-- 
2.31.1

Re: [PATCH v2 3/4] ppc: spapr: cleanup spapr_exit_nested() with helper routines.

[Nicholas Piggin]

On Tue Apr 25, 2023 at 12:47 AM AEST, Harsh Prateek Bora wrote:
> Currently, in spapr_exit_nested(), it does a lot of register state
> restoring from ptregs/hvstate after mapping each of those before
> restoring the L1 host state. This patch breaks down those set of ops
> to respective helper routines for better code readability/maintenance.
>
> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
> ---
>  hw/ppc/spapr_hcall.c | 120 ++++++++++++++++++++++++++-----------------
>  1 file changed, 72 insertions(+), 48 deletions(-)
>
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index f24d4b368e..e69634bc22 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -1719,45 +1719,14 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
>      return env->gpr[3];
>  }
>  
> -void spapr_exit_nested(PowerPCCPU *cpu, int excp)
> +static void restore_hvstate_from_env(struct kvmppc_hv_guest_state *hvstate,
> +                                     CPUPPCState *env, int excp)
>  {
> -    CPUState *cs = CPU(cpu);
> -    CPUPPCState *env = &cpu->env;
> -    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> -    target_ulong r3_return = env->excp_vectors[excp]; /* hcall return value */
> -    target_ulong hv_ptr = spapr_cpu->nested_host_state->gpr[4];
> -    target_ulong regs_ptr = spapr_cpu->nested_host_state->gpr[5];
> -    struct kvmppc_hv_guest_state *hvstate;
> -    struct kvmppc_pt_regs *regs;
> -    hwaddr len;
> -
> -    assert(spapr_cpu->in_nested);
> -
> -    cpu_ppc_hdecr_exit(env);
> -
> -    len = sizeof(*hvstate);
> -    hvstate = address_space_map(CPU(cpu)->as, hv_ptr, &len, true,
> -                                MEMTXATTRS_UNSPECIFIED);
> -    if (len != sizeof(*hvstate)) {
> -        address_space_unmap(CPU(cpu)->as, hvstate, len, 0, true);
> -        r3_return = H_PARAMETER;
> -        goto out_restore_l1;
> -    }
> -
>      hvstate->cfar = env->cfar;
>      hvstate->lpcr = env->spr[SPR_LPCR];
>      hvstate->pcr = env->spr[SPR_PCR];
>      hvstate->dpdes = env->spr[SPR_DPDES];
>      hvstate->hfscr = env->spr[SPR_HFSCR];
> -
> -    if (excp == POWERPC_EXCP_HDSI) {
> -        hvstate->hdar = env->spr[SPR_HDAR];
> -        hvstate->hdsisr = env->spr[SPR_HDSISR];
> -        hvstate->asdr = env->spr[SPR_ASDR];
> -    } else if (excp == POWERPC_EXCP_HISI) {
> -        hvstate->asdr = env->spr[SPR_ASDR];
> -    }
> -
>      /* HEIR should be implemented for HV mode and saved here. */
>      hvstate->srr0 = env->spr[SPR_SRR0];
>      hvstate->srr1 = env->spr[SPR_SRR1];
> @@ -1768,27 +1737,43 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
>      hvstate->pidr = env->spr[SPR_BOOKS_PID];
>      hvstate->ppr = env->spr[SPR_PPR];
>  
> -    /* Is it okay to specify write length larger than actual data written? */
> -    address_space_unmap(CPU(cpu)->as, hvstate, len, len, true);
> +    if (excp == POWERPC_EXCP_HDSI) {
> +        hvstate->hdar = env->spr[SPR_HDAR];
> +        hvstate->hdsisr = env->spr[SPR_HDSISR];
> +        hvstate->asdr = env->spr[SPR_ASDR];
> +    } else if (excp == POWERPC_EXCP_HISI) {
> +        hvstate->asdr = env->spr[SPR_ASDR];
> +    }
> +}
>  
> -    len = sizeof(*regs);
> -    regs = address_space_map(CPU(cpu)->as, regs_ptr, &len, true,
> +static int map_and_restore_l2_hvstate(PowerPCCPU *cpu, int excp, target_ulong *r3)
> +{
> +    CPUPPCState *env = &cpu->env;
> +    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> +    target_ulong hv_ptr = spapr_cpu->nested_host_state->gpr[4];
> +    struct kvmppc_hv_guest_state *hvstate;
> +    hwaddr len = sizeof(*hvstate);
> +
> +    hvstate = address_space_map(CPU(cpu)->as, hv_ptr, &len, true,
>                                  MEMTXATTRS_UNSPECIFIED);
> -    if (!regs || len != sizeof(*regs)) {
> -        address_space_unmap(CPU(cpu)->as, regs, len, 0, true);
> -        r3_return = H_P2;
> -        goto out_restore_l1;
> +    if (len != sizeof(*hvstate)) {
> +        address_space_unmap(CPU(cpu)->as, hvstate, len, 0, true);
> +        *r3 = H_PARAMETER;
> +        return -1;
>      }
> +    restore_hvstate_from_env(hvstate, env, excp);
> +    /* Is it okay to specify write length larger than actual data written? */
> +    address_space_unmap(CPU(cpu)->as, hvstate, len, len, true);
> +    return 0;
> +}
>  
> +static void restore_ptregs_from_env(struct kvmppc_pt_regs *regs,
> +                                    CPUPPCState *env, int excp)
> +{
> +    hwaddr len;
>      len = sizeof(env->gpr);
>      assert(len == sizeof(regs->gpr));
>      memcpy(regs->gpr, env->gpr, len);
> -
> -    regs->link = env->lr;
> -    regs->ctr = env->ctr;
> -    regs->xer = cpu_read_xer(env);
> -    regs->ccr = ppc_get_cr(env);
> -
>      if (excp == POWERPC_EXCP_MCHECK ||
>          excp == POWERPC_EXCP_RESET ||
>          excp == POWERPC_EXCP_SYSCALL) {
> @@ -1798,11 +1783,50 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
>          regs->nip = env->spr[SPR_HSRR0];
>          regs->msr = env->spr[SPR_HSRR1] & env->msr_mask;
>      }
> +    regs->link = env->lr;
> +    regs->ctr = env->ctr;
> +    regs->xer = cpu_read_xer(env);
> +    regs->ccr = ppc_get_cr(env);
> +}
>  
> +static int map_and_restore_l2_ptregs(PowerPCCPU *cpu, int excp, target_ulong *r3)
> +{
> +    CPUPPCState *env = &cpu->env;
> +    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> +    target_ulong regs_ptr = spapr_cpu->nested_host_state->gpr[5];
> +    hwaddr len;
> +    struct kvmppc_pt_regs *regs = NULL;
> +
> +    len = sizeof(*regs);
> +    regs = address_space_map(CPU(cpu)->as, regs_ptr, &len, true,
> +                             MEMTXATTRS_UNSPECIFIED);
> +    if (!regs || len != sizeof(*regs)) {
> +        address_space_unmap(CPU(cpu)->as, regs, len, 0, true);
> +        *r3 = H_P2;
> +        return -1;
> +    }
> +    restore_ptregs_from_env(regs, env, excp);
>      /* Is it okay to specify write length larger than actual data written? */
>      address_space_unmap(CPU(cpu)->as, regs, len, len, true);
> +    return 0;
> +}
> +
> +void spapr_exit_nested(PowerPCCPU *cpu, int excp)
> +{
> +    CPUState *cs = CPU(cpu);
> +    CPUPPCState *env = &cpu->env;
> +    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> +    target_ulong r3_return = env->excp_vectors[excp]; /* hcall return value */
> +
> +    assert(spapr_cpu->in_nested);
> +
> +    cpu_ppc_hdecr_exit(env);
> +
> +   if (!map_and_restore_l2_hvstate(cpu, excp, &r3_return)) {
> +       map_and_restore_l2_ptregs (cpu, excp, &r3_return);
> +   }

Same for this one really. Enter/exit nested *is* entirely about
switching from L1 to L2 environment and back so I'm not seeing
where the abstraction is. Just seems more clunky to me.

Abstracting the hcall, error checking, address space mapping and
copying etc into one function and the state switch itself into
another I could see, but that's now spread across the new functions.

So, not sure about this. I think I'd have to see if it was a
precursor to a larger series.

Thanks,
Nick

Re: [PATCH v2 3/4] ppc: spapr: cleanup spapr_exit_nested() with helper routines.

[Harsh Prateek Bora]

Hi Nick,

On 5/2/23 10:36, Nicholas Piggin wrote:
> On Tue Apr 25, 2023 at 12:47 AM AEST, Harsh Prateek Bora wrote:
>> Currently, in spapr_exit_nested(), it does a lot of register state
>> restoring from ptregs/hvstate after mapping each of those before
>> restoring the L1 host state. This patch breaks down those set of ops
>> to respective helper routines for better code readability/maintenance.
>>
>> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>> ---
>>   hw/ppc/spapr_hcall.c | 120 ++++++++++++++++++++++++++-----------------
>>   1 file changed, 72 insertions(+), 48 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index f24d4b368e..e69634bc22 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1719,45 +1719,14 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
>>       return env->gpr[3];
>>   }
>>   
>> -void spapr_exit_nested(PowerPCCPU *cpu, int excp)
>> +static void restore_hvstate_from_env(struct kvmppc_hv_guest_state *hvstate,
>> +                                     CPUPPCState *env, int excp)
>>   {
>> -    CPUState *cs = CPU(cpu);
>> -    CPUPPCState *env = &cpu->env;
>> -    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>> -    target_ulong r3_return = env->excp_vectors[excp]; /* hcall return value */
>> -    target_ulong hv_ptr = spapr_cpu->nested_host_state->gpr[4];
>> -    target_ulong regs_ptr = spapr_cpu->nested_host_state->gpr[5];
>> -    struct kvmppc_hv_guest_state *hvstate;
>> -    struct kvmppc_pt_regs *regs;
>> -    hwaddr len;
>> -
>> -    assert(spapr_cpu->in_nested);
>> -
>> -    cpu_ppc_hdecr_exit(env);
>> -
>> -    len = sizeof(*hvstate);
>> -    hvstate = address_space_map(CPU(cpu)->as, hv_ptr, &len, true,
>> -                                MEMTXATTRS_UNSPECIFIED);
>> -    if (len != sizeof(*hvstate)) {
>> -        address_space_unmap(CPU(cpu)->as, hvstate, len, 0, true);
>> -        r3_return = H_PARAMETER;
>> -        goto out_restore_l1;
>> -    }
>> -
>>       hvstate->cfar = env->cfar;
>>       hvstate->lpcr = env->spr[SPR_LPCR];
>>       hvstate->pcr = env->spr[SPR_PCR];
>>       hvstate->dpdes = env->spr[SPR_DPDES];
>>       hvstate->hfscr = env->spr[SPR_HFSCR];
>> -
>> -    if (excp == POWERPC_EXCP_HDSI) {
>> -        hvstate->hdar = env->spr[SPR_HDAR];
>> -        hvstate->hdsisr = env->spr[SPR_HDSISR];
>> -        hvstate->asdr = env->spr[SPR_ASDR];
>> -    } else if (excp == POWERPC_EXCP_HISI) {
>> -        hvstate->asdr = env->spr[SPR_ASDR];
>> -    }
>> -
>>       /* HEIR should be implemented for HV mode and saved here. */
>>       hvstate->srr0 = env->spr[SPR_SRR0];
>>       hvstate->srr1 = env->spr[SPR_SRR1];
>> @@ -1768,27 +1737,43 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
>>       hvstate->pidr = env->spr[SPR_BOOKS_PID];
>>       hvstate->ppr = env->spr[SPR_PPR];
>>   
>> -    /* Is it okay to specify write length larger than actual data written? */
>> -    address_space_unmap(CPU(cpu)->as, hvstate, len, len, true);
>> +    if (excp == POWERPC_EXCP_HDSI) {
>> +        hvstate->hdar = env->spr[SPR_HDAR];
>> +        hvstate->hdsisr = env->spr[SPR_HDSISR];
>> +        hvstate->asdr = env->spr[SPR_ASDR];
>> +    } else if (excp == POWERPC_EXCP_HISI) {
>> +        hvstate->asdr = env->spr[SPR_ASDR];
>> +    }
>> +}
>>   
>> -    len = sizeof(*regs);
>> -    regs = address_space_map(CPU(cpu)->as, regs_ptr, &len, true,
>> +static int map_and_restore_l2_hvstate(PowerPCCPU *cpu, int excp, target_ulong *r3)
>> +{
>> +    CPUPPCState *env = &cpu->env;
>> +    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>> +    target_ulong hv_ptr = spapr_cpu->nested_host_state->gpr[4];
>> +    struct kvmppc_hv_guest_state *hvstate;
>> +    hwaddr len = sizeof(*hvstate);
>> +
>> +    hvstate = address_space_map(CPU(cpu)->as, hv_ptr, &len, true,
>>                                   MEMTXATTRS_UNSPECIFIED);
>> -    if (!regs || len != sizeof(*regs)) {
>> -        address_space_unmap(CPU(cpu)->as, regs, len, 0, true);
>> -        r3_return = H_P2;
>> -        goto out_restore_l1;
>> +    if (len != sizeof(*hvstate)) {
>> +        address_space_unmap(CPU(cpu)->as, hvstate, len, 0, true);
>> +        *r3 = H_PARAMETER;
>> +        return -1;
>>       }
>> +    restore_hvstate_from_env(hvstate, env, excp);
>> +    /* Is it okay to specify write length larger than actual data written? */
>> +    address_space_unmap(CPU(cpu)->as, hvstate, len, len, true);
>> +    return 0;
>> +}
>>   
>> +static void restore_ptregs_from_env(struct kvmppc_pt_regs *regs,
>> +                                    CPUPPCState *env, int excp)
>> +{
>> +    hwaddr len;
>>       len = sizeof(env->gpr);
>>       assert(len == sizeof(regs->gpr));
>>       memcpy(regs->gpr, env->gpr, len);
>> -
>> -    regs->link = env->lr;
>> -    regs->ctr = env->ctr;
>> -    regs->xer = cpu_read_xer(env);
>> -    regs->ccr = ppc_get_cr(env);
>> -
>>       if (excp == POWERPC_EXCP_MCHECK ||
>>           excp == POWERPC_EXCP_RESET ||
>>           excp == POWERPC_EXCP_SYSCALL) {
>> @@ -1798,11 +1783,50 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
>>           regs->nip = env->spr[SPR_HSRR0];
>>           regs->msr = env->spr[SPR_HSRR1] & env->msr_mask;
>>       }
>> +    regs->link = env->lr;
>> +    regs->ctr = env->ctr;
>> +    regs->xer = cpu_read_xer(env);
>> +    regs->ccr = ppc_get_cr(env);
>> +}
>>   
>> +static int map_and_restore_l2_ptregs(PowerPCCPU *cpu, int excp, target_ulong *r3)
>> +{
>> +    CPUPPCState *env = &cpu->env;
>> +    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>> +    target_ulong regs_ptr = spapr_cpu->nested_host_state->gpr[5];
>> +    hwaddr len;
>> +    struct kvmppc_pt_regs *regs = NULL;
>> +
>> +    len = sizeof(*regs);
>> +    regs = address_space_map(CPU(cpu)->as, regs_ptr, &len, true,
>> +                             MEMTXATTRS_UNSPECIFIED);
>> +    if (!regs || len != sizeof(*regs)) {
>> +        address_space_unmap(CPU(cpu)->as, regs, len, 0, true);
>> +        *r3 = H_P2;
>> +        return -1;
>> +    }
>> +    restore_ptregs_from_env(regs, env, excp);
>>       /* Is it okay to specify write length larger than actual data written? */
>>       address_space_unmap(CPU(cpu)->as, regs, len, len, true);
>> +    return 0;
>> +}
>> +
>> +void spapr_exit_nested(PowerPCCPU *cpu, int excp)
>> +{
>> +    CPUState *cs = CPU(cpu);
>> +    CPUPPCState *env = &cpu->env;
>> +    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>> +    target_ulong r3_return = env->excp_vectors[excp]; /* hcall return value */
>> +
>> +    assert(spapr_cpu->in_nested);
>> +
>> +    cpu_ppc_hdecr_exit(env);
>> +
>> +   if (!map_and_restore_l2_hvstate(cpu, excp, &r3_return)) {
>> +       map_and_restore_l2_ptregs (cpu, excp, &r3_return);
>> +   }
> 
> Same for this one really. Enter/exit nested *is* entirely about
> switching from L1 to L2 environment and back so I'm not seeing
> where the abstraction is. Just seems more clunky to me.
> 
> Abstracting the hcall, error checking, address space mapping and
> copying etc into one function and the state switch itself into
> another I could see, but that's now spread across the new functions.
> 
> So, not sure about this. I think I'd have to see if it was a
> precursor to a larger series.
> 
I have responded on your similar comment on 2/4 patch trying to clarify 
the motivation behind these changes. Hope it gives you a different 
perspective about the motivation behind this change. Let me know if you 
still think otherwise or any further patch improvements if needed.

regards,
Harsh

> Thanks,
> Nick

[PATCH v2 4/4] MAINTAINERS: Adding myself in the list for ppc/spapr

[Harsh Prateek Bora]

Would like to get notified of changes in this area and review them.

Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 2c2068ea5c..b5d290cf92 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1420,6 +1420,7 @@ M: Daniel Henrique Barboza <danielhb413@gmail.com>
 R: Cédric Le Goater <clg@kaod.org>
 R: David Gibson <david@gibson.dropbear.id.au>
 R: Greg Kurz <groug@kaod.org>
+R: Harsh Prateek Bora <harshpb@linux.ibm.com>
 L: qemu-ppc@nongnu.org
 S: Odd Fixes
 F: hw/*/spapr*
-- 
2.31.1