| Message ID | 1367329288-27178-4-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Apr 30, 2013 at 03:41:26PM +0200, Igor Mammedov wrote: > 1. Use default cpu_model from machine if user haven't provided it. > It will allow statically define default cpu_model instead of > dynamically setting default value. > 2. Provides globally accessible cpu_model via current_machine pointer. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> My concern is that we already have a QEMUMachineInitArgs.cpu_model field, and now QEMUMachine.cpu_model and QEMUMachineInitArgs.cpu_model are redundant. To make it worse, both variables can disagree with each other because we have other code that set QEMUMachineInitArgs.cpu_model outside of main(): hw/arm/realview.c:338: args->cpu_model = "arm926"; hw/arm/realview.c:346: args->cpu_model = "arm11mpcore"; hw/arm/realview.c:354: args->cpu_model = "cortex-a8"; hw/arm/realview.c:362: args->cpu_model = "cortex-a9"; hw/arm/versatilepb.c:189: args->cpu_model = "arm926"; ARM doesn't have CPU hotplug, but this is still a bug waiting to happen. The most appropriate solution would be to kill QEMUMachineInitArgs and just put all the machine arguments inside QEMUMachine, but this is not feasible in time for 1.5. But still, I would prefer an alternative solution like this: * Add a machine_args field to QEMUMachine * Add a default_cpu_model field to QEMUMachine (to replace the one in this patch) * Use machine->machine_args->cpu_model inside pc_hot_add() But considering that: * The only new code using the new field is main() and pc_hot_add() (that's x86-specific), * QEMUMachineInitArgs.cpu_model can disagree with QEMUMachine.cpu_model only on ARM, * I am documenting my concerns above for future reference, I believe this is a reasonable intermediate solution for 1.5, so: Reviewed-by: Eduardo Habkost <ehabkost@redhat.com> > --- > Note: > 1. it will allow to remove dynamic setting of default cpu_model in pc.c:pc_cpus_init() > and simplify pc_init_isa() since default value will be defined as machine > definition. > 2. it will be used for cpu-add hook that will be set by target-i386. > --- > include/hw/boards.h | 1 + > vl.c | 6 ++++++ > 2 files changed, 7 insertions(+), 0 deletions(-) > > diff --git a/include/hw/boards.h b/include/hw/boards.h > index 75cd127..4ced60a 100644 > --- a/include/hw/boards.h > +++ b/include/hw/boards.h > @@ -44,6 +44,7 @@ typedef struct QEMUMachine { > struct QEMUMachine *next; > const char *hw_version; > void (*hot_add_cpu)(const int64_t id, Error **errp); > + const char *cpu_model; > } QEMUMachine; > > int qemu_register_machine(QEMUMachine *m); > diff --git a/vl.c b/vl.c > index 7d30af5..3df6021 100644 > --- a/vl.c > +++ b/vl.c > @@ -4283,6 +4283,12 @@ int main(int argc, char **argv, char **envp) > > qdev_machine_init(); > > + if (cpu_model) { > + machine->cpu_model = cpu_model; > + } else { > + cpu_model = machine->cpu_model; > + } > + > QEMUMachineInitArgs args = { .ram_size = ram_size, > .boot_device = (boot_devices[0] == '\0') ? > machine->boot_order : > -- > 1.7.1 >
On 30 April 2013 15:30, Eduardo Habkost <ehabkost@redhat.com> wrote: > My concern is that we already have a QEMUMachineInitArgs.cpu_model > field, and now QEMUMachine.cpu_model and QEMUMachineInitArgs.cpu_model > are redundant. > > To make it worse, both variables can disagree with each other because we > have other code that set QEMUMachineInitArgs.cpu_model outside of > main(): > > hw/arm/realview.c:338: args->cpu_model = "arm926"; > hw/arm/realview.c:346: args->cpu_model = "arm11mpcore"; > hw/arm/realview.c:354: args->cpu_model = "cortex-a8"; > hw/arm/realview.c:362: args->cpu_model = "cortex-a9"; > hw/arm/versatilepb.c:189: args->cpu_model = "arm926"; > > ARM doesn't have CPU hotplug, but this is still a bug waiting to happen. This ARM code is just borrowing the cpu_model field as a convenient place to stash the default value for the board. There are other ARM boards which do a similar thing but in a purely local variable (eg vexpress)... I think really if you want to know what the current CPU model is you need to fish the relevant QOM qbject out from somewhere at runtime. -- PMM
On Tue, 30 Apr 2013 15:54:34 +0100 Peter Maydell <peter.maydell@linaro.org> wrote: > On 30 April 2013 15:30, Eduardo Habkost <ehabkost@redhat.com> wrote: > > My concern is that we already have a QEMUMachineInitArgs.cpu_model > > field, and now QEMUMachine.cpu_model and QEMUMachineInitArgs.cpu_model > > are redundant. > > > > To make it worse, both variables can disagree with each other because we > > have other code that set QEMUMachineInitArgs.cpu_model outside of > > main(): > > > > hw/arm/realview.c:338: args->cpu_model = "arm926"; > > hw/arm/realview.c:346: args->cpu_model = "arm11mpcore"; > > hw/arm/realview.c:354: args->cpu_model = "cortex-a8"; > > hw/arm/realview.c:362: args->cpu_model = "cortex-a9"; > > hw/arm/versatilepb.c:189: args->cpu_model = "arm926"; > > > > ARM doesn't have CPU hotplug, but this is still a bug waiting to happen. > > This ARM code is just borrowing the cpu_model field as a convenient > place to stash the default value for the board. There are other > ARM boards which do a similar thing but in a purely local variable > (eg vexpress)... yes, patch addresses Eduardo's concern about not setting cpu_model of machine args anywhere else except of main. And later we could cleanup machine args usage and default cpu_models for all targets. > > I think really if you want to know what the current CPU model > is you need to fish the relevant QOM qbject out from somewhere > at runtime. Ideally that QOM object would be QOMifyed QEMUMachine, but we are not there yet. > > -- PMM >
On 30 April 2013 16:14, Igor Mammedov <imammedo@redhat.com> wrote: > On Tue, 30 Apr 2013 15:54:34 +0100 > Peter Maydell <peter.maydell@linaro.org> wrote: >> I think really if you want to know what the current CPU model >> is you need to fish the relevant QOM qbject out from somewhere >> at runtime. > > Ideally that QOM object would be QOMifyed QEMUMachine, but we > are not there yet. I don't want to close the door to being able to have a QEMUMachine with two QOM CPUs on it (ie heterogenous)... -- PMM
diff --git a/include/hw/boards.h b/include/hw/boards.h index 75cd127..4ced60a 100644 --- a/include/hw/boards.h +++ b/include/hw/boards.h @@ -44,6 +44,7 @@ typedef struct QEMUMachine { struct QEMUMachine *next; const char *hw_version; void (*hot_add_cpu)(const int64_t id, Error **errp); + const char *cpu_model; } QEMUMachine; int qemu_register_machine(QEMUMachine *m); diff --git a/vl.c b/vl.c index 7d30af5..3df6021 100644 --- a/vl.c +++ b/vl.c @@ -4283,6 +4283,12 @@ int main(int argc, char **argv, char **envp) qdev_machine_init(); + if (cpu_model) { + machine->cpu_model = cpu_model; + } else { + cpu_model = machine->cpu_model; + } + QEMUMachineInitArgs args = { .ram_size = ram_size, .boot_device = (boot_devices[0] == '\0') ? machine->boot_order :
1. Use default cpu_model from machine if user haven't provided it. It will allow statically define default cpu_model instead of dynamically setting default value. 2. Provides globally accessible cpu_model via current_machine pointer. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- Note: 1. it will allow to remove dynamic setting of default cpu_model in pc.c:pc_cpus_init() and simplify pc_init_isa() since default value will be defined as machine definition. 2. it will be used for cpu-add hook that will be set by target-i386. --- include/hw/boards.h | 1 + vl.c | 6 ++++++ 2 files changed, 7 insertions(+), 0 deletions(-)