diff mbox series

[v3,35/60] target/arm: Handle cpreg registration for missing EL

Message ID 20220417174426.711829-36-richard.henderson@linaro.org
State New
Headers show
Series target/arm: Cleanups, new features, new cpus | expand

Commit Message

Richard Henderson April 17, 2022, 5:44 p.m. UTC 
More gracefully handle cpregs when EL2 and/or EL3 are missing.
If the reg is entirely inaccessible, do not register it at all.
If the reg is for EL2, and EL3 is present but EL2 is not,
squash to ARM_CP_CONST.

This will simplify cpreg registration for conditional arm features.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/helper.c | 109 ++++++++++++++++++++++++++++++++------------
 1 file changed, 79 insertions(+), 30 deletions(-)

Comments

Peter Maydell April 22, 2022, 10:57 a.m. UTC | #1 
On Sun, 17 Apr 2022 at 19:21, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> More gracefully handle cpregs when EL2 and/or EL3 are missing.
> If the reg is entirely inaccessible, do not register it at all.
> If the reg is for EL2, and EL3 is present but EL2 is not,
> squash to ARM_CP_CONST.

I don't think we should do this unconditionally. Just because
the architecture usually defines that an _EL2 register is
RES0 if EL2 is not present doesn't mean that it does so for
every register or that it guarantees that it will continue
to do so in future. (For instance I found ZCR_EL2 and TFSR_EL2
don't have that statement in their documentation, which might or
might not be an oversight.) You could add an ARM_CP_ flag for
"RES0 if no EL2" or something I guess?

thanks
-- PMM
Peter Maydell April 26, 2022, 9:40 a.m. UTC | #2 
On Fri, 22 Apr 2022 at 11:57, Peter Maydell <peter.maydell@linaro.org> wrote:
> Just because
> the architecture usually defines that an _EL2 register is
> RES0 if EL2 is not present doesn't mean that it does so for
> every register or that it guarantees that it will continue
> to do so in future. (For instance I found ZCR_EL2 and TFSR_EL2
> don't have that statement in their documentation, which might or
> might not be an oversight.)

In those two cases, it probably is a documentation omission.

-- PMM
Peter Maydell April 26, 2022, 3:31 p.m. UTC | #3 
On Fri, 22 Apr 2022 at 11:57, Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Sun, 17 Apr 2022 at 19:21, Richard Henderson
> <richard.henderson@linaro.org> wrote:
> >
> > More gracefully handle cpregs when EL2 and/or EL3 are missing.
> > If the reg is entirely inaccessible, do not register it at all.
> > If the reg is for EL2, and EL3 is present but EL2 is not,
> > squash to ARM_CP_CONST.
>
> I don't think we should do this unconditionally. Just because
> the architecture usually defines that an _EL2 register is
> RES0 if EL2 is not present doesn't mean that it does so for
> every register or that it guarantees that it will continue
> to do so in future. (For instance I found ZCR_EL2 and TFSR_EL2
> don't have that statement in their documentation, which might or
> might not be an oversight.) You could add an ARM_CP_ flag for
> "RES0 if no EL2" or something I guess?

I've just found rule R_RJFFP in section D1.1.3 of DDI0487H.a,
which explicitly documents that if EL2 is not implemented and
EL3 is, then "every accessible register associated with EL2 is
RES0" except for an enumerated list of six registers which are
not. So I'm more open to "default to RES0, with a flag to
suppress that default" than I was when I first reviewed this
patchset.

thanks
-- PMM
diff mbox series

Patch

diff --git a/target/arm/helper.c b/target/arm/helper.c
index 562ea5c418..d9837b5bd2 100644
--- a/target/arm/helper.c
+++ b/target/arm/helper.c
@@ -8519,13 +8519,14 @@  static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
                                    int crm, int opc1, int opc2,
                                    const char *name)
 {
+    CPUARMState *env = &cpu->env;
     uint32_t key;
     ARMCPRegInfo *r2;
     bool is64 = r->type & ARM_CP_64BIT;
     bool ns = secstate & ARM_CP_SECSTATE_NS;
     int cp = r->cp;
-    bool isbanked;
     size_t name_len;
+    bool make_const;
 
     switch (state) {
     case ARM_CP_STATE_AA32:
@@ -8560,6 +8561,24 @@  static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
         }
     }
 
+    /*
+     * Eliminate registers that are not present because the EL is missing.
+     * Doing this here makes it easier to put all registers for a given
+     * feature into the same ARMCPRegInfo array and define them all at once.
+     */
+    if (arm_feature(env, ARM_FEATURE_EL3)) {
+        /* An EL2 register without EL2 but with EL3 is (usually) RES0. */
+        int min_el = ctz32(r->access) / 2;
+        make_const = min_el == 2 && !arm_feature(env, ARM_FEATURE_EL2);
+    } else {
+        CPAccessRights max_el = (arm_feature(env, ARM_FEATURE_EL2)
+                                 ? PL2_RW : PL1_RW);
+        if ((r->access & max_el) == 0) {
+            return;
+        }
+        make_const = false;
+    }
+
     /* Combine cpreg and name into one allocation. */
     name_len = strlen(name) + 1;
     r2 = g_malloc(sizeof(*r2) + name_len);
