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

[Harsh Prateek Bora <harshpb@linux.ibm.com>]

On 5/2/23 12:11, Nicholas Piggin wrote:
> 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.
> 
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.

>> 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.
> 
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.

>> 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
>