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

[Greg Kurz <groug@kaod.org>]

[-- Attachment #1: Type: text/plain, Size: 4726 bytes --]

On Fri, 4 May 2018 15:00:20 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, May 03, 2018 at 06:34:16PM +0200, Greg Kurz wrote:
> > On Thu,  3 May 2018 16:21:40 +1000
> > David Gibson <david@gibson.dropbear.id.au> 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 <david@gibson.dropbear.id.au>
> > > ---
> > >  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
> 

Ha ha ! I now have an impression of deja-vu, probably on this very same
list :D

> 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.
> 

Sure, I'll try to remember that the so-called French spacing doesn't
rule the world... yet ;)


> > > +     * 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).
> 

Understandable.

> > 
> > Anyway,
> > 
> > Reviewed-by: Greg Kurz <groug@kaod.org>
> >   
> > >      env->nip = start;
> > >      env->gpr[3] = r3;  
> >   
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]