| Message ID | 529E8ECC.7000403@ozlabs.ru |
|---|---|
| State | New |
| Headers | show |
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 <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> Ping, anyone? Patch is too bad? Any new idea how to rework it? Someone else is coming with a new patch? 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 = { > > >
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 <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> > > > 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... > > 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 = { >> >> >> > >
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 <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> >> >> >> 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 = { >>> >>> >>> >> >> > >
Am 20.02.2014 14:24, schrieb 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 <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> >>> >>> >>> 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? No, I just mentioned on IRC today that I have it on my TODO list after X86CPU subclasses, to rework in a linux-user compatible way (which does not use QemuOpts but an environment variable). Sorry for the delays. Andreas
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; }