Re: [Qemu-devel] [RFC 11/12] target-sparc: QOM'ify CPU

[Andreas Färber]

Am 14.03.2012 21:16, schrieb Blue Swirl:
> On Wed, Mar 14, 2012 at 17:53, Andreas Färber <address@hidden> wrote:
>> diff --git a/target-sparc/cpu-qom.h b/target-sparc/cpu-qom.h
>> new file mode 100644
>> index 0000000..15dcf84
>> --- /dev/null
>> +++ b/target-sparc/cpu-qom.h
[...]
>> +/**
>> + * SPARCCPUClass:
>> + * @parent_reset: The parent class' reset handler.
>> + *
>> + * A SPARC CPU model.
>> + */
>> +typedef struct SPARCCPUClass {
>> +    /*< private >*/
>> +    CPUClass parent_class;
>> +    /*< public >*/
>> +
>> +    void (*parent_reset)(CPUState *cpu);
>> +
>> +    target_ulong iu_version;
>> +    uint32_t fpu_version;
>> +    uint32_t mmu_version;
>> +    uint32_t mmu_bm;
>> +    uint32_t mmu_ctpr_mask;
>> +    uint32_t mmu_cxr_mask;
>> +    uint32_t mmu_sfsr_mask;
>> +    uint32_t mmu_trcr_mask;
>> +    uint32_t mxcc_version;
>> +    uint32_t features;
>> +    uint32_t nwindows;
>> +    uint32_t maxtl;
>> +} SPARCCPUClass;
>> +
>> +/**
>> + * SPARCCPU:
>> + * @env: Legacy CPU state.
>> + *
>> + * A SPARC CPU.
>> + */
>> +typedef struct SPARCCPU {
>> +    /*< private >*/
>> +    CPUState parent_obj;
>> +    /*< public >*/
>> +
>> +    CPUSPARCState env;
>> +
>> +    target_ulong iu_version;
>> +    uint32_t fpu_version;
>> +    uint32_t mmu_version;
>> +    uint32_t features;
>> +    uint32_t nwindows;
>> +} SPARCCPU;
> 
> The fields do not look correct at all, the same fields are in both
> structs.

Formerly you had the model of an array of sparc_def_t structs, which you
would duplicate and then associate with CPUSPARCState, modifying the
duplicate.
SPARCCPUClass exists only once though. Therefore we cannot modify
classes based on command line parameters and must do so on the instance
instead. I have therefore duplicated some fields from class to instance
as you have noticed, initialized the object's value from the class' and
let any -cpu options modify the latter.
The same pattern has been used for arm and i386. On arm my v5 postpones
this by doing a bare-bones conversion for now; on x86 the only request
so far was to set the values via QOM properties.

> Moreover Sparc32 and Sparc64 fields are mixed. Maybe I don't
> fully understand the conversion.

Mixed fields likely means they were mixed in your original code. It is a
mostly mechanical conversion.

> Would it be possible to make a common parent class which is then
> specialized by Sparc32 and Sparc64 classes? There are many common
> fields but also many 32/64 specific ones. Also cpu_common.c, cpu32.c
> and cpu64.c to avoid #ifdeffery?

That is possible, but I would ask that you do such split-ups later.
Discussions for arm and ppc have shown that the structure of subclasses
in lack of multi-inheritence can be a tricky and controversial issue and
requires a lot of target knowledge that I do not posses for sparc.
Also, restructuring target code, e.g., into multiple files is orthogonal
to the goal of QOM'ifying all target CPUs and doing cleanups in common code.

If you think this is heading into a totally wrong direction due to some
in-progress work of yours, we could strip it down like target-arm v5,
but ATM I don't believe so. ;)

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg