[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-ppc] [PATCH 3/8] spapr: Remove unhelpful helpers from rtas_sta
From: |
Greg Kurz |
Subject: |
Re: [Qemu-ppc] [PATCH 3/8] spapr: Remove unhelpful helpers from rtas_start_cpu() |
Date: |
Fri, 4 May 2018 08:43:53 +0200 |
On Fri, 4 May 2018 15:00:20 +1000
David Gibson <address@hidden> 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 <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
>
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 <address@hidden>
> >
> > > env->nip = start;
> > > env->gpr[3] = r3;
> >
>
pgpNnQ2jYs5gO.pgp
Description: OpenPGP digital signature