| Message ID | 20190412171420.29065-3-julien.grall@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64/fpsimd: Don't disable softirq when touching FPSIMD/SVE state | expand |
On Fri, Apr 12, 2019 at 06:14:19PM +0100, Julien Grall wrote: > The only external user of fpsimd_save() and fpsimd_flush_cpu_state() is > the KVM FPSIMD code. > > A follow-up patch will mandate hold of the FPSIMD context while This is a bit hard to read without understanding the following patch. Maybe elaborate a bit with something like "The following patch will introduce a mechanism to acquire ownership of the FPSIMD/SVE context for performing context management operations. Rather than having to export this [...]" > modifying it. Rather than having to export the new helpers to get/put > the context, we can just introduce a new function to combine > fpsimd_save() and fpsimd_flush_cpu_state(). > > This has also the advantage to remove any external call of fpsimd_save() > and fpsimd_flush_cpu_state(), so they can be turned static. > > Lastly, the new function can also be used in the PM notifier. > > Signed-off-by: Julien Grall <julien.grall@arm.com> > > --- > Changes in v2: > - Patch added > --- > arch/arm64/include/asm/fpsimd.h | 5 ++--- > arch/arm64/kernel/fpsimd.c | 17 +++++++++++++---- > arch/arm64/kvm/fpsimd.c | 3 +-- > 3 files changed, 16 insertions(+), 9 deletions(-) > > diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h > index 32abbfd852e8..88c223f6ded6 100644 > --- a/arch/arm64/include/asm/fpsimd.h > +++ b/arch/arm64/include/asm/fpsimd.h > @@ -48,11 +48,11 @@ struct task_struct; > extern void fpsimd_save_state(struct user_fpsimd_state *state); > extern void fpsimd_load_state(struct user_fpsimd_state *state); > > -extern void fpsimd_save(void); > - > extern void fpsimd_thread_switch(struct task_struct *next); > extern void fpsimd_flush_thread(void); > > +extern void fpsimd_save_and_flush_cpu_state(void); > + Nit, bit I think this makes more sense alongside the other flush functions. Can you just drop this new declaration in place of the old declaration for sve_flush_cpu_state()? > extern void fpsimd_signal_preserve_current_state(void); > extern void fpsimd_preserve_current_state(void); > extern void fpsimd_restore_current_state(void); > @@ -63,7 +63,6 @@ extern void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *state, > void *sve_state, unsigned int sve_vl); > > extern void fpsimd_flush_task_state(struct task_struct *target); > -extern void fpsimd_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/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c > index 577296bba730..9e4e4b6acd93 100644 > --- a/arch/arm64/kernel/fpsimd.c > +++ b/arch/arm64/kernel/fpsimd.c > @@ -241,7 +241,7 @@ static void task_fpsimd_load(void) > * > * Softirqs (and preemption) must be disabled. > */ > -void fpsimd_save(void) > +static void fpsimd_save(void) > { > struct fpsimd_last_state_struct const *last = > this_cpu_ptr(&fpsimd_last_state); > @@ -1117,12 +1117,22 @@ void fpsimd_flush_task_state(struct task_struct *t) > * Invalidate any task's FPSIMD state that is present on this cpu. > * This function must be called with softirqs disabled. > */ > -void fpsimd_flush_cpu_state(void) > +static void fpsimd_flush_cpu_state(void) > { > __this_cpu_write(fpsimd_last_state.st, NULL); > set_thread_flag(TIF_FOREIGN_FPSTATE); > } > > +/* > + * Save the FPSIMD state to memory and invalidate cpu view. > + * This function must be called with softirqs (and preemption) disabled. > + */ > +void fpsimd_save_and_flush_cpu_state(void) > +{ > + fpsimd_save(); > + fpsimd_flush_cpu_state(); > +} > + > #ifdef CONFIG_KERNEL_MODE_NEON > > DEFINE_PER_CPU(bool, kernel_neon_busy); > @@ -1279,8 +1289,7 @@ static int fpsimd_cpu_pm_notifier(struct notifier_block *self, > { > switch (cmd) { > case CPU_PM_ENTER: > - fpsimd_save(); > - fpsimd_flush_cpu_state(); > + fpsimd_save_and_flush_cpu_state(); Any reason not to do this in kernel_neon_begin() too? > break; > case CPU_PM_EXIT: > break; > diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c > index 6e3c9c8b2df9..01bd2e862506 100644 > --- a/arch/arm64/kvm/fpsimd.c > +++ b/arch/arm64/kvm/fpsimd.c > @@ -113,8 +113,7 @@ void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu) > u64 *guest_zcr = &vcpu->arch.ctxt.sys_regs[ZCR_EL1]; > > /* Clean guest FP state to memory and invalidate cpu view */ You can lose the comment here, since the call is now self-explanatory. > - fpsimd_save(); > - fpsimd_flush_cpu_state(); > + fpsimd_save_and_flush_cpu_state(); [...] Otherwise, looks sensible. Cheers ---Dave
Hi Dave, On 4/16/19 12:53 PM, Dave Martin wrote: > On Fri, Apr 12, 2019 at 06:14:19PM +0100, Julien Grall wrote: >> The only external user of fpsimd_save() and fpsimd_flush_cpu_state() is >> the KVM FPSIMD code. >> >> A follow-up patch will mandate hold of the FPSIMD context while > > This is a bit hard to read without understanding the following patch. > > Maybe elaborate a bit with something like > > "The following patch will introduce a mechanism to acquire ownership of the FPSIMD/SVE context for performing context management operations. Rather than having to export this [...]" I will update the commit message with your suggestion. > >> modifying it. Rather than having to export the new helpers to get/put >> the context, we can just introduce a new function to combine >> fpsimd_save() and fpsimd_flush_cpu_state(). >> >> This has also the advantage to remove any external call of fpsimd_save() >> and fpsimd_flush_cpu_state(), so they can be turned static. >> >> Lastly, the new function can also be used in the PM notifier. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> >> --- >> Changes in v2: >> - Patch added >> --- >> arch/arm64/include/asm/fpsimd.h | 5 ++--- >> arch/arm64/kernel/fpsimd.c | 17 +++++++++++++---- >> arch/arm64/kvm/fpsimd.c | 3 +-- >> 3 files changed, 16 insertions(+), 9 deletions(-) >> >> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h >> index 32abbfd852e8..88c223f6ded6 100644 >> --- a/arch/arm64/include/asm/fpsimd.h >> +++ b/arch/arm64/include/asm/fpsimd.h >> @@ -48,11 +48,11 @@ struct task_struct; >> extern void fpsimd_save_state(struct user_fpsimd_state *state); >> extern void fpsimd_load_state(struct user_fpsimd_state *state); >> >> -extern void fpsimd_save(void); >> - >> extern void fpsimd_thread_switch(struct task_struct *next); >> extern void fpsimd_flush_thread(void); >> >> +extern void fpsimd_save_and_flush_cpu_state(void); >> + > > Nit, bit I think this makes more sense alongside the other flush > functions. > > Can you just drop this new declaration in place of the old declaration > for sve_flush_cpu_state()? Sure, I will do it in the next version. > >> extern void fpsimd_signal_preserve_current_state(void); >> extern void fpsimd_preserve_current_state(void); >> extern void fpsimd_restore_current_state(void); >> @@ -63,7 +63,6 @@ extern void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *state, >> void *sve_state, unsigned int sve_vl); >> >> extern void fpsimd_flush_task_state(struct task_struct *target); >> -extern void fpsimd_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/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c >> index 577296bba730..9e4e4b6acd93 100644 >> --- a/arch/arm64/kernel/fpsimd.c >> +++ b/arch/arm64/kernel/fpsimd.c >> @@ -241,7 +241,7 @@ static void task_fpsimd_load(void) >> * >> * Softirqs (and preemption) must be disabled. >> */ >> -void fpsimd_save(void) >> +static void fpsimd_save(void) >> { >> struct fpsimd_last_state_struct const *last = >> this_cpu_ptr(&fpsimd_last_state); >> @@ -1117,12 +1117,22 @@ void fpsimd_flush_task_state(struct task_struct *t) >> * Invalidate any task's FPSIMD state that is present on this cpu. >> * This function must be called with softirqs disabled. >> */ >> -void fpsimd_flush_cpu_state(void) >> +static void fpsimd_flush_cpu_state(void) >> { >> __this_cpu_write(fpsimd_last_state.st, NULL); >> set_thread_flag(TIF_FOREIGN_FPSTATE); >> } >> >> +/* >> + * Save the FPSIMD state to memory and invalidate cpu view. >> + * This function must be called with softirqs (and preemption) disabled. >> + */ >> +void fpsimd_save_and_flush_cpu_state(void) >> +{ >> + fpsimd_save(); >> + fpsimd_flush_cpu_state(); >> +} >> + >> #ifdef CONFIG_KERNEL_MODE_NEON >> >> DEFINE_PER_CPU(bool, kernel_neon_busy); >> @@ -1279,8 +1289,7 @@ static int fpsimd_cpu_pm_notifier(struct notifier_block *self, >> { >> switch (cmd) { >> case CPU_PM_ENTER: >> - fpsimd_save(); >> - fpsimd_flush_cpu_state(); >> + fpsimd_save_and_flush_cpu_state(); > > Any reason not to do this in kernel_neon_begin() too? In the next patch, fpsimd_save_and_flush_cpu_state() will also call {get, put}_cpu_fpsimd_context(). So we avoid to expose the two new functions outside of the file. I could introduce a double-underscore version of this function if you think it is worth it. > >> break; >> case CPU_PM_EXIT: >> break; >> diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c >> index 6e3c9c8b2df9..01bd2e862506 100644 >> --- a/arch/arm64/kvm/fpsimd.c >> +++ b/arch/arm64/kvm/fpsimd.c >> @@ -113,8 +113,7 @@ void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu) >> u64 *guest_zcr = &vcpu->arch.ctxt.sys_regs[ZCR_EL1]; >> >> /* Clean guest FP state to memory and invalidate cpu view */ > > You can lose the comment here, since the call is now self-explanatory. Sure. > >> - fpsimd_save(); >> - fpsimd_flush_cpu_state(); >> + fpsimd_save_and_flush_cpu_state(); > > [...] > > Otherwise, looks sensible. Thank you for the review! Cheers,
diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h index 32abbfd852e8..88c223f6ded6 100644 --- a/arch/arm64/include/asm/fpsimd.h +++ b/arch/arm64/include/asm/fpsimd.h @@ -48,11 +48,11 @@ struct task_struct; extern void fpsimd_save_state(struct user_fpsimd_state *state); extern void fpsimd_load_state(struct user_fpsimd_state *state); -extern void fpsimd_save(void); - extern void fpsimd_thread_switch(struct task_struct *next); extern void fpsimd_flush_thread(void); +extern void fpsimd_save_and_flush_cpu_state(void); + extern void fpsimd_signal_preserve_current_state(void); extern void fpsimd_preserve_current_state(void); extern void fpsimd_restore_current_state(void); @@ -63,7 +63,6 @@ extern void fpsimd_bind_state_to_cpu(struct user_fpsimd_state *state, void *sve_state, unsigned int sve_vl); extern void fpsimd_flush_task_state(struct task_struct *target); -extern void fpsimd_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/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index 577296bba730..9e4e4b6acd93 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -241,7 +241,7 @@ static void task_fpsimd_load(void) * * Softirqs (and preemption) must be disabled. */ -void fpsimd_save(void) +static void fpsimd_save(void) { struct fpsimd_last_state_struct const *last = this_cpu_ptr(&fpsimd_last_state); @@ -1117,12 +1117,22 @@ void fpsimd_flush_task_state(struct task_struct *t) * Invalidate any task's FPSIMD state that is present on this cpu. * This function must be called with softirqs disabled. */ -void fpsimd_flush_cpu_state(void) +static void fpsimd_flush_cpu_state(void) { __this_cpu_write(fpsimd_last_state.st, NULL); set_thread_flag(TIF_FOREIGN_FPSTATE); } +/* + * Save the FPSIMD state to memory and invalidate cpu view. + * This function must be called with softirqs (and preemption) disabled. + */ +void fpsimd_save_and_flush_cpu_state(void) +{ + fpsimd_save(); + fpsimd_flush_cpu_state(); +} + #ifdef CONFIG_KERNEL_MODE_NEON DEFINE_PER_CPU(bool, kernel_neon_busy); @@ -1279,8 +1289,7 @@ static int fpsimd_cpu_pm_notifier(struct notifier_block *self, { switch (cmd) { case CPU_PM_ENTER: - fpsimd_save(); - fpsimd_flush_cpu_state(); + fpsimd_save_and_flush_cpu_state(); break; case CPU_PM_EXIT: break; diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c index 6e3c9c8b2df9..01bd2e862506 100644 --- a/arch/arm64/kvm/fpsimd.c +++ b/arch/arm64/kvm/fpsimd.c @@ -113,8 +113,7 @@ void kvm_arch_vcpu_put_fp(struct kvm_vcpu *vcpu) u64 *guest_zcr = &vcpu->arch.ctxt.sys_regs[ZCR_EL1]; /* Clean guest FP state to memory and invalidate cpu view */ - fpsimd_save(); - fpsimd_flush_cpu_state(); + fpsimd_save_and_flush_cpu_state(); if (guest_has_sve) *guest_zcr = read_sysreg_s(SYS_ZCR_EL12);
The only external user of fpsimd_save() and fpsimd_flush_cpu_state() is the KVM FPSIMD code. A follow-up patch will mandate hold of the FPSIMD context while modifying it. Rather than having to export the new helpers to get/put the context, we can just introduce a new function to combine fpsimd_save() and fpsimd_flush_cpu_state(). This has also the advantage to remove any external call of fpsimd_save() and fpsimd_flush_cpu_state(), so they can be turned static. Lastly, the new function can also be used in the PM notifier. Signed-off-by: Julien Grall <julien.grall@arm.com> --- Changes in v2: - Patch added --- arch/arm64/include/asm/fpsimd.h | 5 ++--- arch/arm64/kernel/fpsimd.c | 17 +++++++++++++---- arch/arm64/kvm/fpsimd.c | 3 +-- 3 files changed, 16 insertions(+), 9 deletions(-)