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

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

[Glauber Costa]

Hello,

This is a resent (rebased) of an old patch set of mine, I sent
some time ago. With that, we should have all the needed infrastructure
to select the in-kernel irqchip for KVM.


Glauber Costa (2):
  early set current_machine
  basic machine opts framework

 hw/boards.h     |   10 +++++++++
 hw/pc.c         |   45 ++++++++++++++++++++++++++++++++++++-----
 qemu-config.c   |   16 ++++++++++++++
 qemu-config.h   |    1 +
 qemu-options.hx |    9 ++++++++
 vl.c            |   59 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 6 files changed, 132 insertions(+), 8 deletions(-)

[Qemu-devel] [PATCH v2 1/2] early set current_machine

[Glauber Costa]

this way, the machine_init function itself can know which machine is current
in use, not only the late init code.
Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 vl.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index 96838f8..7a8b20b 100644
--- a/vl.c
+++ b/vl.c
@@ -5824,6 +5824,9 @@ int main(int argc, char **argv, char **envp)
     if (machine->compat_props) {
         qdev_prop_register_compat(machine->compat_props);
     }
+
+    current_machine = machine;
+
     machine->init(ram_size, boot_devices,
                   kernel_filename, kernel_cmdline, initrd_filename, cpu_model);
 
@@ -5841,8 +5844,6 @@ int main(int argc, char **argv, char **envp)
         }
     }
 
-    current_machine = machine;
-
     /* init USB devices */
     if (usb_enabled) {
         if (foreach_device_config(DEV_USB, usb_parse) < 0)
-- 
1.6.2.2

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

[Glauber Costa]

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,
+        },
+        { /* 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
+@item -machine @var{machine}[,@var{option}]
+@findex -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 == '?') {
-- 
1.6.2.2

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 9030 bytes --]

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
> +@item -machine @var{machine}[,@var{option}]
> +@findex -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 == '?') {



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

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

[Glauber Costa]

On Wed, Jun 02, 2010 at 09:15:10AM +0200, Jan Kiszka wrote:
> >  
> > +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.
Yeah, in general, I do agree with you.

Me and anthony talked about it for a while some time ago, and more or less
concluded that it could be possible to avoid that, putting a little think
which options to add.

the "irqchip" option, if you note, is not x86-specific, in any case.
Any machine has an irqchip. The first idea was to use something like
"apic=in_kernel|userspace" which would be, that, very x86-centric.

So, since letting machines define their own options adds complexity,
my take would be to add those common options, and add infrastructure
for machine-specific options when we see something that makes it
unavoidable.

What do you think? 

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

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1991 bytes --]

Glauber Costa wrote:
> On Wed, Jun 02, 2010 at 09:15:10AM +0200, Jan Kiszka wrote:
>>>  
>>> +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.
> Yeah, in general, I do agree with you.
> 
> Me and anthony talked about it for a while some time ago, and more or less
> concluded that it could be possible to avoid that, putting a little think
> which options to add.
> 
> the "irqchip" option, if you note, is not x86-specific, in any case.
> Any machine has an irqchip.

...but the majority has no choice among different models. This option
simply makes only sense for x86 now and in the foreseeable future.

> The first idea was to use something like
> "apic=in_kernel|userspace" which would be, that, very x86-centric.
> 
> So, since letting machines define their own options adds complexity,
> my take would be to add those common options, and add infrastructure
> for machine-specific options when we see something that makes it
> unavoidable.
> 
> What do you think? 
> 

I have no general concerns if you document irqchip as a x86-only machine
option without effect on other machines and you promise to clean this up
once done with in-kernel irqchip support (which is clearly more
important). But the current design should not stay that way for a longer
period to avoid what I sketched above.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

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

[Anthony Liguori]

On 06/03/2010 01:07 AM, Jan Kiszka wrote:
> Glauber Costa wrote:
>    
>> On Wed, Jun 02, 2010 at 09:15:10AM +0200, Jan Kiszka wrote:
>>      
>>>>
>>>> +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.
>>>        
>> Yeah, in general, I do agree with you.
>>
>> Me and anthony talked about it for a while some time ago, and more or less
>> concluded that it could be possible to avoid that, putting a little think
>> which options to add.
>>
>> the "irqchip" option, if you note, is not x86-specific, in any case.
>> Any machine has an irqchip.
>>      
> ...but the majority has no choice among different models. This option
> simply makes only sense for x86 now and in the foreseeable future.
>
>    
>> The first idea was to use something like
>> "apic=in_kernel|userspace" which would be, that, very x86-centric.
>>
>> So, since letting machines define their own options adds complexity,
>> my take would be to add those common options, and add infrastructure
>> for machine-specific options when we see something that makes it
>> unavoidable.
>>
>> What do you think?
>>
>>      
> I have no general concerns if you document irqchip as a x86-only machine
> option without effect on other machines and you promise to clean this up
> once done with in-kernel irqchip support (which is clearly more
> important). But the current design should not stay that way for a longer
> period to avoid what I sketched above.
>    

What I think we need to do is actually use an empty QemuOptsList for the 
-machine option, make sure that the driver is present, then re-validate 
the list with a QemuOptsList that's included in the machine state.

We should, of course, have a #define of MACHINE_COMMON_OPTS.  This would 
allow machine specific options (like irqchip).  I don't think irqchip is 
the best name really.  I think it should be apic=kernel|user.

Regards,

Anthony Liguori

> Jan
>    

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

[Paul Brook]

> the "irqchip" option, if you note, is not x86-specific, in any case.
> Any machine has an irqchip. The first idea was to use something like
> "apic=in_kernel|userspace" which would be, that, very x86-centric.

How is this not x86-pc specific? All you're doing is creating two different 
machines, one with an APIC and one without.  In principle this is no different 
to what we have with pc v.s. isapc.

If you want machine properties (to avoid having to enumerate all the available 
machine variants) then these properties should be machine specific.

Incidentally, you patch appears to allow creation of a machine with a cpu that 
claims to have an APIC, but without an APIC present. Is that intentional?

Paul

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

[Anthony Liguori]

On 06/01/2010 12:56 PM, 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;
>    

We really need machine specific state.  I've sent a patch out to add this.

> 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,
>   };
>    

I don't think it's really useful to specify the apic vs. pic like this.  
I think the current scheme of cpu flag is adequate.

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

machine

> +    .head = QTAILQ_HEAD_INITIALIZER(qemu_machine_opts.head),
> +    .desc = {
> +        {
> +            .name = "mach",
> +            .type = QEMU_OPT_STRING,
> +        },{
>    

driver, and it ought to be an implied option so that '-machine pc' works.

> +            .name = "irqchip",
> +            .type = QEMU_OPT_STRING,
> +        },
> +        { /* end of list */ }
> +    },
> +};
>    

