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

[Michael Mueller <mimu@linux.vnet.ibm.com>]

On Wed, 6 May 2015 09:37:41 -0300
Eduardo Habkost <ehabkost@redhat.com> wrote:

> 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?

Yes it is. The list of runnable CPU models is derived from the set of S390 CPU
facilities available. One subset of facilities depends on the accelerator and
the second on the capabilities of QEMU which is represented by the mask. In the
case the no machine is specified only those of the accelerator are taken into
account for the calculation.

It is even possible to omit both, the accelerator and the machine, in that case
only the model none is runnable and becomes the default: 

{"return":[{"order":3345,"name":"2964-ga1","live-migration-safe":true},{"name":"zBC12"},
{"name":"2828"},{"order":3105,"name":"2828-ga1","live-migration-safe":true},{"name":"host"},
{"name":"zEC12"},{"name":"2827"},{"order":3090,"name":"2827-ga2","live-migration-safe":true},
{"order":3089,"name":"2827-ga1","live-migration-safe":true},{"name":"z114"},{"name":"2818"},
{"order":2849,"name":"2818-ga1","live-migration-safe":true},{"name":"z196"},{"name":"2817"},
{"order":2834,"name":"2817-ga2","live-migration-safe":true},{"order":2833,"name":"2817-ga1",
"live-migration-safe":true},{"name":"z10-bc"},{"name":"2098"},{"order":2594,"name":"2098-ga2",
"live-migration-safe":true},{"order":2593,"name":"2098-ga1","live-migration-safe":true},
{"name":"z10-ec"},{"name":"z10"},{"name":"2097"},{"order":2579,"name":"2097-ga3",
"live-migration-safe":true},{"order":2578,"name":"2097-ga2","live-migration-safe":true},
{"order":2577,"name":"2097-ga1","live-migration-safe":true},{"name":"z9-bc"},{"name":"2096"},
{"order":2338,"name":"2096-ga2","live-migration-safe":true},{"order":2337,"name":"2096-ga1",
"live-migration-safe":true},{"name":"z9-ec"},{"name":"z9"},{"name":"2094"},{"order":2323,
"name":"2094-ga3","live-migration-safe":true},{"order":2322,"name":"2094-ga2",
"live-migration-safe":true},{"name":"z9-109"},{"order":2321,"name":"2094-ga1",
"live-migration-safe":true},{"name":"z890"},{"name":"2086"},{"order":2083,"name":"2086-ga3",
"live-migration-safe":true},{"order":2082,"name":"2086-ga2","live-migration-safe":true},
{"order":2081,"name":"2086-ga1","live-migration-safe":true},{"name":"z990"},{"name":"2084"},
{"order":2069,"name":"2084-ga5","live-migration-safe":true},{"order":2068,"name":"2084-ga4",
"live-migration-safe":true},{"order":2067,"name":"2084-ga3","live-migration-safe":true},
{"order":2066,"name":"2084-ga2","live-migration-safe":true},{"order":2065,"name":"2084-ga1",
"live-migration-safe":true},{"name":"z800"},{"name":"2066"},{"order":1825,"name":"2066-ga1",
"live-migration-safe":true},{"name":"z900"},{"name":"2064"},{"order":1811,"name":"2064-ga3",
"live-migration-safe":true},{"order":1810,"name":"2064-ga2","live-migration-safe":true},
{"order":1809,"name":"2064-ga1","live-migration-safe":true},{"name":"none","runnable":true,
"is-default":true}],"id":"libvirt-15"}

> 
> 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?

As it is a lower limit all machines shall be able to run it. Although a somewhat
clever management interface should iterate through all accel/machine combinations. 

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

That is the !KVM case where I keep the behavior currently unchanged to
the existing implementation. But right for the TCG case there has to be
a comparable get_machine_props() call returning whatever TCG is implementing.
But that is not part of this patch series. 

> 
> > +        }
> > +    }
> >  
> > -    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
> >  
> [...]
>