[kvm-unit-tests PATCH] x86: Prevent APIC base address from changing in test_enable_x2apic()

[kvm-unit-tests PATCH] x86: Prevent APIC base address from changing in test_enable_x2apic()

[nadav.amit]

From: Nadav Amit <nadav.amit@gmail.com>

test_enable_x2apic() unintentionally changes the APIC base address to
zero and resets the BSP indication. This actually causes the local APIC
to overlap the IDT area, which is wrong. Prevent it from happening by
keeping the APIC base address and BSP-bit as it was before.

Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
---
 x86/apic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/x86/apic.c b/x86/apic.c
index 51744cf..0849f87 100644
--- a/x86/apic.c
+++ b/x86/apic.c
@@ -90,11 +90,11 @@ static void test_enable_x2apic(void)
         report("disabled to x2apic enabled",
                test_write_apicbase_exception(APIC_EN | APIC_EXTD));
 
-        wrmsr(MSR_IA32_APICBASE, APIC_EN);
+        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_DEFAULT_PHYS_BASE | APIC_BSP);
         report("apic enabled to invalid state",
                test_write_apicbase_exception(APIC_EXTD));
 
-        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_EXTD);
+        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_EXTD | APIC_DEFAULT_PHYS_BASE | APIC_BSP);
         apic_write(APIC_SPIV, 0x1ff);
     } else {
         printf("x2apic not detected\n");
-- 
2.17.1

Re: [kvm-unit-tests PATCH] x86: Prevent APIC base address from changing in test_enable_x2apic()

[Sean Christopherson]

[-- Attachment #1: Type: text/plain, Size: 1707 bytes --]

On Mon, Apr 15, 2019 at 11:31:00AM -0700, nadav.amit@gmail.com wrote:
> From: Nadav Amit <nadav.amit@gmail.com>
> 
> test_enable_x2apic() unintentionally changes the APIC base address to
> zero and resets the BSP indication. This actually causes the local APIC
> to overlap the IDT area, which is wrong. Prevent it from happening by
> keeping the APIC base address and BSP-bit as it was before.
> 
> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
> ---
>  x86/apic.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/x86/apic.c b/x86/apic.c
> index 51744cf..0849f87 100644
> --- a/x86/apic.c
> +++ b/x86/apic.c
> @@ -90,11 +90,11 @@ static void test_enable_x2apic(void)
>          report("disabled to x2apic enabled",
>                 test_write_apicbase_exception(APIC_EN | APIC_EXTD));
>  
> -        wrmsr(MSR_IA32_APICBASE, APIC_EN);
> +        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_DEFAULT_PHYS_BASE | APIC_BSP);
>          report("apic enabled to invalid state",
>                 test_write_apicbase_exception(APIC_EXTD));
>  
> -        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_EXTD);
> +        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_EXTD | APIC_DEFAULT_PHYS_BASE | APIC_BSP);

It probably doesn't matter since AFAIK kvm-unit-tests always uses the
default base, but preserving the current base+BSP would be preferred.
The #GP tests get away with using APIC_DEFAULT_PHYS_BASE because the
WRMSR will never succeed, but even that is poor form.  And the test
should also reset to xAPIC when it's done.

I think the attached patch covers everything.

