Re: [PATCH] arm64: archrandom: avoid trapping ID register read in __cpu_has_rng()

[Will Deacon]

On Tue, Aug 04, 2026 at 04:50:35PM +0100, Aman Priyadarshi wrote:
> > On 4 Aug 2026, at 15:56, Will Deacon <will@xxxxxxxxxx> wrote:
> > On Fri, Jul 31, 2026 at 06:26:44PM +0100, Aman Priyadarshi wrote:
> >>> On 31 Jul 2026, at 15:43, Will Deacon <will@xxxxxxxxxx> wrote:
> >>> On Mon, Jul 20, 2026 at 03:06:15PM +0100, Aman Priyadarshi wrote:
> >>>> diff --git a/arch/arm64/include/asm/archrandom.h b/arch/arm64/include/asm/archrandom.h
> >>>> index 8babfbe31f95..8067e9a35641 100644
> >>>> --- a/arch/arm64/include/asm/archrandom.h
> >>>> +++ b/arch/arm64/include/asm/archrandom.h
> >>>> @@ -61,8 +61,22 @@ static inline bool __arm64_rndrrs(unsigned long *v)
> >>>> 
> >>>> static __always_inline bool __cpu_has_rng(void)
> >>>> {
> >>>> - if (unlikely(!system_capabilities_finalized() && !preemptible()))
> >>>> - return this_cpu_has_cap(ARM64_HAS_RNG);
> >>>> + if (unlikely(!system_capabilities_finalized() && !preemptible())) {
> >>>> + /*
> >>>> + * Until the ARM64_HAS_RNG alternative is patched we can't use
> >>>> + * the static-branch form, so consult the feature register
> >>>> + * directly. Don't use this_cpu_has_cap() here: it reads
> >>>> + * ID_AA64ISAR0_EL1 from hardware on every call, under
> >>>> + * virtualization each ID register read traps to the hypervisor
> >>>> + * (HCR_EL2.TID3) -- producing a storm of vmexits during boot.
> >>>> + * The sanitised value is cached in memory.
> >>>> + */
> >>>> + u64 isar0 = read_sanitised_ftr_reg(SYS_ID_AA64ISAR0_EL1);
> >>>> +
> >>>> + return cpuid_feature_extract_unsigned_field(isar0,
> >>>> + ID_AA64ISAR0_EL1_RNDR_SHIFT) >=
> >>>> + ID_AA64ISAR0_EL1_RNDR_IMP;
> >>>> + }
> >>> 
> >>> You can probably rewrite this a little more cleanly along the lines of
> >>> the (not even compile-tested) diff below. I was about to do that, but
> >>> then I got a bit confused by the whole thing. The preemptible() check is
> >>> presumably not needed if we're accessing the in-memory feature registers
> >>> rather than the per-CPU id registers, but then how do you handle races
> >>> with concurrent updates to the "safe value" made by CPUs concurrently
> >>> coming online?
> >> 
> >> I kept preemptible() check for this exact reason: the updates made by CPUs
> >> concurrently coming online will always take the downgrade path (a secondary
> >> CPU can clear RNDR, never set it), and therefore by taking the non-preemptible
> >> branch I can guarantee that the pinned CPU supports the said feature.
> >> I agree, this assumes that a secondary CPU folds its own ID registers into sys_val
> >> before it can ever be a randomness consumer, but looking at the code that seems
> >> the case, please feel free to correct me.
> >> Besides, in my opinion, it's hard to argue correctness of this code without
> >> preemptible() check.
> > 
> > My point is that this change introduces a data race on 'reg->sys_val' for
> > the ID_AA64ISAR0_EL1 entry in the arm64_ftr_regs array.
> 
> Agreed, you're right. My reasoning was that existing read_sanitised_ftr_reg() callers
> already race with sys_val updates during hotplug CPU bringup, so this wasn't a
> new problem. But I agree it's not much of a defence.

I think system_capabilities_finalized() is supposed to handle that race:

  * If the capabilities are not finalised, you shouldn't query the sanitised
    feature registers and they will be updated by CPUs coming online.

  * If the capabailities are finalised, you can query the sanitised feature
    registers and CPUs coming online must be compatible with them.

That's not to say we're bug-free here, though. The hw_breakpoint code
queries the sanitised view of SYS_ID_AA64DFR0_EL1 before it should and
you're trying to add another caller in the random code.

> It looks like an easy fix, though: mark the reader and the writer. What do you think
> of the patch below? I'm happy to post it as a separate patch ahead of this fix once
> you confirm it works for you.

I don't think we should try to make this thread safe. It would be much
better if this_cpu_has_cap() could query the saved id registers in
this_cpu_ptr(&cpu_data) instead of reading the id registers again.

Something along the lines of the untested and incomplete diff below...

Will

--->8

diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
index a57870fa96db..b72e9e22ca79 100644
--- a/arch/arm64/include/asm/cpufeature.h
+++ b/arch/arm64/include/asm/cpufeature.h
@@ -641,7 +641,6 @@ void __init setup_user_features(void);
 void check_local_cpu_capabilities(void);
 
 u64 read_sanitised_ftr_reg(u32 id);
-u64 __read_sysreg_by_encoding(u32 sys_id);
 
 static inline bool cpu_supports_mixed_endian_el0(void)
 {
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 9a22df0c5120..6462863fb78b 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1533,11 +1533,7 @@ EXPORT_SYMBOL_GPL(read_sanitised_ftr_reg);
 #define read_sysreg_case(r)    \
        case r:         val = read_sysreg_s(r); break;
 
-/*
- * __read_sysreg_by_encoding() - Used by a STARTING cpu before cpuinfo is populated.
- * Read the system register on the current CPU
- */
-u64 __read_sysreg_by_encoding(u32 sys_id)
+static u64 __read_sysreg_by_encoding(struct cpuinfo_arm64 *info, u32 sys_id)
 {
        struct arm64_ftr_reg *regp;
        u64 val;
@@ -1578,7 +1574,9 @@ u64 __read_sysreg_by_encoding(u32 sys_id)
        read_sysreg_case(SYS_ID_AA64MMFR2_EL1);
        read_sysreg_case(SYS_ID_AA64MMFR3_EL1);
        read_sysreg_case(SYS_ID_AA64MMFR4_EL1);
-       read_sysreg_case(SYS_ID_AA64ISAR0_EL1);
+       case SYS_ID_AA64ISAR0_EL1:
+               val = info->reg_id_aa64isar0;
+               break;
        read_sysreg_case(SYS_ID_AA64ISAR1_EL1);
        read_sysreg_case(SYS_ID_AA64ISAR2_EL1);
        read_sysreg_case(SYS_ID_AA64ISAR3_EL1);
@@ -1639,11 +1637,19 @@ feature_matches(u64 reg, const struct arm64_cpu_capabilities *entry)
 static u64
 read_scoped_sysreg(const struct arm64_cpu_capabilities *entry, int scope)
 {
-       WARN_ON(scope == SCOPE_LOCAL_CPU && preemptible());
-       if (scope == SCOPE_SYSTEM)
+       switch (scope) {
+       case SCOPE_SYSTEM:
                return read_sanitised_ftr_reg(entry->sys_reg);
-       else
-               return __read_sysreg_by_encoding(entry->sys_reg);
+       case SCOPE_LOCAL_CPU:
+               WARN_ON(preemptible());
+               fallthrough;
+       case SCOPE_BOOT_CPU:
+               return __read_sysreg_by_encoding(this_cpu_ptr(&cpu_data),
+                                                entry->sys_reg);
+       default:
+               BUG();
+               return 0;
+       }
 }
 
 static bool
@@ -2232,7 +2238,7 @@ static bool has_address_auth_cpucap(const struct arm64_cpu_capabilities *entry,
        if (scope & SCOPE_BOOT_CPU)
                return boot_val >= entry->min_field_value;
        /* Now check for the secondary CPUs with SCOPE_LOCAL_CPU scope */
-       sec_val = cpuid_feature_extract_field(__read_sysreg_by_encoding(entry->sys_reg),
+       sec_val = cpuid_feature_extract_field(read_scoped_sysreg(entry, SCOPE_LOCAL_CPU),
                                              entry->field_pos, entry->sign);
        return (sec_val >= entry->min_field_value) && (sec_val == boot_val);
 }