Re: [Xen-devel] [v3 04/12] x86/fsgsbase/64: Enable FSGSBASE instructions in the helper functions

[Andy Lutomirski <luto@xxxxxxxxxx>]

On Tue, Oct 23, 2018 at 11:43 AM Chang S. Bae <chang.seok.bae@xxxxxxxxx> 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.
>
> Signed-off-by: Chang S. Bae <chang.seok.bae@xxxxxxxxx>
> Reviewed-by: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> Cc: Any Lutomirski <luto@xxxxxxxxxx>
> Cc: H. Peter Anvin <hpa@xxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> ---
>  arch/x86/include/asm/fsgsbase.h | 17 +++----
>  arch/x86/kernel/process_64.c    | 82 +++++++++++++++++++++++++++------
>  2 files changed, 75 insertions(+), 24 deletions(-)
>
> diff --git a/arch/x86/include/asm/fsgsbase.h b/arch/x86/include/asm/fsgsbase.h
> index b4d4509b786c..e500d771155f 100644
> --- a/arch/x86/include/asm/fsgsbase.h
> +++ b/arch/x86/include/asm/fsgsbase.h
> @@ -57,26 +57,23 @@ static __always_inline void wrgsbase(unsigned long 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;
> -}
> -
> +extern unsigned long x86_gsbase_read_cpu_inactive(void);
>  extern void x86_fsbase_write_cpu(unsigned long fsbase);
>  extern void x86_gsbase_write_cpu_inactive(unsigned long gsbase);
>
> diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
> index 31b4755369f0..fcf18046c3d6 100644
> --- a/arch/x86/kernel/process_64.c
> +++ b/arch/x86/kernel/process_64.c
> @@ -159,6 +159,36 @@ enum which_selector {
>         GS
>  };
>
> +/*
> + * Interrupts are disabled here. Out of line to be protected from kprobes.
> + */
> +static noinline __kprobes unsigned long rd_inactive_gsbase(void)
> +{
> +       unsigned long gsbase, flags;
> +
> +       local_irq_save(flags);
> +       native_swapgs();
> +       gsbase = rdgsbase();
> +       native_swapgs();
> +       local_irq_restore(flags);
> +
> +       return gsbase;
> +}

Please fold this into its only caller and make *that* noinline.

Also, this function, and its "write" equivalent, will access the
*active* gsbase.  So it either needs to be fixed for Xen PV or some
clear comment and careful auditing needs to be added to ensure that
it's not used on Xen PV.  Or it needs to be renamed
native_x86_fsgsbase_... and add paravirt hooks, since Xen PV allows a
very efficient but different implementation, I think.  The latter is
probably the right solution.

(Hi Xen people -- how does CR4.FSGSBASE work on Xen?  Is it always
set?  Never set?  Set only if the guest tries to set it?)

>  void x86_fsbase_write_cpu(unsigned long fsbase)
>  {
> -       /*
> -        * Set the selector to 0 as a notion, that the segment base is
> -        * overwritten, which will be checked for skipping the segment load
> -        * during context switch.
> -        */
> -       loadseg(FS, 0);
> -       wrmsrl(MSR_FS_BASE, fsbase);
> +       if (static_cpu_has(X86_FEATURE_FSGSBASE)) {
> +               wrfsbase(fsbase);
> +       } else {
> +               /*
> +                * Set the selector to 0 as a notion, that the segment base is
> +                * overwritten, which will be checked for skipping the 
> segment load
> +                * during context switch.
> +                */
> +               loadseg(FS, 0);
> +               wrmsrl(MSR_FS_BASE, fsbase);
> +       }
>  }
>
>  void x86_gsbase_write_cpu_inactive(unsigned long gsbase)
>  {
> -       /* Set the selector to 0 for the same reason as %fs above. */
> -       loadseg(GS, 0);
> -       wrmsrl(MSR_KERNEL_GS_BASE, gsbase);
> +       if (static_cpu_has(X86_FEATURE_FSGSBASE)) {
> +               wr_inactive_gsbase(gsbase);
> +       } else {
> +               /* Set the selector to 0 for the same reason as %fs above. */
> +               loadseg(GS, 0);
> +               wrmsrl(MSR_KERNEL_GS_BASE, gsbase);

I still don't get what this code is trying to do.  See other email.  I
think it will straight up crash the kernel on some CPUs, since writing
0 to %%gs will zero out the *active* base on some CPUs.

I think that, if you really want some fancy optimization for the
non-FSGSBASE case, you need to pull that out into the callers of these
helpers.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel