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
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(-)
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
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 <[email protected]>
---
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
This should have no functional effect at all.
Signed-off-by: Andy Lutomirski <[email protected]>
---
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
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 <[email protected]>
---
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
Now that we have a valid stack between SYSENTER and SWAPGS, let
paravirt use it.
Signed-off-by: Andy Lutomirski <[email protected]>
---
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
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?
Thanks!
-- Steve
>
> Signed-off-by: Andy Lutomirski <[email protected]>
> ---
> 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();
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.
-- Steve
>
> This eliminates the only place in the kernel in which TF could be set
> without a valid stack.
>
On Thu, Jul 23, 2015 at 8:56 AM, Steven Rostedt <[email protected]> wrote:
> 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.
--Andy
On Thu, Jul 23, 2015 at 9:01 AM, Steven Rostedt <[email protected]> wrote:
> 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.
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
On Thu, 23 Jul 2015 08:31:41 -0700
Andy Lutomirski <[email protected]> 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);
On Thu, 23 Jul 2015 09:04:38 -0700
Andy Lutomirski <[email protected]> wrote:
> On Thu, Jul 23, 2015 at 9:01 AM, Steven Rostedt <[email protected]> wrote:
> > 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.
>
> 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
On Thu, Jul 23, 2015 at 9:06 AM, Steven Rostedt <[email protected]> wrote:
> On Thu, 23 Jul 2015 08:31:41 -0700
> Andy Lutomirski <[email protected]> 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
On Thu, Jul 23, 2015 at 9:10 AM, Steven Rostedt <[email protected]> wrote:
> On Thu, 23 Jul 2015 09:04:38 -0700
> Andy Lutomirski <[email protected]> wrote:
>
>> On Thu, Jul 23, 2015 at 9:01 AM, Steven Rostedt <[email protected]> wrote:
>> > 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.
>>
>> 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
On Thu, 23 Jul 2015 09:46:45 -0700
Andy Lutomirski <[email protected]> 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
On Thu, Jul 23, 2015 at 12:02 PM, Andy Lutomirski <[email protected]> wrote:
> On Thu, Jul 23, 2015 at 8:56 AM, Steven Rostedt <[email protected]> wrote:
>> 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.
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
On Thu, Jul 23, 2015 at 10:27 AM, Brian Gerst <[email protected]> wrote:
> On Thu, Jul 23, 2015 at 12:02 PM, Andy Lutomirski <[email protected]> wrote:
>> On Thu, Jul 23, 2015 at 8:56 AM, Steven Rostedt <[email protected]> wrote:
>>> 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.
>
> 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
On Thu, Jul 23, 2015 at 11:01 AM, Andy Lutomirski <[email protected]> wrote:
> On Thu, Jul 23, 2015 at 10:27 AM, Brian Gerst <[email protected]> wrote:
>> On Thu, Jul 23, 2015 at 12:02 PM, Andy Lutomirski <[email protected]> wrote:
>>> On Thu, Jul 23, 2015 at 8:56 AM, Steven Rostedt <[email protected]> wrote:
>>>> 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.
>>
>> 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
On Thu, Jul 23, 2015 at 11:33 AM, Andy Lutomirski <[email protected]> wrote:
> On Thu, Jul 23, 2015 at 11:01 AM, Andy Lutomirski <[email protected]> wrote:
>> On Thu, Jul 23, 2015 at 10:27 AM, Brian Gerst <[email protected]> wrote:
>>> On Thu, Jul 23, 2015 at 12:02 PM, Andy Lutomirski <[email protected]> wrote:
>>>> On Thu, Jul 23, 2015 at 8:56 AM, Steven Rostedt <[email protected]> wrote:
>>>>> 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.
>>>
>>> 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
On Thu, Jul 23, 2015 at 11:42 AM, Andy Lutomirski <[email protected]> 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