[kvm-unit-tests PATCH] cstart64: do not assume CR4 should be zero

[kvm-unit-tests PATCH] cstart64: do not assume CR4 should be zero

[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 <namit@vmware.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---

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

diff --git a/x86/cstart64.S b/x86/cstart64.S
index 3ae98d3..2d16688 100644
--- a/x86/cstart64.S
+++ b/x86/cstart64.S
@@ -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
-- 
2.26.0

Re: [kvm-unit-tests PATCH] cstart64: do not assume CR4 should be zero

[Nadav Amit]

> On Jul 15, 2020, at 1:52 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 
> 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 <namit@vmware.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
> 
> 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.

Reviewed-by: Nadav Amit <namit@vmware.com>

Re: [kvm-unit-tests PATCH] cstart64: do not assume CR4 should be zero

[Paolo Bonzini]

On 15/07/20 22:52, Sean Christopherson wrote:
> 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 <namit@vmware.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
> 
> 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

>  x86/cstart64.S | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/x86/cstart64.S b/x86/cstart64.S
> index 3ae98d3..2d16688 100644
> --- a/x86/cstart64.S
> +++ b/x86/cstart64.S
> @@ -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
> 

Re: [kvm-unit-tests PATCH] cstart64: do not assume CR4 should be zero

[Paolo Bonzini]

On 15/07/20 23:46, Nadav Amit wrote:
>> On Jul 15, 2020, at 1:52 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
>>
>> 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 <namit@vmware.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
>> ---
>>
>> 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.
> 
> Reviewed-by: Nadav Amit <namit@vmware.com>

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