> On May 10, 2019, at 4:52 PM, Nadav Amit <nadav.amit@xxxxxxxxx> wrote:
>
>> On May 10, 2019, at 4:37 PM, Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx> wrote:
>>
>>
>>
>> On 05/02/2019 07:08 AM, nadav.amit@xxxxxxxxx wrote:
>>> From: Nadav Amit <nadav.amit@xxxxxxxxx>
>>>
>>> After the APIC is reset, some of its registers might be reset. As the
>>> SDM says: "When IA32_APIC_BASE[11] is set to 0, prior initialization to
>>> the APIC may be lost and the APIC may return to the state described in
>>> Section 10.4.7.1". The SDM also says that after APIC reset "the
>>> spurious-interrupt vector register is initialized to 000000FFH". This
>>> means that after the APIC is reset it needs to be software-enabled
>>> through the SPIV.
>>>
>>> This is done one occasion, but there are (at least) two occasions that
>>> do not software-enable the APIC after reset (__test_apic_id() and main()
>>> in vmx.c).
>>>
>>> Move APIC SPIV reinitialization into reset_apic(). Remove SPIV settings
>>> which are unnecessary after reset_apic() is modified.
>>>
>>> Cc: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
>>> Signed-off-by: Nadav Amit <nadav.amit@xxxxxxxxx>
>>> ---
>>> lib/x86/apic.c | 1 +
>>> x86/apic.c | 1 -
>>> 2 files changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/lib/x86/apic.c b/lib/x86/apic.c
>>> index 2aeffbd..4e7d43c 100644
>>> --- a/lib/x86/apic.c
>>> +++ b/lib/x86/apic.c
>>> @@ -161,6 +161,7 @@ void reset_apic(void)
>>> {
>>> disable_apic();
>>> wrmsr(MSR_IA32_APICBASE, rdmsr(MSR_IA32_APICBASE) | APIC_EN);
>>> + apic_write(APIC_SPIV, 0x1ff);
>>> }
>>> u32 ioapic_read_reg(unsigned reg)
>>> diff --git a/x86/apic.c b/x86/apic.c
>>> index 3eff588..7ef4a27 100644
>>> --- a/x86/apic.c
>>> +++ b/x86/apic.c
>>> @@ -148,7 +148,6 @@ static void test_apic_disable(void)
>>> verify_disabled_apic_mmio();
>>> reset_apic();
>>> - apic_write(APIC_SPIV, 0x1ff);
>>> report("Local apic enabled in xAPIC mode",
>>> (rdmsr(MSR_IA32_APICBASE) & (APIC_EN | APIC_EXTD)) == APIC_EN);
>>> report("CPUID.1H:EDX.APIC[bit 9] is set", cpuid(1).d & (1 << 9));
>>
>> While you are at it, would you mind replacing "0xf0" with APIC_SPIV in enable_apic() also ?
>>
>> Reviewed-by: Krish Sadhukhan <krish.sadhukhan@xxxxxxxxxx>
>
> Will do. Thanks.
Actually, adding write to APIC_SPIV to reset_apic() was wrong. I’ll send a
fixed version later that just modifies the places where APIC_SPIV should be
reset.
