Re: [Qemu-devel] [PATCH 09/19] target/ppc: Don't clobber MSR:EE on PM instructions

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4674 bytes --]

On Mon, Jan 28, 2019 at 10:46:15AM +0100, Cédric Le Goater wrote:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> When issuing a power management instruction, we set MSR:EE
> to force ppc_hw_interrupt() into calling powerpc_excp()
> to deal with the fact that on P7 and P8, the system reset
> caused by the wakeup needs to be generated regardless of
> the MSR:EE value (using LPCR only).
> 
> This however means that the OS will see a bogus SRR1:EE
> value which is a problem. It also prevents properly
> implementing P9 STOP "light".
> 
> So fix this by instead putting some logic in ppc_hw_interrupt()
> to decide whether to deliver or not by taking into account the
> fact that we are waking up from sleep.
> 
> The LPCR isn't checked as this is done in the has_work() test.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  target/ppc/excp_helper.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 8407e0ade938..7c7c8d1b9dc6 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -748,6 +748,7 @@ void ppc_cpu_do_interrupt(CPUState *cs)
>  static void ppc_hw_interrupt(CPUPPCState *env)
>  {
>      PowerPCCPU *cpu = ppc_env_get_cpu(env);
> +    bool async_deliver;
>  
>      /* External reset */
>      if (env->pending_interrupts & (1 << PPC_INTERRUPT_RESET)) {
> @@ -769,11 +770,20 @@ static void ppc_hw_interrupt(CPUPPCState *env)
>          return;
>      }
>  #endif
> +
> +    /*
> +     * For interrupts that gate on MSR:EE, we need to do something a
> +     * bit more subtle, as we need to let them through even when EE is
> +     * clear when coming out of some power management states (in order
> +     * for them to become a 0x100).
> +     */
> +    async_deliver = (msr_ee != 0) || env->in_pm_state;
> +
>      /* Hypervisor decrementer exception */
>      if (env->pending_interrupts & (1 << PPC_INTERRUPT_HDECR)) {
>          /* LPCR will be clear when not supported so this will work */
>          bool hdice = !!(env->spr[SPR_LPCR] & LPCR_HDICE);
> -        if ((msr_ee != 0 || msr_hv == 0) && hdice) {
> +        if ((async_deliver || msr_hv == 0) && hdice) {
>              /* HDEC clears on delivery */
>              env->pending_interrupts &= ~(1 << PPC_INTERRUPT_HDECR);
>              powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_HDECR);
> @@ -783,7 +793,7 @@ static void ppc_hw_interrupt(CPUPPCState *env)
>      /* Extermal interrupt can ignore MSR:EE under some circumstances */
>      if (env->pending_interrupts & (1 << PPC_INTERRUPT_EXT)) {
>          bool lpes0 = !!(env->spr[SPR_LPCR] & LPCR_LPES0);
> -        if (msr_ee != 0 || (env->has_hv_mode && msr_hv == 0 && !lpes0)) {
> +        if (async_deliver || (env->has_hv_mode && msr_hv == 0 && !lpes0)) {
>              powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_EXTERNAL);
>              return;
>          }
> @@ -795,7 +805,7 @@ static void ppc_hw_interrupt(CPUPPCState *env)
>              return;
>          }
>      }
> -    if (msr_ee != 0) {
> +    if (async_deliver != 0) {
>          /* Watchdog timer on embedded PowerPC */
>          if (env->pending_interrupts & (1 << PPC_INTERRUPT_WDT)) {
>              env->pending_interrupts &= ~(1 << PPC_INTERRUPT_WDT);
> @@ -943,21 +953,14 @@ void helper_pminsn(CPUPPCState *env, powerpc_pm_insn_t insn)
>  
>      cs = CPU(ppc_env_get_cpu(env));
>      cs->halted = 1;
> -    env->in_pm_state = true;
>  
>      /* The architecture specifies that HDEC interrupts are
>       * discarded in PM states
>       */
>      env->pending_interrupts &= ~(1 << PPC_INTERRUPT_HDECR);
>  
> -    /* Technically, nap doesn't set EE, but if we don't set it
> -     * then ppc_hw_interrupt() won't deliver. We could add some
> -     * other tests there based on LPCR but it's simpler to just
> -     * whack EE in. It will be cleared by the 0x100 at wakeup
> -     * anyway. It will still be observable by the guest in SRR1
> -     * but this doesn't seem to be a problem.
> -     */
> -    env->msr |= (1ull << MSR_EE);
> +    /* Condition for waking up at 0x100 */
> +    env->in_pm_state = true;
>  }
>  #endif /* defined(TARGET_PPC64) */
>  

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]