[3.6] x86: cr2 and cmpxchg issues of NMI
Ingo,
As Avi brought to attention, there may be issues with NMIs and page faults as if a NMI takes a page fault it can corrupt the cr2 for a page fault handler that it preempted. The first and third patch fixes this for x86_64 and i386 respectively.
The second patch is to fix the cmpxchg issue brought up by someone on LWN. The NMI handler for i386 uses cmpxchg to handle nested NMIs. But some older i386 boxes do not have a true cmpxchg and this will fail for them. Luckily, there is a simple fix that also makes the code cleaner.
Please pull the latest tip/x86/core tree, which can be found at:
git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-trace.git
tip/x86/core
Head SHA1: 7fefda3109021f98926d76a13f90d0b116ce3db5
Steven Rostedt (3):
x86: Save cr2 in NMI in case NMIs take a page fault
x86: Remove cmpxchg from i386 NMI nesting code
x86: Save cr2 in NMI in case NMIs take a page fault (for i386)
----
arch/x86/kernel/entry_64.S | 20 +++++++++++++++++ arch/x86/kernel/nmi.c | 51 +++++++++++++++++++++++++++++++------------- 2 files changed, 56 insertions(+), 15 deletions(-)
PATCH 1/3 x86: Save cr2 in NMI in case NMIs take a page fault
From: Steven Rostedt <[email protected]>
Avi Kivity reported that page faults in NMIs could cause havic if the NMI preempted another page fault handler:
The recent changes to NMI allow exceptions to take place in NMI handlers, but I think that a #PF (say, due to access to vmalloc space) is still problematic. Consider the sequence
#PF (cr2 set by processor)
NMI
...
#PF (cr2 clobbered)
do_page_fault()
IRET
...
IRET
do_page_fault()
address = read_cr2()The last line reads the overwritten cr2 value.
Originally I wrote a patch to solve this by saving the cr2 on the stack. Brian Gerst suggested to save it in the r12 register as both r12 and rbx are saved by the do_nmi handler as required by the C standard. But rbx is already used for saving if swapgs needs to be run on exit of the NMI handler.
--- arch/x86/kernel/entry_64.S | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
arch/x86/kernel/entry_64.S +20 -0
@@ -1758,10 +1758,30 @@ end_repeat_nmi: */ call save_paranoid DEFAULT_FRAME 0 + + /* + * Save off the CR2 register. If we take a page fault in the NMI then + * it could corrupt the CR2 value. If the NMI preempts a page fault + * handler before it was able to read the CR2 register, and then the + * NMI itself takes a page fault, the page fault that was preempted + * will read the information from the NMI page fault and not the + * origin fault. Save it off and restore it if it changes. + * Use the r12 callee-saved register. + */ + movq %cr2, %r12 + /* paranoidentry do_nmi, 0; without TRACE_IRQS_OFF */ movq %rsp,%rdi movq $-1,%rsi call do_nmi + + /* Did the NMI take a page fault? Restore cr2 if it did */ + movq %cr2, %rcx + cmpq %rcx, %r12 + je 1f + movq %r12, %cr2 +1: + testl %ebx,%ebx /* swapgs needed? */ jnz nmi_restore nmi_swapgs:
Signature · 1 line
1.7.10
PATCH 2/3 x86: Remove cmpxchg from i386 NMI nesting code
In reply to Steven RostedtFrom: Steven Rostedt <[email protected]>
I've been informed by someone on LWN called 'slashdot' that some i386 machines do not support a true cmpxchg. The cmpxchg used by the i386 NMI nesting code must be a true cmpxchg as disabling interrupts will not work for NMIs (which is the work around for i386s that do not have a true cmpxchg).
This 'slashdot' character also suggested a fix to the issue. As the state of the nesting NMIs goes as follows:
NOT_RUNNING -> EXECUTING
EXECUTING -> NOT_RUNNING EXECUTING -> LATCHED LATCHED -> EXECUTING
Having these states as enum values of:
NOT_RUNNING = 0
EXECUTING = 1
LATCHED = 2
Instead of a cmpxchg to make EXECUTING -> NOT_RUNNING a dec_and_test() would work as well. If the dec_and_test brings the state to NOT_RUNNING, that is the same as a cmpxchg succeeding to change EXECUTING to NOT_RUNNING. If a nested NMI were to come in and change it to LATCHED, the dec_and_test() would convert the state to EXECUTING (what we want it to be in such a case anyway).
I asked 'slashdot' to post this as a patch, but it never came to be. I decided to do the work instead.
--- arch/x86/kernel/nmi.c | 39 ++++++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 15 deletions(-)
arch/x86/kernel/nmi.c +24 -15
@@ -28,6 +28,7 @@#include <asm/mach_traps.h> #include <asm/nmi.h> #include <asm/x86_init.h> +#include <asm/local.h> struct nmi_desc {spinlock_t lock;
@@ -365,8 +366,9 @@ static __kprobes void default_do_nmi(struct pt_regs *regs)#ifdef CONFIG_X86_32 /* * For i386, NMIs use the same stack as the kernel, and we can -* add a workaround to the iret problem in C. Simply have 3 states -* the NMI can be in. +* add a workaround to the iret problem in C (preventing nested +* NMIs if an NMI takes a trap). Simply have 3 states the NMI +* can be in: * * 1) not running * 2) executing
@@ -383,32 +385,39 @@ static __kprobes void default_do_nmi(struct pt_regs *regs)* If an NMI hits a breakpoint that executes an iret, another * NMI can preempt it. We do not want to allow this new NMI * to run, but we want to execute it when the first one finishes. -* We set the state to "latched", and the first NMI will perform -* an cmpxchg on the state, and if it doesn't successfully -* reset the state to "not running" it will restart the next -* NMI. +* We set the state to "latched", and the exit of the first NMI will +* perform a dec_and_test, if the result is zero (NOT_RUNNING), then +* it will simply exit the NMI handler. If not, the dec_and_test +* would have set the state to NMI_EXECUTING (what we want it to +* be when we are running). In this case, we simply jump back +* to rerun the NMI handler again, and restart the 'latched' NMI. +* +* No trap (breakpoint or page fault) should be hit before nmi_restart, +* thus there is no race between the first check of state for NOT_RUNNING +* and setting it to NMI_EXECUTING. The HW will prevent nested NMIs +* at this point. */ enum nmi_states { -NMI_NOT_RUNNING , +NMI_NOT_RUNNING =0 ,NMI_EXECUTING ,NMI_LATCHED , }; -static DEFINE_PER_CPU(enum nmi_states, nmi_state); +static DEFINE_PER_CPU(local_t, nmi_state);#define nmi_nesting_preprocess (regs) \ do { \ - if (__get_cpu_var(nmi_state) != NMI_NOT_RUNNING) { \ - __get_cpu_var(nmi_state) = NMI_LATCHED; \ + local_t *__state = &__get_cpu_var(nmi_state); \ + if (local_read(__state) != NMI_NOT_RUNNING) { \ + local_set(__state, NMI_LATCHED); \ return; \ } \ - nmi_restart: \ - __get_cpu_var(nmi_state) = NMI_EXECUTING; \ - } while (0) + local_set(__state, NMI_EXECUTING); \ + } while (0); \ + nmi_restart:#define nmi_nesting_postprocess () \ do { \ - if (cmpxchg(&__get_cpu_var(nmi_state), \ - NMI_EXECUTING, NMI_NOT_RUNNING) != NMI_EXECUTING) \ + if (!local_dec_and_test(&__get_cpu_var(nmi_state))) \ goto nmi_restart; \ } while (0)#else /* x86_64 */
Signature · 1 line
1.7.10
H. Peter Anvin
reviewerRe: [PATCH 2/3] x86: Remove cmpxchg from i386 NMI nesting code
Quoted 30 lines · Steven Rostedt
From: Steven Rostedt <[email protected]>
I've been informed by someone on LWN called 'slashdot' that some i386 machines do not support a true cmpxchg. The cmpxchg used by the i386 NMI nesting code must be a true cmpxchg as disabling interrupts will not work for NMIs (which is the work around for i386s that do not have a true cmpxchg).
This 'slashdot' character also suggested a fix to the issue. As the state of the nesting NMIs goes as follows:
NOT_RUNNING-> EXECUTINGEXECUTING -> NOT_RUNNING EXECUTING -> LATCHED LATCHED -> EXECUTINGHaving these states as enum values of:
NOT_RUNNING= 0
EXECUTING = 1
LATCHED = 2Instead of a cmpxchg to make EXECUTING ->
NOT_RUNNINGadec_and_test()would work as well. If thedec_and_testbrings the state toNOT_RUNNING, that is the same as a cmpxchg succeeding to change EXECUTING toNOT_RUNNING. If a nested NMI were to come in and change it to LATCHED, thedec_and_test()would convert the state to EXECUTING (what we want it to be in such a case anyway).
Quoted 4 lines
I asked 'slashdot' to post this as a patch, but it never came to be. I decided to do the work instead.
Okay, slight interrupt here.
The cost of this on real hardware better be zero (which I cannot immediately judge.)
Why? Because cmpxchg has been in every CPU since the i486, the i386 is royally crippled on Linux anyway (due to minor architectural defects, the main one being the write protect issue) and NMI is almost never used on i386 as anything other than a fatal error indication.
Most "real" NMI users generate the NMI from the local APIC, but the i386 has no local APIC, and unlike the i486 cannot even have an external local APIC to the best of my knowledge.
-hpa
Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf.
Re: [PATCH 2/3] x86: Remove cmpxchg from i386 NMI nesting code
Quoted 1 line · H. Peter Anvin
Okay, slight interrupt here.
Pun intended?
Quoted 3 lines
The cost of this on real hardware better be zero (which I cannot immediately judge.)
Is dec_and_test cheaper than cmpxchg? I would think so.
Quoted 9 lines
Why? Because cmpxchg has been in every CPU since the i486, the i386 is royally crippled on Linux anyway (due to minor architectural defects, the main one being the write protect issue) and NMI is almost never used on i386 as anything other than a fatal error indication.
Most "real" NMI users generate the NMI from the local APIC, but the i386 has no local APIC, and unlike the i486 cannot even have an external local APIC to the best of my knowledge.
Yeah, this is why I didn't rush to do this change. But it does seem to make the code simpler and it may actually speed things up. It replaces a cmpxchg with a local_dec_and_test, which, I believe, doesn't even lock the cachelines.
So lets look at the patch in detail, shall we?
arch/x86/kernel/nmi.c static __kprobes void default_do_nmi(struct pt_regs *regs) · line 401
Quoted 3 lines
enum
nmi_states{arch/x86/kernel/nmi.c +1
-1- NMI_NOT_RUNNING, + NMI_NOT_RUNNING = 0,
This change was done more for documenting that the first element must be zero. Even though C guarantees this. I wanted to point out that we expect it to be zero and that it being zero really does matter. No functionality change whats-so-ever.
arch/x86/kernel/nmi.c static __kprobes void default_do_nmi(struct pt_regs *regs) · line 405
Quoted 5 lines
NMI_EXECUTING,
NMI_LATCHED,
};arch/x86/kernel/nmi.c +1
-1-static DEFINE_PER_CPU(enum nmi_states, nmi_state); +static DEFINE_PER_CPU(local_t, nmi_state);
local_t is is just an atomic_long_t, which on i386 is nothing different than what an enum would be.
arch/x86/kernel/nmi.c static __kprobes void default_do_nmi(struct pt_regs *regs) · line 411
Quoted 8 lines
#define nmi_nesting_preprocess(regs) \ do { \arch/x86/kernel/nmi.c +3
-2-if (__get_cpu_var (nmi_state) !=NMI_NOT_RUNNING ) { \ -__get_cpu_var (nmi_state) =NMI_LATCHED ; \ +local_t *__state = &__get_cpu_var (nmi_state); \ +if (local_read (__state) !=NMI_NOT_RUNNING ) { \ +local_set (__state,NMI_LATCHED ); \
The above change is probably a little bit of a speed up because we remove the double '__get_cpu_var()' and replace it with a pointer that is reused. I haven't looked at the assembly for this, but it is either the same or better with the patch.
Sure, we could improve this by using this_cpu_var() which may make it better than the patch. But the patch is currently the same or better than what is there now.
arch/x86/kernel/nmi.c static __kprobes void default_do_nmi(struct pt_regs *regs) · line 416
Quoted 8 lines
return; \ } \arch/x86/kernel/nmi.c +3
-3- nmi_restart: \ -__get_cpu_var (nmi_state) =NMI_EXECUTING ; \ - }while (0 ) +local_set (__state,NMI_EXECUTING ); \ + }while (0 ); \ + nmi_restart:
Here it's better or the same than what is there now as we again remove the reference to getting the pointer. In case gcc doesn't optimize it nicely. But again we could have switched to this_cpu_write() which could be better.
The movement of nmi_restart does help too. I'll explain that below.
arch/x86/kernel/nmi.c static __kprobes void default_do_nmi(struct pt_regs *regs) · line 420
Quoted 6 lines
#define nmi_nesting_postprocess() \ do { \arch/x86/kernel/nmi.c +1
-2-if (cmpxchg (&__get_cpu_var (nmi_state), \ -NMI_EXECUTING ,NMI_NOT_RUNNING ) !=NMI_EXECUTING ) \ +if (!local_dec_and_test (&__get_cpu_var (nmi_state))) \
Now this is where I think the patch helps. I'm almost certain that local_dec_and_test is faster than a cmpxchg by many cycles. Especially on i386.
Quoted 1 line
goto nmi_restart; \
And now that the local_dec_and_test on failure set the state to NMI_EXECUTING, we can simply jump back to the new location of nmi_restart instead of having to reset it above.
This also removes the race in case another NMI comes in at this moment and we lose the latch. But this is a small problem, as we are about to repeat the NMI anyway, and it would be the same as HW if the NMI came in just before we finished the last NMI. We only repeat once. This is just changing where we consider the start of the repeated NMI is (where a new latch can happen). But functionality wise, it's no different.
-- Steve
Quoted 1 line
} while (0)
H. Peter Anvin
reviewerRe: [PATCH 2/3] x86: Remove cmpxchg from i386 NMI nesting code
Quoted 6 lines · Steven Rostedt
Quoted 3 lines
The cost of this on real hardware better be zero (which I cannot immediately judge.)
Is
dec_and_testcheaper than cmpxchg? I would think so.
Should be more or less the same (but see below w.r.t. _local).
Quoted 6 lines
Quoted 6 lines
Why? Because cmpxchg has been in every CPU since the i486, the i386 is royally crippled on Linux anyway (due to minor architectural defects, the main one being the write protect issue) and NMI is almost never used on i386 as anything other than a fatal error indication.
Quoted 9 lines
Quoted 3 lines
Most "real" NMI users generate the NMI from the local APIC, but the i386 has no local APIC, and unlike the i486 cannot even have an external local APIC to the best of my knowledge.
Yeah, this is why I didn't rush to do this change. But it does seem to make the code simpler and it may actually speed things up. It replaces a cmpxchg with a
local_dec_and_test, which, I believe, doesn't even lock the cachelines.
Yeah, the cmpxchg here rather than cmpxchg_local seems like it just was a plain bug, no?
Quoted 12 lines
So lets look at the patch in detail, shall we?
arch/x86/kernel/nmi.c static __kprobes void default_do_nmi(struct pt_regs *regs) · line 401
Quoted 3 lines
enum
nmi_states{arch/x86/kernel/nmi.c +1
-1- NMI_NOT_RUNNING, + NMI_NOT_RUNNING = 0,This change was done more for documenting that the first element must be zero. Even though C guarantees this. I wanted to point out that we expect it to be zero and that it being zero really does matter. No functionality change whats-so-ever.
Yes, that makes sense.
Quoted 18 lines
arch/x86/kernel/nmi.c static __kprobes void default_do_nmi(struct pt_regs *regs) · line 405
Quoted 5 lines
NMI_EXECUTING,
NMI_LATCHED,
};arch/x86/kernel/nmi.c +1
-1-static DEFINE_PER_CPU(enum nmi_states, nmi_state); +static DEFINE_PER_CPU(local_t, nmi_state);
local_tis is just anatomic_long_t, which on i386 is nothing different than what an enum would be.arch/x86/kernel/nmi.c static __kprobes void default_do_nmi(struct pt_regs *regs) · line 411
Quoted 8 lines
#define nmi_nesting_preprocess(regs) \ do { \arch/x86/kernel/nmi.c +3
-2-if (__get_cpu_var (nmi_state) !=NMI_NOT_RUNNING ) { \ -__get_cpu_var (nmi_state) =NMI_LATCHED ; \ +local_t *__state = &__get_cpu_var (nmi_state); \ +if (local_read (__state) !=NMI_NOT_RUNNING ) { \ +local_set (__state,NMI_LATCHED ); \
Quoted 8 lines
The above change is probably a little bit of a speed up because we remove the double '
__get_cpu_var()' and replace it with a pointer that is reused. I haven't looked at the assembly for this, but it is either the same or better with the patch.Sure, we could improve this by using
this_cpu_var()which may make it better than the patch. But the patch is currently the same or better than what is there now.
But yes, if you're going to modify this use this_cpu_read() and this_cpu_write() and avoid the pointer completely.
Quoted 23 lines
arch/x86/kernel/nmi.c static __kprobes void default_do_nmi(struct pt_regs *regs) · line 416
Quoted 8 lines
return; \ } \arch/x86/kernel/nmi.c +3
-3- nmi_restart: \ -__get_cpu_var (nmi_state) =NMI_EXECUTING ; \ - }while (0 ) +local_set (__state,NMI_EXECUTING ); \ + }while (0 ); \ + nmi_restart:Here it's better or the same than what is there now as we again remove the reference to getting the pointer. In case gcc doesn't optimize it nicely. But again we could have switched to
this_cpu_write()which could be better.The movement of
nmi_restartdoes help too. I'll explain that below.arch/x86/kernel/nmi.c static __kprobes void default_do_nmi(struct pt_regs *regs) · line 420
Quoted 6 lines
#define nmi_nesting_postprocess() \ do { \arch/x86/kernel/nmi.c +1
-2-if (cmpxchg (&__get_cpu_var (nmi_state), \ -NMI_EXECUTING ,NMI_NOT_RUNNING ) !=NMI_EXECUTING ) \ +if (!local_dec_and_test (&__get_cpu_var (nmi_state))) \
Quoted 4 lines
Now this is where I think the patch helps. I'm almost certain that
local_dec_and_testis faster than a cmpxchg by many cycles. Especially on i386.
On i386 it's infinite, but again, I don't think the code will ever be exercised on i386. I'm much more concerned about performance on current processors.
But yes, local_dec_and_test should at least not be more expensive. Even better, use this_cpu_dec_return().
-hpa
Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf.
Re: [PATCH 2/3] x86: Remove cmpxchg from i386 NMI nesting code
Quoted 9 lines · H. Peter Anvin
Quoted 7 lines · Steven Rostedt
Quoted 6 lines
#define nmi_nesting_postprocess() \ do { \arch/x86/kernel/nmi.c +1
-2-if (cmpxchg (&__get_cpu_var (nmi_state), \ -NMI_EXECUTING ,NMI_NOT_RUNNING ) !=NMI_EXECUTING ) \ +if (!local_dec_and_test (&__get_cpu_var (nmi_state))) \
Quoted 8 lines
Quoted 4 lines
Now this is where I think the patch helps. I'm almost certain that
local_dec_and_testis faster than a cmpxchg by many cycles. Especially on i386.On i386 it's infinite, but again, I don't think the code will ever be exercised on i386. I'm much more concerned about performance on current processors.
I'm not worried about non i386 here. Some context missing from the patch is that this code is surrounded by:
#ifdef CONFIG_X86_32
[...]
#else
The '#else' part does something completely different, as this work is done in the assembly handler on x86_64.
And yes, this code is executed on i386 (well when I boot my x86_64 into an i386 kernel it does).
Quoted 3 lines
But yes,
local_dec_and_testshould at least not be more expensive. Even better, usethis_cpu_dec_return().
Oh! That would make this much better! I'll update the patch set.
Thanks!
-- Steve
H. Peter Anvin
reviewerRe: [PATCH 2/3] x86: Remove cmpxchg from i386 NMI nesting code
Quoted 8 lines · Steven Rostedt
Quoted 4 lines
On i386 it's infinite, but again, I don't think the code will ever be exercised on i386. I'm much more concerned about performance on current processors.
I'm not worried about non i386 here. Some context missing from the patch is that this code is surrounded by:
Sorry, I meant i386 as in actual i386 processors, as opposed to modern processors in 32-bit mode.
-hpa
Signature · 2 lines
H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf.
Re: [PATCH 2/3] x86: Remove cmpxchg from i386 NMI nesting code
Quoted 6 lines · H. Peter Anvin
Quoted 5 lines · Steven Rostedt
Quoted 4 lines
On i386 it's infinite, but again, I don't think the code will ever be exercised on i386. I'm much more concerned about performance on current processors.
Quoted 7 lines
Quoted 3 lines
I'm not worried about non i386 here. Some context missing from the patch is that this code is surrounded by:
Sorry, I meant i386 as in actual i386 processors, as opposed to modern processors in 32-bit mode.
Fair enough. But still, I think a dec_and_test() is nicer (even if we use this_cpu_dec_and_return()). That's the rational for this change in the first place, with an added benefit that it would work with the fictional i386 old processor that does NMIs but no cmpxchg.
Not to mention that I would love to make all users that have x86_64 machines running i386 kernels, for something other than testing i386, thrown into a large boiling pot of clue stew. It really pisses me off when people run these i386 machines with 16 gigs of memory and complain about performance. highmem should be disabled for any x86_64 box with more than 1G of RAM running an i386 kernel with a nasty message on boot up:
"Idiot! Why the f*ck are you running i386 on your nice shiny new x86_64 machine, with Umpteen Gigs of RAM. Boot a x86_64 kernel for Christ sake and get your full potential. Your i386 userspace will still work just fine on it. You're like one of those 75 year old retirees that can finally buy a Porsche just to drive it 10 miles per hour below the speed limit with a line of cars behind them!"
-- Steve
H. Peter Anvin
reviewerRe: [PATCH 2/3] x86: Remove cmpxchg from i386 NMI nesting code
Quoted 8 lines · Steven Rostedt
"Idiot! Why the f*ck are you running i386 on your nice shiny new
x86_64machine, with Umpteen Gigs of RAM. Boot ax86_64kernel for Christ sake and get your full potential. Your i386 userspace will still work just fine on it. You're like one of those 75 year old retirees that can finally buy a Porsche just to drive it 10 miles per hour below the speed limit with a line of cars behind them!"
LOL!
-hpa
Borislav Petkov
maintainerRe: [PATCH 2/3] x86: Remove cmpxchg from i386 NMI nesting code
In reply to Steven RostedtQuoted 6 lines · Steven Rostedt
"Idiot! Why the f*ck are you running i386 on your nice shiny new
x86_64machine, with Umpteen Gigs of RAM. Boot ax86_64kernel for Christ sake and get your full potential. Your i386 userspace will still work just fine on it. You're like one of those 75 year old retirees that can finally buy a Porsche just to drive it 10 miles per hour below the speed limit with a line of cars behind them!"
If you make this into a patch, you can have my vehement ACK.
Signature · 2 lines
Regards/Gruss,
Boris.Re: [PATCH 2/3] x86: Remove cmpxchg from i386 NMI nesting code
In reply to Steven RostedtQuoted 6 lines · Steven Rostedt
"Idiot! Why the f*ck are you running i386 on your nice shiny new
x86_64machine, with Umpteen Gigs of RAM. Boot ax86_64kernel for Christ sake and get your full potential. Your i386 userspace will still work just fine on it. You're like one of those 75 year old retirees that can finally buy a Porsche just to drive it 10 miles per hour below the speed limit with a line of cars behind them!"
:) Yeah, if only native x86_64 could run vm86 so as to ask the VGA BIOS
to unbotch the graphics card that shot itself into the foot for whatever reason, sigh...
Maciej
H. Peter Anvin
reviewerRe: [PATCH 2/3] x86: Remove cmpxchg from i386 NMI nesting code
Just use a VM or a simulator.
Quoted 19 lines · Maciej W. Rozycki
Quoted 1 line · Steven Rostedt
"Idiot! Why the f*ck are you running i386 on your nice shiny new
x86_64Quoted 1 line
machine, with Umpteen Gigs of RAM. Boot a
x86_64kernel for Christsake
Quoted 3 lines
and get your full potential. Your i386 userspace will still work just fine on it. You're like one of those 75 year old retirees that can finally buy a Porsche just to drive it 10 miles per hour below the
speed
Quoted 1 line
limit with a line of cars behind them!"
:) Yeah, if only native
x86_64could run vm86 so as to ask the VGA BIOS
to unbotch the graphics card that shot itself into the foot for whatever
reason, sigh...Maciej
Signature · 1 line
Sent from my mobile phone. Please excuse brevity and lack of formatting.
Re: [PATCH 2/3] x86: Remove cmpxchg from i386 NMI nesting code
Quoted 1 line · H. Peter Anvin
Just use a VM or a simulator.
That wasn't completely serious ;) -- however thanks for the hint. I
wonder actually if any piece of software would let the code run in a "pass-through" mode and record what I/O accesses went past. I reckon DOSEMU used to support that (and running the VGA BIOS) -- but that piece did indeed require vm86. Then the sequence recorded could be replayed natively.
Thanks for the food for thought! :)
Maciej
H. Peter Anvin
reviewerRe: [PATCH 2/3] x86: Remove cmpxchg from i386 NMI nesting code
libx86 is used for this. Of course, when you interpret you can also record...
Quoted 15 lines · Maciej W. Rozycki
Quoted 1 line · H. Peter Anvin
Just use a VM or a simulator.
That wasn't completely serious ;) -- however thanks for the hint. I wonder actually if any piece of software would let the code run in a "pass-through" mode and record what I/O accesses went past. I reckon DOSEMU used to support that (and running the VGA BIOS) -- but that piece
did indeed require vm86. Then the sequence recorded could be replayed natively.Thanks for the food for thought! :)
Maciej
Signature · 1 line
Sent from my mobile phone. Please excuse brevity and lack of formatting.
PATCH 3/3 x86: Save cr2 in NMI in case NMIs take a page fault (for i386)
In reply to Steven RostedtFrom: Steven Rostedt <[email protected]>
Avi Kivity reported that page faults in NMIs could cause havic if the NMI preempted another page fault handler:
The recent changes to NMI allow exceptions to take place in NMI handlers, but I think that a #PF (say, due to access to vmalloc space) is still problematic. Consider the sequence
#PF (cr2 set by processor)
NMI
...
#PF (cr2 clobbered)
do_page_fault()
IRET
...
IRET
do_page_fault()
address = read_cr2()The last line reads the overwritten cr2 value.
This is the i386 version, which has the luxury of doing the work in C code.
--- arch/x86/kernel/nmi.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
arch/x86/kernel/nmi.c +12 -0
@@ -396,6 +396,14 @@ static __kprobes void default_do_nmi(struct pt_regs *regs)* thus there is no race between the first check of state for NOT_RUNNING * and setting it to NMI_EXECUTING. The HW will prevent nested NMIs * at this point. +* +* In case the NMI takes a page fault, we need to save off the CR2 +* because the NMI could have preempted another page fault and corrupt +* the CR2 that is about to be read. As nested NMIs must be restarted +* and they can not take breakpoints or page faults, the update of the +* CR2 must be done before converting the nmi state back to NOT_RUNNING. +* Otherwise, there would be a race of another nested NMI coming in +* after setting state to NOT_RUNNING but before updating the nmi_cr2. */ enum nmi_states {NMI_NOT_RUNNING =0 ,
@@ -403,6 +411,7 @@ enum nmi_states { NMI_LATCHED, }; static DEFINE_PER_CPU(local_t, nmi_state); +static DEFINE_PER_CPU(unsigned long, nmi_cr2);#define nmi_nesting_preprocess (regs) \ do { \
@@ -412,11 +421,14 @@ static DEFINE_PER_CPU(local_t, nmi_state);return ; \ } \local_set (__state,NMI_EXECUTING ); \ +this_cpu_write (nmi_cr2,read_cr2 ()); \ }while (0 ); \ nmi_restart:#define nmi_nesting_postprocess () \ do { \ + if (unlikely(this_cpu_read(nmi_cr2) != read_cr2())) \ + write_cr2(this_cpu_read(nmi_cr2)); \ if (!local_dec_and_test(&__get_cpu_var(nmi_state))) \ goto nmi_restart; \ } while (0)
Signature · 1 line
1.7.10