Re: [Qemu-devel] [PATCH] spapr: add "compat" machine option

[Alexey Kardashevskiy]

On 11/06/2013 12:53 AM, Andreas Färber wrote:> Am 05.11.2013 10:52, schrieb 
Paolo Bonzini:
>> Il 05/11/2013 10:16, Alexander Graf ha scritto:
>>>
>>> On 05.11.2013, at 10:06, Paolo Bonzini <address@hidden> wrote:
>>>
>>>> Il 30/09/2013 14:57, Alexey Kardashevskiy ha scritto:
>>>>>>> Why is the option under -machine instead of -cpu?
>>>>> Because it is still the same CPU and the guest will still read the real
>>>>> PVR from the hardware (which it may not support but this is why we need
>>>>> compatibility mode).
>>>>
>>>> How do you support migration from a newer to an older CPU then?  I think
>>>> the guest should never see anything about the hardware CPU model.
>>>
>>> POWER can't model that. It always leaks the host CPU information into the 
>>> guest. It's the guest kernel's responsibility to not expose that change to 
>>> user space.
>>>
>>> Yes, it's broken :). I'm not even sure there is any sensible way to do live 
>>> migration between different CPU types.
>>
>> Still in my opinion it should be "-cpu", not "-machine".  Even if it's
>> just a "virtual" CPU model.
>
> PowerPC currently does not have -cpu option parsing. If you need to
> implement it, I would ask for a generic hook in CPUClass set by
> TYPE_POWERPC_CPU, so that the logic does not get hardcoded in cpu_init,
> and for the p=v parsing logic to be so generic as to just set property p
> to value v on the CPU instance. I.e. please make the compatibility
> settings a static property or dynamic property of the CPU.
>
> Maybe the parsing code could even live in generic qom/cpu.c, overridden
> by x86/sparc and reused for arm? Somewhere down my to-do list but
> patches appreciated...


I spent some time today trying to digest what you said, still having problems
with understanding of what you meant and what Igor meant about global variables
(I just do not see the point in them).

Below is the result of my excercise. At the moment I would just like to know
if I am going in the right direction or not.

And few question while we are here:
1. the proposed common code handles both static and dynamic properties.
What is the current QEMU trend about choosing static vs. dynamic? Can do
both in POWERPC, both have benifits.

2. The static powerpc_properties array only works if defined with POWER7 family
but not POWER family. Both families are abstract so I did not expect any
difference but it is there. Any clue before I continue debugging? :)

Thanks!

---

This adds suboptions support for -cpu and demonstrates the use ot this
by adding a static "compat1" and a dynamic "compat" options to POWERPC
CPUs.

---
 include/qom/cpu.h           |  6 ++++++
 include/sysemu/sysemu.h     |  1 +
 qom/cpu.c                   | 24 ++++++++++++++++++++++++
 target-ppc/translate_init.c | 40 ++++++++++++++++++++++++++++++++++++++++
 vl.c                        | 35 ++++++++++++++++++++++++++++++++++-
 5 files changed, 105 insertions(+), 1 deletion(-)

diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 7739e00..a17cd73 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -327,6 +327,12 @@ static inline hwaddr cpu_get_phys_page_debug(CPUState 
*cpu, vaddr addr)
 #endif
 
 /**
+ * cpu_common_set_properties:
+ * @cpu: The CPU whose properties to initialize from the command line.
+ */
+int cpu_common_set_properties(Object *obj);
+
+/**
  * cpu_reset:
  * @cpu: The CPU whose state is to be reset.
  */
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index b27a1df..9007e44 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -192,6 +192,7 @@ DeviceState *get_boot_device_ex(uint32_t position, char 
**suffix);
 DeviceState *get_boot_device(uint32_t position);
 
 QemuOpts *qemu_get_machine_opts(void);
+QemuOpts *qemu_get_cpu_opts(void);
 
 bool usb_enabled(bool default_usb);
 
diff --git a/qom/cpu.c b/qom/cpu.c
index 818fb26..6516c63 100644
--- a/qom/cpu.c
+++ b/qom/cpu.c
@@ -24,6 +24,8 @@
 #include "qemu/notify.h"
 #include "qemu/log.h"
 #include "sysemu/sysemu.h"
+#include "hw/qdev-properties.h"
+#include "qapi/qmp/qerror.h"
 
 bool cpu_exists(int64_t id)
 {
@@ -186,6 +188,28 @@ void cpu_reset(CPUState *cpu)
     }
 }
 
