Re: [PATCH 09/10] KVM: arm64: Save/restore SVE state for nVHE

From: Marc Zyngier <maz@kernel.org>
To: Will Deacon <will@kernel.org>
Cc: kvmarm@lists.cs.columbia.edu,
	linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org,
	dave.martin@arm.com, daniel.kiss@arm.com,
	Catalin Marinas <catalin.marinas@arm.com>,
	James Morse <james.morse@arm.com>,
	Julien Thierry <julien.thierry.kdev@gmail.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	broonie@kernel.org, kernel-team@android.com
Subject: Re: [PATCH 09/10] KVM: arm64: Save/restore SVE state for nVHE
Date: Thu, 18 Mar 2021 09:12:19 +0000	[thread overview]
Message-ID: <875z1oj05o.wl-maz@kernel.org> (raw)
In-Reply-To: <20210317175734.GA5713@willie-the-truck>

On Wed, 17 Mar 2021 17:57:35 +0000,
Will Deacon <will@kernel.org> wrote:
> 
> On Tue, Mar 16, 2021 at 10:13:11AM +0000, Marc Zyngier wrote:
> > Implement the SVE save/restore for nVHE, following a similar
> > logic to that of the VHE implementation:
> > 
> > - the SVE state is switched on trap from EL1 to EL2
> > 
> > - no further changes to ZCR_EL2 occur as long as the guest isn't
> >   preempted or exit to userspace
> > 
> > - on vcpu_put(), ZCR_EL2 is reset to its default value, and ZCR_EL1
> >   restored to the default guest value
> > 
> > Signed-off-by: Marc Zyngier <maz@kernel.org>
> > ---
> >  arch/arm64/kvm/fpsimd.c                 | 15 ++++++++--
> >  arch/arm64/kvm/hyp/include/hyp/switch.h | 37 +++++++++----------------
> >  arch/arm64/kvm/hyp/nvhe/switch.c        |  4 +--
> >  3 files changed, 28 insertions(+), 28 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
> > index b5f95abd23f5..cc6cdea69596 100644
> > --- a/arch/arm64/kvm/fpsimd.c
> > +++ b/arch/arm64/kvm/fpsimd.c
> > @@ -121,11 +121,22 @@ void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu)
> >  	local_irq_save(flags);
> >  
> >  	if (vcpu->arch.flags & KVM_ARM64_FP_ENABLED) {
> > -		if (guest_has_sve)
> > +		if (guest_has_sve) {
> >  			__vcpu_sys_reg(vcpu, ZCR_EL1) = read_sysreg_el1(SYS_ZCR);
> >  
> > +			/* Restore the VL that was saved when bound to the CPU */
> > +			if (!has_vhe()) {
> > +				u64 zcr;
> > +
> > +				kvm_call_hyp(__kvm_reset_sve_vq);
> 
> What would go wrong if we did this unconditionally on return to the host, or
> is it just a performance thing to move work off the fast path where we
> return back to the same vCPU?

Nothing would go wrong, but we'd also need to re-adjust it on entry if
the FPSIMD file is dirty, making it doubly painful on the fast path.

Doing the reset on vcpu_put() ensures this is only done once, either
on preemption or on return to userspace, at the expense of an EL2
round trip. That's consistent with what we do in general for CPU state
that doesn't directly affect the execution of the kernel.

	M.

-- 
Without deviation from the norm, progress is not possible.