Patchwork [RFT,09/15] hpet/rtc: Rework RTC IRQ replacement by HPET

login
register
mail settings
Submitter Jan Kiszka
Date May 24, 2010, 8:13 p.m.
Message ID <b46a7c542c3b0b5e6f800d966b31f21672c6cae9.1274732025.git.jan.kiszka@web.de>
Download mbox | patch
Permalink /patch/53472/
State New
Headers show

Comments

Jan Kiszka - May 24, 2010, 8:13 p.m.
From: Jan Kiszka <jan.kiszka@siemens.com>

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 <jan.kiszka@siemens.com>
---
 hw/hpet.c        |   27 ++++++++++++++++++++-------
 hw/hpet_emul.h   |    4 +---
 hw/mc146818rtc.c |   52 ++++++++++++++++------------------------------------
 hw/mc146818rtc.h |    4 +++-
 hw/mips_jazz.c   |    2 +-
 hw/mips_malta.c  |    2 +-
 hw/mips_r4k.c    |    2 +-
 hw/pc.c          |   15 +++++++++------
 hw/ppc_prep.c    |    2 +-
 9 files changed, 53 insertions(+), 57 deletions(-)
Paul Brook - May 25, 2010, 9:29 a.m.
>          for (i = 0; i < 24; i++) {
>              sysbus_connect_irq(sysbus_from_qdev(hpet), i, isa_irq[i]);
>          }
> +        rtc_irq = qemu_allocate_feedback_irqs(hpet_handle_rtc_irq,
> +                                              sysbus_from_qdev(hpet), 1);
>      }

This is wrong. The hpet device should expose this as an IO pin.

Paul
Jan Kiszka - May 25, 2010, 10:23 a.m.
Paul Brook wrote:
>>          for (i = 0; i < 24; i++) {
>>              sysbus_connect_irq(sysbus_from_qdev(hpet), i, isa_irq[i]);
>>          }
>> +        rtc_irq = qemu_allocate_feedback_irqs(hpet_handle_rtc_irq,
>> +                                              sysbus_from_qdev(hpet), 1);
>>      }
> 
> This is wrong. The hpet device should expose this as an IO pin.

Will look into this.

BTW, I just realized that the GPIO handling is apparently lacking
support for attaching an output to multiple inputs. Or am I missing
something?

Jan
Paul Brook - May 25, 2010, 11:05 a.m.
> > This is wrong. The hpet device should expose this as an IO pin.
> 
> Will look into this.
> 
> BTW, I just realized that the GPIO handling is apparently lacking
> support for attaching an output to multiple inputs. Or am I missing
> something?

Use an explicit mux.

Incidentally I suspect your handling of the ISA IRQs is broken. You may never 
have more than one source connected to a sink.  Shared IRQ lines must be done 
explicitly.

Paul
Jan Kiszka - May 25, 2010, 11:19 a.m.
Paul Brook wrote:
>>> This is wrong. The hpet device should expose this as an IO pin.
>> Will look into this.
>>
>> BTW, I just realized that the GPIO handling is apparently lacking
>> support for attaching an output to multiple inputs. Or am I missing
>> something?
> 
> Use an explicit mux.
> 
> Incidentally I suspect your handling of the ISA IRQs is broken. You may never 
> have more than one source connected to a sink.  Shared IRQ lines must be done 
> explicitly.

No, the other way around: one source (RTC) multiple sinks (HPET, ACPI).
Will probably draft a generic irq/gpio mux.

Jan
Paul Brook - May 25, 2010, 11:23 a.m.
> Paul Brook wrote:
> >>> This is wrong. The hpet device should expose this as an IO pin.
> >> 
> >> Will look into this.
> >> 
> >> BTW, I just realized that the GPIO handling is apparently lacking
> >> support for attaching an output to multiple inputs. Or am I missing
> >> something?
> > 
> > Use an explicit mux.
> > 
> > Incidentally I suspect your handling of the ISA IRQs is broken. You may
> > never have more than one source connected to a sink.  Shared IRQ lines
> > must be done explicitly.
> 
> No, the other way around: one source (RTC) multiple sinks (HPET, ACPI).
> Will probably draft a generic irq/gpio mux.

I realise that. However I'd expect things to break if the guest OS devices to 
share an IRQ line between the HPET and some other device.

Paul
Jan Kiszka - May 25, 2010, 11:26 a.m.
Paul Brook wrote:
>> Paul Brook wrote:
>>>>> This is wrong. The hpet device should expose this as an IO pin.
>>>> Will look into this.
>>>>
>>>> BTW, I just realized that the GPIO handling is apparently lacking
>>>> support for attaching an output to multiple inputs. Or am I missing
>>>> something?
>>> Use an explicit mux.
>>>
>>> Incidentally I suspect your handling of the ISA IRQs is broken. You may
>>> never have more than one source connected to a sink.  Shared IRQ lines
>>> must be done explicitly.
>> No, the other way around: one source (RTC) multiple sinks (HPET, ACPI).
>> Will probably draft a generic irq/gpio mux.
> 
> I realise that. However I'd expect things to break if the guest OS devices to 
> share an IRQ line between the HPET and some other device.

