| Message ID | 20180215210332.8648-32-christoffer.dall@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 15 Feb 2018 21:03:23 +0000, Christoffer Dall wrote: > > As we are about to be more lazy with some of the trap configuration > register read/writes for VHE systems, move the logic that is currently > shared between VHE and non-VHE into a separate function which can be > called from either the world-switch path or from vcpu_load/vcpu_put. > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > --- > > Notes: > Changes since v3: > - Separate fpsimd32 trap configuration into a separate function > which is still called from __activate_traps, because we no longer > defer saving/restoring of VFP registers to load/put. > > arch/arm64/kvm/hyp/switch.c | 76 +++++++++++++++++++++++++++------------------ > 1 file changed, 45 insertions(+), 31 deletions(-) > > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > index 909aa3fe9196..17e3c6f26a34 100644 > --- a/arch/arm64/kvm/hyp/switch.c > +++ b/arch/arm64/kvm/hyp/switch.c > @@ -56,7 +56,45 @@ static inline void __hyp_text __fpsimd_save_fpexc32(struct kvm_vcpu *vcpu) > vcpu->arch.ctxt.sys_regs[FPEXC32_EL2] = read_sysreg(fpexc32_el2); > } > > -static void __hyp_text __activate_traps_vhe(void) > +static void __hyp_text __activate_traps_fpsimd32(struct kvm_vcpu *vcpu) > +{ > + /* > + * We are about to set CPTR_EL2.TFP to trap all floating point > + * register accesses to EL2, however, the ARM ARM clearly states that > + * traps are only taken to EL2 if the operation would not otherwise > + * trap to EL1. Therefore, always make sure that for 32-bit guests, > + * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit. > + * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to > + * it will cause an exception. > + */ > + if (vcpu_el1_is_32bit(vcpu) && system_supports_fpsimd()) { > + write_sysreg(1 << 30, fpexc32_el2); > + isb(); > + } > +} > + > +static void __hyp_text __activate_traps_common(struct kvm_vcpu *vcpu) > +{ > + /* Trap on AArch32 cp15 c15 (impdef sysregs) accesses (EL1 or EL0) */ > + write_sysreg(1 << 15, hstr_el2); > + /* > + * Make sure we trap PMU access from EL0 to EL2. Also sanitize > + * PMSELR_EL0 to make sure it never contains the cycle > + * counter, which could make a PMXEVCNTR_EL0 access UNDEF at > + * EL1 instead of being trapped to EL2. > + */ > + write_sysreg(0, pmselr_el0); > + write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0); > + write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2); > +} > + > +static void __hyp_text __deactivate_traps_common(void) > +{ > + write_sysreg(0, hstr_el2); > + write_sysreg(0, pmuserenr_el0); > +} > + > +static void __hyp_text __activate_traps_vhe(struct kvm_vcpu *vcpu) > { > u64 val; > > @@ -68,7 +106,7 @@ static void __hyp_text __activate_traps_vhe(void) > write_sysreg(kvm_get_hyp_vector(), vbar_el1); > } > > -static void __hyp_text __activate_traps_nvhe(void) > +static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu) I have the ugly feeling that this hunk should not be in this patch. Have you tried bisecting the compilation of this series? > { > u64 val; > > @@ -85,37 +123,14 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) > { > u64 hcr = vcpu->arch.hcr_el2; > > - /* > - * We are about to set CPTR_EL2.TFP to trap all floating point > - * register accesses to EL2, however, the ARM ARM clearly states that > - * traps are only taken to EL2 if the operation would not otherwise > - * trap to EL1. Therefore, always make sure that for 32-bit guests, > - * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit. > - * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to > - * it will cause an exception. > - */ > - if (vcpu_el1_is_32bit(vcpu) && system_supports_fpsimd()) { > - write_sysreg(1 << 30, fpexc32_el2); > - isb(); > - } > + write_sysreg(hcr, hcr_el2); > > if (cpus_have_const_cap(ARM64_HAS_RAS_EXTN) && (hcr & HCR_VSE)) > write_sysreg_s(vcpu->arch.vsesr_el2, SYS_VSESR_EL2); > > - write_sysreg(hcr, hcr_el2); > - > - /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ > - write_sysreg(1 << 15, hstr_el2); > - /* > - * Make sure we trap PMU access from EL0 to EL2. Also sanitize > - * PMSELR_EL0 to make sure it never contains the cycle > - * counter, which could make a PMXEVCNTR_EL0 access UNDEF at > - * EL1 instead of being trapped to EL2. > - */ > - write_sysreg(0, pmselr_el0); > - write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0); > - write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2); > - __activate_traps_arch()(); > + __activate_traps_fpsimd32(vcpu); > + __activate_traps_common(vcpu); > + __activate_traps_arch()(vcpu); > } > > static void __hyp_text __deactivate_traps_vhe(void) > @@ -160,9 +175,8 @@ static void __hyp_text __deactivate_traps(struct kvm_vcpu *vcpu) > if (vcpu->arch.hcr_el2 & HCR_VSE) > vcpu->arch.hcr_el2 = read_sysreg(hcr_el2); > > + __deactivate_traps_common(); > __deactivate_traps_arch()(); > - write_sysreg(0, hstr_el2); > - write_sysreg(0, pmuserenr_el0); > } > > static void __hyp_text __activate_vm(struct kvm *kvm) > -- > 2.14.2 > Otherwise: Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> M.
On Thu, Feb 15, 2018 at 10:03:23PM +0100, Christoffer Dall wrote: > As we are about to be more lazy with some of the trap configuration > register read/writes for VHE systems, move the logic that is currently > shared between VHE and non-VHE into a separate function which can be > called from either the world-switch path or from vcpu_load/vcpu_put. > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > --- > > Notes: > Changes since v3: > - Separate fpsimd32 trap configuration into a separate function > which is still called from __activate_traps, because we no longer > defer saving/restoring of VFP registers to load/put. > > arch/arm64/kvm/hyp/switch.c | 76 +++++++++++++++++++++++++++------------------ > 1 file changed, 45 insertions(+), 31 deletions(-) > > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > index 909aa3fe9196..17e3c6f26a34 100644 > --- a/arch/arm64/kvm/hyp/switch.c > +++ b/arch/arm64/kvm/hyp/switch.c > @@ -56,7 +56,45 @@ static inline void __hyp_text __fpsimd_save_fpexc32(struct kvm_vcpu *vcpu) > vcpu->arch.ctxt.sys_regs[FPEXC32_EL2] = read_sysreg(fpexc32_el2); > } > > -static void __hyp_text __activate_traps_vhe(void) > +static void __hyp_text __activate_traps_fpsimd32(struct kvm_vcpu *vcpu) > +{ > + /* > + * We are about to set CPTR_EL2.TFP to trap all floating point > + * register accesses to EL2, however, the ARM ARM clearly states that > + * traps are only taken to EL2 if the operation would not otherwise > + * trap to EL1. Therefore, always make sure that for 32-bit guests, > + * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit. > + * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to > + * it will cause an exception. > + */ > + if (vcpu_el1_is_32bit(vcpu) && system_supports_fpsimd()) { > + write_sysreg(1 << 30, fpexc32_el2); > + isb(); > + } > +} > + > +static void __hyp_text __activate_traps_common(struct kvm_vcpu *vcpu) > +{ > + /* Trap on AArch32 cp15 c15 (impdef sysregs) accesses (EL1 or EL0) */ > + write_sysreg(1 << 15, hstr_el2); Could use a blank line here. > + /* > + * Make sure we trap PMU access from EL0 to EL2. Also sanitize > + * PMSELR_EL0 to make sure it never contains the cycle > + * counter, which could make a PMXEVCNTR_EL0 access UNDEF at > + * EL1 instead of being trapped to EL2. > + */ > + write_sysreg(0, pmselr_el0); > + write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0); > + write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2); > +} > + > +static void __hyp_text __deactivate_traps_common(void) > +{ > + write_sysreg(0, hstr_el2); > + write_sysreg(0, pmuserenr_el0); > +} > + > +static void __hyp_text __activate_traps_vhe(struct kvm_vcpu *vcpu) > { > u64 val; > > @@ -68,7 +106,7 @@ static void __hyp_text __activate_traps_vhe(void) > write_sysreg(kvm_get_hyp_vector(), vbar_el1); > } > > -static void __hyp_text __activate_traps_nvhe(void) > +static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu) > { > u64 val; > > @@ -85,37 +123,14 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) > { > u64 hcr = vcpu->arch.hcr_el2; > > - /* > - * We are about to set CPTR_EL2.TFP to trap all floating point > - * register accesses to EL2, however, the ARM ARM clearly states that > - * traps are only taken to EL2 if the operation would not otherwise > - * trap to EL1. Therefore, always make sure that for 32-bit guests, > - * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit. > - * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to > - * it will cause an exception. > - */ > - if (vcpu_el1_is_32bit(vcpu) && system_supports_fpsimd()) { > - write_sysreg(1 << 30, fpexc32_el2); > - isb(); > - } > + write_sysreg(hcr, hcr_el2); > > if (cpus_have_const_cap(ARM64_HAS_RAS_EXTN) && (hcr & HCR_VSE)) > write_sysreg_s(vcpu->arch.vsesr_el2, SYS_VSESR_EL2); > > - write_sysreg(hcr, hcr_el2); > - > - /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ > - write_sysreg(1 << 15, hstr_el2); > - /* > - * Make sure we trap PMU access from EL0 to EL2. Also sanitize > - * PMSELR_EL0 to make sure it never contains the cycle > - * counter, which could make a PMXEVCNTR_EL0 access UNDEF at > - * EL1 instead of being trapped to EL2. > - */ > - write_sysreg(0, pmselr_el0); > - write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0); > - write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2); > - __activate_traps_arch()(); > + __activate_traps_fpsimd32(vcpu); > + __activate_traps_common(vcpu); > + __activate_traps_arch()(vcpu); > } > > static void __hyp_text __deactivate_traps_vhe(void) > @@ -160,9 +175,8 @@ static void __hyp_text __deactivate_traps(struct kvm_vcpu *vcpu) > if (vcpu->arch.hcr_el2 & HCR_VSE) > vcpu->arch.hcr_el2 = read_sysreg(hcr_el2); > > + __deactivate_traps_common(); > __deactivate_traps_arch()(); > - write_sysreg(0, hstr_el2); > - write_sysreg(0, pmuserenr_el0); > } > > static void __hyp_text __activate_vm(struct kvm *kvm) > -- > 2.14.2 > Reviewed-by: Andrew Jones <drjones@redhat.com>
On Wed, Feb 21, 2018 at 05:59:37PM +0000, Marc Zyngier wrote: > On Thu, 15 Feb 2018 21:03:23 +0000, > Christoffer Dall wrote: > > > > As we are about to be more lazy with some of the trap configuration > > register read/writes for VHE systems, move the logic that is currently > > shared between VHE and non-VHE into a separate function which can be > > called from either the world-switch path or from vcpu_load/vcpu_put. > > > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > > --- > > > > Notes: > > Changes since v3: > > - Separate fpsimd32 trap configuration into a separate function > > which is still called from __activate_traps, because we no longer > > defer saving/restoring of VFP registers to load/put. > > > > arch/arm64/kvm/hyp/switch.c | 76 +++++++++++++++++++++++++++------------------ > > 1 file changed, 45 insertions(+), 31 deletions(-) > > > > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > > index 909aa3fe9196..17e3c6f26a34 100644 > > --- a/arch/arm64/kvm/hyp/switch.c > > +++ b/arch/arm64/kvm/hyp/switch.c > > @@ -56,7 +56,45 @@ static inline void __hyp_text __fpsimd_save_fpexc32(struct kvm_vcpu *vcpu) > > vcpu->arch.ctxt.sys_regs[FPEXC32_EL2] = read_sysreg(fpexc32_el2); > > } > > > > -static void __hyp_text __activate_traps_vhe(void) > > +static void __hyp_text __activate_traps_fpsimd32(struct kvm_vcpu *vcpu) > > +{ > > + /* > > + * We are about to set CPTR_EL2.TFP to trap all floating point > > + * register accesses to EL2, however, the ARM ARM clearly states that > > + * traps are only taken to EL2 if the operation would not otherwise > > + * trap to EL1. Therefore, always make sure that for 32-bit guests, > > + * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit. > > + * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to > > + * it will cause an exception. > > + */ > > + if (vcpu_el1_is_32bit(vcpu) && system_supports_fpsimd()) { > > + write_sysreg(1 << 30, fpexc32_el2); > > + isb(); > > + } > > +} > > + > > +static void __hyp_text __activate_traps_common(struct kvm_vcpu *vcpu) > > +{ > > + /* Trap on AArch32 cp15 c15 (impdef sysregs) accesses (EL1 or EL0) */ > > + write_sysreg(1 << 15, hstr_el2); > > + /* > > + * Make sure we trap PMU access from EL0 to EL2. Also sanitize > > + * PMSELR_EL0 to make sure it never contains the cycle > > + * counter, which could make a PMXEVCNTR_EL0 access UNDEF at > > + * EL1 instead of being trapped to EL2. > > + */ > > + write_sysreg(0, pmselr_el0); > > + write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0); > > + write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2); > > +} > > + > > +static void __hyp_text __deactivate_traps_common(void) > > +{ > > + write_sysreg(0, hstr_el2); > > + write_sysreg(0, pmuserenr_el0); > > +} > > + > > +static void __hyp_text __activate_traps_vhe(struct kvm_vcpu *vcpu) > > { > > u64 val; > > > > @@ -68,7 +106,7 @@ static void __hyp_text __activate_traps_vhe(void) > > write_sysreg(kvm_get_hyp_vector(), vbar_el1); > > } > > > > -static void __hyp_text __activate_traps_nvhe(void) > > +static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu) > > I have the ugly feeling that this hunk should not be in this > patch. Have you tried bisecting the compilation of this series? > I have, and I seem to remember catching this one during that exact exercise, but I probably committed the change to the wrong patch. Duh. Thanks for spotting. -Christoffer > > { > > u64 val; > > > > @@ -85,37 +123,14 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) > > { > > u64 hcr = vcpu->arch.hcr_el2; > > > > - /* > > - * We are about to set CPTR_EL2.TFP to trap all floating point > > - * register accesses to EL2, however, the ARM ARM clearly states that > > - * traps are only taken to EL2 if the operation would not otherwise > > - * trap to EL1. Therefore, always make sure that for 32-bit guests, > > - * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit. > > - * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to > > - * it will cause an exception. > > - */ > > - if (vcpu_el1_is_32bit(vcpu) && system_supports_fpsimd()) { > > - write_sysreg(1 << 30, fpexc32_el2); > > - isb(); > > - } > > + write_sysreg(hcr, hcr_el2); > > > > if (cpus_have_const_cap(ARM64_HAS_RAS_EXTN) && (hcr & HCR_VSE)) > > write_sysreg_s(vcpu->arch.vsesr_el2, SYS_VSESR_EL2); > > > > - write_sysreg(hcr, hcr_el2); > > - > > - /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ > > - write_sysreg(1 << 15, hstr_el2); > > - /* > > - * Make sure we trap PMU access from EL0 to EL2. Also sanitize > > - * PMSELR_EL0 to make sure it never contains the cycle > > - * counter, which could make a PMXEVCNTR_EL0 access UNDEF at > > - * EL1 instead of being trapped to EL2. > > - */ > > - write_sysreg(0, pmselr_el0); > > - write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0); > > - write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2); > > - __activate_traps_arch()(); > > + __activate_traps_fpsimd32(vcpu); > > + __activate_traps_common(vcpu); > > + __activate_traps_arch()(vcpu); > > } > > > > static void __hyp_text __deactivate_traps_vhe(void) > > @@ -160,9 +175,8 @@ static void __hyp_text __deactivate_traps(struct kvm_vcpu *vcpu) > > if (vcpu->arch.hcr_el2 & HCR_VSE) > > vcpu->arch.hcr_el2 = read_sysreg(hcr_el2); > > > > + __deactivate_traps_common(); > > __deactivate_traps_arch()(); > > - write_sysreg(0, hstr_el2); > > - write_sysreg(0, pmuserenr_el0); > > } > > > > static void __hyp_text __activate_vm(struct kvm *kvm) > > -- > > 2.14.2 > > > > Otherwise: > > Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> > > M. > > -- > Jazz is not dead, it just smell funny.
Hi Christoffer, On 15/02/18 21:03, Christoffer Dall wrote: > @@ -85,37 +123,14 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) > { > u64 hcr = vcpu->arch.hcr_el2; > > - /* > - * We are about to set CPTR_EL2.TFP to trap all floating point > - * register accesses to EL2, however, the ARM ARM clearly states that > - * traps are only taken to EL2 if the operation would not otherwise > - * trap to EL1. Therefore, always make sure that for 32-bit guests, > - * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit. > - * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to > - * it will cause an exception. > - */ > - if (vcpu_el1_is_32bit(vcpu) && system_supports_fpsimd()) { > - write_sysreg(1 << 30, fpexc32_el2); > - isb(); > - } > + write_sysreg(hcr, hcr_el2); > > if (cpus_have_const_cap(ARM64_HAS_RAS_EXTN) && (hcr & HCR_VSE)) > write_sysreg_s(vcpu->arch.vsesr_el2, SYS_VSESR_EL2); > > - write_sysreg(hcr, hcr_el2); OOI, any reason to move the write to HCR_EL2 just before the if? > - > - /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ > - write_sysreg(1 << 15, hstr_el2); > - /* > - * Make sure we trap PMU access from EL0 to EL2. Also sanitize > - * PMSELR_EL0 to make sure it never contains the cycle > - * counter, which could make a PMXEVCNTR_EL0 access UNDEF at > - * EL1 instead of being trapped to EL2. > - */ > - write_sysreg(0, pmselr_el0); > - write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0); > - write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2); > - __activate_traps_arch()(); > + __activate_traps_fpsimd32(vcpu); > + __activate_traps_common(vcpu); > + __activate_traps_arch()(vcpu); > } > > static void __hyp_text __deactivate_traps_vhe(void) > @@ -160,9 +175,8 @@ static void __hyp_text __deactivate_traps(struct kvm_vcpu *vcpu) > if (vcpu->arch.hcr_el2 & HCR_VSE) > vcpu->arch.hcr_el2 = read_sysreg(hcr_el2); > > + __deactivate_traps_common(); > __deactivate_traps_arch()(); > - write_sysreg(0, hstr_el2); > - write_sysreg(0, pmuserenr_el0); > } > > static void __hyp_text __activate_vm(struct kvm *kvm) > Cheers,
On Fri, Feb 23, 2018 at 02:30:54PM +0000, Julien Grall wrote: > Hi Christoffer, > > On 15/02/18 21:03, Christoffer Dall wrote: > >@@ -85,37 +123,14 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) > > { > > u64 hcr = vcpu->arch.hcr_el2; > >- /* > >- * We are about to set CPTR_EL2.TFP to trap all floating point > >- * register accesses to EL2, however, the ARM ARM clearly states that > >- * traps are only taken to EL2 if the operation would not otherwise > >- * trap to EL1. Therefore, always make sure that for 32-bit guests, > >- * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit. > >- * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to > >- * it will cause an exception. > >- */ > >- if (vcpu_el1_is_32bit(vcpu) && system_supports_fpsimd()) { > >- write_sysreg(1 << 30, fpexc32_el2); > >- isb(); > >- } > >+ write_sysreg(hcr, hcr_el2); > > if (cpus_have_const_cap(ARM64_HAS_RAS_EXTN) && (hcr & HCR_VSE)) > > write_sysreg_s(vcpu->arch.vsesr_el2, SYS_VSESR_EL2); > >- write_sysreg(hcr, hcr_el2); > > OOI, any reason to move the write to HCR_EL2 just before the if? > I suppose not, I think this is just a rebasing artifact. > >- > >- /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ > >- write_sysreg(1 << 15, hstr_el2); > >- /* > >- * Make sure we trap PMU access from EL0 to EL2. Also sanitize > >- * PMSELR_EL0 to make sure it never contains the cycle > >- * counter, which could make a PMXEVCNTR_EL0 access UNDEF at > >- * EL1 instead of being trapped to EL2. > >- */ > >- write_sysreg(0, pmselr_el0); > >- write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0); > >- write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2); > >- __activate_traps_arch()(); > >+ __activate_traps_fpsimd32(vcpu); > >+ __activate_traps_common(vcpu); > >+ __activate_traps_arch()(vcpu); > > } > > static void __hyp_text __deactivate_traps_vhe(void) > >@@ -160,9 +175,8 @@ static void __hyp_text __deactivate_traps(struct kvm_vcpu *vcpu) > > if (vcpu->arch.hcr_el2 & HCR_VSE) > > vcpu->arch.hcr_el2 = read_sysreg(hcr_el2); > >+ __deactivate_traps_common(); > > __deactivate_traps_arch()(); > >- write_sysreg(0, hstr_el2); > >- write_sysreg(0, pmuserenr_el0); > > } > > static void __hyp_text __activate_vm(struct kvm *kvm) > > > Thanks, -Christoffer
On Wed, Feb 21, 2018 at 05:59:37PM +0000, Marc Zyngier wrote: > On Thu, 15 Feb 2018 21:03:23 +0000, > Christoffer Dall wrote: > > > > As we are about to be more lazy with some of the trap configuration > > register read/writes for VHE systems, move the logic that is currently > > shared between VHE and non-VHE into a separate function which can be > > called from either the world-switch path or from vcpu_load/vcpu_put. > > > > Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> > > --- > > > > Notes: > > Changes since v3: > > - Separate fpsimd32 trap configuration into a separate function > > which is still called from __activate_traps, because we no longer > > defer saving/restoring of VFP registers to load/put. > > > > arch/arm64/kvm/hyp/switch.c | 76 +++++++++++++++++++++++++++------------------ > > 1 file changed, 45 insertions(+), 31 deletions(-) > > > > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > > index 909aa3fe9196..17e3c6f26a34 100644 > > --- a/arch/arm64/kvm/hyp/switch.c > > +++ b/arch/arm64/kvm/hyp/switch.c > > @@ -56,7 +56,45 @@ static inline void __hyp_text __fpsimd_save_fpexc32(struct kvm_vcpu *vcpu) > > vcpu->arch.ctxt.sys_regs[FPEXC32_EL2] = read_sysreg(fpexc32_el2); > > } > > > > -static void __hyp_text __activate_traps_vhe(void) > > +static void __hyp_text __activate_traps_fpsimd32(struct kvm_vcpu *vcpu) > > +{ > > + /* > > + * We are about to set CPTR_EL2.TFP to trap all floating point > > + * register accesses to EL2, however, the ARM ARM clearly states that > > + * traps are only taken to EL2 if the operation would not otherwise > > + * trap to EL1. Therefore, always make sure that for 32-bit guests, > > + * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit. > > + * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to > > + * it will cause an exception. > > + */ > > + if (vcpu_el1_is_32bit(vcpu) && system_supports_fpsimd()) { > > + write_sysreg(1 << 30, fpexc32_el2); > > + isb(); > > + } > > +} > > + > > +static void __hyp_text __activate_traps_common(struct kvm_vcpu *vcpu) > > +{ > > + /* Trap on AArch32 cp15 c15 (impdef sysregs) accesses (EL1 or EL0) */ > > + write_sysreg(1 << 15, hstr_el2); > > + /* > > + * Make sure we trap PMU access from EL0 to EL2. Also sanitize > > + * PMSELR_EL0 to make sure it never contains the cycle > > + * counter, which could make a PMXEVCNTR_EL0 access UNDEF at > > + * EL1 instead of being trapped to EL2. > > + */ > > + write_sysreg(0, pmselr_el0); > > + write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0); > > + write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2); > > +} > > + > > +static void __hyp_text __deactivate_traps_common(void) > > +{ > > + write_sysreg(0, hstr_el2); > > + write_sysreg(0, pmuserenr_el0); > > +} > > + > > +static void __hyp_text __activate_traps_vhe(struct kvm_vcpu *vcpu) > > { > > u64 val; > > > > @@ -68,7 +106,7 @@ static void __hyp_text __activate_traps_vhe(void) > > write_sysreg(kvm_get_hyp_vector(), vbar_el1); > > } > > > > -static void __hyp_text __activate_traps_nvhe(void) > > +static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu) > > I have the ugly feeling that this hunk should not be in this > patch. Have you tried bisecting the compilation of this series? > Actually, I don't see why this shouldn't be in this patch? We change both versions of the functions and we change the call to __activate_traps_arch() below. We just don't change the hyp alternative, because it doesn't care about arguments and parameters, it just cares the the types of the functions are the same. Admittedly, this patch is really hard to read, it's easier to apply it and just look at the code. > > { > > u64 val; > > > > @@ -85,37 +123,14 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) > > { > > u64 hcr = vcpu->arch.hcr_el2; > > > > - /* > > - * We are about to set CPTR_EL2.TFP to trap all floating point > > - * register accesses to EL2, however, the ARM ARM clearly states that > > - * traps are only taken to EL2 if the operation would not otherwise > > - * trap to EL1. Therefore, always make sure that for 32-bit guests, > > - * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit. > > - * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to > > - * it will cause an exception. > > - */ > > - if (vcpu_el1_is_32bit(vcpu) && system_supports_fpsimd()) { > > - write_sysreg(1 << 30, fpexc32_el2); > > - isb(); > > - } > > + write_sysreg(hcr, hcr_el2); > > > > if (cpus_have_const_cap(ARM64_HAS_RAS_EXTN) && (hcr & HCR_VSE)) > > write_sysreg_s(vcpu->arch.vsesr_el2, SYS_VSESR_EL2); > > > > - write_sysreg(hcr, hcr_el2); > > - > > - /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ > > - write_sysreg(1 << 15, hstr_el2); > > - /* > > - * Make sure we trap PMU access from EL0 to EL2. Also sanitize > > - * PMSELR_EL0 to make sure it never contains the cycle > > - * counter, which could make a PMXEVCNTR_EL0 access UNDEF at > > - * EL1 instead of being trapped to EL2. > > - */ > > - write_sysreg(0, pmselr_el0); > > - write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0); > > - write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2); > > - __activate_traps_arch()(); > > + __activate_traps_fpsimd32(vcpu); > > + __activate_traps_common(vcpu); > > + __activate_traps_arch()(vcpu); > > } > > > > static void __hyp_text __deactivate_traps_vhe(void) > > @@ -160,9 +175,8 @@ static void __hyp_text __deactivate_traps(struct kvm_vcpu *vcpu) > > if (vcpu->arch.hcr_el2 & HCR_VSE) > > vcpu->arch.hcr_el2 = read_sysreg(hcr_el2); > > > > + __deactivate_traps_common(); > > __deactivate_traps_arch()(); > > - write_sysreg(0, hstr_el2); > > - write_sysreg(0, pmuserenr_el0); > > } > > > > static void __hyp_text __activate_vm(struct kvm *kvm) > > -- > > 2.14.2 > > > > Otherwise: > > Reviewed-by: Marc Zyngier <marc.zyngier@arm.com> > Thanks! -Christoffer
On Fri, Feb 23, 2018 at 02:30:54PM +0000, Julien Grall wrote: > Hi Christoffer, > > On 15/02/18 21:03, Christoffer Dall wrote: > >@@ -85,37 +123,14 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) > > { > > u64 hcr = vcpu->arch.hcr_el2; > >- /* > >- * We are about to set CPTR_EL2.TFP to trap all floating point > >- * register accesses to EL2, however, the ARM ARM clearly states that > >- * traps are only taken to EL2 if the operation would not otherwise > >- * trap to EL1. Therefore, always make sure that for 32-bit guests, > >- * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit. > >- * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to > >- * it will cause an exception. > >- */ > >- if (vcpu_el1_is_32bit(vcpu) && system_supports_fpsimd()) { > >- write_sysreg(1 << 30, fpexc32_el2); > >- isb(); > >- } > >+ write_sysreg(hcr, hcr_el2); > > if (cpus_have_const_cap(ARM64_HAS_RAS_EXTN) && (hcr & HCR_VSE)) > > write_sysreg_s(vcpu->arch.vsesr_el2, SYS_VSESR_EL2); > >- write_sysreg(hcr, hcr_el2); > > OOI, any reason to move the write to HCR_EL2 just before the if? > Just to keep the two lines together where we read the value from the vcpu structure and write it to hardware. It's hard to tell from this patch, but I think it looks nicer in the end. Thanks, -Christoffer
diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c index 909aa3fe9196..17e3c6f26a34 100644 --- a/arch/arm64/kvm/hyp/switch.c +++ b/arch/arm64/kvm/hyp/switch.c @@ -56,7 +56,45 @@ static inline void __hyp_text __fpsimd_save_fpexc32(struct kvm_vcpu *vcpu) vcpu->arch.ctxt.sys_regs[FPEXC32_EL2] = read_sysreg(fpexc32_el2); } -static void __hyp_text __activate_traps_vhe(void) +static void __hyp_text __activate_traps_fpsimd32(struct kvm_vcpu *vcpu) +{ + /* + * We are about to set CPTR_EL2.TFP to trap all floating point + * register accesses to EL2, however, the ARM ARM clearly states that + * traps are only taken to EL2 if the operation would not otherwise + * trap to EL1. Therefore, always make sure that for 32-bit guests, + * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit. + * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to + * it will cause an exception. + */ + if (vcpu_el1_is_32bit(vcpu) && system_supports_fpsimd()) { + write_sysreg(1 << 30, fpexc32_el2); + isb(); + } +} + +static void __hyp_text __activate_traps_common(struct kvm_vcpu *vcpu) +{ + /* Trap on AArch32 cp15 c15 (impdef sysregs) accesses (EL1 or EL0) */ + write_sysreg(1 << 15, hstr_el2); + /* + * Make sure we trap PMU access from EL0 to EL2. Also sanitize + * PMSELR_EL0 to make sure it never contains the cycle + * counter, which could make a PMXEVCNTR_EL0 access UNDEF at + * EL1 instead of being trapped to EL2. + */ + write_sysreg(0, pmselr_el0); + write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0); + write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2); +} + +static void __hyp_text __deactivate_traps_common(void) +{ + write_sysreg(0, hstr_el2); + write_sysreg(0, pmuserenr_el0); +} + +static void __hyp_text __activate_traps_vhe(struct kvm_vcpu *vcpu) { u64 val; @@ -68,7 +106,7 @@ static void __hyp_text __activate_traps_vhe(void) write_sysreg(kvm_get_hyp_vector(), vbar_el1); } -static void __hyp_text __activate_traps_nvhe(void) +static void __hyp_text __activate_traps_nvhe(struct kvm_vcpu *vcpu) { u64 val; @@ -85,37 +123,14 @@ static void __hyp_text __activate_traps(struct kvm_vcpu *vcpu) { u64 hcr = vcpu->arch.hcr_el2; - /* - * We are about to set CPTR_EL2.TFP to trap all floating point - * register accesses to EL2, however, the ARM ARM clearly states that - * traps are only taken to EL2 if the operation would not otherwise - * trap to EL1. Therefore, always make sure that for 32-bit guests, - * we set FPEXC.EN to prevent traps to EL1, when setting the TFP bit. - * If FP/ASIMD is not implemented, FPEXC is UNDEFINED and any access to - * it will cause an exception. - */ - if (vcpu_el1_is_32bit(vcpu) && system_supports_fpsimd()) { - write_sysreg(1 << 30, fpexc32_el2); - isb(); - } + write_sysreg(hcr, hcr_el2); if (cpus_have_const_cap(ARM64_HAS_RAS_EXTN) && (hcr & HCR_VSE)) write_sysreg_s(vcpu->arch.vsesr_el2, SYS_VSESR_EL2); - write_sysreg(hcr, hcr_el2); - - /* Trap on AArch32 cp15 c15 accesses (EL1 or EL0) */ - write_sysreg(1 << 15, hstr_el2); - /* - * Make sure we trap PMU access from EL0 to EL2. Also sanitize - * PMSELR_EL0 to make sure it never contains the cycle - * counter, which could make a PMXEVCNTR_EL0 access UNDEF at - * EL1 instead of being trapped to EL2. - */ - write_sysreg(0, pmselr_el0); - write_sysreg(ARMV8_PMU_USERENR_MASK, pmuserenr_el0); - write_sysreg(vcpu->arch.mdcr_el2, mdcr_el2); - __activate_traps_arch()(); + __activate_traps_fpsimd32(vcpu); + __activate_traps_common(vcpu); + __activate_traps_arch()(vcpu); } static void __hyp_text __deactivate_traps_vhe(void) @@ -160,9 +175,8 @@ static void __hyp_text __deactivate_traps(struct kvm_vcpu *vcpu) if (vcpu->arch.hcr_el2 & HCR_VSE) vcpu->arch.hcr_el2 = read_sysreg(hcr_el2); + __deactivate_traps_common(); __deactivate_traps_arch()(); - write_sysreg(0, hstr_el2); - write_sysreg(0, pmuserenr_el0); } static void __hyp_text __activate_vm(struct kvm *kvm)
As we are about to be more lazy with some of the trap configuration register read/writes for VHE systems, move the logic that is currently shared between VHE and non-VHE into a separate function which can be called from either the world-switch path or from vcpu_load/vcpu_put. Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org> --- Notes: Changes since v3: - Separate fpsimd32 trap configuration into a separate function which is still called from __activate_traps, because we no longer defer saving/restoring of VFP registers to load/put. arch/arm64/kvm/hyp/switch.c | 76 +++++++++++++++++++++++++++------------------ 1 file changed, 45 insertions(+), 31 deletions(-)