Re: [Qemu-devel] [PATCH v1] s390x/cpu_models: Add "-cpu max" support

[Cornelia Huck <cohuck@redhat.com>]

On Wed, 25 Jul 2018 11:12:33 +0200
David Hildenbrand <david@redhat.com> 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

That's a z13 feature, so I think it's fine as we don't care about
machine generations for the max mode anyway, correct?

> - msa5-base

That's just the warning, but as things are continuing to work, it's
fine as well.

> - zpci

That one theoretically has a dependency on CONFIG_PCI, but as that is
always set, I think it's fine as well.

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

s/mdel/model/

> 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 <david@redhat.com>
> ---
>  target/s390x/cpu_models.c | 81 +++++++++++++++++++++++++++------------
>  1 file changed, 56 insertions(+), 25 deletions(-)
> 

> +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;
> +    }
> +
> +    max_model = get_max_cpu_model(&local_err);
> +    if (local_err) {
> +        g_assert(kvm_enabled());

Maybe add a comment that for kvm we try the host model, and only that
can fail (i.e., for tcg this will always work)?

> +        error_report_err(local_err);
> +        /* fallback to unsupported CPU models */
> +        return;
> +    }
> +
> +    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);

Looks sane to me.