diff mbox series

[5/5] target/arm: Correctly implement Feat_DoubleLock

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

Commit Message

Peter Maydell June 30, 2022, 7:41 p.m. UTC
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(-)

Comments

Richard Henderson July 2, 2022, 2:19 p.m. UTC | #1
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~
Peter Maydell July 3, 2022, 8:57 a.m. UTC | #2
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
Richard Henderson July 3, 2022, 9:01 a.m. UTC | #3
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~
Peter Maydell July 5, 2022, 3:36 p.m. UTC | #4
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
Richard Henderson July 5, 2022, 5:16 p.m. UTC | #5
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 mbox series

Patch

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.