| Message ID | 1507660725-7986-23-git-send-email-Dave.Martin@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | ARM Scalable Vector Extension (SVE) | expand |
[+ Christoffer] On 10/10/17 19:38, Dave Martin wrote: > Until KVM has full SVE support, guests must not be allowed to > execute SVE instructions. > > This patch enables the necessary traps, and also ensures that the > traps are disabled again on exit from the guest so that the host > can still use SVE if it wants to. > > This patch introduces another instance of > __this_cpu_write(fpsimd_last_state, NULL), so this flush operation > is abstracted out as a separate helper fpsimd_flush_cpu_state(). > Other instances are ported appropriately. > > As a side effect of this refactoring, a this_cpu_write() in > fpsimd_cpu_pm_notifier() is changed to __this_cpu_write(). This > should be fine, since cpu_pm_enter() is supposed to be called only > with interrupts disabled. > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > Cc: Marc Zyngier <marc.zyngier@arm.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/arm/include/asm/kvm_host.h | 3 +++ > arch/arm64/include/asm/fpsimd.h | 1 + > arch/arm64/include/asm/kvm_arm.h | 4 +++- > arch/arm64/include/asm/kvm_host.h | 11 +++++++++++ > arch/arm64/kernel/fpsimd.c | 31 +++++++++++++++++++++++++++++-- > arch/arm64/kvm/hyp/switch.c | 6 +++--- > virt/kvm/arm/arm.c | 3 +++ > 7 files changed, 53 insertions(+), 6 deletions(-) > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > index 4a879f6..242151e 100644 > --- a/arch/arm/include/asm/kvm_host.h > +++ b/arch/arm/include/asm/kvm_host.h > @@ -293,4 +293,7 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, > int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, > struct kvm_device_attr *attr); > > +/* All host FP/SIMD state is restored on guest exit, so nothing to save: */ > +static inline void kvm_fpsimd_flush_cpu_state(void) {} > + > #endif /* __ARM_KVM_HOST_H__ */ > diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h > index 3cfdfbe..10b2824 100644 > --- a/arch/arm64/include/asm/fpsimd.h > +++ b/arch/arm64/include/asm/fpsimd.h > @@ -75,6 +75,7 @@ extern void fpsimd_restore_current_state(void); > extern void fpsimd_update_current_state(struct fpsimd_state *state); > > extern void fpsimd_flush_task_state(struct task_struct *target); > +extern void sve_flush_cpu_state(void); > > /* Maximum VL that SVE VL-agnostic software can transparently support */ > #define SVE_VL_ARCH_MAX 0x100 > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h > index dbf0537..7f069ff 100644 > --- a/arch/arm64/include/asm/kvm_arm.h > +++ b/arch/arm64/include/asm/kvm_arm.h > @@ -186,7 +186,8 @@ > #define CPTR_EL2_TTA (1 << 20) > #define CPTR_EL2_TFP (1 << CPTR_EL2_TFP_SHIFT) > #define CPTR_EL2_TZ (1 << 8) > -#define CPTR_EL2_DEFAULT 0x000033ff > +#define CPTR_EL2_RES1 0x000032ff /* known RES1 bits in CPTR_EL2 */ > +#define CPTR_EL2_DEFAULT CPTR_EL2_RES1 > > /* Hyp Debug Configuration Register bits */ > #define MDCR_EL2_TPMS (1 << 14) > @@ -237,5 +238,6 @@ > > #define CPACR_EL1_FPEN (3 << 20) > #define CPACR_EL1_TTA (1 << 28) > +#define CPACR_EL1_DEFAULT (CPACR_EL1_FPEN | CPACR_EL1_ZEN_EL1EN) > > #endif /* __ARM64_KVM_ARM_H__ */ > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index e923b58..674912d 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -25,6 +25,7 @@ > #include <linux/types.h> > #include <linux/kvm_types.h> > #include <asm/cpufeature.h> > +#include <asm/fpsimd.h> > #include <asm/kvm.h> > #include <asm/kvm_asm.h> > #include <asm/kvm_mmio.h> > @@ -384,4 +385,14 @@ static inline void __cpu_init_stage2(void) > "PARange is %d bits, unsupported configuration!", parange); > } > > +/* > + * All host FP/SIMD state is restored on guest exit, so nothing needs > + * doing here except in the SVE case: > +*/ > +static inline void kvm_fpsimd_flush_cpu_state(void) > +{ > + if (system_supports_sve()) > + sve_flush_cpu_state(); Hmmm. How does this work if... > +} > + > #endif /* __ARM64_KVM_HOST_H__ */ > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index a9cb794..6ae3703 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -1073,6 +1073,33 @@ void fpsimd_flush_task_state(struct task_struct *t) > t->thread.fpsimd_state.cpu = NR_CPUS; > } > > +static inline void fpsimd_flush_cpu_state(void) > +{ > + __this_cpu_write(fpsimd_last_state, NULL); > +} > + > +/* > + * Invalidate any task SVE state currently held in this CPU's regs. > + * > + * This is used to prevent the kernel from trying to reuse SVE register data > + * that is detroyed by KVM guest enter/exit. This function should go away when > + * KVM SVE support is implemented. Don't use it for anything else. > + */ > +#ifdef CONFIG_ARM64_SVE > +void sve_flush_cpu_state(void) > +{ > + struct fpsimd_state *const fpstate = __this_cpu_read(fpsimd_last_state); > + struct task_struct *tsk; > + > + if (!fpstate) > + return; > + > + tsk = container_of(fpstate, struct task_struct, thread.fpsimd_state); > + if (test_tsk_thread_flag(tsk, TIF_SVE)) > + fpsimd_flush_cpu_state(); > +} > +#endif /* CONFIG_ARM64_SVE */ ... CONFIG_ARM64_SVE is not set? Fixing this should just be a matter of moving the #ifdef/#endif inside the function... > + > #ifdef CONFIG_KERNEL_MODE_NEON > > DEFINE_PER_CPU(bool, kernel_neon_busy); > @@ -1113,7 +1140,7 @@ void kernel_neon_begin(void) > } > > /* Invalidate any task state remaining in the fpsimd regs: */ > - __this_cpu_write(fpsimd_last_state, NULL); > + fpsimd_flush_cpu_state(); > > preempt_disable(); > > @@ -1234,7 +1261,7 @@ static int fpsimd_cpu_pm_notifier(struct notifier_block *self, > case CPU_PM_ENTER: > if (current->mm) > task_fpsimd_save(); > - this_cpu_write(fpsimd_last_state, NULL); > + fpsimd_flush_cpu_state(); > break; > case CPU_PM_EXIT: > if (current->mm) > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > index 35a90b8..951f3eb 100644 > --- a/arch/arm64/kvm/hyp/switch.c > +++ b/arch/arm64/kvm/hyp/switch.c > @@ -48,7 +48,7 @@ static void __hyp_text __activate_traps_vhe(void) > > val = read_sysreg(cpacr_el1); > val |= CPACR_EL1_TTA; > - val &= ~CPACR_EL1_FPEN; > + val &= ~(CPACR_EL1_FPEN | CPACR_EL1_ZEN); > write_sysreg(val, cpacr_el1); > > write_sysreg(__kvm_hyp_vector, vbar_el1); > @@ -59,7 +59,7 @@ static void __hyp_text __activate_traps_nvhe(void) > u64 val; > > val = CPTR_EL2_DEFAULT; > - val |= CPTR_EL2_TTA | CPTR_EL2_TFP; > + val |= CPTR_EL2_TTA | CPTR_EL2_TFP | CPTR_EL2_TZ; > write_sysreg(val, cptr_el2); > } > > @@ -117,7 +117,7 @@ static void __hyp_text __deactivate_traps_vhe(void) > > write_sysreg(mdcr_el2, mdcr_el2); > write_sysreg(HCR_HOST_VHE_FLAGS, hcr_el2); > - write_sysreg(CPACR_EL1_FPEN, cpacr_el1); > + write_sysreg(CPACR_EL1_DEFAULT, cpacr_el1); > write_sysreg(vectors, vbar_el1); > } > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c > index b9f68e4..4d3cf9c 100644 > --- a/virt/kvm/arm/arm.c > +++ b/virt/kvm/arm/arm.c > @@ -652,6 +652,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) > */ > preempt_disable(); > > + /* Flush FP/SIMD state that can't survive guest entry/exit */ > + kvm_fpsimd_flush_cpu_state(); > + > kvm_pmu_flush_hwstate(vcpu); > > kvm_timer_flush_hwstate(vcpu); > Thanks, M.
On Wed, Oct 11, 2017 at 05:28:06PM +0100, Marc Zyngier wrote: > [+ Christoffer] > > On 10/10/17 19:38, Dave Martin wrote: > > Until KVM has full SVE support, guests must not be allowed to > > execute SVE instructions. > > > > This patch enables the necessary traps, and also ensures that the > > traps are disabled again on exit from the guest so that the host > > can still use SVE if it wants to. > > > > This patch introduces another instance of > > __this_cpu_write(fpsimd_last_state, NULL), so this flush operation > > is abstracted out as a separate helper fpsimd_flush_cpu_state(). > > Other instances are ported appropriately. > > > > As a side effect of this refactoring, a this_cpu_write() in > > fpsimd_cpu_pm_notifier() is changed to __this_cpu_write(). This > > should be fine, since cpu_pm_enter() is supposed to be called only > > with interrupts disabled. > > > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > > Cc: Marc Zyngier <marc.zyngier@arm.com> > > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > > --- [...] > > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > > index e923b58..674912d 100644 > > --- a/arch/arm64/include/asm/kvm_host.h > > +++ b/arch/arm64/include/asm/kvm_host.h [...] > > @@ -384,4 +385,14 @@ static inline void __cpu_init_stage2(void) [...] > > +static inline void kvm_fpsimd_flush_cpu_state(void) > > +{ > > + if (system_supports_sve()) > > + sve_flush_cpu_state(); > > Hmmm. How does this work if... !IS_ENABLED(CONFIG_ARM64_SVE) implies !system_supports_sve(), so if CONFIG_ARM64_SVE is not set, the call is optimised away. [...] > > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > > index a9cb794..6ae3703 100644 > > --- a/arch/arm64/kernel/fpsimd.c > > +++ b/arch/arm64/kernel/fpsimd.c > > @@ -1073,6 +1073,33 @@ void fpsimd_flush_task_state(struct task_struct *t) [...] > > +#ifdef CONFIG_ARM64_SVE > > +void sve_flush_cpu_state(void) > > +{ > > + struct fpsimd_state *const fpstate = __this_cpu_read(fpsimd_last_state); > > + struct task_struct *tsk; > > + > > + if (!fpstate) > > + return; > > + > > + tsk = container_of(fpstate, struct task_struct, thread.fpsimd_state); > > + if (test_tsk_thread_flag(tsk, TIF_SVE)) > > + fpsimd_flush_cpu_state(); > > +} > > +#endif /* CONFIG_ARM64_SVE */ > > ... CONFIG_ARM64_SVE is not set? Fixing this should just be a matter of > moving the #ifdef/#endif inside the function... Because sve_flush_cpu_state() is not in the same compilation unit it can't be static, and that means the compiler won't remove it automatically if it's unused -- hence the #ifdef. Because the call site is optimised away, there is no link failure. Don't we rely on this sort of thing all over the place? Cheers ---Dave
On 12/10/17 12:04, Dave Martin wrote: > On Wed, Oct 11, 2017 at 05:28:06PM +0100, Marc Zyngier wrote: >> [+ Christoffer] >> >> On 10/10/17 19:38, Dave Martin wrote: >>> Until KVM has full SVE support, guests must not be allowed to >>> execute SVE instructions. >>> >>> This patch enables the necessary traps, and also ensures that the >>> traps are disabled again on exit from the guest so that the host >>> can still use SVE if it wants to. >>> >>> This patch introduces another instance of >>> __this_cpu_write(fpsimd_last_state, NULL), so this flush operation >>> is abstracted out as a separate helper fpsimd_flush_cpu_state(). >>> Other instances are ported appropriately. >>> >>> As a side effect of this refactoring, a this_cpu_write() in >>> fpsimd_cpu_pm_notifier() is changed to __this_cpu_write(). This >>> should be fine, since cpu_pm_enter() is supposed to be called only >>> with interrupts disabled. >>> >>> Signed-off-by: Dave Martin <Dave.Martin@arm.com> >>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> >>> Cc: Marc Zyngier <marc.zyngier@arm.com> >>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>> --- > > [...] > >>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >>> index e923b58..674912d 100644 >>> --- a/arch/arm64/include/asm/kvm_host.h >>> +++ b/arch/arm64/include/asm/kvm_host.h > > [...] > >>> @@ -384,4 +385,14 @@ static inline void __cpu_init_stage2(void) > > [...] > >>> +static inline void kvm_fpsimd_flush_cpu_state(void) >>> +{ >>> + if (system_supports_sve()) >>> + sve_flush_cpu_state(); >> >> Hmmm. How does this work if... > > !IS_ENABLED(CONFIG_ARM64_SVE) implies !system_supports_sve(), so > if CONFIG_ARM64_SVE is not set, the call is optimised away. > > [...] > >>> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c >>> index a9cb794..6ae3703 100644 >>> --- a/arch/arm64/kernel/fpsimd.c >>> +++ b/arch/arm64/kernel/fpsimd.c >>> @@ -1073,6 +1073,33 @@ void fpsimd_flush_task_state(struct task_struct *t) > > [...] > >>> +#ifdef CONFIG_ARM64_SVE >>> +void sve_flush_cpu_state(void) >>> +{ >>> + struct fpsimd_state *const fpstate = __this_cpu_read(fpsimd_last_state); >>> + struct task_struct *tsk; >>> + >>> + if (!fpstate) >>> + return; >>> + >>> + tsk = container_of(fpstate, struct task_struct, thread.fpsimd_state); >>> + if (test_tsk_thread_flag(tsk, TIF_SVE)) >>> + fpsimd_flush_cpu_state(); >>> +} >>> +#endif /* CONFIG_ARM64_SVE */ >> >> ... CONFIG_ARM64_SVE is not set? Fixing this should just be a matter of >> moving the #ifdef/#endif inside the function... > > Because sve_flush_cpu_state() is not in the same compilation unit it > can't be static, and that means the compiler won't remove it > automatically if it's unused -- hence the #ifdef. > > Because the call site is optimised away, there is no link failure. > > Don't we rely on this sort of thing all over the place? Dunno. It just feels weird. But if you are sure that it won't break, fine by me. I guess we'll find out pretty quickly how this fares, specially with older toolchains. M.
On Tue, Oct 10, 2017 at 07:38:39PM +0100, Dave P Martin wrote: > Until KVM has full SVE support, guests must not be allowed to > execute SVE instructions. > > This patch enables the necessary traps, and also ensures that the > traps are disabled again on exit from the guest so that the host > can still use SVE if it wants to. > > This patch introduces another instance of > __this_cpu_write(fpsimd_last_state, NULL), so this flush operation > is abstracted out as a separate helper fpsimd_flush_cpu_state(). > Other instances are ported appropriately. > > As a side effect of this refactoring, a this_cpu_write() in > fpsimd_cpu_pm_notifier() is changed to __this_cpu_write(). This > should be fine, since cpu_pm_enter() is supposed to be called only > with interrupts disabled. > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > Cc: Marc Zyngier <marc.zyngier@arm.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
On Thu, Oct 12, 2017 at 12:28:32PM +0100, Marc Zyngier wrote: > On 12/10/17 12:04, Dave Martin wrote: > > On Wed, Oct 11, 2017 at 05:28:06PM +0100, Marc Zyngier wrote: > >> [+ Christoffer] > >> > >> On 10/10/17 19:38, Dave Martin wrote: > >>> Until KVM has full SVE support, guests must not be allowed to > >>> execute SVE instructions. > >>> > >>> This patch enables the necessary traps, and also ensures that the > >>> traps are disabled again on exit from the guest so that the host > >>> can still use SVE if it wants to. > >>> > >>> This patch introduces another instance of > >>> __this_cpu_write(fpsimd_last_state, NULL), so this flush operation > >>> is abstracted out as a separate helper fpsimd_flush_cpu_state(). > >>> Other instances are ported appropriately. > >>> > >>> As a side effect of this refactoring, a this_cpu_write() in > >>> fpsimd_cpu_pm_notifier() is changed to __this_cpu_write(). This > >>> should be fine, since cpu_pm_enter() is supposed to be called only > >>> with interrupts disabled. > >>> > >>> Signed-off-by: Dave Martin <Dave.Martin@arm.com> > >>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > >>> Cc: Marc Zyngier <marc.zyngier@arm.com> > >>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > >>> --- > > > > [...] > > > >>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > >>> index e923b58..674912d 100644 > >>> --- a/arch/arm64/include/asm/kvm_host.h > >>> +++ b/arch/arm64/include/asm/kvm_host.h > > > > [...] > > > >>> @@ -384,4 +385,14 @@ static inline void __cpu_init_stage2(void) > > > > [...] > > > >>> +static inline void kvm_fpsimd_flush_cpu_state(void) > >>> +{ > >>> + if (system_supports_sve()) > >>> + sve_flush_cpu_state(); > >> > >> Hmmm. How does this work if... > > > > !IS_ENABLED(CONFIG_ARM64_SVE) implies !system_supports_sve(), so > > if CONFIG_ARM64_SVE is not set, the call is optimised away. > > > > [...] > > > >>> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > >>> index a9cb794..6ae3703 100644 > >>> --- a/arch/arm64/kernel/fpsimd.c > >>> +++ b/arch/arm64/kernel/fpsimd.c > >>> @@ -1073,6 +1073,33 @@ void fpsimd_flush_task_state(struct task_struct *t) > > > > [...] > > > >>> +#ifdef CONFIG_ARM64_SVE > >>> +void sve_flush_cpu_state(void) > >>> +{ > >>> + struct fpsimd_state *const fpstate = __this_cpu_read(fpsimd_last_state); > >>> + struct task_struct *tsk; > >>> + > >>> + if (!fpstate) > >>> + return; > >>> + > >>> + tsk = container_of(fpstate, struct task_struct, thread.fpsimd_state); > >>> + if (test_tsk_thread_flag(tsk, TIF_SVE)) > >>> + fpsimd_flush_cpu_state(); > >>> +} > >>> +#endif /* CONFIG_ARM64_SVE */ > >> > >> ... CONFIG_ARM64_SVE is not set? Fixing this should just be a matter of > >> moving the #ifdef/#endif inside the function... > > > > Because sve_flush_cpu_state() is not in the same compilation unit it > > can't be static, and that means the compiler won't remove it > > automatically if it's unused -- hence the #ifdef. > > > > Because the call site is optimised away, there is no link failure. > > > > Don't we rely on this sort of thing all over the place? > Dunno. It just feels weird. But if you are sure that it won't break, > fine by me. I guess we'll find out pretty quickly how this fares, > specially with older toolchains. I thought this was why the kernel doesn't support building with -O0. There are many instances of this in the series, not just here. Let me know if you feel this isn't good enough though. Do you have any other comments on this patch? Cheers ---Dave
On 13/10/17 15:15, Dave Martin wrote: > On Thu, Oct 12, 2017 at 12:28:32PM +0100, Marc Zyngier wrote: >> On 12/10/17 12:04, Dave Martin wrote: >>> On Wed, Oct 11, 2017 at 05:28:06PM +0100, Marc Zyngier wrote: >>>> [+ Christoffer] >>>> >>>> On 10/10/17 19:38, Dave Martin wrote: >>>>> Until KVM has full SVE support, guests must not be allowed to >>>>> execute SVE instructions. >>>>> >>>>> This patch enables the necessary traps, and also ensures that the >>>>> traps are disabled again on exit from the guest so that the host >>>>> can still use SVE if it wants to. >>>>> >>>>> This patch introduces another instance of >>>>> __this_cpu_write(fpsimd_last_state, NULL), so this flush operation >>>>> is abstracted out as a separate helper fpsimd_flush_cpu_state(). >>>>> Other instances are ported appropriately. >>>>> >>>>> As a side effect of this refactoring, a this_cpu_write() in >>>>> fpsimd_cpu_pm_notifier() is changed to __this_cpu_write(). This >>>>> should be fine, since cpu_pm_enter() is supposed to be called only >>>>> with interrupts disabled. >>>>> >>>>> Signed-off-by: Dave Martin <Dave.Martin@arm.com> >>>>> Reviewed-by: Alex Bennée <alex.bennee@linaro.org> >>>>> Cc: Marc Zyngier <marc.zyngier@arm.com> >>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >>>>> --- >>> >>> [...] >>> >>>>> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h >>>>> index e923b58..674912d 100644 >>>>> --- a/arch/arm64/include/asm/kvm_host.h >>>>> +++ b/arch/arm64/include/asm/kvm_host.h >>> >>> [...] >>> >>>>> @@ -384,4 +385,14 @@ static inline void __cpu_init_stage2(void) >>> >>> [...] >>> >>>>> +static inline void kvm_fpsimd_flush_cpu_state(void) >>>>> +{ >>>>> + if (system_supports_sve()) >>>>> + sve_flush_cpu_state(); >>>> >>>> Hmmm. How does this work if... >>> >>> !IS_ENABLED(CONFIG_ARM64_SVE) implies !system_supports_sve(), so >>> if CONFIG_ARM64_SVE is not set, the call is optimised away. >>> >>> [...] >>> >>>>> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c >>>>> index a9cb794..6ae3703 100644 >>>>> --- a/arch/arm64/kernel/fpsimd.c >>>>> +++ b/arch/arm64/kernel/fpsimd.c >>>>> @@ -1073,6 +1073,33 @@ void fpsimd_flush_task_state(struct task_struct *t) >>> >>> [...] >>> >>>>> +#ifdef CONFIG_ARM64_SVE >>>>> +void sve_flush_cpu_state(void) >>>>> +{ >>>>> + struct fpsimd_state *const fpstate = __this_cpu_read(fpsimd_last_state); >>>>> + struct task_struct *tsk; >>>>> + >>>>> + if (!fpstate) >>>>> + return; >>>>> + >>>>> + tsk = container_of(fpstate, struct task_struct, thread.fpsimd_state); >>>>> + if (test_tsk_thread_flag(tsk, TIF_SVE)) >>>>> + fpsimd_flush_cpu_state(); >>>>> +} >>>>> +#endif /* CONFIG_ARM64_SVE */ >>>> >>>> ... CONFIG_ARM64_SVE is not set? Fixing this should just be a matter of >>>> moving the #ifdef/#endif inside the function... >>> >>> Because sve_flush_cpu_state() is not in the same compilation unit it >>> can't be static, and that means the compiler won't remove it >>> automatically if it's unused -- hence the #ifdef. >>> >>> Because the call site is optimised away, there is no link failure. >>> >>> Don't we rely on this sort of thing all over the place? >> Dunno. It just feels weird. But if you are sure that it won't break, >> fine by me. I guess we'll find out pretty quickly how this fares, >> specially with older toolchains. > > I thought this was why the kernel doesn't support building with -O0. > There are many instances of this in the series, not just here. > > Let me know if you feel this isn't good enough though. That's OK to me. As I said, we'll find out pretty quickly if anything breaks unexpectedly. > Do you have any other comments on this patch? None. You can add my: Acked-by: Marc Zyngier <marc.zyngier@arm.com> M.
On Fri, Oct 13, 2017 at 03:21:59PM +0100, Marc Zyngier wrote: > On 13/10/17 15:15, Dave Martin wrote: > > On Thu, Oct 12, 2017 at 12:28:32PM +0100, Marc Zyngier wrote: > >> On 12/10/17 12:04, Dave Martin wrote: > >>> On Wed, Oct 11, 2017 at 05:28:06PM +0100, Marc Zyngier wrote: > >>>> [+ Christoffer] > >>>> > >>>> On 10/10/17 19:38, Dave Martin wrote: [...] > >>>> Hmmm. How does this work if... > >>> > >>> !IS_ENABLED(CONFIG_ARM64_SVE) implies !system_supports_sve(), so > >>> if CONFIG_ARM64_SVE is not set, the call is optimised away. > >>> > >>> [...] > >>> > >>>>> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > >>>>> index a9cb794..6ae3703 100644 > >>>>> --- a/arch/arm64/kernel/fpsimd.c > >>>>> +++ b/arch/arm64/kernel/fpsimd.c > >>>>> @@ -1073,6 +1073,33 @@ void fpsimd_flush_task_state(struct task_struct *t) > >>> > >>> [...] > >>> > >>>>> +#ifdef CONFIG_ARM64_SVE > >>>>> +void sve_flush_cpu_state(void) > >>>>> +{ > >>>>> + struct fpsimd_state *const fpstate = __this_cpu_read(fpsimd_last_state); > >>>>> + struct task_struct *tsk; > >>>>> + > >>>>> + if (!fpstate) > >>>>> + return; > >>>>> + > >>>>> + tsk = container_of(fpstate, struct task_struct, thread.fpsimd_state); > >>>>> + if (test_tsk_thread_flag(tsk, TIF_SVE)) > >>>>> + fpsimd_flush_cpu_state(); > >>>>> +} > >>>>> +#endif /* CONFIG_ARM64_SVE */ > >>>> > >>>> ... CONFIG_ARM64_SVE is not set? Fixing this should just be a matter of > >>>> moving the #ifdef/#endif inside the function... > >>> > >>> Because sve_flush_cpu_state() is not in the same compilation unit it > >>> can't be static, and that means the compiler won't remove it > >>> automatically if it's unused -- hence the #ifdef. > >>> > >>> Because the call site is optimised away, there is no link failure. > >>> > >>> Don't we rely on this sort of thing all over the place? > >> Dunno. It just feels weird. But if you are sure that it won't break, > >> fine by me. I guess we'll find out pretty quickly how this fares, > >> specially with older toolchains. > > > > I thought this was why the kernel doesn't support building with -O0. > > There are many instances of this in the series, not just here. > > > > Let me know if you feel this isn't good enough though. > > That's OK to me. As I said, we'll find out pretty quickly if anything > breaks unexpectedly. Yup > > > Do you have any other comments on this patch? > None. You can add my: > > Acked-by: Marc Zyngier <marc.zyngier@arm.com> OK, thanks for the input. Cheers ---Dave
On Tue, Oct 10, 2017 at 07:38:39PM +0100, Dave Martin wrote: > Until KVM has full SVE support, guests must not be allowed to > execute SVE instructions. > > This patch enables the necessary traps, and also ensures that the > traps are disabled again on exit from the guest so that the host > can still use SVE if it wants to. > > This patch introduces another instance of > __this_cpu_write(fpsimd_last_state, NULL), so this flush operation > is abstracted out as a separate helper fpsimd_flush_cpu_state(). > Other instances are ported appropriately. I don't understand this paragraph, beginning from ", so this...". >From reading the code, what I think is the reason for having to flush the SVE state (and mark the host state invalid) is that even though we disallow SVE usage in the guest, the guest can use the normal FP state, and while we always fully preserve the host state, this could still corrupt some additional SVE state not properly preserved for the host. Is that correct? > > As a side effect of this refactoring, a this_cpu_write() in > fpsimd_cpu_pm_notifier() is changed to __this_cpu_write(). This > should be fine, since cpu_pm_enter() is supposed to be called only > with interrupts disabled. Otherwise the patch itself looks good to me. Thanks, -Christoffer > > Signed-off-by: Dave Martin <Dave.Martin@arm.com> > Reviewed-by: Alex Bennée <alex.bennee@linaro.org> > Cc: Marc Zyngier <marc.zyngier@arm.com> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/arm/include/asm/kvm_host.h | 3 +++ > arch/arm64/include/asm/fpsimd.h | 1 + > arch/arm64/include/asm/kvm_arm.h | 4 +++- > arch/arm64/include/asm/kvm_host.h | 11 +++++++++++ > arch/arm64/kernel/fpsimd.c | 31 +++++++++++++++++++++++++++++-- > arch/arm64/kvm/hyp/switch.c | 6 +++--- > virt/kvm/arm/arm.c | 3 +++ > 7 files changed, 53 insertions(+), 6 deletions(-) > > diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h > index 4a879f6..242151e 100644 > --- a/arch/arm/include/asm/kvm_host.h > +++ b/arch/arm/include/asm/kvm_host.h > @@ -293,4 +293,7 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, > int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, > struct kvm_device_attr *attr); > > +/* All host FP/SIMD state is restored on guest exit, so nothing to save: */ > +static inline void kvm_fpsimd_flush_cpu_state(void) {} > + > #endif /* __ARM_KVM_HOST_H__ */ > diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h > index 3cfdfbe..10b2824 100644 > --- a/arch/arm64/include/asm/fpsimd.h > +++ b/arch/arm64/include/asm/fpsimd.h > @@ -75,6 +75,7 @@ extern void fpsimd_restore_current_state(void); > extern void fpsimd_update_current_state(struct fpsimd_state *state); > > extern void fpsimd_flush_task_state(struct task_struct *target); > +extern void sve_flush_cpu_state(void); > > /* Maximum VL that SVE VL-agnostic software can transparently support */ > #define SVE_VL_ARCH_MAX 0x100 > diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h > index dbf0537..7f069ff 100644 > --- a/arch/arm64/include/asm/kvm_arm.h > +++ b/arch/arm64/include/asm/kvm_arm.h > @@ -186,7 +186,8 @@ > #define CPTR_EL2_TTA (1 << 20) > #define CPTR_EL2_TFP (1 << CPTR_EL2_TFP_SHIFT) > #define CPTR_EL2_TZ (1 << 8) > -#define CPTR_EL2_DEFAULT 0x000033ff > +#define CPTR_EL2_RES1 0x000032ff /* known RES1 bits in CPTR_EL2 */ > +#define CPTR_EL2_DEFAULT CPTR_EL2_RES1 > > /* Hyp Debug Configuration Register bits */ > #define MDCR_EL2_TPMS (1 << 14) > @@ -237,5 +238,6 @@ > > #define CPACR_EL1_FPEN (3 << 20) > #define CPACR_EL1_TTA (1 << 28) > +#define CPACR_EL1_DEFAULT (CPACR_EL1_FPEN | CPACR_EL1_ZEN_EL1EN) > > #endif /* __ARM64_KVM_ARM_H__ */ > diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h > index e923b58..674912d 100644 > --- a/arch/arm64/include/asm/kvm_host.h > +++ b/arch/arm64/include/asm/kvm_host.h > @@ -25,6 +25,7 @@ > #include <linux/types.h> > #include <linux/kvm_types.h> > #include <asm/cpufeature.h> > +#include <asm/fpsimd.h> > #include <asm/kvm.h> > #include <asm/kvm_asm.h> > #include <asm/kvm_mmio.h> > @@ -384,4 +385,14 @@ static inline void __cpu_init_stage2(void) > "PARange is %d bits, unsupported configuration!", parange); > } > > +/* > + * All host FP/SIMD state is restored on guest exit, so nothing needs > + * doing here except in the SVE case: > +*/ > +static inline void kvm_fpsimd_flush_cpu_state(void) > +{ > + if (system_supports_sve()) > + sve_flush_cpu_state(); > +} > + > #endif /* __ARM64_KVM_HOST_H__ */ > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index a9cb794..6ae3703 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -1073,6 +1073,33 @@ void fpsimd_flush_task_state(struct task_struct *t) > t->thread.fpsimd_state.cpu = NR_CPUS; > } > > +static inline void fpsimd_flush_cpu_state(void) > +{ > + __this_cpu_write(fpsimd_last_state, NULL); > +} > + > +/* > + * Invalidate any task SVE state currently held in this CPU's regs. > + * > + * This is used to prevent the kernel from trying to reuse SVE register data > + * that is detroyed by KVM guest enter/exit. This function should go away when > + * KVM SVE support is implemented. Don't use it for anything else. > + */ > +#ifdef CONFIG_ARM64_SVE > +void sve_flush_cpu_state(void) > +{ > + struct fpsimd_state *const fpstate = __this_cpu_read(fpsimd_last_state); > + struct task_struct *tsk; > + > + if (!fpstate) > + return; > + > + tsk = container_of(fpstate, struct task_struct, thread.fpsimd_state); > + if (test_tsk_thread_flag(tsk, TIF_SVE)) > + fpsimd_flush_cpu_state(); > +} > +#endif /* CONFIG_ARM64_SVE */ > + > #ifdef CONFIG_KERNEL_MODE_NEON > > DEFINE_PER_CPU(bool, kernel_neon_busy); > @@ -1113,7 +1140,7 @@ void kernel_neon_begin(void) > } > > /* Invalidate any task state remaining in the fpsimd regs: */ > - __this_cpu_write(fpsimd_last_state, NULL); > + fpsimd_flush_cpu_state(); > > preempt_disable(); > > @@ -1234,7 +1261,7 @@ static int fpsimd_cpu_pm_notifier(struct notifier_block *self, > case CPU_PM_ENTER: > if (current->mm) > task_fpsimd_save(); > - this_cpu_write(fpsimd_last_state, NULL); > + fpsimd_flush_cpu_state(); > break; > case CPU_PM_EXIT: > if (current->mm) > diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c > index 35a90b8..951f3eb 100644 > --- a/arch/arm64/kvm/hyp/switch.c > +++ b/arch/arm64/kvm/hyp/switch.c > @@ -48,7 +48,7 @@ static void __hyp_text __activate_traps_vhe(void) > > val = read_sysreg(cpacr_el1); > val |= CPACR_EL1_TTA; > - val &= ~CPACR_EL1_FPEN; > + val &= ~(CPACR_EL1_FPEN | CPACR_EL1_ZEN); > write_sysreg(val, cpacr_el1); > > write_sysreg(__kvm_hyp_vector, vbar_el1); > @@ -59,7 +59,7 @@ static void __hyp_text __activate_traps_nvhe(void) > u64 val; > > val = CPTR_EL2_DEFAULT; > - val |= CPTR_EL2_TTA | CPTR_EL2_TFP; > + val |= CPTR_EL2_TTA | CPTR_EL2_TFP | CPTR_EL2_TZ; > write_sysreg(val, cptr_el2); > } > > @@ -117,7 +117,7 @@ static void __hyp_text __deactivate_traps_vhe(void) > > write_sysreg(mdcr_el2, mdcr_el2); > write_sysreg(HCR_HOST_VHE_FLAGS, hcr_el2); > - write_sysreg(CPACR_EL1_FPEN, cpacr_el1); > + write_sysreg(CPACR_EL1_DEFAULT, cpacr_el1); > write_sysreg(vectors, vbar_el1); > } > > diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c > index b9f68e4..4d3cf9c 100644 > --- a/virt/kvm/arm/arm.c > +++ b/virt/kvm/arm/arm.c > @@ -652,6 +652,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) > */ > preempt_disable(); > > + /* Flush FP/SIMD state that can't survive guest entry/exit */ > + kvm_fpsimd_flush_cpu_state(); > + > kvm_pmu_flush_hwstate(vcpu); > > kvm_timer_flush_hwstate(vcpu); > -- > 2.1.4 > > _______________________________________________ > kvmarm mailing list > kvmarm@lists.cs.columbia.edu > https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
On Tue, Oct 17, 2017 at 01:50:24PM +0200, Christoffer Dall wrote: > On Tue, Oct 10, 2017 at 07:38:39PM +0100, Dave Martin wrote: > > Until KVM has full SVE support, guests must not be allowed to > > execute SVE instructions. > > > > This patch enables the necessary traps, and also ensures that the > > traps are disabled again on exit from the guest so that the host > > can still use SVE if it wants to. > > > > This patch introduces another instance of > > __this_cpu_write(fpsimd_last_state, NULL), so this flush operation > > is abstracted out as a separate helper fpsimd_flush_cpu_state(). > > Other instances are ported appropriately. > > I don't understand this paragraph, beginning from ", so this...". > > > From reading the code, what I think is the reason for having to flush > the SVE state (and mark the host state invalid) is that even though we > disallow SVE usage in the guest, the guest can use the normal FP state, > and while we always fully preserve the host state, this could still > corrupt some additional SVE state not properly preserved for the host. > Is that correct? Yes, that's right: the guest can't touch the SVE-specific registers Pn/FFR, but FPSIMD accesses to Vn regs cause the high bits of the corresponding SVE Zn registers to be clobbered. In any case, the FPSIMD restore done by KVM after guest exit is sufficient to clobber those bits even if the guest didn't do it. This is a band-aid for not making the KVM world switch code properly SVE-aware yet. Does the following wording sound better: --8<-- On guest exit, high bits of the SVE Zn registers may have been clobbered as a side-effect the execution of FPSIMD instructions in the guest. The existing KVM host FPSIMD restore code is not sufficient to restore these bits, so this patch explicitly marks the CPU as not containing cached vector state for any task, this forcing a reload on the next return to userspace. This is an interim measure, in advance of adding full SVE awareness to KVM. Because of the duplication of this operation (__this_cpu_write(fpsimd_last_state, NULL)), it is factored out as a new helper fpsimd_flush_cpu_state() to make the purpose clearer. -->8-- > > > > As a side effect of this refactoring, a this_cpu_write() in > > fpsimd_cpu_pm_notifier() is changed to __this_cpu_write(). This > > should be fine, since cpu_pm_enter() is supposed to be called only > > with interrupts disabled. > > Otherwise the patch itself looks good to me. Thanks, let me know about the above wording change though. ---Dave
On Tue, Oct 17, 2017 at 03:31:42PM +0100, Dave Martin wrote: > On Tue, Oct 17, 2017 at 01:50:24PM +0200, Christoffer Dall wrote: > > On Tue, Oct 10, 2017 at 07:38:39PM +0100, Dave Martin wrote: > > > Until KVM has full SVE support, guests must not be allowed to > > > execute SVE instructions. > > > > > > This patch enables the necessary traps, and also ensures that the > > > traps are disabled again on exit from the guest so that the host > > > can still use SVE if it wants to. > > > > > > This patch introduces another instance of > > > __this_cpu_write(fpsimd_last_state, NULL), so this flush operation > > > is abstracted out as a separate helper fpsimd_flush_cpu_state(). > > > Other instances are ported appropriately. > > > > I don't understand this paragraph, beginning from ", so this...". > > > > > > From reading the code, what I think is the reason for having to flush > > the SVE state (and mark the host state invalid) is that even though we > > disallow SVE usage in the guest, the guest can use the normal FP state, > > and while we always fully preserve the host state, this could still > > corrupt some additional SVE state not properly preserved for the host. > > Is that correct? > > Yes, that's right: the guest can't touch the SVE-specific registers > Pn/FFR, but FPSIMD accesses to Vn regs cause the high bits of the > corresponding SVE Zn registers to be clobbered. In any case, the > FPSIMD restore done by KVM after guest exit is sufficient to clobber > those bits even if the guest didn't do it. > > This is a band-aid for not making the KVM world switch code properly > SVE-aware yet. > > Does the following wording sound better: > > --8<-- > > On guest exit, high bits of the SVE Zn registers may have been > clobbered as a side-effect the execution of FPSIMD instructions in > the guest. The existing KVM host FPSIMD restore code is not > sufficient to restore these bits, so this patch explicitly marks > the CPU as not containing cached vector state for any task, this > forcing a reload on the next return to userspace. This is an > interim measure, in advance of adding full SVE awareness to KVM. > > Because of the duplication of this operation > (__this_cpu_write(fpsimd_last_state, NULL)), it is factored out as s/it is/is/ (I think) > a new helper fpsimd_flush_cpu_state() to make the purpose clearer. > > -->8-- > > > > > > > As a side effect of this refactoring, a this_cpu_write() in > > > fpsimd_cpu_pm_notifier() is changed to __this_cpu_write(). This > > > should be fine, since cpu_pm_enter() is supposed to be called only > > > with interrupts disabled. > > > > Otherwise the patch itself looks good to me. > > Thanks, let me know about the above wording change though. > Yes, the wording is good and helps a lot. Thanks for writing that. Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
On Wed, Oct 18, 2017 at 03:23:23PM +0200, Christoffer Dall wrote: > On Tue, Oct 17, 2017 at 03:31:42PM +0100, Dave Martin wrote: > > On Tue, Oct 17, 2017 at 01:50:24PM +0200, Christoffer Dall wrote: > > > On Tue, Oct 10, 2017 at 07:38:39PM +0100, Dave Martin wrote: > > > > Until KVM has full SVE support, guests must not be allowed to > > > > execute SVE instructions. > > > > > > > > This patch enables the necessary traps, and also ensures that the > > > > traps are disabled again on exit from the guest so that the host > > > > can still use SVE if it wants to. > > > > > > > > This patch introduces another instance of > > > > __this_cpu_write(fpsimd_last_state, NULL), so this flush operation > > > > is abstracted out as a separate helper fpsimd_flush_cpu_state(). > > > > Other instances are ported appropriately. > > > > > > I don't understand this paragraph, beginning from ", so this...". > > > > > > > > > From reading the code, what I think is the reason for having to flush > > > the SVE state (and mark the host state invalid) is that even though we > > > disallow SVE usage in the guest, the guest can use the normal FP state, > > > and while we always fully preserve the host state, this could still > > > corrupt some additional SVE state not properly preserved for the host. > > > Is that correct? > > > > Yes, that's right: the guest can't touch the SVE-specific registers > > Pn/FFR, but FPSIMD accesses to Vn regs cause the high bits of the > > corresponding SVE Zn registers to be clobbered. In any case, the > > FPSIMD restore done by KVM after guest exit is sufficient to clobber > > those bits even if the guest didn't do it. > > > > This is a band-aid for not making the KVM world switch code properly > > SVE-aware yet. > > > > Does the following wording sound better: > > > > --8<-- > > > > On guest exit, high bits of the SVE Zn registers may have been > > clobbered as a side-effect the execution of FPSIMD instructions in > > the guest. The existing KVM host FPSIMD restore code is not > > sufficient to restore these bits, so this patch explicitly marks > > the CPU as not containing cached vector state for any task, this > > forcing a reload on the next return to userspace. This is an > > interim measure, in advance of adding full SVE awareness to KVM. > > > > Because of the duplication of this operation > > (__this_cpu_write(fpsimd_last_state, NULL)), it is factored out as > > s/it is/is/ (I think) > > > a new helper fpsimd_flush_cpu_state() to make the purpose clearer. > > Yes, the wording is good and helps a lot. Thanks for writing that. > I think it "it is" is correct, but it's a pretty ghastly sentence... I'll split it as: This marking of cached vector state in the CPU as invalid is done using __this_cpu_write(fpsimd_last_state, NULL) in fpsimd.c. Due to the repeated use of this rather obscure operation, it makes sense to factor it out as a separate helper with a clearer name. This patch factors it out as fpsimd_flush_cpu_state(). > Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> I'll assume I can keep keep your Reviewed-by, since this change is just clarification. But if you're not happy, please shout! Cheers ---Dave
On Wed, Oct 18, 2017 at 04:00:05PM +0100, Dave Martin wrote: > On Wed, Oct 18, 2017 at 03:23:23PM +0200, Christoffer Dall wrote: > > On Tue, Oct 17, 2017 at 03:31:42PM +0100, Dave Martin wrote: > > > On Tue, Oct 17, 2017 at 01:50:24PM +0200, Christoffer Dall wrote: > > > > On Tue, Oct 10, 2017 at 07:38:39PM +0100, Dave Martin wrote: > > > > > Until KVM has full SVE support, guests must not be allowed to > > > > > execute SVE instructions. > > > > > > > > > > This patch enables the necessary traps, and also ensures that the > > > > > traps are disabled again on exit from the guest so that the host > > > > > can still use SVE if it wants to. > > > > > > > > > > This patch introduces another instance of > > > > > __this_cpu_write(fpsimd_last_state, NULL), so this flush operation > > > > > is abstracted out as a separate helper fpsimd_flush_cpu_state(). > > > > > Other instances are ported appropriately. > > > > > > > > I don't understand this paragraph, beginning from ", so this...". > > > > > > > > > > > > From reading the code, what I think is the reason for having to flush > > > > the SVE state (and mark the host state invalid) is that even though we > > > > disallow SVE usage in the guest, the guest can use the normal FP state, > > > > and while we always fully preserve the host state, this could still > > > > corrupt some additional SVE state not properly preserved for the host. > > > > Is that correct? > > > > > > Yes, that's right: the guest can't touch the SVE-specific registers > > > Pn/FFR, but FPSIMD accesses to Vn regs cause the high bits of the > > > corresponding SVE Zn registers to be clobbered. In any case, the > > > FPSIMD restore done by KVM after guest exit is sufficient to clobber > > > those bits even if the guest didn't do it. > > > > > > This is a band-aid for not making the KVM world switch code properly > > > SVE-aware yet. > > > > > > Does the following wording sound better: > > > > > > --8<-- > > > > > > On guest exit, high bits of the SVE Zn registers may have been > > > clobbered as a side-effect the execution of FPSIMD instructions in > > > the guest. The existing KVM host FPSIMD restore code is not > > > sufficient to restore these bits, so this patch explicitly marks > > > the CPU as not containing cached vector state for any task, this > > > forcing a reload on the next return to userspace. This is an > > > interim measure, in advance of adding full SVE awareness to KVM. > > > > > > Because of the duplication of this operation > > > (__this_cpu_write(fpsimd_last_state, NULL)), it is factored out as > > > > s/it is/is/ (I think) > > > > > a new helper fpsimd_flush_cpu_state() to make the purpose clearer. > > > > Yes, the wording is good and helps a lot. Thanks for writing that. > > > > I think it "it is" is correct, but it's a pretty ghastly sentence... Ah, I missed the comma, before and read it as __this_cpu_write... is factored... but that doesn't make any sense. Sorry, I was just not paying proper attention. > > I'll split it as: > > This marking of cached vector state in the CPU as invalid is done using > __this_cpu_write(fpsimd_last_state, NULL) in fpsimd.c. Due to the > repeated use of this rather obscure operation, it makes sense to factor > it out as a separate helper with a clearer name. This patch factors it > out as fpsimd_flush_cpu_state(). > That's definitely clear. > > Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org> > > I'll assume I can keep keep your Reviewed-by, since this change is just > clarification. > > But if you're not happy, please shout! > I'm happy - in most aspects of life - indeed keep my reviewed-by. Thanks, -Christoffer
diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h index 4a879f6..242151e 100644 --- a/arch/arm/include/asm/kvm_host.h +++ b/arch/arm/include/asm/kvm_host.h @@ -293,4 +293,7 @@ int kvm_arm_vcpu_arch_get_attr(struct kvm_vcpu *vcpu, int kvm_arm_vcpu_arch_has_attr(struct kvm_vcpu *vcpu, struct kvm_device_attr *attr); +/* All host FP/SIMD state is restored on guest exit, so nothing to save: */ +static inline void kvm_fpsimd_flush_cpu_state(void) {} + #endif /* __ARM_KVM_HOST_H__ */ diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h index 3cfdfbe..10b2824 100644 --- a/arch/arm64/include/asm/fpsimd.h +++ b/arch/arm64/include/asm/fpsimd.h @@ -75,6 +75,7 @@ extern void fpsimd_restore_current_state(void); extern void fpsimd_update_current_state(struct fpsimd_state *state); extern void fpsimd_flush_task_state(struct task_struct *target); +extern void sve_flush_cpu_state(void); /* Maximum VL that SVE VL-agnostic software can transparently support */ #define SVE_VL_ARCH_MAX 0x100 diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h index dbf0537..7f069ff 100644 --- a/arch/arm64/include/asm/kvm_arm.h +++ b/arch/arm64/include/asm/kvm_arm.h @@ -186,7 +186,8 @@ #define CPTR_EL2_TTA (1 << 20) #define CPTR_EL2_TFP (1 << CPTR_EL2_TFP_SHIFT) #define CPTR_EL2_TZ (1 << 8) -#define CPTR_EL2_DEFAULT 0x000033ff +#define CPTR_EL2_RES1 0x000032ff /* known RES1 bits in CPTR_EL2 */ +#define CPTR_EL2_DEFAULT CPTR_EL2_RES1 /* Hyp Debug Configuration Register bits */ #define MDCR_EL2_TPMS (1 << 14) @@ -237,5 +238,6 @@ #define CPACR_EL1_FPEN (3 << 20) #define CPACR_EL1_TTA (1 << 28) +#define CPACR_EL1_DEFAULT (CPACR_EL1_FPEN | CPACR_EL1_ZEN_EL1EN) #endif /* __ARM64_KVM_ARM_H__ */ diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h index e923b58..674912d 100644 --- a/arch/arm64/include/asm/kvm_host.h +++ b/arch/arm64/include/asm/kvm_host.h @@ -25,6 +25,7 @@ #include <linux/types.h> #include <linux/kvm_types.h> #include <asm/cpufeature.h> +#include <asm/fpsimd.h> #include <asm/kvm.h> #include <asm/kvm_asm.h> #include <asm/kvm_mmio.h> @@ -384,4 +385,14 @@ static inline void __cpu_init_stage2(void) "PARange is %d bits, unsupported configuration!", parange); } +/* + * All host FP/SIMD state is restored on guest exit, so nothing needs + * doing here except in the SVE case: +*/ +static inline void kvm_fpsimd_flush_cpu_state(void) +{ + if (system_supports_sve()) + sve_flush_cpu_state(); +} + #endif /* __ARM64_KVM_HOST_H__ */ diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index a9cb794..6ae3703 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -1073,6 +1073,33 @@ void fpsimd_flush_task_state(struct task_struct *t) t->thread.fpsimd_state.cpu = NR_CPUS; } +static inline void fpsimd_flush_cpu_state(void) +{ + __this_cpu_write(fpsimd_last_state, NULL); +} + +/* + * Invalidate any task SVE state currently held in this CPU's regs. + * + * This is used to prevent the kernel from trying to reuse SVE register data + * that is detroyed by KVM guest enter/exit. This function should go away when + * KVM SVE support is implemented. Don't use it for anything else. + */ +#ifdef CONFIG_ARM64_SVE +void sve_flush_cpu_state(void) +{ + struct fpsimd_state *const fpstate = __this_cpu_read(fpsimd_last_state); + struct task_struct *tsk; + + if (!fpstate) + return; + + tsk = container_of(fpstate, struct task_struct, thread.fpsimd_state); + if (test_tsk_thread_flag(tsk, TIF_SVE)) + fpsimd_flush_cpu_state(); +} +#endif /* CONFIG_ARM64_SVE */ + #ifdef CONFIG_KERNEL_MODE_NEON DEFINE_PER_CPU(bool, kernel_neon_busy); @@ -1113,7 +1140,7 @@ void kernel_neon_begin(void) } /* Invalidate any task state remaining in the fpsimd regs: */ - __this_cpu_write(fpsimd_last_state, NULL); + fpsimd_flush_cpu_state(); preempt_disable(); @@ -1234,7 +1261,7 @@ static int fpsimd_cpu_pm_notifier(struct notifier_block *self, case CPU_PM_ENTER: if (current->mm) task_fpsimd_save(); - this_cpu_write(fpsimd_last_state, NULL); + fpsimd_flush_cpu_state(); break; case CPU_PM_EXIT: if (current->mm) diff --git a/arch/arm64/kvm/hyp/switch.c b/arch/arm64/kvm/hyp/switch.c index 35a90b8..951f3eb 100644 --- a/arch/arm64/kvm/hyp/switch.c +++ b/arch/arm64/kvm/hyp/switch.c @@ -48,7 +48,7 @@ static void __hyp_text __activate_traps_vhe(void) val = read_sysreg(cpacr_el1); val |= CPACR_EL1_TTA; - val &= ~CPACR_EL1_FPEN; + val &= ~(CPACR_EL1_FPEN | CPACR_EL1_ZEN); write_sysreg(val, cpacr_el1); write_sysreg(__kvm_hyp_vector, vbar_el1); @@ -59,7 +59,7 @@ static void __hyp_text __activate_traps_nvhe(void) u64 val; val = CPTR_EL2_DEFAULT; - val |= CPTR_EL2_TTA | CPTR_EL2_TFP; + val |= CPTR_EL2_TTA | CPTR_EL2_TFP | CPTR_EL2_TZ; write_sysreg(val, cptr_el2); } @@ -117,7 +117,7 @@ static void __hyp_text __deactivate_traps_vhe(void) write_sysreg(mdcr_el2, mdcr_el2); write_sysreg(HCR_HOST_VHE_FLAGS, hcr_el2); - write_sysreg(CPACR_EL1_FPEN, cpacr_el1); + write_sysreg(CPACR_EL1_DEFAULT, cpacr_el1); write_sysreg(vectors, vbar_el1); } diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c index b9f68e4..4d3cf9c 100644 --- a/virt/kvm/arm/arm.c +++ b/virt/kvm/arm/arm.c @@ -652,6 +652,9 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run) */ preempt_disable(); + /* Flush FP/SIMD state that can't survive guest entry/exit */ + kvm_fpsimd_flush_cpu_state(); + kvm_pmu_flush_hwstate(vcpu); kvm_timer_flush_hwstate(vcpu);