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

[Alexey Kardashevskiy]

On 01/22/2014 11:59 AM, Alexey Kardashevskiy wrote:
> On 12/20/2013 10:36 AM, Alexey Kardashevskiy wrote:
>> On 12/04/2013 01:09 PM, Alexey Kardashevskiy wrote:
>>> On 12/03/2013 10:09 PM, Igor Mammedov wrote:
>>>> On Tue,  3 Dec 2013 14:42:48 +1100
>>>> Alexey Kardashevskiy <address@hidden> wrote:
>>>>
>>>>> This adds suboptions support for -cpu. This keeps @cpu_model in order not
>>>>> to break the existing architectures/machines.
>>>>>
>>>>> Cc: Andreas Färber <address@hidden>
>>>>> Signed-off-by: Alexey Kardashevskiy <address@hidden>
>>
>>
>> Ping, anyone?
>> Patch is too bad? Any new idea how to rework it? Someone else is coming
>> with a new patch?
> 
> 
> Andreas, is that what you asked me to ping you about? :)
> 
> I guess it is outdated and I have to rework it, I just did not have chance
> yet...

Makes sense to repost it against current upstream or not yet?


> 
> 
>>
>> Thanks
>>
>>
>>>>> ---
>>>>> 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