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

[Claudio Fontana <cfontana@suse.de>]

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.


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

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.

But, my 2c of course,

Ciao,

Claudio