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;
MIME-Version: 1.0
References: <20190201205319.15995-1-chang.seok.bae@intel.com> <20190201205319.15995-7-chang.seok.bae@intel.com>
In-Reply-To: <20190201205319.15995-7-chang.seok.bae@intel.com>
From:   Andy Lutomirski <luto@kernel.org>
Date:   Fri, 1 Feb 2019 18:57:32 -0800
Message-ID: <CALCETrUFF3AcO7n_SWs--9YQ=05Swpq5zqjcjWb8_mOLJDRwZw@mail.gmail.com>
Subject: Re: [PATCH v5 05/13] x86/fsgsbase/64: Enable FSGSBASE instructions in
 the helper functions
To:     "Chang S. Bae" <chang.seok.bae@intel.com>
Cc:     Andy Lutomirski <luto@kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@kernel.org>,
        "H . Peter Anvin" <hpa@zytor.com>, Andi Kleen <ak@linux.intel.com>,
        Markus T Metzger <markus.t.metzger@intel.com>,
        Ravi Shankar <ravi.v.shankar@intel.com>,
        LKML <linux-kernel@vger.kernel.org>,
        Andrew Cooper <andrew.cooper3@citrix.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, Feb 1, 2019 at 12:54 PM Chang S. Bae <chang.seok.bae@intel.com> wrote:
>
> The helper functions will switch on faster accesses to FSBASE and GSBASE
> when the FSGSBASE feature is enabled.
>
> Accessing user GSBASE needs a couple of SWAPGS operations. It is avoidable
> if the user GSBASE is saved at kernel entry, being updated as changes, and
> restored back at kernel exit. However, it seems to spend more cycles for
> savings and restorations. Little or no benefit was measured from
> experiments.
>
> Also, introduce __{rd,wr}gsbase_inactive() as helpers to access user GSBASE
> with SWAPGS. Note, for Xen PV, paravirt hooks can be added, since it may
> allow a very efficient but different implementation.
>
> [ Use NOKPROBE_SYMBOL instead of __kprobes ]

^^^ This line looks like it shold be deleted.

