| Message ID | 1478296802-23291-1-git-send-email-jmattson@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);