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@xxxxxxxxxx>
> ---
>
> 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.
> 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@xxxxxxxxxx>
Thanks,
drew
