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

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

Hi Nick,

On 5/2/23 10:36, Nicholas Piggin wrote:
> On Tue Apr 25, 2023 at 12:47 AM AEST, Harsh Prateek Bora wrote:
>> Currently, in spapr_exit_nested(), it does a lot of register state
>> restoring from ptregs/hvstate after mapping each of those before
>> restoring the L1 host state. This patch breaks down those set of ops
>> to respective helper routines for better code readability/maintenance.
>>
>> Signed-off-by: Harsh Prateek Bora <harshpb@linux.ibm.com>
>> ---
>>   hw/ppc/spapr_hcall.c | 120 ++++++++++++++++++++++++++-----------------
>>   1 file changed, 72 insertions(+), 48 deletions(-)
>>
>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>> index f24d4b368e..e69634bc22 100644
>> --- a/hw/ppc/spapr_hcall.c
>> +++ b/hw/ppc/spapr_hcall.c
>> @@ -1719,45 +1719,14 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
>>       return env->gpr[3];
>>   }
>>   
>> -void spapr_exit_nested(PowerPCCPU *cpu, int excp)
>> +static void restore_hvstate_from_env(struct kvmppc_hv_guest_state *hvstate,
>> +                                     CPUPPCState *env, int excp)
>>   {
>> -    CPUState *cs = CPU(cpu);
>> -    CPUPPCState *env = &cpu->env;
>> -    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>> -    target_ulong r3_return = env->excp_vectors[excp]; /* hcall return value */
>> -    target_ulong hv_ptr = spapr_cpu->nested_host_state->gpr[4];
>> -    target_ulong regs_ptr = spapr_cpu->nested_host_state->gpr[5];
>> -    struct kvmppc_hv_guest_state *hvstate;
>> -    struct kvmppc_pt_regs *regs;
>> -    hwaddr len;
>> -
>> -    assert(spapr_cpu->in_nested);
>> -
>> -    cpu_ppc_hdecr_exit(env);
>> -
>> -    len = sizeof(*hvstate);
>> -    hvstate = address_space_map(CPU(cpu)->as, hv_ptr, &len, true,
>> -                                MEMTXATTRS_UNSPECIFIED);
>> -    if (len != sizeof(*hvstate)) {
>> -        address_space_unmap(CPU(cpu)->as, hvstate, len, 0, true);
>> -        r3_return = H_PARAMETER;
>> -        goto out_restore_l1;
>> -    }
>> -
>>       hvstate->cfar = env->cfar;
>>       hvstate->lpcr = env->spr[SPR_LPCR];
>>       hvstate->pcr = env->spr[SPR_PCR];
>>       hvstate->dpdes = env->spr[SPR_DPDES];
>>       hvstate->hfscr = env->spr[SPR_HFSCR];
>> -
>> -    if (excp == POWERPC_EXCP_HDSI) {
>> -        hvstate->hdar = env->spr[SPR_HDAR];
>> -        hvstate->hdsisr = env->spr[SPR_HDSISR];
>> -        hvstate->asdr = env->spr[SPR_ASDR];
>> -    } else if (excp == POWERPC_EXCP_HISI) {
>> -        hvstate->asdr = env->spr[SPR_ASDR];
>> -    }
>> -
>>       /* HEIR should be implemented for HV mode and saved here. */
>>       hvstate->srr0 = env->spr[SPR_SRR0];
>>       hvstate->srr1 = env->spr[SPR_SRR1];
>> @@ -1768,27 +1737,43 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
>>       hvstate->pidr = env->spr[SPR_BOOKS_PID];
>>       hvstate->ppr = env->spr[SPR_PPR];
>>   
>> -    /* Is it okay to specify write length larger than actual data written? */
>> -    address_space_unmap(CPU(cpu)->as, hvstate, len, len, true);
>> +    if (excp == POWERPC_EXCP_HDSI) {
>> +        hvstate->hdar = env->spr[SPR_HDAR];
>> +        hvstate->hdsisr = env->spr[SPR_HDSISR];
>> +        hvstate->asdr = env->spr[SPR_ASDR];
>> +    } else if (excp == POWERPC_EXCP_HISI) {
>> +        hvstate->asdr = env->spr[SPR_ASDR];
>> +    }
>> +}
>>   
>> -    len = sizeof(*regs);
>> -    regs = address_space_map(CPU(cpu)->as, regs_ptr, &len, true,
>> +static int map_and_restore_l2_hvstate(PowerPCCPU *cpu, int excp, target_ulong *r3)
>> +{
>> +    CPUPPCState *env = &cpu->env;
>> +    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>> +    target_ulong hv_ptr = spapr_cpu->nested_host_state->gpr[4];
>> +    struct kvmppc_hv_guest_state *hvstate;
>> +    hwaddr len = sizeof(*hvstate);
>> +
>> +    hvstate = address_space_map(CPU(cpu)->as, hv_ptr, &len, true,
>>                                   MEMTXATTRS_UNSPECIFIED);
>> -    if (!regs || len != sizeof(*regs)) {
>> -        address_space_unmap(CPU(cpu)->as, regs, len, 0, true);
>> -        r3_return = H_P2;
>> -        goto out_restore_l1;
>> +    if (len != sizeof(*hvstate)) {
>> +        address_space_unmap(CPU(cpu)->as, hvstate, len, 0, true);
>> +        *r3 = H_PARAMETER;
>> +        return -1;
>>       }
>> +    restore_hvstate_from_env(hvstate, env, excp);
>> +    /* Is it okay to specify write length larger than actual data written? */
>> +    address_space_unmap(CPU(cpu)->as, hvstate, len, len, true);
>> +    return 0;
>> +}
>>   
>> +static void restore_ptregs_from_env(struct kvmppc_pt_regs *regs,
>> +                                    CPUPPCState *env, int excp)
>> +{
>> +    hwaddr len;
>>       len = sizeof(env->gpr);
>>       assert(len == sizeof(regs->gpr));
>>       memcpy(regs->gpr, env->gpr, len);
>> -
>> -    regs->link = env->lr;
>> -    regs->ctr = env->ctr;
>> -    regs->xer = cpu_read_xer(env);
>> -    regs->ccr = ppc_get_cr(env);
>> -
>>       if (excp == POWERPC_EXCP_MCHECK ||
>>           excp == POWERPC_EXCP_RESET ||
>>           excp == POWERPC_EXCP_SYSCALL) {
>> @@ -1798,11 +1783,50 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
>>           regs->nip = env->spr[SPR_HSRR0];
>>           regs->msr = env->spr[SPR_HSRR1] & env->msr_mask;
>>       }
>> +    regs->link = env->lr;
>> +    regs->ctr = env->ctr;
>> +    regs->xer = cpu_read_xer(env);
>> +    regs->ccr = ppc_get_cr(env);
>> +}
>>   
>> +static int map_and_restore_l2_ptregs(PowerPCCPU *cpu, int excp, target_ulong *r3)
>> +{
>> +    CPUPPCState *env = &cpu->env;
>> +    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>> +    target_ulong regs_ptr = spapr_cpu->nested_host_state->gpr[5];
>> +    hwaddr len;
>> +    struct kvmppc_pt_regs *regs = NULL;
>> +
>> +    len = sizeof(*regs);
>> +    regs = address_space_map(CPU(cpu)->as, regs_ptr, &len, true,
>> +                             MEMTXATTRS_UNSPECIFIED);
>> +    if (!regs || len != sizeof(*regs)) {
>> +        address_space_unmap(CPU(cpu)->as, regs, len, 0, true);
>> +        *r3 = H_P2;
>> +        return -1;
>> +    }
>> +    restore_ptregs_from_env(regs, env, excp);
>>       /* Is it okay to specify write length larger than actual data written? */
>>       address_space_unmap(CPU(cpu)->as, regs, len, len, true);
>> +    return 0;
>> +}
>> +
>> +void spapr_exit_nested(PowerPCCPU *cpu, int excp)
>> +{
>> +    CPUState *cs = CPU(cpu);
>> +    CPUPPCState *env = &cpu->env;
>> +    SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
>> +    target_ulong r3_return = env->excp_vectors[excp]; /* hcall return value */
>> +
>> +    assert(spapr_cpu->in_nested);
>> +
>> +    cpu_ppc_hdecr_exit(env);
>> +
>> +   if (!map_and_restore_l2_hvstate(cpu, excp, &r3_return)) {
>> +       map_and_restore_l2_ptregs (cpu, excp, &r3_return);
>> +   }
> 
> Same for this one really. Enter/exit nested *is* entirely about
> switching from L1 to L2 environment and back so I'm not seeing
> where the abstraction is. Just seems more clunky to me.
> 
> Abstracting the hcall, error checking, address space mapping and
> copying etc into one function and the state switch itself into
> another I could see, but that's now spread across the new functions.
> 
> So, not sure about this. I think I'd have to see if it was a
> precursor to a larger series.
> 
I have responded on your similar comment on 2/4 patch trying to clarify 
the motivation behind these changes. Hope it gives you a different 
perspective about the motivation behind this change. Let me know if you 
still think otherwise or any further patch improvements if needed.

regards,
Harsh

> Thanks,
> Nick