Re: [Qemu-ppc] [PATCH 3/8] spapr: Remove unhelpful helpers from rtas_start_cpu()

[David Gibson]

On Thu, May 03, 2018 at 06:34:16PM +0200, Greg Kurz wrote:
> On Thu,  3 May 2018 16:21:40 +1000
> David Gibson <address@hidden> wrote:
> 
> > rtas_start_cpu() calls spapr_cpu_update_tb_offset() and
> > spapr_cpu_set_endianness() to initialize certain things in the new cpu's
> > state.  This is the only caller of those helpers, and they're each only
> > a few lines long, so we might as well just fold them into the caller.
> > 
> > In addition, those helpers initialize state on the new cpu to match that of
> > the first cpu.  That will generally work, but might be at least logically
> > incorrect if the first cpu has been set offline by the guest.  So, instead
> > base the state on that of the cpu invoking the RTAS call, which is
> > obviously active already.
> > 
> 
> Much better indeed !
> 
> > Signed-off-by: David Gibson <address@hidden>
> > ---
> >  hw/ppc/spapr_rtas.c | 38 ++++++++++++++------------------------
> >  1 file changed, 14 insertions(+), 24 deletions(-)
> > 
> > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > index b251c130cb..df073447c5 100644
> > --- a/hw/ppc/spapr_rtas.c
> > +++ b/hw/ppc/spapr_rtas.c
> > @@ -120,27 +120,6 @@ static void rtas_query_cpu_stopped_state(PowerPCCPU 
> > *cpu_,
> >      rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> >  }
> >  
> > -/*
> > - * Set the timebase offset of the CPU to that of first CPU.
> > - * This helps hotplugged CPU to have the correct timebase offset.
> > - */
> > -static void spapr_cpu_update_tb_offset(PowerPCCPU *cpu)
> > -{
> > -    PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
> > -
> > -    cpu->env.tb_env->tb_offset = fcpu->env.tb_env->tb_offset;
> > -}
> > -
> > -static void spapr_cpu_set_endianness(PowerPCCPU *cpu)
> > -{
> > -    PowerPCCPU *fcpu = POWERPC_CPU(first_cpu);
> > -    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(fcpu);
> > -
> > -    if (!pcc->interrupts_big_endian(fcpu)) {
> > -        cpu->env.spr[SPR_LPCR] |= LPCR_ILE;
> > -    }
> > -}
> > -
> >  static void rtas_start_cpu(PowerPCCPU *callcpu, sPAPRMachineState *spapr,
> >                             uint32_t token, uint32_t nargs,
> >                             target_ulong args,
> > @@ -150,6 +129,7 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, 
> > sPAPRMachineState *spapr,
> >      PowerPCCPU *newcpu;
> >      CPUPPCState *env;
> >      PowerPCCPUClass *pcc;
> > +    target_ulong lpcr;
> >  
> >      if (nargs != 3 || nret != 1) {
> >          rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > @@ -178,10 +158,20 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, 
> > sPAPRMachineState *spapr,
> >      cpu_synchronize_state(CPU(newcpu));
> >  
> >      env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> > -    spapr_cpu_set_endianness(newcpu);
> > -    spapr_cpu_update_tb_offset(newcpu);
> > +
> >      /* Enable Power-saving mode Exit Cause exceptions for the new CPU */
> > -    ppc_store_lpcr(newcpu, env->spr[SPR_LPCR] | pcc->lpcr_pm);
> > +    lpcr = env->spr[SPR_LPCR] | pcc->lpcr_pm;
> > +    if (!pcc->interrupts_big_endian(callcpu)) {
> > +        lpcr |= LPCR_ILE;
> > +    }
> > +    ppc_store_lpcr(newcpu, lpcr);
> > +
> > +    /*
> > +     * Set the timebase offset of the new CPU to that of the invoking
> > +     * CPU.  This helps hotplugged CPU to have the correct timebase
>                ^
> Extraneous space here

Oh my, the can of worms you've opened with that apparently innocuous
comment :)
    https://en.wikipedia.org/wiki/Sentence_spacing#Controversy

tl;dr: I have a habit of putting a double space after full stops.  The
merits of that are highly debatable, but there it is.

> > +     * offset.
> > +     */
> > +    newcpu->env.tb_env->tb_offset = callcpu->env.tb_env->tb_offset;
> >  
> 
> Maybe use env-> instead of newcpu->env. ?

That is better, but I'm already most of the way through my
pre-pull-request testing, so I don't want to risk any code changes
(although I still fold the new R-bs into the commit messages).

> 
> Anyway,
> 
> Reviewed-by: Greg Kurz <address@hidden>
> 
> >      env->nip = start;
> >      env->gpr[3] = r3;
> 

-- 
David Gibson                    | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
                                | _way_ _around_!
http://www.ozlabs.org/~dgibson

signature.asc
Description: PGP signature