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(-)
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.
--
2.25.1On 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.
> 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~
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~
© 2016 - 2026 Red Hat, Inc.