On Wed, Jul 25, 2018 at 11:12:33AM +0200, David Hildenbrand wrote:
> The "max" CPU model behaves like "-cpu host" when KVM is enabled, and like
> a CPU with the maximum possible feature set when TCG is enabled.
>
> While the "host" model can not be used under TCG ("kvm_required"), the
> "max" model can and "Enables all features supported by the accelerator in
> the current host".
>
> So we can treat "host" just as a special case of "max" (like x86 does).
> It differs to the "qemu" CPU model under TCG such that compatibility
> handling will not be performed and that some experimental CPU features
> not yet part of the "qemu" model might be indicated.
>
> These are right now under TCG (see "qemu_MAX"):
> - stfle53
> - msa5-base
> - zpci
>
> This will result right now in the following warning when starting QEMU TCG
> with the "max" model:
> "qemu-system-s390x: warning: 'msa5-base' requires 'kimd-sha-512'."
>
> The "qemu" model (used as default in QEMU under TCG) will continue to
> work without such warnings. The "max" mdel in the current form
> might be interesting for kvm-unit-tests (where we would e.g. now also
> test "msa5-base").
>
> The "max" model is neither static nor migration safe (like the "host"
> model). It is independent of the machine but dependends on the accelerator.
> It can be used to detect the maximum CPU model also under TCG from upper
> layers without having to care about CPU model names for CPU model
> expansion.
>
> Signed-off-by: David Hildenbrand <da...@redhat.com>
> ---
> target/s390x/cpu_models.c | 81 +++++++++++++++++++++++++++------------
> 1 file changed, 56 insertions(+), 25 deletions(-)
>
> diff --git a/target/s390x/cpu_models.c b/target/s390x/cpu_models.c
> index 604898a882..708bf0e3ba 100644
> --- a/target/s390x/cpu_models.c
> +++ b/target/s390x/cpu_models.c
> @@ -307,7 +307,10 @@ static gint s390_cpu_list_compare(gconstpointer a,
> gconstpointer b)
> const char *name_a = object_class_get_name((ObjectClass *)a);
> const char *name_b = object_class_get_name((ObjectClass *)b);
>
> - /* move qemu and host to the top of the list, qemu first, host second */
> + /*
> + * Move qemu, host and max to the top of the list, qemu first, host
> second,
> + * max third.
> + */
> if (name_a[0] == 'q') {
> return -1;
> } else if (name_b[0] == 'q') {
> @@ -316,6 +319,10 @@ static gint s390_cpu_list_compare(gconstpointer a,
> gconstpointer b)
> return -1;
> } else if (name_b[0] == 'h') {
> return 1;
> + } else if (name_a[0] == 'm') {
> + return -1;
> + } else if (name_b[0] == 'm') {
> + return 1;
> }
Isn't it simpler to add a S390CPUClass::ordering field? See
x86_cpu_list_compare() for an example.
>
> /* keep the same order we have in our table (sorted by release date) */
> @@ -1077,27 +1084,6 @@ static void s390_cpu_model_initfn(Object *obj)
> }
> }
>
> -#ifdef CONFIG_KVM
> -static void s390_host_cpu_model_initfn(Object *obj)
> -{
> - S390CPU *cpu = S390_CPU(obj);
> - Error *err = NULL;
> -
> - if (!kvm_enabled() || !kvm_s390_cpu_models_supported()) {
> - return;
> - }
> -
> - cpu->model = g_malloc0(sizeof(*cpu->model));
> - kvm_s390_get_host_cpu_model(cpu->model, &err);
> - if (err) {
> - error_report_err(err);
> - g_free(cpu->model);
> - /* fallback to unsupported cpu models */
> - cpu->model = NULL;
> - }
> -}
> -#endif
> -
> static S390CPUDef s390_qemu_cpu_def;
> static S390CPUModel s390_qemu_cpu_model;
>
> @@ -1136,6 +1122,30 @@ static void s390_qemu_cpu_model_initfn(Object *obj)
> memcpy(cpu->model, &s390_qemu_cpu_model, sizeof(*cpu->model));
> }
>
> +static void s390_max_cpu_model_initfn(Object *obj)
> +{
> + const S390CPUModel *max_model;
> + S390CPU *cpu = S390_CPU(obj);
> + Error *local_err = NULL;
> +
> + if (kvm_enabled() && !kvm_s390_cpu_models_supported()) {
> + /* "max" and "host" always work, even without CPU model support */
> + return;
> + }
What's the use case that requires this check to be here?
What do you expect 'query-cpu-model-expansion model=max' to
return if !kvm_s390_cpu_models_supported()?
> +
> + max_model = get_max_cpu_model(&local_err);
I've just confirmed that get_max_cpu_model() is already ready to
work with TCG.
> + if (local_err) {
> + g_assert(kvm_enabled());
> + error_report_err(local_err);
> + /* fallback to unsupported CPU models */
> + return;
What about moving this outside instance_init?
On x86 we have a x86_cpu_expand_features() function to allow us
to handle CPU model expansion errors more gracefully.
None of my comments are about new code, but existing code from
"host", so:
Reviewed-by: Eduardo Habkost <ehabk...@redhat.com>
> + }
> +
> + cpu->model = g_new(S390CPUModel, 1);
> + /* copy the CPU model so we can modify it */
> + memcpy(cpu->model, max_model, sizeof(*cpu->model));
> +}
> +
> static void s390_cpu_model_finalize(Object *obj)
> {
> S390CPU *cpu = S390_CPU(obj);
> @@ -1209,6 +1219,20 @@ static void s390_qemu_cpu_model_class_init(ObjectClass
> *oc, void *data)
> qemu_hw_version());
> }
>
> +static void s390_max_cpu_model_class_init(ObjectClass *oc, void *data)
> +{
> + S390CPUClass *xcc = S390_CPU_CLASS(oc);
> +
> + /*
> + * The "max" model is neither static nor migration safe. Under KVM
> + * it represents the "host" model. Under TCG it represents some kind of
> + * "qemu" CPU model without compat handling and maybe with some
> additional
> + * CPU features that are not yet unlocked in the "qemu" model.
> + */
> + xcc->desc =
> + "Enables all features supported by the accelerator in the current
> host";
> +}
> +
> /* Generate type name for a cpu model. Caller has to free the string. */
> static char *s390_cpu_type_name(const char *model_name)
> {
> @@ -1239,12 +1263,18 @@ static const TypeInfo qemu_s390_cpu_type_info = {
> .class_init = s390_qemu_cpu_model_class_init,
> };
>
> +static const TypeInfo max_s390_cpu_type_info = {
> + .name = S390_CPU_TYPE_NAME("max"),
> + .parent = TYPE_S390_CPU,
> + .instance_init = s390_max_cpu_model_initfn,
> + .instance_finalize = s390_cpu_model_finalize,
> + .class_init = s390_max_cpu_model_class_init,
> +};
> +
> #ifdef CONFIG_KVM
> static const TypeInfo host_s390_cpu_type_info = {
> .name = S390_CPU_TYPE_NAME("host"),
> - .parent = TYPE_S390_CPU,
> - .instance_init = s390_host_cpu_model_initfn,
> - .instance_finalize = s390_cpu_model_finalize,
> + .parent = S390_CPU_TYPE_NAME("max"),
> .class_init = s390_host_cpu_model_class_init,
> };
> #endif
> @@ -1326,6 +1356,7 @@ static void register_types(void)
> }
>
> type_register_static(&qemu_s390_cpu_type_info);
> + type_register_static(&max_s390_cpu_type_info);
> #ifdef CONFIG_KVM
> type_register_static(&host_s390_cpu_type_info);
> #endif
> --
> 2.17.1
>
--
Eduardo
