Re: [Qemu-devel] [RFC 05/12] target-i386: QOM'ify CPU

[Eduardo Habkost]

On Wed, Mar 14, 2012 at 06:53:29PM +0100, Andreas Färber wrote:
[...]
> +/**
> + * X86CPUClass:
> + * @parent_reset: The parent class' reset handler.
> + *
> + * An x86 CPU model or family.
> + */
> +typedef struct X86CPUClass {
> +    /*< private >*/
> +    CPUClass parent_class;
> +    /*< public >*/
> +
> +    void (*parent_reset)(CPUState *cpu);
> +
> +    uint32_t level;
> +    uint32_t vendor1, vendor2, vendor3;
> +    int family;
> +    int model;
> +    int stepping;
> +    int tsc_khz;
> +    uint32_t features, ext_features, ext2_features, ext3_features;
> +    uint32_t kvm_features, svm_features;
> +    uint32_t xlevel;
> +    char model_id[48];
> +    int vendor_override;
> +    uint32_t flags;
> +    /* Store the results of Centaur's CPUID instructions */
> +    uint32_t ext4_features;
> +    uint32_t xlevel2;
> +} X86CPUClass;
[...]
> +typedef struct X86CPUInfo {
> +    const char *name;
> +    uint32_t level;
> +    uint32_t vendor1, vendor2, vendor3;
> +    int family;
> +    int model;
> +    int stepping;
> +    int tsc_khz;
> +    uint32_t features, ext_features, ext2_features, ext3_features;
> +    uint32_t kvm_features, svm_features;
> +    uint32_t xlevel;
> +    char model_id[48];
> +    int vendor_override;
> +    uint32_t flags;
> +    /* Store the results of Centaur's CPUID instructions */
> +    uint32_t ext4_features;
> +    uint32_t xlevel2;
> +} X86CPUInfo;

Have you considered eliminating this duplication and using a common
struct for both cases? (either by using X86CPUClass for everything, or
by embedding a common struct inside X86CPUClass)

This would simplify (or even make unnecessary) the field-by-field copy
on x86_cpu_class_init().

[...]
> +static void x86_cpu_class_init(ObjectClass *klass, void *data)
> +{
> +    X86CPUClass *k = X86_CPU_CLASS(klass);
> +    const X86CPUInfo *info = data;
> +
> +    k->level = info->level;
> +    k->vendor1 = info->vendor1;
> +    k->vendor2 = info->vendor2;
> +    k->vendor3 = info->vendor3;
> +    k->family = info->family;
> +    k->model = info->model;
> +    k->stepping = info->stepping;
> +    k->tsc_khz = info->tsc_khz;
> +    k->features = info->features;
> +    k->ext_features = info->ext_features;
> +    k->ext2_features = info->ext2_features;
> +    k->ext3_features = info->ext3_features;
> +    k->kvm_features = info->kvm_features;
> +    k->svm_features = info->svm_features;
> +    k->xlevel = info->xlevel;
> +    memcpy(k->model_id, info->model_id, 48);
> +    k->vendor_override = info->vendor_override;
> +    k->flags = info->flags;
> +    k->ext4_features = info->ext4_features;
> +    k->xlevel2 = info->xlevel2;
> +}

-- 
Eduardo