| Message ID | 20180503062145.17899-4-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | spapr: Cleanups to startup and LPCR handling | expand |
On 05/03/2018 08:21 AM, David Gibson 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. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> Reviewed-by: Cédric Le Goater <clg@kaod.org> > --- > 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 > + * offset. > + */ > + newcpu->env.tb_env->tb_offset = callcpu->env.tb_env->tb_offset; > > env->nip = start; > env->gpr[3] = r3; >
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 > + * offset. > + */ > + newcpu->env.tb_env->tb_offset = callcpu->env.tb_env->tb_offset; > Maybe use env-> instead of newcpu->env. ? Anyway, Reviewed-by: Greg Kurz <groug@kaod.org> > env->nip = start; > env->gpr[3] = r3;
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 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 <groug@kaod.org> > > > env->nip = start; > > env->gpr[3] = r3; >
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; > > >
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 + * offset. + */ + newcpu->env.tb_env->tb_offset = callcpu->env.tb_env->tb_offset; env->nip = start; env->gpr[3] = r3;
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. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/ppc/spapr_rtas.c | 38 ++++++++++++++------------------------ 1 file changed, 14 insertions(+), 24 deletions(-)