[PATCH] kvm: nVMX: CPUID.01H:EDX.APIC[bit 9] should mirror IA32_APIC

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH] kvm: nVMX: CPUID.01H:EDX.APIC[bit 9] should mirror IA32_APIC_BASE[11]
@ 2016-11-04 22:00 Jim Mattson
  2016-11-08 16:33 ` Radim Krčmář
  0 siblings, 1 reply; 17+ messages in thread
From: Jim Mattson @ 2016-11-04 22:00 UTC (permalink / raw)
  To: kvm; +Cc: Jim Mattson

>From the Intel SDM, volume 3, section 10.4.3, "Enabling or Disabling the
Local APIC,"

  When IA32_APIC_BASE[11] is 0, the processor is functionally equivalent
  to an IA-32 processor without an on-chip APIC. The CPUID feature flag
  for the APIC (see Section 10.4.2, "Presence of the Local APIC") is
  also set to 0.

Signed-off-by: Jim Mattson <jmattson@google.com>
Reviewed-by: Ben Serebrin <serebrin@google.com>
Reviewed-by: David Matlack <dmatlack@google.com>
---
 arch/x86/kvm/cpuid.c | 3 +++
 arch/x86/kvm/lapic.c | 1 +
 2 files changed, 4 insertions(+)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index afa7bbb..0a49fd2 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -81,7 +81,10 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
 			best->ecx |= F(OSXSAVE);
 	}
 
+	best->edx &= ~F(APIC);
 	if (apic) {
+		if (vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE)
+			best->edx |= F(APIC);
 		if (best->ecx & F(TSC_DEADLINE_TIMER))
 			apic->lapic_timer.timer_mode_mask = 3 << 17;
 		else
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 23b99f3..eda4284e 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1758,6 +1758,7 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
 
 	/* update jump label if enable bit changes */
 	if ((old_value ^ value) & MSR_IA32_APICBASE_ENABLE) {
+		kvm_update_cpuid(vcpu);
 		if (value & MSR_IA32_APICBASE_ENABLE) {
 			kvm_apic_set_xapic_id(apic, vcpu->vcpu_id);
 			static_key_slow_dec_deferred(&apic_hw_disabled);
-- 
2.8.0.rc3.226.g39d4020


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH] kvm: nVMX: CPUID.01H:EDX.APIC[bit 9] should mirror IA32_APIC_BASE[11]
  2016-11-04 22:00 [PATCH] kvm: nVMX: CPUID.01H:EDX.APIC[bit 9] should mirror IA32_APIC_BASE[11] Jim Mattson
@ 2016-11-08 16:33 ` Radim Krčmář
  2016-11-09 16:53   ` Jim Mattson
  0 siblings, 1 reply; 17+ messages in thread
From: Radim Krčmář @ 2016-11-08 16:33 UTC (permalink / raw)
  To: Jim Mattson; +Cc: kvm

2016-11-04 15:00-0700, Jim Mattson:
> From the Intel SDM, volume 3, section 10.4.3, "Enabling or Disabling the
> Local APIC,"
> 
>   When IA32_APIC_BASE[11] is 0, the processor is functionally equivalent
>   to an IA-32 processor without an on-chip APIC. The CPUID feature flag
>   for the APIC (see Section 10.4.2, "Presence of the Local APIC") is
>   also set to 0.
> 
> Signed-off-by: Jim Mattson <jmattson@google.com>
> Reviewed-by: Ben Serebrin <serebrin@google.com>
> Reviewed-by: David Matlack <dmatlack@google.com>
> ---
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> @@ -81,7 +81,10 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
>  			best->ecx |= F(OSXSAVE);
>  	}
>  
> +	best->edx &= ~F(APIC);

This might prevent userspace LAPIC from working.
(The bit will always be zero.)

>  	if (apic) {
> +		if (vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE)
> +			best->edx |= F(APIC);

vcpu->arch.apic_base should be correct regardless of lapic_in_kernel().
Userspace can update CPUID when it changes MSR_IA32_APICBASE_ENABLE, so
not handling CPUID update in KVM is fine, but KVM must not touch the
CPUID bit in that case.

>  		if (best->ecx & F(TSC_DEADLINE_TIMER))
>  			apic->lapic_timer.timer_mode_mask = 3 << 17;
>  		else

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH] kvm: nVMX: CPUID.01H:EDX.APIC[bit 9] should mirror IA32_APIC_BASE[11]
  2016-11-08 16:33 ` Radim Krčmář
@ 2016-11-09 16:53   ` Jim Mattson
  2016-11-09 17:04     ` [PATCH v2] " Jim Mattson
  0 siblings, 1 reply; 17+ messages in thread
From: Jim Mattson @ 2016-11-09 16:53 UTC (permalink / raw)
  To: Radim Krčmář; +Cc: kvm

Oops. I missed the subtlety of the "if (apic)" test. I thought it was
testing for the existence of an APIC, rather than where the APIC was
implemented.

On Tue, Nov 8, 2016 at 8:33 AM, Radim Krčmář <rkrcmar@redhat.com> wrote:
> 2016-11-04 15:00-0700, Jim Mattson:
>> From the Intel SDM, volume 3, section 10.4.3, "Enabling or Disabling the
>> Local APIC,"
>>
>>   When IA32_APIC_BASE[11] is 0, the processor is functionally equivalent
>>   to an IA-32 processor without an on-chip APIC. The CPUID feature flag
>>   for the APIC (see Section 10.4.2, "Presence of the Local APIC") is
>>   also set to 0.
>>
>> Signed-off-by: Jim Mattson <jmattson@google.com>
>> Reviewed-by: Ben Serebrin <serebrin@google.com>
>> Reviewed-by: David Matlack <dmatlack@google.com>
>> ---
>> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
>> @@ -81,7 +81,10 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
>>                       best->ecx |= F(OSXSAVE);
>>       }
>>
>> +     best->edx &= ~F(APIC);
>
> This might prevent userspace LAPIC from working.
> (The bit will always be zero.)
>
>>       if (apic) {
>> +             if (vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE)
>> +                     best->edx |= F(APIC);
>
> vcpu->arch.apic_base should be correct regardless of lapic_in_kernel().
> Userspace can update CPUID when it changes MSR_IA32_APICBASE_ENABLE, so
> not handling CPUID update in KVM is fine, but KVM must not touch the
> CPUID bit in that case.
>
>>               if (best->ecx & F(TSC_DEADLINE_TIMER))
>>                       apic->lapic_timer.timer_mode_mask = 3 << 17;
>>               else

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2] kvm: nVMX: CPUID.01H:EDX.APIC[bit 9] should mirror IA32_APIC_BASE[11]
  2016-11-09 16:53   ` Jim Mattson
@ 2016-11-09 17:04     ` Jim Mattson
  2016-11-09 17:14       ` Paolo Bonzini
  2016-11-09 17:15       ` [PATCH v2] " Igor Mammedov
  0 siblings, 2 replies; 17+ messages in thread
From: Jim Mattson @ 2016-11-09 17:04 UTC (permalink / raw)
  To: Radim Krčmář, kvm; +Cc: Jim Mattson

>From the Intel SDM, volume 3, section 10.4.3, "Enabling or Disabling the
Local APIC,"

  When IA32_APIC_BASE[11] is 0, the processor is functionally equivalent
  to an IA-32 processor without an on-chip APIC. The CPUID feature flag
  for the APIC (see Section 10.4.2, "Presence of the Local APIC") is
  also set to 0.

Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/cpuid.c | 4 ++++
 arch/x86/kvm/lapic.c | 1 +
 2 files changed, 5 insertions(+)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index afa7bbb..84b62ee 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -81,6 +81,10 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
 			best->ecx |= F(OSXSAVE);
 	}
 
+	best->edx &= ~F(APIC);
+	if (vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE)
+		best->edx |= F(APIC);
+
 	if (apic) {
 		if (best->ecx & F(TSC_DEADLINE_TIMER))
 			apic->lapic_timer.timer_mode_mask = 3 << 17;
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 23b99f3..eda4284e 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1758,6 +1758,7 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
 
 	/* update jump label if enable bit changes */
 	if ((old_value ^ value) & MSR_IA32_APICBASE_ENABLE) {
+		kvm_update_cpuid(vcpu);
 		if (value & MSR_IA32_APICBASE_ENABLE) {
 			kvm_apic_set_xapic_id(apic, vcpu->vcpu_id);
 			static_key_slow_dec_deferred(&apic_hw_disabled);
-- 
2.8.0.rc3.226.g39d4020


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] kvm: nVMX: CPUID.01H:EDX.APIC[bit 9] should mirror IA32_APIC_BASE[11]
  2016-11-09 17:04     ` [PATCH v2] " Jim Mattson
@ 2016-11-09 17:14       ` Paolo Bonzini
  2016-11-09 17:28         ` Jim Mattson
  2016-11-09 17:15       ` [PATCH v2] " Igor Mammedov
  1 sibling, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2016-11-09 17:14 UTC (permalink / raw)
  To: Jim Mattson, Radim Krčmář, kvm



On 09/11/2016 18:04, Jim Mattson wrote:
>  
> +	best->edx &= ~F(APIC);
> +	if (vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE)
> +		best->edx |= F(APIC);
> +
>  	if (apic) {
>  		if (best->ecx & F(TSC_DEADLINE_TIMER))
>  			apic->lapic_timer.timer_mode_mask = 3 << 17;

The three new lines should be inside "if (apic)", because you are not
calling kvm_update_cpuid at all in kvm_lapic_set_base's "if (!apic)" case.

Thanks,

Paolo

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] kvm: nVMX: CPUID.01H:EDX.APIC[bit 9] should mirror IA32_APIC_BASE[11]
  2016-11-09 17:14       ` Paolo Bonzini
@ 2016-11-09 17:28         ` Jim Mattson
  2016-11-09 17:37           ` Paolo Bonzini
  0 siblings, 1 reply; 17+ messages in thread
From: Jim Mattson @ 2016-11-09 17:28 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Radim Krčmář, kvm

Better would be to call kvm_update_cpuid regardless, I think.

On Wed, Nov 9, 2016 at 9:14 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 09/11/2016 18:04, Jim Mattson wrote:
>>
>> +     best->edx &= ~F(APIC);
>> +     if (vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE)
>> +             best->edx |= F(APIC);
>> +
>>       if (apic) {
>>               if (best->ecx & F(TSC_DEADLINE_TIMER))
>>                       apic->lapic_timer.timer_mode_mask = 3 << 17;
>
> The three new lines should be inside "if (apic)", because you are not
> calling kvm_update_cpuid at all in kvm_lapic_set_base's "if (!apic)" case.
>
> Thanks,
>
> Paolo

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] kvm: nVMX: CPUID.01H:EDX.APIC[bit 9] should mirror IA32_APIC_BASE[11]
  2016-11-09 17:28         ` Jim Mattson
@ 2016-11-09 17:37           ` Paolo Bonzini
  2016-11-09 17:41             ` Jim Mattson
  0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2016-11-09 17:37 UTC (permalink / raw)
  To: Jim Mattson; +Cc: Radim Krčmář, kvm



On 09/11/2016 18:28, Jim Mattson wrote:
> Better would be to call kvm_update_cpuid regardless, I think.

Sure.  A kvm-unit-tests patch would be even better so that we can ensure
it works with QEMU userspace irqchip. :)

Paolo

> On Wed, Nov 9, 2016 at 9:14 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 09/11/2016 18:04, Jim Mattson wrote:
>>>
>>> +     best->edx &= ~F(APIC);
>>> +     if (vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE)
>>> +             best->edx |= F(APIC);
>>> +
>>>       if (apic) {
>>>               if (best->ecx & F(TSC_DEADLINE_TIMER))
>>>                       apic->lapic_timer.timer_mode_mask = 3 << 17;
>>
>> The three new lines should be inside "if (apic)", because you are not
>> calling kvm_update_cpuid at all in kvm_lapic_set_base's "if (!apic)" case.
>>
>> Thanks,
>>
>> Paolo

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] kvm: nVMX: CPUID.01H:EDX.APIC[bit 9] should mirror IA32_APIC_BASE[11]
  2016-11-09 17:37           ` Paolo Bonzini
@ 2016-11-09 17:41             ` Jim Mattson
  2016-11-09 17:43               ` Paolo Bonzini
  2016-11-09 17:50               ` [PATCH v3] kvm: nVMX: CPUID.01H:EDX.APIC[bit 9] should mirror IA32_APIC_BASE[11] Jim Mattson
  0 siblings, 2 replies; 17+ messages in thread
From: Jim Mattson @ 2016-11-09 17:41 UTC (permalink / raw)
  To: Paolo Bonzini; +Cc: Radim Krčmář, kvm

Agreed, but as you can probably tell, I don't use QEMU, so I will
leave that to someone who does.

On Wed, Nov 9, 2016 at 9:37 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 09/11/2016 18:28, Jim Mattson wrote:
>> Better would be to call kvm_update_cpuid regardless, I think.
>
> Sure.  A kvm-unit-tests patch would be even better so that we can ensure
> it works with QEMU userspace irqchip. :)
>
> Paolo
>
>> On Wed, Nov 9, 2016 at 9:14 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>
>>>
>>> On 09/11/2016 18:04, Jim Mattson wrote:
>>>>
>>>> +     best->edx &= ~F(APIC);
>>>> +     if (vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE)
>>>> +             best->edx |= F(APIC);
>>>> +
>>>>       if (apic) {
>>>>               if (best->ecx & F(TSC_DEADLINE_TIMER))
>>>>                       apic->lapic_timer.timer_mode_mask = 3 << 17;
>>>
>>> The three new lines should be inside "if (apic)", because you are not
>>> calling kvm_update_cpuid at all in kvm_lapic_set_base's "if (!apic)" case.
>>>
>>> Thanks,
>>>
>>> Paolo

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] kvm: nVMX: CPUID.01H:EDX.APIC[bit 9] should mirror IA32_APIC_BASE[11]
  2016-11-09 17:41             ` Jim Mattson
@ 2016-11-09 17:43               ` Paolo Bonzini
       [not found]                 ` <CALMp9eRek+NYZJYfT5TwCR+w=1hdS6e-O+Hm_RzF5MCykOHtWA@mail.gmail.com>
  2016-11-09 17:50               ` [PATCH v3] kvm: nVMX: CPUID.01H:EDX.APIC[bit 9] should mirror IA32_APIC_BASE[11] Jim Mattson
  1 sibling, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2016-11-09 17:43 UTC (permalink / raw)
  To: Jim Mattson; +Cc: Radim Krčmář, kvm



