Blue Swirl wrote:
> 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?
Nope. But my current understanding is that those gates map to
ACPI_BITMASK_RT_CLOCK_STATUS and ACPI_BITMASK_RT_CLOCK_ENABLE in our
acpi code, and SCI handling is already there. Just takes inject the RTC
IRQs when all bits are set.
>
>> 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.
That's what I did, at least for the RTC. Haven't looked at the
i8254-vs.-HPET bits yet as I wanted to send out the series. :)
>
>>>>> - 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.
The latter has happened, hopefully in the right direction.
Jan
signature.asc
Description: OpenPGP digital signature