>          apic_write(APIC_SPIV, 0x1ff);
>      } else {
>          printf("x2apic not detected\n");
> -- 
> 2.17.1
> 

[-- Attachment #2: 0001-x86-apic-Preserve-APIC-base-and-BSP-bits-during-x2AP.patch --]
[-- Type: text/x-diff, Size: 2541 bytes --]

From 0905302c23f20f67483b281e33f1930135087e3a Mon Sep 17 00:00:00 2001
From: Sean Christopherson <sean.j.christopherson@intel.com>
Date: Mon, 15 Apr 2019 11:53:00 -0700
Subject: [PATCH] x86: apic: Preserve APIC base and BSP bits during x2APIC
 tests

...and reset to xAPIC mode unless x2APIC was already enabled.

Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
---
 x86/apic.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/x86/apic.c b/x86/apic.c
index 51744cf..02b6128 100644
--- a/x86/apic.c
+++ b/x86/apic.c
@@ -69,33 +69,39 @@ static void do_write_apicbase(void *data)
 
 static bool test_write_apicbase_exception(u64 data)
 {
-    data |= APIC_DEFAULT_PHYS_BASE | APIC_BSP;
     return test_for_exception(GP_VECTOR, do_write_apicbase, &data);
 }
 
 static void test_enable_x2apic(void)
 {
+    u64 orig_apicbase = rdmsr(MSR_IA32_APICBASE);
+    u64 apicbase;
+
     if (enable_x2apic()) {
         printf("x2apic enabled\n");
 
+        apicbase = orig_apicbase & ~(APIC_EN | APIC_EXTD);
         report("x2apic enabled to invalid state",
-               test_write_apicbase_exception(APIC_EXTD));
+               test_write_apicbase_exception(apicbase | APIC_EXTD));
         report("x2apic enabled to apic enabled",
-               test_write_apicbase_exception(APIC_EN));
+               test_write_apicbase_exception(apicbase | APIC_EN));
 
         report("x2apic enabled to disabled state",
-               !test_write_apicbase_exception(0));
+               !test_write_apicbase_exception(apicbase | 0));
         report("disabled to invalid state",
-               test_write_apicbase_exception(APIC_EXTD));
+               test_write_apicbase_exception(apicbase | APIC_EXTD));
         report("disabled to x2apic enabled",
-               test_write_apicbase_exception(APIC_EN | APIC_EXTD));
+               test_write_apicbase_exception(apicbase | APIC_EN | APIC_EXTD));
 
-        wrmsr(MSR_IA32_APICBASE, APIC_EN);
+        wrmsr(MSR_IA32_APICBASE, apicbase | APIC_EN);
         report("apic enabled to invalid state",
-               test_write_apicbase_exception(APIC_EXTD));
+               test_write_apicbase_exception(apicbase | APIC_EXTD));
 
-        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_EXTD);
+        wrmsr(MSR_IA32_APICBASE, apicbase | APIC_EN | APIC_EXTD);
         apic_write(APIC_SPIV, 0x1ff);
+
+        if (!(orig_apicbase & APIC_EXTD))
+            reset_apic();
     } else {
         printf("x2apic not detected\n");
 
-- 
2.21.0

Re: [kvm-unit-tests PATCH] x86: Prevent APIC base address from changing in test_enable_x2apic()

[Nadav Amit]

> On Apr 15, 2019, at 12:00 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 
> On Mon, Apr 15, 2019 at 11:31:00AM -0700, nadav.amit@gmail.com wrote:
>> From: Nadav Amit <nadav.amit@gmail.com>
>> 
>> test_enable_x2apic() unintentionally changes the APIC base address to
>> zero and resets the BSP indication. This actually causes the local APIC
>> to overlap the IDT area, which is wrong. Prevent it from happening by
>> keeping the APIC base address and BSP-bit as it was before.
>> 
>> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
>> ---
>> x86/apic.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/x86/apic.c b/x86/apic.c
>> index 51744cf..0849f87 100644
>> --- a/x86/apic.c
>> +++ b/x86/apic.c
>> @@ -90,11 +90,11 @@ static void test_enable_x2apic(void)
>>         report("disabled to x2apic enabled",
>>                test_write_apicbase_exception(APIC_EN | APIC_EXTD));
>> 
>> -        wrmsr(MSR_IA32_APICBASE, APIC_EN);
>> +        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_DEFAULT_PHYS_BASE | APIC_BSP);
>>         report("apic enabled to invalid state",
>>                test_write_apicbase_exception(APIC_EXTD));
>> 
>> -        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_EXTD);
>> +        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_EXTD | APIC_DEFAULT_PHYS_BASE | APIC_BSP);
> 
> It probably doesn't matter since AFAIK kvm-unit-tests always uses the
> default base, but preserving the current base+BSP would be preferred.
> The #GP tests get away with using APIC_DEFAULT_PHYS_BASE because the
> WRMSR will never succeed, but even that is poor form.  And the test
> should also reset to xAPIC when it's done.
> 
> I think the attached patch covers everything.

