Re: [PATCH 4/7] ppc/spapr: Fix FWNMI machine check interrupt delivery

[David Gibson]

On Thu, Mar 05, 2020 at 02:59:35PM +1000, Nicholas Piggin wrote:
> FWNMI machine check delivery misses a few things that will make it fail
> with TCG at least (which we would like to allow in future to improve
> testing).
> 
> It's not nice to scatter interrupt delivery logic around the tree, so
> move it to excp_helper.c and share code where possible.
> 
> Signed-off-by: Nicholas Piggin <address@hidden>

Applied to ppc-for-5.0.

> ---
>  hw/ppc/spapr_events.c    | 21 ++----------
>  target/ppc/cpu.h         |  1 +
>  target/ppc/excp_helper.c | 74 ++++++++++++++++++++++++++++------------
>  3 files changed, 55 insertions(+), 41 deletions(-)
> 
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 815cfa1479..cb7bcabf61 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -788,26 +788,10 @@ static void spapr_mce_dispatch_elog(PowerPCCPU *cpu, 
> bool recovered)
>      CPUState *cs = CPU(cpu);
>      uint64_t rtas_addr;
>      CPUPPCState *env = &cpu->env;
> -    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> -    target_ulong msr = 0;
>      struct rtas_error_log log;
>      struct mc_extended_log *ext_elog;
>      uint32_t summary;
>  
> -    /*
> -     * Properly set bits in MSR before we invoke the handler.
> -     * SRR0/1, DAR and DSISR are properly set by KVM
> -     */
> -    if (!(*pcc->interrupts_big_endian)(cpu)) {
> -        msr |= (1ULL << MSR_LE);
> -    }
> -
> -    if (env->msr & (1ULL << MSR_SF)) {
> -        msr |= (1ULL << MSR_SF);
> -    }
> -
> -    msr |= (1ULL << MSR_ME);
> -
>      ext_elog = g_malloc0(sizeof(*ext_elog));
>      summary = spapr_mce_get_elog_type(cpu, recovered, ext_elog);
>  
> @@ -835,12 +819,11 @@ static void spapr_mce_dispatch_elog(PowerPCCPU *cpu, 
> bool recovered)
>      cpu_physical_memory_write(rtas_addr + RTAS_ERROR_LOG_OFFSET +
>                                sizeof(env->gpr[3]) + sizeof(log), ext_elog,
>                                sizeof(*ext_elog));
> +    g_free(ext_elog);
>  
>      env->gpr[3] = rtas_addr + RTAS_ERROR_LOG_OFFSET;
> -    env->msr = msr;
> -    env->nip = spapr->fwnmi_machine_check_addr;
>  
> -    g_free(ext_elog);
> +    ppc_cpu_do_fwnmi_machine_check(cs, spapr->fwnmi_machine_check_addr);
>  }
>  
>  void spapr_mce_req_event(PowerPCCPU *cpu, bool recovered)
> diff --git a/target/ppc/cpu.h b/target/ppc/cpu.h
> index b283042515..f8567a0621 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1232,6 +1232,7 @@ int ppc32_cpu_write_elf32_note(WriteCoreDumpFunction f, 
> CPUState *cs,
>                                 int cpuid, void *opaque);
>  #ifndef CONFIG_USER_ONLY
>  void ppc_cpu_do_system_reset(CPUState *cs);
> +void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector);
>  extern const VMStateDescription vmstate_ppc_cpu;
>  #endif
>  
> diff --git a/target/ppc/excp_helper.c b/target/ppc/excp_helper.c
> index 027f54c0ed..7f2b5899d3 100644
> --- a/target/ppc/excp_helper.c
> +++ b/target/ppc/excp_helper.c
> @@ -128,6 +128,37 @@ static uint64_t ppc_excp_vector_offset(CPUState *cs, int 
> ail)
>      return offset;
>  }
>  
> +static inline void powerpc_set_excp_state(PowerPCCPU *cpu,
> +                                          target_ulong vector, target_ulong 
> msr)
> +{
> +    CPUState *cs = CPU(cpu);
> +    CPUPPCState *env = &cpu->env;
> +
> +    /*
> +     * We don't use hreg_store_msr here as already have treated any
> +     * special case that could occur. Just store MSR and update hflags
> +     *
> +     * Note: We *MUST* not use hreg_store_msr() as-is anyway because it
> +     * will prevent setting of the HV bit which some exceptions might need
> +     * to do.
> +     */
> +    env->msr = msr & env->msr_mask;
> +    hreg_compute_hflags(env);
> +    env->nip = vector;
> +    /* Reset exception state */
> +    cs->exception_index = POWERPC_EXCP_NONE;
> +    env->error_code = 0;
> +
> +    /* Reset the reservation */
> +    env->reserve_addr = -1;
> +
> +    /*
> +     * Any interrupt is context synchronizing, check if TCG TLB needs
> +     * a delayed flush on ppc64
> +     */
> +    check_tlb_flush(env, false);
> +}
> +
>  /*
>   * Note that this function should be greatly optimized when called
>   * with a constant excp, from ppc_hw_interrupt
> @@ -768,29 +799,8 @@ static inline void powerpc_excp(PowerPCCPU *cpu, int 
> excp_model, int excp)
>          }
>      }
>  #endif
> -    /*
> -     * We don't use hreg_store_msr here as already have treated any
> -     * special case that could occur. Just store MSR and update hflags
> -     *
> -     * Note: We *MUST* not use hreg_store_msr() as-is anyway because it
> -     * will prevent setting of the HV bit which some exceptions might need
> -     * to do.
> -     */
> -    env->msr = new_msr & env->msr_mask;
> -    hreg_compute_hflags(env);
> -    env->nip = vector;
> -    /* Reset exception state */
> -    cs->exception_index = POWERPC_EXCP_NONE;
> -    env->error_code = 0;
>  
> -    /* Reset the reservation */
> -    env->reserve_addr = -1;
> -
> -    /*
> -     * Any interrupt is context synchronizing, check if TCG TLB needs
> -     * a delayed flush on ppc64
> -     */
> -    check_tlb_flush(env, false);
> +    powerpc_set_excp_state(cpu, vector, new_msr);
>  }
>  
>  void ppc_cpu_do_interrupt(CPUState *cs)
> @@ -958,6 +968,26 @@ void ppc_cpu_do_system_reset(CPUState *cs)
>  
>      powerpc_excp(cpu, env->excp_model, POWERPC_EXCP_RESET);
>  }
> +
> +void ppc_cpu_do_fwnmi_machine_check(CPUState *cs, target_ulong vector)
> +{
> +    PowerPCCPU *cpu = POWERPC_CPU(cs);
> +    CPUPPCState *env = &cpu->env;
> +    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> +    target_ulong msr = 0;
> +
> +    /*
> +     * Set MSR and NIP for the handler, SRR0/1, DAR and DSISR have already
> +     * been set by KVM.
> +     */
> +    msr = (1ULL << MSR_ME);
> +    msr |= env->msr & (1ULL << MSR_SF);
> +    if (!(*pcc->interrupts_big_endian)(cpu)) {
> +        msr |= (1ULL << MSR_LE);
> +    }
> +
> +    powerpc_set_excp_state(cpu, vector, msr);
> +}
>  #endif /* !CONFIG_USER_ONLY */
>  
>  bool ppc_cpu_exec_interrupt(CPUState *cs, int interrupt_request)

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