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