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

[Cédric Le Goater]

On 3/16/20 3:26 PM, 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).

I don't understand which issues are addressed in the patch.

> It's not nice to scatter interrupt delivery logic around the tree, so
> move it to excp_helper.c and share code where possible.

It looks correct but this is touching the ugliest routine in the QEMU 
PPC universe. I would split the patch in two to introduce the helper
powerpc_set_excp_state().

It does not seem to need to be an inline also.

C. 

> 
> Signed-off-by: Nicholas Piggin <address@hidden>
> ---
>  hw/ppc/spapr_events.c    | 24 +++----------
>  target/ppc/cpu.h         |  1 +
>  target/ppc/excp_helper.c | 74 ++++++++++++++++++++++++++++------------
>  3 files changed, 57 insertions(+), 42 deletions(-)
> 
> diff --git a/hw/ppc/spapr_events.c b/hw/ppc/spapr_events.c
> index 27ba8a2c19..323fcef4aa 100644
> --- a/hw/ppc/spapr_events.c
> +++ b/hw/ppc/spapr_events.c
> @@ -785,28 +785,13 @@ static uint32_t spapr_mce_get_elog_type(PowerPCCPU 
> *cpu, bool recovered,
>  static void spapr_mce_dispatch_elog(PowerPCCPU *cpu, bool recovered)
>  {
>      SpaprMachineState *spapr = SPAPR_MACHINE(qdev_get_machine());
> -    uint64_t rtas_addr;
> +    CPUState *cs = CPU(cpu);
>      CPUPPCState *env = &cpu->env;
> -    PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
> -    target_ulong msr = 0;
> +    uint64_t rtas_addr;
>      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);
>  
> @@ -834,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 5a55fb02bd..3953680534 100644
> --- a/target/ppc/cpu.h
> +++ b/target/ppc/cpu.h
> @@ -1221,6 +1221,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)
>