| Message ID | 20250130182309.717346-13-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | target/arm: Clean up some corner cases of sysreg traps | expand |
On 1/30/25 10:23, Peter Maydell wrote: > There are no longer any uses of CP_ACCESS_TRAP in access functions, > because we have converted them all to use either CP_ACCESS_TRAP_EL1 > or CP_ACCESS_TRAP_UNCATEGORIZED, as appropriate. Remove the handling > of bare CP_ACCESS_TRAP from the access_check_cp_reg() helper, so that > it now asserts if an access function returns it. Wording from an in-development patch? How can an access function return CP_ACCESS_TRAP when it has been removed? That said, the code is correct. Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On Mon, 10 Feb 2025 at 19:35, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 1/30/25 10:23, Peter Maydell wrote: > > There are no longer any uses of CP_ACCESS_TRAP in access functions, > > because we have converted them all to use either CP_ACCESS_TRAP_EL1 > > or CP_ACCESS_TRAP_UNCATEGORIZED, as appropriate. Remove the handling > > of bare CP_ACCESS_TRAP from the access_check_cp_reg() helper, so that > > it now asserts if an access function returns it. > > Wording from an in-development patch? How can an access function return CP_ACCESS_TRAP > when it has been removed? I think more that my wording wasn't super clear. It's kind of a two step thing: we make access_check_cp_reg assert() if you return that value (4), and we also change the constant name so it's clearer that you shouldn't be returning that value. > That said, the code is correct. > Reviewed-by: Richard Henderson <richard.henderson@linaro.org> thanks -- PMM
diff --git a/target/arm/cpregs.h b/target/arm/cpregs.h index fbf5798069d..fb3b84baa1e 100644 --- a/target/arm/cpregs.h +++ b/target/arm/cpregs.h @@ -328,12 +328,13 @@ typedef enum CPAccessResult { * Access fails due to a configurable trap or enable which would * result in a categorized exception syndrome giving information about * the failing instruction (ie syndrome category 0x3, 0x4, 0x5, 0x6, - * 0xc or 0x18). + * 0xc or 0x18). These traps are always to a specified target EL, + * never to the usual target EL. */ - CP_ACCESS_TRAP = (1 << 2), - CP_ACCESS_TRAP_EL1 = CP_ACCESS_TRAP | 1, - CP_ACCESS_TRAP_EL2 = CP_ACCESS_TRAP | 2, - CP_ACCESS_TRAP_EL3 = CP_ACCESS_TRAP | 3, + CP_ACCESS_TRAP_BIT = (1 << 2), + CP_ACCESS_TRAP_EL1 = CP_ACCESS_TRAP_BIT | 1, + CP_ACCESS_TRAP_EL2 = CP_ACCESS_TRAP_BIT | 2, + CP_ACCESS_TRAP_EL3 = CP_ACCESS_TRAP_BIT | 3, /* * Access fails and results in an exception syndrome 0x0 ("uncategorized"). diff --git a/target/arm/tcg/op_helper.c b/target/arm/tcg/op_helper.c index c69d2ac643f..fcee11e29ad 100644 --- a/target/arm/tcg/op_helper.c +++ b/target/arm/tcg/op_helper.c @@ -853,21 +853,24 @@ const void *HELPER(access_check_cp_reg)(CPUARMState *env, uint32_t key, fail: excp = EXCP_UDEF; - switch (res & ~CP_ACCESS_EL_MASK) { - case CP_ACCESS_TRAP: + switch (res) { + /* CP_ACCESS_TRAP* traps are always direct to a specified EL */ + case CP_ACCESS_TRAP_EL3: /* * If EL3 is AArch32 then there's no syndrome register; the cases * where we would raise a SystemAccessTrap to AArch64 EL3 all become * raising a Monitor trap exception. (Because there's no visible * syndrome it doesn't matter what we pass to raise_exception().) */ - if ((res & CP_ACCESS_EL_MASK) == 3 && !arm_el_is_aa64(env, 3)) { + if (!arm_el_is_aa64(env, 3)) { excp = EXCP_MON_TRAP; } break; + case CP_ACCESS_TRAP_EL2: + case CP_ACCESS_TRAP_EL1: + break; case CP_ACCESS_TRAP_UNCATEGORIZED: - /* Only CP_ACCESS_TRAP traps are direct to a specified EL */ - assert((res & CP_ACCESS_EL_MASK) == 0); + /* CP_ACCESS_TRAP_UNCATEGORIZED is never direct to a specified EL */ if (cpu_isar_feature(aa64_ids, cpu) && isread && arm_cpreg_in_idspace(ri)) { /*
There are no longer any uses of CP_ACCESS_TRAP in access functions, because we have converted them all to use either CP_ACCESS_TRAP_EL1 or CP_ACCESS_TRAP_UNCATEGORIZED, as appropriate. Remove the handling of bare CP_ACCESS_TRAP from the access_check_cp_reg() helper, so that it now asserts if an access function returns it. Rename CP_ACCESS_TRAP to CP_ACCESS_TRAP_BIT, to make it clearer that this is an internal-only definition, not something that it makes sense to return from an access function. This should help to avoid future bugs where we return the wrong syndrome value by mistake. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target/arm/cpregs.h | 11 ++++++----- target/arm/tcg/op_helper.c | 13 ++++++++----- 2 files changed, 14 insertions(+), 10 deletions(-)