Re: [PATCH] kvm: arm64: Add SVE support for nVHE.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thanks for the comments, I’m going to send v2 soon.

Thanks
Daniel

> On 26 Jan 2021, at 16:49, Dave Martin <Dave.Martin@xxxxxxx> wrote:
> 
> On Fri, Jan 22, 2021 at 06:21:21PM +0000, Marc Zyngier wrote:
>> Daniel,
>> 
>> Please consider cc'ing the maintainer (me) as well as the KVM/arm64
>> reviewers (Julien, James, Suzuki) and the kvmarm list (all now Cc'd).
>> 
>> On 2021-01-22 01:07, Daniel Kiss wrote:
>>> CPUs that support SVE are architecturally required to support the
>>> Virtualization Host Extensions (VHE), so far the kernel supported
>>> SVE alongside KVM with VHE enabled. In same cases it is desired to
>>> run nVHE config even when VHE is available.
>>> This patch add support for SVE for nVHE configuration too.
>>> 
>>> In case of nVHE the system registers behave a bit differently.
>>> ZCR_EL2 defines the maximum vector length that could be set in ZCR_EL1
>>> effectively. To limit the vector length for the guest the ZCR_EL2 need
>>> to be set accordingly therefore it become part of the context.
>> 
>> Not really. It's just part of the *hypervisor* state for this guest,
>> and not part of the guest state. Not different from HCR_EL2, for example.
> 
> Also, ZCR_EL2 doesn't affect what can be written in ZCR_EL1, so this
> might be reworded to say that it just limits the effective vector length
> available to the guest.
Okay, found a way to switch the ZCR_EL2 without adding to the context.


>>> The sve_state will be loaded in EL2 so it need to be mapped and during
>>> the load ZCR_EL2 will control the vector length.
>>> Trap control is similar to the VHE case except the bit values are the
>>> opposite. ZCR_EL1 access trapping with VHE is ZEN value 0 but in case of
>>> nVHE the TZ need to be set 1 to trigger the exception. Trap control need
>>> to be respected during the context switch even in EL2.
>> 
>> Isn't that exactly the same as FPSIMD accesses?
> 
> (Yes, this isn't really new.  It might be best to let the code speak for
> itself on that point, rather than trying to explain it in the commit
> message.)
> 
>> 
>>> 
>>> Tested on FVP with a Linux guest VM that run with a different VL than
>>> the host system.
>>> 
>>> This patch requires sve_set_vq from
>>> - arm64/sve: Rework SVE trap access to minimise memory access
>> 
>> Care to add a pointer to this patch? This also shouldn't be part
>> of the commit message. When you repost it, please include the
>> other patch as a series unless it has already been merged by then.
Thanks, I will do like this next time.
By the new changes seems it is not even needed.

>>> 
>>> Signed-off-by: Daniel Kiss <daniel.kiss@xxxxxxx>
>>> ---
>>> arch/arm64/Kconfig                         |  7 ----
>>> arch/arm64/include/asm/fpsimd.h            |  4 ++
>>> arch/arm64/include/asm/fpsimdmacros.h      | 38 +++++++++++++++++
>>> arch/arm64/include/asm/kvm_host.h          | 19 +++------
>>> arch/arm64/kernel/fpsimd.c                 | 11 +++++
>>> arch/arm64/kvm/arm.c                       |  5 ---
>>> arch/arm64/kvm/fpsimd.c                    | 22 +++++++---
>>> arch/arm64/kvm/hyp/fpsimd.S                | 15 +++++++
>>> arch/arm64/kvm/hyp/include/hyp/switch.h    | 48 ++++++++++++----------
>>> arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h | 26 ++++++++++++
>>> arch/arm64/kvm/hyp/nvhe/switch.c           |  6 ++-
>>> arch/arm64/kvm/reset.c                     |  8 ++--
>>> 12 files changed, 153 insertions(+), 56 deletions(-)
>>> 
>>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>>> index a6b5b7ef40ae..f17ab095e99f 100644
>>> --- a/arch/arm64/Kconfig
>>> +++ b/arch/arm64/Kconfig
>>> @@ -1692,7 +1692,6 @@ endmenu
>>> config ARM64_SVE
>>> 	bool "ARM Scalable Vector Extension support"
>>> 	default y
>>> -	depends on !KVM || ARM64_VHE
>>> 	help
>>> 	  The Scalable Vector Extension (SVE) is an extension to the AArch64
>>> 	  execution state which complements and extends the SIMD functionality
>>> @@ -1721,12 +1720,6 @@ config ARM64_SVE
>>> 	  booting the kernel.  If unsure and you are not observing these
>>> 	  symptoms, you should assume that it is safe to say Y.
>>> 
>>> -	  CPUs that support SVE are architecturally required to support the
>>> -	  Virtualization Host Extensions (VHE), so the kernel makes no
>>> -	  provision for supporting SVE alongside KVM without VHE enabled.
>>> -	  Thus, you will need to enable CONFIG_ARM64_VHE if you want to support
>>> -	  KVM in the same kernel image.
>>> -
>>> config ARM64_MODULE_PLTS
>>> 	bool "Use PLTs to allow module memory to spill over into vmalloc area"
>>> 	depends on MODULES
>>> diff --git a/arch/arm64/include/asm/fpsimd.h
>>> b/arch/arm64/include/asm/fpsimd.h
>>> index e60aa4ebb351..e7889f4c5cef 100644
>>> --- a/arch/arm64/include/asm/fpsimd.h
>>> +++ b/arch/arm64/include/asm/fpsimd.h
>>> @@ -69,6 +69,10 @@ static inline void *sve_pffr(struct thread_struct
>>> *thread)
>>> extern void sve_save_state(void *state, u32 *pfpsr);
>>> extern void sve_load_state(void const *state, u32 const *pfpsr,
>>> 			   unsigned long vq_minus_1);
>>> +extern void sve_save_state_nvhe(void *state, u32 *pfpsr,
>>> +			   unsigned long vq_minus_1);
>>> +extern void sve_load_state_nvhe(void const *state, u32 const *pfpsr,
>>> +			   unsigned long vq_minus_1);
>> 
>> Why do we need two different entry points?
Those will use ZCR_EL2 to control the vector length instead of ZCR_EL1.
I’ll add some comments. 

>>> extern void sve_flush_live(void);
>>> extern void sve_load_from_fpsimd_state(struct user_fpsimd_state const
>>> *state,
>>> 				       unsigned long vq_minus_1);
>>> diff --git a/arch/arm64/include/asm/fpsimdmacros.h
>>> b/arch/arm64/include/asm/fpsimdmacros.h
>>> index af43367534c7..3a18172ee4f6 100644
>>> --- a/arch/arm64/include/asm/fpsimdmacros.h
>>> +++ b/arch/arm64/include/asm/fpsimdmacros.h
>>> @@ -205,6 +205,17 @@
>>> 921:
>>> .endm
>>> 
>>> +/* Update ZCR_EL2.LEN with the new VQ */
>>> +.macro sve_load_vq_nvhe xvqminus1, xtmp, xtmp2
>>> +		mrs_s		\xtmp, SYS_ZCR_EL2
>>> +		bic		\xtmp2, \xtmp, ZCR_ELx_LEN_MASK
>>> +		orr		\xtmp2, \xtmp2, \xvqminus1
>>> +		cmp		\xtmp2, \xtmp
>>> +		b.eq		922f
>>> +		msr_s		SYS_ZCR_EL2, \xtmp2	//self-synchronising
>>> +922:
>>> +.endm
>>> +
>>> /* Preserve the first 128-bits of Znz and zero the rest. */
>>> .macro _sve_flush_z nz
>>> 	_sve_check_zreg \nz
>>> @@ -230,6 +241,20 @@
>>> 		str		w\nxtmp, [\xpfpsr, #4]
>>> .endm
>>> 
>>> +.macro sve_save_nvhe nxbase, xpfpsr, xvqminus1, nxtmp, xtmp2
>>> +		sve_load_vq_nvhe	\xvqminus1, x\nxtmp, \xtmp2
>>> + _for n, 0, 31,	_sve_str_v	\n, \nxbase, \n - 34
>>> + _for n, 0, 15,	_sve_str_p	\n, \nxbase, \n - 16
>>> +		_sve_rdffr	0
>>> +		_sve_str_p	0, \nxbase
>>> +		_sve_ldr_p	0, \nxbase, -16
>>> +
>>> +		mrs		x\nxtmp, fpsr
>>> +		str		w\nxtmp, [\xpfpsr]
>>> +		mrs		x\nxtmp, fpcr
>>> +		str		w\nxtmp, [\xpfpsr, #4]
>>> +.endm
>> 
>> Please document what this macro does.
ACK.
>> 
>>> +
>>> .macro sve_load nxbase, xpfpsr, xvqminus1, nxtmp, xtmp2
>>> 		sve_load_vq	\xvqminus1, x\nxtmp, \xtmp2
>>> _for n, 0, 31,	_sve_ldr_v	\n, \nxbase, \n - 34
>>> @@ -242,3 +267,16 @@
>>> 		ldr		w\nxtmp, [\xpfpsr, #4]
>>> 		msr		fpcr, x\nxtmp
>>> .endm
>>> +
>>> +.macro sve_load_nvhe nxbase, xpfpsr, xvqminus1, nxtmp, xtmp2
>>> +		sve_load_vq_nvhe	\xvqminus1, x\nxtmp, \xtmp2
>>> + _for n, 0, 31,	_sve_ldr_v	\n, \nxbase, \n - 34
>>> +		_sve_ldr_p	0, \nxbase
>>> +		_sve_wrffr	0
>>> + _for n, 0, 15,	_sve_ldr_p	\n, \nxbase, \n - 16
>>> +
>>> +		ldr		w\nxtmp, [\xpfpsr]
>>> +		msr		fpsr, x\nxtmp
>>> +		ldr		w\nxtmp, [\xpfpsr, #4]
>>> +		msr		fpcr, x\nxtmp
>>> +.endm
> 
> These macros are virtually the same as some existing macros.  We don't
> want to maintain this kind of thing in two places if we can avoid it --
> can you try to keep them unified please?
Done.
> 
>>> diff --git a/arch/arm64/include/asm/kvm_host.h
>>> b/arch/arm64/include/asm/kvm_host.h
>>> index 0cd9f0f75c13..8bfac6f52858 100644
>>> --- a/arch/arm64/include/asm/kvm_host.h
>>> +++ b/arch/arm64/include/asm/kvm_host.h
>>> @@ -137,6 +137,7 @@ enum vcpu_sysreg {
>>> 	ACTLR_EL1,	/* Auxiliary Control Register */
>>> 	CPACR_EL1,	/* Coprocessor Access Control */
>>> 	ZCR_EL1,	/* SVE Control */
>>> +	ZCR_EL2,	/* SVE Control */
>> 
>> No. This obviously isn't a guest register, and it has no purpose
>> in this array.
> 
> Ack, the value in ZCR_EL2 should just be set based on
> vcpu->arch.sve_max_vl, which is locked after kvm_arm_vcpu_finalize().
> The guest can't change it, so we don't need to re-save this anywhere
> when exiting back to the host: we just need to restore the host value.
> 
> We do need somewhere to save the host value for this register while the
> guest is running, though.  The value is determined by
> head.S:init_el2_nvhe() (in the __init_el2_nvhe_sve macro), and isn't
> stored anywhere other than in ZCR_EL2 itself.  I feel it's better just
> to save and restore that, rather than duplicating the logic that
> determines what it should be for the host -- though obvioulsy it would
> work either way.
> 
>> 
>>> 	TTBR0_EL1,	/* Translation Table Base Register 0 */
>>> 	TTBR1_EL1,	/* Translation Table Base Register 1 */
>>> 	TCR_EL1,	/* Translation Control Register */
>>> @@ -283,6 +284,7 @@ struct vcpu_reset_state {
>>> struct kvm_vcpu_arch {
>>> 	struct kvm_cpu_context ctxt;
>>> 	void *sve_state;
>>> +	void *sve_state_hyp;
> 
> [...]
> 
>>> 	unsigned int sve_max_vl;
>>> 
>>> 	/* Stage 2 paging state used by the hardware on next switch */
>>> @@ -386,6 +388,10 @@ struct kvm_vcpu_arch {
>>> #define vcpu_sve_pffr(vcpu) ((void *)((char *)((vcpu)->arch.sve_state) +
>>> \
>>> 				      sve_ffr_offset((vcpu)->arch.sve_max_vl)))
>>> 
>>> +#define vcpu_sve_pffr_hyp(vcpu) \
>>> +			((void *)((char *)((vcpu)->arch.sve_state_hyp) + \
>>> +			sve_ffr_offset((vcpu)->arch.sve_max_vl)))
>> 
>> Why can't you have a single macro that embeds kern_hyp_va()?
Done.

>> 
>>> +
>>> #define vcpu_sve_state_size(vcpu) ({					\
>>> 	size_t __size_ret;						\
>>> 	unsigned int __vcpu_vq;						\
>>> @@ -579,19 +585,6 @@ static inline void
>>> kvm_init_host_cpu_context(struct kvm_cpu_context *cpu_ctxt)
>>> 	ctxt_sys_reg(cpu_ctxt, MPIDR_EL1) = read_cpuid_mpidr();
>>> }
>>> 
>>> -static inline bool kvm_arch_requires_vhe(void)
>>> -{
>>> -	/*
>>> -	 * The Arm architecture specifies that implementation of SVE
>>> -	 * requires VHE also to be implemented.  The KVM code for arm64
>>> -	 * relies on this when SVE is present:
>>> -	 */
>>> -	if (system_supports_sve())
>>> -		return true;
>>> -
>>> -	return false;
>>> -}
>>> -
>>> void kvm_arm_vcpu_ptrauth_trap(struct kvm_vcpu *vcpu);
>>> 
>>> static inline void kvm_arch_hardware_unsetup(void) {}
>>> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
>>> index 062b21f30f94..1f94c5ca4943 100644
>>> --- a/arch/arm64/kernel/fpsimd.c
>>> +++ b/arch/arm64/kernel/fpsimd.c
>>> @@ -308,6 +308,17 @@ static void fpsimd_save(void)
>>> 
>>> 	if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) {
>>> 		if (system_supports_sve() && test_thread_flag(TIF_SVE)) {
>>> +
>>> +			if (!has_vhe() && (sve_get_vl() != last->sve_vl)) {
>>> +				/* last->sve_state could belong to a vcpu, nVHE
>>> +				   case the context is loaded in EL2 where ZCR_EL2
>>> +				   is used. In this context the ZCR_EL1 is used but
>>> +				   it could be different because it was not
>>> +				   set when the vcpu context is loaded. Let's set
>>> +				   it to the guest's VL.
>>> +				 */
>> 
>> Please use the kernel comment style.
>> 
>>> +				sve_set_vq(sve_vq_from_vl(last->sve_vl) - 1);
>>> +			}
> 
> On the surface of it, this looks like it's in the wrong place -- there
> shouldn't really be any need for KVM-specific hacks in the core context
> switch code.
> 
> Thinking about this, don't we always save out any guest state in
> kvm_arch_vcpu_put_fp(), no?  That code calls
> fpsimd_save_and_flush_cpu_state(), so TIF_FOREIGN_FPSTATE should always
> be set when reenabling preemption and exiting the KVM run loop.  The
> core context switch code should never see dangling guest state.
> 
> There's potential for optimisation here -- we could avoid flushing the
> guest state, but just save it.  This would allow the state to be reused
> if the next user of FPSIMD/SVE on this cpu is the KVM run loop.  I
> think we figured that the extra complexity wasn't worth it, since
> exiting the guest is already costly and KVM does its best to avoid it
> -- the additional cost of reloading the guest vector regs may therefore
> not be worth caring about.
> 
> (Disclaimer -- my understanding of the code may be a bit out of date, so
> shout if the above sounds suspicious!)
> 
> 
> So in conclusion, actually I'm wondering whether we need to do anything
> at all here.  Which is just as well, since fudging ZCR.LEN here would be
> broken anyway: if it was already too small, the extra bits of the SVE
> vector registers could have been thrown away by the hardware already.

last->sve_vl will be set to the VL for the vcpu. in case of VHE it is fine because
the save runs with ZCR_EL2 which holds the maximum VL. Value in the ZCR_EL1
does not matter.
nVHE case the save_state will use ZCR_EL1 that is managed by the guest.
The guest is allowed to set it to other values and does if /proc/sys/abi/sve_default_vector_length 
is set.
This can be managed in the kvm part, where ZCR_EL1 is saved.

> 
>>> 			if (WARN_ON(sve_get_vl() != last->sve_vl)) {
>>> 				/*
>>> 				 * Can't save the user regs, so current would
>>> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
>>> index c0ffb019ca8b..3d37f4ad2228 100644
>>> --- a/arch/arm64/kvm/arm.c
>>> +++ b/arch/arm64/kvm/arm.c
>>> @@ -1749,11 +1749,6 @@ int kvm_arch_init(void *opaque)
>>> 
>>> 	in_hyp_mode = is_kernel_in_hyp_mode();
>>> 
>>> -	if (!in_hyp_mode && kvm_arch_requires_vhe()) {
>>> -		kvm_pr_unimpl("CPU unsupported in non-VHE mode, not initializing\n");
>>> -		return -ENODEV;
>>> -	}
>>> -
>>> 	if (cpus_have_final_cap(ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE) ||
>>> 	    cpus_have_final_cap(ARM64_WORKAROUND_1508412))
>>> 		kvm_info("Guests without required CPU erratum workarounds can
>>> deadlock system!\n" \
>>> diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
>>> index 3e081d556e81..cfbc869caeeb 100644
>>> --- a/arch/arm64/kvm/fpsimd.c
>>> +++ b/arch/arm64/kvm/fpsimd.c
>>> @@ -42,6 +42,15 @@ int kvm_arch_vcpu_run_map_fp(struct kvm_vcpu *vcpu)
>>> 	if (ret)
>>> 		goto error;
>>> 
>>> +	if (!has_vhe() && vcpu->arch.sve_state) {
>>> +		ret = create_hyp_mappings(vcpu->arch.sve_state,
>>> +					vcpu->arch.sve_state +
>>> +					SVE_SIG_REGS_SIZE(sve_vq_from_vl(vcpu->arch.sve_max_vl)),
>>> +					PAGE_HYP);
>>> +		if (ret)
>>> +			goto error;
>>> +		vcpu->arch.sve_state_hyp = kern_hyp_va(vcpu->arch.sve_state);
>> 
>> How useful is it to keep that duplicated pointer around, given that
>> we can compute it on the fly at a very low cost?
>> 
>>> +	}
>>> 	vcpu->arch.host_thread_info = kern_hyp_va(ti);
>>> 	vcpu->arch.host_fpsimd_state = kern_hyp_va(fpsimd);
>>> error:
>>> @@ -111,8 +120,9 @@ void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
>>> 	if (vcpu->arch.flags & KVM_ARM64_FP_ENABLED) {
>>> 		fpsimd_save_and_flush_cpu_state();
>>> 
>>> -		if (guest_has_sve)
>>> +		if (guest_has_sve && has_vhe())
>>> 			__vcpu_sys_reg(vcpu, ZCR_EL1) = read_sysreg_s(SYS_ZCR_EL12);
>>> +
> 
> Spurious blank line?
Done
> 
>>> 	} else if (host_has_sve) {
>>> 		/*
>>> 		 * The FPSIMD/SVE state in the CPU has not been touched, and we
>>> @@ -121,10 +131,12 @@ void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
>>> 		 * for EL0.  To avoid spurious traps, restore the trap state
>>> 		 * seen by kvm_arch_vcpu_load_fp():
>>> 		 */
> 
> With nVHE, the HYP exit code already restored CPACR_EL1, right?
> 
> Might be worth adding a sentence explaining why skipping this code for
> nVHE is OK.
Done
> 
>>> -		if (vcpu->arch.flags & KVM_ARM64_HOST_SVE_ENABLED)
>>> -			sysreg_clear_set(CPACR_EL1, 0, CPACR_EL1_ZEN_EL0EN);
>>> -		else
>>> -			sysreg_clear_set(CPACR_EL1, CPACR_EL1_ZEN_EL0EN, 0);
>>> +		if (has_vhe()) {
>>> +			if (vcpu->arch.flags & KVM_ARM64_HOST_SVE_ENABLED)
>>> +				sysreg_clear_set(CPACR_EL1, 0, CPACR_EL1_ZEN_EL0EN);
>>> +			else
>>> +				sysreg_clear_set(CPACR_EL1, CPACR_EL1_ZEN_EL0EN, 0);
>>> +		}
>>> 	}
>>> 
>>> 	update_thread_flag(TIF_SVE,
>>> diff --git a/arch/arm64/kvm/hyp/fpsimd.S b/arch/arm64/kvm/hyp/fpsimd.S
>>> index 01f114aa47b0..15d47ebdd30a 100644
>>> --- a/arch/arm64/kvm/hyp/fpsimd.S
>>> +++ b/arch/arm64/kvm/hyp/fpsimd.S
>>> @@ -6,6 +6,7 @@
>>> 
>>> #include <linux/linkage.h>
>>> 
>>> +#include <asm/assembler.h>
>>> #include <asm/fpsimdmacros.h>
>>> 
>>> 	.text
>>> @@ -19,3 +20,17 @@ SYM_FUNC_START(__fpsimd_restore_state)
>>> 	fpsimd_restore	x0, 1
>>> 	ret
>>> SYM_FUNC_END(__fpsimd_restore_state)
>>> +
>>> +#ifdef CONFIG_ARM64_SVE
>>> +
>>> +SYM_FUNC_START(sve_save_state_nvhe)
>>> +	sve_save_nvhe 0, x1, x2, 3, x4
>>> +	ret
>>> +SYM_FUNC_END(sve_save_state_nvhe)
>>> +
>>> +SYM_FUNC_START(sve_load_state_nvhe)
>>> +	sve_load_nvhe 0, x1, x2, 3, x4
>>> +	ret
>>> +SYM_FUNC_END(sve_load_state_nvhe)
>>> +
>>> +#endif
>>> diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h
>>> b/arch/arm64/kvm/hyp/include/hyp/switch.h
>>> index 1f875a8f20c4..a14b0d92d9ca 100644
>>> --- a/arch/arm64/kvm/hyp/include/hyp/switch.h
>>> +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
>>> @@ -197,25 +197,18 @@ static inline bool __populate_fault_info(struct
>>> kvm_vcpu *vcpu)
>>> /* Check for an FPSIMD/SVE trap and handle as appropriate */
>>> static inline bool __hyp_handle_fpsimd(struct kvm_vcpu *vcpu)
>>> {
>>> -	bool vhe, sve_guest, sve_host;
>>> +	bool sve_guest, sve_host;
>>> 	u8 esr_ec;
>>> 
>>> 	if (!system_supports_fpsimd())
>>> 		return false;
>>> 
>>> -	/*
>>> -	 * Currently system_supports_sve() currently implies has_vhe(),
>>> -	 * so the check is redundant. However, has_vhe() can be determined
>>> -	 * statically and helps the compiler remove dead code.
>>> -	 */
>>> -	if (has_vhe() && system_supports_sve()) {
>>> +	if (system_supports_sve()) {
>>> 		sve_guest = vcpu_has_sve(vcpu);
>>> 		sve_host = vcpu->arch.flags & KVM_ARM64_HOST_SVE_IN_USE;
>>> -		vhe = true;
>>> 	} else {
>>> 		sve_guest = false;
>>> 		sve_host = false;
>>> -		vhe = has_vhe();
>>> 	}
>>> 
>>> 	esr_ec = kvm_vcpu_trap_get_class(vcpu);
>>> @@ -229,8 +222,7 @@ static inline bool __hyp_handle_fpsimd(struct
>>> kvm_vcpu *vcpu)
>>> 			return false;
>>> 
>>> 	/* Valid trap.  Switch the context: */
>>> -
>>> -	if (vhe) {
>>> +	if (has_vhe()) {
> 
> You might still want to cache this.
> 
> There's no way to tell the compiler that consecutive checks on the same
> static key yield the same result, so repeatedly calling has_vhe() may
> emit a small amount of redundant code, IIUC.
Done
> 
>>> 		u64 reg = read_sysreg(cpacr_el1) | CPACR_EL1_FPEN;
>>> 
>>> 		if (sve_guest)
>>> @@ -238,24 +230,30 @@ static inline bool __hyp_handle_fpsimd(struct
>>> kvm_vcpu *vcpu)
>>> 
>>> 		write_sysreg(reg, cpacr_el1);
>>> 	} else {
>>> -		write_sysreg(read_sysreg(cptr_el2) & ~(u64)CPTR_EL2_TFP,
>>> -			     cptr_el2);
>>> +		u64 reg = read_sysreg(cptr_el2) & ~CPTR_EL2_TFP ;
> 
> Spurious space before ;
> 
> Also, you changed ~(u64)CPTR_EL2_TFP to ~CPTR_EL2_TFP here.  Does it
> make a difference?
Done
> 
>>> +		if (sve_guest)
>>> +			reg &= ~CPTR_EL2_TZ;
> 
> Same here.
> 
>>> +		write_sysreg(reg, cptr_el2);
>>> 	}
>>> 
>>> 	isb();
>>> 
>>> 	if (vcpu->arch.flags & KVM_ARM64_FP_HOST) {
>>> -		/*
>>> -		 * In the SVE case, VHE is assumed: it is enforced by
>>> -		 * Kconfig and kvm_arch_init().
>>> -		 */
>>> 		if (sve_host) {
>>> 			struct thread_struct *thread = container_of(
>>> 				vcpu->arch.host_fpsimd_state,
>>> 				struct thread_struct, uw.fpsimd_state);
>>> +			if (has_vhe())
>>> +				sve_save_state(sve_pffr(thread),
>>> +						&vcpu->arch.host_fpsimd_state->fpsr);
>>> +			else {
>>> +				struct kvm_cpu_context *host_ctxt =
>>> +					&this_cpu_ptr(&kvm_host_data)->host_ctxt;
>> 
>> Assignments on a single line please.
>> 
>>> +				sve_save_state_nvhe(sve_pffr(thread),
>>> +						&vcpu->arch.host_fpsimd_state->fpsr,
>>> +						ctxt_sys_reg(host_ctxt, ZCR_EL2));
>>> +			}
> 
> Does this actually need to be different from the VHE case, if we just
> leave ZCR_EL2 permanently set with the maxmium VL allowed for the guest?
> 
>>> 
>>> -			sve_save_state(sve_pffr(thread),
>>> -				       &vcpu->arch.host_fpsimd_state->fpsr);
>>> 		} else {
>>> 			__fpsimd_save_state(vcpu->arch.host_fpsimd_state);
>>> 		}
>>> @@ -264,10 +262,18 @@ static inline bool __hyp_handle_fpsimd(struct
>>> kvm_vcpu *vcpu)
>>> 	}
>>> 
>>> 	if (sve_guest) {
>>> -		sve_load_state(vcpu_sve_pffr(vcpu),
>>> +		if (has_vhe()) {
>>> +			sve_load_state(vcpu_sve_pffr(vcpu),
>>> 			       &vcpu->arch.ctxt.fp_regs.fpsr,
>>> 			       sve_vq_from_vl(vcpu->arch.sve_max_vl) - 1);
>>> -		write_sysreg_s(__vcpu_sys_reg(vcpu, ZCR_EL1), SYS_ZCR_EL12);
>>> +			write_sysreg_s(__vcpu_sys_reg(vcpu, ZCR_EL1), SYS_ZCR_EL12);
>>> +		} else {
>>> +			sve_load_state_nvhe(vcpu_sve_pffr_hyp(vcpu),
>>> +			       &vcpu->arch.ctxt.fp_regs.fpsr,
>>> +			       sve_vq_from_vl(vcpu->arch.sve_max_vl) - 1);
>>> +			write_sysreg_s(__vcpu_sys_reg(vcpu, ZCR_EL2), SYS_ZCR_EL2);
>> 
>> I think you really need to explain why you restore ZCR_EL2 here, and not
>> ZCR_EL1.
> 
> Something doesn't smell quite right here.  ZCR_EL2.LEN determines how
> regs are saved/restored, so for nVHE I think it needs to change as
> follows:
> 
> * In host, after saving guest SVE regs:
> 
> 	ZCR_EL2.LEN = ZCR_ELx_LEN_MASK (from __init_el2_nvhe_sve)
> 
> * Before saving the host SVE regs here:
> 
> 	ZCR_EL2.LEN = host ZCR_EL1.LEN
> (since the size and layout of current->thread.sve_state is VL dependent)
> 
> * Before loading the guest SVE regs, and before running in the guest with
> SVE untrapped:
> 
> 	ZCR_EL2.LEN = sve_vq_from_vl(vcpu->arch.sve_max_vl) - 1
> 
> 
> I don't quite see how the code achieves this, though I might be missing
> something.
> 
> 
> The alternative would be to dump the registers in a common layout by
> setting ZCR_EL2.LEN permanently ZCR_ELx_LEN_MASK except when actually
> running the guest.  But that may tend to waste a bit of memory, and it
> would be necessary to save the host regs in a side buffer and convert
> them to host layout when exiting the guest.
> 
> This would avoid slowness arising from saving regs immediately after a
> ZCR.LEN write, but would add complexity elsewhere.
> 
>> 
>>> +
>>> +		}
>>> 	} else {
>>> 		__fpsimd_restore_state(&vcpu->arch.ctxt.fp_regs);
>>> 	}
>>> diff --git a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
>>> b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
>>> index cce43bfe158f..45c16b81b826 100644
>>> --- a/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
>>> +++ b/arch/arm64/kvm/hyp/include/hyp/sysreg-sr.h
>>> @@ -46,6 +46,19 @@ static inline void __sysreg_save_el1_state(struct
>>> kvm_cpu_context *ctxt)
>>> 	ctxt_sys_reg(ctxt, PAR_EL1)	= read_sysreg_par();
>>> 	ctxt_sys_reg(ctxt, TPIDR_EL1)	= read_sysreg(tpidr_el1);
>>> 
>>> +	if (!has_vhe()) {
>>> +		u64 reg = read_sysreg(cptr_el2);
>>> +		if (reg & CPTR_EL2_TZ) {
>>> +			write_sysreg(reg & ~CPTR_EL2_TZ, cptr_el2);
>>> +			ctxt_sys_reg(ctxt, ZCR_EL1) = read_sysreg_s(SYS_ZCR_EL1);
>>> +			ctxt_sys_reg(ctxt, ZCR_EL2) = read_sysreg_s(SYS_ZCR_EL2);
>>> +			write_sysreg(reg, cptr_el2);
>>> +		} else {
>>> +			ctxt_sys_reg(ctxt, ZCR_EL1) = read_sysreg_s(SYS_ZCR_EL1);
>>> +			ctxt_sys_reg(ctxt, ZCR_EL2) = read_sysreg_s(SYS_ZCR_EL2);
>> 
>> Why do you need to save ZCR_EL2? It cannot change behind your back, can it?
>> Also, why aren't these registers saved lazily? If TZ is set, the guest
>> cannot
>> possibly have changed ZCR_EL1.
>> 
>>> +		}
>>> +	}
>>> +
>>> 	ctxt_sys_reg(ctxt, SP_EL1)	= read_sysreg(sp_el1);
>>> 	ctxt_sys_reg(ctxt, ELR_EL1)	= read_sysreg_el1(SYS_ELR);
>>> 	ctxt_sys_reg(ctxt, SPSR_EL1)	= read_sysreg_el1(SYS_SPSR);
>>> @@ -107,6 +120,19 @@ static inline void
>>> __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt)
>>> 	write_sysreg(ctxt_sys_reg(ctxt, PAR_EL1),	par_el1);
>>> 	write_sysreg(ctxt_sys_reg(ctxt, TPIDR_EL1),	tpidr_el1);
>>> 
>>> +	if (!has_vhe()) {
>>> +		u64 reg = read_sysreg(cptr_el2);
>>> +		if (reg & CPTR_EL2_TZ) {
>>> +			write_sysreg(reg & ~CPTR_EL2_TZ, cptr_el2);
>>> +			write_sysreg_s(ctxt_sys_reg(ctxt, ZCR_EL1), SYS_ZCR_EL1);
>>> +			write_sysreg_s(ctxt_sys_reg(ctxt, ZCR_EL2), SYS_ZCR_EL2);
>>> +			write_sysreg(reg, cptr_el2);
>> 
>> Same thing here. If TZ is set, there is no point in restoring these
>> registers,
>> and it should be delegated to the point where you actually restore the bulk
>> of the SVE state.
> 
> Ack
> 
>> 
>>> +		} else {
>>> +			write_sysreg_s(ctxt_sys_reg(ctxt, ZCR_EL1), SYS_ZCR_EL1);
>>> +			write_sysreg_s(ctxt_sys_reg(ctxt, ZCR_EL2), SYS_ZCR_EL2);
>>> +		}
>>> +	}
>>> +
>>> 	if (!has_vhe() &&
>>> 	    cpus_have_final_cap(ARM64_WORKAROUND_SPECULATIVE_AT) &&
>>> 	    ctxt->__hyp_running_vcpu) {
>>> diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c
>>> b/arch/arm64/kvm/hyp/nvhe/switch.c
>>> index 8ae8160bc93a..bffd78203448 100644
>>> --- a/arch/arm64/kvm/hyp/nvhe/switch.c
>>> +++ b/arch/arm64/kvm/hyp/nvhe/switch.c
>>> @@ -41,7 +41,11 @@ static void __activate_traps(struct kvm_vcpu *vcpu)
>>> 
>>> 	val = CPTR_EL2_DEFAULT;
>>> 	val |= CPTR_EL2_TTA | CPTR_EL2_TZ | CPTR_EL2_TAM;
>>> -	if (!update_fp_enabled(vcpu)) {
>>> +
>>> +	if (update_fp_enabled(vcpu)) {
>>> +		if (vcpu_has_sve(vcpu))
>>> +			val &= ~CPTR_EL2_TZ;
>>> +	} else {
>> 
>> nit: please don't invert conditions like this.
Done, just wanted to have the same flow as in vhe code.

>> 
>>> 		val |= CPTR_EL2_TFP;
>>> 		__activate_traps_fpsimd32(vcpu);
>>> 	}
>>> diff --git a/arch/arm64/kvm/reset.c b/arch/arm64/kvm/reset.c
>>> index f32490229a4c..3c62fb10a3fe 100644
>>> --- a/arch/arm64/kvm/reset.c
>>> +++ b/arch/arm64/kvm/reset.c
>>> @@ -127,10 +127,6 @@ static int kvm_vcpu_enable_sve(struct kvm_vcpu *vcpu)
>>> 	if (!system_supports_sve())
>>> 		return -EINVAL;
>>> 
>>> -	/* Verify that KVM startup enforced this when SVE was detected: */
>>> -	if (WARN_ON(!has_vhe()))
>>> -		return -EINVAL;
>>> -
>>> 	vcpu->arch.sve_max_vl = kvm_sve_max_vl;
>>> 
>>> 	/*
>>> @@ -168,6 +164,10 @@ static int kvm_vcpu_finalize_sve(struct kvm_vcpu
>>> *vcpu)
>>> 		return -ENOMEM;
>>> 
>>> 	vcpu->arch.sve_state = buf;
>>> +
>>> +	if (!has_vhe())
>>> +		__vcpu_sys_reg(vcpu, ZCR_EL2) = sve_vq_from_vl(vl) - 1;
>> 
>> As I understand it, this is the only time where ZCR_EL2 is ever set
>> for a given guest. It only has to be restored on guest entry, never
>> saved back.
> 
> [...]
>> 
>> Thanks,
>> 
>>        M.
> 
> (Except that the hyp code may need to mess with ZCR_EL2 as outlined
> above.  But ack that the value that needs to be in ZCR_EL2 while the
> guest runs should never change from the value determined here IIUC.)
> 
> Cheers
> ---Dave

_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm




[Index of Archives]     [Linux KVM]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux