|
From: | Harsh Prateek Bora |
Subject: | Re: [PATCH v2 2/4] ppc: spapr: cleanup h_enter_nested() with helper routines. |
Date: | Tue, 2 May 2023 13:06:29 +0530 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.9.0 |
On 5/2/23 12:11, Nicholas Piggin wrote:
Yes, It's a tricky code, and this patch was an attempt to simplify the tricky-ness by giving names to set of related ops with helper routines.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.
Absolutely, ideally we would want to contain everything inside the handler, but if a logical name could be given to a set of related ops (ptregs/hvstate specific), that certainly helps the reader to look into bigger picture at first and then get into specific details as needed.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.
I think I am fine holding the cleanup for enter/exit nested for now until we bring the next set of API changes upstream, as that will provide a better context to the value these changes would bring along.
Meanwhile, I shall address your comments on 1/4 and post a v3. Thanks for all your review inputs. regards, Harsh
Thanks, Nick
[Prev in Thread] | Current Thread | [Next in Thread] |