Re: [PATCH 3/4] ppc/spapr: load and store l2 state with helper functions

[Nicholas Piggin]

On Fri Jun 9, 2023 at 6:00 PM AEST, Harsh Prateek Bora wrote:
>
>
> On 6/8/23 14:43, Nicholas Piggin wrote:
> > Arguably this is just shuffling around register accesses, but one nice
> > thing it does is allow the exit to save away the L2 state then switch
> > the environment to the L1 before copying L2 data back to the L1, which
> > logically flows more naturally and simplifies the error paths.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >   hw/ppc/spapr_hcall.c | 164 ++++++++++++++++++++++---------------------
> >   1 file changed, 85 insertions(+), 79 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> > index d5b8d54692..da6440f235 100644
> > --- a/hw/ppc/spapr_hcall.c
> > +++ b/hw/ppc/spapr_hcall.c
> > @@ -1663,9 +1663,9 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
> >                                      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);
> > +    struct nested_ppc_state l2_state;
> >       target_ulong hv_ptr = args[0];
> >       target_ulong regs_ptr = args[1];
> >       target_ulong hdec, now = cpu_ppc_load_tbl(env);
> > @@ -1699,6 +1699,10 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
> >           return H_PARAMETER;
> >       }
> >   
> > +    if (hv_state.lpid == 0) {
> > +        return H_PARAMETER;
> > +    }
> > +
> >       spapr_cpu->nested_host_state = g_try_new(struct nested_ppc_state, 1);
> >       if (!spapr_cpu->nested_host_state) {
> >           return H_NO_MEM;
> > @@ -1717,46 +1721,49 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
> >           return H_P2;
> >       }
> >   
> > -    len = sizeof(env->gpr);
> > +    len = sizeof(l2_state.gpr);
> >       assert(len == sizeof(regs->gpr));
> > -    memcpy(env->gpr, regs->gpr, len);
> > +    memcpy(l2_state.gpr, regs->gpr, len);
> >   
> > -    env->lr = regs->link;
> > -    env->ctr = regs->ctr;
> > -    cpu_write_xer(env, regs->xer);
> > -    ppc_set_cr(env, regs->ccr);
> > -
> > -    env->msr = regs->msr;
> > -    env->nip = regs->nip;
> > +    l2_state.lr = regs->link;
> > +    l2_state.ctr = regs->ctr;
> > +    l2_state.xer = regs->xer;
> > +    l2_state.cr = regs->ccr;
> > +    l2_state.msr = regs->msr;
> > +    l2_state.nip = regs->nip;
> >   
> >       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;
> > +    l2_state.cfar = hv_state.cfar;
> > +    l2_state.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;
> > +    l2_state.lpcr = lpcr & pcc->lpcr_mask;
> >   
> > -    env->spr[SPR_PCR] = hv_state.pcr;
> > +    l2_state.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;
> > +    l2_state.dpdes = hv_state.dpdes;
> > +    l2_state.hfscr = hv_state.hfscr;
> >       /* 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;
> > +    l2_state.srr0 = hv_state.srr0;
> > +    l2_state.srr1 = hv_state.srr1;
> > +    l2_state.sprg0 = hv_state.sprg[0];
> > +    l2_state.sprg1 = hv_state.sprg[1];
> > +    l2_state.sprg2 = hv_state.sprg[2];
> > +    l2_state.sprg3 = hv_state.sprg[3];
> > +    l2_state.pidr = hv_state.pidr;
> > +    l2_state.ppr = hv_state.ppr;
> > +    l2_state.tb_offset = env->tb_env->tb_offset + hv_state.tb_offset;
> > +
> > +    /*
> > +     * Switch to the nested guest environment and start the "hdec" timer.
> > +     */
> > +    nested_load_state(cpu, &l2_state);
> >   
> > +    hdec = hv_state.hdec_expiry - now;
> >       cpu_ppc_hdecr_init(env);
> >       cpu_ppc_store_hdecr(env, hdec);
> >   
> > @@ -1772,14 +1779,8 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
> >        * and it's not obviously worth a new data structure to do it.
> >        */
> >   
> > -    env->tb_env->tb_offset += hv_state.tb_offset;
> >       spapr_cpu->in_nested = true;
> >   
> > -    hreg_compute_hflags(env);
> > -    ppc_maybe_interrupt(env);
> > -    tlb_flush(cs);
> > -    env->reserve_addr = -1; /* Reset the reservation */
> > -
> >       /*
> >        * The spapr hcall helper sets env->gpr[3] to the return value, but at
> >        * this point the L1 is not returning from the hcall but rather we
> > @@ -1793,49 +1794,69 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
> >   {
> >       CPUPPCState *env = &cpu->env;
> >       SpaprCpuState *spapr_cpu = spapr_cpu_state(cpu);
> > -    target_ulong r3_return = env->excp_vectors[excp]; /* hcall return 
> > value */
> > +    struct nested_ppc_state l2_state;
> >       target_ulong hv_ptr = spapr_cpu->nested_host_state->gpr[4];
> >       target_ulong regs_ptr = spapr_cpu->nested_host_state->gpr[5];
> > +    target_ulong hsrr0, hsrr1, hdar, asdr, hdsisr;
> >       struct kvmppc_hv_guest_state *hvstate;
> >       struct kvmppc_pt_regs *regs;
> >       hwaddr len;
> >   
> >       assert(spapr_cpu->in_nested);
> >   
> > +    nested_save_state(&l2_state, cpu);
> > +    hsrr0 = env->spr[SPR_HSRR0];
> > +    hsrr1 = env->spr[SPR_HSRR1];
> > +    hdar = env->spr[SPR_HDAR];
> > +    hdsisr = env->spr[SPR_HDSISR];
> > +    asdr = env->spr[SPR_ASDR];
> > +
> > +    /*
> > +     * Switch back to the host environment (including for any error).
> > +     */
> > +    assert(env->spr[SPR_LPIDR] != 0);
> > +    nested_load_state(cpu, spapr_cpu->nested_host_state);
> > +    env->gpr[3] = env->excp_vectors[excp]; /* hcall return value */
> > +
> >       cpu_ppc_hdecr_exit(env);
> >   
> > +    spapr_cpu->in_nested = false;
> > +
> > +    g_free(spapr_cpu->nested_host_state);
> > +    spapr_cpu->nested_host_state = NULL;
> > +
> >       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;
> > +        env->gpr[3] = H_PARAMETER;
> > +   return;
> >       }
> >   
> > -    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 = l2_state.cfar;
> > +    hvstate->lpcr = l2_state.lpcr;
> > +    hvstate->pcr = l2_state.pcr;
> > +    hvstate->dpdes = l2_state.dpdes;
> > +    hvstate->hfscr = l2_state.hfscr;
> >   
> >       if (excp == POWERPC_EXCP_HDSI) {
> > -        hvstate->hdar = env->spr[SPR_HDAR];
> > -        hvstate->hdsisr = env->spr[SPR_HDSISR];
> > -        hvstate->asdr = env->spr[SPR_ASDR];
> > +        hvstate->hdar = hdar;
> > +        hvstate->hdsisr = hdsisr;
> > +        hvstate->asdr = asdr;
> >       } else if (excp == POWERPC_EXCP_HISI) {
> > -        hvstate->asdr = env->spr[SPR_ASDR];
> > +        hvstate->asdr = 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];
> > +    hvstate->srr0 = l2_state.srr0;
> > +    hvstate->srr1 = l2_state.srr1;
> > +    hvstate->sprg[0] = l2_state.sprg0;
> > +    hvstate->sprg[1] = l2_state.sprg1;
> > +    hvstate->sprg[2] = l2_state.sprg2;
> > +    hvstate->sprg[3] = l2_state.sprg3;
> > +    hvstate->pidr = l2_state.pidr;
> > +    hvstate->ppr = l2_state.ppr;
> >   
> >       /* Is it okay to specify write length larger than actual data 
> > written? */
> >       address_space_unmap(CPU(cpu)->as, hvstate, len, len, true);
> > @@ -1845,46 +1866,31 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
> >                                   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;
> > +        env->gpr[3] = H_P2;
> > +   return;
> >       }
> >   
> >       len = sizeof(env->gpr);
> >       assert(len == sizeof(regs->gpr));
> > -    memcpy(regs->gpr, env->gpr, len);
> > +    memcpy(regs->gpr, l2_state.gpr, len);
> >   
> > -    regs->link = env->lr;
> > -    regs->ctr = env->ctr;
> > -    regs->xer = cpu_read_xer(env);
> > -    regs->ccr = ppc_get_cr(env);
> > +    regs->link = l2_state.lr;
> > +    regs->ctr = l2_state.ctr;
> > +    regs->xer = l2_state.xer;
> > +    regs->ccr = l2_state.cr;
> >   
> >       if (excp == POWERPC_EXCP_MCHECK ||
> >           excp == POWERPC_EXCP_RESET ||
> >           excp == POWERPC_EXCP_SYSCALL) {
> > -        regs->nip = env->spr[SPR_SRR0];
> > -        regs->msr = env->spr[SPR_SRR1] & env->msr_mask;
> > +        regs->nip = l2_state.srr0;
> > +        regs->msr = l2_state.srr1 & env->msr_mask;
> >       } else {
> > -        regs->nip = env->spr[SPR_HSRR0];
> > -        regs->msr = env->spr[SPR_HSRR1] & env->msr_mask;
> > +        regs->nip = hsrr0;
> > +        regs->msr = hsrr1 & env->msr_mask;
> >       }
> >   
>
> Can the copying of L2 data to L1 be abstraced with an inline helper
> routine as well ? Then the spapr_exit_nested will look more neat:
>   - nested_save_state(l2)
>   - nested_load_state(host)
>   - copy_l2_state_to_l1() <=== something like this?

I did start to look at that, but I thought having the code to move
the data in and out of the actual the hcall API format in the
hcall functions themselves worked okay.

If there could be some streamlining or code sharing with the v2
nested-HV API, I wouldn't mind that but would probably like to see
it done as part of the v2 patches so we can see how it works.

Thanks,
Nick