+static int cpu_set_property(const char *name, const char *value, void *opaque)
+{
+    DeviceState *dev = opaque;
+    Error *err = NULL;
+
+    if (strcmp(name, "type") == 0)
+        return 0;
+
+    qdev_prop_parse(dev, name, value, &err);
+    if (err != NULL) {
+        qerror_report_err(err);
+        error_free(err);
+        return -1;
+    }
+    return 0;
+}
+
+int cpu_common_set_properties(Object *obj)
+{
+    return qemu_opt_foreach(qemu_get_cpu_opts(), cpu_set_property, obj, 1);
+}
+
 static void cpu_common_reset(CPUState *cpu)
 {
     CPUClass *cc = CPU_GET_CLASS(cpu);
diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 93f7cba..d357b1f 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -28,6 +28,8 @@
 #include "mmu-hash32.h"
 #include "mmu-hash64.h"
 #include "qemu/error-report.h"
+#include "hw/qdev-properties.h"
+#include "qapi/visitor.h"
 
 //#define PPC_DUMP_CPU
 //#define PPC_DEBUG_SPR
@@ -4733,6 +4735,11 @@ POWERPC_FAMILY(e5500)(ObjectClass *oc, void *data)
 
 /* Non-embedded PowerPC                                                      */
 
+static Property powerpc_properties[] = {
+    DEFINE_PROP_INT32("compat1", PowerPCCPU, env.compat, -1),
+    DEFINE_PROP_END_OF_LIST(),
+};
+
 /* POWER : same as 601, without mfmsr, mfsr                                  */
 POWERPC_FAMILY(POWER)(ObjectClass *oc, void *data)
 {
@@ -4743,6 +4750,7 @@ POWERPC_FAMILY(POWER)(ObjectClass *oc, void *data)
     /* pcc->insns_flags = XXX_TODO; */
     /* POWER RSC (from RAD6000) */
     pcc->msr_mask = 0x00000000FEF0ULL;
+    dc->props = powerpc_properties;
 }
 
 #define POWERPC_MSRR_601     (0x0000000000001040ULL)
@@ -7262,6 +7270,7 @@ POWERPC_FAMILY(POWER7)(ObjectClass *oc, void *data)
                  POWERPC_FLAG_BUS_CLK | POWERPC_FLAG_CFAR;
     pcc->l1_dcache_size = 0x8000;
     pcc->l1_icache_size = 0x8000;
+    dc->props = powerpc_properties;
 }
 
 POWERPC_FAMILY(POWER7P)(ObjectClass *oc, void *data)
@@ -8390,6 +8399,10 @@ PowerPCCPU *cpu_ppc_init(const char *cpu_model)
 
     cpu = POWERPC_CPU(object_new(object_class_get_name(oc)));
 
+    if (cpu_common_set_properties(OBJECT(cpu))) {
+        return NULL;
+    }
+
     object_property_set_bool(OBJECT(cpu), true, "realized", &err);
     if (err != NULL) {
         error_report("%s", error_get_pretty(err));
@@ -8611,6 +8624,30 @@ static void ppc_cpu_reset(CPUState *s)
     tlb_flush(env, 1);
 }
 
+static void powerpc_get_compat(Object *obj, Visitor *v,
+                               void *opaque, const char *name, Error **errp)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(obj);
+    int64_t value = cpu->env.compat;
+
+    visit_type_int(v, &value, name, errp);
+}
+
+static void powerpc_set_compat(Object *obj, Visitor *v,
+                               void *opaque, const char *name, Error **errp)
+{
+    PowerPCCPU *cpu = POWERPC_CPU(obj);
+    Error *error = NULL;
+    int64_t value;
+
+    visit_type_int(v, &value, name, &error);
+    if (error) {
+        error_propagate(errp, error);
+        return;
+    }
+    cpu->env.compat = value;
+}
+
 static void ppc_cpu_initfn(Object *obj)
 {
     CPUState *cs = CPU(obj);
@@ -8618,6 +8655,9 @@ static void ppc_cpu_initfn(Object *obj)
     PowerPCCPUClass *pcc = POWERPC_CPU_GET_CLASS(cpu);
     CPUPPCState *env = &cpu->env;
 
+    object_property_add(obj, "compat", "int",
+                        powerpc_get_compat, powerpc_set_compat,
+                        NULL, NULL, NULL);
     cs->env_ptr = env;
     cpu_exec_init(env);
 
diff --git a/vl.c b/vl.c
index d5c5d8c..a5fbc38 100644
--- a/vl.c
+++ b/vl.c
@@ -437,6 +437,16 @@ static QemuOptsList qemu_machine_opts = {
     },
 };
 
+static QemuOptsList qemu_cpu_opts = {
+    .name = "cpu",
+    .implied_opt_name = "type",
+    .merge_lists = true,
+    .head = QTAILQ_HEAD_INITIALIZER(qemu_cpu_opts.head),
+    .desc = {
+        { /* End of list */ }
+    },
+};
+
 static QemuOptsList qemu_boot_opts = {
     .name = "boot-opts",
     .implied_opt_name = "order",
@@ -552,6 +562,20 @@ QemuOpts *qemu_get_machine_opts(void)
     return opts;
 }
 
+QemuOpts *qemu_get_cpu_opts(void)
+{
+    QemuOptsList *list;
+    QemuOpts *opts;
+
+    list = qemu_find_opts("cpu");
+    assert(list);
+    opts = qemu_opts_find(list, NULL);
+    if (!opts) {
+        opts = qemu_opts_create_nofail(list);
+    }
+    return opts;
+}
+
 const char *qemu_get_vm_name(void)
 {
     return qemu_name;
@@ -2942,6 +2966,7 @@ int main(int argc, char **argv, char **envp)
     qemu_add_opts(&qemu_trace_opts);
     qemu_add_opts(&qemu_option_rom_opts);
     qemu_add_opts(&qemu_machine_opts);
+    qemu_add_opts(&qemu_cpu_opts);
     qemu_add_opts(&qemu_smp_opts);
     qemu_add_opts(&qemu_boot_opts);
     qemu_add_opts(&qemu_sandbox_opts);
@@ -3037,7 +3062,15 @@ int main(int argc, char **argv, char **envp)
             }
             case QEMU_OPTION_cpu:
                 /* hw initialization will check this */
-                cpu_model = optarg;
+                olist = qemu_find_opts("cpu");
+                opts = qemu_opts_parse(olist, optarg, 1);
+                if (!opts) {
+                    exit(1);
+                }
+                optarg = qemu_opt_get(opts, "type");
+                if (optarg) {
+                    cpu_model = optarg;
+                }
                 break;
             case QEMU_OPTION_hda:
                 {
-- 
1.8.4.rc4