| Message ID | 1453737235-16522-11-git-send-email-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 25, 2016 at 03:53:44PM +0000, Marc Zyngier wrote: > A handful of system registers are still shared between EL1 and EL2, > even while using VHE. These are tpidr*_el[01], actlr_el1, sp0, elr, > and spsr. So by shared registers you mean registers that do both have an EL0/1 version as well as an EL2 version, but where accesses aren't rewritten transparently? also, by sp0 do you mean sp_el0, and by elr you mean elr_el1, and by spsr you mean spsr_el1 ? > > In order to facilitate the introduction of a VHE-specific sysreg > save/restore, make move the access to these registers to their > own save/restore functions. > > No functionnal change. Otherwise: Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > arch/arm64/kvm/hyp/sysreg-sr.c | 48 +++++++++++++++++++++++++++++------------- > 1 file changed, 33 insertions(+), 15 deletions(-) > > diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c > index bd5b543..61bad17 100644 > --- a/arch/arm64/kvm/hyp/sysreg-sr.c > +++ b/arch/arm64/kvm/hyp/sysreg-sr.c > @@ -23,13 +23,29 @@ > > #include "hyp.h" > > -/* ctxt is already in the HYP VA space */ > +/* > + * Non-VHE: Both host and guest must save everything. > + * > + * VHE: Host must save tpidr*_el[01], actlr_el1, sp0, pc, pstate, and > + * guest must save everything. > + */ > + > +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->gp_regs.regs.sp = read_sysreg(sp_el0); > + ctxt->gp_regs.regs.pc = read_sysreg(elr_el2); > + ctxt->gp_regs.regs.pstate = read_sysreg(spsr_el2); > +} > + > 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(sctlr_el1); > - ctxt->sys_regs[ACTLR_EL1] = read_sysreg(actlr_el1); > ctxt->sys_regs[CPACR_EL1] = read_sysreg(cpacr_el1); > ctxt->sys_regs[TTBR0_EL1] = read_sysreg(ttbr0_el1); > ctxt->sys_regs[TTBR1_EL1] = read_sysreg(ttbr1_el1); > @@ -41,17 +57,11 @@ static void __hyp_text __sysreg_save_state(struct kvm_cpu_context *ctxt) > ctxt->sys_regs[MAIR_EL1] = read_sysreg(mair_el1); > ctxt->sys_regs[VBAR_EL1] = read_sysreg(vbar_el1); > ctxt->sys_regs[CONTEXTIDR_EL1] = read_sysreg(contextidr_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[AMAIR_EL1] = read_sysreg(amair_el1); > ctxt->sys_regs[CNTKCTL_EL1] = read_sysreg(cntkctl_el1); > ctxt->sys_regs[PAR_EL1] = read_sysreg(par_el1); > ctxt->sys_regs[MDSCR_EL1] = read_sysreg(mdscr_el1); > > - ctxt->gp_regs.regs.sp = read_sysreg(sp_el0); > - ctxt->gp_regs.regs.pc = read_sysreg(elr_el2); > - ctxt->gp_regs.regs.pstate = read_sysreg(spsr_el2); > ctxt->gp_regs.sp_el1 = read_sysreg(sp_el1); > ctxt->gp_regs.elr_el1 = read_sysreg(elr_el1); > ctxt->gp_regs.spsr[KVM_SPSR_EL1]= read_sysreg(spsr_el1); > @@ -60,11 +70,24 @@ static void __hyp_text __sysreg_save_state(struct kvm_cpu_context *ctxt) > void __hyp_text __sysreg_save_host_state(struct kvm_cpu_context *ctxt) > { > __sysreg_save_state(ctxt); > + __sysreg_save_common_state(ctxt); > } > > void __hyp_text __sysreg_save_guest_state(struct kvm_cpu_context *ctxt) > { > __sysreg_save_state(ctxt); > + __sysreg_save_common_state(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->gp_regs.regs.sp, sp_el0); > + write_sysreg(ctxt->gp_regs.regs.pc, elr_el2); > + write_sysreg(ctxt->gp_regs.regs.pstate, spsr_el2); > } > > static void __hyp_text __sysreg_restore_state(struct kvm_cpu_context *ctxt) > @@ -72,7 +95,6 @@ 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(ctxt->sys_regs[SCTLR_EL1], sctlr_el1); > - write_sysreg(ctxt->sys_regs[ACTLR_EL1], actlr_el1); > write_sysreg(ctxt->sys_regs[CPACR_EL1], cpacr_el1); > write_sysreg(ctxt->sys_regs[TTBR0_EL1], ttbr0_el1); > write_sysreg(ctxt->sys_regs[TTBR1_EL1], ttbr1_el1); > @@ -84,17 +106,11 @@ static void __hyp_text __sysreg_restore_state(struct kvm_cpu_context *ctxt) > write_sysreg(ctxt->sys_regs[MAIR_EL1], mair_el1); > write_sysreg(ctxt->sys_regs[VBAR_EL1], vbar_el1); > write_sysreg(ctxt->sys_regs[CONTEXTIDR_EL1], contextidr_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[AMAIR_EL1], amair_el1); > write_sysreg(ctxt->sys_regs[CNTKCTL_EL1], cntkctl_el1); > write_sysreg(ctxt->sys_regs[PAR_EL1], par_el1); > write_sysreg(ctxt->sys_regs[MDSCR_EL1], mdscr_el1); > > - write_sysreg(ctxt->gp_regs.regs.sp, sp_el0); > - write_sysreg(ctxt->gp_regs.regs.pc, elr_el2); > - write_sysreg(ctxt->gp_regs.regs.pstate, spsr_el2); > write_sysreg(ctxt->gp_regs.sp_el1, sp_el1); > write_sysreg(ctxt->gp_regs.elr_el1, elr_el1); > write_sysreg(ctxt->gp_regs.spsr[KVM_SPSR_EL1], spsr_el1); > @@ -103,11 +119,13 @@ static void __hyp_text __sysreg_restore_state(struct kvm_cpu_context *ctxt) > void __hyp_text __sysreg_restore_host_state(struct kvm_cpu_context *ctxt) > { > __sysreg_restore_state(ctxt); > + __sysreg_restore_common_state(ctxt); > } > > void __hyp_text __sysreg_restore_guest_state(struct kvm_cpu_context *ctxt) > { > __sysreg_restore_state(ctxt); > + __sysreg_restore_common_state(ctxt); > } > > void __hyp_text __sysreg32_save_state(struct kvm_vcpu *vcpu) > -- > 2.1.4 > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/02/16 13:54, Christoffer Dall wrote: > On Mon, Jan 25, 2016 at 03:53:44PM +0000, Marc Zyngier wrote: >> A handful of system registers are still shared between EL1 and EL2, >> even while using VHE. These are tpidr*_el[01], actlr_el1, sp0, elr, >> and spsr. > > So by shared registers you mean registers that do both have an EL0/1 > version as well as an EL2 version, but where accesses aren't rewritten > transparently? No, I mean that these registers do *not* have a separate banked version. There is only a single set of registers, which have to be save/restored the old way. > > also, by sp0 do you mean sp_el0, and by elr you mean elr_el1, and by > spsr you mean spsr_el1 ? sp0 -> sp_el0 indeed. elr and spsr really are the guest PC and PSTATE, so I should really reword this commit message, it is utterly confusing. > >> >> In order to facilitate the introduction of a VHE-specific sysreg >> save/restore, make move the access to these registers to their >> own save/restore functions. >> >> No functionnal change. > > Otherwise: > > Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> Thanks, M.
On Tue, Feb 02, 2016 at 09:46:05AM +0000, Marc Zyngier wrote: > On 01/02/16 13:54, Christoffer Dall wrote: > > On Mon, Jan 25, 2016 at 03:53:44PM +0000, Marc Zyngier wrote: > >> A handful of system registers are still shared between EL1 and EL2, > >> even while using VHE. These are tpidr*_el[01], actlr_el1, sp0, elr, > >> and spsr. > > > > So by shared registers you mean registers that do both have an EL0/1 > > version as well as an EL2 version, but where accesses aren't rewritten > > transparently? > > No, I mean that these registers do *not* have a separate banked version. > There is only a single set of registers, which have to be save/restored > the old way. huh, ARMv8 clearly specifies the existence of TPIDR_EL0, TPIDR_EL1, and TPIDR_EL2, for example. I cannot seem to find anywhere in the VHE spec that says that the TPIDR_EL2 goes away. I'm confused now. > > > > > also, by sp0 do you mean sp_el0, and by elr you mean elr_el1, and by > > spsr you mean spsr_el1 ? > > sp0 -> sp_el0 indeed. elr and spsr really are the guest PC and PSTATE, > so I should really reword this commit message, it is utterly confusing. > I guess I don't understand the definition of a 'shared' register given your comments here... Thanks, -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/02/16 15:46, Christoffer Dall wrote: > On Tue, Feb 02, 2016 at 09:46:05AM +0000, Marc Zyngier wrote: >> On 01/02/16 13:54, Christoffer Dall wrote: >>> On Mon, Jan 25, 2016 at 03:53:44PM +0000, Marc Zyngier wrote: >>>> A handful of system registers are still shared between EL1 and EL2, >>>> even while using VHE. These are tpidr*_el[01], actlr_el1, sp0, elr, >>>> and spsr. >>> >>> So by shared registers you mean registers that do both have an EL0/1 >>> version as well as an EL2 version, but where accesses aren't rewritten >>> transparently? >> >> No, I mean that these registers do *not* have a separate banked version. >> There is only a single set of registers, which have to be save/restored >> the old way. > > huh, ARMv8 clearly specifies the existence of TPIDR_EL0, TPIDR_EL1, and > TPIDR_EL2, for example. > > I cannot seem to find anywhere in the VHE spec that says that the > TPIDR_EL2 goes away. I'm confused now. Nothing goes away, but these registers do not get renamed either. For example, TPIDR_EL1 doesn't magically access TPIDR_EL2 when running at EL2+VHE, and there is no TPIDR_EL12 accessor either. So TPIDR_EL1 is effectively "shared" between host and guest, and must be save/restored (note that the host kernel still uses TIPDR_EL1 even when running with VHE, and that KVM still uses TPIDR_EL2 to cache the current vcpu). >> >>> >>> also, by sp0 do you mean sp_el0, and by elr you mean elr_el1, and by >>> spsr you mean spsr_el1 ? >> >> sp0 -> sp_el0 indeed. elr and spsr really are the guest PC and PSTATE, >> so I should really reword this commit message, it is utterly confusing. >> > I guess I don't understand the definition of a 'shared' register given > your comments here... Does this make it clearer? Thanks, M.
On Tue, Feb 02, 2016 at 04:19:44PM +0000, Marc Zyngier wrote: > On 02/02/16 15:46, Christoffer Dall wrote: > > On Tue, Feb 02, 2016 at 09:46:05AM +0000, Marc Zyngier wrote: > >> On 01/02/16 13:54, Christoffer Dall wrote: > >>> On Mon, Jan 25, 2016 at 03:53:44PM +0000, Marc Zyngier wrote: > >>>> A handful of system registers are still shared between EL1 and EL2, > >>>> even while using VHE. These are tpidr*_el[01], actlr_el1, sp0, elr, > >>>> and spsr. > >>> > >>> So by shared registers you mean registers that do both have an EL0/1 > >>> version as well as an EL2 version, but where accesses aren't rewritten > >>> transparently? > >> > >> No, I mean that these registers do *not* have a separate banked version. > >> There is only a single set of registers, which have to be save/restored > >> the old way. > > > > huh, ARMv8 clearly specifies the existence of TPIDR_EL0, TPIDR_EL1, and > > TPIDR_EL2, for example. > > > > I cannot seem to find anywhere in the VHE spec that says that the > > TPIDR_EL2 goes away. I'm confused now. > > Nothing goes away, but these registers do not get renamed either. For > example, TPIDR_EL1 doesn't magically access TPIDR_EL2 when running at > EL2+VHE, and there is no TPIDR_EL12 accessor either. > > So TPIDR_EL1 is effectively "shared" between host and guest, and must be > save/restored (note that the host kernel still uses TIPDR_EL1 even when > running with VHE, and that KVM still uses TPIDR_EL2 to cache the current > vcpu). > ok, I can understand as long as we're saying a register is shared between the host and the guest, but it was the "registers are shared between EL1 and EL2" that threw me off. > >> > >>> > >>> also, by sp0 do you mean sp_el0, and by elr you mean elr_el1, and by > >>> spsr you mean spsr_el1 ? > >> > >> sp0 -> sp_el0 indeed. elr and spsr really are the guest PC and PSTATE, > >> so I should really reword this commit message, it is utterly confusing. > >> > > I guess I don't understand the definition of a 'shared' register given > > your comments here... > > Does this make it clearer? > yes. You could change the host to path it when using VHE to use TPIDR_EL2 if you wanted and store the vcpu pointer on the stack while running the guest, but there's probably no real benefit of doing so. I'll be shutting up now... Thanks, -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c index bd5b543..61bad17 100644 --- a/arch/arm64/kvm/hyp/sysreg-sr.c +++ b/arch/arm64/kvm/hyp/sysreg-sr.c @@ -23,13 +23,29 @@ #include "hyp.h" -/* ctxt is already in the HYP VA space */ +/* + * Non-VHE: Both host and guest must save everything. + * + * VHE: Host must save tpidr*_el[01], actlr_el1, sp0, pc, pstate, and + * guest must save everything. + */ + +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->gp_regs.regs.sp = read_sysreg(sp_el0); + ctxt->gp_regs.regs.pc = read_sysreg(elr_el2); + ctxt->gp_regs.regs.pstate = read_sysreg(spsr_el2); +} + 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(sctlr_el1); - ctxt->sys_regs[ACTLR_EL1] = read_sysreg(actlr_el1); ctxt->sys_regs[CPACR_EL1] = read_sysreg(cpacr_el1); ctxt->sys_regs[TTBR0_EL1] = read_sysreg(ttbr0_el1); ctxt->sys_regs[TTBR1_EL1] = read_sysreg(ttbr1_el1); @@ -41,17 +57,11 @@ static void __hyp_text __sysreg_save_state(struct kvm_cpu_context *ctxt) ctxt->sys_regs[MAIR_EL1] = read_sysreg(mair_el1); ctxt->sys_regs[VBAR_EL1] = read_sysreg(vbar_el1); ctxt->sys_regs[CONTEXTIDR_EL1] = read_sysreg(contextidr_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[AMAIR_EL1] = read_sysreg(amair_el1); ctxt->sys_regs[CNTKCTL_EL1] = read_sysreg(cntkctl_el1); ctxt->sys_regs[PAR_EL1] = read_sysreg(par_el1); ctxt->sys_regs[MDSCR_EL1] = read_sysreg(mdscr_el1); - ctxt->gp_regs.regs.sp = read_sysreg(sp_el0); - ctxt->gp_regs.regs.pc = read_sysreg(elr_el2); - ctxt->gp_regs.regs.pstate = read_sysreg(spsr_el2); ctxt->gp_regs.sp_el1 = read_sysreg(sp_el1); ctxt->gp_regs.elr_el1 = read_sysreg(elr_el1); ctxt->gp_regs.spsr[KVM_SPSR_EL1]= read_sysreg(spsr_el1); @@ -60,11 +70,24 @@ static void __hyp_text __sysreg_save_state(struct kvm_cpu_context *ctxt) void __hyp_text __sysreg_save_host_state(struct kvm_cpu_context *ctxt) { __sysreg_save_state(ctxt); + __sysreg_save_common_state(ctxt); } void __hyp_text __sysreg_save_guest_state(struct kvm_cpu_context *ctxt) { __sysreg_save_state(ctxt); + __sysreg_save_common_state(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->gp_regs.regs.sp, sp_el0); + write_sysreg(ctxt->gp_regs.regs.pc, elr_el2); + write_sysreg(ctxt->gp_regs.regs.pstate, spsr_el2); } static void __hyp_text __sysreg_restore_state(struct kvm_cpu_context *ctxt) @@ -72,7 +95,6 @@ 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(ctxt->sys_regs[SCTLR_EL1], sctlr_el1); - write_sysreg(ctxt->sys_regs[ACTLR_EL1], actlr_el1); write_sysreg(ctxt->sys_regs[CPACR_EL1], cpacr_el1); write_sysreg(ctxt->sys_regs[TTBR0_EL1], ttbr0_el1); write_sysreg(ctxt->sys_regs[TTBR1_EL1], ttbr1_el1); @@ -84,17 +106,11 @@ static void __hyp_text __sysreg_restore_state(struct kvm_cpu_context *ctxt) write_sysreg(ctxt->sys_regs[MAIR_EL1], mair_el1); write_sysreg(ctxt->sys_regs[VBAR_EL1], vbar_el1); write_sysreg(ctxt->sys_regs[CONTEXTIDR_EL1], contextidr_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[AMAIR_EL1], amair_el1); write_sysreg(ctxt->sys_regs[CNTKCTL_EL1], cntkctl_el1); write_sysreg(ctxt->sys_regs[PAR_EL1], par_el1); write_sysreg(ctxt->sys_regs[MDSCR_EL1], mdscr_el1); - write_sysreg(ctxt->gp_regs.regs.sp, sp_el0); - write_sysreg(ctxt->gp_regs.regs.pc, elr_el2); - write_sysreg(ctxt->gp_regs.regs.pstate, spsr_el2); write_sysreg(ctxt->gp_regs.sp_el1, sp_el1); write_sysreg(ctxt->gp_regs.elr_el1, elr_el1); write_sysreg(ctxt->gp_regs.spsr[KVM_SPSR_EL1], spsr_el1); @@ -103,11 +119,13 @@ static void __hyp_text __sysreg_restore_state(struct kvm_cpu_context *ctxt) void __hyp_text __sysreg_restore_host_state(struct kvm_cpu_context *ctxt) { __sysreg_restore_state(ctxt); + __sysreg_restore_common_state(ctxt); } void __hyp_text __sysreg_restore_guest_state(struct kvm_cpu_context *ctxt) { __sysreg_restore_state(ctxt); + __sysreg_restore_common_state(ctxt); } void __hyp_text __sysreg32_save_state(struct kvm_vcpu *vcpu)
A handful of system registers are still shared between EL1 and EL2, even while using VHE. These are tpidr*_el[01], actlr_el1, sp0, elr, and spsr. In order to facilitate the introduction of a VHE-specific sysreg save/restore, make move the access to these registers to their own save/restore functions. No functionnal change. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- arch/arm64/kvm/hyp/sysreg-sr.c | 48 +++++++++++++++++++++++++++++------------- 1 file changed, 33 insertions(+), 15 deletions(-)