Thanks. The base is indeed the default, so it should not matter, but your
change should make the code more robust.

One important comment regarding your patch:

> +        if (!(orig_apicbase & APIC_EXTD))
> +            reset_apic();

This is not needed, since enable_x2apic() will only return true if it
succeeded in enabling x2apic. In addition, this is wrong, since reset_apic()
should (usually, and specifically in this case) be followed with:

	apic_write(APIC_SPIV, 0x1ff);

And not the other way around (there are actually a couple of missing writes
to software-enable the APIC after reset_apic(), which I’ll send later).

Re: [kvm-unit-tests PATCH] x86: Prevent APIC base address from changing in test_enable_x2apic()

[Nadav Amit]

> On Apr 15, 2019, at 12:09 PM, Nadav Amit <nadav.amit@gmail.com> wrote:
> 
>> On Apr 15, 2019, at 12:00 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
>> 
>> On Mon, Apr 15, 2019 at 11:31:00AM -0700, nadav.amit@gmail.com wrote:
>>> From: Nadav Amit <nadav.amit@gmail.com>
>>> 
>>> test_enable_x2apic() unintentionally changes the APIC base address to
>>> zero and resets the BSP indication. This actually causes the local APIC
>>> to overlap the IDT area, which is wrong. Prevent it from happening by
>>> keeping the APIC base address and BSP-bit as it was before.
>>> 
>>> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
>>> ---
>>> x86/apic.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/x86/apic.c b/x86/apic.c
>>> index 51744cf..0849f87 100644
>>> --- a/x86/apic.c
>>> +++ b/x86/apic.c
>>> @@ -90,11 +90,11 @@ static void test_enable_x2apic(void)
>>>        report("disabled to x2apic enabled",
>>>               test_write_apicbase_exception(APIC_EN | APIC_EXTD));
>>> 
>>> -        wrmsr(MSR_IA32_APICBASE, APIC_EN);
>>> +        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_DEFAULT_PHYS_BASE | APIC_BSP);
>>>        report("apic enabled to invalid state",
>>>               test_write_apicbase_exception(APIC_EXTD));
>>> 
>>> -        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_EXTD);
>>> +        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_EXTD | APIC_DEFAULT_PHYS_BASE | APIC_BSP);
>> 
>> It probably doesn't matter since AFAIK kvm-unit-tests always uses the
>> default base, but preserving the current base+BSP would be preferred.
>> The #GP tests get away with using APIC_DEFAULT_PHYS_BASE because the
>> WRMSR will never succeed, but even that is poor form.  And the test
>> should also reset to xAPIC when it's done.
>> 
>> I think the attached patch covers everything.
> 
> Thanks. The base is indeed the default, so it should not matter, but your
> change should make the code more robust.
> 
> One important comment regarding your patch:
> 
>> +        if (!(orig_apicbase & APIC_EXTD))
>> +            reset_apic();
> 
> This is not needed, since enable_x2apic() will only return true if it
> succeeded in enabling x2apic. In addition, this is wrong, since reset_apic()
> should (usually, and specifically in this case) be followed with:
> 
> 	apic_write(APIC_SPIV, 0x1ff);
> 
> And not the other way around (there are actually a couple of missing writes
> to software-enable the APIC after reset_apic(), which I’ll send later).

Correcting myself - I understand (now) that you want to reset back to xapic, so
that’s fine. Just keep "apic_write(APIC_SPIV, 0x1ff);” after the reset, please.

