On Mon, May 24, 2010 at 3:30 PM, Jan Kiszka <jan.kis...@web.de> wrote:
> Blue Swirl wrote:
>> 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.
>
> I think now it is OK to leave it ticking.
>
> We are currently lacking proper RTC routing through ACPI to SCI [1,
> Figure 11]. Adding this will add a parallel user of the RTC IRQ line. I
What a poor picture BTW, even the arrow heads are missing. Would you
have a pointer for the SCI specs?
> briefly thought about some user registration API for the RTC, but that
> appears over-engineered on second thought. Let's go the simple path.
I think it's easier to add some logic to HPET to route the
RTC/HPET/i8254 irqs. If there is no HPET, the irqs are routed
directly.
>>
>>>> - 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.
>
> I think both new and old code is wrong. I'm reworking this ATM.
> Actually, the hpet handling belongs to, well, the hpet. That avoids
I see, that would make things simpler.
> having to deal with all those details via complex APIs. But the
> coalescing mess is still causing headaches to me, at least when trying
> to come up with something long-term ready.
Maybe the coalescing should be pushed to APIC, or even generalized.
