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 <ehabk...@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
> https://lore.kernel.org/qemu-devel/8f829e99-c346-00bc-efdd-3e6d69cfb...@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
