Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757652AbcCRU77 (ORCPT ); Fri, 18 Mar 2016 16:59:59 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34140 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757314AbcCRU7z (ORCPT ); Fri, 18 Mar 2016 16:59:55 -0400 Date: Fri, 18 Mar 2016 21:59:49 +0100 From: Radim =?utf-8?B?S3LEjW3DocWZ?= To: Suravee Suthikulpanit Cc: pbonzini@redhat.com, joro@8bytes.org, bp@alien8.de, gleb@kernel.org, alex.williamson@redhat.com, kvm@vger.kernel.org, linux-kernel@vger.kernel.org, wei@redhat.com, sherry.hurwitz@amd.com Subject: Re: [PART1 RFC v3 10/12] svm: Do not expose x2APIC when enable AVIC Message-ID: <20160318205948.GA26119@potion.brq.redhat.com> References: <1458281388-14452-1-git-send-email-Suravee.Suthikulpanit@amd.com> <1458281388-14452-11-git-send-email-Suravee.Suthikulpanit@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1458281388-14452-11-git-send-email-Suravee.Suthikulpanit@amd.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3958 Lines: 121 2016-03-18 01:09-0500, Suravee Suthikulpanit: > From: Suravee Suthikulpanit > > Since AVIC only virtualizes xAPIC hardware for the guest, we need to: > * Intercept APIC BAR msr accesses to disable x2APIC > * Intercept CPUID access to not advertise x2APIC support > * Hide x2APIC support when checking via KVM ioctl > > Signed-off-by: Suravee Suthikulpanit > --- > arch/x86/kvm/svm.c | 49 ++++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 42 insertions(+), 7 deletions(-) > > diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c > index 6303147..ba84d57 100644 > --- a/arch/x86/kvm/svm.c > +++ b/arch/x86/kvm/svm.c > @@ -209,6 +209,7 @@ static const struct svm_direct_access_msrs { > { .index = MSR_IA32_LASTBRANCHTOIP, .always = false }, > { .index = MSR_IA32_LASTINTFROMIP, .always = false }, > { .index = MSR_IA32_LASTINTTOIP, .always = false }, > + { .index = MSR_IA32_APICBASE, .always = false }, > { .index = MSR_INVALID, .always = false }, > }; > > @@ -853,6 +854,9 @@ static void svm_vcpu_init_msrpm(u32 *msrpm) > > set_msr_interception(msrpm, direct_access_msrs[i].index, 1, 1); > } > + > + if (svm_vcpu_avic_enabled(svm)) > + set_msr_interception(msrpm, MSR_IA32_APICBASE, 1, 1); AVIC really won't exit on writes to MSR_IA32_APICBASE otherwise? > @@ -3308,6 +3312,18 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info) > msr_info->data = 0x1E; > } > break; > + case MSR_IA32_APICBASE: > + if (svm_vcpu_avic_enabled(svm)) { > + /* Note: > + * For AVIC, we need to disable X2APIC > + * and enable XAPIC > + */ > + kvm_get_msr_common(vcpu, msr_info); > + msr_info->data &= ~X2APIC_ENABLE; > + msr_info->data |= XAPIC_ENABLE; > + break; No. This won't make the guest switch to xAPIC. x2APIC can only be enabled if CPUID has that flag and it's impossible to toggle that CPUID flag it during runtime. > + } > + /* Follow through if not AVIC */ > default: > return kvm_get_msr_common(vcpu, msr_info); > } > @@ -3436,6 +3452,10 @@ static int svm_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr) > case MSR_VM_IGNNE: > vcpu_unimpl(vcpu, "unimplemented wrmsr: 0x%x data 0x%llx\n", ecx, data); > break; > + case MSR_IA32_APICBASE: > + if (svm_vcpu_avic_enabled(svm)) > + avic_update_vapic_bar(to_svm(vcpu), data); There is no connection to x2APIC, please do it in a different patch. > + /* Follow through */ > default: > return kvm_set_msr_common(vcpu, msr); > } > @@ -4554,11 +4574,26 @@ static void svm_cpuid_update(struct kvm_vcpu *vcpu) > > /* Update nrips enabled cache */ > svm->nrips_enabled = !!guest_cpuid_has_nrips(&svm->vcpu); > + > + /* Do not support X2APIC when enable AVIC */ > + if (svm_vcpu_avic_enabled(svm)) { > + int i; > + > + for (i = 0 ; i < vcpu->arch.cpuid_nent ; i++) { > + if (vcpu->arch.cpuid_entries[i].function == 1) Please use kvm_find_cpuid_entry for the search. > + vcpu->arch.cpuid_entries[i].ecx &= ~(1 << 21); and X86_FEATURE_X2APIC (or something with X2APIC in name) for the bit. The code will become so obvious that the comment can be removed. :) > + } > + } > } > > static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_entry2 *entry) > { > switch (func) { > + case 0x00000001: > + /* Do not support X2APIC when enable AVIC */ > + if (avic) > + entry->ecx &= ~(1 << 21); I think this might be the right place for the code you have in svm_cpuid_update. Btw. how does x2APIC behave under AVIC? We definitely shouldn't recommend/expose x2APIC with AVIC as AVIC doesn't accelerate x2APIC guest-facing interface, but the MSR interface is going to exit and host-side interrupt delivery will probably still work, so I don't see a huge problem with it. > + break; > case 0x80000001: > if (nested) > entry->ecx |= (1 << 2); /* Set SVM bit */ > -- > 1.9.1 >