| Message ID | 1345226022-21654-3-git-send-email-ehabkost@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 17.08.2012 19:53, schrieb Eduardo Habkost: > It's nice to have a flexible system to maintain CPU models as data, but > this is holding us from making improvements in the CPU code because it's > not using the common infra-structure, and because the machine-type data > is still inside C code. > > Users who want to configure CPU features directly may simply use the > "-cpu" command-line option (and maybe an equivalent -device option in > the future) to set CPU features. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > target-i386/cpu.c | 101 ++---------------------------------------------------- > 1 file changed, 2 insertions(+), 99 deletions(-) Thanks for looking into this! I didn't get around to it myself yet. Andreas
On Fri, 17 Aug 2012 14:53:38 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > It's nice to have a flexible system to maintain CPU models as data, but > this is holding us from making improvements in the CPU code because it's > not using the common infra-structure, and because the machine-type data > is still inside C code. > > Users who want to configure CPU features directly may simply use the > "-cpu" command-line option (and maybe an equivalent -device option in > the future) to set CPU features. Haven't we agreed on a kvm call that config cpu section would be marked obsolete in next release and only after that to be removed? if we'll target this patch for 1.3 then I'll rebase CPU propeties on top of this series. > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > target-i386/cpu.c | 101 ++---------------------------------------------------- > 1 file changed, 2 insertions(+), 99 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 519a104..71c2ee7 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -237,7 +237,6 @@ typedef struct x86_def_t { > uint32_t xlevel; > char model_id[48]; > int vendor_override; > - uint32_t flags; > /* Store the results of Centaur's CPUID instructions */ > uint32_t ext4_features; > uint32_t xlevel2; > @@ -1286,7 +1285,7 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf) > char buf[256]; > > for (def = x86_defs; def; def = def->next) { > - snprintf(buf, sizeof (buf), def->flags ? "[%s]": "%s", def->name); > + snprintf(buf, sizeof(buf), "%s", def->name); > (*cpu_fprintf)(f, "x86 %16s %-48s\n", buf, def->model_id); > } > if (kvm_enabled()) { > @@ -1380,16 +1379,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model) > } > > #if !defined(CONFIG_USER_ONLY) > -/* copy vendor id string to 32 bit register, nul pad as needed > - */ > -static void cpyid(const char *s, uint32_t *id) > -{ > - char *d = (char *)id; > - char i; > - > - for (i = sizeof (*id); i--; ) > - *d++ = *s ? *s++ : '\0'; > -} > > /* interpret radix and convert from string to arbitrary scalar, > * otherwise flag failure > @@ -1403,87 +1392,6 @@ static void cpyid(const char *s, uint32_t *id) > *str && !*pend ? (*pval = ul) : (*perr = 1); \ > } > > -/* map cpuid options to feature bits, otherwise return failure > - * (option tags in *str are delimited by whitespace) > - */ > -static void setfeatures(uint32_t *pval, const char *str, > - const char **featureset, int *perr) > -{ > - const char *p, *q; > - > - for (q = p = str; *p || *q; q = p) { > - while (iswhite(*p)) > - q = ++p; > - while (*p && !iswhite(*p)) > - ++p; > - if (!*q && !*p) > - return; > - if (!lookup_feature(pval, q, p, featureset)) { > - fprintf(stderr, "error: feature \"%.*s\" not available in set\n", > - (int)(p - q), q); > - *perr = 1; > - return; > - } > - } > -} > - > -/* map config file options to x86_def_t form > - */ > -static int cpudef_setfield(const char *name, const char *str, void *opaque) > -{ > - x86_def_t *def = opaque; > - int err = 0; > - > - if (!strcmp(name, "name")) { > - g_free((void *)def->name); > - def->name = g_strdup(str); > - } else if (!strcmp(name, "model_id")) { > - strncpy(def->model_id, str, sizeof (def->model_id)); > - } else if (!strcmp(name, "level")) { > - setscalar(&def->level, str, &err) > - } else if (!strcmp(name, "vendor")) { > - cpyid(&str[0], &def->vendor1); > - cpyid(&str[4], &def->vendor2); > - cpyid(&str[8], &def->vendor3); > - } else if (!strcmp(name, "family")) { > - setscalar(&def->family, str, &err) > - } else if (!strcmp(name, "model")) { > - setscalar(&def->model, str, &err) > - } else if (!strcmp(name, "stepping")) { > - setscalar(&def->stepping, str, &err) > - } else if (!strcmp(name, "feature_edx")) { > - setfeatures(&def->features, str, feature_name, &err); > - } else if (!strcmp(name, "feature_ecx")) { > - setfeatures(&def->ext_features, str, ext_feature_name, &err); > - } else if (!strcmp(name, "extfeature_edx")) { > - setfeatures(&def->ext2_features, str, ext2_feature_name, &err); > - } else if (!strcmp(name, "extfeature_ecx")) { > - setfeatures(&def->ext3_features, str, ext3_feature_name, &err); > - } else if (!strcmp(name, "xlevel")) { > - setscalar(&def->xlevel, str, &err) > - } else { > - fprintf(stderr, "error: unknown option [%s = %s]\n", name, str); > - return (1); > - } > - if (err) { > - fprintf(stderr, "error: bad option value [%s = %s]\n", name, str); > - return (1); > - } > - return (0); > -} > - > -/* register config file entry as x86_def_t > - */ > -static int cpudef_register(QemuOpts *opts, void *opaque) > -{ > - x86_def_t *def = g_malloc0(sizeof (x86_def_t)); > - > - qemu_opt_foreach(opts, cpudef_setfield, def, 1); > - def->next = x86_defs; > - x86_defs = def; > - return (0); > -} > - > void cpu_clear_apic_feature(CPUX86State *env) > { > env->cpuid_features &= ~CPUID_APIC; > @@ -1491,8 +1399,7 @@ void cpu_clear_apic_feature(CPUX86State *env) > > #endif /* !CONFIG_USER_ONLY */ > > -/* register "cpudef" models defined in configuration file. Here we first > - * preload any built-in definitions > +/* Initialize list of CPU models, filling some non-static fields if necessary > */ > void x86_cpudef_setup(void) > { > @@ -1502,7 +1409,6 @@ void x86_cpudef_setup(void) > for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) { > x86_def_t *def = &builtin_x86_defs[i]; > def->next = x86_defs; > - def->flags = 1; > > /* Look for specific "cpudef" models that */ > /* have the QEMU version in .model_id */ > @@ -1518,9 +1424,6 @@ void x86_cpudef_setup(void) > > x86_defs = def; > } > -#if !defined(CONFIG_USER_ONLY) > - qemu_opts_foreach(qemu_find_opts("cpudef"), cpudef_register, NULL, 0); > -#endif > } > > static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx, > -- > 1.7.11.2 > >
On Mon, Aug 20, 2012 at 02:00:14PM +0200, Igor Mammedov wrote: > On Fri, 17 Aug 2012 14:53:38 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > It's nice to have a flexible system to maintain CPU models as data, but > > this is holding us from making improvements in the CPU code because it's > > not using the common infra-structure, and because the machine-type data > > is still inside C code. > > > > Users who want to configure CPU features directly may simply use the > > "-cpu" command-line option (and maybe an equivalent -device option in > > the future) to set CPU features. > Haven't we agreed on a kvm call that config cpu section would be marked > obsolete in next release and only after that to be removed? Yes. If I recall correctly, it will be deprecated on 1.2 and removed on 1.3. > > if we'll target this patch for 1.3 then I'll rebase CPU propeties on top of > this series. That was my plan, yes. > > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > target-i386/cpu.c | 101 ++---------------------------------------------------- > > 1 file changed, 2 insertions(+), 99 deletions(-) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 519a104..71c2ee7 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -237,7 +237,6 @@ typedef struct x86_def_t { > > uint32_t xlevel; > > char model_id[48]; > > int vendor_override; > > - uint32_t flags; > > /* Store the results of Centaur's CPUID instructions */ > > uint32_t ext4_features; > > uint32_t xlevel2; > > @@ -1286,7 +1285,7 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf) > > char buf[256]; > > > > for (def = x86_defs; def; def = def->next) { > > - snprintf(buf, sizeof (buf), def->flags ? "[%s]": "%s", def->name); > > + snprintf(buf, sizeof(buf), "%s", def->name); > > (*cpu_fprintf)(f, "x86 %16s %-48s\n", buf, def->model_id); > > } > > if (kvm_enabled()) { > > @@ -1380,16 +1379,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model) > > } > > > > #if !defined(CONFIG_USER_ONLY) > > -/* copy vendor id string to 32 bit register, nul pad as needed > > - */ > > -static void cpyid(const char *s, uint32_t *id) > > -{ > > - char *d = (char *)id; > > - char i; > > - > > - for (i = sizeof (*id); i--; ) > > - *d++ = *s ? *s++ : '\0'; > > -} > > > > /* interpret radix and convert from string to arbitrary scalar, > > * otherwise flag failure > > @@ -1403,87 +1392,6 @@ static void cpyid(const char *s, uint32_t *id) > > *str && !*pend ? (*pval = ul) : (*perr = 1); \ > > } > > > > -/* map cpuid options to feature bits, otherwise return failure > > - * (option tags in *str are delimited by whitespace) > > - */ > > -static void setfeatures(uint32_t *pval, const char *str, > > - const char **featureset, int *perr) > > -{ > > - const char *p, *q; > > - > > - for (q = p = str; *p || *q; q = p) { > > - while (iswhite(*p)) > > - q = ++p; > > - while (*p && !iswhite(*p)) > > - ++p; > > - if (!*q && !*p) > > - return; > > - if (!lookup_feature(pval, q, p, featureset)) { > > - fprintf(stderr, "error: feature \"%.*s\" not available in set\n", > > - (int)(p - q), q); > > - *perr = 1; > > - return; > > - } > > - } > > -} > > - > > -/* map config file options to x86_def_t form > > - */ > > -static int cpudef_setfield(const char *name, const char *str, void *opaque) > > -{ > > - x86_def_t *def = opaque; > > - int err = 0; > > - > > - if (!strcmp(name, "name")) { > > - g_free((void *)def->name); > > - def->name = g_strdup(str); > > - } else if (!strcmp(name, "model_id")) { > > - strncpy(def->model_id, str, sizeof (def->model_id)); > > - } else if (!strcmp(name, "level")) { > > - setscalar(&def->level, str, &err) > > - } else if (!strcmp(name, "vendor")) { > > - cpyid(&str[0], &def->vendor1); > > - cpyid(&str[4], &def->vendor2); > > - cpyid(&str[8], &def->vendor3); > > - } else if (!strcmp(name, "family")) { > > - setscalar(&def->family, str, &err) > > - } else if (!strcmp(name, "model")) { > > - setscalar(&def->model, str, &err) > > - } else if (!strcmp(name, "stepping")) { > > - setscalar(&def->stepping, str, &err) > > - } else if (!strcmp(name, "feature_edx")) { > > - setfeatures(&def->features, str, feature_name, &err); > > - } else if (!strcmp(name, "feature_ecx")) { > > - setfeatures(&def->ext_features, str, ext_feature_name, &err); > > - } else if (!strcmp(name, "extfeature_edx")) { > > - setfeatures(&def->ext2_features, str, ext2_feature_name, &err); > > - } else if (!strcmp(name, "extfeature_ecx")) { > > - setfeatures(&def->ext3_features, str, ext3_feature_name, &err); > > - } else if (!strcmp(name, "xlevel")) { > > - setscalar(&def->xlevel, str, &err) > > - } else { > > - fprintf(stderr, "error: unknown option [%s = %s]\n", name, str); > > - return (1); > > - } > > - if (err) { > > - fprintf(stderr, "error: bad option value [%s = %s]\n", name, str); > > - return (1); > > - } > > - return (0); > > -} > > - > > -/* register config file entry as x86_def_t > > - */ > > -static int cpudef_register(QemuOpts *opts, void *opaque) > > -{ > > - x86_def_t *def = g_malloc0(sizeof (x86_def_t)); > > - > > - qemu_opt_foreach(opts, cpudef_setfield, def, 1); > > - def->next = x86_defs; > > - x86_defs = def; > > - return (0); > > -} > > - > > void cpu_clear_apic_feature(CPUX86State *env) > > { > > env->cpuid_features &= ~CPUID_APIC; > > @@ -1491,8 +1399,7 @@ void cpu_clear_apic_feature(CPUX86State *env) > > > > #endif /* !CONFIG_USER_ONLY */ > > > > -/* register "cpudef" models defined in configuration file. Here we first > > - * preload any built-in definitions > > +/* Initialize list of CPU models, filling some non-static fields if necessary > > */ > > void x86_cpudef_setup(void) > > { > > @@ -1502,7 +1409,6 @@ void x86_cpudef_setup(void) > > for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) { > > x86_def_t *def = &builtin_x86_defs[i]; > > def->next = x86_defs; > > - def->flags = 1; > > > > /* Look for specific "cpudef" models that */ > > /* have the QEMU version in .model_id */ > > @@ -1518,9 +1424,6 @@ void x86_cpudef_setup(void) > > > > x86_defs = def; > > } > > -#if !defined(CONFIG_USER_ONLY) > > - qemu_opts_foreach(qemu_find_opts("cpudef"), cpudef_register, NULL, 0); > > -#endif > > } > > > > static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx, > > -- > > 1.7.11.2 > > > > > > > -- > Regards, > Igor
On Fri, 17 Aug 2012 14:53:38 -0300 Eduardo Habkost <ehabkost@redhat.com> wrote: > It's nice to have a flexible system to maintain CPU models as data, but > this is holding us from making improvements in the CPU code because it's > not using the common infra-structure, and because the machine-type data > is still inside C code. > > Users who want to configure CPU features directly may simply use the > "-cpu" command-line option (and maybe an equivalent -device option in > the future) to set CPU features. > Have I missed a patch that moves CPU definitions from cpus-x86_64.conf into the code? Perhaps they should be added to builtin_x86_defs in this patch. > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > --- > target-i386/cpu.c | 101 > ++---------------------------------------------------- 1 file changed, 2 > insertions(+), 99 deletions(-) > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > index 519a104..71c2ee7 100644 > --- a/target-i386/cpu.c > +++ b/target-i386/cpu.c > @@ -237,7 +237,6 @@ typedef struct x86_def_t { > uint32_t xlevel; > char model_id[48]; > int vendor_override; > - uint32_t flags; > /* Store the results of Centaur's CPUID instructions */ > uint32_t ext4_features; > uint32_t xlevel2; > @@ -1286,7 +1285,7 @@ void x86_cpu_list(FILE *f, fprintf_function > cpu_fprintf) char buf[256]; > > for (def = x86_defs; def; def = def->next) { > - snprintf(buf, sizeof (buf), def->flags ? "[%s]": "%s", def->name); > + snprintf(buf, sizeof(buf), "%s", def->name); > (*cpu_fprintf)(f, "x86 %16s %-48s\n", buf, def->model_id); > } > if (kvm_enabled()) { > @@ -1380,16 +1379,6 @@ int cpu_x86_register(X86CPU *cpu, const char > *cpu_model) } > > #if !defined(CONFIG_USER_ONLY) > -/* copy vendor id string to 32 bit register, nul pad as needed > - */ > -static void cpyid(const char *s, uint32_t *id) > -{ > - char *d = (char *)id; > - char i; > - > - for (i = sizeof (*id); i--; ) > - *d++ = *s ? *s++ : '\0'; > -} > > /* interpret radix and convert from string to arbitrary scalar, > * otherwise flag failure > @@ -1403,87 +1392,6 @@ static void cpyid(const char *s, uint32_t *id) > *str && !*pend ? (*pval = ul) : (*perr = 1); \ > } > > -/* map cpuid options to feature bits, otherwise return failure > - * (option tags in *str are delimited by whitespace) > - */ > -static void setfeatures(uint32_t *pval, const char *str, > - const char **featureset, int *perr) > -{ > - const char *p, *q; > - > - for (q = p = str; *p || *q; q = p) { > - while (iswhite(*p)) > - q = ++p; > - while (*p && !iswhite(*p)) > - ++p; > - if (!*q && !*p) > - return; > - if (!lookup_feature(pval, q, p, featureset)) { > - fprintf(stderr, "error: feature \"%.*s\" not available in > set\n", > - (int)(p - q), q); > - *perr = 1; > - return; > - } > - } > -} > - > -/* map config file options to x86_def_t form > - */ > -static int cpudef_setfield(const char *name, const char *str, void *opaque) > -{ > - x86_def_t *def = opaque; > - int err = 0; > - > - if (!strcmp(name, "name")) { > - g_free((void *)def->name); > - def->name = g_strdup(str); > - } else if (!strcmp(name, "model_id")) { > - strncpy(def->model_id, str, sizeof (def->model_id)); > - } else if (!strcmp(name, "level")) { > - setscalar(&def->level, str, &err) > - } else if (!strcmp(name, "vendor")) { > - cpyid(&str[0], &def->vendor1); > - cpyid(&str[4], &def->vendor2); > - cpyid(&str[8], &def->vendor3); > - } else if (!strcmp(name, "family")) { > - setscalar(&def->family, str, &err) > - } else if (!strcmp(name, "model")) { > - setscalar(&def->model, str, &err) > - } else if (!strcmp(name, "stepping")) { > - setscalar(&def->stepping, str, &err) > - } else if (!strcmp(name, "feature_edx")) { > - setfeatures(&def->features, str, feature_name, &err); > - } else if (!strcmp(name, "feature_ecx")) { > - setfeatures(&def->ext_features, str, ext_feature_name, &err); > - } else if (!strcmp(name, "extfeature_edx")) { > - setfeatures(&def->ext2_features, str, ext2_feature_name, &err); > - } else if (!strcmp(name, "extfeature_ecx")) { > - setfeatures(&def->ext3_features, str, ext3_feature_name, &err); > - } else if (!strcmp(name, "xlevel")) { > - setscalar(&def->xlevel, str, &err) > - } else { > - fprintf(stderr, "error: unknown option [%s = %s]\n", name, str); > - return (1); > - } > - if (err) { > - fprintf(stderr, "error: bad option value [%s = %s]\n", name, str); > - return (1); > - } > - return (0); > -} > - > -/* register config file entry as x86_def_t > - */ > -static int cpudef_register(QemuOpts *opts, void *opaque) > -{ > - x86_def_t *def = g_malloc0(sizeof (x86_def_t)); > - > - qemu_opt_foreach(opts, cpudef_setfield, def, 1); > - def->next = x86_defs; > - x86_defs = def; > - return (0); > -} > - > void cpu_clear_apic_feature(CPUX86State *env) > { > env->cpuid_features &= ~CPUID_APIC; > @@ -1491,8 +1399,7 @@ void cpu_clear_apic_feature(CPUX86State *env) > > #endif /* !CONFIG_USER_ONLY */ > > -/* register "cpudef" models defined in configuration file. Here we first > - * preload any built-in definitions > +/* Initialize list of CPU models, filling some non-static fields if > necessary */ > void x86_cpudef_setup(void) > { > @@ -1502,7 +1409,6 @@ void x86_cpudef_setup(void) > for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) { > x86_def_t *def = &builtin_x86_defs[i]; > def->next = x86_defs; > - def->flags = 1; > > /* Look for specific "cpudef" models that */ > /* have the QEMU version in .model_id */ > @@ -1518,9 +1424,6 @@ void x86_cpudef_setup(void) > > x86_defs = def; > } > -#if !defined(CONFIG_USER_ONLY) > - qemu_opts_foreach(qemu_find_opts("cpudef"), cpudef_register, NULL, 0); > -#endif > } > > static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
On Wed, 5 Sep 2012 11:09:16 +0200 Igor Mammedov <imammedo@redhat.com> wrote: > On Fri, 17 Aug 2012 14:53:38 -0300 > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > It's nice to have a flexible system to maintain CPU models as data, but > > this is holding us from making improvements in the CPU code because it's > > not using the common infra-structure, and because the machine-type data > > is still inside C code. > > > > Users who want to configure CPU features directly may simply use the > > "-cpu" command-line option (and maybe an equivalent -device option in > > the future) to set CPU features. > > > Have I missed a patch that moves CPU definitions from cpus-x86_64.conf into > the code? > Perhaps they should be added to builtin_x86_defs in this patch. Ah, found it http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg00223.html It seems missed 1.2 and this series just don't mention that it depends on above mentioned series. > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > --- > > target-i386/cpu.c | 101 > > ++---------------------------------------------------- 1 file changed, 2 > > insertions(+), 99 deletions(-) > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > index 519a104..71c2ee7 100644 > > --- a/target-i386/cpu.c > > +++ b/target-i386/cpu.c > > @@ -237,7 +237,6 @@ typedef struct x86_def_t { > > uint32_t xlevel; > > char model_id[48]; > > int vendor_override; > > - uint32_t flags; > > /* Store the results of Centaur's CPUID instructions */ > > uint32_t ext4_features; > > uint32_t xlevel2; > > @@ -1286,7 +1285,7 @@ void x86_cpu_list(FILE *f, fprintf_function > > cpu_fprintf) char buf[256]; > > > > for (def = x86_defs; def; def = def->next) { > > - snprintf(buf, sizeof (buf), def->flags ? "[%s]": "%s", > > def->name); > > + snprintf(buf, sizeof(buf), "%s", def->name); > > (*cpu_fprintf)(f, "x86 %16s %-48s\n", buf, def->model_id); > > } > > if (kvm_enabled()) { > > @@ -1380,16 +1379,6 @@ int cpu_x86_register(X86CPU *cpu, const char > > *cpu_model) } > > > > #if !defined(CONFIG_USER_ONLY) > > -/* copy vendor id string to 32 bit register, nul pad as needed > > - */ > > -static void cpyid(const char *s, uint32_t *id) > > -{ > > - char *d = (char *)id; > > - char i; > > - > > - for (i = sizeof (*id); i--; ) > > - *d++ = *s ? *s++ : '\0'; > > -} > > > > /* interpret radix and convert from string to arbitrary scalar, > > * otherwise flag failure > > @@ -1403,87 +1392,6 @@ static void cpyid(const char *s, uint32_t *id) > > *str && !*pend ? (*pval = ul) : (*perr = 1); \ > > } > > > > -/* map cpuid options to feature bits, otherwise return failure > > - * (option tags in *str are delimited by whitespace) > > - */ > > -static void setfeatures(uint32_t *pval, const char *str, > > - const char **featureset, int *perr) > > -{ > > - const char *p, *q; > > - > > - for (q = p = str; *p || *q; q = p) { > > - while (iswhite(*p)) > > - q = ++p; > > - while (*p && !iswhite(*p)) > > - ++p; > > - if (!*q && !*p) > > - return; > > - if (!lookup_feature(pval, q, p, featureset)) { > > - fprintf(stderr, "error: feature \"%.*s\" not available in > > set\n", > > - (int)(p - q), q); > > - *perr = 1; > > - return; > > - } > > - } > > -} > > - > > -/* map config file options to x86_def_t form > > - */ > > -static int cpudef_setfield(const char *name, const char *str, void > > *opaque) -{ > > - x86_def_t *def = opaque; > > - int err = 0; > > - > > - if (!strcmp(name, "name")) { > > - g_free((void *)def->name); > > - def->name = g_strdup(str); > > - } else if (!strcmp(name, "model_id")) { > > - strncpy(def->model_id, str, sizeof (def->model_id)); > > - } else if (!strcmp(name, "level")) { > > - setscalar(&def->level, str, &err) > > - } else if (!strcmp(name, "vendor")) { > > - cpyid(&str[0], &def->vendor1); > > - cpyid(&str[4], &def->vendor2); > > - cpyid(&str[8], &def->vendor3); > > - } else if (!strcmp(name, "family")) { > > - setscalar(&def->family, str, &err) > > - } else if (!strcmp(name, "model")) { > > - setscalar(&def->model, str, &err) > > - } else if (!strcmp(name, "stepping")) { > > - setscalar(&def->stepping, str, &err) > > - } else if (!strcmp(name, "feature_edx")) { > > - setfeatures(&def->features, str, feature_name, &err); > > - } else if (!strcmp(name, "feature_ecx")) { > > - setfeatures(&def->ext_features, str, ext_feature_name, &err); > > - } else if (!strcmp(name, "extfeature_edx")) { > > - setfeatures(&def->ext2_features, str, ext2_feature_name, &err); > > - } else if (!strcmp(name, "extfeature_ecx")) { > > - setfeatures(&def->ext3_features, str, ext3_feature_name, &err); > > - } else if (!strcmp(name, "xlevel")) { > > - setscalar(&def->xlevel, str, &err) > > - } else { > > - fprintf(stderr, "error: unknown option [%s = %s]\n", name, str); > > - return (1); > > - } > > - if (err) { > > - fprintf(stderr, "error: bad option value [%s = %s]\n", name, > > str); > > - return (1); > > - } > > - return (0); > > -} > > - > > -/* register config file entry as x86_def_t > > - */ > > -static int cpudef_register(QemuOpts *opts, void *opaque) > > -{ > > - x86_def_t *def = g_malloc0(sizeof (x86_def_t)); > > - > > - qemu_opt_foreach(opts, cpudef_setfield, def, 1); > > - def->next = x86_defs; > > - x86_defs = def; > > - return (0); > > -} > > - > > void cpu_clear_apic_feature(CPUX86State *env) > > { > > env->cpuid_features &= ~CPUID_APIC; > > @@ -1491,8 +1399,7 @@ void cpu_clear_apic_feature(CPUX86State *env) > > > > #endif /* !CONFIG_USER_ONLY */ > > > > -/* register "cpudef" models defined in configuration file. Here we first > > - * preload any built-in definitions > > +/* Initialize list of CPU models, filling some non-static fields if > > necessary */ > > void x86_cpudef_setup(void) > > { > > @@ -1502,7 +1409,6 @@ void x86_cpudef_setup(void) > > for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) { > > x86_def_t *def = &builtin_x86_defs[i]; > > def->next = x86_defs; > > - def->flags = 1; > > > > /* Look for specific "cpudef" models that */ > > /* have the QEMU version in .model_id */ > > @@ -1518,9 +1424,6 @@ void x86_cpudef_setup(void) > > > > x86_defs = def; > > } > > -#if !defined(CONFIG_USER_ONLY) > > - qemu_opts_foreach(qemu_find_opts("cpudef"), cpudef_register, NULL, > > 0); -#endif > > } > > > > static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx, > > >
On Wed, Sep 05, 2012 at 11:39:21AM +0200, Igor Mammedov wrote: > On Wed, 5 Sep 2012 11:09:16 +0200 > Igor Mammedov <imammedo@redhat.com> wrote: > > > On Fri, 17 Aug 2012 14:53:38 -0300 > > Eduardo Habkost <ehabkost@redhat.com> wrote: > > > > > It's nice to have a flexible system to maintain CPU models as data, but > > > this is holding us from making improvements in the CPU code because it's > > > not using the common infra-structure, and because the machine-type data > > > is still inside C code. > > > > > > Users who want to configure CPU features directly may simply use the > > > "-cpu" command-line option (and maybe an equivalent -device option in > > > the future) to set CPU features. > > > > > Have I missed a patch that moves CPU definitions from cpus-x86_64.conf into > > the code? > > Perhaps they should be added to builtin_x86_defs in this patch. > > Ah, found it > http://lists.gnu.org/archive/html/qemu-devel/2012-08/msg00223.html > It seems missed 1.2 and this series just don't mention that it depends on > above mentioned series. Yes. Sorry if I didn't mention that. We have so many series in the queue that sometimes I get confused by the dependencies myself. > > > > > > Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> > > > --- > > > target-i386/cpu.c | 101 > > > ++---------------------------------------------------- 1 file changed, 2 > > > insertions(+), 99 deletions(-) > > > > > > diff --git a/target-i386/cpu.c b/target-i386/cpu.c > > > index 519a104..71c2ee7 100644 > > > --- a/target-i386/cpu.c > > > +++ b/target-i386/cpu.c > > > @@ -237,7 +237,6 @@ typedef struct x86_def_t { > > > uint32_t xlevel; > > > char model_id[48]; > > > int vendor_override; > > > - uint32_t flags; > > > /* Store the results of Centaur's CPUID instructions */ > > > uint32_t ext4_features; > > > uint32_t xlevel2; > > > @@ -1286,7 +1285,7 @@ void x86_cpu_list(FILE *f, fprintf_function > > > cpu_fprintf) char buf[256]; > > > > > > for (def = x86_defs; def; def = def->next) { > > > - snprintf(buf, sizeof (buf), def->flags ? "[%s]": "%s", > > > def->name); > > > + snprintf(buf, sizeof(buf), "%s", def->name); > > > (*cpu_fprintf)(f, "x86 %16s %-48s\n", buf, def->model_id); > > > } > > > if (kvm_enabled()) { > > > @@ -1380,16 +1379,6 @@ int cpu_x86_register(X86CPU *cpu, const char > > > *cpu_model) } > > > > > > #if !defined(CONFIG_USER_ONLY) > > > -/* copy vendor id string to 32 bit register, nul pad as needed > > > - */ > > > -static void cpyid(const char *s, uint32_t *id) > > > -{ > > > - char *d = (char *)id; > > > - char i; > > > - > > > - for (i = sizeof (*id); i--; ) > > > - *d++ = *s ? *s++ : '\0'; > > > -} > > > > > > /* interpret radix and convert from string to arbitrary scalar, > > > * otherwise flag failure > > > @@ -1403,87 +1392,6 @@ static void cpyid(const char *s, uint32_t *id) > > > *str && !*pend ? (*pval = ul) : (*perr = 1); \ > > > } > > > > > > -/* map cpuid options to feature bits, otherwise return failure > > > - * (option tags in *str are delimited by whitespace) > > > - */ > > > -static void setfeatures(uint32_t *pval, const char *str, > > > - const char **featureset, int *perr) > > > -{ > > > - const char *p, *q; > > > - > > > - for (q = p = str; *p || *q; q = p) { > > > - while (iswhite(*p)) > > > - q = ++p; > > > - while (*p && !iswhite(*p)) > > > - ++p; > > > - if (!*q && !*p) > > > - return; > > > - if (!lookup_feature(pval, q, p, featureset)) { > > > - fprintf(stderr, "error: feature \"%.*s\" not available in > > > set\n", > > > - (int)(p - q), q); > > > - *perr = 1; > > > - return; > > > - } > > > - } > > > -} > > > - > > > -/* map config file options to x86_def_t form > > > - */ > > > -static int cpudef_setfield(const char *name, const char *str, void > > > *opaque) -{ > > > - x86_def_t *def = opaque; > > > - int err = 0; > > > - > > > - if (!strcmp(name, "name")) { > > > - g_free((void *)def->name); > > > - def->name = g_strdup(str); > > > - } else if (!strcmp(name, "model_id")) { > > > - strncpy(def->model_id, str, sizeof (def->model_id)); > > > - } else if (!strcmp(name, "level")) { > > > - setscalar(&def->level, str, &err) > > > - } else if (!strcmp(name, "vendor")) { > > > - cpyid(&str[0], &def->vendor1); > > > - cpyid(&str[4], &def->vendor2); > > > - cpyid(&str[8], &def->vendor3); > > > - } else if (!strcmp(name, "family")) { > > > - setscalar(&def->family, str, &err) > > > - } else if (!strcmp(name, "model")) { > > > - setscalar(&def->model, str, &err) > > > - } else if (!strcmp(name, "stepping")) { > > > - setscalar(&def->stepping, str, &err) > > > - } else if (!strcmp(name, "feature_edx")) { > > > - setfeatures(&def->features, str, feature_name, &err); > > > - } else if (!strcmp(name, "feature_ecx")) { > > > - setfeatures(&def->ext_features, str, ext_feature_name, &err); > > > - } else if (!strcmp(name, "extfeature_edx")) { > > > - setfeatures(&def->ext2_features, str, ext2_feature_name, &err); > > > - } else if (!strcmp(name, "extfeature_ecx")) { > > > - setfeatures(&def->ext3_features, str, ext3_feature_name, &err); > > > - } else if (!strcmp(name, "xlevel")) { > > > - setscalar(&def->xlevel, str, &err) > > > - } else { > > > - fprintf(stderr, "error: unknown option [%s = %s]\n", name, str); > > > - return (1); > > > - } > > > - if (err) { > > > - fprintf(stderr, "error: bad option value [%s = %s]\n", name, > > > str); > > > - return (1); > > > - } > > > - return (0); > > > -} > > > - > > > -/* register config file entry as x86_def_t > > > - */ > > > -static int cpudef_register(QemuOpts *opts, void *opaque) > > > -{ > > > - x86_def_t *def = g_malloc0(sizeof (x86_def_t)); > > > - > > > - qemu_opt_foreach(opts, cpudef_setfield, def, 1); > > > - def->next = x86_defs; > > > - x86_defs = def; > > > - return (0); > > > -} > > > - > > > void cpu_clear_apic_feature(CPUX86State *env) > > > { > > > env->cpuid_features &= ~CPUID_APIC; > > > @@ -1491,8 +1399,7 @@ void cpu_clear_apic_feature(CPUX86State *env) > > > > > > #endif /* !CONFIG_USER_ONLY */ > > > > > > -/* register "cpudef" models defined in configuration file. Here we first > > > - * preload any built-in definitions > > > +/* Initialize list of CPU models, filling some non-static fields if > > > necessary */ > > > void x86_cpudef_setup(void) > > > { > > > @@ -1502,7 +1409,6 @@ void x86_cpudef_setup(void) > > > for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) { > > > x86_def_t *def = &builtin_x86_defs[i]; > > > def->next = x86_defs; > > > - def->flags = 1; > > > > > > /* Look for specific "cpudef" models that */ > > > /* have the QEMU version in .model_id */ > > > @@ -1518,9 +1424,6 @@ void x86_cpudef_setup(void) > > > > > > x86_defs = def; > > > } > > > -#if !defined(CONFIG_USER_ONLY) > > > - qemu_opts_foreach(qemu_find_opts("cpudef"), cpudef_register, NULL, > > > 0); -#endif > > > } > > > > > > static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx, > > > > > > >
diff --git a/target-i386/cpu.c b/target-i386/cpu.c index 519a104..71c2ee7 100644 --- a/target-i386/cpu.c +++ b/target-i386/cpu.c @@ -237,7 +237,6 @@ typedef struct x86_def_t { uint32_t xlevel; char model_id[48]; int vendor_override; - uint32_t flags; /* Store the results of Centaur's CPUID instructions */ uint32_t ext4_features; uint32_t xlevel2; @@ -1286,7 +1285,7 @@ void x86_cpu_list(FILE *f, fprintf_function cpu_fprintf) char buf[256]; for (def = x86_defs; def; def = def->next) { - snprintf(buf, sizeof (buf), def->flags ? "[%s]": "%s", def->name); + snprintf(buf, sizeof(buf), "%s", def->name); (*cpu_fprintf)(f, "x86 %16s %-48s\n", buf, def->model_id); } if (kvm_enabled()) { @@ -1380,16 +1379,6 @@ int cpu_x86_register(X86CPU *cpu, const char *cpu_model) } #if !defined(CONFIG_USER_ONLY) -/* copy vendor id string to 32 bit register, nul pad as needed - */ -static void cpyid(const char *s, uint32_t *id) -{ - char *d = (char *)id; - char i; - - for (i = sizeof (*id); i--; ) - *d++ = *s ? *s++ : '\0'; -} /* interpret radix and convert from string to arbitrary scalar, * otherwise flag failure @@ -1403,87 +1392,6 @@ static void cpyid(const char *s, uint32_t *id) *str && !*pend ? (*pval = ul) : (*perr = 1); \ } -/* map cpuid options to feature bits, otherwise return failure - * (option tags in *str are delimited by whitespace) - */ -static void setfeatures(uint32_t *pval, const char *str, - const char **featureset, int *perr) -{ - const char *p, *q; - - for (q = p = str; *p || *q; q = p) { - while (iswhite(*p)) - q = ++p; - while (*p && !iswhite(*p)) - ++p; - if (!*q && !*p) - return; - if (!lookup_feature(pval, q, p, featureset)) { - fprintf(stderr, "error: feature \"%.*s\" not available in set\n", - (int)(p - q), q); - *perr = 1; - return; - } - } -} - -/* map config file options to x86_def_t form - */ -static int cpudef_setfield(const char *name, const char *str, void *opaque) -{ - x86_def_t *def = opaque; - int err = 0; - - if (!strcmp(name, "name")) { - g_free((void *)def->name); - def->name = g_strdup(str); - } else if (!strcmp(name, "model_id")) { - strncpy(def->model_id, str, sizeof (def->model_id)); - } else if (!strcmp(name, "level")) { - setscalar(&def->level, str, &err) - } else if (!strcmp(name, "vendor")) { - cpyid(&str[0], &def->vendor1); - cpyid(&str[4], &def->vendor2); - cpyid(&str[8], &def->vendor3); - } else if (!strcmp(name, "family")) { - setscalar(&def->family, str, &err) - } else if (!strcmp(name, "model")) { - setscalar(&def->model, str, &err) - } else if (!strcmp(name, "stepping")) { - setscalar(&def->stepping, str, &err) - } else if (!strcmp(name, "feature_edx")) { - setfeatures(&def->features, str, feature_name, &err); - } else if (!strcmp(name, "feature_ecx")) { - setfeatures(&def->ext_features, str, ext_feature_name, &err); - } else if (!strcmp(name, "extfeature_edx")) { - setfeatures(&def->ext2_features, str, ext2_feature_name, &err); - } else if (!strcmp(name, "extfeature_ecx")) { - setfeatures(&def->ext3_features, str, ext3_feature_name, &err); - } else if (!strcmp(name, "xlevel")) { - setscalar(&def->xlevel, str, &err) - } else { - fprintf(stderr, "error: unknown option [%s = %s]\n", name, str); - return (1); - } - if (err) { - fprintf(stderr, "error: bad option value [%s = %s]\n", name, str); - return (1); - } - return (0); -} - -/* register config file entry as x86_def_t - */ -static int cpudef_register(QemuOpts *opts, void *opaque) -{ - x86_def_t *def = g_malloc0(sizeof (x86_def_t)); - - qemu_opt_foreach(opts, cpudef_setfield, def, 1); - def->next = x86_defs; - x86_defs = def; - return (0); -} - void cpu_clear_apic_feature(CPUX86State *env) { env->cpuid_features &= ~CPUID_APIC; @@ -1491,8 +1399,7 @@ void cpu_clear_apic_feature(CPUX86State *env) #endif /* !CONFIG_USER_ONLY */ -/* register "cpudef" models defined in configuration file. Here we first - * preload any built-in definitions +/* Initialize list of CPU models, filling some non-static fields if necessary */ void x86_cpudef_setup(void) { @@ -1502,7 +1409,6 @@ void x86_cpudef_setup(void) for (i = 0; i < ARRAY_SIZE(builtin_x86_defs); ++i) { x86_def_t *def = &builtin_x86_defs[i]; def->next = x86_defs; - def->flags = 1; /* Look for specific "cpudef" models that */ /* have the QEMU version in .model_id */ @@ -1518,9 +1424,6 @@ void x86_cpudef_setup(void) x86_defs = def; } -#if !defined(CONFIG_USER_ONLY) - qemu_opts_foreach(qemu_find_opts("cpudef"), cpudef_register, NULL, 0); -#endif } static void get_cpuid_vendor(CPUX86State *env, uint32_t *ebx,
It's nice to have a flexible system to maintain CPU models as data, but this is holding us from making improvements in the CPU code because it's not using the common infra-structure, and because the machine-type data is still inside C code. Users who want to configure CPU features directly may simply use the "-cpu" command-line option (and maybe an equivalent -device option in the future) to set CPU features. Signed-off-by: Eduardo Habkost <ehabkost@redhat.com> --- target-i386/cpu.c | 101 ++---------------------------------------------------- 1 file changed, 2 insertions(+), 99 deletions(-)