| Message ID | 20210526091626.3388262-3-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ppc: LPCR synchronisation fixes | expand |
On 5/26/21 11:16 AM, Nicholas Piggin 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. That's two patches in one but no big deal. > > 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 <npiggin@gmail.com> Reviewed-by: Cédric Le Goater <clg@kaod.org> C. > --- > 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); > + > + /* Set AIL the same way */ > + lpcr = (lpcr & ~LPCR_AIL) | (caller_lpcr & LPCR_AIL); > + > if (env->mmu_model == POWERPC_MMU_3_00) { > /* > * New cpus are expected to start in the same radix/hash mode >
On Wed, 26 May 2021 19:16:25 +1000 Nicholas Piggin <npiggin@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 <npiggin@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() > + /* 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. Reviewed-by: Greg Kurz <groug@kaod.org> > if (env->mmu_model == POWERPC_MMU_3_00) { > /* > * New cpus are expected to start in the same radix/hash mode
On Wed, May 26, 2021 at 06:03:09PM +0200, Greg Kurz wrote: > On Wed, 26 May 2021 19:16:25 +1000 > Nicholas Piggin <npiggin@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 <npiggin@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 <groug@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 >
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); + + /* Set AIL the same way */ + lpcr = (lpcr & ~LPCR_AIL) | (caller_lpcr & LPCR_AIL); + if (env->mmu_model == POWERPC_MMU_3_00) { /* * New cpus are expected to start in the same radix/hash mode
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 <npiggin@gmail.com> --- hw/ppc/spapr_rtas.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)