| Message ID | 20191010123008.GA19158@amt.cnet |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] mc146818rtc: fix timer interrupt reinjection | expand |
On 10/10/19 14:30, Marcelo Tosatti 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> > > 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) > Queued, thanks. Paolo
On 10/10/19 2:30 PM, Marcelo Tosatti 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) ^ This line shouldn't be part of git history, Paolo if possible can you drop it? > > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> > > 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; > + } This is the first change, simplify the if statement with a 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); > } > + } This is the second change, changing the logic and fixing the bug. > > - 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); This is part of the 1st change. I'd rather see this patch split in 2 logical changes... > } > > static void rtc_periodic_timer(void *opaque) > >
On 10/24/19 1:22 PM, Philippe Mathieu-Daudé wrote: > On 10/10/19 2:30 PM, Marcelo Tosatti 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) > > ^ This line shouldn't be part of git history, > Paolo if possible can you drop it? > >> >> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> >> >> 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; >> + } > > This is the first change, simplify the if statement with a 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); >> } >> + } > > This is the second change, changing the logic and fixing the bug. > >> - 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); > > This is part of the 1st change. > > I'd rather see this patch split in 2 logical changes... Since this split makes sense to me (easier to understand the patch diff when reviewing), I can quickly do it if you don't mind. >> } >> static void rtc_periodic_timer(void *opaque) >> >>
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) > >
On Sat, Nov 16, 2019 at 01:58:55PM -0700, Alex Williamson wrote: > 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 Alex, -rtc base=localtime,driftfix=none should fix it. Can you confirm?
On Sun, 17 Nov 2019 01:20:19 -0200 Marcelo Tosatti <mtosatti@redhat.com> wrote: > On Sat, Nov 16, 2019 at 01:58:55PM -0700, Alex Williamson wrote: > > 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 > > Alex, > > -rtc base=localtime,driftfix=none should fix it. Can you confirm? How do I translate that to xml? I'm currently using: <clock offset='localtime'> <timer name='rtc' tickpolicy='catchup'/> <timer name='pit' tickpolicy='delay'/> <timer name='hpet' present='no'/> <timer name='hypervclock' present='yes'/> </clock> According to this[1] bz, 'catchup' translates to 'slew' and seems to be the default policy in use. The 'discard' option seemed the most likely, but my VM fails to start with that: error: unsupported configuration: unsupported rtc timer tickpolicy 'discard' The 'merge' option gives me a similar error. The 'delay' option is the only other choice where I can actually start the VM, but this results in the commandline: -rtc base=localtime (no driftfix specified) This does appear to resolve the issue, but of course compatibility with existing configurations has regressed. Thanks, Alex [1] https://bugzilla.redhat.com/show_bug.cgi?id=865315
On 17/11/19 05:31, Alex Williamson wrote: > The 'merge' option gives me a similar error. The 'delay' option is > the only other choice where I can actually start the VM, but this > results in the commandline: > > -rtc base=localtime > > (no driftfix specified) none is the default, so that's okay. > This does appear to resolve the issue, but of course compatibility > with existing configurations has regressed. Thanks, Yeah, I guess this was just a suggestion to double-check the cause of the regression. The problem could be that periodic_timer_update is using old_period == 0 for two cases: no period change, and old period was 0 (periodic timer off). Something like the following distinguishes the two cases by always using s->period (currently it was only used for driftfix=slew) and passing s->period instead of 0 when there is no period change. More cleanups are possible, but this is the smallest patch that implements the idea. The first patch is big but, indentation changes aside, it's moving a single closed brace. Alex/Marcelo, can you check if it fixes both of your test cases? ------------- 8< --------------- From 48dc9d140c636067b8de1ab8e25b819151c83162 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini <pbonzini@redhat.com> Date: Sun, 17 Nov 2019 10:07:38 +0100 Subject: [PATCH 1/2] Revert "mc146818rtc: fix timer interrupt reinjection" This reverts commit b429de730174b388ea5760e3debb0d542ea3c261, except that the reversal of the outer "if (period)" is left in. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/rtc/mc146818rtc.c | 67 ++++++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 34 deletions(-) diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c index ee6bf82b40..9869dc5031 100644 --- a/hw/rtc/mc146818rtc.c +++ b/hw/rtc/mc146818rtc.c @@ -174,7 +174,6 @@ periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period) int64_t cur_clock, next_irq_clock, lost_clock = 0; period = rtc_periodic_clock_ticks(s); - if (!period) { s->irq_coalesced = 0; timer_del(s->periodic_timer); @@ -197,42 +196,42 @@ periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period) 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: + * + * a) if one or more periodic timer interrupts have been lost, + * lost_clock will be more that a period. + * + * b) when the period may be reconfigured, we expect the OS to + * treat delayed tick as the new period. So, when switching + * from a shorter to a longer period, scale down the missing, + * because the OS will treat past delayed ticks as longer + * (leftovers are put back into lost_clock). When switching + * to a shorter period, scale up the missing ticks since the + * OS handler will treat past delayed ticks as shorter. + */ + if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) { + uint32_t old_irq_coalesced = s->irq_coalesced; + + s->period = period; + lost_clock += old_irq_coalesced * old_period; + s->irq_coalesced = lost_clock / s->period; + lost_clock %= s->period; + if (old_irq_coalesced != s->irq_coalesced || + old_period != s->period) { + DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, " + "period scaled from %d to %d\n", old_irq_coalesced, + s->irq_coalesced, old_period, s->period); + rtc_coalesced_timer_update(s); + } + } else { /* - * s->irq_coalesced can change for two reasons: - * - * a) if one or more periodic timer interrupts have been lost, - * lost_clock will be more that a period. - * - * b) when the period may be reconfigured, we expect the OS to - * treat delayed tick as the new period. So, when switching - * from a shorter to a longer period, scale down the missing, - * because the OS will treat past delayed ticks as longer - * (leftovers are put back into lost_clock). When switching - * to a shorter period, scale up the missing ticks since the - * OS handler will treat past delayed ticks as shorter. + * no way to compensate the interrupt if LOST_TICK_POLICY_SLEW + * is not used, we should make the time progress anyway. */ - if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) { - uint32_t old_irq_coalesced = s->irq_coalesced; - - s->period = period; - lost_clock += old_irq_coalesced * old_period; - s->irq_coalesced = lost_clock / s->period; - lost_clock %= s->period; - if (old_irq_coalesced != s->irq_coalesced || - old_period != s->period) { - DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, " - "period scaled from %d to %d\n", old_irq_coalesced, - s->irq_coalesced, old_period, s->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); - } + lost_clock = MIN(lost_clock, period); } assert(lost_clock >= 0 && lost_clock <= period);
On Sun, 17 Nov 2019 11:12:43 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 17/11/19 05:31, Alex Williamson wrote: > > The 'merge' option gives me a similar error. The 'delay' option is > > the only other choice where I can actually start the VM, but this > > results in the commandline: > > > > -rtc base=localtime > > > > (no driftfix specified) > > none is the default, so that's okay. > > > This does appear to resolve the issue, but of course compatibility > > with existing configurations has regressed. Thanks, > > Yeah, I guess this was just a suggestion to double-check the cause of > the regression. > > The problem could be that periodic_timer_update is using old_period == 0 > for two cases: no period change, and old period was 0 (periodic timer > off). > > Something like the following distinguishes the two cases by always using > s->period (currently it was only used for driftfix=slew) and passing > s->period instead of 0 when there is no period change. > > More cleanups are possible, but this is the smallest patch that implements > the idea. The first patch is big but, indentation changes aside, it's > moving a single closed brace. > > Alex/Marcelo, can you check if it fixes both of your test cases? It resolves the majority of the regression, but I think there's still a performance issue. Passmark PerformanceTest in the guest shows a 5+% decrease versus a straight revert. Thanks, Alex > ------------- 8< --------------- > From 48dc9d140c636067b8de1ab8e25b819151c83162 Mon Sep 17 00:00:00 2001 > From: Paolo Bonzini <pbonzini@redhat.com> > Date: Sun, 17 Nov 2019 10:07:38 +0100 > Subject: [PATCH 1/2] Revert "mc146818rtc: fix timer interrupt reinjection" > > This reverts commit b429de730174b388ea5760e3debb0d542ea3c261, except > that the reversal of the outer "if (period)" is left in. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/rtc/mc146818rtc.c | 67 ++++++++++++++++++++++---------------------- > 1 file changed, 33 insertions(+), 34 deletions(-) > > diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c > index ee6bf82b40..9869dc5031 100644 > --- a/hw/rtc/mc146818rtc.c > +++ b/hw/rtc/mc146818rtc.c > @@ -174,7 +174,6 @@ periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period) > int64_t cur_clock, next_irq_clock, lost_clock = 0; > > period = rtc_periodic_clock_ticks(s); > - > if (!period) { > s->irq_coalesced = 0; > timer_del(s->periodic_timer); > @@ -197,42 +196,42 @@ periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period) > 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: > + * > + * a) if one or more periodic timer interrupts have been lost, > + * lost_clock will be more that a period. > + * > + * b) when the period may be reconfigured, we expect the OS to > + * treat delayed tick as the new period. So, when switching > + * from a shorter to a longer period, scale down the missing, > + * because the OS will treat past delayed ticks as longer > + * (leftovers are put back into lost_clock). When switching > + * to a shorter period, scale up the missing ticks since the > + * OS handler will treat past delayed ticks as shorter. > + */ > + if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) { > + uint32_t old_irq_coalesced = s->irq_coalesced; > + > + s->period = period; > + lost_clock += old_irq_coalesced * old_period; > + s->irq_coalesced = lost_clock / s->period; > + lost_clock %= s->period; > + if (old_irq_coalesced != s->irq_coalesced || > + old_period != s->period) { > + DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, " > + "period scaled from %d to %d\n", old_irq_coalesced, > + s->irq_coalesced, old_period, s->period); > + rtc_coalesced_timer_update(s); > + } > + } else { > /* > - * s->irq_coalesced can change for two reasons: > - * > - * a) if one or more periodic timer interrupts have been lost, > - * lost_clock will be more that a period. > - * > - * b) when the period may be reconfigured, we expect the OS to > - * treat delayed tick as the new period. So, when switching > - * from a shorter to a longer period, scale down the missing, > - * because the OS will treat past delayed ticks as longer > - * (leftovers are put back into lost_clock). When switching > - * to a shorter period, scale up the missing ticks since the > - * OS handler will treat past delayed ticks as shorter. > + * no way to compensate the interrupt if LOST_TICK_POLICY_SLEW > + * is not used, we should make the time progress anyway. > */ > - if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) { > - uint32_t old_irq_coalesced = s->irq_coalesced; > - > - s->period = period; > - lost_clock += old_irq_coalesced * old_period; > - s->irq_coalesced = lost_clock / s->period; > - lost_clock %= s->period; > - if (old_irq_coalesced != s->irq_coalesced || > - old_period != s->period) { > - DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, " > - "period scaled from %d to %d\n", old_irq_coalesced, > - s->irq_coalesced, old_period, s->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); > - } > + lost_clock = MIN(lost_clock, period); > } > > assert(lost_clock >= 0 && lost_clock <= period);
On Sun, Nov 17, 2019 at 11:12:43AM +0100, Paolo Bonzini wrote: > On 17/11/19 05:31, Alex Williamson wrote: > > The 'merge' option gives me a similar error. The 'delay' option is > > the only other choice where I can actually start the VM, but this > > results in the commandline: > > > > -rtc base=localtime > > > > (no driftfix specified) > > none is the default, so that's okay. > > > This does appear to resolve the issue, but of course compatibility > > with existing configurations has regressed. Thanks, > > Yeah, I guess this was just a suggestion to double-check the cause of > the regression. > > The problem could be that periodic_timer_update is using old_period == 0 > for two cases: no period change, and old period was 0 (periodic timer > off). > Something like the following distinguishes the two cases by always using > s->period (currently it was only used for driftfix=slew) and passing > s->period instead of 0 when there is no period change. > > More cleanups are possible, but this is the smallest patch that implements > the idea. The first patch is big but, indentation changes aside, it's > moving a single closed brace. > > Alex/Marcelo, can you check if it fixes both of your test cases? Second patch blocks NTPd from synchronizing to a source at all (can't even confirm if it fails to synchronize after a while). Problem seems to be that calling from timer interrupt path: /* * 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); } Adds the difference between when the timer interrupt actually executed (cur_clock) and when it should have executed (last_periodic_clock) as reinject time (which will end up injecting more timer interrupts than necessary, so the clock runs faster than it should). Perhaps this is the reason for the 5%+ performance delta? The following, on top of Paolo's two patches, fixes it for me (and NTPd is able to maintain clock synchronized while playing video on Windows 7). Alex perhaps you can give it a try? --- hw/rtc/mc146818rtc.c.orig 2019-11-18 19:16:49.077479836 -0200 +++ hw/rtc/mc146818rtc.c 2019-11-18 19:22:35.706803090 -0200 @@ -168,7 +168,7 @@ * is just due to period adjustment. */ static void -periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period) +periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period, bool period_change) { uint32_t period; int64_t cur_clock, next_irq_clock, lost_clock = 0; @@ -190,7 +190,7 @@ * if the periodic timer's update is due to period re-configuration, * we should count the clock since last interrupt. */ - if (old_period) { + if (old_period && period_change) { int64_t last_periodic_clock, next_periodic_clock; next_periodic_clock = muldiv64(s->next_periodic_time, @@ -246,7 +246,7 @@ { RTCState *s = opaque; - periodic_timer_update(s, s->next_periodic_time, s->period); + periodic_timer_update(s, s->next_periodic_time, s->period, false); s->cmos_data[RTC_REG_C] |= REG_C_PF; if (s->cmos_data[RTC_REG_B] & REG_B_PIE) { s->cmos_data[RTC_REG_C] |= REG_C_IRQF; @@ -512,7 +512,7 @@ if (update_periodic_timer) { periodic_timer_update(s, qemu_clock_get_ns(rtc_clock), - old_period); + old_period, true); } check_update_timer(s); @@ -551,7 +551,7 @@ if (update_periodic_timer) { periodic_timer_update(s, qemu_clock_get_ns(rtc_clock), - old_period); + old_period, true); } check_update_timer(s); @@ -805,7 +805,7 @@ uint64_t now = qemu_clock_get_ns(rtc_clock); if (now < s->next_periodic_time || now > (s->next_periodic_time + get_max_clock_jump())) { - periodic_timer_update(s, qemu_clock_get_ns(rtc_clock), s->period); + periodic_timer_update(s, qemu_clock_get_ns(rtc_clock), s->period, false); } }
On 18/11/19 22:44, Marcelo Tosatti wrote: > Second patch blocks NTPd from synchronizing to a source at all > (can't even confirm if it fails to synchronize after a while). > > Problem seems to be that calling from timer interrupt path: > > /* > * 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); > } > > Adds the difference between when the timer interrupt actually executed > (cur_clock) and when it should have executed (last_periodic_clock) > as reinject time (which will end up injecting more timer interrupts > than necessary, so the clock runs faster than it should). Yes, I made a similar reasoning. Thanks for the patch, I don't like that it adds complication over complication but I guess it would be okay for 4.2, if Alex confirms it works for him. Mine is a much bigger change . > Perhaps this is the reason for the 5%+ performance delta? > > The following, on top of Paolo's two patches, fixes it for me > (and NTPd is able to maintain clock synchronized while playing video on Windows 7). FYI, here is my attempt at cleaning up everything: ------------------- 8< ---------------- From c0a53b1c9a331ac4cf5846b477ba0fb795933a34 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini <pbonzini@redhat.com> Date: Sun, 17 Nov 2019 10:07:38 +0100 Subject: [PATCH 1/5] Revert "mc146818rtc: fix timer interrupt reinjection" This reverts commit b429de730174b388ea5760e3debb0d542ea3c261, except that the reversal of the outer "if (period)" is left in. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/rtc/mc146818rtc.c | 67 ++++++++++++++++++++++++++-------------------------- 1 file changed, 33 insertions(+), 34 deletions(-) diff --git a/hw/rtc/mc146818rtc.c b/hw/rtc/mc146818rtc.c index ee6bf82..9869dc5 100644 --- a/hw/rtc/mc146818rtc.c +++ b/hw/rtc/mc146818rtc.c @@ -174,7 +174,6 @@ periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period) int64_t cur_clock, next_irq_clock, lost_clock = 0; period = rtc_periodic_clock_ticks(s); - if (!period) { s->irq_coalesced = 0; timer_del(s->periodic_timer); @@ -197,42 +196,42 @@ periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period) 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: + * + * a) if one or more periodic timer interrupts have been lost, + * lost_clock will be more that a period. + * + * b) when the period may be reconfigured, we expect the OS to + * treat delayed tick as the new period. So, when switching + * from a shorter to a longer period, scale down the missing, + * because the OS will treat past delayed ticks as longer + * (leftovers are put back into lost_clock). When switching + * to a shorter period, scale up the missing ticks since the + * OS handler will treat past delayed ticks as shorter. + */ + if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) { + uint32_t old_irq_coalesced = s->irq_coalesced; + + s->period = period; + lost_clock += old_irq_coalesced * old_period; + s->irq_coalesced = lost_clock / s->period; + lost_clock %= s->period; + if (old_irq_coalesced != s->irq_coalesced || + old_period != s->period) { + DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, " + "period scaled from %d to %d\n", old_irq_coalesced, + s->irq_coalesced, old_period, s->period); + rtc_coalesced_timer_update(s); + } + } else { /* - * s->irq_coalesced can change for two reasons: - * - * a) if one or more periodic timer interrupts have been lost, - * lost_clock will be more that a period. - * - * b) when the period may be reconfigured, we expect the OS to - * treat delayed tick as the new period. So, when switching - * from a shorter to a longer period, scale down the missing, - * because the OS will treat past delayed ticks as longer - * (leftovers are put back into lost_clock). When switching - * to a shorter period, scale up the missing ticks since the - * OS handler will treat past delayed ticks as shorter. + * no way to compensate the interrupt if LOST_TICK_POLICY_SLEW + * is not used, we should make the time progress anyway. */ - if (s->lost_tick_policy == LOST_TICK_POLICY_SLEW) { - uint32_t old_irq_coalesced = s->irq_coalesced; - - s->period = period; - lost_clock += old_irq_coalesced * old_period; - s->irq_coalesced = lost_clock / s->period; - lost_clock %= s->period; - if (old_irq_coalesced != s->irq_coalesced || - old_period != s->period) { - DPRINTF_C("cmos: coalesced irqs scaled from %d to %d, " - "period scaled from %d to %d\n", old_irq_coalesced, - s->irq_coalesced, old_period, s->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); - } + lost_clock = MIN(lost_clock, period); } assert(lost_clock >= 0 && lost_clock <= period);
On Mon, 18 Nov 2019 19:44:30 -0200 Marcelo Tosatti <mtosatti@redhat.com> wrote: > On Sun, Nov 17, 2019 at 11:12:43AM +0100, Paolo Bonzini wrote: > > On 17/11/19 05:31, Alex Williamson wrote: > > > The 'merge' option gives me a similar error. The 'delay' option is > > > the only other choice where I can actually start the VM, but this > > > results in the commandline: > > > > > > -rtc base=localtime > > > > > > (no driftfix specified) > > > > none is the default, so that's okay. > > > > > This does appear to resolve the issue, but of course compatibility > > > with existing configurations has regressed. Thanks, > > > > Yeah, I guess this was just a suggestion to double-check the cause of > > the regression. > > > > The problem could be that periodic_timer_update is using old_period == 0 > > for two cases: no period change, and old period was 0 (periodic timer > > off). > > > > Something like the following distinguishes the two cases by always using > > s->period (currently it was only used for driftfix=slew) and passing > > s->period instead of 0 when there is no period change. > > > > More cleanups are possible, but this is the smallest patch that implements > > the idea. The first patch is big but, indentation changes aside, it's > > moving a single closed brace. > > > > Alex/Marcelo, can you check if it fixes both of your test cases? > > Second patch blocks NTPd from synchronizing to a source at all > (can't even confirm if it fails to synchronize after a while). > > Problem seems to be that calling from timer interrupt path: > > /* > * 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); > } > > Adds the difference between when the timer interrupt actually executed > (cur_clock) and when it should have executed (last_periodic_clock) > as reinject time (which will end up injecting more timer interrupts > than necessary, so the clock runs faster than it should). > > Perhaps this is the reason for the 5%+ performance delta? > > The following, on top of Paolo's two patches, fixes it for me > (and NTPd is able to maintain clock synchronized while playing video on Windows 7). > > Alex perhaps you can give it a try? Yes, Paolo's two patches plus this seems to resolve both the functional and performance issues. Thanks, Alex > --- hw/rtc/mc146818rtc.c.orig 2019-11-18 19:16:49.077479836 -0200 > +++ hw/rtc/mc146818rtc.c 2019-11-18 19:22:35.706803090 -0200 > @@ -168,7 +168,7 @@ > * is just due to period adjustment. > */ > static void > -periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period) > +periodic_timer_update(RTCState *s, int64_t current_time, uint32_t old_period, bool period_change) > { > uint32_t period; > int64_t cur_clock, next_irq_clock, lost_clock = 0; > @@ -190,7 +190,7 @@ > * if the periodic timer's update is due to period re-configuration, > * we should count the clock since last interrupt. > */ > - if (old_period) { > + if (old_period && period_change) { > int64_t last_periodic_clock, next_periodic_clock; > > next_periodic_clock = muldiv64(s->next_periodic_time, > @@ -246,7 +246,7 @@ > { > RTCState *s = opaque; > > - periodic_timer_update(s, s->next_periodic_time, s->period); > + periodic_timer_update(s, s->next_periodic_time, s->period, false); > s->cmos_data[RTC_REG_C] |= REG_C_PF; > if (s->cmos_data[RTC_REG_B] & REG_B_PIE) { > s->cmos_data[RTC_REG_C] |= REG_C_IRQF; > @@ -512,7 +512,7 @@ > > if (update_periodic_timer) { > periodic_timer_update(s, qemu_clock_get_ns(rtc_clock), > - old_period); > + old_period, true); > } > > check_update_timer(s); > @@ -551,7 +551,7 @@ > > if (update_periodic_timer) { > periodic_timer_update(s, qemu_clock_get_ns(rtc_clock), > - old_period); > + old_period, true); > } > > check_update_timer(s); > @@ -805,7 +805,7 @@ > uint64_t now = qemu_clock_get_ns(rtc_clock); > if (now < s->next_periodic_time || > now > (s->next_periodic_time + get_max_clock_jump())) { > - periodic_timer_update(s, qemu_clock_get_ns(rtc_clock), s->period); > + periodic_timer_update(s, qemu_clock_get_ns(rtc_clock), s->period, false); > } > } >
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)
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>