1. Patchew
  2. QEMU
  3. [PATCH v1 0/3] ppc: LPCR synchronisation fixes
  4. View patch

[PATCH v1 2/3] spapr: Set LPCR to current AIL mode when starting a new CPU

Nicholas Piggin posted 3 patches 4 years, 8 months ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Greg Kurz <groug@kaod.org>, David Gibson <david@gibson.dropbear.id.au>
[PATCH v1 2/3] spapr: Set LPCR to current AIL mode when starting a new CPU
Posted by Nicholas Piggin 4 years, 8 months ago 
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);
+
+    /* 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
-- 
2.23.0
Re: [PATCH v1 2/3] spapr: Set LPCR to current AIL mode when starting a new CPU
Posted by Cédric Le Goater 4 years, 8 months ago 
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
>
Re: [PATCH v1 2/3] spapr: Set LPCR to current AIL mode when starting a new CPU
Posted by Greg Kurz 4 years, 8 months ago 
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
Re: [PATCH v1 2/3] spapr: Set LPCR to current AIL mode when starting a new CPU
Posted by David Gibson 4 years, 8 months ago 
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
> 

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.