From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=54603 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1OKBBj-0006C1-OI for qemu-devel@nongnu.org; Thu, 03 Jun 2010 10:14:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1OKBBi-0001eD-2G for qemu-devel@nongnu.org; Thu, 03 Jun 2010 10:14:27 -0400 Received: from e5.ny.us.ibm.com ([32.97.182.145]:34920) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1OKBBh-0001e5-Vd for qemu-devel@nongnu.org; Thu, 03 Jun 2010 10:14:26 -0400 Received: from d01relay01.pok.ibm.com (d01relay01.pok.ibm.com [9.56.227.233]) by e5.ny.us.ibm.com (8.14.4/8.13.1) with ESMTP id o53Dvo9I011106 for ; Thu, 3 Jun 2010 09:57:50 -0400 Received: from d03av05.boulder.ibm.com (d03av05.boulder.ibm.com [9.17.195.85]) by d01relay01.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id o53EEMlJ093386 for ; Thu, 3 Jun 2010 10:14:22 -0400 Received: from d03av05.boulder.ibm.com (loopback [127.0.0.1]) by d03av05.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id o53EDPJF008625 for ; Thu, 3 Jun 2010 08:13:26 -0600 Message-ID: <4C07B883.3080408@linux.vnet.ibm.com> Date: Thu, 03 Jun 2010 09:13:23 -0500 From: Anthony Liguori MIME-Version: 1.0 References: <1275414976-18258-1-git-send-email-glommer@redhat.com> <1275414976-18258-2-git-send-email-glommer@redhat.com> <1275414976-18258-3-git-send-email-glommer@redhat.com> In-Reply-To: <1275414976-18258-3-git-send-email-glommer@redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: [Qemu-devel] Re: [PATCH v2 2/2] basic machine opts framework List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Glauber Costa Cc: qemu-devel@nongnu.org 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 == '?') { >