>
> Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
> Cc: Any Lutomirski <luto@kernel.org>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  arch/x86/include/asm/fsgsbase.h | 27 +++++++-------
>  arch/x86/kernel/process_64.c    | 62 +++++++++++++++++++++++++++++++--
>  2 files changed, 72 insertions(+), 17 deletions(-)
>
> diff --git a/arch/x86/include/asm/fsgsbase.h b/arch/x86/include/asm/fsgsbase.h
> index fdd1177499b4..aefd53767a5d 100644
> --- a/arch/x86/include/asm/fsgsbase.h
> +++ b/arch/x86/include/asm/fsgsbase.h
> @@ -49,35 +49,32 @@ static __always_inline void wrgsbase(unsigned long gsbase)
>         asm volatile("wrgsbase %0" :: "r" (gsbase) : "memory");
>  }
>
> +#include <asm/cpufeature.h>
> +
>  /* Helper functions for reading/writing FS/GS base */
>
>  static inline unsigned long x86_fsbase_read_cpu(void)
>  {
>         unsigned long fsbase;
>
> -       rdmsrl(MSR_FS_BASE, fsbase);
> +       if (static_cpu_has(X86_FEATURE_FSGSBASE))
> +               fsbase = rdfsbase();
> +       else
> +               rdmsrl(MSR_FS_BASE, fsbase);
>
>         return fsbase;
>  }
>
> -static inline unsigned long x86_gsbase_read_cpu_inactive(void)
> -{
> -       unsigned long gsbase;
> -
> -       rdmsrl(MSR_KERNEL_GS_BASE, gsbase);
> -
> -       return gsbase;
> -}
> -
>  static inline void x86_fsbase_write_cpu(unsigned long fsbase)
>  {
> -       wrmsrl(MSR_FS_BASE, fsbase);
> +       if (static_cpu_has(X86_FEATURE_FSGSBASE))
> +               wrfsbase(fsbase);
> +       else
> +               wrmsrl(MSR_FS_BASE, fsbase);
>  }
>
> -static inline void x86_gsbase_write_cpu_inactive(unsigned long gsbase)
> -{
> -       wrmsrl(MSR_KERNEL_GS_BASE, gsbase);
> -}
> +extern unsigned long x86_gsbase_read_cpu_inactive(void);
> +extern void x86_gsbase_write_cpu_inactive(unsigned long gsbase);
>
>  #endif /* CONFIG_X86_64 */
>
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index 6a62f4af9fcf..ebc55ed31fe7 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -160,6 +160,42 @@ enum which_selector {
>         GS
>  };
>
> +/*
> + * Interrupts are disabled here. Out of line to be protected
> + * from kprobes. It is not used on Xen paravirt. When paravirt
> + * support is needed, it needs to be renamed with native_ prefix.
> + */
> +static noinline unsigned long __rdgsbase_inactive(void)
> +{
> +       unsigned long gsbase, flags;
> +
> +       local_irq_save(flags);
> +       native_swapgs();
> +       gsbase = rdgsbase();
> +       native_swapgs();
> +       local_irq_restore(flags);
> +
> +       return gsbase;
> +}
> +NOKPROBE_SYMBOL(__rdgsbase_inactive);
> +
> +/*
> + * Interrupts are disabled here. Out of line to be protected
> + * from kprobes. It is not used on Xen paravirt. When paravirt
> + * support is needed, it needs to be renamed with native_ prefix.
> + */
> +static noinline void __wrgsbase_inactive(unsigned long gsbase)
> +{
> +       unsigned long flags;
> +
> +       local_irq_save(flags);
> +       native_swapgs();
> +       wrgsbase(gsbase);
> +       native_swapgs();
> +       local_irq_restore(flags);
> +}
> +NOKPROBE_SYMBOL(__wrgsbase_inactive);
> +
>  /*
>   * Saves the FS or GS base for an outgoing thread if FSGSBASE extensions are
>   * not available.  The goal is to be reasonably fast on non-FSGSBASE systems.
> @@ -338,13 +374,34 @@ static unsigned long x86_fsgsbase_read_task(struct task_struct *task,
>         return base;
>  }
>
> +unsigned long x86_gsbase_read_cpu_inactive(void)
> +{
> +       unsigned long gsbase;
> +
> +       if (static_cpu_has(X86_FEATURE_FSGSBASE))
> +               gsbase = __rdgsbase_inactive();
> +       else
> +               rdmsrl(MSR_KERNEL_GS_BASE, gsbase);
> +
> +       return gsbase;
> +}
> +
> +void x86_gsbase_write_cpu_inactive(unsigned long gsbase)
> +{
> +       if (static_cpu_has(X86_FEATURE_FSGSBASE))
> +               __wrgsbase_inactive(gsbase);
> +       else
> +               wrmsrl(MSR_KERNEL_GS_BASE, gsbase);
> +}
> +
>  unsigned long x86_fsbase_read_task(struct task_struct *task)
>  {
>         unsigned long fsbase;
>
>         if (task == current)
>                 fsbase = x86_fsbase_read_cpu();
> -       else if (task->thread.fsindex == 0)
> +       else if (static_cpu_has(X86_FEATURE_FSGSBASE) ||
> +                (task->thread.fsindex == 0))
>                 fsbase = task->thread.fsbase;
>         else
>                 fsbase = x86_fsgsbase_read_task(task, task->thread.fsindex);
> @@ -358,7 +415,8 @@ unsigned long x86_gsbase_read_task(struct task_struct *task)
>
>         if (task == current)
>                 gsbase = x86_gsbase_read_cpu_inactive();
> -       else if (task->thread.gsindex == 0)
> +       else if (static_cpu_has(X86_FEATURE_FSGSBASE) ||
> +                (task->thread.gsindex == 0))
>                 gsbase = task->thread.gsbase;
>         else
>                 gsbase = x86_fsgsbase_read_task(task, task->thread.gsindex);

These last two hunks changes do not belong in this patch.  Presumably
they belong in patch 6.

--Andy


> --
> 2.19.1
>