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

[Jan Kiszka]

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

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

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

signature.asc
Description: OpenPGP digital signature