From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sheng Yang Subject: Re: [PATCH 1/3] KVM: x86: Relax accept conditions of kvm_apic_accept_pic_intr Date: Sat, 18 Oct 2008 02:15:20 +0800 Message-ID: <20081017181520.GA24525@yukikaze> References: <20081015142748.385784583@mchn012c.ww002.siemens.net> <20081015142748.606503565@mchn012c.ww002.siemens.net> <200810171311.11309.sheng@linux.intel.com> <48F8488E.9070700@siemens.com> <20081017163530.GA20831@yukikaze> <48F8CCC5.8060502@web.de> <20081017174722.GA24078@yukikaze> <48F8D1BD.5050709@web.de> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Sheng Yang , Jan Kiszka , kvm@vger.kernel.org, avi@redhat.com, jiajun.xu@intel.com To: Jan Kiszka Return-path: Received: from ti-out-0910.google.com ([209.85.142.184]:48469 "EHLO ti-out-0910.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751420AbYJQSP2 (ORCPT ); Fri, 17 Oct 2008 14:15:28 -0400 Received: by ti-out-0910.google.com with SMTP id b6so345322tic.23 for ; Fri, 17 Oct 2008 11:15:26 -0700 (PDT) Content-Disposition: inline In-Reply-To: <48F8D1BD.5050709@web.de> Sender: kvm-owner@vger.kernel.org List-ID: On Fri, Oct 17, 2008 at 07:56:13PM +0200, Jan Kiszka wrote: > Sheng Yang wrote: > > On Fri, Oct 17, 2008 at 07:35:01PM +0200, Jan Kiszka wrote: > >> Sheng Yang wrote: > >>> On Fri, Oct 17, 2008 at 10:10:54AM +0200, Jan Kiszka wrote: > >>>> Sheng Yang wrote: > >>>>> On Wednesday 15 October 2008 22:27:49 Jan Kiszka wrote: > >>>>>> Aligning in-kernel kvm_apic_accept_pic_intr with its user space mate, > >>>>>> this patch relaxes the conditions under which PIC IRQs are accepted > >>>>>> by LVT0. This reflects reality and allows to reuse the service for the > >>>>>> NMI watchdog use case. > >>>>>> > >>>>>> Signed-off-by: Jan Kiszka > >>>>>> --- > >>>>>> arch/x86/kvm/lapic.c | 13 ++++--------- > >>>>>> 1 file changed, 4 insertions(+), 9 deletions(-) > >>>>>> > >>>>>> Index: b/arch/x86/kvm/lapic.c > >>>>>> =================================================================== > >>>>>> --- a/arch/x86/kvm/lapic.c > >>>>>> +++ b/arch/x86/kvm/lapic.c > >>>>>> @@ -1072,16 +1072,11 @@ int kvm_apic_has_interrupt(struct kvm_vc > >>>>>> int kvm_apic_accept_pic_intr(struct kvm_vcpu *vcpu) > >>>>>> { > >>>>>> u32 lvt0 = apic_get_reg(vcpu->arch.apic, APIC_LVT0); > >>>>>> - int r = 0; > >>>>>> > >>>>>> - if (vcpu->vcpu_id == 0) { > >>>>>> - if (!apic_hw_enabled(vcpu->arch.apic)) > >>>>>> - r = 1; > >>>>>> - if ((lvt0 & APIC_LVT_MASKED) == 0 && > >>>>>> - GET_APIC_DELIVERY_MODE(lvt0) == APIC_MODE_EXTINT) > >>>>>> - r = 1; > >>>>>> - } > >>>>>> - return r; > >>>>>> + if (!apic_hw_enabled(vcpu->arch.apic) || > >>>>>> + (lvt0 & APIC_LVT_MASKED) == 0) > >>>>>> + return 1; > >>>>>> + return 0; > >>>>>> } > >>>>>> > >>>>>> void kvm_inject_apic_timer_irqs(struct kvm_vcpu *vcpu) > >>>>>> > >>>>> (sorry for late review...) > >>>>> > >>>>> Thanks to find out the root cause of BSOD! > >>>>> > >>>>> But I am a little concern about this change. As you know, PIC only connect to > >>>>> cpu0. So I think it's not proper to make it generic. > >>>> I don't think so - and if it were true, qemu would have a bug then, see > >>>> its corresponding code. > >>> You can refer to Intel MP spec, virtual wire mode. Google > >>> "MP spec" can find it. > >> Ah, good reference. > >> > >>> Normally PIC is only used in BSP boot up for SMP guest(PIC can't afford SMP, > >>> otherwise we won't need IOAPIC/LAPIC). After that, it should be disabled. > >>> And virtual wire mode works with APIC_MODE_EXTINT on LVT0 of BSP lapic, so > >>> that's why you see > >>> > >>> GET_APIC_DELIVERY_MODE(lvt0) == APIC_MODE_EXTINT > >>> > >>> KVM follow virtual wire mode exactly. > >> According to my understanding of the spec, the virtual wire mode means > >> that the PIC signal is delivered via LVT0, and thus can be received by > >> _all_ CPUs in the system. However, only the BSP usually enables LVT0, > >> thus is receiving the IRQ. When Linux switches to NMI watchdog mode 1, > >> it also unmasks the other CPUs (and reprograms all to deliver NMIs > >> instead of EXTINTs). > >> > >> Then there is also the "PIC Mode", ie. direct delivery to the BSP, and > >> only the latter. That mode is obviously target by the current > >> kvm_apic_accept_pic_intr implementation. But I find no indication in the > >> spec yet that both modes cannot exists in the same system. But I also > >> fail to understand how one could switch between both modes (via software). > > > > No. If so, we don't need to check LVT0. > > OK, there is that IMCR register to enable/disable the PIC Mode - but > neither KVM nor QEMU implement it (which may indicate they both want to > provide Virtual Wire Mode?). When enabled, Virtual Wire Mode is > effectively disabled (for the BSP at least) as the LAPIC is disconnected > (from the BSP). Please check the figure for the difference between PIC mode and virtual wire mode. > > Still, I find not trace in the spec that says only the BSP can receive > PIC interrupts in Virtual Wire Mode (the LVT0 line is connected to _all_ > CPUs). > Only the one to BSP works normally. And I am not meant to go throught PIC when implement NMI Watchdog, otherwise I won't use that apic_local_deliver(). NMI watchdog is too specific case, I don't want to mix them up. -- regards Yang, Sheng > Jan >