From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suravee Suthikulpanit Subject: Re: [PART1 RFC v3 10/12] svm: Do not expose x2APIC when enable AVIC Date: Thu, 31 Mar 2016 11:15:36 +0700 Message-ID: <56FCA468.8090406@amd.com> References: <1458281388-14452-1-git-send-email-Suravee.Suthikulpanit@amd.com> <1458281388-14452-11-git-send-email-Suravee.Suthikulpanit@amd.com> <20160318205948.GA26119@potion.brq.redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: , , , , , , , , To: =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= Return-path: In-Reply-To: <20160318205948.GA26119@potion.brq.redhat.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org Hi Radim, On 03/19/2016 03:59 AM, Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: > 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 =3D MSR_IA32_LASTBRANCHTOIP, .always =3D false }, >> { .index =3D MSR_IA32_LASTINTFROMIP, .always =3D false }, >> { .index =3D MSR_IA32_LASTINTTOIP, .always =3D false }, >> + { .index =3D MSR_IA32_APICBASE, .always =3D false }, >> { .index =3D MSR_INVALID, .always =3D 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? Actually, I got confused about this part. This should not be needed. > >> @@ -3308,6 +3312,18 @@ static int svm_get_msr(struct kvm_vcpu *vcpu,= struct msr_data *msr_info) >> msr_info->data =3D 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 &=3D ~X2APIC_ENABLE; >> + msr_info->data |=3D 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. This is also not needed since we already disable the x2APIC in the CPUI= D=20 below. >> + } >> + /* 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. Right. I'll move this. > >> + /* 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 =3D !!guest_cpuid_has_nrips(&svm->vcpu); >> + >> + /* Do not support X2APIC when enable AVIC */ >> + if (svm_vcpu_avic_enabled(svm)) { >> + int i; >> + >> + for (i =3D 0 ; i < vcpu->arch.cpuid_nent ; i++) { >> + if (vcpu->arch.cpuid_entries[i].function =3D=3D 1) > > Please use kvm_find_cpuid_entry for the search. > >> + vcpu->arch.cpuid_entries[i].ecx &=3D ~(1 << 21); > > and X86_FEATURE_X2APIC (or something with X2APIC in name) for the bit= =2E > > The code will become so obvious that the comment can be removed. :) Good point. I can only find example of using (X86_FEATURE_X2APIC % 32)=20 =3D=3D 21. >> + } >> + } >> } >> >> static void svm_set_supported_cpuid(u32 func, struct kvm_cpuid_ent= ry2 *entry) >> { >> switch (func) { >> + case 0x00000001: >> + /* Do not support X2APIC when enable AVIC */ >> + if (avic) >> + entry->ecx &=3D ~(1 << 21); > > I think this might be the right place for the code you have in > svm_cpuid_update. Right. I'll also make change to use (X86_FEATURE_X2APIC % 32) > 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, Access to offset 0x400+ would generate #VMEXIT no accel fault=20 read/write. So, we will need to handle and emulate this in the host. > 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. Agree that it will still work. However, in such case, the guest code=20 would likely default to using x2APIC interface, which will not be=20 handled by the AVIC hardware, and resulting in no performance=20 improvement that we are trying to introduce. Thanks, Suravee