Re: [Qemu-devel] [PATCH for-2.11 01/27] sparc: convert cpu models to SPARC cpu subclasses

[Philippe Mathieu-Daudé]

> > On 08/18/2017 07:08 AM, Igor Mammedov wrote:
> > > QOMfy cpu models handling introducing propper cpu types
> > > for each cpu model.
> > >
> > > Signed-off-by: Igor Mammedov <address@hidden>
> > > ---
> > > with this and conversion of features to properties,
> > > it would be possible to replace cpu_sparc_init() with
> > > cpu_generic_init() and reuse common -cpu handling
> > > infrastructure.
> > >
> > > CC: Mark Cave-Ayland <address@hidden>
> > > CC: Artyom Tarasenko <address@hidden>
> > > CC: Philippe Mathieu-Daudé <address@hidden>
> > >
> > > v2:
> > >     * make base class abstract (Philippe Mathieu-Daudé <address@hidden>)
> > > ---
> > >    target/sparc/cpu-qom.h |   2 +
> > >    target/sparc/cpu.c     | 121 
> > > +++++++++++++++++++++++++++++++++----------------
> > >    2 files changed, 84 insertions(+), 39 deletions(-)
> > >
> > > diff --git a/target/sparc/cpu-qom.h b/target/sparc/cpu-qom.h
> > > index f63af72..af6d57a 100644
> > > --- a/target/sparc/cpu-qom.h
> > > +++ b/target/sparc/cpu-qom.h
> > > @@ -35,6 +35,7 @@
> > >    #define SPARC_CPU_GET_CLASS(obj) \
> > >        OBJECT_GET_CLASS(SPARCCPUClass, (obj), TYPE_SPARC_CPU)
> > >    
> > > +typedef struct sparc_def_t sparc_def_t;
> > >    /**
> > >     * SPARCCPUClass:
> > >     * @parent_realize: The parent class' realize handler.
> > > @@ -49,6 +50,7 @@ typedef struct SPARCCPUClass {
> > >    
> > >        DeviceRealize parent_realize;
> > >        void (*parent_reset)(CPUState *cpu);
> > > +    sparc_def_t *cpu_def;
> > >    } SPARCCPUClass;
> > >    
> > >    typedef struct SPARCCPU SPARCCPU;
> > > diff --git a/target/sparc/cpu.c b/target/sparc/cpu.c
> > > index d606eb5..2994c09 100644
> > > --- a/target/sparc/cpu.c
> > > +++ b/target/sparc/cpu.c
> > > @@ -25,8 +25,6 @@
> > >    
> > >    //#define DEBUG_FEATURES
> > >    
> > > -static int cpu_sparc_find_by_name(sparc_def_t *cpu_def, const char *cpu_model);
> > > -
> > >    /* CPUClass::reset() */
> > >    static void sparc_cpu_reset(CPUState *s)
> > >    {
> > > @@ -111,17 +109,9 @@ static int cpu_sparc_register(SPARCCPU *cpu, const char 
> > > *cpu_model)
> > >    {
> > >        CPUSPARCState *env = &cpu->env;
> > >        char *s = g_strdup(cpu_model);
> > > -    char *featurestr, *name = strtok(s, ",");
> > > -    sparc_def_t def1, *def = &def1;
> > > +    char *featurestr = strtok(s, ",");
> > >        Error *err = NULL;
> > >    
> > > -    if (cpu_sparc_find_by_name(def, name) < 0) {
> > > -        g_free(s);
> > > -        return -1;
> > > -    }
> > > -
> > > -    env->def = g_memdup(def, sizeof(*def));
> > > -
> > >        featurestr = strtok(NULL, ",");
> > >        sparc_cpu_parse_features(CPU(cpu), featurestr, &err);
> > >        g_free(s);
> > > @@ -130,18 +120,18 @@ static int cpu_sparc_register(SPARCCPU *cpu, const char 
> > > *cpu_model)
> > >            return -1;
> > >        }
> > >    
> > > -    env->version = def->iu_version;
> > > -    env->fsr = def->fpu_version;
> > > -    env->nwindows = def->nwindows;
> > > +    env->version = env->def->iu_version;
> > > +    env->fsr = env->def->fpu_version;
> > > +    env->nwindows = env->def->nwindows;
> > >    #if !defined(TARGET_SPARC64)
> > > -    env->mmuregs[0] |= def->mmu_version;
> > > +    env->mmuregs[0] |= env->def->mmu_version;
> > >        cpu_sparc_set_id(env, 0);
> > > -    env->mxccregs[7] |= def->mxcc_version;
> > > +    env->mxccregs[7] |= env->def->mxcc_version;
> > >    #else
> > > -    env->mmu_version = def->mmu_version;
> > > -    env->maxtl = def->maxtl;
> > > -    env->version |= def->maxtl << 8;
> > > -    env->version |= def->nwindows - 1;
> > > +    env->mmu_version = env->def->mmu_version;
> > > +    env->maxtl = env->def->maxtl;
> > > +    env->version |= env->def->maxtl << 8;
> > > +    env->version |= env->def->nwindows - 1;
> > >    #endif
> > >        return 0;
> > >    }
> > > @@ -149,8 +139,19 @@ static int cpu_sparc_register(SPARCCPU *cpu, const char 
> > > *cpu_model)
> > >    SPARCCPU *cpu_sparc_init(const char *cpu_model)
> > >    {
> > >        SPARCCPU *cpu;
> > > +    ObjectClass *oc;
> > > +    char *str, *name;
> > > +
> > > +    str = g_strdup(cpu_model);
> > > +    name = strtok(str, ",");
> >
> > you can free 'str' once here:
> >
> >          g_free(str);
> >
> > > +    oc = cpu_class_by_name(TYPE_SPARC_CPU, name);
> > > +    if (oc == NULL) {
> > > +        g_free(str);
> >
> > drop
> >
> > > +        return NULL;
> > > +    }
> > > +    g_free(str);
> >
> > drop
>
> I'll fix it up if I have to respin series (note this function is removed within 
> series)
>
> [...]
>   
>
> > Anyway I can't start Aurelien's image [1] with this commit:
> Can't reproduce it locally (either dirty/clean tree rebuild)
> with ./configure --enable-debug
>
>   
> > $ sparc-softmmu/qemu-system-sparc -hda debian_etch_sparc_small.qcow2
> > qemu: Unable to find Sparc CPU definition
> for this kind of error guest image doesn't matter as error happens during 
> machine_init()
>
> > [1] https://people.debian.org/~aurel32/qemu/sparc/
> It boots just fine with default CPU model:
>
> ./sparc-softmmu/qemu-system-sparc -hda debian_etch_sparc_small.qcow2

Sorry I'v been distracted by something else :/
I might have done something wrong, I'll test it again tomorrow.

> and with explicitly specified one:
>
> sparc-softmmu/qemu-system-sparc -cpu "Fujitsu MB86904"
>
> I get from "info qom-tree"
> ...
>   /device[0] (Fujitsu-MB86904-sparc-cpu)
> ...