linux-kernel - Re: [PATCH] KVM: arm64: Only default to enabling SVE when present

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZQJODdfwZLxc9o1l@linux.dev>
Date:   Thu, 14 Sep 2023 00:04:29 +0000
From:   Oliver Upton <oliver.upton@...ux.dev>
To:     Mark Brown <broonie@...nel.org>
Cc:     Marc Zyngier <maz@...nel.org>, James Morse <james.morse@....com>,
        Suzuki K Poulose <suzuki.poulose@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.linux.dev,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] KVM: arm64: Only default to enabling SVE when present

Hi Mark,

On Wed, Sep 13, 2023 at 07:34:16PM +0100, Mark Brown wrote:
> For unclear reasons our handling of SVE and SME when setting the default
> value of CPTR_EL2 for VHE mode is inconsistent. For normal VHE we
> unconditionally set CPTR_EL2.ZEN to 0b01 but only set the equivalent
> field CPTR_EL2.SMEN to 0b01 if SME is present, for hVHE we will always
> set the field 0b11 if SVE is not supported. Given the similarities
> between the two extensions it would generally be expected that the code
> handling SVE and SME would be very similar.
> 
> Since CPTR_ELx.ZEN is RES0 when SVE is not implemented it is probably not
> harmful to try to set the bits but it is better practice to not set
> unimplemented bits so resolve the inconsistency in favour of checking if
> SVE is present too.

It is entirely possible that implementers 'disable' a feature by hiding
it from the ID register, leaving the control bits completely functional.
These systems are at odds with the architecture, though we probably
shouldn't engage in _deliberately_ hostile programming patterns in the
kernel :)

> FPSIMD is also in theory optional though there's probably much more work to
> handle the case where it is not implemented properly and that is not
> something we see in practical systems.
>
> Signed-off-by: Mark Brown <broonie@...nel.org>
> ---
>  arch/arm64/include/asm/kvm_emulate.h | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h
> index 3d6725ff0bf6..4cf53b4aa226 100644
> --- a/arch/arm64/include/asm/kvm_emulate.h
> +++ b/arch/arm64/include/asm/kvm_emulate.h
> @@ -584,15 +584,17 @@ static __always_inline u64 kvm_get_reset_cptr_el2(struct kvm_vcpu *vcpu)
>  	u64 val;
>  
>  	if (has_vhe()) {
> -		val = (CPACR_EL1_FPEN_EL0EN | CPACR_EL1_FPEN_EL1EN |
> -		       CPACR_EL1_ZEN_EL1EN);
> +		val = (CPACR_EL1_FPEN_EL0EN | CPACR_EL1_FPEN_EL1EN);
> +		if (cpus_have_final_cap(ARM64_SVE))
> +			val |= CPACR_EL1_ZEN_EL1EN;
>  		if (cpus_have_final_cap(ARM64_SME))
>  			val |= CPACR_EL1_SMEN_EL1EN;
>  	} else if (has_hvhe()) {
>  		val = (CPACR_EL1_FPEN_EL0EN | CPACR_EL1_FPEN_EL1EN);
>  
> -		if (!vcpu_has_sve(vcpu) ||
> -		    (vcpu->arch.fp_state != FP_STATE_GUEST_OWNED))
> +		if (cpus_have_final_cap(ARM64_SVE) &&
> +		    (!vcpu_has_sve(vcpu) ||
> +		     (vcpu->arch.fp_state != FP_STATE_GUEST_OWNED)))

vcpu_has_sve() already tests system_supports_sve(), so I don't believe
this hunk is necessary.

-- 
Thanks,
Oliver