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 <afaerber@xxxxxxx>
[...]
> +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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
