| Message ID | 4CADC186.3050309@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
(Sorry for the late reply) On Thu, Oct 07, 2010 at 08:48:06AM -0400, Anthony Liguori wrote: > On 10/07/2010 03:42 AM, Roedel, Joerg wrote: > > On Wed, Oct 06, 2010 at 03:24:59PM -0400, Anthony Liguori wrote: > > > >>>> + qemu_compat_version = machine->compat_version; > >>>> + > >>>> if (display_type == DT_NOGRAPHIC) { > >>>> if (default_parallel) > >>>> add_device_config(DEV_PARALLEL, "null"); > >>>> -- > >>>> 1.7.0.4 > >>>> > >>>> > >>> Looks fine to me, given CPUs are not in qdev. Anthony? > >>> > >>> > >> The idea is fine, but why not just add the default CPU to the machine > >> description? > >> > > If I remember correctly the reason was that the machine description was > > not accessible in the cpuid initialization path because it is a function > > local variable. > > Not tested at all but I think the attached patch addresses it in a > pretty nice way. > > There's a couple ways you could support your patch on top of this. You > could add a kvm_cpu_model to the machine structure that gets defaulted > too if kvm_enabled(). You could also introduce a new KVM machine type > that gets defaulted to if no explicit machine is specified. I had something similar in mind but then I realized that we need at least a cpu_model and a cpu_model_kvm to distinguish between the TCG and the KVM case. Further the QEMUMachine data structure is used for all architectures in QEMU and the model-names only make sense for x86. So I decided for the comapt-version way (which doesn't mean I object against this one ;-) ) Joerg > From d2370c88cef4b07d48ba3c4804e35ae2db8db7c0 Mon Sep 17 00:00:00 2001 > From: Anthony Liguori <aliguori@us.ibm.com> > Date: Thu, 7 Oct 2010 07:43:42 -0500 > Subject: [PATCH] machine: make default cpu model part of machine structure > > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > > diff --git a/hw/boards.h b/hw/boards.h > index 6f0f0d7..8c6ef27 100644 > --- a/hw/boards.h > +++ b/hw/boards.h > @@ -16,6 +16,7 @@ typedef struct QEMUMachine { > const char *name; > const char *alias; > const char *desc; > + const char *cpu_model; > QEMUMachineInitFunc *init; > int use_scsi; > int max_cpus; > diff --git a/hw/pc.c b/hw/pc.c > index 69b13bf..0826107 100644 > --- a/hw/pc.c > +++ b/hw/pc.c > @@ -866,14 +866,6 @@ void pc_cpus_init(const char *cpu_model) > int i; > > /* init CPUs */ > - if (cpu_model == NULL) { > -#ifdef TARGET_X86_64 > - cpu_model = "qemu64"; > -#else > - cpu_model = "qemu32"; > -#endif > - } > - > for(i = 0; i < smp_cpus; i++) { > pc_new_cpu(cpu_model); > } > diff --git a/hw/pc_piix.c b/hw/pc_piix.c > index 12359a7..919b4d6 100644 > --- a/hw/pc_piix.c > +++ b/hw/pc_piix.c > @@ -204,17 +204,22 @@ static void pc_init_isa(ram_addr_t ram_size, > const char *initrd_filename, > const char *cpu_model) > { > - if (cpu_model == NULL) > - cpu_model = "486"; > pc_init1(ram_size, boot_device, > kernel_filename, kernel_cmdline, > initrd_filename, cpu_model, 0); > } > > +#ifdef TARGET_X86_64 > +#define DEF_CPU_MODEL "qemu64" > +#else > +#define DEF_CPU_MODEL "qemu32" > +#endif > + > static QEMUMachine pc_machine = { > .name = "pc-0.13", > .alias = "pc", > .desc = "Standard PC", > + .cpu_model = DEF_CPU_MODEL, > .init = pc_init_pci, > .max_cpus = 255, > .is_default = 1, > @@ -223,6 +228,7 @@ static QEMUMachine pc_machine = { > static QEMUMachine pc_machine_v0_12 = { > .name = "pc-0.12", > .desc = "Standard PC", > + .cpu_model = DEF_CPU_MODEL, > .init = pc_init_pci, > .max_cpus = 255, > .compat_props = (GlobalProperty[]) { > @@ -242,6 +248,7 @@ static QEMUMachine pc_machine_v0_12 = { > static QEMUMachine pc_machine_v0_11 = { > .name = "pc-0.11", > .desc = "Standard PC, qemu 0.11", > + .cpu_model = DEF_CPU_MODEL, > .init = pc_init_pci, > .max_cpus = 255, > .compat_props = (GlobalProperty[]) { > @@ -277,6 +284,7 @@ static QEMUMachine pc_machine_v0_11 = { > static QEMUMachine pc_machine_v0_10 = { > .name = "pc-0.10", > .desc = "Standard PC, qemu 0.10", > + .cpu_model = DEF_CPU_MODEL, > .init = pc_init_pci, > .max_cpus = 255, > .compat_props = (GlobalProperty[]) { > @@ -324,6 +332,7 @@ static QEMUMachine pc_machine_v0_10 = { > static QEMUMachine isapc_machine = { > .name = "isapc", > .desc = "ISA-only PC", > + .cpu_model = "486", > .init = pc_init_isa, > .max_cpus = 1, > }; > diff --git a/vl.c b/vl.c > index df414ef..3a55cc8 100644 > --- a/vl.c > +++ b/vl.c > @@ -2904,6 +2904,10 @@ int main(int argc, char **argv, char **envp) > } > qemu_add_globals(); > > + if (cpu_model == NULL) { > + cpu_model = machine->cpu_model; > + } > + > machine->init(ram_size, boot_devices, > kernel_filename, kernel_cmdline, initrd_filename, cpu_model); > > -- > 1.7.0.4 >
On 10/18/2010 03:22 AM, Roedel, Joerg wrote: > (Sorry for the late reply) > > On Thu, Oct 07, 2010 at 08:48:06AM -0400, Anthony Liguori wrote: > >> On 10/07/2010 03:42 AM, Roedel, Joerg wrote: >> >>> On Wed, Oct 06, 2010 at 03:24:59PM -0400, Anthony Liguori wrote: >>> >>> >>>>>> + qemu_compat_version = machine->compat_version; >>>>>> + >>>>>> if (display_type == DT_NOGRAPHIC) { >>>>>> if (default_parallel) >>>>>> add_device_config(DEV_PARALLEL, "null"); >>>>>> -- >>>>>> 1.7.0.4 >>>>>> >>>>>> >>>>>> >>>>> Looks fine to me, given CPUs are not in qdev. Anthony? >>>>> >>>>> >>>>> >>>> The idea is fine, but why not just add the default CPU to the machine >>>> description? >>>> >>>> >>> If I remember correctly the reason was that the machine description was >>> not accessible in the cpuid initialization path because it is a function >>> local variable. >>> >> Not tested at all but I think the attached patch addresses it in a >> pretty nice way. >> >> There's a couple ways you could support your patch on top of this. You >> could add a kvm_cpu_model to the machine structure that gets defaulted >> too if kvm_enabled(). You could also introduce a new KVM machine type >> that gets defaulted to if no explicit machine is specified. >> > I had something similar in mind but then I realized that we need at > least a cpu_model and a cpu_model_kvm to distinguish between the TCG and > the KVM case. > I would think that having different default machines for KVM and TCG would be a better solution. > Further the QEMUMachine data structure is used for all architectures in > QEMU and the model-names only make sense for x86. SPARC uses cpu_model too FWIW. I believe Blue Swirl has even discussed using a feature-format similar to how x86 does it for SPARC CPUs. Regards, Anthony Liguori > So I decided for the > comapt-version way (which doesn't mean I object against this one ;-) ) > > Joerg > > >> From d2370c88cef4b07d48ba3c4804e35ae2db8db7c0 Mon Sep 17 00:00:00 2001 >> From: Anthony Liguori<aliguori@us.ibm.com> >> Date: Thu, 7 Oct 2010 07:43:42 -0500 >> Subject: [PATCH] machine: make default cpu model part of machine structure >> >> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com> >> >> diff --git a/hw/boards.h b/hw/boards.h >> index 6f0f0d7..8c6ef27 100644 >> --- a/hw/boards.h >> +++ b/hw/boards.h >> @@ -16,6 +16,7 @@ typedef struct QEMUMachine { >> const char *name; >> const char *alias; >> const char *desc; >> + const char *cpu_model; >> QEMUMachineInitFunc *init; >> int use_scsi; >> int max_cpus; >> diff --git a/hw/pc.c b/hw/pc.c >> index 69b13bf..0826107 100644 >> --- a/hw/pc.c >> +++ b/hw/pc.c >> @@ -866,14 +866,6 @@ void pc_cpus_init(const char *cpu_model) >> int i; >> >> /* init CPUs */ >> - if (cpu_model == NULL) { >> -#ifdef TARGET_X86_64 >> - cpu_model = "qemu64"; >> -#else >> - cpu_model = "qemu32"; >> -#endif >> - } >> - >> for(i = 0; i< smp_cpus; i++) { >> pc_new_cpu(cpu_model); >> } >> diff --git a/hw/pc_piix.c b/hw/pc_piix.c >> index 12359a7..919b4d6 100644 >> --- a/hw/pc_piix.c >> +++ b/hw/pc_piix.c >> @@ -204,17 +204,22 @@ static void pc_init_isa(ram_addr_t ram_size, >> const char *initrd_filename, >> const char *cpu_model) >> { >> - if (cpu_model == NULL) >> - cpu_model = "486"; >> pc_init1(ram_size, boot_device, >> kernel_filename, kernel_cmdline, >> initrd_filename, cpu_model, 0); >> } >> >> +#ifdef TARGET_X86_64 >> +#define DEF_CPU_MODEL "qemu64" >> +#else >> +#define DEF_CPU_MODEL "qemu32" >> +#endif >> + >> static QEMUMachine pc_machine = { >> .name = "pc-0.13", >> .alias = "pc", >> .desc = "Standard PC", >> + .cpu_model = DEF_CPU_MODEL, >> .init = pc_init_pci, >> .max_cpus = 255, >> .is_default = 1, >> @@ -223,6 +228,7 @@ static QEMUMachine pc_machine = { >> static QEMUMachine pc_machine_v0_12 = { >> .name = "pc-0.12", >> .desc = "Standard PC", >> + .cpu_model = DEF_CPU_MODEL, >> .init = pc_init_pci, >> .max_cpus = 255, >> .compat_props = (GlobalProperty[]) { >> @@ -242,6 +248,7 @@ static QEMUMachine pc_machine_v0_12 = { >> static QEMUMachine pc_machine_v0_11 = { >> .name = "pc-0.11", >> .desc = "Standard PC, qemu 0.11", >> + .cpu_model = DEF_CPU_MODEL, >> .init = pc_init_pci, >> .max_cpus = 255, >> .compat_props = (GlobalProperty[]) { >> @@ -277,6 +284,7 @@ static QEMUMachine pc_machine_v0_11 = { >> static QEMUMachine pc_machine_v0_10 = { >> .name = "pc-0.10", >> .desc = "Standard PC, qemu 0.10", >> + .cpu_model = DEF_CPU_MODEL, >> .init = pc_init_pci, >> .max_cpus = 255, >> .compat_props = (GlobalProperty[]) { >> @@ -324,6 +332,7 @@ static QEMUMachine pc_machine_v0_10 = { >> static QEMUMachine isapc_machine = { >> .name = "isapc", >> .desc = "ISA-only PC", >> + .cpu_model = "486", >> .init = pc_init_isa, >> .max_cpus = 1, >> }; >> diff --git a/vl.c b/vl.c >> index df414ef..3a55cc8 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -2904,6 +2904,10 @@ int main(int argc, char **argv, char **envp) >> } >> qemu_add_globals(); >> >> + if (cpu_model == NULL) { >> + cpu_model = machine->cpu_model; >> + } >> + >> machine->init(ram_size, boot_devices, >> kernel_filename, kernel_cmdline, initrd_filename, cpu_model); >> >> -- >> 1.7.0.4 >> >> > >
On Mon, Oct 18, 2010 at 2:16 PM, Anthony Liguori <aliguori@linux.vnet.ibm.com> wrote: > On 10/18/2010 03:22 AM, Roedel, Joerg wrote: >> >> (Sorry for the late reply) >> >> On Thu, Oct 07, 2010 at 08:48:06AM -0400, Anthony Liguori wrote: >> >>> >>> On 10/07/2010 03:42 AM, Roedel, Joerg wrote: >>> >>>> >>>> On Wed, Oct 06, 2010 at 03:24:59PM -0400, Anthony Liguori wrote: >>>> >>>> >>>>>>> >>>>>>> + qemu_compat_version = machine->compat_version; >>>>>>> + >>>>>>> if (display_type == DT_NOGRAPHIC) { >>>>>>> if (default_parallel) >>>>>>> add_device_config(DEV_PARALLEL, "null"); >>>>>>> -- >>>>>>> 1.7.0.4 >>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>>> Looks fine to me, given CPUs are not in qdev. Anthony? >>>>>> >>>>>> >>>>>> >>>>> >>>>> The idea is fine, but why not just add the default CPU to the machine >>>>> description? >>>>> >>>>> >>>> >>>> If I remember correctly the reason was that the machine description was >>>> not accessible in the cpuid initialization path because it is a function >>>> local variable. >>>> >>> >>> Not tested at all but I think the attached patch addresses it in a >>> pretty nice way. >>> >>> There's a couple ways you could support your patch on top of this. You >>> could add a kvm_cpu_model to the machine structure that gets defaulted >>> too if kvm_enabled(). You could also introduce a new KVM machine type >>> that gets defaulted to if no explicit machine is specified. >>> >> >> I had something similar in mind but then I realized that we need at >> least a cpu_model and a cpu_model_kvm to distinguish between the TCG and >> the KVM case. >> > > I would think that having different default machines for KVM and TCG would > be a better solution. > >> Further the QEMUMachine data structure is used for all architectures in >> QEMU and the model-names only make sense for x86. > > SPARC uses cpu_model too FWIW. I believe Blue Swirl has even discussed > using a feature-format similar to how x86 does it for SPARC CPUs. Actually I copied Sparc feature support from x86. Generic feature support would be nice.
From d2370c88cef4b07d48ba3c4804e35ae2db8db7c0 Mon Sep 17 00:00:00 2001 From: Anthony Liguori <aliguori@us.ibm.com> Date: Thu, 7 Oct 2010 07:43:42 -0500 Subject: [PATCH] machine: make default cpu model part of machine structure Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> diff --git a/hw/boards.h b/hw/boards.h index 6f0f0d7..8c6ef27 100644 --- a/hw/boards.h +++ b/hw/boards.h @@ -16,6 +16,7 @@ typedef struct QEMUMachine { const char *name; const char *alias; const char *desc; + const char *cpu_model; QEMUMachineInitFunc *init; int use_scsi; int max_cpus; diff --git a/hw/pc.c b/hw/pc.c index 69b13bf..0826107 100644 --- a/hw/pc.c +++ b/hw/pc.c @@ -866,14 +866,6 @@ void pc_cpus_init(const char *cpu_model) int i; /* init CPUs */ - if (cpu_model == NULL) { -#ifdef TARGET_X86_64 - cpu_model = "qemu64"; -#else - cpu_model = "qemu32"; -#endif - } - for(i = 0; i < smp_cpus; i++) { pc_new_cpu(cpu_model); } diff --git a/hw/pc_piix.c b/hw/pc_piix.c index 12359a7..919b4d6 100644 --- a/hw/pc_piix.c +++ b/hw/pc_piix.c @@ -204,17 +204,22 @@ static void pc_init_isa(ram_addr_t ram_size, const char *initrd_filename, const char *cpu_model) { - if (cpu_model == NULL) - cpu_model = "486"; pc_init1(ram_size, boot_device, kernel_filename, kernel_cmdline, initrd_filename, cpu_model, 0); } +#ifdef TARGET_X86_64 +#define DEF_CPU_MODEL "qemu64" +#else +#define DEF_CPU_MODEL "qemu32" +#endif + static QEMUMachine pc_machine = { .name = "pc-0.13", .alias = "pc", .desc = "Standard PC", + .cpu_model = DEF_CPU_MODEL, .init = pc_init_pci, .max_cpus = 255, .is_default = 1, @@ -223,6 +228,7 @@ static QEMUMachine pc_machine = { static QEMUMachine pc_machine_v0_12 = { .name = "pc-0.12", .desc = "Standard PC", + .cpu_model = DEF_CPU_MODEL, .init = pc_init_pci, .max_cpus = 255, .compat_props = (GlobalProperty[]) { @@ -242,6 +248,7 @@ static QEMUMachine pc_machine_v0_12 = { static QEMUMachine pc_machine_v0_11 = { .name = "pc-0.11", .desc = "Standard PC, qemu 0.11", + .cpu_model = DEF_CPU_MODEL, .init = pc_init_pci, .max_cpus = 255, .compat_props = (GlobalProperty[]) { @@ -277,6 +284,7 @@ static QEMUMachine pc_machine_v0_11 = { static QEMUMachine pc_machine_v0_10 = { .name = "pc-0.10", .desc = "Standard PC, qemu 0.10", + .cpu_model = DEF_CPU_MODEL, .init = pc_init_pci, .max_cpus = 255, .compat_props = (GlobalProperty[]) { @@ -324,6 +332,7 @@ static QEMUMachine pc_machine_v0_10 = { static QEMUMachine isapc_machine = { .name = "isapc", .desc = "ISA-only PC", + .cpu_model = "486", .init = pc_init_isa, .max_cpus = 1, }; diff --git a/vl.c b/vl.c index df414ef..3a55cc8 100644 --- a/vl.c +++ b/vl.c @@ -2904,6 +2904,10 @@ int main(int argc, char **argv, char **envp) } qemu_add_globals(); + if (cpu_model == NULL) { + cpu_model = machine->cpu_model; + } + machine->init(ram_size, boot_devices, kernel_filename, kernel_cmdline, initrd_filename, cpu_model); -- 1.7.0.4