From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756954AbcIMUoE (ORCPT ); Tue, 13 Sep 2016 16:44:04 -0400 Received: from mail-wm0-f65.google.com ([74.125.82.65]:34978 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752325AbcIMUoC (ORCPT ); Tue, 13 Sep 2016 16:44:02 -0400 Subject: Re: kvm-unit-test fail for split irqchip To: Radim Krcmar , Wanpeng Li References: <20160913190145.GE15680@potion> Cc: kvm , "linux-kernel@vger.kernel.org" From: Paolo Bonzini Message-ID: <992bf785-c4bb-b2c1-dd9b-87b74d9127c2@redhat.com> Date: Tue, 13 Sep 2016 22:43:41 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20160913190145.GE15680@potion> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 13/09/2016 21:01, Radim Krcmar wrote: > kvm_handle_interrupt() does > > interrupt_request |= CPU_INTERRUPT_HARD > > which later calls cpu_get_pic_interrupt() in kvm_arch_pre_run(), but > that function uses stale information from APIC and injects 62 again. > If we synchronized the APIC, then the test would #GP, because there > would be no injectable interrupt in LAPIC or PIC, so pic_read_irq() > would return 15, thinking it was spurious. > > I think the bug starts in pic_irq_request(), which should not touch > LAPIC. The patch below makes it work (just the second hunk is > sufficient), but it's still far from sane code ... This makes sense. Most of the functions exported by hw/intc/apic.c should only be used with a userspace APIC: 0000000000000b70 T apic_accept_pic_intr 00000000000010f0 T apic_deliver_irq 00000000000011e0 T apic_deliver_pic_intr 0000000000001310 T apic_get_interrupt 0000000000001270 T apic_poll_irq 0000000000000a40 T apic_sipi The patch is okay, though for consistency with other code I'd use !kvm_irqchip_in_kernel() rather than !kvm_irqchip_is_split(). Wanpeng, can you do that, and change hw/intc/apic.c to use a new casting macro APICCommonState *s = APIC(dev); instead of APIC_COMMON? Thanks, Paolo > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 47593b741a5b..6983e9f13813 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -161,13 +161,16 @@ int cpu_get_pic_interrupt(CPUX86State *env) > X86CPU *cpu = x86_env_get_cpu(env); > int intno; > > - intno = apic_get_interrupt(cpu->apic_state); > - if (intno >= 0) { > - return intno; > - } > - /* read the irq from the PIC */ > - if (!apic_accept_pic_intr(cpu->apic_state)) { > - return -1; > + if (!kvm_irqchip_is_split()) { > + /* XXX: why query APIC at all? */ > + intno = apic_get_interrupt(cpu->apic_state); > + if (intno >= 0) { > + return intno; > + } > + /* read the irq from the PIC */ > + if (!apic_accept_pic_intr(cpu->apic_state)) { > + return -1; > + } > } > > intno = pic_read_irq(isa_pic); > @@ -180,7 +183,7 @@ static void pic_irq_request(void *opaque, int irq, int level) > X86CPU *cpu = X86_CPU(cs); > > DPRINTF("pic_irqs: %s irq %d\n", level? "raise" : "lower", irq); > - if (cpu->apic_state) { > + if (cpu->apic_state && !kvm_irqchip_is_split()) { > CPU_FOREACH(cs) { > cpu = X86_CPU(cs); > if (apic_accept_pic_intr(cpu->apic_state)) { > >