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

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

[Blue Swirl]

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.
-     */
-    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);
+    }
+}
+
+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)
 {
-- 
1.6.2.4

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

[Jan Kiszka]

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

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

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


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

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

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

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

[Jan Kiszka]

[-- 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 --]

[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 <jan.kiszka@web.de> wrote:
> 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

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.

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

[Jan Kiszka]

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

Blue Swirl wrote:
> On Mon, May 24, 2010 at 3:30 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> 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
> 
> 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


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

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

[Gleb Natapov]

On Mon, May 24, 2010 at 10:20:31PM +0200, Jan Kiszka wrote:
> > Maybe the coalescing should be pushed to APIC, or even generalized.
> 
> The latter has happened, hopefully in the right direction.
> 
What do you mean by that?

--
			Gleb.

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

[Gleb Natapov]

On Mon, May 24, 2010 at 07:58:28PM +0000, Blue Swirl wrote:
> > 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.
At APIC level you miss some information. What if RTC frequency was
changed? What if it was reset?

--
			Gleb.

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

[Jan Kiszka]

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

Gleb Natapov wrote:
> On Mon, May 24, 2010 at 10:20:31PM +0200, Jan Kiszka wrote:
>>> Maybe the coalescing should be pushed to APIC, or even generalized.
>> The latter has happened, hopefully in the right direction.
>>
> What do you mean by that?

The latter: I tried to generalize. Please look at patch 7 & 8 of my HPET
series.

Jan


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

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

[Gleb Natapov]

On Tue, May 25, 2010 at 08:09:02AM +0200, Jan Kiszka wrote:
> Gleb Natapov wrote:
> > On Mon, May 24, 2010 at 10:20:31PM +0200, Jan Kiszka wrote:
> >>> Maybe the coalescing should be pushed to APIC, or even generalized.
> >> The latter has happened, hopefully in the right direction.
> >>
> > What do you mean by that?
> 
> The latter: I tried to generalize. Please look at patch 7 & 8 of my HPET
> series.
> 
Already did. I hope you will have more luck with this (obviously
correct) approach than I had two years ago.

--
			Gleb.