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

[Blue Swirl]

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.

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

>> +
>> +void isa_irq_handler(void *opaque, int n, int level)
>> +{
>> +    IsaIrqState *isa = (IsaIrqState *)opaque;
>> +
>> +    if (n == RTC_ISA_IRQ) {
>> +        rtc_irq_handler(isa, level);
>> +    } else {
>> +        isa_set_irq(isa, n, level);
>> +    }
>> +}
>>
>>  static void ioport80_write(void *opaque, uint32_t addr, uint32_t data)
>>  {
>
> Jan
>
>