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

["Nicholas Piggin" <npiggin@gmail.com>]

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