[Qemu-devel] [PATCH 0/6] arm: support -cpu max (and gic-version=max)

[Qemu-devel] [PATCH 0/6] arm: support -cpu max (and gic-version=max)

[Peter Maydell]

This patchset adds support for '-cpu max' to Arm, along the lines
of the existing support we have for x86 targets:

 * under KVM, -cpu max is the same as -cpu host
 * under TCG, -cpu max means "emulate with as many features as
   possible"

-cpu max is supported for both usermode and system emulation,
again following the x86 line.

NB that cross-QEMU-version migration is not supported for -cpu max:
in future the definition of "maximum set of features" will change
as we add more emulation features.

The patchset also adds support to the virt board for the "max"
option to -machine gic-version, requesting "best available
interrupt controller", with the same semantics as -cpu max.

Together these should assist users like libguestfs that just want
to be able to run code without having to figure out what the
right command line arguments for this particular host system are.


Patch 1 is a bugfix, needed because now "-cpu host" without
-enable-kvm will only be detected when the CPU object fails
realize, rather than because we don't register the 'host'
CPU type at all.

(This is something that I meant to do much earlier: see
https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg06183.html
but at the time we were about to freeze for 2.9, so it fell
off my immediate todo list and then I never got back to it...)

thanks
-- PMM

Peter Maydell (6):
  hw/arm/virt: Check that the CPU realize method succeeded
  target/arm: Query host CPU features on-demand at instance init
  target/arm: Move definition of 'host' cpu type into cpu.c
  target/arm: Add "-cpu max" support
  hw/arm/virt: Add "max" to the list of CPU types "virt" supports
  hw/arm/virt: Support -machine gic-version=max

 target/arm/cpu-qom.h |  2 ++
 target/arm/cpu.h     |  5 +++++
 target/arm/kvm_arm.h | 35 ++++++++++++++++++++----------
 hw/arm/virt.c        | 32 +++++++++++++++++----------
 target/arm/cpu.c     | 61 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 target/arm/cpu64.c   | 21 ++++++++++++++++++
 target/arm/kvm.c     | 51 +++++++++++++++----------------------------
 target/arm/kvm32.c   |  8 +++----
 target/arm/kvm64.c   |  8 +++----
 9 files changed, 159 insertions(+), 64 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/6] hw/arm/virt: Check that the CPU realize method succeeded

[Peter Maydell]

We were passing a NULL error pointer to the object_property_set_bool()
call that realizes the CPU object. This meant that we wouldn't detect
failure, and would plough blindly on to crash later trying to use a
NULL CPU object pointer. Detect errors and fail instead.

In particular, this will be necessary to detect the user error
of using "-cpu host" without "-enable-kvm" once we make the host
CPU type be registered unconditionally rather than only in
kvm_arch_init().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 151592b..62af013 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1402,7 +1402,7 @@ static void machvirt_init(MachineState *machine)
                                      "secure-memory", &error_abort);
         }
 
-        object_property_set_bool(cpuobj, true, "realized", NULL);
+        object_property_set_bool(cpuobj, true, "realized", &error_fatal);
         object_unref(cpuobj);
     }
     fdt_add_timer_nodes(vms);
-- 
2.7.4

[Qemu-devel] [PATCH 2/6] target/arm: Query host CPU features on-demand at instance init

[Peter Maydell]

Currently we query the host CPU features in the class init function
for the TYPE_ARM_HOST_CPU class, so that we can later copy them
from the class object into the instance object in the object
instance init function. This is awkward for implementing "-cpu max",
which should work like "-cpu host" for KVM but like "cpu with all
implemented features" for TCG.

Move the place where we store the information about the host CPU from
a class object to static variables in kvm.c, and then in the instance
init function call a new kvm_arm_set_cpu_features_from_host()
function which will query the host kernel if necessary and then
fill in the CPU instance fields.

This allows us to drop the special class struct and class init
function for TYPE_ARM_HOST_CPU entirely.

We can't delay the probe until realize, because the ARM
instance_post_init hook needs to look at the feature bits we
set, so we need to do it in the initfn. This is safe because
the probing doesn't affect the actual VM state (it creates a
separate scratch VM to do its testing), but the probe might fail.
Because we can't report errors in retrieving the host features
in the initfn, we check this belatedly in the realize function
(the intervening code will be able to cope with the relevant
fields in the CPU structure being zero).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.h     |  5 +++++
 target/arm/kvm_arm.h | 35 ++++++++++++++++++++++++-----------
 target/arm/cpu.c     | 13 +++++++++++++
 target/arm/kvm.c     | 36 +++++++++++++++++++-----------------
 target/arm/kvm32.c   |  8 ++++----
 target/arm/kvm64.c   |  8 ++++----
 6 files changed, 69 insertions(+), 36 deletions(-)

diff --git a/target/arm/cpu.h b/target/arm/cpu.h
index 89d49cd..5b01cf9 100644
--- a/target/arm/cpu.h
+++ b/target/arm/cpu.h
@@ -690,6 +690,11 @@ struct ARMCPU {
     /* Uniprocessor system with MP extensions */
     bool mp_is_up;
 
+    /* True if we tried kvm_arm_host_cpu_features() during CPU instance_init
+     * and the probe failed (so we need to report the error in realize)
+     */
+    bool host_cpu_probe_failed;
+
     /* The instance init functions for implementation-specific subclasses
      * set these fields to specify the implementation-dependent values of
      * various constant registers and reset values of non-constant
diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
index ff53e9f..89d1b67 100644
--- a/target/arm/kvm_arm.h
+++ b/target/arm/kvm_arm.h
@@ -152,20 +152,16 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try,
 void kvm_arm_destroy_scratch_host_vcpu(int *fdarray);
 
 #define TYPE_ARM_HOST_CPU "host-" TYPE_ARM_CPU
-#define ARM_HOST_CPU_CLASS(klass) \
-    OBJECT_CLASS_CHECK(ARMHostCPUClass, (klass), TYPE_ARM_HOST_CPU)
-#define ARM_HOST_CPU_GET_CLASS(obj) \
-    OBJECT_GET_CLASS(ARMHostCPUClass, (obj), TYPE_ARM_HOST_CPU)
-
-typedef struct ARMHostCPUClass {
-    /*< private >*/
-    ARMCPUClass parent_class;
-    /*< public >*/
 
+/**
+ * ARMHostCPUFeatures: information about the host CPU (identified
+ * by asking the host kernel)
+ */
+typedef struct ARMHostCPUFeatures {
     uint64_t features;
     uint32_t target;
     const char *dtb_compatible;
-} ARMHostCPUClass;
+} ARMHostCPUFeatures;
 
 /**
  * kvm_arm_get_host_cpu_features:
@@ -174,8 +170,16 @@ typedef struct ARMHostCPUClass {
  * Probe the capabilities of the host kernel's preferred CPU and fill
  * in the ARMHostCPUClass struct accordingly.
  */
-bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc);
+bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf);
 
+/**
+ * kvm_arm_set_cpu_features_from_host:
+ * @cpu: ARMCPU to set the features for
+ *
+ * Set up the ARMCPU struct fields up to match the information probed
+ * from the host CPU.
+ */
+void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu);
 
 /**
  * kvm_arm_sync_mpstate_to_kvm
@@ -200,6 +204,15 @@ void kvm_arm_pmu_init(CPUState *cs);
 
 #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;
+}
+
 static inline int kvm_arm_vgic_probe(void)
 {
     return 0;
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 7f7a3d1..a7deb10 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -709,6 +709,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
     AddressSpace *as;
 #endif
 
+    /* If we needed to query the host kernel for the CPU features
+     * then it's possible that might have failed in the initfn, but
+     * this is the first point where we can report it.
+     */
+    if (cpu->host_cpu_probe_failed) {
+        if (!kvm_enabled()) {
+            error_setg(errp, "The 'host' CPU type can only be used with KVM");
+        } else {
+            error_setg(errp, "Failed to retrieve host CPU features");
+        }
+        return;
+    }
+
     cpu_exec_realizefn(cs, &local_err);
     if (local_err != NULL) {
         error_propagate(errp, local_err);
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 211a7bf..945696c 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -33,6 +33,8 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
 
 static bool cap_has_mp_state;
 
+static ARMHostCPUFeatures arm_host_cpu_features;
+
 int kvm_arm_vcpu_init(CPUState *cs)
 {
     ARMCPU *cpu = ARM_CPU(cs);
@@ -129,30 +131,32 @@ void kvm_arm_destroy_scratch_host_vcpu(int *fdarray)
     }
 }
 
-static void kvm_arm_host_cpu_class_init(ObjectClass *oc, void *data)
+void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
 {
-    ARMHostCPUClass *ahcc = ARM_HOST_CPU_CLASS(oc);
+    CPUARMState *env = &cpu->env;
 
-    /* All we really need to set up for the 'host' CPU
-     * is the feature bits -- we rely on the fact that the
-     * various ID register values in ARMCPU are only used for
-     * TCG CPUs.
-     */
-    if (!kvm_arm_get_host_cpu_features(ahcc)) {
-        fprintf(stderr, "Failed to retrieve host CPU features!\n");
-        abort();
+    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().
+             */
+            cpu->kvm_target = QEMU_KVM_ARM_TARGET_NONE;
+            cpu->host_cpu_probe_failed = true;
+            return;
+        }
     }
+
+    cpu->kvm_target = arm_host_cpu_features.target;
+    cpu->dtb_compatible = arm_host_cpu_features.dtb_compatible;
+    env->features = arm_host_cpu_features.features;
 }
 
 static void kvm_arm_host_cpu_initfn(Object *obj)
 {
-    ARMHostCPUClass *ahcc = ARM_HOST_CPU_GET_CLASS(obj);
     ARMCPU *cpu = ARM_CPU(obj);
-    CPUARMState *env = &cpu->env;
 
-    cpu->kvm_target = ahcc->target;
-    cpu->dtb_compatible = ahcc->dtb_compatible;
-    env->features = ahcc->features;
+    kvm_arm_set_cpu_features_from_host(cpu);
 }
 
 static const TypeInfo host_arm_cpu_type_info = {
@@ -163,8 +167,6 @@ static const TypeInfo host_arm_cpu_type_info = {
     .parent = TYPE_ARM_CPU,
 #endif
     .instance_init = kvm_arm_host_cpu_initfn,
-    .class_init = kvm_arm_host_cpu_class_init,
-    .class_size = sizeof(ARMHostCPUClass),
 };
 
 int kvm_arch_init(MachineState *ms, KVMState *s)
diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
index f925a21..cc326ea 100644
--- a/target/arm/kvm32.c
+++ b/target/arm/kvm32.c
@@ -28,7 +28,7 @@ static inline void set_feature(uint64_t *features, int feature)
     *features |= 1ULL << feature;
 }
 
-bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc)
+bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
 {
     /* Identify the feature bits corresponding to the host CPU, and
      * fill out the ARMHostCPUClass fields accordingly. To do this
@@ -74,13 +74,13 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc)
         return false;
     }
 
-    ahcc->target = init.target;
+    ahcf->target = init.target;
 
     /* This is not strictly blessed by the device tree binding docs yet,
      * but in practice the kernel does not care about this string so
      * there is no point maintaining an KVM_ARM_TARGET_* -> string table.
      */
-    ahcc->dtb_compatible = "arm,arm-v7";
+    ahcf->dtb_compatible = "arm,arm-v7";
 
     for (i = 0; i < ARRAY_SIZE(idregs); i++) {
         ret = ioctl(fdarray[2], KVM_GET_ONE_REG, &idregs[i]);
@@ -132,7 +132,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc)
         set_feature(&features, ARM_FEATURE_VFP4);
     }
 
-    ahcc->features = features;
+    ahcf->features = features;
 
     return true;
 }
diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
index 6554c30..8f8f828 100644
--- a/target/arm/kvm64.c
+++ b/target/arm/kvm64.c
@@ -443,7 +443,7 @@ static inline void unset_feature(uint64_t *features, int feature)
     *features &= ~(1ULL << feature);
 }
 
-bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc)
+bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
 {
     /* Identify the feature bits corresponding to the host CPU, and
      * fill out the ARMHostCPUClass fields accordingly. To do this
@@ -471,8 +471,8 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc)
         return false;
     }
 
-    ahcc->target = init.target;
-    ahcc->dtb_compatible = "arm,arm-v8";
+    ahcf->target = init.target;
+    ahcf->dtb_compatible = "arm,arm-v8";
 
     kvm_arm_destroy_scratch_host_vcpu(fdarray);
 
@@ -486,7 +486,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc)
     set_feature(&features, ARM_FEATURE_AARCH64);
     set_feature(&features, ARM_FEATURE_PMU);
 
-    ahcc->features = features;
+    ahcf->features = features;
 
     return true;
 }
-- 
2.7.4

[Qemu-devel] [PATCH 3/6] target/arm: Move definition of 'host' cpu type into cpu.c

[Peter Maydell]

Move the definition of the 'host' cpu type into cpu.c, where all the
other CPU types are defined.  We can do this now we've decoupled it
from the KVM-specific host feature probing.  This means we now create
the type unconditionally (assuming we were built with KVM support at
all), but if you try to use it without -enable-kvm this will end
up in the "host cpu probe failed and KVM not enabled" path in
arm_cpu_realizefn(), for an appropriate error message.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu.c | 24 ++++++++++++++++++++++++
 target/arm/kvm.c | 19 -------------------
 2 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index a7deb10..9304277 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1785,6 +1785,26 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
 #endif
 }
 
+#ifdef CONFIG_KVM
+static void arm_host_initfn(Object *obj)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    kvm_arm_set_cpu_features_from_host(cpu);
+}
+
+static const TypeInfo host_arm_cpu_type_info = {
+    .name = TYPE_ARM_HOST_CPU,
+#ifdef TARGET_AARCH64
+    .parent = TYPE_AARCH64_CPU,
+#else
+    .parent = TYPE_ARM_CPU,
+#endif
+    .instance_init = arm_host_initfn,
+};
+
+#endif
+
 static void cpu_register(const ARMCPUInfo *info)
 {
     TypeInfo type_info = {
@@ -1822,6 +1842,10 @@ static void arm_cpu_register_types(void)
         cpu_register(info);
         info++;
     }
+
+#ifdef CONFIG_KVM
+    type_register_static(&host_arm_cpu_type_info);
+#endif
 }
 
 type_init(arm_cpu_register_types)
diff --git a/target/arm/kvm.c b/target/arm/kvm.c
index 945696c..6bdc027 100644
--- a/target/arm/kvm.c
+++ b/target/arm/kvm.c
@@ -152,23 +152,6 @@ void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
     env->features = arm_host_cpu_features.features;
 }
 
-static void kvm_arm_host_cpu_initfn(Object *obj)
-{
-    ARMCPU *cpu = ARM_CPU(obj);
-
-    kvm_arm_set_cpu_features_from_host(cpu);
-}
-
-static const TypeInfo host_arm_cpu_type_info = {
-    .name = TYPE_ARM_HOST_CPU,
-#ifdef TARGET_AARCH64
-    .parent = TYPE_AARCH64_CPU,
-#else
-    .parent = TYPE_ARM_CPU,
-#endif
-    .instance_init = kvm_arm_host_cpu_initfn,
-};
-
 int kvm_arch_init(MachineState *ms, KVMState *s)
 {
     /* For ARM interrupt delivery is always asynchronous,
@@ -184,8 +167,6 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
 
     cap_has_mp_state = kvm_check_extension(s, KVM_CAP_MP_STATE);
 
-    type_register_static(&host_arm_cpu_type_info);
-
     return 0;
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH 4/6] target/arm: Add "-cpu max" support

[Peter Maydell]

Add support for "-cpu max" for ARM guests. This CPU type behaves
like "-cpu host" when KVM is enabled, and like a system CPU with
the maximum possible feature set otherwise. (Note that this means
it won't be migratable across versions, as we will likely add
features to it in future.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target/arm/cpu-qom.h |  2 ++
 target/arm/cpu.c     | 24 ++++++++++++++++++++++++
 target/arm/cpu64.c   | 21 +++++++++++++++++++++
 3 files changed, 47 insertions(+)

diff --git a/target/arm/cpu-qom.h b/target/arm/cpu-qom.h
index a42495b..d135ff8 100644
--- a/target/arm/cpu-qom.h
+++ b/target/arm/cpu-qom.h
@@ -33,6 +33,8 @@ struct arm_boot_info;
 #define ARM_CPU_GET_CLASS(obj) \
     OBJECT_GET_CLASS(ARMCPUClass, (obj), TYPE_ARM_CPU)
 
+#define TYPE_ARM_MAX_CPU "max-" TYPE_ARM_CPU
+
 /**
  * ARMCPUClass:
  * @parent_realize: The parent class' realize handler.
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 9304277..190da97 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -1628,6 +1628,27 @@ static void pxa270c5_initfn(Object *obj)
     cpu->reset_sctlr = 0x00000078;
 }
 
+#ifndef TARGET_AARCH64
+/* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
+ * otherwise, a CPU with as many features enabled as our emulation supports.
+ * The version of '-cpu max' for qemu-system-aarch64 is defined in cpu64.c;
+ * this only needs to handle 32 bits.
+ */
+static void arm_max_initfn(Object *obj)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    if (kvm_enabled()) {
+        kvm_arm_set_cpu_features_from_host(cpu);
+    } else {
+        cortex_a15_initfn(obj);
+        /* In future we might add feature bits here even if the
+         * real-world A15 doesn't implement them.
+         */
+    }
+}
+#endif
+
 #ifdef CONFIG_USER_ONLY
 static void arm_any_initfn(Object *obj)
 {
@@ -1691,6 +1712,9 @@ static const ARMCPUInfo arm_cpus[] = {
     { .name = "pxa270-b1",   .initfn = pxa270b1_initfn },
     { .name = "pxa270-c0",   .initfn = pxa270c0_initfn },
     { .name = "pxa270-c5",   .initfn = pxa270c5_initfn },
+#ifndef TARGET_AARCH64
+    { .name = "max",         .initfn = arm_max_initfn },
+#endif
 #ifdef CONFIG_USER_ONLY
     { .name = "any",         .initfn = arm_any_initfn },
 #endif
diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
index 670c07a..38dcf32 100644
--- a/target/arm/cpu64.c
+++ b/target/arm/cpu64.c
@@ -28,6 +28,7 @@
 #include "hw/arm/arm.h"
 #include "sysemu/sysemu.h"
 #include "sysemu/kvm.h"
+#include "kvm_arm.h"
 
 static inline void set_feature(CPUARMState *env, int feature)
 {
@@ -212,6 +213,25 @@ static void aarch64_a53_initfn(Object *obj)
     define_arm_cp_regs(cpu, cortex_a57_a53_cp_reginfo);
 }
 
+/* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
+ * otherwise, a CPU with as many features enabled as our emulation supports.
+ * The version of '-cpu max' for qemu-system-arm is defined in cpu.c;
+ * this only needs to handle 64 bits.
+ */
+static void aarch64_max_initfn(Object *obj)
+{
+    ARMCPU *cpu = ARM_CPU(obj);
+
+    if (kvm_enabled()) {
+        kvm_arm_set_cpu_features_from_host(cpu);
+    } else {
+        aarch64_a57_initfn(obj);
+        /* In future we might add feature bits here even if the
+         * real-world A57 doesn't implement them.
+         */
+    }
+}
+
 #ifdef CONFIG_USER_ONLY
 static void aarch64_any_initfn(Object *obj)
 {
@@ -240,6 +260,7 @@ typedef struct ARMCPUInfo {
 static const ARMCPUInfo aarch64_cpus[] = {
     { .name = "cortex-a57",         .initfn = aarch64_a57_initfn },
     { .name = "cortex-a53",         .initfn = aarch64_a53_initfn },
+    { .name = "max",                .initfn = aarch64_max_initfn },
 #ifdef CONFIG_USER_ONLY
     { .name = "any",         .initfn = aarch64_any_initfn },
 #endif
-- 
2.7.4

[Qemu-devel] [PATCH 5/6] hw/arm/virt: Add "max" to the list of CPU types "virt" supports

[Peter Maydell]

Allow the virt board to support '-cpu max' in the same way
it already handles '-cpu host'.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 62af013..92bd776 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -167,6 +167,7 @@ static const char *valid_cpus[] = {
     ARM_CPU_TYPE_NAME("cortex-a53"),
     ARM_CPU_TYPE_NAME("cortex-a57"),
     ARM_CPU_TYPE_NAME("host"),
+    ARM_CPU_TYPE_NAME("max"),
 };
 
 static bool cpu_type_valid(const char *cpu)
-- 
2.7.4

[Qemu-devel] [PATCH 6/6] hw/arm/virt: Support -machine gic-version=max

[Peter Maydell]

Add support for passing 'max' to -machine gic-version. By analogy
with the -cpu max option, this picks the "best available" GIC version
whether you're using KVM or TCG, so it behaves like 'host' when
using KVM, and gives you GICv3 when using TCG.

Also like '-cpu host', using -machine gic-version=max' means there
is no guarantee of migration compatibility between QEMU versions;
in future 'max' might mean '4'.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/virt.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 92bd776..603ba56 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1265,16 +1265,23 @@ static void machvirt_init(MachineState *machine)
     /* We can probe only here because during property set
      * KVM is not available yet
      */
-    if (!vms->gic_version) {
+    if (vms->gic_version <= 0) {
+        /* "host" or "max" */
         if (!kvm_enabled()) {
-            error_report("gic-version=host requires KVM");
-            exit(1);
-        }
-
-        vms->gic_version = kvm_arm_vgic_probe();
-        if (!vms->gic_version) {
-            error_report("Unable to determine GIC version supported by host");
-            exit(1);
+            if (vms->gic_version == 0) {
+                error_report("gic-version=host requires KVM");
+                exit(1);
+            } else {
+                /* "max": currently means 3 for TCG */
+                vms->gic_version = 3;
+            }
+        } else {
+            vms->gic_version = kvm_arm_vgic_probe();
+            if (!vms->gic_version) {
+                error_report(
+                    "Unable to determine GIC version supported by host");
+                exit(1);
+            }
         }
     }
 
@@ -1539,9 +1546,11 @@ static void virt_set_gic_version(Object *obj, const char *value, Error **errp)
         vms->gic_version = 2;
     } else if (!strcmp(value, "host")) {
         vms->gic_version = 0; /* Will probe later */
+    } else if (!strcmp(value, "max")) {
+        vms->gic_version = -1; /* Will probe later */
     } else {
         error_setg(errp, "Invalid gic-version value");
-        error_append_hint(errp, "Valid values are 3, 2, host.\n");
+        error_append_hint(errp, "Valid values are 3, 2, host, max.\n");
     }
 }
 
-- 
2.7.4

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/6] arm: support -cpu max (and gic-version=max)

[Peter Maydell]

On 7 December 2017 at 18:14, Peter Maydell <peter.maydell@linaro.org> wrote:
> This patchset adds support for '-cpu max' to Arm, along the lines
> of the existing support we have for x86 targets:
>
>  * under KVM, -cpu max is the same as -cpu host
>  * under TCG, -cpu max means "emulate with as many features as
>    possible"

Forgot to mention: -cpu max for qemu-system-aarch64 will
be a 64-bit cpu, and for qemu-system-arm it will be a 32-bit
cpu. (This differs from all the other TCG CPU types, which
behave the same for the 32-bit and 64-bit binaries. I think
it is the same way that x86 -cpu max works, though.)

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/6] arm: support -cpu max (and gic-version=max)

[Eduardo Habkost]

On Thu, Dec 07, 2017 at 07:37:31PM +0000, Peter Maydell wrote:
> On 7 December 2017 at 18:14, Peter Maydell <peter.maydell@linaro.org> wrote:
> > This patchset adds support for '-cpu max' to Arm, along the lines
> > of the existing support we have for x86 targets:
> >
> >  * under KVM, -cpu max is the same as -cpu host
> >  * under TCG, -cpu max means "emulate with as many features as
> >    possible"
> 
> Forgot to mention: -cpu max for qemu-system-aarch64 will
> be a 64-bit cpu, and for qemu-system-arm it will be a 32-bit
> cpu. (This differs from all the other TCG CPU types, which
> behave the same for the 32-bit and 64-bit binaries. I think
> it is the same way that x86 -cpu max works, though.)

Are they going to be represented by two different QOM type names?

(In the case of x86, all the CPU classes have different names on
qemu-system-x86_64 and qemu-system-i386).

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/6] hw/arm/virt: Check that the CPU realize method succeeded

[Eduardo Habkost]

On Thu, Dec 07, 2017 at 06:14:48PM +0000, Peter Maydell wrote:
> We were passing a NULL error pointer to the object_property_set_bool()
> call that realizes the CPU object. This meant that we wouldn't detect
> failure, and would plough blindly on to crash later trying to use a
> NULL CPU object pointer. Detect errors and fail instead.
> 
> In particular, this will be necessary to detect the user error
> of using "-cpu host" without "-enable-kvm" once we make the host
> CPU type be registered unconditionally rather than only in
> kvm_arch_init().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/6] arm: support -cpu max (and gic-version=max)

[Peter Maydell]

On 9 December 2017 at 01:08, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Thu, Dec 07, 2017 at 07:37:31PM +0000, Peter Maydell wrote:
>> On 7 December 2017 at 18:14, Peter Maydell <peter.maydell@linaro.org> wrote:
>> > This patchset adds support for '-cpu max' to Arm, along the lines
>> > of the existing support we have for x86 targets:
>> >
>> >  * under KVM, -cpu max is the same as -cpu host
>> >  * under TCG, -cpu max means "emulate with as many features as
>> >    possible"
>>
>> Forgot to mention: -cpu max for qemu-system-aarch64 will
>> be a 64-bit cpu, and for qemu-system-arm it will be a 32-bit
>> cpu. (This differs from all the other TCG CPU types, which
>> behave the same for the 32-bit and 64-bit binaries. I think
>> it is the same way that x86 -cpu max works, though.)
>
> Are they going to be represented by two different QOM type names?
>
> (In the case of x86, all the CPU classes have different names on
> qemu-system-x86_64 and qemu-system-i386).

(Just pulling this thread up from before Christmas...)

I guess a better way to approach this would be to ask: how is
x86 implementing -cpu max, ie what is the required view of things
that I need to provide for target/arm in order to have QEMU
behave the same way x86 does? Did we write any user-facing
documentation for this feature?

(The code in this patchset makes '-cpu max' give the same
QOM type name for both qemu-system-arm and qemu-system-aarch64,
with different behaviour depending on the binary. If that means
we don't provide the same behaviour as x86 then I can change that,
but I'm not sure where the difference is exposed to the user?)

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/6] arm: support -cpu max (and gic-version=max)

[Eduardo Habkost]

On Mon, Jan 22, 2018 at 06:06:26PM +0000, Peter Maydell wrote:
> On 9 December 2017 at 01:08, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Thu, Dec 07, 2017 at 07:37:31PM +0000, Peter Maydell wrote:
> >> On 7 December 2017 at 18:14, Peter Maydell <peter.maydell@linaro.org> wrote:
> >> > This patchset adds support for '-cpu max' to Arm, along the lines
> >> > of the existing support we have for x86 targets:
> >> >
> >> >  * under KVM, -cpu max is the same as -cpu host
> >> >  * under TCG, -cpu max means "emulate with as many features as
> >> >    possible"
> >>
> >> Forgot to mention: -cpu max for qemu-system-aarch64 will
> >> be a 64-bit cpu, and for qemu-system-arm it will be a 32-bit
> >> cpu. (This differs from all the other TCG CPU types, which
> >> behave the same for the 32-bit and 64-bit binaries. I think
> >> it is the same way that x86 -cpu max works, though.)
> >
> > Are they going to be represented by two different QOM type names?
> >
> > (In the case of x86, all the CPU classes have different names on
> > qemu-system-x86_64 and qemu-system-i386).
> 
> (Just pulling this thread up from before Christmas...)
> 
> I guess a better way to approach this would be to ask: how is
> x86 implementing -cpu max, ie what is the required view of things
> that I need to provide for target/arm in order to have QEMU
> behave the same way x86 does? Did we write any user-facing
> documentation for this feature?

About QOM type names:

On x86, all CPU models are resolved to "<model>-<suffix>", and
i386 and x86_64 have different suffixes.  So the QOM type name is
"max-x86_64-cpu" on qemu-system-x86_64, and "max-i386-cpu" on
qemu-system-i386.

About the implementation:

On x86, the most important differences between CPU models are on
(boolean) CPUID feature flags.  "max" just set all feature flags
to *get_supported_cpuid(...), meaning all features supported by
the host will be enabled.  The other fields (e.g. CPU
vendor/family/model IDs) depend on the accelerator: on TCG we use
constant values, on KVM we use the host CPU values).

About how it should behave:

An important expectation about "max" is about the
query-cpu-model-expansion return value.  Having a property set to
true on the return value of "query-cpu-model-expansion model=max"
means the corresponding feature is supported on the current host
and can be enabled on the command-line.

The most important user-facing documentation related to "max" is
the query-cpu-model-* QMP docs.  It probably needs to be updated
to contain more specific details about how "max" behaves.
Probably we could have automated tests to confirm if some
expectations are really fulfilled.


> 
> (The code in this patchset makes '-cpu max' give the same
> QOM type name for both qemu-system-arm and qemu-system-aarch64,
> with different behaviour depending on the binary. If that means
> we don't provide the same behaviour as x86 then I can change that,
> but I'm not sure where the difference is exposed to the user?)

This is not how the QOM names work on x86, but I don't think QOM
type names choices have important user-visible side-effects
today.  Choosing unique QOM type names is more important to make
the code future-proof for when we merge QEMU binaries, than to
make user-visible behavior consistent.

-- 
Eduardo

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/6] arm: support -cpu max (and gic-version=max)

[Peter Maydell]

On 22 January 2018 at 18:33, Eduardo Habkost <ehabkost@redhat.com> wrote:
> About QOM type names:
>
> On x86, all CPU models are resolved to "<model>-<suffix>", and
> i386 and x86_64 have different suffixes.  So the QOM type name is
> "max-x86_64-cpu" on qemu-system-x86_64, and "max-i386-cpu" on
> qemu-system-i386.

OK. Looking at the target/arm code we do a similar suffix
trick, but we seem to have cut-n-pasted the handling in
aarch64_cpu_register(), so it uses the TYPE_ARM_CPU as the
suffix, rather the TYPE_AARCH64_CPU.

Am I right in thinking that we can fix this (changing the
QOM type names for all the aarch64 CPUs) without breaking
migration? (I guess I can just test this easily enough.)

If we did that then we'd have, like x86, "max-arm-cpu" in
the qemu-system-arm binary, and "max-aarch64-cpu" in
the qemu-system-aarch64 binary.

Does x86 provide a way to say "give me the max-i386-cpu"
in the qemu-system-x86_64 binary ?

> About how it should behave:
>
> An important expectation about "max" is about the
> query-cpu-model-expansion return value.  Having a property set to
> true on the return value of "query-cpu-model-expansion model=max"
> means the corresponding feature is supported on the current host
> and can be enabled on the command-line.

On Arm when I try to use this I get:
{ "execute": "query-cpu-model-expansion", "arguments": { "type":
"static", "model": { "model": { "name": "max" } } } }
{
    "error": {
        "class": "CommandNotFound",
        "desc": "The command query-cpu-model-expansion has not been found"
    }
}

It looks like we only implement this QMP API for x86 and S390
(via #ifdeffery in monitor.c).

I'm not sure if we actually support command line setting/unsetting
of features for Arm CPUs -- is there a command line option to
get QEMU to print the features it thinks can be set this way?

>> (The code in this patchset makes '-cpu max' give the same
>> QOM type name for both qemu-system-arm and qemu-system-aarch64,
>> with different behaviour depending on the binary. If that means
>> we don't provide the same behaviour as x86 then I can change that,
>> but I'm not sure where the difference is exposed to the user?)
>
> This is not how the QOM names work on x86, but I don't think QOM
> type names choices have important user-visible side-effects
> today.  Choosing unique QOM type names is more important to make
> the code future-proof for when we merge QEMU binaries, than to
> make user-visible behavior consistent.

Good point -- assuming it doesn't break migration I can fix
the aarch64 types to use the right suffix string and then they'll
have different QOM type names.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/6] arm: support -cpu max (and gic-version=max)

[Peter Maydell]

On 25 January 2018 at 14:41, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 22 January 2018 at 18:33, Eduardo Habkost <ehabkost@redhat.com> wrote:
>> About QOM type names:
>>
>> On x86, all CPU models are resolved to "<model>-<suffix>", and
>> i386 and x86_64 have different suffixes.  So the QOM type name is
>> "max-x86_64-cpu" on qemu-system-x86_64, and "max-i386-cpu" on
>> qemu-system-i386.
>
> OK. Looking at the target/arm code we do a similar suffix
> trick, but we seem to have cut-n-pasted the handling in
> aarch64_cpu_register(), so it uses the TYPE_ARM_CPU as the
> suffix, rather the TYPE_AARCH64_CPU.

...and that's not as simple a fix as I thought, because the
code in helper.c for implementing arch_query_cpu_definitions() and
arm_cpu_list() assumes it can create the QOM type name by appending
TYPE_ARM_CPU. The ARM_CPU_TYPE_NAME() macro which we use pretty
extensively also assumes the suffix is the same regardless of
what CPU type it's being applied to.

Looking at x86 it seems that TYPE_X86_CPU expands to a different
string for qemu-system-x86_64 and qemu-system-i386. I could do
that, but it seems very confusing: I would expect a QOM type
name like TYPE_FOO to always mean the same QOM type.

Given that the type names don't appear to the user, I think
we can go ahead with implementing "-cpu max" for Arm without
having to first disentangle this? "max" isn't in any worse
a position than the existing "host" and "any" types.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/6] arm: support -cpu max (and gic-version=max)

[Eduardo Habkost]

On Thu, Jan 25, 2018 at 02:41:50PM +0000, Peter Maydell wrote:
> On 22 January 2018 at 18:33, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > About QOM type names:
> >
> > On x86, all CPU models are resolved to "<model>-<suffix>", and
> > i386 and x86_64 have different suffixes.  So the QOM type name is
> > "max-x86_64-cpu" on qemu-system-x86_64, and "max-i386-cpu" on
> > qemu-system-i386.
> 
> OK. Looking at the target/arm code we do a similar suffix
> trick, but we seem to have cut-n-pasted the handling in
> aarch64_cpu_register(), so it uses the TYPE_ARM_CPU as the
> suffix, rather the TYPE_AARCH64_CPU.
> 
> Am I right in thinking that we can fix this (changing the
> QOM type names for all the aarch64 CPUs) without breaking
> migration? (I guess I can just test this easily enough.)

This is not supposed to affect migration (at least it didn't when
we introduced per-cpu-model subclasses), but it's a good idea to
test it anyway.  It might break other code that tries to extract
info from the class names.


> 
> If we did that then we'd have, like x86, "max-arm-cpu" in
> the qemu-system-arm binary, and "max-aarch64-cpu" in
> the qemu-system-aarch64 binary.
> 
> Does x86 provide a way to say "give me the max-i386-cpu"
> in the qemu-system-x86_64 binary ?

No, the *-i386-cpu classes aren't even compiled in on
qemu-system-x86_64.

> 
> > About how it should behave:
> >
> > An important expectation about "max" is about the
> > query-cpu-model-expansion return value.  Having a property set to
> > true on the return value of "query-cpu-model-expansion model=max"
> > means the corresponding feature is supported on the current host
> > and can be enabled on the command-line.
> 
> On Arm when I try to use this I get:
> { "execute": "query-cpu-model-expansion", "arguments": { "type":
> "static", "model": { "model": { "name": "max" } } } }
> {
>     "error": {
>         "class": "CommandNotFound",
>         "desc": "The command query-cpu-model-expansion has not been found"
>     }
> }
> 
> It looks like we only implement this QMP API for x86 and S390
> (via #ifdeffery in monitor.c).
> 
> I'm not sure if we actually support command line setting/unsetting
> of features for Arm CPUs -- is there a command line option to
> get QEMU to print the features it thinks can be set this way?

Unfortunately -cpu command-line parsing is still a mess (we
currently have lots of arch-specific parsing hooks).  Once we
make this uniform across targets, we could make "-cpu ?" print
all known properties.

But you can look at the list of QOM properties for your CPU
classes (-cpu options are simply translated to QOM properties).
e.g.:

  (QEMU) device-list-properties typename=pxa270-a0-arm-cpu
  {"return": [{"type": "uint32", "name": "midr"}, {"type": "uint64", "name": "mp-affinity"}, {"type": "child<irq>", "name": "unnamed-gpio-in[0]"}, {"type": "uint32", "name": "psci-conduit"}, {"type": "bool", "name": "reset-hivecs"}, {"type": "link<qemu:memory-region>", "name": "memory"}, {"type": "link<irq>", "name": "unnamed-gpio-out[2]"}, {"type": "link<irq>", "name": "unnamed-gpio-out[3]"}, {"type": "int32", "name": "node-id"}, {"type": "bool", "name": "start-powered-off"}, {"type": "link<irq>", "name": "unnamed-gpio-out[1]"}, {"type": "link<irq>", "name": "unnamed-gpio-out[0]"}, {"type": "link<irq>", "name": "gicv3-maintenance-interrupt[0]"}, {"type": "bool", "name": "cfgend"}, {"type": "child<irq>", "name": "unnamed-gpio-in[2]"}, {"type": "child<irq>", "name": "unnamed-gpio-in[3]"}, {"type": "child<irq>", "name": "unnamed-gpio-in[1]"}]}



> 
> >> (The code in this patchset makes '-cpu max' give the same
> >> QOM type name for both qemu-system-arm and qemu-system-aarch64,
> >> with different behaviour depending on the binary. If that means
> >> we don't provide the same behaviour as x86 then I can change that,
> >> but I'm not sure where the difference is exposed to the user?)
> >
> > This is not how the QOM names work on x86, but I don't think QOM
> > type names choices have important user-visible side-effects
> > today.  Choosing unique QOM type names is more important to make
> > the code future-proof for when we merge QEMU binaries, than to
> > make user-visible behavior consistent.
> 
> Good point -- assuming it doesn't break migration I can fix
> the aarch64 types to use the right suffix string and then they'll
> have different QOM type names.
> 
> thanks
> -- PMM

-- 
Eduardo

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/6] arm: support -cpu max (and gic-version=max)

[Eduardo Habkost]

On Thu, Jan 25, 2018 at 03:10:31PM +0000, Peter Maydell wrote:
> On 25 January 2018 at 14:41, Peter Maydell <peter.maydell@linaro.org> wrote:
> > On 22 January 2018 at 18:33, Eduardo Habkost <ehabkost@redhat.com> wrote:
> >> About QOM type names:
> >>
> >> On x86, all CPU models are resolved to "<model>-<suffix>", and
> >> i386 and x86_64 have different suffixes.  So the QOM type name is
> >> "max-x86_64-cpu" on qemu-system-x86_64, and "max-i386-cpu" on
> >> qemu-system-i386.
> >
> > OK. Looking at the target/arm code we do a similar suffix
> > trick, but we seem to have cut-n-pasted the handling in
> > aarch64_cpu_register(), so it uses the TYPE_ARM_CPU as the
> > suffix, rather the TYPE_AARCH64_CPU.
> 
> ...and that's not as simple a fix as I thought, because the
> code in helper.c for implementing arch_query_cpu_definitions() and
> arm_cpu_list() assumes it can create the QOM type name by appending
> TYPE_ARM_CPU. The ARM_CPU_TYPE_NAME() macro which we use pretty
> extensively also assumes the suffix is the same regardless of
> what CPU type it's being applied to.
> 
> Looking at x86 it seems that TYPE_X86_CPU expands to a different
> string for qemu-system-x86_64 and qemu-system-i386. I could do
> that, but it seems very confusing: I would expect a QOM type
> name like TYPE_FOO to always mean the same QOM type.

Yeah, I don't like the way TYPE_x86_CPU works, and I don't
recommend doing the same elsewhere.

> 
> Given that the type names don't appear to the user, I think
> we can go ahead with implementing "-cpu max" for Arm without
> having to first disentangle this? "max" isn't in any worse
> a position than the existing "host" and "any" types.

Sounds reasonable to me.

-- 
Eduardo

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/6] arm: support -cpu max (and gic-version=max)

[Peter Maydell]

On 26 January 2018 at 10:42, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Thu, Jan 25, 2018 at 02:41:50PM +0000, Peter Maydell wrote:
>> I'm not sure if we actually support command line setting/unsetting
>> of features for Arm CPUs -- is there a command line option to
>> get QEMU to print the features it thinks can be set this way?
>
> Unfortunately -cpu command-line parsing is still a mess (we
> currently have lots of arch-specific parsing hooks).  Once we
> make this uniform across targets, we could make "-cpu ?" print
> all known properties.
>
> But you can look at the list of QOM properties for your CPU
> classes (-cpu options are simply translated to QOM properties).
> e.g.:
>
>   (QEMU) device-list-properties typename=pxa270-a0-arm-cpu
>   {"return": [{"type": "uint32", "name": "midr"}, {"type": "uint64", "name": "mp-affinity"}, {"type": "child<irq>", "name": "unnamed-gpio-in[0]"}, {"type": "uint32", "name": "psci-conduit"}, {"type": "bool", "name": "reset-hivecs"}, {"type": "link<qemu:memory-region>", "name": "memory"}, {"type": "link<irq>", "name": "unnamed-gpio-out[2]"}, {"type": "link<irq>", "name": "unnamed-gpio-out[3]"}, {"type": "int32", "name": "node-id"}, {"type": "bool", "name": "start-powered-off"}, {"type": "link<irq>", "name": "unnamed-gpio-out[1]"}, {"type": "link<irq>", "name": "unnamed-gpio-out[0]"}, {"type": "link<irq>", "name": "gicv3-maintenance-interrupt[0]"}, {"type": "bool", "name": "cfgend"}, {"type": "child<irq>", "name": "unnamed-gpio-in[2]"}, {"type": "child<irq>", "name": "unnamed-gpio-in[3]"}, {"type": "child<irq>", "name": "unnamed-gpio-in[1]"}]}

None of those are things we'd want to expose to the user, really
(except maybe 'cfgend'): they're all intended for the QEMU board
or SoC code that needs to configure and wire the CPU up. Ideally
there'd be a mechanism for screening them out of the -cpu option
list.

There are some things that we could in theory have as user
settable properties (like "does this CPU have an FPU"), but
we don't currently have QOM properties for them (each CPU
just hardcodes which ARM_FEATURE_* flags it has).

In your other email you write:
> I wrote:
>> Given that the type names don't appear to the user, I think
>> we can go ahead with implementing "-cpu max" for Arm without
>> having to first disentangle this? "max" isn't in any worse
>> a position than the existing "host" and "any" types.
>
>Sounds reasonable to me.

Cool. I think that means that patches 2-6 here don't need
any changes, but I'll rebase, retest and resend just to
avoid confusion.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 3/6] target/arm: Move definition of 'host' cpu type into cpu.c

[Philippe Mathieu-Daudé]

