[PATCH 0/5] x86: Unify SYSENTER setup and add a 64-bit SYSENTER stack

[PATCH 0/5] x86: Unify SYSENTER setup and add a 64-bit SYSENTER stack

[Andy Lutomirski]

These are boring patches.  They're a cleanup, and something like them is
mandatory if we want to wean the 64-bit #DB handler off IST.

The latter will be useful if we want to eliminate the IST reprogramming
that we during NMIs unless we ban #DB outright inside NMIs.  Even if we
ban #DB inside NMI, it'll still be nice to get reduce IST usage to the
bare minimum.

Andy Lutomirski (5):
  x86/entry/32: Clean up enable_sep_cpu to prepare for 64-bit merge
  x86/entry/64, entry: Set up a valid sysenter stack and prepare for
    32-bit merge
  x86/entry: Merge 32-bit and 64-bit sysenter setup code
  x86/entry: Only allocate space for SYSENTER_stack if needed
  x86/entry: Replace SWAPGS_UNSAFE_STACK with SWAPGS in
    entry_SYSENTER_compat

 arch/x86/entry/entry_64_compat.S |  2 +-
 arch/x86/include/asm/processor.h |  2 ++
 arch/x86/kernel/cpu/common.c     | 44 ++++++++++++++++++++++------------------
 3 files changed, 27 insertions(+), 21 deletions(-)

-- 
2.4.3

[PATCH 1/5] x86/entry/32: Clean up enable_sep_cpu to prepare for 64-bit merge

[Andy Lutomirski]

Switch from wrmsr to wrmsrl_safe to prepare to merge the 32-bit and
64-bit code, and use __KERNEL_CS explicitly to initialize
MSR_IA32_SYSENTER_CS.  While we're at it, tweak the whitespace a
bit.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/kernel/cpu/common.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 922c5e0cea4c..9b3a43583f81 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1001,15 +1001,13 @@ void enable_sep_cpu(void)
 	 * We cache MSR_IA32_SYSENTER_CS's value in the TSS's ss1 field --
 	 * see the big comment in struct x86_hw_tss's definition.
 	 */
-
 	tss->x86_tss.ss1 = __KERNEL_CS;
-	wrmsr(MSR_IA32_SYSENTER_CS, tss->x86_tss.ss1, 0);
-
-	wrmsr(MSR_IA32_SYSENTER_ESP,
-	      (unsigned long)tss + offsetofend(struct tss_struct, SYSENTER_stack),
-	      0);
 
-	wrmsr(MSR_IA32_SYSENTER_EIP, (unsigned long)entry_SYSENTER_32, 0);
+	wrmsrl_safe(MSR_IA32_SYSENTER_CS, __KERNEL_CS);
+	wrmsrl_safe(MSR_IA32_SYSENTER_ESP,
+		    (unsigned long)tss +
+		    offsetofend(struct tss_struct, SYSENTER_stack));
+	wrmsrl_safe(MSR_IA32_SYSENTER_EIP, (unsigned long)entry_SYSENTER_32);
 
 out:
 	put_cpu();
-- 
2.4.3

Re: [PATCH 1/5] x86/entry/32: Clean up enable_sep_cpu to prepare for 64-bit merge

[Steven Rostedt]

On Thu, 23 Jul 2015 08:31:39 -0700
Andy Lutomirski <luto@kernel.org> wrote:

> Switch from wrmsr to wrmsrl_safe to prepare to merge the 32-bit and
> 64-bit code, and use __KERNEL_CS explicitly to initialize
> MSR_IA32_SYSENTER_CS.  While we're at it, tweak the whitespace a
> bit.

Saying "prepare to merge the 32-bit and 64-bit" isn't that informative
to why this patch is needed. Can you please describe in the change log
what can fault with the wrmsr call, when we do the merge?

Thanks!

-- Steve


