| Message ID | 1478711076-1426-1-git-send-email-jmattson@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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); -- 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
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 -- 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
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); >
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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 > > -- 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..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);
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(+)