[Qemu-devel] Re: [PATCH 11/16] hpet/rtc: Rework RTC IRQ replacement by HPET

[Blue Swirl]

On Sun, Jun 6, 2010 at 8:11 AM, Jan Kiszka <address@hidden> wrote:
> From: Jan Kiszka <address@hidden>
>
> Allow the intercept the RTC IRQ for the HPET legacy mode. Then push
> routing to IRQ8 completely into the HPET. This allows to turn
> hpet_in_legacy_mode() into a private function. Furthermore, this stops
> the RTC from clearing IRQ8 even if the HPET is in control.
>
> This patch comes with a side effect: The RTC timers will no longer be
> stoppend when there is no IRQ consumer, possibly causing a minor
> performance degration. But as the guest may want to redirect the RTC to
> the SCI in that mode, it should normally disable unused IRQ source
> anyway.
>
> Signed-off-by: Jan Kiszka <address@hidden>
> ---
>  hw/hpet.c        |   42 +++++++++++++++++++++++++++++++++++-------
>  hw/hpet_emul.h   |    4 ----
>  hw/mc146818rtc.c |   54 
> +++++++++++++++---------------------------------------
>  hw/mc146818rtc.h |    4 +++-
>  hw/mips_jazz.c   |    2 +-
>  hw/mips_malta.c  |    2 +-
>  hw/mips_r4k.c    |    2 +-
>  hw/pc.c          |   14 ++++++++------
>  hw/ppc_prep.c    |    2 +-
>  9 files changed, 65 insertions(+), 61 deletions(-)
>
> diff --git a/hw/hpet.c b/hw/hpet.c
> index 041dd84..d26cad5 100644
> --- a/hw/hpet.c
> +++ b/hw/hpet.c
> @@ -30,6 +30,7 @@
>  #include "qemu-timer.h"
>  #include "hpet_emul.h"
>  #include "sysbus.h"
> +#include "mc146818rtc.h"
>
>  //#define HPET_DEBUG
>  #ifdef HPET_DEBUG
> @@ -58,6 +59,7 @@ typedef struct HPETState {
>     SysBusDevice busdev;
>     uint64_t hpet_offset;
>     qemu_irq irqs[HPET_NUM_IRQ_ROUTES];
> +    uint8_t rtc_irq_level;
>     HPETTimer timer[HPET_NUM_TIMERS];
>
>     /* Memory-mapped, software visible registers */
> @@ -69,12 +71,9 @@ typedef struct HPETState {
>
>  static HPETState *hpet_statep;
>
> -uint32_t hpet_in_legacy_mode(void)
> +static uint32_t hpet_in_legacy_mode(HPETState *s)
>  {
> -    if (!hpet_statep) {
> -        return 0;
> -    }
> -    return hpet_statep->config & HPET_CFG_LEGACY;
> +    return s->config & HPET_CFG_LEGACY;
>  }
>
>  static uint32_t timer_int_route(struct HPETTimer *timer)
> @@ -166,12 +165,12 @@ static void update_irq(struct HPETTimer *timer)
>  {
>     int route;
>
> -    if (timer->tn <= 1 && hpet_in_legacy_mode()) {
> +    if (timer->tn <= 1 && hpet_in_legacy_mode(timer->state)) {
>         /* if LegacyReplacementRoute bit is set, HPET specification requires
>          * timer0 be routed to IRQ0 in NON-APIC or IRQ2 in the I/O APIC,
>          * timer1 be routed to IRQ8 in NON-APIC or IRQ8 in the I/O APIC.
>          */
> -        route = (timer->tn == 0) ? 0 : 8;
> +        route = (timer->tn == 0) ? 0 : RTC_ISA_IRQ;
>     } else {
>         route = timer_int_route(timer);
>     }
> @@ -515,8 +514,10 @@ static void hpet_ram_writel(void *opaque, 
> target_phys_addr_t addr,
>             /* i8254 and RTC are disabled when HPET is in legacy mode */
>             if (activating_bit(old_val, new_val, HPET_CFG_LEGACY)) {
>                 hpet_pit_disable();
> +                qemu_irq_lower(s->irqs[RTC_ISA_IRQ]);
>             } else if (deactivating_bit(old_val, new_val, HPET_CFG_LEGACY)) {
>                 hpet_pit_enable();
> +                qemu_set_irq(s->irqs[RTC_ISA_IRQ], s->rtc_irq_level);
>             }
>             break;
>         case HPET_CFG + 4:
> @@ -607,6 +608,30 @@ static void hpet_reset(DeviceState *d)
>     count = 1;
>  }
>
> +static void hpet_rtc_delivery_cb(qemu_irq irq, void *opaque, int n, int 
> level,
> +                                 int result)
> +{
> +    qemu_irq orig_irq = opaque;
> +
> +    qemu_irq_fire_delivery_cb(orig_irq, level, result);
> +}
> +
> +static void hpet_handle_rtc_irq(qemu_irq irq, void *opaque, int n, int level)
> +{
> +    HPETState *s = FROM_SYSBUS(HPETState, opaque);
> +    IRQMsg msg = {
> +        .delivery_cb = hpet_rtc_delivery_cb,
> +        .delivery_opaque = irq,
> +    };
> +
> +    s->rtc_irq_level = level;
> +    if (hpet_in_legacy_mode(s)) {
> +        qemu_irq_fire_delivery_cb(irq, level, QEMU_IRQ_MASKED);
> +    } else {
> +        qemu_set_irq_msg(s->irqs[RTC_ISA_IRQ], level, &msg);

This is the problem with passing around stack allocated objects: after
this function finishes, s->irqs[RTC_ISA_IRQ].msg is a dangling pointer
to some stack space.

> +    }
> +}
> +
>  static int hpet_init(SysBusDevice *dev)
>  {
>     HPETState *s = FROM_SYSBUS(HPETState, dev);
> @@ -625,6 +650,9 @@ static int hpet_init(SysBusDevice *dev)
>         timer->state = s;
>     }
>
> +    isa_reserve_irq(RTC_ISA_IRQ);
> +    qdev_init_gpio_in(&dev->qdev, hpet_handle_rtc_irq, 1);
> +
>     /* HPET Area */
>     iomemtype = cpu_register_io_memory(hpet_ram_read,
>                                        hpet_ram_write, s);
> diff --git a/hw/hpet_emul.h b/hw/hpet_emul.h
> index 785f850..9c268cc 100644
> --- a/hw/hpet_emul.h
> +++ b/hw/hpet_emul.h
> @@ -47,8 +47,4 @@
>  #define HPET_TN_INT_ROUTE_CAP_SHIFT 32
>  #define HPET_TN_CFG_BITS_READONLY_OR_RESERVED 0xffff80b1U
>
> -#if defined TARGET_I386
> -extern uint32_t hpet_in_legacy_mode(void);
> -#endif
> -
>  #endif
> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> index cbb98a4..ac82810 100644
> --- a/hw/mc146818rtc.c
> +++ b/hw/mc146818rtc.c
> @@ -26,7 +26,6 @@
>  #include "sysemu.h"
>  #include "pc.h"
>  #include "isa.h"
> -#include "hpet_emul.h"
>  #include "mc146818rtc.h"
>
>  //#define DEBUG_CMOS
> @@ -100,24 +99,6 @@ typedef struct RTCState {
>     QEMUTimer *second_timer2;
>  } RTCState;
>
> -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
> -     * mode is established while interrupt is raised. We want it to
> -     * be lowered in any case
> -     */
> -#if defined TARGET_I386
> -    if (hpet_in_legacy_mode()) {
> -        if (msg) {
> -            msg->delivery_cb(s->irq, s, -1, -1, QEMU_IRQ_MASKED);
> -        }
> -        return;
> -    }
> -#endif
> -    qemu_irq_raise_msg(s->irq, msg);
> -}
> -
>  static void rtc_set_time(RTCState *s);
>  static void rtc_copy_date(RTCState *s);
>
> @@ -169,7 +150,7 @@ static void rtc_coalesced_timer(void *opaque)
>     if (s->irq_coalesced != 0) {
>         s->cmos_data[RTC_REG_C] |= 0xc0;
>         DPRINTF_C("cmos: injecting from timer\n");
> -        rtc_irq_raise(s, &msg);
> +        qemu_irq_raise_msg(s->irq, &msg);
>     }
>
>     rtc_coalesced_timer_update(s);
> @@ -180,19 +161,10 @@ static void rtc_timer_update(RTCState *s, int64_t 
> current_time)
>  {
>     int period_code, period;
>     int64_t cur_clock, next_irq_clock;
> -    int enable_pie;
>
>     period_code = s->cmos_data[RTC_REG_A] & 0x0f;
> -#if defined TARGET_I386
> -    /* disable periodic timer if hpet is in legacy mode, since interrupts are
> -     * disabled anyway.
> -     */
> -    enable_pie = !hpet_in_legacy_mode();
> -#else
> -    enable_pie = 1;
> -#endif
>     if (period_code != 0
> -        && (((s->cmos_data[RTC_REG_B] & REG_B_PIE) && enable_pie)
> +        && ((s->cmos_data[RTC_REG_B] & REG_B_PIE)
>             || ((s->cmos_data[RTC_REG_B] & REG_B_SQWE) && s->sqw_irq))) {
>         if (period_code <= 2)
>             period_code += 7;
> @@ -236,10 +208,10 @@ static void rtc_periodic_timer(void *opaque)
>             if (s->irq_reinject_on_ack_count >= RTC_REINJECT_ON_ACK_COUNT) {
>                 s->irq_reinject_on_ack_count = 0;
>             }
> -            rtc_irq_raise(s, &msg);
> +            qemu_irq_raise_msg(s->irq, &msg);
>         } else
>  #endif
> -        rtc_irq_raise(s, NULL);
> +        qemu_irq_raise(s->irq);
>     }
>     if (s->cmos_data[RTC_REG_B] & REG_B_SQWE) {
>         /* Not square wave at all but we don't want 2048Hz interrupts!
> @@ -468,15 +440,15 @@ static void rtc_update_second2(void *opaque)
>              s->cmos_data[RTC_HOURS_ALARM] == s->current_tm.tm_hour)) {
>
>             s->cmos_data[RTC_REG_C] |= 0xa0;
> -            rtc_irq_raise(s, NULL);
> +            qemu_irq_raise(s->irq);
>         }
>     }
>
>     /* update ended interrupt */
>     s->cmos_data[RTC_REG_C] |= REG_C_UF;
>     if (s->cmos_data[RTC_REG_B] & REG_B_UIE) {
> -      s->cmos_data[RTC_REG_C] |= REG_C_IRQF;
> -      rtc_irq_raise(s, NULL);
> +        s->cmos_data[RTC_REG_C] |= REG_C_IRQF;
> +        qemu_irq_raise(s->irq);
>     }
>
>     /* clear update in progress bit */
> @@ -629,9 +601,6 @@ static int rtc_initfn(ISADevice *dev)
>  {
>     RTCState *s = DO_UPCAST(RTCState, dev, dev);
>     int base = 0x70;
> -    int isairq = 8;
> -
> -    isa_init_irq(dev, &s->irq, isairq);
>
>     s->cmos_data[RTC_REG_A] = 0x26;
>     s->cmos_data[RTC_REG_B] = 0x02;
> @@ -661,13 +630,20 @@ static int rtc_initfn(ISADevice *dev)
>     return 0;
>  }
>
> -ISADevice *rtc_init(int base_year)
> +ISADevice *rtc_init(int base_year, qemu_irq intercept_irq)
>  {
>     ISADevice *dev;
> +    RTCState *s;
>
>     dev = isa_create("mc146818rtc");
> +    s = DO_UPCAST(RTCState, dev, dev);
>     qdev_prop_set_int32(&dev->qdev, "base_year", base_year);
>     qdev_init_nofail(&dev->qdev);
> +    if (intercept_irq) {
> +        s->irq = intercept_irq;
> +    } else {
> +        isa_init_irq(dev, &s->irq, RTC_ISA_IRQ);
> +    }
>     return dev;
>  }
>
> diff --git a/hw/mc146818rtc.h b/hw/mc146818rtc.h
> index 6f46a68..575968c 100644
> --- a/hw/mc146818rtc.h
> +++ b/hw/mc146818rtc.h
> @@ -3,7 +3,9 @@
>
>  #include "isa.h"
>
> -ISADevice *rtc_init(int base_year);
> +#define RTC_ISA_IRQ 8
> +
> +ISADevice *rtc_init(int base_year, qemu_irq intercept_irq);
>  void rtc_set_memory(ISADevice *dev, int addr, int val);
>  void rtc_set_date(ISADevice *dev, const struct tm *tm);
>
> diff --git a/hw/mips_jazz.c b/hw/mips_jazz.c
> index da1bf6e..5e52f59 100644
> --- a/hw/mips_jazz.c
> +++ b/hw/mips_jazz.c
> @@ -259,7 +259,7 @@ void mips_jazz_init (ram_addr_t ram_size,
>     fdctrl_init_sysbus(rc4030[1], 0, 0x80003000, fds);
>
>     /* Real time clock */
> -    rtc_init(1980);
> +    rtc_init(1980, NULL);
>     s_rtc = cpu_register_io_memory(rtc_read, rtc_write, NULL);
>     cpu_register_physical_memory(0x80004000, 0x00001000, s_rtc);
>
> diff --git a/hw/mips_malta.c b/hw/mips_malta.c
> index bd86636..438e4e3 100644
> --- a/hw/mips_malta.c
> +++ b/hw/mips_malta.c
> @@ -959,7 +959,7 @@ void mips_malta_init (ram_addr_t ram_size,
>     /* Super I/O */
>     isa_dev = isa_create_simple("i8042");
>
> -    rtc_state = rtc_init(2000);
> +    rtc_state = rtc_init(2000, NULL);
>     serial_isa_init(0, serial_hds[0]);
>     serial_isa_init(1, serial_hds[1]);
>     if (parallel_hds[0])
> diff --git a/hw/mips_r4k.c b/hw/mips_r4k.c
> index f1fcfcd..5a96dea 100644
> --- a/hw/mips_r4k.c
> +++ b/hw/mips_r4k.c
> @@ -267,7 +267,7 @@ void mips_r4k_init (ram_addr_t ram_size,
>     isa_bus_new(NULL);
>     isa_bus_irqs(i8259);
>
> -    rtc_state = rtc_init(2000);
> +    rtc_state = rtc_init(2000, NULL);
>
>     /* Register 64 KB of ISA IO space at 0x14000000 */
>  #ifdef TARGET_WORDS_BIGENDIAN
> diff --git a/hw/pc.c b/hw/pc.c
> index 6129e59..8460303 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -965,6 +965,7 @@ void pc_basic_device_init(qemu_irq *isa_irq,
>     int i;
>     DriveInfo *fd[MAX_FD];
>     PITState *pit;
> +    qemu_irq rtc_irq = NULL;
>     qemu_irq *a20_line;
>     ISADevice *i8042;
>     qemu_irq *cpu_exit_irq;
> @@ -973,19 +974,20 @@ void pc_basic_device_init(qemu_irq *isa_irq,
>
>     register_ioport_write(0xf0, 1, 1, ioportF0_write, NULL);
>
> -    *rtc_state = rtc_init(2000);
> -
> -    qemu_register_boot_set(pc_boot_set, *rtc_state);
> -
> -    pit = pit_init(0x40, isa_reserve_irq(0));
> -    pcspk_init(pit);
>     if (!no_hpet) {
>         DeviceState *hpet = sysbus_create_simple("hpet", HPET_BASE, NULL);
>
>         for (i = 0; i < 24; i++) {
>             sysbus_connect_irq(sysbus_from_qdev(hpet), i, isa_irq[i]);
>         }
> +        rtc_irq = qdev_get_gpio_in(hpet, 0);
>     }
> +    *rtc_state = rtc_init(2000, rtc_irq);
> +
> +    qemu_register_boot_set(pc_boot_set, *rtc_state);
> +
> +    pit = pit_init(0x40, isa_reserve_irq(0));
> +    pcspk_init(pit);
>
>     for(i = 0; i < MAX_SERIAL_PORTS; i++) {
>         if (serial_hds[i]) {
> diff --git a/hw/ppc_prep.c b/hw/ppc_prep.c
> index fd1ca86..f44a144 100644
> --- a/hw/ppc_prep.c
> +++ b/hw/ppc_prep.c
> @@ -696,7 +696,7 @@ static void ppc_prep_init (ram_addr_t ram_size,
>     pci_vga_init(pci_bus, 0, 0);
>     //    openpic = openpic_init(0x00000000, 0xF0000000, 1);
>     //    pit = pit_init(0x40, i8259[0]);
> -    rtc_init(2000);
> +    rtc_init(2000, NULL);
>
>     if (serial_hds[0])
>         serial_isa_init(0, serial_hds[0]);
> --
> 1.6.0.2
>
>