| Message ID | 1483943091-1364-8-git-send-email-jintack@cs.columbia.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 09, 2017 at 01:24:03AM -0500, Jintack Lim wrote: > From: Christoffer Dall <christoffer.dall@linaro.org> > > Add a framework to set up the guest's context depending on the guest's > exception level. A chosen context is written to hardware in the lowvisor. > We don't set the virtual EL2 context yet. We need to improve this commit message. I think this commit is just trying to prepare to be able to switch between the normal EL1 sysreg state or using the shadow sysreg to emulate virtual EL2, but without any functional change so far. Is that correct? > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > Signed-off-by: Jintack Lim <jintack@cs.columbia.edu> > --- > arch/arm/include/asm/kvm_emulate.h | 4 ++ > arch/arm/kvm/arm.c | 5 ++ > arch/arm64/include/asm/kvm_emulate.h | 4 ++ > arch/arm64/kvm/Makefile | 2 +- > arch/arm64/kvm/context.c | 49 ++++++++++++++++ > arch/arm64/kvm/hyp/sysreg-sr.c | 109 +++++++++++++++++++---------------- > 6 files changed, 122 insertions(+), 51 deletions(-) > create mode 100644 arch/arm64/kvm/context.c > > diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h > index 399cd75e..0a03b7d 100644 > --- a/arch/arm/include/asm/kvm_emulate.h > +++ b/arch/arm/include/asm/kvm_emulate.h > @@ -47,6 +47,10 @@ static inline void vcpu_set_reg(struct kvm_vcpu *vcpu, u8 reg_num, > void kvm_inject_dabt(struct kvm_vcpu *vcpu, unsigned long addr); > void kvm_inject_pabt(struct kvm_vcpu *vcpu, unsigned long addr); > > +static inline void kvm_arm_setup_shadow_state(struct kvm_vcpu *vcpu) { }; > +static inline void kvm_arm_restore_shadow_state(struct kvm_vcpu *vcpu) { }; > +static inline void kvm_arm_init_cpu_context(kvm_cpu_context_t *cpu_ctxt) { }; > + > static inline bool kvm_condition_valid(const struct kvm_vcpu *vcpu) > { > return kvm_condition_valid32(vcpu); > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c > index d2dfa32..436bf5a 100644 > --- a/arch/arm/kvm/arm.c > +++ b/arch/arm/kvm/arm.c > @@ -41,6 +41,7 @@ > #include <asm/virt.h> > #include <asm/kvm_arm.h> > #include <asm/kvm_asm.h> > +#include <asm/kvm_hyp.h> > #include <asm/kvm_mmu.h> > #include <asm/kvm_emulate.h> > #include <asm/kvm_coproc.h> > @@ -646,6 +647,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) > } > > kvm_arm_setup_debug(vcpu); > + kvm_arm_setup_shadow_state(vcpu); > > /************************************************************** > * Enter the guest > @@ -662,6 +664,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) > * Back from guest > *************************************************************/ > > + kvm_arm_restore_shadow_state(vcpu); > kvm_arm_clear_debug(vcpu); > > /* > @@ -1369,6 +1372,8 @@ static int init_hyp_mode(void) > kvm_err("Cannot map host CPU state: %d\n", err); > goto out_err; > } > + > + kvm_arm_init_cpu_context(cpu_ctxt); > } > > kvm_info("Hyp mode initialized successfully\n"); > diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h > index 830be2e..8892c82 100644 > --- a/arch/arm64/include/asm/kvm_emulate.h > +++ b/arch/arm64/include/asm/kvm_emulate.h > @@ -42,6 +42,10 @@ > void kvm_inject_dabt(struct kvm_vcpu *vcpu, unsigned long addr); > void kvm_inject_pabt(struct kvm_vcpu *vcpu, unsigned long addr); > > +void kvm_arm_setup_shadow_state(struct kvm_vcpu *vcpu); > +void kvm_arm_restore_shadow_state(struct kvm_vcpu *vcpu); > +void kvm_arm_init_cpu_context(kvm_cpu_context_t *cpu_ctxt); > + > static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu) > { > vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS; > diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile > index d50a82a..7811d27 100644 > --- a/arch/arm64/kvm/Makefile > +++ b/arch/arm64/kvm/Makefile > @@ -16,7 +16,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/e > kvm-$(CONFIG_KVM_ARM_HOST) += $(ARM)/arm.o $(ARM)/mmu.o $(ARM)/mmio.o > kvm-$(CONFIG_KVM_ARM_HOST) += $(ARM)/psci.o $(ARM)/perf.o > > -kvm-$(CONFIG_KVM_ARM_HOST) += inject_fault.o regmap.o > +kvm-$(CONFIG_KVM_ARM_HOST) += inject_fault.o regmap.o context.o > kvm-$(CONFIG_KVM_ARM_HOST) += hyp.o hyp-init.o handle_exit.o > kvm-$(CONFIG_KVM_ARM_HOST) += guest.o debug.o reset.o sys_regs.o sys_regs_generic_v8.o > kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/aarch32.o > diff --git a/arch/arm64/kvm/context.c b/arch/arm64/kvm/context.c > new file mode 100644 > index 0000000..320afc6 > --- /dev/null > +++ b/arch/arm64/kvm/context.c > @@ -0,0 +1,49 @@ > +/* > + * Copyright (C) 2016 - Linaro Ltd. > + * Author: Christoffer Dall <christoffer.dall@linaro.org> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see <http://www.gnu.org/licenses/>. > + */ > + > +#include <linux/kvm_host.h> > +#include <asm/kvm_emulate.h> > + > +/** > + * kvm_arm_setup_shadow_state -- prepare shadow state based on emulated mode > + * @vcpu: The VCPU pointer > + */ > +void kvm_arm_setup_shadow_state(struct kvm_vcpu *vcpu) > +{ > + struct kvm_cpu_context *ctxt = &vcpu->arch.ctxt; > + > + ctxt->hw_pstate = *vcpu_cpsr(vcpu); > + ctxt->hw_sys_regs = ctxt->sys_regs; > + ctxt->hw_sp_el1 = ctxt->gp_regs.sp_el1; > +} > + > +/** > + * kvm_arm_restore_shadow_state -- write back shadow state from guest > + * @vcpu: The VCPU pointer > + */ > +void kvm_arm_restore_shadow_state(struct kvm_vcpu *vcpu) > +{ > + struct kvm_cpu_context *ctxt = &vcpu->arch.ctxt; > + > + *vcpu_cpsr(vcpu) = ctxt->hw_pstate; > + ctxt->gp_regs.sp_el1 = ctxt->hw_sp_el1; > +} > + > +void kvm_arm_init_cpu_context(kvm_cpu_context_t *cpu_ctxt) > +{ > + cpu_ctxt->hw_sys_regs = &cpu_ctxt->sys_regs[0]; > +} > diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c > index 9341376..f2a1b32 100644 > --- a/arch/arm64/kvm/hyp/sysreg-sr.c > +++ b/arch/arm64/kvm/hyp/sysreg-sr.c > @@ -19,6 +19,7 @@ > #include <linux/kvm_host.h> > > #include <asm/kvm_asm.h> > +#include <asm/kvm_emulate.h> > #include <asm/kvm_hyp.h> > > /* Yes, this does nothing, on purpose */ > @@ -33,37 +34,41 @@ static void __hyp_text __sysreg_do_nothing(struct kvm_cpu_context *ctxt) { } > > static void __hyp_text __sysreg_save_common_state(struct kvm_cpu_context *ctxt) > { > - ctxt->sys_regs[ACTLR_EL1] = read_sysreg(actlr_el1); > - ctxt->sys_regs[TPIDR_EL0] = read_sysreg(tpidr_el0); > - ctxt->sys_regs[TPIDRRO_EL0] = read_sysreg(tpidrro_el0); > - ctxt->sys_regs[TPIDR_EL1] = read_sysreg(tpidr_el1); > - ctxt->sys_regs[MDSCR_EL1] = read_sysreg(mdscr_el1); > + u64 *sys_regs = kern_hyp_va(ctxt->hw_sys_regs); > + > + sys_regs[ACTLR_EL1] = read_sysreg(actlr_el1); > + sys_regs[TPIDR_EL0] = read_sysreg(tpidr_el0); > + sys_regs[TPIDRRO_EL0] = read_sysreg(tpidrro_el0); > + sys_regs[TPIDR_EL1] = read_sysreg(tpidr_el1); > + sys_regs[MDSCR_EL1] = read_sysreg(mdscr_el1); > ctxt->gp_regs.regs.sp = read_sysreg(sp_el0); > ctxt->gp_regs.regs.pc = read_sysreg_el2(elr); > - ctxt->gp_regs.regs.pstate = read_sysreg_el2(spsr); > + ctxt->hw_pstate = read_sysreg_el2(spsr); > } > > static void __hyp_text __sysreg_save_state(struct kvm_cpu_context *ctxt) > { > - ctxt->sys_regs[MPIDR_EL1] = read_sysreg(vmpidr_el2); > - ctxt->sys_regs[CSSELR_EL1] = read_sysreg(csselr_el1); > - ctxt->sys_regs[SCTLR_EL1] = read_sysreg_el1(sctlr); > - ctxt->sys_regs[CPACR_EL1] = read_sysreg_el1(cpacr); > - ctxt->sys_regs[TTBR0_EL1] = read_sysreg_el1(ttbr0); > - ctxt->sys_regs[TTBR1_EL1] = read_sysreg_el1(ttbr1); > - ctxt->sys_regs[TCR_EL1] = read_sysreg_el1(tcr); > - ctxt->sys_regs[ESR_EL1] = read_sysreg_el1(esr); > - ctxt->sys_regs[AFSR0_EL1] = read_sysreg_el1(afsr0); > - ctxt->sys_regs[AFSR1_EL1] = read_sysreg_el1(afsr1); > - ctxt->sys_regs[FAR_EL1] = read_sysreg_el1(far); > - ctxt->sys_regs[MAIR_EL1] = read_sysreg_el1(mair); > - ctxt->sys_regs[VBAR_EL1] = read_sysreg_el1(vbar); > - ctxt->sys_regs[CONTEXTIDR_EL1] = read_sysreg_el1(contextidr); > - ctxt->sys_regs[AMAIR_EL1] = read_sysreg_el1(amair); > - ctxt->sys_regs[CNTKCTL_EL1] = read_sysreg_el1(cntkctl); > - ctxt->sys_regs[PAR_EL1] = read_sysreg(par_el1); > - > - ctxt->gp_regs.sp_el1 = read_sysreg(sp_el1); > + u64 *sys_regs = kern_hyp_va(ctxt->hw_sys_regs); > + > + sys_regs[MPIDR_EL1] = read_sysreg(vmpidr_el2); > + sys_regs[CSSELR_EL1] = read_sysreg(csselr_el1); > + sys_regs[SCTLR_EL1] = read_sysreg_el1(sctlr); > + sys_regs[CPACR_EL1] = read_sysreg_el1(cpacr); > + sys_regs[TTBR0_EL1] = read_sysreg_el1(ttbr0); > + sys_regs[TTBR1_EL1] = read_sysreg_el1(ttbr1); > + sys_regs[TCR_EL1] = read_sysreg_el1(tcr); > + sys_regs[ESR_EL1] = read_sysreg_el1(esr); > + sys_regs[AFSR0_EL1] = read_sysreg_el1(afsr0); > + sys_regs[AFSR1_EL1] = read_sysreg_el1(afsr1); > + sys_regs[FAR_EL1] = read_sysreg_el1(far); > + sys_regs[MAIR_EL1] = read_sysreg_el1(mair); > + sys_regs[VBAR_EL1] = read_sysreg_el1(vbar); > + sys_regs[CONTEXTIDR_EL1] = read_sysreg_el1(contextidr); > + sys_regs[AMAIR_EL1] = read_sysreg_el1(amair); > + sys_regs[CNTKCTL_EL1] = read_sysreg_el1(cntkctl); > + sys_regs[PAR_EL1] = read_sysreg(par_el1); > + > + ctxt->hw_sp_el1 = read_sysreg(sp_el1); > ctxt->gp_regs.elr_el1 = read_sysreg_el1(elr); > ctxt->gp_regs.spsr[KVM_SPSR_EL1]= read_sysreg_el1(spsr); > } > @@ -86,37 +91,41 @@ void __hyp_text __sysreg_save_guest_state(struct kvm_cpu_context *ctxt) > > static void __hyp_text __sysreg_restore_common_state(struct kvm_cpu_context *ctxt) > { > - write_sysreg(ctxt->sys_regs[ACTLR_EL1], actlr_el1); > - write_sysreg(ctxt->sys_regs[TPIDR_EL0], tpidr_el0); > - write_sysreg(ctxt->sys_regs[TPIDRRO_EL0], tpidrro_el0); > - write_sysreg(ctxt->sys_regs[TPIDR_EL1], tpidr_el1); > - write_sysreg(ctxt->sys_regs[MDSCR_EL1], mdscr_el1); > + u64 *sys_regs = kern_hyp_va(ctxt->hw_sys_regs); > + > + write_sysreg(sys_regs[ACTLR_EL1], actlr_el1); > + write_sysreg(sys_regs[TPIDR_EL0], tpidr_el0); > + write_sysreg(sys_regs[TPIDRRO_EL0], tpidrro_el0); > + write_sysreg(sys_regs[TPIDR_EL1], tpidr_el1); > + write_sysreg(sys_regs[MDSCR_EL1], mdscr_el1); > write_sysreg(ctxt->gp_regs.regs.sp, sp_el0); > write_sysreg_el2(ctxt->gp_regs.regs.pc, elr); > - write_sysreg_el2(ctxt->gp_regs.regs.pstate, spsr); > + write_sysreg_el2(ctxt->hw_pstate, spsr); > } > > static void __hyp_text __sysreg_restore_state(struct kvm_cpu_context *ctxt) > { > - write_sysreg(ctxt->sys_regs[MPIDR_EL1], vmpidr_el2); > - write_sysreg(ctxt->sys_regs[CSSELR_EL1], csselr_el1); > - write_sysreg_el1(ctxt->sys_regs[SCTLR_EL1], sctlr); > - write_sysreg_el1(ctxt->sys_regs[CPACR_EL1], cpacr); > - write_sysreg_el1(ctxt->sys_regs[TTBR0_EL1], ttbr0); > - write_sysreg_el1(ctxt->sys_regs[TTBR1_EL1], ttbr1); > - write_sysreg_el1(ctxt->sys_regs[TCR_EL1], tcr); > - write_sysreg_el1(ctxt->sys_regs[ESR_EL1], esr); > - write_sysreg_el1(ctxt->sys_regs[AFSR0_EL1], afsr0); > - write_sysreg_el1(ctxt->sys_regs[AFSR1_EL1], afsr1); > - write_sysreg_el1(ctxt->sys_regs[FAR_EL1], far); > - write_sysreg_el1(ctxt->sys_regs[MAIR_EL1], mair); > - write_sysreg_el1(ctxt->sys_regs[VBAR_EL1], vbar); > - write_sysreg_el1(ctxt->sys_regs[CONTEXTIDR_EL1],contextidr); > - write_sysreg_el1(ctxt->sys_regs[AMAIR_EL1], amair); > - write_sysreg_el1(ctxt->sys_regs[CNTKCTL_EL1], cntkctl); > - write_sysreg(ctxt->sys_regs[PAR_EL1], par_el1); > - > - write_sysreg(ctxt->gp_regs.sp_el1, sp_el1); > + u64 *sys_regs = kern_hyp_va(ctxt->hw_sys_regs); > + > + write_sysreg(sys_regs[MPIDR_EL1], vmpidr_el2); > + write_sysreg(sys_regs[CSSELR_EL1], csselr_el1); > + write_sysreg_el1(sys_regs[SCTLR_EL1], sctlr); > + write_sysreg_el1(sys_regs[CPACR_EL1], cpacr); > + write_sysreg_el1(sys_regs[TTBR0_EL1], ttbr0); > + write_sysreg_el1(sys_regs[TTBR1_EL1], ttbr1); > + write_sysreg_el1(sys_regs[TCR_EL1], tcr); > + write_sysreg_el1(sys_regs[ESR_EL1], esr); > + write_sysreg_el1(sys_regs[AFSR0_EL1], afsr0); > + write_sysreg_el1(sys_regs[AFSR1_EL1], afsr1); > + write_sysreg_el1(sys_regs[FAR_EL1], far); > + write_sysreg_el1(sys_regs[MAIR_EL1], mair); > + write_sysreg_el1(sys_regs[VBAR_EL1], vbar); > + write_sysreg_el1(sys_regs[CONTEXTIDR_EL1], contextidr); > + write_sysreg_el1(sys_regs[AMAIR_EL1], amair); > + write_sysreg_el1(sys_regs[CNTKCTL_EL1], cntkctl); > + write_sysreg(sys_regs[PAR_EL1], par_el1); > + > + write_sysreg(ctxt->hw_sp_el1, sp_el1); > write_sysreg_el1(ctxt->gp_regs.elr_el1, elr); > write_sysreg_el1(ctxt->gp_regs.spsr[KVM_SPSR_EL1],spsr); > } > -- > 1.9.1 > >
Jintack Lim <jintack@cs.columbia.edu> writes: ... > +/** > + * kvm_arm_setup_shadow_state -- prepare shadow state based on emulated mode > + * @vcpu: The VCPU pointer > + */ > +void kvm_arm_setup_shadow_state(struct kvm_vcpu *vcpu) > +{ > + struct kvm_cpu_context *ctxt = &vcpu->arch.ctxt; > + > + ctxt->hw_pstate = *vcpu_cpsr(vcpu); > + ctxt->hw_sys_regs = ctxt->sys_regs; > + ctxt->hw_sp_el1 = ctxt->gp_regs.sp_el1; > +} > + > +/** > + * kvm_arm_restore_shadow_state -- write back shadow state from guest > + * @vcpu: The VCPU pointer > + */ > +void kvm_arm_restore_shadow_state(struct kvm_vcpu *vcpu) > +{ > + struct kvm_cpu_context *ctxt = &vcpu->arch.ctxt; > + > + *vcpu_cpsr(vcpu) = ctxt->hw_pstate; > + ctxt->gp_regs.sp_el1 = ctxt->hw_sp_el1; > +} > + > +void kvm_arm_init_cpu_context(kvm_cpu_context_t *cpu_ctxt) > +{ > + cpu_ctxt->hw_sys_regs = &cpu_ctxt->sys_regs[0]; > +} IIUC, the *_shadow_state() functions will set hw_* pointers to either point to the "real" state or the shadow state to manage L2 ? Maybe, it might make sense to make these function names a little more generic since they are not dealing with setting the shadow state alone. > diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c > index 9341376..f2a1b32 100644 > --- a/arch/arm64/kvm/hyp/sysreg-sr.c > +++ b/arch/arm64/kvm/hyp/sysreg-sr.c > @@ -19,6 +19,7 @@ > #include <linux/kvm_host.h> > > #include <asm/kvm_asm.h> > +#include <asm/kvm_emulate.h> > #include <asm/kvm_hyp.h> > > /* Yes, this does nothing, on purpose */ > @@ -33,37 +34,41 @@ static void __hyp_text __sysreg_do_nothing(struct kvm_cpu_context *ctxt) { } > > static void __hyp_text __sysreg_save_common_state(struct kvm_cpu_context *ctxt) > { > - ctxt->sys_regs[ACTLR_EL1] = read_sysreg(actlr_el1); > - ctxt->sys_regs[TPIDR_EL0] = read_sysreg(tpidr_el0); > - ctxt->sys_regs[TPIDRRO_EL0] = read_sysreg(tpidrro_el0); > - ctxt->sys_regs[TPIDR_EL1] = read_sysreg(tpidr_el1); > - ctxt->sys_regs[MDSCR_EL1] = read_sysreg(mdscr_el1); > + u64 *sys_regs = kern_hyp_va(ctxt->hw_sys_regs); > + > + sys_regs[ACTLR_EL1] = read_sysreg(actlr_el1); > + sys_regs[TPIDR_EL0] = read_sysreg(tpidr_el0); > + sys_regs[TPIDRRO_EL0] = read_sysreg(tpidrro_el0); > + sys_regs[TPIDR_EL1] = read_sysreg(tpidr_el1); > + sys_regs[MDSCR_EL1] = read_sysreg(mdscr_el1); > ctxt->gp_regs.regs.sp = read_sysreg(sp_el0); > ctxt->gp_regs.regs.pc = read_sysreg_el2(elr); > - ctxt->gp_regs.regs.pstate = read_sysreg_el2(spsr); > + ctxt->hw_pstate = read_sysreg_el2(spsr); > } > > static void __hyp_text __sysreg_save_state(struct kvm_cpu_context *ctxt) > { > - ctxt->sys_regs[MPIDR_EL1] = read_sysreg(vmpidr_el2); > - ctxt->sys_regs[CSSELR_EL1] = read_sysreg(csselr_el1); > - ctxt->sys_regs[SCTLR_EL1] = read_sysreg_el1(sctlr); > - ctxt->sys_regs[CPACR_EL1] = read_sysreg_el1(cpacr); > - ctxt->sys_regs[TTBR0_EL1] = read_sysreg_el1(ttbr0); > - ctxt->sys_regs[TTBR1_EL1] = read_sysreg_el1(ttbr1); > - ctxt->sys_regs[TCR_EL1] = read_sysreg_el1(tcr); > - ctxt->sys_regs[ESR_EL1] = read_sysreg_el1(esr); > - ctxt->sys_regs[AFSR0_EL1] = read_sysreg_el1(afsr0); > - ctxt->sys_regs[AFSR1_EL1] = read_sysreg_el1(afsr1); > - ctxt->sys_regs[FAR_EL1] = read_sysreg_el1(far); > - ctxt->sys_regs[MAIR_EL1] = read_sysreg_el1(mair); > - ctxt->sys_regs[VBAR_EL1] = read_sysreg_el1(vbar); > - ctxt->sys_regs[CONTEXTIDR_EL1] = read_sysreg_el1(contextidr); > - ctxt->sys_regs[AMAIR_EL1] = read_sysreg_el1(amair); > - ctxt->sys_regs[CNTKCTL_EL1] = read_sysreg_el1(cntkctl); > - ctxt->sys_regs[PAR_EL1] = read_sysreg(par_el1); > - > - ctxt->gp_regs.sp_el1 = read_sysreg(sp_el1); > + u64 *sys_regs = kern_hyp_va(ctxt->hw_sys_regs); > + > + sys_regs[MPIDR_EL1] = read_sysreg(vmpidr_el2); > + sys_regs[CSSELR_EL1] = read_sysreg(csselr_el1); > + sys_regs[SCTLR_EL1] = read_sysreg_el1(sctlr); > + sys_regs[CPACR_EL1] = read_sysreg_el1(cpacr); > + sys_regs[TTBR0_EL1] = read_sysreg_el1(ttbr0); > + sys_regs[TTBR1_EL1] = read_sysreg_el1(ttbr1); > + sys_regs[TCR_EL1] = read_sysreg_el1(tcr); > + sys_regs[ESR_EL1] = read_sysreg_el1(esr); > + sys_regs[AFSR0_EL1] = read_sysreg_el1(afsr0); > + sys_regs[AFSR1_EL1] = read_sysreg_el1(afsr1); > + sys_regs[FAR_EL1] = read_sysreg_el1(far); > + sys_regs[MAIR_EL1] = read_sysreg_el1(mair); > + sys_regs[VBAR_EL1] = read_sysreg_el1(vbar); > + sys_regs[CONTEXTIDR_EL1] = read_sysreg_el1(contextidr); > + sys_regs[AMAIR_EL1] = read_sysreg_el1(amair); > + sys_regs[CNTKCTL_EL1] = read_sysreg_el1(cntkctl); > + sys_regs[PAR_EL1] = read_sysreg(par_el1); > + > + ctxt->hw_sp_el1 = read_sysreg(sp_el1); > ctxt->gp_regs.elr_el1 = read_sysreg_el1(elr); > ctxt->gp_regs.spsr[KVM_SPSR_EL1]= read_sysreg_el1(spsr); > } > @@ -86,37 +91,41 @@ void __hyp_text __sysreg_save_guest_state(struct kvm_cpu_context *ctxt) > > static void __hyp_text __sysreg_restore_common_state(struct kvm_cpu_context *ctxt) > { > - write_sysreg(ctxt->sys_regs[ACTLR_EL1], actlr_el1); > - write_sysreg(ctxt->sys_regs[TPIDR_EL0], tpidr_el0); > - write_sysreg(ctxt->sys_regs[TPIDRRO_EL0], tpidrro_el0); > - write_sysreg(ctxt->sys_regs[TPIDR_EL1], tpidr_el1); > - write_sysreg(ctxt->sys_regs[MDSCR_EL1], mdscr_el1); > + u64 *sys_regs = kern_hyp_va(ctxt->hw_sys_regs); > + > + write_sysreg(sys_regs[ACTLR_EL1], actlr_el1); > + write_sysreg(sys_regs[TPIDR_EL0], tpidr_el0); > + write_sysreg(sys_regs[TPIDRRO_EL0], tpidrro_el0); > + write_sysreg(sys_regs[TPIDR_EL1], tpidr_el1); > + write_sysreg(sys_regs[MDSCR_EL1], mdscr_el1); > write_sysreg(ctxt->gp_regs.regs.sp, sp_el0); > write_sysreg_el2(ctxt->gp_regs.regs.pc, elr); > - write_sysreg_el2(ctxt->gp_regs.regs.pstate, spsr); > + write_sysreg_el2(ctxt->hw_pstate, spsr); > } > > static void __hyp_text __sysreg_restore_state(struct kvm_cpu_context *ctxt) > { > - write_sysreg(ctxt->sys_regs[MPIDR_EL1], vmpidr_el2); > - write_sysreg(ctxt->sys_regs[CSSELR_EL1], csselr_el1); > - write_sysreg_el1(ctxt->sys_regs[SCTLR_EL1], sctlr); > - write_sysreg_el1(ctxt->sys_regs[CPACR_EL1], cpacr); > - write_sysreg_el1(ctxt->sys_regs[TTBR0_EL1], ttbr0); > - write_sysreg_el1(ctxt->sys_regs[TTBR1_EL1], ttbr1); > - write_sysreg_el1(ctxt->sys_regs[TCR_EL1], tcr); > - write_sysreg_el1(ctxt->sys_regs[ESR_EL1], esr); > - write_sysreg_el1(ctxt->sys_regs[AFSR0_EL1], afsr0); > - write_sysreg_el1(ctxt->sys_regs[AFSR1_EL1], afsr1); > - write_sysreg_el1(ctxt->sys_regs[FAR_EL1], far); > - write_sysreg_el1(ctxt->sys_regs[MAIR_EL1], mair); > - write_sysreg_el1(ctxt->sys_regs[VBAR_EL1], vbar); > - write_sysreg_el1(ctxt->sys_regs[CONTEXTIDR_EL1],contextidr); > - write_sysreg_el1(ctxt->sys_regs[AMAIR_EL1], amair); > - write_sysreg_el1(ctxt->sys_regs[CNTKCTL_EL1], cntkctl); > - write_sysreg(ctxt->sys_regs[PAR_EL1], par_el1); > - > - write_sysreg(ctxt->gp_regs.sp_el1, sp_el1); > + u64 *sys_regs = kern_hyp_va(ctxt->hw_sys_regs); > + > + write_sysreg(sys_regs[MPIDR_EL1], vmpidr_el2); > + write_sysreg(sys_regs[CSSELR_EL1], csselr_el1); > + write_sysreg_el1(sys_regs[SCTLR_EL1], sctlr); > + write_sysreg_el1(sys_regs[CPACR_EL1], cpacr); > + write_sysreg_el1(sys_regs[TTBR0_EL1], ttbr0); > + write_sysreg_el1(sys_regs[TTBR1_EL1], ttbr1); > + write_sysreg_el1(sys_regs[TCR_EL1], tcr); > + write_sysreg_el1(sys_regs[ESR_EL1], esr); > + write_sysreg_el1(sys_regs[AFSR0_EL1], afsr0); > + write_sysreg_el1(sys_regs[AFSR1_EL1], afsr1); > + write_sysreg_el1(sys_regs[FAR_EL1], far); > + write_sysreg_el1(sys_regs[MAIR_EL1], mair); > + write_sysreg_el1(sys_regs[VBAR_EL1], vbar); > + write_sysreg_el1(sys_regs[CONTEXTIDR_EL1], contextidr); > + write_sysreg_el1(sys_regs[AMAIR_EL1], amair); > + write_sysreg_el1(sys_regs[CNTKCTL_EL1], cntkctl); > + write_sysreg(sys_regs[PAR_EL1], par_el1); > + > + write_sysreg(ctxt->hw_sp_el1, sp_el1); > write_sysreg_el1(ctxt->gp_regs.elr_el1, elr); > write_sysreg_el1(ctxt->gp_regs.spsr[KVM_SPSR_EL1],spsr); > }
On Thu, Jun 01, 2017 at 04:05:49PM -0400, Bandan Das wrote: > Jintack Lim <jintack@cs.columbia.edu> writes: > ... > > +/** > > + * kvm_arm_setup_shadow_state -- prepare shadow state based on emulated mode > > + * @vcpu: The VCPU pointer > > + */ > > +void kvm_arm_setup_shadow_state(struct kvm_vcpu *vcpu) > > +{ > > + struct kvm_cpu_context *ctxt = &vcpu->arch.ctxt; > > + > > + ctxt->hw_pstate = *vcpu_cpsr(vcpu); > > + ctxt->hw_sys_regs = ctxt->sys_regs; > > + ctxt->hw_sp_el1 = ctxt->gp_regs.sp_el1; > > +} > > + > > +/** > > + * kvm_arm_restore_shadow_state -- write back shadow state from guest > > + * @vcpu: The VCPU pointer > > + */ > > +void kvm_arm_restore_shadow_state(struct kvm_vcpu *vcpu) > > +{ > > + struct kvm_cpu_context *ctxt = &vcpu->arch.ctxt; > > + > > + *vcpu_cpsr(vcpu) = ctxt->hw_pstate; > > + ctxt->gp_regs.sp_el1 = ctxt->hw_sp_el1; > > +} > > + > > +void kvm_arm_init_cpu_context(kvm_cpu_context_t *cpu_ctxt) > > +{ > > + cpu_ctxt->hw_sys_regs = &cpu_ctxt->sys_regs[0]; > > +} > > > IIUC, the *_shadow_state() functions will set hw_* pointers to > either point to the "real" state or the shadow state to manage L2 ? > Maybe, it might make sense to make these function names a little more > generic since they are not dealing with setting the shadow state > alone. > The notion of 'shadow state' is borrowed from shadow page tables, in which you always load some 'shadow copy' of the 'real value' into the hardware, so the shadow state is the one that's used for execution by the hardware. The shadow state may be the same as the VCPU's EL1 state, for example, or it may be a modified version of the VCPU's EL2 state, for example. If you have better suggestions for naming, we're open to that though. Thanks, -Christoffer
Christoffer Dall <cdall@linaro.org> writes: > On Thu, Jun 01, 2017 at 04:05:49PM -0400, Bandan Das wrote: >> Jintack Lim <jintack@cs.columbia.edu> writes: >> ... >> > +/** >> > + * kvm_arm_setup_shadow_state -- prepare shadow state based on emulated mode >> > + * @vcpu: The VCPU pointer >> > + */ >> > +void kvm_arm_setup_shadow_state(struct kvm_vcpu *vcpu) >> > +{ >> > + struct kvm_cpu_context *ctxt = &vcpu->arch.ctxt; >> > + >> > + ctxt->hw_pstate = *vcpu_cpsr(vcpu); >> > + ctxt->hw_sys_regs = ctxt->sys_regs; >> > + ctxt->hw_sp_el1 = ctxt->gp_regs.sp_el1; >> > +} >> > + >> > +/** >> > + * kvm_arm_restore_shadow_state -- write back shadow state from guest >> > + * @vcpu: The VCPU pointer >> > + */ >> > +void kvm_arm_restore_shadow_state(struct kvm_vcpu *vcpu) >> > +{ >> > + struct kvm_cpu_context *ctxt = &vcpu->arch.ctxt; >> > + >> > + *vcpu_cpsr(vcpu) = ctxt->hw_pstate; >> > + ctxt->gp_regs.sp_el1 = ctxt->hw_sp_el1; >> > +} >> > + >> > +void kvm_arm_init_cpu_context(kvm_cpu_context_t *cpu_ctxt) >> > +{ >> > + cpu_ctxt->hw_sys_regs = &cpu_ctxt->sys_regs[0]; >> > +} >> >> >> IIUC, the *_shadow_state() functions will set hw_* pointers to >> either point to the "real" state or the shadow state to manage L2 ? >> Maybe, it might make sense to make these function names a little more >> generic since they are not dealing with setting the shadow state >> alone. >> > > The notion of 'shadow state' is borrowed from shadow page tables, in > which you always load some 'shadow copy' of the 'real value' into the > hardware, so the shadow state is the one that's used for execution by > the hardware. > > The shadow state may be the same as the VCPU's EL1 state, for example, > or it may be a modified version of the VCPU's EL2 state, for example. Yes, it can be the same. Although, as you said above, "shadow" conventionally refers to the latter. When it's pointing to EL1 state, it's not really shadow state anymore. > If you have better suggestions for naming, we're open to that though. > Oh nothing specifically, I just felt like "shadow" in the function name could be confusing. Borrowing from kvm_arm_init_cpu_context(), how about kvm_arm_setup/restore_cpu_context() ? BTW, on a separate note, we might as well get away with the typedef and call struct kvm_cpu_context directly. > Thanks, > -Christoffer
On Fri, Jun 02, 2017 at 01:36:23PM -0400, Bandan Das wrote: > Christoffer Dall <cdall@linaro.org> writes: > > > On Thu, Jun 01, 2017 at 04:05:49PM -0400, Bandan Das wrote: > >> Jintack Lim <jintack@cs.columbia.edu> writes: > >> ... > >> > +/** > >> > + * kvm_arm_setup_shadow_state -- prepare shadow state based on emulated mode > >> > + * @vcpu: The VCPU pointer > >> > + */ > >> > +void kvm_arm_setup_shadow_state(struct kvm_vcpu *vcpu) > >> > +{ > >> > + struct kvm_cpu_context *ctxt = &vcpu->arch.ctxt; > >> > + > >> > + ctxt->hw_pstate = *vcpu_cpsr(vcpu); > >> > + ctxt->hw_sys_regs = ctxt->sys_regs; > >> > + ctxt->hw_sp_el1 = ctxt->gp_regs.sp_el1; > >> > +} > >> > + > >> > +/** > >> > + * kvm_arm_restore_shadow_state -- write back shadow state from guest > >> > + * @vcpu: The VCPU pointer > >> > + */ > >> > +void kvm_arm_restore_shadow_state(struct kvm_vcpu *vcpu) > >> > +{ > >> > + struct kvm_cpu_context *ctxt = &vcpu->arch.ctxt; > >> > + > >> > + *vcpu_cpsr(vcpu) = ctxt->hw_pstate; > >> > + ctxt->gp_regs.sp_el1 = ctxt->hw_sp_el1; > >> > +} > >> > + > >> > +void kvm_arm_init_cpu_context(kvm_cpu_context_t *cpu_ctxt) > >> > +{ > >> > + cpu_ctxt->hw_sys_regs = &cpu_ctxt->sys_regs[0]; > >> > +} > >> > >> > >> IIUC, the *_shadow_state() functions will set hw_* pointers to > >> either point to the "real" state or the shadow state to manage L2 ? > >> Maybe, it might make sense to make these function names a little more > >> generic since they are not dealing with setting the shadow state > >> alone. > >> > > > > The notion of 'shadow state' is borrowed from shadow page tables, in > > which you always load some 'shadow copy' of the 'real value' into the > > hardware, so the shadow state is the one that's used for execution by > > the hardware. > > > > The shadow state may be the same as the VCPU's EL1 state, for example, > > or it may be a modified version of the VCPU's EL2 state, for example. > > Yes, it can be the same. Although, as you said above, "shadow" conventionally > refers to the latter. That's not what I said. I said shadow is the thing you use in the hardware, which may be the same, and may be something different. The important point being, that it is what gets used by the hardware, and that it's decoupled, not necessarily different, from the virtual state. > When it's pointing to EL1 state, it's not really > shadow state anymore. > You can argue it both ways, in the end, all that's important is whether or not it's clear what the functions do. > > If you have better suggestions for naming, we're open to that though. > > > > Oh nothing specifically, I just felt like "shadow" in the function name > could be confusing. Borrowing from kvm_arm_init_cpu_context(), > how about kvm_arm_setup/restore_cpu_context() ? I have no objection to these names. > > BTW, on a separate note, we might as well get away with the typedef and > call struct kvm_cpu_context directly. > I don't think it's worth changing the code just for that, but if you feel it's a significant cleanup, you can send a patch with a good argument for why it's worth changing in the commit message. Thanks, -Christoffer
Christoffer Dall <cdall@linaro.org> writes: > On Fri, Jun 02, 2017 at 01:36:23PM -0400, Bandan Das wrote: >> Christoffer Dall <cdall@linaro.org> writes: >> >> > On Thu, Jun 01, 2017 at 04:05:49PM -0400, Bandan Das wrote: >> >> Jintack Lim <jintack@cs.columbia.edu> writes: >> >> ... >> >> > +/** >> >> > + * kvm_arm_setup_shadow_state -- prepare shadow state based on emulated mode >> >> > + * @vcpu: The VCPU pointer >> >> > + */ >> >> > +void kvm_arm_setup_shadow_state(struct kvm_vcpu *vcpu) >> >> > +{ >> >> > + struct kvm_cpu_context *ctxt = &vcpu->arch.ctxt; >> >> > + >> >> > + ctxt->hw_pstate = *vcpu_cpsr(vcpu); >> >> > + ctxt->hw_sys_regs = ctxt->sys_regs; >> >> > + ctxt->hw_sp_el1 = ctxt->gp_regs.sp_el1; >> >> > +} >> >> > + >> >> > +/** >> >> > + * kvm_arm_restore_shadow_state -- write back shadow state from guest >> >> > + * @vcpu: The VCPU pointer >> >> > + */ >> >> > +void kvm_arm_restore_shadow_state(struct kvm_vcpu *vcpu) >> >> > +{ >> >> > + struct kvm_cpu_context *ctxt = &vcpu->arch.ctxt; >> >> > + >> >> > + *vcpu_cpsr(vcpu) = ctxt->hw_pstate; >> >> > + ctxt->gp_regs.sp_el1 = ctxt->hw_sp_el1; >> >> > +} >> >> > + >> >> > +void kvm_arm_init_cpu_context(kvm_cpu_context_t *cpu_ctxt) >> >> > +{ >> >> > + cpu_ctxt->hw_sys_regs = &cpu_ctxt->sys_regs[0]; >> >> > +} >> >> >> >> >> >> IIUC, the *_shadow_state() functions will set hw_* pointers to >> >> either point to the "real" state or the shadow state to manage L2 ? >> >> Maybe, it might make sense to make these function names a little more >> >> generic since they are not dealing with setting the shadow state >> >> alone. >> >> >> > >> > The notion of 'shadow state' is borrowed from shadow page tables, in >> > which you always load some 'shadow copy' of the 'real value' into the >> > hardware, so the shadow state is the one that's used for execution by >> > the hardware. >> > >> > The shadow state may be the same as the VCPU's EL1 state, for example, >> > or it may be a modified version of the VCPU's EL2 state, for example. >> >> Yes, it can be the same. Although, as you said above, "shadow" conventionally >> refers to the latter. > > That's not what I said. I said shadow is the thing you use in the > hardware, which may be the same, and may be something different. The > important point being, that it is what gets used by the hardware, and > that it's decoupled, not necessarily different, from the virtual > state. I was referring to your first paragraph. And conventionally, in the context of shadow page tables, it is always different. >> When it's pointing to EL1 state, it's not really >> shadow state anymore. >> > > You can argue it both ways, in the end, all that's important is whether > or not it's clear what the functions do. > >> > If you have better suggestions for naming, we're open to that though. >> > >> >> Oh nothing specifically, I just felt like "shadow" in the function name >> could be confusing. Borrowing from kvm_arm_init_cpu_context(), >> how about kvm_arm_setup/restore_cpu_context() ? > > I have no objection to these names. > >> >> BTW, on a separate note, we might as well get away with the typedef and >> call struct kvm_cpu_context directly. >> > I don't think it's worth changing the code just for that, but if you > feel it's a significant cleanup, you can send a patch with a good > argument for why it's worth changing in the commit message. Sure! The cleanup is not part of the series but sticking to either one of them in this patch is. As for the argument, typedefs for structs are discouraged as part of the coding style. > Thanks, > -Christoffer
Christoffer Dall <cdall@linaro.org> writes: > [off list] Please keep discussions on list. ... > No, it's actually not. You can have entries, which are the same, and > entire tables where the guest's page table happen to be the same as what > gets put in the shadow page table. I've worked on hypervisors where > this was always pretty much the case, because memory was segmented and > physical addresses were exposed to the guest. Ok cool, I believe you! > I don't need a lecture from you about how shadow page tables work. > Go back to the review I posted. I merely asked if using "shadow" in the name could be confusing and if you think it isn't, you could have just pointed it out and that would have been the end of it. > Nevertheless, I was just trying to explain our rationale for choosing > the word shadow in our naming, and I suggest in the future you make > constructive comments with suggesting a better name, instead of pointing > out your dissatiscation, which is not helpful. > >> >> When it's pointing to EL1 state, it's not really >> >> shadow state anymore. >> >> >> > >> > You can argue it both ways, in the end, all that's important is whether >> > or not it's clear what the functions do. >> > >> >> > If you have better suggestions for naming, we're open to that though. >> >> > >> >> >> >> Oh nothing specifically, I just felt like "shadow" in the function name >> >> could be confusing. Borrowing from kvm_arm_init_cpu_context(), >> >> how about kvm_arm_setup/restore_cpu_context() ? >> > >> > I have no objection to these names. >> > >> >> >> >> BTW, on a separate note, we might as well get away with the typedef and >> >> call struct kvm_cpu_context directly. >> >> >> > I don't think it's worth changing the code just for that, but if you >> > feel it's a significant cleanup, you can send a patch with a good >> > argument for why it's worth changing in the commit message. >> >> Sure! The cleanup is not part of the series but sticking to either one >> of them in this patch is. > > I've lost track of what you're refering to. If there is a problem with > these patches, please comment on that in the right context. But, this > is an RFC, lots of things will change, and you would provide a more > helpful code review by focusing on overall design issues etc. at this > point. I know this is a RFC. Changes to RFC include small changes too like being consistent with using one type or the other. If you don't want that in your reviews, that's fine too. >> As for the argument, typedefs for structs are >> discouraged as part of the coding style. >> > Please... > > I encourage you to consider your audience. I don't think you > need to lecture me on the Linux kernel coding style. I encourage you too to take a review as a review and not as a judgement of your knowledge of the coding style or other abilities. You are discouraging people from jumping in. > -Christoffer I started discussing with Jintack and reviewing this series so that I can understand it better. If I have to constantly think that with every sentence I write I am judging someone's abilities, or that I have to follow someone's rulebook for constructive comments, well, thanks and good luck!
On Fri, Jun 02, 2017 at 04:18:08PM -0400, Bandan Das wrote: > Christoffer Dall <cdall@linaro.org> writes: > > > [off list] > > Please keep discussions on list. > ... > > No, it's actually not. You can have entries, which are the same, and > > entire tables where the guest's page table happen to be the same as what > > gets put in the shadow page table. I've worked on hypervisors where > > this was always pretty much the case, because memory was segmented and > > physical addresses were exposed to the guest. > > Ok cool, I believe you! > > > I don't need a lecture from you about how shadow page tables work. > > > > Go back to the review I posted. I merely asked if using "shadow" in the name could > be confusing And that was just fine. > > Nevertheless, I was just trying to explain our rationale for choosing > > the word shadow in our naming, and I suggest in the future you make > > constructive comments with suggesting a better name, instead of pointing > > out your dissatiscation, which is not helpful. > > [...] > > > > I've lost track of what you're refering to. If there is a problem with > > these patches, please comment on that in the right context. But, this > > is an RFC, lots of things will change, and you would provide a more > > helpful code review by focusing on overall design issues etc. at this > > point. > > I know this is a RFC. Changes to RFC include small changes too like > being consistent with using one type or the other. I just didn't want you to waste time on reviewing details thay may change, prematurely, that's all. And I honestly didn't know what you were referring to, and didn't want to go over a very large RFC patch set to find a typedef or missing typedef. Maybe I should have. > If you don't want that > in your reviews, that's fine too. > Anyone can comment on anything they like in a review. > > >> As for the argument, typedefs for structs are > >> discouraged as part of the coding style. > >> > > Please... > > > > I encourage you to consider your audience. I don't think you > > need to lecture me on the Linux kernel coding style. > > I encourage you too to take a review as a review and not as a judgement of your > knowledge of the coding style or other abilities. You are discouraging people > from jumping in. > That was not my intention. > > I started discussing with Jintack and reviewing this series so that I can understand > it better. If I have to constantly think that with every sentence I write I am judging > someone's abilities, or that I have to follow someone's rulebook for constructive comments, > well, thanks and good luck! What can I say. I of course don't have a rulebook. I did not find your tone friendly or the review particularly helpful, but I'll try to be more patient next time. -Christoffer
Christoffer Dall <cdall@linaro.org> writes: ... >> >> I started discussing with Jintack and reviewing this series so that I can understand >> it better. If I have to constantly think that with every sentence I write I am judging >> someone's abilities, or that I have to follow someone's rulebook for constructive comments, >> well, thanks and good luck! > > What can I say. I of course don't have a rulebook. I did not find your > tone friendly or the review particularly helpful, but I'll try to be > more patient next time. While I don't agree with what you are saying above, I appreciate that you will try to be patient. Not every review can be outright helpful, sometimes they are just pointers to something the reviewer prefers/likes and it's ok to ignore them. The best thing to do with an unhelpful review is to just set it aside. I didn't start out reviewing by thinking that I will pull out all tiny places where words are misspelled or structures have a typedef. I noticed the typedef usage while replying to you and thought of mentioning it. It's right there in the same patch, not across different patches and I probably would have posted a patch myself. Anyway, I will make another attempt based on your input that we should focus on the design aspect. Thanks, Bandan > -Christoffer
diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h index 399cd75e..0a03b7d 100644 --- a/arch/arm/include/asm/kvm_emulate.h +++ b/arch/arm/include/asm/kvm_emulate.h @@ -47,6 +47,10 @@ static inline void vcpu_set_reg(struct kvm_vcpu *vcpu, u8 reg_num, void kvm_inject_dabt(struct kvm_vcpu *vcpu, unsigned long addr); void kvm_inject_pabt(struct kvm_vcpu *vcpu, unsigned long addr); +static inline void kvm_arm_setup_shadow_state(struct kvm_vcpu *vcpu) { }; +static inline void kvm_arm_restore_shadow_state(struct kvm_vcpu *vcpu) { }; +static inline void kvm_arm_init_cpu_context(kvm_cpu_context_t *cpu_ctxt) { }; + static inline bool kvm_condition_valid(const struct kvm_vcpu *vcpu) { return kvm_condition_valid32(vcpu); diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c index d2dfa32..436bf5a 100644 --- a/arch/arm/kvm/arm.c +++ b/arch/arm/kvm/arm.c @@ -41,6 +41,7 @@ #include <asm/virt.h> #include <asm/kvm_arm.h> #include <asm/kvm_asm.h> +#include <asm/kvm_hyp.h> #include <asm/kvm_mmu.h> #include <asm/kvm_emulate.h> #include <asm/kvm_coproc.h> @@ -646,6 +647,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) } kvm_arm_setup_debug(vcpu); + kvm_arm_setup_shadow_state(vcpu); /************************************************************** * Enter the guest @@ -662,6 +664,7 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) * Back from guest *************************************************************/ + kvm_arm_restore_shadow_state(vcpu); kvm_arm_clear_debug(vcpu); /* @@ -1369,6 +1372,8 @@ static int init_hyp_mode(void) kvm_err("Cannot map host CPU state: %d\n", err); goto out_err; } + + kvm_arm_init_cpu_context(cpu_ctxt); } kvm_info("Hyp mode initialized successfully\n"); diff --git a/arch/arm64/include/asm/kvm_emulate.h b/arch/arm64/include/asm/kvm_emulate.h index 830be2e..8892c82 100644 --- a/arch/arm64/include/asm/kvm_emulate.h +++ b/arch/arm64/include/asm/kvm_emulate.h @@ -42,6 +42,10 @@ void kvm_inject_dabt(struct kvm_vcpu *vcpu, unsigned long addr); void kvm_inject_pabt(struct kvm_vcpu *vcpu, unsigned long addr); +void kvm_arm_setup_shadow_state(struct kvm_vcpu *vcpu); +void kvm_arm_restore_shadow_state(struct kvm_vcpu *vcpu); +void kvm_arm_init_cpu_context(kvm_cpu_context_t *cpu_ctxt); + static inline void vcpu_reset_hcr(struct kvm_vcpu *vcpu) { vcpu->arch.hcr_el2 = HCR_GUEST_FLAGS; diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile index d50a82a..7811d27 100644 --- a/arch/arm64/kvm/Makefile +++ b/arch/arm64/kvm/Makefile @@ -16,7 +16,7 @@ kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/kvm_main.o $(KVM)/coalesced_mmio.o $(KVM)/e kvm-$(CONFIG_KVM_ARM_HOST) += $(ARM)/arm.o $(ARM)/mmu.o $(ARM)/mmio.o kvm-$(CONFIG_KVM_ARM_HOST) += $(ARM)/psci.o $(ARM)/perf.o -kvm-$(CONFIG_KVM_ARM_HOST) += inject_fault.o regmap.o +kvm-$(CONFIG_KVM_ARM_HOST) += inject_fault.o regmap.o context.o kvm-$(CONFIG_KVM_ARM_HOST) += hyp.o hyp-init.o handle_exit.o kvm-$(CONFIG_KVM_ARM_HOST) += guest.o debug.o reset.o sys_regs.o sys_regs_generic_v8.o kvm-$(CONFIG_KVM_ARM_HOST) += $(KVM)/arm/aarch32.o diff --git a/arch/arm64/kvm/context.c b/arch/arm64/kvm/context.c new file mode 100644 index 0000000..320afc6 --- /dev/null +++ b/arch/arm64/kvm/context.c @@ -0,0 +1,49 @@ +/* + * Copyright (C) 2016 - Linaro Ltd. + * Author: Christoffer Dall <christoffer.dall@linaro.org> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#include <linux/kvm_host.h> +#include <asm/kvm_emulate.h> + +/** + * kvm_arm_setup_shadow_state -- prepare shadow state based on emulated mode + * @vcpu: The VCPU pointer + */ +void kvm_arm_setup_shadow_state(struct kvm_vcpu *vcpu) +{ + struct kvm_cpu_context *ctxt = &vcpu->arch.ctxt; + + ctxt->hw_pstate = *vcpu_cpsr(vcpu); + ctxt->hw_sys_regs = ctxt->sys_regs; + ctxt->hw_sp_el1 = ctxt->gp_regs.sp_el1; +} + +/** + * kvm_arm_restore_shadow_state -- write back shadow state from guest + * @vcpu: The VCPU pointer + */ +void kvm_arm_restore_shadow_state(struct kvm_vcpu *vcpu) +{ + struct kvm_cpu_context *ctxt = &vcpu->arch.ctxt; + + *vcpu_cpsr(vcpu) = ctxt->hw_pstate; + ctxt->gp_regs.sp_el1 = ctxt->hw_sp_el1; +} + +void kvm_arm_init_cpu_context(kvm_cpu_context_t *cpu_ctxt) +{ + cpu_ctxt->hw_sys_regs = &cpu_ctxt->sys_regs[0]; +} diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c index 9341376..f2a1b32 100644 --- a/arch/arm64/kvm/hyp/sysreg-sr.c +++ b/arch/arm64/kvm/hyp/sysreg-sr.c @@ -19,6 +19,7 @@ #include <linux/kvm_host.h> #include <asm/kvm_asm.h> +#include <asm/kvm_emulate.h> #include <asm/kvm_hyp.h> /* Yes, this does nothing, on purpose */ @@ -33,37 +34,41 @@ static void __hyp_text __sysreg_do_nothing(struct kvm_cpu_context *ctxt) { } static void __hyp_text __sysreg_save_common_state(struct kvm_cpu_context *ctxt) { - ctxt->sys_regs[ACTLR_EL1] = read_sysreg(actlr_el1); - ctxt->sys_regs[TPIDR_EL0] = read_sysreg(tpidr_el0); - ctxt->sys_regs[TPIDRRO_EL0] = read_sysreg(tpidrro_el0); - ctxt->sys_regs[TPIDR_EL1] = read_sysreg(tpidr_el1); - ctxt->sys_regs[MDSCR_EL1] = read_sysreg(mdscr_el1); + u64 *sys_regs = kern_hyp_va(ctxt->hw_sys_regs); + + sys_regs[ACTLR_EL1] = read_sysreg(actlr_el1); + sys_regs[TPIDR_EL0] = read_sysreg(tpidr_el0); + sys_regs[TPIDRRO_EL0] = read_sysreg(tpidrro_el0); + sys_regs[TPIDR_EL1] = read_sysreg(tpidr_el1); + sys_regs[MDSCR_EL1] = read_sysreg(mdscr_el1); ctxt->gp_regs.regs.sp = read_sysreg(sp_el0); ctxt->gp_regs.regs.pc = read_sysreg_el2(elr); - ctxt->gp_regs.regs.pstate = read_sysreg_el2(spsr); + ctxt->hw_pstate = read_sysreg_el2(spsr); } static void __hyp_text __sysreg_save_state(struct kvm_cpu_context *ctxt) { - ctxt->sys_regs[MPIDR_EL1] = read_sysreg(vmpidr_el2); - ctxt->sys_regs[CSSELR_EL1] = read_sysreg(csselr_el1); - ctxt->sys_regs[SCTLR_EL1] = read_sysreg_el1(sctlr); - ctxt->sys_regs[CPACR_EL1] = read_sysreg_el1(cpacr); - ctxt->sys_regs[TTBR0_EL1] = read_sysreg_el1(ttbr0); - ctxt->sys_regs[TTBR1_EL1] = read_sysreg_el1(ttbr1); - ctxt->sys_regs[TCR_EL1] = read_sysreg_el1(tcr); - ctxt->sys_regs[ESR_EL1] = read_sysreg_el1(esr); - ctxt->sys_regs[AFSR0_EL1] = read_sysreg_el1(afsr0); - ctxt->sys_regs[AFSR1_EL1] = read_sysreg_el1(afsr1); - ctxt->sys_regs[FAR_EL1] = read_sysreg_el1(far); - ctxt->sys_regs[MAIR_EL1] = read_sysreg_el1(mair); - ctxt->sys_regs[VBAR_EL1] = read_sysreg_el1(vbar); - ctxt->sys_regs[CONTEXTIDR_EL1] = read_sysreg_el1(contextidr); - ctxt->sys_regs[AMAIR_EL1] = read_sysreg_el1(amair); - ctxt->sys_regs[CNTKCTL_EL1] = read_sysreg_el1(cntkctl); - ctxt->sys_regs[PAR_EL1] = read_sysreg(par_el1); - - ctxt->gp_regs.sp_el1 = read_sysreg(sp_el1); + u64 *sys_regs = kern_hyp_va(ctxt->hw_sys_regs); + + sys_regs[MPIDR_EL1] = read_sysreg(vmpidr_el2); + sys_regs[CSSELR_EL1] = read_sysreg(csselr_el1); + sys_regs[SCTLR_EL1] = read_sysreg_el1(sctlr); + sys_regs[CPACR_EL1] = read_sysreg_el1(cpacr); + sys_regs[TTBR0_EL1] = read_sysreg_el1(ttbr0); + sys_regs[TTBR1_EL1] = read_sysreg_el1(ttbr1); + sys_regs[TCR_EL1] = read_sysreg_el1(tcr); + sys_regs[ESR_EL1] = read_sysreg_el1(esr); + sys_regs[AFSR0_EL1] = read_sysreg_el1(afsr0); + sys_regs[AFSR1_EL1] = read_sysreg_el1(afsr1); + sys_regs[FAR_EL1] = read_sysreg_el1(far); + sys_regs[MAIR_EL1] = read_sysreg_el1(mair); + sys_regs[VBAR_EL1] = read_sysreg_el1(vbar); + sys_regs[CONTEXTIDR_EL1] = read_sysreg_el1(contextidr); + sys_regs[AMAIR_EL1] = read_sysreg_el1(amair); + sys_regs[CNTKCTL_EL1] = read_sysreg_el1(cntkctl); + sys_regs[PAR_EL1] = read_sysreg(par_el1); + + ctxt->hw_sp_el1 = read_sysreg(sp_el1); ctxt->gp_regs.elr_el1 = read_sysreg_el1(elr); ctxt->gp_regs.spsr[KVM_SPSR_EL1]= read_sysreg_el1(spsr); } @@ -86,37 +91,41 @@ void __hyp_text __sysreg_save_guest_state(struct kvm_cpu_context *ctxt) static void __hyp_text __sysreg_restore_common_state(struct kvm_cpu_context *ctxt) { - write_sysreg(ctxt->sys_regs[ACTLR_EL1], actlr_el1); - write_sysreg(ctxt->sys_regs[TPIDR_EL0], tpidr_el0); - write_sysreg(ctxt->sys_regs[TPIDRRO_EL0], tpidrro_el0); - write_sysreg(ctxt->sys_regs[TPIDR_EL1], tpidr_el1); - write_sysreg(ctxt->sys_regs[MDSCR_EL1], mdscr_el1); + u64 *sys_regs = kern_hyp_va(ctxt->hw_sys_regs); + + write_sysreg(sys_regs[ACTLR_EL1], actlr_el1); + write_sysreg(sys_regs[TPIDR_EL0], tpidr_el0); + write_sysreg(sys_regs[TPIDRRO_EL0], tpidrro_el0); + write_sysreg(sys_regs[TPIDR_EL1], tpidr_el1); + write_sysreg(sys_regs[MDSCR_EL1], mdscr_el1); write_sysreg(ctxt->gp_regs.regs.sp, sp_el0); write_sysreg_el2(ctxt->gp_regs.regs.pc, elr); - write_sysreg_el2(ctxt->gp_regs.regs.pstate, spsr); + write_sysreg_el2(ctxt->hw_pstate, spsr); } static void __hyp_text __sysreg_restore_state(struct kvm_cpu_context *ctxt) { - write_sysreg(ctxt->sys_regs[MPIDR_EL1], vmpidr_el2); - write_sysreg(ctxt->sys_regs[CSSELR_EL1], csselr_el1); - write_sysreg_el1(ctxt->sys_regs[SCTLR_EL1], sctlr); - write_sysreg_el1(ctxt->sys_regs[CPACR_EL1], cpacr); - write_sysreg_el1(ctxt->sys_regs[TTBR0_EL1], ttbr0); - write_sysreg_el1(ctxt->sys_regs[TTBR1_EL1], ttbr1); - write_sysreg_el1(ctxt->sys_regs[TCR_EL1], tcr); - write_sysreg_el1(ctxt->sys_regs[ESR_EL1], esr); - write_sysreg_el1(ctxt->sys_regs[AFSR0_EL1], afsr0); - write_sysreg_el1(ctxt->sys_regs[AFSR1_EL1], afsr1); - write_sysreg_el1(ctxt->sys_regs[FAR_EL1], far); - write_sysreg_el1(ctxt->sys_regs[MAIR_EL1], mair); - write_sysreg_el1(ctxt->sys_regs[VBAR_EL1], vbar); - write_sysreg_el1(ctxt->sys_regs[CONTEXTIDR_EL1],contextidr); - write_sysreg_el1(ctxt->sys_regs[AMAIR_EL1], amair); - write_sysreg_el1(ctxt->sys_regs[CNTKCTL_EL1], cntkctl); - write_sysreg(ctxt->sys_regs[PAR_EL1], par_el1); - - write_sysreg(ctxt->gp_regs.sp_el1, sp_el1); + u64 *sys_regs = kern_hyp_va(ctxt->hw_sys_regs); + + write_sysreg(sys_regs[MPIDR_EL1], vmpidr_el2); + write_sysreg(sys_regs[CSSELR_EL1], csselr_el1); + write_sysreg_el1(sys_regs[SCTLR_EL1], sctlr); + write_sysreg_el1(sys_regs[CPACR_EL1], cpacr); + write_sysreg_el1(sys_regs[TTBR0_EL1], ttbr0); + write_sysreg_el1(sys_regs[TTBR1_EL1], ttbr1); + write_sysreg_el1(sys_regs[TCR_EL1], tcr); + write_sysreg_el1(sys_regs[ESR_EL1], esr); + write_sysreg_el1(sys_regs[AFSR0_EL1], afsr0); + write_sysreg_el1(sys_regs[AFSR1_EL1], afsr1); + write_sysreg_el1(sys_regs[FAR_EL1], far); + write_sysreg_el1(sys_regs[MAIR_EL1], mair); + write_sysreg_el1(sys_regs[VBAR_EL1], vbar); + write_sysreg_el1(sys_regs[CONTEXTIDR_EL1], contextidr); + write_sysreg_el1(sys_regs[AMAIR_EL1], amair); + write_sysreg_el1(sys_regs[CNTKCTL_EL1], cntkctl); + write_sysreg(sys_regs[PAR_EL1], par_el1); + + write_sysreg(ctxt->hw_sp_el1, sp_el1); write_sysreg_el1(ctxt->gp_regs.elr_el1, elr); write_sysreg_el1(ctxt->gp_regs.spsr[KVM_SPSR_EL1],spsr); }