[Qemu-devel] Re: [PATCH, RFC 1/4] mc146818: move hpet handling to pc.c

[Jan Kiszka <jan.kiszka@web.de>]

[-- Attachment #1: Type: text/plain, Size: 7556 bytes --]

Blue Swirl wrote:
> On Sun, May 23, 2010 at 3:40 PM, Jan Kiszka <jan.kiszka@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 <blauwirbel@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
briefly thought about some user registration API for the RTC, but that
appears over-engineered on second thought. Let's go the simple path.

> 
>>> -    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
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.

Jan

[1] http://www.intel.com/hardwaredesign/hpetspec_1.pdf


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]