Re: [RFC v3 8/9] module: introduce MODULE_INIT_ACCEL_CPU

[Claudio Fontana]

On 11/20/20 7:09 PM, Eduardo Habkost wrote:
> On Fri, Nov 20, 2020 at 06:41:35PM +0100, Claudio Fontana wrote:
>> On 11/20/20 6:19 PM, Eduardo Habkost wrote:
>>> On Fri, Nov 20, 2020 at 01:13:33PM +0100, Claudio Fontana wrote:
>>>> On 11/18/20 11:07 PM, Eduardo Habkost wrote:
>>>>> On Wed, Nov 18, 2020 at 08:13:18PM +0100, Paolo Bonzini wrote:
>>>>>> On 18/11/20 18:30, Eduardo Habkost wrote:
>>>>>>>> Adding a layer of indirect calls is not very different from monkey 
>>>>>>>> patching
>>>>>>>> though.
>>>>>>>
>>>>>>> I'm a little bothered by monkey patching, but I'm more
>>>>>>> bothered by having to:
>>>>>>>
>>>>>>> (1) register (module_init()) a function (kvm_cpu_accel_register()) that
>>>>>>>    (2) register (accel_register_call()) a function 
>>>>>>> (kvm_cpu_accel_init()) that
>>>>>>>      (3) register (x86_cpu_accel_init()) a data structure (X86CPUAccel 
>>>>>>> kvm_cpu_accel) that
>>>>>>>        (4) will be saved in multiple QOM classes, so that
>>>>>>>          (5) we will call the right X86CPUClass.accel method at the 
>>>>>>> right moment
>>>>>>>              (common_class_init(), instance_init(), realizefn()),
>>>>>>> where:
>>>>>>>    step 4 must be done before any CPU object is created
>>>>>>>      (otherwise X86CPUAccel.instance_init & X86CPUAccel.realizefn
>>>>>>>       will be silently ignored), and
>>>>>>>    step 3 must be done after all QOM types were registered.
>>>>>>>
>>>>>>>> You also have to consider that accel currently does not exist in 
>>>>>>>> usermode
>>>>>>>> emulators, so that's an issue too. I would rather get a simple change 
>>>>>>>> in
>>>>>>>> quickly, instead of designing the perfect class hierarchy.
>>>>>>>
>>>>>>> It doesn't have to be perfect.  I agree that simple is better.
>>>>>>>
>>>>>>> To me, registering a QOM type and looking it up when necessary is
>>>>>>> simpler than the above.  Even if it's a weird class having no
>>>>>>> object instances.  It probably could be an interface type.
>>>>>>
>>>>>> Registering a QOM type still has quite some boilerplate.  [...]
>>>>>
>>>>> We're working on that.  :)
>>>>>
>>>>>>                                                    [...]  Also 
>>>>>> registering a
>>>>>> QOM type has a public side effect (shows up in qom-list-types).  In 
>>>>>> general
>>>>>> I don't look at QOM unless I want its property mechanism, but maybe 
>>>>>> that's
>>>>>> just me.
>>>>>
>>>>> We have lots of internal-use-only types returned by
>>>>> qom-list-types, I don't think it's a big deal.
>>>>>
>>>>>>
>>>>>>>> Perhaps another idea would be to allow adding interfaces to classes
>>>>>>>> *separately from the registration of the types*. Then we can use it to 
>>>>>>>> add
>>>>>>>> SOFTMMU_ACCEL and I386_ACCEL interfaces to a bare bones accel class, 
>>>>>>>> and
>>>>>>>> add the accel object to usermode emulators.
>>>>>>>
>>>>>>> I'm not sure I follow.  What do you mean by bare bones accel
>>>>>>> class, and when exactly would you add the new interfaces to the
>>>>>>> classes?
>>>>>>
>>>>>> A bare bones accel class would not have init_machine and setup_post 
>>>>>> methods;
>>>>>> those would be in a TYPE_SOFTMMU_ACCEL interface.  It would still have
>>>>>> properties (such as tb-size for TCG) and would be able to register compat
>>>>>> properties.
>>>
>>> [1]
>>>
>>>>>
>>>>> Oh, I think I see.  This could save us having a lot of parallel type
>>>>> hierarchies.
>>>>>
>>>>>>
>>>>>> Where would I add it, I don't know.  It could be a simple public wrapper
>>>>>> around type_initialize_interface() if we add a new MODULE_INIT_* phase 
>>>>>> after
>>>>>> QOM.
>>>>>>
>>>>>> Or without adding a new phase, it could be a class_type->array of
>>>>>> (interface_type, init_fn) hash table.  type_initialize would look up the
>>>>>> class_type by name, add the interfaces would to the class with
>>>>>> type_initialize_interface, and then call the init_fn to fill in the 
>>>>>> vtable.
>>>>>
>>>>> That sounds nice.  I don't think Claudio's cleanup should be
>>>>> blocked until this new mechanism is ready, though.
>>>>>
>>>>> We don't really need the type representing X86CPUAccel to be a
>>>>> subtype of TYPE_ACCEL or an interface implemented by
>>>>> current_machine->accelerator, in the first version.  We just need
>>>>> a simple way for the CPU initialization code to find the correct
>>>>> X86CPUAccel struct.
>>>>>
>>>>> While we don't have the new mechanism, it can be just a:
>>>>>   object_class_by_name("%s-x86-cpu-accel" % (accel->name))
>>>>> call.
>>>>>
>>>>> Below is a rough draft of what I mean.  There's still lots of
>>>>> room for cleaning it up (especially getting rid of the
>>>>> X86CPUClass.common_class_init and X86CPUClass.accel fields).
>>>>>
>>>>> git tree at 
>>>>> https://gitlab.com/ehabkost/qemu/-/commits/work/qom-based-x86-cpu-accel
>>>>>
>>>>> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
>>> [...]
>>>>>  /**
>>>>> - * X86CPUAccel:
>>>>> - * @name: string name of the X86 CPU Accelerator
>>>>> - *
>>>>> + * X86CPUAccelInterface:
>>>>>   * @common_class_init: initializer for the common cpu
>>>>>   * @instance_init: cpu instance initialization
>>>>>   * @realizefn: realize function, called first in x86 cpu realize
>>>>> @@ -85,14 +83,16 @@ struct X86CPUClass {
>>>>>   * X86 CPU accelerator-specific CPU initializations
>>>>>   */
>>>>>  
>>>>> -struct X86CPUAccel {
>>>>> -    const char *name;
>>>>> -
>>>>> +struct X86CPUAccelInterface {
>>>>> +    ObjectClass parent_class;
>>>>>      void (*common_class_init)(X86CPUClass *xcc);
>>>>>      void (*instance_init)(X86CPU *cpu);
>>>>>      void (*realizefn)(X86CPU *cpu, Error **errp);
>>>>>  };
>>>>>  
>>>>> -void x86_cpu_accel_init(const X86CPUAccel *accel);
>>>>> +#define TYPE_X86_CPU_ACCEL "x86-cpu-accel"
>>>>> +OBJECT_DECLARE_INTERFACE(X86CPUAccel, X86CPUAccelInterface, 
>>>>> X86_CPU_ACCEL);
>>>>
>>>>
>>>> I am not exactly sure what precisely you are doing here,
>>>>
>>>> I get the general intention to use the existing interface-related "stuff" 
>>>> in QOM,
>>>> but I do not see any OBJECT_DECLARE_INTERFACE around, and does not seem 
>>>> like the other boilerplate used for interfaces.
>>>
>>> See the git URL I sent above, for other related changes:
>>>
>>>   https://gitlab.com/ehabkost/qemu/-/commits/work/qom-based-x86-cpu-accel
>>
>>
>> Aaah I missed this, there are quite a few more changes there;
>>
>> for me it's great if you take it from there, I see you are
>> developing a solution on top of the previous series.
> 
> I'm a bit busy with other stuff, so I'm probably not going to be
> able to make sure the patches are in a good shape to be submitted
> soon.
> 
> I don't want to impose any obstacles for the work you are doing,
> either.  Please consider the patch I sent (and the git tree
> above) as just an example of a possible solution to the two issues
> Paolo raised at 
> 8f829e99-c346-00bc-efdd-3e6d69cfba35@redhat.com">https://lore.kernel.org/qemu-devel/8f829e99-c346-00bc-efdd-3e6d69cfba35@redhat.com


Ok, thanks for the clarification,
will take those two issues up in my next attempt.


> 
>>
>>
>>>
>>>>
>>>> Could you clarify what happens here? Should we just use a normal object, 
>>>> call it "Interface" and call it a day?
>>>
>>> An interface is declared in a very similar way, but instance_size
>>> and the instance type cast macro is a bit different (because
>>> instances of interface types are never created directly).
>>>
>>> For the draft we have here, it shouldn't make any difference if
>>> you use OBJECT_DECLARE_TYPE, because the instance type cast
>>> macros are left unused.
>>>
>>> Normally the use case for interfaces is not like what I did here.
>>> Interfaces are usually attached to other classes (to declare that
>>> object instances of that class implement the methods of that
>>> interface).  Using interfaces would be just an intermediate step
>>> to the solution Paolo was mentioning (dynamically adding
>>> interface to classes, see [1] above).
>>>
>>
>> Makes sense to me,
>> let me know how you guys would like to proceed from here.
>>
> 
> To me, the main issue (also raised by Paolo above) is the fact
> that you are doing *_enabled() checks in the module init
> functions.  Every single use case we have for module init
> functions today is for unconditionally registering code or data
> structures provided by a code module (config group names, QOM
> types, block backends, multifd methods, etc.), and none of them
> depend on runtime options (like machine or accelerator options).


Ok, no _enabled() checks, got it.

Just to note, since _enabled() has multiple meanings depending on configuration 
(CONFIG_KVM),
the _enabled() used in kvm/cpu.c has actually the meaning of:

if (accelerator_finally_chosen == KVM).

But we can refactor this implicit check out, and make it instead a

accel_cpu_init("kvm"), like you suggest, so that the ifs disappear.



> 
> The x86_cpu_accel_init() calls, on the other hand, are not module
> initialization, but just one additional step of
> machine/accelerator/cpu initialization.
> 
> 
>> The thing I am still uncertain about, looking at your tree at:
>>
>> https://gitlab.com/ehabkost/qemu/-/commits/work/qom-based-x86-cpu-accel
>>
>> is the removal of MODULE_INIT_ACCEL_CPU, it would be way simpler to 
>> understand I think,
>> both for CpuAccelOps and X86CPUAccel, and is actualy in my view a perfect 
>> fit for
>> the problem that module_call_init is supposed to solve.
> 
> That was one of my goals.  My first goal was the removal of the
> (hvm|kvm|tcg)_enabled() checks in the accel init functions.  My
> secondary goal (and a side effect of the first goal) was making
> MODULE_INIT_ACCEL_CPU unnecessary.
> 
> If we are not trying to remove the *_enabled() checks in the
> accel init functions (nor trying to make MODULE_INIT_ACCEL_CPU
> unnecessary), my suggestion of using QOM doesn't make things
> simpler.
> 
> Let's hear what Paolo thinks.
> 
> If you want to proceed with the accel_register_call() solution
> suggested by Paolo, that's OK to me.  I just don't think we
> really need it, because QOM already solves the problem for us.
> 

Ok, thanks for all the input, will need some time to process,

thanks,

Claudio