Message ID | 20230331065344.112341-3-harshpb@linux.ibm.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Cleanup [h_enter|spapr_exit]_nested routines | expand |
Harsh Prateek Bora <harshpb@linux.ibm.com> writes: > 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 | 99 +++++++++++++++++++++++++++----------------- > 1 file changed, 60 insertions(+), 39 deletions(-) > > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c > index 124cee5e53..a13e5256ab 100644 > --- a/hw/ppc/spapr_hcall.c > +++ b/hw/ppc/spapr_hcall.c > @@ -1544,6 +1544,62 @@ 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; add a blank line here > + 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; here as well > + 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_hvstate(CPUPPCState *env, Take a look at how the kernel does it. It might be better to have ptregs and hv regs separate. Also probably better to have some terms specific to the domain (l2 state, l1 state, etc). > + struct kvmppc_pt_regs *regs, > + struct kvmppc_hv_guest_state *hv_state) > +{ > + assert(env); > + assert(regs); > + assert(hv_state); > + 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); > + /* hv_state->amor is not used in api v1 */ That's not really an API thing. More of an oversight. > + 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; I would advise against the extra spacing inside functions.
On 4/14/23 17:23, Fabiano Rosas wrote: > Harsh Prateek Bora <harshpb@linux.ibm.com> writes: > >> 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 | 99 +++++++++++++++++++++++++++----------------- >> 1 file changed, 60 insertions(+), 39 deletions(-) >> >> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c >> index 124cee5e53..a13e5256ab 100644 >> --- a/hw/ppc/spapr_hcall.c >> +++ b/hw/ppc/spapr_hcall.c >> @@ -1544,6 +1544,62 @@ 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; > > add a blank line here ok > >> + 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; > > here as well ok > >> + 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_hvstate(CPUPPCState *env, > > Take a look at how the kernel does it. It might be better to have ptregs > and hv regs separate. Also probably better to have some terms specific > to the domain (l2 state, l1 state, etc). > ok, will separate them out and include l2/l1 wherever applicable. >> + struct kvmppc_pt_regs *regs, >> + struct kvmppc_hv_guest_state *hv_state) >> +{ >> + assert(env); >> + assert(regs); >> + assert(hv_state); >> + 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); >> + /* hv_state->amor is not used in api v1 */ > > That's not really an API thing. More of an oversight. > yeh, that comment was retained from existing source, will remove. >> + 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; > > I would advise against the extra spacing inside functions. > sure, will keep the spacing as-is inside functions. Thanks Harsh
diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c index 124cee5e53..a13e5256ab 100644 --- a/hw/ppc/spapr_hcall.c +++ b/hw/ppc/spapr_hcall.c @@ -1544,6 +1544,62 @@ 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_hvstate(CPUPPCState *env, + struct kvmppc_pt_regs *regs, + struct kvmppc_hv_guest_state *hv_state) +{ + assert(env); + assert(regs); + assert(hv_state); + 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); + /* hv_state->amor is not used in api v1 */ + 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; +} + /* * 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 +1610,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 +1661,16 @@ 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_env_from_ptregs_hvstate(env, regs, &hv_state); + restore_lpcr_from_hvstate(cpu, &hv_state); + restore_hdec_from_hvstate(env, &hv_state, now); address_space_unmap(CPU(cpu)->as, regs, len, len, false); - env->cfar = hv_state.cfar; - assert(env->spr[SPR_LPIDR] == 0); env->spr[SPR_LPIDR] = hv_state.lpid; - lpcr_mask = LPCR_DPFD | LPCR_ILE | LPCR_AIL | LPCR_LD | LPCR_MER; - lpcr = (env->spr[SPR_LPCR] & ~lpcr_mask) | (hv_state.lpcr & lpcr_mask); - lpcr |= LPCR_HR | LPCR_UPRT | LPCR_GTSE | LPCR_HVICE | LPCR_HDICE; - lpcr &= ~LPCR_LPES0; - env->spr[SPR_LPCR] = lpcr & pcc->lpcr_mask; - - env->spr[SPR_PCR] = hv_state.pcr; - /* hv_state.amor is not used */ - env->spr[SPR_DPDES] = hv_state.dpdes; - env->spr[SPR_HFSCR] = hv_state.hfscr; - hdec = hv_state.hdec_expiry - now; spapr_cpu->nested_tb_offset = hv_state.tb_offset; - /* TCG does not implement DAWR*, CIABR, PURR, SPURR, IC, VTB, HEIR SPRs*/ - 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; - - cpu_ppc_hdecr_init(env); - cpu_ppc_store_hdecr(env, hdec); /* * The hv_state.vcpu_token is not needed. It is used by the KVM
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 | 99 +++++++++++++++++++++++++++----------------- 1 file changed, 60 insertions(+), 39 deletions(-)