Re: [Qemu-devel] [PATCH] target/ppc: Set PSSCR_EC on cpu halt to prevent spurious wakeup

[David Gibson]

On Thu, May 16, 2019 at 10:57:44AM +1000, Suraj Jitindar Singh wrote:
> The processor stop status and control register (PSSCR) is used to
> control the power saving facilities of the thread. The exit criterion
> bit (EC) is used to specify whether the thread should be woken by any
> interrupt (EC == 0) or only an interrupt enabled in the LPCR to wake the
> thread (EC == 1).
> 
> The rtas facilities start-cpu and self-stop are used to transition a
> vcpu between the stopped and running states. When a vcpu is stopped it
> may only be started again by the start-cpu rtas call.
> 
> Currently a vcpu in the stopped state will start again whenever an
> interrupt comes along due to PSSCR_EC being cleared, and while this is
> architecturally correct for a hardware thread, a vcpu is expected to
> only be woken by calling start-cpu. This means when performing a reboot
> on a tcg machine that the secondary threads will restart while the
> primary is still in slof, this is unsupported and causes call traces
> like:
> 
> SLOF **********************************************************************
> QEMU Starting
>  Build Date = Jan 14 2019 18:00:39
>  FW Version = git-a5b428e1c1eae703
>  Press "s" to enter Open Firmware.
> 
> qemu: fatal: Trying to deliver HV exception (MSR) 70 with no HV support
> 
> NIP 6d61676963313230   LR 000000003dbe0308 CTR 6d61676963313233 XER 
> 0000000000000000 CPU#1
> MSR 0000000000000000 HID0 0000000000000000  HF 0000000000000000 iidx 3 didx 3
> TB 00000026 115746031956 DECR 18446744073326238463
> GPR00 000000003dbe0308 000000003e669fe0 000000003dc10700 0000000000000003
> GPR04 000000003dc62198 000000003dc62178 000000003dc0ea48 0000000000000030
> GPR08 000000003dc621a8 0000000000000018 000000003e466008 000000003dc50700
> GPR12 c00000000093a4e0 c00000003ffff300 c00000003e533f90 0000000000000000
> GPR16 0000000000000000 0000000000000000 000000003e466010 000000003dc0b040
> GPR20 0000000000008000 000000000000f003 0000000000000006 000000003e66a050
> GPR24 000000003dc06400 000000003dc0ae70 0000000000000003 000000000000f001
> GPR28 000000003e66a060 ffffffffffffffff 6d61676963313233 0000000000000028
> CR 28000222  [ E  L  -  -  -  E  E  E  ]             RES ffffffffffffffff
> FPR00 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> FPR04 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> FPR08 0000000000000000 0000000000000000 0000000000000000 00000000311825e0
> FPR12 00000000311825e0 0000000000000000 0000000000000000 0000000000000000
> FPR16 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> FPR20 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> FPR24 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> FPR28 0000000000000000 0000000000000000 0000000000000000 0000000000000000
> FPSCR 0000000000000000
>  SRR0 000000003dbe06b0  SRR1 0000000000080000    PVR 00000000004e1200 VRSAVE 
> 0000000000000000
> SPRG0 000000003dbe0308 SPRG1 000000003e669fe0  SPRG2 00000000000000d8  SPRG3 
> 000000003dbe0308
> SPRG4 0000000000000000 SPRG5 0000000000000000  SPRG6 0000000000000000  SPRG7 
> 0000000000000000
> HSRR0 6d61676963313230 HSRR1 0000000000000000
>  CFAR 000000003dbe3e64
>  LPCR 0000000004020008
>  PTCR 0000000000000000   DAR 0000000000000000  DSISR 0000000000000000
> Aborted (core dumped)
> 
> To fix this, set the PSSCR_EC bit when a vcpu is stopped to disable it
> from coming back online until the start-cpu rtas call is made.
> 
> Fixes: 21c0d66a9c99 ("target/ppc: Fix support for "STOP light" states on 
> POWER9")
> 
> Signed-off-by: Suraj Jitindar Singh <address@hidden>

Applied, thanks.

> ---
>  hw/ppc/spapr_cpu_core.c | 2 ++
>  hw/ppc/spapr_rtas.c     | 6 +++++-
>  2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/ppc/spapr_cpu_core.c b/hw/ppc/spapr_cpu_core.c
> index f04e06cdf6..5621fb9a3d 100644
> --- a/hw/ppc/spapr_cpu_core.c
> +++ b/hw/ppc/spapr_cpu_core.c
> @@ -58,9 +58,11 @@ static void spapr_cpu_reset(void *opaque)
>       *
>       * Disable Power-saving mode Exit Cause exceptions for the CPU, so
>       * we don't get spurious wakups before an RTAS start-cpu call.
> +     * For the same reason, set PSSCR_EC.
>       */
>      lpcr &= ~(LPCR_VPM0 | LPCR_VPM1 | LPCR_ISL | LPCR_KBV | pcc->lpcr_pm);
>      lpcr |= LPCR_LPES0 | LPCR_LPES1;
> +    env->spr[SPR_PSSCR] |= PSSCR_EC;
>  
>      /* Set RMLS to the max (ie, 16G) */
>      lpcr &= ~LPCR_RMLS;
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index ee24212765..5bc1a93271 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -177,6 +177,7 @@ static void rtas_start_cpu(PowerPCCPU *callcpu, 
> SpaprMachineState *spapr,
>          } else {
>              lpcr &= ~(LPCR_UPRT | LPCR_GTSE | LPCR_HR);
>          }
> +        env->spr[SPR_PSSCR] &= ~PSSCR_EC;
>      }
>      ppc_store_lpcr(newcpu, lpcr);
>  
> @@ -205,8 +206,11 @@ static void rtas_stop_self(PowerPCCPU *cpu, 
> SpaprMachineState *spapr,
>  
>      /* Disable Power-saving mode Exit Cause exceptions for the CPU.
>       * This could deliver an interrupt on a dying CPU and crash the
> -     * guest */
> +     * guest.
> +     * For the same reason, set PSSCR_EC.
> +     */
>      ppc_store_lpcr(cpu, env->spr[SPR_LPCR] & ~pcc->lpcr_pm);
> +    env->spr[SPR_PSSCR] |= PSSCR_EC;
>      cs->halted = 1;
>      kvmppc_set_reg_ppc_online(cpu, 0);
>      qemu_cpu_kick(cs);

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