> 
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
>  arch/x86/kernel/cpu/common.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 922c5e0cea4c..9b3a43583f81 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1001,15 +1001,13 @@ void enable_sep_cpu(void)
>  	 * We cache MSR_IA32_SYSENTER_CS's value in the TSS's ss1 field --
>  	 * see the big comment in struct x86_hw_tss's definition.
>  	 */
> -
>  	tss->x86_tss.ss1 = __KERNEL_CS;
> -	wrmsr(MSR_IA32_SYSENTER_CS, tss->x86_tss.ss1, 0);
> -
> -	wrmsr(MSR_IA32_SYSENTER_ESP,
> -	      (unsigned long)tss + offsetofend(struct tss_struct, SYSENTER_stack),
> -	      0);
>  
> -	wrmsr(MSR_IA32_SYSENTER_EIP, (unsigned long)entry_SYSENTER_32, 0);
> +	wrmsrl_safe(MSR_IA32_SYSENTER_CS, __KERNEL_CS);
> +	wrmsrl_safe(MSR_IA32_SYSENTER_ESP,
> +		    (unsigned long)tss +
> +		    offsetofend(struct tss_struct, SYSENTER_stack));
> +	wrmsrl_safe(MSR_IA32_SYSENTER_EIP, (unsigned long)entry_SYSENTER_32);
>  
>  out:
>  	put_cpu();

Re: [PATCH 1/5] x86/entry/32: Clean up enable_sep_cpu to prepare for 64-bit merge

[Andy Lutomirski]

On Thu, Jul 23, 2015 at 8:56 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 23 Jul 2015 08:31:39 -0700
> Andy Lutomirski <luto@kernel.org> wrote:
>
>> Switch from wrmsr to wrmsrl_safe to prepare to merge the 32-bit and
>> 64-bit code, and use __KERNEL_CS explicitly to initialize
>> MSR_IA32_SYSENTER_CS.  While we're at it, tweak the whitespace a
>> bit.
>
> Saying "prepare to merge the 32-bit and 64-bit" isn't that informative
> to why this patch is needed. Can you please describe in the change log
> what can fault with the wrmsr call, when we do the merge?

Oh...

Nothing can fault in the wrmsr as far as I know, but maybe there's a
CPU out there that advertises SEP but doesn't have the MSRs.

Ingo, Thomas, want a v2 that explains that the wrmsr_safe is there for
consistency but that I don't know why 32bit does it?  Another option
would be to remove the _safe from the 32-bit version.

--Andy

Re: [PATCH 1/5] x86/entry/32: Clean up enable_sep_cpu to prepare for 64-bit merge

[Brian Gerst]

On Thu, Jul 23, 2015 at 12:02 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Thu, Jul 23, 2015 at 8:56 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> On Thu, 23 Jul 2015 08:31:39 -0700
>> Andy Lutomirski <luto@kernel.org> wrote:
>>
>>> Switch from wrmsr to wrmsrl_safe to prepare to merge the 32-bit and
>>> 64-bit code, and use __KERNEL_CS explicitly to initialize
>>> MSR_IA32_SYSENTER_CS.  While we're at it, tweak the whitespace a
>>> bit.
>>
>> Saying "prepare to merge the 32-bit and 64-bit" isn't that informative
>> to why this patch is needed. Can you please describe in the change log
>> what can fault with the wrmsr call, when we do the merge?
>
> Oh...
>
> Nothing can fault in the wrmsr as far as I know, but maybe there's a
> CPU out there that advertises SEP but doesn't have the MSRs.
>
> Ingo, Thomas, want a v2 that explains that the wrmsr_safe is there for
> consistency but that I don't know why 32bit does it?  Another option
> would be to remove the _safe from the 32-bit version.

There is an erratum with SEP being advertised but not available on
early P6 models, but we already check for that in the cpu setup code.
There shouldn't be a problem with a fault.

--
Brian Gerst

Re: [PATCH 1/5] x86/entry/32: Clean up enable_sep_cpu to prepare for 64-bit merge

[Andy Lutomirski]

