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

[Blue Swirl]

On Mon, May 24, 2010 at 3:30 PM, Jan Kiszka <address@hidden> wrote:
> Blue Swirl wrote:
>> On Sun, May 23, 2010 at 3:40 PM, Jan Kiszka <address@hidden> 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 <address@hidden>
>>>> ---
>>>>  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.