| Message ID | 20200316142613.121089-5-npiggin@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | FWNMI fixes / changes | expand |
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 <npiggin@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) >
Cédric Le Goater's on March 17, 2020 3:59 am: > 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. The existing code does not compute hflags, at least. There's a few possible other things, I didn't dig into qemu enough to know if they might be a problem (e.g., reservation and TLB). I figure it's better to keep these consistent. Keep in mind this is a bit academic right now, because we can't (AFAIKS) inject an MCE from TCG. It would be good to wire that up, but I didn't get to it. >> 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. Yeah it's all pretty ugly. I didn't yet find a nice way to do split things up that did not require a lot of code churn, but that can come later. Inline was just because powerpc_excp is inline, I didn't want to change behaviour too much there (it obviously wants to do a lot of constant propagation but maybe only on the case statement). Anyway I just wanted to be minimal for now, it could be changed. Thanks, Nick
On Tue, Mar 17, 2020 at 09:19:57AM +1000, Nicholas Piggin wrote: > Cédric Le Goater's on March 17, 2020 3:59 am: > > 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. > > The existing code does not compute hflags, at least. > > There's a few possible other things, I didn't dig into qemu enough > to know if they might be a problem (e.g., reservation and TLB). I > figure it's better to keep these consistent. > > Keep in mind this is a bit academic right now, because we can't > (AFAIKS) inject an MCE from TCG. It would be good to wire that up, > but I didn't get to it. > > >> 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. > > Yeah it's all pretty ugly. I didn't yet find a nice way to do > split things up that did not require a lot of code churn, but that > can come later. > > Inline was just because powerpc_excp is inline, I didn't want to > change behaviour too much there (it obviously wants to do a lot of > constant propagation but maybe only on the case statement). Anyway > I just wanted to be minimal for now, it could be changed. Yeah, I definitely want to get this in, so despite imperfections that could probably be polished with time, I've applied to ppc-for-5.0.
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)
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 <npiggin@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(-)