On Thu, Jul 23, 2015 at 10:27 AM, Brian Gerst <brgerst@gmail.com> wrote:
> On Thu, Jul 23, 2015 at 12:02 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Thu, Jul 23, 2015 at 8:56 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>>> On Thu, 23 Jul 2015 08:31:39 -0700
>>> Andy Lutomirski <luto@kernel.org> wrote:
>>>
>>>> Switch from wrmsr to wrmsrl_safe to prepare to merge the 32-bit and
>>>> 64-bit code, and use __KERNEL_CS explicitly to initialize
>>>> MSR_IA32_SYSENTER_CS.  While we're at it, tweak the whitespace a
>>>> bit.
>>>
>>> Saying "prepare to merge the 32-bit and 64-bit" isn't that informative
>>> to why this patch is needed. Can you please describe in the change log
>>> what can fault with the wrmsr call, when we do the merge?
>>
>> Oh...
>>
>> Nothing can fault in the wrmsr as far as I know, but maybe there's a
>> CPU out there that advertises SEP but doesn't have the MSRs.
>>
>> Ingo, Thomas, want a v2 that explains that the wrmsr_safe is there for
>> consistency but that I don't know why 32bit does it?  Another option
>> would be to remove the _safe from the 32-bit version.
>
> There is an erratum with SEP being advertised but not available on
> early P6 models, but we already check for that in the cpu setup code.
> There shouldn't be a problem with a fault.

I can make a v2 that gets rid of the _safe.

--Andy

Re: [PATCH 1/5] x86/entry/32: Clean up enable_sep_cpu to prepare for 64-bit merge

[Andy Lutomirski]

On Thu, Jul 23, 2015 at 11:01 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Thu, Jul 23, 2015 at 10:27 AM, Brian Gerst <brgerst@gmail.com> wrote:
>> On Thu, Jul 23, 2015 at 12:02 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>> On Thu, Jul 23, 2015 at 8:56 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>>>> On Thu, 23 Jul 2015 08:31:39 -0700
>>>> Andy Lutomirski <luto@kernel.org> wrote:
>>>>
>>>>> Switch from wrmsr to wrmsrl_safe to prepare to merge the 32-bit and
>>>>> 64-bit code, and use __KERNEL_CS explicitly to initialize
>>>>> MSR_IA32_SYSENTER_CS.  While we're at it, tweak the whitespace a
>>>>> bit.
>>>>
>>>> Saying "prepare to merge the 32-bit and 64-bit" isn't that informative
>>>> to why this patch is needed. Can you please describe in the change log
>>>> what can fault with the wrmsr call, when we do the merge?
>>>
>>> Oh...
>>>
>>> Nothing can fault in the wrmsr as far as I know, but maybe there's a
>>> CPU out there that advertises SEP but doesn't have the MSRs.
>>>
>>> Ingo, Thomas, want a v2 that explains that the wrmsr_safe is there for
>>> consistency but that I don't know why 32bit does it?  Another option
>>> would be to remove the _safe from the 32-bit version.
>>
>> There is an erratum with SEP being advertised but not available on
>> early P6 models, but we already check for that in the cpu setup code.
>> There shouldn't be a problem with a fault.
>
> I can make a v2 that gets rid of the _safe.
>

Ugh.  wrmsrl_safe is a function but wrmsrl isn't.  I'll fix that and send a v2.

Gah, macros.

--Andy

Re: [PATCH 1/5] x86/entry/32: Clean up enable_sep_cpu to prepare for 64-bit merge

[Andy Lutomirski]

