Re: [Qemu-devel] [PATCH v4] cpu: add suboptions support

[Alexey Kardashevskiy <aik@ozlabs.ru>]

On 12/03/2013 10:09 PM, Igor Mammedov wrote:
> On Tue,  3 Dec 2013 14:42:48 +1100
> Alexey Kardashevskiy <aik@ozlabs.ru> wrote:
> 
>> This adds suboptions support for -cpu. This keeps @cpu_model in order not
>> to break the existing architectures/machines.
>>
>> Cc: Andreas Färber <afaerber@suse.de>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> Changes:
>> v4:
>> * moved QemuOpts logic to qeom/cpu.c
>> * added cpu_opt_get() as the machine init code wants to know the CPU name
>> to create a CPU object
>> ---
>>  include/qom/cpu.h | 41 +++++++++++++++++++++++++++++++++++++++++
>>  qom/cpu.c         | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  vl.c              | 23 +++++++++++++++++++++++
>>  3 files changed, 113 insertions(+)
>>
>> diff --git a/include/qom/cpu.h b/include/qom/cpu.h
>> index 7739e00..07330e1 100644
>> --- a/include/qom/cpu.h
>> +++ b/include/qom/cpu.h
>> @@ -124,6 +124,7 @@ typedef struct CPUClass {
>>                              int cpuid, void *opaque);
>>      int (*write_elf32_qemunote)(WriteCoreDumpFunction f, CPUState *cpu,
>>                                  void *opaque);
>> +    void (*parse_options)(CPUState *cpu, Error **errp);
>>  
>>      const struct VMStateDescription *vmsd;
>>      int gdb_num_core_regs;
>> @@ -327,6 +328,46 @@ static inline hwaddr cpu_get_phys_page_debug(CPUState *cpu, vaddr addr)
>>  #endif
>>  
>>  /**
>> + * cpu_parse_options:
>> + * @cpu: The CPU to set options for.
>> + *
>> + * Parses CPU options if the CPU class has a custom handler defined.
>> + *
>> + * Returns: -1 if a custom handler is defined and failed, 0 otherwise.
>> + */
>> +static inline int cpu_parse_options(CPUState *cpu)
> why not pass up the stack errp, vs returning -1?


Oh. Overlooked it. Not sure if we really need Error** here, do we?

object_property_parse() will print an error anyway and QEMU then exits and
qemu_opt_foreach() cannot carry Error** to the caller so I'll better drop
it on this level.


> And of cause, there should be a patch that demonstrates an actual user
> of  cpu_parse_options().
> 
>> +{
>> +    CPUClass *cc = CPU_GET_CLASS(cpu);
>> +    Error *err = NULL;
>> +
>> +    if (cc->parse_options) {
>> +        cc->parse_options(cpu, &err);
>> +        if (err) {
>> +            return -1;
>> +        }
>> +    }
>> +
>> +    /* No callback, let arch do it the old way */
>> +    return 0;
>> +}
>> +
>> +/**
>> + * cpu_default_parse_options_func:
>> + * The default handler for CPUClass::parse_options
>> + * @cpu: the CPU to set option for.
>> + * @errp: the handling error descriptor.
>> + */
>> +void cpu_default_parse_options_func(CPUState *cpu, Error **errp);
> where is it used?
> Maybe it should be static inside of qom/cpu.c
> and assigned in cpu_class_init() for TYPE_CPU.


It is assigned for TYPE_POWERPC_CPU as I really do not want to switch every
single CPU in QEMU to use this so this is why it is not static.

Below is how I use it for POWERPC. Next time I'll post these patches
together, sorry for inconvenience.



diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index d93abdc..7e0fb68 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -33,6 +33,7 @@
 #include "sysemu/kvm.h"
 #include "kvm_ppc.h"
 #include "mmu-hash64.h"
+#include "qom/cpu.h"

 #include "hw/boards.h"
 #include "hw/ppc/ppc.h"
@@ -1111,7 +1112,7 @@ static SaveVMHandlers savevm_htab_handlers = {
 static void ppc_spapr_init(QEMUMachineInitArgs *args)
 {
     ram_addr_t ram_size = args->ram_size;
-    const char *cpu_model = args->cpu_model;
+    const char *cpu_model;
     const char *kernel_filename = args->kernel_filename;
     const char *kernel_cmdline = args->kernel_cmdline;
     const char *initrd_filename = args->initrd_filename;
@@ -1131,6 +1132,8 @@ static void ppc_spapr_init(QEMUMachineInitArgs *args)

     msi_supported = true;

+    cpu_model = cpu_opt_get("type");
+
     spapr = g_malloc0(sizeof(*spapr));
     QLIST_INIT(&spapr->phbs);

diff --git a/target-ppc/translate_init.c b/target-ppc/translate_init.c
index 1b0ff38..d24920b 100644
--- a/target-ppc/translate_init.c
+++ b/target-ppc/translate_init.c
@@ -7381,6 +7381,10 @@ static void init_ppc_proc(PowerPCCPU *cpu)
     /* PowerPC implementation specific initialisations (SPRs, timers, ...) */
     (*pcc->init_proc)(env);

+    if (cpu_parse_options(CPU(cpu))) {
+        exit(1);
+    }
+
     /* MSR bits & flags consistency checks */
     if (env->msr_mask & (1 << 25)) {
         switch (env->flags & (POWERPC_FLAG_SPE | POWERPC_FLAG_VRE)) {
@@ -8676,6 +8680,7 @@ static void ppc_cpu_class_init(ObjectClass *oc, void
*data)
 #endif

     dc->fw_name = "PowerPC,UNKNOWN";
+    cc->parse_options = cpu_default_parse_options_func;
 }

 static const TypeInfo ppc_cpu_type_info = {



-- 
Alexey