Re: [kvm-unit-tests PATCH] x86: Prevent APIC base address from changing in test_enable_x2apic()

[Sean Christopherson]

On Mon, Apr 15, 2019 at 12:12:58PM -0700, Nadav Amit wrote:
> > On Apr 15, 2019, at 12:09 PM, Nadav Amit <nadav.amit@gmail.com> wrote:
> > 
> >> On Apr 15, 2019, at 12:00 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> >> 
> >> On Mon, Apr 15, 2019 at 11:31:00AM -0700, nadav.amit@gmail.com wrote:
> >>> From: Nadav Amit <nadav.amit@gmail.com>
> >>> 
> >>> test_enable_x2apic() unintentionally changes the APIC base address to
> >>> zero and resets the BSP indication. This actually causes the local APIC
> >>> to overlap the IDT area, which is wrong. Prevent it from happening by
> >>> keeping the APIC base address and BSP-bit as it was before.
> >>> 
> >>> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
> >>> ---
> >>> x86/apic.c | 4 ++--
> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>> 
> >>> diff --git a/x86/apic.c b/x86/apic.c
> >>> index 51744cf..0849f87 100644
> >>> --- a/x86/apic.c
> >>> +++ b/x86/apic.c
> >>> @@ -90,11 +90,11 @@ static void test_enable_x2apic(void)
> >>>        report("disabled to x2apic enabled",
> >>>               test_write_apicbase_exception(APIC_EN | APIC_EXTD));
> >>> 
> >>> -        wrmsr(MSR_IA32_APICBASE, APIC_EN);
> >>> +        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_DEFAULT_PHYS_BASE | APIC_BSP);
> >>>        report("apic enabled to invalid state",
> >>>               test_write_apicbase_exception(APIC_EXTD));
> >>> 
> >>> -        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_EXTD);
> >>> +        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_EXTD | APIC_DEFAULT_PHYS_BASE | APIC_BSP);
> >> 
> >> It probably doesn't matter since AFAIK kvm-unit-tests always uses the
> >> default base, but preserving the current base+BSP would be preferred.
> >> The #GP tests get away with using APIC_DEFAULT_PHYS_BASE because the
> >> WRMSR will never succeed, but even that is poor form.  And the test
> >> should also reset to xAPIC when it's done.
> >> 
> >> I think the attached patch covers everything.
> > 
> > Thanks. The base is indeed the default, so it should not matter, but your
> > change should make the code more robust.
> > 
> > One important comment regarding your patch:
> > 
> >> +        if (!(orig_apicbase & APIC_EXTD))
> >> +            reset_apic();
> > 
> > This is not needed, since enable_x2apic() will only return true if it
> > succeeded in enabling x2apic. In addition, this is wrong, since reset_apic()
> > should (usually, and specifically in this case) be followed with:
> > 
> > 	apic_write(APIC_SPIV, 0x1ff);
> > 
> > And not the other way around (there are actually a couple of missing writes
> > to software-enable the APIC after reset_apic(), which I’ll send later).
> 
> Correcting myself - I understand (now) that you want to reset back to xapic, so
> that’s fine. Just keep "apic_write(APIC_SPIV, 0x1ff);” after the reset, please.

Ah, will do, I glazed over the apic_write() and didn't consider its purpose.

Re: [kvm-unit-tests PATCH] x86: Prevent APIC base address from changing in test_enable_x2apic()

[Nadav Amit]

