Patchwork [v2] pc: Fix and clean up PIC-to-APIC IRQ path

login
register
mail settings
Submitter Jan Kiszka
Date Sept. 1, 2011, 9:08 a.m.
Message ID <4E5F4B81.2050905@siemens.com>
Download mbox | patch
Permalink /patch/112845/
State New
Headers show

Comments

Jan Kiszka - Sept. 1, 2011, 9:08 a.m.
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.kiszka@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(-)
Blue Swirl - Sept. 3, 2011, 8:58 a.m.
On Thu, Sep 1, 2011 at 9:08 AM, Jan Kiszka <jan.kiszka@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.kiszka@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);
>
Jan Kiszka - Sept. 3, 2011, 11:17 a.m.
On 2011-09-03 10:58, Blue Swirl wrote:
> On Thu, Sep 1, 2011 at 9:08 AM, Jan Kiszka <jan.kiszka@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.kiszka@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?

For the reasons explained above.

Jan
Blue Swirl - Sept. 3, 2011, 11:37 a.m.
On Sat, Sep 3, 2011 at 11:17 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
> On 2011-09-03 10:58, Blue Swirl wrote:
>> On Thu, Sep 1, 2011 at 9:08 AM, Jan Kiszka <jan.kiszka@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.kiszka@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?
>
> For the reasons explained above.

Breaking vmstate compatibility? Don't we have all kinds of
compatibility stuff for this?

I'm not opposed to the patch as is, it's still a cleanup, but I wish
this ugliness could be avoided.
Jan Kiszka - Sept. 3, 2011, 6:14 p.m.
On 2011-09-03 13:37, Blue Swirl wrote:
> On Sat, Sep 3, 2011 at 11:17 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> On 2011-09-03 10:58, Blue Swirl wrote:
>>> On Thu, Sep 1, 2011 at 9:08 AM, Jan Kiszka <jan.kiszka@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.kiszka@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?
>>
>> For the reasons explained above.
> 
> Breaking vmstate compatibility? Don't we have all kinds of
> compatibility stuff for this?

As I said, we do not have access to some pic_level equivalent with the
in-kernel APIC model. And I don't want to break vmstate compatibility
between user and kernel space models needlessly.

> 
> I'm not opposed to the patch as is, it's still a cleanup, but I wish
> this ugliness could be avoided.

It's primarily a bug fix now. And the ugliness level should drop again
when IRQ management will be reworked.

Jan

Patch

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)) {
+        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);