On 12/07/2017 03:14 PM, Peter Maydell wrote:
> Move the definition of the 'host' cpu type into cpu.c, where all the
> other CPU types are defined.  We can do this now we've decoupled it
> from the KVM-specific host feature probing.  This means we now create
> the type unconditionally (assuming we were built with KVM support at
> all), but if you try to use it without -enable-kvm this will end
> up in the "host cpu probe failed and KVM not enabled" path in
> arm_cpu_realizefn(), for an appropriate error message.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  target/arm/cpu.c | 24 ++++++++++++++++++++++++
>  target/arm/kvm.c | 19 -------------------
>  2 files changed, 24 insertions(+), 19 deletions(-)
> 
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index a7deb10..9304277 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1785,6 +1785,26 @@ static void arm_cpu_class_init(ObjectClass *oc, void *data)
>  #endif
>  }
>  
> +#ifdef CONFIG_KVM
> +static void arm_host_initfn(Object *obj)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    kvm_arm_set_cpu_features_from_host(cpu);
> +}
> +
> +static const TypeInfo host_arm_cpu_type_info = {
> +    .name = TYPE_ARM_HOST_CPU,
> +#ifdef TARGET_AARCH64
> +    .parent = TYPE_AARCH64_CPU,
> +#else
> +    .parent = TYPE_ARM_CPU,
> +#endif
> +    .instance_init = arm_host_initfn,
> +};
> +
> +#endif
> +
>  static void cpu_register(const ARMCPUInfo *info)
>  {
>      TypeInfo type_info = {
> @@ -1822,6 +1842,10 @@ static void arm_cpu_register_types(void)
>          cpu_register(info);
>          info++;
>      }
> +
> +#ifdef CONFIG_KVM
> +    type_register_static(&host_arm_cpu_type_info);
> +#endif
>  }
>  
>  type_init(arm_cpu_register_types)
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 945696c..6bdc027 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -152,23 +152,6 @@ void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
>      env->features = arm_host_cpu_features.features;
>  }
>  
> -static void kvm_arm_host_cpu_initfn(Object *obj)
> -{
> -    ARMCPU *cpu = ARM_CPU(obj);
> -
> -    kvm_arm_set_cpu_features_from_host(cpu);
> -}
> -
> -static const TypeInfo host_arm_cpu_type_info = {
> -    .name = TYPE_ARM_HOST_CPU,
> -#ifdef TARGET_AARCH64
> -    .parent = TYPE_AARCH64_CPU,
> -#else
> -    .parent = TYPE_ARM_CPU,
> -#endif
> -    .instance_init = kvm_arm_host_cpu_initfn,
> -};
> -
>  int kvm_arch_init(MachineState *ms, KVMState *s)
>  {
>      /* For ARM interrupt delivery is always asynchronous,
> @@ -184,8 +167,6 @@ int kvm_arch_init(MachineState *ms, KVMState *s)
>  
>      cap_has_mp_state = kvm_check_extension(s, KVM_CAP_MP_STATE);
>  
> -    type_register_static(&host_arm_cpu_type_info);
> -
>      return 0;
>  }
>  
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 2/6] target/arm: Query host CPU features on-demand at instance init

[Philippe Mathieu-Daudé]

On 12/07/2017 03:14 PM, Peter Maydell wrote:
> Currently we query the host CPU features in the class init function
> for the TYPE_ARM_HOST_CPU class, so that we can later copy them
> from the class object into the instance object in the object
> instance init function. This is awkward for implementing "-cpu max",
> which should work like "-cpu host" for KVM but like "cpu with all
> implemented features" for TCG.
> 
> Move the place where we store the information about the host CPU from
> a class object to static variables in kvm.c, and then in the instance
> init function call a new kvm_arm_set_cpu_features_from_host()
> function which will query the host kernel if necessary and then
> fill in the CPU instance fields.
> 
> This allows us to drop the special class struct and class init
> function for TYPE_ARM_HOST_CPU entirely.
> 
> We can't delay the probe until realize, because the ARM
> instance_post_init hook needs to look at the feature bits we
> set, so we need to do it in the initfn. This is safe because
> the probing doesn't affect the actual VM state (it creates a
> separate scratch VM to do its testing), but the probe might fail.
> Because we can't report errors in retrieving the host features
> in the initfn, we check this belatedly in the realize function
> (the intervening code will be able to cope with the relevant
> fields in the CPU structure being zero).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  target/arm/cpu.h     |  5 +++++
>  target/arm/kvm_arm.h | 35 ++++++++++++++++++++++++-----------
>  target/arm/cpu.c     | 13 +++++++++++++
>  target/arm/kvm.c     | 36 +++++++++++++++++++-----------------
>  target/arm/kvm32.c   |  8 ++++----
>  target/arm/kvm64.c   |  8 ++++----
>  6 files changed, 69 insertions(+), 36 deletions(-)
> 
> diff --git a/target/arm/cpu.h b/target/arm/cpu.h
> index 89d49cd..5b01cf9 100644
> --- a/target/arm/cpu.h
> +++ b/target/arm/cpu.h
> @@ -690,6 +690,11 @@ struct ARMCPU {
>      /* Uniprocessor system with MP extensions */
>      bool mp_is_up;
>  
> +    /* True if we tried kvm_arm_host_cpu_features() during CPU instance_init
> +     * and the probe failed (so we need to report the error in realize)
> +     */
> +    bool host_cpu_probe_failed;
> +
>      /* The instance init functions for implementation-specific subclasses
>       * set these fields to specify the implementation-dependent values of
>       * various constant registers and reset values of non-constant
> diff --git a/target/arm/kvm_arm.h b/target/arm/kvm_arm.h
> index ff53e9f..89d1b67 100644
> --- a/target/arm/kvm_arm.h
> +++ b/target/arm/kvm_arm.h
> @@ -152,20 +152,16 @@ bool kvm_arm_create_scratch_host_vcpu(const uint32_t *cpus_to_try,
>  void kvm_arm_destroy_scratch_host_vcpu(int *fdarray);
>  
>  #define TYPE_ARM_HOST_CPU "host-" TYPE_ARM_CPU
> -#define ARM_HOST_CPU_CLASS(klass) \
> -    OBJECT_CLASS_CHECK(ARMHostCPUClass, (klass), TYPE_ARM_HOST_CPU)
> -#define ARM_HOST_CPU_GET_CLASS(obj) \
> -    OBJECT_GET_CLASS(ARMHostCPUClass, (obj), TYPE_ARM_HOST_CPU)
> -
> -typedef struct ARMHostCPUClass {
> -    /*< private >*/
> -    ARMCPUClass parent_class;
> -    /*< public >*/
>  
> +/**
> + * ARMHostCPUFeatures: information about the host CPU (identified
> + * by asking the host kernel)
> + */
> +typedef struct ARMHostCPUFeatures {
>      uint64_t features;
>      uint32_t target;
>      const char *dtb_compatible;
> -} ARMHostCPUClass;
> +} ARMHostCPUFeatures;
>  
>  /**
>   * kvm_arm_get_host_cpu_features:
> @@ -174,8 +170,16 @@ typedef struct ARMHostCPUClass {
>   * Probe the capabilities of the host kernel's preferred CPU and fill
>   * in the ARMHostCPUClass struct accordingly.
>   */
> -bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc);
> +bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf);
>  
> +/**
> + * kvm_arm_set_cpu_features_from_host:
> + * @cpu: ARMCPU to set the features for
> + *
> + * Set up the ARMCPU struct fields up to match the information probed
> + * from the host CPU.
> + */
> +void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu);
>  
>  /**
>   * kvm_arm_sync_mpstate_to_kvm
> @@ -200,6 +204,15 @@ void kvm_arm_pmu_init(CPUState *cs);
>  
>  #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;
> +}
> +
>  static inline int kvm_arm_vgic_probe(void)
>  {
>      return 0;
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 7f7a3d1..a7deb10 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -709,6 +709,19 @@ static void arm_cpu_realizefn(DeviceState *dev, Error **errp)
>      AddressSpace *as;
>  #endif
>  
> +    /* If we needed to query the host kernel for the CPU features
> +     * then it's possible that might have failed in the initfn, but
> +     * this is the first point where we can report it.
> +     */
> +    if (cpu->host_cpu_probe_failed) {
> +        if (!kvm_enabled()) {
> +            error_setg(errp, "The 'host' CPU type can only be used with KVM");
> +        } else {
> +            error_setg(errp, "Failed to retrieve host CPU features");
> +        }
> +        return;
> +    }
> +
>      cpu_exec_realizefn(cs, &local_err);
>      if (local_err != NULL) {
>          error_propagate(errp, local_err);
> diff --git a/target/arm/kvm.c b/target/arm/kvm.c
> index 211a7bf..945696c 100644
> --- a/target/arm/kvm.c
> +++ b/target/arm/kvm.c
> @@ -33,6 +33,8 @@ const KVMCapabilityInfo kvm_arch_required_capabilities[] = {
>  
>  static bool cap_has_mp_state;
>  
> +static ARMHostCPUFeatures arm_host_cpu_features;
> +
>  int kvm_arm_vcpu_init(CPUState *cs)
>  {
>      ARMCPU *cpu = ARM_CPU(cs);
> @@ -129,30 +131,32 @@ void kvm_arm_destroy_scratch_host_vcpu(int *fdarray)
>      }
>  }
>  
> -static void kvm_arm_host_cpu_class_init(ObjectClass *oc, void *data)
> +void kvm_arm_set_cpu_features_from_host(ARMCPU *cpu)
>  {
> -    ARMHostCPUClass *ahcc = ARM_HOST_CPU_CLASS(oc);
> +    CPUARMState *env = &cpu->env;
>  
> -    /* All we really need to set up for the 'host' CPU
> -     * is the feature bits -- we rely on the fact that the
> -     * various ID register values in ARMCPU are only used for
> -     * TCG CPUs.
> -     */
> -    if (!kvm_arm_get_host_cpu_features(ahcc)) {
> -        fprintf(stderr, "Failed to retrieve host CPU features!\n");
> -        abort();
> +    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().
> +             */
> +            cpu->kvm_target = QEMU_KVM_ARM_TARGET_NONE;
> +            cpu->host_cpu_probe_failed = true;
> +            return;
> +        }
>      }
> +
> +    cpu->kvm_target = arm_host_cpu_features.target;
> +    cpu->dtb_compatible = arm_host_cpu_features.dtb_compatible;
> +    env->features = arm_host_cpu_features.features;
>  }
>  
>  static void kvm_arm_host_cpu_initfn(Object *obj)
>  {
> -    ARMHostCPUClass *ahcc = ARM_HOST_CPU_GET_CLASS(obj);
>      ARMCPU *cpu = ARM_CPU(obj);
> -    CPUARMState *env = &cpu->env;
>  
> -    cpu->kvm_target = ahcc->target;
> -    cpu->dtb_compatible = ahcc->dtb_compatible;
> -    env->features = ahcc->features;
> +    kvm_arm_set_cpu_features_from_host(cpu);
>  }
>  
>  static const TypeInfo host_arm_cpu_type_info = {
> @@ -163,8 +167,6 @@ static const TypeInfo host_arm_cpu_type_info = {
>      .parent = TYPE_ARM_CPU,
>  #endif
>      .instance_init = kvm_arm_host_cpu_initfn,
> -    .class_init = kvm_arm_host_cpu_class_init,
> -    .class_size = sizeof(ARMHostCPUClass),
>  };
>  
>  int kvm_arch_init(MachineState *ms, KVMState *s)
> diff --git a/target/arm/kvm32.c b/target/arm/kvm32.c
> index f925a21..cc326ea 100644
> --- a/target/arm/kvm32.c
> +++ b/target/arm/kvm32.c
> @@ -28,7 +28,7 @@ static inline void set_feature(uint64_t *features, int feature)
>      *features |= 1ULL << feature;
>  }
>  
> -bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc)
> +bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>  {
>      /* Identify the feature bits corresponding to the host CPU, and
>       * fill out the ARMHostCPUClass fields accordingly. To do this
> @@ -74,13 +74,13 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc)
>          return false;
>      }
>  
> -    ahcc->target = init.target;
> +    ahcf->target = init.target;
>  
>      /* This is not strictly blessed by the device tree binding docs yet,
>       * but in practice the kernel does not care about this string so
>       * there is no point maintaining an KVM_ARM_TARGET_* -> string table.
>       */
> -    ahcc->dtb_compatible = "arm,arm-v7";
> +    ahcf->dtb_compatible = "arm,arm-v7";
>  
>      for (i = 0; i < ARRAY_SIZE(idregs); i++) {
>          ret = ioctl(fdarray[2], KVM_GET_ONE_REG, &idregs[i]);
> @@ -132,7 +132,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc)
>          set_feature(&features, ARM_FEATURE_VFP4);
>      }
>  
> -    ahcc->features = features;
> +    ahcf->features = features;
>  
>      return true;
>  }
> diff --git a/target/arm/kvm64.c b/target/arm/kvm64.c
> index 6554c30..8f8f828 100644
> --- a/target/arm/kvm64.c
> +++ b/target/arm/kvm64.c
> @@ -443,7 +443,7 @@ static inline void unset_feature(uint64_t *features, int feature)
>      *features &= ~(1ULL << feature);
>  }
>  
> -bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc)
> +bool kvm_arm_get_host_cpu_features(ARMHostCPUFeatures *ahcf)
>  {
>      /* Identify the feature bits corresponding to the host CPU, and
>       * fill out the ARMHostCPUClass fields accordingly. To do this
> @@ -471,8 +471,8 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc)
>          return false;
>      }
>  
> -    ahcc->target = init.target;
> -    ahcc->dtb_compatible = "arm,arm-v8";
> +    ahcf->target = init.target;
> +    ahcf->dtb_compatible = "arm,arm-v8";
>  
>      kvm_arm_destroy_scratch_host_vcpu(fdarray);
>  
> @@ -486,7 +486,7 @@ bool kvm_arm_get_host_cpu_features(ARMHostCPUClass *ahcc)
>      set_feature(&features, ARM_FEATURE_AARCH64);
>      set_feature(&features, ARM_FEATURE_PMU);
>  
> -    ahcc->features = features;
> +    ahcf->features = features;
>  
>      return true;
>  }
> 

Re: [Qemu-devel] [PATCH 4/6] target/arm: Add "-cpu max" support

[Philippe Mathieu-Daudé]

Hi Peter,

On 12/07/2017 03:14 PM, Peter Maydell wrote:
> Add support for "-cpu max" for ARM guests. This CPU type behaves
> like "-cpu host" when KVM is enabled, and like a system CPU with
> the maximum possible feature set otherwise. (Note that this means
> it won't be migratable across versions, as we will likely add
> features to it in future.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target/arm/cpu-qom.h |  2 ++
>  target/arm/cpu.c     | 24 ++++++++++++++++++++++++
>  target/arm/cpu64.c   | 21 +++++++++++++++++++++
>  3 files changed, 47 insertions(+)
> 
> diff --git a/target/arm/cpu-qom.h b/target/arm/cpu-qom.h
> index a42495b..d135ff8 100644
> --- a/target/arm/cpu-qom.h
> +++ b/target/arm/cpu-qom.h
> @@ -33,6 +33,8 @@ struct arm_boot_info;
>  #define ARM_CPU_GET_CLASS(obj) \
>      OBJECT_GET_CLASS(ARMCPUClass, (obj), TYPE_ARM_CPU)
>  
> +#define TYPE_ARM_MAX_CPU "max-" TYPE_ARM_CPU
> +
>  /**
>   * ARMCPUClass:
>   * @parent_realize: The parent class' realize handler.
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 9304277..190da97 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -1628,6 +1628,27 @@ static void pxa270c5_initfn(Object *obj)
>      cpu->reset_sctlr = 0x00000078;
>  }
>  
> +#ifndef TARGET_AARCH64
> +/* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
> + * otherwise, a CPU with as many features enabled as our emulation supports.
> + * The version of '-cpu max' for qemu-system-aarch64 is defined in cpu64.c;
> + * this only needs to handle 32 bits.
> + */
> +static void arm_max_initfn(Object *obj)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    if (kvm_enabled()) {
> +        kvm_arm_set_cpu_features_from_host(cpu);
> +    } else {
> +        cortex_a15_initfn(obj);> +        /* In future we might add feature bits here even if the
> +         * real-world A15 doesn't implement them.
> +         */

Why not use arm_any_initfn() here?

Actually what seems cleaner is to move "any" features here, and kill the
"any" cpu, using "max" for this purpose.

> +    }
> +}
> +#endif
> +
>  #ifdef CONFIG_USER_ONLY
>  static void arm_any_initfn(Object *obj)
>  {
> @@ -1691,6 +1712,9 @@ static const ARMCPUInfo arm_cpus[] = {
>      { .name = "pxa270-b1",   .initfn = pxa270b1_initfn },
>      { .name = "pxa270-c0",   .initfn = pxa270c0_initfn },
>      { .name = "pxa270-c5",   .initfn = pxa270c5_initfn },
> +#ifndef TARGET_AARCH64
> +    { .name = "max",         .initfn = arm_max_initfn },
> +#endif

>  #ifdef CONFIG_USER_ONLY
>      { .name = "any",         .initfn = arm_any_initfn },
>  #endif

and we can remove the last 3 lines.

> diff --git a/target/arm/cpu64.c b/target/arm/cpu64.c
> index 670c07a..38dcf32 100644
> --- a/target/arm/cpu64.c
> +++ b/target/arm/cpu64.c
> @@ -28,6 +28,7 @@
>  #include "hw/arm/arm.h"
>  #include "sysemu/sysemu.h"
>  #include "sysemu/kvm.h"
> +#include "kvm_arm.h"
>  
>  static inline void set_feature(CPUARMState *env, int feature)
>  {
> @@ -212,6 +213,25 @@ static void aarch64_a53_initfn(Object *obj)
>      define_arm_cp_regs(cpu, cortex_a57_a53_cp_reginfo);
>  }
>  
> +/* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
> + * otherwise, a CPU with as many features enabled as our emulation supports.
> + * The version of '-cpu max' for qemu-system-arm is defined in cpu.c;
> + * this only needs to handle 64 bits.
> + */
> +static void aarch64_max_initfn(Object *obj)
> +{
> +    ARMCPU *cpu = ARM_CPU(obj);
> +
> +    if (kvm_enabled()) {
> +        kvm_arm_set_cpu_features_from_host(cpu);
> +    } else {
> +        aarch64_a57_initfn(obj);
> +        /* In future we might add feature bits here even if the
> +         * real-world A57 doesn't implement them.
> +         */
> +    }
> +}
> +
>  #ifdef CONFIG_USER_ONLY
>  static void aarch64_any_initfn(Object *obj)
>  {
> @@ -240,6 +260,7 @@ typedef struct ARMCPUInfo {
>  static const ARMCPUInfo aarch64_cpus[] = {
>      { .name = "cortex-a57",         .initfn = aarch64_a57_initfn },
>      { .name = "cortex-a53",         .initfn = aarch64_a53_initfn },
> +    { .name = "max",                .initfn = aarch64_max_initfn },
>  #ifdef CONFIG_USER_ONLY
>      { .name = "any",         .initfn = aarch64_any_initfn },
>  #endif
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/6] hw/arm/virt: Check that the CPU realize method succeeded

[Philippe Mathieu-Daudé]

On 12/07/2017 03:14 PM, Peter Maydell wrote:
> We were passing a NULL error pointer to the object_property_set_bool()
> call that realizes the CPU object. This meant that we wouldn't detect
> failure, and would plough blindly on to crash later trying to use a
> NULL CPU object pointer. Detect errors and fail instead.
> 
> In particular, this will be necessary to detect the user error
> of using "-cpu host" without "-enable-kvm" once we make the host
> CPU type be registered unconditionally rather than only in
> kvm_arch_init().
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/arm/virt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 151592b..62af013 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -1402,7 +1402,7 @@ static void machvirt_init(MachineState *machine)
>                                       "secure-memory", &error_abort);
>          }
>  
> -        object_property_set_bool(cpuobj, true, "realized", NULL);
> +        object_property_set_bool(cpuobj, true, "realized", &error_fatal);
>          object_unref(cpuobj);
>      }
>      fdt_add_timer_nodes(vms);
> 

Re: [Qemu-devel] [PATCH 4/6] target/arm: Add "-cpu max" support

[Peter Maydell]

On 26 January 2018 at 14:29, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Peter,
>
> On 12/07/2017 03:14 PM, Peter Maydell wrote:
>> Add support for "-cpu max" for ARM guests. This CPU type behaves
>> like "-cpu host" when KVM is enabled, and like a system CPU with
>> the maximum possible feature set otherwise. (Note that this means
>> it won't be migratable across versions, as we will likely add
>> features to it in future.)
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---

>> +#ifndef TARGET_AARCH64
>> +/* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
>> + * otherwise, a CPU with as many features enabled as our emulation supports.
>> + * The version of '-cpu max' for qemu-system-aarch64 is defined in cpu64.c;
>> + * this only needs to handle 32 bits.
>> + */
>> +static void arm_max_initfn(Object *obj)
>> +{
>> +    ARMCPU *cpu = ARM_CPU(obj);
>> +
>> +    if (kvm_enabled()) {
>> +        kvm_arm_set_cpu_features_from_host(cpu);
>> +    } else {
>> +        cortex_a15_initfn(obj);> +        /* In future we might add feature bits here even if the
>> +         * real-world A15 doesn't implement them.
>> +         */
>
> Why not use arm_any_initfn() here?

That function (and the 'any' cpu) are deliberately only
included in the linux-user binaries, not the system-emulation binaries.
(Also arm_any_initfn() only initializes userspace-visible stuff, it
doesn't provide ID register values etc for kernel-visible things.)

> Actually what seems cleaner is to move "any" features here, and kill the
> "any" cpu, using "max" for this purpose.

We can't kill 'any', that would break back-compatibility
of command lines.

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH 1/6] hw/arm/virt: Check that the CPU realize method succeeded

[Peter Maydell]

On 26 January 2018 at 14:32, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 12/07/2017 03:14 PM, Peter Maydell wrote:
>> We were passing a NULL error pointer to the object_property_set_bool()
>> call that realizes the CPU object. This meant that we wouldn't detect
>> failure, and would plough blindly on to crash later trying to use a
>> NULL CPU object pointer. Detect errors and fail instead.
>>
>> In particular, this will be necessary to detect the user error
>> of using "-cpu host" without "-enable-kvm" once we make the host
>> CPU type be registered unconditionally rather than only in
>> kvm_arch_init().
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Thanks; I already put this patch in master (commit c88bc3e0dbe7), though.

-- PMM

Re: [Qemu-devel] [PATCH 4/6] target/arm: Add "-cpu max" support

[Philippe Mathieu-Daudé]

On 01/26/2018 11:33 AM, Peter Maydell wrote:
> On 26 January 2018 at 14:29, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Hi Peter,
>>
>> On 12/07/2017 03:14 PM, Peter Maydell wrote:
>>> Add support for "-cpu max" for ARM guests. This CPU type behaves
>>> like "-cpu host" when KVM is enabled, and like a system CPU with
>>> the maximum possible feature set otherwise. (Note that this means
>>> it won't be migratable across versions, as we will likely add
>>> features to it in future.)
>>>
>>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>>> ---
> 
>>> +#ifndef TARGET_AARCH64
>>> +/* -cpu max: if KVM is enabled, like -cpu host (best possible with this host);
>>> + * otherwise, a CPU with as many features enabled as our emulation supports.
>>> + * The version of '-cpu max' for qemu-system-aarch64 is defined in cpu64.c;
>>> + * this only needs to handle 32 bits.
>>> + */
>>> +static void arm_max_initfn(Object *obj)
>>> +{
>>> +    ARMCPU *cpu = ARM_CPU(obj);
>>> +
>>> +    if (kvm_enabled()) {
>>> +        kvm_arm_set_cpu_features_from_host(cpu);
>>> +    } else {
>>> +        cortex_a15_initfn(obj);> +        /* In future we might add feature bits here even if the
>>> +         * real-world A15 doesn't implement them.
>>> +         */
>>
>> Why not use arm_any_initfn() here?
> 
> That function (and the 'any' cpu) are deliberately only
> included in the linux-user binaries, not the system-emulation binaries.

why not use the V8 features?

> (Also arm_any_initfn() only initializes userspace-visible stuff, it
> doesn't provide ID register values etc for kernel-visible things.)

I'd still use an unique arm_max_initfn() such

  // initializes userspace-visible stuff
#ifndef CONFIG_USER_ONLY
  // initializes kernel-visible things
#endif

> 
>> Actually what seems cleaner is to move "any" features here, and kill the
>> "any" cpu, using "max" for this purpose.
> 
> We can't kill 'any', that would break back-compatibility
> of command lines.

and use an alias for 'any' -> 'max' or just

  { .name = "any", .initfn = arm_max_initfn }, /* backward compat */

Anyway:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/6] arm: support -cpu max (and gic-version=max)

[Eduardo Habkost]

On Fri, Jan 26, 2018 at 11:02:24AM +0000, Peter Maydell wrote:
> On 26 January 2018 at 10:42, Eduardo Habkost <ehabkost@redhat.com> wrote:
> > On Thu, Jan 25, 2018 at 02:41:50PM +0000, Peter Maydell wrote:
> >> I'm not sure if we actually support command line setting/unsetting
> >> of features for Arm CPUs -- is there a command line option to
> >> get QEMU to print the features it thinks can be set this way?
> >
> > Unfortunately -cpu command-line parsing is still a mess (we
> > currently have lots of arch-specific parsing hooks).  Once we
> > make this uniform across targets, we could make "-cpu ?" print
> > all known properties.
> >
> > But you can look at the list of QOM properties for your CPU
> > classes (-cpu options are simply translated to QOM properties).
> > e.g.:
> >
> >   (QEMU) device-list-properties typename=pxa270-a0-arm-cpu
> >   {"return": [{"type": "uint32", "name": "midr"}, {"type": "uint64", "name": "mp-affinity"}, {"type": "child<irq>", "name": "unnamed-gpio-in[0]"}, {"type": "uint32", "name": "psci-conduit"}, {"type": "bool", "name": "reset-hivecs"}, {"type": "link<qemu:memory-region>", "name": "memory"}, {"type": "link<irq>", "name": "unnamed-gpio-out[2]"}, {"type": "link<irq>", "name": "unnamed-gpio-out[3]"}, {"type": "int32", "name": "node-id"}, {"type": "bool", "name": "start-powered-off"}, {"type": "link<irq>", "name": "unnamed-gpio-out[1]"}, {"type": "link<irq>", "name": "unnamed-gpio-out[0]"}, {"type": "link<irq>", "name": "gicv3-maintenance-interrupt[0]"}, {"type": "bool", "name": "cfgend"}, {"type": "child<irq>", "name": "unnamed-gpio-in[2]"}, {"type": "child<irq>", "name": "unnamed-gpio-in[3]"}, {"type": "child<irq>", "name": "unnamed-gpio-in[1]"}]}
> 
> None of those are things we'd want to expose to the user, really
> (except maybe 'cfgend'): they're all intended for the QEMU board
> or SoC code that needs to configure and wire the CPU up. Ideally
> there'd be a mechanism for screening them out of the -cpu option
> list.
> 

Yeah, it's becoming clearer to me that we need to address this
better than with a simple "x-" prefix convention.

If we remove the link and child properties, we have:

midr (uint32)
mp-affinity (uint64)
psci-conduit (uint32)
reset-hivecs (bool)
node-id (int32)
start-powered-off (bool)
cfgend (bool)

I wonder how many of them aren't useful with -cpu but might be
useful with -device in the future.


> There are some things that we could in theory have as user
> settable properties (like "does this CPU have an FPU"), but
> we don't currently have QOM properties for them (each CPU
> just hardcodes which ARM_FEATURE_* flags it has).
> 
> In your other email you write:
> > I wrote:
> >> Given that the type names don't appear to the user, I think
> >> we can go ahead with implementing "-cpu max" for Arm without
> >> having to first disentangle this? "max" isn't in any worse
> >> a position than the existing "host" and "any" types.
> >
> >Sounds reasonable to me.
> 
> Cool. I think that means that patches 2-6 here don't need
> any changes, but I'll rebase, retest and resend just to
> avoid confusion.
> 
> thanks
> -- PMM

-- 
Eduardo

Re: [Qemu-devel] [Qemu-arm] [PATCH 0/6] arm: support -cpu max (and gic-version=max)

[Peter Maydell]

On 26 January 2018 at 17:54, Eduardo Habkost <ehabkost@redhat.com> wrote:
> On Fri, Jan 26, 2018 at 11:02:24AM +0000, Peter Maydell wrote:
>> None of those are things we'd want to expose to the user, really
>> (except maybe 'cfgend'): they're all intended for the QEMU board
>> or SoC code that needs to configure and wire the CPU up. Ideally
>> there'd be a mechanism for screening them out of the -cpu option
>> list.
>>
>
> Yeah, it's becoming clearer to me that we need to address this
> better than with a simple "x-" prefix convention.
>
> If we remove the link and child properties, we have:
>
> midr (uint32)
> mp-affinity (uint64)
> psci-conduit (uint32)
> reset-hivecs (bool)
> node-id (int32)
> start-powered-off (bool)
> cfgend (bool)
>
> I wonder how many of them aren't useful with -cpu but might be
> useful with -device in the future.

It would require a lot of work to be able to create an Arm CPU
with -device -- there's no way at the moment to wire it up
to the interrupt controller...

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/6] target/arm: Add "-cpu max" support

[Peter Maydell]

On 26 January 2018 at 15:44, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> On 01/26/2018 11:33 AM, Peter Maydell wrote:
>> On 26 January 2018 at 14:29, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>> Why not use arm_any_initfn() here?
>>
>> That function (and the 'any' cpu) are deliberately only
>> included in the linux-user binaries, not the system-emulation binaries.
>
> why not use the V8 features?

What v8 features?

>> (Also arm_any_initfn() only initializes userspace-visible stuff, it
>> doesn't provide ID register values etc for kernel-visible things.)
>
> I'd still use an unique arm_max_initfn() such
>
>   // initializes userspace-visible stuff
> #ifndef CONFIG_USER_ONLY
>   // initializes kernel-visible things
> #endif

>>> Actually what seems cleaner is to move "any" features here, and kill the
>>> "any" cpu, using "max" for this purpose.
>>
>> We can't kill 'any', that would break back-compatibility
>> of command lines.
>
> and use an alias for 'any' -> 'max' or just
>
>   { .name = "any", .initfn = arm_max_initfn }, /* backward compat */

Yes, we could probably do something similar to this.

thanks
-- PMM

Re: [Qemu-devel] [PATCH 4/6] target/arm: Add "-cpu max" support

[Igor Mammedov]

On Fri, 2 Feb 2018 17:54:43 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 26 January 2018 at 15:44, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > On 01/26/2018 11:33 AM, Peter Maydell wrote:  
> >> On 26 January 2018 at 14:29, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:  
> >>> Why not use arm_any_initfn() here?  
> >>
> >> That function (and the 'any' cpu) are deliberately only
> >> included in the linux-user binaries, not the system-emulation binaries.  
> >
> > why not use the V8 features?  
> 
> What v8 features?
> 
> >> (Also arm_any_initfn() only initializes userspace-visible stuff, it
> >> doesn't provide ID register values etc for kernel-visible things.)  
> >
> > I'd still use an unique arm_max_initfn() such
> >
> >   // initializes userspace-visible stuff
> > #ifndef CONFIG_USER_ONLY
> >   // initializes kernel-visible things
> > #endif  
> 
> >>> Actually what seems cleaner is to move "any" features here, and kill the
> >>> "any" cpu, using "max" for this purpose.  
> >>
> >> We can't kill 'any', that would break back-compatibility
> >> of command lines.  
> >
> > and use an alias for 'any' -> 'max' or just
I'd suggest to place easy any -> max compat hack into
arm_cpu_class_by_name()

> >
> >   { .name = "any", .initfn = arm_max_initfn }, /* backward compat */  
> 
> Yes, we could probably do something similar to this.
> 
> thanks
> -- PMM
>