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

[Igor Mammedov <imammedo@redhat.com>]

On Fri, 18 Aug 2017 16:00:37 -0300
Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:

> Hi Igor,
> 
> 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 <imammedo@redhat.com>
> > ---
> > 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 <mark.cave-ayland@ilande.co.uk>
> > CC: Artyom Tarasenko <atar4qemu@gmail.com>
> > CC: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > 
> > v2:
> >    * make base class abstract (Philippe Mathieu-Daudé <f4bug@amsat.org>)
> > ---
> >   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

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)
...