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

On 4 Aug 2026, at 15:56, Will Deacon [off-list ref] wrote:

On Fri, Jul 31, 2026 at 06:26:44PM +0100, Aman Priyadarshi wrote:

quoted

quoted

On 31 Jul 2026, at 15:43, Will Deacon [off-list ref] wrote:
On Mon, Jul 20, 2026 at 03:06:15PM +0100, Aman Priyadarshi wrote:

quoted

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.

Will

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.
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.

- Aman Priyadarshi

--->8

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 9a22df0c5120..e1c10a23da3c 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c

@@ -1235,18 +1235,20 @@ void __init init_cpu_features(struct cpuinfo_arm64 *info)
 static void update_cpu_ftr_reg(struct arm64_ftr_reg *reg, u64 new)
 {
        const struct arm64_ftr_bits *ftrp;
+       u64 sys_val = reg->sys_val;
 
        for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
-               s64 ftr_cur = arm64_ftr_value(ftrp, reg->sys_val);
+               s64 ftr_cur = arm64_ftr_value(ftrp, sys_val);
                s64 ftr_new = arm64_ftr_value(ftrp, new);
 
                if (ftr_cur == ftr_new)
                        continue;
                /* Find a safe value */
                ftr_new = arm64_ftr_safe_value(ftrp, ftr_new, ftr_cur);
-               reg->sys_val = arm64_ftr_set_value(ftrp, reg->sys_val, ftr_new);
+               sys_val = arm64_ftr_set_value(ftrp, sys_val, ftr_new);
        }
 
+       WRITE_ONCE(reg->sys_val, sys_val);
 }
 
 static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot)

@@ -1526,7 +1528,8 @@ u64 read_sanitised_ftr_reg(u32 id)
 
        if (!regp)
                return 0;
-       return regp->sys_val;
+
+       return READ_ONCE(regp->sys_val);
 }
 EXPORT_SYMBOL_GPL(read_sanitised_ftr_reg);