mirrors/qemu
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

target/ppc: Fix PMU hflags calculation

Browse Source 
Some of the PMU hflags bits can go out of synch, for example a store to
MMCR0 with PMCjCE=1 fails to update hflags correctly and results in
hflags mismatch:

  qemu: fatal: TCG hflags mismatch (current:0x2408003d rebuilt:0x240a003d)

This can be reproduced by running perf on a recent machine.

Some of the fragility here is the duplication of PMU hflags calculations.
This change consolidates that in a single place to update pmu-related
hflags, to be called after a well defined state changes.

The post-load PMU update is pulled out of the MSR update because it does
not depend on the MSR value.

Fixes: 8b3d1c49a9 ("target/ppc: Add new PMC HFLAGS")
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>
Message-Id: <20230530130447.372617-1-npiggin@gmail.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
This commit is contained in:
Nicholas Piggin 2023-05-30 23:04:47 +10:00 committed by Daniel Henrique Barboza
parent 34b4313070
commit 6494d2c1fd
6 changed files with 85 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
target/ppc/cpu_init.c
View File

@ -7083,7 +7083,7 @@ static void ppc_cpu_reset_hold(Object *obj)
if (env->mmu_model != POWERPC_MMU_REAL) {
ppc_tlb_invalidate_all(env);
}
pmu_update_summaries(env);
pmu_mmcr01_updated(env);
}
/* clean any pending stop state */

73
target/ppc/helper_regs.c
View File

@ -47,6 +47,48 @@ void hreg_swap_gpr_tgpr(CPUPPCState *env)
env->tgpr[3] = tmp;
}
static uint32_t hreg_compute_pmu_hflags_value(CPUPPCState *env)
{
uint32_t hflags = 0;
#if defined(TARGET_PPC64)
if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMCC0) {
hflags |= 1 << HFLAGS_PMCC0;
}
if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMCC1) {
hflags |= 1 << HFLAGS_PMCC1;
}
if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMCjCE) {
hflags |= 1 << HFLAGS_PMCJCE;
}
#ifndef CONFIG_USER_ONLY
if (env->pmc_ins_cnt) {
hflags |= 1 << HFLAGS_INSN_CNT;
}
if (env->pmc_ins_cnt & 0x1e) {
hflags |= 1 << HFLAGS_PMC_OTHER;
}
#endif
#endif
return hflags;
}
/* Mask of all PMU hflags */
static uint32_t hreg_compute_pmu_hflags_mask(CPUPPCState *env)
{
uint32_t hflags_mask = 0;
#if defined(TARGET_PPC64)
hflags_mask |= 1 << HFLAGS_PMCC0;
hflags_mask |= 1 << HFLAGS_PMCC1;
hflags_mask |= 1 << HFLAGS_PMCJCE;
hflags_mask |= 1 << HFLAGS_INSN_CNT;
hflags_mask |= 1 << HFLAGS_PMC_OTHER;
#endif
return hflags_mask;
}
static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
{
target_ulong msr = env->msr;
@ -104,30 +146,12 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
if (env->spr[SPR_LPCR] & LPCR_HR) {
hflags |= 1 << HFLAGS_HR;
}
if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMCC0) {
hflags |= 1 << HFLAGS_PMCC0;
}
if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMCC1) {
hflags |= 1 << HFLAGS_PMCC1;
}
if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMCjCE) {
hflags |= 1 << HFLAGS_PMCJCE;
}
#ifndef CONFIG_USER_ONLY
if (!env->has_hv_mode || (msr & (1ull << MSR_HV))) {
hflags |= 1 << HFLAGS_HV;
}
#if defined(TARGET_PPC64)
if (env->pmc_ins_cnt) {
hflags |= 1 << HFLAGS_INSN_CNT;
}
if (env->pmc_ins_cnt & 0x1e) {
hflags |= 1 << HFLAGS_PMC_OTHER;
}
#endif
/*
* This is our encoding for server processors. The architecture
* specifies that there is no such thing as userspace with
@ -172,6 +196,8 @@ static uint32_t hreg_compute_hflags_value(CPUPPCState *env)
hflags |= dmmu_idx << HFLAGS_DMMU_IDX;
#endif
hflags |= hreg_compute_pmu_hflags_value(env);
return hflags | (msr & msr_mask);
}
@ -180,6 +206,17 @@ void hreg_compute_hflags(CPUPPCState *env)
env->hflags = hreg_compute_hflags_value(env);
}
/*
* This can be used as a lighter-weight alternative to hreg_compute_hflags
* when PMU MMCR0 or pmc_ins_cnt changes. pmc_ins_cnt is changed by
* pmu_update_summaries.
*/
void hreg_update_pmu_hflags(CPUPPCState *env)
{
env->hflags &= ~hreg_compute_pmu_hflags_mask(env);
env->hflags |= hreg_compute_pmu_hflags_value(env);
}
#ifdef CONFIG_DEBUG_TCG
void cpu_get_tb_cpu_state(CPUPPCState *env, target_ulong *pc,
target_ulong *cs_base, uint32_t *flags)

1
target/ppc/helper_regs.h
View File

@ -22,6 +22,7 @@
void hreg_swap_gpr_tgpr(CPUPPCState *env);
void hreg_compute_hflags(CPUPPCState *env);
void hreg_update_pmu_hflags(CPUPPCState *env);
void cpu_interrupt_exittb(CPUState *cs);
int hreg_store_msr(CPUPPCState *env, target_ulong value, int alter_hv);

