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

[Eduardo Habkost]

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 <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
> 
> 
> 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 
8f829e99-c346-00bc-efdd-3e6d69cfba35@redhat.com">https://lore.kernel.org/qemu-devel/8f829e99-c346-00bc-efdd-3e6d69cfba35@redhat.com

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

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.

-- 
Eduardo