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