On Thu, Jul 23, 2015 at 11:33 AM, Andy Lutomirski <luto@amacapital.net> wrote:
> On Thu, Jul 23, 2015 at 11:01 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Thu, Jul 23, 2015 at 10:27 AM, Brian Gerst <brgerst@gmail.com> wrote:
>>> On Thu, Jul 23, 2015 at 12:02 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>>> On Thu, Jul 23, 2015 at 8:56 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>>>>> On Thu, 23 Jul 2015 08:31:39 -0700
>>>>> Andy Lutomirski <luto@kernel.org> wrote:
>>>>>
>>>>>> Switch from wrmsr to wrmsrl_safe to prepare to merge the 32-bit and
>>>>>> 64-bit code, and use __KERNEL_CS explicitly to initialize
>>>>>> MSR_IA32_SYSENTER_CS.  While we're at it, tweak the whitespace a
>>>>>> bit.
>>>>>
>>>>> Saying "prepare to merge the 32-bit and 64-bit" isn't that informative
>>>>> to why this patch is needed. Can you please describe in the change log
>>>>> what can fault with the wrmsr call, when we do the merge?
>>>>
>>>> Oh...
>>>>
>>>> Nothing can fault in the wrmsr as far as I know, but maybe there's a
>>>> CPU out there that advertises SEP but doesn't have the MSRs.
>>>>
>>>> Ingo, Thomas, want a v2 that explains that the wrmsr_safe is there for
>>>> consistency but that I don't know why 32bit does it?  Another option
>>>> would be to remove the _safe from the 32-bit version.
>>>
>>> There is an erratum with SEP being advertised but not available on
>>> early P6 models, but we already check for that in the cpu setup code.
>>> There shouldn't be a problem with a fault.
>>
>> I can make a v2 that gets rid of the _safe.
>>
>
> Ugh.  wrmsrl_safe is a function but wrmsrl isn't.  I'll fix that and send a v2.
>
> Gah, macros.

On third thought, no v2.  It's *64-bit* CPUs that might want the
_safe.  We aren't currently checking SEP.  Maybe there's a non-SEP AMD
CPU and we never noticed because SEP was never useful on AMD 64-bit
CPUs.

I'll send a followup.

>
> --Andy



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH 1/5] x86/entry/32: Clean up enable_sep_cpu to prepare for 64-bit merge

[Linus Torvalds]

On Thu, Jul 23, 2015 at 11:42 AM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> On third thought, no v2.  It's *64-bit* CPUs that might want the
> _safe.  We aren't currently checking SEP.  Maybe there's a non-SEP AMD
> CPU and we never noticed because SEP was never useful on AMD 64-bit
> CPUs.

I don't see the downside of using the safe version unconditionally. No
need to worry about any corner cases.

                  Linus

[PATCH 2/5] x86/entry/64, entry: Set up a valid sysenter stack and prepare for 32-bit merge

[Andy Lutomirski]

Oddly, 64-bit kernels already allocate a percpu sysenter stack, but
they don't enable it.  Enable the stack and tweak the rest of the
sysenter setup code to be similar to the 32-bit version.

This eliminates the only place in the kernel in which TF could be set
without a valid stack.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/kernel/cpu/common.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 9b3a43583f81..fba86ed46aa6 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1193,9 +1193,11 @@ void syscall_init(void)
 	 * This does not cause SYSENTER to jump to the wrong location, because
 	 * AMD doesn't allow SYSENTER in long mode (either 32- or 64-bit).
 	 */
-	wrmsrl_safe(MSR_IA32_SYSENTER_CS, (u64)__KERNEL_CS);
-	wrmsrl_safe(MSR_IA32_SYSENTER_ESP, 0ULL);
-	wrmsrl_safe(MSR_IA32_SYSENTER_EIP, (u64)entry_SYSENTER_compat);
+	wrmsrl_safe(MSR_IA32_SYSENTER_CS, __KERNEL_CS);
+	wrmsrl_safe(MSR_IA32_SYSENTER_ESP,
+		    (unsigned long)&per_cpu(cpu_tss, smp_processor_id()) +
+		    offsetofend(struct tss_struct, SYSENTER_stack));
+	wrmsrl_safe(MSR_IA32_SYSENTER_EIP, (unsigned long)entry_SYSENTER_compat);
 #else
 	wrmsrl(MSR_CSTAR, ignore_sysret);
 	wrmsrl_safe(MSR_IA32_SYSENTER_CS, (u64)GDT_ENTRY_INVALID_SEG);
-- 
2.4.3

Re: [PATCH 2/5] x86/entry/64, entry: Set up a valid sysenter stack and prepare for 32-bit merge

[Steven Rostedt]

On Thu, 23 Jul 2015 08:31:40 -0700
Andy Lutomirski <luto@kernel.org> wrote:

> Oddly, 64-bit kernels already allocate a percpu sysenter stack, but
> they don't enable it.  Enable the stack and tweak the rest of the
> sysenter setup code to be similar to the 32-bit version.

I'm curious. Did you do any investigation into the 64bit code to why it
wasn't set? Do you know if that was just overlooked with some of the
merging between i386 and x86_64 systems?

I'm not asking you to do it if you have not, but if you have, I think
it would be more comforting to know that it was just overlooked than
there being some other subtle reason.

-- Steve

> 
> This eliminates the only place in the kernel in which TF could be set
> without a valid stack.
> 

Re: [PATCH 2/5] x86/entry/64, entry: Set up a valid sysenter stack and prepare for 32-bit merge

[Andy Lutomirski]

On Thu, Jul 23, 2015 at 9:01 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 23 Jul 2015 08:31:40 -0700
> Andy Lutomirski <luto@kernel.org> wrote:
>
>> Oddly, 64-bit kernels already allocate a percpu sysenter stack, but
>> they don't enable it.  Enable the stack and tweak the rest of the
>> sysenter setup code to be similar to the 32-bit version.
>
> I'm curious. Did you do any investigation into the 64bit code to why it
> wasn't set? Do you know if that was just overlooked with some of the
> merging between i386 and x86_64 systems?
>
> I'm not asking you to do it if you have not, but if you have, I think
> it would be more comforting to know that it was just overlooked than
> there being some other subtle reason.

I think it's just that it was never necessary.  #DB has always used
IST on x86_64 (IIRC it was like that in the very first commit), so the
sysenter stack was never actually referenced.


--Andy

Re: [PATCH 2/5] x86/entry/64, entry: Set up a valid sysenter stack and prepare for 32-bit merge

[Steven Rostedt]

On Thu, 23 Jul 2015 09:04:38 -0700
Andy Lutomirski <luto@amacapital.net> wrote:

> On Thu, Jul 23, 2015 at 9:01 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> > On Thu, 23 Jul 2015 08:31:40 -0700
> > Andy Lutomirski <luto@kernel.org> wrote:
> >
> >> Oddly, 64-bit kernels already allocate a percpu sysenter stack, but
> >> they don't enable it.  Enable the stack and tweak the rest of the
> >> sysenter setup code to be similar to the 32-bit version.
> >
> > I'm curious. Did you do any investigation into the 64bit code to why it
> > wasn't set? Do you know if that was just overlooked with some of the
> > merging between i386 and x86_64 systems?
> >
> > I'm not asking you to do it if you have not, but if you have, I think
> > it would be more comforting to know that it was just overlooked than
> > there being some other subtle reason.
> 
> I think it's just that it was never necessary.  #DB has always used
> IST on x86_64 (IIRC it was like that in the very first commit), so the
> sysenter stack was never actually referenced.
> 

Hmm, should this be commented? If it is never referenced, I could
imagine that would add some confusion to future reviewers of this code.

-- Steve

Re: [PATCH 2/5] x86/entry/64, entry: Set up a valid sysenter stack and prepare for 32-bit merge

[Andy Lutomirski]

On Thu, Jul 23, 2015 at 9:10 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 23 Jul 2015 09:04:38 -0700
> Andy Lutomirski <luto@amacapital.net> wrote:
>
>> On Thu, Jul 23, 2015 at 9:01 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
>> > On Thu, 23 Jul 2015 08:31:40 -0700
>> > Andy Lutomirski <luto@kernel.org> wrote:
>> >
>> >> Oddly, 64-bit kernels already allocate a percpu sysenter stack, but
>> >> they don't enable it.  Enable the stack and tweak the rest of the
>> >> sysenter setup code to be similar to the 32-bit version.
>> >
>> > I'm curious. Did you do any investigation into the 64bit code to why it
>> > wasn't set? Do you know if that was just overlooked with some of the
>> > merging between i386 and x86_64 systems?
>> >
>> > I'm not asking you to do it if you have not, but if you have, I think
>> > it would be more comforting to know that it was just overlooked than
>> > there being some other subtle reason.
>>
>> I think it's just that it was never necessary.  #DB has always used
>> IST on x86_64 (IIRC it was like that in the very first commit), so the
>> sysenter stack was never actually referenced.
>>
>
> Hmm, should this be commented? If it is never referenced, I could
> imagine that would add some confusion to future reviewers of this code.
>

