linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Andy Lutomirski <luto@kernel.org>,
	"Bae, Chang Seok" <chang.seok.bae@intel.com>,
	Boris Ostrovsky <boris.ostrovsky@oracle.com>,
	Juergen Gross <jgross@suse.com>,
	xen-devel <xen-devel@lists.xenproject.org>
Cc: "Ravi V. Shankar" <ravi.v.shankar@intel.com>,
	Andi Kleen <ak@linux.intel.com>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	LKML <linux-kernel@vger.kernel.org>,
	"Metzger, Markus T" <markus.t.metzger@intel.com>,
	"H. Peter Anvin" <hpa@zytor.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@kernel.org>
Subject: Re: [Xen-devel] [v3 04/12] x86/fsgsbase/64: Enable FSGSBASE instructions in the helper functions
Date: Wed, 24 Oct 2018 20:41:52 +0100	[thread overview]
Message-ID: <0d64fe9d-0cc3-5901-0d6f-4bcb94aa9ee4@citrix.com> (raw)
In-Reply-To: <CALCETrVcJ08c6yf6y3ruOA_Hx_7srkHw5iYfx1c2p8_ySiL2WA@mail.gmail.com>

On 24/10/18 20:16, Andy Lutomirski wrote:
> On Tue, Oct 23, 2018 at 11:43 AM 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.
>>
>> Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
>> Reviewed-by: Andi Kleen <ak@linux.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: Dave Hansen <dave.hansen@linux.intel.com>
>> ---
>>  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?)

FML.  Seriously - whoever put this code into the hypervisor in the past
did an atrocious job.  After some experimentation, you're going to be
sad and I'm declaring this borderline unusable.

Looks like Xen unconditionally enabled CR4.FSGSBASE if it is available. 
Therefore, PV guests can use the instructions, even if the bit is clear
in vCR4.

The CPUID bits are exposed to guests by default, and Xen will emulate
vCR4.FSGSBASE being set and cleared.

We don't however emulate swapgs (which is a cpl0 instruction).  The
guest gets handed a #GP[0] instead.

The Linux WRMSR PVop uses the set_segment_base() hypercall in instead of
going through the full wrmsr emulation path.

There is no equivalent get hypercall, so the only way I can see of
getting the value is to actually read MSR_KERNEL_GS_BASE and take the
full rdmsr emulation path.

~Andrew

  reply	other threads:[~2018-10-24 19:41 UTC|newest]

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-23 18:42 [v3 00/12] x86: Enable FSGSBASE instructions Chang S. Bae
2018-10-23 18:42 ` [v3 01/12] taint: Introduce a new taint flag (insecure) Chang S. Bae
2018-10-24 18:50   ` Andy Lutomirski
2018-10-23 18:42 ` [v3 02/12] x86/fsgsbase/64: Add 'unsafe_fsgsbase' to enable CR4.FSGSBASE Chang S. Bae
2018-10-24 18:51   ` Andy Lutomirski
2018-10-23 18:42 ` [v3 03/12] x86/fsgsbase/64: Add intrinsics/macros for FSGSBASE instructions Chang S. Bae
2018-10-24 18:53   ` Andy Lutomirski
2018-10-24 19:21     ` Andi Kleen
2018-10-25 23:14       ` Andy Lutomirski
2018-10-25 23:31         ` Linus Torvalds
2018-10-26  0:09           ` Andy Lutomirski
2018-10-23 18:42 ` [v3 04/12] x86/fsgsbase/64: Enable FSGSBASE instructions in the helper functions Chang S. Bae
2018-10-24 19:16   ` Andy Lutomirski
2018-10-24 19:41     ` Andrew Cooper [this message]
2018-10-25  6:09       ` [Xen-devel] " Juergen Gross
2018-10-25 23:08         ` Andrew Cooper
2018-10-25 23:11           ` Andy Lutomirski
2018-10-25 23:14             ` Andrew Cooper
2018-10-25  7:32     ` Bae, Chang Seok
2018-10-25 23:00       ` Andy Lutomirski
2018-10-25 23:03         ` Bae, Chang Seok
2018-10-25 23:16     ` Andy Lutomirski
2018-10-23 18:42 ` [v3 05/12] x86/fsgsbase/64: Preserve FS/GS state in __switch_to() if FSGSBASE is on Chang S. Bae
2018-10-24 19:21   ` Andy Lutomirski
2018-10-24 19:36     ` Bae, Chang Seok
2018-10-23 18:42 ` [v3 06/12] x86/fsgsbase/64: When copying a thread, use the FSGSBASE instructions if available Chang S. Bae
2018-10-23 18:42 ` [v3 07/12] x86/fsgsbase/64: Introduce the new FIND_PERCPU_BASE macro Chang S. Bae
2018-10-26  0:25   ` Andy Lutomirski
2018-10-26  0:59     ` Nadav Amit
2018-10-23 18:42 ` [v3 08/12] x86/fsgsbase/64: Use the per-CPU base as GSBASE at the paranoid_entry Chang S. Bae
2018-10-23 18:42 ` [v3 09/12] selftests/x86/fsgsbase: Test WRGSBASE Chang S. Bae
2018-10-23 18:42 ` [v3 10/12] x86/fsgsbase/64: Enable FSGSBASE by default and add a chicken bit Chang S. Bae
2018-10-23 18:42 ` [v3 11/12] x86/elf: Enumerate kernel FSGSBASE capability in AT_HWCAP2 Chang S. Bae
2018-10-23 18:42 ` [v3 12/12] x86/fsgsbase/64: Add documentation for FSGSBASE Chang S. Bae

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0d64fe9d-0cc3-5901-0d6f-4bcb94aa9ee4@citrix.com \
    --to=andrew.cooper3@citrix.com \
    --cc=ak@linux.intel.com \
    --cc=boris.ostrovsky@oracle.com \
    --cc=chang.seok.bae@intel.com \
    --cc=dave.hansen@linux.intel.com \
    --cc=hpa@zytor.com \
    --cc=jgross@suse.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=markus.t.metzger@intel.com \
    --cc=mingo@kernel.org \
    --cc=ravi.v.shankar@intel.com \
    --cc=tglx@linutronix.de \
    --cc=xen-devel@lists.xenproject.org \
    --subject='Re: [Xen-devel] [v3 04/12] x86/fsgsbase/64: Enable FSGSBASE instructions in the helper functions' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).