diff mbox series

[RFC,5/6] target/arm: Conditionalize DBGDIDR vs ID_AA64DFR0_EL1 assert

Message ID 20190223023957.18865-6-richard.henderson@linaro.org
State New
Headers show
Series target/arm: Define cortex-a{73, 75, 76} | expand

Commit Message

Richard Henderson Feb. 23, 2019, 2:39 a.m. UTC 
Only perform the assert when both registers exist.
Extract the variables from ID_AA64DFR0_EL1 for AArch64.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

---
 target/arm/helper.c | 58 +++++++++++++++++++++++++++++----------------
 1 file changed, 38 insertions(+), 20 deletions(-)

-- 
2.17.2

Comments

Peter Maydell April 30, 2019, 12:40 p.m. UTC | #1 
On Sat, 23 Feb 2019 at 02:40, Richard Henderson
<richard.henderson@linaro.org> wrote:
>

> Only perform the assert when both registers exist.

> Extract the variables from ID_AA64DFR0_EL1 for AArch64.

>

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>


> +    if (have_aa32) {

> +        ARMCPRegInfo dbgdidr = {

> +            .name = "DBGDIDR", .cp = 14, .crn = 0, .crm = 0,

> +            .opc1 = 0, .opc2 = 0, .access = PL0_R, .accessfn = access_tda,

> +            .type = ARM_CP_CONST, .resetvalue = cpu->dbgdidr,

> +        };

> +        define_one_arm_cp_reg(cpu, &dbgdidr);

> +    }


So if only EL0 has AArch32 it doesn't architecturally require
that this AArch32 system register doesn't exist, because the
register is still readable from EL0. The Arm ARM says that
"implementation of this register is optional and deprecated".
I would suggest that we should probably go with "implement the
register if cpu->dbgdidr is non-zero", since at least bit 15
must be set so zero isn't a valid real value for it.

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>


thanks
-- PMM
diff mbox series

Patch

diff --git a/target/arm/helper.c b/target/arm/helper.c
index fbdca9324b..1d8c8998c4 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -5544,32 +5544,50 @@  static void define_debug_regs(ARMCPU *cpu)
     /* Define v7 and v8 architectural debug registers.
      * These are just dummy implementations for now.
      */
-    int i;
-    int wrps, brps, ctx_cmps;
-    ARMCPRegInfo dbgdidr = {
-        .name = "DBGDIDR", .cp = 14, .crn = 0, .crm = 0, .opc1 = 0, .opc2 = 0,
-        .access = PL0_R, .accessfn = access_tda,
-        .type = ARM_CP_CONST, .resetvalue = cpu->dbgdidr,
-    };
+    int i, wrps, brps, ctx_cmps;
+    bool have_aa32;
 
-    /* Note that all these register fields hold "number of Xs minus 1". */
-    brps = extract32(cpu->dbgdidr, 24, 4);
-    wrps = extract32(cpu->dbgdidr, 28, 4);
-    ctx_cmps = extract32(cpu->dbgdidr, 20, 4);
-
-    assert(ctx_cmps <= brps);
-
-    /* The DBGDIDR and ID_AA64DFR0_EL1 define various properties
+    /*
+     * The DBGDIDR and ID_AA64DFR0_EL1 define various properties
      * of the debug registers such as number of breakpoints;
      * check that if they both exist then they agree.
+     *
+     * Note that all these register fields hold "number of Xs minus 1".
      */
     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);
-    }
+        brps = extract32(cpu->id_aa64dfr0, 12, 4);
+        wrps = extract32(cpu->id_aa64dfr0, 20, 4);
+        ctx_cmps = extract32(cpu->id_aa64dfr0, 28, 4);
 
-    define_one_arm_cp_reg(cpu, &dbgdidr);
+        /*
+         * There are cpus with aarch32 only at EL0, and which do not
+         * have the 32-bit system registers.
+         */
+        have_aa32
+            = (FIELD_EX64(cpu->isar.id_aa64pfr0, ID_AA64PFR0, EL1) >= 2 ||
+               FIELD_EX64(cpu->isar.id_aa64pfr0, ID_AA64PFR0, EL2) >= 2 ||
+               FIELD_EX64(cpu->isar.id_aa64pfr0, ID_AA64PFR0, EL3) >= 2);
+        if (have_aa32) {
+            assert(extract32(cpu->dbgdidr, 24, 4) == brps);
+            assert(extract32(cpu->dbgdidr, 28, 4) == wrps);
+            assert(extract32(cpu->dbgdidr, 20, 4) == ctx_cmps);
+        }
+    } else {
+        have_aa32 = true;
+        brps = extract32(cpu->dbgdidr, 24, 4);
+        wrps = extract32(cpu->dbgdidr, 28, 4);
+        ctx_cmps = extract32(cpu->dbgdidr, 20, 4);
+    }
+    assert(ctx_cmps <= brps);
+
+    if (have_aa32) {
+        ARMCPRegInfo dbgdidr = {
+            .name = "DBGDIDR", .cp = 14, .crn = 0, .crm = 0,
+            .opc1 = 0, .opc2 = 0, .access = PL0_R, .accessfn = access_tda,
+            .type = ARM_CP_CONST, .resetvalue = cpu->dbgdidr,
+        };
+        define_one_arm_cp_reg(cpu, &dbgdidr);
+    }
     define_arm_cp_regs(cpu, debug_cp_reginfo);
 
     if (arm_feature(&cpu->env, ARM_FEATURE_LPAE)) {