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

[Will Deacon <will@kernel.org>]

On Fri, Jul 31, 2026 at 06:26:44PM +0100, Aman Priyadarshi wrote:
> > On 31 Jul 2026, at 15:43, Will Deacon <will@kernel.org> 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.

Will