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