| Message ID | 20200211173726.22541-6-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm: Implement ARMv8.1-PMU and ARMv8.4-PMU | expand |
On 2/11/20 9:37 AM, Peter Maydell wrote: > if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) { > - assert(extract32(cpu->id_aa64dfr0, 12, 4) == brps); > - assert(extract32(cpu->id_aa64dfr0, 20, 4) == wrps); > - assert(extract32(cpu->id_aa64dfr0, 28, 4) == ctx_cmps); > + assert(FIELD_EX32(cpu->id_aa64dfr0, ID_AA64DFR0, BRPS) == brps); > + assert(FIELD_EX32(cpu->id_aa64dfr0, ID_AA64DFR0, WRPS) == wrps); > + assert(FIELD_EX32(cpu->id_aa64dfr0, ID_AA64DFR0, CTX_CMPS) == ctx_cmps); Should really be FIELD_EX64. Otherwise, Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 2/11/20 7:34 PM, Richard Henderson wrote: > On 2/11/20 9:37 AM, Peter Maydell wrote: >> if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) { >> - assert(extract32(cpu->id_aa64dfr0, 12, 4) == brps); >> - assert(extract32(cpu->id_aa64dfr0, 20, 4) == wrps); >> - assert(extract32(cpu->id_aa64dfr0, 28, 4) == ctx_cmps); >> + assert(FIELD_EX32(cpu->id_aa64dfr0, ID_AA64DFR0, BRPS) == brps); >> + assert(FIELD_EX32(cpu->id_aa64dfr0, ID_AA64DFR0, WRPS) == wrps); >> + assert(FIELD_EX32(cpu->id_aa64dfr0, ID_AA64DFR0, CTX_CMPS) == ctx_cmps); > > Should really be FIELD_EX64. Otherwise, Similarly to the other previous call, FIELD_DP64: cpu->id_aa64dfr0 = FIELD_DP32(cpu->id_aa64dfr0, ID_AA64DFR0, PMUVER, 0); So far the code is safe because the >31 bits macros aren't used: FIELD(ID_AA64DFR0, PMSVER, 32, 4) FIELD(ID_AA64DFR0, DOUBLELOCK, 36, 4) FIELD(ID_AA64DFR0, TRACEFILT, 40, 4) But you are right, let's fix it now to avoid copy/pasting 32bit macros and unpleasant debugging sessions. > > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> Using 64bit macros: Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
diff --git a/target/arm/cpu.h b/target/arm/cpu.h index b1f3ecfd942..f2194b27ba3 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -1806,6 +1806,16 @@ FIELD(ID_AA64MMFR1, PAN, 20, 4) FIELD(ID_AA64MMFR1, SPECSEI, 24, 4) FIELD(ID_AA64MMFR1, XNX, 28, 4) +FIELD(ID_AA64DFR0, DEBUGVER, 0, 4) +FIELD(ID_AA64DFR0, TRACEVER, 4, 4) +FIELD(ID_AA64DFR0, PMUVER, 8, 4) +FIELD(ID_AA64DFR0, BRPS, 12, 4) +FIELD(ID_AA64DFR0, WRPS, 20, 4) +FIELD(ID_AA64DFR0, CTX_CMPS, 28, 4) +FIELD(ID_AA64DFR0, PMSVER, 32, 4) +FIELD(ID_AA64DFR0, DOUBLELOCK, 36, 4) +FIELD(ID_AA64DFR0, TRACEFILT, 40, 4) + FIELD(ID_DFR0, COPDBG, 0, 4) FIELD(ID_DFR0, COPSDBG, 4, 4) FIELD(ID_DFR0, MMAPDBG, 8, 4) diff --git a/target/arm/cpu.c b/target/arm/cpu.c index 5712082c0b9..dc582da8fa4 100644 --- a/target/arm/cpu.c +++ b/target/arm/cpu.c @@ -1602,7 +1602,7 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp) cpu); #endif } else { - cpu->id_aa64dfr0 &= ~0xf00; + cpu->id_aa64dfr0 = FIELD_DP32(cpu->id_aa64dfr0, ID_AA64DFR0, PMUVER, 0); cpu->id_dfr0 &= ~(0xf << 24); cpu->pmceid0 = 0; cpu->pmceid1 = 0; diff --git a/target/arm/helper.c b/target/arm/helper.c index 0011a22f42d..2a57bfd9e86 100644 --- a/target/arm/helper.c +++ b/target/arm/helper.c @@ -5771,9 +5771,9 @@ static void define_debug_regs(ARMCPU *cpu) * check that if they both exist then they agree. */ if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) { - assert(extract32(cpu->id_aa64dfr0, 12, 4) == brps); - assert(extract32(cpu->id_aa64dfr0, 20, 4) == wrps); - assert(extract32(cpu->id_aa64dfr0, 28, 4) == ctx_cmps); + assert(FIELD_EX32(cpu->id_aa64dfr0, ID_AA64DFR0, BRPS) == brps); + assert(FIELD_EX32(cpu->id_aa64dfr0, ID_AA64DFR0, WRPS) == wrps); + assert(FIELD_EX32(cpu->id_aa64dfr0, ID_AA64DFR0, CTX_CMPS) == ctx_cmps); } define_one_arm_cp_reg(cpu, &dbgdidr);
Add FIELD() definitions for the ID_AA64DFR0_EL1 and use them where we currently have hard-coded bit values. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/cpu.h | 10 ++++++++++ target/arm/cpu.c | 2 +- target/arm/helper.c | 6 +++--- 3 files changed, 14 insertions(+), 4 deletions(-) -- 2.20.1