[kvm-unit-tests] i386: setup segment registers before percpu areas
diff mbox series

Message ID 20200624141429.382157-1-pbonzini@redhat.com
State New
Headers show
Series
  • [kvm-unit-tests] i386: setup segment registers before percpu areas
Related show

Commit Message

Paolo Bonzini June 24, 2020, 2:14 p.m. UTC
The base of the percpu area is stored in the %gs base, and writing
to %gs destroys it.  Move setup_segments earlier, before the %gs
base is written.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 x86/cstart.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Nadav Amit June 24, 2020, 7:58 p.m. UTC | #1
> On Jun 24, 2020, at 7:14 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> The base of the percpu area is stored in the %gs base, and writing
> to %gs destroys it.  Move setup_segments earlier, before the %gs
> base is written.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> x86/cstart.S | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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
> — 
> 2.26.2

As I said in a different thread, this change breaks my setup. It is better
not to make any assumption (or as few as possible) about the GDT content
after boot and load the GDTR before setting up the segments. So I prefer to
load the GDT before the segments. How about this change instead of yours?

-- >8 --

From: Nadav Amit <namit@vmware.com>
Date: Wed, 24 Jun 2020 19:50:36 +0000
Subject: [PATCH] x86: load gdt while loading segments

Signed-off-by: Nadav Amit <namit@vmware.com>
---
 x86/cstart.S | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/x86/cstart.S b/x86/cstart.S
index dd33d4d..1d8b8ac 100644
--- a/x86/cstart.S
+++ b/x86/cstart.S
@@ -95,6 +95,8 @@ MSR_GS_BASE = 0xc0000101
 .endm
 
 .macro setup_segments
+	lgdtl gdt32_descr
+
 	mov $0x10, %ax
 	mov %ax, %ds
 	mov %ax, %es
@@ -106,6 +108,8 @@ MSR_GS_BASE = 0xc0000101
 .globl start
 start:
         mov $stacktop, %esp
+	setup_segments
+
         push %ebx
         call setup_multiboot
         call setup_libcflat
@@ -117,9 +121,6 @@ start:
         jmpl $8, $start32
 
 prepare_32:
-        lgdtl gdt32_descr
-	setup_segments
-
 	mov %cr4, %eax
 	bts $4, %eax  // pse
 	mov %eax, %cr4
--
Paolo Bonzini June 25, 2020, 8:01 a.m. UTC | #2
On 24/06/20 21:58, Nadav Amit wrote:
>> On Jun 24, 2020, at 7:14 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> The base of the percpu area is stored in the %gs base, and writing
>> to %gs destroys it.  Move setup_segments earlier, before the %gs
>> base is written.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>> x86/cstart.S | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> 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
>> — 
>> 2.26.2
> 
> As I said in a different thread, this change breaks my setup. It is better
> not to make any assumption (or as few as possible) about the GDT content
> after boot and load the GDTR before setting up the segments. So I prefer to
> load the GDT before the segments. How about this change instead of yours?

Yes, this is better.

Paolo

> -- >8 --
> 
> From: Nadav Amit <namit@vmware.com>
> Date: Wed, 24 Jun 2020 19:50:36 +0000
> Subject: [PATCH] x86: load gdt while loading segments
> 
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
>  x86/cstart.S | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/x86/cstart.S b/x86/cstart.S
> index dd33d4d..1d8b8ac 100644
> --- a/x86/cstart.S
> +++ b/x86/cstart.S
> @@ -95,6 +95,8 @@ MSR_GS_BASE = 0xc0000101
>  .endm
>  
>  .macro setup_segments
> +	lgdtl gdt32_descr
> +
>  	mov $0x10, %ax
>  	mov %ax, %ds
>  	mov %ax, %es
> @@ -106,6 +108,8 @@ MSR_GS_BASE = 0xc0000101
>  .globl start
>  start:
>          mov $stacktop, %esp
> +	setup_segments
> +
>          push %ebx
>          call setup_multiboot
>          call setup_libcflat
> @@ -117,9 +121,6 @@ start:
>          jmpl $8, $start32
>  
>  prepare_32:
> -        lgdtl gdt32_descr
> -	setup_segments
> -
>  	mov %cr4, %eax
>  	bts $4, %eax  // pse
>  	mov %eax, %cr4
>
Paolo Bonzini June 25, 2020, 11:09 a.m. UTC | #3
On 24/06/20 21:58, Nadav Amit wrote:
> From: Nadav Amit <namit@vmware.com>
> Date: Wed, 24 Jun 2020 19:50:36 +0000
> Subject: [PATCH] x86: load gdt while loading segments
> 
> Signed-off-by: Nadav Amit <namit@vmware.com>
> ---
>  x86/cstart.S | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/x86/cstart.S b/x86/cstart.S
> index dd33d4d..1d8b8ac 100644
> --- a/x86/cstart.S
> +++ b/x86/cstart.S
> @@ -95,6 +95,8 @@ MSR_GS_BASE = 0xc0000101
>  .endm
>  
>  .macro setup_segments
> +	lgdtl gdt32_descr
> +
>  	mov $0x10, %ax
>  	mov %ax, %ds
>  	mov %ax, %es
> @@ -106,6 +108,8 @@ MSR_GS_BASE = 0xc0000101
>  .globl start
>  start:
>          mov $stacktop, %esp
> +	setup_segments
> +
>          push %ebx
>          call setup_multiboot
>          call setup_libcflat
> @@ -117,9 +121,6 @@ start:
>          jmpl $8, $start32
>  
>  prepare_32:
> -        lgdtl gdt32_descr
> -	setup_segments
> -
>  	mov %cr4, %eax
>  	bts $4, %eax  // pse
>  	mov %eax, %cr4
> -- 

The GDT is already loaded elsewhere for APs, but the gist of the patch
is good.  I'll send v2

Paolo

Patch
diff mbox series

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