Re: [PATCH v1 2/2] s390x/cpumodel: Introduce "best" model variants

[David Hildenbrand]

On 08.11.19 20:51, Eduardo Habkost wrote:
> On Fri, Nov 08, 2019 at 12:07:14PM +0100, David Hildenbrand wrote:
> > For a specific CPU model, we have a lot of feature variability depending on
> > - The microcode version of the HW
> > - The hypervisor we're running on (LPAR vs. KVM vs. z/VM)
> > - The hypervisor version we're running on
> > - The KVM version
> > - KVM module parameters (especially, "nested=1")
> > - The accelerator
> >
> > Our default models are migration safe, however can only be changed
> > between QEMU releases (glued to QEMU machine). This somewhat collides
> > with the feature variability we have. E.g., the z13 model will not run
> > under TCG. There is the demand from higher levels in the stack to "have the
> > best CPU model possible on a given accelerator, firmware and HW", which
> > should especially include all features that fix security issues.
> > Especially, if we have a new feature due to a security flaw, we want to
> > have a way to backport this feature to older QEMU versions and a way to
> > automatically enable it when asked.
> >
> > This is where "best" CPU models come into play. If upper layers specify
> > "z14-best" on a z14, they will get the best possible feature set in that
> > configuration. "best" usually means "maximum features", besides deprecated
> > features. This will then, for example, include nested virtualization
> > ("SIE" feature) when KVM+HW support is enabled, or fixes via
> > microcode updates (e.g., spectre)
> >
> > "best" models are not migration safe. Upper layers can expand these
> > models to migration-safe and static variants, allowing them to be
> > migrated.
> >
> > Signed-off-by: David Hildenbrand <address@hidden>
>
> Makes sense to me, and the code looks good.  I just have one
> question below:
>
> > ---
> [...]
> > +static void s390_best_cpu_model_initfn(Object *obj)
> > +{
> > +    const S390CPUModel *max_model;
> > +    S390CPU *cpu = S390_CPU(obj);
> > +    S390CPUClass *xcc = S390_CPU_GET_CLASS(cpu);
> > +    Error *local_err = NULL;
> > +    int i;
> > +
> > +    if (kvm_enabled() && !kvm_s390_cpu_models_supported()) {
> > +        return;
> > +    }
> > +
> > +    max_model = get_max_cpu_model(&local_err);
> > +    if (local_err) {
> > +        /* we expect errors only under KVM, when actually querying the kernel 
> > */
> > +        g_assert(kvm_enabled());
> > +        error_report_err(local_err);
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * Similar to baselining against the "max" model. However, features
> > +     * are handled differently and are not used for the search for a 
> > definition.
> > +     */
> > +    if (xcc->cpu_def->gen == max_model->def->gen) {
> > +        if (xcc->cpu_def->ec_ga > max_model->def->ec_ga) {
> > +            return;
> > +        }
> > +    } else if (xcc->cpu_def->gen > max_model->def->gen) {
> > +        return;
> > +    }
>
> What exactly is expected to happen if we return from the function
> here?

cpu->model is NULL. That fact (and xcc->is_best) is checked when the 
model is to be used (e.g., via qmp or when creating VCPUs), and a rather 
generic error is reported. This is suboptimal and ...

> (In x86, we worked around the inability to report errors inside
> instance_init by adding another step to CPU object initialization
> called "CPU expansion", implemented by
> x86_cpu_expand_features().)

... doing something like that makes a lot of sense. We also have to 
rework this for the "host" and "max" model.

I'll look into that when I'm back from holidays in one week.

Thanks!

--

Thanks,

David / dhildenb