Re: [PATCH 2/5] ppc: spapr: cleanup h_enter_nested() with helper routines.

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

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