Re: [PULL 17/21] target/arm: implement FEAT_RNG_TRAP for RNDR/RNDRRS

["Alex Bennée" <alex.bennee@linaro.org>]

"Jason L. Wright'" <wrigjl@proton.me> writes:

> On Sun, Jun 07, 2026 at 06:42:30PM +0800, Zenghui Yu wrote:
>> + Alexander and qemu-arm (for HVF/arm),
>> 
>> On 5/29/26 7:47 PM, Peter Maydell wrote:
>> > From: Jason Wright <wrigjl@proton.me>
>> >
>> > Add an .accessfn to the RNDR and RNDRRS system registers that traps
>> > reads to EL3 when SCR_EL3.TRNDR is set, as required by FEAT_RNG_TRAP.
>> > Mark SCR_EL3.TRNDR (bit 40) as a writable field in scr_write() when
>> > the CPU advertises the feature. The pseudocode in DDI0487 revision M.b
>> > shows the trap firing from EL0, EL1, EL2, and EL3, so there is no
>> > check of arm_current_el().
>> >
>> > When FEAT_RNG_TRAP is implemented without FEAT_RNG, an RNDR/RNDRRS read
>> > with SCR_EL3.TRNDR=0 should UNDEF rather than succeed; handle that case
>> > in access_rndr(). Register the rndr_reginfo CP reg entries whenever either
>> > FEAT_RNG or FEAT_RNG_TRAP is implemented, so the accessfn fires even on a
>> > FEAT_RNG_TRAP-only CPU.
>> >
>> > When SCR_EL3.TRNDR is set, ID_AA64ISAR0_EL1.RNDR reads as 1 regardless
>> > of whether FEAT_RNG is implemented; give ID_AA64ISAR0_EL1 a readfn so it
>> > reports this at runtime, as we already do for ID_AA64PFR0_EL1.
>> >
>> > Suggested-by: Richard Henderson <richard.henderson@linaro.org>
>> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> > Signed-off-by: Jason Wright <wrigjl@proton.me>
>> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> > ---
>> >  target/arm/cpu-features.h |  5 ++++
>> >  target/arm/helper.c       | 58 +++++++++++++++++++++++++++++++++++----
>> >  2 files changed, 58 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/target/arm/cpu-features.h b/target/arm/cpu-features.h
>> > index 4e8d844fea..38a695ded7 100644
>> > --- a/target/arm/cpu-features.h
>> > +++ b/target/arm/cpu-features.h
>> > @@ -908,6 +908,11 @@ static inline bool isar_feature_aa64_rndr(const ARMISARegisters *id)
>> >      return FIELD_EX64_IDREG(id, ID_AA64ISAR0, RNDR) != 0;
>> >  }
>> >
>> > +static inline bool isar_feature_aa64_rng_trap(const ARMISARegisters *id)
>> > +{
>> > +    return FIELD_EX64_IDREG(id, ID_AA64PFR1, RNDR_TRAP) != 0;
>> > +}
>> > +
>> >  static inline bool isar_feature_aa64_tlbirange(const ARMISARegisters *id)
>> >  {
>> >      return FIELD_EX64_IDREG(id, ID_AA64ISAR0, TLB) == 2;
>> > diff --git a/target/arm/helper.c b/target/arm/helper.c
>> > index 34487eeaa3..9dd8fdfa41 100644
>> > --- a/target/arm/helper.c
>> > +++ b/target/arm/helper.c
>> > @@ -790,6 +790,9 @@ static void scr_write(CPUARMState *env, const ARMCPRegInfo *ri, uint64_t value)
>> >          if (cpu_isar_feature(aa64_fpmr, cpu)) {
>> >              valid_mask |= SCR_ENFPM;
>> >          }
>> > +        if (cpu_isar_feature(aa64_rng_trap, cpu)) {
>> > +            valid_mask |= SCR_TRNDR;
>> > +        }
>> >      } else {
>> >          valid_mask &= ~(SCR_RW | SCR_ST);
>> >          if (cpu_isar_feature(aa32_ras, cpu)) {
>> > @@ -5170,6 +5173,21 @@ static uint64_t id_aa64pfr0_read(CPUARMState *env, const ARMCPRegInfo *ri)
>> >      }
>> >      return pfr0;
>> >  }
>> > +
>> > +static uint64_t id_aa64isar0_read(CPUARMState *env, const ARMCPRegInfo *ri)
>> > +{
>> > +    ARMCPU *cpu = env_archcpu(env);
>> > +    uint64_t isar0 = GET_IDREG(&cpu->isar, ID_AA64ISAR0);
>> > +
>> > +    /*
>> > +     * When FEAT_RNG_TRAP is active (SCR_EL3.TRNDR set), ID_AA64ISAR0_EL1.RNDR
>> > +     * reads as 1 regardless of whether FEAT_RNG is implemented.
>> > +     */
>> > +    if (env->cp15.scr_el3 & SCR_TRNDR) {
>> > +        isar0 = FIELD_DP64(isar0, ID_AA64ISAR0, RNDR, 1);
>> > +    }
>> > +    return isar0;
>> > +}
>> >  #endif
>> >
>> >  /*
>> > @@ -5304,6 +5322,22 @@ static const ARMCPRegInfo pauth_reginfo[] = {
>> >        .fieldoffset = offsetof(CPUARMState, keys.apib.hi) },
>> >  };
>> >
>> > +static CPAccessResult access_rndr(CPUARMState *env, const ARMCPRegInfo *ri,
>> > +                                  bool isread)
>> > +{
>> > +    if (env->cp15.scr_el3 & SCR_TRNDR) {
>> > +        return CP_ACCESS_TRAP_EL3;
>> > +    }
>> > +    /*
>> > +     * Note that FEAT_RNG_TRAP may be implemented without FEAT_RNG.
>> > +     * In that case, if the trap is not enabled, the read undefs.
>> > +     */
>> > +    if (!cpu_isar_feature(aa64_rndr, env_archcpu(env))) {
>> > +        return CP_ACCESS_UNDEFINED;
>> > +    }
>> > +    return CP_ACCESS_OK;
>> > +}
>> > +
>> >  static uint64_t rndr_readfn(CPUARMState *env, const ARMCPRegInfo *ri)
>> >  {
>> >      Error *err = NULL;
>> > @@ -5335,11 +5369,11 @@ static const ARMCPRegInfo rndr_reginfo[] = {
>> >      { .name = "RNDR", .state = ARM_CP_STATE_AA64,
>> >        .type = ARM_CP_NO_RAW | ARM_CP_SUPPRESS_TB_END | ARM_CP_IO,
>> >        .opc0 = 3, .opc1 = 3, .crn = 2, .crm = 4, .opc2 = 0,
>> > -      .access = PL0_R, .readfn = rndr_readfn },
>> > +      .access = PL0_R, .accessfn = access_rndr, .readfn = rndr_readfn },
>> >      { .name = "RNDRRS", .state = ARM_CP_STATE_AA64,
>> >        .type = ARM_CP_NO_RAW | ARM_CP_SUPPRESS_TB_END | ARM_CP_IO,
>> >        .opc0 = 3, .opc1 = 3, .crn = 2, .crm = 4, .opc2 = 1,
>> > -      .access = PL0_R, .readfn = rndr_readfn },
>> > +      .access = PL0_R, .accessfn = access_rndr, .readfn = rndr_readfn },
>> >  };
>> >
>> >  static void dccvap_writefn(CPUARMState *env, const ARMCPRegInfo *ri,
>> > @@ -6522,11 +6556,24 @@ void register_cp_regs_for_features(ARMCPU *cpu)
>> >                .access = PL1_R, .type = ARM_CP_CONST,
>> >                .accessfn = access_tid3,
>> >                .resetvalue = 0 },
>> > +            /*
>> > +             * ID_AA64ISAR0_EL1 is not a plain ARM_CP_CONST in system
>> > +             * emulation because the RNDR field depends on SCR_EL3.TRNDR
>> > +             * at read time when FEAT_RNG_TRAP is implemented.
>> > +             */
>> >              { .name = "ID_AA64ISAR0_EL1", .state = ARM_CP_STATE_AA64,
>> >                .opc0 = 3, .opc1 = 0, .crn = 0, .crm = 6, .opc2 = 0,
>> > -              .access = PL1_R, .type = ARM_CP_CONST,
>> > +              .access = PL1_R,
>> > +#ifdef CONFIG_USER_ONLY
>> > +              .type = ARM_CP_CONST,
>> > +              .resetvalue = GET_IDREG(isar, ID_AA64ISAR0)
>> > +#else
>> > +              .type = ARM_CP_NO_RAW,
>> >                .accessfn = access_tid3,
>> > -              .resetvalue = GET_IDREG(isar, ID_AA64ISAR0)},
>> > +              .readfn = id_aa64isar0_read,
>> > +              .writefn = arm_cp_write_ignore
>> > +#endif
>> > +            },
>> 
>> A new assert() was triggered when booting guest on M1 since this change:
>> 
>> Assertion failed: (!(ri->type & ARM_CP_NO_RAW)), function hvf_arch_init_vcpu, file hvf.c, line 1442.
>> 
>> Thanks,
>> Zenghui
>> 
> Thanks for the report and the bisect, Zenghui. I can reproduce on M1 with:
>
> qemu-system-aarch64 -M virt,accel=hvf -cpu host \
>                       -nographic -display none -bios /dev/null
>
> ID_AA64PFR0_EL1 has the same NO_RAW + readfn shape that the FEAT_RNG_TRAP
> change gave ID_AA64ISAR0_EL1, and HVF already accommodates it by listing
> the cpreg in the SYNC_NO_RAW_REGS block in target/arm/hvf/sysreg.c.inc
> (so the assert loop skips it) and pushing QEMU's value to the vCPU at
> init time. Mirroring that pattern for ID_AA64ISAR0_EL1 clears the assert
> without disturbing the readfn semantics that the spec requires if a
> FEAT_RNG_TRAP-only CPU eventually appears.
>
> I'll send a fix-up as [PATCH] target/arm/hvf shortly.

This highlights why we need some bare metal MacOS machines in the
CI to exercise this code path.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro