Re: [Qemu-devel] [PATCH RFC 2/6] target-i386: add cpu-model property to x86_cpu

[Igor Mammedov]

On 04/18/2012 12:03 PM, Andreas Färber wrote:
> Am 17.04.2012 01:36, schrieb Igor Mammedov:
> > From: Igor Mammedov<address@hidden>
> >
> > Signed-off-by: Igor Mammedov<address@hidden>
> > ---
> >   cpu-defs.h           |    2 +-
> >   hw/pc.c              |   10 ----------
> >   target-i386/cpu.c    |   33 +++++++++++++++++++++++++++++++++
> >   target-i386/helper.c |   12 ++++++++----
> >   4 files changed, 42 insertions(+), 15 deletions(-)
> >
> > diff --git a/cpu-defs.h b/cpu-defs.h
> > index 3268968..8f1caaf 100644
> > --- a/cpu-defs.h
> > +++ b/cpu-defs.h
> > @@ -221,7 +221,7 @@ typedef struct CPUWatchpoint {
> >       struct QemuCond *halt_cond;                                         \
> >       int thread_kicked;                                                  \
> >       struct qemu_work_item *queued_work_first, *queued_work_last;        \
> > -    const char *cpu_model_str;                                          \
> > +    char *cpu_model_str;                                          \
>
> Breaks alignment of \.
>
> >       struct KVMState *kvm_state;                                         \
> >       struct kvm_run *kvm_run;                                            \
> >       int kvm_fd;                                                         \
> > diff --git a/hw/pc.c b/hw/pc.c
> > index d2c122e..7f0de99 100644
> > --- a/hw/pc.c
> > +++ b/hw/pc.c
> > @@ -931,7 +931,6 @@ static CPUX86State *pc_new_cpu(const char *cpu_model)
> >
> >       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) {
> > @@ -950,15 +949,6 @@ void pc_cpus_init(const char *cpu_model)
> >   {
> >       int i;
> >
> > -    /* init CPUs */
> > -    if (cpu_model == NULL) {
> > -#ifdef TARGET_X86_64
> > -        cpu_model = "qemu64";
> > -#else
> > -        cpu_model = "qemu32";
> > -#endif
> > -    }
> > -
> >       for(i = 0; i<  smp_cpus; i++) {
> >           pc_new_cpu(cpu_model);
> >       }
> > diff --git a/target-i386/cpu.c b/target-i386/cpu.c
> > index e12c851..30ae0c2 100644
> > --- a/target-i386/cpu.c
> > +++ b/target-i386/cpu.c
> > @@ -29,6 +29,8 @@
> >
> >   #include "hyperv.h"
> >
> > +#include "qerror.h"
> > +
> >   /* feature flags taken from "Intel Processor Identification and the CPUID
> >    * Instruction" and AMD's "CPUID Specification".  In cases of disagreement
> >    * between feature naming conventions, aliases may be added.
> > @@ -1468,6 +1470,27 @@ static void mce_init(X86CPU *cpu)
> >       }
> >   }
> >
> > +static char *x86_get_cpu_model(Object *obj, Error **errp)
> > +{
> > +    X86CPU *cpu = X86_CPU(obj);
> > +    CPUX86State *env =&cpu->env;
> > +    return g_strdup(env->cpu_model_str);
> > +}
> > +
> > +static void x86_set_cpu_model(Object *obj, const char *value, Error **errp)
> > +{
> > +    X86CPU *cpu = X86_CPU(obj);
> > +    CPUX86State *env =&cpu->env;
> > +
> > +    g_free((gpointer)env->cpu_model_str);
> > +    env->cpu_model_str = g_strdup(value);
> > +
> > +    if (cpu_x86_register(env, env->cpu_model_str)<  0) {
> > +        fprintf(stderr, "Unable to find x86 CPU definition\n");
> > +        error_set(errp, QERR_INVALID_PARAMETER_COMBINATION);
> > +    }
> > +}
>
> This will cease to work when we model CPU definitions as subclasses - in
> that case we cannot change the type after instantiating it. No one
> objected to that aspect of the RFC so far, so I was planning to post
> that as "part 3" of my series.
subclass will model only a fixed set of features, but we might wish to override
(add/remove) features provided by it. Now we can do it by adding +/-FEATURE
to cpu_model string. So we need somehow parse features list from
cpu_model string, we've got from cmdline

> We might inspect the model string (typename plus read-only - question is
> whether that is useful (cf. CPU feature flag modelling).
>
> Andreas
>
> > +
> >   static void x86_cpu_initfn(Object *obj)
> >   {
> >       X86CPU *cpu = X86_CPU(obj);
> > @@ -1475,6 +1498,16 @@ static void x86_cpu_initfn(Object *obj)
> >
> >       cpu_exec_init(env);
> >       env->cpuid_apic_id = env->cpu_index;
> > +
> > +    object_property_add_str(obj, "cpu-model",
> > +        x86_get_cpu_model, x86_set_cpu_model, NULL);
> > +
> > +#ifdef TARGET_X86_64
> > +    object_property_set_str(OBJECT(cpu), "qemu64", "cpu-model", NULL);
> > +#else
> > +    object_property_set_str(OBJECT(cpu), "qemu32", "cpu-model", NULL);
> > +#endif
> > +
> >       mce_init(cpu);
> >   }
> >
> > diff --git a/target-i386/helper.c b/target-i386/helper.c
> > index d92d3d4..df33d83 100644
> > --- a/target-i386/helper.c
> > +++ b/target-i386/helper.c
> > @@ -1161,11 +1161,11 @@ CPUX86State *cpu_x86_init(const char *cpu_model)
> >   {
> >       X86CPU *cpu;
> >       CPUX86State *env;
> > +    Error *errp = NULL;
> >       static int inited;
> >
> >       cpu = X86_CPU(object_new(TYPE_X86_CPU));
> >       env =&cpu->env;
> > -    env->cpu_model_str = cpu_model;
> >
> >       /* init various static tables used in TCG mode */
> >       if (tcg_enabled()&&  !inited) {
> > @@ -1176,9 +1176,13 @@ CPUX86State *cpu_x86_init(const char *cpu_model)
> >               cpu_set_debug_excp_handler(breakpoint_handler);
> >   #endif
> >       }
> > -    if (cpu_x86_register(env, cpu_model)<  0) {
> > -        object_delete(OBJECT(cpu));
> > -        return NULL;
> > +
> > +    if (cpu_model) {
> > +        object_property_set_str(OBJECT(cpu), cpu_model, "cpu-model",&errp);
> > +        if (errp) {
> > +            object_delete(OBJECT(cpu));
> > +            return NULL;
> > +        }
> >       }
> >
> >       qemu_init_vcpu(env);

--
-----
 Igor