| Message ID | 20201124162210.8796-12-cfontana@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | i386 cleanup | expand |
On Tue, Nov 24, 2020 at 05:22:09PM +0100, Claudio Fontana wrote: > Signed-off-by: Claudio Fontana <cfontana@suse.de> Probably this can be squashed into patch 10/12. > --- > target/i386/cpu-qom.h | 2 -- > target/i386/cpu.c | 27 ++++++++++++++++++++------- > target/i386/hvf/cpu.c | 9 --------- > target/i386/kvm/cpu.c | 8 -------- > target/i386/tcg/cpu.c | 9 --------- > 5 files changed, 20 insertions(+), 35 deletions(-) > > diff --git a/target/i386/cpu-qom.h b/target/i386/cpu-qom.h > index 9316e78e71..2cea5394c6 100644 > --- a/target/i386/cpu-qom.h > +++ b/target/i386/cpu-qom.h > @@ -98,6 +98,4 @@ struct X86CPUAccelClass { > void (*cpu_realizefn)(X86CPU *cpu, Error **errp); > }; > > -void x86_cpu_accel_init(const char *accel_name); > - > #endif > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index b799723e53..f6fd055046 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -7066,18 +7066,31 @@ type_init(x86_cpu_register_types) > static void x86_cpu_accel_init_aux(ObjectClass *klass, void *opaque) > { > X86CPUClass *xcc = X86_CPU_CLASS(klass); > - const X86CPUAccelClass **accel = opaque; > + X86CPUAccelClass *accel = opaque; > > - xcc->accel = *accel; > + xcc->accel = accel; > xcc->accel->cpu_common_class_init(xcc); > } > > -void x86_cpu_accel_init(const char *accel_name) > +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); This keeps the hidden initialization ordering dependency between MODULE_INIT_ACCEL_CPU and current_accel(). I thought we were going to get rid of module init functions that depend on runtime state. This is an improvement to the code in patch 10/12, though. If others believe it is an acceptable (temporary) solution, I won't block it. I would still prefer to have a void arch_accel_cpu_init(AccelState*) function which would call a void x86_cpu_accel_init(AccelState*) function. That would make the dependency between x86_cpu_accel_init() and accelerator creation explicit. > diff --git a/target/i386/hvf/cpu.c b/target/i386/hvf/cpu.c > index 7e7dc044d3..70b6dbfc10 100644 > --- a/target/i386/hvf/cpu.c > +++ b/target/i386/hvf/cpu.c > @@ -65,12 +65,3 @@ static void hvf_cpu_accel_register_types(void) > type_register_static(&hvf_cpu_accel_type_info); > } > type_init(hvf_cpu_accel_register_types); > - > -static void hvf_cpu_accel_init(void) > -{ > - if (hvf_enabled()) { > - x86_cpu_accel_init(X86_CPU_ACCEL_TYPE_NAME("hvf")); > - } > -} > - > -accel_cpu_init(hvf_cpu_accel_init); > diff --git a/target/i386/kvm/cpu.c b/target/i386/kvm/cpu.c > index bc5f519479..c17ed5a3f2 100644 > --- a/target/i386/kvm/cpu.c > +++ b/target/i386/kvm/cpu.c > @@ -147,11 +147,3 @@ static void kvm_cpu_accel_register_types(void) > type_register_static(&kvm_cpu_accel_type_info); > } > type_init(kvm_cpu_accel_register_types); > - > -static void kvm_cpu_accel_init(void) > -{ > - if (kvm_enabled()) { > - x86_cpu_accel_init(X86_CPU_ACCEL_TYPE_NAME("kvm")); > - } > -} > -accel_cpu_init(kvm_cpu_accel_init); > diff --git a/target/i386/tcg/cpu.c b/target/i386/tcg/cpu.c > index e7d4effdd0..00166c36e9 100644 > --- a/target/i386/tcg/cpu.c > +++ b/target/i386/tcg/cpu.c > @@ -170,12 +170,3 @@ static void tcg_cpu_accel_register_types(void) > type_register_static(&tcg_cpu_accel_type_info); > } > type_init(tcg_cpu_accel_register_types); > - > -static void tcg_cpu_accel_init(void) > -{ > - if (tcg_enabled()) { > - x86_cpu_accel_init(X86_CPU_ACCEL_TYPE_NAME("tcg")); > - } > -} > - > -accel_cpu_init(tcg_cpu_accel_init); > -- > 2.26.2 >
On 11/24/20 5:59 PM, Eduardo Habkost wrote: > On Tue, Nov 24, 2020 at 05:22:09PM +0100, Claudio Fontana wrote: >> Signed-off-by: Claudio Fontana <cfontana@suse.de> > > Probably this can be squashed into patch 10/12. Yes, you are right, no point building things fragmented and then merging together later. > >> --- >> target/i386/cpu-qom.h | 2 -- >> target/i386/cpu.c | 27 ++++++++++++++++++++------- >> target/i386/hvf/cpu.c | 9 --------- >> target/i386/kvm/cpu.c | 8 -------- >> target/i386/tcg/cpu.c | 9 --------- >> 5 files changed, 20 insertions(+), 35 deletions(-) >> >> diff --git a/target/i386/cpu-qom.h b/target/i386/cpu-qom.h >> index 9316e78e71..2cea5394c6 100644 >> --- a/target/i386/cpu-qom.h >> +++ b/target/i386/cpu-qom.h >> @@ -98,6 +98,4 @@ struct X86CPUAccelClass { >> void (*cpu_realizefn)(X86CPU *cpu, Error **errp); >> }; >> >> -void x86_cpu_accel_init(const char *accel_name); >> - >> #endif >> diff --git a/target/i386/cpu.c b/target/i386/cpu.c >> index b799723e53..f6fd055046 100644 >> --- a/target/i386/cpu.c >> +++ b/target/i386/cpu.c >> @@ -7066,18 +7066,31 @@ type_init(x86_cpu_register_types) >> static void x86_cpu_accel_init_aux(ObjectClass *klass, void *opaque) >> { >> X86CPUClass *xcc = X86_CPU_CLASS(klass); >> - const X86CPUAccelClass **accel = opaque; >> + X86CPUAccelClass *accel = opaque; >> >> - xcc->accel = *accel; >> + xcc->accel = accel; >> xcc->accel->cpu_common_class_init(xcc); >> } >> >> -void x86_cpu_accel_init(const char *accel_name) >> +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); > > This keeps the hidden initialization ordering dependency between > MODULE_INIT_ACCEL_CPU and current_accel(). I thought we were > going to get rid of module init functions that depend on runtime > state. > > This is an improvement to the code in patch 10/12, though. If > others believe it is an acceptable (temporary) solution, I won't > block it. In the way I thought about it, MODULE_INIT_ACCEL_CPU meant exactly that: initializations to be done after accel is chosen. So in my view the relationship with current_accel() was then following naturally. > > I would still prefer to have a > void arch_accel_cpu_init(AccelState*) > function which would call a > void x86_cpu_accel_init(AccelState*) > function. That would make the dependency between > x86_cpu_accel_init() and accelerator creation explicit. > not a bad idea either, what I would lose here is a single point to discover the codebase, ie MODULE_INIT_ACCEL_CPU via a simple grep or gid MODULE_INIT_ACCEL_CPU gives me all initializations done for this phase, not only the arch_ stuff, but also currently the Ops stuff. > >> diff --git a/target/i386/hvf/cpu.c b/target/i386/hvf/cpu.c >> index 7e7dc044d3..70b6dbfc10 100644 >> --- a/target/i386/hvf/cpu.c >> +++ b/target/i386/hvf/cpu.c >> @@ -65,12 +65,3 @@ static void hvf_cpu_accel_register_types(void) >> type_register_static(&hvf_cpu_accel_type_info); >> } >> type_init(hvf_cpu_accel_register_types); >> - >> -static void hvf_cpu_accel_init(void) >> -{ >> - if (hvf_enabled()) { >> - x86_cpu_accel_init(X86_CPU_ACCEL_TYPE_NAME("hvf")); >> - } >> -} >> - >> -accel_cpu_init(hvf_cpu_accel_init); >> diff --git a/target/i386/kvm/cpu.c b/target/i386/kvm/cpu.c >> index bc5f519479..c17ed5a3f2 100644 >> --- a/target/i386/kvm/cpu.c >> +++ b/target/i386/kvm/cpu.c >> @@ -147,11 +147,3 @@ static void kvm_cpu_accel_register_types(void) >> type_register_static(&kvm_cpu_accel_type_info); >> } >> type_init(kvm_cpu_accel_register_types); >> - >> -static void kvm_cpu_accel_init(void) >> -{ >> - if (kvm_enabled()) { >> - x86_cpu_accel_init(X86_CPU_ACCEL_TYPE_NAME("kvm")); >> - } >> -} >> -accel_cpu_init(kvm_cpu_accel_init); >> diff --git a/target/i386/tcg/cpu.c b/target/i386/tcg/cpu.c >> index e7d4effdd0..00166c36e9 100644 >> --- a/target/i386/tcg/cpu.c >> +++ b/target/i386/tcg/cpu.c >> @@ -170,12 +170,3 @@ static void tcg_cpu_accel_register_types(void) >> type_register_static(&tcg_cpu_accel_type_info); >> } >> type_init(tcg_cpu_accel_register_types); >> - >> -static void tcg_cpu_accel_init(void) >> -{ >> - if (tcg_enabled()) { >> - x86_cpu_accel_init(X86_CPU_ACCEL_TYPE_NAME("tcg")); >> - } >> -} >> - >> -accel_cpu_init(tcg_cpu_accel_init); >> -- >> 2.26.2 >> >
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. Paolo
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.
On 11/24/20 9:13 PM, 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. > > Paolo > > I don't expect others (actually I do, but it's going to be one additional call per target). So based on RFCv5, I would see additional calls to accel_cpu_init for target/arm/cpu.c, target/s390x/cpu.c in patches to come. I'll look into this. Thanks, Claudio
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. I really don't like stubs, and trying to link the proper ones for each of the targets we need to build. It screams "this hasn't been refactored properly yet - sorry!" If we could avoid them it would be a clear benefit from my perspective. > > 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. > Ciao, Claudio
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 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/ But it is tough to find a header where CPU_RESOLVING_TYPE can be used. 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. Ciao, Claudio
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`?
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? Ciao, Claudio
On 11/24/20 9:13 PM, 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. > > Paolo > Hi Paolo, this works well. The only thing is, setting the ops in the AccelClass is basically useless: we need to cache the AccelClass * in cpus.c anyway, so I ended up not putting it in AccelClass, as nobody ends up using it. Maybe you could look for this in the series I am posting later today, Ciao, thanks! Claudio
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.
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" if we agree to use "accel-x86_64" aka "accel-" CPU_RESOLVING_TYPE, I would lookup "kvm-accel-" CPU_RESOLVING_TYPE * initialize the arch-specific accel CpuClass interfaces */ static void accel_init_cpu_interfaces(AccelClass *ac, const char *cpu_type) { const char *ac_name; /* AccelClass name */ char *acc_name; /* AccelCPUClass name */ ObjectClass *acc; /* AccelCPUClass */ ac_name = object_class_get_name(OBJECT_CLASS(ac)); g_assert(ac_name != NULL); acc_name = g_strdup_printf("%s-cpu", ac_name); acc = object_class_by_name(acc_name); g_free(acc_name); if (acc) { object_class_foreach(accel_init_cpu_interfaces_aux, cpu_type, false, acc); } } Ciao, CLaudio
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 question would apply to target/*/tcg*, and to other accelerators. > if we agree to use "accel-x86_64" aka "accel-" CPU_RESOLVING_TYPE, I would lookup "kvm-accel-" CPU_RESOLVING_TYPE > > * initialize the arch-specific accel CpuClass interfaces */ > static void accel_init_cpu_interfaces(AccelClass *ac, const char *cpu_type) > { > const char *ac_name; /* AccelClass name */ > char *acc_name; /* AccelCPUClass name */ > ObjectClass *acc; /* AccelCPUClass */ > > ac_name = object_class_get_name(OBJECT_CLASS(ac)); > g_assert(ac_name != NULL); > > acc_name = g_strdup_printf("%s-cpu", ac_name); > acc = object_class_by_name(acc_name); > g_free(acc_name); > > if (acc) { > object_class_foreach(accel_init_cpu_interfaces_aux, cpu_type, false, acc); > } > } > > Ciao, > > CLaudio >
On 26/11/20 15:55, Claudio Fontana wrote: > If we agree to use "accel-cpu" I would lookup "kvm-accel-cpu" > if we agree to use "accel-x86_64" aka "accel-" CPU_RESOLVING_TYPE, I would lookup "kvm-accel-" CPU_RESOLVING_TYPE > > * initialize the arch-specific accel CpuClass interfaces */ > static void accel_init_cpu_interfaces(AccelClass *ac, const char *cpu_type) > { > const char *ac_name; /* AccelClass name */ > char *acc_name; /* AccelCPUClass name */ > ObjectClass *acc; /* AccelCPUClass */ > > ac_name = object_class_get_name(OBJECT_CLASS(ac)); > g_assert(ac_name != NULL); > > acc_name = g_strdup_printf("%s-cpu", ac_name); > acc = object_class_by_name(acc_name); > g_free(acc_name); > > if (acc) { > object_class_foreach(accel_init_cpu_interfaces_aux, cpu_type, false, acc); > } > } I would use the second; there's no reason to have allow only one CPU type, it's just a limitation of QEMU. Paolo
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, but I wonder whether we want separate names anyway. Ciao, Claudio > > The same question would apply to target/*/tcg*, and to other > accelerators. > >> if we agree to use "accel-x86_64" aka "accel-" CPU_RESOLVING_TYPE, I would lookup "kvm-accel-" CPU_RESOLVING_TYPE >> >> * initialize the arch-specific accel CpuClass interfaces */ >> static void accel_init_cpu_interfaces(AccelClass *ac, const char *cpu_type) >> { >> const char *ac_name; /* AccelClass name */ >> char *acc_name; /* AccelCPUClass name */ >> ObjectClass *acc; /* AccelCPUClass */ >> >> ac_name = object_class_get_name(OBJECT_CLASS(ac)); >> g_assert(ac_name != NULL); >> >> acc_name = g_strdup_printf("%s-cpu", ac_name); >> acc = object_class_by_name(acc_name); >> g_free(acc_name); >> >> if (acc) { >> object_class_foreach(accel_init_cpu_interfaces_aux, cpu_type, false, acc); >> } >> } >> >> Ciao, >> >> CLaudio >> >
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.
On 11/26/20 4:48 PM, Eduardo Habkost wrote: > 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. > Ok will try, for now I CPU_RESOLVING_TYPE is not playing nice with my attempts.. Ciao, Claudio
diff --git a/target/i386/cpu-qom.h b/target/i386/cpu-qom.h index 9316e78e71..2cea5394c6 100644 --- a/target/i386/cpu-qom.h +++ b/target/i386/cpu-qom.h @@ -98,6 +98,4 @@ struct X86CPUAccelClass { void (*cpu_realizefn)(X86CPU *cpu, Error **errp); }; -void x86_cpu_accel_init(const char *accel_name); - #endif diff --git a/target/i386/cpu.c b/target/i386/cpu.c index b799723e53..f6fd055046 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -7066,18 +7066,31 @@ type_init(x86_cpu_register_types) static void x86_cpu_accel_init_aux(ObjectClass *klass, void *opaque) { X86CPUClass *xcc = X86_CPU_CLASS(klass); - const X86CPUAccelClass **accel = opaque; + X86CPUAccelClass *accel = opaque; - xcc->accel = *accel; + xcc->accel = accel; xcc->accel->cpu_common_class_init(xcc); } -void x86_cpu_accel_init(const char *accel_name) +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); diff --git a/target/i386/hvf/cpu.c b/target/i386/hvf/cpu.c index 7e7dc044d3..70b6dbfc10 100644 --- a/target/i386/hvf/cpu.c +++ b/target/i386/hvf/cpu.c @@ -65,12 +65,3 @@ static void hvf_cpu_accel_register_types(void) type_register_static(&hvf_cpu_accel_type_info); } type_init(hvf_cpu_accel_register_types); - -static void hvf_cpu_accel_init(void) -{ - if (hvf_enabled()) { - x86_cpu_accel_init(X86_CPU_ACCEL_TYPE_NAME("hvf")); - } -} - -accel_cpu_init(hvf_cpu_accel_init); diff --git a/target/i386/kvm/cpu.c b/target/i386/kvm/cpu.c index bc5f519479..c17ed5a3f2 100644 --- a/target/i386/kvm/cpu.c +++ b/target/i386/kvm/cpu.c @@ -147,11 +147,3 @@ static void kvm_cpu_accel_register_types(void) type_register_static(&kvm_cpu_accel_type_info); } type_init(kvm_cpu_accel_register_types); - -static void kvm_cpu_accel_init(void) -{ - if (kvm_enabled()) { - x86_cpu_accel_init(X86_CPU_ACCEL_TYPE_NAME("kvm")); - } -} -accel_cpu_init(kvm_cpu_accel_init); diff --git a/target/i386/tcg/cpu.c b/target/i386/tcg/cpu.c index e7d4effdd0..00166c36e9 100644 --- a/target/i386/tcg/cpu.c +++ b/target/i386/tcg/cpu.c @@ -170,12 +170,3 @@ static void tcg_cpu_accel_register_types(void) type_register_static(&tcg_cpu_accel_type_info); } type_init(tcg_cpu_accel_register_types); - -static void tcg_cpu_accel_init(void) -{ - if (tcg_enabled()) { - x86_cpu_accel_init(X86_CPU_ACCEL_TYPE_NAME("tcg")); - } -} - -accel_cpu_init(tcg_cpu_accel_init);
Signed-off-by: Claudio Fontana <cfontana@suse.de> --- target/i386/cpu-qom.h | 2 -- target/i386/cpu.c | 27 ++++++++++++++++++++------- target/i386/hvf/cpu.c | 9 --------- target/i386/kvm/cpu.c | 8 -------- target/i386/tcg/cpu.c | 9 --------- 5 files changed, 20 insertions(+), 35 deletions(-)