On Sat, 30 Aug 2025 at 17:18, Richard Henderson
<[email protected]> wrote:
>
> If PSTATE.EXLOCK is set, and the GCS EXLOCK enable bit is set,
> and nested virt is in the appropriate state, then we need to
> raise an EXLOCK exception.
>
> Since PSTATE.EXLOCK cannot be set without GCS being present
> and enabled, no explicit check for GCS is required.
>
> Reviewed-by: Pierrick Bouvier <[email protected]>
> Signed-off-by: Richard Henderson <[email protected]>
> ---
> target/arm/cpregs.h | 3 ++
> target/arm/cpu.h | 1 +
> target/arm/helper.c | 83 +++++++++++++++++++++++++++++++++++---
> target/arm/tcg/op_helper.c | 4 ++
> 4 files changed, 85 insertions(+), 6 deletions(-)
>
> diff --git a/target/arm/cpregs.h b/target/arm/cpregs.h
> index bc6adf5956..15894332b2 100644
> --- a/target/arm/cpregs.h
> +++ b/target/arm/cpregs.h
> @@ -346,6 +346,9 @@ typedef enum CPAccessResult {
> * specified target EL.
> */
> CP_ACCESS_UNDEFINED = (2 << 2),
> +
> + /* Access fails with EXLOCK, a GCS exception syndrome. */
> + CP_ACCESS_EXLOCK = (3 << 2),
The values in this enum are documented as to whether they can
validly include the target-EL in the low bits. In this case
we can't, so I would expand the comment to say so:
/*
* Access fails with EXLOCK, a GCS exception syndrome.
* These traps are always to the current execution EL,
* which is the same as the usual target EL because
* they cannot occur from EL0.
*/
(Last time I was in this area of the code I was tempted to
drop the idea of "low bits might indicate target EL" and
just have CP_ACCESS_TRAP_EL* be normal enum values, rolling
the two switch() statements at the bottom of access_check_cp_reg
together. But perhaps some future trap type will also want to
go to a specified-target-EL...)
> } CPAccessResult;
>
> /* Indexes into fgt_read[] */
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index d5a5152a9c..17902eb40d 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -1511,6 +1511,7 @@ void pmu_init(ARMCPU *cpu);
> #define PSTATE_C (1U << 29)
> #define PSTATE_Z (1U << 30)
> #define PSTATE_N (1U << 31)
> +#define PSTATE_EXLOCK (1ULL << 34)
> #define PSTATE_NZCV (PSTATE_N | PSTATE_Z | PSTATE_C | PSTATE_V)
> #define PSTATE_DAIF (PSTATE_D | PSTATE_A | PSTATE_I | PSTATE_F)
> #define CACHED_PSTATE_BITS (PSTATE_NZCV | PSTATE_DAIF | PSTATE_BTYPE)
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index 83a7d6ae36..2f19695d82 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -3432,6 +3432,77 @@ static CPAccessResult access_nv1(CPUARMState *env,
> const ARMCPRegInfo *ri,
> return CP_ACCESS_OK;
> }
>
> +static CPAccessResult access_exlock_el1(CPUARMState *env,
> + const ARMCPRegInfo *ri, bool isread)
> +{
> + int el = arm_current_el(env);
> +
> + if (el == 1) {
> + uint64_t hcr = arm_hcr_el2_eff(env);
> +
> + /*
> + * EXLOCK check is disabled for NVx in 'x11'.
> + * Since we have to diagnose that, dispatch NV1 trap too.
> + */
> + if ((hcr & HCR_NV) && (hcr & HCR_NV1)) {
> + if (hcr & HCR_NV2) {
> + return CP_ACCESS_OK;
> + }
> + return CP_ACCESS_TRAP_EL2;
> + }
> + }
> +
> + if (!isread &&
> + (env->pstate & PSTATE_EXLOCK) &&
> + (el_is_in_host(env, el) ? el == 2 : el == 1) &&
> + (env->cp15.gcscr_el[el] & GCSCR_EXLOCKEN)) {
> + return CP_ACCESS_EXLOCK;
> + }
> + return CP_ACCESS_OK;
> +}
> +
> +static CPAccessResult access_exlock_el2(CPUARMState *env,
> + const ARMCPRegInfo *ri, bool isread)
> +{
> + int el = arm_current_el(env);
> +
> + if (el == 3) {
> + return CP_ACCESS_OK;
> + }
> + if (el == 1) {
> + uint64_t hcr = arm_hcr_el2_eff(env);
> +
> + /*
> + * EXLOCK check is disabled for NVx in 'xx1'.
"not in 'xx1'", if I'm reading the pseudocode correctly ?
> + * Since we have to diagnose that, dispatch NV1 trap too.
> + */
> + if (hcr & HCR_NV) {
> + if (hcr & HCR_NV2) {
> + return CP_ACCESS_OK;
> + }
> + return CP_ACCESS_TRAP_EL2;
> + }
I find this confusing. Before, we had no access fn for
these ELR_EL2 and SPSR_EL2 registers. So the changes
for GCS should not be adding extra trap cases that we
weren't doing before: just do the EXLOCK check when you
need to, otherwise return ACCESS_OK and let the existing
code continue to handle the NV traps/redirections.
Otherwise I have to reread all that code to remember
how it works and what order it does all the checks and
redirects in to confirm that we're doing things in the
right order.
(This is different from access_exlock_el1(), where we
are replacing the use of access_nv1() and so need to
do the same tests it does.)
Incidentally if we get to this function and el == 1 then
I'm pretty sure that HCR_NV must be set (or we would have
UNDEFed because ri->access permits access only at EL2 and
above). So for us we don't need to do any test on NVx
to bypass the EXLOCK check: we can do it unconditionally here.
> + }
> +
> + if (!isread &&
> + (env->pstate & PSTATE_EXLOCK) &&
> + (env->cp15.gcscr_el[el] & GCSCR_EXLOCKEN)) {
> + return CP_ACCESS_EXLOCK;
> + }
> + return CP_ACCESS_OK;
> +}
> +
> +static CPAccessResult access_exlock_el3(CPUARMState *env,
> + const ARMCPRegInfo *ri, bool isread)
> +{
> + if (!isread &&
> + (env->pstate & PSTATE_EXLOCK) &&
> + (env->cp15.gcscr_el[3] & GCSCR_EXLOCKEN)) {
> + return CP_ACCESS_EXLOCK;
> + }
> + return CP_ACCESS_OK;
> +}
> +
> #ifdef CONFIG_USER_ONLY
> /*
> * `IC IVAU` is handled to improve compatibility with JITs that dual-map
> their
> @@ -3603,13 +3674,13 @@ static const ARMCPRegInfo v8_cp_reginfo[] = {
> { .name = "ELR_EL1", .state = ARM_CP_STATE_AA64,
> .type = ARM_CP_ALIAS,
> .opc0 = 3, .opc1 = 0, .crn = 4, .crm = 0, .opc2 = 1,
> - .access = PL1_RW, .accessfn = access_nv1,
> + .access = PL1_RW, .accessfn = access_exlock_el1,
> .nv2_redirect_offset = 0x230 | NV2_REDIR_NV1,
> .fieldoffset = offsetof(CPUARMState, elr_el[1]) },
> { .name = "SPSR_EL1", .state = ARM_CP_STATE_AA64,
> .type = ARM_CP_ALIAS,
> .opc0 = 3, .opc1 = 0, .crn = 4, .crm = 0, .opc2 = 0,
> - .access = PL1_RW, .accessfn = access_nv1,
> + .access = PL1_RW, .accessfn = access_exlock_el1,
> .nv2_redirect_offset = 0x160 | NV2_REDIR_NV1,
> .fieldoffset = offsetof(CPUARMState, banked_spsr[BANK_SVC]) },
> /*
> @@ -4080,7 +4151,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
> { .name = "ELR_EL2", .state = ARM_CP_STATE_AA64,
> .type = ARM_CP_ALIAS | ARM_CP_NV2_REDIRECT,
> .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 0, .opc2 = 1,
> - .access = PL2_RW,
> + .access = PL2_RW, .accessfn = access_exlock_el2,
> .fieldoffset = offsetof(CPUARMState, elr_el[2]) },
> { .name = "ESR_EL2", .state = ARM_CP_STATE_BOTH,
> .type = ARM_CP_NV2_REDIRECT,
> @@ -4098,7 +4169,7 @@ static const ARMCPRegInfo el2_cp_reginfo[] = {
> { .name = "SPSR_EL2", .state = ARM_CP_STATE_AA64,
> .type = ARM_CP_ALIAS | ARM_CP_NV2_REDIRECT,
> .opc0 = 3, .opc1 = 4, .crn = 4, .crm = 0, .opc2 = 0,
> - .access = PL2_RW,
> + .access = PL2_RW, .accessfn = access_exlock_el2,
> .fieldoffset = offsetof(CPUARMState, banked_spsr[BANK_HYP]) },
> { .name = "VBAR_EL2", .state = ARM_CP_STATE_BOTH,
> .opc0 = 3, .opc1 = 4, .crn = 12, .crm = 0, .opc2 = 0,
> @@ -4380,7 +4451,7 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
> { .name = "ELR_EL3", .state = ARM_CP_STATE_AA64,
> .type = ARM_CP_ALIAS,
> .opc0 = 3, .opc1 = 6, .crn = 4, .crm = 0, .opc2 = 1,
> - .access = PL3_RW,
> + .access = PL3_RW, .accessfn = access_exlock_el3,
> .fieldoffset = offsetof(CPUARMState, elr_el[3]) },
> { .name = "ESR_EL3", .state = ARM_CP_STATE_AA64,
> .opc0 = 3, .opc1 = 6, .crn = 5, .crm = 2, .opc2 = 0,
> @@ -4391,7 +4462,7 @@ static const ARMCPRegInfo el3_cp_reginfo[] = {
> { .name = "SPSR_EL3", .state = ARM_CP_STATE_AA64,
> .type = ARM_CP_ALIAS,
> .opc0 = 3, .opc1 = 6, .crn = 4, .crm = 0, .opc2 = 0,
> - .access = PL3_RW,
> + .access = PL3_RW, .accessfn = access_exlock_el3,
> .fieldoffset = offsetof(CPUARMState, banked_spsr[BANK_MON]) },
> { .name = "VBAR_EL3", .state = ARM_CP_STATE_AA64,
> .opc0 = 3, .opc1 = 6, .crn = 12, .crm = 0, .opc2 = 0,
> diff --git a/target/arm/tcg/op_helper.c b/target/arm/tcg/op_helper.c
> index 46a3b911ec..56e117c01e 100644
> --- a/target/arm/tcg/op_helper.c
> +++ b/target/arm/tcg/op_helper.c
> @@ -887,6 +887,10 @@ const void *HELPER(access_check_cp_reg)(CPUARMState
> *env, uint32_t key,
> }
> syndrome = syn_uncategorized();
> break;
> + case CP_ACCESS_EXLOCK:
> + /* CP_ACCESS_EXLOCK is always directed to the current EL */
", which is going to be the same as the usual target EL."
> + syndrome = syn_gcs_exlock();
> + break;
> default:
> g_assert_not_reached();
> }
thanks
-- PMM
