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 00:35:30 +0800 Message-ID: <20081017163530.GA20831@yukikaze> References: <20081015142748.385784583@mchn012c.ww002.siemens.net> <20081015142748.606503565@mchn012c.ww002.siemens.net> <200810171311.11309.sheng@linux.intel.com> <48F8488E.9070700@siemens.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Sheng Yang , 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.187]:43992 "EHLO ti-out-0910.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754634AbYJQQfl (ORCPT ); Fri, 17 Oct 2008 12:35:41 -0400 Received: by ti-out-0910.google.com with SMTP id b6so328278tic.23 for ; Fri, 17 Oct 2008 09:35:39 -0700 (PDT) Content-Disposition: inline In-Reply-To: <48F8488E.9070700@siemens.com> Sender: kvm-owner@vger.kernel.org List-ID: 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. 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. For QEmu, it just check if lapic LVT0 is masked, and don't check vcpu0. That's indeed a little problematic, for it's not that sufficient to determine if it's programmed as virtual wire mode and used for deliver interrupts from PIC. Well, in most condition, it can work. But maybe it's not clean in logic. For NMI watchdog here, we use a little more tricky way other than normal PIC/LAPIC interaction. IIRC, NMI watchdog don't mask PIC after enable IOAPIC, it also don't mask LVT0 of every LAPIC. It use physical connection of PIT to PIC then to LAPIC LVT0 to send NMI. Program LVT0 to NMI, then every PIT interrupt would go through PIC, arrive at LVT0, trig a NMI. So I think the key problem for Windows is, they don't need it, but we send the NMIs. We send the NMI when LVT0 is masked. Base on this, I think your optimize patch also can resolve this issue? It's already including necessary judgment. We will try it next week. -- regards Yang, Sheng > > > > > Maybe you can use kvm_apic_accept_pic_intr(vcpu0) in later patch? > > Sorry, don't get what you mean with this (independent of the above). > > Jan > > -- > Siemens AG, Corporate Technology, CT SE 2 > Corporate Competence Center Embedded Linux > -- > To unsubscribe from this list: send the line "unsubscribe kvm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html