On 09/11/2016 18:41, Jim Mattson wrote:
> Agreed, but as you can probably tell, I don't use QEMU, so I will
> leave that to someone who does.

If you can find a colleague or minion to do it, fine, otherwise I'll
drop the patch.  The benefit is small and the corner cases large.

Paolo

> On Wed, Nov 9, 2016 at 9:37 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 09/11/2016 18:28, Jim Mattson wrote:
>>> Better would be to call kvm_update_cpuid regardless, I think.
>>
>> Sure.  A kvm-unit-tests patch would be even better so that we can ensure
>> it works with QEMU userspace irqchip. :)
>>
>> Paolo
>>
>>> On Wed, Nov 9, 2016 at 9:14 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>
>>>>
>>>> On 09/11/2016 18:04, Jim Mattson wrote:
>>>>>
>>>>> +     best->edx &= ~F(APIC);
>>>>> +     if (vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE)
>>>>> +             best->edx |= F(APIC);
>>>>> +
>>>>>       if (apic) {
>>>>>               if (best->ecx & F(TSC_DEADLINE_TIMER))
>>>>>                       apic->lapic_timer.timer_mode_mask = 3 << 17;
>>>>
>>>> The three new lines should be inside "if (apic)", because you are not
>>>> calling kvm_update_cpuid at all in kvm_lapic_set_base's "if (!apic)" case.
>>>>
>>>> Thanks,
>>>>
>>>> Paolo

^ permalink raw reply	[flat|nested] 17+ messages in thread

[parent not found: <CALMp9eRek+NYZJYfT5TwCR+w=1hdS6e-O+Hm_RzF5MCykOHtWA@mail.gmail.com>]

* Re: [PATCH v2] kvm: nVMX: CPUID.01H:EDX.APIC[bit 9] should mirror IA32_APIC_BASE[11]
       [not found]                 ` <CALMp9eRek+NYZJYfT5TwCR+w=1hdS6e-O+Hm_RzF5MCykOHtWA@mail.gmail.com>
@ 2016-11-10 17:18                   ` Paolo Bonzini
  2016-11-14 18:14                     ` [kvm-unit-tests PATCH] x86: Test disabled local APIC Jim Mattson
  0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2016-11-10 17:18 UTC (permalink / raw)
  To: Jim Mattson; +Cc: Radim Krčmář, kvm



On 10/11/2016 18:16, Jim Mattson wrote:
> What if I were to adopt your previous suggestion:
> 
>> The three new lines should be inside "if (apic)", because you are not calling kvm_update_cpuid at all in kvm_lapic_set_base's "if (!apic)" case.
> 
> That would preserve the existing behavior for the QEMU userspace irqchip.
> 
> I do have a kvm-unit-test for the in-kernel APIC.

Send it, we can test it with QEMU userspace irqchip.  Always send unit
tests if you have them. :)

Paolo

> On Wed, Nov 9, 2016 at 9:43 AM, Paolo Bonzini <pbonzini@redhat.com
> <mailto:pbonzini@redhat.com>> wrote:
> 
> 
> 
>     On 09/11/2016 18:41, Jim Mattson wrote:
>     > Agreed, but as you can probably tell, I don't use QEMU, so I will
>     > leave that to someone who does.
> 
>     If you can find a colleague or minion to do it, fine, otherwise I'll
>     drop the patch.  The benefit is small and the corner cases large.
> 
>     Paolo
> 
>     > On Wed, Nov 9, 2016 at 9:37 AM, Paolo Bonzini <pbonzini@redhat.com
>     <mailto:pbonzini@redhat.com>> wrote:
>     >>
>     >>
>     >> On 09/11/2016 18:28, Jim Mattson wrote:
>     >>> Better would be to call kvm_update_cpuid regardless, I think.
>     >>
>     >> Sure.  A kvm-unit-tests patch would be even better so that we can
>     ensure
>     >> it works with QEMU userspace irqchip. :)
>     >>
>     >> Paolo
>     >>
>     >>> On Wed, Nov 9, 2016 at 9:14 AM, Paolo Bonzini
>     <pbonzini@redhat.com <mailto:pbonzini@redhat.com>> wrote:
>     >>>>
>     >>>>
>     >>>> On 09/11/2016 18:04, Jim Mattson wrote:
>     >>>>>
>     >>>>> +     best->edx &= ~F(APIC);
>     >>>>> +     if (vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE)
>     >>>>> +             best->edx |= F(APIC);
>     >>>>> +
>     >>>>>       if (apic) {
>     >>>>>               if (best->ecx & F(TSC_DEADLINE_TIMER))
>     >>>>>                       apic->lapic_timer.timer_mode_mask = 3 << 17;
>     >>>>
>     >>>> The three new lines should be inside "if (apic)", because you
>     are not
>     >>>> calling kvm_update_cpuid at all in kvm_lapic_set_base's "if
>     (!apic)" case.
>     >>>>
>     >>>> Thanks,
>     >>>>
>     >>>> Paolo
> 
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [kvm-unit-tests PATCH] x86: Test disabled local APIC
  2016-11-10 17:18                   ` Paolo Bonzini
@ 2016-11-14 18:14                     ` Jim Mattson
  2016-11-22 13:55                       ` Radim Krčmář
  0 siblings, 1 reply; 17+ messages in thread
From: Jim Mattson @ 2016-11-14 18:14 UTC (permalink / raw)
  To: Paolo Bonzini, Radim Krčmář, kvm; +Cc: Jim Mattson

This test disables and re-enables the local APIC and ensures that
CPUID.1H:EDX.APIC[bit 9] mirrors IA32_APIC_BASE[11].

Signed-off-by: Jim Mattson <jmattson@google.com>
---
 x86/apic.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/x86/apic.c b/x86/apic.c
index eff9a11..39c7fd1 100644
--- a/x86/apic.c
+++ b/x86/apic.c
@@ -104,6 +104,28 @@ void test_enable_x2apic(void)
     }
 }
 
