[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/4] ppc: spapr: cleanup h_enter_nested() with helper rout
From: |
Nicholas Piggin |
Subject: |
Re: [PATCH v2 2/4] ppc: spapr: cleanup h_enter_nested() with helper routines. |
Date: |
Tue, 02 May 2023 16:41:01 +1000 |
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