On Thu, Sep 1, 2011 at 9:08 AM, Jan Kiszka <jan.kis...@siemens.com> wrote: > The master PIC is connected to the LINTIN0 of the APICs. As the APIC > currently does not track the state of that line, we have to ask the PIC > to reinject its IRQ after the CPU picked up an event from the APIC. > > This introduces pic_get_output to read the master PIC IRQ line state > without changing it. The APIC uses this function to decide if a PIC IRQ > should be reinjected on apic_update_irq. This reflects better how the > real hardware works. > > The patch fixes some failures of the kvm unit tests apic and eventinj by > allowing to enable the proper CPU IRQ deassertion when the guest masks > some pending IRQs at PIC level. > > Signed-off-by: Jan Kiszka <jan.kis...@siemens.com> > --- > > Changes in v2: > - Avoid adding pic_level to the APIC state, obtain PIC output via > pic_get_level instead > - Do not reassert PIC interrupt if APIC is not accepting it > - Use apic_deliver_pic_intr for reassertion to ensure correct > processing > > This is not as nice as the previous version /wrt the interaction of PIC > and APIC. But it avoids breaking the APIC vmstate for the sake of > internal changes, also keeping it compatible with the upcoming KVM > in-kernel APIC (that allows no easy pic_level state extraction). The > interconnection between PIC and APIC may look nicer in the future with > QOM. And in the end this just reflects the "beauty" of the x86 > architecture. > > hw/apic.c | 4 ++++ > hw/i8259.c | 15 +++++++-------- > hw/pc.c | 3 --- > hw/pc.h | 2 +- > 4 files changed, 12 insertions(+), 12 deletions(-) > > diff --git a/hw/apic.c b/hw/apic.c > index d8f56c8..8289eef 100644 > --- a/hw/apic.c > +++ b/hw/apic.c > @@ -23,6 +23,7 @@ > #include "host-utils.h" > #include "sysbus.h" > #include "trace.h" > +#include "pc.h" > > /* APIC Local Vector Table */ > #define APIC_LVT_TIMER 0 > @@ -399,6 +400,9 @@ static void apic_update_irq(APICState *s) > } > if (apic_irq_pending(s) > 0) { > cpu_interrupt(s->cpu_env, CPU_INTERRUPT_HARD); > + } else if (apic_accept_pic_intr(&s->busdev.qdev) && > + pic_get_output(isa_pic)) {
This is indeed ugly. Why doesn't APIC track PIC output? > + apic_deliver_pic_intr(&s->busdev.qdev, 1); > } > } > > diff --git a/hw/i8259.c b/hw/i8259.c > index c0b96ab..5498e5b 100644 > --- a/hw/i8259.c > +++ b/hw/i8259.c > @@ -144,8 +144,7 @@ static int pic_get_irq(PicState *s) > > /* raise irq to CPU if necessary. must be called every time the active > irq may change */ > -/* XXX: should not export it, but it is needed for an APIC kludge */ > -void pic_update_irq(PicState2 *s) > +static void pic_update_irq(PicState2 *s) > { > int irq2, irq; > > @@ -172,14 +171,9 @@ void pic_update_irq(PicState2 *s) > printf("pic: cpu_interrupt\n"); > #endif > qemu_irq_raise(s->parent_irq); > - } > - > -/* all targets should do this rather than acking the IRQ in the cpu */ > -#if defined(TARGET_MIPS) || defined(TARGET_PPC) || defined(TARGET_ALPHA) > - else { > + } else { > qemu_irq_lower(s->parent_irq); > } > -#endif > } > > #ifdef DEBUG_IRQ_LATENCY > @@ -436,6 +430,11 @@ uint32_t pic_intack_read(PicState2 *s) > return ret; > } > > +int pic_get_output(PicState2 *s) > +{ > + return (pic_get_irq(&s->pics[0]) >= 0); > +} > + > static void elcr_ioport_write(void *opaque, uint32_t addr, uint32_t val) > { > PicState *s = opaque; > diff --git a/hw/pc.c b/hw/pc.c > index 263fb1a..b7b5d6f 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -156,9 +156,6 @@ int cpu_get_pic_interrupt(CPUState *env) > > intno = apic_get_interrupt(env->apic_state); > if (intno >= 0) { > - /* set irq request if a PIC irq is still pending */ > - /* XXX: improve that */ > - pic_update_irq(isa_pic); > return intno; > } > /* read the irq from the PIC */ > diff --git a/hw/pc.h b/hw/pc.h > index dae736e..958c77d 100644 > --- a/hw/pc.h > +++ b/hw/pc.h > @@ -65,7 +65,7 @@ void pic_set_irq(int irq, int level); > void pic_set_irq_new(void *opaque, int irq, int level); > qemu_irq *i8259_init(qemu_irq parent_irq); > int pic_read_irq(PicState2 *s); > -void pic_update_irq(PicState2 *s); > +int pic_get_output(PicState2 *s); > uint32_t pic_intack_read(PicState2 *s); > void pic_info(Monitor *mon); > void irq_info(Monitor *mon); >