[PATCH v4 30/40] KVM: arm64: Defer saving/restoring 32-bit sysregs to vcpu load/put

[christoffer.dall@linaro.org (Christoffer Dall)]

On Thu, Feb 22, 2018 at 03:35:06PM +0100, Andrew Jones wrote:
> On Thu, Feb 15, 2018 at 10:03:22PM +0100, Christoffer Dall wrote:
> > When running a 32-bit VM (EL1 in AArch32), the AArch32 system registers
> > can be deferred to vcpu load/put on VHE systems because neither
> > the host kernel nor host userspace uses these registers.
> > 
> > Note that we can not defer saving DBGVCR32_EL2 conditionally based
> > on the state of the debug dirty flag on VHE, but since we do the
> > load/put pretty rarely, this comes out as a win anyway.
> > 
> > We can also not defer saving FPEXC32_32 because this register only holds
> > a guest-valid value for 32-bit guests during the exit path when the
> > guest has used FPSIMD registers and restored the register in the early
> > assembly handler from taking the EL2 fault, and therefore we have to
> > check if fpsimd is enabled for the guest in the exit path and save the
> > register then, for both VHE and non-VHE guests.
> > 
> > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
> > ---
> > 
> > Notes:
> >     Changes since v3:
> >      - Rework the FPEXC32 save/restore logic to no longer attempt to
> >        save/restore this register lazily.
> >     
> >     Changes since v2:
> >      - New patch (deferred register handling has been reworked)
> > 
> >  arch/arm64/kvm/hyp/switch.c    | 17 +++++++++++------
> >  arch/arm64/kvm/hyp/sysreg-sr.c | 15 ++++++++++-----
> >  2 files changed, 21 insertions(+), 11 deletions(-)
> > 
> > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c
> > index 22e77deb8e2e..909aa3fe9196 100644
> > --- a/arch/arm64/kvm/hyp/switch.c
> > +++ b/arch/arm64/kvm/hyp/switch.c
> > @@ -47,6 +47,15 @@ bool __hyp_text __fpsimd_enabled(void)
> >  	return __fpsimd_is_enabled()();
> >  }
> >  
> > +/* Save the 32-bit only FPSIMD system register state */
> > +static inline void __hyp_text __fpsimd_save_fpexc32(struct kvm_vcpu *vcpu)
> > +{
> > +	if (!vcpu_el1_is_32bit(vcpu))
> > +		return;
> > +
> > +	vcpu->arch.ctxt.sys_regs[FPEXC32_EL2] = read_sysreg(fpexc32_el2);
> > +}
> > +
> 
> I realize it's much more convenient to have this function here, but it
> feels a bit out of place, being a _save_ function. Its logical place is
> an -sr file.
> 

Yes, maybe, but that would make it a function call to perform a mrs and
a store, which is a bit unfortunate.

> >  static void __hyp_text __activate_traps_vhe(void)
> >  {
> >  	u64 val;
> > @@ -380,11 +389,6 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
> >  
> >  	__vgic_restore_state(vcpu);
> >  
> > -	/*
> > -	 * We must restore the 32-bit state before the sysregs, thanks
> > -	 * to erratum #852523 (Cortex-A57) or #853709 (Cortex-A72).
> > -	 */
> > -	__sysreg32_restore_state(vcpu);
> >  	sysreg_restore_guest_state_vhe(guest_ctxt);
> >  	__debug_switch_to_guest(vcpu);
> >  
> > @@ -398,7 +402,6 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
> >  	fp_enabled = __fpsimd_enabled();
> >  
> >  	sysreg_save_guest_state_vhe(guest_ctxt);
> > -	__sysreg32_save_state(vcpu);
> >  	__vgic_save_state(vcpu);
> >  
> >  	__deactivate_traps(vcpu);
> > @@ -408,6 +411,7 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
> >  	if (fp_enabled) {
> >  		__fpsimd_save_state(&guest_ctxt->gp_regs.fp_regs);
> >  		__fpsimd_restore_state(&host_ctxt->gp_regs.fp_regs);
> > +		__fpsimd_save_fpexc32(vcpu);
> >  	}
> >  
> >  	__debug_switch_to_host(vcpu);
> > @@ -475,6 +479,7 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
> >  	if (fp_enabled) {
> >  		__fpsimd_save_state(&guest_ctxt->gp_regs.fp_regs);
> >  		__fpsimd_restore_state(&host_ctxt->gp_regs.fp_regs);
> > +		__fpsimd_save_fpexc32(vcpu);
> >  	}
> >  
> >  	/*
> > diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c
> > index 9c60b8062724..aacba4636871 100644
> > --- a/arch/arm64/kvm/hyp/sysreg-sr.c
> > +++ b/arch/arm64/kvm/hyp/sysreg-sr.c
> > @@ -196,10 +196,7 @@ void __hyp_text __sysreg32_save_state(struct kvm_vcpu *vcpu)
> >  	sysreg[DACR32_EL2] = read_sysreg(dacr32_el2);
> >  	sysreg[IFSR32_EL2] = read_sysreg(ifsr32_el2);
> >  
> > -	if (__fpsimd_enabled())
> > -		sysreg[FPEXC32_EL2] = read_sysreg(fpexc32_el2);
> > -
> > -	if (vcpu->arch.debug_flags & KVM_ARM64_DEBUG_DIRTY)
> > +	if (has_vhe() || vcpu->arch.debug_flags & KVM_ARM64_DEBUG_DIRTY)
> >  		sysreg[DBGVCR32_EL2] = read_sysreg(dbgvcr32_el2);
> >  }
> >  
> > @@ -221,7 +218,7 @@ void __hyp_text __sysreg32_restore_state(struct kvm_vcpu *vcpu)
> >  	write_sysreg(sysreg[DACR32_EL2], dacr32_el2);
> >  	write_sysreg(sysreg[IFSR32_EL2], ifsr32_el2);
> >  
> > -	if (vcpu->arch.debug_flags & KVM_ARM64_DEBUG_DIRTY)
> > +	if (has_vhe() || vcpu->arch.debug_flags & KVM_ARM64_DEBUG_DIRTY)
> >  		write_sysreg(sysreg[DBGVCR32_EL2], dbgvcr32_el2);
> >  }
> >  
> > @@ -246,6 +243,13 @@ void kvm_vcpu_load_sysregs(struct kvm_vcpu *vcpu)
> >  
> >  	__sysreg_save_user_state(host_ctxt);
> >  
> > +	/*
> > +	 * Load guest EL1 and user state
> > +	 *
> > +	 * We must restore the 32-bit state before the sysregs, thanks
> > +	 * to erratum #852523 (Cortex-A57) or #853709 (Cortex-A72).
> > +	 */
> > +	__sysreg32_restore_state(vcpu);
> >  	__sysreg_restore_user_state(guest_ctxt);
> >  	__sysreg_restore_el1_state(guest_ctxt);
> >  
> > @@ -273,6 +277,7 @@ void kvm_vcpu_put_sysregs(struct kvm_vcpu *vcpu)
> >  
> >  	__sysreg_save_el1_state(guest_ctxt);
> >  	__sysreg_save_user_state(guest_ctxt);
> > +	__sysreg32_save_state(vcpu);
> >  
> >  	/* Restore host user state */
> >  	__sysreg_restore_user_state(host_ctxt);
> > -- 
> > 2.14.2
> >
> 
> Besides the function location being a bit debatable, it looks good to me
> 
> Reviewed-by: Andrew Jones <drjones@redhat.com>
> 
Thanks,
-Christoffer