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

["Philippe Mathieu-Daudé" <f4bug@amsat.org>]

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

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