Re: [Qemu-devel] [PATCH][RESEND] Time drift again.

[Anthony Liguori <anthony@codemonkey.ws>]

Gleb Natapov wrote:
> Hello,
>
>  After my last patch to fix interrupt coalescing was rejected
> on the basis that it is too intrusive we decided to make the
> fix much more localized and only fix the problem for RTC time
> source. Unfortunately it is impossible to fix the problem entirely
> inside RTC code like Andrzej proposed since Windows reads RTC
> register C more then once on each time interrupt so it is impossible
> to count reliably how many interrupt windows actually handled.
> Proposed solution is localized to I386 target and is disabled by
> default. To enable it "-rtc-td-hack" flag should be used.
>
> Signed-off-by: Gleb Natapov <gleb@redhat.com>
>   

Applied.  Thanks for all your work on this.  I know this has gone 
through a lot of iterations to come up with something that made everyone 
happy and the persistence is appreciated :-)

Regards,

Anthony Liguori

> diff --git a/hw/apic.c b/hw/apic.c
> index bad137f..3052843 100644
> --- a/hw/apic.c
> +++ b/hw/apic.c
> @@ -100,6 +100,8 @@ struct IOAPICState {
>  static int apic_io_memory;
>  static APICState *local_apics[MAX_APICS + 1];
>  static int last_apic_id = 0;
> +static int apic_irq_delivered;
> +
>  
>  static void apic_init_ipi(APICState *s);
>  static void apic_set_irq(APICState *s, int vector_num, int trigger_mode);
> @@ -133,6 +135,14 @@ static inline void reset_bit(uint32_t *tab, int index)
>      tab[i] &= ~mask;
>  }
>  
> +static inline int get_bit(uint32_t *tab, int index)
> +{
> +    int i, mask;
> +    i = index >> 5;
> +    mask = 1 << (index & 0x1f);
> +    return !!(tab[i] & mask);
> +}
> +
>  static void apic_local_deliver(CPUState *env, int vector)
>  {
>      APICState *s = env->apic_state;
> @@ -349,8 +359,20 @@ static void apic_update_irq(APICState *s)
>      cpu_interrupt(s->cpu_env, CPU_INTERRUPT_HARD);
>  }
>  
> +void apic_reset_irq_delivered(void)
> +{
> +    apic_irq_delivered = 0;
> +}
> +
> +int apic_get_irq_delivered(void)
> +{
> +    return apic_irq_delivered;
> +}
> +
>  static void apic_set_irq(APICState *s, int vector_num, int trigger_mode)
>  {
> +    apic_irq_delivered += !get_bit(s->irr, vector_num);
> +
>      set_bit(s->irr, vector_num);
>      if (trigger_mode)
>          set_bit(s->tmr, vector_num);
> diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c
> index faf847d..1c71bda 100644
> --- a/hw/mc146818rtc.c
> +++ b/hw/mc146818rtc.c
> @@ -66,6 +66,10 @@ struct RTCState {
>      int64_t next_periodic_time;
>      /* second update */
>      int64_t next_second_time;
> +#ifdef TARGET_I386
> +    uint32_t irq_coalesced;
> +    uint32_t period;
> +#endif
>      QEMUTimer *second_timer;
>      QEMUTimer *second_timer2;
>  };
> @@ -103,12 +107,20 @@ static void rtc_timer_update(RTCState *s, int64_t current_time)
>              period_code += 7;
>          /* period in 32 Khz cycles */
>          period = 1 << (period_code - 1);
> +#ifdef TARGET_I386
> +        if(period != s->period)
> +            s->irq_coalesced = (s->irq_coalesced * s->period) / period;
> +        s->period = period;
> +#endif
>          /* compute 32 khz clock */
>          cur_clock = muldiv64(current_time, 32768, ticks_per_sec);
>          next_irq_clock = (cur_clock & ~(period - 1)) + period;
>          s->next_periodic_time = muldiv64(next_irq_clock, ticks_per_sec, 32768) + 1;
>          qemu_mod_timer(s->periodic_timer, s->next_periodic_time);
>      } else {
> +#ifdef TARGET_I386
> +        s->irq_coalesced = 0;
> +#endif
>          qemu_del_timer(s->periodic_timer);
>      }
>  }
> @@ -118,6 +130,12 @@ static void rtc_periodic_timer(void *opaque)
>      RTCState *s = opaque;
>  
>      rtc_timer_update(s, s->next_periodic_time);
> +#ifdef TARGET_I386
> +    if ((s->cmos_data[RTC_REG_C] & 0xc0) && rtc_td_hack) {
> +        s->irq_coalesced++;
> +        return;
> +    }
> +#endif
>      s->cmos_data[RTC_REG_C] |= 0xc0;
>      rtc_irq_raise(s->irq);
>  }
> @@ -378,6 +396,15 @@ static uint32_t cmos_ioport_read(void *opaque, uint32_t addr)
>          case RTC_REG_C:
>              ret = s->cmos_data[s->cmos_index];
>              qemu_irq_lower(s->irq);
> +#ifdef TARGET_I386
> +            if(s->irq_coalesced) {
> +                apic_reset_irq_delivered();
> +                qemu_irq_raise(s->irq);
> +                if (apic_get_irq_delivered())
> +                    s->irq_coalesced--;
> +                break;
> +            }
> +#endif
>              s->cmos_data[RTC_REG_C] = 0x00;
>              break;
>          default:
> @@ -472,6 +499,28 @@ static int rtc_load(QEMUFile *f, void *opaque, int version_id)
>      return 0;
>  }
>  
> +#ifdef TARGET_I386
> +static void rtc_save_td(QEMUFile *f, void *opaque)
> +{
> +    RTCState *s = opaque;
> +
> +    qemu_put_be32(f, s->irq_coalesced);
> +    qemu_put_be32(f, s->period);
> +}
> +
> +static int rtc_load_td(QEMUFile *f, void *opaque, int version_id)
> +{
> +    RTCState *s = opaque;
> +
> +    if (version_id != 1)
> +        return -EINVAL;
> +
> +    s->irq_coalesced = qemu_get_be32(f);
> +    s->period = qemu_get_be32(f);
> +    return 0;
> +}
> +#endif
> +
>  RTCState *rtc_init(int base, qemu_irq irq)
>  {
>      RTCState *s;
> @@ -502,6 +551,10 @@ RTCState *rtc_init(int base, qemu_irq irq)
>      register_ioport_read(base, 2, 1, cmos_ioport_read, s);
>  
>      register_savevm("mc146818rtc", base, 1, rtc_save, rtc_load, s);
> +#ifdef TARGET_I386
> +    if (rtc_td_hack)
> +        register_savevm("mc146818rtc-td", base, 1, rtc_save_td, rtc_load_td, s);
> +#endif
>      return s;
>  }
>  
> @@ -608,5 +661,9 @@ RTCState *rtc_mm_init(target_phys_addr_t base, int it_shift, qemu_irq irq)
>      cpu_register_physical_memory(base, 2 << it_shift, io_memory);
>  
>      register_savevm("mc146818rtc", base, 1, rtc_save, rtc_load, s);
> +#ifdef TARGET_I386
> +    if (rtc_td_hack)
> +        register_savevm("mc146818rtc-td", base, 1, rtc_save_td, rtc_load_td, s);
> +#endif
>      return s;
>  }
> diff --git a/hw/pc.h b/hw/pc.h
> index a60bddf..5dc7709 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -46,6 +46,8 @@ void apic_deliver_pic_intr(CPUState *env, int level);
>  int apic_get_interrupt(CPUState *env);
>  IOAPICState *ioapic_init(void);
>  void ioapic_set_irq(void *opaque, int vector, int level);
> +void apic_reset_irq_delivered(void);
> +int apic_get_irq_delivered(void);
>  
>  /* i8254.c */
>  
> diff --git a/qemu-doc.texi b/qemu-doc.texi
> index 5a12dc9..f61ae34 100644
> --- a/qemu-doc.texi
> +++ b/qemu-doc.texi
> @@ -420,6 +420,11 @@ Use it when installing Windows 2000 to avoid a disk full bug. After
>  Windows 2000 is installed, you no longer need this option (this option
>  slows down the IDE transfers).
>  
> +@item -rtc-td-hack
> +Use it if you experience time drift problem in Windows with ACPI HAL.
> +This option will try to figure out how many timer interrupts were not
> +processed by the Windows guest and will re-inject them.
> +
>  @item -option-rom @var{file}
>  Load the contents of @var{file} as an option ROM.
>  This option is useful to load things like EtherBoot.
> diff --git a/sysemu.h b/sysemu.h
> index 47c1fea..a9e156c 100644
> --- a/sysemu.h
> +++ b/sysemu.h
> @@ -90,6 +90,7 @@ extern int graphic_depth;
>  extern int nographic;
>  extern const char *keyboard_layout;
>  extern int win2k_install_hack;
> +extern int rtc_td_hack;
>  extern int alt_grab;
>  extern int usb_enabled;
>  extern int smp_cpus;
> diff --git a/vl.c b/vl.c
> index e29072b..244d1c3 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -211,6 +211,7 @@ CharDriverState *serial_hds[MAX_SERIAL_PORTS];
>  CharDriverState *parallel_hds[MAX_PARALLEL_PORTS];
>  #ifdef TARGET_I386
>  int win2k_install_hack = 0;
> +int rtc_td_hack = 0;
>  #endif
>  int usb_enabled = 0;
>  int smp_cpus = 1;
> @@ -3877,6 +3878,7 @@ static void help(int exitcode)
>             "-full-screen    start in full screen\n"
>  #ifdef TARGET_I386
>             "-win2k-hack     use it when installing Windows 2000 to avoid a disk full bug\n"
> +           "-rtc-td-hack    use it to fix time drift in Windows ACPI HAL\n"
>  #endif
>             "-usb            enable the USB driver (will be the default soon)\n"
>             "-usbdevice name add the host or guest USB device 'name'\n"
> @@ -4072,6 +4074,7 @@ enum {
>      QEMU_OPTION_kernel_kqemu,
>      QEMU_OPTION_enable_kvm,
>      QEMU_OPTION_win2k_hack,
> +    QEMU_OPTION_rtc_td_hack,
>      QEMU_OPTION_usb,
>      QEMU_OPTION_usbdevice,
>      QEMU_OPTION_smp,
> @@ -4180,6 +4183,7 @@ static const QEMUOption qemu_options[] = {
>  #endif
>      { "pidfile", HAS_ARG, QEMU_OPTION_pidfile },
>      { "win2k-hack", 0, QEMU_OPTION_win2k_hack },
> +    { "rtc-td-hack", 0, QEMU_OPTION_rtc_td_hack },
>      { "usbdevice", HAS_ARG, QEMU_OPTION_usbdevice },
>      { "smp", HAS_ARG, QEMU_OPTION_smp },
>      { "vnc", HAS_ARG, QEMU_OPTION_vnc },
> @@ -4993,6 +4997,9 @@ int main(int argc, char **argv, char **envp)
>              case QEMU_OPTION_win2k_hack:
>                  win2k_install_hack = 1;
>                  break;
> +            case QEMU_OPTION_rtc_td_hack:
> +                rtc_td_hack = 1;
> +                break;
>  #endif
>  #ifdef USE_KQEMU
>              case QEMU_OPTION_no_kqemu:
> --
> 			Gleb.
>
>
>