| Message ID | 20200715205235.13113-1-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] cstart64: do not assume CR4 should be zero | expand |
> 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>
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 >
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
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
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(-)