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 10:19, Nicholas Piggin wrote:
> On Tue Apr 25, 2023 at 12:47 AM AEST, Harsh Prateek Bora wrote:
>> h_enter_nested() currently does a lot of register specific operations
>> which should be abstracted logically to simplify the code for better
>> readability. This patch breaks down relevant blocks into respective
>> helper routines to make use of them for better readability/maintenance.
>>
>> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>> ---
>>   hw/ppc/spapr_hcall.c | 117 ++++++++++++++++++++++++++++---------------
>>   1 file changed, 78 insertions(+), 39 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index 124cee5e53..f24d4b368e 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1544,6 +1544,81 @@ static target_ulong h_copy_tofrom_guest(PowerPCCPU *cpu,
>>       return H_FUNCTION;
>>   }
>>   
>> +static void restore_hdec_from_hvstate(CPUPPCState *dst,
>> +                                      struct kvmppc_hv_guest_state *hv_state,
>> +                                      target_ulong now)
>> +{
>> +    target_ulong hdec;
>> +
>> +    assert(hv_state);
>> +    hdec = hv_state->hdec_expiry - now;
>> +    cpu_ppc_hdecr_init(dst);
>> +    cpu_ppc_store_hdecr(dst, hdec);
>> +}
>> +
>> +static void restore_lpcr_from_hvstate(PowerPCCPU *cpu,
>> +                                      struct kvmppc_hv_guest_state *hv_state)
>> +{
>> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>> +    CPUPPCState *dst = &cpu->env;
>> +    target_ulong lpcr, lpcr_mask;
>> +
>> +    assert(hv_state);
>> +    lpcr_mask = LPCR_DPFD | LPCR_ILE | LPCR_AIL | LPCR_LD | LPCR_MER;
>> +    lpcr = (dst->spr[SPR_LPCR] & ~lpcr_mask) | (hv_state->lpcr & lpcr_mask);
>> +    lpcr |= LPCR_HR | LPCR_UPRT | LPCR_GTSE | LPCR_HVICE | LPCR_HDICE;
>> +    lpcr &= ~LPCR_LPES0;
>> +    dst->spr[SPR_LPCR] = lpcr & pcc->lpcr_mask;
>> +}
>> +
>> +static void restore_env_from_ptregs(CPUPPCState *env,
>> +                                    struct kvmppc_pt_regs *regs)
>> +{
>> +    assert(env);
>> +    assert(regs);
>> +    assert(sizeof(env->gpr) == sizeof(regs->gpr));
>> +    memcpy(env->gpr, regs->gpr, sizeof(env->gpr));
>> +    env->nip = regs->nip;
>> +    env->msr = regs->msr;
>> +    env->lr = regs->link;
>> +    env->ctr = regs->ctr;
>> +    cpu_write_xer(env, regs->xer);
>> +    ppc_store_cr(env, regs->ccr);
>> +}
>> +
>> +static void restore_env_from_hvstate(CPUPPCState *env,
>> +                                     struct kvmppc_hv_guest_state *hv_state)
>> +{
>> +    assert(env);
>> +    assert(hv_state);
>> +    env->spr[SPR_HFSCR] = hv_state->hfscr;
>> +    /* TCG does not implement DAWR*, CIABR, PURR, SPURR, IC, VTB, HEIR SPRs*/
>> +    env->cfar = hv_state->cfar;
>> +    env->spr[SPR_PCR] = hv_state->pcr;
>> +    env->spr[SPR_DPDES] = hv_state->dpdes;
>> +    env->spr[SPR_SRR0] = hv_state->srr0;
>> +    env->spr[SPR_SRR1] = hv_state->srr1;
>> +    env->spr[SPR_SPRG0] = hv_state->sprg[0];
>> +    env->spr[SPR_SPRG1] = hv_state->sprg[1];
>> +    env->spr[SPR_SPRG2] = hv_state->sprg[2];
>> +    env->spr[SPR_SPRG3] = hv_state->sprg[3];
>> +    env->spr[SPR_BOOKS_PID] = hv_state->pidr;
>> +    env->spr[SPR_PPR] = hv_state->ppr;
>> +}
>> +
>> +static inline void restore_l2_env(PowerPCCPU *cpu,
>> +		                  struct kvmppc_hv_guest_state *hv_state,
>> +				  struct kvmppc_pt_regs *regs,
>> +				  target_ulong now)
>> +{
>> +    CPUPPCState *env = &cpu->env;
>> +
>> +    restore_env_from_ptregs(env, regs);
>> +    restore_env_from_hvstate(env, hv_state);
>> +    restore_lpcr_from_hvstate(cpu, hv_state);
>> +    restore_hdec_from_hvstate(env, hv_state, now);
>> +}
>> +
>>   /*
>>    * When this handler returns, the environment is switched to the L2 guest
>>    * and TCG begins running that. spapr_exit_nested() performs the switch from
>> @@ -1554,14 +1629,12 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
>>                                      target_ulong opcode,
>>                                      target_ulong *args)
>>   {
>> -    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
>>       CPUState *cs = CPU(cpu);
>>       CPUPPCState *env = &cpu->env;
>>       SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>>       target_ulong hv_ptr = args[0];
>>       target_ulong regs_ptr = args[1];
>> -    target_ulong hdec, now = cpu_ppc_load_tbl(env);
>> -    target_ulong lpcr, lpcr_mask;
>> +    target_ulong now = cpu_ppc_load_tbl(env);
>>       struct kvmppc_hv_guest_state *hvstate;
>>       struct kvmppc_hv_guest_state hv_state;
>>       struct kvmppc_pt_regs *regs;
>> @@ -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. Giving semantics to the set of 
operations related to ptregs/hvstate register load/store is the first 
step towards it.

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.

regards,
Harsh

> Thanks,
> Nick