[Qemu-devel] [RFC 0/3] generalize parsing of cpu_model

[Qemu-devel] [RFC 0/3] generalize parsing of cpu_model

[Igor Mammedov]

Some callers call CPUClass->parse_features manually to convert
'-cpu cpufoo,featurestr' string to cpu type and featurestr
into a set of global properties. And theni do controlled
cpu creation with setting properties and completing it with realize.
That's a lot of code duplication as they are practically
reimplement the same parsing logic.

Some don't and use cpu_generic_init() instead which does
the same parsing along with creation/realizing cpu within one
wrapper.

And some trying to switch to controlled cpu creation,
implement object_new()/set properties/realize steps
but forget feature parsing logic witch lieads to 'bugs'
commit 00909b585 (hw/arm/integratorcp: Support specifying features via -cpu)


This series moves -cpu option parsing to generic machine code
that removes a little of code duplication and makes cpus creation
process more unified.

PS:
As I don't have time to rewrite this part QEMU, being busy
rewritting yet another part of QEMU,
SERIES IS UNFINISHED AND SERVERS TO SHOW IDEA HOW IT SHOULD
BE DONE. FEEL FREE TO PICK UP AND COMPLETE THIS PATCHES
TO HANDLE ALL BOARDS (series does it only for virt-arm/spapr/pc)

It compiles and pc machine even starts but otherwise is untested.

CC: Eduardo Habkost <ehabkost@redhat.com>
CC: patches@linaro.org
CC: Peter Maydell <peter.maydell@linaro.org>
CC: Marcel Apfelbaum <marcel@redhat.com>
CC: Paolo Bonzini <pbonzini@redhat.com>
CC: David Gibson <david@gibson.dropbear.id.au>
CC: qemu-arm@nongnu.org
CC: qemu-ppc@nongnu.org
CC: "Michael S. Tsirkin" <mst@redhat.com>

Igor Mammedov (3):
  machine: call machine init from wrapper
  machine: generalize handling of default cpu_model
  machine: generilize cpu_model parsing

 include/hw/boards.h  |  5 +++++
 include/hw/ppc/ppc.h |  2 --
 hw/arm/virt.c        | 46 ++++++++++---------------------------------
 hw/core/machine.c    | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 hw/i386/pc.c         | 42 +++++++++++++--------------------------
 hw/ppc/ppc.c         | 25 ------------------------
 hw/ppc/spapr.c       | 14 +++++++------
 vl.c                 |  2 +-
 8 files changed, 92 insertions(+), 99 deletions(-)

-- 
2.7.4

[Qemu-devel] [RFC 1/3] machine: call machine init from wrapper

[Igor Mammedov]

add machine_run_board_init() wrapper that calls
machine init for now but in follow up patches
it will be used to run generic code that should run
before machine init.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/boards.h | 1 +
 hw/core/machine.c   | 6 ++++++
 vl.c                | 2 +-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 269d0ba..04f5352 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -32,6 +32,7 @@ void memory_region_allocate_system_memory(MemoryRegion *mr, Object *owner,
 MachineClass *find_default_machine(void);
 extern MachineState *current_machine;
 
+void machine_run_board_init(MachineState *machine);
 bool machine_usb(MachineState *machine);
 bool machine_kernel_irqchip_allowed(MachineState *machine);
 bool machine_kernel_irqchip_required(MachineState *machine);
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 0699750..fe82529 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -575,6 +575,12 @@ bool machine_mem_merge(MachineState *machine)
     return machine->mem_merge;
 }
 
+void machine_run_board_init(MachineState *machine)
+{
+    MachineClass *machine_class = MACHINE_GET_CLASS(machine);
+    machine_class->init(machine);
+}
+
 static void machine_class_finalize(ObjectClass *klass, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(klass);
diff --git a/vl.c b/vl.c
index 93406ba..9af4462 100644
--- a/vl.c
+++ b/vl.c
@@ -4484,7 +4484,7 @@ int main(int argc, char **argv, char **envp)
     current_machine->boot_order = boot_order;
     current_machine->cpu_model = cpu_model;
 
-    machine_class->init(current_machine);
+    machine_run_board_init(current_machine);
 
     realtime_init();
 
-- 
2.7.4

[Qemu-devel] [RFC 2/3] machine: generalize handling of default cpu_model

[Igor Mammedov]

currently all boards have opencoded default cpu_model
selection. I most cases it's just a string and in others
it's a 'function'. Add to machine callback
that returns default cpu_model and make boards return
it as const string.

That allows to move detection of non specified cpu_model
i.e. missing CLI '-cpu' option and move detection
to generic machine code. And would allow to generalize
parsing cpu features in follow up patch.

TODO:
 complete conversion for all boards

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/boards.h |  1 +
 hw/arm/virt.c       | 10 ++++++----
 hw/core/machine.c   |  7 +++++++
 hw/i386/pc.c        | 18 ++++++++++--------
 hw/ppc/spapr.c      | 11 +++++++----
 5 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 04f5352..9f2dbfd 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -141,6 +141,7 @@ struct MachineClass {
                                            DeviceState *dev);
     unsigned (*cpu_index_to_socket_id)(unsigned cpu_index);
     const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
+    const char *(*default_cpu_model)(MachineState *machine);
 };
 
 /**
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index a98cb91..8380540 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -1252,10 +1252,6 @@ static void machvirt_init(MachineState *machine)
     Error *err = NULL;
     bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
 
-    if (!cpu_model) {
-        cpu_model = "cortex-a15";
-    }
-
     /* We can probe only here because during property set
      * KVM is not available yet
      */
@@ -1564,6 +1560,11 @@ static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
     return ms->possible_cpus;
 }
 
+static const char *virt_default_cpu_model(MachineState *machine)
+{
+    return "cortex-a15";
+}
+
 static void virt_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -1581,6 +1582,7 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
     /* We know we will never create a pre-ARMv7 CPU which needs 1K pages */
     mc->minimum_page_bits = 12;
     mc->possible_cpu_arch_ids = virt_possible_cpu_arch_ids;
