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

[Jan Kiszka]

Blue Swirl wrote:
> 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?

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