> 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).
