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 14:09, Nicholas Piggin wrote:
> On Tue May 2, 2023 at 5:36 PM AEST, Harsh Prateek Bora wrote:
>>
>>
>> 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.
> 
> The H_ENTER_NESTED hcall says "here are a bunch of registers, set the
> environment to that and switch to the L2 guest.
> 
> So having a long list of registers may be a bit tedious but it's at the
> same level of abstraction as the call itself. Nothing really wrong with
> it. And you have to put that somewhere.
> 
> It can help to read tricky logic by factoring out something, but in this
> case the entire hcall just about is switching state, so
> 
>     switch_some_state();
>     ... switch other state ...
> 
> Isn't *necessarily* an improvement over
> 
>     ... switch some state...
>     ... switch other state...
> 
> There is no complicated logic around enter/exit, so there's really no
> additional clarity you get by being able to abstract some of it. The
> difficult part is how switching that state is entirely what causes the
> hcall interrupt to return to the L2 guest.
> 
I think the cleanup may look more appropriate when we have the new 
incoming changes in the same set of enter/exit routines, to ensure it 
doesn't look bloated then.

>>>> 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.
> 
> But those related ops don't necesarily make sense to pull out like this,
> because they are tied to the API. So depending on what the bigger series
> is, it might not make sense. If you are to add another hcall API for
> nested HV, then I would say it's probably wrong. What you want to
> abstract is the switching between L1 and L2, not moving register values
> in and out of the hcall structs.
> 
There will be a set of new hcalls (to provide more capabilities) and it 
does reuse most of the existing logic/code in enter/exit path as well. 
As suggested, focus of cleanup shall remain on abstracting the switching 
between L1/L2 for common routines. We can discuss more later when we 
have the newer API changes ready for upstream.

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

Thanks
Harsh
> 
> Thanks,
> Nick