Re: [RFC v5 11/12] i386: centralize initialization of cpu accel interfaces

[Eduardo Habkost]

On Thu, Nov 26, 2020 at 11:57:28AM +0100, Claudio Fontana wrote:
> On 11/24/20 10:31 PM, Eduardo Habkost wrote:
> > On Tue, Nov 24, 2020 at 09:13:13PM +0100, Paolo Bonzini wrote:
> >> On 24/11/20 17:22, Claudio Fontana wrote:
> >>> +static void x86_cpu_accel_init(void)
> >>>  {
> >>> -    X86CPUAccelClass *acc;
> >>> +    const char *ac_name;
> >>> +    ObjectClass *ac;
> >>> +    char *xac_name;
> >>> +    ObjectClass *xac;
> >>> -    acc = X86_CPU_ACCEL_CLASS(object_class_by_name(accel_name));
> >>> -    g_assert(acc != NULL);
> >>> +    ac = object_get_class(OBJECT(current_accel()));
> >>> +    g_assert(ac != NULL);
> >>> +    ac_name = object_class_get_name(ac);
> >>> +    g_assert(ac_name != NULL);
> >>> -    object_class_foreach(x86_cpu_accel_init_aux, TYPE_X86_CPU, false, 
> >>> &acc);
> >>> +    xac_name = g_strdup_printf("%s-%s", ac_name, TYPE_X86_CPU);
> >>> +    xac = object_class_by_name(xac_name);
> >>> +    g_free(xac_name);
> >>> +
> >>> +    if (xac) {
> >>> +        object_class_foreach(x86_cpu_accel_init_aux, TYPE_X86_CPU, 
> >>> false, xac);
> >>> +    }
> >>>  }
> >>> +
> >>> +accel_cpu_init(x86_cpu_accel_init);
> >>
> >> If this and cpus_accel_ops_init are the only call to accel_cpu_init, I'd
> >> rather make them functions in CPUClass (which you find and call via
> >> CPU_RESOLVING_TYPE) and AccelClass respectively.
> > 
> > Making x86_cpu_accel_init() be a CPUClass method sounds like a
> > good idea.  This way we won't need a arch_cpu_accel_init() stub
> > for non-x86.
> > 
> > accel.c can't use cpu.h, correct?  We can add a:
> > 
> >   CPUClass *arch_base_cpu_type(void)
> >   {
> >       return object_class_by_name(CPU_RESOLVING_TYPE);
> >   }
> > 
> > function to arch_init.c, to allow target-independent code call
> > target-specific code.
> > 
> 
> Hi Eduardo,
> 
> we can't use arch-init because it is softmmu only, but we could put this in 
> $(top_srcdir)/cpu.c

That would work, too.

> 
> however, it would be very useful to put a:
> 
> #define TYPE_ACCEL_CPU "accel-" CPU_RESOLVING_TYPE
> #define ACCEL_CPU_NAME(name) (name "-" TYPE_ACCEL_CPU)
> 
> in an H file somewhere, for convenience for the programmer that
> has to implement subclasses in target/xxx/

Absolutely.

> 
> But it is tough to find a header where CPU_RESOLVING_TYPE can be used.

cpu-all.h?

> 
> We could I guess just use plain "cpu" instead of CPU_RESOLVING_TYPE,
> maybe that would be acceptable too? The interface ends up in CPUClass, so 
> maybe ok?
> 
> So we'd end up having
> 
> accel-cpu
> 
> instead of the previous
> 
> accel-x86_64-cpu
> 
> on top of the hierarchy.

It seems OK to have a accel-cpu type at the top, but I don't see
why it solves the problem above.  What exactly would be the value
of `kvm_cpu_accel.name`?

-- 
Eduardo