| Message ID | 20220630194116.3438513-6-peter.maydell@linaro.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | target/arm: Implement (or don't) OS Lock and DoubleLock properly | expand |
On 7/1/22 01:11, Peter Maydell wrote: > +static inline bool isar_feature_any_doublelock(const ARMISARegisters *id) > +{ > + /* > + * We can't just OR together the aa32 and aa64 checks, because > + * if there is no AArch64 support the aa64 function will default > + * to returning true for a zero id_aa64dfr0. > + * We use "is id_aa64pfr0 non-zero" as a proxy for "do we have > + * the AArch64 ID register values in id", because it's always the > + * case that ID_AA64PFR0_EL1.EL0 at least will be non-zero. > + */ > + if (id->id_aa64pfr0) { > + return isar_feature_aa64_doublelock(id); > + } > + return isar_feature_aa32_doublelock(id); > +} If you're going to rely on this, you need to clear this register for -cpu aarch64=off. It's probably easier to drop this function... > +static void osdlr_write(CPUARMState *env, const ARMCPRegInfo *ri, > + uint64_t value) > +{ > + /* > + * Only defined bit is bit 0 (DLK); if Feat_DoubleLock is not > + * implemented this is RAZ/WI. > + */ > + if (cpu_isar_feature(any_doublelock, env_archcpu(env))) { ... and use if (arm_feature(env, ARM_FEATURE_AARCH64) ? cpu_isar_feature(aa64_doublelock, cpu) : cpu_isar_feature(aa32_doublelock, cpu)) { since it's just used once. r~
On Sat, 2 Jul 2022 at 15:19, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 7/1/22 01:11, Peter Maydell wrote: > > +static inline bool isar_feature_any_doublelock(const ARMISARegisters *id) > > +{ > > + /* > > + * We can't just OR together the aa32 and aa64 checks, because > > + * if there is no AArch64 support the aa64 function will default > > + * to returning true for a zero id_aa64dfr0. > > + * We use "is id_aa64pfr0 non-zero" as a proxy for "do we have > > + * the AArch64 ID register values in id", because it's always the > > + * case that ID_AA64PFR0_EL1.EL0 at least will be non-zero. > > + */ > > + if (id->id_aa64pfr0) { > > + return isar_feature_aa64_doublelock(id); > > + } > > + return isar_feature_aa32_doublelock(id); > > +} > > If you're going to rely on this, you need to clear this register for -cpu aarch64=off. Why? The AArch32 version of the CPU is going to either implement or not implement DoubleLock the same as the AArch64 version: the answer will be the same in both ID registers. We only need to avoid looking at the AA64 ID value if we don't have it at all. thanks -- PMM
On 7/3/22 14:27, Peter Maydell wrote: > On Sat, 2 Jul 2022 at 15:19, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> On 7/1/22 01:11, Peter Maydell wrote: >>> +static inline bool isar_feature_any_doublelock(const ARMISARegisters *id) >>> +{ >>> + /* >>> + * We can't just OR together the aa32 and aa64 checks, because >>> + * if there is no AArch64 support the aa64 function will default >>> + * to returning true for a zero id_aa64dfr0. >>> + * We use "is id_aa64pfr0 non-zero" as a proxy for "do we have >>> + * the AArch64 ID register values in id", because it's always the >>> + * case that ID_AA64PFR0_EL1.EL0 at least will be non-zero. >>> + */ >>> + if (id->id_aa64pfr0) { >>> + return isar_feature_aa64_doublelock(id); >>> + } >>> + return isar_feature_aa32_doublelock(id); >>> +} >> >> If you're going to rely on this, you need to clear this register for -cpu aarch64=off. > > Why? The AArch32 version of the CPU is going to either implement or not > implement DoubleLock the same as the AArch64 version: the answer > will be the same in both ID registers. We only need to avoid looking > at the AA64 ID value if we don't have it at all. I suppose. It looks weird, and isn't a proper replacement for "is aa64 mode supported". r~
On Sat, 2 Jul 2022 at 15:19, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 7/1/22 01:11, Peter Maydell wrote: > > +static inline bool isar_feature_any_doublelock(const ARMISARegisters *id) > > +{ > > + /* > > + * We can't just OR together the aa32 and aa64 checks, because > > + * if there is no AArch64 support the aa64 function will default > > + * to returning true for a zero id_aa64dfr0. > > + * We use "is id_aa64pfr0 non-zero" as a proxy for "do we have > > + * the AArch64 ID register values in id", because it's always the > > + * case that ID_AA64PFR0_EL1.EL0 at least will be non-zero. > > + */ > > + if (id->id_aa64pfr0) { > > + return isar_feature_aa64_doublelock(id); > > + } > > + return isar_feature_aa32_doublelock(id); > > +} > > If you're going to rely on this, you need to clear this register for -cpu aarch64=off. > It's probably easier to drop this function... > > > +static void osdlr_write(CPUARMState *env, const ARMCPRegInfo *ri, > > + uint64_t value) > > +{ > > + /* > > + * Only defined bit is bit 0 (DLK); if Feat_DoubleLock is not > > + * implemented this is RAZ/WI. > > + */ > > + if (cpu_isar_feature(any_doublelock, env_archcpu(env))) { > > ... and use > > if (arm_feature(env, ARM_FEATURE_AARCH64) > ? cpu_isar_feature(aa64_doublelock, cpu) > : cpu_isar_feature(aa32_doublelock, cpu)) { > > since it's just used once. If I squash in this change: diff --git a/target/arm/cpu.h b/target/arm/cpu.h index 069dfe1d308..1f4f3e0485c 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -4218,22 +4218,6 @@ static inline bool isar_feature_any_ras(const ARMISARegisters *id) return isar_feature_aa64_ras(id) || isar_feature_aa32_ras(id); } -static inline bool isar_feature_any_doublelock(const ARMISARegisters *id) -{ - /* - * We can't just OR together the aa32 and aa64 checks, because - * if there is no AArch64 support the aa64 function will default - * to returning true for a zero id_aa64dfr0. - * We use "is id_aa64pfr0 non-zero" as a proxy for "do we have - * the AArch64 ID register values in id", because it's always the - * case that ID_AA64PFR0_EL1.EL0 at least will be non-zero. - */ - if (id->id_aa64pfr0) { - return isar_feature_aa64_doublelock(id); - } - return isar_feature_aa32_doublelock(id); -} - /* * Forward to the above feature tests given an ARMCPU pointer. */ diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c index 62bd8f85383..d09fccb0a4f 100644 --- a/target/arm/debug_helper.c +++ b/target/arm/debug_helper.c @@ -617,11 +617,14 @@ static void oslar_write(CPUARMState *env, const ARMCPRegInfo *ri, static void osdlr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value) { + ARMCPU *cpu = env_archcpu(env); /* * Only defined bit is bit 0 (DLK); if Feat_DoubleLock is not * implemented this is RAZ/WI. */ - if (cpu_isar_feature(any_doublelock, env_archcpu(env))) { + if(arm_feature(env, ARM_FEATURE_AARCH64) + ? cpu_isar_feature(aa64_doublelock, cpu) + : cpu_isar_feature(aa32_doublelock, cpu)) { env->cp15.osdlr_el1 = value & 1; } } can I have a reviewed-by? Would save me doing a respin. thanks -- PMM
On 7/5/22 21:06, Peter Maydell wrote: > On Sat, 2 Jul 2022 at 15:19, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> On 7/1/22 01:11, Peter Maydell wrote: >>> +static inline bool isar_feature_any_doublelock(const ARMISARegisters *id) >>> +{ >>> + /* >>> + * We can't just OR together the aa32 and aa64 checks, because >>> + * if there is no AArch64 support the aa64 function will default >>> + * to returning true for a zero id_aa64dfr0. >>> + * We use "is id_aa64pfr0 non-zero" as a proxy for "do we have >>> + * the AArch64 ID register values in id", because it's always the >>> + * case that ID_AA64PFR0_EL1.EL0 at least will be non-zero. >>> + */ >>> + if (id->id_aa64pfr0) { >>> + return isar_feature_aa64_doublelock(id); >>> + } >>> + return isar_feature_aa32_doublelock(id); >>> +} >> >> If you're going to rely on this, you need to clear this register for -cpu aarch64=off. >> It's probably easier to drop this function... >> >>> +static void osdlr_write(CPUARMState *env, const ARMCPRegInfo *ri, >>> + uint64_t value) >>> +{ >>> + /* >>> + * Only defined bit is bit 0 (DLK); if Feat_DoubleLock is not >>> + * implemented this is RAZ/WI. >>> + */ >>> + if (cpu_isar_feature(any_doublelock, env_archcpu(env))) { >> >> ... and use >> >> if (arm_feature(env, ARM_FEATURE_AARCH64) >> ? cpu_isar_feature(aa64_doublelock, cpu) >> : cpu_isar_feature(aa32_doublelock, cpu)) { >> >> since it's just used once. > > If I squash in this change: > > diff --git a/target/arm/cpu.h b/target/arm/cpu.h > index 069dfe1d308..1f4f3e0485c 100644 > --- a/target/arm/cpu.h > +++ b/target/arm/cpu.h > @@ -4218,22 +4218,6 @@ static inline bool isar_feature_any_ras(const > ARMISARegisters *id) > return isar_feature_aa64_ras(id) || isar_feature_aa32_ras(id); > } > > -static inline bool isar_feature_any_doublelock(const ARMISARegisters *id) > -{ > - /* > - * We can't just OR together the aa32 and aa64 checks, because > - * if there is no AArch64 support the aa64 function will default > - * to returning true for a zero id_aa64dfr0. > - * We use "is id_aa64pfr0 non-zero" as a proxy for "do we have > - * the AArch64 ID register values in id", because it's always the > - * case that ID_AA64PFR0_EL1.EL0 at least will be non-zero. > - */ > - if (id->id_aa64pfr0) { > - return isar_feature_aa64_doublelock(id); > - } > - return isar_feature_aa32_doublelock(id); > -} > - > /* > * Forward to the above feature tests given an ARMCPU pointer. > */ > diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c > index 62bd8f85383..d09fccb0a4f 100644 > --- a/target/arm/debug_helper.c > +++ b/target/arm/debug_helper.c > @@ -617,11 +617,14 @@ static void oslar_write(CPUARMState *env, const > ARMCPRegInfo *ri, > static void osdlr_write(CPUARMState *env, const ARMCPRegInfo *ri, > uint64_t value) > { > + ARMCPU *cpu = env_archcpu(env); > /* > * Only defined bit is bit 0 (DLK); if Feat_DoubleLock is not > * implemented this is RAZ/WI. > */ > - if (cpu_isar_feature(any_doublelock, env_archcpu(env))) { > + if(arm_feature(env, ARM_FEATURE_AARCH64) > + ? cpu_isar_feature(aa64_doublelock, cpu) > + : cpu_isar_feature(aa32_doublelock, cpu)) { > env->cp15.osdlr_el1 = value & 1; > } > } > > > > > can I have a reviewed-by? Would save me doing a respin. You may. Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
diff --git a/target/arm/cpu.h b/target/arm/cpu.h index c533ad0b64d..069dfe1d308 100644 --- a/target/arm/cpu.h +++ b/target/arm/cpu.h @@ -500,6 +500,7 @@ typedef struct CPUArchState { uint64_t dbgwcr[16]; /* watchpoint control registers */ uint64_t mdscr_el1; uint64_t oslsr_el1; /* OS Lock Status */ + uint64_t osdlr_el1; /* OS DoubleLock status */ uint64_t mdcr_el2; uint64_t mdcr_el3; /* Stores the architectural value of the counter *the last time it was @@ -2253,6 +2254,15 @@ FIELD(DBGDIDR, CTX_CMPS, 20, 4) FIELD(DBGDIDR, BRPS, 24, 4) FIELD(DBGDIDR, WRPS, 28, 4) +FIELD(DBGDEVID, PCSAMPLE, 0, 4) +FIELD(DBGDEVID, WPADDRMASK, 4, 4) +FIELD(DBGDEVID, BPADDRMASK, 8, 4) +FIELD(DBGDEVID, VECTORCATCH, 12, 4) +FIELD(DBGDEVID, VIRTEXTNS, 16, 4) +FIELD(DBGDEVID, DOUBLELOCK, 20, 4) +FIELD(DBGDEVID, AUXREGS, 24, 4) +FIELD(DBGDEVID, CIDMASK, 28, 4) + FIELD(MVFR0, SIMDREG, 0, 4) FIELD(MVFR0, FPSP, 4, 4) FIELD(MVFR0, FPDP, 8, 4) @@ -3731,6 +3741,11 @@ static inline bool isar_feature_aa32_debugv8p2(const ARMISARegisters *id) return FIELD_EX32(id->id_dfr0, ID_DFR0, COPDBG) >= 8; } +static inline bool isar_feature_aa32_doublelock(const ARMISARegisters *id) +{ + return FIELD_EX32(id->dbgdevid, DBGDEVID, DOUBLELOCK) > 0; +} + /* * 64-bit feature tests via id registers. */ @@ -4155,6 +4170,11 @@ static inline bool isar_feature_aa64_sme_fa64(const ARMISARegisters *id) return FIELD_EX64(id->id_aa64smfr0, ID_AA64SMFR0, FA64); } +static inline bool isar_feature_aa64_doublelock(const ARMISARegisters *id) +{ + return FIELD_SEX64(id->id_aa64dfr0, ID_AA64DFR0, DOUBLELOCK) >= 0; +} + /* * Feature tests for "does this exist in either 32-bit or 64-bit?" */ @@ -4198,6 +4218,22 @@ static inline bool isar_feature_any_ras(const ARMISARegisters *id) return isar_feature_aa64_ras(id) || isar_feature_aa32_ras(id); } +static inline bool isar_feature_any_doublelock(const ARMISARegisters *id) +{ + /* + * We can't just OR together the aa32 and aa64 checks, because + * if there is no AArch64 support the aa64 function will default + * to returning true for a zero id_aa64dfr0. + * We use "is id_aa64pfr0 non-zero" as a proxy for "do we have + * the AArch64 ID register values in id", because it's always the + * case that ID_AA64PFR0_EL1.EL0 at least will be non-zero. + */ + if (id->id_aa64pfr0) { + return isar_feature_aa64_doublelock(id); + } + return isar_feature_aa32_doublelock(id); +} + /* * Forward to the above feature tests given an ARMCPU pointer. */ diff --git a/target/arm/debug_helper.c b/target/arm/debug_helper.c index e96a4ffd28d..62bd8f85383 100644 --- a/target/arm/debug_helper.c +++ b/target/arm/debug_helper.c @@ -142,7 +142,7 @@ static bool aa32_generate_debug_exceptions(CPUARMState *env) */ bool arm_generate_debug_exceptions(CPUARMState *env) { - if (env->cp15.oslsr_el1 & 1) { + if ((env->cp15.oslsr_el1 & 1) || (env->cp15.osdlr_el1 & 1)) { return false; } if (is_a64(env)) { @@ -614,6 +614,18 @@ static void oslar_write(CPUARMState *env, const ARMCPRegInfo *ri, env->cp15.oslsr_el1 = deposit32(env->cp15.oslsr_el1, 1, 1, oslock); } +static void osdlr_write(CPUARMState *env, const ARMCPRegInfo *ri, + uint64_t value) +{ + /* + * Only defined bit is bit 0 (DLK); if Feat_DoubleLock is not + * implemented this is RAZ/WI. + */ + if (cpu_isar_feature(any_doublelock, env_archcpu(env))) { + env->cp15.osdlr_el1 = value & 1; + } +} + static const ARMCPRegInfo debug_cp_reginfo[] = { /* * DBGDRAR, DBGDSAR: always RAZ since we don't implement memory mapped @@ -670,7 +682,8 @@ static const ARMCPRegInfo debug_cp_reginfo[] = { { .name = "OSDLR_EL1", .state = ARM_CP_STATE_BOTH, .cp = 14, .opc0 = 2, .opc1 = 0, .crn = 1, .crm = 3, .opc2 = 4, .access = PL1_RW, .accessfn = access_tdosa, - .type = ARM_CP_NOP }, + .writefn = osdlr_write, + .fieldoffset = offsetof(CPUARMState, cp15.osdlr_el1) }, /* * Dummy DBGVCR: Linux wants to clear this on startup, but we don't * implement vector catch debug events yet.
The architecture defines the OS DoubleLock as a register which (similarly to the OS Lock) suppresses debug events for use in CPU powerdown sequences. This functionality is required in Arm v7 and v8.0; from v8.2 it becomes optional and in v9 it must not be implemented. Currently in QEMU we implement the OSDLR_EL1 register as a NOP. This is wrong both for the "feature implemented" and the "feature not implemented" cases: if the feature is implemented then the DLK bit should read as written and cause suppression of debug exceptions, and if it is not implemented then the bit must be RAZ/WI. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- Is there a nicer way to implement the isar_any feature test ? What I have here is correct but a bit ad-hoc. --- target/arm/cpu.h | 36 ++++++++++++++++++++++++++++++++++++ target/arm/debug_helper.c | 17 +++++++++++++++-- 2 files changed, 51 insertions(+), 2 deletions(-)