On Wed, Jan 09, 2019 at 12:47:31PM +0100, Sebastian Andrzej Siewior wrote:
> From: Rik van Riel <riel@xxxxxxxxxxx>
>
> Add helper function that ensures the floating point registers for
> the current task are active. Use with preemption disabled.
>
> Signed-off-by: Rik van Riel <riel@xxxxxxxxxxx>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx>
> ---
> arch/x86/include/asm/fpu/api.h | 11 +++++++++++
> arch/x86/include/asm/fpu/internal.h | 19 +++++++++++--------
> 2 files changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/include/asm/fpu/api.h b/arch/x86/include/asm/fpu/api.h
> index b56d504af6545..31b66af8eb914 100644
> --- a/arch/x86/include/asm/fpu/api.h
> +++ b/arch/x86/include/asm/fpu/api.h
> @@ -10,6 +10,7 @@
>
> #ifndef _ASM_X86_FPU_API_H
> #define _ASM_X86_FPU_API_H
> +#include <linux/preempt.h>
>
> /*
> * Use kernel_fpu_begin/end() if you intend to use FPU in kernel context. It
> @@ -22,6 +23,16 @@ extern void kernel_fpu_begin(void);
> extern void kernel_fpu_end(void);
> extern bool irq_fpu_usable(void);
>
> +static inline void __fpregs_changes_begin(void)
> +{
> + preempt_disable();
> +}
> +
> +static inline void __fpregs_changes_end(void)
How am I to understand that "fpregs_changes" thing? That FPU registers
changes will begin and end respectively?
I probably would call them fpregs_lock and fpregs_unlock even if
it isn't doing any locking to denote that FPU regs are locked and
inaccessible inside the region.
And why the "__" prefix? Is there a counterpart without the "__" coming?
> +{
> + preempt_enable();
> +}
> +
> /*
> * Query the presence of one or more xfeatures. Works on any legacy CPU as well.
> *
> diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
> index 03acb9aeb32fc..795a0a2df135e 100644
> --- a/arch/x86/include/asm/fpu/internal.h
> +++ b/arch/x86/include/asm/fpu/internal.h
> @@ -515,6 +515,15 @@ static inline void fpregs_activate(struct fpu *fpu)
> trace_x86_fpu_regs_activated(fpu);
> }
>
> +static inline void __fpregs_load_activate(struct fpu *fpu, int cpu)
> +{
> + if (!fpregs_state_valid(fpu, cpu)) {
> + if (current->mm)
> + copy_kernel_to_fpregs(&fpu->state);
> + fpregs_activate(fpu);
> + }
> +}
> +
> /*
> * FPU state switching for scheduling.
> *
> @@ -550,14 +559,8 @@ switch_fpu_prepare(struct fpu *old_fpu, int cpu)
> */
> static inline void switch_fpu_finish(struct fpu *new_fpu, int cpu)
> {
> - if (static_cpu_has(X86_FEATURE_FPU)) {
> - if (!fpregs_state_valid(new_fpu, cpu)) {
> - if (current->mm)
> - copy_kernel_to_fpregs(&new_fpu->state);
> - }
> -
> - fpregs_activate(new_fpu);
> - }
> + if (static_cpu_has(X86_FEATURE_FPU))
> + __fpregs_load_activate(new_fpu, cpu);
And that second part of a cleanup looks strange in this patch. Why isn't
it in a separate patch or how is it related to the addition of the
helpers?
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
