From: Andy Lutomirski <luto@kernel.org>
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: LKML <linux-kernel@vger.kernel.org>, X86 ML <x86@kernel.org>,
Andrew Lutomirski <luto@kernel.org>,
Paolo Bonzini <pbonzini@redhat.com>,
Radim Krcmar <rkrcmar@redhat.com>, kvm list <kvm@vger.kernel.org>,
"Jason A. Donenfeld" <Jason@zx2c4.com>,
Rik van Riel <riel@surriel.com>,
Dave Hansen <dave.hansen@linux.intel.com>
Subject: Re: [PATCH 11/11] x86/fpu: defer FPU state load until return to userspace
Date: Thu, 4 Oct 2018 09:14:33 -0700 [thread overview]
Message-ID: <CALCETrV01t-4gya0WEY0=R7XvuDA4dkf_pssfPZKDm9=1fCBmg@mail.gmail.com> (raw)
In-Reply-To: <20181004140547.13014-12-bigeasy@linutronix.de>
On Thu, Oct 4, 2018 at 7:06 AM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> From: Rik van Riel <riel@surriel.com>
>
> Defer loading of FPU state until return to userspace. This gives
> the kernel the potential to skip loading FPU state for tasks that
> stay in kernel mode, or for tasks that end up with repeated
> invocations of kernel_fpu_begin.
>
> It also increases the chances that a task's FPU state will remain
> valid in the FPU registers until it is scheduled back in, allowing
> us to skip restoring that task's FPU state altogether.
>
> The __fpregs_changes_{begin|end}() section ensures that the register
> remain unchanged. Otherwise a context switch or a BH could save the
> registers to its FPU context and processor's FPU register would remain
> random.
> fpu__restore() has one user so I pulled that preempt_disable() part into
> fpu__restore(). While the function did *load* the registers, it now just
> makes sure that they are loaded on return to userland.
>
> KVM swaps the host/guest register on enry/exit path. I kept the flow as
> is. First it ensures that the registers are loaded and then saves the
> current (host) state before it loads the guest's register. Before
> entring the guest, it ensures that the register are still loaded.
>
> Signed-off-by: Rik van Riel <riel@surriel.com>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
> arch/x86/entry/common.c | 9 +++
> arch/x86/include/asm/fpu/api.h | 11 +++
> arch/x86/include/asm/fpu/internal.h | 25 ++++---
> arch/x86/include/asm/trace/fpu.h | 5 +-
> arch/x86/kernel/fpu/core.c | 108 ++++++++++++++++++++--------
> arch/x86/kernel/fpu/signal.c | 3 -
> arch/x86/kernel/process.c | 2 +-
> arch/x86/kernel/process_32.c | 7 +-
> arch/x86/kernel/process_64.c | 7 +-
> arch/x86/kvm/x86.c | 18 +++--
> 10 files changed, 143 insertions(+), 52 deletions(-)
>
> diff --git a/arch/x86/entry/common.c b/arch/x86/entry/common.c
> index 3b2490b819181..3dad5c3b335eb 100644
> --- a/arch/x86/entry/common.c
> +++ b/arch/x86/entry/common.c
> @@ -31,6 +31,7 @@
> #include <asm/vdso.h>
> #include <linux/uaccess.h>
> #include <asm/cpufeature.h>
> +#include <asm/fpu/api.h>
>
> #define CREATE_TRACE_POINTS
> #include <trace/events/syscalls.h>
> @@ -196,6 +197,14 @@ __visible inline void prepare_exit_to_usermode(struct pt_regs *regs)
> if (unlikely(cached_flags & EXIT_TO_USERMODE_LOOP_FLAGS))
> exit_to_usermode_loop(regs, cached_flags);
>
> + /* Reload ti->flags; we may have rescheduled above. */
> + cached_flags = READ_ONCE(ti->flags);
> +
> + if (unlikely(cached_flags & _TIF_LOAD_FPU))
> + switch_fpu_return();
> + else
> + fpregs_is_state_consistent();
Shouldn't this be:
fpregs_assert_state_consistent(); /* see below */
if (unlikely(cached_flags & _TIF_LOAD_FPU))
switch_fpu_return();
> diff --git a/arch/x86/include/asm/fpu/api.h b/arch/x86/include/asm/fpu/api.h
> index a9caac9d4a729..e3077860f7333 100644
> --- a/arch/x86/include/asm/fpu/api.h
> +++ b/arch/x86/include/asm/fpu/api.h
> @@ -27,6 +27,17 @@ extern void kernel_fpu_begin(void);
> extern void kernel_fpu_end(void);
> extern bool irq_fpu_usable(void);
>
> +#ifdef CONFIG_X86_DEBUG_FPU
> +extern void fpregs_is_state_consistent(void);
> +#else
> +static inline void fpregs_is_state_consistent(void) { }
> +#endif
Can you name this something like fpregs_assert_state_consistent()?
The "is" name makes it sound like it's:
bool fpregs_is_state_consistent();
and you're supposed to do:
WARN_ON(!fpregs_is_state_consistent());
next prev parent reply other threads:[~2018-10-04 16:14 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-04 14:05 [PATCH 00/11 v3] x86: load FPU registers on return to userland Sebastian Andrzej Siewior
2018-10-04 14:05 ` [PATCH 01/11] x86/entry: remove _TIF_ALLWORK_MASK Sebastian Andrzej Siewior
2018-10-11 16:27 ` Borislav Petkov
2018-10-04 14:05 ` [PATCH 02/11] x86/fpu: add (__)make_fpregs_active helpers Sebastian Andrzej Siewior
2018-10-04 14:05 ` [PATCH 03/11] x86/fpu: make __raw_xsave_addr() use feature number instead of mask Sebastian Andrzej Siewior
2018-10-11 17:30 ` Christophe de Dinechin
2018-10-18 11:19 ` Sebastian Andrzej Siewior
2018-10-12 15:52 ` Dave Hansen
2018-10-18 11:17 ` Sebastian Andrzej Siewior
2018-10-18 11:21 ` Sebastian Andrzej Siewior
2018-10-17 10:01 ` Borislav Petkov
2018-10-18 11:48 ` Sebastian Andrzej Siewior
2018-10-04 14:05 ` [PATCH 04/11] x86/fpu: eager switch PKRU state Sebastian Andrzej Siewior
2018-10-12 17:51 ` Dave Hansen
2018-10-12 18:09 ` Andy Lutomirski
2018-10-12 19:44 ` Dave Hansen
2018-10-18 16:13 ` Sebastian Andrzej Siewior
2018-10-18 17:50 ` Dave Hansen
2018-10-04 14:05 ` [PATCH 05/11] x86/fpu: set PKRU state for kernel threads Sebastian Andrzej Siewior
2018-10-12 17:54 ` Dave Hansen
2018-10-12 18:02 ` Andy Lutomirski
2018-10-18 16:26 ` Sebastian Andrzej Siewior
2018-10-18 16:48 ` Andy Lutomirski
2018-10-18 17:47 ` Dave Hansen
2018-10-18 18:25 ` Sebastian Andrzej Siewior
2018-10-18 20:46 ` Andy Lutomirski
2018-10-18 20:56 ` Dave Hansen
2018-10-18 21:24 ` Sebastian Andrzej Siewior
2018-10-18 21:58 ` Andy Lutomirski
2018-10-19 7:44 ` Paolo Bonzini
2018-10-19 16:59 ` Andy Lutomirski
2018-10-19 17:01 ` Dave Hansen
2018-10-19 17:37 ` Andy Lutomirski
2018-10-19 18:26 ` Dave Hansen
2018-10-04 14:05 ` [PATCH 06/11] x86/pkeys: make init_pkru_value static Sebastian Andrzej Siewior
2018-10-12 17:55 ` Dave Hansen
2018-10-04 14:05 ` [PATCH 07/11] x86/pkeys: Drop the preempt-disable section Sebastian Andrzej Siewior
2018-10-12 17:58 ` Dave Hansen
2018-10-12 18:07 ` Andy Lutomirski
2018-10-12 20:26 ` Sebastian Andrzej Siewior
2018-10-04 14:05 ` [PATCH 08/11] x86/fpu: Always store the registers in copy_fpstate_to_sigframe() Sebastian Andrzej Siewior
2018-10-11 17:50 ` Christophe de Dinechin
2018-10-11 21:18 ` Andy Lutomirski
2018-10-12 18:15 ` Dave Hansen
2018-11-02 14:42 ` Sebastian Andrzej Siewior
2018-10-04 14:05 ` [PATCH 09/11] x86/entry: add TIF_LOAD_FPU Sebastian Andrzej Siewior
2018-10-04 14:05 ` [PATCH 10/11] x86/fpu: prepare copy_fpstate_to_sigframe for TIF_LOAD_FPU Sebastian Andrzej Siewior
2018-10-12 19:40 ` Dave Hansen
2018-10-15 15:24 ` Borislav Petkov
2018-11-02 15:44 ` Sebastian Andrzej Siewior
2018-11-02 22:55 ` Sebastian Andrzej Siewior
2018-10-04 14:05 ` [PATCH 11/11] x86/fpu: defer FPU state load until return to userspace Sebastian Andrzej Siewior
2018-10-04 16:14 ` Andy Lutomirski [this message]
2018-10-12 20:25 ` Sebastian Andrzej Siewior
2018-10-04 16:45 ` [PATCH 00/11 v3] x86: load FPU registers on return to userland Rik van Riel
2018-10-04 16:50 ` Andy Lutomirski
2018-10-05 11:55 ` Sebastian Andrzej Siewior
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CALCETrV01t-4gya0WEY0=R7XvuDA4dkf_pssfPZKDm9=1fCBmg@mail.gmail.com' \
--to=luto@kernel.org \
--cc=Jason@zx2c4.com \
--cc=bigeasy@linutronix.de \
--cc=dave.hansen@linux.intel.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pbonzini@redhat.com \
--cc=riel@surriel.com \
--cc=rkrcmar@redhat.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).