Re: [Qemu-devel] [PATCH 3/5] add cpu_model to QEMUMachine

[Eduardo Habkost]

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 <address@hidden>

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 <address@hidden>



> ---
> 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
> 

-- 
Eduardo