@@ -8580,44 +8599,74 @@  static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
         r2->opaque = opaque;
     }
 
-    isbanked = r->bank_fieldoffsets[0] && r->bank_fieldoffsets[1];
-    if (isbanked) {
+    if (make_const) {
+        /* This should not have been a very special register to begin. */
+        int old_special = r2->type & ARM_CP_SPECIAL_MASK;
+        assert(old_special == 0 || old_special == ARM_CP_CONST);
         /*
-         * Register is banked (using both entries in array).
-         * Overwriting fieldoffset as the array is only used to define
-         * banked registers but later only fieldoffset is used.
+         * Set the special function to CONST, retaining the flags.
+         * This is important for e.g. ARM_CP_SVE so that we still
+         * take the SVE trap if CPTR_EL3.EZ == 0.
          */
-        r2->fieldoffset = r->bank_fieldoffsets[ns];
-    }
-    if (state == ARM_CP_STATE_AA32) {
+        r2->type |= ARM_CP_CONST;
+        /*
+         * Usually, these registers become RES0, but there are a few
+         * special cases like VPIDR_EL2 which have a constant non-zero
+         * value with writes ignored.  So leave resetvalue as is.
+         *
+         * ARM_CP_SPECIAL_MASK has precedence, so removing the callbacks
+         * and offsets are not strictly necessary, but is potentially
+         * less confusing to debug later.
+         */
+        r2->readfn = NULL;
+        r2->writefn = NULL;
+        r2->raw_readfn = NULL;
+        r2->raw_writefn = NULL;
+        r2->resetfn = NULL;
+        r2->fieldoffset = 0;
+        r2->bank_fieldoffsets[0] = 0;
+        r2->bank_fieldoffsets[1] = 0;
+    } else {
+        bool isbanked = r->bank_fieldoffsets[0] && r->bank_fieldoffsets[1];
+
         if (isbanked) {
             /*
-             * If the register is banked then we don't need to migrate or
-             * reset the 32-bit instance in certain cases:
-             *
-             * 1) If the register has both 32-bit and 64-bit instances then we
-             *    can count on the 64-bit instance taking care of the
-             *    non-secure bank.
-             * 2) If ARMv8 is enabled then we can count on a 64-bit version
-             *    taking care of the secure bank.  This requires that separate
-             *    32 and 64-bit definitions are provided.
+             * Register is banked (using both entries in array).
+             * Overwriting fieldoffset as the array is only used to define
+             * banked registers but later only fieldoffset is used.
              */
-            if ((r->state == ARM_CP_STATE_BOTH && ns) ||
-                (arm_feature(&cpu->env, ARM_FEATURE_V8) && !ns)) {
+            r2->fieldoffset = r->bank_fieldoffsets[ns];
+        }
+        if (state == ARM_CP_STATE_AA32) {
+            if (isbanked) {
+                /*
+                 * If the register is banked then we don't need to migrate or
+                 * reset the 32-bit instance in certain cases:
+                 *
+                 * 1) If the register has both 32-bit and 64-bit instances
+                 *    then we can count on the 64-bit instance taking care
+                 *    of the non-secure bank.
+                 * 2) If ARMv8 is enabled then we can count on a 64-bit
+                 *    version taking care of the secure bank.  This requires
+                 *    that separate 32 and 64-bit definitions are provided.
+                 */
+                if ((r->state == ARM_CP_STATE_BOTH && ns) ||
+                    (arm_feature(env, ARM_FEATURE_V8) && !ns)) {
+                    r2->type |= ARM_CP_ALIAS;
+                }
+            } else if ((secstate != r->secure) && !ns) {
+                /*
+                 * The register is not banked so we only want to allow
+                 * migration of the non-secure instance.
+                 */
                 r2->type |= ARM_CP_ALIAS;
             }
-        } else if ((secstate != r->secure) && !ns) {
-            /*
-             * The register is not banked so we only want to allow migration
-             * of the non-secure instance.
-             */
-            r2->type |= ARM_CP_ALIAS;
-        }
 
-        if (r->state == ARM_CP_STATE_BOTH && r->fieldoffset) {
+            if (r->state == ARM_CP_STATE_BOTH && r->fieldoffset) {
 #ifdef HOST_WORDS_BIGENDIAN
-            r2->fieldoffset += sizeof(uint32_t);
+                r2->fieldoffset += sizeof(uint32_t);
 #endif
+            }
         }
     }
 
@@ -8628,7 +8677,7 @@  static void add_cpreg_to_hashtable(ARMCPU *cpu, const ARMCPRegInfo *r,
      * multiple times. Special registers (ie NOP/WFI) are
      * never migratable and not even raw-accessible.
      */
-    if (r->type & ARM_CP_SPECIAL_MASK) {
+    if (r2->type & ARM_CP_SPECIAL_MASK) {
         r2->type |= ARM_CP_NO_RAW;
     }
     if ((r->crm == CP_ANY && crm != 0) ||