| Message ID | 20230331065344.112341-5-harshpb@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | Cleanup [h_enter|spapr_exit]_nested routines | expand |
Harsh Prateek Bora <harshpb@linux.ibm.com> writes: > 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 | 132 +++++++++++++++++++++++++------------------ > 1 file changed, 78 insertions(+), 54 deletions(-) > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > index a77b4c9076..ed3a21471d 100644 > --- a/hw/ppc/spapr_hcall.c > +++ b/hw/ppc/spapr_hcall.c > @@ -1701,36 +1701,23 @@ 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]; > + 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]; > + /* HEIR should be implemented for HV mode and saved here. */ > + hvstate->srr0 = env->spr[SPR_SRR0]; > + hvstate->srr1 = env->spr[SPR_SRR1]; > + hvstate->sprg[0] = env->spr[SPR_SPRG0]; > + hvstate->sprg[1] = env->spr[SPR_SPRG1]; > + hvstate->sprg[2] = env->spr[SPR_SPRG2]; > + hvstate->sprg[3] = env->spr[SPR_SPRG3]; > + hvstate->pidr = env->spr[SPR_BOOKS_PID]; > + hvstate->ppr = env->spr[SPR_PPR]; Just leave these lines as they were. Let's avoid spending time discussing code style. Also, the patch became harder to review by having these unrelated changes interspersed.
On 4/14/23 17:28, Fabiano Rosas wrote: > Harsh Prateek Bora <harshpb@linux.ibm.com> writes: > >> 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 | 132 +++++++++++++++++++++++++------------------ >> 1 file changed, 78 insertions(+), 54 deletions(-) >> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c >> index a77b4c9076..ed3a21471d 100644 >> --- a/hw/ppc/spapr_hcall.c >> +++ b/hw/ppc/spapr_hcall.c >> @@ -1701,36 +1701,23 @@ 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]; >> + 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]; >> + /* HEIR should be implemented for HV mode and saved here. */ >> + hvstate->srr0 = env->spr[SPR_SRR0]; >> + hvstate->srr1 = env->spr[SPR_SRR1]; >> + hvstate->sprg[0] = env->spr[SPR_SPRG0]; >> + hvstate->sprg[1] = env->spr[SPR_SPRG1]; >> + hvstate->sprg[2] = env->spr[SPR_SPRG2]; >> + hvstate->sprg[3] = env->spr[SPR_SPRG3]; >> + hvstate->pidr = env->spr[SPR_BOOKS_PID]; >> + hvstate->ppr = env->spr[SPR_PPR]; > > Just leave these lines as they were. Let's avoid spending time > discussing code style. Also, the patch became harder to review by having > these unrelated changes interspersed. > Ok, will keep the spacing as-is inside helper routine restore_hvstate_from_env() as well.
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c index a77b4c9076..ed3a21471d 100644 --- a/hw/ppc/spapr_hcall.c +++ b/hw/ppc/spapr_hcall.c @@ -1701,36 +1701,23 @@ 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]; + 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]; + /* HEIR should be implemented for HV mode and saved here. */ + hvstate->srr0 = env->spr[SPR_SRR0]; + hvstate->srr1 = env->spr[SPR_SRR1]; + hvstate->sprg[0] = env->spr[SPR_SPRG0]; + hvstate->sprg[1] = env->spr[SPR_SPRG1]; + hvstate->sprg[2] = env->spr[SPR_SPRG2]; + hvstate->sprg[3] = env->spr[SPR_SPRG3]; + hvstate->pidr = env->spr[SPR_BOOKS_PID]; + hvstate->ppr = env->spr[SPR_PPR]; if (excp == POWERPC_EXCP_HDSI) { hvstate->hdar = env->spr[SPR_HDAR]; @@ -1739,38 +1726,36 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp) } 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]; - hvstate->sprg[0] = env->spr[SPR_SPRG0]; - hvstate->sprg[1] = env->spr[SPR_SPRG1]; - hvstate->sprg[2] = env->spr[SPR_SPRG2]; - hvstate->sprg[3] = env->spr[SPR_SPRG3]; - 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); +static int map_and_restore_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); - len = sizeof(*regs); - regs = address_space_map(CPU(cpu)->as, regs_ptr, &len, true, + 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) { @@ -1780,11 +1765,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_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_hvstate(cpu, excp, &r3_return)) { + map_and_restore_ptregs (cpu, excp, &r3_return); + } -out_restore_l1: + /* out_restore_l1 */ memcpy(env->gpr, spapr_cpu->nested_host_state->gpr, sizeof(env->gpr)); env->lr = spapr_cpu->nested_host_state->lr; env->ctr = spapr_cpu->nested_host_state->ctr;
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 | 132 +++++++++++++++++++++++++------------------ 1 file changed, 78 insertions(+), 54 deletions(-)