[RFC PATCH v2 0/5] target/arm/kvm: Adjust virtual time

[RFC PATCH v2 0/5] target/arm/kvm: Adjust virtual time

[Andrew Jones]

v2:
 - Reworked it enough that I brought back the RFC tag and retitled the
   series. Also had to drop r-b's from a couple of patches, and even
   drop patches.
 - Changed approach from writing the QEMU virtual time to the guest
   vtime counter to saving and restoring the guest vtime counter.
 - Changed the kvm-adjvtime property, which was off by default, to a
   kvm-no-adjvtime property, which is also off by default, meaning the
   effective "adjust vtime" property is now on by default (but only
   for 5.0 virt machine types and later)

v1:
 - 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 over
a year ago and by the patch[2] posted by Heyi Guo almost 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 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 adjustment optional. Besides the adjustment being
optional, this series approaches the needed changes differently to apply
them in more appropriate locations and also integrates some of the
approach posted in [2].

Additional notes
----------------

Note 1
------

As described above, when running a guest with kvm-no-adjtime disabled
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://lists.gnu.org/archive/html/qemu-devel/2019-03/msg03695.html

Thanks,
drew


Andrew Jones (5):
  hw: add compat machines for 5.0
  target/arm/kvm64: kvm64 cpus have timer registers
  target/arm/kvm: Implement virtual time adjustment
  tests/arm-cpu-features: Check feature default values
  target/arm/cpu: Add the kvm-no-adjvtime CPU property

 docs/arm-cpu-features.rst  | 31 +++++++++++++++-
 hw/arm/virt.c              | 17 ++++++++-
 hw/core/machine.c          |  3 ++
 hw/i386/pc.c               |  3 ++
 hw/i386/pc_piix.c          | 14 ++++++-
 hw/i386/pc_q35.c           | 13 ++++++-
 hw/ppc/spapr.c             | 15 +++++++-
 hw/s390x/s390-virtio-ccw.c | 15 +++++++-
 include/hw/arm/virt.h      |  1 +
 include/hw/boards.h        |  3 ++
 include/hw/i386/pc.h       |  3 ++
 target/arm/cpu.c           |  2 +
 target/arm/cpu.h           |  9 +++++
 target/arm/cpu64.c         |  1 +
 target/arm/kvm.c           | 76 ++++++++++++++++++++++++++++++++++++++
 target/arm/kvm32.c         |  3 ++
 target/arm/kvm64.c         |  4 ++
 target/arm/kvm_arm.h       | 34 +++++++++++++++++
 target/arm/monitor.c       |  1 +
 tests/arm-cpu-features.c   | 48 +++++++++++++++++++-----
 20 files changed, 280 insertions(+), 16 deletions(-)

-- 
2.21.0

[RFC PATCH v2 1/5] hw: add compat machines for 5.0

[Andrew Jones]

Add 5.0 machine types for arm/i440fx/q35/s390x/spapr.

Signed-off-by: Andrew Jones <drjones@redhat.com>

---

Hi Eduardo,

If we need to do something special for i440fx and q35, as
9aec2e52ce9d ("hw: add compat machines for 4.2") implies, then
I'll need guidance as to what.
---
 hw/arm/virt.c              |  9 ++++++++-
 hw/core/machine.c          |  3 +++
 hw/i386/pc.c               |  3 +++
 hw/i386/pc_piix.c          | 14 +++++++++++++-
 hw/i386/pc_q35.c           | 13 ++++++++++++-
 hw/ppc/spapr.c             | 15 +++++++++++++--
 hw/s390x/s390-virtio-ccw.c | 15 ++++++++++++++-
 include/hw/boards.h        |  3 +++
 include/hw/i386/pc.h       |  3 +++
 9 files changed, 72 insertions(+), 6 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index d4bedc260712..cb7041e9677a 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2147,10 +2147,17 @@ static void machvirt_machine_init(void)
 }
 type_init(machvirt_machine_init);
 
+static void virt_machine_5_0_options(MachineClass *mc)
+{
+}
+DEFINE_VIRT_MACHINE_AS_LATEST(5, 0)
+
 static void virt_machine_4_2_options(MachineClass *mc)
 {
+    virt_machine_5_0_options(mc);
+    compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
 }
-DEFINE_VIRT_MACHINE_AS_LATEST(4, 2)
+DEFINE_VIRT_MACHINE(4, 2)
 
 static void virt_machine_4_1_options(MachineClass *mc)
 {
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 1689ad3bf8af..21fe2d974817 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -27,6 +27,9 @@
 #include "hw/pci/pci.h"
 #include "hw/mem/nvdimm.h"
 
+GlobalProperty hw_compat_4_2[] = {};
+const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2);
+
 GlobalProperty hw_compat_4_1[] = {
     { "virtio-pci", "x-pcie-flr-init", "off" },
 };
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index ac08e6360437..58867f987d88 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -103,6 +103,9 @@
 
 struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
 
+GlobalProperty pc_compat_4_2[] = {};
+const size_t pc_compat_4_2_len = G_N_ELEMENTS(pc_compat_4_2);
+
 GlobalProperty pc_compat_4_1[] = {};
 const size_t pc_compat_4_1_len = G_N_ELEMENTS(pc_compat_4_1);
 
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index 1bd70d1abbc4..aa2c6147a7ea 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -424,7 +424,7 @@ static void pc_i440fx_machine_options(MachineClass *m)
     machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
 }
 
-static void pc_i440fx_4_2_machine_options(MachineClass *m)
+static void pc_i440fx_5_0_machine_options(MachineClass *m)
 {
     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_i440fx_machine_options(m);
@@ -433,6 +433,18 @@ static void pc_i440fx_4_2_machine_options(MachineClass *m)
     pcmc->default_cpu_version = 1;
 }
 
+DEFINE_I440FX_MACHINE(v5_0, "pc-i440fx-5.0", NULL,
+                      pc_i440fx_5_0_machine_options)
+
+static void pc_i440fx_4_2_machine_options(MachineClass *m)
+{
+    pc_i440fx_5_0_machine_options(m);
+    m->alias = NULL;
+    m->is_default = 0;
+    compat_props_add(m->compat_props, hw_compat_4_2, hw_compat_4_2_len);
+    compat_props_add(m->compat_props, pc_compat_4_2, pc_compat_4_2_len);
+}
+
 DEFINE_I440FX_MACHINE(v4_2, "pc-i440fx-4.2", NULL,
                       pc_i440fx_4_2_machine_options);
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index 385e5cffb167..ddd485d608c0 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -348,7 +348,7 @@ static void pc_q35_machine_options(MachineClass *m)
     m->max_cpus = 288;
 }
 
-static void pc_q35_4_2_machine_options(MachineClass *m)
+static void pc_q35_5_0_machine_options(MachineClass *m)
 {
     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_q35_machine_options(m);
@@ -356,6 +356,17 @@ static void pc_q35_4_2_machine_options(MachineClass *m)
     pcmc->default_cpu_version = 1;
 }
 
+DEFINE_Q35_MACHINE(v5_0, "pc-q35-5.0", NULL,
+                   pc_q35_5_0_machine_options);
+
+static void pc_q35_4_2_machine_options(MachineClass *m)
+{
+    pc_q35_5_0_machine_options(m);
+    m->alias = NULL;
+    compat_props_add(m->compat_props, hw_compat_4_2, hw_compat_4_2_len);
+    compat_props_add(m->compat_props, pc_compat_4_2, pc_compat_4_2_len);
+}
+
 DEFINE_Q35_MACHINE(v4_2, "pc-q35-4.2", NULL,
                    pc_q35_4_2_machine_options);
 
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index e076f6023c73..3ae7db156303 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4491,15 +4491,26 @@ static const TypeInfo spapr_machine_info = {
     }                                                                \
     type_init(spapr_machine_register_##suffix)
 
+/*
+ * pseries-5.0
+ */
+static void spapr_machine_5_0_class_options(MachineClass *mc)
+{
+    /* Defaults for the latest behaviour inherited from the base class */
+}
+
+DEFINE_SPAPR_MACHINE(5_0, "5.0", true);
+
 /*
  * pseries-4.2
  */
 static void spapr_machine_4_2_class_options(MachineClass *mc)
 {
-    /* Defaults for the latest behaviour inherited from the base class */
+    spapr_machine_5_0_class_options(mc);
+    compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
 }
 
-DEFINE_SPAPR_MACHINE(4_2, "4.2", true);
+DEFINE_SPAPR_MACHINE(4_2, "4.2", false);
 
 /*
  * pseries-4.1
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index d3edeef0ad92..a40f79e20733 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -639,14 +639,27 @@ bool css_migration_enabled(void)
     }                                                                         \
     type_init(ccw_machine_register_##suffix)
 
+
+static void ccw_machine_5_0_instance_options(MachineState *machine)
+{
+}
+
+static void ccw_machine_5_0_class_options(MachineClass *mc)
+{
+}
+DEFINE_CCW_MACHINE(5_0, "5.0", true);
+
 static void ccw_machine_4_2_instance_options(MachineState *machine)
 {
+    ccw_machine_5_0_instance_options(machine);
 }
 
 static void ccw_machine_4_2_class_options(MachineClass *mc)
 {
+    ccw_machine_5_0_class_options(mc);
+    compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
 }
-DEFINE_CCW_MACHINE(4_2, "4.2", true);
+DEFINE_CCW_MACHINE(4_2, "4.2", false);
 
 static void ccw_machine_4_1_instance_options(MachineState *machine)
 {
diff --git a/include/hw/boards.h b/include/hw/boards.h
index de45087f34cb..24cbeecbaecc 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -329,6 +329,9 @@ struct MachineState {
     } \
     type_init(machine_initfn##_register_types)
 
+extern GlobalProperty hw_compat_4_2[];
+extern const size_t hw_compat_4_2_len;
+
 extern GlobalProperty hw_compat_4_1[];
 extern const size_t hw_compat_4_1_len;
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 1f86eba3f998..61a998de4665 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -237,6 +237,9 @@ void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
 void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
                        const CPUArchIdList *apic_ids, GArray *entry);
 
+extern GlobalProperty pc_compat_4_2[];
+extern const size_t pc_compat_4_2_len;
+
 extern GlobalProperty pc_compat_4_1[];
 extern const size_t pc_compat_4_1_len;
 
-- 
2.21.0

[RFC PATCH v2 2/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 876184b8fe4d..5cafcb7d36dd 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

[RFC PATCH v2 3/5] target/arm/kvm: Implement virtual time adjustment

[Andrew Jones]

When a VM is stopped (guest is paused) guest virtual time
should stop counting. Otherwise, when the VM is resumed it
will experience time jumps and its kernel may report soft
lockups. Not counting virtual time while the VM is stopped
has the side effect of making the guest's time appear to lag
when compared with real time, and even with time derived from
the physical counter. For this reason, this change, which is
enabled by default, comes with a KVM CPU feature allowing it
to be disabled, restoring legacy behavior.

This patch only provides the implementation of the virtual
time adjustment. A subsequent patch will provide the CPU
property allowing the change to be enabled and disabled.

Reported-by: Bijan Mottahedeh <bijan.mottahedeh@oracle.com>
Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 target/arm/cpu.h     |  9 +++++++++
 target/arm/kvm.c     | 48 ++++++++++++++++++++++++++++++++++++++++++++
 target/arm/kvm32.c   |  3 +++
 target/arm/kvm64.c   |  3 +++
 target/arm/kvm_arm.h | 23 +++++++++++++++++++++
 5 files changed, 86 insertions(+)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 83a809d4bac4..a79ea74125b3 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -821,6 +821,15 @@ struct ARMCPU {
     /* KVM init features for this CPU */
     uint32_t kvm_init_features[7];
 
+    /* KVM CPU features */
+    bool kvm_adjvtime;
+
+    /* VCPU virtual counter value used with kvm_adjvtime */
+    uint64_t kvm_vtime;
+
+    /* True if the run state is, or transitioning from, RUN_STATE_PAUSED */
+    bool runstate_paused;
+
     /* Uniprocessor system with MP extensions */
     bool mp_is_up;
 
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 5b82cefef608..a55fe7d7aefd 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -348,6 +348,24 @@ 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 && cpu->runstate_paused) {
+            kvm_arm_set_virtual_time(cs, cpu->kvm_vtime);
+        }
+        cpu->runstate_paused = false;
+    } else if (state == RUN_STATE_PAUSED) {
+        cpu->runstate_paused = true;
+        if (cpu->kvm_adjvtime) {
+            kvm_arm_get_virtual_time(cs, &cpu->kvm_vtime);
+        }
+    }
+}
+
 static int compare_u64(const void *a, const void *b)
 {
     if (*(uint64_t *)a > *(uint64_t *)b) {
@@ -579,6 +597,36 @@ int kvm_arm_sync_mpstate_to_qemu(ARMCPU *cpu)
     return 0;
 }
 
+void kvm_arm_get_virtual_time(CPUState *cs, uint64_t *cnt)
+{
+    struct kvm_one_reg reg = {
+        .id = KVM_REG_ARM_TIMER_CNT,
+        .addr = (uintptr_t)cnt,
+    };
+    int ret;
+
+    ret = kvm_vcpu_ioctl(cs, KVM_GET_ONE_REG, &reg);
+    if (ret) {
+        error_report("Failed to get KVM_REG_ARM_TIMER_CNT");
+        abort();
+    }
+}
+
+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;
+
+    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 5cafcb7d36dd..e486eaf1f944 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..16b53e45377d 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -232,6 +232,24 @@ void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map);
  */
 void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu);
 
+/**
+ * void kvm_arm_get_virtual_time:
+ * @cs: CPUState
+ * @cnt: the virtual counter to fill in
+ *
+ * Gets the VCPU's virtual counter and stores it in @cnt.
+ */
+void kvm_arm_get_virtual_time(CPUState *cs, uint64_t *cnt);
+
+/**
+ * void kvm_arm_set_virtual_time:
+ * @cs: CPUState
+ * @cnt: new virtual counter value
+ *
+ * Sets the VCPU's virtual counter to @cnt.
+ */
+void kvm_arm_set_virtual_time(CPUState *cs, uint64_t cnt);
+
 /**
  * kvm_arm_aarch32_supported:
  * @cs: CPUState
@@ -288,6 +306,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 +344,9 @@ static inline int kvm_arm_vgic_probe(void)
     return 0;
 }
 
+static inline void kvm_arm_get_virtual_time(CPUState *cs, uint64_t *cnt) {}
+static inline void kvm_arm_set_virtual_time(CPUState *cs, uint64_t cnt) {}
+
 static inline void kvm_arm_pmu_set_irq(CPUState *cs, int irq) {}
 static inline void kvm_arm_pmu_init(CPUState *cs) {}
 
-- 
2.21.0

[RFC PATCH v2 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 6e99aa951e74..06ed05e934e6 100644
--- a/tests/arm-cpu-features.c
+++ b/tests/arm-cpu-features.c
@@ -159,6 +159,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;
@@ -405,16 +431,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");
 
@@ -451,8 +477,8 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
         QDict *resp;
         char *error;
 
-        assert_has_feature(qts, "host", "aarch64");
-        assert_has_feature(qts, "host", "pmu");
+        assert_has_feature_enabled(qts, "host", "aarch64");
+        assert_has_feature_enabled(qts, "host", "pmu");
 
         assert_error(qts, "cortex-a15",
             "We cannot guarantee the CPU type 'cortex-a15' works "
-- 
2.21.0

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

[Andrew Jones]

kvm-no-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>
---
 docs/arm-cpu-features.rst | 31 ++++++++++++++++++++++++++++++-
 hw/arm/virt.c             |  8 ++++++++
 include/hw/arm/virt.h     |  1 +
 target/arm/cpu.c          |  2 ++
 target/arm/cpu64.c        |  1 +
 target/arm/kvm.c          | 28 ++++++++++++++++++++++++++++
 target/arm/kvm_arm.h      | 11 +++++++++++
 target/arm/monitor.c      |  1 +
 tests/arm-cpu-features.c  |  4 ++++
 9 files changed, 86 insertions(+), 1 deletion(-)

diff --git a/docs/arm-cpu-features.rst b/docs/arm-cpu-features.rst
index 1b367e22e16e..641ec9cb8f4a 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,33 @@ 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-no-adjvtime          When disabled, each time the VM transitions
+                           back to running state from the paused state the
+                           VCPU's vitual counter is updated to ensure the
+                           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.  Enabling this KVM VCPU
+                           feature provides the legacy behavior, which is
+                           to also count stopped time with the virtual
+                           counter.
+
 SVE CPU Properties
 ==================
 
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index cb7041e9677a..e9a08eb883bf 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1664,6 +1664,11 @@ static void machvirt_init(MachineState *machine)
             }
         }
 
+        if (vmc->kvm_no_adjvtime &&
+            object_property_find(cpuobj, "kvm-no-adjvtime", NULL)) {
+            object_property_set_bool(cpuobj, true, "kvm-no-adjvtime", NULL);
+        }
+
         if (vmc->no_pmu && object_property_find(cpuobj, "pmu", NULL)) {
             object_property_set_bool(cpuobj, false, "pmu", NULL);
         }
@@ -2154,8 +2159,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(5, 0)
 
 static void virt_machine_4_2_options(MachineClass *mc)
 {
+    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
+
     virt_machine_5_0_options(mc);
     compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
+    vmc->kvm_no_adjvtime = true;
 }
 DEFINE_VIRT_MACHINE(4, 2)
 
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index 0b41083e9d5c..80e00161b8f2 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -109,6 +109,7 @@ typedef struct {
     bool smbios_old_sys_ver;
     bool no_highmem_ecam;
     bool no_ged;   /* Machines < 4.2 has no support for ACPI GED device */
+    bool kvm_no_adjvtime;
 } VirtMachineClass;
 
 typedef struct {
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 7a4ac9339bf9..53c73c25a67f 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -2482,6 +2482,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);
 
@@ -2673,6 +2674,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 a39d6fcea34f..3cd416db8089 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -605,6 +605,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 a55fe7d7aefd..7666b250ab96 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,32 @@ void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
     env->features = arm_host_cpu_features.features;
 }
 
+static bool kvm_no_adjvtime_get(Object *obj, Error **errp)
+{
+    return !ARM_CPU(obj)->kvm_adjvtime;
+}
+
+static void kvm_no_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;
+    }
+
+    ARM_CPU(obj)->kvm_adjvtime = true;
+    object_property_add_bool(obj, "kvm-no-adjvtime", kvm_no_adjvtime_get,
+                             kvm_no_adjvtime_set, &error_abort);
+    object_property_set_description(obj, "kvm-no-adjvtime",
+                                    "Set on to disable the adjustment of "
+                                    "the virtual counter. VM stopped time "
+                                    "will 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 16b53e45377d..7d76f26beaca 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_get_virtual_time:
  * @cs: CPUState
@@ -319,6 +328,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..9725dfff16d4 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-no-adjvtime",
     NULL
 };
 
diff --git a/tests/arm-cpu-features.c b/tests/arm-cpu-features.c
index 06ed05e934e6..738e49447377 100644
--- a/tests/arm-cpu-features.c
+++ b/tests/arm-cpu-features.c
@@ -435,6 +435,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-no-adjvtime");
+
     if (g_str_equal(qtest_get_arch(), "aarch64")) {
         assert_has_feature_enabled(qts, "max", "aarch64");
         assert_has_feature_enabled(qts, "max", "sve");
@@ -469,6 +471,8 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
         return;
     }
 
+    assert_has_feature_disabled(qts, "host", "kvm-no-adjvtime");
+
     if (g_str_equal(qtest_get_arch(), "aarch64")) {
         bool kvm_supports_sve;
         char max_name[8], name[8];
-- 
2.21.0

Re: [RFC PATCH v2 1/5] hw: add compat machines for 5.0

[David Hildenbrand]

On 12.12.19 18:33, Andrew Jones wrote:
> Add 5.0 machine types for arm/i440fx/q35/s390x/spapr.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> 

[...]

>  /*
>   * pseries-4.1
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index d3edeef0ad92..a40f79e20733 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -639,14 +639,27 @@ bool css_migration_enabled(void)
>      }                                                                         \
>      type_init(ccw_machine_register_##suffix)
>  
> +
> +static void ccw_machine_5_0_instance_options(MachineState *machine)
> +{
> +}
> +
> +static void ccw_machine_5_0_class_options(MachineClass *mc)
> +{
> +}
> +DEFINE_CCW_MACHINE(5_0, "5.0", true);
> +
>  static void ccw_machine_4_2_instance_options(MachineState *machine)
>  {
> +    ccw_machine_5_0_instance_options(machine);
>  }
>  
>  static void ccw_machine_4_2_class_options(MachineClass *mc)
>  {
> +    ccw_machine_5_0_class_options(mc);
> +    compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
>  }
> -DEFINE_CCW_MACHINE(4_2, "4.2", true);
> +DEFINE_CCW_MACHINE(4_2, "4.2", false);
>  


s390x parts LGTM

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [RFC PATCH v2 1/5] hw: add compat machines for 5.0

[Eduardo Habkost]

On Thu, Dec 12, 2019 at 06:33:16PM +0100, Andrew Jones wrote:
> Add 5.0 machine types for arm/i440fx/q35/s390x/spapr.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> 
> ---
> 
> Hi Eduardo,
> 
> If we need to do something special for i440fx and q35, as
> 9aec2e52ce9d ("hw: add compat machines for 4.2") implies, then
> I'll need guidance as to what.

Keeping default_cpu_version==1 in pc-*-5.0 (like you did) is
correct.

However, you might want to use Cornelia's patch (which is
probably already queued in the s390 tree) instead:
https://patchew.org/QEMU/20191112104811.30323-1-cohuck@redhat.com

-- 
Eduardo

Re: [RFC PATCH v2 1/5] hw: add compat machines for 5.0

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 8201 bytes --]

On Thu, Dec 12, 2019 at 06:33:16PM +0100, Andrew Jones wrote:
> Add 5.0 machine types for arm/i440fx/q35/s390x/spapr.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>

ppc parts
Acked-by: David Gibson <david@gibson.dropbear.id.au>

> 
> ---
> 
> Hi Eduardo,
> 
> If we need to do something special for i440fx and q35, as
> 9aec2e52ce9d ("hw: add compat machines for 4.2") implies, then
> I'll need guidance as to what.
> ---
>  hw/arm/virt.c              |  9 ++++++++-
>  hw/core/machine.c          |  3 +++
>  hw/i386/pc.c               |  3 +++
>  hw/i386/pc_piix.c          | 14 +++++++++++++-
>  hw/i386/pc_q35.c           | 13 ++++++++++++-
>  hw/ppc/spapr.c             | 15 +++++++++++++--
>  hw/s390x/s390-virtio-ccw.c | 15 ++++++++++++++-
>  include/hw/boards.h        |  3 +++
>  include/hw/i386/pc.h       |  3 +++
>  9 files changed, 72 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index d4bedc260712..cb7041e9677a 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2147,10 +2147,17 @@ static void machvirt_machine_init(void)
>  }
>  type_init(machvirt_machine_init);
>  
> +static void virt_machine_5_0_options(MachineClass *mc)
> +{
> +}
> +DEFINE_VIRT_MACHINE_AS_LATEST(5, 0)
> +
>  static void virt_machine_4_2_options(MachineClass *mc)
>  {
> +    virt_machine_5_0_options(mc);
> +    compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
>  }
> -DEFINE_VIRT_MACHINE_AS_LATEST(4, 2)
> +DEFINE_VIRT_MACHINE(4, 2)
>  
>  static void virt_machine_4_1_options(MachineClass *mc)
>  {
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 1689ad3bf8af..21fe2d974817 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -27,6 +27,9 @@
>  #include "hw/pci/pci.h"
>  #include "hw/mem/nvdimm.h"
>  
> +GlobalProperty hw_compat_4_2[] = {};
> +const size_t hw_compat_4_2_len = G_N_ELEMENTS(hw_compat_4_2);
> +
>  GlobalProperty hw_compat_4_1[] = {
>      { "virtio-pci", "x-pcie-flr-init", "off" },
>  };
> diff --git a/hw/i386/pc.c b/hw/i386/pc.c
> index ac08e6360437..58867f987d88 100644
> --- a/hw/i386/pc.c
> +++ b/hw/i386/pc.c
> @@ -103,6 +103,9 @@
>  
>  struct hpet_fw_config hpet_cfg = {.count = UINT8_MAX};
>  
> +GlobalProperty pc_compat_4_2[] = {};
> +const size_t pc_compat_4_2_len = G_N_ELEMENTS(pc_compat_4_2);
> +
>  GlobalProperty pc_compat_4_1[] = {};
>  const size_t pc_compat_4_1_len = G_N_ELEMENTS(pc_compat_4_1);
>  
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index 1bd70d1abbc4..aa2c6147a7ea 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -424,7 +424,7 @@ static void pc_i440fx_machine_options(MachineClass *m)
>      machine_class_allow_dynamic_sysbus_dev(m, TYPE_RAMFB_DEVICE);
>  }
>  
> -static void pc_i440fx_4_2_machine_options(MachineClass *m)
> +static void pc_i440fx_5_0_machine_options(MachineClass *m)
>  {
>      PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>      pc_i440fx_machine_options(m);
> @@ -433,6 +433,18 @@ static void pc_i440fx_4_2_machine_options(MachineClass *m)
>      pcmc->default_cpu_version = 1;
>  }
>  
> +DEFINE_I440FX_MACHINE(v5_0, "pc-i440fx-5.0", NULL,
> +                      pc_i440fx_5_0_machine_options)
> +
> +static void pc_i440fx_4_2_machine_options(MachineClass *m)
> +{
> +    pc_i440fx_5_0_machine_options(m);
> +    m->alias = NULL;
> +    m->is_default = 0;
> +    compat_props_add(m->compat_props, hw_compat_4_2, hw_compat_4_2_len);
> +    compat_props_add(m->compat_props, pc_compat_4_2, pc_compat_4_2_len);
> +}
> +
>  DEFINE_I440FX_MACHINE(v4_2, "pc-i440fx-4.2", NULL,
>                        pc_i440fx_4_2_machine_options);
>  
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index 385e5cffb167..ddd485d608c0 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -348,7 +348,7 @@ static void pc_q35_machine_options(MachineClass *m)
>      m->max_cpus = 288;
>  }
>  
> -static void pc_q35_4_2_machine_options(MachineClass *m)
> +static void pc_q35_5_0_machine_options(MachineClass *m)
>  {
>      PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
>      pc_q35_machine_options(m);
> @@ -356,6 +356,17 @@ static void pc_q35_4_2_machine_options(MachineClass *m)
>      pcmc->default_cpu_version = 1;
>  }
>  
> +DEFINE_Q35_MACHINE(v5_0, "pc-q35-5.0", NULL,
> +                   pc_q35_5_0_machine_options);
> +
> +static void pc_q35_4_2_machine_options(MachineClass *m)
> +{
> +    pc_q35_5_0_machine_options(m);
> +    m->alias = NULL;
> +    compat_props_add(m->compat_props, hw_compat_4_2, hw_compat_4_2_len);
> +    compat_props_add(m->compat_props, pc_compat_4_2, pc_compat_4_2_len);
> +}
> +
>  DEFINE_Q35_MACHINE(v4_2, "pc-q35-4.2", NULL,
>                     pc_q35_4_2_machine_options);
>  
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index e076f6023c73..3ae7db156303 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -4491,15 +4491,26 @@ static const TypeInfo spapr_machine_info = {
>      }                                                                \
>      type_init(spapr_machine_register_##suffix)
>  
> +/*
> + * pseries-5.0
> + */
> +static void spapr_machine_5_0_class_options(MachineClass *mc)
> +{
> +    /* Defaults for the latest behaviour inherited from the base class */
> +}
> +
> +DEFINE_SPAPR_MACHINE(5_0, "5.0", true);
> +
>  /*
>   * pseries-4.2
>   */
>  static void spapr_machine_4_2_class_options(MachineClass *mc)
>  {
> -    /* Defaults for the latest behaviour inherited from the base class */
> +    spapr_machine_5_0_class_options(mc);
> +    compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
>  }
>  
> -DEFINE_SPAPR_MACHINE(4_2, "4.2", true);
> +DEFINE_SPAPR_MACHINE(4_2, "4.2", false);
>  
>  /*
>   * pseries-4.1
> diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
> index d3edeef0ad92..a40f79e20733 100644
> --- a/hw/s390x/s390-virtio-ccw.c
> +++ b/hw/s390x/s390-virtio-ccw.c
> @@ -639,14 +639,27 @@ bool css_migration_enabled(void)
>      }                                                                         \
>      type_init(ccw_machine_register_##suffix)
>  
> +
> +static void ccw_machine_5_0_instance_options(MachineState *machine)
> +{
> +}
> +
> +static void ccw_machine_5_0_class_options(MachineClass *mc)
> +{
> +}
> +DEFINE_CCW_MACHINE(5_0, "5.0", true);
> +
>  static void ccw_machine_4_2_instance_options(MachineState *machine)
>  {
> +    ccw_machine_5_0_instance_options(machine);
>  }
>  
>  static void ccw_machine_4_2_class_options(MachineClass *mc)
>  {
> +    ccw_machine_5_0_class_options(mc);
> +    compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
>  }
> -DEFINE_CCW_MACHINE(4_2, "4.2", true);
> +DEFINE_CCW_MACHINE(4_2, "4.2", false);
>  
>  static void ccw_machine_4_1_instance_options(MachineState *machine)
>  {
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index de45087f34cb..24cbeecbaecc 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -329,6 +329,9 @@ struct MachineState {
>      } \
>      type_init(machine_initfn##_register_types)
>  
> +extern GlobalProperty hw_compat_4_2[];
> +extern const size_t hw_compat_4_2_len;
> +
>  extern GlobalProperty hw_compat_4_1[];
>  extern const size_t hw_compat_4_1_len;
>  
> diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
> index 1f86eba3f998..61a998de4665 100644
> --- a/include/hw/i386/pc.h
> +++ b/include/hw/i386/pc.h
> @@ -237,6 +237,9 @@ void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
>  void pc_madt_cpu_entry(AcpiDeviceIf *adev, int uid,
>                         const CPUArchIdList *apic_ids, GArray *entry);
>  
> +extern GlobalProperty pc_compat_4_2[];
> +extern const size_t pc_compat_4_2_len;
> +
>  extern GlobalProperty pc_compat_4_1[];
>  extern const size_t pc_compat_4_1_len;
>  

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC PATCH v2 1/5] hw: add compat machines for 5.0

[Andrew Jones]

On Thu, Dec 12, 2019 at 04:24:19PM -0300, Eduardo Habkost wrote:
> On Thu, Dec 12, 2019 at 06:33:16PM +0100, Andrew Jones wrote:
> > Add 5.0 machine types for arm/i440fx/q35/s390x/spapr.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > 
> > ---
> > 
> > Hi Eduardo,
> > 
> > If we need to do something special for i440fx and q35, as
> > 9aec2e52ce9d ("hw: add compat machines for 4.2") implies, then
> > I'll need guidance as to what.
> 
> Keeping default_cpu_version==1 in pc-*-5.0 (like you did) is
> correct.
> 
> However, you might want to use Cornelia's patch (which is
> probably already queued in the s390 tree) instead:
> https://patchew.org/QEMU/20191112104811.30323-1-cohuck@redhat.com
>

Drat. I did search the mailing list for a posting from someone else first,
but I made the mistake of searching subjects for 'machine type' rather
than 'compat machines'. Certainly we should use Cornelia's. Mine is
just noise.

Thank you reviewers, and sorry for the duplicated effort.

drew

Re: [RFC PATCH v2 5/5] target/arm/cpu: Add the kvm-no-adjvtime CPU property

[Peter Maydell]

On Thu, 12 Dec 2019 at 17:33, Andrew Jones <drjones@redhat.com> wrote:
>
> kvm-no-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>
> ---
>  docs/arm-cpu-features.rst | 31 ++++++++++++++++++++++++++++++-
>  hw/arm/virt.c             |  8 ++++++++
>  include/hw/arm/virt.h     |  1 +
>  target/arm/cpu.c          |  2 ++
>  target/arm/cpu64.c        |  1 +
>  target/arm/kvm.c          | 28 ++++++++++++++++++++++++++++
>  target/arm/kvm_arm.h      | 11 +++++++++++
>  target/arm/monitor.c      |  1 +
>  tests/arm-cpu-features.c  |  4 ++++
>  9 files changed, 86 insertions(+), 1 deletion(-)
>
> diff --git a/docs/arm-cpu-features.rst b/docs/arm-cpu-features.rst
> index 1b367e22e16e..641ec9cb8f4a 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,33 @@ 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-no-adjvtime          When disabled, each time the VM transitions
> +                           back to running state from the paused state the
> +                           VCPU's vitual counter is updated to ensure the

"virtual"

> +                           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.  Enabling this KVM VCPU
> +                           feature provides the legacy behavior, which is
> +                           to also count stopped time with the virtual
> +                           counter.

This phrasing reads a bit confusingly to me. What I would usually expect
is that you get
  name-of-option              Description of what the option does.

But here we have
  name-of-option              Long description of the default behaviour,
                              taking many lines and several sentences.
                              Brief note at the end that enabling this
                              feature gives the opposite effect.

Especially since the default-behaviour description isn't prefaced
with "By default" or similar, it's quite easy to start reading the
text assuming it's defining what the option is going to do, only
to get to the end and realise that it's defining what the option
is *not* going to do...

Incidentally, if I understand things correctly, for TCG the
behaviour is (and has always been) that VM-stopped time is
not counted, because we run the emulated versions of these counters
off QEMU_CLOCK_VIRTUAL. So having the KVM default be the same as
the TCG default is nicely consistent.

thanks
-- PMM

Re: [RFC PATCH v2 3/5] target/arm/kvm: Implement virtual time adjustment

[Peter Maydell]

On Thu, 12 Dec 2019 at 17:33, Andrew Jones <drjones@redhat.com> wrote:
>
> When a VM is stopped (guest is paused) guest virtual time
> should stop counting. Otherwise, when the VM is resumed it
> will experience time jumps and its kernel may report soft
> lockups. Not counting virtual time while the VM is stopped
> has the side effect of making the guest's time appear to lag
> when compared with real time, and even with time derived from
> the physical counter. For this reason, this change, which is
> enabled by default, comes with a KVM CPU feature allowing it
> to be disabled, restoring legacy behavior.
>
> This patch only provides the implementation of the virtual
> time adjustment. A subsequent patch will provide the CPU
> property allowing the change to be enabled and disabled.
>
> Reported-by: Bijan Mottahedeh <bijan.mottahedeh@oracle.com>
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  target/arm/cpu.h     |  9 +++++++++
>  target/arm/kvm.c     | 48 ++++++++++++++++++++++++++++++++++++++++++++
>  target/arm/kvm32.c   |  3 +++
>  target/arm/kvm64.c   |  3 +++
>  target/arm/kvm_arm.h | 23 +++++++++++++++++++++
>  5 files changed, 86 insertions(+)
>
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 83a809d4bac4..a79ea74125b3 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -821,6 +821,15 @@ struct ARMCPU {
>      /* KVM init features for this CPU */
>      uint32_t kvm_init_features[7];
>
> +    /* KVM CPU features */
> +    bool kvm_adjvtime;
> +
> +    /* VCPU virtual counter value used with kvm_adjvtime */
> +    uint64_t kvm_vtime;

How does this new state interact with migration ?

> +
> +    /* True if the run state is, or transitioning from, RUN_STATE_PAUSED */
> +    bool runstate_paused;
> +
>      /* Uniprocessor system with MP extensions */
>      bool mp_is_up;
>
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 5b82cefef608..a55fe7d7aefd 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -348,6 +348,24 @@ 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 && cpu->runstate_paused) {
> +            kvm_arm_set_virtual_time(cs, cpu->kvm_vtime);
> +        }
> +        cpu->runstate_paused = false;
> +    } else if (state == RUN_STATE_PAUSED) {
> +        cpu->runstate_paused = true;
> +        if (cpu->kvm_adjvtime) {
> +            kvm_arm_get_virtual_time(cs, &cpu->kvm_vtime);
> +        }
> +    }
> +}

How does this interact with the usual register sync to/from
KVM (ie kvm_arch_get_registers(), which I think will do a
GET_ONE_REG read of the TIMER_CNT register the way it does
any other sysreg, inside write_kvmstate_to_list(), plus
kvm_arch_set_registers() which does the write back to the
kernel in write_list_to_kvmstate()) ? Presumably we want this
version to take precedence by the set_virtual_time call
happening after the kvm_arch_set_registers, but is this
guaranteed ?

thanks
-- PMM

Re: [RFC PATCH v2 0/5] target/arm/kvm: Adjust virtual time

[Peter Maydell]

On Thu, 12 Dec 2019 at 17:33, Andrew Jones <drjones@redhat.com> wrote:

> 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.

So, to be clear, you mean that:

(1) the kernel headers say:

/* EL0 Virtual Timer Registers */
#define KVM_REG_ARM_TIMER_CTL           ARM64_SYS_REG(3, 3, 14, 3, 1)
#define KVM_REG_ARM_TIMER_CNT           ARM64_SYS_REG(3, 3, 14, 3, 2)
#define KVM_REG_ARM_TIMER_CVAL          ARM64_SYS_REG(3, 3, 14, 0, 2)

(2) some of the RHSes of these are wrong

(3) but the kernel internally is using the same 'wrong' value, so
userspace also needs to use that value, ie trust the #defined name
rather than manufacturing one ?

That's awkward. I think it would be worth at least having a kernel
patch to add a comment clearly documenting this bug.

(This error seems to only be in the 64-bit ABI, not 32-bit.)

QEMU does assume that the kernel's ID register values match
the hardware for sysregs in some ways -- we use this when we
construct our mapping from KVM register IDs as returned by
KVM_GET_REG_LIST to QEMU cpreg definitions and thus CPUState
struct fields. I *think* that in this case the only visible
effect will be that gdbstub will show you the CNT value
if you ask it to print the value of the CVAL sysreg.

thanks
-- PMM

Re: [RFC PATCH v2 3/5] target/arm/kvm: Implement virtual time adjustment

[Peter Maydell]

On Mon, 16 Dec 2019 at 15:14, Peter Maydell <peter.maydell@linaro.org> wrote:
> How does this interact with the usual register sync to/from
> KVM (ie kvm_arch_get_registers(), which I think will do a
> GET_ONE_REG read of the TIMER_CNT register the way it does
> any other sysreg, inside write_kvmstate_to_list(), plus
> kvm_arch_set_registers() which does the write back to the
> kernel in write_list_to_kvmstate()) ? Presumably we want this
> version to take precedence by the set_virtual_time call
> happening after the kvm_arch_set_registers, but is this
> guaranteed ?

...you might also want to look at the effects of simply
removing the KVM_REG_ARM_TIMER_CNT entry from the
'non_runtime_cpregs[]' array -- in commit 4b7a6bf402bd064
we explicitly stopped reading/writing this register's value
to/from the kernel except for inbound migration, and it
feels like this patchset is now rolling back that approach,
so maybe we should also be (configurably) rolling back some
of its implementation rather than just leaving it in place.

I note also that the commit message there had a remark
about inconsistencies between VCPUs -- is the right thing
to handle this per-VM rather than per-VCPU somehow?

thanks
-- PMM

Re: [RFC PATCH v2 0/5] target/arm/kvm: Adjust virtual time

[Peter Maydell]

On Mon, 16 Dec 2019 at 15:33, Peter Maydell <peter.maydell@linaro.org> wrote:
> So, to be clear, you mean that:
>
> (1) the kernel headers say:
>
> /* EL0 Virtual Timer Registers */
> #define KVM_REG_ARM_TIMER_CTL           ARM64_SYS_REG(3, 3, 14, 3, 1)
> #define KVM_REG_ARM_TIMER_CNT           ARM64_SYS_REG(3, 3, 14, 3, 2)
> #define KVM_REG_ARM_TIMER_CVAL          ARM64_SYS_REG(3, 3, 14, 0, 2)
>
> (2) some of the RHSes of these are wrong
>
> (3) but the kernel internally is using the same 'wrong' value, so
> userspace also needs to use that value, ie trust the #defined name
> rather than manufacturing one ?
>
> That's awkward. I think it would be worth at least having a kernel
> patch to add a comment clearly documenting this bug.
>
> (This error seems to only be in the 64-bit ABI, not 32-bit.)
>
> QEMU does assume that the kernel's ID register values match
> the hardware for sysregs in some ways -- we use this when we
> construct our mapping from KVM register IDs as returned by
> KVM_GET_REG_LIST to QEMU cpreg definitions and thus CPUState
> struct fields. I *think* that in this case the only visible
> effect will be that gdbstub will show you the CNT value
> if you ask it to print the value of the CVAL sysreg.

...perhaps we should work around this kernel bug in the
kvm_to_cpreg_id() and cpreg_to_kvm_id() functions. (Need
to think through/test whether that would break migration.)

thanks
-- PMM

Re: [RFC PATCH v2 0/5] target/arm/kvm: Adjust virtual time

[Marc Zyngier]

On 2019-12-16 15:33, Peter Maydell wrote:
> On Thu, 12 Dec 2019 at 17:33, Andrew Jones <drjones@redhat.com> 
> wrote:
>
>> 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.
>
> So, to be clear, you mean that:
>
> (1) the kernel headers say:
>
> /* EL0 Virtual Timer Registers */
> #define KVM_REG_ARM_TIMER_CTL           ARM64_SYS_REG(3, 3, 14, 3, 1)
> #define KVM_REG_ARM_TIMER_CNT           ARM64_SYS_REG(3, 3, 14, 3, 2)
> #define KVM_REG_ARM_TIMER_CVAL          ARM64_SYS_REG(3, 3, 14, 0, 2)
>
> (2) some of the RHSes of these are wrong
>
> (3) but the kernel internally is using the same 'wrong' value, so
> userspace also needs to use that value, ie trust the #defined name
> rather than manufacturing one ?
>
> That's awkward. I think it would be worth at least having a kernel
> patch to add a comment clearly documenting this bug.
>
> (This error seems to only be in the 64-bit ABI, not 32-bit.)

Yeah, this is pretty bad. I wonder how we managed not to notice
this for so long... :-(.

Andrew, could you please write a patch documenting this (both in
the UAPI headers and in the documentation)?

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC PATCH v2 3/5] target/arm/kvm: Implement virtual time adjustment

[Andrew Jones]

On Mon, Dec 16, 2019 at 03:14:00PM +0000, Peter Maydell wrote:
> On Thu, 12 Dec 2019 at 17:33, Andrew Jones <drjones@redhat.com> wrote:
> >
> > When a VM is stopped (guest is paused) guest virtual time
> > should stop counting. Otherwise, when the VM is resumed it
> > will experience time jumps and its kernel may report soft
> > lockups. Not counting virtual time while the VM is stopped
> > has the side effect of making the guest's time appear to lag
> > when compared with real time, and even with time derived from
> > the physical counter. For this reason, this change, which is
> > enabled by default, comes with a KVM CPU feature allowing it
> > to be disabled, restoring legacy behavior.
> >
> > This patch only provides the implementation of the virtual
> > time adjustment. A subsequent patch will provide the CPU
> > property allowing the change to be enabled and disabled.
> >
> > Reported-by: Bijan Mottahedeh <bijan.mottahedeh@oracle.com>
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  target/arm/cpu.h     |  9 +++++++++
> >  target/arm/kvm.c     | 48 ++++++++++++++++++++++++++++++++++++++++++++
> >  target/arm/kvm32.c   |  3 +++
> >  target/arm/kvm64.c   |  3 +++
> >  target/arm/kvm_arm.h | 23 +++++++++++++++++++++
> >  5 files changed, 86 insertions(+)
> >
> > diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> > index 83a809d4bac4..a79ea74125b3 100644
> > --- a/target/arm/cpu.h
> > +++ b/target/arm/cpu.h
> > @@ -821,6 +821,15 @@ struct ARMCPU {
> >      /* KVM init features for this CPU */
> >      uint32_t kvm_init_features[7];
> >
> > +    /* KVM CPU features */
> > +    bool kvm_adjvtime;
> > +
> > +    /* VCPU virtual counter value used with kvm_adjvtime */
> > +    uint64_t kvm_vtime;
> 
> How does this new state interact with migration ?

I don't think we should need to worry about this state, because migration
will do its own save/restore of the virtual counter, and as that restore
comes later, it'll take precedence. We still need this state for the usual
save/restore when not migrating, though, because KVM_REG_ARM_TIMER_CNT is
a non-runtime cpreg with its level set to KVM_PUT_FULL_STATE.

> 
> > +
> > +    /* True if the run state is, or transitioning from, RUN_STATE_PAUSED */
> > +    bool runstate_paused;
> > +
> >      /* Uniprocessor system with MP extensions */
> >      bool mp_is_up;
> >
> > diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> > index 5b82cefef608..a55fe7d7aefd 100644
> > --- a/target/arm/kvm.c
> > +++ b/target/arm/kvm.c
> > @@ -348,6 +348,24 @@ 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 && cpu->runstate_paused) {
> > +            kvm_arm_set_virtual_time(cs, cpu->kvm_vtime);
> > +        }
> > +        cpu->runstate_paused = false;
> > +    } else if (state == RUN_STATE_PAUSED) {
> > +        cpu->runstate_paused = true;
> > +        if (cpu->kvm_adjvtime) {
> > +            kvm_arm_get_virtual_time(cs, &cpu->kvm_vtime);
> > +        }
> > +    }
> > +}
> 
> How does this interact with the usual register sync to/from
> KVM (ie kvm_arch_get_registers(), which I think will do a
> GET_ONE_REG read of the TIMER_CNT register the way it does
> any other sysreg, inside write_kvmstate_to_list(), plus
> kvm_arch_set_registers() which does the write back to the
> kernel in write_list_to_kvmstate()) ?

It will, but only when level == KVM_PUT_FULL_STATE.


> Presumably we want this
> version to take precedence by the set_virtual_time call
> happening after the kvm_arch_set_registers, but is this
> guaranteed ?

Actually it doesn't really matter which takes precedence (I don't think),
which is why we can rely on the usual save/restore for migration. We only
need the new state this patch adds because we don't have any recent state
otherwise, and because then we can be selective and only do the
save/restore when transitioning to/from paused state.

Thanks,
drew

Re: [RFC PATCH v2 3/5] target/arm/kvm: Implement virtual time adjustment

[Andrew Jones]

On Mon, Dec 16, 2019 at 03:40:16PM +0000, Peter Maydell wrote:
> On Mon, 16 Dec 2019 at 15:14, Peter Maydell <peter.maydell@linaro.org> wrote:
> > How does this interact with the usual register sync to/from
> > KVM (ie kvm_arch_get_registers(), which I think will do a
> > GET_ONE_REG read of the TIMER_CNT register the way it does
> > any other sysreg, inside write_kvmstate_to_list(), plus
> > kvm_arch_set_registers() which does the write back to the
> > kernel in write_list_to_kvmstate()) ? Presumably we want this
> > version to take precedence by the set_virtual_time call
> > happening after the kvm_arch_set_registers, but is this
> > guaranteed ?
> 
> ...you might also want to look at the effects of simply
> removing the KVM_REG_ARM_TIMER_CNT entry from the
> 'non_runtime_cpregs[]' array -- in commit 4b7a6bf402bd064
> we explicitly stopped reading/writing this register's value
> to/from the kernel except for inbound migration, and it
> feels like this patchset is now rolling back that approach,
> so maybe we should also be (configurably) rolling back some
> of its implementation rather than just leaving it in place.

I feel like I already considered that, maybe even tried it, a few months
ago when I first looked at this. I must have decided against it for some
reason at the time, but I don't recall what. Now I can say the reason is
because we only do this save/restore when we transition to/from paused
state, though.

> 
> I note also that the commit message there had a remark
> about inconsistencies between VCPUs -- is the right thing
> to handle this per-VM rather than per-VCPU somehow?

per-VM would make sense, because the counters should be synchronized
among the VCPUs. KVM does that for us, though, so whichever VCPU last
restores its counter is the one that will determine the final value.

Maybe we should have a VM ioctl instead, but ATM we only have VCPU ioctls.

Thanks,
drew

Re: [RFC PATCH v2 5/5] target/arm/cpu: Add the kvm-no-adjvtime CPU property

[Andrew Jones]

On Mon, Dec 16, 2019 at 03:06:57PM +0000, Peter Maydell wrote:
> On Thu, 12 Dec 2019 at 17:33, Andrew Jones <drjones@redhat.com> wrote:
> >
> > kvm-no-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>
> > ---
> >  docs/arm-cpu-features.rst | 31 ++++++++++++++++++++++++++++++-
> >  hw/arm/virt.c             |  8 ++++++++
> >  include/hw/arm/virt.h     |  1 +
> >  target/arm/cpu.c          |  2 ++
> >  target/arm/cpu64.c        |  1 +
> >  target/arm/kvm.c          | 28 ++++++++++++++++++++++++++++
> >  target/arm/kvm_arm.h      | 11 +++++++++++
> >  target/arm/monitor.c      |  1 +
> >  tests/arm-cpu-features.c  |  4 ++++
> >  9 files changed, 86 insertions(+), 1 deletion(-)
> >
> > diff --git a/docs/arm-cpu-features.rst b/docs/arm-cpu-features.rst
> > index 1b367e22e16e..641ec9cb8f4a 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,33 @@ 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-no-adjvtime          When disabled, each time the VM transitions
> > +                           back to running state from the paused state the
> > +                           VCPU's vitual counter is updated to ensure the
> 
> "virtual"
> 
> > +                           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.  Enabling this KVM VCPU
> > +                           feature provides the legacy behavior, which is
> > +                           to also count stopped time with the virtual
> > +                           counter.
> 
> This phrasing reads a bit confusingly to me. What I would usually expect
> is that you get
>   name-of-option              Description of what the option does.
> 
> But here we have
>   name-of-option              Long description of the default behaviour,
>                               taking many lines and several sentences.
>                               Brief note at the end that enabling this
>                               feature gives the opposite effect.
> 
> Especially since the default-behaviour description isn't prefaced
> with "By default" or similar, it's quite easy to start reading the
> text assuming it's defining what the option is going to do, only
> to get to the end and realise that it's defining what the option
> is *not* going to do...

