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

[Claudio Fontana <cfontana@suse.de>]

On 11/10/20 7:14 PM, Eduardo Habkost wrote:
> 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.


Hi Eduardo,

it might very well be that we can extend the QOM hierarchy to achieve this,
this is probably the focus of the other thread with Paolo.

I have some reservations with this approach, which maybe I should mention there (briefly: current accel implementation, its lack of user mode coverage, and the imperfect mapping between the accel cpu behavior we need and the actual accel class (in particular for whpx, hax, qtest).

It might be that these are actually only TODOs, and not blockers.

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


Fair enough, so that's a point towards keeping the existing "cpu models as subclasses of X86 cpu".

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

can compatibility issues be addressed effectively with this option?
If so, would it make sense to go even further and expand the whole type tree to have accel-specific models?

Just trying to explore the whole range of possibilities here.

Thanks,

Claudio


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