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

[Eduardo Habkost <ehabkost@redhat.com>]

On Thu, Nov 26, 2020 at 04:34:17PM +0100, Claudio Fontana wrote:
> On 11/26/20 4:14 PM, Eduardo Habkost wrote:
> > On Thu, Nov 26, 2020 at 03:55:37PM +0100, Claudio Fontana wrote:
> >> On 11/26/20 3:49 PM, Eduardo Habkost wrote:
> >>> On Thu, Nov 26, 2020 at 03:33:17PM +0100, Claudio Fontana wrote:
> >>>> On 11/26/20 2:44 PM, Eduardo Habkost wrote:
> >>>>> 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`?
> >>>>>
> >>>>
> >>>> It does solve the problem, because we can put then all AccelOpsClass and AccelCPUClass stuff in accel.h,
> >>>> resolve everything in accel/accel-*.c, and make a generic solution fairly self-contained (already tested, will post soonish).
> >>>>
> >>>> But I'll try cpu-all.h if it's preferred to have accel-x86_64-cpu, accel-XXX-cpu on top, I wonder what the preference would be?
> >>>
> >>> I don't have a specific preference, but I still wonder how
> >>> exactly you would name the X86CPUAccel implemented at
> >>> target/i386/kvm, and how exactly you would look for it when
> >>> initializing the accelerator.
> >>>
> >>
> >> If we agree to use "accel-cpu" I would lookup "kvm-accel-cpu"
> > 
> > The structure in target/i386/kvm is x86-specific and
> > kvm-specific.  If we name it "kvm-accel-cpu", how would you name
> > the equivalent structures at target/s390x/kvm, target/arm/kvm,
> > target/ppc/kvm?
> 
> The same way; only one of them would be compiled into the target binary, so the lookup would not collide in practice,

That's not always going to be true.  Maybe for KVM it will, but
not necessarily for TCG.

> but I wonder whether we want separate names anyway.

I believe we do.  Avoiding duplicate QOM type names is a good
idea in either case.

-- 
Eduardo