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 <npig...@gmail.com> > --- > 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) >