From mboxrd@z Thu Jan 1 00:00:00 1970 From: Liran Alon Subject: Re: [PATCH] KVM: nVMX: Do not recalc IOAPIC handled vectors while running L2 Date: Fri, 10 Nov 2017 23:47:22 +0200 Message-ID: <5A061E6A.8070001@ORACLE.COM> References: <1510277042-15191-1-git-send-email-liran.alon@oracle.com> <1510277042-15191-2-git-send-email-liran.alon@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: jmattson@google.com, wanpeng.li@hotmail.com, idan.brown@ORACLE.COM, Krish Sadhukhan To: Paolo Bonzini , rkrcmar@redhat.com, kvm@vger.kernel.org Return-path: Received: from aserp1040.oracle.com ([141.146.126.69]:19241 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754275AbdKJVrb (ORCPT ); Fri, 10 Nov 2017 16:47:31 -0500 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On 10/11/17 10:37, Paolo Bonzini wrote: > On 10/11/2017 02:24, Liran Alon wrote: >> >> @@ -6720,6 +6720,12 @@ static void vcpu_scan_ioapic(struct kvm_vcpu *vcpu) >> if (!kvm_apic_hw_enabled(vcpu->arch.apic)) >> return; >> >> + if (is_guest_mode(vcpu)) { >> + vcpu->arch.scan_ioapic_pending = true; >> + return; >> + } >> + vcpu->arch.scan_ioapic_pending = false; >> + >> bitmap_zero(vcpu->arch.ioapic_handled_vectors, 256); >> >> if (irqchip_split(vcpu->kvm)) > > I am not sure it is correct to exit immediately. The bug is that you're > losing the EOI exit bitmap in the vmcs01; however, with your patch > you're losing it in the vmcs02. If the L1 hypervisor passes the local > APIC registers to the L2 guest, you would have a similar bug. I have indeed not thought about this case. However, I think it is also currently not supported. Assume that at some point of time L1 IOAPIC redirection-table is already fully-configured and is not going to be changed by L1 anymore. Further assume that L1 wants to run L2 and give him full access to L1 LAPIC. In this case, vmcs12->eoi_exit_bitmap will be empty (all zeros) as L1 assumes EOIs to L1 LAPIC are always broadcasted to L1 IOAPIC (as happens in real hardware). When L1 will exec VMRESUME, prepare_vmcs02() will just make vmcs02->eoi_exit_bitmap a copy of vmcs12->eoi_exit_bitmap and therefore empty as-well. Therefore, L2 EOIs will not cause EOI_INDUCED exits to L0 and therefore will not reach L1 IOAPIC emulation code at L0. In order to fully fix the issue at hand here, you will basically need vmcs02->eoi_exit_bitmap to be a logical OR of vmcs12->eoi_exit_bitmap and vmcs01->eoi_exit_bitmap. Then on EOI_INDUCED exits to L0, nested_vmx_exit_reflected() will decide if it should be forwarded to L1 or not. That will also fix the bug this patch aims to fix. However, I am a bit worry about this approach as it can cause a significant performance overhead for the common case of not pass-through L1 LAPIC to L2 for supporting this esoteric case. We will try to think if we can come up with a better approach to solving this without such a performance hit. However, I do think that because this scenario is currently not supported by KVM, we should consider first apply the v2 of this patch (Because the other small issue you found) and only then try to find a good-enough fix for the case you just mentioned here. Am I missing something here or does it make sense? > > However, the patch is generally in the right direction. Another small > issue: > >> @@ -6833,7 +6839,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) >> goto out; >> } >> } >> - if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu)) >> + if (kvm_check_request(KVM_REQ_SCAN_IOAPIC, vcpu) || >> + (!is_guest_mode(vcpu) && vcpu->arch.scan_ioapic_pending)) >> vcpu_scan_ioapic(vcpu); > > I think this is not reliable because kvm_request_pending(vcpu) might be > false here---and then you never reach the "if" in the first place. > > Maybe you can add a > > if (vcpu->arch.scan_ioapic_pending) > kvm_make_request(KVM_REQ_SCAN_IOAPIC, vcpu); > > to leave_guest_mode in arch/x86/kvm/kvm_cache_regs.h? > Agree. Sorry I missed that. Our unit-test didn't caught this case :). I will fix this in v2 of this patch and re-send. Thanks for pointing this out. -Liran > Thanks, > > Paolo > >> if (kvm_check_request(KVM_REQ_APIC_PAGE_RELOAD, vcpu)) >> kvm_vcpu_reload_apic_access_page(vcpu); >> @@ -7981,6 +7988,7 @@ int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu) >> kvm = vcpu->kvm; >> >> vcpu->arch.apicv_active = kvm_x86_ops->get_enable_apicv(vcpu); >> + vcpu->arch.scan_ioapic_pending = false; >> vcpu->arch.pv.pv_unhalted = false; >> vcpu->arch.emulate_ctxt.ops = &emulate_ops; >> if (!irqchip_in_kernel(kvm) || kvm_vcpu_is_reset_bsp(vcpu)) >> >