: Fix MSR_K7_EVNTSEL{0,3} for SVM
PATCH 1/1 : Fix MSR_K7_EVNTSEL{0,3} for SVM
Avi, Joerg,
While trying to boot a RHEL-4 guest on latest KVM tip on an AMD machine, I
found that the guest would consistently crash when trying to setup the NMI watchdog. I traced it down to the following commit:
51ef1ac7b23ee32bfcc61c229d634fdc1c68b38a
It seems that in that commit, the K7_EVNTSEL MSR's were set to fail if the data != 0. That test is actually fine, the problem is how the code around it is generated. That is, we are only supposed to go to unhandled if data != 0; but for some reason, we are *always* going to unhandled, even when the data == 0. That causes RHEL-4 kernel to crash. If I rearrange the code to look like this:
case MSR_K7_EVNTSEL0: case MSR_K7_EVNTSEL1: case MSR_K7_EVNTSEL2: case MSR_K7_EVNTSEL3: if (data != 0) return kvm_set_msr_common(vcpu, ecx, data); default: return kvm_set_msr_common(vcpu, ecx, data); }
Then everything works again. A patch that does just this is attached. It might be slightly nicer to say "if (data == 0) return 0" and then just fall through to the default case, but I don't much care either way.
arch/x86/kvm/svm.c +2 -3
@@ -1291,10 +1291,9 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 data)* performance counter emulation later. */ if (data !=0 ) - goto unhandled; -break ; +return kvm_set_msr_common (vcpu, ecx, data); +default : - unhandled:return kvm_set_msr_common (vcpu, ecx, data); }return 0 ;
-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
_______________________________________________
kvm-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel
Quoted 25 lines · Chris Lalancette
Avi, Joerg,
While trying to boot a RHEL-4 guest on latest KVM tip on an AMD machine, I
found that the guest would consistently crash when trying to setup the NMI watchdog. I traced it down to the following commit:
51ef1ac7b23ee32bfcc61c229d634fdc1c68b38aIt seems that in that commit, the
K7_EVNTSELMSR's were set to fail if the data != 0. That test is actually fine, the problem is how the code around it is generated. That is, we are only supposed to go to unhandled if data != 0; but for some reason, we are *always* going to unhandled, even when the data == 0. That causes RHEL-4 kernel to crash. If I rearrange the code to look like this:case MSR_K7_EVNTSEL0: case MSR_K7_EVNTSEL1: case MSR_K7_EVNTSEL2: case MSR_K7_EVNTSEL3: if (data != 0) return kvm_set_msr_common(vcpu, ecx, data); default: return kvm_set_msr_common(vcpu, ecx, data); }
Quoted 4 lines
Then everything works again. A patch that does just this is attached. It might be slightly nicer to say "if (data == 0) return 0" and then just fall through to the default case, but I don't much care either way.
You mean the gcc generates wrong code? It seems fine here (though wonderfully obfuscated).
Can you attach an objdump -Sr svm.o? Also, what gcc version are you using?
Signature · 8 lines
error compiling committee.c: too many arguments to function ------------------------------------------------------------------------- This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2. http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 9 lines · Chris Lalancette
Quoted 6 lines · Avi Kivity
You mean the gcc generates wrong code? It seems fine here (though wonderfully obfuscated).
Can you attach an objdump -Sr svm.o? Also, what gcc version are you using?
Quoted 7 lines
(sending attachment in private mail, so I don't spam the whole list with 189K of objdump). Note that this is an objdump -Sr of the current code, with my patch *not* applied.
gcc is gcc-4.3.0-7 in Fedora 9.
It's a gcc bug. svm_set_msr() places ecx in %rsi, and consistently uses %esi to refer to the first 32 bits. But when it compiles this bit:
Quoted 4 lines
case MSR_K7_EVNTSEL0:
case MSR_K7_EVNTSEL1:
case MSR_K7_EVNTSEL2:
case MSR_K7_EVNTSEL3:Quoted 8 lines
/*
* only support writing 0 to the performance counters for now
* to make Windows happy. Should be replaced by a real
* performance counter emulation later.
*/
if (data != 0)
goto unhandled;
break;(where MSR_K7_EVENTSEL[0123] == 0xc001000[0123])
it compiles it into
Quoted 3 lines
1811: 8d 86 00 00 ff 3f lea 0x3fff0000(%rsi),%eax
1817: 83 f8 03 cmp $0x3,%eax
181a: 0f 87 e2 01 00 00 ja 1a02 <svm_set_msr+0x27f>Now it uses %rsi instead of %esi, and any junk in the upper bits will cause the ja to be taken.
We need to get a reduced testcase to the gcc folks, this is a serious bug. Any changes in the code to work around this would be fragile.
Signature · 8 lines
error compiling committee.c: too many arguments to function ------------------------------------------------------------------------- This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2. http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
Quoted 10 lines · Avi Kivity
Quoted 4 lines
1811: 8d 86 00 00 ff 3f lea 0x3fff0000(%rsi),%eax 1817: 83 f8 03 cmp $0x3,%eax 181a: 0f 87 e2 01 00 00 ja 1a02 <svm_set_msr+0x27f>Now it uses %rsi instead of %esi, and any junk in the upper bits will cause the ja to be taken.
We need to get a reduced testcase to the gcc folks, this is a serious bug. Any changes in the code to work around this would be fragile.
Ouch, I missed this on my reading of it. I'll try to come up with a standalone program that shows this.
Thanks, Avi.
Chris Lalancette
-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
Quoted 6 lines · Avi Kivity
Now it uses %rsi instead of %esi, and any junk in the upper bits will cause the ja to be taken.
We need to get a reduced testcase to the gcc folks, this is a serious bug. Any changes in the code to work around this would be fragile.
Avi,
I've now filed a bug in the upstream gcc database:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=36040
And I came up with a reduced test case, available here:
http://people.redhat.com/clalance/rsi-test-case.tar.bz2
If I compile the code in the above and look at the disassembly, it shows the problem; however, I can't reproduce the bug by actually running the code. I suspect the %rsi register is always 0 when we start in this userland code, so I never run into the bogus ja, but I just thought I'd mention it.
Chris Lalancette
-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
Quoted 15 lines · Chris Lalancette
Quoted 7 lines · Avi Kivity
Now it uses %rsi instead of %esi, and any junk in the upper bits will cause the ja to be taken.
We need to get a reduced testcase to the gcc folks, this is a serious bug. Any changes in the code to work around this would be fragile.
Avi,
I've now filed a bug in the upstream gcc database:
Quoted 10 lines
And I came up with a reduced test case, available here:
http://people.redhat.com/clalance/rsi-test-case.tar.bz2
If I compile the code in the above and look at the disassembly, it shows the problem; however, I can't reproduce the bug by actually running the code. I suspect the %rsi register is always 0 when we start in this userland code, so I never run into the bogus ja, but I just thought I'd mention it.
Hmm, looking back at the dump:
Quoted 3 lines
1811: 8d 86 00 00 ff 3f lea 0x3fff0000(%rsi),%eax
1817: 83 f8 03 cmp $0x3,%eax
181a: 0f 87 e2 01 00 00 ja 1a02 <svm_set_msr+0x27f>So while gcc is using %rsi, it loads the result back into %eax, which has the effect of dropping back into 32-bits. So looks like gcc was right here. Sorry for spreading confusion and apologies to gcc.
Signature · 8 lines
Do not meddle in the internals of kernels, for they are subtle and quick to panic. ------------------------------------------------------------------------- This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2. http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
Quoted 11 lines · Avi Kivity
Hmm, looking back at the dump:
Quoted 3 lines
1811: 8d 86 00 00 ff 3f lea 0x3fff0000(%rsi),%eax 1817: 83 f8 03 cmp $0x3,%eax 181a: 0f 87 e2 01 00 00 ja 1a02 <svm_set_msr+0x27f>So while gcc is using %rsi, it loads the result back into %eax, which has the effect of dropping back into 32-bits. So looks like gcc was right here. Sorry for spreading confusion and apologies to gcc.
OK. Well, then I can't explain why we are unconditionally calling kvm_set_msr_common(), regardless of whether data == 0 or not. Avi, you said it works for you; what version of gcc are you using, and can you send me your objdump -Sr? I'd like to compare the assembly output with what 4.3.0 is spitting out.
Chris Lalancette
-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
Quoted 11 lines · Avi Kivity
Hmm, looking back at the dump:
Quoted 3 lines
1811: 8d 86 00 00 ff 3f lea 0x3fff0000(%rsi),%eax 1817: 83 f8 03 cmp $0x3,%eax 181a: 0f 87 e2 01 00 00 ja 1a02 <svm_set_msr+0x27f>So while gcc is using %rsi, it loads the result back into %eax, which has the effect of dropping back into 32-bits. So looks like gcc was right here. Sorry for spreading confusion and apologies to gcc.
Avi,
Arg. I was completely, utterly wrong about the problem here (although
there is definitely still a problem). I'm sorry for making a confusing mess out of this. Here is what is actually happening:
During startup, the RHEL-4 x86_64 kernel (2.6.9-67.EL, if you care) setups up the NMI watchdog. It does the following:
for(i = 0; i < 4; ++i) {
/* Simulator may not support it */
if (checking_wrmsrl(MSR_K7_EVNTSEL0+i, 0UL))
return;
wrmsrl(MSR_K7_PERFCTR0+i, 0UL);
}checking_wrmsrl() just does a "test write" to the msr; because of the code that is currently in there, this succeeds. However, when it tries to do the MSR_K7_PERFCTR0 wrmsr, *that* is where it fails, since we don't currently handle that MSR, and KVM injects a GPF into the guest (which kills it). My previous patch just happened to fix this because it was making checking_wrmsrl() fail on the EVNTSEL0, so we just returned out of this loop rather than trying to write to the PERFCTR0.
Unfortunately, we can't just "fake emulate" MSR_K7_PERFCTR[0-3] like we are doing for MSR_K7_EVNTSEL[0-3]; if they are there, linux expects to be able to put values into them. I think the correct solution here is to emulate MSR_K7_PERFCTR[0-3] and MSR_K7_EVNTSEL[0-3] for real. I'm working on a patch to do this now.
Chris Lalancette
-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
Quoted 5 lines · Chris Lalancette
Unfortunately, we can't just "fake emulate" MSR_K7_PERFCTR[0-3] like we are doing for MSR_K7_EVNTSEL[0-3]; if they are there, linux expects to be able to put values into them. I think the correct solution here is to emulate MSR_K7_PERFCTR[0-3] and MSR_K7_EVNTSEL[0-3] for real. I'm working on a patch to do this now.
We already discussed the emulation of the performance counter registers in the past. The conclusion is, that we loose live migration with that emulation because performance monitoring is implemented differently between AMD and Intel systems. Maybe discarding _any_ writes to the performance counter MSRs will fix the guest crash. What we should not do is injecting GPF on zero writes to the MSRs because that will break Windows XP 64bit installation.
Joerg
Signature · 13 lines
| AMD Saxony Limited Liability Company & Co. KG
Operating | Wilschdorfer Landstr. 101, 01109 Dresden, Germany
System | Register Court Dresden: HRA 4896
Research | General Partner authorized to represent:
Center | AMD Saxony LLC (Wilmington, Delaware, US)
| General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy
-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference
Don't miss this year's exciting event. There's still time to save $100.
Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaoneQuoted 7 lines · Joerg Roedel
We already discussed the emulation of the performance counter registers in the past. The conclusion is, that we loose live migration with that emulation because performance monitoring is implemented differently between AMD and Intel systems. Maybe discarding _any_ writes to the performance counter MSRs will fix the guest crash. What we should not do is injecting GPF on zero writes to the MSRs because that will break Windows XP 64bit installation.
OK, yeah, I read some of those threads over the weekend. It is a larger
problem than just this crash, and probably one to be solved later.
I have attached a patch which just discards all writes, as you suggested;
this allows my RHEL-4 guest to successfully boot, although obviously the NMI watchdog which it thinks it set up is not actually working. Joerg, can you test this on your 64-bit Windows XP guest to make sure it is still working?
Thanks,
Chris Lalancette
arch/x86/kvm/svm.c +8 -6
@@ -1284,16 +1284,18 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, unsigned ecx, u64 data)case MSR_K7_EVNTSEL1 :case MSR_K7_EVNTSEL2 :case MSR_K7_EVNTSEL3 : +case MSR_K7_PERFCTR0 : +case MSR_K7_PERFCTR1 : +case MSR_K7_PERFCTR2 : +case MSR_K7_PERFCTR3 :/* -* only support writing 0 to the performance counters for now -* to make Windows happy. Should be replaced by a real -* performance counter emulation later. +* Just discard all writes to the performance counters; this +* should keep both older linux and windows 64-bit guests +* happy */ -if (data !=0 ) - goto unhandled; +break ;default : - unhandled:return kvm_set_msr_common (vcpu, ecx, data); }return 0 ;
-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
_______________________________________________
kvm-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel
Quoted 9 lines · Chris Lalancette
Quoted 7 lines · Joerg Roedel
We already discussed the emulation of the performance counter registers in the past. The conclusion is, that we loose live migration with that emulation because performance monitoring is implemented differently between AMD and Intel systems. Maybe discarding _any_ writes to the performance counter MSRs will fix the guest crash. What we should not do is injecting GPF on zero writes to the MSRs because that will break Windows XP 64bit installation.
Quoted 6 lines
OK, yeah, I read some of those threads over the weekend. It is a larger
problem than just this crash, and probably one to be solved later.I have attached a patch which just discards all writes, as you suggested;this allows my RHEL-4 guest to successfully boot, although obviously the NMI watchdog which it thinks it set up is not actually working. Joerg, can you test this on your 64-bit Windows XP guest to make sure it is still working?
XP 64 bit installs and works fine with your patch.
Joerg
Signature · 13 lines
| AMD Saxony Limited Liability Company & Co. KG
Operating | Wilschdorfer Landstr. 101, 01109 Dresden, Germany
System | Register Court Dresden: HRA 4896
Research | General Partner authorized to represent:
Center | AMD Saxony LLC (Wilmington, Delaware, US)
| General Manager of AMD Saxony LLC: Dr. Hans-R. Deppe, Thomas McCoy
-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference
Don't miss this year's exciting event. There's still time to save $100.
Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaoneQuoted 8 lines · Joerg Roedel
Quoted 6 lines
OK, yeah, I read some of those threads over the weekend. It is a larger
problem than just this crash, and probably one to be solved later.I have attached a patch which just discards all writes, as you suggested;this allows my RHEL-4 guest to successfully boot, although obviously the NMI watchdog which it thinks it set up is not actually working. Joerg, can you test this on your 64-bit Windows XP guest to make sure it is still working?
XP 64 bit installs and works fine with your patch.
Great, thanks for testing, I'll post the patch again with a proper changelog for Avi.
Chris Lalancette
-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone
Quoted 2 lines · Chris Lalancette
this allows my RHEL-4 guest to successfully boot, although obviously the NMI watchdog which it thinks it set up is not actually working.
That's fine. On a virtualized environment you don't really need a NMI watchdog because you can always debug from the outside
-Andi
-------------------------------------------------------------------------
This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Don't miss this year's exciting event. There's still time to save $100. Use priority code J8TL2D2.
http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone