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

[Harsh Prateek Bora]

On 4/14/23 17:23, Fabiano Rosas wrote:
> Harsh Prateek Bora <harshpb@linux.ibm.com> writes:
>
> > 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 | 99 +++++++++++++++++++++++++++-----------------
> >   1 file changed, 60 insertions(+), 39 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index 124cee5e53..a13e5256ab 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -1544,6 +1544,62 @@ 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;
>
> add a blank line here

ok
> > +    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;
>
> here as well

ok
> > +    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_hvstate(CPUPPCState *env,
>
> Take a look at how the kernel does it. It might be better to have ptregs
> and hv regs separate. Also probably better to have some terms specific
> to the domain (l2 state, l1 state, etc).

ok, will separate them out and include l2/l1 wherever applicable.

> > +                                            struct kvmppc_pt_regs *regs,
> > +                                            struct kvmppc_hv_guest_state 
> > *hv_state)
> > +{
> > +    assert(env);
> > +    assert(regs);
> > +    assert(hv_state);
> > +    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);
> > +    /* hv_state->amor is not used in api v1 */
>
> That's not really an API thing. More of an oversight.
yeh, that comment was retained from existing source, will remove.

> > +    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;
>
> I would advise against the extra spacing inside functions.
sure, will keep the spacing as-is inside functions.

Thanks
Harsh