The guest would share IRQ8, not the RTC output. So there would be no
difference to the current situation.

Jan
Paul Brook - May 25, 2010, 12:03 p.m.
> > I realise that. However I'd expect things to break if the guest OS
> > devices to share an IRQ line between the HPET and some other device.
> 
> The guest would share IRQ8, not the RTC output. So there would be no
> difference to the current situation.

The difference is that you've removed the check that prevented overlap between 
the PIC and annother device.  You should be using isa_reserve_irq/isa_init_irq 
before you use an ISA IRQ line.  Any uses of isa_bus_irqs (including teh 
existing HPET code) are probably broken.

Paul
Jan Kiszka - May 25, 2010, 12:39 p.m.
Paul Brook wrote:
>>> I realise that. However I'd expect things to break if the guest OS
>>> devices to share an IRQ line between the HPET and some other device.
>> The guest would share IRQ8, not the RTC output. So there would be no
>> difference to the current situation.
> 
> The difference is that you've removed the check that prevented overlap between 
> the PIC and annother device.  You should be using isa_reserve_irq/isa_init_irq 
> before you use an ISA IRQ line.  Any uses of isa_bus_irqs (including teh 
> existing HPET code) are probably broken.

...at least fragile. OK, will address this as well.

Thanks,
Jan

Patch

diff --git a/hw/hpet.c b/hw/hpet.c
index 041dd84..12d91e7 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:
@@ -632,6 +633,18 @@  static int hpet_init(SysBusDevice *dev)
     return 0;
 }
 
+int hpet_handle_rtc_irq(void *opaque, int n, int level)
+{
+    HPETState *s = FROM_SYSBUS(HPETState, opaque);
+
+    s->rtc_irq_level = level;
+    if (hpet_in_legacy_mode(s)) {
+        return QEMU_IRQ_MASKED;
+    } else {
+        return qemu_set_irq(s->irqs[RTC_ISA_IRQ], level);
+    }
+}
+
 static SysBusDeviceInfo hpet_device_info = {
     .qdev.name    = "hpet",
     .qdev.size    = sizeof(HPETState),
diff --git a/hw/hpet_emul.h b/hw/hpet_emul.h
index 785f850..0608e34 100644
--- a/hw/hpet_emul.h
+++ b/hw/hpet_emul.h
@@ -47,8 +47,6 @@ 
 #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
+int hpet_handle_rtc_irq(void *opaque, int n, int level);
 
 #endif
diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
index 697f723..a0f6951 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
@@ -93,21 +92,6 @@  typedef struct RTCState {
     QEMUTimer *second_timer2;
 } RTCState;
 
-static int rtc_irq_raise(qemu_irq irq)
-{
-    /* 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()) {
-        return QEMU_IRQ_MASKED;
-    }
-#endif
-    return qemu_irq_raise(irq);
-}
-
 static void rtc_set_time(RTCState *s);
 static void rtc_copy_date(RTCState *s);
 
@@ -131,7 +115,7 @@  static void rtc_coalesced_timer(void *opaque)
 
     if (s->irq_coalesced != 0) {
         s->cmos_data[RTC_REG_C] |= 0xc0;
-        if (rtc_irq_raise(s->irq) != QEMU_IRQ_COALESCED) {
+        if (qemu_irq_raise(s->irq) != QEMU_IRQ_COALESCED) {
             s->irq_coalesced--;
         }
     }
@@ -144,19 +128,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;
@@ -192,13 +167,13 @@  static void rtc_periodic_timer(void *opaque)
         if(rtc_td_hack) {
             if (s->irq_reinject_on_ack_count >= RTC_REINJECT_ON_ACK_COUNT)
                 s->irq_reinject_on_ack_count = 0;		
-            if (rtc_irq_raise(s->irq) == QEMU_IRQ_COALESCED) {
+            if (qemu_irq_raise(s->irq) == QEMU_IRQ_COALESCED) {
                 s->irq_coalesced++;
                 rtc_coalesced_timer_update(s);
             }
         } else
 #endif
-        rtc_irq_raise(s->irq);
+        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!
@@ -427,15 +402,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->irq);
+            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->irq);
+        s->cmos_data[RTC_REG_C] |= REG_C_IRQF;
+        qemu_irq_raise(s->irq);
     }
 
     /* clear update in progress bit */
@@ -584,9 +559,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;
@@ -616,13 +588,21 @@  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;
+        qemu_free(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..e656d1c 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 ead3a00..22db7a2 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 a8f9d15..23de7f0 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 ec6c32b..13bcecb 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -938,6 +938,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;
@@ -946,19 +947,21 @@  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 = qemu_allocate_feedback_irqs(hpet_handle_rtc_irq,
+                                              sysbus_from_qdev(hpet), 1);
     }
+    *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 16c9950..bb9e15f 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]);