On Wed, Nov 18, 2020 at 04:43:19PM +0100, Paolo Bonzini wrote:
> Il mer 18 nov 2020, 16:26 Eduardo Habkost <ehabkost@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.
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.
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.
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.
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