+static void test_apic_disable(void)
+{
+    u64 orig_apicbase = rdmsr(MSR_IA32_APICBASE);
+
+    report_prefix_push("apic_disable");
+
+    report("Local apic enabled", orig_apicbase & APIC_EN);
+    report("CPUID.1H:EDX.APIC[bit 9] is set", cpuid(1).d & (1 << 9));
+
+    wrmsr(MSR_IA32_APICBASE, orig_apicbase & ~(APIC_EN | APIC_EXTD));
+    report("Local apic disabled", !(rdmsr(MSR_IA32_APICBASE) & APIC_EN));
+    report("CPUID.1H:EDX.APIC[bit 9] is clear", !(cpuid(1).d & (1 << 9)));
+
+    wrmsr(MSR_IA32_APICBASE, orig_apicbase & ~APIC_EXTD);
+    wrmsr(MSR_IA32_APICBASE, orig_apicbase);
+    apic_write(APIC_SPIV, 0x1ff);
+    report("Local apic enabled", rdmsr(MSR_IA32_APICBASE) & APIC_EN);
+    report("CPUID.1H:EDX.APIC[bit 9] is set", cpuid(1).d & (1 << 9));
+
+    report_prefix_pop();
+}
+
 #define ALTERNATE_APIC_BASE	0x42000000
 
 static void test_apicbase(void)
@@ -398,6 +420,7 @@ int main()
 
     mask_pic_interrupts();
     test_apic_id();
+    test_apic_disable();
     test_enable_x2apic();
     test_apicbase();
 
