Re: [RFC v3 8/9] module: introduce MODULE_INIT_ACCEL_CPU

[Eduardo Habkost]

On Wed, Nov 18, 2020 at 05:22:46PM +0100, Paolo Bonzini wrote:
> Il mer 18 nov 2020, 17:11 Eduardo Habkost <ehabkost@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 <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.

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