| Message ID | 20171012104141.26902-27-christoffer.dall@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Oct 12, 2017 at 12:41:30PM +0200, Christoffer Dall wrote: > 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 <christoffer.dall@linaro.org> > --- > 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. > + } > + > 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? > } > } > > -- > 2.9.0 > Thanks, drew
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: > > 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 <christoffer.dall@linaro.org> > > --- > > 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. > > + } > > + > > 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
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; + } + 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); } }
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 <christoffer.dall@linaro.org> --- arch/arm64/kvm/inject_fault.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)