-- 
2.8.0.rc3.226.g39d4020


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [kvm-unit-tests PATCH] x86: Test disabled local APIC
  2016-11-14 18:14                     ` [kvm-unit-tests PATCH] x86: Test disabled local APIC Jim Mattson
@ 2016-11-22 13:55                       ` Radim Krčmář
  0 siblings, 0 replies; 17+ messages in thread
From: Radim Krčmář @ 2016-11-22 13:55 UTC (permalink / raw)
  To: Jim Mattson; +Cc: Paolo Bonzini, kvm

2016-11-14 10:14-0800, Jim Mattson:
> This test disables and re-enables the local APIC and ensures that
> CPUID.1H:EDX.APIC[bit 9] mirrors IA32_APIC_BASE[11].
> 
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---

Applied, thanks.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v3] kvm: nVMX: CPUID.01H:EDX.APIC[bit 9] should mirror IA32_APIC_BASE[11]
  2016-11-09 17:41             ` Jim Mattson
  2016-11-09 17:43               ` Paolo Bonzini
@ 2016-11-09 17:50               ` Jim Mattson
  2016-11-22 13:53                 ` Radim Krčmář
  1 sibling, 1 reply; 17+ messages in thread
From: Jim Mattson @ 2016-11-09 17:50 UTC (permalink / raw)
  To: Paolo Bonzini, Radim Krčmář, kvm; +Cc: Jim Mattson

>From the Intel SDM, volume 3, section 10.4.3, "Enabling or Disabling the
Local APIC,"

  When IA32_APIC_BASE[11] is 0, the processor is functionally equivalent
  to an IA-32 processor without an on-chip APIC. The CPUID feature flag
  for the APIC (see Section 10.4.2, "Presence of the Local APIC") is
  also set to 0.

Signed-off-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/kvm/cpuid.c |  4 ++++
 arch/x86/kvm/lapic.c | 11 +++++++----
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index afa7bbb..84b62ee 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -81,6 +81,10 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
 			best->ecx |= F(OSXSAVE);
 	}
 
