qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/5] hw/arm/virt: Introduce kvm-steal-time
@ 2020-08-05  9:16 Andrew Jones
  2020-08-05  9:16 ` [PATCH v2 1/5] hw: add compat machines for 5.2 Andrew Jones
                   ` (5 more replies)
  0 siblings, 6 replies; 14+ messages in thread
From: Andrew Jones @ 2020-08-05  9:16 UTC (permalink / raw)
  To: qemu-devel, qemu-arm; +Cc: peter.maydell

v2:
  - Changed the introduction of the feature to 5.2 from 5.1
    (The 5.2 machine type patch posted by Cornelia was thrown
     in for completeness, but I suppose that'll get picked up
     separately.)
  - Added a patch adding g_assert_not_reached() to many KVM
    stubs. (This isn't exactly related to the series, but the
    series does add two more stubs that can now be added in
    the same way.)
  - Fixed a patch that wasn't suppose to have a functional
    change, but did (fdt_add_gic_node() shouldn't generate
    the node without the PMU) [Peter]
  - Pass sysmem to virt_cpu_post_init() [Peter]
  - Introduced a define for the pvtime struct size
  - Calculate the pvtime memory region size based on max-cpus
    and host-page-size [Beata]
  - Renamed kvm_no_steal_time to no_kvm_steal_time [Peter]
  - Fixed a parameter misordering with object_property_set_bool()
  - Added a comment explaining why the feature isn't supported
    for AArch32 guests
  - Changed a !kvm_steal_time to a kvm_steal_time == ON_OFF_AUTO_OFF
    as it should be
  - Picked up one r-b from Peter


KVM supports the ability to publish the amount of time that VCPUs
were runnable, but not running due to other host threads running
instead, to the guest. The guest scheduler may use that information
when making decisions and the guest may expose it to its userspace
(Linux publishes this information in /proc/stat). This feature is
called "steal time" as it represents the amount of time stolen from
a guest by scheduling out its VCPUs. To enable this feature KVM
userspace must provide a memory region that will be used to publish
the information to the guest. The memory region is typical migratable
region. The GPA of the region is given to KVM through a VCPU device
ioctl interface. This feature is only available for 64-bit hosts
running 64-bit guests.

This series provides the QEMU support of this feature. It will
be enabled by default for 5.2 machine types and later, but may
be disabled with a new CPU property "kvm-steal-time".

While testing migration it was observed that the amount of
steal time as viewed by the guest was getting reset on each
migration. Patch 4/6 of a pvtime fix series posted[*] for KVM
should fix that. Also, we may still want to change the way we
probe KVM for the feature in this QEMU series to the new KVM
cap proposed in that KVM series.

Migration testing:

* virt-5.1 can migrate as usual, no steal-time enabled

* virt-5.2 can migrate between hosts with steal-time enabled
  (the default) and disabled when both hosts support steal-time

* virt-5.2 with steal-time disabled can migrate to a host that
  does not support steal-time

* virt-5.2 with steal-time enabled will cleanly fail when migrating
  to a host that does not support steal-time

* virt-5.2 without the kvm-steal-time property specified can
  boot on a host that does not support the feature - the feature
  will be disabled. However, if the guest is migrated to a host
  that does support the feature, then after the guest has reboot
  the feature will become available with the guest kernel supports
  it (this is the nature of migrating guests that use '-cpu host').
  Additionally, once this guest has been migrated to a host that
  does have the feature, then it will fail (cleanly) to migrate to
  a host that does not have the feature. If this behavior isn't
  desired, then the user should explicitly disable steal-time with
  the kvm-steal-time property if they want to boot a 5.2 guest on
  a host that doesn't support steal-time.

[*] https://lists.cs.columbia.edu/pipermail/kvmarm/2020-August/041823.html

Thanks,
drew


Andrew Jones (4):
  target/arm/kvm: Make uncalled stubs explicitly unreachable
  hw/arm/virt: Move post cpu realize check into its own function
  hw/arm/virt: Move kvm pmu setup to virt_cpu_post_init
  hw/arm/virt: Implement kvm-steal-time

Cornelia Huck (1):
  hw: add compat machines for 5.2

 docs/system/arm/cpu-features.rst |  11 +++
 hw/arm/virt.c                    | 119 +++++++++++++++++++++++--------
 hw/core/machine.c                |   3 +
 hw/i386/pc.c                     |   6 +-
 hw/i386/pc_piix.c                |  14 +++-
 hw/i386/pc_q35.c                 |  13 +++-
 hw/ppc/spapr.c                   |  15 +++-
 hw/s390x/s390-virtio-ccw.c       |  14 +++-
 include/hw/arm/virt.h            |   5 ++
 include/hw/boards.h              |   3 +
 include/hw/i386/pc.h             |   3 +
 target/arm/cpu.c                 |  10 +++
 target/arm/cpu.h                 |   4 ++
 target/arm/kvm.c                 |  20 ++++++
 target/arm/kvm32.c               |   5 ++
 target/arm/kvm64.c               |  77 ++++++++++++++++++--
 target/arm/kvm_arm.h             |  74 ++++++++++++++-----
 target/arm/monitor.c             |   2 +-
 tests/qtest/arm-cpu-features.c   |  25 +++++--
 19 files changed, 358 insertions(+), 65 deletions(-)

-- 
2.25.4



^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v2 1/5] hw: add compat machines for 5.2
  2020-08-05  9:16 [PATCH v2 0/5] hw/arm/virt: Introduce kvm-steal-time Andrew Jones
@ 2020-08-05  9:16 ` Andrew Jones
  2020-08-05  9:16 ` [PATCH v2 2/5] target/arm/kvm: Make uncalled stubs explicitly unreachable Andrew Jones
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 14+ messages in thread
From: Andrew Jones @ 2020-08-05  9:16 UTC (permalink / raw)
  To: qemu-devel, qemu-arm; +Cc: peter.maydell

From: Cornelia Huck <cohuck@redhat.com>

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

Signed-off-by: Cornelia Huck <cohuck@redhat.com>
---
 hw/arm/virt.c              |  9 ++++++++-
 hw/core/machine.c          |  3 +++
 hw/i386/pc.c               |  6 ++++--
 hw/i386/pc_piix.c          | 14 +++++++++++++-
 hw/i386/pc_q35.c           | 13 ++++++++++++-
 hw/ppc/spapr.c             | 15 +++++++++++++--
 hw/s390x/s390-virtio-ccw.c | 14 +++++++++++++-
 include/hw/boards.h        |  3 +++
 include/hw/i386/pc.h       |  3 +++
 9 files changed, 72 insertions(+), 8 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index ecfee362a182..acf9bfbeceaf 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -2546,10 +2546,17 @@ static void machvirt_machine_init(void)
 }
 type_init(machvirt_machine_init);
 
+static void virt_machine_5_2_options(MachineClass *mc)
+{
+}
+DEFINE_VIRT_MACHINE_AS_LATEST(5, 2)
+
 static void virt_machine_5_1_options(MachineClass *mc)
 {
+    virt_machine_5_2_options(mc);
+    compat_props_add(mc->compat_props, hw_compat_5_1, hw_compat_5_1_len);
 }
-DEFINE_VIRT_MACHINE_AS_LATEST(5, 1)
+DEFINE_VIRT_MACHINE(5, 1)
 
 static void virt_machine_5_0_options(MachineClass *mc)
 {
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 8d1a90c6cf45..cf5f2dfaeb34 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -28,6 +28,9 @@
 #include "hw/mem/nvdimm.h"
 #include "migration/vmstate.h"
 
+GlobalProperty hw_compat_5_1[] = {};
+const size_t hw_compat_5_1_len = G_N_ELEMENTS(hw_compat_5_1);
+
 GlobalProperty hw_compat_5_0[] = {
     { "pci-host-bridge", "x-config-reg-migration-enabled", "off" },
     { "virtio-balloon-device", "page-poison", "false" },
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 47c5ca3e342b..1733b5341a62 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -97,8 +97,10 @@
 #include "fw_cfg.h"
 #include "trace.h"
 
-GlobalProperty pc_compat_5_0[] = {
-};
+GlobalProperty pc_compat_5_1[] = {};
+const size_t pc_compat_5_1_len = G_N_ELEMENTS(pc_compat_5_1);
+
+GlobalProperty pc_compat_5_0[] = {};
 const size_t pc_compat_5_0_len = G_N_ELEMENTS(pc_compat_5_0);
 
 GlobalProperty pc_compat_4_2[] = {
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index b789e83f9acb..c5ba70ca17cb 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -426,7 +426,7 @@ static void pc_i440fx_machine_options(MachineClass *m)
     machine_class_allow_dynamic_sysbus_dev(m, TYPE_VMBUS_BRIDGE);
 }
 
-static void pc_i440fx_5_1_machine_options(MachineClass *m)
+static void pc_i440fx_5_2_machine_options(MachineClass *m)
 {
     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_i440fx_machine_options(m);
@@ -435,6 +435,18 @@ static void pc_i440fx_5_1_machine_options(MachineClass *m)
     pcmc->default_cpu_version = 1;
 }
 
+DEFINE_I440FX_MACHINE(v5_2, "pc-i440fx-5.2", NULL,
+                      pc_i440fx_5_2_machine_options);
+
+static void pc_i440fx_5_1_machine_options(MachineClass *m)
+{
+    pc_i440fx_5_2_machine_options(m);
+    m->alias = NULL;
+    m->is_default = false;
+    compat_props_add(m->compat_props, hw_compat_5_1, hw_compat_5_1_len);
+    compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
+}
+
 DEFINE_I440FX_MACHINE(v5_1, "pc-i440fx-5.1", NULL,
                       pc_i440fx_5_1_machine_options);
 
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index a3e607a544a5..0cb9c18cd44d 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -353,7 +353,7 @@ static void pc_q35_machine_options(MachineClass *m)
     m->max_cpus = 288;
 }
 
-static void pc_q35_5_1_machine_options(MachineClass *m)
+static void pc_q35_5_2_machine_options(MachineClass *m)
 {
     PCMachineClass *pcmc = PC_MACHINE_CLASS(m);
     pc_q35_machine_options(m);
@@ -361,6 +361,17 @@ static void pc_q35_5_1_machine_options(MachineClass *m)
     pcmc->default_cpu_version = 1;
 }
 
+DEFINE_Q35_MACHINE(v5_2, "pc-q35-5.2", NULL,
+                   pc_q35_5_2_machine_options);
+
+static void pc_q35_5_1_machine_options(MachineClass *m)
+{
+    pc_q35_5_2_machine_options(m);
+    m->alias = NULL;
+    compat_props_add(m->compat_props, hw_compat_5_1, hw_compat_5_1_len);
+    compat_props_add(m->compat_props, pc_compat_5_1, pc_compat_5_1_len);
+}
+
 DEFINE_Q35_MACHINE(v5_1, "pc-q35-5.1", NULL,
                    pc_q35_5_1_machine_options);
 
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 0ae293ec9431..1c8d0981b382 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -4579,15 +4579,26 @@ static void spapr_machine_latest_class_options(MachineClass *mc)
     }                                                                \
     type_init(spapr_machine_register_##suffix)
 
+/*
+ * pseries-5.2
+ */
+static void spapr_machine_5_2_class_options(MachineClass *mc)
+{
+    /* Defaults for the latest behaviour inherited from the base class */
+}
+
+DEFINE_SPAPR_MACHINE(5_2, "5.2", true);
+
 /*
  * pseries-5.1
  */
 static void spapr_machine_5_1_class_options(MachineClass *mc)
 {
-    /* Defaults for the latest behaviour inherited from the base class */
+    spapr_machine_5_2_class_options(mc);
+    compat_props_add(mc->compat_props, hw_compat_5_1, hw_compat_5_1_len);
 }
 
-DEFINE_SPAPR_MACHINE(5_1, "5.1", true);
+DEFINE_SPAPR_MACHINE(5_1, "5.1", false);
 
 /*
  * pseries-5.0
diff --git a/hw/s390x/s390-virtio-ccw.c b/hw/s390x/s390-virtio-ccw.c
index e72c61d2eae0..f4ea6a954597 100644
--- a/hw/s390x/s390-virtio-ccw.c
+++ b/hw/s390x/s390-virtio-ccw.c
@@ -801,14 +801,26 @@ bool css_migration_enabled(void)
     }                                                                         \
     type_init(ccw_machine_register_##suffix)
 
+static void ccw_machine_5_2_instance_options(MachineState *machine)
+{
+}
+
+static void ccw_machine_5_2_class_options(MachineClass *mc)
+{
+}
+DEFINE_CCW_MACHINE(5_2, "5.2", true);
+
 static void ccw_machine_5_1_instance_options(MachineState *machine)
 {
+    ccw_machine_5_2_instance_options(machine);
 }
 
 static void ccw_machine_5_1_class_options(MachineClass *mc)
 {
+    ccw_machine_5_2_class_options(mc);
+    compat_props_add(mc->compat_props, hw_compat_5_1, hw_compat_5_1_len);
 }
-DEFINE_CCW_MACHINE(5_1, "5.1", true);
+DEFINE_CCW_MACHINE(5_1, "5.1", false);
 
 static void ccw_machine_5_0_instance_options(MachineState *machine)
 {
diff --git a/include/hw/boards.h b/include/hw/boards.h
index 426ce5f625a4..bc5b82ad209e 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -319,6 +319,9 @@ struct MachineState {
     } \
     type_init(machine_initfn##_register_types)
 
+extern GlobalProperty hw_compat_5_1[];
+extern const size_t hw_compat_5_1_len;
+
 extern GlobalProperty hw_compat_5_0[];
 extern const size_t hw_compat_5_0_len;
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 3d7ed3a55e30..fe52e165b27c 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -193,6 +193,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_5_1[];
+extern const size_t pc_compat_5_1_len;
+
 extern GlobalProperty pc_compat_5_0[];
 extern const size_t pc_compat_5_0_len;
 
-- 
2.25.4



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 2/5] target/arm/kvm: Make uncalled stubs explicitly unreachable
  2020-08-05  9:16 [PATCH v2 0/5] hw/arm/virt: Introduce kvm-steal-time Andrew Jones
  2020-08-05  9:16 ` [PATCH v2 1/5] hw: add compat machines for 5.2 Andrew Jones
@ 2020-08-05  9:16 ` Andrew Jones
  2020-08-12 10:16   ` Auger Eric
  2020-08-05  9:16 ` [PATCH v2 3/5] hw/arm/virt: Move post cpu realize check into its own function Andrew Jones
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Andrew Jones @ 2020-08-05  9:16 UTC (permalink / raw)
  To: qemu-devel, qemu-arm; +Cc: peter.maydell

When we compile without KVM support !defined(CONFIG_KVM) we generate
stubs for functions that the linker will still encounter. Sometimes
these stubs can be executed safely and are placed in paths where they
get executed with or without KVM. Other functions should never be
called without KVM. Those functions should be guarded by kvm_enabled(),
but should also be robust to refactoring mistakes. Putting a
g_assert_not_reached() in the function should help. Additionally,
the g_assert_not_reached() calls may actually help the linker remove
some code.

We remove the stubs for kvm_arm_get/put_virtual_time(), as they aren't
necessary at all - the only caller is in kvm.c

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 target/arm/kvm_arm.h | 44 +++++++++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index adb38514bf20..0da00eb6b20c 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -344,16 +344,10 @@ int kvm_arm_set_irq(int cpu, int irqtype, int irq, int level);
 
 #else
 
-static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
-{
-    /*
-     * This should never actually be called in the "not KVM" case,
-     * but set up the fields to indicate an error anyway.
-     */
-    cpu->kvm_target = QEMU_KVM_ARM_TARGET_NONE;
-    cpu->host_cpu_probe_failed = true;
-}
-
+/*
+ * It's safe to call these functions without KVM support.
+ * They should either do nothing or return "not supported".
+ */
 static inline void kvm_arm_add_vcpu_properties(Object *obj) {}
 
 static inline bool kvm_arm_aarch32_supported(void)
@@ -371,23 +365,39 @@ static inline bool kvm_arm_sve_supported(void)
     return false;
 }
 
+/*
+ * These functions should never actually be called without KVM support.
+ */
+static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
+{
+    g_assert_not_reached();
+}
+
 static inline int kvm_arm_get_max_vm_ipa_size(MachineState *ms)
 {
-    return -ENOENT;
+    g_assert_not_reached();
 }
 
 static inline int kvm_arm_vgic_probe(void)
 {
-    return 0;
+    g_assert_not_reached();
 }
 
-static inline void kvm_arm_pmu_set_irq(CPUState *cs, int irq) {}
-static inline void kvm_arm_pmu_init(CPUState *cs) {}
+static inline void kvm_arm_pmu_set_irq(CPUState *cs, int irq)
+{
+    g_assert_not_reached();
+}
 
-static inline void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map) {}
+static inline void kvm_arm_pmu_init(CPUState *cs)
+{
+    g_assert_not_reached();
+}
+
+static inline void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map)
+{
+    g_assert_not_reached();
+}
 
-static inline void kvm_arm_get_virtual_time(CPUState *cs) {}
-static inline void kvm_arm_put_virtual_time(CPUState *cs) {}
 #endif
 
 static inline const char *gic_class_name(void)
-- 
2.25.4



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 3/5] hw/arm/virt: Move post cpu realize check into its own function
  2020-08-05  9:16 [PATCH v2 0/5] hw/arm/virt: Introduce kvm-steal-time Andrew Jones
  2020-08-05  9:16 ` [PATCH v2 1/5] hw: add compat machines for 5.2 Andrew Jones
  2020-08-05  9:16 ` [PATCH v2 2/5] target/arm/kvm: Make uncalled stubs explicitly unreachable Andrew Jones
@ 2020-08-05  9:16 ` Andrew Jones
  2020-08-12 10:16   ` Auger Eric
  2020-08-05  9:16 ` [PATCH v2 4/5] hw/arm/virt: Move kvm pmu setup to virt_cpu_post_init Andrew Jones
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 14+ messages in thread
From: Andrew Jones @ 2020-08-05  9:16 UTC (permalink / raw)
  To: qemu-devel, qemu-arm; +Cc: peter.maydell

We'll add more to this new function in coming patches so we also
state the gic must be created and call it below create_gic().

No functional change intended.

Signed-off-by: Andrew Jones <drjones@redhat.com>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 43 +++++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index acf9bfbeceaf..2cba21fe3ad9 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1672,6 +1672,31 @@ static void finalize_gic_version(VirtMachineState *vms)
     }
 }
 
+/*
+ * virt_cpu_post_init() must be called after the CPUs have
+ * been realized and the GIC has been created.
+ */
+static void virt_cpu_post_init(VirtMachineState *vms)
+{
+    bool aarch64;
+
+    aarch64 = object_property_get_bool(OBJECT(first_cpu), "aarch64", NULL);
+
+    if (!kvm_enabled()) {
+        if (aarch64 && vms->highmem) {
+            int requested_pa_size = 64 - clz64(vms->highest_gpa);
+            int pamax = arm_pamax(ARM_CPU(first_cpu));
+
+            if (pamax < requested_pa_size) {
+                error_report("VCPU supports less PA bits (%d) than "
+                             "requested by the memory map (%d)",
+                             pamax, requested_pa_size);
+                exit(1);
+            }
+        }
+    }
+}
+
 static void machvirt_init(MachineState *machine)
 {
     VirtMachineState *vms = VIRT_MACHINE(machine);
@@ -1886,22 +1911,6 @@ static void machvirt_init(MachineState *machine)
     fdt_add_timer_nodes(vms);
     fdt_add_cpu_nodes(vms);
 
-   if (!kvm_enabled()) {
-        ARMCPU *cpu = ARM_CPU(first_cpu);
-        bool aarch64 = object_property_get_bool(OBJECT(cpu), "aarch64", NULL);
-
-        if (aarch64 && vms->highmem) {
-            int requested_pa_size, pamax = arm_pamax(cpu);
-
-            requested_pa_size = 64 - clz64(vms->highest_gpa);
-            if (pamax < requested_pa_size) {
-                error_report("VCPU supports less PA bits (%d) than requested "
-                            "by the memory map (%d)", pamax, requested_pa_size);
-                exit(1);
-            }
-        }
-    }
-
     memory_region_add_subregion(sysmem, vms->memmap[VIRT_MEM].base,
                                 machine->ram);
     if (machine->device_memory) {
@@ -1913,6 +1922,8 @@ static void machvirt_init(MachineState *machine)
 
     create_gic(vms);
 
+    virt_cpu_post_init(vms);
+
     fdt_add_pmu_nodes(vms);
 
     create_uart(vms, VIRT_UART, sysmem, serial_hd(0));
-- 
2.25.4



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 4/5] hw/arm/virt: Move kvm pmu setup to virt_cpu_post_init
  2020-08-05  9:16 [PATCH v2 0/5] hw/arm/virt: Introduce kvm-steal-time Andrew Jones
                   ` (2 preceding siblings ...)
  2020-08-05  9:16 ` [PATCH v2 3/5] hw/arm/virt: Move post cpu realize check into its own function Andrew Jones
@ 2020-08-05  9:16 ` Andrew Jones
  2020-08-12 10:16   ` Auger Eric
  2020-08-25 14:27   ` Peter Maydell
  2020-08-05  9:16 ` [PATCH v2 5/5] hw/arm/virt: Implement kvm-steal-time Andrew Jones
  2020-09-07  7:04 ` [PATCH v2 0/5] hw/arm/virt: Introduce kvm-steal-time Andrew Jones
  5 siblings, 2 replies; 14+ messages in thread
From: Andrew Jones @ 2020-08-05  9:16 UTC (permalink / raw)
  To: qemu-devel, qemu-arm; +Cc: peter.maydell

Move the KVM PMU setup part of fdt_add_pmu_nodes() to
virt_cpu_post_init(), which is a more appropriate location. Now
fdt_add_pmu_nodes() is also named more appropriately, because it
no longer does anything but fdt node creation.

No functional change intended.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 hw/arm/virt.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 2cba21fe3ad9..6797eb397a7a 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -521,21 +521,12 @@ static void fdt_add_gic_node(VirtMachineState *vms)
 
 static void fdt_add_pmu_nodes(const VirtMachineState *vms)
 {
-    CPUState *cpu;
-    ARMCPU *armcpu;
+    ARMCPU *armcpu = ARM_CPU(first_cpu);
     uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI;
 
-    CPU_FOREACH(cpu) {
-        armcpu = ARM_CPU(cpu);
-        if (!arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
-            return;
-        }
-        if (kvm_enabled()) {
-            if (kvm_irqchip_in_kernel()) {
-                kvm_arm_pmu_set_irq(cpu, PPI(VIRTUAL_PMU_IRQ));
-            }
-            kvm_arm_pmu_init(cpu);
-        }
+    if (!arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
+        assert(!object_property_get_bool(OBJECT(armcpu), "pmu", NULL));
+        return;
     }
 
     if (vms->gic_version == VIRT_GIC_VERSION_2) {
@@ -544,7 +535,6 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms)
                              (1 << vms->smp_cpus) - 1);
     }
 
-    armcpu = ARM_CPU(qemu_get_cpu(0));
     qemu_fdt_add_subnode(vms->fdt, "/pmu");
     if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
         const char compat[] = "arm,armv8-pmuv3";
@@ -1678,11 +1668,23 @@ static void finalize_gic_version(VirtMachineState *vms)
  */
 static void virt_cpu_post_init(VirtMachineState *vms)
 {
-    bool aarch64;
+    bool aarch64, pmu;
+    CPUState *cpu;
 
     aarch64 = object_property_get_bool(OBJECT(first_cpu), "aarch64", NULL);
+    pmu = object_property_get_bool(OBJECT(first_cpu), "pmu", NULL);
 
-    if (!kvm_enabled()) {
+    if (kvm_enabled()) {
+        CPU_FOREACH(cpu) {
+            if (pmu) {
+                assert(arm_feature(&ARM_CPU(cpu)->env, ARM_FEATURE_PMU));
+                if (kvm_irqchip_in_kernel()) {
+                    kvm_arm_pmu_set_irq(cpu, PPI(VIRTUAL_PMU_IRQ));
+                }
+                kvm_arm_pmu_init(cpu);
+            }
+        }
+    } else {
         if (aarch64 && vms->highmem) {
             int requested_pa_size = 64 - clz64(vms->highest_gpa);
             int pamax = arm_pamax(ARM_CPU(first_cpu));
-- 
2.25.4



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH v2 5/5] hw/arm/virt: Implement kvm-steal-time
  2020-08-05  9:16 [PATCH v2 0/5] hw/arm/virt: Introduce kvm-steal-time Andrew Jones
                   ` (3 preceding siblings ...)
  2020-08-05  9:16 ` [PATCH v2 4/5] hw/arm/virt: Move kvm pmu setup to virt_cpu_post_init Andrew Jones
@ 2020-08-05  9:16 ` Andrew Jones
  2020-08-12 12:41   ` Auger Eric
  2020-09-07  7:04 ` [PATCH v2 0/5] hw/arm/virt: Introduce kvm-steal-time Andrew Jones
  5 siblings, 1 reply; 14+ messages in thread
From: Andrew Jones @ 2020-08-05  9:16 UTC (permalink / raw)
  To: qemu-devel, qemu-arm; +Cc: peter.maydell

We add the kvm-steal-time CPU property and implement it for machvirt.
A tiny bit of refactoring was also done to allow pmu and pvtime to
use the same vcpu device helper functions.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 docs/system/arm/cpu-features.rst | 11 +++++
 hw/arm/virt.c                    | 43 ++++++++++++++++--
 include/hw/arm/virt.h            |  5 +++
 target/arm/cpu.c                 | 10 +++++
 target/arm/cpu.h                 |  4 ++
 target/arm/kvm.c                 | 20 +++++++++
 target/arm/kvm32.c               |  5 +++
 target/arm/kvm64.c               | 77 +++++++++++++++++++++++++++++---
 target/arm/kvm_arm.h             | 30 +++++++++++++
 target/arm/monitor.c             |  2 +-
 tests/qtest/arm-cpu-features.c   | 25 +++++++++--
 11 files changed, 219 insertions(+), 13 deletions(-)

diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
index 2d5c06cd016b..35196a6b759d 100644
--- a/docs/system/arm/cpu-features.rst
+++ b/docs/system/arm/cpu-features.rst
@@ -200,6 +200,17 @@ the list of KVM VCPU features and their descriptions.
                            adjustment, also restoring the legacy (pre-5.0)
                            behavior.
 
+  kvm-steal-time           Since v5.2, kvm-steal-time is enabled by
+                           default when KVM is enabled, the feature is
+                           supported, and the guest is 64-bit.
+
+                           When kvm-steal-time is enabled a 64-bit guest
+                           can account for time its CPUs were not running
+                           due to the host not scheduling the corresponding
+                           VCPU threads.  The accounting statistics may
+                           influence the guest scheduler behavior and/or be
+                           exposed to the guest userspace.
+
 SVE CPU Properties
 ==================
 
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 6797eb397a7a..12efc2f095cb 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -151,6 +151,7 @@ static const MemMapEntry base_memmap[] = {
     [VIRT_PCDIMM_ACPI] =        { 0x09070000, MEMORY_HOTPLUG_IO_LEN },
     [VIRT_ACPI_GED] =           { 0x09080000, ACPI_GED_EVT_SEL_LEN },
     [VIRT_NVDIMM_ACPI] =        { 0x09090000, NVDIMM_ACPI_IO_LEN},
+    [VIRT_PVTIME] =             { 0x090a0000, 0x00010000 },
     [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
     /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
     [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
@@ -1666,15 +1667,39 @@ static void finalize_gic_version(VirtMachineState *vms)
  * virt_cpu_post_init() must be called after the CPUs have
  * been realized and the GIC has been created.
  */
-static void virt_cpu_post_init(VirtMachineState *vms)
+static void virt_cpu_post_init(VirtMachineState *vms, int max_cpus,
+                               MemoryRegion *sysmem)
 {
-    bool aarch64, pmu;
+    bool aarch64, pmu, steal_time;
     CPUState *cpu;
 
     aarch64 = object_property_get_bool(OBJECT(first_cpu), "aarch64", NULL);
     pmu = object_property_get_bool(OBJECT(first_cpu), "pmu", NULL);
+    steal_time = object_property_get_bool(OBJECT(first_cpu),
+                                          "kvm-steal-time", NULL);
 
     if (kvm_enabled()) {
+        hwaddr pvtime_reg_base = vms->memmap[VIRT_PVTIME].base;
+        hwaddr pvtime_reg_size = vms->memmap[VIRT_PVTIME].size;
+
+        if (steal_time) {
+            MemoryRegion *pvtime = g_new(MemoryRegion, 1);
+            hwaddr pvtime_size = max_cpus * PVTIME_SIZE_PER_CPU;
+
+            /* The memory region size must be a multiple of host page size. */
+            pvtime_size = REAL_HOST_PAGE_ALIGN(pvtime_size);
+
+            if (pvtime_size > pvtime_reg_size) {
+                error_report("pvtime requires a %ld byte memory region for "
+                             "%d CPUs, but only %ld has been reserved",
+                             pvtime_size, max_cpus, pvtime_reg_size);
+                exit(1);
+            }
+
+            memory_region_init_ram(pvtime, NULL, "pvtime", pvtime_size, NULL);
+            memory_region_add_subregion(sysmem, pvtime_reg_base, pvtime);
+        }
+
         CPU_FOREACH(cpu) {
             if (pmu) {
                 assert(arm_feature(&ARM_CPU(cpu)->env, ARM_FEATURE_PMU));
@@ -1683,6 +1708,10 @@ static void virt_cpu_post_init(VirtMachineState *vms)
                 }
                 kvm_arm_pmu_init(cpu);
             }
+            if (steal_time) {
+                kvm_arm_pvtime_init(cpu, pvtime_reg_base +
+                                         cpu->cpu_index * PVTIME_SIZE_PER_CPU);
+            }
         }
     } else {
         if (aarch64 && vms->highmem) {
@@ -1853,6 +1882,11 @@ static void machvirt_init(MachineState *machine)
             object_property_set_bool(cpuobj, "kvm-no-adjvtime", true, NULL);
         }
 
+        if (vmc->no_kvm_steal_time &&
+            object_property_find(cpuobj, "kvm-steal-time", NULL)) {
+            object_property_set_bool(cpuobj, "kvm-steal-time", false, NULL);
+        }
+
         if (vmc->no_pmu && object_property_find(cpuobj, "pmu", NULL)) {
             object_property_set_bool(cpuobj, "pmu", false, NULL);
         }
@@ -1924,7 +1958,7 @@ static void machvirt_init(MachineState *machine)
 
     create_gic(vms);
 
-    virt_cpu_post_init(vms);
+    virt_cpu_post_init(vms, possible_cpus->len, sysmem);
 
     fdt_add_pmu_nodes(vms);
 
@@ -2566,8 +2600,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(5, 2)
 
 static void virt_machine_5_1_options(MachineClass *mc)
 {
+    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
+
     virt_machine_5_2_options(mc);
     compat_props_add(mc->compat_props, hw_compat_5_1, hw_compat_5_1_len);
+    vmc->no_kvm_steal_time = true;
 }
 DEFINE_VIRT_MACHINE(5, 1)
 
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index dff67e1bef03..5fb43a009f7a 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -53,6 +53,9 @@
 
 #define PPI(irq) ((irq) + 16)
 
+/* See Linux kernel arch/arm64/include/asm/pvclock-abi.h */
+#define PVTIME_SIZE_PER_CPU 64
+
 enum {
     VIRT_FLASH,
     VIRT_MEM,
@@ -80,6 +83,7 @@ enum {
     VIRT_PCDIMM_ACPI,
     VIRT_ACPI_GED,
     VIRT_NVDIMM_ACPI,
+    VIRT_PVTIME,
     VIRT_LOWMEMMAP_LAST,
 };
 
@@ -126,6 +130,7 @@ typedef struct {
     bool no_ged;   /* Machines < 4.2 has no support for ACPI GED device */
     bool kvm_no_adjvtime;
     bool acpi_expose_flash;
+    bool no_kvm_steal_time;
 } VirtMachineClass;
 
 typedef struct {
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 111579554fb9..866d0a9fb146 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1308,6 +1308,16 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
             return;
         }
     }
+
+    if (kvm_enabled()) {
+#ifdef TARGET_AARCH64
+        kvm_arm_steal_time_finalize(cpu, &local_err);
+        if (local_err != NULL) {
+            error_propagate(errp, local_err);
+            return;
+        }
+#endif
+    }
 }
 
 static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 9e8ed423ea1d..a4d4cb640c77 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -24,6 +24,7 @@
 #include "hw/registerfields.h"
 #include "cpu-qom.h"
 #include "exec/cpu-defs.h"
+#include "qapi/qapi-types-common.h"
 
 /* ARM processors have a weak memory model */
 #define TCG_GUEST_DEFAULT_MO      (0)
@@ -859,6 +860,9 @@ struct ARMCPU {
     bool kvm_vtime_dirty;
     uint64_t kvm_vtime;
 
+    /* KVM steal time */
+    OnOffAuto kvm_steal_time;
+
     /* Uniprocessor system with MP extensions */
     bool mp_is_up;
 
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 8bb7318378b5..093a290453f6 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -192,6 +192,18 @@ static void kvm_no_adjvtime_set(Object *obj, bool value, Error **errp)
     ARM_CPU(obj)->kvm_adjvtime = !value;
 }
 
+#ifdef TARGET_AARCH64
+static bool kvm_steal_time_get(Object *obj, Error **errp)
+{
+    return ARM_CPU(obj)->kvm_steal_time != ON_OFF_AUTO_OFF;
+}
+
+static void kvm_steal_time_set(Object *obj, bool value, Error **errp)
+{
+    ARM_CPU(obj)->kvm_steal_time = value ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
+}
+#endif
+
 /* KVM VCPU properties should be prefixed with "kvm-". */
 void kvm_arm_add_vcpu_properties(Object *obj)
 {
@@ -207,6 +219,14 @@ void kvm_arm_add_vcpu_properties(Object *obj)
                                         "the virtual counter. VM stopped time "
                                         "will be counted.");
     }
+
+#ifdef TARGET_AARCH64
+    cpu->kvm_steal_time = ON_OFF_AUTO_AUTO;
+    object_property_add_bool(obj, "kvm-steal-time", kvm_steal_time_get,
+                             kvm_steal_time_set);
+    object_property_set_description(obj, "kvm-steal-time",
+                                    "Set off to disable KVM steal time.");
+#endif
 }
 
 bool kvm_arm_pmu_supported(void)
diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
index 0af46b41c847..d3f3195077fa 100644
--- a/target/arm/kvm32.c
+++ b/target/arm/kvm32.c
@@ -560,6 +560,11 @@ void kvm_arm_pmu_init(CPUState *cs)
     qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
 }
 
+void kvm_arm_pvtime_init(CPUState *cs, uint64_t ipa)
+{
+    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
+}
+
 #define ARM_REG_DFSR  ARM_CP15_REG32(0, 5, 0, 0)
 #define ARM_REG_TTBCR ARM_CP15_REG32(0, 2, 0, 2)
 /*
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 116923790550..4de1eeb723ac 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -17,6 +17,7 @@
 #include <linux/kvm.h>
 
 #include "qemu-common.h"
+#include "qapi/error.h"
 #include "cpu.h"
 #include "qemu/timer.h"
 #include "qemu/error-report.h"
@@ -398,19 +399,20 @@ static CPUWatchpoint *find_hw_watchpoint(CPUState *cpu, target_ulong addr)
     return NULL;
 }
 
-static bool kvm_arm_pmu_set_attr(CPUState *cs, struct kvm_device_attr *attr)
+static bool kvm_arm_set_device_attr(CPUState *cs, struct kvm_device_attr *attr,
+                                    const char *name)
 {
     int err;
 
     err = kvm_vcpu_ioctl(cs, KVM_HAS_DEVICE_ATTR, attr);
     if (err != 0) {
-        error_report("PMU: KVM_HAS_DEVICE_ATTR: %s", strerror(-err));
+        error_report("%s: KVM_HAS_DEVICE_ATTR: %s", name, strerror(-err));
         return false;
     }
 
     err = kvm_vcpu_ioctl(cs, KVM_SET_DEVICE_ATTR, attr);
     if (err != 0) {
-        error_report("PMU: KVM_SET_DEVICE_ATTR: %s", strerror(-err));
+        error_report("%s: KVM_SET_DEVICE_ATTR: %s", name, strerror(-err));
         return false;
     }
 
@@ -427,7 +429,7 @@ void kvm_arm_pmu_init(CPUState *cs)
     if (!ARM_CPU(cs)->has_pmu) {
         return;
     }
-    if (!kvm_arm_pmu_set_attr(cs, &attr)) {
+    if (!kvm_arm_set_device_attr(cs, &attr, "PMU")) {
         error_report("failed to init PMU");
         abort();
     }
@@ -444,12 +446,29 @@ void kvm_arm_pmu_set_irq(CPUState *cs, int irq)
     if (!ARM_CPU(cs)->has_pmu) {
         return;
     }
-    if (!kvm_arm_pmu_set_attr(cs, &attr)) {
+    if (!kvm_arm_set_device_attr(cs, &attr, "PMU")) {
         error_report("failed to set irq for PMU");
         abort();
     }
 }
 
+void kvm_arm_pvtime_init(CPUState *cs, uint64_t ipa)
+{
+    struct kvm_device_attr attr = {
+        .group = KVM_ARM_VCPU_PVTIME_CTRL,
+        .attr = KVM_ARM_VCPU_PVTIME_IPA,
+        .addr = (uint64_t)&ipa,
+    };
+
+    if (ARM_CPU(cs)->kvm_steal_time == ON_OFF_AUTO_OFF) {
+        return;
+    }
+    if (!kvm_arm_set_device_attr(cs, &attr, "PVTIME IPA")) {
+        error_report("failed to init PVTIME IPA");
+        abort();
+    }
+}
+
 static int read_sys_reg32(int fd, uint32_t *pret, uint64_t id)
 {
     uint64_t ret;
@@ -652,6 +671,54 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
     return true;
 }
 
+void kvm_arm_steal_time_finalize(ARMCPU *cpu, Error **errp)
+{
+    static bool has_steal_time;
+    static bool probed;
+    int fdarray[3];
+
+    if (!probed) {
+        probed = true;
+        if (kvm_check_extension(kvm_state, KVM_CAP_VCPU_ATTRIBUTES)) {
+            if (!kvm_arm_create_scratch_host_vcpu(NULL, fdarray, NULL)) {
+                error_report("Failed to create scratch VCPU");
+                abort();
+            }
+
+            has_steal_time = kvm_device_check_attr(fdarray[2],
+                                                   KVM_ARM_VCPU_PVTIME_CTRL,
+                                                   KVM_ARM_VCPU_PVTIME_IPA);
+
+            kvm_arm_destroy_scratch_host_vcpu(fdarray);
+        }
+    }
+
+    if (cpu->kvm_steal_time == ON_OFF_AUTO_AUTO) {
+        if (!has_steal_time || !arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
+            cpu->kvm_steal_time = ON_OFF_AUTO_OFF;
+        } else {
+            cpu->kvm_steal_time = ON_OFF_AUTO_ON;
+        }
+    } else if (cpu->kvm_steal_time == ON_OFF_AUTO_ON) {
+        if (!has_steal_time) {
+            error_setg(errp, "'kvm-steal-time' cannot be enabled "
+                             "on this host");
+            return;
+        } else if (!arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
+            /*
+             * DEN0057A chapter 2 says "This specification only covers
+             * systems in which the Execution state of the hypervisor
+             * as well as EL1 of virtual machines is AArch64.". And,
+             * to ensure that, the smc/hvc calls are only specified as
+             * smc64/hvc64.
+             */
+            error_setg(errp, "'kvm-steal-time' cannot be enabled "
+                             "for AArch32 guests");
+            return;
+        }
+    }
+}
+
 bool kvm_arm_aarch32_supported(void)
 {
     return kvm_check_extension(kvm_state, KVM_CAP_ARM_EL1_32BIT);
diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index 0da00eb6b20c..4a8969505967 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -267,6 +267,16 @@ void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu);
  */
 void kvm_arm_add_vcpu_properties(Object *obj);
 
+/**
+ * kvm_arm_steal_time_finalize:
+ * @cpu: ARMCPU for which to finalize kvm-steal-time
+ * @errp: Pointer to Error* for error propagation
+ *
+ * Validate the kvm-steal-time property selection and set its default
+ * based on KVM support and guest configuration.
+ */
+void kvm_arm_steal_time_finalize(ARMCPU *cpu, Error **errp);
+
 /**
  * kvm_arm_aarch32_supported:
  *
@@ -340,6 +350,16 @@ int kvm_arm_vgic_probe(void);
 
 void kvm_arm_pmu_set_irq(CPUState *cs, int irq);
 void kvm_arm_pmu_init(CPUState *cs);
+
+/**
+ * kvm_arm_pvtime_init:
+ * @cs: CPUState
+ * @ipa: Per-vcpu guest physical base address of the pvtime structures
+ *
+ * Initializes PVTIME for the VCPU, setting the PVTIME IPA to @ipa.
+ */
+void kvm_arm_pvtime_init(CPUState *cs, uint64_t ipa);
+
 int kvm_arm_set_irq(int cpu, int irqtype, int irq, int level);
 
 #else
@@ -393,6 +413,16 @@ static inline void kvm_arm_pmu_init(CPUState *cs)
     g_assert_not_reached();
 }
 
+static inline void kvm_arm_pvtime_init(CPUState *cs, uint64_t ipa)
+{
+    g_assert_not_reached();
+}
+
+static inline void kvm_arm_steal_time_finalize(ARMCPU *cpu, Error **errp)
+{
+    g_assert_not_reached();
+}
+
 static inline void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map)
 {
     g_assert_not_reached();
diff --git a/target/arm/monitor.c b/target/arm/monitor.c
index ba6e01abd037..bd3590604a71 100644
--- a/target/arm/monitor.c
+++ b/target/arm/monitor.c
@@ -103,7 +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",
+    "kvm-no-adjvtime", "kvm-steal-time",
     NULL
 };
 
diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
index f7e062c1891e..91b181f38268 100644
--- a/tests/qtest/arm-cpu-features.c
+++ b/tests/qtest/arm-cpu-features.c
@@ -452,6 +452,7 @@ static void test_query_cpu_model_expansion(const void *data)
     assert_set_feature(qts, "max", "pmu", true);
 
     assert_has_not_feature(qts, "max", "kvm-no-adjvtime");
+    assert_has_not_feature(qts, "max", "kvm-steal-time");
 
     if (g_str_equal(qtest_get_arch(), "aarch64")) {
         assert_has_feature_enabled(qts, "max", "aarch64");
@@ -493,6 +494,7 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
     assert_set_feature(qts, "host", "kvm-no-adjvtime", false);
 
     if (g_str_equal(qtest_get_arch(), "aarch64")) {
+        bool kvm_supports_steal_time;
         bool kvm_supports_sve;
         char max_name[8], name[8];
         uint32_t max_vq, vq;
@@ -500,6 +502,10 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
         QDict *resp;
         char *error;
 
+        assert_error(qts, "cortex-a15",
+            "We cannot guarantee the CPU type 'cortex-a15' works "
+            "with KVM on this host", NULL);
+
         assert_has_feature_enabled(qts, "host", "aarch64");
 
         /* Enabling and disabling pmu should always work. */
@@ -507,16 +513,26 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
         assert_set_feature(qts, "host", "pmu", false);
         assert_set_feature(qts, "host", "pmu", true);
 
-        assert_error(qts, "cortex-a15",
-            "We cannot guarantee the CPU type 'cortex-a15' works "
-            "with KVM on this host", NULL);
-
+        /*
+         * Some features would be enabled by default, but they're disabled
+         * because this instance of KVM doesn't support them. Test that the
+         * features are present, and, when enabled, issue further tests.
+         */
+        assert_has_feature(qts, "host", "kvm-steal-time");
         assert_has_feature(qts, "host", "sve");
+
         resp = do_query_no_props(qts, "host");
+        kvm_supports_steal_time = resp_get_feature(resp, "kvm-steal-time");
         kvm_supports_sve = resp_get_feature(resp, "sve");
         vls = resp_get_sve_vls(resp);
         qobject_unref(resp);
 
+        if (kvm_supports_steal_time) {
+            /* If we have steal-time then we should be able to toggle it. */
+            assert_set_feature(qts, "host", "kvm-steal-time", false);
+            assert_set_feature(qts, "host", "kvm-steal-time", true);
+        }
+
         if (kvm_supports_sve) {
             g_assert(vls != 0);
             max_vq = 64 - __builtin_clzll(vls);
@@ -577,6 +593,7 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
         assert_has_not_feature(qts, "host", "aarch64");
         assert_has_not_feature(qts, "host", "pmu");
         assert_has_not_feature(qts, "host", "sve");
+        assert_has_not_feature(qts, "host", "kvm-steal-time");
     }
 
     qtest_quit(qts);
-- 
2.25.4



^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 4/5] hw/arm/virt: Move kvm pmu setup to virt_cpu_post_init
  2020-08-05  9:16 ` [PATCH v2 4/5] hw/arm/virt: Move kvm pmu setup to virt_cpu_post_init Andrew Jones
@ 2020-08-12 10:16   ` Auger Eric
  2020-08-12 10:35     ` Andrew Jones
  2020-08-25 14:27   ` Peter Maydell
  1 sibling, 1 reply; 14+ messages in thread
From: Auger Eric @ 2020-08-12 10:16 UTC (permalink / raw)
  To: Andrew Jones, qemu-devel, qemu-arm; +Cc: peter.maydell

Hi Drew,
On 8/5/20 11:16 AM, Andrew Jones wrote:
> Move the KVM PMU setup part of fdt_add_pmu_nodes() to
> virt_cpu_post_init(), which is a more appropriate location. Now
> fdt_add_pmu_nodes() is also named more appropriately, because it
> no longer does anything but fdt node creation.
> 
> No functional change intended.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  hw/arm/virt.c | 34 ++++++++++++++++++----------------
>  1 file changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 2cba21fe3ad9..6797eb397a7a 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -521,21 +521,12 @@ static void fdt_add_gic_node(VirtMachineState *vms)
>  
>  static void fdt_add_pmu_nodes(const VirtMachineState *vms)
>  {
> -    CPUState *cpu;
> -    ARMCPU *armcpu;
> +    ARMCPU *armcpu = ARM_CPU(first_cpu);
>      uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI;
>  
> -    CPU_FOREACH(cpu) {
> -        armcpu = ARM_CPU(cpu);
> -        if (!arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
> -            return;
> -        }
> -        if (kvm_enabled()) {
> -            if (kvm_irqchip_in_kernel()) {
> -                kvm_arm_pmu_set_irq(cpu, PPI(VIRTUAL_PMU_IRQ));
> -            }
> -            kvm_arm_pmu_init(cpu);
> -        }
> +    if (!arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
> +        assert(!object_property_get_bool(OBJECT(armcpu), "pmu", NULL));
I don't rget the relevance of the assert. If the PMU is set, isn't is
the consequence of arm_set_pmu?
> +        return;
>      }
>  
>      if (vms->gic_version == VIRT_GIC_VERSION_2) {
> @@ -544,7 +535,6 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms)
>                               (1 << vms->smp_cpus) - 1);
>      }
>  
> -    armcpu = ARM_CPU(qemu_get_cpu(0));
>      qemu_fdt_add_subnode(vms->fdt, "/pmu");
>      if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
>          const char compat[] = "arm,armv8-pmuv3";
> @@ -1678,11 +1668,23 @@ static void finalize_gic_version(VirtMachineState *vms)
>   */
>  static void virt_cpu_post_init(VirtMachineState *vms)
>  {
> -    bool aarch64;
> +    bool aarch64, pmu;
> +    CPUState *cpu;
>  
>      aarch64 = object_property_get_bool(OBJECT(first_cpu), "aarch64", NULL);
> +    pmu = object_property_get_bool(OBJECT(first_cpu), "pmu", NULL);
>  
> -    if (!kvm_enabled()) {
> +    if (kvm_enabled()) {
> +        CPU_FOREACH(cpu) {
> +            if (pmu) {
> +                assert(arm_feature(&ARM_CPU(cpu)->env, ARM_FEATURE_PMU));
same here?
> +                if (kvm_irqchip_in_kernel()) {
> +                    kvm_arm_pmu_set_irq(cpu, PPI(VIRTUAL_PMU_IRQ));
> +                }
> +                kvm_arm_pmu_init(cpu);
> +            }
> +        }
> +    } else {
>          if (aarch64 && vms->highmem) {
>              int requested_pa_size = 64 - clz64(vms->highest_gpa);
>              int pamax = arm_pamax(ARM_CPU(first_cpu));
> 
Thanks

Eric



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 3/5] hw/arm/virt: Move post cpu realize check into its own function
  2020-08-05  9:16 ` [PATCH v2 3/5] hw/arm/virt: Move post cpu realize check into its own function Andrew Jones
@ 2020-08-12 10:16   ` Auger Eric
  0 siblings, 0 replies; 14+ messages in thread
From: Auger Eric @ 2020-08-12 10:16 UTC (permalink / raw)
  To: Andrew Jones, qemu-devel, qemu-arm; +Cc: peter.maydell

Hi Drew,

On 8/5/20 11:16 AM, Andrew Jones wrote:
> We'll add more to this new function in coming patches so we also
> state the gic must be created and call it below create_gic().
> 
> No functional change intended.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  hw/arm/virt.c | 43 +++++++++++++++++++++++++++----------------
>  1 file changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index acf9bfbeceaf..2cba21fe3ad9 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1672,6 +1672,31 @@ static void finalize_gic_version(VirtMachineState *vms)
>      }
>  }
>  
> +/*
> + * virt_cpu_post_init() must be called after the CPUs have
> + * been realized and the GIC has been created.
> + */
> +static void virt_cpu_post_init(VirtMachineState *vms)
> +{
> +    bool aarch64;
> +
> +    aarch64 = object_property_get_bool(OBJECT(first_cpu), "aarch64", NULL);
> +
> +    if (!kvm_enabled()) {
> +        if (aarch64 && vms->highmem) {
> +            int requested_pa_size = 64 - clz64(vms->highest_gpa);
> +            int pamax = arm_pamax(ARM_CPU(first_cpu));
> +
> +            if (pamax < requested_pa_size) {
> +                error_report("VCPU supports less PA bits (%d) than "
> +                             "requested by the memory map (%d)",
> +                             pamax, requested_pa_size);
> +                exit(1);
> +            }
> +        }
> +    }
> +}
> +
>  static void machvirt_init(MachineState *machine)
>  {
>      VirtMachineState *vms = VIRT_MACHINE(machine);
> @@ -1886,22 +1911,6 @@ static void machvirt_init(MachineState *machine)
>      fdt_add_timer_nodes(vms);
>      fdt_add_cpu_nodes(vms);
>  
> -   if (!kvm_enabled()) {
> -        ARMCPU *cpu = ARM_CPU(first_cpu);
> -        bool aarch64 = object_property_get_bool(OBJECT(cpu), "aarch64", NULL);
> -
> -        if (aarch64 && vms->highmem) {
> -            int requested_pa_size, pamax = arm_pamax(cpu);
> -
> -            requested_pa_size = 64 - clz64(vms->highest_gpa);
> -            if (pamax < requested_pa_size) {
> -                error_report("VCPU supports less PA bits (%d) than requested "
> -                            "by the memory map (%d)", pamax, requested_pa_size);
> -                exit(1);
> -            }
> -        }
> -    }
> -
>      memory_region_add_subregion(sysmem, vms->memmap[VIRT_MEM].base,
>                                  machine->ram);
>      if (machine->device_memory) {
> @@ -1913,6 +1922,8 @@ static void machvirt_init(MachineState *machine)
>  
>      create_gic(vms);
>  
> +    virt_cpu_post_init(vms);
> +
>      fdt_add_pmu_nodes(vms);
>  
>      create_uart(vms, VIRT_UART, sysmem, serial_hd(0));
> 



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 2/5] target/arm/kvm: Make uncalled stubs explicitly unreachable
  2020-08-05  9:16 ` [PATCH v2 2/5] target/arm/kvm: Make uncalled stubs explicitly unreachable Andrew Jones
@ 2020-08-12 10:16   ` Auger Eric
  2020-08-12 10:26     ` Andrew Jones
  0 siblings, 1 reply; 14+ messages in thread
From: Auger Eric @ 2020-08-12 10:16 UTC (permalink / raw)
  To: Andrew Jones, qemu-devel, qemu-arm; +Cc: peter.maydell

Hi Drew,

On 8/5/20 11:16 AM, Andrew Jones wrote:
> When we compile without KVM support !defined(CONFIG_KVM) we generate
> stubs for functions that the linker will still encounter. Sometimes
> these stubs can be executed safely and are placed in paths where they
> get executed with or without KVM. Other functions should never be
> called without KVM. Those functions should be guarded by kvm_enabled(),
> but should also be robust to refactoring mistakes. Putting a
> g_assert_not_reached() in the function should help. Additionally,
> the g_assert_not_reached() calls may actually help the linker remove
> some code.
> 
> We remove the stubs for kvm_arm_get/put_virtual_time(), as they aren't
> necessary at all - the only caller is in kvm.c
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  target/arm/kvm_arm.h | 44 +++++++++++++++++++++++++++-----------------
>  1 file changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> index adb38514bf20..0da00eb6b20c 100644
> --- a/target/arm/kvm_arm.h
> +++ b/target/arm/kvm_arm.h
> @@ -344,16 +344,10 @@ int kvm_arm_set_irq(int cpu, int irqtype, int irq, int level);
>  
>  #else
>  
> -static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
> -{
> -    /*
> -     * This should never actually be called in the "not KVM" case,
> -     * but set up the fields to indicate an error anyway.
> -     */
> -    cpu->kvm_target = QEMU_KVM_ARM_TARGET_NONE;
> -    cpu->host_cpu_probe_failed = true;
> -}
> -
> +/*
> + * It's safe to call these functions without KVM support.
> + * They should either do nothing or return "not supported".
> + */
>  static inline void kvm_arm_add_vcpu_properties(Object *obj) {}
this one also is guarded by kvm_enabled() in target/arm/cpu.c
don't you want to add g_assert_not_reached()?
>  
>  static inline bool kvm_arm_aarch32_supported(void)
> @@ -371,23 +365,39 @@ static inline bool kvm_arm_sve_supported(void)
>      return false;
>  }
>  
> +/*
> + * These functions should never actually be called without KVM support.
> + */
> +static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
> +{
> +    g_assert_not_reached();
> +}
> +
>  static inline int kvm_arm_get_max_vm_ipa_size(MachineState *ms)
>  {
> -    return -ENOENT;
> +    g_assert_not_reached();
>  }
>  
>  static inline int kvm_arm_vgic_probe(void)
>  {
> -    return 0;
> +    g_assert_not_reached();
>  }
>  
> -static inline void kvm_arm_pmu_set_irq(CPUState *cs, int irq) {}
> -static inline void kvm_arm_pmu_init(CPUState *cs) {}
> +static inline void kvm_arm_pmu_set_irq(CPUState *cs, int irq)
> +{
> +    g_assert_not_reached();
> +}
>  
> -static inline void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map) {}
> +static inline void kvm_arm_pmu_init(CPUState *cs)
> +{
> +    g_assert_not_reached();
> +}
> +
> +static inline void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map)
> +{
> +    g_assert_not_reached();
> +}
>  
> -static inline void kvm_arm_get_virtual_time(CPUState *cs) {}
> -static inline void kvm_arm_put_virtual_time(CPUState *cs) {}
>  #endif
>  
>  static inline const char *gic_class_name(void)
> 

Besides,
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 2/5] target/arm/kvm: Make uncalled stubs explicitly unreachable
  2020-08-12 10:16   ` Auger Eric
@ 2020-08-12 10:26     ` Andrew Jones
  0 siblings, 0 replies; 14+ messages in thread
From: Andrew Jones @ 2020-08-12 10:26 UTC (permalink / raw)
  To: Auger Eric; +Cc: peter.maydell, qemu-arm, qemu-devel

On Wed, Aug 12, 2020 at 12:16:43PM +0200, Auger Eric wrote:
> Hi Drew,
> 
> On 8/5/20 11:16 AM, Andrew Jones wrote:
> > When we compile without KVM support !defined(CONFIG_KVM) we generate
> > stubs for functions that the linker will still encounter. Sometimes
> > these stubs can be executed safely and are placed in paths where they
> > get executed with or without KVM. Other functions should never be
> > called without KVM. Those functions should be guarded by kvm_enabled(),
> > but should also be robust to refactoring mistakes. Putting a
> > g_assert_not_reached() in the function should help. Additionally,
> > the g_assert_not_reached() calls may actually help the linker remove
> > some code.
> > 
> > We remove the stubs for kvm_arm_get/put_virtual_time(), as they aren't
> > necessary at all - the only caller is in kvm.c
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  target/arm/kvm_arm.h | 44 +++++++++++++++++++++++++++-----------------
> >  1 file changed, 27 insertions(+), 17 deletions(-)
> > 
> > diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> > index adb38514bf20..0da00eb6b20c 100644
> > --- a/target/arm/kvm_arm.h
> > +++ b/target/arm/kvm_arm.h
> > @@ -344,16 +344,10 @@ int kvm_arm_set_irq(int cpu, int irqtype, int irq, int level);
> >  
> >  #else
> >  
> > -static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
> > -{
> > -    /*
> > -     * This should never actually be called in the "not KVM" case,
> > -     * but set up the fields to indicate an error anyway.
> > -     */
> > -    cpu->kvm_target = QEMU_KVM_ARM_TARGET_NONE;
> > -    cpu->host_cpu_probe_failed = true;
> > -}
> > -
> > +/*
> > + * It's safe to call these functions without KVM support.
> > + * They should either do nothing or return "not supported".
> > + */
> >  static inline void kvm_arm_add_vcpu_properties(Object *obj) {}
> this one also is guarded by kvm_enabled() in target/arm/cpu.c
> don't you want to add g_assert_not_reached()?

Yes, that would make sense. I'll do that for v3.

> >  
> >  static inline bool kvm_arm_aarch32_supported(void)
> > @@ -371,23 +365,39 @@ static inline bool kvm_arm_sve_supported(void)
> >      return false;
> >  }
> >  
> > +/*
> > + * These functions should never actually be called without KVM support.
> > + */
> > +static inline void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
> > +{
> > +    g_assert_not_reached();
> > +}
> > +
> >  static inline int kvm_arm_get_max_vm_ipa_size(MachineState *ms)
> >  {
> > -    return -ENOENT;
> > +    g_assert_not_reached();
> >  }
> >  
> >  static inline int kvm_arm_vgic_probe(void)
> >  {
> > -    return 0;
> > +    g_assert_not_reached();
> >  }
> >  
> > -static inline void kvm_arm_pmu_set_irq(CPUState *cs, int irq) {}
> > -static inline void kvm_arm_pmu_init(CPUState *cs) {}
> > +static inline void kvm_arm_pmu_set_irq(CPUState *cs, int irq)
> > +{
> > +    g_assert_not_reached();
> > +}
> >  
> > -static inline void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map) {}
> > +static inline void kvm_arm_pmu_init(CPUState *cs)
> > +{
> > +    g_assert_not_reached();
> > +}
> > +
> > +static inline void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map)
> > +{
> > +    g_assert_not_reached();
> > +}
> >  
> > -static inline void kvm_arm_get_virtual_time(CPUState *cs) {}
> > -static inline void kvm_arm_put_virtual_time(CPUState *cs) {}
> >  #endif
> >  
> >  static inline const char *gic_class_name(void)
> > 
> 
> Besides,
> Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks,
drew



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 4/5] hw/arm/virt: Move kvm pmu setup to virt_cpu_post_init
  2020-08-12 10:16   ` Auger Eric
@ 2020-08-12 10:35     ` Andrew Jones
  0 siblings, 0 replies; 14+ messages in thread
From: Andrew Jones @ 2020-08-12 10:35 UTC (permalink / raw)
  To: Auger Eric; +Cc: peter.maydell, qemu-arm, qemu-devel

On Wed, Aug 12, 2020 at 12:16:31PM +0200, Auger Eric wrote:
> Hi Drew,
> On 8/5/20 11:16 AM, Andrew Jones wrote:
> > Move the KVM PMU setup part of fdt_add_pmu_nodes() to
> > virt_cpu_post_init(), which is a more appropriate location. Now
> > fdt_add_pmu_nodes() is also named more appropriately, because it
> > no longer does anything but fdt node creation.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  hw/arm/virt.c | 34 ++++++++++++++++++----------------
> >  1 file changed, 18 insertions(+), 16 deletions(-)
> > 
> > diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> > index 2cba21fe3ad9..6797eb397a7a 100644
> > --- a/hw/arm/virt.c
> > +++ b/hw/arm/virt.c
> > @@ -521,21 +521,12 @@ static void fdt_add_gic_node(VirtMachineState *vms)
> >  
> >  static void fdt_add_pmu_nodes(const VirtMachineState *vms)
> >  {
> > -    CPUState *cpu;
> > -    ARMCPU *armcpu;
> > +    ARMCPU *armcpu = ARM_CPU(first_cpu);
> >      uint32_t irqflags = GIC_FDT_IRQ_FLAGS_LEVEL_HI;
> >  
> > -    CPU_FOREACH(cpu) {
> > -        armcpu = ARM_CPU(cpu);
> > -        if (!arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
> > -            return;
> > -        }
> > -        if (kvm_enabled()) {
> > -            if (kvm_irqchip_in_kernel()) {
> > -                kvm_arm_pmu_set_irq(cpu, PPI(VIRTUAL_PMU_IRQ));
> > -            }
> > -            kvm_arm_pmu_init(cpu);
> > -        }
> > +    if (!arm_feature(&armcpu->env, ARM_FEATURE_PMU)) {
> > +        assert(!object_property_get_bool(OBJECT(armcpu), "pmu", NULL));
> I don't rget the relevance of the assert. If the PMU is set, isn't is
> the consequence of arm_set_pmu?

It's just defensive coding to ensure the property matches the feature
flag.

> > +        return;
> >      }
> >  
> >      if (vms->gic_version == VIRT_GIC_VERSION_2) {
> > @@ -544,7 +535,6 @@ static void fdt_add_pmu_nodes(const VirtMachineState *vms)
> >                               (1 << vms->smp_cpus) - 1);
> >      }
> >  
> > -    armcpu = ARM_CPU(qemu_get_cpu(0));
> >      qemu_fdt_add_subnode(vms->fdt, "/pmu");
> >      if (arm_feature(&armcpu->env, ARM_FEATURE_V8)) {
> >          const char compat[] = "arm,armv8-pmuv3";
> > @@ -1678,11 +1668,23 @@ static void finalize_gic_version(VirtMachineState *vms)
> >   */
> >  static void virt_cpu_post_init(VirtMachineState *vms)
> >  {
> > -    bool aarch64;
> > +    bool aarch64, pmu;
> > +    CPUState *cpu;
> >  
> >      aarch64 = object_property_get_bool(OBJECT(first_cpu), "aarch64", NULL);
> > +    pmu = object_property_get_bool(OBJECT(first_cpu), "pmu", NULL);
> >  
> > -    if (!kvm_enabled()) {
> > +    if (kvm_enabled()) {
> > +        CPU_FOREACH(cpu) {
> > +            if (pmu) {
> > +                assert(arm_feature(&ARM_CPU(cpu)->env, ARM_FEATURE_PMU));
> same here?

It's the same defensive check. Actually the check in fdt_add_pmu_nodes()
can definitely be removed, since this check will have already caught
anything before fdt_add_pmu_nodes() can run. We could probably just
remove both though.

> > +                if (kvm_irqchip_in_kernel()) {
> > +                    kvm_arm_pmu_set_irq(cpu, PPI(VIRTUAL_PMU_IRQ));
> > +                }
> > +                kvm_arm_pmu_init(cpu);
> > +            }
> > +        }
> > +    } else {
> >          if (aarch64 && vms->highmem) {
> >              int requested_pa_size = 64 - clz64(vms->highest_gpa);
> >              int pamax = arm_pamax(ARM_CPU(first_cpu));
> > 
> Thanks
> 
> Eric

Thanks,
drew



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 5/5] hw/arm/virt: Implement kvm-steal-time
  2020-08-05  9:16 ` [PATCH v2 5/5] hw/arm/virt: Implement kvm-steal-time Andrew Jones
@ 2020-08-12 12:41   ` Auger Eric
  0 siblings, 0 replies; 14+ messages in thread
From: Auger Eric @ 2020-08-12 12:41 UTC (permalink / raw)
  To: Andrew Jones, qemu-devel, qemu-arm; +Cc: peter.maydell

Drew,

On 8/5/20 11:16 AM, Andrew Jones wrote:
> We add the kvm-steal-time CPU property and implement it for machvirt.
> A tiny bit of refactoring was also done to allow pmu and pvtime to
> use the same vcpu device helper functions.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  docs/system/arm/cpu-features.rst | 11 +++++
>  hw/arm/virt.c                    | 43 ++++++++++++++++--
>  include/hw/arm/virt.h            |  5 +++
>  target/arm/cpu.c                 | 10 +++++
>  target/arm/cpu.h                 |  4 ++
>  target/arm/kvm.c                 | 20 +++++++++
>  target/arm/kvm32.c               |  5 +++
>  target/arm/kvm64.c               | 77 +++++++++++++++++++++++++++++---
>  target/arm/kvm_arm.h             | 30 +++++++++++++
>  target/arm/monitor.c             |  2 +-
>  tests/qtest/arm-cpu-features.c   | 25 +++++++++--
>  11 files changed, 219 insertions(+), 13 deletions(-)
> 
> diff --git a/docs/system/arm/cpu-features.rst b/docs/system/arm/cpu-features.rst
> index 2d5c06cd016b..35196a6b759d 100644
> --- a/docs/system/arm/cpu-features.rst
> +++ b/docs/system/arm/cpu-features.rst
> @@ -200,6 +200,17 @@ the list of KVM VCPU features and their descriptions.
>                             adjustment, also restoring the legacy (pre-5.0)
>                             behavior.
>  
> +  kvm-steal-time           Since v5.2, kvm-steal-time is enabled by
> +                           default when KVM is enabled, the feature is
> +                           supported, and the guest is 64-bit.
> +
> +                           When kvm-steal-time is enabled a 64-bit guest
> +                           can account for time its CPUs were not running
> +                           due to the host not scheduling the corresponding
> +                           VCPU threads.  The accounting statistics may
> +                           influence the guest scheduler behavior and/or be
> +                           exposed to the guest userspace.
> +
>  SVE CPU Properties
>  ==================
>  
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 6797eb397a7a..12efc2f095cb 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -151,6 +151,7 @@ static const MemMapEntry base_memmap[] = {
>      [VIRT_PCDIMM_ACPI] =        { 0x09070000, MEMORY_HOTPLUG_IO_LEN },
>      [VIRT_ACPI_GED] =           { 0x09080000, ACPI_GED_EVT_SEL_LEN },
>      [VIRT_NVDIMM_ACPI] =        { 0x09090000, NVDIMM_ACPI_IO_LEN},
> +    [VIRT_PVTIME] =             { 0x090a0000, 0x00010000 },
>      [VIRT_MMIO] =               { 0x0a000000, 0x00000200 },
>      /* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
>      [VIRT_PLATFORM_BUS] =       { 0x0c000000, 0x02000000 },
> @@ -1666,15 +1667,39 @@ static void finalize_gic_version(VirtMachineState *vms)
>   * virt_cpu_post_init() must be called after the CPUs have
>   * been realized and the GIC has been created.
>   */
> -static void virt_cpu_post_init(VirtMachineState *vms)
> +static void virt_cpu_post_init(VirtMachineState *vms, int max_cpus,
> +                               MemoryRegion *sysmem)
>  {
> -    bool aarch64, pmu;
> +    bool aarch64, pmu, steal_time;
>      CPUState *cpu;
>  
>      aarch64 = object_property_get_bool(OBJECT(first_cpu), "aarch64", NULL);
>      pmu = object_property_get_bool(OBJECT(first_cpu), "pmu", NULL);
> +    steal_time = object_property_get_bool(OBJECT(first_cpu),
> +                                          "kvm-steal-time", NULL);
>  
>      if (kvm_enabled()) {> +        hwaddr pvtime_reg_base = vms->memmap[VIRT_PVTIME].base;
> +        hwaddr pvtime_reg_size = vms->memmap[VIRT_PVTIME].size;
> +
> +        if (steal_time) {
> +            MemoryRegion *pvtime = g_new(MemoryRegion, 1);
> +            hwaddr pvtime_size = max_cpus * PVTIME_SIZE_PER_CPU;
> +
> +            /* The memory region size must be a multiple of host page size. */
> +            pvtime_size = REAL_HOST_PAGE_ALIGN(pvtime_size);
> +
> +            if (pvtime_size > pvtime_reg_size) {
> +                error_report("pvtime requires a %ld byte memory region for "
> +                             "%d CPUs, but only %ld has been reserved",
> +                             pvtime_size, max_cpus, pvtime_reg_size);
> +                exit(1);
> +            }
> +
> +            memory_region_init_ram(pvtime, NULL, "pvtime", pvtime_size, NULL);
> +            memory_region_add_subregion(sysmem, pvtime_reg_base, pvtime);
> +        }
> +
>          CPU_FOREACH(cpu) {
>              if (pmu) {
>                  assert(arm_feature(&ARM_CPU(cpu)->env, ARM_FEATURE_PMU));
> @@ -1683,6 +1708,10 @@ static void virt_cpu_post_init(VirtMachineState *vms)
>                  }
>                  kvm_arm_pmu_init(cpu);
>              }
> +            if (steal_time) {
> +                kvm_arm_pvtime_init(cpu, pvtime_reg_base +
> +                                         cpu->cpu_index * PVTIME_SIZE_PER_CPU);
> +            }
>          }
>      } else {
>          if (aarch64 && vms->highmem) {
> @@ -1853,6 +1882,11 @@ static void machvirt_init(MachineState *machine)
>              object_property_set_bool(cpuobj, "kvm-no-adjvtime", true, NULL);
>          }
>  
> +        if (vmc->no_kvm_steal_time &&
> +            object_property_find(cpuobj, "kvm-steal-time", NULL)) {
> +            object_property_set_bool(cpuobj, "kvm-steal-time", false, NULL);
> +        }
> +
>          if (vmc->no_pmu && object_property_find(cpuobj, "pmu", NULL)) {
>              object_property_set_bool(cpuobj, "pmu", false, NULL);
>          }
> @@ -1924,7 +1958,7 @@ static void machvirt_init(MachineState *machine)
>  
>      create_gic(vms);
>  
> -    virt_cpu_post_init(vms);
> +    virt_cpu_post_init(vms, possible_cpus->len, sysmem);
>  
>      fdt_add_pmu_nodes(vms);
>  
> @@ -2566,8 +2600,11 @@ DEFINE_VIRT_MACHINE_AS_LATEST(5, 2)
>  
>  static void virt_machine_5_1_options(MachineClass *mc)
>  {
> +    VirtMachineClass *vmc = VIRT_MACHINE_CLASS(OBJECT_CLASS(mc));
> +
>      virt_machine_5_2_options(mc);
>      compat_props_add(mc->compat_props, hw_compat_5_1, hw_compat_5_1_len);
> +    vmc->no_kvm_steal_time = true;
>  }
>  DEFINE_VIRT_MACHINE(5, 1)
>  
> diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
> index dff67e1bef03..5fb43a009f7a 100644
> --- a/include/hw/arm/virt.h
> +++ b/include/hw/arm/virt.h
> @@ -53,6 +53,9 @@
>  
>  #define PPI(irq) ((irq) + 16)
>  
> +/* See Linux kernel arch/arm64/include/asm/pvclock-abi.h */
> +#define PVTIME_SIZE_PER_CPU 64
> +
>  enum {
>      VIRT_FLASH,
>      VIRT_MEM,
> @@ -80,6 +83,7 @@ enum {
>      VIRT_PCDIMM_ACPI,
>      VIRT_ACPI_GED,
>      VIRT_NVDIMM_ACPI,
> +    VIRT_PVTIME,
>      VIRT_LOWMEMMAP_LAST,
>  };
>  
> @@ -126,6 +130,7 @@ typedef struct {
>      bool no_ged;   /* Machines < 4.2 has no support for ACPI GED device */
>      bool kvm_no_adjvtime;
>      bool acpi_expose_flash;
> +    bool no_kvm_steal_time;
>  } VirtMachineClass;
>  
>  typedef struct {
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 111579554fb9..866d0a9fb146 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1308,6 +1308,16 @@ void arm_cpu_finalize_features(ARMCPU *cpu, Error **errp)
>              return;
>          }
>      }
> +
> +    if (kvm_enabled()) {
> +#ifdef TARGET_AARCH64
> +        kvm_arm_steal_time_finalize(cpu, &local_err);
> +        if (local_err != NULL) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +#endif
> +    }
>  }
>  
>  static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 9e8ed423ea1d..a4d4cb640c77 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -24,6 +24,7 @@
>  #include "hw/registerfields.h"
>  #include "cpu-qom.h"
>  #include "exec/cpu-defs.h"
> +#include "qapi/qapi-types-common.h"
>  
>  /* ARM processors have a weak memory model */
>  #define TCG_GUEST_DEFAULT_MO      (0)
> @@ -859,6 +860,9 @@ struct ARMCPU {
>      bool kvm_vtime_dirty;
>      uint64_t kvm_vtime;
>  
> +    /* KVM steal time */
> +    OnOffAuto kvm_steal_time;
> +
>      /* Uniprocessor system with MP extensions */
>      bool mp_is_up;
>  
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 8bb7318378b5..093a290453f6 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -192,6 +192,18 @@ static void kvm_no_adjvtime_set(Object *obj, bool value, Error **errp)
>      ARM_CPU(obj)->kvm_adjvtime = !value;
>  }
>  
> +#ifdef TARGET_AARCH64
> +static bool kvm_steal_time_get(Object *obj, Error **errp)
> +{
> +    return ARM_CPU(obj)->kvm_steal_time != ON_OFF_AUTO_OFF;
> +}
> +
> +static void kvm_steal_time_set(Object *obj, bool value, Error **errp)
> +{
> +    ARM_CPU(obj)->kvm_steal_time = value ? ON_OFF_AUTO_ON : ON_OFF_AUTO_OFF;
> +}
> +#endif
> +
>  /* KVM VCPU properties should be prefixed with "kvm-". */
>  void kvm_arm_add_vcpu_properties(Object *obj)
>  {
> @@ -207,6 +219,14 @@ void kvm_arm_add_vcpu_properties(Object *obj)
>                                          "the virtual counter. VM stopped time "
>                                          "will be counted.");
>      }
> +
> +#ifdef TARGET_AARCH64
> +    cpu->kvm_steal_time = ON_OFF_AUTO_AUTO;
> +    object_property_add_bool(obj, "kvm-steal-time", kvm_steal_time_get,
> +                             kvm_steal_time_set);
> +    object_property_set_description(obj, "kvm-steal-time",
> +                                    "Set off to disable KVM steal time.");
> +#endif
>  }
>  
>  bool kvm_arm_pmu_supported(void)
> diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
> index 0af46b41c847..d3f3195077fa 100644
> --- a/target/arm/kvm32.c
> +++ b/target/arm/kvm32.c
> @@ -560,6 +560,11 @@ void kvm_arm_pmu_init(CPUState *cs)
>      qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
>  }
>  
> +void kvm_arm_pvtime_init(CPUState *cs, uint64_t ipa)
> +{
> +    qemu_log_mask(LOG_UNIMP, "%s: not implemented\n", __func__);
> +}
> +
>  #define ARM_REG_DFSR  ARM_CP15_REG32(0, 5, 0, 0)
>  #define ARM_REG_TTBCR ARM_CP15_REG32(0, 2, 0, 2)
>  /*
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index 116923790550..4de1eeb723ac 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -17,6 +17,7 @@
>  #include <linux/kvm.h>
>  
>  #include "qemu-common.h"
> +#include "qapi/error.h"
>  #include "cpu.h"
>  #include "qemu/timer.h"
>  #include "qemu/error-report.h"
> @@ -398,19 +399,20 @@ static CPUWatchpoint *find_hw_watchpoint(CPUState *cpu, target_ulong addr)
>      return NULL;
>  }
>  
> -static bool kvm_arm_pmu_set_attr(CPUState *cs, struct kvm_device_attr *attr)
> +static bool kvm_arm_set_device_attr(CPUState *cs, struct kvm_device_attr *attr,
> +                                    const char *name)
>  {
>      int err;
>  
>      err = kvm_vcpu_ioctl(cs, KVM_HAS_DEVICE_ATTR, attr);
>      if (err != 0) {
> -        error_report("PMU: KVM_HAS_DEVICE_ATTR: %s", strerror(-err));
> +        error_report("%s: KVM_HAS_DEVICE_ATTR: %s", name, strerror(-err));
>          return false;
>      }
>  
>      err = kvm_vcpu_ioctl(cs, KVM_SET_DEVICE_ATTR, attr);
>      if (err != 0) {
> -        error_report("PMU: KVM_SET_DEVICE_ATTR: %s", strerror(-err));
> +        error_report("%s: KVM_SET_DEVICE_ATTR: %s", name, strerror(-err));
>          return false;
>      }
>  
> @@ -427,7 +429,7 @@ void kvm_arm_pmu_init(CPUState *cs)
>      if (!ARM_CPU(cs)->has_pmu) {
>          return;
>      }
> -    if (!kvm_arm_pmu_set_attr(cs, &attr)) {
> +    if (!kvm_arm_set_device_attr(cs, &attr, "PMU")) {
>          error_report("failed to init PMU");
>          abort();
>      }
> @@ -444,12 +446,29 @@ void kvm_arm_pmu_set_irq(CPUState *cs, int irq)
>      if (!ARM_CPU(cs)->has_pmu) {
>          return;
>      }
> -    if (!kvm_arm_pmu_set_attr(cs, &attr)) {
> +    if (!kvm_arm_set_device_attr(cs, &attr, "PMU")) {
>          error_report("failed to set irq for PMU");
>          abort();
>      }
>  }
>  
> +void kvm_arm_pvtime_init(CPUState *cs, uint64_t ipa)
> +{
> +    struct kvm_device_attr attr = {
> +        .group = KVM_ARM_VCPU_PVTIME_CTRL,
> +        .attr = KVM_ARM_VCPU_PVTIME_IPA,
> +        .addr = (uint64_t)&ipa,
> +    };
> +
> +    if (ARM_CPU(cs)->kvm_steal_time == ON_OFF_AUTO_OFF) {
> +        return;
> +    }
> +    if (!kvm_arm_set_device_attr(cs, &attr, "PVTIME IPA")) {
> +        error_report("failed to init PVTIME IPA");
> +        abort();
> +    }
> +}
> +
>  static int read_sys_reg32(int fd, uint32_t *pret, uint64_t id)
>  {
>      uint64_t ret;
> @@ -652,6 +671,54 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>      return true;
>  }
>  
> +void kvm_arm_steal_time_finalize(ARMCPU *cpu, Error **errp)
> +{
> +    static bool has_steal_time;
> +    static bool probed;
> +    int fdarray[3];
> +
> +    if (!probed) {
> +        probed = true;
> +        if (kvm_check_extension(kvm_state, KVM_CAP_VCPU_ATTRIBUTES)) {
> +            if (!kvm_arm_create_scratch_host_vcpu(NULL, fdarray, NULL)) {
> +                error_report("Failed to create scratch VCPU");
> +                abort();
> +            }
> +
> +            has_steal_time = kvm_device_check_attr(fdarray[2],
> +                                                   KVM_ARM_VCPU_PVTIME_CTRL,
> +                                                   KVM_ARM_VCPU_PVTIME_IPA);
> +
> +            kvm_arm_destroy_scratch_host_vcpu(fdarray);
> +        }
> +    }
> +
> +    if (cpu->kvm_steal_time == ON_OFF_AUTO_AUTO) {
> +        if (!has_steal_time || !arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
> +            cpu->kvm_steal_time = ON_OFF_AUTO_OFF;
> +        } else {
> +            cpu->kvm_steal_time = ON_OFF_AUTO_ON;
> +        }
> +    } else if (cpu->kvm_steal_time == ON_OFF_AUTO_ON) {
> +        if (!has_steal_time) {
> +            error_setg(errp, "'kvm-steal-time' cannot be enabled "
> +                             "on this host");
> +            return;
> +        } else if (!arm_feature(&cpu->env, ARM_FEATURE_AARCH64)) {
> +            /*
> +             * DEN0057A chapter 2 says "This specification only covers
> +             * systems in which the Execution state of the hypervisor
> +             * as well as EL1 of virtual machines is AArch64.". And,
> +             * to ensure that, the smc/hvc calls are only specified as
> +             * smc64/hvc64.
> +             */
> +            error_setg(errp, "'kvm-steal-time' cannot be enabled "
> +                             "for AArch32 guests");
> +            return;
> +        }
> +    }
> +}
> +
>  bool kvm_arm_aarch32_supported(void)
>  {
>      return kvm_check_extension(kvm_state, KVM_CAP_ARM_EL1_32BIT);
> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> index 0da00eb6b20c..4a8969505967 100644
> --- a/target/arm/kvm_arm.h
> +++ b/target/arm/kvm_arm.h
> @@ -267,6 +267,16 @@ void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu);
>   */
>  void kvm_arm_add_vcpu_properties(Object *obj);
>  
> +/**
> + * kvm_arm_steal_time_finalize:
> + * @cpu: ARMCPU for which to finalize kvm-steal-time
> + * @errp: Pointer to Error* for error propagation
> + *
> + * Validate the kvm-steal-time property selection and set its default
> + * based on KVM support and guest configuration.
> + */
> +void kvm_arm_steal_time_finalize(ARMCPU *cpu, Error **errp);
> +
>  /**
>   * kvm_arm_aarch32_supported:
>   *
> @@ -340,6 +350,16 @@ int kvm_arm_vgic_probe(void);
>  
>  void kvm_arm_pmu_set_irq(CPUState *cs, int irq);
>  void kvm_arm_pmu_init(CPUState *cs);
> +
> +/**
> + * kvm_arm_pvtime_init:
> + * @cs: CPUState
> + * @ipa: Per-vcpu guest physical base address of the pvtime structures
> + *
> + * Initializes PVTIME for the VCPU, setting the PVTIME IPA to @ipa.
> + */
> +void kvm_arm_pvtime_init(CPUState *cs, uint64_t ipa);
> +
>  int kvm_arm_set_irq(int cpu, int irqtype, int irq, int level);
>  
>  #else
> @@ -393,6 +413,16 @@ static inline void kvm_arm_pmu_init(CPUState *cs)
>      g_assert_not_reached();
>  }
>  
> +static inline void kvm_arm_pvtime_init(CPUState *cs, uint64_t ipa)
> +{
> +    g_assert_not_reached();
> +}
> +
> +static inline void kvm_arm_steal_time_finalize(ARMCPU *cpu, Error **errp)
> +{
> +    g_assert_not_reached();
> +}
> +
>  static inline void kvm_arm_sve_get_vls(CPUState *cs, unsigned long *map)
>  {
>      g_assert_not_reached();
> diff --git a/target/arm/monitor.c b/target/arm/monitor.c
> index ba6e01abd037..bd3590604a71 100644
> --- a/target/arm/monitor.c
> +++ b/target/arm/monitor.c
> @@ -103,7 +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",
> +    "kvm-no-adjvtime", "kvm-steal-time",
>      NULL
>  };
>  
> diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
> index f7e062c1891e..91b181f38268 100644
> --- a/tests/qtest/arm-cpu-features.c
> +++ b/tests/qtest/arm-cpu-features.c
> @@ -452,6 +452,7 @@ static void test_query_cpu_model_expansion(const void *data)
>      assert_set_feature(qts, "max", "pmu", true);
>  
>      assert_has_not_feature(qts, "max", "kvm-no-adjvtime");
> +    assert_has_not_feature(qts, "max", "kvm-steal-time");
>  
>      if (g_str_equal(qtest_get_arch(), "aarch64")) {
>          assert_has_feature_enabled(qts, "max", "aarch64");
> @@ -493,6 +494,7 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
>      assert_set_feature(qts, "host", "kvm-no-adjvtime", false);
>  
>      if (g_str_equal(qtest_get_arch(), "aarch64")) {
> +        bool kvm_supports_steal_time;
>          bool kvm_supports_sve;
>          char max_name[8], name[8];
>          uint32_t max_vq, vq;
> @@ -500,6 +502,10 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
>          QDict *resp;
>          char *error;
>  
> +        assert_error(qts, "cortex-a15",
> +            "We cannot guarantee the CPU type 'cortex-a15' works "
> +            "with KVM on this host", NULL);
> +
>          assert_has_feature_enabled(qts, "host", "aarch64");
>  
>          /* Enabling and disabling pmu should always work. */
> @@ -507,16 +513,26 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
>          assert_set_feature(qts, "host", "pmu", false);
>          assert_set_feature(qts, "host", "pmu", true);
>  
> -        assert_error(qts, "cortex-a15",
> -            "We cannot guarantee the CPU type 'cortex-a15' works "
> -            "with KVM on this host", NULL);
> -
> +        /*
> +         * Some features would be enabled by default, but they're disabled
> +         * because this instance of KVM doesn't support them. Test that the
> +         * features are present, and, when enabled, issue further tests.
> +         */
> +        assert_has_feature(qts, "host", "kvm-steal-time");
>          assert_has_feature(qts, "host", "sve");
> +
>          resp = do_query_no_props(qts, "host");
> +        kvm_supports_steal_time = resp_get_feature(resp, "kvm-steal-time");
>          kvm_supports_sve = resp_get_feature(resp, "sve");
>          vls = resp_get_sve_vls(resp);
>          qobject_unref(resp);
>  
> +        if (kvm_supports_steal_time) {
> +            /* If we have steal-time then we should be able to toggle it. */
> +            assert_set_feature(qts, "host", "kvm-steal-time", false);
> +            assert_set_feature(qts, "host", "kvm-steal-time", true);
> +        }
> +
>          if (kvm_supports_sve) {
>              g_assert(vls != 0);
>              max_vq = 64 - __builtin_clzll(vls);
> @@ -577,6 +593,7 @@ static void test_query_cpu_model_expansion_kvm(const void *data)
>          assert_has_not_feature(qts, "host", "aarch64");
>          assert_has_not_feature(qts, "host", "pmu");
>          assert_has_not_feature(qts, "host", "sve");
> +        assert_has_not_feature(qts, "host", "kvm-steal-time");
>      }
>  
>      qtest_quit(qts);
> 
This looks good to me
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 4/5] hw/arm/virt: Move kvm pmu setup to virt_cpu_post_init
  2020-08-05  9:16 ` [PATCH v2 4/5] hw/arm/virt: Move kvm pmu setup to virt_cpu_post_init Andrew Jones
  2020-08-12 10:16   ` Auger Eric
@ 2020-08-25 14:27   ` Peter Maydell
  1 sibling, 0 replies; 14+ messages in thread
From: Peter Maydell @ 2020-08-25 14:27 UTC (permalink / raw)
  To: Andrew Jones; +Cc: qemu-arm, QEMU Developers

On Wed, 5 Aug 2020 at 10:16, Andrew Jones <drjones@redhat.com> wrote:
>
> Move the KVM PMU setup part of fdt_add_pmu_nodes() to
> virt_cpu_post_init(), which is a more appropriate location. Now
> fdt_add_pmu_nodes() is also named more appropriately, because it
> no longer does anything but fdt node creation.
>
> No functional change intended.
>
> Signed-off-by: Andrew Jones <drjones@redhat.com>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v2 0/5] hw/arm/virt: Introduce kvm-steal-time
  2020-08-05  9:16 [PATCH v2 0/5] hw/arm/virt: Introduce kvm-steal-time Andrew Jones
                   ` (4 preceding siblings ...)
  2020-08-05  9:16 ` [PATCH v2 5/5] hw/arm/virt: Implement kvm-steal-time Andrew Jones
@ 2020-09-07  7:04 ` Andrew Jones
  5 siblings, 0 replies; 14+ messages in thread
From: Andrew Jones @ 2020-09-07  7:04 UTC (permalink / raw)
  To: qemu-devel, qemu-arm; +Cc: peter.maydell


FYI, the new KVM cap is on its way into the kernel and Peter has submit
patches to remove kvm32 from qemu. So I'll be refreshing this series to
incorporate both those changes.

Thanks,
drew


On Wed, Aug 05, 2020 at 11:16:35AM +0200, Andrew Jones wrote:
> v2:
>   - Changed the introduction of the feature to 5.2 from 5.1
>     (The 5.2 machine type patch posted by Cornelia was thrown
>      in for completeness, but I suppose that'll get picked up
>      separately.)
>   - Added a patch adding g_assert_not_reached() to many KVM
>     stubs. (This isn't exactly related to the series, but the
>     series does add two more stubs that can now be added in
>     the same way.)
>   - Fixed a patch that wasn't suppose to have a functional
>     change, but did (fdt_add_gic_node() shouldn't generate
>     the node without the PMU) [Peter]
>   - Pass sysmem to virt_cpu_post_init() [Peter]
>   - Introduced a define for the pvtime struct size
>   - Calculate the pvtime memory region size based on max-cpus
>     and host-page-size [Beata]
>   - Renamed kvm_no_steal_time to no_kvm_steal_time [Peter]
>   - Fixed a parameter misordering with object_property_set_bool()
>   - Added a comment explaining why the feature isn't supported
>     for AArch32 guests
>   - Changed a !kvm_steal_time to a kvm_steal_time == ON_OFF_AUTO_OFF
>     as it should be
>   - Picked up one r-b from Peter
> 
> 
> KVM supports the ability to publish the amount of time that VCPUs
> were runnable, but not running due to other host threads running
> instead, to the guest. The guest scheduler may use that information
> when making decisions and the guest may expose it to its userspace
> (Linux publishes this information in /proc/stat). This feature is
> called "steal time" as it represents the amount of time stolen from
> a guest by scheduling out its VCPUs. To enable this feature KVM
> userspace must provide a memory region that will be used to publish
> the information to the guest. The memory region is typical migratable
> region. The GPA of the region is given to KVM through a VCPU device
> ioctl interface. This feature is only available for 64-bit hosts
> running 64-bit guests.
> 
> This series provides the QEMU support of this feature. It will
> be enabled by default for 5.2 machine types and later, but may
> be disabled with a new CPU property "kvm-steal-time".
> 
> While testing migration it was observed that the amount of
> steal time as viewed by the guest was getting reset on each
> migration. Patch 4/6 of a pvtime fix series posted[*] for KVM
> should fix that. Also, we may still want to change the way we
> probe KVM for the feature in this QEMU series to the new KVM
> cap proposed in that KVM series.
> 
> Migration testing:
> 
> * virt-5.1 can migrate as usual, no steal-time enabled
> 
> * virt-5.2 can migrate between hosts with steal-time enabled
>   (the default) and disabled when both hosts support steal-time
> 
> * virt-5.2 with steal-time disabled can migrate to a host that
>   does not support steal-time
> 
> * virt-5.2 with steal-time enabled will cleanly fail when migrating
>   to a host that does not support steal-time
> 
> * virt-5.2 without the kvm-steal-time property specified can
>   boot on a host that does not support the feature - the feature
>   will be disabled. However, if the guest is migrated to a host
>   that does support the feature, then after the guest has reboot
>   the feature will become available with the guest kernel supports
>   it (this is the nature of migrating guests that use '-cpu host').
>   Additionally, once this guest has been migrated to a host that
>   does have the feature, then it will fail (cleanly) to migrate to
>   a host that does not have the feature. If this behavior isn't
>   desired, then the user should explicitly disable steal-time with
>   the kvm-steal-time property if they want to boot a 5.2 guest on
>   a host that doesn't support steal-time.
> 
> [*] https://lists.cs.columbia.edu/pipermail/kvmarm/2020-August/041823.html
> 
> Thanks,
> drew
> 
> 
> Andrew Jones (4):
>   target/arm/kvm: Make uncalled stubs explicitly unreachable
>   hw/arm/virt: Move post cpu realize check into its own function
>   hw/arm/virt: Move kvm pmu setup to virt_cpu_post_init
>   hw/arm/virt: Implement kvm-steal-time
> 
> Cornelia Huck (1):
>   hw: add compat machines for 5.2
> 
>  docs/system/arm/cpu-features.rst |  11 +++
>  hw/arm/virt.c                    | 119 +++++++++++++++++++++++--------
>  hw/core/machine.c                |   3 +
>  hw/i386/pc.c                     |   6 +-
>  hw/i386/pc_piix.c                |  14 +++-
>  hw/i386/pc_q35.c                 |  13 +++-
>  hw/ppc/spapr.c                   |  15 +++-
>  hw/s390x/s390-virtio-ccw.c       |  14 +++-
>  include/hw/arm/virt.h            |   5 ++
>  include/hw/boards.h              |   3 +
>  include/hw/i386/pc.h             |   3 +
>  target/arm/cpu.c                 |  10 +++
>  target/arm/cpu.h                 |   4 ++
>  target/arm/kvm.c                 |  20 ++++++
>  target/arm/kvm32.c               |   5 ++
>  target/arm/kvm64.c               |  77 ++++++++++++++++++--
>  target/arm/kvm_arm.h             |  74 ++++++++++++++-----
>  target/arm/monitor.c             |   2 +-
>  tests/qtest/arm-cpu-features.c   |  25 +++++--
>  19 files changed, 358 insertions(+), 65 deletions(-)
> 
> -- 
> 2.25.4
> 



^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-09-07  7:06 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-05  9:16 [PATCH v2 0/5] hw/arm/virt: Introduce kvm-steal-time Andrew Jones
2020-08-05  9:16 ` [PATCH v2 1/5] hw: add compat machines for 5.2 Andrew Jones
2020-08-05  9:16 ` [PATCH v2 2/5] target/arm/kvm: Make uncalled stubs explicitly unreachable Andrew Jones
2020-08-12 10:16   ` Auger Eric
2020-08-12 10:26     ` Andrew Jones
2020-08-05  9:16 ` [PATCH v2 3/5] hw/arm/virt: Move post cpu realize check into its own function Andrew Jones
2020-08-12 10:16   ` Auger Eric
2020-08-05  9:16 ` [PATCH v2 4/5] hw/arm/virt: Move kvm pmu setup to virt_cpu_post_init Andrew Jones
2020-08-12 10:16   ` Auger Eric
2020-08-12 10:35     ` Andrew Jones
2020-08-25 14:27   ` Peter Maydell
2020-08-05  9:16 ` [PATCH v2 5/5] hw/arm/virt: Implement kvm-steal-time Andrew Jones
2020-08-12 12:41   ` Auger Eric
2020-09-07  7:04 ` [PATCH v2 0/5] hw/arm/virt: Introduce kvm-steal-time Andrew Jones

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).