KVM: arm64: Treat FPEXC32_EL2 as a regular guest system register

PATCH In review Dave Martin 1 participant 2 messages Dec 11, 2018 – Dec 14, 2018

Subsystems (2)

arm64 KVM arm64

lore

Share & tools

lore

Dave Martin

author

2018-12-11 18:48 UTC #0 lore raw

PATCH 1/1 KVM: arm64: Treat FPEXC32_EL2 as a regular guest system register

Currently FPEXC32_EL2 is handled specially when context-switching.

However, FPEXC has no architectural effect when running in AArch64. The only case where an arm64 host may execute in AArch32 is when running compat user code at EL0: the architecture explicitly documents FPEXC as having no effect in this case either when the kernel (EL2 in the VHE case; otherwise EL1) is AArch64.

So, we can just handle FPEXC32_EL2 (which is the AArch64 alias for this register) as a regular AArch32-only system register and switch it without any special handling or synchronisation.

This allows some gnarly special-case code to be eliminated from hyp. The extra isb() in __activate_traps_fpsimd32() is dropped
too: for the reasons explained above arm64 never required this anyway.

Note that the comment in the deleted __activate_traps_fpsimd32() function about the need to inhibit traps to EL1 is only meaningful if the guest's value for FPEXC is not restored (thus potentially causing an unexpected trap to EL1 in the guest unless we're careful to set the EN bit). This arises from the treatment of FPEXC32_EL2 as if it were part of the the FPSIMD state to be switched lazily, whereas it is really more natural to consider it as part of the EL1 system register state to be switched eagerly at the appropriate times.

In fact, we explicitly don't care about seeing any trap until the guest accesses the FPSIMD registers for real, provided that the guest sees the behaviour it expects. So ensuring that the guest's view of FPEXC is alawys up to date (as implemented by this patch) should be sufficient. Since we always had to write FPEXC32_EL2 anyway (in order to work around the possibility of not restoring it properly for the guest) this should not add significant cost. In any case, this patch moves any cost off the hyp switch critical path, to vcpu_load/put().

Cc Marc Zyngier <[email protected]>

Cc Christoffer Dall <[email protected]>

Cc Alexander Graf <[email protected]>

Signed-off-by Dave Martin <[email protected]>

---

**Build-tested only**

This is a repost of an otherwise unmodified patch discussed previously [1]. It turned out that I had confused myself: I planned to move the FPEXC32_EL2 save/restore to the vcpu_load/put() path, but this was precisely what the patch already implemented.

Changes since RFC:

* Removed inaccurate comment concerning the historical reason for the current FPEXC32_EL2 handling behaviour from the commit message.

* Added the paragraphs about the implications of making the FPEXC restore non-lazy, to the commit message.

[1] [RFC PATCH] KVM: arm64: Treat FPEXC32_EL2 as a regular guest system register
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-September/599985.html

---
 arch/arm64/kvm/hyp/switch.c    | 45 ++----------------------------------------
 arch/arm64/kvm/hyp/sysreg-sr.c |  2 ++
 2 files changed, 4 insertions(+), 43 deletions(-)

arch/arm64/kvm/hyp/switch.c +2 ~~-43~~

index 7cc175c..583fcd7 100644
@@ -43,32 +43,6 @@ static bool __hyp_text update_fp_enabled(struct kvm_vcpu *vcpu)
 	return !!(vcpu->arch.flags & KVM_ARM64_FP_ENABLED);
 }
 
-/* Save the 32-bit only FPSIMD system register state */
-static 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);
-}
-
-static void __hyp_text __activate_traps_fpsimd32(struct kvm_vcpu *vcpu)
-{
-	/*
-	 * We are about to set CPTR_EL2.TFP to trap all floating point
-	 * register accesses to EL2, however, the ARM ARM clearly states that
-	 * traps are only taken to EL2 if the operation would not otherwise
-	 * trap to EL1.  Therefore, always make sure that for 32-bit guests,
-	 * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit.
-	 * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to
-	 * it will cause an exception.
-	 */
-	if (vcpu_el1_is_32bit(vcpu) && system_supports_fpsimd()) {
-		write_sysreg(1 << 30, fpexc32_el2);
-		isb();
-	}
-}
-
 static void __hyp_text __activate_traps_common(struct kvm_vcpu *vcpu)
 {
 	/* Trap on AArch32 cp15 c15 (impdef sysregs) accesses (EL1 or EL0) */

@@ -98,10 +72,8 @@ static void activate_traps_vhe(struct kvm_vcpu *vcpu)
 	val = read_sysreg(cpacr_el1);
 	val |= CPACR_EL1_TTA;
 	val &= ~CPACR_EL1_ZEN;
-	if (!update_fp_enabled(vcpu)) {
+	if (!update_fp_enabled(vcpu))
 		val &= ~CPACR_EL1_FPEN;
-		__activate_traps_fpsimd32(vcpu);
-	}
 
 	write_sysreg(val, cpacr_el1);

@@ -116,10 +88,8 @@ static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu)
 
 	val = CPTR_EL2_DEFAULT;
 	val |= CPTR_EL2_TTA | CPTR_EL2_TZ;
