Patchwork [3/5] add cpu_model to QEMUMachine

login
register
mail settings
Submitter Igor Mammedov
Date April 30, 2013, 1:41 p.m.
Message ID <1367329288-27178-4-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/240618/
State New
Headers show

Comments

Igor Mammedov - April 30, 2013, 1:41 p.m.
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(-)
Eduardo Habkost - April 30, 2013, 2:30 p.m.
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
>
Peter Maydell - April 30, 2013, 2:54 p.m.
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
Igor Mammedov - April 30, 2013, 3:14 p.m.
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
>
Peter Maydell - April 30, 2013, 3:32 p.m.
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

Patch

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 :