| Message ID | 20210128092814.8676-19-cfontana@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i386 cleanup PART 2 | expand |
On 1/28/21 10:28 AM, Claudio Fontana wrote: > add a new optional interface to CPUClass, > which allows accelerators to extend the CPUClass > with additional accelerator-specific initializations. > > Add the field before tcg_ops, and mark tcg_ops as > needing to be last in the struct until we rework this > further in a later patch. > > Signed-off-by: Claudio Fontana <cfontana@suse.de> > --- > include/hw/core/accel-cpu.h | 35 +++++++++++++++++++++++++++++ > include/hw/core/cpu.h | 1 + > accel/accel-common.c | 44 +++++++++++++++++++++++++++++++++++++ > MAINTAINERS | 1 + > 4 files changed, 81 insertions(+) > create mode 100644 include/hw/core/accel-cpu.h > > diff --git a/include/hw/core/accel-cpu.h b/include/hw/core/accel-cpu.h > new file mode 100644 > index 0000000000..246b3e2fcb > --- /dev/null > +++ b/include/hw/core/accel-cpu.h > @@ -0,0 +1,35 @@ > +/* > + * Accelerator interface, specializes CPUClass > + * > + * Copyright 2020 SUSE LLC 2020-2021 ;) > + * > + * This work is licensed under the terms of the GNU GPL, version 2 or later. > + * See the COPYING file in the top-level directory. > + */ > + > +#ifndef ACCEL_CPU_H > +#define ACCEL_CPU_H > + > +/* > + * these defines cannot be in cpu.h, because we are using > + * CPU_RESOLVING_TYPE here. > + * Use this header to define your accelerator-specific > + * cpu-specific accelerator interfaces. > + */ > + > +#define TYPE_ACCEL_CPU "accel-" CPU_RESOLVING_TYPE > +#define ACCEL_CPU_NAME(name) (name "-" TYPE_ACCEL_CPU) > +typedef struct AccelCPUClass AccelCPUClass; > +DECLARE_CLASS_CHECKERS(AccelCPUClass, ACCEL_CPU, TYPE_ACCEL_CPU) > + > +typedef struct AccelCPUClass { > + /*< private >*/ > + ObjectClass parent_class; > + /*< public >*/ > + > + void (*cpu_class_init)(CPUClass *cc); > + void (*cpu_instance_init)(CPUState *cpu); > + void (*cpu_realizefn)(CPUState *cpu, Error **errp); If we want callers to check errp, better have the prototype return a boolean. > +} AccelCPUClass;
On 1/28/21 2:03 PM, Philippe Mathieu-Daudé wrote: > On 1/28/21 10:28 AM, Claudio Fontana wrote: >> add a new optional interface to CPUClass, >> which allows accelerators to extend the CPUClass >> with additional accelerator-specific initializations. >> >> Add the field before tcg_ops, and mark tcg_ops as >> needing to be last in the struct until we rework this >> further in a later patch. >> >> Signed-off-by: Claudio Fontana <cfontana@suse.de> >> --- >> include/hw/core/accel-cpu.h | 35 +++++++++++++++++++++++++++++ >> include/hw/core/cpu.h | 1 + >> accel/accel-common.c | 44 +++++++++++++++++++++++++++++++++++++ >> MAINTAINERS | 1 + >> 4 files changed, 81 insertions(+) >> create mode 100644 include/hw/core/accel-cpu.h >> >> diff --git a/include/hw/core/accel-cpu.h b/include/hw/core/accel-cpu.h >> new file mode 100644 >> index 0000000000..246b3e2fcb >> --- /dev/null >> +++ b/include/hw/core/accel-cpu.h >> @@ -0,0 +1,35 @@ >> +/* >> + * Accelerator interface, specializes CPUClass >> + * >> + * Copyright 2020 SUSE LLC > > 2020-2021 ;) Thanks Philippe, will check them all! It's a bright new year! > >> + * >> + * This work is licensed under the terms of the GNU GPL, version 2 or later. >> + * See the COPYING file in the top-level directory. >> + */ >> + >> +#ifndef ACCEL_CPU_H >> +#define ACCEL_CPU_H >> + >> +/* >> + * these defines cannot be in cpu.h, because we are using >> + * CPU_RESOLVING_TYPE here. >> + * Use this header to define your accelerator-specific >> + * cpu-specific accelerator interfaces. >> + */ >> + >> +#define TYPE_ACCEL_CPU "accel-" CPU_RESOLVING_TYPE >> +#define ACCEL_CPU_NAME(name) (name "-" TYPE_ACCEL_CPU) >> +typedef struct AccelCPUClass AccelCPUClass; >> +DECLARE_CLASS_CHECKERS(AccelCPUClass, ACCEL_CPU, TYPE_ACCEL_CPU) >> + >> +typedef struct AccelCPUClass { >> + /*< private >*/ >> + ObjectClass parent_class; >> + /*< public >*/ >> + >> + void (*cpu_class_init)(CPUClass *cc); >> + void (*cpu_instance_init)(CPUState *cpu); >> + void (*cpu_realizefn)(CPUState *cpu, Error **errp); > > If we want callers to check errp, better have the prototype return > a boolean. Good point, the whole errp thing is worth revisiting in the series, there are many cases (which are basically reproduced in the refactoring from existing code), where errp is passed but is really unused. I am constantly internally debating whether to remove the parameter altogether, or to keep it in there. What would you suggest? > >> +} AccelCPUClass; > > Thanks for looking at this, Claudio
Claudio Fontana <cfontana@suse.de> writes: > On 1/28/21 2:03 PM, Philippe Mathieu-Daudé wrote: >> On 1/28/21 10:28 AM, Claudio Fontana wrote: <snip> >>> + >>> +#define TYPE_ACCEL_CPU "accel-" CPU_RESOLVING_TYPE >>> +#define ACCEL_CPU_NAME(name) (name "-" TYPE_ACCEL_CPU) >>> +typedef struct AccelCPUClass AccelCPUClass; >>> +DECLARE_CLASS_CHECKERS(AccelCPUClass, ACCEL_CPU, TYPE_ACCEL_CPU) >>> + >>> +typedef struct AccelCPUClass { >>> + /*< private >*/ >>> + ObjectClass parent_class; >>> + /*< public >*/ >>> + >>> + void (*cpu_class_init)(CPUClass *cc); >>> + void (*cpu_instance_init)(CPUState *cpu); >>> + void (*cpu_realizefn)(CPUState *cpu, Error **errp); >> >> If we want callers to check errp, better have the prototype return >> a boolean. > > Good point, the whole errp thing is worth revisiting in the series, > there are many cases (which are basically reproduced in the refactoring from existing code), > where errp is passed but is really unused. > > I am constantly internally debating whether to remove the parameter altogether, or to keep it in there. > > What would you suggest? I think it really depends on if we can expect the realizefn to usefully return an error message that can be read and understood by the user. I guess this comes down to how much user config is going to be checked at the point we realize the CPU? The bool returning realizefn with Error is a fairly common pattern. > >> >>> +} AccelCPUClass; >> >> > > Thanks for looking at this, > > Claudio
On 1/28/21 5:08 PM, Alex Bennée wrote: > > Claudio Fontana <cfontana@suse.de> writes: > >> On 1/28/21 2:03 PM, Philippe Mathieu-Daudé wrote: >>> On 1/28/21 10:28 AM, Claudio Fontana wrote: > <snip> >>>> + >>>> +#define TYPE_ACCEL_CPU "accel-" CPU_RESOLVING_TYPE >>>> +#define ACCEL_CPU_NAME(name) (name "-" TYPE_ACCEL_CPU) >>>> +typedef struct AccelCPUClass AccelCPUClass; >>>> +DECLARE_CLASS_CHECKERS(AccelCPUClass, ACCEL_CPU, TYPE_ACCEL_CPU) >>>> + >>>> +typedef struct AccelCPUClass { >>>> + /*< private >*/ >>>> + ObjectClass parent_class; >>>> + /*< public >*/ >>>> + >>>> + void (*cpu_class_init)(CPUClass *cc); >>>> + void (*cpu_instance_init)(CPUState *cpu); >>>> + void (*cpu_realizefn)(CPUState *cpu, Error **errp); >>> >>> If we want callers to check errp, better have the prototype return >>> a boolean. >> >> Good point, the whole errp thing is worth revisiting in the series, >> there are many cases (which are basically reproduced in the refactoring from existing code), >> where errp is passed but is really unused. >> >> I am constantly internally debating whether to remove the parameter altogether, or to keep it in there. >> >> What would you suggest? > > I think it really depends on if we can expect the realizefn to usefully > return an error message that can be read and understood by the user. I > guess this comes down to how much user config is going to be checked at > the point we realize the CPU? cpu_realize() is were various feature checks are, isn't it? -cpu mycpu,feat1=on,feat2=off CPU 'mycpu' can not disable feature 'feat2' because of REASON. > The bool returning realizefn with Error is a fairly common pattern. >
On 1/28/21 6:29 AM, Philippe Mathieu-Daudé wrote: > On 1/28/21 5:08 PM, Alex Bennée wrote: >> >> Claudio Fontana <cfontana@suse.de> writes: >> >>> On 1/28/21 2:03 PM, Philippe Mathieu-Daudé wrote: >>>> On 1/28/21 10:28 AM, Claudio Fontana wrote: >> <snip> >>>>> + >>>>> +#define TYPE_ACCEL_CPU "accel-" CPU_RESOLVING_TYPE >>>>> +#define ACCEL_CPU_NAME(name) (name "-" TYPE_ACCEL_CPU) >>>>> +typedef struct AccelCPUClass AccelCPUClass; >>>>> +DECLARE_CLASS_CHECKERS(AccelCPUClass, ACCEL_CPU, TYPE_ACCEL_CPU) >>>>> + >>>>> +typedef struct AccelCPUClass { >>>>> + /*< private >*/ >>>>> + ObjectClass parent_class; >>>>> + /*< public >*/ >>>>> + >>>>> + void (*cpu_class_init)(CPUClass *cc); >>>>> + void (*cpu_instance_init)(CPUState *cpu); >>>>> + void (*cpu_realizefn)(CPUState *cpu, Error **errp); >>>> >>>> If we want callers to check errp, better have the prototype return >>>> a boolean. >>> >>> Good point, the whole errp thing is worth revisiting in the series, >>> there are many cases (which are basically reproduced in the refactoring from existing code), >>> where errp is passed but is really unused. >>> >>> I am constantly internally debating whether to remove the parameter altogether, or to keep it in there. >>> >>> What would you suggest? >> >> I think it really depends on if we can expect the realizefn to usefully >> return an error message that can be read and understood by the user. I >> guess this comes down to how much user config is going to be checked at >> the point we realize the CPU? > > cpu_realize() is were various feature checks are, isn't it? > > -cpu mycpu,feat1=on,feat2=off > CPU 'mycpu' can not disable feature 'feat2' because of REASON. Yes. And while changing the return type of realize is probably a good idea, it should be a separate patch. r~
On 1/29/21 1:13 AM, Richard Henderson wrote: > On 1/28/21 6:29 AM, Philippe Mathieu-Daudé wrote: >> On 1/28/21 5:08 PM, Alex Bennée wrote: >>> >>> Claudio Fontana <cfontana@suse.de> writes: >>> >>>> On 1/28/21 2:03 PM, Philippe Mathieu-Daudé wrote: >>>>> On 1/28/21 10:28 AM, Claudio Fontana wrote: >>> <snip> >>>>>> + >>>>>> +#define TYPE_ACCEL_CPU "accel-" CPU_RESOLVING_TYPE >>>>>> +#define ACCEL_CPU_NAME(name) (name "-" TYPE_ACCEL_CPU) >>>>>> +typedef struct AccelCPUClass AccelCPUClass; >>>>>> +DECLARE_CLASS_CHECKERS(AccelCPUClass, ACCEL_CPU, TYPE_ACCEL_CPU) >>>>>> + >>>>>> +typedef struct AccelCPUClass { >>>>>> + /*< private >*/ >>>>>> + ObjectClass parent_class; >>>>>> + /*< public >*/ >>>>>> + >>>>>> + void (*cpu_class_init)(CPUClass *cc); >>>>>> + void (*cpu_instance_init)(CPUState *cpu); >>>>>> + void (*cpu_realizefn)(CPUState *cpu, Error **errp); >>>>> >>>>> If we want callers to check errp, better have the prototype return >>>>> a boolean. >>>> >>>> Good point, the whole errp thing is worth revisiting in the series, >>>> there are many cases (which are basically reproduced in the refactoring from existing code), >>>> where errp is passed but is really unused. >>>> >>>> I am constantly internally debating whether to remove the parameter altogether, or to keep it in there. >>>> >>>> What would you suggest? >>> >>> I think it really depends on if we can expect the realizefn to usefully >>> return an error message that can be read and understood by the user. I >>> guess this comes down to how much user config is going to be checked at >>> the point we realize the CPU? >> >> cpu_realize() is were various feature checks are, isn't it? >> >> -cpu mycpu,feat1=on,feat2=off >> CPU 'mycpu' can not disable feature 'feat2' because of REASON. > > Yes. And while changing the return type of realize is probably a good idea, it > should be a separate patch. > > > r~ > To summarize: 1) accel->cpu_realizefn extends the current cpu target-specific realize functions with accelerator-specific code, which currently does not make use of errp at all (thus, the temptation to remove errp from the interface until it is actually needed by a target). 2) Regarding the target-specific cpu realize functions themselves, registered with the pattern: device_class_set_parent_realize(dc, x86_cpu_realizefn, &xcc->parent_realize); device_class_set_parent_realize(dc, arm_cpu_realizefn, &acc->parent_realize); ... etc ... these currently all return void, and the code that realizes devices (f.e. in hw/core/qdev.c) calls these callbacks like this: static void device_set_realized(...) { ... if (dc->realize) { dc->realize(dev, &local_err); if (local_err != NULL) { goto fail; } } Ie it does not expect a bool return type for realize(). So making realize return bool means changing all the realize functions for all devices in my view, which I don't think should be crammed in here.. Wdty? Thanks, Claudio
On 1/30/21 12:53 AM, Claudio Fontana wrote: > To summarize: > > 1) accel->cpu_realizefn extends the current cpu target-specific realize functions with accelerator-specific code, > which currently does not make use of errp at all (thus, the temptation to remove errp from the interface until it is actually needed by a target). No, arm does use errp in realizefn already. It's just that the void return value is hinky. r~
Richard Henderson <richard.henderson@linaro.org> writes: > On 1/30/21 12:53 AM, Claudio Fontana wrote: >> To summarize: >> >> 1) accel->cpu_realizefn extends the current cpu target-specific realize functions with accelerator-specific code, >> which currently does not make use of errp at all (thus, the temptation to remove errp from the interface until it is actually needed by a target). > > No, arm does use errp in realizefn already. > It's just that the void return value is hinky. Sounds like fixing the void return would be part of a larger qdev clean-up rather than this particular series. If I recall there have been various phases of clean-ups and refactoring of the error code paths in the past. > > > r~
On 2/1/21 10:15 AM, Alex Bennée wrote: > > Richard Henderson <richard.henderson@linaro.org> writes: > >> On 1/30/21 12:53 AM, Claudio Fontana wrote: >>> To summarize: >>> >>> 1) accel->cpu_realizefn extends the current cpu target-specific realize functions with accelerator-specific code, >>> which currently does not make use of errp at all (thus, the temptation to remove errp from the interface until it is actually needed by a target). >> >> No, arm does use errp in realizefn already. >> It's just that the void return value is hinky. > > Sounds like fixing the void return would be part of a larger qdev > clean-up rather than this particular series. If I recall there have been > various phases of clean-ups and refactoring of the error code paths in > the past. > >> >> >> r~ > > I think you are right. I added a patch in this series to at least not make the problem worse, by adding a return value to accel_cpu::cpu_realizefn, and cleaning up the only code path case that signals an error there for now in i386. (will add as the last patch in the series) Thanks, Claudio
diff --git a/include/hw/core/accel-cpu.h b/include/hw/core/accel-cpu.h new file mode 100644 index 0000000000..246b3e2fcb --- /dev/null +++ b/include/hw/core/accel-cpu.h @@ -0,0 +1,35 @@ +/* + * Accelerator interface, specializes CPUClass + * + * Copyright 2020 SUSE LLC + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ + +#ifndef ACCEL_CPU_H +#define ACCEL_CPU_H + +/* + * these defines cannot be in cpu.h, because we are using + * CPU_RESOLVING_TYPE here. + * Use this header to define your accelerator-specific + * cpu-specific accelerator interfaces. + */ + +#define TYPE_ACCEL_CPU "accel-" CPU_RESOLVING_TYPE +#define ACCEL_CPU_NAME(name) (name "-" TYPE_ACCEL_CPU) +typedef struct AccelCPUClass AccelCPUClass; +DECLARE_CLASS_CHECKERS(AccelCPUClass, ACCEL_CPU, TYPE_ACCEL_CPU) + +typedef struct AccelCPUClass { + /*< private >*/ + ObjectClass parent_class; + /*< public >*/ + + void (*cpu_class_init)(CPUClass *cc); + void (*cpu_instance_init)(CPUState *cpu); + void (*cpu_realizefn)(CPUState *cpu, Error **errp); +} AccelCPUClass; + +#endif /* ACCEL_CPU_H */ diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h index 500e2c4fce..81153c349f 100644 --- a/include/hw/core/cpu.h +++ b/include/hw/core/cpu.h @@ -190,6 +190,7 @@ struct CPUClass { /* Keep non-pointer data at the end to minimize holes. */ int gdb_num_core_regs; bool gdb_stop_before_watchpoint; + struct AccelCPUClass *accel_cpu; /* * NB: this should be covered by CONFIG_TCG, but it is unsafe to do it here, diff --git a/accel/accel-common.c b/accel/accel-common.c index 6b59873419..9901b0531c 100644 --- a/accel/accel-common.c +++ b/accel/accel-common.c @@ -26,6 +26,9 @@ #include "qemu/osdep.h" #include "qemu/accel.h" +#include "cpu.h" +#include "hw/core/accel-cpu.h" + #ifndef CONFIG_USER_ONLY #include "accel-softmmu.h" #endif /* !CONFIG_USER_ONLY */ @@ -46,16 +49,57 @@ AccelClass *accel_find(const char *opt_name) return ac; } +static void accel_init_cpu_int_aux(ObjectClass *klass, void *opaque) +{ + CPUClass *cc = CPU_CLASS(klass); + AccelCPUClass *accel_cpu = opaque; + + cc->accel_cpu = accel_cpu; + if (accel_cpu->cpu_class_init) { + accel_cpu->cpu_class_init(cc); + } +} + +/* initialize the arch-specific accel CpuClass interfaces */ +static void accel_init_cpu_interfaces(AccelClass *ac) +{ + 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-%s", ac_name, CPU_RESOLVING_TYPE); + acc = object_class_by_name(acc_name); + g_free(acc_name); + + if (acc) { + object_class_foreach(accel_init_cpu_int_aux, + CPU_RESOLVING_TYPE, false, acc); + } +} + void accel_init_interfaces(AccelClass *ac) { #ifndef CONFIG_USER_ONLY accel_init_ops_interfaces(ac); #endif /* !CONFIG_USER_ONLY */ + + accel_init_cpu_interfaces(ac); } +static const TypeInfo accel_cpu_type = { + .name = TYPE_ACCEL_CPU, + .parent = TYPE_OBJECT, + .abstract = true, + .class_size = sizeof(AccelCPUClass), +}; + static void register_accel_types(void) { type_register_static(&accel_type); + type_register_static(&accel_cpu_type); } type_init(register_accel_types); diff --git a/MAINTAINERS b/MAINTAINERS index 4cc8d9e502..1a83b60312 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -440,6 +440,7 @@ R: Paolo Bonzini <pbonzini@redhat.com> S: Maintained F: include/qemu/accel.h F: include/sysemu/accel-ops.h +F: include/hw/core/accel-cpu.h F: accel/accel-*.c F: accel/Makefile.objs F: accel/stubs/Makefile.objs
add a new optional interface to CPUClass, which allows accelerators to extend the CPUClass with additional accelerator-specific initializations. Add the field before tcg_ops, and mark tcg_ops as needing to be last in the struct until we rework this further in a later patch. Signed-off-by: Claudio Fontana <cfontana@suse.de> --- include/hw/core/accel-cpu.h | 35 +++++++++++++++++++++++++++++ include/hw/core/cpu.h | 1 + accel/accel-common.c | 44 +++++++++++++++++++++++++++++++++++++ MAINTAINERS | 1 + 4 files changed, 81 insertions(+) create mode 100644 include/hw/core/accel-cpu.h