Re: [PATCH v2] arm64: Defer the GMID_EL1 read to {init,update}_cpu_features()
Will Deacon <[email protected]>
| Newsgroups | dev.linux.lists.kvmarm,org.infradead.lists.linux-arm-kernel,org.kernel.vger.linux-kernel |
|---|---|
| Message-ID | <aoxCk5lFQv6lFwHQ@willie-the-truck> |
On Mon, Aug 24, 2026 at 08:10:04AM +0100, Fuad Tabba wrote:
> __cpuinfo_store_cpu() gates the GMID_EL1 read on the raw
> ID_AA64PFR1_EL1, so it reads the register on MTE hardware even when the
> kernel has disabled MTE (CONFIG_ARM64_MTE=n or arm64.nomte). KVM sets
> HCR_EL2.TID5 in that case, which traps the read to EL2 and injects an
> UNDEF:
>
> Internal error: Oops - Undefined instruction: 0000000002000000 [#1]
> pc : __cpuinfo_store_cpu+0xf4/0x264
> Call trace:
> __cpuinfo_store_cpu+0xf4/0x264 (P)
> secondary_start_kernel+0xc8/0x1d0
> __secondary_switched+0xc0/0xc4
> Kernel panic - not syncing: Attempted to kill the idle task!
>
> Only pKVM is affected, and only for a CPU that is offlined and brought
> back online. Every CPU's first bring-up precedes KVM's initcall, and
> pKVM refuses CPU_ON for a CPU that was not online when KVM initialised,
> so hotplug is the only path to cpuinfo_store_cpu() with TID5 set. pKVM's
> PSCI CPU_ON relay sets the host HCR before the CPU enters EL1, whereas
> plain nVHE sets it at CPUHP_AP_KVM_ONLINE, after cpuinfo_store_cpu().
>
> Defer the read to {init,update}_cpu_features() and gate it on the
> sanitised ID register, as MPAM already does. system_supports_mte()
> cannot serve as the gate, as update_cpu_features() also runs during
> initial SMP bring-up, before smp_cpus_done() calls
> setup_system_features(). The init path gains the CONFIG_ARM64_MTE test
> the update path already had, leaving SYS_GMID_EL1 uninitialised when
> MTE is compiled out, where its only other user (lib/mte.S) is not
> built.
>
> Fixes: f35abcbb8a084 ("KVM: arm64: Trap MTE access and discovery when MTE is disabled")
> Cc: [email protected]
> Signed-off-by: Fuad Tabba <[email protected]>
> ---
[...]
> diff --git a/arch/arm64/kernel/cpuinfo.c b/arch/arm64/kernel/cpuinfo.c
> index d50e2a9b066b3..c8967f185e3dd 100644
> --- a/arch/arm64/kernel/cpuinfo.c
> +++ b/arch/arm64/kernel/cpuinfo.c
> @@ -502,12 +502,14 @@ static void __cpuinfo_store_cpu(struct cpuinfo_arm64 *info)
> info->reg_id_aa64smfr0 = read_cpuid(ID_AA64SMFR0_EL1);
> info->reg_id_aa64fpfr0 = read_cpuid(ID_AA64FPFR0_EL1);
>
> - if (id_aa64pfr1_mte(info->reg_id_aa64pfr1))
> - info->reg_gmid = read_cpuid(GMID_EL1);
> -
> if (id_aa64pfr0_32bit_el0(info->reg_id_aa64pfr0))
> __cpuinfo_store_cpu_32bit(&info->aarch32);
>
> + /*
> + * info->reg_gmid deferred to {init,update}_cpu_features because
> + * reading it traps to EL2 when MTE is disabled.
> + */
I don't think we should defer this, as I've been actively doing the
opposite for parallel CPU onlining (where the ID registers can be read
concurrently by incoming CPUs to amortise the cost of a trap) and also
for the RNG traps during early boot:
https://lore.kernel.org/all/annJ0oDB2HObQC5j@willie-the-truck/
If you look at the diff I sent in the thread above (I didn't get a
reply), the idea is that __read_sysreg_by_encoding() reads from the ID
register values stashed by cpuinfo_store_cpu(). So you could use that
to check id_aa64pfr1 before reading gmid, as it will give you the
sanitised view.
The only snag is that I didn't convert all the registers over, so
there's some work there.
WDYT?
Will