8
target/ppc/machine.c
View File

@ -21,10 +21,6 @@ static void post_load_update_msr(CPUPPCState *env)
*/
env->msr ^= env->msr_mask & ~((1ULL << MSR_TGPR) | MSR_HVB);
ppc_store_msr(env, msr);
if (tcg_enabled()) {
pmu_update_summaries(env);
}
}
static int get_avr(QEMUFile *f, void *pv, size_t size,
@ -317,6 +313,10 @@ static int cpu_post_load(void *opaque, int version_id)
post_load_update_msr(env);
if (tcg_enabled()) {
pmu_mmcr01_updated(env);
}
return 0;
}

38
target/ppc/power8-pmu.c
View File

@ -31,7 +31,11 @@ static bool pmc_has_overflow_enabled(CPUPPCState *env, int sprn)
return env->spr[SPR_POWER_MMCR0] & MMCR0_PMCjCE;
}
void pmu_update_summaries(CPUPPCState *env)
/*
* Called after MMCR0 or MMCR1 changes to update pmc_ins_cnt and pmc_cyc_cnt.
* hflags must subsequently be updated.
*/
static void pmu_update_summaries(CPUPPCState *env)
{
target_ulong mmcr0 = env->spr[SPR_POWER_MMCR0];
target_ulong mmcr1 = env->spr[SPR_POWER_MMCR1];
@ -39,7 +43,7 @@ void pmu_update_summaries(CPUPPCState *env)
int cyc_cnt = 0;
if (mmcr0 & MMCR0_FC) {
goto hflags_calc;
goto out;
}
if (!(mmcr0 & MMCR0_FC14) && mmcr1 != 0) {
@ -73,10 +77,19 @@ void pmu_update_summaries(CPUPPCState *env)
ins_cnt |= !(mmcr0 & MMCR0_FC56) << 5;
cyc_cnt |= !(mmcr0 & MMCR0_FC56) << 6;
hflags_calc:
out:
env->pmc_ins_cnt = ins_cnt;
env->pmc_cyc_cnt = cyc_cnt;
env->hflags = deposit32(env->hflags, HFLAGS_INSN_CNT, 1, ins_cnt != 0);
}
void pmu_mmcr01_updated(CPUPPCState *env)
{
pmu_update_summaries(env);
hreg_update_pmu_hflags(env);
/*
* Should this update overflow timers (if mmcr0 is updated) so they
* get set in cpu_post_load?
*/
}
static bool pmu_increment_insns(CPUPPCState *env, uint32_t num_insns)
@ -234,18 +247,11 @@ static void pmu_delete_timers(CPUPPCState *env)
void helper_store_mmcr0(CPUPPCState *env, target_ulong value)
{
bool hflags_pmcc0 = (value & MMCR0_PMCC0) != 0;
bool hflags_pmcc1 = (value & MMCR0_PMCC1) != 0;
pmu_update_cycles(env);
env->spr[SPR_POWER_MMCR0] = value;
/* MMCR0 writes can change HFLAGS_PMCC[01] and HFLAGS_INSN_CNT */
env->hflags = deposit32(env->hflags, HFLAGS_PMCC0, 1, hflags_pmcc0);
env->hflags = deposit32(env->hflags, HFLAGS_PMCC1, 1, hflags_pmcc1);
pmu_update_summaries(env);
pmu_mmcr01_updated(env);
/* Update cycle overflow timers with the current MMCR0 state */
pmu_update_overflow_timers(env);
@ -257,8 +263,7 @@ void helper_store_mmcr1(CPUPPCState *env, uint64_t value)
env->spr[SPR_POWER_MMCR1] = value;
/* MMCR1 writes can change HFLAGS_INSN_CNT */
pmu_update_summaries(env);
pmu_mmcr01_updated(env);
}
target_ulong helper_read_pmc(CPUPPCState *env, uint32_t sprn)
@ -287,8 +292,8 @@ static void fire_PMC_interrupt(PowerPCCPU *cpu)
env->spr[SPR_POWER_MMCR0] &= ~MMCR0_FCECE;
env->spr[SPR_POWER_MMCR0] |= MMCR0_FC;
/* Changing MMCR0_FC requires a new HFLAGS_INSN_CNT calc */
pmu_update_summaries(env);
/* Changing MMCR0_FC requires summaries and hflags update */
pmu_mmcr01_updated(env);
/*
* Delete all pending timers if we need to freeze
@ -299,6 +304,7 @@ static void fire_PMC_interrupt(PowerPCCPU *cpu)
}
if (env->spr[SPR_POWER_MMCR0] & MMCR0_PMAE) {
/* These MMCR0 bits do not require summaries or hflags update. */
env->spr[SPR_POWER_MMCR0] &= ~MMCR0_PMAE;
env->spr[SPR_POWER_MMCR0] |= MMCR0_PMAO;
}

4
target/ppc/power8-pmu.h
View File

@ -18,10 +18,10 @@
#define PMC_COUNTER_NEGATIVE_VAL 0x80000000UL
void cpu_ppc_pmu_init(CPUPPCState *env);
void pmu_update_summaries(CPUPPCState *env);
void pmu_mmcr01_updated(CPUPPCState *env);
#else
static inline void cpu_ppc_pmu_init(CPUPPCState *env) { }
static inline void pmu_update_summaries(CPUPPCState *env) { }
static inline void pmu_mmcr01_updated(CPUPPCState *env) { }
#endif
#endif