Re: [Qemu-devel] [PATCH qom-cpu 3/4] target-ppc: Slim conversion of model definitions to QOM subclasses

[Eduardo Habkost]

On Tue, Dec 18, 2012 at 08:53:42AM +0100, Andreas Färber wrote:
> Since the model list is highly macrofied, keep ppc_def_t for now and
> save a pointer to it in PowerPCCPUClass. This results in a flat list of
> subclasses including aliases, to be refined later.
> 
> Move cpu_ppc_init() to translate_init.c and drop helper.c.
> Long-term the idea is to turn translate_init.c into a standalone cpu.c.
> 
> Inline cpu_ppc_usable() into type registration.
> 
> Split cpu_ppc_register() in two by code movement into the initfn and
> by turning the remaining part into a realizefn.
> Move qemu_init_vcpu() call into the new realizefn and adapt
> create_ppc_opcodes() to return an Error.
> 
> Change ppc_find_by_pvr() -> ppc_cpu_class_by_pvr().
> Change ppc_find_by_name() -> ppc_cpu_class_by_name().
> 
> Turn -cpu host into its own subclass. This requires to move the
> kvm_enabled() check in ppc_cpu_class_by_name() to avoid the class being
> found via the normal name lookup in the !kvm_enabled() case.
> Turn kvmppc_host_cpu_def() into the class_init and add an initfn that
> asserts KVM is in fact enabled.
> 
> Implement -cpu ? and the QMP equivalent in terms of subclasses.
> This newly exposes -cpu host to the user, ordered last for -cpu ?.
> 
> Signed-off-by: Andreas Färber <address@hidden>
[...]
> +static void kvmppc_host_cpu_class_init(ObjectClass *oc, void *data)
> +{
> +    PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
>      uint32_t host_pvr = mfpvr();
> -    const ppc_def_t *base_spec;
> +    PowerPCCPUClass *pvr_pcc;
>      ppc_def_t *spec;
>      uint32_t vmx = kvmppc_get_vmx();
>      uint32_t dfp = kvmppc_get_dfp();
>  
> -    base_spec = ppc_find_by_pvr(host_pvr);
> -
>      spec = g_malloc0(sizeof(*spec));
> -    memcpy(spec, base_spec, sizeof(*spec));
> +
> +    pvr_pcc = ppc_cpu_class_by_pvr(host_pvr);

I wonder if it's really safe to do this inside class_init.

ppc_cpu_class_by_pvr() calls object_class_get_list(TYPE_POWERPC_CPU),
and TYPE_HOST_POWERPC_CPU will be included in that list, but hasn't
finished its initialization yet.

I see that you took care of checking for TYPE_HOST_POWERPC_CPU inside
ppc_cpu_compare_class_pvr(). So, it looks like this is OK today, but I
am not 100% sure this doesn't break any assumptions/requirements of QOM.



> +    if (pvr_pcc != NULL) {
> +        memcpy(spec, pvr_pcc->info, sizeof(*spec));
> +    }
> +    pcc->info = spec;
> +    /* Override the display name for -cpu ? and QMP */
> +    pcc->info->name = "host";
>  
>      /* Now fix up the spec with information we can query from the host */
>  
> @@ -1208,8 +1219,6 @@ const ppc_def_t *kvmppc_host_cpu_def(void)
>          /* Only override when we know what the host supports */
>          alter_insns(&spec->insns_flags2, PPC2_DFP, dfp);
>      }
> -
> -    return spec;
>  }
>  
>  int kvmppc_fixup_cpu(CPUPPCState *env)
[...]
> +static void ppc_cpu_list_entry(gpointer data, gpointer user_data)
> +{
> +    ObjectClass *oc = data;
> +    CPUListState *s = user_data;
> +    PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> +
> +    (*s->cpu_fprintf)(s->file, "PowerPC %-16s PVR %08x\n",
> +                      pcc->info->name, pcc->info->pvr);
> +}
> +
> +void ppc_cpu_list(FILE *f, fprintf_function cpu_fprintf)
> +{
> +    CPUListState s = {
> +        .file = f,
> +        .cpu_fprintf = cpu_fprintf,
> +    };
> +    GSList *list;
> +
> +    list = object_class_get_list(TYPE_POWERPC_CPU, false);
> +    list = g_slist_sort(list, ppc_cpu_list_compare);
> +    g_slist_foreach(list, ppc_cpu_list_entry, &s);
> +    g_slist_free(list);
> +}
> +
> +static void ppc_cpu_defs_entry(gpointer data, gpointer user_data)
> +{
> +    ObjectClass *oc = data;
> +    CpuDefinitionInfoList **first = user_data;
> +    PowerPCCPUClass *pcc = POWERPC_CPU_CLASS(oc);
> +    CpuDefinitionInfoList *entry;
> +    CpuDefinitionInfo *info;
> +
> +    info = g_malloc0(sizeof(*info));
> +    info->name = g_strdup(pcc->info->name);
> +
> +    entry = g_malloc0(sizeof(*entry));
> +    entry->value = info;
> +    entry->next = *first;
> +    *first = entry;
>  }
>  
>  CpuDefinitionInfoList *arch_query_cpu_definitions(Error **errp)
>  {
>      CpuDefinitionInfoList *cpu_list = NULL;
> -    int i;
> +    GSList *list;
>  
> -    for (i = 0; i < ARRAY_SIZE(ppc_defs); i++) {
> -        CpuDefinitionInfoList *entry;
> -        CpuDefinitionInfo *info;
> +    list = object_class_get_list(TYPE_POWERPC_CPU, false);
> +    g_slist_foreach(list, ppc_cpu_defs_entry, &cpu_list);
> +    g_slist_free(list);
>  
> -        if (!ppc_cpu_usable(&ppc_defs[i])) {
> -            continue;
> -        }
> +    return cpu_list;
> +}

I still think we could reuse arch_query_cpu_definitions() for both
"-cpu ?" and "query-cpu-definitions". But, anyway: we can unify that one
step at a time. This at least matches the same pattern used in other
architectures.

[...]

I didn't review the ppc-specific code that got moved around. I will try
to take a deeper look today, but I hope this gets reviewed by somebody
with more experience dealing with the target-ppc code.

-- 
Eduardo