diff mbox series

[kvm-unit-tests] x86: Set "APIC Software Enable" after APIC reset

Message ID 20190502140856.4136-1-nadav.amit@gmail.com (mailing list archive)
State New, archived
Headers show
Series [kvm-unit-tests] x86: Set "APIC Software Enable" after APIC reset | expand

Commit Message

Nadav Amit May 2, 2019, 2:08 p.m. UTC 
From: Nadav Amit <nadav.amit@gmail.com>

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@intel.com>
Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
---
 lib/x86/apic.c | 1 +
 x86/apic.c     | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

Comments

Krish Sadhukhan May 10, 2019, 11:37 p.m. UTC | #1 
On 05/02/2019 07:08 AM, nadav.amit@gmail.com wrote:
> From: Nadav Amit <nadav.amit@gmail.com>
>
> 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@intel.com>
> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
> ---
>   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@oracle.com>
Nadav Amit May 10, 2019, 11:52 p.m. UTC | #2 
> On May 10, 2019, at 4:37 PM, Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote:
> 
> 
> 
> On 05/02/2019 07:08 AM, nadav.amit@gmail.com wrote:
>> From: Nadav Amit <nadav.amit@gmail.com>
>> 
>> 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@intel.com>
>> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
>> ---
>>  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@oracle.com>

Will do. Thanks.
Nadav Amit May 14, 2019, 1:06 a.m. UTC | #3 
> On May 10, 2019, at 4:52 PM, Nadav Amit <nadav.amit@gmail.com> wrote:
> 
>> On May 10, 2019, at 4:37 PM, Krish Sadhukhan <krish.sadhukhan@oracle.com> wrote:
>> 
>> 
>> 
>> On 05/02/2019 07:08 AM, nadav.amit@gmail.com wrote:
>>> From: Nadav Amit <nadav.amit@gmail.com>
>>> 
>>> 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@intel.com>
>>> Signed-off-by: Nadav Amit <nadav.amit@gmail.com>
>>> ---
>>> 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@oracle.com>
> 
> 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.
diff mbox series

Patch

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