On Sun, May 23, 2010 at 3:40 PM, Jan Kiszka <jan.kis...@web.de> wrote: > Blue Swirl wrote: >> Move hpet_in_legacy_mode check from mc146818.c to pc.c. Remove >> the optimization where the periodic timer is disabled if >> hpet is in legacy mode. >> >> Signed-off-by: Blue Swirl <blauwir...@gmail.com> >> --- >> hw/mc146818rtc.c | 37 +++++++------------------------------ >> hw/mc146818rtc.h | 2 ++ >> hw/pc.c | 32 +++++++++++++++++++++++++++----- >> 3 files changed, 36 insertions(+), 35 deletions(-) >> >> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c >> index 571c593..e0c33c5 100644 >> --- a/hw/mc146818rtc.c >> +++ b/hw/mc146818rtc.c >> @@ -27,7 +27,6 @@ >> #include "pc.h" >> #include "apic.h" >> #include "isa.h" >> -#include "hpet_emul.h" >> #include "mc146818rtc.h" >> >> //#define DEBUG_CMOS >> @@ -94,19 +93,6 @@ typedef struct RTCState { >> QEMUTimer *second_timer2; >> } RTCState; >> >> -static void 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()) >> -#endif >> - qemu_irq_raise(irq); >> -} >> - >> static void rtc_set_time(RTCState *s); >> static void rtc_copy_date(RTCState *s); >> >> @@ -131,7 +117,7 @@ static void rtc_coalesced_timer(void *opaque) >> if (s->irq_coalesced != 0) { >> apic_reset_irq_delivered(); >> s->cmos_data[RTC_REG_C] |= 0xc0; >> - rtc_irq_raise(s->irq); >> + qemu_irq_raise(s->irq); >> if (apic_get_irq_delivered()) { >> s->irq_coalesced--; >> } >> @@ -145,19 +131,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. >> - */ > > Does some dumb OS we care about (specifically in KVM mode) first enable > the periodic RTC, then discovers the HPET, switches over, forgetting > about the RTC? Otherwise: the guest will get what it deserves (degraded > performance).
No idea. The performance penalty also depends on the trigger frequency. >> - 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; >> @@ -194,14 +171,14 @@ 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; >> apic_reset_irq_delivered(); >> - rtc_irq_raise(s->irq); >> + qemu_irq_raise(s->irq); >> if (!apic_get_irq_delivered()) { >> 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! >> @@ -430,7 +407,7 @@ 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); >> } >> } >> >> @@ -438,7 +415,7 @@ static void rtc_update_second2(void *opaque) >> 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); >> + qemu_irq_raise(s->irq); >> } >> >> /* clear update in progress bit */ >> @@ -588,7 +565,7 @@ static int rtc_initfn(ISADevice *dev) >> { >> RTCState *s = DO_UPCAST(RTCState, dev, dev); >> int base = 0x70; >> - int isairq = 8; >> + int isairq = RTC_ISA_IRQ; >> >> isa_init_irq(dev, &s->irq, isairq); >> >> diff --git a/hw/mc146818rtc.h b/hw/mc146818rtc.h >> index 6f46a68..d630485 100644 >> --- a/hw/mc146818rtc.h >> +++ b/hw/mc146818rtc.h >> @@ -7,4 +7,6 @@ ISADevice *rtc_init(int base_year); >> void rtc_set_memory(ISADevice *dev, int addr, int val); >> void rtc_set_date(ISADevice *dev, const struct tm *tm); >> >> +#define RTC_ISA_IRQ 8 >> + >> #endif /* !MC146818RTC_H */ >> diff --git a/hw/pc.c b/hw/pc.c >> index e7f31d3..5a703e1 100644 >> --- a/hw/pc.c >> +++ b/hw/pc.c >> @@ -66,16 +66,38 @@ struct e820_table { >> >> static struct e820_table e820_table; >> >> -void isa_irq_handler(void *opaque, int n, int level) >> +static void isa_set_irq(IsaIrqState *isa, int n, int level) >> { >> - IsaIrqState *isa = (IsaIrqState *)opaque; >> - >> if (n < 16) { >> qemu_set_irq(isa->i8259[n], level); >> } >> - if (isa->ioapic) >> + if (isa->ioapic) { >> qemu_set_irq(isa->ioapic[n], level); >> -}; >> + } >> +} >> + >> +static void rtc_irq_handler(IsaIrqState *isa, int level) >> +{ >> + /* 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 (!hpet_in_legacy_mode() || !level) { >> + isa_set_irq(isa, RTC_ISA_IRQ, level); >> + } >> +} > > If you clear the RTC IRQ unconditionally, I could imagine that the > enable_pie removal is more than a de-optimization. At least in some > corner cases. But this clearing looks suspicious anyway. Instead, when > switching, the new IRQ line owner should set the level - once. This was how the original version worked, the check with hpet_in_legacy_mode() was only used for qemu_irq_raise(), not qemu_irq_lower(). The new handler will be called for both cases, so the check must consider also the level. >> + >> +void isa_irq_handler(void *opaque, int n, int level) >> +{ >> + IsaIrqState *isa = (IsaIrqState *)opaque; >> + >> + if (n == RTC_ISA_IRQ) { >> + rtc_irq_handler(isa, level); >> + } else { >> + isa_set_irq(isa, n, level); >> + } >> +} >> >> static void ioport80_write(void *opaque, uint32_t addr, uint32_t data) >> { > > Jan > >