[kvm-unit-tests PATCH] x86: Reset lapic after boot

[kvm-unit-tests PATCH] x86: Reset lapic after boot

[Nadav Amit]

Do not assume that the local APIC is in a xAPIC mode after reset.
Instead reset it first, since it might be in x2APIC mode, from which a
transition in xAPIC is invalid.

Note that we do not use the existing disable_apic() for the matter,
since it also re-initializes apic_ops.

Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
---
 x86/cstart64.S | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/x86/cstart64.S b/x86/cstart64.S
index 9791282..03726a6 100644
--- a/x86/cstart64.S
+++ b/x86/cstart64.S
@@ -118,6 +118,15 @@ MSR_GS_BASE = 0xc0000101
 	wrmsr
 .endm
 
+lapic_reset:
+	mov $0x1b, %ecx
+	rdmsr
+	and $~(APIC_EN | APIC_EXTD), %eax
+	wrmsr
+	or $(APIC_EN), %eax
+	wrmsr
+	ret
+
 .macro setup_segments
 	mov $MSR_GS_BASE, %ecx
 	rdmsr
@@ -228,6 +237,7 @@ save_id:
 	retq
 
 ap_start64:
+	call lapic_reset
 	call load_tss
 	call enable_apic
 	call save_id
@@ -240,6 +250,7 @@ ap_start64:
 	jmp 1b
 
 start64:
+	call lapic_reset
 	call load_tss
 	call mask_pic_interrupts
 	call enable_apic
-- 
2.17.1

Re: [kvm-unit-tests PATCH] x86: Reset lapic after boot

[Krish Sadhukhan]

On 6/25/19 5:10 AM, Nadav Amit wrote:
> Do not assume that the local APIC is in a xAPIC mode after reset.
> Instead reset it first, since it might be in x2APIC mode, from which a
> transition in xAPIC is invalid.
>
> Note that we do not use the existing disable_apic() for the matter,
> since it also re-initializes apic_ops.


Is there any issue if apic_ops is reset ?


>
> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
> ---
>   x86/cstart64.S | 11 +++++++++++
>   1 file changed, 11 insertions(+)
>
> diff --git a/x86/cstart64.S b/x86/cstart64.S
> index 9791282..03726a6 100644
> --- a/x86/cstart64.S
> +++ b/x86/cstart64.S
> @@ -118,6 +118,15 @@ MSR_GS_BASE = 0xc0000101
>   	wrmsr
>   .endm
>   
> +lapic_reset:
> +	mov $0x1b, %ecx


Why not use MSR_IA32_APICBASE instead of 0x1b ?


> +	rdmsr
> +	and $~(APIC_EN | APIC_EXTD), %eax
> +	wrmsr
> +	or $(APIC_EN), %eax
> +	wrmsr
> +	ret
> +
>   .macro setup_segments
>   	mov $MSR_GS_BASE, %ecx
>   	rdmsr
> @@ -228,6 +237,7 @@ save_id:
>   	retq
>   
>   ap_start64:
> +	call lapic_reset
>   	call load_tss
>   	call enable_apic
>   	call save_id
> @@ -240,6 +250,7 @@ ap_start64:
>   	jmp 1b
>   
>   start64:
> +	call lapic_reset
>   	call load_tss
>   	call mask_pic_interrupts
>   	call enable_apic

Re: [kvm-unit-tests PATCH] x86: Reset lapic after boot

[Nadav Amit]

> On Jun 26, 2019, at 5:26 PM, Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote:
> 
> 
> On 6/25/19 5:10 AM, Nadav Amit wrote:
>> Do not assume that the local APIC is in a xAPIC mode after reset.
>> Instead reset it first, since it might be in x2APIC mode, from which a
>> transition in xAPIC is invalid.
>> 
>> Note that we do not use the existing disable_apic() for the matter,
>> since it also re-initializes apic_ops.
> 
> 
> Is there any issue if apic_ops is reset ?
> 
> 
>> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
>> ---
>>  x86/cstart64.S | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>> 
>> diff --git a/x86/cstart64.S b/x86/cstart64.S
>> index 9791282..03726a6 100644
>> --- a/x86/cstart64.S
>> +++ b/x86/cstart64.S
>> @@ -118,6 +118,15 @@ MSR_GS_BASE = 0xc0000101
>>  	wrmsr
>>  .endm
>>  +lapic_reset:
>> +	mov $0x1b, %ecx
> 
> 
> Why not use MSR_IA32_APICBASE instead of 0x1b ?

I don’t remember, but it does require taking care of MSR_GS_BASE.

I can include “msr.h” and remove MSR_GS_BASE and MSR_IA32_APICBASE. I’ll add
another patch to do so (since MSR_GS_BASE must be taken care of too).

Re: [kvm-unit-tests PATCH] x86: Reset lapic after boot

[Nadav Amit]

> On Jun 26, 2019, at 5:26 PM, Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote:
> 
> 
> On 6/25/19 5:10 AM, Nadav Amit wrote:
>> Do not assume that the local APIC is in a xAPIC mode after reset.
>> Instead reset it first, since it might be in x2APIC mode, from which a
>> transition in xAPIC is invalid.
>> 
>> Note that we do not use the existing disable_apic() for the matter,
>> since it also re-initializes apic_ops.
> 
> 
> Is there any issue if apic_ops is reset ?

So I checked again, and actually the problem was different. Beforehand, I
used reset_apic(), which used apic_ops to write to SPIV. And the race with
setting x2apic caused it to occasionally use the x2APIC MSR interface to set
SPIV, which triggered an exception.

I’ll send v2 that changes reset_apic() not to use apic_ops.

Thanks.