Messages in this thread | | | Subject | Re: [PATCH 3/3] KVM: SVM: Add irqchip_split() checks before enabling AVIC | From | Suravee Suthikulpanit <> | Date | Tue, 12 Sep 2017 01:59:55 -0700 |
| |
Hi Radim,
On 9/8/17 08:53, Radim Krčmář wrote: > 2017-09-05 22:39-0500, Suravee Suthikulpanit: >> SVM AVIC hardware accelerates guest write to APIC_EOI register >> (for edge-trigger interrupt), which means it does not trap to KVM. >> >> So, only enable SVM AVIC only in split irqchip mode. >> (e.g. launching qemu w/ option '-machine kernel_irqchip=split'). > > Yeah, hacking TMR to get the VM exit could result in future bugs. > We have to push split irqchip as the deafult in userspaces with this > change.
Actually, I'm not quite sure about the advantages/disadvantages with split irqchip, and how it would affect other cases, and why it was not used as default currently.
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com> >> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com> >> --- >> arch/x86/kvm/svm.c | 13 +++++++++---- >> 1 file changed, 9 insertions(+), 4 deletions(-) >> >> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c >> index d1b3eb4..7ce191b 100644 >> --- a/arch/x86/kvm/svm.c >> +++ b/arch/x86/kvm/svm.c >> @@ -1292,7 +1291,7 @@ static void init_vmcb(struct vcpu_svm *svm) >> set_intercept(svm, INTERCEPT_PAUSE); >> } >> >> - if (avic) >> + if (kvm_vcpu_apicv_active(&svm->vcpu)) >> avic_init_vmcb(svm); >> /* >> @@ -1594,6 +1593,12 @@ static int avic_init_vcpu(struct vcpu_svm *svm) >> if (!avic) >> return 0; >> >> + if (!irqchip_split(svm->vcpu.kvm)) { > > The other place used kvm_vcpu_apicv_active() instead of checking > irqchip_split() directly, so I think it would be better to be consistent > and do it here too. > I'd also like if this workaround used !irqchip_split() exactly once.
Ok, I'll clean up in V2.
>> + pr_debug("%s: Disable AVIC due to non-split irqchip.\n", >> + __func__); > > There is going to be too much of those. pr_debug_once() would be a > better notification. We can also report it in svm_get_enable_apicv().
pr_debug_once does not use dynamic debug APIs. I think I can call pr_debug only when vcpu_id == 0.
>> + return 0; >> + } >> + >> ret = avic_init_backing_page(&svm->vcpu); >> if (ret) >> return ret; >> @@ -4388,7 +4393,7 @@ static void svm_set_virtual_x2apic_mode(struct kvm_vcpu *vcpu, bool set) >> >> static bool svm_get_enable_apicv(struct kvm_vcpu *vcpu) >> { >> - return avic; >> + return avic && irqchip_split(vcpu->kvm); >> } >> >> static void svm_hwapic_irr_update(struct kvm_vcpu *vcpu, int max_irr) >> @@ -4405,7 +4410,7 @@ static void svm_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu) >> struct vcpu_svm *svm = to_svm(vcpu); >> struct vmcb *vmcb = svm->vmcb; >> >> - if (!avic) >> + if (!avic || !irqchip_split(svm->vcpu.kvm)) > > Seems like we want !kvm_vcpu_apicv_active() here too.
Right.
> >> return; >> >> vmcb->control.int_ctl &= ~AVIC_ENABLE_MASK; > > (separate bug: refresh should be able to enable avic as well.) > > thanks. >
Thanks, Suravee
| |