From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH] KVM: x86: Rework INIT and SIPI handling Date: Wed, 13 Mar 2013 13:40:33 +0100 Message-ID: <514073C1.6010203@siemens.com> References: <51403DEF.3090403@siemens.com> <20130313103139.GI11223@redhat.com> <51405FFD.2040102@siemens.com> <514064DA.1080009@siemens.com> <20130313122902.GL11223@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Marcelo Tosatti , kvm , Paolo Bonzini To: Gleb Natapov Return-path: Received: from goliath.siemens.de ([192.35.17.28]:17740 "EHLO goliath.siemens.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932672Ab3CMMkj (ORCPT ); Wed, 13 Mar 2013 08:40:39 -0400 In-Reply-To: <20130313122902.GL11223@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 2013-03-13 13:29, Gleb Natapov wrote: > On Wed, Mar 13, 2013 at 12:36:58PM +0100, Jan Kiszka wrote: >> On 2013-03-13 12:16, Jan Kiszka wrote: >>>>> @@ -5871,8 +5867,8 @@ static int __vcpu_run(struct kvm_vcpu *vcpu) >>>>> srcu_read_unlock(&kvm->srcu, vcpu->srcu_idx); >>>>> kvm_vcpu_block(vcpu); >>>>> vcpu->srcu_idx = srcu_read_lock(&kvm->srcu); >>>>> - if (kvm_check_request(KVM_REQ_UNHALT, vcpu)) >>>>> - { >>>>> + if (kvm_check_request(KVM_REQ_UNHALT, vcpu)) { >>>>> + kvm_apic_accept_events(vcpu); >>>> I think we can drop this. If INIT happens while vcpu is halted it will >>>> become runnable here and kvm_apic_accept_events() will be called in >>>> vcpu_enter_guest(). >>> >>> I'm not that sure, but I will recheck carefully. >> >> Doesn't work: If the state was INIT_RECEIVED, we will not process the >> SIPI but reenter kvm_vcpu_block. > Which raises the question. What if vcpu is in INIT_RECEIVED and it > receives NMI. It will make kvm_arch_vcpu_runnable() return true but code > will get back to kvm_vcpu_block() again. Sounds like we had a bug in this area before. This patch won't improve it yet. We need to "block NMIs" while in wait-for-sipi state. BTW, I'v just stumbled over more suspicious code: static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode, ... switch (delivery_mode) { case APIC_DM_LOWEST: vcpu->arch.apic_arb_prio++; What makes this (remote) increment safe that we can avoid an atomic inc? > >> And it's more consistent to process the >> events here IMHO. >> > I would like to minimize a number of places kvm_apic_accept_events() > is called, but it looks like we cannot remove it from here indeed. What > about calling it in "case: KVM_MP_STATE_INIT_RECEIVED"? Should work, but what is the benefit? I'd prefer to avoid temporary switching to RUNNABLE, entering vcpu_enter_guest just to find out it was an INIT_RECEIVED transition. Checking unconditionally makes the control flow simpler. Jan -- Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux