Re: [PATCH 2/4] ppc/spapr: Add a nested state struct

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

On 6/14/23 17:26, Nicholas Piggin wrote:
> On Fri Jun 9, 2023 at 5:09 PM AEST, Harsh Prateek Bora wrote:
>>
>>
>> On 6/8/23 14:43, Nicholas Piggin wrote:
>>> Rather than use a copy of CPUPPCState to store the host state while
>>> the environment has been switched to the L2, use a new struct for
>>> this purpose.
>>>
>>> Have helper functions to save and load this host state.
>>>
>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
>>> ---
>>>    hw/ppc/spapr_hcall.c            | 150 ++++++++++++++++++++++++--------
>>>    include/hw/ppc/spapr_cpu_core.h |   5 +-
>>>    2 files changed, 115 insertions(+), 40 deletions(-)
>>>
>>> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
>>> index 0582b524d1..d5b8d54692 100644
>>> --- a/hw/ppc/spapr_hcall.c
>>> +++ b/hw/ppc/spapr_hcall.c
>>> @@ -1546,6 +1546,112 @@ static target_ulong h_copy_tofrom_guest(PowerPCCPU *cpu,
>>>        return H_FUNCTION;
>>>    }
>>>    
>>> +struct nested_ppc_state {
>>> +    uint64_t gpr[32];
>>> +    uint64_t lr;
>>> +    uint64_t ctr;
>>> +    uint64_t cfar;
>>> +    uint64_t msr;
>>> +    uint64_t nip;
>>> +    uint32_t cr;
>>> +
>>> +    uint64_t xer;
>>> +
>>> +    uint64_t lpcr;
>>> +    uint64_t lpidr;
>>> +    uint64_t pidr;
>>> +    uint64_t pcr;
>>> +    uint64_t dpdes;
>>> +    uint64_t hfscr;
>>> +    uint64_t srr0;
>>> +    uint64_t srr1;
>>> +    uint64_t sprg0;
>>> +    uint64_t sprg1;
>>> +    uint64_t sprg2;
>>> +    uint64_t sprg3;
>>> +    uint64_t ppr;
>>> +
>>> +    int64_t tb_offset;
>>> +};
>>> +
>>
>> <snip>
>>
>>> +static void nested_load_state(PowerPCCPU *cpu, struct nested_ppc_state *load)
>>> +{
>>> +    CPUState *cs = CPU(cpu);
>>> +    CPUPPCState *env = &cpu->env;
>>> +
>>> +    memcpy(env->gpr, load->gpr, sizeof(env->gpr));
>>> +
>>> +    env->lr = load->lr;
>>> +    env->ctr = load->ctr;
>>> +    env->cfar = load->cfar;
>>> +    env->msr = load->msr;
>>> +    env->nip = load->nip;
>>> +
>>> +    ppc_set_cr(env, load->cr);
>>> +    cpu_write_xer(env, load->xer);
>>> +
>>> +    env->spr[SPR_LPCR] = load->lpcr;
>>> +    env->spr[SPR_LPIDR] = load->lpidr;
>>> +    env->spr[SPR_PCR] = load->pcr;
>>> +    env->spr[SPR_DPDES] = load->dpdes;
>>> +    env->spr[SPR_HFSCR] = load->hfscr;
>>> +    env->spr[SPR_SRR0] = load->srr0;
>>> +    env->spr[SPR_SRR1] = load->srr1;
>>> +    env->spr[SPR_SPRG0] = load->sprg0;
>>> +    env->spr[SPR_SPRG1] = load->sprg1;
>>> +    env->spr[SPR_SPRG2] = load->sprg2;
>>> +    env->spr[SPR_SPRG3] = load->sprg3;
>>> +    env->spr[SPR_BOOKS_PID] = load->pidr;
>>> +    env->spr[SPR_PPR] = load->ppr;
>>> +
>>> +    env->tb_env->tb_offset = load->tb_offset;
>>> +
>>> +    /*
>>> +     * MSR updated, compute hflags and possible interrupts.
>>> +     */
>>> +    hreg_compute_hflags(env);
>>> +    ppc_maybe_interrupt(env);
>>> +
>>> +    /*
>>> +     * Nested HV does not tag TLB entries between L1 and L2, so must
>>> +     * flush on transition.
>>> +     */
>>> +    tlb_flush(cs);
>>> +    env->reserve_addr = -1; /* Reset the reservation */
>>> +}
>>> +
>>
>> <snip>
>>
>>> @@ -1766,34 +1872,8 @@ void spapr_exit_nested(PowerPCCPU *cpu, int excp)
>>>        address_space_unmap(CPU(cpu)->as, regs, len, len, true);
>>>    
>>>    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;
>>> -    memcpy(env->crf, spapr_cpu->nested_host_state->crf, sizeof(env->crf));
>>> -    env->cfar = spapr_cpu->nested_host_state->cfar;
>>> -    env->xer = spapr_cpu->nested_host_state->xer;
>>> -    env->so = spapr_cpu->nested_host_state->so;
>>> -    env->ca = spapr_cpu->nested_host_state->ca;
>>> -    env->ov = spapr_cpu->nested_host_state->ov;
>>> -    env->ov32 = spapr_cpu->nested_host_state->ov32;
>>> -    env->ca32 = spapr_cpu->nested_host_state->ca32;
>>
>> Above fields so, ca, ov, ov32, ca32 are not taken care in
>> nested_load_state, ca being introduced in previous patch.
> 
> They should be, by cpu_write_xer.
> 
I see. In that case, do we really need the previous 1/4 patch?

Reviewed-by: Harsh Prateek Bora <harshpb@linux.ibm.com>

> Thanks,
> Nick
>