But let's actually make this an empty list, then do a #define that 
containers just the "machine" option.  Then we can setup a pc-specific 
qemu_machine_opts that contains the apic option.

Once we've found the machine based on the driver property, we can 
validate the machine-specific options in vl.c.
> 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);
> +
> +                    }
> +                }
>    

Can't do this type of work here because it won't get run when loaded via 
-readconfig.

We don't need to do it as part of this series, but we should fold all 
the parameters (mem_size, kernel_cmdline, etc.) into this qemuopts and 
make the other command lines syntactic wrappers.

Regards,

Anthony Liguori

> +                break;
> +            }
>               case QEMU_OPTION_cpu:
>                   /* hw initialization will check this */
>                   if (*optarg == '?') {
>    

[Qemu-devel] Re: [PATCH v2 1/2] early set current_machine

[Anthony Liguori]

On 06/01/2010 12:56 PM, Glauber Costa wrote:
> this way, the machine_init function itself can know which machine is current
> in use, not only the late init code.
>    

While your touching it...

We only use current_machine in hw/device-hotplug.c.  I think it would be 
better to introduce an accessor function (get_current_machine()) and 
then make this global static.

Regards,

Anthony Liguori

> Signed-off-by: Glauber Costa<glommer@redhat.com>
> ---
>   vl.c |    5 +++--
>   1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index 96838f8..7a8b20b 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -5824,6 +5824,9 @@ int main(int argc, char **argv, char **envp)
>       if (machine->compat_props) {
>           qdev_prop_register_compat(machine->compat_props);
>       }
> +
> +    current_machine = machine;
> +
>       machine->init(ram_size, boot_devices,
>                     kernel_filename, kernel_cmdline, initrd_filename, cpu_model);
>
> @@ -5841,8 +5844,6 @@ int main(int argc, char **argv, char **envp)
>           }
>       }
>
> -    current_machine = machine;
> -
>       /* init USB devices */
>       if (usb_enabled) {
>           if (foreach_device_config(DEV_USB, usb_parse)<  0)
>