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

[Jan Kiszka]

On 2011-08-28 09:10, Blue Swirl wrote:
> On Sat, Aug 27, 2011 at 2:16 PM, Jan Kiszka <address@hidden> wrote:
>> From: Jan Kiszka <address@hidden>
>>
>> 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 <address@hidden>
>> ---
>>
>> 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.

Not yet, but at the end of my queue (pic_info has to be refactored first).

Jan

signature.asc
Description: OpenPGP digital signature