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

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

[Andrew Jones]

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/20191001125845.8793-1-drjones@redhat.com/

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

[PATCH v1 1/5] target/arm/kvm64: kvm64 cpus have timer registers

[Andrew Jones]

Add the missing GENERIC_TIMER feature to kvm64 cpus.

We don't currently use these registers when KVM is enabled, but it's
probably best we add the feature flag for consistency and potential
future use. There's also precedent, as we add the PMU feature flag to
KVM enabled guests, even though we don't use those registers either.

This change was originally posted as a hunk of a different, never
merged patch from Bijan Mottahedeh.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/kvm64.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index e2da756e65ed..666a81a5ce6f 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -605,6 +605,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
     set_feature(&features, ARM_FEATURE_NEON);
     set_feature(&features, ARM_FEATURE_AARCH64);
     set_feature(&features, ARM_FEATURE_PMU);
+    set_feature(&features, ARM_FEATURE_GENERIC_TIMER);
 
     ahcf->features = features;
 
-- 
2.21.0

[PATCH v1 2/5] timer: arm: Introduce functions to get the host cntfrq

[Andrew Jones]

When acceleration like KVM is in use it's necessary to use the host's
counter frequency when converting ticks to or from time units.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/qemu/timer.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/qemu/timer.h b/include/qemu/timer.h
index 85bc6eb00b21..8941ddea8242 100644
--- a/include/qemu/timer.h
+++ b/include/qemu/timer.h
@@ -1006,6 +1006,22 @@ static inline int64_t cpu_get_host_ticks(void)
 }
 #endif
 
+#if defined(__aarch64__)
+static inline uint32_t cpu_get_host_tick_frequency(void)
+{
+    uint64_t frq;
+    asm volatile("mrs %0, cntfrq_el0" : "=r" (frq));
+    return frq;
+}
+#elif defined(__arm__)
+static inline uint32_t cpu_get_host_tick_frequency(void)
+{
+    uint32_t frq;
+    asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r" (frq));
+    return frq;
+}
+#endif
+
 #ifdef CONFIG_PROFILER
 static inline int64_t profile_getclock(void)
 {
-- 
2.21.0

[PATCH v1 3/5] target/arm/kvm: Implement cpu feature kvm-adjvtime

[Andrew Jones]

When kvm-adjvtime is enabled the guest's cntvct[_el0] won't count
the time when the VM is stopped. That time is skipped by updating
cntvoff[_el2] on each transition to vm_running using the current
QEMU_CLOCK_VIRTUAL time. QEMU_CLOCK_VIRTUAL only ticks when the VM
is running.

This patch only provides the implementation. A subsequent patch
will provide the CPU property allowing the feature to be enabled.

Reported-by: Bijan Mottahedeh <bijan.mottahedeh@oracle.com>
Suggested-by: Bijan Mottahedeh <bijan.mottahedeh@oracle.com>
Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/arm/cpu.h     |  3 +++
 target/arm/kvm.c     | 32 ++++++++++++++++++++++++++++++++
 target/arm/kvm32.c   |  3 +++
 target/arm/kvm64.c   |  3 +++
 target/arm/kvm_arm.h | 14 ++++++++++++++
 5 files changed, 55 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index b3092e5213e6..7c07c7c5272e 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -821,6 +821,9 @@ struct ARMCPU {
     /* KVM init features for this CPU */
     uint32_t kvm_init_features[7];
 
+    /* KVM CPU features */
+    bool kvm_adjvtime;
+
     /* Uniprocessor system with MP extensions */
     bool mp_is_up;
 
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 5b82cefef608..373b868dc248 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -348,6 +348,18 @@ void kvm_arm_register_device(MemoryRegion *mr, uint64_t devid, uint64_t group,
     memory_region_ref(kd->mr);
 }
 
+void kvm_arm_vm_state_change(void *opaque, int running, RunState state)
+{
+    CPUState *cs = opaque;
+    ARMCPU *cpu = ARM_CPU(cs);
+
+    if (running) {
+        if (cpu->kvm_adjvtime) {
+            kvm_arm_set_virtual_time(cs);
+        }
+    }
+}
+
 static int compare_u64(const void *a, const void *b)
 {
     if (*(uint64_t *)a > *(uint64_t *)b) {
@@ -579,6 +591,26 @@ int kvm_arm_sync_mpstate_to_qemu(ARMCPU *cpu)
     return 0;
 }
 
+void kvm_arm_set_virtual_time(CPUState *cs)
+{
+    uint64_t cnt;
+    struct kvm_one_reg reg = {
+        .id = KVM_REG_ARM_TIMER_CNT,
+        .addr = (uintptr_t)&cnt,
+    };
+    int ret;
+
+    cnt = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
+                   cpu_get_host_tick_frequency(),
+                   NANOSECONDS_PER_SECOND);
+
+    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
+    if (ret) {
+        error_report("Failed to set KVM_REG_ARM_TIMER_CNT");
+        abort();
+    }
+}
+
 int kvm_put_vcpu_events(ARMCPU *cpu)
 {
     CPUARMState *env = &cpu->env;
diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
index 32bf8d6757c4..3a8b437eef0b 100644
--- a/target/arm/kvm32.c
+++ b/target/arm/kvm32.c
@@ -16,6 +16,7 @@
 #include "qemu-common.h"
 #include "cpu.h"
 #include "qemu/timer.h"
+#include "sysemu/runstate.h"
 #include "sysemu/kvm.h"
 #include "kvm_arm.h"
 #include "internals.h"
@@ -198,6 +199,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
         return -EINVAL;
     }
 
+    qemu_add_vm_change_state_handler(kvm_arm_vm_state_change, cs);
+
     /* Determine init features for this CPU */
     memset(cpu->kvm_init_features, 0, sizeof(cpu->kvm_init_features));
     if (cpu->start_powered_off) {
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 666a81a5ce6f..d368189fbce6 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -23,6 +23,7 @@
 #include "qemu/host-utils.h"
 #include "qemu/main-loop.h"
 #include "exec/gdbstub.h"
+#include "sysemu/runstate.h"
 #include "sysemu/kvm.h"
 #include "sysemu/kvm_int.h"
 #include "kvm_arm.h"
@@ -735,6 +736,8 @@ int kvm_arch_init_vcpu(CPUState *cs)
         return -EINVAL;
     }
 
+    qemu_add_vm_change_state_handler(kvm_arm_vm_state_change, cs);
+
     /* Determine init features for this CPU */
     memset(cpu->kvm_init_features, 0, sizeof(cpu->kvm_init_features));
     if (cpu->start_powered_off) {
diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index 8e14d400e8ab..3bb7e331aa06 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -232,6 +232,16 @@ void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map);
  */
 void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu);
 
+/**
+ * void kvm_arm_set_virtual_time:
+ * @cs: CPUState
+ *
+ * Sets the guest's virtual counter offset to the difference of the host's
+ * current time and QEMU's QEMU_CLOCK_VIRTUAL time. This allows the
+ * guest's virtual counter to only reflect VM running time.
+ */
+void kvm_arm_set_virtual_time(CPUState *cs);
+
 /**
  * kvm_arm_aarch32_supported:
  * @cs: CPUState
@@ -288,6 +298,8 @@ void kvm_arm_pmu_set_irq(CPUState *cs, int irq);
 void kvm_arm_pmu_init(CPUState *cs);
 int kvm_arm_set_irq(int cpu, int irqtype, int irq, int level);
 
+void kvm_arm_vm_state_change(void *opaque, int running, RunState state);
+
 #else
 
 static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
@@ -324,6 +336,8 @@ static inline int kvm_arm_vgic_probe(void)
     return 0;
 }
 
+static inline void kvm_arm_set_virtual_time(CPUState *cs) {}
+
 static inline void kvm_arm_pmu_set_irq(CPUState *cs, int irq) {}
 static inline void kvm_arm_pmu_init(CPUState *cs) {}
 
-- 
2.21.0

[PATCH v1 4/5] tests/arm-cpu-features: Check feature default values

[Andrew Jones]

If we know what the default value should be then we can test for
that as well as the feature existence.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 tests/arm-cpu-features.c | 44 ++++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/tests/arm-cpu-features.c b/tests/arm-cpu-features.c
index 92668efb8f56..ee444b04010f 100644
--- a/tests/arm-cpu-features.c
+++ b/tests/arm-cpu-features.c
@@ -141,6 +141,32 @@ static bool resp_get_feature(QDict *resp, const char *feature)
     qobject_unref(_resp);                                              \
 })
 
+#define assert_has_feature_enabled(qts, cpu_type, feature)             \
+({                                                                     \
+    QDict *_resp, *_props;                                             \
+                                                                       \
+    _resp = do_query_no_props(qts, cpu_type);                          \
+    g_assert(_resp);                                                   \
+    g_assert(resp_has_props(_resp));                                   \
+    _props = resp_get_props(_resp);                                    \
+    g_assert(qdict_get(_props, feature));                              \
+    g_assert(qdict_get_bool(_props, feature));                         \
+    qobject_unref(_resp);                                              \
+})
+
+#define assert_has_feature_disabled(qts, cpu_type, feature)            \
+({                                                                     \
+    QDict *_resp, *_props;                                             \
+                                                                       \
+    _resp = do_query_no_props(qts, cpu_type);                          \
+    g_assert(_resp);                                                   \
+    g_assert(resp_has_props(_resp));                                   \
+    _props = resp_get_props(_resp);                                    \
+    g_assert(qdict_get(_props, feature));                              \
+    g_assert(!qdict_get_bool(_props, feature));                        \
+    qobject_unref(_resp);                                              \
+})
+
 static void assert_type_full(QTestState *qts)
 {
     const char *error;
@@ -387,16 +413,16 @@ static void test_query_cpu_model_expansion(const void *data)
     assert_error(qts, "host", "The CPU type 'host' requires KVM", NULL);
 
     /* Test expected feature presence/absence for some cpu types */
-    assert_has_feature(qts, "max", "pmu");
-    assert_has_feature(qts, "cortex-a15", "pmu");
+    assert_has_feature_enabled(qts, "max", "pmu");
+    assert_has_feature_enabled(qts, "cortex-a15", "pmu");
     assert_has_not_feature(qts, "cortex-a15", "aarch64");
 
     if (g_str_equal(qtest_get_arch(), "aarch64")) {
-        assert_has_feature(qts, "max", "aarch64");
-        assert_has_feature(qts, "max", "sve");
-        assert_has_feature(qts, "max", "sve128");
-        assert_has_feature(qts, "cortex-a57", "pmu");
-        assert_has_feature(qts, "cortex-a57", "aarch64");
+        assert_has_feature_enabled(qts, "max", "aarch64");
+        assert_has_feature_enabled(qts, "max", "sve");
+        assert_has_feature_enabled(qts, "max", "sve128");
+        assert_has_feature_enabled(qts, "cortex-a57", "pmu");
+        assert_has_feature_enabled(qts, "cortex-a57", "aarch64");
 
         sve_tests_default(qts, "max");
 
@@ -417,7 +443,7 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
 
     qts = qtest_init(MACHINE "-accel kvm -cpu host");
 
-    assert_has_feature(qts, "host", "pmu");
+    assert_has_feature_enabled(qts, "host", "pmu");
 
     if (g_str_equal(qtest_get_arch(), "aarch64")) {
         bool kvm_supports_sve;
@@ -427,7 +453,7 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
         QDict *resp;
         char *error;
 
-        assert_has_feature(qts, "host", "aarch64");
+        assert_has_feature_enabled(qts, "host", "aarch64");
 
         assert_error(qts, "cortex-a15",
             "We cannot guarantee the CPU type 'cortex-a15' works "
-- 
2.21.0

[PATCH v1 5/5] target/arm/cpu: Add the kvm-adjvtime CPU property

[Andrew Jones]

kvm-adjvtime is a KVM specific CPU property and a first of its kind.
To accommodate it we also add kvm_arm_add_vcpu_properties() and a
KVM specific CPU properties description to the CPU features document.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 docs/arm-cpu-features.rst | 27 ++++++++++++++++++++++++++-
 target/arm/cpu.c          |  2 ++
 target/arm/cpu64.c        |  1 +
 target/arm/kvm.c          | 27 +++++++++++++++++++++++++++
 target/arm/kvm_arm.h      | 11 +++++++++++
 target/arm/monitor.c      |  1 +
 tests/arm-cpu-features.c  |  4 ++++
 7 files changed, 72 insertions(+), 1 deletion(-)

diff --git a/docs/arm-cpu-features.rst b/docs/arm-cpu-features.rst
index 1b367e22e16e..5c317296845f 100644
--- a/docs/arm-cpu-features.rst
+++ b/docs/arm-cpu-features.rst
@@ -31,7 +31,9 @@ supporting the feature or only supporting the feature under certain
 configurations.  For example, the `aarch64` CPU feature, which, when
 disabled, enables the optional AArch32 CPU feature, is only supported
 when using the KVM accelerator and when running on a host CPU type that
-supports the feature.
+supports the feature.  While `aarch64` currently only works with KVM,
+it could work with TCG.  CPU features that are specific to KVM are
+prefixed with "kvm-" and are described in "KVM VCPU Features".
 
 CPU Feature Probing
 ===================
@@ -171,6 +173,29 @@ disabling many SVE vector lengths would be quite verbose, the `sve<N>` CPU
 properties have special semantics (see "SVE CPU Property Parsing
 Semantics").
 
+KVM VCPU Features
+=================
+
+KVM VCPU features are CPU features that are specific to KVM, such as
+paravirt features or features that enable CPU virtualization extensions.
+The features' CPU properties are only available when KVM is enabled and are
+named with the prefix "kvm-".  KVM VCPU features may be probed, enabled, and
+disabled in the same way as other CPU features.  Below is the list of KVM
+VCPU features and their descriptions.
+
+  kvm-adjvtime             When enabled, each time the VM transitions back
+                           to running state the VCPU's vitual counter is
+                           updated to ensure stopped time is not counted.
+                           This avoids time jumps surprising guest OSes and
+                           applications, as long as they use the virtual
+                           counter for timekeeping, but has the side effect
+                           of the virtual and physical counters diverging.
+                           All timekeeping based on the virtual counter will
+                           appear to lag behind any timekeeping that does
+                           not subtract VM stopped time.  The guest may
+                           resynchronize its virtual counter with other time
+                           sources as needed.
+
 SVE CPU Properties
 ==================
 
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 0c4465880ddd..151771e12bc5 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2481,6 +2481,7 @@ static void arm_max_initfn(Object *obj)
 
     if (kvm_enabled()) {
         kvm_arm_set_cpu_features_from_host(cpu);
+        kvm_arm_add_vcpu_properties(obj);
     } else {
         cortex_a15_initfn(obj);
 
@@ -2672,6 +2673,7 @@ static void arm_host_initfn(Object *obj)
     if (arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
         aarch64_add_sve_properties(obj);
     }
+    kvm_arm_add_vcpu_properties(obj);
     arm_cpu_post_init(obj);
 }
 
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 68baf0482ffa..c9a657a178ce 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -620,6 +620,7 @@ static void aarch64_max_initfn(Object *obj)
 
     if (kvm_enabled()) {
         kvm_arm_set_cpu_features_from_host(cpu);
+        kvm_arm_add_vcpu_properties(obj);
     } else {
         uint64_t t;
         uint32_t u;
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 373b868dc248..35160e8d0525 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -17,6 +17,8 @@
 #include "qemu/timer.h"
 #include "qemu/error-report.h"
 #include "qemu/main-loop.h"
+#include "qom/object.h"
+#include "qapi/error.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
 #include "sysemu/kvm_int.h"
@@ -179,6 +181,31 @@ void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
     env->features = arm_host_cpu_features.features;
 }
 
+static bool kvm_adjvtime_get(Object *obj, Error **errp)
+{
+    return ARM_CPU(obj)->kvm_adjvtime;
+}
+
+static void kvm_adjvtime_set(Object *obj, bool value, Error **errp)
+{
+    ARM_CPU(obj)->kvm_adjvtime = value;
+}
+
+/* KVM VCPU properties should be prefixed with "kvm-". */
+void kvm_arm_add_vcpu_properties(Object *obj)
+{
+    if (!kvm_enabled()) {
+        return;
+    }
+
+    object_property_add_bool(obj, "kvm-adjvtime", kvm_adjvtime_get,
+                             kvm_adjvtime_set, &error_abort);
+    object_property_set_description(obj, "kvm-adjvtime",
+                                    "Set on to enable the adjustment of "
+                                    "the virtual counter. VM stopped time "
+                                    "will not be counted.", &error_abort);
+}
+
 bool kvm_arm_pmu_supported(CPUState *cpu)
 {
     KVMState *s = KVM_STATE(current_machine->accelerator);
diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index 3bb7e331aa06..a5429bfc0dbf 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -232,6 +232,15 @@ void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map);
  */
 void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu);
 
+/**
+ * void kvm_arm_add_vcpu_properties:
+ * @obj: The CPU object to add the properties to
+ *
+ * Add all KVM specific CPU properties to the CPU object. These
+ * are the CPU properties with "kvm-" prefixed names.
+ */
+void kvm_arm_add_vcpu_properties(Object *obj);
+
 /**
  * void kvm_arm_set_virtual_time:
  * @cs: CPUState
@@ -311,6 +320,8 @@ static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
     cpu->host_cpu_probe_failed = true;
 }
 
+static inline void kvm_arm_add_vcpu_properties(Object *obj) {}
+
 static inline bool kvm_arm_aarch32_supported(CPUState *cs)
 {
     return false;
diff --git a/target/arm/monitor.c b/target/arm/monitor.c
index fa054f8a369c..d9b2d94ac3fa 100644
--- a/target/arm/monitor.c
+++ b/target/arm/monitor.c
@@ -103,6 +103,7 @@ static const char *cpu_model_advertised_features[] = {
     "sve128", "sve256", "sve384", "sve512",
     "sve640", "sve768", "sve896", "sve1024", "sve1152", "sve1280",
     "sve1408", "sve1536", "sve1664", "sve1792", "sve1920", "sve2048",
+    "kvm-adjvtime",
     NULL
 };
 
diff --git a/tests/arm-cpu-features.c b/tests/arm-cpu-features.c
index ee444b04010f..c207a2bec9e9 100644
--- a/tests/arm-cpu-features.c
+++ b/tests/arm-cpu-features.c
@@ -417,6 +417,8 @@ static void test_query_cpu_model_expansion(const void *data)
     assert_has_feature_enabled(qts, "cortex-a15", "pmu");
     assert_has_not_feature(qts, "cortex-a15", "aarch64");
 
+    assert_has_not_feature(qts, "max", "kvm-adjvtime");
+
     if (g_str_equal(qtest_get_arch(), "aarch64")) {
         assert_has_feature_enabled(qts, "max", "aarch64");
         assert_has_feature_enabled(qts, "max", "sve");
@@ -445,6 +447,8 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
 
     assert_has_feature_enabled(qts, "host", "pmu");
 
+    assert_has_feature_disabled(qts, "host", "kvm-adjvtime");
+
     if (g_str_equal(qtest_get_arch(), "aarch64")) {
         bool kvm_supports_sve;
         char max_name[8], name[8];
-- 
2.21.0

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/20191001125845.8793-1-drjones@redhat.com/
> 
> 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
> 
> 

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

[Andrew Jones]

On Thu, Oct 17, 2019 at 05:17:59PM -0400, Masayoshi Mizuma wrote:
> 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 for the testing Masa. Your timer test is another good example of
what can happen when a guest is paused. I'm sure there are many other ways
a pause could be problematic as well, especially if the guest has devices
passed through to it that it's actively using. I also don't expect
kvm-adjvtime to solve all those problems (like, for example, potential
problems with passthrough devices, networks, etc.)  This means that guest
pausing should only be done by host admins that are intimately familiar
with the guest OS, workload, and network connections. They should be sure
that it can tolerate and recover from the pauses. Since the admins need to
make the decision to pause at all, then I think it's fair for them to also
decide if they want to try and mitigate some of the issues with
kvm-adjvtime, i.e. require them to enable it, rather than have it on by
default. Also, if we choose to enable it by default, then we'll need to
deal with the compatibility issues that come with changing a behavior.
That's not impossible, as this feature could be disabled for older
machine types, but it's messy.

All that said, I won't argue too hard against kvm-adjvtime being on by
default, but let's see if others like Peter or Marc want to chime in on
it too.

Thanks,
drew

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

[Masayoshi Mizuma]

On Fri, Oct 18, 2019 at 02:02:52PM +0200, Andrew Jones wrote:
> On Thu, Oct 17, 2019 at 05:17:59PM -0400, Masayoshi Mizuma wrote:
> > 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 for the testing Masa. Your timer test is another good example of
> what can happen when a guest is paused. I'm sure there are many other ways
> a pause could be problematic as well, especially if the guest has devices
> passed through to it that it's actively using. I also don't expect
> kvm-adjvtime to solve all those problems (like, for example, potential
> problems with passthrough devices, networks, etc.)  This means that guest
> pausing should only be done by host admins that are intimately familiar
> with the guest OS, workload, and network connections. They should be sure
> that it can tolerate and recover from the pauses. Since the admins need to
> make the decision to pause at all, then I think it's fair for them to also
> decide if they want to try and mitigate some of the issues with
> kvm-adjvtime, i.e. require them to enable it, rather than have it on by
> default. 

make sense to me, thanks.

>          Also, if we choose to enable it by default, then we'll need to
> deal with the compatibility issues that come with changing a behavior.
> That's not impossible, as this feature could be disabled for older
> machine types, but it's messy.

I agree with you, we shouldn't add such messy codes to resolve
the compatibility issues...

> 
> All that said, I won't argue too hard against kvm-adjvtime being on by
> default, but let's see if others like Peter or Marc want to chime in on
> it too.

Yeah, I look forward to their comments.

Thanks,
Masa

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

[Peter Maydell]

On Wed, 16 Oct 2019 at 15:34, Andrew Jones <drjones@redhat.com> 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.

So I guess my overall question is "what is the x86 solution to
this problem, and why is this all arm-specific?" It would also
be helpful to know how it fits into all the other proposals regarding
time in VMs.

thanks
-- PMM

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

[Andrew Jones]

On Fri, Dec 06, 2019 at 03:22:58PM +0000, Peter Maydell wrote:
> On Wed, 16 Oct 2019 at 15:34, Andrew Jones <drjones@redhat.com> 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.
> 
> So I guess my overall question is "what is the x86 solution to
> this problem, and why is this all arm-specific?" It would also

x86 adjusts the counter offset by default, and I don't think there's any
way to turn that behavior off. I think it's too late to follow that
default for arm, but this series provides a way to opt into the same
behavior.

> be helpful to know how it fits into all the other proposals regarding
> time in VMs.

I've been lightly following the other stuff, but haven't yet seen any
overlap.

BTW, this series needs to be rebased and reposted. I've been waiting for
4.2 though.

Thanks,
drew

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

[Andrea Bolognani]

On Fri, 2019-12-06 at 16:53 +0100, Andrew Jones wrote:
> On Fri, Dec 06, 2019 at 03:22:58PM +0000, Peter Maydell wrote:
> > On Wed, 16 Oct 2019 at 15:34, Andrew Jones <drjones@redhat.com> wrote:
> > > 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.
> > 
> > So I guess my overall question is "what is the x86 solution to
> > this problem, and why is this all arm-specific?" It would also
> 
> x86 adjusts the counter offset by default, and I don't think there's any
> way to turn that behavior off. I think it's too late to follow that
> default for arm, but this series provides a way to opt into the same
> behavior.

My understanding is that turning kvm-adjvtime either on or off
results in a different set of advantages and drawbacks, with neither
begin a one-size-fits-all solution. So it's good that we offer a way
for the user to pick one or the other based on their specific needs.

The main difference is that kvm-adjvtime=on matches x86's behavior,
which is fairly well understood and thoroughly documented, along with
the corresponding workarounds, dos and don'ts.

With that in mind, in my opinion it would make sense to make
kvm-adjvtime=on the behavior for newer machine types so that people
coming from an x86 background and/or having to manage both at the
same time will get a consistent experience and will be able to draw,
even for aarch64, on the considerable amount of existing x86-centric
literature on the subject.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[Peter Maydell]

On Tue, 10 Dec 2019 at 09:51, Andrea Bolognani <abologna@redhat.com> wrote:
>
> On Fri, 2019-12-06 at 16:53 +0100, Andrew Jones wrote:
> > On Fri, Dec 06, 2019 at 03:22:58PM +0000, Peter Maydell wrote:
> > > On Wed, 16 Oct 2019 at 15:34, Andrew Jones <drjones@redhat.com> wrote:
> > > > 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.
> > >
> > > So I guess my overall question is "what is the x86 solution to
> > > this problem, and why is this all arm-specific?" It would also
> >
> > x86 adjusts the counter offset by default, and I don't think there's any
> > way to turn that behavior off. I think it's too late to follow that
> > default for arm, but this series provides a way to opt into the same
> > behavior.
>
> My understanding is that turning kvm-adjvtime either on or off
> results in a different set of advantages and drawbacks, with neither
> begin a one-size-fits-all solution. So it's good that we offer a way
> for the user to pick one or the other based on their specific needs.

If this is the case, shouldn't we be looking at having the
option exist for all architectures, not just arm? Obviously
pre-existing behaviour would imply having the default have
to differ for some archs/machines.

thanks
-- PMM

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

[Andrew Jones]

On Tue, Dec 10, 2019 at 10:29:22AM +0000, Peter Maydell wrote:
> On Tue, 10 Dec 2019 at 09:51, Andrea Bolognani <abologna@redhat.com> wrote:
> >
> > On Fri, 2019-12-06 at 16:53 +0100, Andrew Jones wrote:
> > > On Fri, Dec 06, 2019 at 03:22:58PM +0000, Peter Maydell wrote:
> > > > On Wed, 16 Oct 2019 at 15:34, Andrew Jones <drjones@redhat.com> wrote:
> > > > > 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.
> > > >
> > > > So I guess my overall question is "what is the x86 solution to
> > > > this problem, and why is this all arm-specific?" It would also
> > >
> > > x86 adjusts the counter offset by default, and I don't think there's any
> > > way to turn that behavior off. I think it's too late to follow that
> > > default for arm, but this series provides a way to opt into the same
> > > behavior.
> >
> > My understanding is that turning kvm-adjvtime either on or off
> > results in a different set of advantages and drawbacks, with neither
> > begin a one-size-fits-all solution. So it's good that we offer a way
> > for the user to pick one or the other based on their specific needs.
> 
> If this is the case, shouldn't we be looking at having the
> option exist for all architectures, not just arm? Obviously
> pre-existing behaviour would imply having the default have
> to differ for some archs/machines.
>

x86 developers could easily add this feature if/when they need a way to
disable their current default behavior. But, since the kvm-adjvtime
default would likely be 'on' for them, then they'd probably prefer the
feature be named kvm-no-adjvtime, and default 'off'. Should we try to
anticipate what x86 might want when naming this feature? IMO, we should
not, especially because I'm doubtful that x86 will ever want to implement
it. Also, what about the other KVM capable architectures? Which defaults
do they have now? And do we expect them to want to expose a switch to the
user to change it?

OTOH, I agree with Andrea that it would be nice if arm had the same
default as x86, allowing the documentation regarding this stuff to apply
to both. If we did choose to turn this feature on by default for virt-5.0,
then maybe we should introduce the feature with the name kvm-no-adjvtime
instead.

Thanks,
drew

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

[Peter Maydell]

On Tue, 10 Dec 2019 at 11:05, Andrew Jones <drjones@redhat.com> wrote:
> x86 developers could easily add this feature if/when they need a way to
> disable their current default behavior. But, since the kvm-adjvtime
> default would likely be 'on' for them, then they'd probably prefer the
> feature be named kvm-no-adjvtime, and default 'off'. Should we try to
> anticipate what x86 might want when naming this feature? IMO, we should
> not, especially because I'm doubtful that x86 will ever want to implement
> it. Also, what about the other KVM capable architectures? Which defaults
> do they have now? And do we expect them to want to expose a switch to the
> user to change it?

My perspective here is mostly that I don't really understand
the ins and outs of KVM and in particular handling of
time in VMs, beyond knowing that it's complicated. So I
prefer approaches that push back to "do everything the same
for all architectures rather than having something that's
arm-specific", because then things get more review from
the larger mass of non-arm KVM/QEMU developers. Arm-specific
switches/interfaces/designs just make arm more of a
special-snowflake. I don't really have a basis to be able
to review the patchset beyond those general biases.

thanks
-- PMM

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

[Andrew Jones]

On Tue, Dec 10, 2019 at 11:48:38AM +0000, Peter Maydell wrote:
> On Tue, 10 Dec 2019 at 11:05, Andrew Jones <drjones@redhat.com> wrote:
> > x86 developers could easily add this feature if/when they need a way to
> > disable their current default behavior. But, since the kvm-adjvtime
> > default would likely be 'on' for them, then they'd probably prefer the
> > feature be named kvm-no-adjvtime, and default 'off'. Should we try to
> > anticipate what x86 might want when naming this feature? IMO, we should
> > not, especially because I'm doubtful that x86 will ever want to implement
> > it. Also, what about the other KVM capable architectures? Which defaults
> > do they have now? And do we expect them to want to expose a switch to the
> > user to change it?
> 
> My perspective here is mostly that I don't really understand
> the ins and outs of KVM and in particular handling of
> time in VMs, beyond knowing that it's complicated. So I
> prefer approaches that push back to "do everything the same
> for all architectures rather than having something that's
> arm-specific", because then things get more review from
> the larger mass of non-arm KVM/QEMU developers. Arm-specific
> switches/interfaces/designs just make arm more of a
> special-snowflake. I don't really have a basis to be able
> to review the patchset beyond those general biases.
>

So the ins and outs of this particular timekeeping issue (to the best of
my knowledge) is that x86 has implemented this behavior since
00f4d64ee76e ("kvmclock: clock should count only if vm is running"), which
was committed over six years ago. Possibly x86 VM time would behave more
like arm VM time if kvmclock was disabled, but that's not a recommended
configuration.

PPC got an equivalent patch to the x86 one in 2017, 42043e4f1241 ("spapr:
clock should count only if vm is running"), but when stopping time during
pause on spapr they actually *keep* 'date' and 'hwclock' in synch. I guess
whatever clocksource 'hwclock' uses on spapr was already stopping when
paused? For x86 those values diverge, and for arm without this series they
stay the same but experience jumps, and with this series they diverge like
x86. I don't see any way to disable the behaviour 42043e4f1241 introduces.

s390x got what appears to be its equivalent patch last year 9bc9d3d1ae3b
("s390x/tod: Properly stop the KVM TOD while the guest is not running").
The commit message doesn't state how hwclock and date values change /
don't change, and I don't see any way to disable the behavior.

MIPS has had this implemented since KVM support was introduced. No way
to disable it that I know of.

So why is this arm-specific? arm is just trying to catch up, but also
offer a switch allowing the current behavior to be selected. If other
architectures see value in the switch then they're free to adopt it too.
After having done this git mining, it looks more and more like we should
at least consider naming this feature 'kvm-no-adjvtime' and probably
also change arm's default.

Thanks,
drew

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

[Andrea Bolognani]

On Tue, 2019-12-10 at 14:32 +0100, Andrew Jones wrote:
> After having done this git mining, it looks more and more like we should
> at least consider naming this feature 'kvm-no-adjvtime' and probably
> also change arm's default.

I agree with everything except the naming: why would

  kvm-no-adjvtime=off  vtime is adjusted      (default)
  kvm-no-adjvtime=on   vtime is not adjusted

be better than

  kvm-adjvtime=on   vtime is adjusted      (default)
  kvm-adjvtime=off  vtime is not adjusted

? Both offer the exact same amount of flexibility, but the latter has
the advantage of not introducing any unwieldy double negatives.

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[Andrew Jones]

On Tue, Dec 10, 2019 at 03:21:02PM +0100, Andrea Bolognani wrote:
> On Tue, 2019-12-10 at 14:32 +0100, Andrew Jones wrote:
> > After having done this git mining, it looks more and more like we should
> > at least consider naming this feature 'kvm-no-adjvtime' and probably
> > also change arm's default.
> 
> I agree with everything except the naming: why would
> 
>   kvm-no-adjvtime=off  vtime is adjusted      (default)
>   kvm-no-adjvtime=on   vtime is not adjusted
> 
> be better than
> 
>   kvm-adjvtime=on   vtime is adjusted      (default)
>   kvm-adjvtime=off  vtime is not adjusted
> 
> ? Both offer the exact same amount of flexibility, but the latter has
> the advantage of not introducing any unwieldy double negatives.
>

A default of 'off' == 0 means not setting anything at all. There's
already precedent for 'kvm-no*' prefixed cpu features,

kvm-no-smi-migration
kvm-nopiodelay

(Unless there's something called a 'nopio' then I guess that one is
missing a -, but whatever...)

Thanks,
drew

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

[Peter Maydell]

On Tue, 10 Dec 2019 at 13:33, Andrew Jones <drjones@redhat.com> wrote:
> So the ins and outs of this particular timekeeping issue (to the best of
> my knowledge) is that x86 has implemented this behavior since
> 00f4d64ee76e ("kvmclock: clock should count only if vm is running"), which
> was committed over six years ago. Possibly x86 VM time would behave more
> like arm VM time if kvmclock was disabled, but that's not a recommended
> configuration.
>
> PPC got an equivalent patch to the x86 one in 2017, 42043e4f1241 ("spapr:
> clock should count only if vm is running"), but when stopping time during
> pause on spapr they actually *keep* 'date' and 'hwclock' in synch. I guess
> whatever clocksource 'hwclock' uses on spapr was already stopping when
> paused? For x86 those values diverge, and for arm without this series they
> stay the same but experience jumps, and with this series they diverge like
> x86. I don't see any way to disable the behaviour 42043e4f1241 introduces.
>
> s390x got what appears to be its equivalent patch last year 9bc9d3d1ae3b
> ("s390x/tod: Properly stop the KVM TOD while the guest is not running").
> The commit message doesn't state how hwclock and date values change /
> don't change, and I don't see any way to disable the behavior.
>
> MIPS has had this implemented since KVM support was introduced. No way
> to disable it that I know of.
>
> So why is this arm-specific? arm is just trying to catch up, but also
> offer a switch allowing the current behavior to be selected. If other
> architectures see value in the switch then they're free to adopt it too.
> After having done this git mining, it looks more and more like we should
> at least consider naming this feature 'kvm-no-adjvtime' and probably
> also change arm's default.

Thanks for pulling up the handling by other architectures.
I think I agree that we should change the arm default (ie
we should just call this a bug fix, since the old behaviour
seems unhelpful generally and is more random accident than
a deliberate choice), with a switch provided just in case
anybody had something depending on the old behaviour.

-- PMM

Re: [PATCH v1 2/5] timer: arm: Introduce functions to get the host cntfrq

[Peter Maydell]

On Wed, 16 Oct 2019 at 15:34, Andrew Jones <drjones@redhat.com> wrote:
>
> When acceleration like KVM is in use it's necessary to use the host's
> counter frequency when converting ticks to or from time units.
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  include/qemu/timer.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/include/qemu/timer.h b/include/qemu/timer.h
> index 85bc6eb00b21..8941ddea8242 100644
> --- a/include/qemu/timer.h
> +++ b/include/qemu/timer.h
> @@ -1006,6 +1006,22 @@ static inline int64_t cpu_get_host_ticks(void)
>  }
>  #endif
>
> +#if defined(__aarch64__)
> +static inline uint32_t cpu_get_host_tick_frequency(void)
> +{
> +    uint64_t frq;
> +    asm volatile("mrs %0, cntfrq_el0" : "=r" (frq));
> +    return frq;
> +}
> +#elif defined(__arm__)
> +static inline uint32_t cpu_get_host_tick_frequency(void)
> +{
> +    uint32_t frq;
> +    asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r" (frq));
> +    return frq;
> +}
> +#endif

Don't we want to know what the guest counter frequency
is, not the host counter frequency? That is, I would have
expected that we get this value via doing a KVM ONE_REG ioctl
to ask the kernel what the guest cntfrq is. Are we using
the host value on the assumption that the guest might have
misprogrammed their copy of the register?

thanks
-- PMM

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

[Andrea Bolognani]

On Tue, 2019-12-10 at 15:33 +0100, Andrew Jones wrote:
> On Tue, Dec 10, 2019 at 03:21:02PM +0100, Andrea Bolognani wrote:
> > I agree with everything except the naming: why would
> > 
> >   kvm-no-adjvtime=off  vtime is adjusted      (default)
> >   kvm-no-adjvtime=on   vtime is not adjusted
> > 
> > be better than
> > 
> >   kvm-adjvtime=on   vtime is adjusted      (default)
> >   kvm-adjvtime=off  vtime is not adjusted
> > 
> > ? Both offer the exact same amount of flexibility, but the latter has
> > the advantage of not introducing any unwieldy double negatives.
> 
> A default of 'off' == 0 means not setting anything at all. There's
> already precedent for 'kvm-no*' prefixed cpu features,
> 
> kvm-no-smi-migration
> kvm-nopiodelay

Sorry, I'm not sure I understand. Do you mean that the array where
you store CPU features is 0-inizialized, so it's more convenient to
have off (0) as the default because it means you don't have to touch
it beforehand? Or something different?

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH v1 3/5] target/arm/kvm: Implement cpu feature kvm-adjvtime

[Peter Maydell]

On Wed, 16 Oct 2019 at 15:34, Andrew Jones <drjones@redhat.com> wrote:
>
> When kvm-adjvtime is enabled the guest's cntvct[_el0] won't count
> the time when the VM is stopped. That time is skipped by updating
> cntvoff[_el2] on each transition to vm_running using the current
> QEMU_CLOCK_VIRTUAL time. QEMU_CLOCK_VIRTUAL only ticks when the VM
> is running.
>
> This patch only provides the implementation. A subsequent patch
> will provide the CPU property allowing the feature to be enabled.


> +void kvm_arm_set_virtual_time(CPUState *cs)
> +{
> +    uint64_t cnt;
> +    struct kvm_one_reg reg = {
> +        .id = KVM_REG_ARM_TIMER_CNT,
> +        .addr = (uintptr_t)&cnt,
> +    };
> +    int ret;
> +
> +    cnt = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> +                   cpu_get_host_tick_frequency(),
> +                   NANOSECONDS_PER_SECOND);
> +
> +    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> +    if (ret) {
> +        error_report("Failed to set KVM_REG_ARM_TIMER_CNT");
> +        abort();
> +    }

The commit message (and the doc comment for this function)
say that we're updating the counter offset, but the
kvm_one_reg operation here is updating the timer count
(and relying on the kernel's handling of "if we update
the timer count implement that by changing the offset").
That seems a bit confusing.

Would it be possible to implement "cntvct should not change while the
VM is stopped" with "read cntvct when the VM stops, and just write
back that value when the VM is restarted", rather than
"write back a new value calculated from QEMU_CLOCK_VIRTUAL"?
If I understand commit 00f4d64ee76e873be8 correctly, that's
basically how x86 is doing it. It would also let you sidestep
the need to know the tick frequency of the counter.

thanks
-- PMM

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

[Andrew Jones]

On Tue, Dec 10, 2019 at 04:47:49PM +0100, Andrea Bolognani wrote:
> On Tue, 2019-12-10 at 15:33 +0100, Andrew Jones wrote:
> > On Tue, Dec 10, 2019 at 03:21:02PM +0100, Andrea Bolognani wrote:
> > > I agree with everything except the naming: why would
> > > 
> > >   kvm-no-adjvtime=off  vtime is adjusted      (default)
> > >   kvm-no-adjvtime=on   vtime is not adjusted
> > > 
> > > be better than
> > > 
> > >   kvm-adjvtime=on   vtime is adjusted      (default)
> > >   kvm-adjvtime=off  vtime is not adjusted
> > > 
> > > ? Both offer the exact same amount of flexibility, but the latter has
> > > the advantage of not introducing any unwieldy double negatives.
> > 
> > A default of 'off' == 0 means not setting anything at all. There's
> > already precedent for 'kvm-no*' prefixed cpu features,
> > 
> > kvm-no-smi-migration
> > kvm-nopiodelay
> 
> Sorry, I'm not sure I understand. Do you mean that the array where
> you store CPU features is 0-inizialized, so it's more convenient to
> have off (0) as the default because it means you don't have to touch
> it beforehand? Or something different?
>

Right. The CPU feature flag (a boolean member of the CPU state) will
be zero by default because C. It's not a big deal, though, because the
property default can easily be set to true while it's added to a cpu
type.

I don't have a strong enough opinion about kvm-adjvtime vs.
kvm-no-adjvtime to insist one way or another. I agree double inversions
are easier to mess up, but I also like the way the '-no-' better
communicates that the default is [probably] 'yes'.

All interested parties, please vote. I'll be sending v2 soon and I can
call this thing anything the majority (or the dominate minority) prefer.

Thanks,
drew

Re: [PATCH v1 3/5] target/arm/kvm: Implement cpu feature kvm-adjvtime

[Andrew Jones]

On Tue, Dec 10, 2019 at 03:54:11PM +0000, Peter Maydell wrote:
> On Wed, 16 Oct 2019 at 15:34, Andrew Jones <drjones@redhat.com> wrote:
> >
> > When kvm-adjvtime is enabled the guest's cntvct[_el0] won't count
> > the time when the VM is stopped. That time is skipped by updating
> > cntvoff[_el2] on each transition to vm_running using the current
> > QEMU_CLOCK_VIRTUAL time. QEMU_CLOCK_VIRTUAL only ticks when the VM
> > is running.
> >
> > This patch only provides the implementation. A subsequent patch
> > will provide the CPU property allowing the feature to be enabled.
> 
> 
> > +void kvm_arm_set_virtual_time(CPUState *cs)
> > +{
> > +    uint64_t cnt;
> > +    struct kvm_one_reg reg = {
> > +        .id = KVM_REG_ARM_TIMER_CNT,
> > +        .addr = (uintptr_t)&cnt,
> > +    };
> > +    int ret;
> > +
> > +    cnt = muldiv64(qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL),
> > +                   cpu_get_host_tick_frequency(),
> > +                   NANOSECONDS_PER_SECOND);
> > +
> > +    ret = kvm_vcpu_ioctl(cs, KVM_SET_ONE_REG, &reg);
> > +    if (ret) {
> > +        error_report("Failed to set KVM_REG_ARM_TIMER_CNT");
> > +        abort();
> > +    }
> 
> The commit message (and the doc comment for this function)
> say that we're updating the counter offset, but the
> kvm_one_reg operation here is updating the timer count
> (and relying on the kernel's handling of "if we update
> the timer count implement that by changing the offset").
> That seems a bit confusing.
> 
> Would it be possible to implement "cntvct should not change while the
> VM is stopped" with "read cntvct when the VM stops, and just write
> back that value when the VM is restarted", rather than
> "write back a new value calculated from QEMU_CLOCK_VIRTUAL"?
> If I understand commit 00f4d64ee76e873be8 correctly, that's
> basically how x86 is doing it. It would also let you sidestep
> the need to know the tick frequency of the counter.

That's definitely worth some experimenting. Will do.

Thanks,
drew

Re: [PATCH v1 2/5] timer: arm: Introduce functions to get the host cntfrq

[Andrew Jones]

On Tue, Dec 10, 2019 at 03:47:39PM +0000, Peter Maydell wrote:
> On Wed, 16 Oct 2019 at 15:34, Andrew Jones <drjones@redhat.com> wrote:
> >
> > When acceleration like KVM is in use it's necessary to use the host's
> > counter frequency when converting ticks to or from time units.
> >
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >  include/qemu/timer.h | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/include/qemu/timer.h b/include/qemu/timer.h
> > index 85bc6eb00b21..8941ddea8242 100644
> > --- a/include/qemu/timer.h
> > +++ b/include/qemu/timer.h
> > @@ -1006,6 +1006,22 @@ static inline int64_t cpu_get_host_ticks(void)
> >  }
> >  #endif
> >
> > +#if defined(__aarch64__)
> > +static inline uint32_t cpu_get_host_tick_frequency(void)
> > +{
> > +    uint64_t frq;
> > +    asm volatile("mrs %0, cntfrq_el0" : "=r" (frq));
> > +    return frq;
> > +}
> > +#elif defined(__arm__)
> > +static inline uint32_t cpu_get_host_tick_frequency(void)
> > +{
> > +    uint32_t frq;
> > +    asm volatile("mrc p15, 0, %0, c14, c0, 0" : "=r" (frq));
> > +    return frq;
> > +}
> > +#endif
> 
> Don't we want to know what the guest counter frequency
> is, not the host counter frequency? That is, I would have
> expected that we get this value via doing a KVM ONE_REG ioctl
> to ask the kernel what the guest cntfrq is. Are we using
> the host value on the assumption that the guest might have
> misprogrammed their copy of the register?
>

Indeed it would be best to get whatever KVM is using for the given VCPU's
CNTFRQ through GET_ONE_REG, but KVM doesn't seem to allow it. I hadn't
tested before, but to be sure, I did now with the following kselftests
test

 #include "kvm_util.h"
 #include "processor.h"
 
 static void guest_code(void) { }
 
 int main(void)
 {
   struct kvm_vm *vm = vm_create_default(0, 0, guest_code);
   uint64_t reg;
 
   get_reg(vm, 0, ARM64_SYS_REG(3, 3, 14, 0, 0), &reg);
   printf("%lx\n", reg);
   return 0;
 }

The vcpu ioctl fails with ENOENT. Currently KVM requires all guests to
have the same frequency as the host, but I guess that will change
eventually. It's definitely best to do the save/restore thing, as you
suggest.

Thanks,
drew

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

[Andrea Bolognani]

On Tue, 2019-12-10 at 17:08 +0100, Andrew Jones wrote:
> I don't have a strong enough opinion about kvm-adjvtime vs.
> kvm-no-adjvtime to insist one way or another. I agree double inversions
> are easier to mess up, but I also like the way the '-no-' better
> communicates that the default is [probably] 'yes'.
> 
> All interested parties, please vote. I'll be sending v2 soon and I can
> call this thing anything the majority (or the dominate minority) prefer.

I like kvm-adjvtime better because it avoids the double negative,
but on the other hand if the new default is be to adjust the time
then I don't expect many people will actually need to use the
parameter, so the name doesn't matter that much after all :)

-- 
Andrea Bolognani / Red Hat / Virtualization

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

[Guoheyi]

在 2019/12/6 23:22, Peter Maydell 写道:
> On Wed, 16 Oct 2019 at 15:34, Andrew Jones <drjones@redhat.com> 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.
> So I guess my overall question is "what is the x86 solution to
> this problem, and why is this all arm-specific?" It would also
> be helpful to know how it fits into all the other proposals regarding
> time in VMs.

I also sent a RFC in March and ARM KVM experts were also invoved in the 
discussion:

https://lists.cs.columbia.edu/pipermail/kvmarm/2019-March/035026.html

According to the discussion, qemu on x86 is using KVM_KVMCLOCK_CTRL to 
request KVM to set a flag "PVCLOCK_GUEST_STOPPED" in pvclock, informing 
VM kernel about external force stop.

Thanks,

Heyi


>
> thanks
> -- PMM
>
>

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

[Andrew Jones]

On Wed, Dec 11, 2019 at 04:02:52PM +0800, Guoheyi wrote:
> 
> 在 2019/12/6 23:22, Peter Maydell 写道:
> > On Wed, 16 Oct 2019 at 15:34, Andrew Jones <drjones@redhat.com> 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.
> > So I guess my overall question is "what is the x86 solution to
> > this problem, and why is this all arm-specific?" It would also
> > be helpful to know how it fits into all the other proposals regarding
> > time in VMs.
> 
> I also sent a RFC in March and ARM KVM experts were also invoved in the
> discussion:
> 
> https://lists.cs.columbia.edu/pipermail/kvmarm/2019-March/035026.html
> 
> According to the discussion, qemu on x86 is using KVM_KVMCLOCK_CTRL to
> request KVM to set a flag "PVCLOCK_GUEST_STOPPED" in pvclock, informing VM
> kernel about external force stop.
> 
> Thanks,
> 
> Heyi

Hi Heyi,

Apologies for having forgotten about that thread. I recall now having
followed it last March. Indeed it even addresses the issue in the way
we're coming around to now (save/restore vs. update with virtual time).

I just reread the whole thread, and my feeling is that, while there are
still many issues left to work, until we get a pvclock for arm, a patch
like this one, but with a way to opt-in/out in order to give users a
chance to choose their poison, is the best we can do. Also a patch like
this one is a step in the right direction, as it will be needed as part
of the bigger pvclock solution eventually, just as it is for x86.

One comment on the patch is that I would prefer to do the save/restore
for all VCPUs, even though KVM does currently handle synchronization.
Not only does it "feel" more correct to apply VCPU APIs to all VCPUs,
but I assume it will be less problematic to implement CPU hotplug at
some point.

Thanks,
drew

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

[Guoheyi]

在 2019/12/11 17:00, Andrew Jones 写道:
> On Wed, Dec 11, 2019 at 04:02:52PM +0800, Guoheyi wrote:
>> 在 2019/12/6 23:22, Peter Maydell 写道:
>>> On Wed, 16 Oct 2019 at 15:34, Andrew Jones <drjones@redhat.com> 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.
>>> So I guess my overall question is "what is the x86 solution to
>>> this problem, and why is this all arm-specific?" It would also
>>> be helpful to know how it fits into all the other proposals regarding
>>> time in VMs.
>> I also sent a RFC in March and ARM KVM experts were also invoved in the
>> discussion:
>>
>> https://lists.cs.columbia.edu/pipermail/kvmarm/2019-March/035026.html
>>
>> According to the discussion, qemu on x86 is using KVM_KVMCLOCK_CTRL to
>> request KVM to set a flag "PVCLOCK_GUEST_STOPPED" in pvclock, informing VM
>> kernel about external force stop.
>>
>> Thanks,
>>
>> Heyi
> Hi Heyi,
>
> Apologies for having forgotten about that thread. I recall now having
> followed it last March. Indeed it even addresses the issue in the way
> we're coming around to now (save/restore vs. update with virtual time).

Never mind :) We were also blocked by this issue when trying to support 
VM suspend/resume, save/restore, etc, so I'm happy to see your patches 
that we have the chance to fix it.

>
> I just reread the whole thread, and my feeling is that, while there are
> still many issues left to work, until we get a pvclock for arm, a patch
> like this one, but with a way to opt-in/out in order to give users a
> chance to choose their poison, is the best we can do. Also a patch like
> this one is a step in the right direction, as it will be needed as part
> of the bigger pvclock solution eventually, just as it is for x86.

Yes, it is simple and it works with current version of ARM64 OS 
distributions.

Thanks,

Heyi

>
> One comment on the patch is that I would prefer to do the save/restore
> for all VCPUs, even though KVM does currently handle synchronization.
> Not only does it "feel" more correct to apply VCPU APIs to all VCPUs,
> but I assume it will be less problematic to implement CPU hotplug at
> some point.
>
> Thanks,
> drew
>
>
> .