| Message ID | 20200623084132.36213-1-namit@vmware.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] x86: Initialize segment selectors | expand |
On Tue, Jun 23, 2020 at 1:44 AM Nadav Amit <namit@vmware.com> wrote: > > Currently, the BSP's segment selectors are not initialized in 32-bit > (cstart.S). As a result the tests implicitly rely on the segment > selector values that are set by the BIOS. If this assumption is not > kept, the task-switch test fails. > > Fix it by initializing them. > > Signed-off-by: Nadav Amit <namit@vmware.com> Reviewed-by: Jim Mattson <jmattson@google.com>
On 23/06/20 10:41, Nadav Amit wrote: > Currently, the BSP's segment selectors are not initialized in 32-bit > (cstart.S). As a result the tests implicitly rely on the segment > selector values that are set by the BIOS. If this assumption is not > kept, the task-switch test fails. > > Fix it by initializing them. > > Signed-off-by: Nadav Amit <namit@vmware.com> > --- > x86/cstart.S | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/x86/cstart.S b/x86/cstart.S > index fa62e09..5ad70b5 100644 > --- a/x86/cstart.S > +++ b/x86/cstart.S > @@ -94,6 +94,15 @@ MSR_GS_BASE = 0xc0000101 > wrmsr > .endm > > +.macro setup_segments > + mov $0x10, %ax > + mov %ax, %ds > + mov %ax, %es > + mov %ax, %fs > + mov %ax, %gs > + mov %ax, %ss > +.endm > + > .globl start > start: > mov $stacktop, %esp > @@ -109,6 +118,7 @@ start: > > prepare_32: > lgdtl gdt32_descr > + setup_segments > > mov %cr4, %eax > bts $4, %eax // pse > @@ -133,12 +143,7 @@ save_id: > retl > > ap_start32: > - mov $0x10, %ax > - mov %ax, %ds > - mov %ax, %es > - mov %ax, %fs > - mov %ax, %gs > - mov %ax, %ss > + setup_segments > mov $-4096, %esp > lock/xaddl %esp, smp_stacktop > setup_percpu_area > Applied, thanks. Paolo
On 23/06/2020 10.41, Nadav Amit wrote: > Currently, the BSP's segment selectors are not initialized in 32-bit > (cstart.S). As a result the tests implicitly rely on the segment > selector values that are set by the BIOS. If this assumption is not > kept, the task-switch test fails. > > Fix it by initializing them. > > Signed-off-by: Nadav Amit <namit@vmware.com> > --- > x86/cstart.S | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) I'm sorry to be the bearer of bad news again, but this commit broke another set of tests in the Travis CI: https://travis-ci.com/github/huth/kvm-unit-tests/jobs/353103187#L796 smptest, smptest3, kvmclock_test, hyperv_synic and hyperv_stimer are failing now in the 32-bit kvm-unit-tests :-( Thomas
On 24/06/20 14:52, Thomas Huth wrote: > On 23/06/2020 10.41, Nadav Amit wrote: >> Currently, the BSP's segment selectors are not initialized in 32-bit >> (cstart.S). As a result the tests implicitly rely on the segment >> selector values that are set by the BIOS. If this assumption is not >> kept, the task-switch test fails. >> >> Fix it by initializing them. >> >> Signed-off-by: Nadav Amit <namit@vmware.com> >> --- >> x86/cstart.S | 17 +++++++++++------ >> 1 file changed, 11 insertions(+), 6 deletions(-) > > I'm sorry to be the bearer of bad news again, but this commit broke > another set of tests in the Travis CI: > > https://travis-ci.com/github/huth/kvm-unit-tests/jobs/353103187#L796 > > smptest, smptest3, kvmclock_test, hyperv_synic and hyperv_stimer are > failing now in the 32-bit kvm-unit-tests :-( And that's just bad testing (both Nadav's and mine). Writing to %gs clobbers the PERCPU area. The fix is as simple as this: diff --git a/x86/cstart.S b/x86/cstart.S index 5ad70b5..77dc34d 100644 --- a/x86/cstart.S +++ b/x86/cstart.S @@ -106,6 +106,7 @@ MSR_GS_BASE = 0xc0000101 .globl start start: mov $stacktop, %esp + setup_segments push %ebx call setup_multiboot call setup_libcflat @@ -118,7 +119,6 @@ start: prepare_32: lgdtl gdt32_descr - setup_segments mov %cr4, %eax bts $4, %eax // pse
> On Jun 24, 2020, at 7:10 AM, Paolo Bonzini <pbonzini@redhat.com> wrote: > > On 24/06/20 14:52, Thomas Huth wrote: >> On 23/06/2020 10.41, Nadav Amit wrote: >>> Currently, the BSP's segment selectors are not initialized in 32-bit >>> (cstart.S). As a result the tests implicitly rely on the segment >>> selector values that are set by the BIOS. If this assumption is not >>> kept, the task-switch test fails. >>> >>> Fix it by initializing them. >>> >>> Signed-off-by: Nadav Amit <namit@vmware.com> >>> --- >>> x86/cstart.S | 17 +++++++++++------ >>> 1 file changed, 11 insertions(+), 6 deletions(-) >> >> I'm sorry to be the bearer of bad news again, but this commit broke >> another set of tests in the Travis CI: >> >> https://travis-ci.com/github/huth/kvm-unit-tests/jobs/353103187#L796 >> >> smptest, smptest3, kvmclock_test, hyperv_synic and hyperv_stimer are >> failing now in the 32-bit kvm-unit-tests :-( > > And that's just bad testing (both Nadav's and mine). Writing to %gs > clobbers the PERCPU area. The fix is as simple as this: > > diff --git a/x86/cstart.S b/x86/cstart.S > index 5ad70b5..77dc34d 100644 > --- a/x86/cstart.S > +++ b/x86/cstart.S > @@ -106,6 +106,7 @@ MSR_GS_BASE = 0xc0000101 > .globl start > start: > mov $stacktop, %esp > + setup_segments > push %ebx > call setup_multiboot > call setup_libcflat > @@ -118,7 +119,6 @@ start: > > prepare_32: > lgdtl gdt32_descr > - setup_segments > > mov %cr4, %eax > bts $4, %eax // pse Unfortunately this change does not work for me (i.e., it breaks the task-switch test on my setup). Admittedly, my patch was wrong. I actually did not notice this scheme of having the GS descriptor and GS-base out of sync. It seems very fragile, specifically when you have a task-switch that reloads the GS and overwrites the original value. I will try to investigate it further later.
On 24/06/20 19:28, Nadav Amit wrote: > Unfortunately this change does not work for me (i.e., it breaks the > task-switch test on my setup). Admittedly, my patch was wrong. > > I actually did not notice this scheme of having the GS descriptor and > GS-base out of sync. It seems very fragile, specifically when you have a > task-switch that reloads the GS and overwrites the original value. > > I will try to investigate it further later. Yeah, it's fragile but it's limited to the task switch tests; and it's the only way to do it without having a separate selector or LDT for each CPU. Paolo
diff --git a/x86/cstart.S b/x86/cstart.S index fa62e09..5ad70b5 100644 --- a/x86/cstart.S +++ b/x86/cstart.S @@ -94,6 +94,15 @@ MSR_GS_BASE = 0xc0000101 wrmsr .endm +.macro setup_segments + mov $0x10, %ax + mov %ax, %ds + mov %ax, %es + mov %ax, %fs + mov %ax, %gs + mov %ax, %ss +.endm + .globl start start: mov $stacktop, %esp @@ -109,6 +118,7 @@ start: prepare_32: lgdtl gdt32_descr + setup_segments mov %cr4, %eax bts $4, %eax // pse @@ -133,12 +143,7 @@ save_id: retl ap_start32: - mov $0x10, %ax - mov %ax, %ds - mov %ax, %es - mov %ax, %fs - mov %ax, %gs - mov %ax, %ss + setup_segments mov $-4096, %esp lock/xaddl %esp, smp_stacktop setup_percpu_area
Currently, the BSP's segment selectors are not initialized in 32-bit (cstart.S). As a result the tests implicitly rely on the segment selector values that are set by the BIOS. If this assumption is not kept, the task-switch test fails. Fix it by initializing them. Signed-off-by: Nadav Amit <namit@vmware.com> --- x86/cstart.S | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)