| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20170126150525.GA31833@gmail.com>
Date: Thu, 26 Jan 2017 16:05:25 +0100
From: Ingo Molnar <mingo@...nel.org>
To: Rik van Riel <riel@...hat.com>
Cc: linux-kernel@...r.kernel.org,
Andrew Morton <akpm@...ux-foundation.org>,
Andy Lutomirski <luto@...capital.net>,
Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>,
Fenghua Yu <fenghua.yu@...el.com>,
"H . Peter Anvin" <hpa@...or.com>,
Linus Torvalds <torvalds@...ux-foundation.org>,
Oleg Nesterov <oleg@...hat.com>,
Peter Zijlstra <peterz@...radead.org>,
Thomas Gleixner <tglx@...utronix.de>,
Yu-cheng Yu <yu-cheng.yu@...el.com>
Subject: [PATCH] x86/fpu: Unify the naming of the FPU register cache validity
flags
* Ingo Molnar <mingo@...nel.org> wrote:
> What we could do is to unify the naming to explain all this a bit better - right
> now there's very little indication that ->fpregs_cached is closely related to
> fpu_fpregs_owner_ctx.
>
> For example we could rename them to:
>
> ->fpregs_cached => ->fpregs_owner [bool]
> fpu_fpregs_owner_ctx => fpregs_owner_ctx [ptr]
>
> ?
>
> Clearing ->fpregs_owner or setting fpregs_owner_ctx to NULL invalidates the
> cache and it's clear from the naming that the two values are closely related.
Something like the patch below - only minimally tested.
Thanks,
Ingo
================>
>From d5b99e1e25f86d4880bf85588eb4a4769610dd47 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@...nel.org>
Date: Thu, 26 Jan 2017 16:01:00 +0100
Subject: [PATCH] x86/fpu: Unify the naming of the FPU register cache validity flags
Rik pointed out that the fpu_fpregs_owner_ctx and the ->fpregs_cached
flags are still described in a confusing way.
Clarify this some more by renaming them to:
->fpregs_cached => ->fpregs_owner [bool]
fpu_fpregs_owner_ctx => fpregs_owner_ctx [ptr]
... which better expresses that they are a cache validity flag
split into two parts, where the cache can be invalidated if any
of the flags is cleared.
Also describe this relationship more accurately in the fpu/types.h header.
No change in functionality.
Reported-by: Rik van Riel <riel@...hat.com>
Cc: Andy Lutomirski <luto@...nel.org>
Cc: Borislav Petkov <bp@...en8.de>
Cc: Dave Hansen <dave.hansen@...ux.intel.com>
Cc: Fenghua Yu <fenghua.yu@...el.com>
Cc: H. Peter Anvin <hpa@...or.com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: Oleg Nesterov <oleg@...hat.com>
Cc: Thomas Gleixner <tglx@...utronix.de>
Cc: Yu-cheng Yu <yu-cheng.yu@...el.com>
Cc: Fenghua Yu <fenghua.yu@...el.com>
Signed-off-by: Ingo Molnar <mingo@...nel.org>
---
arch/x86/include/asm/fpu/internal.h | 22 +++++++++++-----------
arch/x86/include/asm/fpu/types.h | 12 ++++++++++--
arch/x86/include/asm/switch_to.h | 2 +-
arch/x86/kernel/fpu/core.c | 4 ++--
arch/x86/kernel/smpboot.c | 2 +-
5 files changed, 25 insertions(+), 17 deletions(-)
diff --git a/arch/x86/include/asm/fpu/internal.h b/arch/x86/include/asm/fpu/internal.h
index e62eee2e989e..bbee00aac864 100644
--- a/arch/x86/include/asm/fpu/internal.h
+++ b/arch/x86/include/asm/fpu/internal.h
@@ -486,11 +486,11 @@ extern int copy_fpstate_to_sigframe(void __user *buf, void __user *fp, int size)
* FPU context switch related helper methods:
*/
-DECLARE_PER_CPU(struct fpu *, fpu_fpregs_owner_ctx);
+DECLARE_PER_CPU(struct fpu *, fpregs_owner_ctx);
/*
* The in-register FPU state for an FPU context on a CPU is assumed to be
- * valid if fpu->fpregs_cached is still set, and if the fpu_fpregs_owner_ctx
+ * valid if fpu->fpregs_owner is still set, and if the fpregs_owner_ctx
* matches the FPU.
*
* If the FPU register state is valid, the kernel can skip restoring the
@@ -507,17 +507,17 @@ DECLARE_PER_CPU(struct fpu *, fpu_fpregs_owner_ctx);
*/
static inline void __cpu_invalidate_fpregs_state(void)
{
- __this_cpu_write(fpu_fpregs_owner_ctx, NULL);
+ __this_cpu_write(fpregs_owner_ctx, NULL);
}
static inline void __fpu_invalidate_fpregs_state(struct fpu *fpu)
{
- fpu->fpregs_cached = 0;
+ fpu->fpregs_owner = 0;
}
static inline int fpregs_state_valid(struct fpu *fpu, unsigned int cpu)
{
- return fpu == this_cpu_read_stable(fpu_fpregs_owner_ctx) && fpu->fpregs_cached;
+ return fpu == this_cpu_read_stable(fpregs_owner_ctx) && fpu->fpregs_owner;
}
/*
@@ -526,13 +526,13 @@ static inline int fpregs_state_valid(struct fpu *fpu, unsigned int cpu)
*/
static inline void fpregs_deactivate(struct fpu *fpu)
{
- this_cpu_write(fpu_fpregs_owner_ctx, NULL);
+ this_cpu_write(fpregs_owner_ctx, NULL);
trace_x86_fpu_regs_deactivated(fpu);
}
static inline void fpregs_activate(struct fpu *fpu)
{
- this_cpu_write(fpu_fpregs_owner_ctx, fpu);
+ this_cpu_write(fpregs_owner_ctx, fpu);
trace_x86_fpu_regs_activated(fpu);
}
@@ -552,14 +552,14 @@ switch_fpu_prepare(struct fpu *old_fpu, int cpu)
{
if (old_fpu->fpstate_active) {
if (!copy_fpregs_to_fpstate(old_fpu))
- old_fpu->fpregs_cached = 0;
+ old_fpu->fpregs_owner = 0;
else
- old_fpu->fpregs_cached = 1;
+ old_fpu->fpregs_owner = 1;
- /* But leave fpu_fpregs_owner_ctx! */
+ /* But leave fpregs_owner_ctx! */
trace_x86_fpu_regs_deactivated(old_fpu);
} else {
- old_fpu->fpregs_cached = 0;
+ old_fpu->fpregs_owner = 0;
}
}
diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
index 07452fbd7867..d15cbfe0e8c4 100644
--- a/arch/x86/include/asm/fpu/types.h
+++ b/arch/x86/include/asm/fpu/types.h
@@ -285,14 +285,22 @@ struct fpu {
unsigned char fpstate_active;
/*
- * @fpregs_cached:
+ * @fpregs_owner:
*
* This flag tells us whether this context is loaded into a CPU
* right now.
*
* This is set to 0 if a task is migrated to another CPU.
+ *
+ * NOTE: the fpregs_owner_ctx percpu pointer also has to point to
+ * this FPU context for the register cache to be valid. If any
+ * of these two flags is cleared then the cache is invalid.
+ * Some internals can access the context-flag more easily,
+ * others have easier access to the percpu variable. The
+ * FPU context-switching code has access to both so there's
+ * very little cost of having the cache indexed in two ways:
*/
- unsigned char fpregs_cached;
+ unsigned char fpregs_owner;
/*
* @state:
diff --git a/arch/x86/include/asm/switch_to.h b/arch/x86/include/asm/switch_to.h
index a7146dadb31d..7a4915dd0547 100644
--- a/arch/x86/include/asm/switch_to.h
+++ b/arch/x86/include/asm/switch_to.h
@@ -78,7 +78,7 @@ do { \
*/
static inline void arch_task_migrate(struct task_struct *p)
{
- p->thread.fpu.fpregs_cached = 0;
+ p->thread.fpu.fpregs_owner = 0;
}
#define arch_task_migrate arch_task_migrate
diff --git a/arch/x86/kernel/fpu/core.c b/arch/x86/kernel/fpu/core.c
index 217e37029585..1b3bf98072fe 100644
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -39,7 +39,7 @@ static DEFINE_PER_CPU(bool, in_kernel_fpu);
/*
* Track which context is using the FPU on the CPU:
*/
-DEFINE_PER_CPU(struct fpu *, fpu_fpregs_owner_ctx);
+DEFINE_PER_CPU(struct fpu *, fpregs_owner_ctx);
static void kernel_fpu_disable(void)
{
@@ -189,7 +189,7 @@ EXPORT_SYMBOL_GPL(fpstate_init);
int fpu__copy(struct fpu *dst_fpu, struct fpu *src_fpu)
{
- dst_fpu->fpregs_cached = 0;
+ dst_fpu->fpregs_owner = 0;
if (!src_fpu->fpstate_active || !static_cpu_has(X86_FEATURE_FPU))
return 0;
diff --git a/arch/x86/kernel/smpboot.c b/arch/x86/kernel/smpboot.c
index 46732dc3b73c..8bf24aa01878 100644
--- a/arch/x86/kernel/smpboot.c
+++ b/arch/x86/kernel/smpboot.c
@@ -1118,7 +1118,7 @@ int native_cpu_up(unsigned int cpu, struct task_struct *tidle)
return err;
/* the FPU context is blank, nobody can own it */
- per_cpu(fpu_fpregs_owner_ctx, cpu) = NULL;
+ per_cpu(fpregs_owner_ctx, cpu) = NULL;
common_cpu_up(cpu, tidle);
Powered by blists - more mailing lists