> On Apr 15, 2019, at 12:37 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> 
> On Mon, Apr 15, 2019 at 12:12:58PM -0700, Nadav Amit wrote:
>>> On Apr 15, 2019, at 12:09 PM, Nadav Amit <nadav.amit@gmail.com> wrote:
>>> 
>>>> On Apr 15, 2019, at 12:00 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
>>>> 
>>>> On Mon, Apr 15, 2019 at 11:31:00AM -0700, nadav.amit@gmail.com wrote:
>>>>> From: Nadav Amit <nadav.amit@gmail.com>
>>>>> 
>>>>> test_enable_x2apic() unintentionally changes the APIC base address to
>>>>> zero and resets the BSP indication. This actually causes the local APIC
>>>>> to overlap the IDT area, which is wrong. Prevent it from happening by
>>>>> keeping the APIC base address and BSP-bit as it was before.
>>>>> 
>>>>> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
>>>>> ---
>>>>> x86/apic.c | 4 ++--
>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>> 
>>>>> diff --git a/x86/apic.c b/x86/apic.c
>>>>> index 51744cf..0849f87 100644
>>>>> --- a/x86/apic.c
>>>>> +++ b/x86/apic.c
>>>>> @@ -90,11 +90,11 @@ static void test_enable_x2apic(void)
>>>>>       report("disabled to x2apic enabled",
>>>>>              test_write_apicbase_exception(APIC_EN | APIC_EXTD));
>>>>> 
>>>>> -        wrmsr(MSR_IA32_APICBASE, APIC_EN);
>>>>> +        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_DEFAULT_PHYS_BASE | APIC_BSP);
>>>>>       report("apic enabled to invalid state",
>>>>>              test_write_apicbase_exception(APIC_EXTD));
>>>>> 
>>>>> -        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_EXTD);
>>>>> +        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_EXTD | APIC_DEFAULT_PHYS_BASE | APIC_BSP);
>>>> 
>>>> It probably doesn't matter since AFAIK kvm-unit-tests always uses the
>>>> default base, but preserving the current base+BSP would be preferred.
>>>> The #GP tests get away with using APIC_DEFAULT_PHYS_BASE because the
>>>> WRMSR will never succeed, but even that is poor form.  And the test
>>>> should also reset to xAPIC when it's done.
>>>> 
>>>> I think the attached patch covers everything.
>>> 
>>> Thanks. The base is indeed the default, so it should not matter, but your
>>> change should make the code more robust.
>>> 
>>> One important comment regarding your patch:
>>> 
>>>> +        if (!(orig_apicbase & APIC_EXTD))
>>>> +            reset_apic();
>>> 
>>> This is not needed, since enable_x2apic() will only return true if it
>>> succeeded in enabling x2apic. In addition, this is wrong, since reset_apic()
>>> should (usually, and specifically in this case) be followed with:
>>> 
>>> 	apic_write(APIC_SPIV, 0x1ff);
>>> 
>>> And not the other way around (there are actually a couple of missing writes
>>> to software-enable the APIC after reset_apic(), which I’ll send later).
>> 
>> Correcting myself - I understand (now) that you want to reset back to xapic, so
>> that’s fine. Just keep "apic_write(APIC_SPIV, 0x1ff);” after the reset, please.
> 
> Ah, will do, I glazed over the apic_write() and didn't consider its purpose.

Sorry for not being consistent, but actually the apic_reset() seems wrong in
general.

enable_x2apic() is used in the context of test_enable_x2apic() to figure out
whether x2apic was enabled before. It is actually initially enabled after
boot (see cstart64.S). So disabling it here would not be appropriate.

Re: [kvm-unit-tests PATCH] x86: Prevent APIC base address from changing in test_enable_x2apic()

[Sean Christopherson]

