On Mon, Nov 13, 2017 at 08:07:01PM +0100, Andrew Jones wrote:

On Thu, Oct 12, 2017 at 12:41:30PM +0200, Christoffer Dall wrote:

quoted

Handle accesses to any AArch32 EL1 system registers where we can defer
saving and restoring them to vcpu_load and vcpu_put, and which are
stored in special EL2 registers only used support 32-bit guests.

Signed-off-by: Christoffer Dall <redacted>
---
 arch/arm64/kvm/inject_fault.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/kvm/inject_fault.c b/arch/arm64/kvm/inject_fault.c
index f4513fc..02990f5 100644
--- a/arch/arm64/kvm/inject_fault.c
+++ b/arch/arm64/kvm/inject_fault.c

@@ -59,9 +59,18 @@ static void vcpu_set_elr_el1(struct kvm_vcpu *vcpu, u64 val)
 /* Set the SPSR for the current mode */
 static void vcpu_set_spsr(struct kvm_vcpu *vcpu, u64 val)
 {
-	if (vcpu_mode_is_32bit(vcpu))
+	if (vcpu_mode_is_32bit(vcpu)) {
+		if (vcpu->arch.sysregs_loaded_on_cpu)
+			__sysreg32_save_state(vcpu);
+
 		*vcpu_spsr32(vcpu) = val;
 
+		if (vcpu->arch.sysregs_loaded_on_cpu)
+			__sysreg32_restore_state(vcpu);
+
+		return;

Is this a fix? I don't understand why it's necessary now, but it wasn't
before.

All of these patches are not necessary before "KVM: arm64: Defer
saving/restoring system registers to vcpu load/put on VHE", but I needed
to find a way to split the patches into some smaller pieces, both for
debugging/bisecting and for easing the review.

quoted

+	}
+
 	if (vcpu->arch.sysregs_loaded_on_cpu)
 		write_sysreg_el1(val, spsr);
 	else

@@ -129,11 +138,13 @@ static void inject_abt32(struct kvm_vcpu *vcpu, bool is_pabt,
 	 *   IFAR:  mapped to FAR_EL1
 	 *   DFSR:  mapped to ESR_EL1
 	 *   TTBCR: mapped to TCR_EL1
+	 *   IFSR:  stored in IFSR32_EL2
 	 */
 	if (vcpu->arch.sysregs_loaded_on_cpu) {
 		vcpu->arch.ctxt.sys_regs[FAR_EL1] = read_sysreg_el1(far);
 		vcpu->arch.ctxt.sys_regs[ESR_EL1] = read_sysreg_el1(esr);
 		vcpu->arch.ctxt.sys_regs[TCR_EL1] = read_sysreg_el1(tcr);
+		vcpu->arch.ctxt.sys_regs[IFSR32_EL2] = read_sysreg(ifsr32_el2);
 	}
 
 	if (is_pabt) {

@@ -161,6 +172,7 @@ static void inject_abt32(struct kvm_vcpu *vcpu, bool is_pabt,
 	if (vcpu->arch.sysregs_loaded_on_cpu) {
 		write_sysreg_el1(vcpu->arch.ctxt.sys_regs[FAR_EL1], far);
 		write_sysreg_el1(vcpu->arch.ctxt.sys_regs[ESR_EL1], esr);
+		write_sysreg(vcpu->arch.ctxt.sys_regs[IFSR32_EL2], ifsr32_el2);

This appears to be a fix. Why not squash it into the patch that
save/restore the other registers?

It's just more work we can do before we start being lazy.

I thought it would be easier to review in pieces, and I thought the
sequence of patches begining with "Prepare..." was a commonly used
theme, bt apparently not.

In any cae, this hunk has now gone away due to the shared logic of
32-bit fault injection, so it should be more obvious next time around.

Thanks,
-Christoffer