| Message ID | 1458281388-14452-11-git-send-email-Suravee.Suthikulpanit@amd.com |
|---|---|
| State | New |
| Headers | show |
Hi Radim, On 03/19/2016 03:59 AM, Radim Krčmář wrote: > 2016-03-18 01:09-0500, Suravee Suthikulpanit: >> From: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> >> >> 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 <suravee.suthikulpanit@amd.com> >> --- >> 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? 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 = 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. This is also not needed since we already disable the x2APIC in the CPUID 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 = !!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. :) Good point. I can only find example of using (X86_FEATURE_X2APIC % 32) == 21. >> + } >> + } >> } >> >> 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. 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 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 would likely default to using x2APIC interface, which will not be handled by the AVIC hardware, and resulting in no performance improvement that we are trying to introduce. Thanks, Suravee
Hi Paolo, On 3/31/16 18:23, Paolo Bonzini wrote: > > > On 31/03/2016 06:15, Suravee Suthikulpanit wrote: >>>> + 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. >>> :) >> >> Good point. I can only find example of using (X86_FEATURE_X2APIC % >> 32) == 21. > > You can use bit(X86_FEATURE_X2APIC), it is defined in arch/x86/kvm/x86.h. Ahh, thanks. > >>> 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 >> would likely default to using x2APIC interface, which will not be >> handled by the AVIC hardware, and resulting in no performance >> improvement that we are trying to introduce. > > You would still get some improvement from exit-free interrupt delivery. Let me look into this and investigate some more. Thanks, Suravee
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 }, }; @@ -841,7 +842,7 @@ static void set_msr_interception(u32 *msrpm, unsigned msr, msrpm[offset] = tmp; } -static void svm_vcpu_init_msrpm(u32 *msrpm) +static void svm_vcpu_init_msrpm(struct vcpu_svm *svm, u32 *msrpm) { int i; @@ -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); } static void add_msr_offset(u32 offset) @@ -1394,18 +1398,18 @@ static struct kvm_vcpu *svm_create_vcpu(struct kvm *kvm, unsigned int id) svm->nested.hsave = page_address(hsave_page); - svm->msrpm = page_address(msrpm_pages); - svm_vcpu_init_msrpm(svm->msrpm); - - svm->nested.msrpm = page_address(nested_msrpm_pages); - svm_vcpu_init_msrpm(svm->nested.msrpm); - svm->vmcb = page_address(page); clear_page(svm->vmcb); svm->vmcb_pa = page_to_pfn(page) << PAGE_SHIFT; svm->asid_generation = 0; init_vmcb(svm); + svm->msrpm = page_address(msrpm_pages); + svm_vcpu_init_msrpm(svm, svm->msrpm); + + svm->nested.msrpm = page_address(nested_msrpm_pages); + svm_vcpu_init_msrpm(svm, svm->nested.msrpm); + svm_init_osvw(&svm->vcpu); return &svm->vcpu; @@ -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; + } + /* 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); + /* 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) + vcpu->arch.cpuid_entries[i].ecx &= ~(1 << 21); + } + } } 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); + break; case 0x80000001: if (nested) entry->ecx |= (1 << 2); /* Set SVM bit */