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