From: Paolo Bonzini <pbonzini@redhat.com>
To: Radim Krcmar <rkrcmar@redhat.com>, Wanpeng Li <kernellwp@gmail.com>
Cc: kvm <kvm@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: kvm-unit-test fail for split irqchip
Date: Tue, 13 Sep 2016 22:43:41 +0200 [thread overview]
Message-ID: <992bf785-c4bb-b2c1-dd9b-87b74d9127c2@redhat.com> (raw)
In-Reply-To: <20160913190145.GE15680@potion>
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)) {
>
>
next prev parent reply other threads:[~2016-09-13 20:44 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-09-13 11:06 kvm-unit-test fail for split irqchip Wanpeng Li
2016-09-13 19:01 ` Radim Krcmar
2016-09-13 20:43 ` Paolo Bonzini [this message]
2016-09-14 3:57 ` Wanpeng Li
2016-09-14 9:42 ` Paolo Bonzini
2016-09-15 0:58 ` Wanpeng Li
2016-09-14 3:52 ` Wanpeng Li
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=992bf785-c4bb-b2c1-dd9b-87b74d9127c2@redhat.com \
--to=pbonzini@redhat.com \
--cc=kernellwp@gmail.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=rkrcmar@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.