linux-kernel - Re: [PATCH 09/22] x86/fpu: Add (__)make_fpregs

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20190207104325.ai5fzaxa7xy7yydv@linutronix.de>
Date:   Thu, 7 Feb 2019 11:43:25 +0100
From:   Sebastian Andrzej Siewior <bigeasy@...utronix.de>
To:     Borislav Petkov <bp@...en8.de>
Cc:     linux-kernel@...r.kernel.org, x86@...nel.org,
        Andy Lutomirski <luto@...nel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        kvm@...r.kernel.org, "Jason A. Donenfeld" <Jason@...c4.com>,
        Rik van Riel <riel@...riel.com>,
        Dave Hansen <dave.hansen@...ux.intel.com>
Subject: Re: [PATCH 09/22] x86/fpu: Add (__)make_fpregs_active helpers

On 2019-01-28 19:23:49 [+0100], Borislav Petkov wrote:
> > 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?

correct.

> 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.

They are accessible inside the region. But they should not be touched by
context switch code (and later BH).
Is that what you meant?

> And why the "__" prefix? Is there a counterpart without the "__" coming?

No. I picked up the patches, that function was named like that. I kept
it. That __ probably denotes that it is an internal function but then it
has to be used outside (KVM) if they plan to "reload" registers (which
happens if they switch between host/guest registers).

> > +{
> > +	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?

Two helpers are added:
- __fpregs_changes_{begin|end}()
  new.

- __fpregs_load_activate()
  refactored from switch_fpu_finish(),

> Thx.
> 

Sebastian