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

[Eduardo Habkost]

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

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

-- 
Eduardo