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

[Harsh Prateek Bora]

On 4/14/23 17:28, Fabiano Rosas wrote:
> Harsh Prateek Bora <harshpb@linux.ibm.com> writes:
>
> > 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 | 132 +++++++++++++++++++++++++------------------
> >   1 file changed, 78 insertions(+), 54 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index a77b4c9076..ed3a21471d 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -1701,36 +1701,23 @@ 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];
> > +    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];
> > +    /* HEIR should be implemented for HV mode and saved here. */
> > +    hvstate->srr0    = env->spr[SPR_SRR0];
> > +    hvstate->srr1    = env->spr[SPR_SRR1];
> > +    hvstate->sprg[0] = env->spr[SPR_SPRG0];
> > +    hvstate->sprg[1] = env->spr[SPR_SPRG1];
> > +    hvstate->sprg[2] = env->spr[SPR_SPRG2];
> > +    hvstate->sprg[3] = env->spr[SPR_SPRG3];
> > +    hvstate->pidr    = env->spr[SPR_BOOKS_PID];
> > +    hvstate->ppr     = env->spr[SPR_PPR];
>
> Just leave these lines as they were. Let's avoid spending time
> discussing code style. Also, the patch became harder to review by having
> these unrelated changes interspersed.
Ok, will keep the spacing as-is inside helper routine 
restore_hvstate_from_env() as well.