-	if (!update_fp_enabled(vcpu)) {
+	if (!update_fp_enabled(vcpu))
 		val |= CPTR_EL2_TFP;
-		__activate_traps_fpsimd32(vcpu);
-	}
 
 	write_sysreg(val, cptr_el2);
 }

@@ -366,11 +336,6 @@ static bool __hyp_text __hyp_switch_fpsimd(struct kvm_vcpu *vcpu)
 
 	__fpsimd_restore_state(&vcpu->arch.ctxt.gp_regs.fp_regs);
 
-	/* Skip restoring fpexc32 for AArch64 guests */
-	if (!(read_sysreg(hcr_el2) & HCR_RW))
-		write_sysreg(vcpu->arch.ctxt.sys_regs[FPEXC32_EL2],
-			     fpexc32_el2);
-
 	vcpu->arch.flags |= KVM_ARM64_FP_ENABLED;
 
 	return true;

@@ -522,9 +487,6 @@ int kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
 
 	sysreg_restore_host_state_vhe(host_ctxt);
 
-	if (vcpu->arch.flags & KVM_ARM64_FP_ENABLED)
-		__fpsimd_save_fpexc32(vcpu);
-
 	__debug_switch_to_host(vcpu);
 
 	return exit_code;

@@ -580,9 +542,6 @@ int __hyp_text __kvm_vcpu_run_nvhe(struct kvm_vcpu *vcpu)
 
 	__sysreg_restore_state_nvhe(host_ctxt);
 
-	if (vcpu->arch.flags & KVM_ARM64_FP_ENABLED)
-		__fpsimd_save_fpexc32(vcpu);
-
 	/*
 	 * This must come after restoring the host sysregs, since a non-VHE
 	 * system may enable SPE here and make use of the TTBRs.

arch/arm64/kvm/hyp/sysreg-sr.c +2 -0

index 68d6f7c..9eaac8c 100644
@@ -212,6 +212,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);
+	sysreg[FPEXC32_EL2] = read_sysreg(fpexc32_el2);
 
 	if (has_vhe() || vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY)
 		sysreg[DBGVCR32_EL2] = read_sysreg(dbgvcr32_el2);

@@ -234,6 +235,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);
+	write_sysreg(sysreg[FPEXC32_EL2], fpexc32_el2);
 
 	if (has_vhe() || vcpu->arch.flags & KVM_ARM64_DEBUG_DIRTY)
 		write_sysreg(sysreg[DBGVCR32_EL2], dbgvcr32_el2);

Signature · 1 line

2.1.4

Dave Martin

author

2018-12-14 19:54 UTC #1 lore raw

Quoted 37 lines · Dave Martin

Currently FPEXC32_EL2 is handled specially when context-switching.
However, FPEXC has no architectural effect when running in AArch64. The only case where an arm64 host may execute in AArch32 is when running compat user code at EL0: the architecture explicitly documents FPEXC as having no effect in this case either when the kernel (EL2 in the VHE case; otherwise EL1) is AArch64.
So, we can just handle FPEXC32_EL2 (which is the AArch64 alias for this register) as a regular AArch32-only system register and switch it without any special handling or synchronisation.
This allows some gnarly special-case code to be eliminated from hyp. The extra isb() in __activate_traps_fpsimd32() is dropped
too: for the reasons explained above arm64 never required this anyway.
Note that the comment in the deleted __activate_traps_fpsimd32() function about the need to inhibit traps to EL1 is only meaningful if the guest's value for FPEXC is not restored (thus potentially causing an unexpected trap to EL1 in the guest unless we're careful to set the EN bit). This arises from the treatment of FPEXC32_EL2 as if it were part of the the FPSIMD state to be switched lazily, whereas it is really more natural to consider it as part of the EL1 system register state to be switched eagerly at the appropriate times.
In fact, we explicitly don't care about seeing any trap until the guest accesses the FPSIMD registers for real, provided that the guest sees the behaviour it expects. So ensuring that the guest's view of FPEXC is alawys up to date (as implemented by this patch) should be sufficient. Since we always had to write FPEXC32_EL2 anyway (in order to work around the possibility of not restoring it properly for the guest) this should not add significant cost. In any case, this patch moves any cost off the hyp switch critical path, to vcpu_load/put().

Quoted 8 lines

Cc Marc Zyngier <[email protected]>
Cc Christoffer Dall <[email protected]>
Cc Alexander Graf <[email protected]>
Signed-off-by Dave Martin <[email protected]>
---
**Build-tested only**

Dang, it turns out that writing FPEXC32_EL2 from EL2 triggers a trap to EL2 if FPSIMD traps to EL2 are enabled via CPTR_EL2.

This seems illogical, but I've seen it trigger a Hyp panic on Juno (r0) (non-VHE, but looking at the Arm ARM it appears that something equivalent happens for VHE too).

So this needs a rethink :|

I think we can still defer the save until vcpu_put(), and save a sysreg write by not restoring it lazily (since we have to write it once anyway to set EN bit if we want to trap FPSIMD to EL2).

Anyway, definitely not a priority for v4.21.

[...]

Cheers
---Dave