+	best->edx &= ~F(APIC);
+	if (vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE)
+		best->edx |= F(APIC);
+
 	if (apic) {
 		if (best->ecx & F(TSC_DEADLINE_TIMER))
 			apic->lapic_timer.timer_mode_mask = 3 << 17;
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 23b99f3..7bd887b 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -1748,14 +1748,17 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
 	u64 old_value = vcpu->arch.apic_base;
 	struct kvm_lapic *apic = vcpu->arch.apic;
 
-	if (!apic) {
+	if (!apic)
 		value |= MSR_IA32_APICBASE_BSP;
-		vcpu->arch.apic_base = value;
-		return;
-	}
 
 	vcpu->arch.apic_base = value;
 
+	if ((old_value ^ value) & MSR_IA32_APICBASE_ENABLE)
+		kvm_update_cpuid(vcpu);
+
+	if (!apic)
+		return;
+
 	/* update jump label if enable bit changes */
 	if ((old_value ^ value) & MSR_IA32_APICBASE_ENABLE) {
 		if (value & MSR_IA32_APICBASE_ENABLE) {
-- 
2.8.0.rc3.226.g39d4020


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH v3] kvm: nVMX: CPUID.01H:EDX.APIC[bit 9] should mirror IA32_APIC_BASE[11]
  2016-11-09 17:50               ` [PATCH v3] kvm: nVMX: CPUID.01H:EDX.APIC[bit 9] should mirror IA32_APIC_BASE[11] Jim Mattson
@ 2016-11-22 13:53                 ` Radim Krčmář
  0 siblings, 0 replies; 17+ messages in thread
From: Radim Krčmář @ 2016-11-22 13:53 UTC (permalink / raw)
  To: Jim Mattson; +Cc: Paolo Bonzini, kvm

2016-11-09 09:50-0800, Jim Mattson:
> From the Intel SDM, volume 3, section 10.4.3, "Enabling or Disabling the
> Local APIC,"
> 
>   When IA32_APIC_BASE[11] is 0, the processor is functionally equivalent
>   to an IA-32 processor without an on-chip APIC. The CPUID feature flag
>   for the APIC (see Section 10.4.2, "Presence of the Local APIC") is
>   also set to 0.
> 
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---

Applied to kvm/queue, thanks.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] kvm: nVMX: CPUID.01H:EDX.APIC[bit 9] should mirror IA32_APIC_BASE[11]
  2016-11-09 17:04     ` [PATCH v2] " Jim Mattson
  2016-11-09 17:14       ` Paolo Bonzini
@ 2016-11-09 17:15       ` Igor Mammedov
  2016-11-09 17:37         ` Eduardo Habkost
  2016-11-09 17:42         ` Paolo Bonzini
  1 sibling, 2 replies; 17+ messages in thread
From: Igor Mammedov @ 2016-11-09 17:15 UTC (permalink / raw)
  To: Jim Mattson; +Cc: Radim Krčmář, kvm, ehabkost

On Wed,  9 Nov 2016 09:04:36 -0800
Jim Mattson <jmattson@google.com> wrote:

> From the Intel SDM, volume 3, section 10.4.3, "Enabling or Disabling the
> Local APIC,"
> 
>   When IA32_APIC_BASE[11] is 0, the processor is functionally equivalent
>   to an IA-32 processor without an on-chip APIC. The CPUID feature flag
>   for the APIC (see Section 10.4.2, "Presence of the Local APIC") is
>   also set to 0.
CCing Eduardo in case it might affect migration.

> 
> Signed-off-by: Jim Mattson <jmattson@google.com>
> ---
>  arch/x86/kvm/cpuid.c | 4 ++++
>  arch/x86/kvm/lapic.c | 1 +
>  2 files changed, 5 insertions(+)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index afa7bbb..84b62ee 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -81,6 +81,10 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
>  			best->ecx |= F(OSXSAVE);
>  	}
>  
> +	best->edx &= ~F(APIC);
> +	if (vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE)
> +		best->edx |= F(APIC);
> +
>  	if (apic) {
>  		if (best->ecx & F(TSC_DEADLINE_TIMER))
>  			apic->lapic_timer.timer_mode_mask = 3 << 17;
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 23b99f3..eda4284e 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -1758,6 +1758,7 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
>  
>  	/* update jump label if enable bit changes */
>  	if ((old_value ^ value) & MSR_IA32_APICBASE_ENABLE) {
> +		kvm_update_cpuid(vcpu);
>  		if (value & MSR_IA32_APICBASE_ENABLE) {
>  			kvm_apic_set_xapic_id(apic, vcpu->vcpu_id);
>  			static_key_slow_dec_deferred(&apic_hw_disabled);


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] kvm: nVMX: CPUID.01H:EDX.APIC[bit 9] should mirror IA32_APIC_BASE[11]
  2016-11-09 17:15       ` [PATCH v2] " Igor Mammedov
@ 2016-11-09 17:37         ` Eduardo Habkost
  2016-11-09 17:42         ` Paolo Bonzini
  1 sibling, 0 replies; 17+ messages in thread
From: Eduardo Habkost @ 2016-11-09 17:37 UTC (permalink / raw)
  To: Igor Mammedov; +Cc: Jim Mattson, Radim Krčmář, kvm

On Wed, Nov 09, 2016 at 06:15:02PM +0100, Igor Mammedov wrote:
> On Wed,  9 Nov 2016 09:04:36 -0800
> Jim Mattson <jmattson@google.com> wrote:
> 
> > From the Intel SDM, volume 3, section 10.4.3, "Enabling or Disabling the
> > Local APIC,"
> > 
> >   When IA32_APIC_BASE[11] is 0, the processor is functionally equivalent
> >   to an IA-32 processor without an on-chip APIC. The CPUID feature flag
> >   for the APIC (see Section 10.4.2, "Presence of the Local APIC") is
> >   also set to 0.
> CCing Eduardo in case it might affect migration.

It depends on the pros/cons of keeping bug compatibility when
migrating existing VMs. If keeping bug compatibility doesn't buy
us anything, we don't need anything extra. If keeping bug
compatibility would avoid breaking something else, then we need
something to allow userspace to keep the old behavior.

I this case, I don't see any advantage in keeping bug
compatibility. If any guest code used the incorrectly-enabled
APIC flag to make any decision while the APIC was disabled, I
don't think the situation will get worse if the guest starts
seeing the APIC flag correctly cleared.

> 
> > 
> > Signed-off-by: Jim Mattson <jmattson@google.com>
> > ---
> >  arch/x86/kvm/cpuid.c | 4 ++++
> >  arch/x86/kvm/lapic.c | 1 +
> >  2 files changed, 5 insertions(+)
> > 
> > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > index afa7bbb..84b62ee 100644
> > --- a/arch/x86/kvm/cpuid.c
> > +++ b/arch/x86/kvm/cpuid.c
> > @@ -81,6 +81,10 @@ int kvm_update_cpuid(struct kvm_vcpu *vcpu)
> >  			best->ecx |= F(OSXSAVE);
> >  	}
> >  
> > +	best->edx &= ~F(APIC);
> > +	if (vcpu->arch.apic_base & MSR_IA32_APICBASE_ENABLE)
> > +		best->edx |= F(APIC);
> > +
> >  	if (apic) {
> >  		if (best->ecx & F(TSC_DEADLINE_TIMER))
> >  			apic->lapic_timer.timer_mode_mask = 3 << 17;
> > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > index 23b99f3..eda4284e 100644
> > --- a/arch/x86/kvm/lapic.c
> > +++ b/arch/x86/kvm/lapic.c
> > @@ -1758,6 +1758,7 @@ void kvm_lapic_set_base(struct kvm_vcpu *vcpu, u64 value)
> >  
> >  	/* update jump label if enable bit changes */
> >  	if ((old_value ^ value) & MSR_IA32_APICBASE_ENABLE) {
> > +		kvm_update_cpuid(vcpu);
> >  		if (value & MSR_IA32_APICBASE_ENABLE) {
> >  			kvm_apic_set_xapic_id(apic, vcpu->vcpu_id);
> >  			static_key_slow_dec_deferred(&apic_hw_disabled);
> 

-- 
Eduardo

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2] kvm: nVMX: CPUID.01H:EDX.APIC[bit 9] should mirror IA32_APIC_BASE[11]
  2016-11-09 17:15       ` [PATCH v2] " Igor Mammedov
  2016-11-09 17:37         ` Eduardo Habkost
@ 2016-11-09 17:42         ` Paolo Bonzini
  1 sibling, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2016-11-09 17:42 UTC (permalink / raw)
  To: Igor Mammedov, Jim Mattson; +Cc: Radim Krčmář, kvm, ehabkost



On 09/11/2016 18:15, Igor Mammedov wrote:
> >   When IA32_APIC_BASE[11] is 0, the processor is functionally equivalent
> >   to an IA-32 processor without an on-chip APIC. The CPUID feature flag
> >   for the APIC (see Section 10.4.2, "Presence of the Local APIC") is
> >   also set to 0.
> 
> CCing Eduardo in case it might affect migration.

If the destination kernel is old, it will re-grow the CPUID APIC bit.  
However, clearing IA32_APIC_BASE[11] should really only happen in the 
BIOS; our firmware is sane and doesn't do that.

See this bit in arch/x86/kernel/apic/apic.c's apic_force_enable:

                rdmsr(MSR_IA32_APICBASE, l, h);
                if (!(l & MSR_IA32_APICBASE_ENABLE)) {
                        pr_info("Local APIC disabled by BIOS -- reenabling.\n");
                        l &= ~MSR_IA32_APICBASE_BASE;
                        l |= MSR_IA32_APICBASE_ENABLE | addr;
                        wrmsr(MSR_IA32_APICBASE, l, h);
                        enabled_via_apicbase = 1;
                }

Paolo

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2016-11-22 13:55 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-11-04 22:00 [PATCH] kvm: nVMX: CPUID.01H:EDX.APIC[bit 9] should mirror IA32_APIC_BASE[11] Jim Mattson
2016-11-08 16:33 ` Radim Krčmář
2016-11-09 16:53   ` Jim Mattson
2016-11-09 17:04     ` [PATCH v2] " Jim Mattson
2016-11-09 17:14       ` Paolo Bonzini
2016-11-09 17:28         ` Jim Mattson
2016-11-09 17:37           ` Paolo Bonzini
2016-11-09 17:41             ` Jim Mattson
2016-11-09 17:43               ` Paolo Bonzini
     [not found]                 ` <CALMp9eRek+NYZJYfT5TwCR+w=1hdS6e-O+Hm_RzF5MCykOHtWA@mail.gmail.com>
2016-11-10 17:18                   ` Paolo Bonzini
2016-11-14 18:14                     ` [kvm-unit-tests PATCH] x86: Test disabled local APIC Jim Mattson
2016-11-22 13:55                       ` Radim Krčmář
2016-11-09 17:50               ` [PATCH v3] kvm: nVMX: CPUID.01H:EDX.APIC[bit 9] should mirror IA32_APIC_BASE[11] Jim Mattson
2016-11-22 13:53                 ` Radim Krčmář
2016-11-09 17:15       ` [PATCH v2] " Igor Mammedov
2016-11-09 17:37         ` Eduardo Habkost
2016-11-09 17:42         ` Paolo Bonzini

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.