| Message ID | 20200526161834.29165-3-james.morse@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: aarch32 ACTLR accesses | expand |
On 2020-05-26 17:18, James Morse wrote: > KVM sets HCR_EL2.TACR (which it calls HCR_TAC) via HCR_GUEST_FLAGS. TAC is a leftover from 32bit. > This means ACTLR* accesses from the guest are always trapped, and > always return the value in the sys_regs array. > > The guest can't change the value of these registers, so we are > save restoring the reset value, which came from the host. > > Stop save/restoring this register. > > This also stops this register being affected by sysregs_loaded_on_cpu, > so we can provide 32 bit accessors that always use the in-memory copy. > > Signed-off-by: James Morse <james.morse@arm.com> > --- > arch/arm64/kvm/hyp/sysreg-sr.c | 2 -- > arch/arm64/kvm/sys_regs.c | 2 -- > 2 files changed, 4 deletions(-) > > diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c > b/arch/arm64/kvm/hyp/sysreg-sr.c > index 75b1925763f1..57116cf3a1a5 100644 > --- a/arch/arm64/kvm/hyp/sysreg-sr.c > +++ b/arch/arm64/kvm/hyp/sysreg-sr.c > @@ -44,7 +44,6 @@ static void __hyp_text > __sysreg_save_el1_state(struct kvm_cpu_context *ctxt) > { > ctxt->sys_regs[CSSELR_EL1] = read_sysreg(csselr_el1); > ctxt->sys_regs[SCTLR_EL1] = read_sysreg_el1(SYS_SCTLR); > - ctxt->sys_regs[ACTLR_EL1] = read_sysreg(actlr_el1); > ctxt->sys_regs[CPACR_EL1] = read_sysreg_el1(SYS_CPACR); > ctxt->sys_regs[TTBR0_EL1] = read_sysreg_el1(SYS_TTBR0); > ctxt->sys_regs[TTBR1_EL1] = read_sysreg_el1(SYS_TTBR1); > @@ -133,7 +132,6 @@ static void __hyp_text > __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt) > isb(); > } > > - write_sysreg(ctxt->sys_regs[ACTLR_EL1], actlr_el1); If we don't need to save/restore it, we can also drop its presence in the sys_regs array. > write_sysreg_el1(ctxt->sys_regs[CPACR_EL1], SYS_CPACR); > write_sysreg_el1(ctxt->sys_regs[TTBR0_EL1], SYS_TTBR0); > write_sysreg_el1(ctxt->sys_regs[TTBR1_EL1], SYS_TTBR1); > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c > index 2eda539f3281..aae58513025c 100644 > --- a/arch/arm64/kvm/sys_regs.c > +++ b/arch/arm64/kvm/sys_regs.c > @@ -81,7 +81,6 @@ u64 vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, > int reg) > switch (reg) { > case CSSELR_EL1: return read_sysreg_s(SYS_CSSELR_EL1); > case SCTLR_EL1: return read_sysreg_s(SYS_SCTLR_EL12); > - case ACTLR_EL1: return read_sysreg_s(SYS_ACTLR_EL1); > case CPACR_EL1: return read_sysreg_s(SYS_CPACR_EL12); > case TTBR0_EL1: return read_sysreg_s(SYS_TTBR0_EL12); > case TTBR1_EL1: return read_sysreg_s(SYS_TTBR1_EL12); > @@ -124,7 +123,6 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 > val, int reg) > switch (reg) { > case CSSELR_EL1: write_sysreg_s(val, SYS_CSSELR_EL1); return; > case SCTLR_EL1: write_sysreg_s(val, SYS_SCTLR_EL12); return; > - case ACTLR_EL1: write_sysreg_s(val, SYS_ACTLR_EL1); return; > case CPACR_EL1: write_sysreg_s(val, SYS_CPACR_EL12); return; > case TTBR0_EL1: write_sysreg_s(val, SYS_TTBR0_EL12); return; > case TTBR1_EL1: write_sysreg_s(val, SYS_TTBR1_EL12); return; It strikes me that we don't even have a trap handler for this sysreg, whether it is 32 or 64bit... That's a bit unfortunate, to say the least... M.
On 2020-05-28 13:36, Marc Zyngier wrote: > On 2020-05-26 17:18, James Morse wrote: >> KVM sets HCR_EL2.TACR (which it calls HCR_TAC) via HCR_GUEST_FLAGS. > > TAC is a leftover from 32bit. > >> This means ACTLR* accesses from the guest are always trapped, and >> always return the value in the sys_regs array. >> >> The guest can't change the value of these registers, so we are >> save restoring the reset value, which came from the host. >> >> Stop save/restoring this register. >> >> This also stops this register being affected by sysregs_loaded_on_cpu, >> so we can provide 32 bit accessors that always use the in-memory copy. >> >> Signed-off-by: James Morse <james.morse@arm.com> >> --- >> arch/arm64/kvm/hyp/sysreg-sr.c | 2 -- >> arch/arm64/kvm/sys_regs.c | 2 -- >> 2 files changed, 4 deletions(-) >> >> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c >> b/arch/arm64/kvm/hyp/sysreg-sr.c >> index 75b1925763f1..57116cf3a1a5 100644 >> --- a/arch/arm64/kvm/hyp/sysreg-sr.c >> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c >> @@ -44,7 +44,6 @@ static void __hyp_text >> __sysreg_save_el1_state(struct kvm_cpu_context *ctxt) >> { >> ctxt->sys_regs[CSSELR_EL1] = read_sysreg(csselr_el1); >> ctxt->sys_regs[SCTLR_EL1] = read_sysreg_el1(SYS_SCTLR); >> - ctxt->sys_regs[ACTLR_EL1] = read_sysreg(actlr_el1); >> ctxt->sys_regs[CPACR_EL1] = read_sysreg_el1(SYS_CPACR); >> ctxt->sys_regs[TTBR0_EL1] = read_sysreg_el1(SYS_TTBR0); >> ctxt->sys_regs[TTBR1_EL1] = read_sysreg_el1(SYS_TTBR1); >> @@ -133,7 +132,6 @@ static void __hyp_text >> __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt) >> isb(); >> } >> >> - write_sysreg(ctxt->sys_regs[ACTLR_EL1], actlr_el1); > > If we don't need to save/restore it, we can also drop its presence > in the sys_regs array. > >> write_sysreg_el1(ctxt->sys_regs[CPACR_EL1], SYS_CPACR); >> write_sysreg_el1(ctxt->sys_regs[TTBR0_EL1], SYS_TTBR0); >> write_sysreg_el1(ctxt->sys_regs[TTBR1_EL1], SYS_TTBR1); >> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c >> index 2eda539f3281..aae58513025c 100644 >> --- a/arch/arm64/kvm/sys_regs.c >> +++ b/arch/arm64/kvm/sys_regs.c >> @@ -81,7 +81,6 @@ u64 vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, >> int reg) >> switch (reg) { >> case CSSELR_EL1: return read_sysreg_s(SYS_CSSELR_EL1); >> case SCTLR_EL1: return read_sysreg_s(SYS_SCTLR_EL12); >> - case ACTLR_EL1: return read_sysreg_s(SYS_ACTLR_EL1); >> case CPACR_EL1: return read_sysreg_s(SYS_CPACR_EL12); >> case TTBR0_EL1: return read_sysreg_s(SYS_TTBR0_EL12); >> case TTBR1_EL1: return read_sysreg_s(SYS_TTBR1_EL12); >> @@ -124,7 +123,6 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 >> val, int reg) >> switch (reg) { >> case CSSELR_EL1: write_sysreg_s(val, SYS_CSSELR_EL1); return; >> case SCTLR_EL1: write_sysreg_s(val, SYS_SCTLR_EL12); return; >> - case ACTLR_EL1: write_sysreg_s(val, SYS_ACTLR_EL1); return; >> case CPACR_EL1: write_sysreg_s(val, SYS_CPACR_EL12); return; >> case TTBR0_EL1: write_sysreg_s(val, SYS_TTBR0_EL12); return; >> case TTBR1_EL1: write_sysreg_s(val, SYS_TTBR1_EL12); return; > > It strikes me that we don't even have a trap handler for this sysreg, > whether it is 32 or 64bit... That's a bit unfortunate, to say the > least... Ah, no. the sucker is hidden away in "generic_v8"... M.
Hi Marc, On 28/05/2020 13:38, Marc Zyngier wrote: > On 2020-05-28 13:36, Marc Zyngier wrote: >> On 2020-05-26 17:18, James Morse wrote: >>> KVM sets HCR_EL2.TACR (which it calls HCR_TAC) via HCR_GUEST_FLAGS. >>> This means ACTLR* accesses from the guest are always trapped, and >>> always return the value in the sys_regs array. >>> >>> The guest can't change the value of these registers, so we are >>> save restoring the reset value, which came from the host. >>> >>> Stop save/restoring this register. >>> >>> This also stops this register being affected by sysregs_loaded_on_cpu, >>> so we can provide 32 bit accessors that always use the in-memory copy. >>> diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c >>> index 75b1925763f1..57116cf3a1a5 100644 >>> --- a/arch/arm64/kvm/hyp/sysreg-sr.c >>> +++ b/arch/arm64/kvm/hyp/sysreg-sr.c >>> @@ -133,7 +132,6 @@ static void __hyp_text >>> __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt) >>> isb(); >>> } >>> >>> - write_sysreg(ctxt->sys_regs[ACTLR_EL1], actlr_el1); >> >> If we don't need to save/restore it, we can also drop its presence >> in the sys_regs array. So even user-space accesses read from the hardware register? Fine by me. >> It strikes me that we don't even have a trap handler for this sysreg, >> whether it is 32 or 64bit... That's a bit unfortunate, to say the >> least... > > Ah, no. the sucker is hidden away in "generic_v8"... That thing is A7/A15 (and then user-ABI) legacy right? I was looking at ripping all that out when I ran over these. RFC grade, known not to bisect: http://www.linux-arm.org/git?p=linux-jm.git;a=shortlog;h=refs/heads/kvm_kill_target_table/v0 Thanks, James
diff --git a/arch/arm64/kvm/hyp/sysreg-sr.c b/arch/arm64/kvm/hyp/sysreg-sr.c index 75b1925763f1..57116cf3a1a5 100644 --- a/arch/arm64/kvm/hyp/sysreg-sr.c +++ b/arch/arm64/kvm/hyp/sysreg-sr.c @@ -44,7 +44,6 @@ static void __hyp_text __sysreg_save_el1_state(struct kvm_cpu_context *ctxt) { ctxt->sys_regs[CSSELR_EL1] = read_sysreg(csselr_el1); ctxt->sys_regs[SCTLR_EL1] = read_sysreg_el1(SYS_SCTLR); - ctxt->sys_regs[ACTLR_EL1] = read_sysreg(actlr_el1); ctxt->sys_regs[CPACR_EL1] = read_sysreg_el1(SYS_CPACR); ctxt->sys_regs[TTBR0_EL1] = read_sysreg_el1(SYS_TTBR0); ctxt->sys_regs[TTBR1_EL1] = read_sysreg_el1(SYS_TTBR1); @@ -133,7 +132,6 @@ static void __hyp_text __sysreg_restore_el1_state(struct kvm_cpu_context *ctxt) isb(); } - write_sysreg(ctxt->sys_regs[ACTLR_EL1], actlr_el1); write_sysreg_el1(ctxt->sys_regs[CPACR_EL1], SYS_CPACR); write_sysreg_el1(ctxt->sys_regs[TTBR0_EL1], SYS_TTBR0); write_sysreg_el1(ctxt->sys_regs[TTBR1_EL1], SYS_TTBR1); diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c index 2eda539f3281..aae58513025c 100644 --- a/arch/arm64/kvm/sys_regs.c +++ b/arch/arm64/kvm/sys_regs.c @@ -81,7 +81,6 @@ u64 vcpu_read_sys_reg(const struct kvm_vcpu *vcpu, int reg) switch (reg) { case CSSELR_EL1: return read_sysreg_s(SYS_CSSELR_EL1); case SCTLR_EL1: return read_sysreg_s(SYS_SCTLR_EL12); - case ACTLR_EL1: return read_sysreg_s(SYS_ACTLR_EL1); case CPACR_EL1: return read_sysreg_s(SYS_CPACR_EL12); case TTBR0_EL1: return read_sysreg_s(SYS_TTBR0_EL12); case TTBR1_EL1: return read_sysreg_s(SYS_TTBR1_EL12); @@ -124,7 +123,6 @@ void vcpu_write_sys_reg(struct kvm_vcpu *vcpu, u64 val, int reg) switch (reg) { case CSSELR_EL1: write_sysreg_s(val, SYS_CSSELR_EL1); return; case SCTLR_EL1: write_sysreg_s(val, SYS_SCTLR_EL12); return; - case ACTLR_EL1: write_sysreg_s(val, SYS_ACTLR_EL1); return; case CPACR_EL1: write_sysreg_s(val, SYS_CPACR_EL12); return; case TTBR0_EL1: write_sysreg_s(val, SYS_TTBR0_EL12); return; case TTBR1_EL1: write_sysreg_s(val, SYS_TTBR1_EL12); return;
KVM sets HCR_EL2.TACR (which it calls HCR_TAC) via HCR_GUEST_FLAGS. This means ACTLR* accesses from the guest are always trapped, and always return the value in the sys_regs array. The guest can't change the value of these registers, so we are save restoring the reset value, which came from the host. Stop save/restoring this register. This also stops this register being affected by sysregs_loaded_on_cpu, so we can provide 32 bit accessors that always use the in-memory copy. Signed-off-by: James Morse <james.morse@arm.com> --- arch/arm64/kvm/hyp/sysreg-sr.c | 2 -- arch/arm64/kvm/sys_regs.c | 2 -- 2 files changed, 4 deletions(-)