Re: [RESEND PATCH v6 04/12] x86/fsgsbase/64: Enable FSGSBASE instructions in the helper functions

[Thomas Gleixner]

On Fri, 15 Mar 2019, Chang S. Bae 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.

This smells fishy and looking at the end result of this series just
confirms it. This ends up being a mixture of SWAPGS and FSGSBASE usage and
as already pointed out in the other reply, it causes inconsistencies.

Let's look at the big picture.

For both variants GS needs to be swapped on kernel entry and on kernel
exit.

1) SWAPGS

   MSR_KERNEL_GS_BASE contains the user space GS when running in the kernel
   and the kernel GS when running in user space.

   SWAPGS is used to swap the content of GS and MSR_KERNEL_GS_BASE on the
   transitions from and to user space.

   On context switch MSR_KERNEL_GS_BASE has to be updated when switching
   between processes.

   User space cannot change GS other than through the PRCTL which updates
   MSR_KERNEL_GS_BASE.

2) FSGSBASE

   User space can set GS without kernel interaction.

   So on user space to kernel space transitions swapping in kernel GS should
   simply do:

     userGS = RDGSBASE()
     WRGSBASE(kernelGS)

   and on the way out:

     WRGSBASE(userGS)

   instead of SWAPGS all over the place.

   userGS is stored in thread_struct, except for the few paranoid
   exceptions which return straight to user space, e.g. NMI. Those can just
   keep it on stack or in a register.

   Context switch does not have to do anything at all vs. GS because
   thread_struct contains the correct value already.

   The PRCTL is straight forward to support. Instead of fiddling with
   MSR_KERNEL_GS_BASE it just updates thread struct.

   I don't see how that's NOT going to be an advantage and I don't see
   either how this seems to cause more cycles for save and restore.

Making it consistently FSGSBASE avoids especially this piece of art in the
context switch path:

        local_irq_save(flags);
        native_swapgs();
        gsbase = rdgsbase();
        native_swapgs();
        local_irq_restore(flags);

along with it's write counterpart.

The whole point of FSGSBASE support is performance, right?

So can please someone explain why having the following in the context
switch path when it can be completely avoided is enhancing performance:

  - 4 x SWAPGS
  - 1 x RDMSR
  - 1 x WRMSR
  - 2 x local_irq_save()
  - 2 x local_irq_restore()

Of course the local_irq_save/restore() pairs are utterly pointless because
switch_to() runs with interrupts disabled already.

SWAPGS instead needs:

  1 x WRMSR

and nothing else.

So trading the single WRMSR against the above in the context switch path is
gaining performance, right?

The only thing which gains performance is user space switching GS. And this
user space performance gain is achieved by:

  - Inconsistent and fragile code with a guarantee for subtle and hard to
    diagnose bugs

  - Pointless overhead in the context switch code

Sorry, not going to happen ever.

Get your act together and make this consistent. Either SWAPGS or FSGSBASE,
but not a mix of it.

Thanks,

	tglx