[kvm-unit-tests PATCH] x86: Initialize segment selectors

[kvm-unit-tests PATCH] x86: Initialize segment selectors

[Nadav Amit]

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
-- 
2.20.1

Re: [kvm-unit-tests PATCH] x86: Initialize segment selectors

[Jim Mattson]

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>

Re: [kvm-unit-tests PATCH] x86: Initialize segment selectors

[Paolo Bonzini]

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

Re: [kvm-unit-tests PATCH] x86: Initialize segment selectors

[Thomas Huth]

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

Re: [kvm-unit-tests PATCH] x86: Initialize segment selectors

[Paolo Bonzini]

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

Re: [kvm-unit-tests PATCH] x86: Initialize segment selectors

[Nadav Amit]

> 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.

Re: [kvm-unit-tests PATCH] x86: Initialize segment selectors

[Paolo Bonzini]

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