Re: [Qemu-devel] [PATCH v6 15/17] target-s390x: Extend arch specific QMP command query-cpu-definitions

[Eduardo Habkost]

On Mon, Apr 27, 2015 at 04:53:29PM +0200, Michael Mueller wrote:
[...]
>  #ifndef CONFIG_USER_ONLY
> +static CpuDefinitionInfoList *qmp_query_cpu_definition_host(void)
> +{
> +    CpuDefinitionInfoList *host = NULL;
> +    CpuDefinitionInfo *info;
> +
> +    info = g_try_new0(CpuDefinitionInfo, 1);
> +    if (!info) {
> +        goto out;
> +    }
> +    info->name = g_strdup("host");
> +
> +    host = g_try_new0(CpuDefinitionInfoList, 1);
> +    if (!host) {
> +        g_free(info->name);
> +        g_free(info);
> +        goto out;
> +    }
> +    host->value = info;
> +out:
> +    return host;
> +}
[...]
>  CpuDefinitionInfoList *arch_query_cpu_definitions(bool has_machine,
>                                                    const char *machine,
>                                                    bool has_accel,
>                                                    AccelId accel,
>                                                    Error **errp)
>  {
> -    CpuDefinitionInfoList *entry;
> -    CpuDefinitionInfo *info;
> +    S390MachineProps mach;
> +    GSList *classes;
> +    uint64_t *mask = NULL;
> +    CpuDefinitionInfoList *list = NULL;
> +
> +    if (has_machine) {
> +        mask = s390_fac_list_mask_by_machine(machine);
> +        if (!mask) {
> +            error_set(errp, QERR_INVALID_PARAMETER_VALUE, "machine",
> +                      "a valid machine type");
> +            return NULL;
> +        }
> +    }

I would like to understand better the meaning of "runnable" when machine
is omitted. Is it really possible to tell if a CPU model is runnable if
no machine/mask info is provided as input?

If machine is omitted and the command returns runnable=true, does that
mean the CPU model is runnable using any machine? Does it mean it is
runnable using some of the available machines? If so, which ones? Does
it mean something else?

>  
> -    info = g_malloc0(sizeof(*info));
> -    info->name = g_strdup("host");
> +    memset(&mach, 0, sizeof(mach));
> +    if (has_accel) {
> +        switch (accel) {
> +        case ACCEL_ID_KVM:
> +            kvm_s390_get_machine_props(NULL, &mach);
> +            break;
> +        default:
> +            return qmp_query_cpu_definition_host();

This will return only a single element. I don't think that's correct. If
machine or accel is omitted, I believe we should just omit the
"runnable" field, but always return the full list of CPU models.

> +        }
> +    }
>  
> -    entry = g_malloc0(sizeof(*entry));
> -    entry->value = info;
> +    s390_setup_cpu_classes(ACCEL_TEMP, &mach, mask);
> +
> +    classes = object_class_get_list(TYPE_S390_CPU, false);
> +    classes = g_slist_sort(classes, s390_cpu_class_asc_order_compare);
> +    g_slist_foreach(classes, qmp_query_cpu_definition_entry, &list);
> +    g_slist_free(classes);
>  
> -    return entry;
> +    return list;
>  }
>  #endif
>  
[...]

-- 
Eduardo