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