I'll take another stab at this, but my feeling is that a '-no-' option
should be one that just turns off the default behavior, which is why I
wrote a long description of the default behavior. If you'd prefer the
description to be more terse, then I can certainly delete a bunch of
the text, but then I fear what this option disables wouldn't be clear
enough.

> 
> Incidentally, if I understand things correctly, for TCG the
> behaviour is (and has always been) that VM-stopped time is
> not counted, because we run the emulated versions of these counters
> off QEMU_CLOCK_VIRTUAL. So having the KVM default be the same as
> the TCG default is nicely consistent.

Thanks,
drew

Re: [RFC PATCH v2 5/5] target/arm/cpu: Add the kvm-no-adjvtime CPU property

[Peter Maydell]

On Mon, 16 Dec 2019 at 16:53, Andrew Jones <drjones@redhat.com> wrote:
>
> On Mon, Dec 16, 2019 at 03:06:57PM +0000, Peter Maydell wrote:
> > This phrasing reads a bit confusingly to me. What I would usually expect
> > is that you get
> >   name-of-option              Description of what the option does.
> >
> > But here we have
> >   name-of-option              Long description of the default behaviour,
> >                               taking many lines and several sentences.
> >                               Brief note at the end that enabling this
> >                               feature gives the opposite effect.
> >
> > Especially since the default-behaviour description isn't prefaced
> > with "By default" or similar, it's quite easy to start reading the
> > text assuming it's defining what the option is going to do, only
> > to get to the end and realise that it's defining what the option
> > is *not* going to do...
>
> I'll take another stab at this, but my feeling is that a '-no-' option
> should be one that just turns off the default behavior, which is why I
> wrote a long description of the default behavior. If you'd prefer the
> description to be more terse, then I can certainly delete a bunch of
> the text, but then I fear what this option disables wouldn't be clear
> enough.

I'm happy with the length of it; it would definitely be helped a lot
just with phrasing that was clearer up front about that it was starting
by describing the default behaviour.

thanks
-- PMM

Re: [RFC PATCH v2 0/5] target/arm/kvm: Adjust virtual time

[Andrew Jones]

