Re: [RFC PATCH v2 27/29] target/ppc: introduce ppc_maybe_interrupt

[Fabiano Rosas <farosas@linux.ibm.com>]

Matheus Ferst <matheus.ferst@eldorado.org.br> writes:

> The method checks if any pending interrupt is unmasked and calls
> cpu_interrupt/cpu_reset_interrupt accordingly. Code that raises/lowers
> or masks/unmasks interrupts should call this method to keep
> CPU_INTERRUPT_HARD coherent with env->pending_interrupts.
>
> Signed-off-by: Matheus Ferst <matheus.ferst@eldorado.org.br>
> ---
> v2:
>   - Found many other places where ppc_maybe_interrupt had to be called
>     with the IO and kvm-nested tests that Cédric suggested.

We might need some words describing the situations in which this
function should be used to avoid new code missing it.

>   - Create a helper to call ppc_maybe_interrupt to avoid using
>     helper_store_msr in WRTEE[I].
>
> I couldn't find a better name for this method, so I used "maybe
> interrupt" just like we have "maybe bswap" for gdbstub registers.
> ---
>  hw/ppc/pnv_core.c        |  1 +
>  hw/ppc/ppc.c             |  7 +------
>  hw/ppc/spapr_hcall.c     |  6 ++++++
>  hw/ppc/spapr_rtas.c      |  2 +-
>  target/ppc/cpu.c         |  2 ++
>  target/ppc/cpu.h         |  1 +
>  target/ppc/excp_helper.c | 29 +++++++++++++++++++++++++++++
>  target/ppc/helper.h      |  1 +
>  target/ppc/helper_regs.c |  2 ++
>  target/ppc/translate.c   |  2 ++
>  10 files changed, 46 insertions(+), 7 deletions(-)
>
> diff --git a/hw/ppc/pnv_core.c b/hw/ppc/pnv_core.c
> index 19e8eb885f..9ee79192dd 100644
> --- a/hw/ppc/pnv_core.c
> +++ b/hw/ppc/pnv_core.c
> @@ -58,6 +58,7 @@ static void pnv_core_cpu_reset(PnvCore *pc, PowerPCCPU *cpu)
>      env->msr |= MSR_HVB; /* Hypervisor mode */
>      env->spr[SPR_HRMOR] = pc->hrmor;
>      hreg_compute_hflags(env);
> +    ppc_maybe_interrupt(env);
>  
>      pcc->intc_reset(pc->chip, cpu);
>  }
> diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
> index 77e611e81c..dc86c1c7db 100644
> --- a/hw/ppc/ppc.c
> +++ b/hw/ppc/ppc.c
> @@ -42,7 +42,6 @@ static void cpu_ppc_tb_start (CPUPPCState *env);
>  
>  void ppc_set_irq(PowerPCCPU *cpu, int irq, int level)
>  {
> -    CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
>      unsigned int old_pending;
>      bool locked = false;
> @@ -57,19 +56,15 @@ void ppc_set_irq(PowerPCCPU *cpu, int irq, int level)
>  
>      if (level) {
>          env->pending_interrupts |= irq;
> -        cpu_interrupt(cs, CPU_INTERRUPT_HARD);
>      } else {
>          env->pending_interrupts &= ~irq;
> -        if (env->pending_interrupts == 0) {
> -            cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
> -        }
>      }
>  
>      if (old_pending != env->pending_interrupts) {
> +        ppc_maybe_interrupt(env);
>          kvmppc_set_interrupt(cpu, irq, level);
>      }
>  
> -
>      trace_ppc_irq_set_exit(env, irq, level, env->pending_interrupts,
>                             CPU(cpu)->interrupt_request);
>  
> diff --git a/hw/ppc/spapr_hcall.c b/hw/ppc/spapr_hcall.c
> index a8d4a6bcf0..23aa41c879 100644
> --- a/hw/ppc/spapr_hcall.c
> +++ b/hw/ppc/spapr_hcall.c
> @@ -490,6 +490,7 @@ static target_ulong h_cede(PowerPCCPU *cpu, SpaprMachineState *spapr,
>  
>      env->msr |= (1ULL << MSR_EE);
>      hreg_compute_hflags(env);
> +    ppc_maybe_interrupt(env);
>  
>      if (spapr_cpu->prod) {
>          spapr_cpu->prod = false;
> @@ -500,6 +501,7 @@ static target_ulong h_cede(PowerPCCPU *cpu, SpaprMachineState *spapr,
>          cs->halted = 1;
>          cs->exception_index = EXCP_HLT;
>          cs->exit_request = 1;
> +        ppc_maybe_interrupt(env);
>      }
>  
>      return H_SUCCESS;
> @@ -521,6 +523,7 @@ static target_ulong h_confer_self(PowerPCCPU *cpu)
>      cs->halted = 1;
>      cs->exception_index = EXCP_HALTED;
>      cs->exit_request = 1;
> +    ppc_maybe_interrupt(&cpu->env);
>  
>      return H_SUCCESS;
>  }
> @@ -633,6 +636,7 @@ static target_ulong h_prod(PowerPCCPU *cpu, SpaprMachineState *spapr,
>      spapr_cpu = spapr_cpu_state(tcpu);
>      spapr_cpu->prod = true;
>      cs->halted = 0;
> +    ppc_maybe_interrupt(&cpu->env);
>      qemu_cpu_kick(cs);
>  
>      return H_SUCCESS;
> @@ -1661,6 +1665,7 @@ static target_ulong h_enter_nested(PowerPCCPU *cpu,
>      spapr_cpu->in_nested = true;
>  
>      hreg_compute_hflags(env);
> +    ppc_maybe_interrupt(env);
>      tlb_flush(cs);
>      env->reserve_addr = -1; /* Reset the reservation */
>  
> @@ -1802,6 +1807,7 @@ out_restore_l1:
>      spapr_cpu->in_nested = false;
>  
>      hreg_compute_hflags(env);
> +    ppc_maybe_interrupt(env);
>      tlb_flush(cs);
>      env->reserve_addr = -1; /* Reset the reservation */
>  
> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c
> index d58b65e88f..3f664ea02c 100644
> --- a/hw/ppc/spapr_rtas.c
> +++ b/hw/ppc/spapr_rtas.c
> @@ -214,9 +214,9 @@ static void rtas_stop_self(PowerPCCPU *cpu, SpaprMachineState *spapr,
>       * 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;
> +    ppc_store_lpcr(cpu, env->spr[SPR_LPCR] & ~pcc->lpcr_pm);
>      kvmppc_set_reg_ppc_online(cpu, 0);
>      qemu_cpu_kick(cs);
>  }
> diff --git a/target/ppc/cpu.c b/target/ppc/cpu.c
> index e95b4c5ee1..1a97b41c6b 100644
> --- a/target/ppc/cpu.c
> +++ b/target/ppc/cpu.c
> @@ -82,6 +82,8 @@ void ppc_store_lpcr(PowerPCCPU *cpu, target_ulong val)
>      env->spr[SPR_LPCR] = val & pcc->lpcr_mask;
>      /* The gtse bit affects hflags */
>      hreg_compute_hflags(env);
> +
> +    ppc_maybe_interrupt(env);
>  }
>  #endif
>  
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index 7b13d4cf86..89c065521f 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1358,6 +1358,7 @@ int ppc64_cpu_write_elf64_note(WriteCoreDumpFunction f, CPUState *cs,
>  int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, CPUState *cs,
>                                 int cpuid, void *opaque);
>  #ifndef CONFIG_USER_ONLY
> +void ppc_maybe_interrupt(CPUPPCState *env);
>  void ppc_cpu_do_interrupt(CPUState *cpu);
>  bool ppc_cpu_exec_interrupt(CPUState *cpu, int int_req);
>  void ppc_cpu_do_system_reset(CPUState *cs);
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 497a9889d1..9708f82b30 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -390,6 +390,7 @@ static void powerpc_set_excp_state(PowerPCCPU *cpu, target_ulong vector,
>      env->nip = vector;
>      env->msr = msr;
>      hreg_compute_hflags(env);
> +    ppc_maybe_interrupt(env);
>  
>      powerpc_reset_excp_state(cpu);
>  
> @@ -2039,6 +2040,27 @@ static int ppc_next_unmasked_interrupt(CPUPPCState *env)
>      }
>  }
>  
> +void ppc_maybe_interrupt(CPUPPCState *env)
> +{
> +    CPUState *cs = env_cpu(env);
> +    bool locked = false;
> +
> +    if (!qemu_mutex_iothread_locked()) {
> +        locked = true;
> +        qemu_mutex_lock_iothread();
> +    }
> +
> +    if (ppc_next_unmasked_interrupt(env)) {
> +        cpu_interrupt(cs, CPU_INTERRUPT_HARD);
> +    } else {
> +        cpu_reset_interrupt(cs, CPU_INTERRUPT_HARD);
> +    }
> +
> +    if (locked) {
> +        qemu_mutex_unlock_iothread();
> +    }
> +}
> +
>  #if defined(TARGET_PPC64)
>  static void p7_deliver_interrupt(CPUPPCState *env, int interrupt)
>  {
> @@ -2474,6 +2496,11 @@ void helper_store_msr(CPUPPCState *env, target_ulong val)
>      }
>  }
>  
> +void helper_ppc_maybe_interrupt(CPUPPCState *env)
> +{
> +    ppc_maybe_interrupt(env);
> +}
> +
>  #if defined(TARGET_PPC64)
>  void helper_scv(CPUPPCState *env, uint32_t lev)
>  {
> @@ -2494,6 +2521,8 @@ void helper_pminsn(CPUPPCState *env, powerpc_pm_insn_t insn)
>      /* Condition for waking up at 0x100 */
>      env->resume_as_sreset = (insn != PPC_PM_STOP) ||
>          (env->spr[SPR_PSSCR] & PSSCR_EC);
> +
> +    ppc_maybe_interrupt(env);
>  }
>  #endif /* defined(TARGET_PPC64) */
>  
> diff --git a/target/ppc/helper.h b/target/ppc/helper.h
> index 57eee07256..3d09aae5fc 100644
> --- a/target/ppc/helper.h
> +++ b/target/ppc/helper.h
> @@ -10,6 +10,7 @@ DEF_HELPER_4(HASHSTP, void, env, tl, tl, tl)
>  DEF_HELPER_4(HASHCHKP, void, env, tl, tl, tl)
>  #if !defined(CONFIG_USER_ONLY)
>  DEF_HELPER_2(store_msr, void, env, tl)
> +DEF_HELPER_1(ppc_maybe_interrupt, void, env)
>  DEF_HELPER_1(rfi, void, env)
>  DEF_HELPER_1(40x_rfci, void, env)
>  DEF_HELPER_1(rfci, void, env)
> diff --git a/target/ppc/helper_regs.c b/target/ppc/helper_regs.c
> index 12235ea2e9..2e85e124ab 100644
> --- a/target/ppc/helper_regs.c
> +++ b/target/ppc/helper_regs.c
> @@ -260,6 +260,8 @@ int hreg_store_msr(CPUPPCState *env, target_ulong value, int alter_hv)
>      env->msr = value;
>      hreg_compute_hflags(env);
>  #if !defined(CONFIG_USER_ONLY)
> +    ppc_maybe_interrupt(env);
> +
>      if (unlikely(FIELD_EX64(env->msr, MSR, POW))) {
>          if (!env->pending_interrupts && (*env->check_pow)(env)) {
>              cs->halted = 1;
> diff --git a/target/ppc/translate.c b/target/ppc/translate.c
> index e810842925..e8336452c4 100644
> --- a/target/ppc/translate.c
> +++ b/target/ppc/translate.c
> @@ -6175,6 +6175,7 @@ static void gen_wrtee(DisasContext *ctx)
>      tcg_gen_andi_tl(t0, cpu_gpr[rD(ctx->opcode)], (1 << MSR_EE));
>      tcg_gen_andi_tl(cpu_msr, cpu_msr, ~(1 << MSR_EE));
>      tcg_gen_or_tl(cpu_msr, cpu_msr, t0);
> +    gen_helper_ppc_maybe_interrupt(cpu_env);
>      tcg_temp_free(t0);
>      /*
>       * Stop translation to have a chance to raise an exception if we
> @@ -6193,6 +6194,7 @@ static void gen_wrteei(DisasContext *ctx)
>      CHK_SV(ctx);
>      if (ctx->opcode & 0x00008000) {
>          tcg_gen_ori_tl(cpu_msr, cpu_msr, (1 << MSR_EE));
> +        gen_helper_ppc_maybe_interrupt(cpu_env);
>          /* Stop translation to have a chance to raise an exception */
>          ctx->base.is_jmp = DISAS_EXIT_UPDATE;
>      } else {