Re: [PATCH v8 26/38] KVM: arm64: Handle SME host state when running guests

[Marc Zyngier <maz@kernel.org>]

On Tue, 25 Jan 2022 00:11:02 +0000,
Mark Brown <broonie@kernel.org> wrote:
> 
> While we don't currently support SME in guests we do currently support it
> for the host system so we need to take care of SME's impact, including
> the floating point register state, when running guests. Simiarly to SVE
> we need to manage the traps in CPACR_RL1, what is new is the handling of
> streaming mode and ZA.
> 
> Normally we defer any handling of the floating point register state until
> the guest first uses it however if the system is in streaming mode FPSIMD
> and SVE operations may generate SME traps which we would need to distinguish
> from actual attempts by the guest to use SME. Rather than do this for the
> time being if we are in streaming mode when entering the guest we force
> the floating point state to be saved immediately and exit streaming mode,
> meaning that the guest won't generate SME traps for supported operations.
> 
> We could handle ZA in the access trap similarly to the FPSIMD/SVE state
> without the disruption caused by streaming mode but for simplicity
> handle it the same way as streaming mode for now.
> 
> This will be revisited when we support SME for guests (hopefully before SME
> hardware becomes available), for now it will only incur additional cost on
> systems with SME and even there only if streaming mode or ZA are enabled.
> 
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
>  arch/arm64/include/asm/kvm_host.h |  1 +
>  arch/arm64/kvm/fpsimd.c           | 38 +++++++++++++++++++++++++++++++
>  2 files changed, 39 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index 7dc85d5a6552..404b7358ba96 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -438,6 +438,7 @@ struct kvm_vcpu_arch {
>  #define KVM_ARM64_DEBUG_STATE_SAVE_SPE	(1 << 12) /* Save SPE context if active  */
>  #define KVM_ARM64_DEBUG_STATE_SAVE_TRBE	(1 << 13) /* Save TRBE context if active  */
>  #define KVM_ARM64_FP_FOREIGN_FPSTATE	(1 << 14)
> +#define KVM_ARM64_HOST_SME_ENABLED	(1 << 15) /* SME enabled for EL0 */
>  
>  #define KVM_GUESTDBG_VALID_MASK (KVM_GUESTDBG_ENABLE | \
>  				 KVM_GUESTDBG_USE_SW_BP | \
> diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> index 338733ac63f8..cecaddb644ce 100644
> --- a/arch/arm64/kvm/fpsimd.c
> +++ b/arch/arm64/kvm/fpsimd.c
> @@ -82,6 +82,26 @@ void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu)
>  
>  	if (read_sysreg(cpacr_el1) & CPACR_EL1_ZEN_EL0EN)
>  		vcpu->arch.flags |= KVM_ARM64_HOST_SVE_ENABLED;
> +
> +	/*
> +	 * We don't currently support SME guests but if we leave
> +	 * things in streaming mode then when the guest starts running
> +	 * FPSIMD or SVE code it may generate SME traps so as a
> +	 * special case if we are in streaming mode we force the host
> +	 * state to be saved now and exit streaming mode so that we
> +	 * don't have to handle any SME traps for valid guest
> +	 * operations. Do this for ZA as well for now for simplicity.
> +	 */
> +	if (system_supports_sme()) {
> +		if (read_sysreg(cpacr_el1) & CPACR_EL1_SMEN_EL0EN)
> +			vcpu->arch.flags |= KVM_ARM64_HOST_SME_ENABLED;
> +
> +		if (read_sysreg_s(SYS_SVCR_EL0) &
> +		    (SYS_SVCR_EL0_SM_MASK | SYS_SVCR_EL0_ZA_MASK)) {
> +			vcpu->arch.flags &= ~KVM_ARM64_FP_HOST;
> +			fpsimd_save_and_flush_cpu_state();
> +		}
> +	}
>  }
>  
>  void kvm_arch_vcpu_ctxflush_fp(struct kvm_vcpu *vcpu)
> @@ -129,6 +149,24 @@ void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
>  
>  	local_irq_save(flags);
>  
> +	/*
> +	 * If we have VHE then the Hyp code will reset CPACR_EL1 to
> +	 * CPACR_EL1_DEFAULT and we need to reenable SME.
> +	 */
> +	if (has_vhe()) {
> +		if (system_supports_sme()) {

nit:	if (has_vhe() && system_supports_sme()) {

saves you one level of indentation.

> +			/* Also restore EL0 state seen on entry */
> +			if (vcpu->arch.flags & KVM_ARM64_HOST_SME_ENABLED)
> +				sysreg_clear_set(CPACR_EL1, 0,
> +						 CPACR_EL1_SMEN_EL0EN |
> +						 CPACR_EL1_SMEN_EL1EN);
> +			else
> +				sysreg_clear_set(CPACR_EL1,
> +						 CPACR_EL1_SMEN_EL0EN,
> +						 CPACR_EL1_SMEN_EL1EN);

I find the use of CPACR_EL1_SMEN in some cases and its individual bits
in some others pretty confusing. I understand that you have modelled
it after the SVE code, but maybe this is a mistake we don't need to
repeat. I'd be in favour of directly exposing the individual bits in
all cases.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm