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

[Blue Swirl <blauwirbel@gmail.com>]

On Fri, Mar 23, 2012 at 17:27, Andreas Färber <afaerber@suse.de> wrote:
> Am 14.03.2012 21:16, schrieb Blue Swirl:
>> On Wed, Mar 14, 2012 at 17:53, Andreas Färber <afaerber@suse.de> 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.

I see, this is only for the sparc_def_t structure.

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

No, the patch is fine as is.

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