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