+    mc->default_cpu_model = virt_default_cpu_model;
 }
 
 static const TypeInfo virt_machine_info = {
diff --git a/hw/core/machine.c b/hw/core/machine.c
index fe82529..2a954f0 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -578,6 +578,13 @@ bool machine_mem_merge(MachineState *machine)
 void machine_run_board_init(MachineState *machine)
 {
     MachineClass *machine_class = MACHINE_GET_CLASS(machine);
+
+    /* Force all boards to provide default_cpu_model callback */
+    assert(machine_class->default_cpu_model);
+    if (machine->cpu_model == NULL) {
+        machine->cpu_model = machine_class->default_cpu_model(machine);
+    }
+
     machine_class->init(machine);
 }
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index a8660d4..0073469 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1149,14 +1149,6 @@ void pc_cpus_init(PCMachineState *pcms)
     MachineClass *mc = MACHINE_GET_CLASS(pcms);
 
     /* init CPUs */
-    if (machine->cpu_model == NULL) {
-#ifdef TARGET_X86_64
-        machine->cpu_model = "qemu64";
-#else
-        machine->cpu_model = "qemu32";
-#endif
-    }
-
     model_pieces = g_strsplit(machine->cpu_model, ",", 2);
     if (!model_pieces[0]) {
         error_report("Invalid/empty CPU model name");
@@ -2296,6 +2288,15 @@ static void x86_nmi(NMIState *n, int cpu_index, Error **errp)
     }
 }
 
+static const char *pc_default_cpu_model(MachineState *machine)
+{
+#ifdef TARGET_X86_64
+        return "qemu64";
+#else
+        return "qemu32";
+#endif
+}
+
 static void pc_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -2324,6 +2325,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     mc->default_boot_order = "cad";
     mc->hot_add_cpu = pc_hot_add_cpu;
     mc->max_cpus = 255;
+    mc->default_cpu_model = pc_default_cpu_model;
     mc->reset = pc_machine_reset;
     hc->pre_plug = pc_machine_device_pre_plug_cb;
     hc->plug = pc_machine_device_plug_cb;
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 6f37288..8f30765 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1920,10 +1920,6 @@ static void ppc_spapr_init(MachineState *machine)
     }
 
     /* init CPUs */
-    if (machine->cpu_model == NULL) {
-        machine->cpu_model = kvm_enabled() ? "host" : smc->tcg_default_cpu;
-    }
-
     ppc_cpu_parse_features(machine->cpu_model);
 
     spapr_init_cpus(spapr);
@@ -2861,6 +2857,12 @@ static void spapr_phb_placement(sPAPRMachineState *spapr, uint32_t index,
     *mmio64 = SPAPR_PCI_BASE + (index + 1) * SPAPR_PCI_MEM64_WIN_SIZE;
 }
 
+static const char *spapr_default_cpu_model(MachineState *machine)
+{
+    sPAPRMachineClass *smc = SPAPR_MACHINE_GET_CLASS(machine);
+    return machine->cpu_model = kvm_enabled() ? "host" : smc->tcg_default_cpu;
+}
+
 static void spapr_machine_class_init(ObjectClass *oc, void *data)
 {
     MachineClass *mc = MACHINE_CLASS(oc);
@@ -2893,6 +2895,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     hc->unplug = spapr_machine_device_unplug;
     mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
     mc->possible_cpu_arch_ids = spapr_possible_cpu_arch_ids;
+    mc->default_cpu_model = spapr_default_cpu_model;
     hc->unplug_request = spapr_machine_device_unplug_request;
 
     smc->dr_lmb_enabled = true;
-- 
2.7.4

[Qemu-devel] [RFC 3/3] machine: generilize cpu_model parsing

[Igor Mammedov]

Parse cpu_model string into cpu_type and
[=-]foo features in common machine code
instead of doing the same on every board.

TODO:
patch handles only virt-arm/spapr/pc boards,
but to avoid bisection breakage it should take
care of all boards.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 include/hw/boards.h  |  3 +++
 include/hw/ppc/ppc.h |  2 --
 hw/arm/virt.c        | 36 ++++--------------------------------
 hw/core/machine.c    | 44 +++++++++++++++++++++++++++++++++++++++++++-
 hw/i386/pc.c         | 24 +++---------------------
 hw/ppc/ppc.c         | 25 -------------------------
 hw/ppc/spapr.c       |  3 +--
 7 files changed, 54 insertions(+), 83 deletions(-)

diff --git a/include/hw/boards.h b/include/hw/boards.h
index 9f2dbfd..3374a49 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -136,12 +136,14 @@ struct MachineClass {
     bool rom_file_has_mr;
     int minimum_page_bits;
     bool has_hotpluggable_cpus;
+    const char *base_cpu_type;
 
     HotplugHandler *(*get_hotplug_handler)(MachineState *machine,
                                            DeviceState *dev);
     unsigned (*cpu_index_to_socket_id)(unsigned cpu_index);
     const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
     const char *(*default_cpu_model)(MachineState *machine);
+    bool (*cpu_model_valid)(MachineState *machine, const char *cpu_model);
 };
 
 /**
@@ -182,6 +184,7 @@ struct MachineState {
     char *kernel_cmdline;
     char *initrd_filename;
     const char *cpu_model;
+    const char *cpu_typename;
     AccelState *accelerator;
     CPUArchIdList *possible_cpus;
 };
diff --git a/include/hw/ppc/ppc.h b/include/hw/ppc/ppc.h
index 4e7fe11..ff0ac30 100644
--- a/include/hw/ppc/ppc.h
+++ b/include/hw/ppc/ppc.h
@@ -105,6 +105,4 @@ enum {
 
 /* ppc_booke.c */
 void ppc_booke_timers_init(PowerPCCPU *cpu, uint32_t freq, uint32_t flags);
-
-void ppc_cpu_parse_features(const char *cpu_model);
 #endif
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 8380540..d767200 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -169,7 +169,7 @@ static const char *valid_cpus[] = {
     "host",
 };
 
-static bool cpuname_valid(const char *cpu)
+static bool cpuname_valid(MachineState *machine, const char *cpu)
 {
     int i;
 
@@ -1244,12 +1244,6 @@ static void machvirt_init(MachineState *machine)
     MemoryRegion *secure_sysmem = NULL;
     int n, virt_max_cpus;
     MemoryRegion *ram = g_new(MemoryRegion, 1);
-    const char *cpu_model = machine->cpu_model;
-    char **cpustr;
-    ObjectClass *oc;
-    const char *typename;
-    CPUClass *cc;
-    Error *err = NULL;
     bool firmware_loaded = bios_name || drive_get(IF_PFLASH, 0, 0);
 
     /* We can probe only here because during property set
@@ -1268,14 +1262,6 @@ static void machvirt_init(MachineState *machine)
         }
     }
 
-    /* Separate the actual CPU model name from any appended features */
-    cpustr = g_strsplit(cpu_model, ",", 2);
-
-    if (!cpuname_valid(cpustr[0])) {
-        error_report("mach-virt: CPU %s not supported", cpustr[0]);
-        exit(1);
-    }
-
     /* If we have an EL3 boot ROM then the assumption is that it will
      * implement PSCI itself, so disable QEMU's internal implementation
      * so it doesn't get in the way. Instead of starting secondary
@@ -1342,22 +1328,6 @@ static void machvirt_init(MachineState *machine)
 
     create_fdt(vms);
 
-    oc = cpu_class_by_name(TYPE_ARM_CPU, cpustr[0]);
-    if (!oc) {
-        error_report("Unable to find CPU definition");
-        exit(1);
-    }
-    typename = object_class_get_name(oc);
-
-    /* convert -smp CPU options specified by the user into global props */
-    cc = CPU_CLASS(oc);
-    cc->parse_features(typename, cpustr[1], &err);
-    g_strfreev(cpustr);
-    if (err) {
-        error_report_err(err);
-        exit(1);
-    }
-
     mc->possible_cpu_arch_ids(machine);
     for (n = 0; n < machine->possible_cpus->len; n++) {
         Object *cpuobj;
@@ -1367,7 +1337,7 @@ static void machvirt_init(MachineState *machine)
             break;
         }
 
-        cpuobj = object_new(typename);
+        cpuobj = object_new(machine->cpu_typename);
         object_property_set_int(cpuobj, machine->possible_cpus->cpus[n].arch_id,
                                 "mp-affinity", NULL);
 
@@ -1583,6 +1553,8 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
     mc->minimum_page_bits = 12;
     mc->possible_cpu_arch_ids = virt_possible_cpu_arch_ids;
     mc->default_cpu_model = virt_default_cpu_model;
+    mc->cpu_model_valid = cpuname_valid;
+    mc->base_cpu_type = TYPE_ARM_CPU;
 }
 
 static const TypeInfo virt_machine_info = {
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 2a954f0..42923b1 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -575,8 +575,12 @@ bool machine_mem_merge(MachineState *machine)
     return machine->mem_merge;
 }
 
-void machine_run_board_init(MachineState *machine)
+static void machine_parse_cpu_model(MachineState *machine)
 {
+    ObjectClass *oc;
+    char **cpustr;
+    CPUClass *cc;
+    Error *err = NULL;
     MachineClass *machine_class = MACHINE_GET_CLASS(machine);
 
     /* Force all boards to provide default_cpu_model callback */
@@ -585,6 +589,44 @@ void machine_run_board_init(MachineState *machine)
         machine->cpu_model = machine_class->default_cpu_model(machine);
     }
 
+    /* Separate the actual CPU model name from any appended features */
+    cpustr = g_strsplit(machine->cpu_model, ",", 2);
+    if (!cpustr[0]) {
+        error_setg(&err, "Invalid/empty CPU model name");
+        goto out;
+    }
+
+    if (machine_class->cpu_model_valid &&
+        !machine_class->cpu_model_valid(machine, cpustr[0])) {
+        error_report("CPU %s not supported", cpustr[0]);
+        exit(1);
+    }
+
+    /* Force all boards to provide base_cpu_type */
+    assert(machine_class->base_cpu_type);
+    oc = cpu_class_by_name(machine_class->base_cpu_type, cpustr[0]);
+    if (!oc) {
+        error_report("Unable to find CPU definition: %s", cpustr[0]);
+        exit(1);
+    }
+    machine->cpu_typename = object_class_get_name(oc);
+
+    /* convert -smp CPU options specified by the user into global props */
+    cc = CPU_CLASS(oc);
+    cc->parse_features(machine->cpu_typename, cpustr[1], &err);
+out:
+    g_strfreev(cpustr);
+    if (err) {
+        error_report_err(err);
+        exit(1);
+    }
+}
+
+void machine_run_board_init(MachineState *machine)
+{
+    MachineClass *machine_class = MACHINE_GET_CLASS(machine);
+
+    machine_parse_cpu_model(machine);
     machine_class->init(machine);
 }
 
diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 0073469..9e6149f 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -1140,31 +1140,11 @@ void pc_hot_add_cpu(const int64_t id, Error **errp)
 void pc_cpus_init(PCMachineState *pcms)
 {
     int i;
-    CPUClass *cc;
-    ObjectClass *oc;
-    const char *typename;
-    gchar **model_pieces;
     const CPUArchIdList *possible_cpus;
     MachineState *machine = MACHINE(pcms);
     MachineClass *mc = MACHINE_GET_CLASS(pcms);
 
     /* init CPUs */
-    model_pieces = g_strsplit(machine->cpu_model, ",", 2);
-    if (!model_pieces[0]) {
-        error_report("Invalid/empty CPU model name");
-        exit(1);
-    }
-
-    oc = cpu_class_by_name(TYPE_X86_CPU, model_pieces[0]);
-    if (oc == NULL) {
-        error_report("Unable to find CPU definition: %s", model_pieces[0]);
-        exit(1);
-    }
-    typename = object_class_get_name(oc);
-    cc = CPU_CLASS(oc);
-    cc->parse_features(typename, model_pieces[1], &error_fatal);
-    g_strfreev(model_pieces);
-
     /* Calculates the limit to CPU APIC ID values
      *
      * Limit for the APIC ID value, so that all
@@ -1175,7 +1155,8 @@ void pc_cpus_init(PCMachineState *pcms)
     pcms->apic_id_limit = x86_cpu_apic_id_from_index(max_cpus - 1) + 1;
     possible_cpus = mc->possible_cpu_arch_ids(machine);
     for (i = 0; i < smp_cpus; i++) {
-        pc_new_cpu(typename, possible_cpus->cpus[i].arch_id, &error_fatal);
+        pc_new_cpu(machine->cpu_typename,
+                   possible_cpus->cpus[i].arch_id, &error_fatal);
     }
 }
 
@@ -2326,6 +2307,7 @@ static void pc_machine_class_init(ObjectClass *oc, void *data)
     mc->hot_add_cpu = pc_hot_add_cpu;
     mc->max_cpus = 255;
     mc->default_cpu_model = pc_default_cpu_model;
+    mc->base_cpu_type = TYPE_X86_CPU;
     mc->reset = pc_machine_reset;
     hc->pre_plug = pc_machine_device_pre_plug_cb;
     hc->plug = pc_machine_device_plug_cb;
diff --git a/hw/ppc/ppc.c b/hw/ppc/ppc.c
index d171e60..8587173 100644
--- a/hw/ppc/ppc.c
+++ b/hw/ppc/ppc.c
@@ -1364,28 +1364,3 @@ PowerPCCPU *ppc_get_vcpu_by_dt_id(int cpu_dt_id)
 
     return NULL;
 }
-
-void ppc_cpu_parse_features(const char *cpu_model)
-{
-    CPUClass *cc;
-    ObjectClass *oc;
-    const char *typename;
-    gchar **model_pieces;
-
-    model_pieces = g_strsplit(cpu_model, ",", 2);
-    if (!model_pieces[0]) {
-        error_report("Invalid/empty CPU model name");
-        exit(1);
-    }
-
-    oc = cpu_class_by_name(TYPE_POWERPC_CPU, model_pieces[0]);
-    if (oc == NULL) {
-        error_report("Unable to find CPU definition: %s", model_pieces[0]);
-        exit(1);
-    }
-
-    typename = object_class_get_name(oc);
-    cc = CPU_CLASS(oc);
-    cc->parse_features(typename, model_pieces[1], &error_fatal);
-    g_strfreev(model_pieces);
-}
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 8f30765..181ea9b 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -1920,8 +1920,6 @@ static void ppc_spapr_init(MachineState *machine)
     }
 
     /* init CPUs */
-    ppc_cpu_parse_features(machine->cpu_model);
-
     spapr_init_cpus(spapr);
 
     if (kvm_enabled()) {
@@ -2896,6 +2894,7 @@ static void spapr_machine_class_init(ObjectClass *oc, void *data)
     mc->cpu_index_to_socket_id = spapr_cpu_index_to_socket_id;
     mc->possible_cpu_arch_ids = spapr_possible_cpu_arch_ids;
     mc->default_cpu_model = spapr_default_cpu_model;
+    mc->base_cpu_type = TYPE_POWERPC_CPU;
     hc->unplug_request = spapr_machine_device_unplug_request;
 
     smc->dr_lmb_enabled = true;
-- 
2.7.4

Re: [Qemu-devel] [RFC 0/3] generalize parsing of cpu_model

[Peter Maydell]

On 17 February 2017 at 18:56, Igor Mammedov <imammedo@redhat.com> wrote:
> Some callers call CPUClass->parse_features manually to convert
> '-cpu cpufoo,featurestr' string to cpu type and featurestr
> into a set of global properties. And theni do controlled
> cpu creation with setting properties and completing it with realize.
> That's a lot of code duplication as they are practically
> reimplement the same parsing logic.
>
> Some don't and use cpu_generic_init() instead which does
> the same parsing along with creation/realizing cpu within one
> wrapper.
>
> And some trying to switch to controlled cpu creation,
> implement object_new()/set properties/realize steps
> but forget feature parsing logic witch lieads to 'bugs'
> commit 00909b585 (hw/arm/integratorcp: Support specifying features via -cpu)
>
>
> This series moves -cpu option parsing to generic machine code
> that removes a little of code duplication and makes cpus creation
> process more unified.

This API seems a little awkward for the SoC case, where
the board model doesn't actually know what the default
CPU model or the valid CPU models are, because it just
wants to say "create me a BCM2836 SoC" and let the SoC
object deal with determining whether it's always a cortex-a15
or if it might allow some user configurability either in
cpu choices or in optional flags.
Any thoughts about that use case?

(The stm32f205 SoC object has a "cpu-model" QOM property
that the board sets, but I think that's as much because
we somewhat awkwardly need to pass it into armv7m_init()
as a deliberate design choice.)

thanks
-- PMM

Re: [Qemu-devel] [RFC 0/3] generalize parsing of cpu_model

[Igor Mammedov]

On Fri, 17 Feb 2017 19:05:20 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 17 February 2017 at 18:56, Igor Mammedov <imammedo@redhat.com> wrote:
> > Some callers call CPUClass->parse_features manually to convert
> > '-cpu cpufoo,featurestr' string to cpu type and featurestr
> > into a set of global properties. And theni do controlled
> > cpu creation with setting properties and completing it with realize.
> > That's a lot of code duplication as they are practically
> > reimplement the same parsing logic.
> >
> > Some don't and use cpu_generic_init() instead which does
> > the same parsing along with creation/realizing cpu within one
> > wrapper.
> >
> > And some trying to switch to controlled cpu creation,
> > implement object_new()/set properties/realize steps
> > but forget feature parsing logic witch lieads to 'bugs'
> > commit 00909b585 (hw/arm/integratorcp: Support specifying features via -cpu)
> >
> >
> > This series moves -cpu option parsing to generic machine code
> > that removes a little of code duplication and makes cpus creation
> > process more unified.  
> 
> This API seems a little awkward for the SoC case, where
> the board model doesn't actually know what the default
> CPU model or the valid CPU models are, because it just
> wants to say "create me a BCM2836 SoC" and let the SoC
> object deal with determining whether it's always a cortex-a15
> or if it might allow some user configurability either in
> cpu choices or in optional flags.
> Any thoughts about that use case?
I've probably been too aggressive trying to force
conversion of all boards, assuming that all boards support
"-cpu CLI" option, currently option could be specified for
any board but it is ignored if board doesn't care
about it explicitly.

 "-cpu cpu_name,feat_str" handling is always based on
 availability of base CPU type supported by target as
 it's needed both for
   - translating cpu_name to QOM CPU type
       cpu_class_by_name(typename, name)
   - converting feat_str into set of global properties
     for matching QOM CPU type
       cc->parse_features(object_class_get_name(oc), featurestr, &err);

Boards that don't care/need '-cpu' support won't need
to do anything as we can skip cpu_name,feat_str parsing
if MachineClass::base_cpu_type isn't set (NULL by default),
that way we won't break ignored CLI if we care.
And currently we don't have convenient way to disable
feat_str parsing, but machine could be extended
with flag to [en|dis]able it explicitly.

The goal of this series is generalizing/consolidating
parsing of '-cpu' for boards that use it, removing
code duplication scattered around codebase and trying
normalize default cpu_model initialization.

For SoC with fixed CPU type it would be better to use
directly CPU type names directly but that hits legacy
cpu_init()/cpu_foo_init() wall, which is currently
cpu_model based and would be a lot of re-factoring.
I would convert cpu_init(cpu_model) to cpu_init(cpu_type)
and call '-cpu' handling logic from generic
machine/user_[bsd|linux] code (3 places in total).

> (The stm32f205 SoC object has a "cpu-model" QOM property
> that the board sets, but I think that's as much because
> we somewhat awkwardly need to pass it into armv7m_init()
> as a deliberate design choice.)
its sole user netduino2 board has cpu_model hardcodded
at board level which could be left alone or converted
to this API. Using API would make code more consistent
at the cost of more code for default/valid callbacks.

> 
> thanks
> -- PMM

Re: [Qemu-devel] [RFC 0/3] generalize parsing of cpu_model

[Peter Maydell]

On 20 February 2017 at 18:55, Igor Mammedov <imammedo@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> wrote:
>> This API seems a little awkward for the SoC case, where
>> the board model doesn't actually know what the default
>> CPU model or the valid CPU models are, because it just
>> wants to say "create me a BCM2836 SoC" and let the SoC
>> object deal with determining whether it's always a cortex-a15
>> or if it might allow some user configurability either in
>> cpu choices or in optional flags.
>> Any thoughts about that use case?
>
> I've probably been too aggressive trying to force
> conversion of all boards, assuming that all boards support
> "-cpu CLI" option, currently option could be specified for
> any board but it is ignored if board doesn't care
> about it explicitly.

I think the conversion in this patchset is OK for
the boards you've touched. It's the ones that it
doesn't touch that I need to think a little more about.

>  "-cpu cpu_name,feat_str" handling is always based on
>  availability of base CPU type supported by target as
>  it's needed both for
>    - translating cpu_name to QOM CPU type
>        cpu_class_by_name(typename, name)
>    - converting feat_str into set of global properties
>      for matching QOM CPU type
>        cc->parse_features(object_class_get_name(oc), featurestr, &err);

> Boards that don't care/need '-cpu' support won't need
> to do anything as we can skip cpu_name,feat_str parsing
> if MachineClass::base_cpu_type isn't set (NULL by default),
> that way we won't break ignored CLI if we care.
> And currently we don't have convenient way to disable
> feat_str parsing, but machine could be extended
> with flag to [en|dis]able it explicitly.
>
> The goal of this series is generalizing/consolidating
> parsing of '-cpu' for boards that use it, removing
> code duplication scattered around codebase and trying
> normalize default cpu_model initialization.

Yeah, I definitely like the cleanup to avoid all the
duplicate parse calls.

> For SoC with fixed CPU type it would be better to use
> directly CPU type names directly but that hits legacy
> cpu_init()/cpu_foo_init() wall, which is currently
> cpu_model based and would be a lot of re-factoring.
> I would convert cpu_init(cpu_model) to cpu_init(cpu_type)
> and call '-cpu' handling logic from generic
> machine/user_[bsd|linux] code (3 places in total).
>
>> (The stm32f205 SoC object has a "cpu-model" QOM property
>> that the board sets, but I think that's as much because
>> we somewhat awkwardly need to pass it into armv7m_init()
>> as a deliberate design choice.)

> its sole user netduino2 board has cpu_model hardcodded
> at board level which could be left alone or converted
> to this API.

I think this may be for the benefit of the not-yet-in-tree
netduinoplus2 board, which is a very similar SoC but
with a cortex-m4. There are likely other ways we could
model that, though.

> Using API would make code more consistent
> at the cost of more code for default/valid callbacks.

Mmm, we could have the board code just forward the
default/valid callbacks through to the SoC (which
would then either implement them or forward them again).
But I think we should definitely see if we can make
the code for handling "the CPU is actually inside some
other object" as clean as we can, because I think
that's by far the most common case. Machines like
the x86 pc and the ARM virt board where the CPU is
instantiated directly in the board code are the
non-standard cases I think. We have a lot of boards
where we do instantiate the CPU in the board code
just for legacy reasons where we don't properly
model the SoC the CPU is inside. But hardware that
really has the CPU as a top level pluggable object
is rare.

thanks
-- PMM

Re: [Qemu-devel] [RFC 0/3] generalize parsing of cpu_model

[Igor Mammedov]

On Mon, 20 Feb 2017 19:11:00 +0000
Peter Maydell <peter.maydell@linaro.org> wrote:

> On 20 February 2017 at 18:55, Igor Mammedov <imammedo@redhat.com> wrote:
> > Peter Maydell <peter.maydell@linaro.org> wrote:  
[...]
> 
> > its sole user netduino2 board has cpu_model hardcodded
> > at board level which could be left alone or converted
> > to this API.  
> 
> I think this may be for the benefit of the not-yet-in-tree
> netduinoplus2 board, which is a very similar SoC but
> with a cortex-m4. There are likely other ways we could
> model that, though.
> 
> > Using API would make code more consistent
> > at the cost of more code for default/valid callbacks.  
> 
> Mmm, we could have the board code just forward the
> default/valid callbacks through to the SoC (which
> would then either implement them or forward them again).
> But I think we should definitely see if we can make
> the code for handling "the CPU is actually inside some
> other object" as clean as we can, because I think
> that's by far the most common case. Machines like
> the x86 pc and the ARM virt board where the CPU is
> instantiated directly in the board code are the
> non-standard cases I think. We have a lot of boards
> where we do instantiate the CPU in the board code
> just for legacy reasons where we don't properly
> model the SoC the CPU is inside. But hardware that
> really has the CPU as a top level pluggable object
> is rare.

If we are to ditch legacy approach,
It would be cleaner for SoC to have fixed/unconfigurable
CPU type and each SoC being modeled as a separate
QOM object/type that would instantiate CPU directly
with QOM style, using type name, like:

    cpu = object_new(TYPE_FOO_CPU)
    set props if necessary               
    object_property_set_bool(cpu, true, "realized", &err)
    object_unref(cpu)

or similar with extra check/logic on top of plain object_new()

    prepare cpu_opts for cpu type TYPE_FOO_CPU
    cpu = qdev_device_add(cpu_opts)
    object_unref(cpu)

instead of using cpu_init/cpu_foo_init/cpu_generic_init()
with cpu_model parsing logic which is not really needed
there as concrete SoC model knows exact CPU type.

It should work for CPUs that are converted to QOM types,
i.e. a cpu_model maps to a distinct QOM type but we
still have legacy cpu_model handling where we have
not completely QOMifed single base CPU types, ex.

  cpu_mips_init(const char *cpu_model) 
      cpudef = get_feature_set_by_name(cpu+model)
      cpu = object_new(TYPE_MIPS_CPU)
      cpu->cpudef = cdef

which rely on cpu_model parsing like x86 used to be
before conversion from a single CPU type to a set
of concrete classes. QOMification of remaining
CPUs that rely on legacy cpu_model could be idea
for gsoc/outreachy project so students could learn
about QEMU internals (CCing Stefan). 

> 
> thanks
> -- PMM
> 

Re: [Qemu-devel] [RFC 0/3] generalize parsing of cpu_model

[Peter Maydell]

On 21 February 2017 at 12:44, Igor Mammedov <imammedo@redhat.com> wrote:
> If we are to ditch legacy approach,
> It would be cleaner for SoC to have fixed/unconfigurable
> CPU type and each SoC being modeled as a separate
> QOM object/type that would instantiate CPU directly
> with QOM style, using type name, like:
>
>     cpu = object_new(TYPE_FOO_CPU)
>     set props if necessary
>     object_property_set_bool(cpu, true, "realized", &err)
>     object_unref(cpu)
>
> or similar with extra check/logic on top of plain object_new()
>
>     prepare cpu_opts for cpu type TYPE_FOO_CPU
>     cpu = qdev_device_add(cpu_opts)
>     object_unref(cpu)
>
> instead of using cpu_init/cpu_foo_init/cpu_generic_init()
> with cpu_model parsing logic which is not really needed
> there as concrete SoC model knows exact CPU type.

Mmm, I think that's probably best. Would the end-user
still be able to set CPU properties if necessary via
a -global command line switch (since -cpu type,foo=on
wouldn't work)?

(I just worry slightly that we might have users doing
weird things which we'd break.)

thanks
-- PMM

Re: [Qemu-devel] [RFC 0/3] generalize parsing of cpu_model

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 21 February 2017 at 12:44, Igor Mammedov <imammedo@redhat.com> wrote:
>> If we are to ditch legacy approach,
>> It would be cleaner for SoC to have fixed/unconfigurable
>> CPU type and each SoC being modeled as a separate
>> QOM object/type that would instantiate CPU directly
>> with QOM style, using type name, like:
>>
>>     cpu = object_new(TYPE_FOO_CPU)
>>     set props if necessary
>>     object_property_set_bool(cpu, true, "realized", &err)
>>     object_unref(cpu)
>>
>> or similar with extra check/logic on top of plain object_new()
>>
>>     prepare cpu_opts for cpu type TYPE_FOO_CPU
>>     cpu = qdev_device_add(cpu_opts)
>>     object_unref(cpu)
>>
>> instead of using cpu_init/cpu_foo_init/cpu_generic_init()
>> with cpu_model parsing logic which is not really needed
>> there as concrete SoC model knows exact CPU type.
>
> Mmm, I think that's probably best. Would the end-user
> still be able to set CPU properties if necessary via
> a -global command line switch (since -cpu type,foo=on
> wouldn't work)?

-global is a qdev thing.  It affects only instances of TYPE_DEVICE and
its subtypes.  Since TYPE_CPU is one...

> (I just worry slightly that we might have users doing
> weird things which we'd break.)

I suspect we'd hand out a foot or two of extra rope, in addition to the
miles our users already got.

Re: [Qemu-devel] [RFC 0/3] generalize parsing of cpu_model

[Peter Maydell]

On 21 February 2017 at 13:56, Markus Armbruster <armbru@redhat.com> wrote:
> -global is a qdev thing.

...which reminds me to ask: is there any hope for unifying
our properties so we don't have both "qdev properties" and
"qom properties" or are we doomed to two distinct sets of
APIs forever?

thanks
-- PMM

Re: [Qemu-devel] [RFC 0/3] generalize parsing of cpu_model

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 21 February 2017 at 13:56, Markus Armbruster <armbru@redhat.com> wrote:
>> -global is a qdev thing.
>
> ...which reminds me to ask: is there any hope for unifying
> our properties so we don't have both "qdev properties" and
> "qom properties" or are we doomed to two distinct sets of
> APIs forever?

Good question!  Paolo, Andreas, any ideas?

Re: [Qemu-devel] [RFC 0/3] generalize parsing of cpu_model

[Eduardo Habkost]

On Tue, Feb 21, 2017 at 04:48:49PM +0100, Markus Armbruster wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
> > On 21 February 2017 at 13:56, Markus Armbruster <armbru@redhat.com> wrote:
> >> -global is a qdev thing.
> >
> > ...which reminds me to ask: is there any hope for unifying
> > our properties so we don't have both "qdev properties" and
> > "qom properties" or are we doomed to two distinct sets of
> > APIs forever?
> 
> Good question!  Paolo, Andreas, any ideas?

qdev properties are supposed to be just wrappers to easily add
QOM properties.

...in theory.

The current obstacles I see are:
1) the qdev static property macros (DEFINE_PROP_*) are very
   simple to use, and QOM property registration requires lots of
   boilerplate code;
2) The set of data types supported by the qdev macros doesn't
   seem to be a subset of the data types supported by QOM
   helpers.
3) qdev properties have the extra check for dev->realized on the
   setter functions.

If we make QOM properties as powerful and easy to use as the qdev
property system, replacing qdev properties with equivalent QOM
counterparts would be straightforward. The exact path we should
follow to do that is not clear to me, but cleanups going in that
direction would be welcome.

Some things I have considered, which may or may not be good
ideas:
* Getting rid of PropertyInfo structs that can't be represented
  by QAPI types (e.g. qdev_prop_ptr).
* Generating PropertyInfo structs for primitive types using QAPI.
* Providing DEFINE_PROP_* macros for QOM, and make the qdev ones
  just wrappers around the QOM macros.

-- 
Eduardo

Re: [Qemu-devel] [RFC 0/3] generalize parsing of cpu_model

[Paolo Bonzini]

On 21/02/2017 14:57, Peter Maydell wrote:
>> -global is a qdev thing.
> ...which reminds me to ask: is there any hope for unifying
> our properties so we don't have both "qdev properties" and
> "qom properties" or are we doomed to two distinct sets of
> APIs forever?

We already have one set of property APIs in some sense.  qdev properties
*are* QOM properties; you visit them with object_property_foreach and so on.

The extensions that qdev adds, and are exclusive to qdev properties, are
the following:

- defining properties via DEFINE_PROPERTY_* (qdev_property_add_static etc.)

- defining default values with -global (global_props and a handful of
functions that use it).

- providing alternative human-readable formatting for info qtree
(qdev_property_add_legacy)

Paolo

Re: [Qemu-devel] [RFC 0/3] generalize parsing of cpu_model

[Peter Maydell]

On 21 February 2017 at 16:18, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 21/02/2017 14:57, Peter Maydell wrote:
>>> -global is a qdev thing.
>> ...which reminds me to ask: is there any hope for unifying
>> our properties so we don't have both "qdev properties" and
>> "qom properties" or are we doomed to two distinct sets of
>> APIs forever?
>
> We already have one set of property APIs in some sense.  qdev properties
> *are* QOM properties; you visit them with object_property_foreach and so on.
>
> The extensions that qdev adds, and are exclusive to qdev properties, are
> the following:
>
> - defining properties via DEFINE_PROPERTY_* (qdev_property_add_static etc.)
>
> - defining default values with -global (global_props and a handful of
> functions that use it).

...I didn't realize -global only worked on qdev properties.
That's kind of awkward (but then -global syntax is awkward
in any case). What's the QOM equivalent?

> - providing alternative human-readable formatting for info qtree
> (qdev_property_add_legacy)

There's also a distinct set of functions for setting them:
qdev_prop_set_uint32() vs object_property_set_int(), etc.
These seem to be sometimes just wrappers (eg set_uint32), sometimes
wrappers with a little convenience functionality (eg set_macaddr),
and sometimes doing something more complex (eg set_enum, set_ptr).

The effect at the board code level is that code for creating,
setting properties on and realizing qdev devices tends to look
very different from that for doing so for plain QOM objects
[qdev_create / qdev_prop_set* / qdev_init_nofail vs object_new /
object_property_set_* / object_property_set_bool(... "realized" ...) ]

Can/should we just deprecate further use of the qdev_* APIs?

thanks
-- PMM

Re: [Qemu-devel] [RFC 0/3] generalize parsing of cpu_model

[Eduardo Habkost]

On Tue, Feb 21, 2017 at 05:59:53PM +0000, Peter Maydell wrote:
> On 21 February 2017 at 16:18, Paolo Bonzini <pbonzini@redhat.com> wrote:
> > On 21/02/2017 14:57, Peter Maydell wrote:
> >>> -global is a qdev thing.
> >> ...which reminds me to ask: is there any hope for unifying
> >> our properties so we don't have both "qdev properties" and
> >> "qom properties" or are we doomed to two distinct sets of
> >> APIs forever?
> >
> > We already have one set of property APIs in some sense.  qdev properties
> > *are* QOM properties; you visit them with object_property_foreach and so on.
> >
> > The extensions that qdev adds, and are exclusive to qdev properties, are
> > the following:
> >
> > - defining properties via DEFINE_PROPERTY_* (qdev_property_add_static etc.)
> >
> > - defining default values with -global (global_props and a handful of
> > functions that use it).
> 
> ...I didn't realize -global only worked on qdev properties.
> That's kind of awkward (but then -global syntax is awkward
> in any case). What's the QOM equivalent?

-global work on any kind of QOM properties (qdev properties are
registered as QOM properties). The only restriction is that it
works only on TYPE_DEVICE subclasses.

> 
> > - providing alternative human-readable formatting for info qtree
> > (qdev_property_add_legacy)
> 
> There's also a distinct set of functions for setting them:
> qdev_prop_set_uint32() vs object_property_set_int(), etc.
> These seem to be sometimes just wrappers (eg set_uint32), sometimes
> wrappers with a little convenience functionality (eg set_macaddr),
> and sometimes doing something more complex (eg set_enum, set_ptr).
> 
> The effect at the board code level is that code for creating,
> setting properties on and realizing qdev devices tends to look
> very different from that for doing so for plain QOM objects
> [qdev_create / qdev_prop_set* / qdev_init_nofail vs object_new /
> object_property_set_* / object_property_set_bool(... "realized" ...) ]
> 
> Can/should we just deprecate further use of the qdev_* APIs?

I don't think we can, until we provide a QOM API that is as
powerful and as easy to use as the DEFINE_PROP_* macros.

This probably isn't as hard as it sounds: I believe the only
thing forcing the existing DEFINE_PROP_* macros and PropertyInfo
structs to be qdev-specific is the device->realized check inside
the property setters.

-- 
Eduardo

Re: [Qemu-devel] [RFC 0/3] generalize parsing of cpu_model

[Eduardo Habkost]

On Fri, Feb 17, 2017 at 07:56:32PM +0100, Igor Mammedov wrote:
> 
> Some callers call CPUClass->parse_features manually to convert
> '-cpu cpufoo,featurestr' string to cpu type and featurestr
> into a set of global properties. And theni do controlled
> cpu creation with setting properties and completing it with realize.
> That's a lot of code duplication as they are practically
> reimplement the same parsing logic.
> 
> Some don't and use cpu_generic_init() instead which does
> the same parsing along with creation/realizing cpu within one
> wrapper.
> 
> And some trying to switch to controlled cpu creation,
> implement object_new()/set properties/realize steps
> but forget feature parsing logic witch lieads to 'bugs'
> commit 00909b585 (hw/arm/integratorcp: Support specifying features via -cpu)
> 
> 
> This series moves -cpu option parsing to generic machine code
> that removes a little of code duplication and makes cpus creation
> process more unified.
> 
> PS:
> As I don't have time to rewrite this part QEMU, being busy
> rewritting yet another part of QEMU,
> SERIES IS UNFINISHED AND SERVERS TO SHOW IDEA HOW IT SHOULD
> BE DONE. FEEL FREE TO PICK UP AND COMPLETE THIS PATCHES
> TO HANDLE ALL BOARDS (series does it only for virt-arm/spapr/pc)
> 
> It compiles and pc machine even starts but otherwise is untested.

The approach makes sense to me, but I haven't reviewed all the
code yet.

That said, I don't think this excludes the possiblity of adding a
temporary cpu_generic_new() wrapper in case it is useful in the
meantime. After this series is applied, cpu_generic_new() could
be automatically replaced by object_new(machine->cpu_typename).

Peter, do you think a wrapper for parse_features + object_new()
would still be necessary to avoid too much code duplication on
arm boards in 2.9, or can we live without it until we apply
Igor's series (probaly after 2.9)?

-- 
Eduardo

Re: [Qemu-devel] [RFC 0/3] generalize parsing of cpu_model

[Peter Maydell]

On 21 February 2017 at 18:28, Eduardo Habkost <ehabkost@redhat.com> wrote:
> Peter, do you think a wrapper for parse_features + object_new()
> would still be necessary to avoid too much code duplication on
> arm boards in 2.9, or can we live without it until we apply
> Igor's series (probaly after 2.9)?

2.9 is very close now. I don't think we need to change any
of the existing board code. Though there are quite a lot that
don't accept -cpu foo,feat=on syntax because they use
object_new() to create the CPU but don't call parse_features,
they've been that way for years (eg since commit 223a72f1179dc0b
from 2014 for versatilepb). We should figure out how we want
to handle this and change all the boards just once, for 2.10.

