Re: [RFC v1 09/10] i386: split cpu.c and defer x86 models registration

[Eduardo Habkost]

On Tue, Nov 10, 2020 at 06:38:49PM +0100, Claudio Fontana wrote:
> On 11/10/20 4:23 PM, Eduardo Habkost wrote:
> > On Tue, Nov 10, 2020 at 11:41:46AM +0100, Paolo Bonzini wrote:
> >> On 10/11/20 11:04, Daniel P. Berrangé wrote:
> >>>
> >>> ie, we should have one class hierarchy for CPU model definitions, and
> >>> one class hierarchy  for accelerator CPU implementations.
> >>>
> >>> So at runtime we then get two object instances - a CPU implementation
> >>> and a CPU definition. The CPU implementation object should have a
> >>> property which is a link to the desired CPU definition.
> >>
> >> It doesn't even have to be two object instances.  The implementation can be
> >> nothing more than a set of function pointers.
> > 
> > A set of function pointers is exactly what a QOM interface is.
> > Could the methods be provided by a TYPE_X86_ACCEL interface type,
> > implemented by the accel object?
> > 
> 
> Looking at the 2 axes mentioned by Daniel before, on the "accelerator cpu 
> axis", we have TYPE_TCG_CPU, TYPE_KVM_CPU, TYPE_HVF_CPU,
> which look like simple subclasses of TYPE_X86_CPU to me, with basically all 
> the divergent functionality being added by composition.
> TYPE_HVF_CPU seems to do everything that TYPE_X86_CPU does on construction 
> (and device realization), and then some.

What I don't get here is: why do we need a new "accelerator CPU
axis" if we already have an accelerator QOM type hierarchy?
accelerator-specific behavior can be delegated to the (existing)
accelerator object.

> 
> On the "cpu models" axis we have all the current subclasses of TYPE_X86_CPU, 
> which include "links" to X86CPUModel objects in the form
> of class_data:
> 
> static void x86_register_cpu_model_type(const char *name, X86CPUModel *model,
>                                         const char *parent_type)
> {
>     g_autofree char *typename = x86_cpu_type_name(name);
>     TypeInfo ti = {
>         .name = typename,
>         .parent = parent_type,
>         .class_init = x86_cpu_cpudef_class_init,
>         .class_data = model,
>     };
> 
>     type_register(&ti);
> }
> 
> so this would be close to the "link" property that Daniel you were speaking 
> about before?
> Should X86CPUmodel be the prime citizen of the "cpu models"
> axis, without constructing a separate TYPE_X86_CPU subclass for
> each cpu model?

I don't think this would be fundamentally wrong, but the
assumption that each CPU model is implemented as a separate
subclass of TYPE_CPU is encoded everywhere in the code and in
management software.

> 
> A separate topic we did not address in comments before, where
> I'd like opinions, is how should we treat cpu types "base" and
> "max" and "host"?
> 
> Just to avoid forgetting about them, currently TYPE_X86_CPU is
> the parent type of "base" and of "max", and "max" is the parent
> type of "host".
> 
> "host" is only allowed when using accelerator kvm or hvf.
> Attempts to create such a cpu without a kvm or hvf accelerator
> enabled will error out.
> "max" behaves differently when using hvf or kvm.

"base" exists only to allow us to implement
`query-cpu-model-expansion type=static` (because it requires a
"static" CPU model[1]).  It is not supposed to be used directly.

"host" is supposed to be used directly by the user, work out of
the box, and is a convenient way to get an optimal configuration
for the current host.  It is supposed to have reasonable defaults
that let you boot a guest, and enable as most features as
possible.  We don't offer it for TCG, because TCG emulation
features are not dependent on host capabilities.

Now, "max" is tricky to define, because its semantics are
overloaded:

For KVM, "max" is used for querying which features are supported
by the host (even if the feature is not enabled by default by
"host").

However, "max" is _also_ usable directly by users with TCG, if
they want all features supported by TCG enabled.  Its use case
for TCG is more similar to the use case for "host".

Probably mixing two use cases in the same "max" CPU model was a
mistake, and we should have added a separate CPU model for each
use case.

Because of the above, having separate accel-specific names for
each of those models sounds like a welcome change.

---
[1] The definition of "static CPU model" is in the documentation
for query-cpu-model-expansion.

-- 
Eduardo