Skip to contentSkip to conversation

cstart64: do not assume CR4 should be zero

PATCH In review Sean Christopherson 3 participants 4 messages Jul 15, 2020 – Jul 28, 2020 KVM

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 cstart64: do not assume CR4 should be zero

Explicitly zero cr4 in prepare_64() instead of "zeroing" it in the common enter_long_mode(). Clobbering cr4 in enter_long_mode() breaks switch_to_5level(), which sets cr4.LA57 before calling enter_long_mode() and obviously expects cr4 to be preserved.

Fixes d86ef58 ("cstart: do not assume CR4 starts as zero")
Cc Nadav Amit <[email protected]>
Cc Paolo Bonzini <[email protected]>

---

Two lines of code, two bugs. I'm pretty sure Paolo should win some kind of award. :-D

 x86/cstart64.S | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
x86/cstart64.S +5 -1
index 3ae98d3..2d16688 100644
@@ -175,8 +175,12 @@ prepare_64:
 	lgdt gdt64_desc
 	setup_segments
 
+	xor %eax, %eax
+	mov %eax, %cr4
+
 enter_long_mode:
-	mov $(1 << 5), %eax // pae
+	mov %cr4, %eax
+	bts $5, %eax  // pae
 	mov %eax, %cr4
 
 	mov pt_root, %eax
Signature · 1 line
2.26.0
Reviewed-by on v1 1/1
#1 lore raw
Quoted 7 lines · Sean Christopherson

Explicitly zero cr4 in prepare_64() instead of "zeroing" it in the common enter_long_mode(). Clobbering cr4 in enter_long_mode() breaks switch_to_5level(), which sets cr4.LA57 before calling enter_long_mode() and obviously expects cr4 to be preserved.

Quoted 8 lines
Fixes d86ef58 ("cstart: do not assume CR4 starts as zero")
Cc Nadav Amit <[email protected]>
Cc Paolo Bonzini <[email protected]>

---

Two lines of code, two bugs. I'm pretty sure Paolo should win some kind of award. :-D

I guess it is my fault for stressing him to push the changes so I can run it on the AMD machine that was lended to me.

maintainer
#2 lore raw
Quoted 13 lines · Nadav Amit
Quoted 13 lines · Sean Christopherson

Explicitly zero cr4 in prepare_64() instead of "zeroing" it in the common enter_long_mode(). Clobbering cr4 in enter_long_mode() breaks switch_to_5level(), which sets cr4.LA57 before calling enter_long_mode() and obviously expects cr4 to be preserved.

Fixes d86ef58 ("cstart: do not assume CR4 starts as zero")
Cc Nadav Amit <[email protected]>
Cc Paolo Bonzini <[email protected]>

---

Quoted 7 lines
Quoted 2 lines

Two lines of code, two bugs. I'm pretty sure Paolo should win some kind of award. :-D

I guess it is my fault for stressing him to push the changes so I can run it on the AMD machine that was lended to me.

I can blame you for this one but not for cstart.S. At least this made me realize that the bus factor is a bit low. Well, if I were really hit by a bus I guess you guys would figure out something, but for more short term issues I should ensure that someone else has write access to kvm.git. If no one volunteers, I'll ask Konstantin Ryabitsev to give back commit access to Marcelo Tosatti for emergency cases.

Paolo

maintainer
#3 lore raw
In reply to Sean Christopherson
Quoted 5 lines · Sean Christopherson

Explicitly zero cr4 in prepare_64() instead of "zeroing" it in the common enter_long_mode(). Clobbering cr4 in enter_long_mode() breaks switch_to_5level(), which sets cr4.LA57 before calling enter_long_mode() and obviously expects cr4 to be preserved.

Quoted 8 lines
Fixes d86ef58 ("cstart: do not assume CR4 starts as zero")
Cc Nadav Amit <[email protected]>
Cc Paolo Bonzini <[email protected]>

---

Two lines of code, two bugs. I'm pretty sure Paolo should win some kind of award. :-D

Two lines of code, two bugs immediately before disappearing for two weeks. 2^3 paper bags...

Paolo

Quoted 22 lines
 x86/cstart64.S | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
x86/cstart64.S +5 -1
index 3ae98d3..2d16688 100644
@@ -175,8 +175,12 @@ prepare_64:
 	lgdt gdt64_desc
 	setup_segments
 
+	xor %eax, %eax
+	mov %eax, %cr4
+
 enter_long_mode:
-	mov $(1 << 5), %eax // pae
+	mov %cr4, %eax
+	bts $5, %eax  // pae
 	mov %eax, %cr4
 
 	mov pt_root, %eax