It is referenced by the SWAPGS_UNSAFE_STACK -> SWAPGS thing, and I'm
planning on referencing it more by removing #DB from the debug stack
soon (patches written but not tested), so the window of confusion
should be short.

--Andy

Re: [PATCH 2/5] x86/entry/64, entry: Set up a valid sysenter stack and prepare for 32-bit merge

[Steven Rostedt]

On Thu, 23 Jul 2015 09:46:45 -0700
Andy Lutomirski <luto@amacapital.net> wrote:
 
> It is referenced by the SWAPGS_UNSAFE_STACK -> SWAPGS thing, and I'm
> planning on referencing it more by removing #DB from the debug stack
> soon (patches written but not tested), so the window of confusion
> should be short.

Fair enough.

-- Steve

[PATCH 3/5] x86/entry: Merge 32-bit and 64-bit sysenter setup code

[Andy Lutomirski]

This should have no functional effect at all.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/kernel/cpu/common.c | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index fba86ed46aa6..6eb8b5a28a05 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -982,10 +982,9 @@ static void identify_cpu(struct cpuinfo_x86 *c)
 }
 
 /*
- * Set up the CPU state needed to execute SYSENTER/SYSEXIT instructions
- * on 32-bit kernels:
+ * Set up the CPU state needed to execute SYSENTER/SYSEXIT instructions:
  */
