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

[Qemu-devel] [PATCH 0/2] 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. It does introduce
a behavioral change, however: In old code, pic would always be a fallback.
Now, it will have to be explicitly selected. I believe it is better behaviour,
but this is not the most important part of it, so I can easily go back
if people want it out.
    
Let the flames begin!

Glauber Costa (2):
  early set current_machine
  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 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.
---
 vl.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index d69250c..ceddeac 100644
--- a/vl.c
+++ b/vl.c
@@ -4841,6 +4841,9 @@ int main(int argc, char **argv, char **envp)
     }
     qemu_add_globals();
 
+
+    current_machine = machine;
+
     machine->init(ram_size, boot_devices,
                   kernel_filename, kernel_cmdline, initrd_filename, cpu_model);
 
@@ -4859,8 +4862,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.6

[Qemu-devel] [PATCH 2/2] 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 6f0f0d7..831728c 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;
@@ -28,6 +37,7 @@ typedef struct QEMUMachine {
         no_sdcard:1;
     int is_default;
     GlobalProperty *compat_props;
+    QEMUIrqchip *irqchip;
     struct QEMUMachine *next;
 } QEMUMachine;
 
diff --git a/hw/pc.c b/hw/pc.c
index ba14df0..43ec022 100644
--- a/hw/pc.c
+++ b/hw/pc.c
@@ -752,21 +752,43 @@ int cpu_is_bsp(CPUState *env)
     return env->cpu_index == 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;
 }
@@ -1074,6 +1096,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 150157c..2b985a9 100644
--- a/qemu-config.c
+++ b/qemu-config.c
@@ -296,6 +296,21 @@ QemuOptsList qemu_cpudef_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,
@@ -306,6 +321,7 @@ static QemuOptsList *lists[] = {
     &qemu_global_opts,
     &qemu_mon_opts,
     &qemu_cpudef_opts,
+    &qemu_machine_opts,
     NULL,
 };
 
diff --git a/qemu-config.h b/qemu-config.h
index f217c58..ea302f0 100644
--- a/qemu-config.h
+++ b/qemu-config.h
@@ -10,6 +10,7 @@ extern QemuOptsList qemu_rtc_opts;
 extern QemuOptsList qemu_global_opts;
 extern QemuOptsList qemu_mon_opts;
 extern QemuOptsList qemu_cpudef_opts;
+extern QemuOptsList qemu_machine_opts;
 
 QemuOptsList *qemu_find_opts(const char *group);
 int qemu_set_option(const char *str);
diff --git a/qemu-options.hx b/qemu-options.hx
index 8450b45..585ecd2 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -34,6 +34,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 ceddeac..02f38e6 100644
--- a/vl.c
+++ b/vl.c
@@ -1920,9 +1920,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;
         }
     }
@@ -3871,6 +3877,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.6

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

[Anthony Liguori]

On 03/24/2010 02:26 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.
>    

I started from a different place.  See machine-qemuopts in my staging tree.

I think we should combine efforts.

Regards,

Anthony Liguori

> ---
>   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 6f0f0d7..831728c 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;
> @@ -28,6 +37,7 @@ typedef struct QEMUMachine {
>           no_sdcard:1;
>       int is_default;
>       GlobalProperty *compat_props;
> +    QEMUIrqchip *irqchip;
>       struct QEMUMachine *next;
>   } QEMUMachine;
>
> diff --git a/hw/pc.c b/hw/pc.c
> index ba14df0..43ec022 100644
> --- a/hw/pc.c
> +++ b/hw/pc.c
> @@ -752,21 +752,43 @@ int cpu_is_bsp(CPUState *env)
>       return env->cpu_index == 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;
>   }
> @@ -1074,6 +1096,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 150157c..2b985a9 100644
> --- a/qemu-config.c
> +++ b/qemu-config.c
> @@ -296,6 +296,21 @@ QemuOptsList qemu_cpudef_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,
> @@ -306,6 +321,7 @@ static QemuOptsList *lists[] = {
>       &qemu_global_opts,
>       &qemu_mon_opts,
>       &qemu_cpudef_opts,
> +&qemu_machine_opts,
>       NULL,
>   };
>
> diff --git a/qemu-config.h b/qemu-config.h
> index f217c58..ea302f0 100644
> --- a/qemu-config.h
> +++ b/qemu-config.h
> @@ -10,6 +10,7 @@ extern QemuOptsList qemu_rtc_opts;
>   extern QemuOptsList qemu_global_opts;
>   extern QemuOptsList qemu_mon_opts;
>   extern QemuOptsList qemu_cpudef_opts;
> +extern QemuOptsList qemu_machine_opts;
>
>   QemuOptsList *qemu_find_opts(const char *group);
>   int qemu_set_option(const char *str);
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 8450b45..585ecd2 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -34,6 +34,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 ceddeac..02f38e6 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1920,9 +1920,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;
>           }
>       }
> @@ -3871,6 +3877,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 == '?') {
>    

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

[Glauber Costa]

On Wed, Mar 24, 2010 at 02:43:35PM -0500, Anthony Liguori wrote:
> On 03/24/2010 02:26 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.
> 
> I started from a different place.  See machine-qemuopts in my staging tree.
> 
> I think we should combine efforts.
> 
> Regards,
> 

Absolutely. I see little overlap between what we did. Just a comment on yours:

-static void an5206_init(ram_addr_t ram_size,
-                     const char *boot_device,
-                     const char *kernel_filename, const char *kernel_cmdline,
-                     const char *initrd_filename, const char *cpu_model)
+static void an5206_init(QemuOpts *opts)
 {

Since we're changing init functions anyway, I believe we should also pass
a pointer to the machine structure. With that, we can avoing using the global
current_machine.

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

[Anthony Liguori]

On 03/24/2010 03:58 PM, Glauber Costa wrote:
> On Wed, Mar 24, 2010 at 02:43:35PM -0500, Anthony Liguori wrote:
>    
>> On 03/24/2010 02:26 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.
>>>        
>> I started from a different place.  See machine-qemuopts in my staging tree.
>>
>> I think we should combine efforts.
>>
>> Regards,
>>
>>      
> Absolutely. I see little overlap between what we did. Just a comment on yours:
>
> -static void an5206_init(ram_addr_t ram_size,
> -                     const char *boot_device,
> -                     const char *kernel_filename, const char *kernel_cmdline,
> -                     const char *initrd_filename, const char *cpu_model)
> +static void an5206_init(QemuOpts *opts)
>   {
>
> Since we're changing init functions anyway, I believe we should also pass
> a pointer to the machine structure. With that, we can avoing using the global
> current_machine.
>    

Yes, I had the same thought.  For instance, with isa-pc is just pc_init 
with an extra parameter.  If we had a structure like:

typedef struct QEMUPCMachine
{
    QEMUMachine parent;
    int pci_enabled;
} QEMUPCMachine;

Then you wouldn't need those dispatch functions.  Not a huge win for x86 
but for sparc and some arm boards, it's pretty significant.

Regards,

Anthony Liguori