| Message ID | 1449728144-6223-3-git-send-email-bharata@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Dec 10, 2015 at 11:45:37AM +0530, Bharata B Rao wrote: > Storing CPU typename in MachineState lets us to create CPU threads > for all architectures in uniform manner from arch-neutral code. > > TODO: Touching only i386 and spapr targets for now > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> Suggestions: * Name the field "cpu_base_type" to indicate it is the base CPU class name, not the actual CPU class name used when creating CPUs. * Put it in MachineClass, as it may be useful for code that runs before machine->init(), in the future. * Maybe make it a CPUClass* field instead of a string? > --- > hw/i386/pc.c | 1 + > hw/ppc/spapr.c | 2 ++ > include/hw/boards.h | 1 + > 3 files changed, 4 insertions(+) > > diff --git a/hw/i386/pc.c b/hw/i386/pc.c > index 5e20e07..ffcd645 100644 > --- a/hw/i386/pc.c > +++ b/hw/i386/pc.c > @@ -1133,6 +1133,7 @@ void pc_cpus_init(PCMachineState *pcms) > machine->cpu_model = "qemu32"; > #endif > } > + machine->cpu_type = TYPE_X86_CPU; > > apic_id_limit = pc_apic_id_limit(max_cpus); > if (apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT) { > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c > index 030ee35..db441f2 100644 > --- a/hw/ppc/spapr.c > +++ b/hw/ppc/spapr.c > @@ -1797,6 +1797,8 @@ static void ppc_spapr_init(MachineState *machine) > if (machine->cpu_model == NULL) { > machine->cpu_model = kvm_enabled() ? "host" : "POWER7"; > } > + machine->cpu_type = TYPE_POWERPC_CPU; > + > for (i = 0; i < smp_cpus; i++) { > cpu = cpu_ppc_init(machine->cpu_model); > if (cpu == NULL) { > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 24eb6f0..a1f9512 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -128,6 +128,7 @@ struct MachineState { > char *kernel_cmdline; > char *initrd_filename; > const char *cpu_model; > + const char *cpu_type; > AccelState *accelerator; > }; > > -- > 2.1.0 >
On Mon, Dec 14, 2015 at 03:29:49PM -0200, Eduardo Habkost wrote: > On Thu, Dec 10, 2015 at 11:45:37AM +0530, Bharata B Rao wrote: > > Storing CPU typename in MachineState lets us to create CPU threads > > for all architectures in uniform manner from arch-neutral code. > > > > TODO: Touching only i386 and spapr targets for now > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > Suggestions: > > * Name the field "cpu_base_type" to indicate it is the base CPU > class name, not the actual CPU class name used when creating > CPUs. > * Put it in MachineClass, as it may be useful for code that > runs before machine->init(), in the future. Ok. > * Maybe make it a CPUClass* field instead of a string? In the current use case, this base cpu type string is being passed to cpu_generic_init(const char *typename, const char *cpu_model) to create boot time CPUs with given typename and cpu_mode. So for now the string makes sense for use case. Making it CPUClass* would necessiate more changes to cpu_generic_init(). Regards, Bharata.
On Tue, Dec 15, 2015 at 02:08:09PM +0530, Bharata B Rao wrote: > On Mon, Dec 14, 2015 at 03:29:49PM -0200, Eduardo Habkost wrote: [...] > > * Maybe make it a CPUClass* field instead of a string? > > In the current use case, this base cpu type string is being passed > to cpu_generic_init(const char *typename, const char *cpu_model) > to create boot time CPUs with given typename and cpu_mode. So for now > the string makes sense for use case. > > Making it CPUClass* would necessiate more changes to cpu_generic_init(). I would consider changing cpu_generic_init() to cpu_generic_init(CPUClass *base_class, const char *cpu_model) too. But I admit I didn't look at cpu_generic_init() code yet, and this can be done later if appropriate.
On Tue, 15 Dec 2015 14:08:09 +0530 Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > On Mon, Dec 14, 2015 at 03:29:49PM -0200, Eduardo Habkost wrote: > > On Thu, Dec 10, 2015 at 11:45:37AM +0530, Bharata B Rao wrote: > > > Storing CPU typename in MachineState lets us to create CPU threads > > > for all architectures in uniform manner from arch-neutral code. > > > > > > TODO: Touching only i386 and spapr targets for now > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > Suggestions: > > > > * Name the field "cpu_base_type" to indicate it is the base CPU > > class name, not the actual CPU class name used when creating > > CPUs. > > * Put it in MachineClass, as it may be useful for code that > > runs before machine->init(), in the future. > > Ok. > > > * Maybe make it a CPUClass* field instead of a string? > > In the current use case, this base cpu type string is being passed > to cpu_generic_init(const char *typename, const char *cpu_model) > to create boot time CPUs with given typename and cpu_mode. So for now > the string makes sense for use case. > > Making it CPUClass* would necessiate more changes to > cpu_generic_init(). how about actually leaving it as "cpu_type" and putting in it actual cpu type that could be used with device_add(). that would get rid of keeping and passing around intermediate cpu_model. > > Regards, > Bharata. > >
On Wed, Dec 16, 2015 at 05:54:25PM +0100, Igor Mammedov wrote: > On Tue, 15 Dec 2015 14:08:09 +0530 > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > On Mon, Dec 14, 2015 at 03:29:49PM -0200, Eduardo Habkost wrote: > > > On Thu, Dec 10, 2015 at 11:45:37AM +0530, Bharata B Rao wrote: > > > > Storing CPU typename in MachineState lets us to create CPU threads > > > > for all architectures in uniform manner from arch-neutral code. > > > > > > > > TODO: Touching only i386 and spapr targets for now > > > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > > > Suggestions: > > > > > > * Name the field "cpu_base_type" to indicate it is the base CPU > > > class name, not the actual CPU class name used when creating > > > CPUs. > > > * Put it in MachineClass, as it may be useful for code that > > > runs before machine->init(), in the future. > > > > Ok. > > > > > * Maybe make it a CPUClass* field instead of a string? > > > > In the current use case, this base cpu type string is being passed > > to cpu_generic_init(const char *typename, const char *cpu_model) > > to create boot time CPUs with given typename and cpu_mode. So for now > > the string makes sense for use case. > > > > Making it CPUClass* would necessiate more changes to > > cpu_generic_init(). > how about actually leaving it as "cpu_type" and putting in it > actual cpu type that could be used with device_add(). > > that would get rid of keeping and passing around intermediate cpu_model. Makes sense. We only need to save both typename and cpu_model today because cpu_generic_init() currently encapsulates three steps: CPU class lookup + CPU creation + CPU feature parsing. But we shouldn't need to redo CPU class lookup every time. We could just split cpu_model once, and save the resulting CPUClass* + featurestr, instead of saving the full cpu_model string and parsing it again every time. The only problem is that it would require refactoring multiple machines/architectures that use a cpu_XXX_init(const char *cpu_model) helper.
On Wed, 16 Dec 2015 17:39:02 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Dec 16, 2015 at 05:54:25PM +0100, Igor Mammedov wrote: > > On Tue, 15 Dec 2015 14:08:09 +0530 > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > > > On Mon, Dec 14, 2015 at 03:29:49PM -0200, Eduardo Habkost wrote: > > > > On Thu, Dec 10, 2015 at 11:45:37AM +0530, Bharata B Rao wrote: > > > > > Storing CPU typename in MachineState lets us to create CPU > > > > > threads for all architectures in uniform manner from > > > > > arch-neutral code. > > > > > > > > > > TODO: Touching only i386 and spapr targets for now > > > > > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > > > > > Suggestions: > > > > > > > > * Name the field "cpu_base_type" to indicate it is the base CPU > > > > class name, not the actual CPU class name used when creating > > > > CPUs. > > > > * Put it in MachineClass, as it may be useful for code that > > > > runs before machine->init(), in the future. > > > > > > Ok. > > > > > > > * Maybe make it a CPUClass* field instead of a string? > > > > > > In the current use case, this base cpu type string is being passed > > > to cpu_generic_init(const char *typename, const char *cpu_model) > > > to create boot time CPUs with given typename and cpu_mode. So for > > > now the string makes sense for use case. > > > > > > Making it CPUClass* would necessiate more changes to > > > cpu_generic_init(). > > how about actually leaving it as "cpu_type" and putting in it > > actual cpu type that could be used with device_add(). > > > > that would get rid of keeping and passing around intermediate > > cpu_model. > > Makes sense. We only need to save both typename and cpu_model > today because cpu_generic_init() currently encapsulates three > steps: CPU class lookup + CPU creation + CPU feature parsing. But > we shouldn't need to redo CPU class lookup every time. BTW: Eduardo do you know if QEMU could somehow provide a list of supported CPU types (i.e. not cpumodels) to libvirt? > > We could just split cpu_model once, and save the resulting > CPUClass* + featurestr, instead of saving the full cpu_model > string and parsing it again every time. isn't featurestr as x86/sparc specific? Could we have field in x86_cpu_class/sparc_cpu_class for it and set it when cpu_model is parsed? That way generic cpu_model parser would handle only cpu names and target specific overrides would handle both. > > The only problem is that it would require refactoring multiple > machines/architectures that use a cpu_XXX_init(const char *cpu_model) > helper. >
On Wed, Dec 16, 2015 at 11:26:20PM +0100, Igor Mammedov wrote: > On Wed, 16 Dec 2015 17:39:02 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Wed, Dec 16, 2015 at 05:54:25PM +0100, Igor Mammedov wrote: > > > On Tue, 15 Dec 2015 14:08:09 +0530 > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > > > > > On Mon, Dec 14, 2015 at 03:29:49PM -0200, Eduardo Habkost wrote: > > > > > On Thu, Dec 10, 2015 at 11:45:37AM +0530, Bharata B Rao wrote: > > > > > > Storing CPU typename in MachineState lets us to create CPU > > > > > > threads for all architectures in uniform manner from > > > > > > arch-neutral code. > > > > > > > > > > > > TODO: Touching only i386 and spapr targets for now > > > > > > > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > > > > > > > Suggestions: > > > > > > > > > > * Name the field "cpu_base_type" to indicate it is the base CPU > > > > > class name, not the actual CPU class name used when creating > > > > > CPUs. > > > > > * Put it in MachineClass, as it may be useful for code that > > > > > runs before machine->init(), in the future. > > > > > > > > Ok. > > > > > > > > > * Maybe make it a CPUClass* field instead of a string? > > > > > > > > In the current use case, this base cpu type string is being passed > > > > to cpu_generic_init(const char *typename, const char *cpu_model) > > > > to create boot time CPUs with given typename and cpu_mode. So for > > > > now the string makes sense for use case. > > > > > > > > Making it CPUClass* would necessiate more changes to > > > > cpu_generic_init(). > > > how about actually leaving it as "cpu_type" and putting in it > > > actual cpu type that could be used with device_add(). > > > > > > that would get rid of keeping and passing around intermediate > > > cpu_model. > > > > Makes sense. We only need to save both typename and cpu_model > > today because cpu_generic_init() currently encapsulates three > > steps: CPU class lookup + CPU creation + CPU feature parsing. But > > we shouldn't need to redo CPU class lookup every time. > BTW: Eduardo do you know if QEMU could somehow provide a list of > supported CPU types (i.e. not cpumodels) to libvirt? Not sure I understand the question. Could you clarify what you mean by "supported CPU types", and what's the problem it would solve? > > > > > We could just split cpu_model once, and save the resulting > > CPUClass* + featurestr, instead of saving the full cpu_model > > string and parsing it again every time. > isn't featurestr as x86/sparc specific? > > Could we have field in x86_cpu_class/sparc_cpu_class for it and set it > when cpu_model is parsed? > That way generic cpu_model parser would handle only cpu names and > target specific overrides would handle both. I always assumed we want to have a generic CPU model + featurestr mechanism that could be reused by multiple architectures.
On Thu, 17 Dec 2015 16:09:23 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Wed, Dec 16, 2015 at 11:26:20PM +0100, Igor Mammedov wrote: > > On Wed, 16 Dec 2015 17:39:02 -0200 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Wed, Dec 16, 2015 at 05:54:25PM +0100, Igor Mammedov wrote: > > > > On Tue, 15 Dec 2015 14:08:09 +0530 > > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > > > > > > > On Mon, Dec 14, 2015 at 03:29:49PM -0200, Eduardo Habkost wrote: > > > > > > On Thu, Dec 10, 2015 at 11:45:37AM +0530, Bharata B Rao wrote: > > > > > > > Storing CPU typename in MachineState lets us to create CPU > > > > > > > threads for all architectures in uniform manner from > > > > > > > arch-neutral code. > > > > > > > > > > > > > > TODO: Touching only i386 and spapr targets for now > > > > > > > > > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > > > > > > > > > Suggestions: > > > > > > > > > > > > * Name the field "cpu_base_type" to indicate it is the base CPU > > > > > > class name, not the actual CPU class name used when creating > > > > > > CPUs. > > > > > > * Put it in MachineClass, as it may be useful for code that > > > > > > runs before machine->init(), in the future. > > > > > > > > > > Ok. > > > > > > > > > > > * Maybe make it a CPUClass* field instead of a string? > > > > > > > > > > In the current use case, this base cpu type string is being passed > > > > > to cpu_generic_init(const char *typename, const char *cpu_model) > > > > > to create boot time CPUs with given typename and cpu_mode. So for > > > > > now the string makes sense for use case. > > > > > > > > > > Making it CPUClass* would necessiate more changes to > > > > > cpu_generic_init(). > > > > how about actually leaving it as "cpu_type" and putting in it > > > > actual cpu type that could be used with device_add(). > > > > > > > > that would get rid of keeping and passing around intermediate > > > > cpu_model. > > > > > > Makes sense. We only need to save both typename and cpu_model > > > today because cpu_generic_init() currently encapsulates three > > > steps: CPU class lookup + CPU creation + CPU feature parsing. But > > > we shouldn't need to redo CPU class lookup every time. > > BTW: Eduardo do you know if QEMU could somehow provide a list of > > supported CPU types (i.e. not cpumodels) to libvirt? > > Not sure I understand the question. Could you clarify what you > mean by "supported CPU types", and what's the problem it would > solve? device_add TYPE, takes only type name so I'd like to kep it that way and make sure that libvirt/user can list cpu types that hi would be able to use with device_add/-device. for PC they are generated from cpu_model with help of x86_cpu_type_name() > > > > > > > > We could just split cpu_model once, and save the resulting > > > CPUClass* + featurestr, instead of saving the full cpu_model > > > string and parsing it again every time. > > isn't featurestr as x86/sparc specific? > > > > Could we have field in x86_cpu_class/sparc_cpu_class for it and set it > > when cpu_model is parsed? > > That way generic cpu_model parser would handle only cpu names and > > target specific overrides would handle both. > > I always assumed we want to have a generic CPU model + featurestr > mechanism that could be reused by multiple architectures. I've thought the opposite way, that we wanted to faze out featurestr in favor of generic option parsing of generic device, i.e. -device TYPE,option=X,... but we would have to keep compatibility with old CLI that supplies cpu definition via -cpu cpu_model,featurestr so cpu_model translated into "cpu_type" field make sense for every target but featurestr is x86/sparc specific and I'd prefer to keep it that way and do not introduce it to other targets.
On Fri, Dec 18, 2015 at 11:46:05AM +0100, Igor Mammedov wrote: > On Thu, 17 Dec 2015 16:09:23 -0200 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > On Wed, Dec 16, 2015 at 11:26:20PM +0100, Igor Mammedov wrote: > > > On Wed, 16 Dec 2015 17:39:02 -0200 > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > On Wed, Dec 16, 2015 at 05:54:25PM +0100, Igor Mammedov wrote: > > > > > On Tue, 15 Dec 2015 14:08:09 +0530 > > > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > > > > > > > > > On Mon, Dec 14, 2015 at 03:29:49PM -0200, Eduardo Habkost wrote: > > > > > > > On Thu, Dec 10, 2015 at 11:45:37AM +0530, Bharata B Rao wrote: > > > > > > > > Storing CPU typename in MachineState lets us to create CPU > > > > > > > > threads for all architectures in uniform manner from > > > > > > > > arch-neutral code. > > > > > > > > > > > > > > > > TODO: Touching only i386 and spapr targets for now > > > > > > > > > > > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > > > > > > > > > > > Suggestions: > > > > > > > > > > > > > > * Name the field "cpu_base_type" to indicate it is the base CPU > > > > > > > class name, not the actual CPU class name used when creating > > > > > > > CPUs. > > > > > > > * Put it in MachineClass, as it may be useful for code that > > > > > > > runs before machine->init(), in the future. > > > > > > > > > > > > Ok. > > > > > > > > > > > > > * Maybe make it a CPUClass* field instead of a string? > > > > > > > > > > > > In the current use case, this base cpu type string is being passed > > > > > > to cpu_generic_init(const char *typename, const char *cpu_model) > > > > > > to create boot time CPUs with given typename and cpu_mode. So for > > > > > > now the string makes sense for use case. > > > > > > > > > > > > Making it CPUClass* would necessiate more changes to > > > > > > cpu_generic_init(). > > > > > how about actually leaving it as "cpu_type" and putting in it > > > > > actual cpu type that could be used with device_add(). > > > > > > > > > > that would get rid of keeping and passing around intermediate > > > > > cpu_model. > > > > > > > > Makes sense. We only need to save both typename and cpu_model > > > > today because cpu_generic_init() currently encapsulates three > > > > steps: CPU class lookup + CPU creation + CPU feature parsing. But > > > > we shouldn't need to redo CPU class lookup every time. > > > BTW: Eduardo do you know if QEMU could somehow provide a list of > > > supported CPU types (i.e. not cpumodels) to libvirt? > > > > Not sure I understand the question. Could you clarify what you > > mean by "supported CPU types", and what's the problem it would > > solve? > device_add TYPE, takes only type name so I'd like to kep it that way > and make sure that libvirt/user can list cpu types that hi would > be able to use with device_add/-device. > > for PC they are generated from cpu_model with help of x86_cpu_type_name() What about adding a "qom-type" field to query-cpu-definitions? > > > > > > > > > > > > We could just split cpu_model once, and save the resulting > > > > CPUClass* + featurestr, instead of saving the full cpu_model > > > > string and parsing it again every time. > > > isn't featurestr as x86/sparc specific? > > > > > > Could we have field in x86_cpu_class/sparc_cpu_class for it and set it > > > when cpu_model is parsed? > > > That way generic cpu_model parser would handle only cpu names and > > > target specific overrides would handle both. > > > > I always assumed we want to have a generic CPU model + featurestr > > mechanism that could be reused by multiple architectures. > > I've thought the opposite way, that we wanted to faze out featurestr > in favor of generic option parsing of generic device, i.e. > -device TYPE,option=X,... > but we would have to keep compatibility with old CLI > that supplies cpu definition via -cpu cpu_model,featurestr > so cpu_model translated into "cpu_type" field make sense for every > target but featurestr is x86/sparc specific and I'd prefer to > keep it that way and do not introduce it to other targets. I see, and it may make sense long term. But do you really think we will be able to deprecate "-cpu" and "-smp" soon? We already have CPUClass::parse_features, and cpu_generic_init() already makes the model/featurestr split. Do you propose we remove that generic code and move it back to x86/sparc? Also, are you sure no other architectures support options in "-cpu"? cpu_common_parse_features() already supports setting QOM properties, and it is already available on every architecture that calls CPUClass::parse_features or uses cpu_generic_init(). I see that both ARM and PPC have properties available in their CPU classes, and call parse_features() too.
On Fri, 18 Dec 2015 13:51:49 -0200 Eduardo Habkost <ehabkost@redhat.com> wrote: > On Fri, Dec 18, 2015 at 11:46:05AM +0100, Igor Mammedov wrote: > > On Thu, 17 Dec 2015 16:09:23 -0200 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > On Wed, Dec 16, 2015 at 11:26:20PM +0100, Igor Mammedov wrote: > > > > On Wed, 16 Dec 2015 17:39:02 -0200 > > > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > > > > > On Wed, Dec 16, 2015 at 05:54:25PM +0100, Igor Mammedov wrote: > > > > > > On Tue, 15 Dec 2015 14:08:09 +0530 > > > > > > Bharata B Rao <bharata@linux.vnet.ibm.com> wrote: > > > > > > > > > > > > > On Mon, Dec 14, 2015 at 03:29:49PM -0200, Eduardo Habkost wrote: > > > > > > > > On Thu, Dec 10, 2015 at 11:45:37AM +0530, Bharata B Rao wrote: > > > > > > > > > Storing CPU typename in MachineState lets us to create CPU > > > > > > > > > threads for all architectures in uniform manner from > > > > > > > > > arch-neutral code. > > > > > > > > > > > > > > > > > > TODO: Touching only i386 and spapr targets for now > > > > > > > > > > > > > > > > > > Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> > > > > > > > > > > > > > > > > Suggestions: > > > > > > > > > > > > > > > > * Name the field "cpu_base_type" to indicate it is the base CPU > > > > > > > > class name, not the actual CPU class name used when creating > > > > > > > > CPUs. > > > > > > > > * Put it in MachineClass, as it may be useful for code that > > > > > > > > runs before machine->init(), in the future. > > > > > > > > > > > > > > Ok. > > > > > > > > > > > > > > > * Maybe make it a CPUClass* field instead of a string? > > > > > > > > > > > > > > In the current use case, this base cpu type string is being passed > > > > > > > to cpu_generic_init(const char *typename, const char *cpu_model) > > > > > > > to create boot time CPUs with given typename and cpu_mode. So for > > > > > > > now the string makes sense for use case. > > > > > > > > > > > > > > Making it CPUClass* would necessiate more changes to > > > > > > > cpu_generic_init(). > > > > > > how about actually leaving it as "cpu_type" and putting in it > > > > > > actual cpu type that could be used with device_add(). > > > > > > > > > > > > that would get rid of keeping and passing around intermediate > > > > > > cpu_model. > > > > > > > > > > Makes sense. We only need to save both typename and cpu_model > > > > > today because cpu_generic_init() currently encapsulates three > > > > > steps: CPU class lookup + CPU creation + CPU feature parsing. But > > > > > we shouldn't need to redo CPU class lookup every time. > > > > BTW: Eduardo do you know if QEMU could somehow provide a list of > > > > supported CPU types (i.e. not cpumodels) to libvirt? > > > > > > Not sure I understand the question. Could you clarify what you > > > mean by "supported CPU types", and what's the problem it would > > > solve? > > device_add TYPE, takes only type name so I'd like to kep it that way > > and make sure that libvirt/user can list cpu types that hi would > > be able to use with device_add/-device. > > > > for PC they are generated from cpu_model with help of x86_cpu_type_name() > > What about adding a "qom-type" field to query-cpu-definitions? Sounds like good idea to me. > > > > > > > > > > > > > > > > > We could just split cpu_model once, and save the resulting > > > > > CPUClass* + featurestr, instead of saving the full cpu_model > > > > > string and parsing it again every time. > > > > isn't featurestr as x86/sparc specific? > > > > > > > > Could we have field in x86_cpu_class/sparc_cpu_class for it and set it > > > > when cpu_model is parsed? > > > > That way generic cpu_model parser would handle only cpu names and > > > > target specific overrides would handle both. > > > > > > I always assumed we want to have a generic CPU model + featurestr > > > mechanism that could be reused by multiple architectures. > > > > I've thought the opposite way, that we wanted to faze out featurestr > > in favor of generic option parsing of generic device, i.e. > > -device TYPE,option=X,... > > but we would have to keep compatibility with old CLI > > that supplies cpu definition via -cpu cpu_model,featurestr > > so cpu_model translated into "cpu_type" field make sense for every > > target but featurestr is x86/sparc specific and I'd prefer to > > keep it that way and do not introduce it to other targets. > > I see, and it may make sense long term. But do you really think > we will be able to deprecate "-cpu" and "-smp" soon? > > We already have CPUClass::parse_features, and cpu_generic_init() > already makes the model/featurestr split. Do you propose we > remove that generic code and move it back to x86/sparc? > > Also, are you sure no other architectures support options in > "-cpu"? cpu_common_parse_features() already supports setting QOM > properties, and it is already available on every architecture > that calls CPUClass::parse_features or uses cpu_generic_init(). I > see that both ARM and PPC have properties available in their CPU > classes, and call parse_features() too. Well if generic handlers already exist and spread to other targets then there isn't much point in pushing it into arch specific code, sorry for noise.
diff --git a/hw/i386/pc.c b/hw/i386/pc.c index 5e20e07..ffcd645 100644 --- a/hw/i386/pc.c +++ b/hw/i386/pc.c @@ -1133,6 +1133,7 @@ void pc_cpus_init(PCMachineState *pcms) machine->cpu_model = "qemu32"; #endif } + machine->cpu_type = TYPE_X86_CPU; apic_id_limit = pc_apic_id_limit(max_cpus); if (apic_id_limit > ACPI_CPU_HOTPLUG_ID_LIMIT) { diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c index 030ee35..db441f2 100644 --- a/hw/ppc/spapr.c +++ b/hw/ppc/spapr.c @@ -1797,6 +1797,8 @@ static void ppc_spapr_init(MachineState *machine) if (machine->cpu_model == NULL) { machine->cpu_model = kvm_enabled() ? "host" : "POWER7"; } + machine->cpu_type = TYPE_POWERPC_CPU; + for (i = 0; i < smp_cpus; i++) { cpu = cpu_ppc_init(machine->cpu_model); if (cpu == NULL) { diff --git a/include/hw/boards.h b/include/hw/boards.h index 24eb6f0..a1f9512 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -128,6 +128,7 @@ struct MachineState { char *kernel_cmdline; char *initrd_filename; const char *cpu_model; + const char *cpu_type; AccelState *accelerator; };
Storing CPU typename in MachineState lets us to create CPU threads for all architectures in uniform manner from arch-neutral code. TODO: Touching only i386 and spapr targets for now Signed-off-by: Bharata B Rao <bharata@linux.vnet.ibm.com> --- hw/i386/pc.c | 1 + hw/ppc/spapr.c | 2 ++ include/hw/boards.h | 1 + 3 files changed, 4 insertions(+)