Re: [PATCH v1 0/5] target/arm/kvm: Provide an option to adjust virtual time

[Masayoshi Mizuma]

Hi Drew,

Thank you for posting the patches, they seems to work well
because the softlockup is gone and the timestamp jump of
dmesg and ftrace record also disappeared after the guest
is virsh resume'ed.

Let me add comments.
I think the kvm-adjvtime behavior should be the default.
How about introducing 'kvm-noadjvtime' parameter instead?
kvm-noadjvtime is to provide the old behavior.

That is because the time jump occurs timeout for timers even though
the timer doesn't reach the timeout in the guest time.

For example, below flow shows that user and/or kernel sets timer A
for +10 sec and B for +20 sec in Guest, then Guest is suspended and
it passes 60 sec, then Guest is resumed. Timer A and B go off because
the Guest timestamp (TS) is jumped to 63. That is unexpected timer
behavior for Guest.

 Host TS(sec) Guest TS(sec) Event
 ============ ============= =============================
 00           00            Guest: Set timer A for +10 sec
 01           01            Guest: Set timer B for +20 sec
 03           03            Host: virsh suspend Guest
 63           63            Host: virsh resume Guest
                            Guest: Timer A and B go off

I believe kvm-adjvtime behavior is as following. So Time A
and B go off as expected time. So, kvm-adjvtime behavior should
be the default.

 Host TS(sec) Guest TS(sec) Event
 ============ ============= =============================
 00           00            Guest: Set timer A for +10 sec
 01           01            Guest: Set timer B for +20 sec
 03           03            Host: virsh suspend guest
 63           03            Host: virsh resume guest
 70           10            Guest: Timer A goes off
 81           21            Guest: Timer B goes off

Thanks,
Masa

On Wed, Oct 16, 2019 at 04:34:05PM +0200, Andrew Jones wrote:
> v2:
>  - move from RFC status to v1
>  - put kvm_arm_vm_state_change() in kvm.c to share among kvm32.c and kvm64.c
>  - add r-b's from Richard
> 
> 
> This series is inspired by a series[1] posted by Bijan Mottahedeh about
> a year ago.  The problem described in the cover letter of [1] is easily
> reproducible and some users would like to have the option to avoid it.
> However the solution, which is to adjust the virtual counter offset each
> time the VM transitions to the running state, introduces a different
> problem, which is that the virtual and physical counters diverge.  As
> described in the cover letter of [1] this divergence is easily observed
> when comparing the output of `date` and `hwclock` after suspending the
> guest, waiting a while, and then resuming it.  Because this different
> problem may actually be worse for some users, unlike [1], the series
> posted here makes the virtual counter offset adjustment optional and not
> even enabled by default.  Besides the adjustment being optional, this
> series approaches the needed changes differently to apply them in more
> appropriate locations.  Finally, unlike [1], this series doesn't attempt
> to measure "pause time" itself.  Simply using QEMU_CLOCK_VIRTUAL, which
> only ticks when the VM is not stopped, is sufficient.
> 
> I've based this series on the SVE series[2] because we're adding a new
> CPU feature (kvm-adjvtime) and the SVE series introduces CPU feature
> documentation, probing, and tests that we can then immediately apply
> to kvm-adjvtime.
> 
> Additional notes
> ----------------
> 
> Note 1
> ------
> 
> As described above, when running a guest with kvm-adjtime enabled it
> will be less likely the guest OS and guest applications get surprise
> time jumps when they use the virtual counter.  However the counter will
> no longer reflect real time.  It will lag behind.  If this is a problem
> then the guest can resynchronize its time from an external source or
> even from its physical counter.  If the suspend/resume is done with
> libvirt's virsh, and the guest is running the guest agent, then it's
> also possible to use a sequence like this
> 
>  $ virsh suspend $GUEST
>  $ virsh resume $GUEST
>  $ virsh domtime --sync $GUEST
> 
> in order to resynchronize a guest right after the resume.  Of course
> there will still be time when the clock is not right, possibly creating
> confusing timestamps in logs, for example, and the guest must still be
> tolerant to the time synchronizations.
> 
> Note 2
> ------
> 
> Userspace that wants to set KVM_REG_ARM_TIMER_CNT should beware that
> the KVM register ID is not correct.  This cannot be fixed because it's
> UAPI and if the UAPI headers are used then it can't be a problem.
> However, if a userspace attempts to create the ID themselves from the
> register's specification, then they will get KVM_REG_ARM_TIMER_CVAL
> instead, as the _CNT and _CVAL definitions have their register
> parameters swapped.
> 
> Note 3
> ------
> 
> I didn't test this with a 32-bit KVM host, but the changes to kvm32.c
> are the same as kvm64.c. So what could go wrong? Test results would be
> appreciated.
>  
> 
> [1] https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg05713.html
> [2] https://patchew.org/QEMU/address@hidden/
> 
> Thanks,
> drew
> 
> 
> Andrew Jones (5):
>   target/arm/kvm64: kvm64 cpus have timer registers
>   timer: arm: Introduce functions to get the host cntfrq
>   target/arm/kvm: Implement cpu feature kvm-adjvtime
>   tests/arm-cpu-features: Check feature default values
>   target/arm/cpu: Add the kvm-adjvtime CPU property
> 
>  docs/arm-cpu-features.rst | 27 +++++++++++++++++-
>  include/qemu/timer.h      | 16 +++++++++++
>  target/arm/cpu.c          |  2 ++
>  target/arm/cpu.h          |  3 ++
>  target/arm/cpu64.c        |  1 +
>  target/arm/kvm.c          | 59 +++++++++++++++++++++++++++++++++++++++
>  target/arm/kvm32.c        |  3 ++
>  target/arm/kvm64.c        |  4 +++
>  target/arm/kvm_arm.h      | 25 +++++++++++++++++
>  target/arm/monitor.c      |  1 +
>  tests/arm-cpu-features.c  | 48 +++++++++++++++++++++++++------
>  11 files changed, 179 insertions(+), 10 deletions(-)
> 
> -- 
> 2.21.0
> 
>