On Fri, Jan 03, 2025 at 02:26:35PM +0000, Marc Zyngier wrote:
> The hwcaps code that exposes SVE features to userspace only
> considers ID_AA64ZFR0_EL1, while this is only valid when
> ID_AA64PFR0_EL1.SVE advertises that SVE is actually supported.
>
> The expectations are that when ID_AA64PFR0_EL1.SVE is 0, the
> ID_AA64ZFR0_EL1 register is also 0. So far, so good.
>
> Things become a bit more interesting if the HW implements SME.
> In this case, a few ID_AA64ZFR0_EL1 fields indicate *SME*
> features. And these fields overlap with their SVE interpretations.
> But the architecture says that the SME and SVE feature sets must
> match, so we're still hunky-dory.
>
> This goes wrong if the HW implements SME, but not SVE. In this
> case, we end-up advertising some SVE features to userspace, even
> if the HW has none. That's because we never consider whether SVE
> is actually implemented. Oh well.
>
> Fix it by restricting all SVE capabilities to ID_AA64PFR0_EL1.SVE
> being non-zero.
>
> Reported-by: Catalin Marinas <catalin.marinas@xxxxxxx>
> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> Cc: Will Deacon <will@xxxxxxxxxx>
> Cc: Mark Rutland <mark.rutland@xxxxxxx>
> Cc: Mark Brown <broonie@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx
I'd add:
Fixes: 06a916feca2b ("arm64: Expose SVE2 features for userspace")
While at the time the code was correct, the architecture messed up our
assumptions with the introduction of SME.
> @@ -3022,6 +3027,13 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
> .matches = match, \
> }
>
> +#define HWCAP_CAP_MATCH_ID(match, reg, field, min_value, cap_type, cap) \
> + { \
> + __HWCAP_CAP(#cap, cap_type, cap) \
> + HWCAP_CPUID_MATCH(reg, field, min_value) \
> + .matches = match, \
> + }
Do we actually need this macro?
> +
> #ifdef CONFIG_ARM64_PTR_AUTH
> static const struct arm64_cpu_capabilities ptr_auth_hwcap_addr_matches[] = {
> {
> @@ -3050,6 +3062,18 @@ static const struct arm64_cpu_capabilities ptr_auth_hwcap_gen_matches[] = {
> };
> #endif
>
> +#ifdef CONFIG_ARM64_SVE
> +static bool has_sve(const struct arm64_cpu_capabilities *cap, int scope)
> +{
> + u64 aa64pfr0 = __read_scoped_sysreg(SYS_ID_AA64PFR0_EL1, scope);
> +
> + if (FIELD_GET(ID_AA64PFR0_EL1_SVE, aa64pfr0) < ID_AA64PFR0_EL1_SVE_IMP)
> + return false;
> +
> + return has_user_cpuid_feature(cap, scope);
> +}
> +#endif
We can name this has_sve_feature() and use it with the existing
HWCAP_CAP_MATCH() macro. I think it would look identical.
We might even be able to use system_supports_sve() directly and avoid
changing read_scoped_sysreg(). setup_user_features() is called in
smp_cpus_done() after setup_system_features(), so using
system_supports_sve() directly should be fine here.
--
Catalin
