Re: [PATCH v2] mc146818rtc: fix timer interrupt reinjection

[Alex Williamson <alex.williamson@redhat.com>]

On Thu, 10 Oct 2019 09:30:08 -0300
Marcelo Tosatti <mtosatti@redhat.com> wrote:

> commit 369b41359af46bded5799c9ef8be2b641d92e043 broke timer interrupt
> reinjection when there is no period change by the guest.
> 
> In that case, old_period is 0, which ends up zeroing irq_coalesced
> (counter of reinjected interrupts).
> 
> The consequence is Windows 7 is unable to synchronize time via NTP.
> Easily reproducible by playing a fullscreen video with cirrus and VNC.
> 
> Fix by not updating s->irq_coalesced when old_period is 0.
> 
> V2: reorganize code (Paolo Bonzini)
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

This causes a regression for me, my win10 VM with assigned GPU
experiences hangs and slowness with this.  Found via bisect, reverting
restores normal behavior.  libvirt uses this commandline:

/usr/local/bin/qemu-system-x86_64 \
-name guest=Steam-GeForce,debug-threads=on \
-S \
-object secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-1-Steam-GeForce/master-key.aes \
-machine pc-i440fx-4.1,accel=kvm,usb=off,vmport=off,dump-guest-core=off \
-cpu host,hv-time,hv-relaxed,hv-vapic,hv-spinlocks=0x1fff,hv-vendor-id=KeenlyKVM,kvm=off \
-drive file=/usr/share/edk2/ovmf/OVMF_CODE.fd,if=pflash,format=raw,unit=0,readonly=on \
-drive file=/var/lib/libvirt/qemu/nvram/Steam_VARS.fd,if=pflash,format=raw,unit=1 \
-m 4096 \
-mem-prealloc \
-mem-path /dev/hugepages/libvirt/qemu/1-Steam-GeForce \
-overcommit mem-lock=off \
-smp 4,sockets=1,cores=2,threads=2 \
-uuid 2b417d4b-f25b-4522-a5be-e105f032f99c \
-display none \
-no-user-config \
-nodefaults \
-chardev socket,id=charmonitor,fd=38,server,nowait \
-mon chardev=charmonitor,id=monitor,mode=control \
-rtc base=localtime,driftfix=slew \
-global kvm-pit.lost_tick_policy=delay \
-no-hpet \
-no-shutdown \
-boot menu=on,strict=on \
-device nec-usb-xhci,id=usb,bus=pci.0,addr=0x8 \
-device virtio-scsi-pci,id=scsi0,num_queues=4,bus=pci.0,addr=0x5 \
-drive file=/mnt/ssd/Steam.qcow2,format=qcow2,if=none,id=drive-scsi0-0-0-0,cache=none \
-device scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,device_id=drive-scsi0-0-0-0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=2,write-cache=on \
-netdev tap,fd=40,id=hostnet0,vhost=on,vhostfd=41 \
-device virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:60:ef:ac,bus=pci.0,addr=0x3 \
-device vfio-pci,host=0000:01:00.0,id=hostdev0,bus=pci.0,addr=0x4,rombar=1 \
-device vfio-pci,host=0000:01:00.1,id=hostdev1,bus=pci.0,addr=0x6,rombar=0 \
-S \
-debugcon file:/tmp/Steam-ovmf-debug.log \
-global isa-debugcon.iobase=0x402 \
-set device.hostdev0.x-pci-vendor-id=0x10de \
-trace events=/var/lib/libvirt/images/Steam-GeForce.events \
-sandbox on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny \
-msg timestamp=on

Thanks,
Alex

> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index 6cb378751b..0e7cf97042 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -203,24 +203,28 @@ periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period)
>  
>      period = rtc_periodic_clock_ticks(s);
>  
> -    if (period) {
> -        /* compute 32 khz clock */
> -        cur_clock =
> -            muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
> +    if (!period) {
> +        s->irq_coalesced = 0;
> +        timer_del(s->periodic_timer);
> +        return;
> +    }
>  
> -        /*
> -        * if the periodic timer's update is due to period re-configuration,
> -        * we should count the clock since last interrupt.
> -        */
> -        if (old_period) {
> -            int64_t last_periodic_clock, next_periodic_clock;
> -
> -            next_periodic_clock = muldiv64(s->next_periodic_time,
> -                                    RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
> -            last_periodic_clock = next_periodic_clock - old_period;
> -            lost_clock = cur_clock - last_periodic_clock;
> -            assert(lost_clock >= 0);
> -        }
> +    /* compute 32 khz clock */
> +    cur_clock =
> +        muldiv64(current_time, RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
> +
> +    /*
> +     * if the periodic timer's update is due to period re-configuration,
> +     * we should count the clock since last interrupt.
> +     */
> +    if (old_period) {
> +        int64_t last_periodic_clock, next_periodic_clock;
> +
> +        next_periodic_clock = muldiv64(s->next_periodic_time,
> +                                RTC_CLOCK_RATE, NANOSECONDS_PER_SECOND);
> +        last_periodic_clock = next_periodic_clock - old_period;
> +        lost_clock = cur_clock - last_periodic_clock;
> +        assert(lost_clock >= 0);
>  
>          /*
>           * s->irq_coalesced can change for two reasons:
> @@ -251,22 +255,19 @@ periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period)
>                  rtc_coalesced_timer_update(s);
>              }
>          } else {
> -           /*
> +            /*
>               * no way to compensate the interrupt if LOST_TICK_POLICY_SLEW
>               * is not used, we should make the time progress anyway.
>               */
>              lost_clock = MIN(lost_clock, period);
>          }
> +    }
>  
> -        assert(lost_clock >= 0 && lost_clock <= period);
> +    assert(lost_clock >= 0 && lost_clock <= period);
>  
> -        next_irq_clock = cur_clock + period - lost_clock;
> -        s->next_periodic_time = periodic_clock_to_ns(next_irq_clock) + 1;
> -        timer_mod(s->periodic_timer, s->next_periodic_time);
> -    } else {
> -        s->irq_coalesced = 0;
> -        timer_del(s->periodic_timer);
> -    }
> +    next_irq_clock = cur_clock + period - lost_clock;
> +    s->next_periodic_time = periodic_clock_to_ns(next_irq_clock) + 1;
> +    timer_mod(s->periodic_timer, s->next_periodic_time);
>  }
>  
>  static void rtc_periodic_timer(void *opaque)
> 
>