Patchwork [RFC,2/6] i386: kill cpudef config section support

login
register
mail settings
Submitter Eduardo Habkost
Date Aug. 17, 2012, 5:53 p.m.
Message ID <1345226022-21654-3-git-send-email-ehabkost@redhat.com>
Download mbox | patch
Permalink /patch/178287/
State New
Headers show

Comments

Eduardo Habkost - Aug. 17, 2012, 5:53 p.m.
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(-)
Andreas Färber - Aug. 20, 2012, 10:51 a.m.
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
Igor Mammedov - Aug. 20, 2012, noon
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
> 
>
Eduardo Habkost - Aug. 20, 2012, 12:30 p.m.
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
Igor Mammedov - Sept. 5, 2012, 9:09 a.m.
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,
Igor Mammedov - Sept. 5, 2012, 9:39 a.m.
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,
> 
> 
>
Eduardo Habkost - Sept. 6, 2012, 5:55 p.m.
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,
> > 
> > 
> > 
>

Patch

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,