-#ifdef CONFIG_X86_32
+#if defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION)
 void enable_sep_cpu(void)
 {
 	struct tss_struct *tss;
@@ -994,20 +993,35 @@ void enable_sep_cpu(void)
 	cpu = get_cpu();
 	tss = &per_cpu(cpu_tss, cpu);
 
-	if (!boot_cpu_has(X86_FEATURE_SEP))
+	/*
+	 * On 64-bit CPUs, enable SEP unconditionally.  On Intel CPUs,
+	 * it works and we use it.  On AMD CPUs, the MSRs exist but EIP
+	 * is truncated to 32 bits.  This doesn't matter because AMD
+	 * CPUs disallow SYSENTER in long mode.  If AMD ever decides to
+	 * support SYSENTER, then they'll have to fix the truncation
+	 * issue, and this code will work as-is.
+	 */
+
+	if (IS_ENABLED(CONFIG_X86_32) && !boot_cpu_has(X86_FEATURE_SEP))
 		goto out;
 
+#ifdef CONFIG_X86_32
 	/*
 	 * We cache MSR_IA32_SYSENTER_CS's value in the TSS's ss1 field --
 	 * see the big comment in struct x86_hw_tss's definition.
 	 */
 	tss->x86_tss.ss1 = __KERNEL_CS;
+#endif
 
 	wrmsrl_safe(MSR_IA32_SYSENTER_CS, __KERNEL_CS);
 	wrmsrl_safe(MSR_IA32_SYSENTER_ESP,
 		    (unsigned long)tss +
 		    offsetofend(struct tss_struct, SYSENTER_stack));
+#ifdef CONFIG_X86_32
 	wrmsrl_safe(MSR_IA32_SYSENTER_EIP, (unsigned long)entry_SYSENTER_32);
+#else
+	wrmsrl_safe(MSR_IA32_SYSENTER_EIP, (unsigned long)entry_SYSENTER_compat);
+#endif
 
 out:
 	put_cpu();
@@ -1187,17 +1201,7 @@ void syscall_init(void)
 
 #ifdef CONFIG_IA32_EMULATION
 	wrmsrl(MSR_CSTAR, entry_SYSCALL_compat);
-	/*
-	 * This only works on Intel CPUs.
-	 * On AMD CPUs these MSRs are 32-bit, CPU truncates MSR_IA32_SYSENTER_EIP.
-	 * This does not cause SYSENTER to jump to the wrong location, because
-	 * AMD doesn't allow SYSENTER in long mode (either 32- or 64-bit).
-	 */
-	wrmsrl_safe(MSR_IA32_SYSENTER_CS, __KERNEL_CS);
-	wrmsrl_safe(MSR_IA32_SYSENTER_ESP,
-		    (unsigned long)&per_cpu(cpu_tss, smp_processor_id()) +
-		    offsetofend(struct tss_struct, SYSENTER_stack));
-	wrmsrl_safe(MSR_IA32_SYSENTER_EIP, (unsigned long)entry_SYSENTER_compat);
+	enable_sep_cpu();
 #else
 	wrmsrl(MSR_CSTAR, ignore_sysret);
 	wrmsrl_safe(MSR_IA32_SYSENTER_CS, (u64)GDT_ENTRY_INVALID_SEG);
-- 
2.4.3

Re: [PATCH 3/5] x86/entry: Merge 32-bit and 64-bit sysenter setup code

[Steven Rostedt]

On Thu, 23 Jul 2015 08:31:41 -0700
Andy Lutomirski <luto@kernel.org> wrote:


> -	if (!boot_cpu_has(X86_FEATURE_SEP))
> +	/*
> +	 * On 64-bit CPUs, enable SEP unconditionally.  On Intel CPUs,
> +	 * it works and we use it.  On AMD CPUs, the MSRs exist but EIP
> +	 * is truncated to 32 bits.  This doesn't matter because AMD
> +	 * CPUs disallow SYSENTER in long mode.  If AMD ever decides to
> +	 * support SYSENTER, then they'll have to fix the truncation
> +	 * issue, and this code will work as-is.
> +	 */
> +
> +	if (IS_ENABLED(CONFIG_X86_32) && !boot_cpu_has(X86_FEATURE_SEP))
>  		goto out;
>  
> +#ifdef CONFIG_X86_32
>  	/*
>  	 * We cache MSR_IA32_SYSENTER_CS's value in the TSS's ss1 field --
>  	 * see the big comment in struct x86_hw_tss's definition.
>  	 */
>  	tss->x86_tss.ss1 = __KERNEL_CS;
> +#endif
>  
>  	wrmsrl_safe(MSR_IA32_SYSENTER_CS, __KERNEL_CS);
>  	wrmsrl_safe(MSR_IA32_SYSENTER_ESP,
>  		    (unsigned long)tss +
>  		    offsetofend(struct tss_struct, SYSENTER_stack));
> +#ifdef CONFIG_X86_32
>  	wrmsrl_safe(MSR_IA32_SYSENTER_EIP, (unsigned long)entry_SYSENTER_32);
> +#else
> +	wrmsrl_safe(MSR_IA32_SYSENTER_EIP, (unsigned long)entry_SYSENTER_compat);
> +#endif

As an additional clean up, what impact would we have to just rename
entry_SYSENTER_compat to entry_SYSENTER_32 on x86_64? It would remove
the need for the above #ifdef logic.

-- Steve


>  
>  out:
>  	put_cpu();
> @@ -1187,17 +1201,7 @@ void syscall_init(void)
>  
>  #ifdef CONFIG_IA32_EMULATION
>  	wrmsrl(MSR_CSTAR, entry_SYSCALL_compat);
> -	/*
> -	 * This only works on Intel CPUs.
> -	 * On AMD CPUs these MSRs are 32-bit, CPU truncates MSR_IA32_SYSENTER_EIP.
> -	 * This does not cause SYSENTER to jump to the wrong location, because
> -	 * AMD doesn't allow SYSENTER in long mode (either 32- or 64-bit).
> -	 */
> -	wrmsrl_safe(MSR_IA32_SYSENTER_CS, __KERNEL_CS);
> -	wrmsrl_safe(MSR_IA32_SYSENTER_ESP,
> -		    (unsigned long)&per_cpu(cpu_tss, smp_processor_id()) +
> -		    offsetofend(struct tss_struct, SYSENTER_stack));
> -	wrmsrl_safe(MSR_IA32_SYSENTER_EIP, (unsigned long)entry_SYSENTER_compat);
> +	enable_sep_cpu();
>  #else
>  	wrmsrl(MSR_CSTAR, ignore_sysret);
>  	wrmsrl_safe(MSR_IA32_SYSENTER_CS, (u64)GDT_ENTRY_INVALID_SEG);

Re: [PATCH 3/5] x86/entry: Merge 32-bit and 64-bit sysenter setup code

[Andy Lutomirski]

On Thu, Jul 23, 2015 at 9:06 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 23 Jul 2015 08:31:41 -0700
> Andy Lutomirski <luto@kernel.org> wrote:
>
>
>> -     if (!boot_cpu_has(X86_FEATURE_SEP))
>> +     /*
>> +      * On 64-bit CPUs, enable SEP unconditionally.  On Intel CPUs,
>> +      * it works and we use it.  On AMD CPUs, the MSRs exist but EIP
>> +      * is truncated to 32 bits.  This doesn't matter because AMD
>> +      * CPUs disallow SYSENTER in long mode.  If AMD ever decides to
>> +      * support SYSENTER, then they'll have to fix the truncation
>> +      * issue, and this code will work as-is.
>> +      */
>> +
>> +     if (IS_ENABLED(CONFIG_X86_32) && !boot_cpu_has(X86_FEATURE_SEP))
>>               goto out;
>>
>> +#ifdef CONFIG_X86_32
>>       /*
>>        * We cache MSR_IA32_SYSENTER_CS's value in the TSS's ss1 field --
>>        * see the big comment in struct x86_hw_tss's definition.
>>        */
>>       tss->x86_tss.ss1 = __KERNEL_CS;
>> +#endif
>>
>>       wrmsrl_safe(MSR_IA32_SYSENTER_CS, __KERNEL_CS);
>>       wrmsrl_safe(MSR_IA32_SYSENTER_ESP,
>>                   (unsigned long)tss +
>>                   offsetofend(struct tss_struct, SYSENTER_stack));
>> +#ifdef CONFIG_X86_32
>>       wrmsrl_safe(MSR_IA32_SYSENTER_EIP, (unsigned long)entry_SYSENTER_32);
>> +#else
>> +     wrmsrl_safe(MSR_IA32_SYSENTER_EIP, (unsigned long)entry_SYSENTER_compat);
>> +#endif
>
> As an additional clean up, what impact would we have to just rename
> entry_SYSENTER_compat to entry_SYSENTER_32 on x86_64? It would remove
> the need for the above #ifdef logic.

I asked Ingo that at one point.  I bet that, if we can get the ABIs to
match, then we can do that.

--Andy

[PATCH 4/5] x86/entry: Only allocate space for SYSENTER_stack if needed

[Andy Lutomirski]

The SYSENTER stack is only used in configurations that support 32-bit
code and, hence, SYSENTER.  Remove it in 64-bit non-compat configurations.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/include/asm/processor.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index befc1341f110..786f50e4bd90 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -298,10 +298,12 @@ struct tss_struct {
 	 */
 	unsigned long		io_bitmap[IO_BITMAP_LONGS + 1];
 
+#if defined(CONFIG_X86_32) || defined(CONFIG_IA32_EMULATION)
 	/*
 	 * Space for the temporary SYSENTER stack:
 	 */
 	unsigned long		SYSENTER_stack[64];
+#endif
 
 } ____cacheline_aligned;
 
-- 
2.4.3

[PATCH 5/5] x86/entry: Replace SWAPGS_UNSAFE_STACK with SWAPGS in entry_SYSENTER_compat

[Andy Lutomirski]

Now that we have a valid stack between SYSENTER and SWAPGS, let
paravirt use it.

Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
 arch/x86/entry/entry_64_compat.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/entry/entry_64_compat.S b/arch/x86/entry/entry_64_compat.S
index d7571532e7ce..35ee218e8d4d 100644
--- a/arch/x86/entry/entry_64_compat.S
+++ b/arch/x86/entry/entry_64_compat.S
@@ -63,7 +63,7 @@ ENTRY(entry_SYSENTER_compat)
 	 * We do not frame this tiny irq-off block with TRACE_IRQS_OFF/ON,
 	 * it is too small to ever cause noticeable irq latency.
 	 */
-	SWAPGS_UNSAFE_STACK
+	SWAPGS
 	movq	PER_CPU_VAR(cpu_current_top_of_stack), %rsp
 	ENABLE_INTERRUPTS(CLBR_NONE)
 
-- 
2.4.3