> On Apr 15, 2019, at 12:09 PM, Nadav Amit <nadav.amit@xxxxxxxxx> wrote:
>
>> On Apr 15, 2019, at 12:00 PM, Sean Christopherson <sean.j.christopherson@xxxxxxxxx> wrote:
>>
>> On Mon, Apr 15, 2019 at 11:31:00AM -0700, nadav.amit@xxxxxxxxx wrote:
>>> From: Nadav Amit <nadav.amit@xxxxxxxxx>
>>>
>>> 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@xxxxxxxxx>
>>> ---
>>> 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.
