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

From: Oliver Upton <oliver.upton@linux.dev>
To: Mark Brown <broonie@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>, James Morse <james.morse@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] KVM: arm64: Only default to enabling SVE when present
Date: Thu, 14 Sep 2023 00:04:29 +0000	[thread overview]
Message-ID: <ZQJODdfwZLxc9o1l@linux.dev> (raw)
In-Reply-To: <20230913-kvm-arm64-fp-init-v1-1-8ce9ba1cc4c4@kernel.org>

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