Re: [PATCH 2/2] Remove -tdf

[Dor Laor <dor.laor@qumranet.com>]

Anthony Liguori wrote:
> The last time I posted the KVM patch series to qemu-devel, the -tdf patch met with
> some opposition.  Since today we implement timer catch-up in the in-kernel PIT and
> the in-kernel PIT is used by default, it doesn't seem all that valuable to have
> timer catch-up in userspace too.
>
> Removing it will reduce our divergence from QEMU.
>
>   
IMHO the in kernel PIT should go away, there is no reason to keep it 
except that userspace PIT drifts.
Currently both in-kernel PIT and even the in kernel irqchips are not 
100% bullet proof.
Of course this code is a hack, Gleb Natapov has send better fix for 
PIT/RTC to qemu list.
Can you look into them:
http://www.mail-archive.com/kvm@vger.kernel.org/msg01181.html

Thanks, Dor
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>
> diff --git a/qemu/hw/i8254.c b/qemu/hw/i8254.c
> index 69eb889..d0394c0 100644
> --- a/qemu/hw/i8254.c
> +++ b/qemu/hw/i8254.c
> @@ -332,11 +332,6 @@ static uint32_t pit_ioport_read(void *opaque, uint32_t addr)
>      return ret;
>  }
>  
> -/* global counters for time-drift fix */
> -int64_t timer_acks=0, timer_interrupts=0, timer_ints_to_push=0;
> -
> -extern int time_drift_fix;
> -
>  static void pit_irq_timer_update(PITChannelState *s, int64_t current_time)
>  {
>      int64_t expire_time;
> @@ -347,24 +342,6 @@ static void pit_irq_timer_update(PITChannelState *s, int64_t current_time)
>      expire_time = pit_get_next_transition_time(s, current_time);
>      irq_level = pit_get_out1(s, current_time);
>      qemu_set_irq(s->irq, irq_level);
> -    if (time_drift_fix && irq_level==1) {
> -        /* FIXME: fine tune timer_max_fix (max fix per tick). 
> -         *        Should it be 1 (double time), 2 , 4, 10 ? 
> -         *        Currently setting it to 5% of PIT-ticks-per-second (per PIT-tick)
> -         */
> -        const long pit_ticks_per_sec = (s->count>0) ? (PIT_FREQ/s->count) : 0;
> -        const long timer_max_fix = pit_ticks_per_sec/20;
> -        const long delta = timer_interrupts - timer_acks;
> -        const long max_delta = pit_ticks_per_sec * 60; /* one minute */
> -        if ((delta >  max_delta) && (pit_ticks_per_sec > 0)) {
> -            printf("time drift is too long, %ld seconds were lost\n", delta/pit_ticks_per_sec);
> -            timer_acks = timer_interrupts;
> -            timer_ints_to_push = 0;
> -        } else if (delta > 0) {
> -            timer_ints_to_push = MIN(delta, timer_max_fix);
> -        }
> -        timer_interrupts++;
> -    }
>  #ifdef DEBUG_PIT
>      printf("irq_level=%d next_delay=%f\n",
>             irq_level,
> diff --git a/qemu/hw/i8259.c b/qemu/hw/i8259.c
> index b266119..1707434 100644
> --- a/qemu/hw/i8259.c
> +++ b/qemu/hw/i8259.c
> @@ -221,35 +221,18 @@ static inline void pic_intack(PicState *s, int irq)
>      } else {
>          s->isr |= (1 << irq);
>      }
> -
>      /* We don't clear a level sensitive interrupt here */
>      if (!(s->elcr & (1 << irq)))
>          s->irr &= ~(1 << irq);
> -
>  }
>  
> -extern int time_drift_fix;
> -
>  int pic_read_irq(PicState2 *s)
>  {
>      int irq, irq2, intno;
>  
>      irq = pic_get_irq(&s->pics[0]);
>      if (irq >= 0) {
> -
>          pic_intack(&s->pics[0], irq);
> -#ifndef TARGET_IA64
> -	if (time_drift_fix && irq == 0) {
> -	    extern int64_t timer_acks, timer_ints_to_push;
> -	    timer_acks++;
> -	    if (timer_ints_to_push > 0) {
> -		timer_ints_to_push--;
> -                /* simulate an edge irq0, like the one generated by i8254 */
> -                pic_set_irq1(&s->pics[0], 0, 0);
> -                pic_set_irq1(&s->pics[0], 0, 1);
> -	    }
> -	}
> -#endif
>          if (irq == 2) {
>              irq2 = pic_get_irq(&s->pics[1]);
>              if (irq2 >= 0) {
> diff --git a/qemu/vl.c b/qemu/vl.c
> index 19c8bbf..d6877cd 100644
> --- a/qemu/vl.c
> +++ b/qemu/vl.c
> @@ -229,7 +229,6 @@ const char *option_rom[MAX_OPTION_ROMS];
>  int nb_option_roms;
>  int semihosting_enabled = 0;
>  int autostart = 1;
> -int time_drift_fix = 0;
>  unsigned int kvm_shadow_memory = 0;
>  const char *mem_path = NULL;
>  int hpagesize = 0;
> @@ -7968,7 +7967,6 @@ static void help(int exitcode)
>  #ifndef _WIN32
>  	   "-daemonize      daemonize QEMU after initializing\n"
>  #endif
> -           "-tdf            inject timer interrupts that got lost\n"
>             "-kvm-shadow-memory megs set the amount of shadow pages to be allocated\n"
>             "-mem-path       set the path to hugetlbfs/tmpfs mounted directory, also enables allocation of guest memory with huge pages\n"
>  	   "-option-rom rom load a file, rom, into the option ROM space\n"
> @@ -8089,7 +8087,6 @@ enum {
>      QEMU_OPTION_tb_size,
>      QEMU_OPTION_icount,
>      QEMU_OPTION_incoming,
> -    QEMU_OPTION_tdf,
>      QEMU_OPTION_kvm_shadow_memory,
>      QEMU_OPTION_mempath,
>  };
> @@ -8202,7 +8199,6 @@ const QEMUOption qemu_options[] = {
>  #if defined(TARGET_ARM) || defined(TARGET_M68K)
>      { "semihosting", 0, QEMU_OPTION_semihosting },
>  #endif
> -    { "tdf", 0, QEMU_OPTION_tdf }, /* enable time drift fix */
>      { "kvm-shadow-memory", HAS_ARG, QEMU_OPTION_kvm_shadow_memory },
>      { "name", HAS_ARG, QEMU_OPTION_name },
>  #if defined(TARGET_SPARC)
> @@ -9092,9 +9088,6 @@ int main(int argc, char **argv)
>              case QEMU_OPTION_semihosting:
>                  semihosting_enabled = 1;
>                  break;
> -            case QEMU_OPTION_tdf:
> -                time_drift_fix = 1;
> -		break;
>              case QEMU_OPTION_kvm_shadow_memory:
>                  kvm_shadow_memory = (int64_t)atoi(optarg) * 1024 * 1024 / 4096;
>                  break;
>