[Qemu-devel] Re: [PATCH v2 2/2] basic machine opts framework

[Jan Kiszka]

Glauber Costa wrote:
> This patch adds initial support for the -machine option, that allows
> command line specification of machine attributes (always relying on safe
> defaults). Besides its value per-se, it is the saner way we found to
> allow for enabling/disabling of kvm's in-kernel irqchip.
> 
> A machine with in-kernel-irqchip could be specified as:
>       -machine irqchip=apic-kvm
> And one without it:
>       -machine irqchip=apic
> 
> To demonstrate how it'd work, this patch introduces a choice between
> "pic" and "apic", pic being the old-style isa thing.
> ---
>  hw/boards.h     |   10 ++++++++++
>  hw/pc.c         |   45 +++++++++++++++++++++++++++++++++++++++------
>  qemu-config.c   |   16 ++++++++++++++++
>  qemu-config.h   |    1 +
>  qemu-options.hx |    9 +++++++++
>  vl.c            |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 129 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/boards.h b/hw/boards.h
> index d889341..187794e 100644
> --- a/hw/boards.h
> +++ b/hw/boards.h
> @@ -12,6 +12,15 @@ typedef void QEMUMachineInitFunc(ram_addr_t ram_size,
>                                   const char *initrd_filename,
>                                   const char *cpu_model);
>  
> +typedef void (QEMUIrqchipFunc)(void *opaque);
> +
> +typedef struct QEMUIrqchip {
> +    const char *name;
> +    QEMUIrqchipFunc *init; 
> +    int used;
> +    int is_default;
> +} QEMUIrqchip;
> +
>  typedef struct QEMUMachine {
>      const char *name;
>      const char *alias;
> @@ -21,6 +30,7 @@ typedef struct QEMUMachine {
>      int max_cpus;
>      int is_default;
>      CompatProperty *compat_props;
> +    QEMUIrqchip *irqchip;
>      struct QEMUMachine *next;
>  } QEMUMachine;
>  
> diff --git a/hw/pc.c b/hw/pc.c
> index 408d6d6..b3de30a 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -1007,21 +1007,43 @@ int cpu_is_bsp(CPUState *env)
>      return env->cpuid_apic_id == 0;
>  }
>  
> +static void qemu_apic_init(void *opaque)
> +{
> +    CPUState *env = opaque;
> +    if (!(env->cpuid_features & CPUID_APIC)) {
> +        fprintf(stderr, "CPU lacks APIC cpuid flag\n");
> +        exit(1);
> +    }
> +    env->cpuid_apic_id = env->cpu_index;
> +    /* APIC reset callback resets cpu */
> +    apic_init(env);
> +}
> +
> +static void qemu_pic_init(void *opaque)
> +{
> +    CPUState *env = opaque;
> +
> +    if (smp_cpus > 1) {
> +        fprintf(stderr, "PIC can't support smp systems\n");
> +        exit(1);
> +    }
> +    qemu_register_reset((QEMUResetHandler*)cpu_reset, env);
> +}
> +
>  static CPUState *pc_new_cpu(const char *cpu_model)
>  {
>      CPUState *env;
> +    QEMUIrqchip *ic;
>  
>      env = cpu_init(cpu_model);
>      if (!env) {
>          fprintf(stderr, "Unable to find x86 CPU definition\n");
>          exit(1);
>      }
> -    if ((env->cpuid_features & CPUID_APIC) || smp_cpus > 1) {
> -        env->cpuid_apic_id = env->cpu_index;
> -        /* APIC reset callback resets cpu */
> -        apic_init(env);
> -    } else {
> -        qemu_register_reset((QEMUResetHandler*)cpu_reset, env);
> +
> +    for (ic = current_machine->irqchip; ic->name != NULL; ic++) {
> +        if (ic->used)
> +            ic->init(env);
>      }
>      return env;
>  }
> @@ -1370,6 +1392,17 @@ static QEMUMachine pc_machine = {
>      .desc = "Standard PC",
>      .init = pc_init_pci,
>      .max_cpus = 255,
> +    .irqchip = (QEMUIrqchip[]){
> +        {
> +            .name = "apic",
> +            .init = qemu_apic_init,
> +            .is_default = 1,
> +        },{
> +            .name = "pic",
> +            .init = qemu_pic_init,
> +        },
> +        { /* end of list */ },
> +    },
>      .is_default = 1,
>  };
>  
> diff --git a/qemu-config.c b/qemu-config.c
> index cae92f7..e83b301 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -196,6 +196,21 @@ QemuOptsList qemu_rtc_opts = {
>      },
>  };
>  
> +QemuOptsList qemu_machine_opts = {
> +    .name = "M",
> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_machine_opts.head),
> +    .desc = {
> +        {
> +            .name = "mach",
> +            .type = QEMU_OPT_STRING,
> +        },{
> +            .name = "irqchip",
> +            .type = QEMU_OPT_STRING,
> +        },

Can't we make the concrete machine define what options it needs? Pushing
this into the generic code may soon end up in a bunch of very special
switches that are unused on most machines or even have no meaning for them.

Also, I would suggest to introduce the generic part first, and then add
first users like the x86 irqchip.

Jan

> +        { /* end of list */ }
> +    },
> +};
> +
>  static QemuOptsList *lists[] = {
>      &qemu_drive_opts,
>      &qemu_chardev_opts,
> @@ -203,6 +218,7 @@ static QemuOptsList *lists[] = {
>      &qemu_netdev_opts,
>      &qemu_net_opts,
>      &qemu_rtc_opts,
> +    &qemu_machine_opts,
>      NULL,
>  };
>  
> diff --git a/qemu-config.h b/qemu-config.h
> index 3cc8864..9b02ee0 100644
> --- a/qemu-config.h
> +++ b/qemu-config.h
> @@ -7,6 +7,7 @@ extern QemuOptsList qemu_device_opts;
>  extern QemuOptsList qemu_netdev_opts;
>  extern QemuOptsList qemu_net_opts;
>  extern QemuOptsList qemu_rtc_opts;
> +extern QemuOptsList qemu_machine_opts;
>  
>  int qemu_set_option(const char *str);
>  
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 20aa242..4dfc55a 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -31,6 +31,15 @@ STEXI
>  Select the emulated @var{machine} (@code{-M ?} for list)
>  ETEXI
>  
> +DEF("machine", HAS_ARG, QEMU_OPTION_machine,
> +    "-machine mach=m[,irqchip=chip]\n"
> +    "    select emulated machine (-machine ? for list)\n")
> +STEXI
> address@hidden -machine @var{machine}[,@var{option}]
> address@hidden -machine
> +Select the emulated @var{machine} (@code{-machine ?} for list)
> +ETEXI
> +
>  DEF("cpu", HAS_ARG, QEMU_OPTION_cpu,
>      "-cpu cpu        select CPU (-cpu ? for list)\n")
>  STEXI
> diff --git a/vl.c b/vl.c
> index 7a8b20b..cabbd1e 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3217,9 +3217,15 @@ static QEMUMachine *find_machine(const char *name)
>  static QEMUMachine *find_default_machine(void)
>  {
>      QEMUMachine *m;
> +    QEMUIrqchip *ic;
>  
>      for(m = first_machine; m != NULL; m = m->next) {
>          if (m->is_default) {
> +            for (ic = m->irqchip; ic->name != NULL; ic++) {
> +                if (ic->is_default) {
> +                    ic->used = 1;
> +                }
> +            }
>              return m;
>          }
>      }
> @@ -4902,6 +4908,54 @@ int main(int argc, char **argv, char **envp)
>                      exit(*optarg != '?');
>                  }
>                  break;
> +            case QEMU_OPTION_machine: {
> +                const char *mach;
> +                const char *op;
> +
> +                opts = qemu_opts_parse(&qemu_machine_opts, optarg, 0);
> +                if (!opts) {
> +                    fprintf(stderr, "parse error: %s\n", optarg);
> +                    exit(1);
> +                }
> +                
> +                mach = qemu_opt_get(opts, "mach");
> +
> +                if (mach) {
> +                    machine = find_machine(mach);
> +                    if (!machine) {
> +                        QEMUMachine *m;
> +                        printf("Supported machines are:\n");
> +                        for(m = first_machine; m != NULL; m = m->next) {
> +                            if (m->alias)
> +                                printf("%-10s %s (alias of %s)\n",
> +                                       m->alias, m->desc, m->name);
> +                            printf("%-10s %s%s\n",
> +                                   m->name, m->desc,
> +                                   m->is_default ? " (default)" : "");
> +                        }
> +                        exit(*optarg != '?');
> +                    }
> +                }
> +
> +                op = qemu_opt_get(opts, "irqchip");
> +                if (op) {
> +                    int found = 0;
> +                    QEMUIrqchip *ic;
> +                    for (ic = machine->irqchip; ic->name != NULL; ic++) {
> +                       if (!strcmp(op, ic->name)) {
> +                        ic->used = 1;
> +                        found = 1;
> +                       } else
> +                        ic->used = 0;
> +                    }
> +                    if (!found) {
> +                        fprintf(stderr, "irqchip %s not found\n", op);
> +                        exit(1);
> +                        
> +                    }
> +                }
> +                break;
> +            }
>              case QEMU_OPTION_cpu:
>                  /* hw initialization will check this */
>                  if (*optarg == '?') {

signature.asc
Description: OpenPGP digital signature