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

[Claudio Fontana]

On 11/23/20 10:29 AM, Claudio Fontana wrote:
> 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
> 

One idea that came to mind is, why not extend accel.h to user mode?

It already contains

#ifndef CONFIG_USER_ONLY

parts, so maybe it was meant to be used by both, and just happened to end up 
confined to include/softmmu ?

Basically I was thinking, we could have an AccelState and an AccelClass for 
user mode as well (without bringing in the whole machine thing),
and from there we could use current_accel() to build up the right name for the 
chosen accelerator?

Ciao,

Claudio