x86: Unify SYSENTER setup and add a 64-bit SYSENTER stack

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 <[email protected]>

---
 arch/x86/kernel/cpu/common.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

arch/x86/kernel/cpu/common.c +5 -7

index 922c5e0cea4c..9b3a43583f81 100644
@@ -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();

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.

Quoted 4 lines · On Thu, 23 Jul 2015 08:31:39 -0700 Andy Lutomirski <[email protected]> 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?

Quoted 3 lines · On Thu, 23 Jul 2015 08:31:40 -0700 Andy Lutomirski <[email protected]> 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.

arch/x86/kernel/cpu/common.c +12 -1

-	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

out:

arch/x86/kernel/cpu/common.c +1 -11

 	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);

Quoted 11 lines · Steven Rostedt

Quoted 3 lines · On Thu, 23 Jul 2015 08:31:40 -0700 Andy Lutomirski <[email protected]> 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?

Quoted 3 lines

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.

Quoted 21 lines · On Thu, 23 Jul 2015 08:31:41 -0700 Andy Lutomirski <[email protected]> wrote:

arch/x86/kernel/cpu/common.c +12 -1

-     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

arch/x86/kernel/cpu/common.c void enable_sep_cpu(void) · line 1024

Quoted 9 lines

      wrmsrl_safe(MSR_IA32_SYSENTER_CS, __KERNEL_CS);
      wrmsrl_safe(MSR_IA32_SYSENTER_ESP,
                  (unsigned long)tss +
                  offsetofend(struct tss_struct, SYSENTER_stack));

arch/x86/kernel/cpu/common.c +4 -0

+#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.

Quoted 16 lines · On Thu, 23 Jul 2015 09:04:38 -0700 Andy Lutomirski <[email protected]> wrote:

Quoted 14 lines · Steven Rostedt

Quoted 3 lines · On Thu, 23 Jul 2015 08:31:40 -0700 Andy Lutomirski <[email protected]> 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.

Quoted 4 lines

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.

Quoted 11 lines · Steven Rostedt

Quoted 4 lines · On Thu, 23 Jul 2015 08:31:39 -0700 Andy Lutomirski <[email protected]> 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.

Quoted 18 lines · Andy Lutomirski

Quoted 11 lines · Steven Rostedt

Quoted 4 lines · On Thu, 23 Jul 2015 08:31:39 -0700 Andy Lutomirski <[email protected]> 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.

Quoted 3 lines

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.

Quoted 23 lines · Brian Gerst

Quoted 21 lines · Andy Lutomirski

Quoted 11 lines · Steven Rostedt

Quoted 4 lines · On Thu, 23 Jul 2015 08:31:39 -0700 Andy Lutomirski <[email protected]> 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.

Quoted 3 lines

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.

Quoted 28 lines · Andy Lutomirski

Quoted 26 lines · Brian Gerst

Quoted 13 lines · Andy Lutomirski

Quoted 11 lines · Steven Rostedt

Quoted 4 lines · On Thu, 23 Jul 2015 08:31:39 -0700 Andy Lutomirski <[email protected]> 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?

Quoted 8 lines

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.

Quoted 2 lines

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

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.