linux-kernel - Re: [PATCH 054/208] x86/fpu: Get rid of PF_USED_MATH usage, convert it to fpu->fpstate

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 6 May 2015 05:24:11 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Andy Lutomirski <luto@...capital.net>
Cc:	Borislav Petkov <bp@...en8.de>, Fenghua Yu <fenghua.yu@...el.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Oleg Nesterov <oleg@...hat.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 054/208] x86/fpu: Get rid of PF_USED_MATH usage, convert
 it to fpu->fpstate_active

* Andy Lutomirski <luto@...capital.net> wrote:

> > diff --git a/arch/x86/include/asm/fpu/types.h b/arch/x86/include/asm/fpu/types.h
> > index efb520dcf38e..f6317d9aa808 100644
> > --- a/arch/x86/include/asm/fpu/types.h
> > +++ b/arch/x86/include/asm/fpu/types.h
> > @@ -137,6 +137,12 @@ struct fpu {
> >          * deal with bursty apps that only use the FPU for a short time:
> >          */
> >         unsigned char                   counter;
> > +       /*
> > +        * This flag indicates whether this context is fpstate_active: if the task is
> > +        * not running then we can restore from this context, if the task
> > +        * is running then we should save into this context.
> > +        */
> > +       unsigned char                   fpstate_active;
> 
> I don't understand.  What does it mean if !fpstate_active?

Yeah, so this was just the 'simple' migration patch from PF_USED_FPU 
to ->fpstate_active.

At the end of the series those fields get more love and a more 
detailed explanation:

        /*
         * @fpstate_active:
         *
         * This flag indicates whether this context is active: if the task
         * is not running then we can restore from this context, if the task
         * is running then we should save into this context.
         */
        unsigned char                   fpstate_active;

and its interaction with fpregs_active is explained as well:

        /*
         * @fpregs_active:
         *
         * This flag determines whether a given context is actively
         * loaded into the FPU's registers and that those registers
         * represent the task's current FPU state.
         *
         * Note the interaction with fpstate_active:
         *
         *   # task does not use the FPU:
         *   fpstate_active == 0
         *
         *   # task uses the FPU and regs are active:
         *   fpstate_active == 1 && fpregs_active == 1
         *
         *   # the regs are inactive but still match fpstate:
         *   fpstate_active == 1 && fpregs_active == 0 && fpregs_owner == fpu
         *
         * The third state is what we use for the lazy restore optimization
         * on lazy-switching CPUs.
         */
        unsigned char                   fpregs_active;

Basically the 'fpstate' is the in-memory FPU state, and if it's 
active, it means it can be copied to (saved to) and copied from 
(restored from). Whether this fpstate is the currently representative 
FPU state depends on the other state flag(s), as described.

Maybe I should have broken out the fourth state as well:

         *   # the fpstate holds all of a task's FPU state:
         *   fpstate_active == 1 && fpregs_active == 0 && fpregs_owner != fpu

?

active/inactive was one idiom that I felt worked pretty well - but I 
considered others as well:

  - dirty/clean (didn't work so well and too MM-ish)
  - valid/invalid (likewise)
  - used/unused (yuck)

Note:

  There's a fifth valid state as well, but I did not want
  to complicate the description even more: kernel_user_begin()/end()
  users create this state with its own private in_kernel_fpu flag, in 
  that they use FPU registers but don't touch these (user-)flags. 
  kernel_user_begin()/end() is atomic and (beyond zapping pending lazy 
  restore state in fpu_fpregs_owner_ctx) it restores the FPU to the 
  previous state so it's pretty orthogonal as far as the other states 
  are concerned.

Note2:

  I also considered renaming kernel_fpu_begin()/end() to the new 
  nomenclature, but it has a good name and I did not want too much
  churn with a well-established API, which also mirrors
  user_fpu_begin() conceptually. I also couldn't find a better name:
  maybe fpu__kernel_save()/restore(), but that felt a bit strained.

Does this make things clearer? I can work on it some more if I got it 
wrong or if the text is confusing somewhere, this is crucial IMHO.

Instead of binary states we could also unify them into a single state 
variable - didn't find any really convincing naming concept for that 
though, mostly because I think those states are fundamentally 
separate, just interrelated.

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/