On Mon, Apr 15, 2019 at 12:55:42PM -0700, Nadav Amit wrote:
> > On Apr 15, 2019, at 12:37 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> > 
> > On Mon, Apr 15, 2019 at 12:12:58PM -0700, Nadav Amit wrote:
> >>> On Apr 15, 2019, at 12:09 PM, Nadav Amit <nadav.amit@gmail.com> wrote:
> >>> 
> >>>> On Apr 15, 2019, at 12:00 PM, Sean Christopherson <sean.j.christopherson@intel.com> wrote:
> >>>> 
> >>>> On Mon, Apr 15, 2019 at 11:31:00AM -0700, nadav.amit@gmail.com wrote:
> >>>>> From: Nadav Amit <nadav.amit@gmail.com>
> >>>>> 
> >>>>> test_enable_x2apic() unintentionally changes the APIC base address to
> >>>>> zero and resets the BSP indication. This actually causes the local APIC
> >>>>> to overlap the IDT area, which is wrong. Prevent it from happening by
> >>>>> keeping the APIC base address and BSP-bit as it was before.
> >>>>> 
> >>>>> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
> >>>>> ---
> >>>>> x86/apic.c | 4 ++--
> >>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>>> 
> >>>>> diff --git a/x86/apic.c b/x86/apic.c
> >>>>> index 51744cf..0849f87 100644
> >>>>> --- a/x86/apic.c
> >>>>> +++ b/x86/apic.c
> >>>>> @@ -90,11 +90,11 @@ static void test_enable_x2apic(void)
> >>>>>       report("disabled to x2apic enabled",
> >>>>>              test_write_apicbase_exception(APIC_EN | APIC_EXTD));
> >>>>> 
> >>>>> -        wrmsr(MSR_IA32_APICBASE, APIC_EN);
> >>>>> +        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_DEFAULT_PHYS_BASE | APIC_BSP);
> >>>>>       report("apic enabled to invalid state",
> >>>>>              test_write_apicbase_exception(APIC_EXTD));
> >>>>> 
> >>>>> -        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_EXTD);
> >>>>> +        wrmsr(MSR_IA32_APICBASE, APIC_EN | APIC_EXTD | APIC_DEFAULT_PHYS_BASE | APIC_BSP);
> >>>> 
> >>>> It probably doesn't matter since AFAIK kvm-unit-tests always uses the
> >>>> default base, but preserving the current base+BSP would be preferred.
> >>>> The #GP tests get away with using APIC_DEFAULT_PHYS_BASE because the
> >>>> WRMSR will never succeed, but even that is poor form.  And the test
> >>>> should also reset to xAPIC when it's done.
> >>>> 
> >>>> I think the attached patch covers everything.
> >>> 
> >>> Thanks. The base is indeed the default, so it should not matter, but your
> >>> change should make the code more robust.
> >>> 
> >>> One important comment regarding your patch:
> >>> 
> >>>> +        if (!(orig_apicbase & APIC_EXTD))
> >>>> +            reset_apic();
> >>> 
> >>> This is not needed, since enable_x2apic() will only return true if it
> >>> succeeded in enabling x2apic. In addition, this is wrong, since reset_apic()
> >>> should (usually, and specifically in this case) be followed with:
> >>> 
> >>> 	apic_write(APIC_SPIV, 0x1ff);
> >>> 
> >>> And not the other way around (there are actually a couple of missing writes
> >>> to software-enable the APIC after reset_apic(), which I’ll send later).
> >> 
> >> Correcting myself - I understand (now) that you want to reset back to xapic, so
> >> that’s fine. Just keep "apic_write(APIC_SPIV, 0x1ff);” after the reset, please.
> > 
> > Ah, will do, I glazed over the apic_write() and didn't consider its purpose.
> 
> Sorry for not being consistent, but actually the apic_reset() seems wrong in
> general.
> 
> enable_x2apic() is used in the context of test_enable_x2apic() to figure out
> whether x2apic was enabled before. It is actually initially enabled after
> boot (see cstart64.S). So disabling it here would not be appropriate.

I more or less noticed the same thing.  Actually, I noticed we end up in
legacy xAPIC and so ended up with:

        if (orig_apicbase & APIC_EXTD)
            enable_x2apic();
        else
            reset_apic();

Along with a blurb in the changelog stating that reset_apic() is overkill
since the vCPU is already in xAPIC.