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