thanks
-- PMM

Re: [Qemu-devel] [RFC 0/3] generalize parsing of cpu_model

[Igor Mammedov]

On Fri, 17 Feb 2017 19:56:32 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> Some callers call CPUClass->parse_features manually to convert
> '-cpu cpufoo,featurestr' string to cpu type and featurestr
> into a set of global properties. And theni do controlled
> cpu creation with setting properties and completing it with realize.
> That's a lot of code duplication as they are practically
> reimplement the same parsing logic.
> 
> Some don't and use cpu_generic_init() instead which does
> the same parsing along with creation/realizing cpu within one
> wrapper.
> 
> And some trying to switch to controlled cpu creation,
> implement object_new()/set properties/realize steps
> but forget feature parsing logic witch lieads to 'bugs'
> commit 00909b585 (hw/arm/integratorcp: Support specifying features via -cpu)
> 
> 
> This series moves -cpu option parsing to generic machine code
> that removes a little of code duplication and makes cpus creation
> process more unified.
> 
> PS:
> As I don't have time to rewrite this part QEMU, being busy
> rewritting yet another part of QEMU,
> SERIES IS UNFINISHED AND SERVERS TO SHOW IDEA HOW IT SHOULD
> BE DONE. FEEL FREE TO PICK UP AND COMPLETE THIS PATCHES
> TO HANDLE ALL BOARDS (series does it only for virt-arm/spapr/pc)

series has been written on top my numa tree which apply it to
upstream master. So here goes rebased on top of current master variant:

https://github.com/imammedo/qemu/commits/generilize_cpu_model_parsing_rfc