Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (1.0)
Subject: Re: [RFC PATCH 10/10] x86/fpu: defer FPU state load until return to userspace
From:   Andy Lutomirski <luto@amacapital.net>
In-Reply-To: <D5D1FB08-9CE8-498B-9636-C0390153814A@amacapital.net>
Date:   Thu, 20 Sep 2018 21:15:35 -0700
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org,
        Andy Lutomirski <luto@kernel.org>,
        Paolo Bonzini <pbonzini@redhat.com>,
        =?utf-8?Q?Radim_Kr=C4=8Dm=C3=A1=C5=99?= <rkrcmar@redhat.com>,
        kvm@vger.kernel.org, "Jason A. Donenfeld" <Jason@zx2c4.com>,
        Rik van Riel <riel@surriel.com>
Content-Transfer-Encoding: quoted-printable
Message-Id: <A5B266A3-1EF7-44A4-9AAF-1AA1858CEFCF@amacapital.net>
References: <20180912133353.20595-1-bigeasy@linutronix.de> <20180912133353.20595-11-bigeasy@linutronix.de> <650FC457-7E4C-473A-9E5F-EAFC74F6444B@amacapital.net> <20180919170515.ptqmmpsxrdjsi64j@linutronix.de> <D5D1FB08-9CE8-498B-9636-C0390153814A@amacapital.net>
To:     Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


> On Sep 20, 2018, at 8:45 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>=20
>=20
>=20
>> On Sep 19, 2018, at 10:05 AM, Sebastian Andrzej Siewior <bigeasy@linutron=
ix.de> wrote:
>>=20
>> On 2018-09-12 08:47:19 [-0700], Andy Lutomirski wrote:
>>>> --- a/arch/x86/kernel/fpu/core.c
>>>> +++ b/arch/x86/kernel/fpu/core.c
>>>> @@ -101,14 +101,14 @@ void __kernel_fpu_begin(void)
>>>>=20
>>>>  kernel_fpu_disable();
>>>>=20
>>>> -    if (fpu->initialized) {
>>>> +    __cpu_invalidate_fpregs_state();
>>>> +
>>>> +    if (!test_and_set_thread_flag(TIF_LOAD_FPU)) {
>>>=20
>>> Since the already-TIF_LOAD_FPU path is supposed to be fast here, use tes=
t_thread_flag() instead. test_and_set operations do unconditional RMW operat=
ions and are always full barriers, so they=E2=80=99re slow.
>> okay.
>>=20
>>> Also, on top of this patch, there should be lots of cleanups available. I=
n particular, all the fpu state accessors could probably be reworked to take=
 TIF_LOAD_FPU into account, which would simplify the callers and maybe even t=
he mess of variables tracking whether the state is in regs.
>>=20
>> Do you refer to the fpu.initilized check or something else?
>>=20
>=20
> I mean the fpu.initialized variable entirely. AFAIK, its only use is for k=
ernel threads =E2=80=94 setting it to false lets us switch to a kernel threa=
d and back without saving and restoring. But TIF_LOAD_FPU should be able to r=
eplace it: when we have FPU regs loaded and we switch to *any* thread, kerne=
l or otherwise, we can set TIF_LOAD_FPU and leave the old regs loaded.  So w=
e don=E2=80=99t need the special case for kernel threads.
>=20
> Which reminds me: if you haven=E2=80=99t already done so, can you add a he=
lper to sanity check the current context?  It should check that the combinat=
ion of owner_ctx, last_cpu, and TIF_LOAD_FPU is sane. For example, if owner_=
ctx or last_cpu is says the cpu regs are invalid for current but TIF_LOAD_FP=
U is clear, it should warn.  I think that at least switch_fpu_finish should c=
all it.  Arguably switch_fpu_prepare should too, at the beginning.

Looking some more, the =E2=80=9Cpreload=E2=80=9D variable needs to go away o=
r be renamed. It hasn=E2=80=99t had anything to do with preloading for some t=
ime.

Also, the interaction between TIF_LOAD_FPU and FPU emulation needs to be doc=
umented somewhere.  Probably FPU-less systems should never have TIF_LOAD_FPU=
 set.

Or we could decide that no one uses FPU emulation any more.=