Re: [Qemu-devel] [PATCH v3 0/5] mc146818rtc: fix Windows VM clock faster

[Paolo Bonzini]

On 10/05/2017 10:32, address@hidden wrote:
> From: Xiao Guangrong <address@hidden>
> 
> Changelog in v3:
> Thanks to Paolo's the elaborate review comments, this version
> simplifies the logic of periodic_timer_update() significantly
> that includes:
> 1) introduce rtc_periodic_clock_ticks() that takes both regA and
>    regB into account and returns the period clock
> 2) count the clocks since last interrupt by always using current
>    clock subtracts the clock when last interrupt happened
> 3) improve the assert() logic
> 
> Paolo, all you comments have been reflected in this version except
> globally using s->period because in the current code, only x86 is
> using s->period so that we should obey this rule to keep compatible
> migration. I have added the comment to explain this suitable in the
> code:
> 
>             /*
>              * as the old QEMUs only used s->period for the case that
>              * LOST_TICK_POLICY_SLEW is used, in order to keep the
>              * compatible migration, we obey the rule as old QEMUs.
>              */
>             s->period = period;
> If anything i missed, please let me know. :)

Looks great.  Thanks for following up quickly on the reviews.

Paolo

> Changelog in v2:
> Thanks to Paolo's review, the changes in this version are:
> 1) merge patch 2, 3, 4 together
> 2) use a nice way ((value ^ new_value) & BIT_MASK) to check if updating
>    for periodic timer is needed
> 3) use a smarter way to calculate coalesced irqs and lost_clock
> 4) make sure only x86 can enable LOST_TICK_POLICY_SLEW
> 5) make the code depends on LOST_TICK_POLICY_SLEW rather than
>    TARGET_I386
> 
> We noticed that the clock on some windows VMs, e.g, Window7 and window8
> is really faster and the issue can be easily reproduced by staring the
> VM with '-rtc base=localtime,clock=vm,driftfix=slew -no-hpet' and 
> running attached code in the guest
> 
> The root cause is that the clock will be lost if the periodic period is
> changed as currently code counts the next periodic time like this:
>       next_irq_clock = (cur_clock & ~(period - 1)) + period;
> 
> consider the case if cur_clock = 0x11FF and period = 0x100, then the
> next_irq_clock is 0x1200, however, there is only 1 clock left to trigger
> the next irq. Unfortunately, Windows guests (at least Windows7) change
> the period very frequently if it runs the attached code, so that the
> lost clock is accumulated, the wall-time become faster and faster
> 
> The main idea to fix the issue is we use a accurate clock period to
> calculate the next irq:
>     next_irq_clock = cur_clock + period;
> 
> After that, it is really convenient to compensate clock if it is needed 
> 
> The code running in windows VM is attached:
> // TimeInternalTest.cpp : Defines the entry point for the console application.
> //
> 
> #include "stdafx.h"
> #pragma comment(lib, "winmm")
> #include <stdio.h>
> #include <windows.h>
> 
> #define SWITCH_PEROID  13
> 
> int _tmain(int argc, _TCHAR* argv[])
> {
>       if (argc != 2)
>       {
>               printf("parameter error!\n");
>               printf("USAGE: *.exe time(ms)\n");
>               printf("example: *.exe 40\n");
>               return 0;
>       }
>       else
>       {
>               DWORD internal = atoi((char *)argv[1]);
>               DWORD count = 0;
> 
>               while (1)
>               {
>                       count++;
>                       timeBeginPeriod(1);
>                       DWORD start = timeGetTime();
>                       Sleep(internal);
>                       timeEndPeriod(1);
>                       if ((count % SWITCH_PEROID) == 0) {
>                               Sleep(1);
>                       }
>               }
>       }
>       return 0;
> }
> 
> Tai Yunfang (1):
>   mc146818rtc: precisely count the clock for periodic timer
> 
> Xiao Guangrong (4):
>   mc146818rtc: update periodic timer only if it is needed
>   mc146818rtc: ensure LOST_TICK_POLICY_SLEW is only enabled on
>     TARGET_I386
>   mc146818rtc: drop unnecessary '#ifdef TARGET_I386'
>   mc146818rtc: embrace all x86 specific code
> 
>  hw/timer/mc146818rtc.c | 212 
> ++++++++++++++++++++++++++++++++++---------------
>  1 file changed, 149 insertions(+), 63 deletions(-)
>