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