qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v2] pc: Fix and clean up PIC-to-APIC IRQ path


From: Blue Swirl
Subject: Re: [Qemu-devel] [PATCH v2] pc: Fix and clean up PIC-to-APIC IRQ path
Date: Sat, 3 Sep 2011 08:58:49 +0000

On Thu, Sep 1, 2011 at 9:08 AM, Jan Kiszka <address@hidden> 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 <address@hidden>
> ---
>
> 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);
>



reply via email to

[Prev in Thread] Current Thread [Next in Thread]