On Wed, May 26, 2021 at 06:03:09PM +0200, Greg Kurz wrote:
> On Wed, 26 May 2021 19:16:25 +1000
> Nicholas Piggin <npig...@gmail.com> wrote:
>
> > TCG does not keep track of AIL mode in a central place, it's based on
> > the current LPCR[AIL] bits. Synchronize the new CPU's LPCR to the
> > current LPCR in rtas_start_cpu(), similarly to the way the ILE bit is
> > synchronized.
> >
> > Open-code the ILE setting as well now that the caller's LPCR is
> > available directly, there is no need for the indirection.
> >
> > Without this, under both TCG and KVM, adding a POWER8/9/10 class CPU
> > with a new core ID after a modern Linux has booted results in the new
> > CPU's LPCR missing the LPCR[AIL]=0b11 setting that the other CPUs have.
> > This can cause crashes and unexpected behaviour.
> >
> > Signed-off-by: Nicholas Piggin <npig...@gmail.com>
> > ---
> > hw/ppc/spapr_rtas.c | 14 +++++++++-----
> > 1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> > index 63d96955c0..b476382ae6 100644
> > --- a/hw/ppc/spapr_rtas.c
> > +++ b/hw/ppc/spapr_rtas.c
> > @@ -132,8 +132,8 @@ static void rtas_start_cpu(PowerPCCPU *callcpu,
> > SpaprMachineState *spapr,
> > target_ulong id, start, r3;
> > PowerPCCPU *newcpu;
> > CPUPPCState *env;
> > - PowerPCCPUClass *pcc;
> > target_ulong lpcr;
> > + target_ulong caller_lpcr;
> >
> > if (nargs != 3 || nret != 1) {
> > rtas_st(rets, 0, RTAS_OUT_PARAM_ERROR);
> > @@ -152,7 +152,6 @@ static void rtas_start_cpu(PowerPCCPU *callcpu,
> > SpaprMachineState *spapr,
> > }
> >
> > env = &newcpu->env;
> > - pcc = POWERPC_CPU_GET_CLASS(newcpu);
> >
> > if (!CPU(newcpu)->halted) {
> > rtas_st(rets, 0, RTAS_OUT_HW_ERROR);
> > @@ -164,10 +163,15 @@ static void rtas_start_cpu(PowerPCCPU *callcpu,
> > SpaprMachineState *spapr,
> > env->msr = (1ULL << MSR_SF) | (1ULL << MSR_ME);
> > hreg_compute_hflags(env);
> >
> > + caller_lpcr = callcpu->env.spr[SPR_LPCR];
> > lpcr = env->spr[SPR_LPCR];
> > - if (!pcc->interrupts_big_endian(callcpu)) {
> > - lpcr |= LPCR_ILE;
> > - }
> > +
> > + /* Set ILE the same way */
> > + lpcr = (lpcr & ~LPCR_ILE) | (caller_lpcr & LPCR_ILE);
> > +
>
> Unrelated change as Cedric already noted but that's nice :)
>
> /me starting to think we might do the same elsewhere and
> maybe get rid of PowerPCCPUClass::interrupts_big_endian()
Yes, that's a nice thought.
> > + /* Set AIL the same way */
> > + lpcr = (lpcr & ~LPCR_AIL) | (caller_lpcr & LPCR_AIL);
> > +
>
> It seems better indeed to rely on the calling CPU here rather
> than the arbitrary first_cpu in the hotplug handler.
I agree.
> Reviewed-by: Greg Kurz <gr...@kaod.org>
Applied to ppc-for-6.1, thanks.
>
> > if (env->mmu_model == POWERPC_MMU_3_00) {
> > /*
> > * New cpus are expected to start in the same radix/hash mode
>
--
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
