Re: [Qemu-devel] [PATCH] pc: Clean up PIC-to-APIC IRQ path

[Blue Swirl <blauwirbel@gmail.com>]

On Sat, Aug 27, 2011 at 2:16 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> 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 re-inject its IRQ after the CPU picked up an event from the APIC.
>
> Adds the proper state tracking so that we can already re-assert the CPU
> IRQ at APIC level if there is a pending PIC IRQ. This allows to remove
> all the old workarounds.
>
> The patch also fixes some failures of the kvm unit tests apic and
> eventinj by enabling a proper CPU IRQ deassert when the guest masks some
> pending IRQs at PIC level.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>
> It turned out that this patch from a larger cleanup series has no
> dependencies and can be applied directly to master to fix the observed
> bug.
>
>  hw/apic.c  |    4 +++-
>  hw/i8259.c |   10 ++--------
>  hw/pc.c    |    3 ---
>  hw/pc.h    |    1 -
>  4 files changed, 5 insertions(+), 13 deletions(-)
>
> diff --git a/hw/apic.c b/hw/apic.c
> index d8f56c8..22ad635 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -104,6 +104,7 @@ struct APICState {
>     QEMUTimer *timer;
>     int sipi_vector;
>     int wait_for_sipi;
> +    int pic_level;
>  };
>
>  static APICState *local_apics[MAX_APICS + 1];
> @@ -186,6 +187,7 @@ void apic_deliver_pic_intr(DeviceState *d, int level)
>  {
>     APICState *s = DO_UPCAST(APICState, busdev.qdev, d);
>
> +    s->pic_level = level;
>     if (level) {
>         apic_local_deliver(s, APIC_LVT_LINT0);
>     } else {
> @@ -397,7 +399,7 @@ static void apic_update_irq(APICState *s)
>     if (!(s->spurious_vec & APIC_SV_ENABLE)) {
>         return;
>     }
> -    if (apic_irq_pending(s) > 0) {
> +    if (apic_irq_pending(s) > 0 || s->pic_level) {
>         cpu_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
>     }
>  }
> diff --git a/hw/i8259.c b/hw/i8259.c
> index c0b96ab..cc6f76b 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 {

Nice cleanup, this was pretty ugly.

Isn't it possible to compile the device in hwlib now? That should save
about 11 compiles for the full build since it is used by many targets.

>         qemu_irq_lower(s->parent_irq);
>     }
> -#endif
>  }
>
>  #ifdef DEBUG_IRQ_LATENCY
> 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..f5ff4c0 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -65,7 +65,6 @@ 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);
>  uint32_t pic_intack_read(PicState2 *s);
>  void pic_info(Monitor *mon);
>  void irq_info(Monitor *mon);
>
>