Re: [PATCH 1/6] target/arm: Add FGWTE3_EL3

On Fri, 24 Jul 2026 at 04:07, Richard Henderson
<[email protected]> wrote:
>
> Signed-off-by: Richard Henderson <[email protected]>
> ---
>  target/arm/cpregs.h       |  1 +
>  target/arm/cpu-features.h |  5 +++++
>  target/arm/cpu.h          |  2 +-
>  target/arm/helper.c       | 19 +++++++++++++++++++
>  4 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/target/arm/cpregs.h b/target/arm/cpregs.h
> index 391c0e322b..34592679e4 100644
> --- a/target/arm/cpregs.h
> +++ b/target/arm/cpregs.h
> @@ -385,6 +385,7 @@ typedef enum CPAccessResult {
>  /* Indexes into fgt_write[] */
>  #define FGTREG_HFGWTR 0
>  #define FGTREG_HDFGWTR 1
> +#define FGTREG_FGWTE3 2
>  /* Indexes into fgt_exec[] */
>  #define FGTREG_HFGITR 0
>
> diff --git a/target/arm/cpu-features.h b/target/arm/cpu-features.h
> index fb5ed25ad0..75f0fc4169 100644
> --- a/target/arm/cpu-features.h
> +++ b/target/arm/cpu-features.h
> @@ -1467,6 +1467,11 @@ static inline bool isar_feature_aa64_asid2(const ARMISARegisters *id)
>      return FIELD_EX64_IDREG(id, ID_AA64MMFR4, ASID2) != 0;
>  }
>
> +static inline bool isar_feature_aa64_fgwte3(const ARMISARegisters *id)
> +{
> +    return FIELD_EX64_IDREG(id, ID_AA64MMFR4, FGWTE3) != 0;
> +}
> +
>  /*
>   * Note the E2H0 ID fields is signed, increasingly negative as more
>   * isn't implemented.
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 03a30afcbe..ca75e379eb 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -553,7 +553,7 @@ typedef struct CPUArchState {
>           * FEAT_FGT2 will add more elements to these arrays.
>           */
>          uint64_t fgt_read[2]; /* HFGRTR, HDFGRTR */
> -        uint64_t fgt_write[2]; /* HFGWTR, HDFGWTR */
> +        uint64_t fgt_write[3]; /* HFGWTR, HDFGWTR, FGWTE3 */
>          uint64_t fgt_exec[1]; /* HFGITR */

GICv5 also has some new fgt registers, incidentally; implementation
of them in my w-i-p gicv5 branch:
https://gitlab.com/pm215/qemu/-/commit/31900667e440a136c60b984c9e7c9e9a95d708df

I don't think there's any particular clash here, though. The
one thing I do wonder about is that FGWTE3 has only a
write trap version, whereas the gicv5 regs have both read
and write trap versions. We rely on being able to use the same
index for fgt_read[] and fgt_write[]. So I think maybe here we
should make fgt_read[] also 3 entries and comment it as
"FGWTE3 (unused)"? Unless we deliberately land the gicv5 traps
first, we'll need to add a gap there later anyway, and doing it
now means we won't forget this oddity.

>          /* RME registers */
> diff --git a/target/arm/helper.c b/target/arm/helper.c
> index af45234ad2..d410a9a1ec 100644
> --- a/target/arm/helper.c
> +++ b/target/arm/helper.c
> @@ -6331,6 +6331,21 @@ static const ARMCPRegInfo fpmr_reginfo[] = {
>      }
>  };
>
> +static void fgwte3_write(CPUARMState *env, const ARMCPRegInfo *ri,
> +                         uint64_t value)
> +{
> +    env->cp15.fgt_write[FGTREG_FGWTE3] |= value;

I think I would comment this as
     /* All bits are sticky and can only be cleared by CPU reset */
because the semantics are odd enough to look like maybe a mistake
otherwise.

> +}
> +
> +static const ARMCPRegInfo fgwte3_reginfo[] = {
> +    { .name = "FGWTE3_EL3", .state = ARM_CP_STATE_AA64,
> +      .opc0 = 3, .opc1 = 6, .crn = 1, .crm = 1, .opc2 = 5,
> +      .access = PL3_RW, .resetvalue = 0,
> +      .writefn = fgwte3_write, .raw_writefn = raw_write,
> +      .fieldoffset = offsetof(CPUARMState, cp15.fgt_write[FGTREG_FGWTE3])
> +    },
> +};
> +
>  void register_cp_regs_for_features(ARMCPU *cpu)
>  {
>      /* Register all the coprocessor registers based on feature bits */
> @@ -7642,6 +7657,10 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>          define_arm_cp_regs(cpu, ccsidr2_reginfo);
>      }
>
> +    if (cpu_isar_feature(aa64_fgwte3, cpu)) {
> +        define_arm_cp_regs(cpu, fgwte3_reginfo);
> +    }
> +
>      define_pm_cpregs(cpu);
>      define_gcs_cpregs(cpu);
>  }

Otherwise
Reviewed-by: Peter Maydell <[email protected]>

thanks
-- PMM