On Wed, Nov 18, 2020 at 05:22:46PM +0100, Paolo Bonzini wrote: > Il mer 18 nov 2020, 17:11 Eduardo Habkost <ehabk...@redhat.com> ha scritto: > > > On Wed, Nov 18, 2020 at 04:43:19PM +0100, Paolo Bonzini wrote: > > > Il mer 18 nov 2020, 16:26 Eduardo Habkost <ehabk...@redhat.com> ha > > scritto: > > > > > > > > > > > > The alternative is to store the (type, function) tuple directly, > > with the > > > > > type as a string. Then you can just use type_init. > > > > > > > > Right. Let's build on top of that: > > > > > > > > Another alternative would be to store a (type, X86CPUAccel) tuple > > > > directly, with the type as string. This would save the extra > > > > indirection of the x86_cpu_accel_init() call. > > > > > > > > It turns out we already have a mechanism to register and store > > > > (type, StructContainingFunctionPointers) tuples at initialization > > > > time: QOM. > > > > > > > > X86CPUAccel can become X86CPUAccelClass, and be registered as a > > > > QOM type. It could be a subtype of TYPE_ACCEL or not, it > > > > shouldn't matter. > > > > > > > > > > It would be a weird type that isn't instantiated, and/or that does > > nothing > > > but monkey patching other classes. I don't think it's a good fit. > > > > The whole point of this would be to avoid monkey patching other > > classes. > > > > 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. > > 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? > > Why wouldn't we instantiate it? There's a huge number of static > > variables in target/i386/kvm.c that could be moved to that > > object. Sounds like a perfect fit for me. > > > > Most of those are properties of the running kernel so there's no need to > move them inside an object. There's no need, correct. Some consistency would be nice, though. All kernel capabilities in kvm-all.c are saved in KVMState. > > Paolo > > I won't try to stop you if you really want to invent a brand new > > (name => CollectionOfFunctionPointers) registry, but it seems > > unnecessary. > > > > > > > > Yet another possibility is to use GHashTable. It is limited to one value > > > per key, but it's enough if everything is kept local to {hw,target}/i386. > > > If needed a new function pointer can be added to MachineClass, > > implemented > > > in X86MachineState (where the GHashTable would also be) and called in > > > accel.c. > > > > > > Paolo > > > > > > Paolo > > > > > > > > > > I remember this was suggested in a previous thread, but I don't > > > > remember if there were any objections. > > > > > > > > > > > > > > > Making sure module_call_init() is called at the correct moment is > > > > > > not easier or safer than just making sure accel_init_machine() > > > > > > (or another init function you create) is called at the correct > > > > > > moment. > > > > > > > > > > Since there is a way to do it without a new level, that would of > > course > > > > be > > > > > fine for me too. Let me explain however why I think Claudio's > > design had > > > > > module_call_init() misplaced and what the fundamental difference > > is. The > > > > > basic phases in qemu_init() are: > > > > > > > > > > - initialize stuff > > > > > - parse command line > > > > > - create machine > > > > > - create accelerator > > > > > - initialize machine > > > > > - create devices > > > > > - start > > > > > > > > > > with a mess of other object creation sprinkled between the various > > phases > > > > > (but we don't care about those). > > > > > > > > > > What I object to, is calling module_call_init() after the "initialize > > > > stuff" > > > > > phase. Claudio was using it to call the function directly, so it > > had to > > > > be > > > > > exactly at "create accelerator". This is different from all other > > > > > module_call_init() calls, which are done very early. > > > > > > > > I agree. > > > > > > > > > > > > > > With the implementation I sketched, accel_register_call must still be > > > > done > > > > > after type_init, so there's still an ordering constraint, but all > > it's > > > > doing > > > > > is registering a callback in the "initialize stuff" phase. > > > > > > > > Makes sense, if we really want to introduce a new accel_register_call() > > > > abstraction. I don't think we need it, though. > > > > > > > > -- > > > > Eduardo > > > > > > > > > > > > -- > > Eduardo > > > > -- Eduardo