On Mon, Dec 16, 2019 at 04:18:23PM +0000, Marc Zyngier wrote:
> On 2019-12-16 15:33, Peter Maydell wrote:
> > On Thu, 12 Dec 2019 at 17:33, Andrew Jones <drjones@redhat.com> wrote:
> > 
> > > 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.
> > 
> > So, to be clear, you mean that:
> > 
> > (1) the kernel headers say:
> > 
> > /* EL0 Virtual Timer Registers */
> > #define KVM_REG_ARM_TIMER_CTL           ARM64_SYS_REG(3, 3, 14, 3, 1)
> > #define KVM_REG_ARM_TIMER_CNT           ARM64_SYS_REG(3, 3, 14, 3, 2)
> > #define KVM_REG_ARM_TIMER_CVAL          ARM64_SYS_REG(3, 3, 14, 0, 2)
> > 
> > (2) some of the RHSes of these are wrong
> > 
> > (3) but the kernel internally is using the same 'wrong' value, so
> > userspace also needs to use that value, ie trust the #defined name
> > rather than manufacturing one ?
> > 
> > That's awkward. I think it would be worth at least having a kernel
> > patch to add a comment clearly documenting this bug.
> > 
> > (This error seems to only be in the 64-bit ABI, not 32-bit.)
> 
> Yeah, this is pretty bad. I wonder how we managed not to notice
> this for so long... :-(.
> 
> Andrew, could you please write a patch documenting this (both in
> the UAPI headers and in the documentation)?
>

Will do. I'll try to get to it this week.

Thanks,
drew

Re: [RFC PATCH v2 0/5] target/arm/kvm: Adjust virtual time

[Marc Zyngier]

On 2019-12-16 16:59, Andrew Jones wrote:
> On Mon, Dec 16, 2019 at 04:18:23PM +0000, Marc Zyngier wrote:
>> On 2019-12-16 15:33, Peter Maydell wrote:
>> > On Thu, 12 Dec 2019 at 17:33, Andrew Jones <drjones@redhat.com> 
>> wrote:
>> >
>> > > 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.
>> >
>> > So, to be clear, you mean that:
>> >
>> > (1) the kernel headers say:
>> >
>> > /* EL0 Virtual Timer Registers */
>> > #define KVM_REG_ARM_TIMER_CTL           ARM64_SYS_REG(3, 3, 14, 3, 
>> 1)
>> > #define KVM_REG_ARM_TIMER_CNT           ARM64_SYS_REG(3, 3, 14, 3, 
>> 2)
>> > #define KVM_REG_ARM_TIMER_CVAL          ARM64_SYS_REG(3, 3, 14, 0, 
>> 2)
>> >
>> > (2) some of the RHSes of these are wrong
>> >
>> > (3) but the kernel internally is using the same 'wrong' value, so
>> > userspace also needs to use that value, ie trust the #defined name
>> > rather than manufacturing one ?
>> >
>> > That's awkward. I think it would be worth at least having a kernel
>> > patch to add a comment clearly documenting this bug.
>> >
>> > (This error seems to only be in the 64-bit ABI, not 32-bit.)
>>
>> Yeah, this is pretty bad. I wonder how we managed not to notice
>> this for so long... :-(.
>>
>> Andrew, could you please write a patch documenting this (both in
>> the UAPI headers and in the documentation)?
>>
>
> Will do. I'll try to get to it this week.

Thanks a lot.

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [RFC PATCH v2 3/5] target/arm/kvm: Implement virtual time adjustment

[Peter Maydell]

On Mon, 16 Dec 2019 at 16:44, Andrew Jones <drjones@redhat.com> wrote:
>
> On Mon, Dec 16, 2019 at 03:40:16PM +0000, Peter Maydell wrote:
> > On Mon, 16 Dec 2019 at 15:14, Peter Maydell <peter.maydell@linaro.org> wrote:
> > > How does this interact with the usual register sync to/from
> > > KVM (ie kvm_arch_get_registers(), which I think will do a
> > > GET_ONE_REG read of the TIMER_CNT register the way it does
> > > any other sysreg, inside write_kvmstate_to_list(), plus
> > > kvm_arch_set_registers() which does the write back to the
> > > kernel in write_list_to_kvmstate()) ? Presumably we want this
> > > version to take precedence by the set_virtual_time call
> > > happening after the kvm_arch_set_registers, but is this
> > > guaranteed ?
> >
> > ...you might also want to look at the effects of simply
> > removing the KVM_REG_ARM_TIMER_CNT entry from the
> > 'non_runtime_cpregs[]' array -- in commit 4b7a6bf402bd064
> > we explicitly stopped reading/writing this register's value
> > to/from the kernel except for inbound migration, and it
> > feels like this patchset is now rolling back that approach,
> > so maybe we should also be (configurably) rolling back some
> > of its implementation rather than just leaving it in place.
>
> I feel like I already considered that, maybe even tried it, a few months
> ago when I first looked at this. I must have decided against it for some
> reason at the time, but I don't recall what. Now I can say the reason is
> because we only do this save/restore when we transition to/from paused
> state, though.

I found the thread which discussed the bug which originally
caused us to add commit 4b7a6bf402bd064:
https://lists.cs.columbia.edu/pipermail/kvmarm/2015-July/015665.html
 -- there are some codepaths which cause us to do a sync from/to
KVM for one VCPU while others are still running. If we do a
read-CNT-and-write-back then we effectively cause time to jump
backwards for the other still-running CPUs.

So we do still want to have TIMER_CNT listed as being KVM_PUT_FULL_STATE
regardless, or we re-introduce that bug.

Your approach in this patchset reads and writes on vm-paused,
so it won't have the pre-2015 problems.

It still feels odd that we're storing this bit of guest state
in two places now though -- in kvm_vtime, and also in its usual
place in the cpreg_array data structures (we write back the
value from kvm_vtime when the VM starts running, and we write
back the value from the cpreg_array for a PUT_FULL_STATE, which
the comments claim is only on startup or when we just loaded
migration state (and also undocumentedly but reasonably on
cpu-hotplug, which arm doesn't have yet).

I've just spent a little while digging through code, and
haven't been able to satisfy myself on the ordering of which
writeback wins: for a loadvm I think we first do a
cpu_synchronize_all_post_init() (writing back the counter
value from the migration data) and then after than we will
unpause the VM -- why doesn't this overwrite the correct
value with the wrong value from kvm_vtime ?

I just noticed also that the logic used in this patch
doesn't match what other architectures do in their vm_state_change
function -- eg cpu_ppc_clock_vm_state_change() has an
"if (running) { load } else { save }", and kvmclock_vm_state_change()
for i386 also has "if (running) { ... } else { ... }", though
it has an extra wrinkle where it captures "are we PAUSED?"
to use in the pre_save function; the comment above
kvmclock_pre_save() suggests maybe that would be useful for other
than x86, too. kvm_s390_tod_vm_state_change() has
logic that's a slightly more complicated variation on just
testing the 'running' flag, but it doesn't look at the
specific new state.

> > I note also that the commit message there had a remark
> > about inconsistencies between VCPUs -- is the right thing
> > to handle this per-VM rather than per-VCPU somehow?
>
> per-VM would make sense, because the counters should be synchronized
> among the VCPUs. KVM does that for us, though, so whichever VCPU last
> restores its counter is the one that will determine the final value.
>
> Maybe we should have a VM ioctl instead, but ATM we only have VCPU ioctls.

I meant more "only do the save/load once per VM in QEMU but
do it by working with just one VCPU". But I guess since migration
works on all the VCPUs we're ok to do pause-resume the same way
(and I've now tracked down what the 'inconsistentencies between VCPUs'
were: they're when we were syncing the CNT value for one vCPU when
others were still running.)

thanks
-- PMM

Re: [RFC PATCH v2 3/5] target/arm/kvm: Implement virtual time adjustment

[Andrew Jones]

On Mon, Dec 16, 2019 at 06:06:30PM +0000, Peter Maydell wrote:
> On Mon, 16 Dec 2019 at 16:44, Andrew Jones <drjones@redhat.com> wrote:
> >
> > On Mon, Dec 16, 2019 at 03:40:16PM +0000, Peter Maydell wrote:
> > > On Mon, 16 Dec 2019 at 15:14, Peter Maydell <peter.maydell@linaro.org> wrote:
> > > > How does this interact with the usual register sync to/from
> > > > KVM (ie kvm_arch_get_registers(), which I think will do a
> > > > GET_ONE_REG read of the TIMER_CNT register the way it does
> > > > any other sysreg, inside write_kvmstate_to_list(), plus
> > > > kvm_arch_set_registers() which does the write back to the
> > > > kernel in write_list_to_kvmstate()) ? Presumably we want this
> > > > version to take precedence by the set_virtual_time call
> > > > happening after the kvm_arch_set_registers, but is this
> > > > guaranteed ?
> > >
> > > ...you might also want to look at the effects of simply
> > > removing the KVM_REG_ARM_TIMER_CNT entry from the
> > > 'non_runtime_cpregs[]' array -- in commit 4b7a6bf402bd064
> > > we explicitly stopped reading/writing this register's value
> > > to/from the kernel except for inbound migration, and it
> > > feels like this patchset is now rolling back that approach,
> > > so maybe we should also be (configurably) rolling back some
> > > of its implementation rather than just leaving it in place.
> >
> > I feel like I already considered that, maybe even tried it, a few months
> > ago when I first looked at this. I must have decided against it for some
> > reason at the time, but I don't recall what. Now I can say the reason is
> > because we only do this save/restore when we transition to/from paused
> > state, though.
> 
> I found the thread which discussed the bug which originally
> caused us to add commit 4b7a6bf402bd064:
> https://lists.cs.columbia.edu/pipermail/kvmarm/2015-July/015665.html
>  -- there are some codepaths which cause us to do a sync from/to
> KVM for one VCPU while others are still running. If we do a
> read-CNT-and-write-back then we effectively cause time to jump
> backwards for the other still-running CPUs.
> 
> So we do still want to have TIMER_CNT listed as being KVM_PUT_FULL_STATE
> regardless, or we re-introduce that bug.

Thanks for digging that up. I now recall also having read that history
back when I first discovered KVM_REG_ARM_TIMER_CNT was special.

> 
> Your approach in this patchset reads and writes on vm-paused,
> so it won't have the pre-2015 problems.
> 
> It still feels odd that we're storing this bit of guest state
> in two places now though -- in kvm_vtime, and also in its usual
> place in the cpreg_array data structures (we write back the
> value from kvm_vtime when the VM starts running, and we write
> back the value from the cpreg_array for a PUT_FULL_STATE, which
> the comments claim is only on startup or when we just loaded
> migration state (and also undocumentedly but reasonably on
> cpu-hotplug, which arm doesn't have yet).
> 
> I've just spent a little while digging through code, and
> haven't been able to satisfy myself on the ordering of which
> writeback wins: for a loadvm I think we first do a
> cpu_synchronize_all_post_init() (writing back the counter
> value from the migration data) and then after than we will
> unpause the VM -- why doesn't this overwrite the correct
> value with the wrong value from kvm_vtime ?

Hmm... I think I may have gotten lost when I went through this before.
I just went through again, and still won't claim that I'm not a bit
lost, but it does appear I got it backwards. When I get a chance I'll
try to test this properly.

We could use the same location as normal, in the cpreg_array. I'd just
need to add a search of cpreg_indexes[] in order to get the index
needed for cpreg_values[]. 

> 
> I just noticed also that the logic used in this patch
> doesn't match what other architectures do in their vm_state_change
> function -- eg cpu_ppc_clock_vm_state_change() has an
> "if (running) { load } else { save }", and kvmclock_vm_state_change()
> for i386 also has "if (running) { ... } else { ... }", though
> it has an extra wrinkle where it captures "are we PAUSED?"
> to use in the pre_save function; the comment above
> kvmclock_pre_save() suggests maybe that would be useful for other
> than x86, too. kvm_s390_tod_vm_state_change() has
> logic that's a slightly more complicated variation on just
> testing the 'running' flag, but it doesn't look at the
> specific new state.

Yes, originally I had just if (running) {} else {}, but after looking at
https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg03695.html and
seeing that the other architectures were careful to track the "are we
paused" state, I got the feeling that we should be more specific and
changed to if (running) {} else if (paused) {}. That's probably wrong,
though, if we want to track all vm-stopped time.

> 
> > > I note also that the commit message there had a remark
> > > about inconsistencies between VCPUs -- is the right thing
> > > to handle this per-VM rather than per-VCPU somehow?
> >
> > per-VM would make sense, because the counters should be synchronized
> > among the VCPUs. KVM does that for us, though, so whichever VCPU last
> > restores its counter is the one that will determine the final value.
> >
> > Maybe we should have a VM ioctl instead, but ATM we only have VCPU ioctls.
> 
> I meant more "only do the save/load once per VM in QEMU but
> do it by working with just one VCPU". But I guess since migration
> works on all the VCPUs we're ok to do pause-resume the same way
> (and I've now tracked down what the 'inconsistentencies between VCPUs'
> were: they're when we were syncing the CNT value for one vCPU when
> others were still running.)
> 
> thanks
> -- PMM
> 

Thanks,
drew

Re: [RFC PATCH v2 3/5] target/arm/kvm: Implement virtual time adjustment

[Andrew Jones]

On Thu, Dec 19, 2019 at 03:30:12PM +0100, Andrew Jones wrote:
> On Mon, Dec 16, 2019 at 06:06:30PM +0000, Peter Maydell wrote:
> > Your approach in this patchset reads and writes on vm-paused,
> > so it won't have the pre-2015 problems.
> > 
> > It still feels odd that we're storing this bit of guest state
> > in two places now though -- in kvm_vtime, and also in its usual
> > place in the cpreg_array data structures (we write back the
> > value from kvm_vtime when the VM starts running, and we write
> > back the value from the cpreg_array for a PUT_FULL_STATE, which
> > the comments claim is only on startup or when we just loaded
> > migration state (and also undocumentedly but reasonably on
> > cpu-hotplug, which arm doesn't have yet).

I tried to get rid of the extra state location (kvm_vtime), but we still
need it because kvm_arch_get_registers() doesn't take 'level', like
kvm_arch_put_registers() does. Maybe it should? Without being able to
filter out TIMER_CNT at get time too, then if we migrate a paused guest
we'll resume with vtime including the ticks between the pause and the
start of the migration. Adding the additional state (kvm_vtime) and a
cpu_pre_save() hook to fixup the cpreg value is a possible way to resolve
that. That's what I've done for v3, which I'll post shortly.

> > 
> > I've just spent a little while digging through code, and
> > haven't been able to satisfy myself on the ordering of which
> > writeback wins: for a loadvm I think we first do a
> > cpu_synchronize_all_post_init() (writing back the counter
> > value from the migration data) and then after than we will
> > unpause the VM -- why doesn't this overwrite the correct
> > value with the wrong value from kvm_vtime ?

It wasn't overwriting because we weren't detecting a runstate
transition from paused to running. However, for v3, I've dropped
the explicit running/pause transition checking and now ensured
we get the right value with a cpu_post_load() hook.

> 
> > I just noticed also that the logic used in this patch
> > doesn't match what other architectures do in their vm_state_change
> > function -- eg cpu_ppc_clock_vm_state_change() has an
> > "if (running) { load } else { save }", and kvmclock_vm_state_change()
> > for i386 also has "if (running) { ... } else { ... }", though
> > it has an extra wrinkle where it captures "are we PAUSED?"
> > to use in the pre_save function; the comment above
> > kvmclock_pre_save() suggests maybe that would be useful for other
> > than x86, too. kvm_s390_tod_vm_state_change() has
> > logic that's a slightly more complicated variation on just
> > testing the 'running' flag, but it doesn't look at the
> > specific new state.

I think I've mimicked this logic now for arm in v3.

Thanks,
drew

Re: [RFC PATCH v2 5/5] target/arm/cpu: Add the kvm-no-adjvtime CPU property

[Andrew Jones]

On Mon, Dec 16, 2019 at 03:06:57PM +0000, Peter Maydell wrote:
> Incidentally, if I understand things correctly, for TCG the
> behaviour is (and has always been) that VM-stopped time is
> not counted, because we run the emulated versions of these counters
> off QEMU_CLOCK_VIRTUAL. So having the KVM default be the same as
> the TCG default is nicely consistent.

It's correct that the vtimer has always been this way for TCG.
However TCG and KVM still won't be the same with the adjvtime
patches. The reason is that TCG also bases the ptimer off
QEMU_CLOCK_VIRTUAL. This means on TCG ptimer == vtimer and
both will lag a clocksource outside the VM.

I'm not sure if TCG people want to change that to match KVM.
If so, then I guess the ptimer should be based off 
QEMU_CLOCK_REALTIME.

Thanks,
drew

Re: [RFC PATCH v2 0/5] target/arm/kvm: Adjust virtual time

[Andrew Jones]

On Mon, Dec 16, 2019 at 03:44:05PM +0000, Peter Maydell wrote:
> On Mon, 16 Dec 2019 at 15:33, Peter Maydell <peter.maydell@linaro.org> wrote:
> > So, to be clear, you mean that:
> >
> > (1) the kernel headers say:
> >
> > /* EL0 Virtual Timer Registers */
> > #define KVM_REG_ARM_TIMER_CTL           ARM64_SYS_REG(3, 3, 14, 3, 1)
> > #define KVM_REG_ARM_TIMER_CNT           ARM64_SYS_REG(3, 3, 14, 3, 2)
> > #define KVM_REG_ARM_TIMER_CVAL          ARM64_SYS_REG(3, 3, 14, 0, 2)
> >
> > (2) some of the RHSes of these are wrong
> >
> > (3) but the kernel internally is using the same 'wrong' value, so
> > userspace also needs to use that value, ie trust the #defined name
> > rather than manufacturing one ?
> >
> > That's awkward. I think it would be worth at least having a kernel
> > patch to add a comment clearly documenting this bug.
> >
> > (This error seems to only be in the 64-bit ABI, not 32-bit.)
> >
> > QEMU does assume that the kernel's ID register values match
> > the hardware for sysregs in some ways -- we use this when we
> > construct our mapping from KVM register IDs as returned by
> > KVM_GET_REG_LIST to QEMU cpreg definitions and thus CPUState
> > struct fields. I *think* that in this case the only visible
> > effect will be that gdbstub will show you the CNT value
> > if you ask it to print the value of the CVAL sysreg.
> 
> ...perhaps we should work around this kernel bug in the
> kvm_to_cpreg_id() and cpreg_to_kvm_id() functions. (Need
> to think through/test whether that would break migration.)
>

I just did some grepping for this too and, while it's easy to get
lost, I think I've confirmed what you state, that the only visible
effect is in gdb. I'll try to test this, but I think the workaround
in kvm_to_cpreg_id and cpreg_to_kvm_id is a probably a good idea,
because

   1) new qemu will be corrected

   2) migrate old qemu to new qemu (with same machine type)

      gdb cnt and cval swapped until first kvmstate sync

      (maybe too small a window of brokenness to notice/care?)

   3) migrate new qemu to old qemu (with same machine type)

      gdb cnt and cval correct until first kvmstate sync

      (old machine type keeps its old bug - except for same small
       window as for (2))

Thanks,
drew

Re: [RFC PATCH v2 5/5] target/arm/cpu: Add the kvm-no-adjvtime CPU property

[Philippe Mathieu-Daudé]

Hi Drew,

On Thu, Dec 12, 2019 at 7:27 PM Andrew Jones <drjones@redhat.com> wrote:
> kvm-no-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>
> ---
>  docs/arm-cpu-features.rst | 31 ++++++++++++++++++++++++++++++-
>  hw/arm/virt.c             |  8 ++++++++
>  include/hw/arm/virt.h     |  1 +
>  target/arm/cpu.c          |  2 ++
>  target/arm/cpu64.c        |  1 +
>  target/arm/kvm.c          | 28 ++++++++++++++++++++++++++++
>  target/arm/kvm_arm.h      | 11 +++++++++++
>  target/arm/monitor.c      |  1 +
>  tests/arm-cpu-features.c  |  4 ++++
>  9 files changed, 86 insertions(+), 1 deletion(-)
>
[...]
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index cb7041e9677a..e9a08eb883bf 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1664,6 +1664,11 @@ static void machvirt_init(MachineState *machine)
>              }
>          }
>
> +        if (vmc->kvm_no_adjvtime &&
> +            object_property_find(cpuobj, "kvm-no-adjvtime", NULL)) {
> +            object_property_set_bool(cpuobj, true, "kvm-no-adjvtime", NULL);
> +        }
> +
>          if (vmc->no_pmu && object_property_find(cpuobj, "pmu", NULL)) {
>              object_property_set_bool(cpuobj, false, "pmu", NULL);
>          }
> @@ -2154,8 +2159,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(5, 0)
>
>  static void virt_machine_4_2_options(MachineClass *mc)
>  {
> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> +
>      virt_machine_5_0_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_4_2, hw_compat_4_2_len);
> +    vmc->kvm_no_adjvtime = true;
>  }
>  DEFINE_VIRT_MACHINE(4, 2)
>
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index 0b41083e9d5c..80e00161b8f2 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -109,6 +109,7 @@ typedef struct {
>      bool smbios_old_sys_ver;
>      bool no_highmem_ecam;
>      bool no_ged;   /* Machines < 4.2 has no support for ACPI GED device */
> +    bool kvm_no_adjvtime;
>  } VirtMachineClass;
>
>  typedef struct {
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 7a4ac9339bf9..53c73c25a67f 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -2482,6 +2482,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);
>
> @@ -2673,6 +2674,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 a39d6fcea34f..3cd416db8089 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -605,6 +605,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 a55fe7d7aefd..7666b250ab96 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,32 @@ void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
>      env->features = arm_host_cpu_features.features;
>  }
>
> +static bool kvm_no_adjvtime_get(Object *obj, Error **errp)
> +{
> +    return !ARM_CPU(obj)->kvm_adjvtime;
> +}
> +
> +static void kvm_no_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()) {

This can't happen, right? Can we turn that into an assertion, or
remove the check?

> +        return;
> +    }
> +
> +    ARM_CPU(obj)->kvm_adjvtime = true;
> +    object_property_add_bool(obj, "kvm-no-adjvtime", kvm_no_adjvtime_get,
> +                             kvm_no_adjvtime_set, &error_abort);
> +    object_property_set_description(obj, "kvm-no-adjvtime",
> +                                    "Set on to disable the adjustment of "
> +                                    "the virtual counter. VM stopped time "
> +                                    "will 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 16b53e45377d..7d76f26beaca 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_get_virtual_time:
>   * @cs: CPUState
> @@ -319,6 +328,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..9725dfff16d4 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-no-adjvtime",
>      NULL
>  };
>
> diff --git a/tests/arm-cpu-features.c b/tests/arm-cpu-features.c
> index 06ed05e934e6..738e49447377 100644
> --- a/tests/arm-cpu-features.c
> +++ b/tests/arm-cpu-features.c
> @@ -435,6 +435,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-no-adjvtime");
> +
>      if (g_str_equal(qtest_get_arch(), "aarch64")) {
>          assert_has_feature_enabled(qts, "max", "aarch64");
>          assert_has_feature_enabled(qts, "max", "sve");
> @@ -469,6 +471,8 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
>          return;
>      }
>
> +    assert_has_feature_disabled(qts, "host", "kvm-no-adjvtime");
> +
>      if (g_str_equal(qtest_get_arch(), "aarch64")) {
>          bool kvm_supports_sve;
>          char max_name[8], name[8];
> --
> 2.21.0
>
>

Re: [RFC PATCH v2 5/5] target/arm/cpu: Add the kvm-no-adjvtime CPU property

[Andrew Jones]

On Thu, Feb 06, 2020 at 01:08:53PM +0100, Philippe Mathieu-Daudé wrote:
...
> > +/* KVM VCPU properties should be prefixed with "kvm-". */
> > +void kvm_arm_add_vcpu_properties(Object *obj)
> > +{
> > +    if (!kvm_enabled()) {
> 
> This can't happen, right? Can we turn that into an assertion, or
> remove the check?

You're right. An assert would be more appropriate. Will you send a patch?

Thanks,
drew

Re: [RFC PATCH v2 5/5] target/arm/cpu: Add the kvm-no-adjvtime CPU property

[Philippe Mathieu-Daudé]

On 2/6/20 1:40 PM, Andrew Jones wrote:
> On Thu, Feb 06, 2020 at 01:08:53PM +0100, Philippe Mathieu-Daudé wrote:
> ...
>>> +/* KVM VCPU properties should be prefixed with "kvm-". */
>>> +void kvm_arm_add_vcpu_properties(Object *obj)
>>> +{
>>> +    if (!kvm_enabled()) {
>>
>> This can't happen, right? Can we turn that into an assertion, or
>> remove the check?
> 
> You're right. An assert would be more appropriate. Will you send a patch?

Apparently this can be called with KVM_CONFIG && !kvm_enabled():

See kvm_arm_set_cpu_features_from_host()
{
     if (!arm_host_cpu_features.dtb_compatible) {
         if (!kvm_enabled() ||
             !kvm_arm_get_host_cpu_features(&arm_host_cpu_features)) {
             /* We can't report this error yet, so flag that we need to
              * in arm_cpu_realizefn().
              */

I won't modify your patch (until I have a better understanding of 
TYPE_ARM_HOST_CPU).

Re: [RFC PATCH v2 5/5] target/arm/cpu: Add the kvm-no-adjvtime CPU property

[Andrew Jones]

On Thu, Feb 06, 2020 at 11:46:49PM +0100, Philippe Mathieu-Daudé wrote:
> On 2/6/20 1:40 PM, Andrew Jones wrote:
> > On Thu, Feb 06, 2020 at 01:08:53PM +0100, Philippe Mathieu-Daudé wrote:
> > ...
> > > > +/* KVM VCPU properties should be prefixed with "kvm-". */
> > > > +void kvm_arm_add_vcpu_properties(Object *obj)
> > > > +{
> > > > +    if (!kvm_enabled()) {
> > > 
> > > This can't happen, right? Can we turn that into an assertion, or
> > > remove the check?
> > 
> > You're right. An assert would be more appropriate. Will you send a patch?
> 
> Apparently this can be called with KVM_CONFIG && !kvm_enabled():
> 
> See kvm_arm_set_cpu_features_from_host()
> {
>     if (!arm_host_cpu_features.dtb_compatible) {
>         if (!kvm_enabled() ||
>             !kvm_arm_get_host_cpu_features(&arm_host_cpu_features)) {
>             /* We can't report this error yet, so flag that we need to
>              * in arm_cpu_realizefn().
>              */
> 
> I won't modify your patch (until I have a better understanding of
> TYPE_ARM_HOST_CPU).
>

Just grepped around and talked to Igor. Indeed we need to be able
to do the host cpu init without erroring out (even if we know we
will in realize) and, as cpu instance_init should add the properties,
then I don't really see a cleaner way to do it than what we already
have.

Thanks,
drew