| Message ID | 20190625121042.8957-1-nadav.amit@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests] x86: Reset lapic after boot | expand |
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
> 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).
> 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.
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
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(+)