[Qemu-devel] Re: [PATCH 10/16] x86: Refactor RTC IRQ coalescing workaround

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 12764 bytes --]

Blue Swirl wrote:
> On Sun, Jun 6, 2010 at 8:10 AM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> Make use of the new IRQ message and report delivery results from the
>> sink to the source. As a by-product, this also adds de-coalescing
>> support to the PIC.
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  hw/apic.c        |   64 +++++++++++++++++++----------------------
>>  hw/apic.h        |    9 ++----
>>  hw/i8259.c       |   16 ++++++++++-
>>  hw/ioapic.c      |   20 ++++++++++---
>>  hw/mc146818rtc.c |   83 ++++++++++++++++++++++++++++++++++-------------------
>>  hw/pc.c          |   29 ++++++++++++++++--
>>  6 files changed, 141 insertions(+), 80 deletions(-)
>>
>> diff --git a/hw/apic.c b/hw/apic.c
>> index 7fbd79b..f9587d1 100644
>> --- a/hw/apic.c
>> +++ b/hw/apic.c
>> @@ -123,10 +123,8 @@ typedef struct APICState {
>>  static int apic_io_memory;
>>  static APICState *local_apics[MAX_APICS + 1];
>>  static int last_apic_idx = 0;
>> -static int apic_irq_delivered;
>>
>> -
>> -static void apic_set_irq(APICState *s, int vector_num, int trigger_mode);
>> +static int apic_set_irq(APICState *s, int vector_num, int trigger_mode);
>>  static void apic_update_irq(APICState *s);
>>  static void apic_get_delivery_bitmask(uint32_t *deliver_bitmask,
>>                                       uint8_t dest, uint8_t dest_mode);
>> @@ -239,12 +237,12 @@ void apic_deliver_pic_intr(CPUState *env, int level)
>>     }\
>>  }
>>
>> -static void apic_bus_deliver(const uint32_t *deliver_bitmask,
>> -                             uint8_t delivery_mode,
>> -                             uint8_t vector_num, uint8_t polarity,
>> -                             uint8_t trigger_mode)
>> +static int apic_bus_deliver(const uint32_t *deliver_bitmask,
>> +                            uint8_t delivery_mode, uint8_t vector_num,
>> +                            uint8_t polarity, uint8_t trigger_mode)
>>  {
>>     APICState *apic_iter;
>> +    int ret;
>>
>>     switch (delivery_mode) {
>>         case APIC_DM_LOWPRI:
>> @@ -261,11 +259,12 @@ static void apic_bus_deliver(const uint32_t *deliver_bitmask,
>>                 if (d >= 0) {
>>                     apic_iter = local_apics[d];
>>                     if (apic_iter) {
>> -                        apic_set_irq(apic_iter, vector_num, trigger_mode);
>> +                        return apic_set_irq(apic_iter, vector_num,
>> +                                            trigger_mode);
>>                     }
>>                 }
>>             }
>> -            return;
>> +            return QEMU_IRQ_MASKED;
>>
>>         case APIC_DM_FIXED:
>>             break;
>> @@ -273,34 +272,42 @@ static void apic_bus_deliver(const uint32_t *deliver_bitmask,
>>         case APIC_DM_SMI:
>>             foreach_apic(apic_iter, deliver_bitmask,
>>                 cpu_interrupt(apic_iter->cpu_env, CPU_INTERRUPT_SMI) );
>> -            return;
>> +            return QEMU_IRQ_DELIVERED;
>>
>>         case APIC_DM_NMI:
>>             foreach_apic(apic_iter, deliver_bitmask,
>>                 cpu_interrupt(apic_iter->cpu_env, CPU_INTERRUPT_NMI) );
>> -            return;
>> +            return QEMU_IRQ_DELIVERED;
>>
>>         case APIC_DM_INIT:
>>             /* normal INIT IPI sent to processors */
>>             foreach_apic(apic_iter, deliver_bitmask,
>>                          cpu_interrupt(apic_iter->cpu_env, CPU_INTERRUPT_INIT) );
>> -            return;
>> +            return QEMU_IRQ_DELIVERED;
>>
>>         case APIC_DM_EXTINT:
>>             /* handled in I/O APIC code */
>>             break;
>>
>>         default:
>> -            return;
>> +            return QEMU_IRQ_MASKED;
>>     }
>>
>> +    ret = QEMU_IRQ_MASKED;
>>     foreach_apic(apic_iter, deliver_bitmask,
>> -                 apic_set_irq(apic_iter, vector_num, trigger_mode) );
>> +        if (ret == QEMU_IRQ_MASKED)
>> +            ret = QEMU_IRQ_COALESCED;
>> +        if (apic_set_irq(apic_iter, vector_num,
>> +                         trigger_mode) == QEMU_IRQ_DELIVERED) {
>> +            ret = QEMU_IRQ_DELIVERED;
>> +        }
>> +    );
>> +    return ret;
>>  }
>>
>> -void apic_deliver_irq(uint8_t dest, uint8_t dest_mode,
>> -                      uint8_t delivery_mode, uint8_t vector_num,
>> -                      uint8_t polarity, uint8_t trigger_mode)
>> +int apic_deliver_irq(uint8_t dest, uint8_t dest_mode,
>> +                     uint8_t delivery_mode, uint8_t vector_num,
>> +                     uint8_t polarity, uint8_t trigger_mode)
>>  {
>>     uint32_t deliver_bitmask[MAX_APIC_WORDS];
>>
>> @@ -308,8 +315,8 @@ void apic_deliver_irq(uint8_t dest, uint8_t dest_mode,
>>             " polarity %d trigger_mode %d\n", __func__, dest, dest_mode,
>>             delivery_mode, vector_num, polarity, trigger_mode);
>>     apic_get_delivery_bitmask(deliver_bitmask, dest, dest_mode);
>> -    apic_bus_deliver(deliver_bitmask, delivery_mode, vector_num, polarity,
>> -                     trigger_mode);
>> +    return apic_bus_deliver(deliver_bitmask, delivery_mode, vector_num,
>> +                            polarity, trigger_mode);
>>  }
>>
>>  void cpu_set_apic_base(CPUState *env, uint64_t val)
>> @@ -402,22 +409,10 @@ static void apic_update_irq(APICState *s)
>>     cpu_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
>>  }
>>
>> -void apic_reset_irq_delivered(void)
>> -{
>> -    DPRINTF_C("%s: old coalescing %d\n", __func__, apic_irq_delivered);
>> -    apic_irq_delivered = 0;
>> -}
>> -
>> -int apic_get_irq_delivered(void)
>> -{
>> -    DPRINTF_C("%s: returning coalescing %d\n", __func__, apic_irq_delivered);
>> -    return apic_irq_delivered;
>> -}
>> -
>> -static void apic_set_irq(APICState *s, int vector_num, int trigger_mode)
>> +static int apic_set_irq(APICState *s, int vector_num, int trigger_mode)
>>  {
>> -    apic_irq_delivered += !get_bit(s->irr, vector_num);
>> -    DPRINTF_C("%s: coalescing %d\n", __func__, apic_irq_delivered);
>> +    int ret = get_bit(s->irr, vector_num) ? QEMU_IRQ_COALESCED
>> +                                          : QEMU_IRQ_DELIVERED;
>>
>>     set_bit(s->irr, vector_num);
>>     if (trigger_mode)
>> @@ -425,6 +420,7 @@ static void apic_set_irq(APICState *s, int vector_num, int trigger_mode)
>>     else
>>         reset_bit(s->tmr, vector_num);
>>     apic_update_irq(s);
>> +    return ret;
>>  }
>>
>>  static void apic_eoi(APICState *s)
>> diff --git a/hw/apic.h b/hw/apic.h
>> index e1954f4..738d98a 100644
>> --- a/hw/apic.h
>> +++ b/hw/apic.h
>> @@ -2,17 +2,14 @@
>>  #define APIC_H
>>
>>  typedef struct IOAPICState IOAPICState;
>> -void apic_deliver_irq(uint8_t dest, uint8_t dest_mode,
>> -                             uint8_t delivery_mode,
>> -                             uint8_t vector_num, uint8_t polarity,
>> -                             uint8_t trigger_mode);
>> +int apic_deliver_irq(uint8_t dest, uint8_t dest_mode,
>> +                     uint8_t delivery_mode, uint8_t vector_num,
>> +                     uint8_t polarity, uint8_t trigger_mode);
>>  int apic_init(CPUState *env);
>>  int apic_accept_pic_intr(CPUState *env);
>>  void apic_deliver_pic_intr(CPUState *env, int level);
>>  int apic_get_interrupt(CPUState *env);
>>  qemu_irq *ioapic_init(void);
>> -void apic_reset_irq_delivered(void);
>> -int apic_get_irq_delivered(void);
>>
>>  int cpu_is_bsp(CPUState *env);
>>
>> diff --git a/hw/i8259.c b/hw/i8259.c
>> index f743ee8..09150c4 100644
>> --- a/hw/i8259.c
>> +++ b/hw/i8259.c
>> @@ -189,6 +189,9 @@ int64_t irq_time[16];
>>  static void i8259_set_irq(qemu_irq irq, void *opaque, int n, int level)
>>  {
>>     PicState2 *s = opaque;
>> +    PicState *pic;
>> +    int result = QEMU_IRQ_DELIVERED;
>> +    int mask;
>>
>>  #if defined(DEBUG_PIC) || defined(DEBUG_IRQ_COUNT)
>>     if (level != irq_level[n]) {
>> @@ -205,8 +208,19 @@ static void i8259_set_irq(qemu_irq irq, void *opaque, int n, int level)
>>         irq_time[n] = qemu_get_clock(vm_clock);
>>     }
>>  #endif
>> -    pic_set_irq1(&s->pics[n >> 3], n & 7, level);
>> +    pic = &s->pics[n >> 3];
>> +    n &= 7;
>> +    mask = 1 << n;
>> +    if (level) {
>> +        if (pic->imr & mask) {
>> +            result = QEMU_IRQ_MASKED;
>> +        } else if (pic->irr & mask) {
>> +            result = QEMU_IRQ_COALESCED;
>> +        }
>> +    }
>> +    pic_set_irq1(pic, n, level);
>>     pic_update_irq(s);
>> +    qemu_irq_fire_delivery_cb(irq, level, result);
>>  }
>>
>>  /* acknowledge interrupt 'irq' */
>> diff --git a/hw/ioapic.c b/hw/ioapic.c
>> index d818573..b54738f 100644
>> --- a/hw/ioapic.c
>> +++ b/hw/ioapic.c
>> @@ -58,7 +58,7 @@ struct IOAPICState {
>>     uint64_t ioredtbl[IOAPIC_NUM_PINS];
>>  };
>>
>> -static void ioapic_service(IOAPICState *s)
>> +static int ioapic_service(IOAPICState *s)
>>  {
>>     uint8_t i;
>>     uint8_t trig_mode;
>> @@ -69,12 +69,16 @@ static void ioapic_service(IOAPICState *s)
>>     uint8_t dest;
>>     uint8_t dest_mode;
>>     uint8_t polarity;
>> +    int ret = QEMU_IRQ_MASKED;
>>
>>     for (i = 0; i < IOAPIC_NUM_PINS; i++) {
>>         mask = 1 << i;
>>         if (s->irr & mask) {
>>             entry = s->ioredtbl[i];
>>             if (!(entry & IOAPIC_LVT_MASKED)) {
>> +                if (ret == QEMU_IRQ_MASKED) {
>> +                    ret = QEMU_IRQ_COALESCED;
>> +                }
>>                 trig_mode = ((entry >> 15) & 1);
>>                 dest = entry >> 56;
>>                 dest_mode = (entry >> 11) & 1;
>> @@ -87,16 +91,21 @@ static void ioapic_service(IOAPICState *s)
>>                 else
>>                     vector = entry & 0xff;
>>
>> -                apic_deliver_irq(dest, dest_mode, delivery_mode,
>> -                                 vector, polarity, trig_mode);
>> +                if (apic_deliver_irq(dest, dest_mode,
>> +                                     delivery_mode, vector, polarity,
>> +                                     trig_mode) == QEMU_IRQ_DELIVERED) {
>> +                    ret = QEMU_IRQ_DELIVERED;
>> +                }
>>             }
>>         }
>>     }
>> +    return ret;
>>  }
>>
>>  static void ioapic_set_irq(qemu_irq irq, void *opaque, int vector, int level)
>>  {
>>     IOAPICState *s = opaque;
>> +    int result = QEMU_IRQ_MASKED;
>>
>>     /* ISA IRQs map to GSI 1-1 except for IRQ0 which maps
>>      * to GSI 2.  GSI maps to ioapic 1-1.  This is not
>> @@ -114,7 +123,7 @@ static void ioapic_set_irq(qemu_irq irq, void *opaque, int vector, int level)
>>             /* level triggered */
>>             if (level) {
>>                 s->irr |= mask;
>> -                ioapic_service(s);
>> +                result = ioapic_service(s);
>>             } else {
>>                 s->irr &= ~mask;
>>             }
>> @@ -122,10 +131,11 @@ static void ioapic_set_irq(qemu_irq irq, void *opaque, int vector, int level)
>>             /* edge triggered */
>>             if (level) {
>>                 s->irr |= mask;
>> -                ioapic_service(s);
>> +                result = ioapic_service(s);
>>             }
>>         }
>>     }
>> +    qemu_irq_fire_delivery_cb(irq, level, result);
>>  }
>>
>>  static uint32_t ioapic_mem_readl(void *opaque, target_phys_addr_t addr)
>> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
>> index c3e6a70..cbb98a4 100644
>> --- a/hw/mc146818rtc.c
>> +++ b/hw/mc146818rtc.c
>> @@ -25,7 +25,6 @@
>>  #include "qemu-timer.h"
>>  #include "sysemu.h"
>>  #include "pc.h"
>> -#include "apic.h"
>>  #include "isa.h"
>>  #include "hpet_emul.h"
>>  #include "mc146818rtc.h"
>> @@ -101,7 +100,7 @@ typedef struct RTCState {
>>     QEMUTimer *second_timer2;
>>  } RTCState;
>>
>> -static void rtc_irq_raise(qemu_irq irq)
>> +static void rtc_irq_raise(RTCState *s, IRQMsg *msg)
>>  {
>>     /* When HPET is operating in legacy mode, RTC interrupts are disabled
>>      * We block qemu_irq_raise, but not qemu_irq_lower, in case legacy
>> @@ -109,9 +108,14 @@ static void rtc_irq_raise(qemu_irq irq)
>>      * be lowered in any case
>>      */
>>  #if defined TARGET_I386
>> -    if (!hpet_in_legacy_mode())
>> +    if (hpet_in_legacy_mode()) {
>> +        if (msg) {
>> +            msg->delivery_cb(s->irq, s, -1, -1, QEMU_IRQ_MASKED);
> 
> Shouldn't you use qemu_irq_fire_delivery_cb() here?

We didn't send msg, so it is not yet associated with s->irq at this
point. Moreover, this hunk is only temporarily, removed in patch 11 again.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]