From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eduardo Habkost Subject: Re: [PATCH v2] kvm: nVMX: CPUID.01H:EDX.APIC[bit 9] should mirror IA32_APIC_BASE[11] Date: Wed, 9 Nov 2016 15:37:16 -0200 Message-ID: <20161109173716.GB5057@thinpad.lan.raisama.net> References: <1478711076-1426-1-git-send-email-jmattson@google.com> <20161109181502.0019f7b3@nial.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Jim Mattson , Radim =?utf-8?B?S3LEjW3DocWZ?= , kvm@vger.kernel.org To: Igor Mammedov Return-path: Received: from mx1.redhat.com ([209.132.183.28]:38002 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750877AbcKIRhS (ORCPT ); Wed, 9 Nov 2016 12:37:18 -0500 Content-Disposition: inline In-Reply-To: <20161109181502.0019f7b3@nial.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Wed, Nov 09, 2016 at 06:15:02PM +0100, Igor Mammedov wrote: > On Wed, 9 Nov 2016 09:04:36 -0800 > Jim Mattson 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 > > --- > > 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